mbox series

[RFC,net-next,0/5] net: phy: C45-over-C22 access

Message ID 20220323183419.2278676-1-michael@walle.cc (mailing list archive)
Headers show
Series net: phy: C45-over-C22 access | expand

Message

Michael Walle March 23, 2022, 6:34 p.m. UTC
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.

Since net-next is closed, this is marked as RFC to get some early feedback.

Michael Walle (5):
  net: phy: mscc-miim: reject clause 45 register accesses
  net: phy: support indirect c45 access in get_phy_c45_ids()
  net: phy: mscc-miim: add probe_capabilities
  net: phy: introduce is_c45_over_c22 flag
  net: phylink: handle the new is_c45_over_c22 property

 drivers/net/mdio/mdio-mscc-miim.c |  7 ++++
 drivers/net/phy/mxl-gpy.c         |  2 +-
 drivers/net/phy/phy_device.c      | 65 ++++++++++++++++++++++++++-----
 drivers/net/phy/phylink.c         |  2 +-
 include/linux/phy.h               |  4 +-
 5 files changed, 68 insertions(+), 12 deletions(-)

Comments

Andrew Lunn March 23, 2022, 8:30 p.m. UTC | #1
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
Michael Walle March 23, 2022, 11:01 p.m. UTC | #2
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