On Sun, May 24, 2020 at 09:46:55PM -0500, Jeremy Linton wrote: > Hi, > > Thanks for taking a look at this. > > On 5/23/20 1:20 PM, Russell King - ARM Linux admin wrote: > > On Fri, May 22, 2020 at 04:30:49PM -0500, Jeremy Linton wrote: > > > C45 devices are to return 0 for registers they haven't > > > implemented. This means in theory we can terminate the > > > device search loop without finding any MMDs. In that > > > case we want to immediately return indicating that > > > nothing was found rather than continuing to probe > > > and falling into the success state at the bottom. > > > > This is a little confusing when you read this comment: > > > > /* If mostly Fs, there is no device there, > > * then let's continue to probe more, as some > > * 10G PHYs have zero Devices In package, > > * e.g. Cortina CS4315/CS4340 PHY. > > */ > > > > Since it appears to be talking about the case of a PHY where *devs will > > be zero. However, tracking down the original submission, it seems this > > is not the case at all, and the comment is grossly misleading. > > > > It seems these PHYs only report a valid data in the Devices In Package > > registers for devad=0 - it has nothing to do with a zero Devices In > > Package. > > Yes, this ended up being my understanding of this commit, and is part of my > justification for starting the devices search at the reserved address 0 > rather than 1. > > > > > Can I suggest that this comment is fixed while we're changing the code > > to explicitly reject this "zero Devices In package" so that it's not > > confusing? > > Its probably better to kill it in 5/11 with a mention that we are starting > at a reserved address? > > OTOH, I'm a bit concerned that reading at 0 as the first address will cause > problems because the original code was only triggering it after a read > returning 0xFFFFFFFF at a valid MMD address. It does simplify/clarify the > loop though. If it weren't for this 0 read, I would have tried to avoid some > of the additional MMD reserved addresses.
Yes, that is the worry, and as MMD 0 is marked as reserved, I don't think we should routinely probe it. As I've already mentioned, note that bit 0 of devices-in-package does not mean that there is a MMD 0 implemented, even if bit 0 is set. Bit 0 means that the clause 22 register set is available through clause 22 cycles. So, simplifying the loop to start at 0 and removing the work- around could end up breaking Cortina PHYs if they don't set bit 0 in their devices in package - but I don't see why we should depend on bit 0 being set for their workaround. So, I think you're going to have to add a work-around to ignore bit 0, which brings up the question whether this is worth it or not. Hence, I think this is a "simplifcation" too far. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up
