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

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

[Michael Walle]

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(-)

-- 
2.30.2

[PATCH RFC net-next 1/5] net: phy: mscc-miim: reject clause 45 register accesses

[Michael Walle]

The driver doesn't support clause 45 register access yet, but doesn't
check if the access is a c45 one either. This leads to spurious register
reads and writes. Add the check.

Fixes: 542671fe4d86 ("net: phy: mscc-miim: Add MDIO driver")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/mdio/mdio-mscc-miim.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index c483ba67c21f..582969751b4c 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -102,6 +102,9 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
 	u32 val;
 	int ret;
 
+	if (regnum & MII_ADDR_C45)
+		return -EOPNOTSUPP;
+
 	ret = mscc_miim_wait_pending(bus);
 	if (ret)
 		goto out;
@@ -145,6 +148,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
 	struct mscc_miim_dev *miim = bus->priv;
 	int ret;
 
+	if (regnum & MII_ADDR_C45)
+		return -EOPNOTSUPP;
+
 	ret = mscc_miim_wait_pending(bus);
 	if (ret < 0)
 		goto out;
-- 
2.30.2

Re: [PATCH RFC net-next 1/5] net: phy: mscc-miim: reject clause 45 register accesses

[Andrew Lunn]

On Wed, Mar 23, 2022 at 07:34:15PM +0100, Michael Walle wrote:
> The driver doesn't support clause 45 register access yet, but doesn't
> check if the access is a c45 one either. This leads to spurious register
> reads and writes. Add the check.
> 
> Fixes: 542671fe4d86 ("net: phy: mscc-miim: Add MDIO driver")
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH RFC net-next 1/5] net: phy: mscc-miim: reject clause 45 register accesses

[Florian Fainelli]

On 3/23/22 11:34, Michael Walle wrote:
> The driver doesn't support clause 45 register access yet, but doesn't
> check if the access is a c45 one either. This leads to spurious register
> reads and writes. Add the check.
> 
> Fixes: 542671fe4d86 ("net: phy: mscc-miim: Add MDIO driver")
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC net-next 2/5] net: phy: support indirect c45 access in get_phy_c45_ids()

[Michael Walle]

There are some PHYs, namely the Maxlinear GPY215, whose driver is
explicitly supporting C45-over-C22 access. At least that was the
intention. In practice, it cannot work because get_phy_c45_ids()
will always issue c45 register accesses.

There is another issue at hand: the Microchip LAN8814, which is
a c22 only quad PHY, has issues with c45 accesses on the same bus
and its address decoder will find a match in the middle of another
c45 transaction. This will lead to spurious reads and writes. The
reads will corrupt the c45 in flight. The write will lead to random
writes to the LAN8814 registers. As a workaround for PHYs which
support C45-over-C22 register accesses, we can make the MDIO bus
c22-only.

For both reasons, extend the register accesses in get_phy_c45_ids()
to allow indirect accesses, indicated by the bus->probe_capabilities
bits. The probe_capabilites can then be degraded by a device tree
property, for example. Or it will just work when the MDIO driver
is c22-only and set the capabilities accordingly.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/phy_device.c | 46 ++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8406ac739def..c766f5bb421a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -649,6 +649,42 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 }
 EXPORT_SYMBOL(phy_device_create);
 
+static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int devad,
+				  u16 regnum)
+{
+	int ret;
+
+	/* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable */
+	if (bus->probe_capabilities == MDIOBUS_NO_CAP ||
+	    bus->probe_capabilities >= MDIOBUS_C45)
+		return mdiobus_c45_read(bus, prtad, devad, regnum);
+
+	mutex_lock(&bus->mdio_lock);
+
+	/* Write the desired MMD Devad */
+	ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, devad);
+	if (ret)
+		goto out;
+
+	/* Write the desired MMD register address */
+	ret = __mdiobus_write(bus, prtad, MII_MMD_DATA, regnum);
+	if (ret)
+		goto out;
+
+	/* Select the Function : DATA with no post increment */
+	ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL,
+			      devad | MII_MMD_CTRL_NOINCR);
+	if (ret)
+		goto out;
+
+	ret = __mdiobus_read(bus, prtad, MII_MMD_DATA);
+
+out:
+	mutex_unlock(&bus->mdio_lock);
+
+	return ret;
+}
+
 /* phy_c45_probe_present - checks to see if a MMD is present in the package
  * @bus: the target MII bus
  * @prtad: PHY package address on the MII bus
@@ -664,7 +700,7 @@ static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
 {
 	int stat2;
 
-	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
+	stat2 = mdiobus_probe_mmd_read(bus, prtad, devad, MDIO_STAT2);
 	if (stat2 < 0)
 		return stat2;
 
@@ -687,12 +723,12 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
 {
 	int phy_reg;
 
-	phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS2);
+	phy_reg = mdiobus_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS2);
 	if (phy_reg < 0)
 		return -EIO;
 	*devices_in_package = phy_reg << 16;
 
-	phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS1);
+	phy_reg = mdiobus_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS1);
 	if (phy_reg < 0)
 		return -EIO;
 	*devices_in_package |= phy_reg;
@@ -776,12 +812,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 				continue;
 		}
 
-		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
+		phy_reg = mdiobus_probe_mmd_read(bus, addr, i, MII_PHYSID1);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] = phy_reg << 16;
 
-		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID2);
+		phy_reg = mdiobus_probe_mmd_read(bus, addr, i, MII_PHYSID2);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] |= phy_reg;
-- 
2.30.2

Re: [PATCH RFC net-next 2/5] net: phy: support indirect c45 access in get_phy_c45_ids()

[Andrew Lunn]

> +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int devad,
> +				  u16 regnum)
> +{
> +	int ret;
> +
> +	/* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable */
> +	if (bus->probe_capabilities == MDIOBUS_NO_CAP ||
> +	    bus->probe_capabilities >= MDIOBUS_C45)

Maybe we should do the work and mark up those that are C45 capable. At
a quick count, see 16 of them.

> +		return mdiobus_c45_read(bus, prtad, devad, regnum);
> +
> +	mutex_lock(&bus->mdio_lock);
> +
> +	/* Write the desired MMD Devad */
> +	ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, devad);
> +	if (ret)
> +		goto out;
> +
> +	/* Write the desired MMD register address */
> +	ret = __mdiobus_write(bus, prtad, MII_MMD_DATA, regnum);
> +	if (ret)
> +		goto out;
> +
> +	/* Select the Function : DATA with no post increment */
> +	ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL,
> +			      devad | MII_MMD_CTRL_NOINCR);
> +	if (ret)
> +		goto out;

Make mmd_phy_indirect() usable, rather then repeat it.

     Andrew

Re: [PATCH RFC net-next 2/5] net: phy: support indirect c45 access in get_phy_c45_ids()

