Re: [RFC 01/11] net: phy: Don't report success if devices weren't found

From: Jeremy Linton
Date: Mon May 25 2020 - 17:02:17 EST


On 5/25/20 4:45 AM, Russell King - ARM Linux admin wrote:
On Sun, May 24, 2020 at 09:46:55PM -0500, Jeremy Linton wrote:

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

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

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.

Hmm, ok, so it gets a bit more complex then. The loop could probe all the valid MMD addresses, then if that doesn't appear to have yielded anything try the reserved ones? Its actually not a big code change because we could have a hardcoded bitfield of valid MMD addresses which we check before trying the probe. Then make one pass through the loop hitting the valid ones, and if we still didn't find anything, try the reserved addresses.

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.
Just to be clear this set probes MMD 0 except to ask for the device list. That is the same behavior as before. That is because all the subsequent id/etc loops are indexed to start at MMD 1. The primary difference for the cortiana PHY's AFAIK, is the order we ask for the devices list. Before it had to fail at a valid addr before reading 0, now it just starts at 0 and continues to probe if that fails. Some of this is required (continuing scan on failure) due to phys that "fail" reading the devices list for the lower MMD's addresses but work on the higher ones.

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.

It does ignore bit 0, it gets turned into the C22 regs flag, and cleared/ignored in the remainder of the code (do to MMD loop indexes starting at 1).

Hence, I think this is a "simplifcation" too far.

Ok, if i'm understanding correctly, avoid the reserved addresses unless we fail to get a device list as before. That isn't a problem, I will include that in the next revision.