| Message ID | 20200522213059.1535892-2-jeremy.linton@arm.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Make C45 autoprobe more robust | expand |
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. 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? Thanks. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/net/phy/phy_device.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index ac2784192472..245899b58a7d 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -742,6 +742,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > } > } > > + /* no reported devices */ > + if (*devs == 0) { > + *phy_id = 0xffffffff; > + return 0; > + } > + > /* Now probe Device Identifiers for each device present. */ > for (i = 1; i < num_ids; i++) { > if (!(c45_ids->devices_in_package & (1 << i))) > -- > 2.26.2 > >
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.
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.
Hi, 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: >> 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. 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. Thanks,
On Mon, May 25, 2020 at 04:02:13PM -0500, Jeremy Linton wrote: > > 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). However, I've already pointed out that that isn't the case in a number of functions that I listed in another email, and I suspect was glossed over.
Hi, On 5/25/20 4:07 PM, Russell King - ARM Linux admin wrote: > On Mon, May 25, 2020 at 04:02:13PM -0500, Jeremy Linton wrote: >>> 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). > > However, I've already pointed out that that isn't the case in a > number of functions that I listed in another email, and I suspect > was glossed over. > Hmm, right, I might not be understanding, I'm still considering your comments in 4/11 and a couple others.. OTOH, the mmd 0 logic could be completely removed, as its actually been broken for a year or so in linux (AFAIK) because the code triggering it was disabled when the C22 sanitation patch was merged. OTOH, this patch is still clearing the C22 flag from devices, so anything dependent entirely on that should have the same behavior as before. So, there is a bug in the is_valid_phy/device macro, because I messed it up when I converted it to a function because its using a signed val, when it should be unsigned. I don't think that is what you were hinting in 4/11 though. Thanks,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index ac2784192472..245899b58a7d 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -742,6 +742,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, } } + /* no reported devices */ + if (*devs == 0) { + *phy_id = 0xffffffff; + return 0; + } + /* Now probe Device Identifiers for each device present. */ for (i = 1; i < num_ids; i++) { if (!(c45_ids->devices_in_package & (1 << i)))
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. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/net/phy/phy_device.c | 6 ++++++ 1 file changed, 6 insertions(+)