[Michael Walle]

Am 2022-03-23 20:39, schrieb Andrew Lunn:
>> +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int 
>> devad,
>> +				  u16 regnum)
>> +{
>> +	int ret;
>> +
>> +	/* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable 
>> */
>> +	if (bus->probe_capabilities == MDIOBUS_NO_CAP ||
>> +	    bus->probe_capabilities >= MDIOBUS_C45)
> 
> Maybe we should do the work and mark up those that are C45 capable. At
> a quick count, see 16 of them.

You mean look at they are MDIOBUS_C45, MDIOBUS_C22_C45 or MDIOBUS_C22
and drop MDIOBUS_NO_CAP?

> 
>> +		return mdiobus_c45_read(bus, prtad, devad, regnum);
>> +
>> +	mutex_lock(&bus->mdio_lock);
>> +
>> +	/* Write the desired MMD Devad */
>> +	ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, devad);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/* Write the desired MMD register address */
>> +	ret = __mdiobus_write(bus, prtad, MII_MMD_DATA, regnum);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/* Select the Function : DATA with no post increment */
>> +	ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL,
>> +			      devad | MII_MMD_CTRL_NOINCR);
>> +	if (ret)
>> +		goto out;
> 
> Make mmd_phy_indirect() usable, rather then repeat it.

I actually had that. But mmd_phy_indirect() doesn't check
the return code and neither does the __phy_write_mmd() it
actually deliberatly sets "ret = 0". So I wasn't sure. If you
are fine with a changed code flow in the error case, then sure.
I.e. mmd_phy_indirect() always (try to) do three accesses; with
error checks it might end after the first. If you are fine
with the error checks, should __phy_write_mmd() also check the
last mdiobus_write()?

-michael

Re: [PATCH RFC net-next 2/5] net: phy: support indirect c45 access in get_phy_c45_ids()

[Russell King (Oracle)]

On Wed, Mar 23, 2022 at 11:14:11PM +0100, Michael Walle wrote:
> I actually had that. But mmd_phy_indirect() doesn't check
> the return code and neither does the __phy_write_mmd() it
> actually deliberatly sets "ret = 0". So I wasn't sure. If you
> are fine with a changed code flow in the error case, then sure.
> I.e. mmd_phy_indirect() always (try to) do three accesses; with
> error checks it might end after the first. If you are fine
> with the error checks, should __phy_write_mmd() also check the
> last mdiobus_write()?

The reason for that goes back to
commit a59a4d1921664da63d801ba477950114c71c88c9
    phy: add the EEE support and the way to access to the MMD registers.

and to maintain compatibility with that; if we start checking for
errors now, we might trigger a kernel regression sadly.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 2/5] net: phy: support indirect c45 access in get_phy_c45_ids()

[Michael Walle]

Am 2022-03-30 18:18, schrieb Russell King (Oracle):
> On Wed, Mar 23, 2022 at 11:14:11PM +0100, Michael Walle wrote:
>> I actually had that. But mmd_phy_indirect() doesn't check
>> the return code and neither does the __phy_write_mmd() it
>> actually deliberatly sets "ret = 0". So I wasn't sure. If you
>> are fine with a changed code flow in the error case, then sure.
>> I.e. mmd_phy_indirect() always (try to) do three accesses; with
>> error checks it might end after the first. If you are fine
>> with the error checks, should __phy_write_mmd() also check the
>> last mdiobus_write()?
> 
> The reason for that goes back to
> commit a59a4d1921664da63d801ba477950114c71c88c9
>     phy: add the EEE support and the way to access to the MMD 
> registers.
> 
> and to maintain compatibility with that; if we start checking for
> errors now, we might trigger a kernel regression sadly.

I see that this is the commit which introduced the mmd_phy_indirect()
function, but I don't see why there is no return code checking.
Unlike now, there is a check for the last read (the one who
reads MII_MMD_DATA). That read which might return garbage if any
write has failed before - or if the bus is completely dead,
return an error. Current code will just return 0.

In any case, I don't have a strong opinion here. I just don't
see how that function could be reused while adding error checks
and without making it ugly, so I've just duplicated it.

Maybe something like this:

static int __phy_mmd_indirect_common(struct mii_bus *bus, int prtad,
                                      int devad, int addr,
                                      bool check_rc)
{
         int ret;

         /* Write the desired MMD Devad */
         ret = __mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
         if (check_rc && ret)
                 return ret;

         /* Write the desired MMD register address */
         ret = __mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
         if (check_rc && ret)
                 return ret;

         /* Select the Function : DATA with no post increment */
         ret = __mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
                               devad | MII_MMD_CTRL_NOINCR);
         if (check_rc && ret)
                 return ret;

         return 0;
}

int __phy_mmd_indirect(struct mii_bus *bus, int prtad,
                        int devad, int addr)
{
         return __phy_mmd_indirect_common(bus, prtad, devad,
                                          addr, true);
}

/* some function doc about deliberatly no error checking.. */
void __phy_mmd_indirect_legacy(struct mii_bus *bus, int prtad,
                                int devad, int addr)
{
         __phy_mmd_indirect_common(bus, prtad, devad,
                                   addr, false);
}

should the last two functions be static inline?

-michael

Re: [PATCH RFC net-next 2/5] net: phy: support indirect c45 access in get_phy_c45_ids()

[Michael Walle]

Am 2022-03-23 20:39, schrieb Andrew Lunn:
>> +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int 
>> devad,
>> +				  u16 regnum)
>> +{
>> +	int ret;
>> +
>> +	/* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable 
>> */
>> +	if (bus->probe_capabilities == MDIOBUS_NO_CAP ||
>> +	    bus->probe_capabilities >= MDIOBUS_C45)
> 
> Maybe we should do the work and mark up those that are C45 capable. At
> a quick count, see 16 of them.

I guess you grepped for MII_ADDR_C45 and had a look who
actually handled it correctly. Correct?

Let's say we mark these as either MDIOBUS_C45 or MDIOBUS_C45_C22,
can we then drop MDIOBUS_NO_CAP and make MDIOBUS_C22 the default
value (i.e. value 0) or do we have to go through all the mdio drivers
and add bus->probe_capabilities = MDIOBUS_C22 ? Grepping for
{of_,}mdiobus_register lists quite a few of them.

-michael

Re: [PATCH RFC net-next 2/5] net: phy: support indirect c45 access in get_phy_c45_ids()

[Andrew Lunn]

On Thu, Mar 24, 2022 at 03:28:43PM +0100, Michael Walle wrote:
> Am 2022-03-23 20:39, schrieb Andrew Lunn:
> > > +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad,
> > > int devad,
> > > +				  u16 regnum)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* For backwards compatibility, treat MDIOBUS_NO_CAP as c45
> > > capable */
> > > +	if (bus->probe_capabilities == MDIOBUS_NO_CAP ||
> > > +	    bus->probe_capabilities >= MDIOBUS_C45)
> > 
> > Maybe we should do the work and mark up those that are C45 capable. At
> > a quick count, see 16 of them.
> 
> I guess you grepped for MII_ADDR_C45 and had a look who
> actually handled it correctly. Correct?

