[RFC PATCH] drivers: net: mdio_bus: try indirect clause 45 regs access

[RFC PATCH] drivers: net: mdio_bus: try indirect clause 45 regs access

[Baruch Siach]

When the MDIO bus does not support directly clause 45 access, fallback
to indirect registers access method to read/write clause 45 registers
using clause 22 registers.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---

Is that the right course?

Currently, this does not really work on the my target machine, which is
using the Armada 385 native MDIO bus (mvmdio) connected to clause 45
Marvell 88E2110 PHY. Registers MDIO_DEVS1 and MDIO_DEVS1 read bogus
values which breaks PHY identification. However, the phytool utility
reads the same registers correctly:

phytool eth1/2:1/5
ieee-phy: reg:0x05 val:0x008a

eth1 is connected to another PHY (clause 22) on the same MDIO bus.

The same hardware works nicely with the mdio-gpio bus implementation,
when mdio pins are muxed as GPIOs.
---
 drivers/net/phy/mdio_bus.c | 12 ++++++++++++
 drivers/net/phy/phy-core.c |  2 +-
 include/linux/phy.h        |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 7a4eb3f2cb74..12e39f794b29 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -790,6 +790,12 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
 	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
 
 	retval = bus->read(bus, addr, regnum);
+	if (retval == -EOPNOTSUPP && regnum & MII_ADDR_C45) {
+		int c45_devad = (regnum >> 16) & 0x1f;
+
+		mmd_phy_indirect(bus, addr, c45_devad, regnum & 0xfff);
+		retval = bus->read(bus, addr, MII_MMD_DATA);
+	}
 
 	trace_mdio_access(bus, 1, addr, regnum, retval, retval);
 	mdiobus_stats_acct(&bus->stats[addr], true, retval);
@@ -816,6 +822,12 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
 
 	err = bus->write(bus, addr, regnum, val);
+	if (err == -EOPNOTSUPP && regnum & MII_ADDR_C45) {
+		int c45_devad = (regnum >> 16) & 0x1f;
+
+		mmd_phy_indirect(bus, addr, c45_devad, regnum & 0xfff);
+		err = bus->write(bus, addr, MII_MMD_DATA, val);
+	}
 
 	trace_mdio_access(bus, 0, addr, regnum, val, err);
 	mdiobus_stats_acct(&bus->stats[addr], false, err);
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 66b8c61ca74c..74b8cf8599aa 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -395,7 +395,7 @@ int phy_speed_down_core(struct phy_device *phydev)
 	return __set_linkmode_max_speed(min_common_speed, phydev->advertising);
 }
 
