Message ID | 20200522213059.1535892-9-jeremy.linton@arm.com |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | Make C45 autoprobe more robust | expand |
> +++ b/include/linux/phy.h > @@ -275,6 +275,11 @@ struct mii_bus { > int reset_delay_us; > /* RESET GPIO descriptor pointer */ > struct gpio_desc *reset_gpiod; > + /* bus capabilities, used for probing */ > + enum { > + MDIOBUS_C22_ONLY = 0, > + MDIOBUS_C45_FIRST, > + } probe_capabilities; > }; I'm not so keen on _FIRST. It suggest _LAST would also be valid. But that then suggests this is not a bus property, but a PHY property, and some PHYs might need _FIRST and other phys need _LAST, and then you have a bus which has both sorts of PHY on it, and you have a problem. So i think it would be better to have enum { MDIOBUS_UNKNOWN = 0, MDIOBUS_C22, MDIOBUS_C45, MDIOBUS_C45_C22, } bus_capabilities; Describe just what the bus master can support. Andrew
Hi, On 5/24/20 9:44 AM, Andrew Lunn wrote: >> +++ b/include/linux/phy.h >> @@ -275,6 +275,11 @@ struct mii_bus { >> int reset_delay_us; >> /* RESET GPIO descriptor pointer */ >> struct gpio_desc *reset_gpiod; >> + /* bus capabilities, used for probing */ >> + enum { >> + MDIOBUS_C22_ONLY = 0, >> + MDIOBUS_C45_FIRST, >> + } probe_capabilities; >> }; > > > I'm not so keen on _FIRST. It suggest _LAST would also be valid. But > that then suggests this is not a bus property, but a PHY property, and > some PHYs might need _FIRST and other phys need _LAST, and then you > have a bus which has both sorts of PHY on it, and you have a problem. > > So i think it would be better to have > > enum { > MDIOBUS_UNKNOWN = 0, > MDIOBUS_C22, > MDIOBUS_C45, > MDIOBUS_C45_C22, > } bus_capabilities; > > Describe just what the bus master can support. Yes, the naming is reasonable and I will update it in the next patch. I went around a bit myself with this naming early on, and the problem I saw was that a C45 capable master, can have C45 electrical phy's that only respond to c22 requests (AFAIK). So the MDIOBUS_C45 (I think I was calling it C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have a C45_ONLY case, but that the assumption is that you wouldn't try and probe c22 registers, which I thought was a mistake. Thanks,
On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote: > Hi, > > On 5/24/20 9:44 AM, Andrew Lunn wrote: > > > +++ b/include/linux/phy.h > > > @@ -275,6 +275,11 @@ struct mii_bus { > > > int reset_delay_us; > > > /* RESET GPIO descriptor pointer */ > > > struct gpio_desc *reset_gpiod; > > > + /* bus capabilities, used for probing */ > > > + enum { > > > + MDIOBUS_C22_ONLY = 0, > > > + MDIOBUS_C45_FIRST, > > > + } probe_capabilities; > > > }; > > > > > > I'm not so keen on _FIRST. It suggest _LAST would also be valid. But > > that then suggests this is not a bus property, but a PHY property, and > > some PHYs might need _FIRST and other phys need _LAST, and then you > > have a bus which has both sorts of PHY on it, and you have a problem. > > > > So i think it would be better to have > > > > enum { > > MDIOBUS_UNKNOWN = 0, > > MDIOBUS_C22, > > MDIOBUS_C45, > > MDIOBUS_C45_C22, > > } bus_capabilities; > > > > Describe just what the bus master can support. > > Yes, the naming is reasonable and I will update it in the next patch. I went > around a bit myself with this naming early on, and the problem I saw was > that a C45 capable master, can have C45 electrical phy's that only respond > to c22 requests (AFAIK). If you have a master that can only generate clause 45 cycles, and someone is daft enough to connect a clause 22 only PHY to it, the result is hardware that doesn't work - there's no getting around that. The MDIO interface can't generate the appropriate cycles to access the clause 22 PHY. So, this is not something we need care about. > So the MDIOBUS_C45 (I think I was calling it > C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have > a C45_ONLY case, but that the assumption is that you wouldn't try and probe > c22 registers, which I thought was a mistake. MDIOBUS_C45 means "I can generate clause 45 cycles". MDIOBUS_C22 means "I can generate clause 22 cycles". MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22 cycles." Notice carefully the values these end up with - MDIOBUS_C22 = BIT(0), MDIOBUS_C45 = BIT(1), MDIOBUS_C45_C22 = BIT(0) | BIT(1). I suspect that was no coincidence in Andrew's suggestion.
> > > So i think it would be better to have > > > > > > enum { > > > MDIOBUS_UNKNOWN = 0, > > > MDIOBUS_C22, > > > MDIOBUS_C45, > > > MDIOBUS_C45_C22, > > > } bus_capabilities; > > > > > > Describe just what the bus master can support. > > > > Yes, the naming is reasonable and I will update it in the next patch. I went > > around a bit myself with this naming early on, and the problem I saw was > > that a C45 capable master, can have C45 electrical phy's that only respond > > to c22 requests (AFAIK). > > If you have a master that can only generate clause 45 cycles, and > someone is daft enough to connect a clause 22 only PHY to it, the > result is hardware that doesn't work - there's no getting around > that. The MDIO interface can't generate the appropriate cycles to > access the clause 22 PHY. So, this is not something we need care > about. > > > So the MDIOBUS_C45 (I think I was calling it > > C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have > > a C45_ONLY case, but that the assumption is that you wouldn't try and probe > > c22 registers, which I thought was a mistake. > > MDIOBUS_C45 means "I can generate clause 45 cycles". > MDIOBUS_C22 means "I can generate clause 22 cycles". > MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22 > cycles." > > Notice carefully the values these end up with - MDIOBUS_C22 = BIT(0), > MDIOBUS_C45 = BIT(1), MDIOBUS_C45_C22 = BIT(0) | BIT(1). I suspect > that was no coincidence in Andrew's suggestion. Hi Russell What was a nice side affect. Since i doubt Jeremy is going to go through every MDIO driver and set the capabilities correctly, i wanted 0 to have a safe meaning. In the code we should treat MDIOBUS_UNKNOWN and MDIOBUS_C22 identically. But maybe some time in the distant future, we can make 0 issue a warning. Andrew
Hi, On 5/25/20 3:25 AM, Russell King - ARM Linux admin wrote: > On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote: >> Hi, >> >> On 5/24/20 9:44 AM, Andrew Lunn wrote: >>>> +++ b/include/linux/phy.h >>>> @@ -275,6 +275,11 @@ struct mii_bus { >>>> int reset_delay_us; >>>> /* RESET GPIO descriptor pointer */ >>>> struct gpio_desc *reset_gpiod; >>>> + /* bus capabilities, used for probing */ >>>> + enum { >>>> + MDIOBUS_C22_ONLY = 0, >>>> + MDIOBUS_C45_FIRST, >>>> + } probe_capabilities; >>>> }; >>> >>> >>> I'm not so keen on _FIRST. It suggest _LAST would also be valid. But >>> that then suggests this is not a bus property, but a PHY property, and >>> some PHYs might need _FIRST and other phys need _LAST, and then you >>> have a bus which has both sorts of PHY on it, and you have a problem. >>> >>> So i think it would be better to have >>> >>> enum { >>> MDIOBUS_UNKNOWN = 0, >>> MDIOBUS_C22, >>> MDIOBUS_C45, >>> MDIOBUS_C45_C22, >>> } bus_capabilities; >>> >>> Describe just what the bus master can support. >> >> Yes, the naming is reasonable and I will update it in the next patch. I went >> around a bit myself with this naming early on, and the problem I saw was >> that a C45 capable master, can have C45 electrical phy's that only respond >> to c22 requests (AFAIK). > > If you have a master that can only generate clause 45 cycles, and > someone is daft enough to connect a clause 22 only PHY to it, the > result is hardware that doesn't work - there's no getting around > that. The MDIO interface can't generate the appropriate cycles to > access the clause 22 PHY. So, this is not something we need care > about. > >> So the MDIOBUS_C45 (I think I was calling it >> C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have >> a C45_ONLY case, but that the assumption is that you wouldn't try and probe >> c22 registers, which I thought was a mistake. > > MDIOBUS_C45 means "I can generate clause 45 cycles". > MDIOBUS_C22 means "I can generate clause 22 cycles". > MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22 > cycles." Hi, to be clear, we are talking about c45 controllers that can access the c22 register space via c45, or controllers which are electrically/level shifting to be compatible with c22 voltages/etc? The nxp hardware in question has 1, 10 and 40Gbit phys on the same MDIO, the 1gbit we fall back to c22 registers because it doesn't respond correctly to c45 registers. Which is AFAIK what the bit0 C22 regs bit is for.. The general logic right now for a C45_FIRST is attempt to detect a c45 phy and if nothing is detected fall back and attempt c22 register access. That is whats picking up the 1G phys. If for whatever reason the MDIO controller can't do the right thing to access the c22 regs, I guess there really isn't anything we can do about it. > > Notice carefully the values these end up with - MDIOBUS_C22 = BIT(0), > MDIOBUS_C45 = BIT(1), MDIOBUS_C45_C22 = BIT(0) | BIT(1). I suspect > that was no coincidence in Andrew's suggestion. >
On Mon, May 25, 2020 at 05:09:56PM -0500, Jeremy Linton wrote: > Hi, > > On 5/25/20 3:25 AM, Russell King - ARM Linux admin wrote: > > On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote: > > > Hi, > > > > > > On 5/24/20 9:44 AM, Andrew Lunn wrote: > > > > > +++ b/include/linux/phy.h > > > > > @@ -275,6 +275,11 @@ struct mii_bus { > > > > > int reset_delay_us; > > > > > /* RESET GPIO descriptor pointer */ > > > > > struct gpio_desc *reset_gpiod; > > > > > + /* bus capabilities, used for probing */ > > > > > + enum { > > > > > + MDIOBUS_C22_ONLY = 0, > > > > > + MDIOBUS_C45_FIRST, > > > > > + } probe_capabilities; > > > > > }; > > > > > > > > > > > > I'm not so keen on _FIRST. It suggest _LAST would also be valid. But > > > > that then suggests this is not a bus property, but a PHY property, and > > > > some PHYs might need _FIRST and other phys need _LAST, and then you > > > > have a bus which has both sorts of PHY on it, and you have a problem. > > > > > > > > So i think it would be better to have > > > > > > > > enum { > > > > MDIOBUS_UNKNOWN = 0, > > > > MDIOBUS_C22, > > > > MDIOBUS_C45, > > > > MDIOBUS_C45_C22, > > > > } bus_capabilities; > > > > > > > > Describe just what the bus master can support. > > > > > > Yes, the naming is reasonable and I will update it in the next patch. I went > > > around a bit myself with this naming early on, and the problem I saw was > > > that a C45 capable master, can have C45 electrical phy's that only respond > > > to c22 requests (AFAIK). > > > > If you have a master that can only generate clause 45 cycles, and > > someone is daft enough to connect a clause 22 only PHY to it, the > > result is hardware that doesn't work - there's no getting around > > that. The MDIO interface can't generate the appropriate cycles to > > access the clause 22 PHY. So, this is not something we need care > > about. > > > > > So the MDIOBUS_C45 (I think I was calling it > > > C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have > > > a C45_ONLY case, but that the assumption is that you wouldn't try and probe > > > c22 registers, which I thought was a mistake. > > > > MDIOBUS_C45 means "I can generate clause 45 cycles". > > MDIOBUS_C22 means "I can generate clause 22 cycles". > > MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22 > > cycles." > > Hi, to be clear, we are talking about c45 controllers that can access the > c22 register space via c45, or controllers which are electrically/level > shifting to be compatible with c22 voltages/etc? To me, Andrew was quite clear that these flags should describe what the MDIO controller can support, and what I understand from Andrew's email is: If it can't support clause 45 accesses, then it should not advertise MDIOBUS_C45 nor MDIOBUS_C45_C22. If it can't support clause 22 accesses, then it should not advertise MDIOBUS_C22. And that's all there is to it. If you want to talk about clause 45 access via the clause 22 register set, that is a property of the PHY, not of the MDIO controller, and the MDIO controller has no business attempting to describe that property in any shape or form since it is a PHY property. If you have access to clause 22 registers, then you likely have the clause 22 ID registers, and the way phylib currently works, that will also match a PHY driver. Once we have a PHY and a driver bound, then even if it is a C45 driver, accesses using the phy_*_mmd() functions will _automatically_ switch to using C22 cycles to the indirect C45 access registers if the PHY has not been detected as a C45 PHY (phydev->is_c45 is false.) So, I'm coming to the conclusion that you're making way more work here than is necessary, your changes to gratuitously change the way stuff in phylib work which is not risk-free are completely unnecessary and really not a risk worth taking when it's likely that we already have the code mostly in place to be able to support your PHY. I think there are some questionable uses of phydev->is_c45 that your point about accessing C45 PHYs through C22 indirect registers brings up which need to be resolved, but I really don't think that's a completely separate issue.
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 7a4eb3f2cb74..3ab618e15f2c 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -732,10 +732,15 @@ EXPORT_SYMBOL(mdiobus_free); */ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr) { - struct phy_device *phydev; + struct phy_device *phydev = ERR_PTR(-ENODEV); int err; - phydev = get_phy_device(bus, addr, false); + if (bus->probe_capabilities == MDIOBUS_C45_FIRST) + phydev = get_phy_device(bus, addr, true); + + if (IS_ERR(phydev)) + phydev = get_phy_device(bus, addr, false); + if (IS_ERR(phydev)) return phydev; diff --git a/include/linux/phy.h b/include/linux/phy.h index 480a6b153227..d68e1ebab484 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -275,6 +275,11 @@ struct mii_bus { int reset_delay_us; /* RESET GPIO descriptor pointer */ struct gpio_desc *reset_gpiod; + /* bus capabilities, used for probing */ + enum { + MDIOBUS_C22_ONLY = 0, + MDIOBUS_C45_FIRST, + } probe_capabilities; }; #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
The mdiobus_scan logic is currently hardcoded to only work with c22 devices. This works fairly well in most cases, but its possible a c45 device doesn't respond despite being a standard phy. If the parent hardware is capable, it makes sense to scan for c45 devices before falling back to c22. As we want this to reflect the capabilities of the STA, lets add a field to the mii_bus structure to represent the capability. That way devices can opt into the extended scanning. Existing users should continue to default to c22 only scanning as long as they are zero'ing the structure before use. At the moment its a yes/no option, but in the future it may be useful to extend this to c45 only policy, or add additional classes and policies. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/net/phy/mdio_bus.c | 9 +++++++-- include/linux/phy.h | 5 +++++ 2 files changed, 12 insertions(+), 2 deletions(-)