Yes.

> Let's say we mark these as either MDIOBUS_C45 or MDIOBUS_C45_C22,
> can we then drop MDIOBUS_NO_CAP and make MDIOBUS_C22 the default
> value (i.e. value 0) or do we have to go through all the mdio drivers
> and add bus->probe_capabilities = MDIOBUS_C22 ? Grepping for
> {of_,}mdiobus_register lists quite a few of them.

The minimum is marking those that support C45 with MDIOBUS_C45 or
MDIOBUS_C45_C22. We can then really trust it does C45. Those that
don't set probe_capabilities we assume are C22 only. That should be
enough for this problem.

FYI: Yesterday i started actually adding probe_capabilities values to
drivers. I did everything in driver/net/mdio. I will work on the rest
over the next few days and then post an RFC patchset.

     Andrew

[PATCH RFC net-next 3/5] net: phy: mscc-miim: add probe_capabilities

[Michael Walle]

The driver is currently only capable of doing c22 accesses. Add the
corresponding probe_capabilities.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/mdio/mdio-mscc-miim.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 582969751b4c..c9efcfa2a1ce 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -225,6 +225,7 @@ int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
 	bus->read = mscc_miim_read;
 	bus->write = mscc_miim_write;
 	bus->reset = mscc_miim_reset;
+	bus->probe_capabilities = MDIOBUS_C22;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
 	bus->parent = dev;
 
-- 
2.30.2

Re: [PATCH RFC net-next 3/5] net: phy: mscc-miim: add probe_capabilities

[Florian Fainelli]

On 3/23/22 11:34, Michael Walle wrote:
> The driver is currently only capable of doing c22 accesses. Add the
> corresponding probe_capabilities.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Michael Walle]

The GPY215 driver supports indirect accesses to c45 over the c22
registers. In its probe function phy_get_c45_ids() is called and the
author descibed their use case as follows:

  The problem comes from condition "phydev->c45_ids.mmds_present &
  MDIO_DEVS_AN".

  Our product supports both C22 and C45.

  In the real system, we found C22 was used by customers (with indirect
  access to C45 registers when necessary).

So it is pretty clear that the intention was to have a method to use the
c45 features over a c22-only MDIO bus. The purpose of calling
phy_get_c45_ids() is to populate the .c45_ids for a PHY which wasn't
probed as a c45 one. Thus, first rename the phy_get_c45_ids() function
to reflect its actual meaning and second, add a new flag which indicates
that this is actually a c45 PHY but behind a c22 bus. The latter is
important for phylink because phylink will treat c45 in a special way by
checking the .is_c45 property. But in our case this isn't set.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/mxl-gpy.c    |  2 +-
 drivers/net/phy/phy_device.c | 20 +++++++++++++++-----
 include/linux/phy.h          |  4 +++-
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 5ce1bf03bbd7..0c825ec20eaa 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -99,7 +99,7 @@ static int gpy_probe(struct phy_device *phydev)
 	int ret;
 
 	if (!phydev->is_c45) {
-		ret = phy_get_c45_ids(phydev);
+		ret = phy_get_c45_ids_by_c22(phydev);
 		if (ret < 0)
 			return ret;
 	}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c766f5bb421a..43354b261bd5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1005,18 +1005,28 @@ void phy_device_remove(struct phy_device *phydev)
 EXPORT_SYMBOL(phy_device_remove);
 
 /**
- * phy_get_c45_ids - Read 802.3-c45 IDs for phy device.
+ * phy_get_c45_ids - Read 802.3-c45 IDs for phy device by using indirect
+ *                   c22 accesses.
  * @phydev: phy_device structure to read 802.3-c45 IDs
  *
  * Returns zero on success, %-EIO on bus access error, or %-ENODEV if
  * the "devices in package" is invalid.
  */
-int phy_get_c45_ids(struct phy_device *phydev)
+int phy_get_c45_ids_by_c22(struct phy_device *phydev)
 {
-	return get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
-			       &phydev->c45_ids);
+	int ret;
+
+	if (WARN(phydev->is_c45, "PHY is already clause 45\n"))
+		return -EINVAL;
+
+	ret = get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
+			      &phydev->c45_ids);
+	if (!ret)
+		phydev->is_c45_over_c22 = true;
+
+	return ret;
 }
-EXPORT_SYMBOL(phy_get_c45_ids);
+EXPORT_SYMBOL(phy_get_c45_ids_by_c22);
 
 /**
  * phy_find_first - finds the first PHY device on the bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36ca2b5c2253..eb436d603feb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -525,6 +525,7 @@ struct macsec_ops;
  * @phy_id: UID for this device found during discovery
  * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
  * @is_c45:  Set to true if this PHY uses clause 45 addressing.
+ * @is_c45_over_c22:  Set to true if this PHY uses c45-over-c22 addressing.
  * @is_internal: Set to true if this PHY is internal to a MAC.
  * @is_pseudo_fixed_link: Set to true if this PHY is an Ethernet switch, etc.
  * @is_gigabit_capable: Set to true if PHY supports 1000Mbps
@@ -606,6 +607,7 @@ struct phy_device {
 
 	struct phy_c45_device_ids c45_ids;
 	unsigned is_c45:1;
+	unsigned is_c45_over_c22:1;
 	unsigned is_internal:1;
 	unsigned is_pseudo_fixed_link:1;
 	unsigned is_gigabit_capable:1;
@@ -1466,7 +1468,7 @@ static inline int phy_device_register(struct phy_device *phy)
 static inline void phy_device_free(struct phy_device *phydev) { }
 #endif /* CONFIG_PHYLIB */
 void phy_device_remove(struct phy_device *phydev);
-int phy_get_c45_ids(struct phy_device *phydev);
+int phy_get_c45_ids_by_c22(struct phy_device *phydev);
 int phy_init_hw(struct phy_device *phydev);
 int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
-- 
2.30.2

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Andrew Lunn]

On Wed, Mar 23, 2022 at 07:34:18PM +0100, Michael Walle wrote:
> The GPY215 driver supports indirect accesses to c45 over the c22
> registers. In its probe function phy_get_c45_ids() is called and the
> author descibed their use case as follows:
> 
>   The problem comes from condition "phydev->c45_ids.mmds_present &
>   MDIO_DEVS_AN".
> 
>   Our product supports both C22 and C45.
> 
>   In the real system, we found C22 was used by customers (with indirect
>   access to C45 registers when necessary).
> 
> So it is pretty clear that the intention was to have a method to use the
> c45 features over a c22-only MDIO bus. The purpose of calling
> phy_get_c45_ids() is to populate the .c45_ids for a PHY which wasn't
> probed as a c45 one. Thus, first rename the phy_get_c45_ids() function
> to reflect its actual meaning and second, add a new flag which indicates
> that this is actually a c45 PHY but behind a c22 bus. The latter is
> important for phylink because phylink will treat c45 in a special way by
> checking the .is_c45 property. But in our case this isn't set.

Thinking out loud...

1) We have a C22 only bus. Easy, C45 over C22 should be used.

2) We have a C45 only bus. Easy, C45 should be used, and it will of
   probed that way.

3) We have a C22 and C45 bus, but MDIOBUS_NO_CAP. It will probe C22,
   but ideally we want to swap to C45.

4) We have a C22 and C45 bus, MDIOBUS_C22_C45. It will probe C22, but
   ideally we want to swap to C45.

> @@ -99,7 +99,7 @@ static int gpy_probe(struct phy_device *phydev)
>  	int ret;
>  
>  	if (!phydev->is_c45) {
> -		ret = phy_get_c45_ids(phydev);
> +		ret = phy_get_c45_ids_by_c22(phydev);
>  		if (ret < 0)
>  			return ret;
>  	}

If we are inside the if, we know we probed C22. We have to achieve two
things:

1) Get the c45 ids,
2) Figure out if C45 works, or if C45 over C22 is needed.

I don't see how we are getting this second bit of information, if we
are explicitly using c45 over c22.

This _by_c22 is also making me think of the previous patch, where we
look at the bus capabilities. We are explicitly saying here was want
c45 over c22, and the PHY driver should know the PHY is capable of
it. So we don't need to look at the capabilities, just do it.

     Andrew

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Michael Walle]

Am 2022-03-23 21:07, schrieb Andrew Lunn:
> On Wed, Mar 23, 2022 at 07:34:18PM +0100, Michael Walle wrote:
>> The GPY215 driver supports indirect accesses to c45 over the c22
>> registers. In its probe function phy_get_c45_ids() is called and the
>> author descibed their use case as follows:
>> 
>>   The problem comes from condition "phydev->c45_ids.mmds_present &
>>   MDIO_DEVS_AN".
>> 
>>   Our product supports both C22 and C45.
>> 
>>   In the real system, we found C22 was used by customers (with 
>> indirect
>>   access to C45 registers when necessary).
>> 
>> So it is pretty clear that the intention was to have a method to use 
>> the
>> c45 features over a c22-only MDIO bus. The purpose of calling
>> phy_get_c45_ids() is to populate the .c45_ids for a PHY which wasn't
>> probed as a c45 one. Thus, first rename the phy_get_c45_ids() function
>> to reflect its actual meaning and second, add a new flag which 
>> indicates
>> that this is actually a c45 PHY but behind a c22 bus. The latter is
>> important for phylink because phylink will treat c45 in a special way 
>> by
>> checking the .is_c45 property. But in our case this isn't set.
> 
> Thinking out loud...
> 
> 1) We have a C22 only bus. Easy, C45 over C22 should be used.
> 
> 2) We have a C45 only bus. Easy, C45 should be used, and it will of
>    probed that way.
> 
> 3) We have a C22 and C45 bus, but MDIOBUS_NO_CAP. It will probe C22,
>    but ideally we want to swap to C45.
> 
> 4) We have a C22 and C45 bus, MDIOBUS_C22_C45. It will probe C22, but
>    ideally we want to swap to C45.

I presume you are speaking of
https://elixir.bootlin.com/linux/v5.17/source/drivers/net/phy/mdio_bus.c#L700

Shouldn't that be the other way around then? How would you tell if
you can do C45?

>> @@ -99,7 +99,7 @@ static int gpy_probe(struct phy_device *phydev)
>>  	int ret;
>> 
>>  	if (!phydev->is_c45) {
>> -		ret = phy_get_c45_ids(phydev);
>> +		ret = phy_get_c45_ids_by_c22(phydev);
>>  		if (ret < 0)
>>  			return ret;
>>  	}
> 
> If we are inside the if, we know we probed C22. We have to achieve two
> things:
> 
> 1) Get the c45 ids,
> 2) Figure out if C45 works, or if C45 over C22 is needed.
> 
> I don't see how we are getting this second bit of information, if we
> are explicitly using c45 over c22.

That is related to how C45 capable PHYs are probed (your 4) above),
right? If the PHY would be probed correctly as C45 we wouldn't have
to worry about it. TBH I didn't consider that a valid case because
I thought there were other means to tell "treat this PHY as C45",
that is by the device tree compatible, for example.

Btw. all of this made me question if this is actually the correct
place, or if if shouldn't be handled in the core. With a flag
in the phy driver which might indicate its capable of doing
c45 over c22.

> This _by_c22 is also making me think of the previous patch, where we
> look at the bus capabilities. We are explicitly saying here was want
> c45 over c22, and the PHY driver should know the PHY is capable of
> it. So we don't need to look at the capabilities, just do it.

Mh? I can't follow you here. Are you talking about the
probe_capabilites? These are for the bus probing, i.e. if you can
call mdiobus_c45_read().

-michael

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Andrew Lunn]

> > Thinking out loud...
> > 
> > 1) We have a C22 only bus. Easy, C45 over C22 should be used.
> > 
> > 2) We have a C45 only bus. Easy, C45 should be used, and it will of
> >    probed that way.
> > 
> > 3) We have a C22 and C45 bus, but MDIOBUS_NO_CAP. It will probe C22,
> >    but ideally we want to swap to C45.
> > 
> > 4) We have a C22 and C45 bus, MDIOBUS_C22_C45. It will probe C22, but
> >    ideally we want to swap to C45.
> 
> I presume you are speaking of
> https://elixir.bootlin.com/linux/v5.17/source/drivers/net/phy/mdio_bus.c#L700

Yes.

> Shouldn't that be the other way around then? How would you tell if
> you can do C45?

For a long time, only C22 was probed. To actually make use of a C45
only PHY, you had to have a compatible string in DT which indicated a
C45 device is present on the bus. But then none DT systems came along
which needed to find a C45 only PHY during probe without the hint it
is was actually there. That is when the probe capabilities we added,
and the scan extended to look for a C45 device if the capabilities
indicates the bus actually supported it. But to keep backwards
compatibility, C22 was scanned first and then C45 afterwards.

To some extent, we need to separate finding the device on the bus to
actually using the device. The device might respond to C22, give us
its ID, get the correct driver loaded based on that ID, and the driver
then uses the C45 address space to actually configure the PHY.

Then there is the Marvel 10G PHY. It responds to C22, but returns 0
for the ID! There is a special case for this in the code, it then
looks in the C45 space and uses the ID from there, if it finds
something useful.

So as i said in my reply to the cover letter, we have two different
state variables:

1) The PHY has the C45 register space.

2) We need to either use C45 transfers, or C45 over C22 transfers to
   access the C45 register space.

And we potentially have a chicken/egg problem. The PHY driver knows
1), but in order to know what driver to load we need the ID registers
from the PHY, or some external hint like DT. We are also currently
only probing C22, or C45, but not C45 over C22. And i'm not sure we
actually can probe C45 over C22 because there are C22 only PHYs which
use those two register for other things. So we are back to the driver
again which does know if C45 over C22 will work.

So phydev->has_c45 we can provisionally set if we probed the PHY by
C45. But the driver should also set it if it knows better, or even the
core can set it the first time the driver uses an _mmd API call.

phydev->c45_over_c22 we are currently in a bad shape for. We cannot
reliably say the bus master supports C45. If the bus capabilities say
C22 only, we can set phydev->c45_over_c22. If the bus capabilities
list C45, we can set it false. But that only covers a few busses, most
don't have any capabilities set. We can try a C45 access and see if we
get an -EOPNOTSUPP, in which case we can set phydev->c45_over_c22. But
the bus driver could also do the wrong thing, issue a C22 transfer and
give us back rubbish.

So i think we do need to review all the bus drivers and any which do
support C45 need their capabilities set to indicate that. We can then
set phydev->c45_over_c22.

    Andrew

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Michael Walle]

Am 2022-03-24 01:41, schrieb Andrew Lunn:
>> > Thinking out loud...
>> >
>> > 1) We have a C22 only bus. Easy, C45 over C22 should be used.
>> >
>> > 2) We have a C45 only bus. Easy, C45 should be used, and it will of
>> >    probed that way.
>> >
>> > 3) We have a C22 and C45 bus, but MDIOBUS_NO_CAP. It will probe C22,
>> >    but ideally we want to swap to C45.
>> >
>> > 4) We have a C22 and C45 bus, MDIOBUS_C22_C45. It will probe C22, but
>> >    ideally we want to swap to C45.
>> 
>> I presume you are speaking of
>> https://elixir.bootlin.com/linux/v5.17/source/drivers/net/phy/mdio_bus.c#L700
> 
> Yes.
> 
>> Shouldn't that be the other way around then? How would you tell if
>> you can do C45?
> 
> For a long time, only C22 was probed. To actually make use of a C45
> only PHY, you had to have a compatible string in DT which indicated a
> C45 device is present on the bus. But then none DT systems came along
> which needed to find a C45 only PHY during probe without the hint it
> is was actually there. That is when the probe capabilities we added,
> and the scan extended to look for a C45 device if the capabilities
> indicates the bus actually supported it. But to keep backwards
> compatibility, C22 was scanned first and then C45 afterwards.

Ok, I figured.

> To some extent, we need to separate finding the device on the bus to
> actually using the device. The device might respond to C22, give us
> its ID, get the correct driver loaded based on that ID, and the driver
> then uses the C45 address space to actually configure the PHY.
> 
> Then there is the Marvel 10G PHY. It responds to C22, but returns 0
> for the ID! There is a special case for this in the code, it then
> looks in the C45 space and uses the ID from there, if it finds
> something useful.
> 
> So as i said in my reply to the cover letter, we have two different
> state variables:
> 
> 1) The PHY has the C45 register space.
> 
> 2) We need to either use C45 transfers, or C45 over C22 transfers to
>    access the C45 register space.
> 
> And we potentially have a chicken/egg problem. The PHY driver knows
> 1), but in order to know what driver to load we need the ID registers
> from the PHY, or some external hint like DT. We are also currently
> only probing C22, or C45, but not C45 over C22. And i'm not sure we
> actually can probe C45 over C22 because there are C22 only PHYs which
> use those two register for other things. So we are back to the driver
> again which does know if C45 over C22 will work.

Isn't it safe to assume that if a PHY implements the indirect
registers for c45 in its c22 space that it will also have a valid
PHY ID and then the it's driver will be probed? So if a PHY is
probed as c22 its driver might tell us "wait, it's actually a c45
phy and hey for your convenience it also have the indirect registers
in c22". We can then set has_c45 and maybe c45_over_c22 (also depending
on the bus capabilities).

> So phydev->has_c45 we can provisionally set if we probed the PHY by
> C45. But the driver should also set it if it knows better, or even the
> core can set it the first time the driver uses an _mmd API call.

I'm not sure about the _mmd calls, there are PHYs which have MMDs
(I guess EEE is an example?) but are not capable of C45 accesses.

> phydev->c45_over_c22 we are currently in a bad shape for. We cannot
> reliably say the bus master supports C45. If the bus capabilities say
> C22 only, we can set phydev->c45_over_c22. If the bus capabilities
> list C45, we can set it false. But that only covers a few busses, most
> don't have any capabilities set. We can try a C45 access and see if we
> get an -EOPNOTSUPP, in which case we can set phydev->c45_over_c22. But
> the bus driver could also do the wrong thing, issue a C22 transfer and
> give us back rubbish.

First question, what do you think about keeping the is_c45 property but
with a different meaning and add use_c45_over_c22. That way it will be
less code churn:

  * @is_c45:  Set to true if this PHY has clause 45 address space.
  * @use_c45_over_c22:  Set to true if c45-over-c22 addressing is used.

  1) c45 phy probed as c45 -> is_c45 = 1, like it is at the moment
     use c45 transfers
2a) c45 phy probed as c22 -> is_c45 = 1, use_c45_over_c22 = 0
     use c45 transfers
2b) c45 phy probed as c22 -> is_c45 = 1, use_c45_over_c22 = 1
     use c22 transfers

Did I miss something?

To promote from c22 to c45 we could look at a flag in the PHY
driver. That should be basically that what the gpy driver is trying
to do with the "if (!is_c45) get_c45_ids()" (but fail).

> So i think we do need to review all the bus drivers and any which do
> support C45 need their capabilities set to indicate that. We can then
> set phydev->c45_over_c22.

I could add the probe_capabilites, or at least I could try. But it won't
tell us if the PHY is capable of the indirect addressing. So we aren't
much further in this regard. But IMHO this can be done incrementally
if someone is interested in having that feature. He should also be in
the position to test it properly.

[just saw your latest mail while writing this, so half of it is done
anyway, btw, I did the same today ;)]

-michael

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Andrew Lunn]