-static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
+void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
 			     u16 regnum)
 {
 	/* Write the desired MMD Devad */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2432ca463ddc..6e7a5fc1906f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -864,6 +864,8 @@ int __phy_modify_mmd(struct phy_device *phydev, int devad, u32 regnum,
 		     u16 mask, u16 set);
 int phy_modify_mmd(struct phy_device *phydev, int devad, u32 regnum,
 		   u16 mask, u16 set);
+void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
+		      u16 regnum);
 
 /**
  * __phy_set_bits - Convenience function for setting bits in a PHY register
-- 
2.26.2

Re: [RFC PATCH] drivers: net: mdio_bus: try indirect clause 45 regs access

[Russell King - ARM Linux admin]

On Sun, May 17, 2020 at 01:20:56PM +0300, Baruch Siach wrote:
> When the MDIO bus does not support directly clause 45 access, fallback
> to indirect registers access method to read/write clause 45 registers
> using clause 22 registers.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> 
> Is that the right course?
> 
> Currently, this does not really work on the my target machine, which is
> using the Armada 385 native MDIO bus (mvmdio) connected to clause 45
> Marvell 88E2110 PHY. Registers MDIO_DEVS1 and MDIO_DEVS1 read bogus
> values which breaks PHY identification. However, the phytool utility
> reads the same registers correctly:
> 
> phytool eth1/2:1/5
> ieee-phy: reg:0x05 val:0x008a
> 
> eth1 is connected to another PHY (clause 22) on the same MDIO bus.
> 
> The same hardware works nicely with the mdio-gpio bus implementation,
> when mdio pins are muxed as GPIOs.

Not all C45 PHYs are required to provide C22.  I'm pretty sure that
accessing a C45 PHY through the indirect method is likely something
that isn't well tested with PHYs, so getting wrong device IDs doesn't
surprise me.  Is there a reason to try switching back to mvmdio on
this device?

Some comments on the patch:

> ---
>  drivers/net/phy/mdio_bus.c | 12 ++++++++++++
>  drivers/net/phy/phy-core.c |  2 +-
>  include/linux/phy.h        |  2 ++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 7a4eb3f2cb74..12e39f794b29 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -790,6 +790,12 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
>  	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
>  
>  	retval = bus->read(bus, addr, regnum);
> +	if (retval == -EOPNOTSUPP && regnum & MII_ADDR_C45) {
> +		int c45_devad = (regnum >> 16) & 0x1f;
> +
> +		mmd_phy_indirect(bus, addr, c45_devad, regnum & 0xfff);
> +		retval = bus->read(bus, addr, MII_MMD_DATA);
> +	}

I don't think this should be done at mdiobus level; I think this is a
layering violation.  It needs to happen at the PHY level because the
indirect C45 access via C22 registers is specific to PHYs.

It also needs to check in the general case that the PHY does indeed
support the C22 register set - not all C45 PHYs do.

So, I think we want this fallback to be conditional on:

- are we probing for the PHY, trying to read its IDs and
  devices-in-package registers - if yes, allow fallback.
- does the C45 PHY support the C22 register set - if yes, allow
  fallback.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [RFC PATCH] drivers: net: mdio_bus: try indirect clause 45 regs access

[Baruch Siach]

Hi Russell,

On Sun, May 17 2020, Russell King - ARM Linux admin wrote:
> On Sun, May 17, 2020 at 01:20:56PM +0300, Baruch Siach wrote:
>> When the MDIO bus does not support directly clause 45 access, fallback
>> to indirect registers access method to read/write clause 45 registers
>> using clause 22 registers.
>> 
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>> 
>> Is that the right course?
>> 
>> Currently, this does not really work on the my target machine, which is
>> using the Armada 385 native MDIO bus (mvmdio) connected to clause 45
>> Marvell 88E2110 PHY. Registers MDIO_DEVS1 and MDIO_DEVS1 read bogus
>> values which breaks PHY identification. However, the phytool utility
>> reads the same registers correctly:
>> 
>> phytool eth1/2:1/5
>> ieee-phy: reg:0x05 val:0x008a
>> 
>> eth1 is connected to another PHY (clause 22) on the same MDIO bus.
>> 
>> The same hardware works nicely with the mdio-gpio bus implementation,
>> when mdio pins are muxed as GPIOs.
>
> Not all C45 PHYs are required to provide C22.  I'm pretty sure that
> accessing a C45 PHY through the indirect method is likely something
> that isn't well tested with PHYs, so getting wrong device IDs doesn't
> surprise me.

The 88E2110 PHY datasheets mentions support for indirect C45 access
(FWIW: Rev B, section 3.9.3).

> Is there a reason to try switching back to mvmdio on this device?

No technical reason. U-Boot does not currently provide bit-band MDIO,
and hardware manufacturing testers would like to do their thing in
U-Boot, for some reason.

I just thought it would be nice to support C45 over C22 mdio if the
hardware allows that.

> Some comments on the patch:
>
>> ---
>>  drivers/net/phy/mdio_bus.c | 12 ++++++++++++
>>  drivers/net/phy/phy-core.c |  2 +-
>>  include/linux/phy.h        |  2 ++
>>  3 files changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 7a4eb3f2cb74..12e39f794b29 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -790,6 +790,12 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
>>  	WARN_ON_ONCE(!mutex_is_locked(&bus->mdio_lock));
>>  
>>  	retval = bus->read(bus, addr, regnum);
>> +	if (retval == -EOPNOTSUPP && regnum & MII_ADDR_C45) {
>> +		int c45_devad = (regnum >> 16) & 0x1f;
>> +
>> +		mmd_phy_indirect(bus, addr, c45_devad, regnum & 0xfff);
>> +		retval = bus->read(bus, addr, MII_MMD_DATA);
>> +	}
>
> I don't think this should be done at mdiobus level; I think this is a
> layering violation.  It needs to happen at the PHY level because the
> indirect C45 access via C22 registers is specific to PHYs.
>
> It also needs to check in the general case that the PHY does indeed
> support the C22 register set - not all C45 PHYs do.
>
> So, I think we want this fallback to be conditional on:
>
> - are we probing for the PHY, trying to read its IDs and
>   devices-in-package registers - if yes, allow fallback.
> - does the C45 PHY support the C22 register set - if yes, allow
>   fallback.

I'll take a look. Thanks.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [RFC PATCH] drivers: net: mdio_bus: try indirect clause 45 regs access

[Andrew Lunn]

> > I don't think this should be done at mdiobus level; I think this is a
> > layering violation.  It needs to happen at the PHY level because the
> > indirect C45 access via C22 registers is specific to PHYs.
> >
> > It also needs to check in the general case that the PHY does indeed
> > support the C22 register set - not all C45 PHYs do.
> >
> > So, I think we want this fallback to be conditional on:
> >
> > - are we probing for the PHY, trying to read its IDs and
> >   devices-in-package registers - if yes, allow fallback.
> > - does the C45 PHY support the C22 register set - if yes, allow
> >   fallback.
> 
> I'll take a look. Thanks.
 
Hi Baruch

Another option to consider is a third compatible string. We have
compatibles for C22, C45. Add another one for C45 over C22, and have
the core support it as the third access method next to C22 and C45.

We already rely on the DT author getting C22 vs C45 correct for the
hardware. Is it too much to ask they get it write when there are three
options?

As to your particular hardware, if i remember correctly, some of the
Marvell SoCs have mdio and xmdio bus masters. The mdio bus can only do
C22, and the xmdio can only do C45. Have the hardware engineers put
the PHY on the wrong bus?

     Andrew

Re: [RFC PATCH] drivers: net: mdio_bus: try indirect clause 45 regs access

[Baruch Siach]

Hi Andrew,

On Sun, May 17 2020, Andrew Lunn wrote:
>> > I don't think this should be done at mdiobus level; I think this is a
>> > layering violation.  It needs to happen at the PHY level because the
>> > indirect C45 access via C22 registers is specific to PHYs.
>> >
>> > It also needs to check in the general case that the PHY does indeed
>> > support the C22 register set - not all C45 PHYs do.
>> >
>> > So, I think we want this fallback to be conditional on:
>> >
>> > - are we probing for the PHY, trying to read its IDs and
>> >   devices-in-package registers - if yes, allow fallback.
>> > - does the C45 PHY support the C22 register set - if yes, allow
>> >   fallback.
>> 
>> I'll take a look. Thanks.
>  
> Another option to consider is a third compatible string. We have
> compatibles for C22, C45. Add another one for C45 over C22, and have
> the core support it as the third access method next to C22 and C45.
>
> We already rely on the DT author getting C22 vs C45 correct for the
> hardware. Is it too much to ask they get it write when there are three
> options?

Networking hardware DT configuration is confusing enough already. Since
we can determine indirect C45 access automatically, I think we should do
that.

> As to your particular hardware, if i remember correctly, some of the
> Marvell SoCs have mdio and xmdio bus masters. The mdio bus can only do
> C22, and the xmdio can only do C45. Have the hardware engineers put
> the PHY on the wrong bus?

The Armada 385 has only C22 MDIO. Other Armada SoCs have both MDIO and
XMDIO.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -