| Message ID | 20230120224011.796097-1-michael@walle.cc (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: C45-over-C22 access | expand |
On Fri, Jan 20, 2023 at 11:40:06PM +0100, Michael Walle wrote: > After the c22 and c45 access split is finally merged. This can now be > posted again. The old version can be found here: > https://lore.kernel.org/netdev/20220325213518.2668832-1-michael@walle.cc/ > Although all the discussion was here: > https://lore.kernel.org/netdev/20220323183419.2278676-1-michael@walle.cc/ > > The goal here is to get the GYP215 and LAN8814 running on the Microchip > LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the > LAN8814 has a bug which makes it impossible to use C45 on that bus. > Fortunately, it was the intention of the GPY215 driver to be used on a C22 > bus. But I think this could have never really worked, because the > phy_get_c45_ids() will always do c45 accesses and thus gpy_probe() will > fail. > > Introduce C45-over-C22 support and use it if the MDIO bus doesn't support > C45. Also enable it when a PHY is promoted from C22 to C45. I see this breaking up into two problems. 1) Scanning the bus and finding device, be it by C22, C45, or C45 over C22. 2) Allowing drivers to access C45 register spaces, without caring if it is C45 transfers or C45 over C22. For scanning the bus we currently have: if (bus->read) { err = mdiobus_scan_bus_c22(bus); if (err) goto error; } prevent_c45_scan = mdiobus_prevent_c45_scan(bus); if (!prevent_c45_scan && bus->read_c45) { err = mdiobus_scan_bus_c45(bus); if (err) goto error; } I think we should be adding something like: else { if (bus->read) { err = mdiobus_scan_bus_c45_over_c22(bus); if (err) goto error; } } That makes the top level pretty obvious what is going on. But i think we need some more cleanup lower down. We now have a clean separation in MDIO bus drivers between C22 bus transactions and C45 transactions bus. But further up it is less clear. PHY drivers should be using phy_read_mmd()/phy_write_mmd() etc, which means access the C45 address space, but says nothing about what bus transactions to use. So that is also quite clean. The problem is in the middle. get_phy_c45_devs_in_pkg() uses mdiobus_c45_read(). Does mdiobus_c45_read() mean perform a C45 bus transaction, or access the C45 address space? I would say it means perform a C45 bus transaction. It does not take a phydev, so we are below the concept of PHYs, and so C45 over C22 does not exist at this level. So i think we need to review all calls to mdiobus_c45_read/mdiobus_c45_write() etc and see if they mean C45 bus transaction or C45 address space. Those meaning address space should be changed to phy_read_mmd()/phy_write_mmd(). get_phy_device(), get_phy_c45_devs_in_pkg(), get_phy_c45_ids(), phy_c45_probe_present() however do not deal with phydev, so cannot use phy_read_mmd()/phy_write_mmd(). They probably need the bool is_c45 replaced with an enum indicating what sort of bus transaction should be performed. Depending on that value, they can call mdiobus_c45_read() or mmd_phy_indirect() and __mdiobus_read(). I don't have time at the moment, but i would like to dig more into phydev->is_c45. has_c45 makes sense to indicate it has c45 address space. But we need to see if it is every used to indicate to use c45 transactions. But it is clear we need a new member to indicate if C45 or C45 over C22 should be performed, and this should be set by how the PHY was found in the first place. Andrew
On Mon, Jan 23, 2023 at 07:03:18PM +0100, Andrew Lunn wrote: > On Fri, Jan 20, 2023 at 11:40:06PM +0100, Michael Walle wrote: > > After the c22 and c45 access split is finally merged. This can now be > > posted again. The old version can be found here: > > https://lore.kernel.org/netdev/20220325213518.2668832-1-michael@walle.cc/ > > Although all the discussion was here: > > https://lore.kernel.org/netdev/20220323183419.2278676-1-michael@walle.cc/ > > > > The goal here is to get the GYP215 and LAN8814 running on the Microchip > > LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the > > LAN8814 has a bug which makes it impossible to use C45 on that bus. > > Fortunately, it was the intention of the GPY215 driver to be used on a C22 > > bus. But I think this could have never really worked, because the > > phy_get_c45_ids() will always do c45 accesses and thus gpy_probe() will > > fail. > > > > Introduce C45-over-C22 support and use it if the MDIO bus doesn't support > > C45. Also enable it when a PHY is promoted from C22 to C45. > > I see this breaking up into two problems. > > 1) Scanning the bus and finding device, be it by C22, C45, or C45 over C22. > > 2) Allowing drivers to access C45 register spaces, without caring if > it is C45 transfers or C45 over C22. > > For scanning the bus we currently have: > > > if (bus->read) { > err = mdiobus_scan_bus_c22(bus); > if (err) > goto error; > } > > prevent_c45_scan = mdiobus_prevent_c45_scan(bus); > > if (!prevent_c45_scan && bus->read_c45) { > err = mdiobus_scan_bus_c45(bus); > if (err) > goto error; > } > > I think we should be adding something like: > > else { > if (bus->read) { > err = mdiobus_scan_bus_c45_over_c22(bus); > if (err) > goto error; > } > } > > That makes the top level pretty obvious what is going on. > > But i think we need some more cleanup lower down. We now have a clean > separation in MDIO bus drivers between C22 bus transactions and C45 > transactions bus. But further up it is less clear. PHY drivers should > be using phy_read_mmd()/phy_write_mmd() etc, which means access the > C45 address space, but says nothing about what bus transactions to > use. So that is also quite clean. > > The problem is in the middle. get_phy_c45_devs_in_pkg() uses > mdiobus_c45_read(). Does mdiobus_c45_read() mean perform a C45 bus > transaction, or access the C45 address space? I would say it means > perform a C45 bus transaction. It does not take a phydev, so we are > below the concept of PHYs, and so C45 over C22 does not exist at this > level. C45-over-C22 is a PHY thing, it isn't generic. We shouldn't go poking at the PHY C45-over-C22 registers unless we know for certain that the C22 device we are accessing is a PHY, otherwise we could be writing into e.g. a switch register or something else. So, the mdiobus_* API should be the raw bus API. If we want C45 bus cycles then mdiobus_c45_*() is the API that gives us that, vs C22 bus cycles through the non-C45 API. C45-over-C22 being a PHY thing is something that should be handled by phylib, and currently is. The phylib accessors there will use C45 or C45-over-C22 as appropriate. The problem comes with PHYs that maybe don't expose C22 ID registers but do have C45-over-C22. These aren't detectable without probing using the C45-over-C22 PHY protocol, but doing that gratuitously will end up writing values to e.g. switch registers and disrupting their operation. So I regard that as a very dangerous thing to be doing. Given that, it seems that such a case could not be automatically probed, and thus must be described in firmware.
> C45-over-C22 is a PHY thing, it isn't generic. We shouldn't go poking > at the PHY C45-over-C22 registers unless we know for certain that the > C22 device we are accessing is a PHY, otherwise we could be writing > into e.g. a switch register or something else. Humm, yes. Good point. > The problem comes with PHYs that maybe don't expose C22 ID registers > but do have C45-over-C22. > > Given that, it seems that such a case could not be automatically > probed, and thus must be described in firmware. We already have the compatible: - const: ethernet-phy-ieee802.3-c45 description: PHYs that implement IEEE802.3 clause 45 But it is not clear what that actually means. Does it mean it has c45 registers, or does it mean it supports C45 bus transactions? If we have that compatible, we could probe first using C45 and if that fails, or is not supported by the bus master, probe using C45 over C22. That seems safe. For Michael use case, the results of mdiobus_prevent_c45_scan(bus) needs keeping as a property of bus, so we know not to perform the C45 scan, and go direct to C45 over C22. Andrew
> - const: ethernet-phy-ieee802.3-c45 > description: PHYs that implement IEEE802.3 clause 45 > > But it is not clear what that actually means. Does it mean it has c45 > registers, or does it mean it supports C45 bus transactions? PHYs which support C45 access but don't have C45 registers aren't a thing I presume - or doesn't make any sense, right? PHYs which have C45 registers but don't support C45 access exists. e.g. the EEE registers of C22 PHYs. But they are C22 PHYs. So I'd say if you have compatible = "ethernet-phy-ieee802.3-c45", it is a PHY with C45 registers _and_ which are accessible by C45 transactions. But they might or might not support C22 access. But I think thats pretty irrelevant because you always do C45 if you can. You cannot do C45 if: (1) the mdio bus doesn't support C45 (2) you have a broken C22 phy on the mdio bus In both cases, if the PHY doesn't support C22-over-C45 you are screwed. Therefore, if you have either (1) or (2) we should always fall back to C22-over-C45. > If we have that compatible, we could probe first using C45 and if that > fails, or is not supported by the bus master, probe using C45 over > C22. That seems safe. For Michael use case, the results of > mdiobus_prevent_c45_scan(bus) needs keeping as a property of bus, so > we know not to perform the C45 scan, and go direct to C45 over C22. So you are talking about DT, in which case there is no auto probing. See phy_mask in the dt code. Only PHYs in the device tree are probed. (unless of course there is no reg property.. then there is some obscure auto scanning). So if you want a C45 PHY you'd have to have that compatible in any case. Btw. I still don't know how you can get a C45 PHY instance without a device tree, except if there is a C45 only bus or the PHY doesn't respond on C22 ids. Maybe I'm missing something here.. -michael
On Tue, Jan 24, 2023 at 01:35:48AM +0100, Michael Walle wrote: > > - const: ethernet-phy-ieee802.3-c45 > > description: PHYs that implement IEEE802.3 clause 45 > > > > But it is not clear what that actually means. Does it mean it has c45 > > registers, or does it mean it supports C45 bus transactions? > > PHYs which support C45 access but don't have C45 registers aren't > a thing I presume - or doesn't make any sense, right? > > PHYs which have C45 registers but don't support C45 access exists. > e.g. the EEE registers of C22 PHYs. But they are C22 PHYs. I wonder if there are any C22 PHYs which only allow access to EEE via C45 transactions. That would be pretty broken... To some extent, i'm still looking at everything and trying to decide if the current definitions/documentation make it clear if means C45 transfers or registers. And the documentation in DT is ambiguous, but as you point out, it probably means registers, not transactions. > So I'd say if you have compatible = "ethernet-phy-ieee802.3-c45", > it is a PHY with C45 registers _and_ which are accessible by > C45 transactions. But they might or might not support C22 access. > But I think thats pretty irrelevant because you always do C45 if > you can. You cannot do C45 if: > (1) the mdio bus doesn't support C45 > (2) you have a broken C22 phy on the mdio bus > > In both cases, if the PHY doesn't support C22-over-C45 you are > screwed. Therefore, if you have either (1) or (2) we should always > fall back to C22-over-C45. > > > If we have that compatible, we could probe first using C45 and if that > > fails, or is not supported by the bus master, probe using C45 over > > C22. That seems safe. For Michael use case, the results of > > mdiobus_prevent_c45_scan(bus) needs keeping as a property of bus, so > > we know not to perform the C45 scan, and go direct to C45 over C22. > > So you are talking about DT, in which case there is no auto probing. > See phy_mask in the dt code. Only PHYs in the device tree are probed. > (unless of course there is no reg property.. then there is some > obscure auto scanning). So if you want a C45 PHY you'd have to > have that compatible in any case. > > Btw. I still don't know how you can get a C45 PHY instance without > a device tree, except if there is a C45 only bus or the PHY doesn't > respond on C22 ids. Maybe I'm missing something here.. In the DT case, you are probably correct. But there are a number of MDIO busses which don't come from DT. Those are typically PCIe or USB devices. Those do get scanned, and my recent changes should mean they first get scanned using C22 and then C45. DSA switches also typically don't have a MDIO node in DT, it is assumed there is a 1:1 mapping between port number and address on the MDIO bus. But as you said, it would require that they don't respond to C22, or the bus master does not support C22, which does actually exist from Marvell at least. In the none DT case, we probably cannot easily do anything about C22-over-C45, because as Russell pointed out, it is potentially a destructive process doing a scan. We want some indication we do expect a PHY to be there. And "ethernet-phy-ieee802.3-c45" would do that. Andrew
>> The problem is in the middle. get_phy_c45_devs_in_pkg() uses >> mdiobus_c45_read(). Does mdiobus_c45_read() mean perform a C45 bus >> transaction, or access the C45 address space? I would say it means >> perform a C45 bus transaction. It does not take a phydev, so we are >> below the concept of PHYs, and so C45 over C22 does not exist at this >> level. > > C45-over-C22 is a PHY thing, it isn't generic. We shouldn't go poking > at the PHY C45-over-C22 registers unless we know for certain that the > C22 device we are accessing is a PHY, otherwise we could be writing > into e.g. a switch register or something else. > > So, the mdiobus_* API should be the raw bus API. If we want C45 bus > cycles then mdiobus_c45_*() is the API that gives us that, vs C22 bus > cycles through the non-C45 API. > > C45-over-C22 being a PHY thing is something that should be handled by > phylib, and currently is. The phylib accessors there will use C45 or > C45-over-C22 as appropriate. I think the crux is get_phy_device(). It is used for two different cases: (1) to scan the mdio bus (2) to add a c45 phy, i.e. in the DT/fwnode case For (1) we must not use indirect access. And for (2) we know for a fact that it must be a PHY and thus we can (and have to) fall back to c45-over-c22. Btw. for the DT case, it seems we need yet another property to indicate broken MDIO busses. -michael
> Btw. for the DT case, it seems we need yet another property > to indicate broken MDIO busses. I would prefer to avoid that. I would suggest you do what i did for the none DT case. First probe using C22 for all devices known in DT. Then call mdiobus_prevent_c45_scan() which will determine if any of the found devices are FUBAR and will break C45. Then do a second probe using C45 and/or C45 over C22 for those devices in DT with the c45 compatible. Andrew
Am 2023-01-24 22:03, schrieb Andrew Lunn: >> Btw. for the DT case, it seems we need yet another property >> to indicate broken MDIO busses. > > I would prefer to avoid that. I would suggest you do what i did for > the none DT case. First probe using C22 for all devices known in DT. > Then call mdiobus_prevent_c45_scan() which will determine if any of > the found devices are FUBAR and will break C45. Then do a second probe > using C45 and/or C45 over C22 for those devices in DT with the c45 > compatible. I tried that yesterday. Have a look at of_mdiobus_register() [1]. There the device tree is walked and each PHY with a reg property is probed. Afterwards, if there was a node without a reg property, the bus is scanned for the missing PHYs. If we would just probe c22 first, the order of the auto scanning might change, if there is a c45 phy in between two c22 phys. I was thinking to just ignore the case that the autoscan would discover a broken PHY. (1) scan c22 (2) scan c45 (maybe using c45-over-c22) (3) do the autoscan -michael [1] https://elixir.bootlin.com/linux/v6.2-rc5/source/drivers/net/mdio/of_mdio.c#L149 > > Andrew
On Tue, Jan 24, 2023 at 10:20:33PM +0100, Michael Walle wrote: > Am 2023-01-24 22:03, schrieb Andrew Lunn: > > > Btw. for the DT case, it seems we need yet another property > > > to indicate broken MDIO busses. > > > > I would prefer to avoid that. I would suggest you do what i did for > > the none DT case. First probe using C22 for all devices known in DT. > > Then call mdiobus_prevent_c45_scan() which will determine if any of > > the found devices are FUBAR and will break C45. Then do a second probe > > using C45 and/or C45 over C22 for those devices in DT with the c45 > > compatible. > > I tried that yesterday. Have a look at of_mdiobus_register() [1]. > There the device tree is walked and each PHY with a reg property > is probed. Afterwards, if there was a node without a reg property, > the bus is scanned for the missing PHYs. If we would just probe c22 > first, the order of the auto scanning might change, if there is a > c45 phy in between two c22 phys. I was thinking to just ignore the > case that the autoscan would discover a broken PHY. I think it is pretty rare to not have a reg value. The DT lint tools will complain about that, etc. So any examples are likely to be old boards. And old board are a lot less likely to have C45 PHYs. So there is a corner case left unhandled, but it seems pretty unlikely. So i agree, lets address it if anybody reports issues. But please mention it in the commit message, just i can somebody does a git bisect, etc. Andrew