> > To some extent, we need to separate finding the device on the bus to
> > actually using the device. The device might respond to C22, give us
> > its ID, get the correct driver loaded based on that ID, and the driver
> > then uses the C45 address space to actually configure the PHY.
> > 
> > Then there is the Marvel 10G PHY. It responds to C22, but returns 0
> > for the ID! There is a special case for this in the code, it then
> > looks in the C45 space and uses the ID from there, if it finds
> > something useful.
> > 
> > So as i said in my reply to the cover letter, we have two different
> > state variables:
> > 
> > 1) The PHY has the C45 register space.
> > 
> > 2) We need to either use C45 transfers, or C45 over C22 transfers to
> >    access the C45 register space.
> > 
> > And we potentially have a chicken/egg problem. The PHY driver knows
> > 1), but in order to know what driver to load we need the ID registers
> > from the PHY, or some external hint like DT. We are also currently
> > only probing C22, or C45, but not C45 over C22. And i'm not sure we
> > actually can probe C45 over C22 because there are C22 only PHYs which
> > use those two register for other things. So we are back to the driver
> > again which does know if C45 over C22 will work.
> 
> Isn't it safe to assume that if a PHY implements the indirect
> registers for c45 in its c22 space that it will also have a valid
> PHY ID and then the it's driver will be probed?

See: https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L895

No valid ID in C22 space.

> So if a PHY is
> probed as c22 its driver might tell us "wait, it's actually a c45
> phy and hey for your convenience it also have the indirect registers
> in c22". We can then set has_c45 and maybe c45_over_c22 (also depending
> on the bus capabilities).

In general, if the core can do something, it is better than the driver
doing it. If the core cannot reliably figure it out, then we have to
leave it to the drivers. It could well be we need the drivers to set
has_c45. I would prefer that drivers don't touch c45_over_c22 because
they don't have the knowledge of what the bus is capable of doing. The
only valid case i can think of is for a very oddball PHY which has C45
register space, but cannot actually do C45 transfers, and so C45 over
C22 is the only option.

> > So phydev->has_c45 we can provisionally set if we probed the PHY by
> > C45. But the driver should also set it if it knows better, or even the
> > core can set it the first time the driver uses an _mmd API call.
> 
> I'm not sure about the _mmd calls, there are PHYs which have MMDs
> (I guess EEE is an example?) but are not capable of C45 accesses.

Ah, yes, i forgot about EEE. That was a bad idea.

> > phydev->c45_over_c22 we are currently in a bad shape for. We cannot
> > reliably say the bus master supports C45. If the bus capabilities say
> > C22 only, we can set phydev->c45_over_c22. If the bus capabilities
> > list C45, we can set it false. But that only covers a few busses, most
> > don't have any capabilities set. We can try a C45 access and see if we
> > get an -EOPNOTSUPP, in which case we can set phydev->c45_over_c22. But
> > the bus driver could also do the wrong thing, issue a C22 transfer and
> > give us back rubbish.
> 
> First question, what do you think about keeping the is_c45 property but
> with a different meaning and add use_c45_over_c22. That way it will be
> less code churn:
> 
>  * @is_c45:  Set to true if this PHY has clause 45 address space.
>  * @use_c45_over_c22:  Set to true if c45-over-c22 addressing is used.

I prefer to change is_c45. We then get the compiler to help us with
code review. The build bots will tell us about any code we fail to
check and change. It will also help anybody with out of tree code
making use of is_c45.

       Andrew

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Michael Walle]

Am 2022-03-24 17:23, schrieb Andrew Lunn:
>> > To some extent, we need to separate finding the device on the bus to
>> > actually using the device. The device might respond to C22, give us
>> > its ID, get the correct driver loaded based on that ID, and the driver
>> > then uses the C45 address space to actually configure the PHY.
>> >
>> > Then there is the Marvel 10G PHY. It responds to C22, but returns 0
>> > for the ID! There is a special case for this in the code, it then
>> > looks in the C45 space and uses the ID from there, if it finds
>> > something useful.
>> >
>> > So as i said in my reply to the cover letter, we have two different
>> > state variables:
>> >
>> > 1) The PHY has the C45 register space.
>> >
>> > 2) We need to either use C45 transfers, or C45 over C22 transfers to
>> >    access the C45 register space.
>> >
>> > And we potentially have a chicken/egg problem. The PHY driver knows
>> > 1), but in order to know what driver to load we need the ID registers
>> > from the PHY, or some external hint like DT. We are also currently
>> > only probing C22, or C45, but not C45 over C22. And i'm not sure we
>> > actually can probe C45 over C22 because there are C22 only PHYs which
>> > use those two register for other things. So we are back to the driver
>> > again which does know if C45 over C22 will work.
>> 
>> Isn't it safe to assume that if a PHY implements the indirect
>> registers for c45 in its c22 space that it will also have a valid
>> PHY ID and then the it's driver will be probed?
> 
> See:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L895
> 
> No valid ID in C22 space.

I actually looked at the datasheet and yes, it implements the
registers 13 and 14 in c22 to access the c45 space. I couldn't
find any descriptions of other c22 registers though.

>> So if a PHY is
>> probed as c22 its driver might tell us "wait, it's actually a c45
>> phy and hey for your convenience it also have the indirect registers
>> in c22". We can then set has_c45 and maybe c45_over_c22 (also 
>> depending
>> on the bus capabilities).
> 
> In general, if the core can do something, it is better than the driver
> doing it. If the core cannot reliably figure it out, then we have to
> leave it to the drivers. It could well be we need the drivers to set
> has_c45. I would prefer that drivers don't touch c45_over_c22 because
> they don't have the knowledge of what the bus is capable of doing. The
> only valid case i can think of is for a very oddball PHY which has C45
> register space, but cannot actually do C45 transfers, and so C45 over
> C22 is the only option.

And how would you know that the PHY has the needed registers in c22
space? Or do we assume that every C45 PHY has these registers?

..

>> > phydev->c45_over_c22 we are currently in a bad shape for. We cannot
>> > reliably say the bus master supports C45. If the bus capabilities say
>> > C22 only, we can set phydev->c45_over_c22. If the bus capabilities
>> > list C45, we can set it false. But that only covers a few busses, most
>> > don't have any capabilities set. We can try a C45 access and see if we
>> > get an -EOPNOTSUPP, in which case we can set phydev->c45_over_c22. But
>> > the bus driver could also do the wrong thing, issue a C22 transfer and
>> > give us back rubbish.
>> 
>> First question, what do you think about keeping the is_c45 property 
>> but
>> with a different meaning and add use_c45_over_c22. That way it will be
>> less code churn:
>> 
>>  * @is_c45:  Set to true if this PHY has clause 45 address space.
>>  * @use_c45_over_c22:  Set to true if c45-over-c22 addressing is used.
> 
> I prefer to change is_c45. We then get the compiler to help us with
> code review. The build bots will tell us about any code we fail to
> check and change. It will also help anybody with out of tree code
> making use of is_c45.

Ok. Fair enough.

-michael

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Andrew Lunn]

