| Message ID | 20220323183419.2278676-1-michael@walle.cc (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: C45-over-C22 access | expand |
On Wed, Mar 23, 2022 at 07:34:14PM +0100, Michael Walle wrote: > Hi, > > This is the result of this discussion: > https://lore.kernel.org/netdev/240354b0a54b37e8b5764773711b8aa3@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 on MDIO bus drivers > which will correctly check for the MII_ADDR_C45 flag and return -EOPNOTSUPP > the function call will fail and thus gpy_probe() will fail. This series > tries to fix that and will lay the foundation to add a workaround for the > LAN8814 bug by forcing an MDIO bus to be c22-only. > > At the moment, the probe_capabilities is taken into account to decide if > we have to use C45-over-C22. What is still missing from this series is the > handling of a device tree property to restrict the probe_capabilities to > c22-only. We have a problem here with phydev->is_c45. In phy-core.c, functions __phy_read_mmd() and __phy_write_mmd() it means perform c45 transactions over the bus. We know we want to access a register in c45 space because we are using an _mmd() function. In phy.c, it means does this PHY have c45 registers and we should access that register space, or should we use the c22 register space. So far example phy_restart_aneg() decides to either call genphy_c45_restart_aneg() or genphy_restart_aneg() depending on is_c45. So a PHY with C45 register space but only accessible by C45 over C22 is probably going to do the wrong thing with the current code. For this patchset to work, we need to cleanly separate the concepts of what sort of transactions to do over the bus, from what register spaces the PHY has. We probably want something like phydev->has_c45 to indicate the register space is implemented, and phydev->c45_over_c22 to indicate what sort of transaction should be used in the _mmd() functions. Your patches start in that direction, but i don't think it goes far enough. Andrew
Am 2022-03-23 21:30, schrieb Andrew Lunn: > On Wed, Mar 23, 2022 at 07:34:14PM +0100, Michael Walle wrote: >> Hi, >> >> This is the result of this discussion: >> https://lore.kernel.org/netdev/240354b0a54b37e8b5764773711b8aa3@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 on MDIO bus >> drivers >> which will correctly check for the MII_ADDR_C45 flag and return >> -EOPNOTSUPP >> the function call will fail and thus gpy_probe() will fail. This >> series >> tries to fix that and will lay the foundation to add a workaround for >> the >> LAN8814 bug by forcing an MDIO bus to be c22-only. >> >> At the moment, the probe_capabilities is taken into account to decide >> if >> we have to use C45-over-C22. What is still missing from this series is >> the >> handling of a device tree property to restrict the probe_capabilities >> to >> c22-only. > > We have a problem here with phydev->is_c45. > > In phy-core.c, functions __phy_read_mmd() and __phy_write_mmd() it > means perform c45 transactions over the bus. We know we want to access > a register in c45 space because we are using an _mmd() function. > > In phy.c, it means does this PHY have c45 registers and we should > access that register space, or should we use the c22 register > space. So far example phy_restart_aneg() decides to either call > genphy_c45_restart_aneg() or genphy_restart_aneg() depending on > is_c45. Yes, that is probably the reason why the gpy215 has explicitly set .aneg_done to genphy_c45_aneg_done() for example. > So a PHY with C45 register space but only accessible by C45 over C22 > is probably going to do the wrong thing with the current code. Oh my, yes. Looks like the whole phy_get_c45_ids() isn't working at all for the gpy at the moment (or maybe it will work because it supports AN via the c22 registers, too). I'll have to dig deeper into that tomorrow. I know that _something_ worked at least ;) > For this patchset to work, we need to cleanly separate the concepts of > what sort of transactions to do over the bus, from what register > spaces the PHY has. We probably want something like phydev->has_c45 to > indicate the register space is implemented, and phydev->c45_over_c22 > to indicate what sort of transaction should be used in the _mmd() > functions. > > Your patches start in that direction, but i don't think it goes far > enough. Thanks for the review! -michael