> The
> > only valid case i can think of is for a very oddball PHY which has C45
> > register space, but cannot actually do C45 transfers, and so C45 over
> > C22 is the only option.
> 
> And how would you know that the PHY has the needed registers in c22
> space? Or do we assume that every C45 PHY has these registers?

I think it is a reasonable assumption at the moment. We have around
170 MDIO bus masters in Linux. All but one can do C22. There are
around 15 which can do both C22 and C45, and only one that i know of
which is C45 only. So anybody making a C45 transfer only PHY is making
their device hard to sell.

I suppose a PHY integrated with the MAC could by C45 only, and not
support C22, but i've not seen such a device yet. My guess such a
device would be an Ethernet switch, or a USB device, or maybe an
automotive device with a T1 PHY.

       Andrew

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Russell King (Oracle)]

On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:
> > The
> > > only valid case i can think of is for a very oddball PHY which has C45
> > > register space, but cannot actually do C45 transfers, and so C45 over
> > > C22 is the only option.
> > 
> > And how would you know that the PHY has the needed registers in c22
> > space? Or do we assume that every C45 PHY has these registers?
> 
> I think it is a reasonable assumption at the moment. We have around
> 170 MDIO bus masters in Linux. All but one can do C22.

I don't think that is correct. I'm aware of the Marvell XMDIO driver
that is C45 only, and also xgene's non-rgmii "xfi" variant which is
also C45 only. Note that the xfi variant doesn't reject C22 and makes
no distinction between a C22 and C45 access (so a C22 access to
phy_id = 0 reg = 0 hits C45 phy_id = 0 mmd 0 reg 0.

MDIO drivers are IMHO an utter mess and are in dire need of fixing...
and I'm coming to the conclusion that the bodge of passing both C22
and C45 accesses through the same read/write functions is a huge
mistake, one that is crying out for fixing to prevent more prolification
of this kind of mess.

Yes, it's a lot of work, but I think it needs to be done. Retrofitting
the MDIO drivers with checks etc sounds nice, but if we assume that
patches will continue to be applied to net-next with little review,
we have a losing battle - it would be better to have interfaces designed
to make this kind of mistake impossible.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Andrew Lunn]

On Thu, Mar 31, 2022 at 12:50:42PM +0100, Russell King (Oracle) wrote:
> On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:
> > > The
> > > > only valid case i can think of is for a very oddball PHY which has C45
> > > > register space, but cannot actually do C45 transfers, and so C45 over
> > > > C22 is the only option.
> > > 
> > > And how would you know that the PHY has the needed registers in c22
> > > space? Or do we assume that every C45 PHY has these registers?
> > 
> > I think it is a reasonable assumption at the moment. We have around
> > 170 MDIO bus masters in Linux. All but one can do C22.
> 
> I don't think that is correct. I'm aware of the Marvell XMDIO driver
> that is C45 only, and also xgene's non-rgmii "xfi" variant which is
> also C45 only. Note that the xfi variant doesn't reject C22 and makes
> no distinction between a C22 and C45 access (so a C22 access to
> phy_id = 0 reg = 0 hits C45 phy_id = 0 mmd 0 reg 0.
> 
> MDIO drivers are IMHO an utter mess and are in dire need of fixing...
> and I'm coming to the conclusion that the bodge of passing both C22
> and C45 accesses through the same read/write functions is a huge
> mistake, one that is crying out for fixing to prevent more prolification
> of this kind of mess.
> 
> Yes, it's a lot of work, but I think it needs to be done. Retrofitting
> the MDIO drivers with checks etc sounds nice, but if we assume that
> patches will continue to be applied to net-next with little review,
> we have a losing battle - it would be better to have interfaces designed
> to make this kind of mistake impossible.

Hi Russell

So what i think you are saying is change the mii_bus structure:

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36ca2b5c2253..26322ee23867 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -353,10 +353,15 @@ struct mii_bus {
        const char *name;
        char id[MII_BUS_ID_SIZE];
        void *priv;
-       /** @read: Perform a read transfer on the bus */
-       int (*read)(struct mii_bus *bus, int addr, int regnum);
-       /** @write: Perform a write transfer on the bus */
-       int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
+       /** @read: Perform a C22 read transfer on the bus */
+       int (*read_c22)(struct mii_bus *bus, int addr, int regnum);
+       /** @write: Perform a C22 write transfer on the bus */
+       int (*write_c22)(struct mii_bus *bus, int addr, int regnum, u16 val);
+       /** @read: Perform a C45 read transfer on the bus */
+       int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
+       /** @write: Perform a C45 write transfer on the bus */
+       int (*write_c45)(struct mii_bus *bus, int addr, int devnum,
+                        int regnum, u16 val);
        /** @reset: Perform a reset of the bus */
        int (*reset)(struct mii_bus *bus);

This way we get a cleaner interface, and the compiler helping us
finding drivers we miss during conversion?

	Andrew

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Russell King (Oracle)]

On Thu, Mar 31, 2022 at 02:06:16PM +0200, Andrew Lunn wrote:
> On Thu, Mar 31, 2022 at 12:50:42PM +0100, Russell King (Oracle) wrote:
> > On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:
> > > > The
> > > > > only valid case i can think of is for a very oddball PHY which has C45
> > > > > register space, but cannot actually do C45 transfers, and so C45 over
> > > > > C22 is the only option.
> > > > 
> > > > And how would you know that the PHY has the needed registers in c22
> > > > space? Or do we assume that every C45 PHY has these registers?
> > > 
> > > I think it is a reasonable assumption at the moment. We have around
> > > 170 MDIO bus masters in Linux. All but one can do C22.
> > 
> > I don't think that is correct. I'm aware of the Marvell XMDIO driver
> > that is C45 only, and also xgene's non-rgmii "xfi" variant which is
> > also C45 only. Note that the xfi variant doesn't reject C22 and makes
> > no distinction between a C22 and C45 access (so a C22 access to
> > phy_id = 0 reg = 0 hits C45 phy_id = 0 mmd 0 reg 0.
> > 
> > MDIO drivers are IMHO an utter mess and are in dire need of fixing...
> > and I'm coming to the conclusion that the bodge of passing both C22
> > and C45 accesses through the same read/write functions is a huge
> > mistake, one that is crying out for fixing to prevent more prolification
> > of this kind of mess.
> > 
> > Yes, it's a lot of work, but I think it needs to be done. Retrofitting
> > the MDIO drivers with checks etc sounds nice, but if we assume that
> > patches will continue to be applied to net-next with little review,
> > we have a losing battle - it would be better to have interfaces designed
> > to make this kind of mistake impossible.
> 
> Hi Russell
> 
> So what i think you are saying is change the mii_bus structure:
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 36ca2b5c2253..26322ee23867 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -353,10 +353,15 @@ struct mii_bus {
>         const char *name;
>         char id[MII_BUS_ID_SIZE];
>         void *priv;
> -       /** @read: Perform a read transfer on the bus */
> -       int (*read)(struct mii_bus *bus, int addr, int regnum);
> -       /** @write: Perform a write transfer on the bus */
> -       int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
> +       /** @read: Perform a C22 read transfer on the bus */
> +       int (*read_c22)(struct mii_bus *bus, int addr, int regnum);
> +       /** @write: Perform a C22 write transfer on the bus */
> +       int (*write_c22)(struct mii_bus *bus, int addr, int regnum, u16 val);
> +       /** @read: Perform a C45 read transfer on the bus */
> +       int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
> +       /** @write: Perform a C45 write transfer on the bus */
> +       int (*write_c45)(struct mii_bus *bus, int addr, int devnum,
> +                        int regnum, u16 val);
>         /** @reset: Perform a reset of the bus */
>         int (*reset)(struct mii_bus *bus);
> 
> This way we get a cleaner interface, and the compiler helping us
> finding drivers we miss during conversion?

Yes, and not only does it mean the compiler helps us find unconverted
drivers, it will also stop new drivers that use the old interfaces being
merged and most importantly it puts an end to the ongoing question about
which clause accesses are actually supported by a MDIO driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Russell King (Oracle)]

On Thu, Mar 24, 2022 at 06:18:14PM +0100, Michael Walle wrote:
> Am 2022-03-24 17:23, schrieb Andrew Lunn:
> > > Isn't it safe to assume that if a PHY implements the indirect
> > > registers for c45 in its c22 space that it will also have a valid
> > > PHY ID and then the it's driver will be probed?
> > 
> > See:
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L895
> > 
> > No valid ID in C22 space.
> 
> I actually looked at the datasheet and yes, it implements the
> registers 13 and 14 in c22 to access the c45 space. I couldn't
> find any descriptions of other c22 registers though.

I'm not sure which PHY you're referring to here, but iirc, the later
hardware revisions of the 88x3310 implement the indirect access, but
earlier revisions do not.

> > In general, if the core can do something, it is better than the driver
> > doing it. If the core cannot reliably figure it out, then we have to
> > leave it to the drivers. It could well be we need the drivers to set
> > has_c45. I would prefer that drivers don't touch c45_over_c22 because
> > they don't have the knowledge of what the bus is capable of doing. The
> > only valid case i can think of is for a very oddball PHY which has C45
> > register space, but cannot actually do C45 transfers, and so C45 over
> > C22 is the only option.
> 
> And how would you know that the PHY has the needed registers in c22
> space? Or do we assume that every C45 PHY has these registers?

That's the problem. Currently C22 PHY drivers that do not support the
C45 register space have to set the .read_mmd and .write_mmd methods to
genphy_read_mmd_unsupported/genphy_write_mmd_unsupported which
effectively disables access to the C45 register space. In order for
that to happen, we must have read the C22 PHY ID and bound the driver.

That doesn't help with reading the PHY ID though.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

[Russell King (Oracle)]

On Thu, Mar 24, 2022 at 01:41:44AM +0100, Andrew Lunn wrote:
ydev->c45_over_c22 we are currently in a bad shape for. We cannot
> reliably say the bus master supports C45. If the bus capabilities say
> C22 only, we can set phydev->c45_over_c22. If the bus capabilities
> list C45, we can set it false. But that only covers a few busses, most
> don't have any capabilities set. We can try a C45 access and see if we
> get an -EOPNOTSUPP, in which case we can set phydev->c45_over_c22. But
> the bus driver could also do the wrong thing, issue a C22 transfer and
> give us back rubbish.

Unfortunately, trying a C45 access will be very hit and miss - we
need to fix all the MDIO drivers before we do that to check the
access type. Many don't, and worse, many assume a C22 formatted
access request, and just try throwing the PHY address and register
address into the register fields without any masking. The result is
that a C45 access will set random bits in the register.

For example:
drivers/net/mdio/mdio-bcm-iproc.c (no bus capability):

        /* Prepare the read operation */
        cmd = (MII_DATA_TA_VAL << MII_DATA_TA_SHIFT) |
                (reg << MII_DATA_RA_SHIFT) |
                (phy_id << MII_DATA_PA_SHIFT) |
                BIT(MII_DATA_SB_SHIFT) |
                (MII_DATA_OP_READ << MII_DATA_OP_SHIFT);

        writel(cmd, priv->base + MII_DATA_OFFSET);

Similar is true for:
drivers/net/mdio/mdio-bcm-unimac.c (no bus capability)
drivers/net/mdio/mdio-hisi-femac.c (no bus capability)
drivers/net/mdio/mdio-moxart.c (no bus capability)
drivers/net/mdio/mdio-mscc-miim.c (no bus capability)
drivers/net/mdio/mdio-mux-bcm6368.c (no bus capability)
drivers/net/mdio/mdio-mux-bcm-iproc.c (no bus capability)
drivers/net/mdio/mdio-sun4i.c (no bus capability)

These truncate the fields, and fwics they don't set the bus type:
drivers/net/mdio/mdio-xgene.c (for the "rgmii" only bus and no bus capability)

So all of the above need at the very least code added to reject a C45
"dev" or "phy_id" address, or they need to set the bus capability
correctly.

My feeling is that the introduction of the bus capability hasn't done
much to improve this situation; it was introduced to hint at whether a
bus is safe for clause 45 accesses, but it hasn't actually solved this
problem because we patently still have this same issue today.

I think we just need to bite the bullet and audit all the MDIO drivers
we currently have, checking what the results would be if they were
passed a C45 access request, and make them reject such a request if
the register address or phy address obviously overflows into different
register fields and also mark them as C22-only.

I can't see any other reasonable option.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 5/5] net: phylink: handle the new is_c45_over_c22 property

[Michael Walle]

Phylink treats C45 PHYs in a special way and assumes they can switch
their SerDes lanes between modes. This check is done by looking at the
is_c45 property. But there are PHYs, namely the GPY215, which are C45
PHYs but behind a C22 bus. Thus while the PHY is a C45 one, the is_c45
property is not set because it uses indirect MMD access via the C22
registers. Therefore, add the is_c45_over_c22  property to the check,
which indicates this sort of PHY handling.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/phylink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 06943889d747..633cccfbd5f4 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1369,7 +1369,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	 * speeds. We really need to know which interface modes the PHY and
 	 * MAC supports to properly work out which linkmodes can be supported.
 	 */
-	if (phy->is_c45 &&
+	if ((phy->is_c45 || phy->is_c45_over_c22) &&
 	    interface != PHY_INTERFACE_MODE_RXAUI &&
 	    interface != PHY_INTERFACE_MODE_XAUI &&
 	    interface != PHY_INTERFACE_MODE_USXGMII)
-- 
2.30.2

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

[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.

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

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

[Michael Walle]

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