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

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

[Michael Walle]

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.

Changes since RFC v2:
 - Reased to latest net-next
 - new check_rc argument in mmd_phy_indirect() to retain old behavior
 - determine bus capabilities by bus->read and bus->read_c45
 - always set phydev->c45_over_c22 if PHY is promoted

Changes since RFC v1:
 - use __phy_mmd_indirect() in mdiobus_probe_mmd_read()
 - add new properties has_c45 c45_over_c22 (and remove is_c45)
 - drop MDIOBUS_NO_CAP handling, Andrew is preparing a series to
   add probe_capabilities to mark all C45 capable MDIO bus drivers

Michael Walle (5):
  net: phy: add error checks in mmd_phy_indirect() and export it
  net: phy: support indirect c45 access in get_phy_c45_ids()
  net: phy: add support for C45-over-C22 transfers
  phy: net: introduce phy_promote_to_c45()
  net: phy: mxl-gpy: remove unneeded ops

 .../net/ethernet/hisilicon/hns/hns_ethtool.c  |  4 +-
 drivers/net/phy/bcm84881.c                    |  2 +-
 drivers/net/phy/marvell10g.c                  |  2 +-
 drivers/net/phy/mxl-gpy.c                     | 33 +-------
 drivers/net/phy/phy-core.c                    | 48 ++++++++---
 drivers/net/phy/phy.c                         |  6 +-
 drivers/net/phy/phy_device.c                  | 80 ++++++++++++++++---
 drivers/net/phy/phylink.c                     |  8 +-
 include/linux/phy.h                           | 12 ++-
 9 files changed, 128 insertions(+), 67 deletions(-)

-- 
2.30.2

[PATCH net-next 1/5] net: phy: add error checks in mmd_phy_indirect() and export it

[Michael Walle]

Add missing error checks in mmd_phy_indirect(). The error checks need to
be disabled to retain the current behavior in phy_read_mmd() and
phy_write_mmd(). Therefore, add a new parameter to enable the error
checks. Add a thin wrapper __phy_mmd_indirect() which is then exported.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/phy-core.c | 44 +++++++++++++++++++++++++++++++-------
 include/linux/phy.h        |  2 ++
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index a64186dc53f8..c9c92b95ace2 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -524,19 +524,47 @@ int phy_speed_down_core(struct phy_device *phydev)
 	return 0;
 }
 
-static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
-			     u16 regnum)
+static int mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
+			    u16 regnum, bool check_rc)
 {
+	int ret;
+
 	/* Write the desired MMD Devad */
-	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
+	ret = __mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
+	if (check_rc && ret)
+		return ret;
 
 	/* Write the desired MMD register address */
-	__mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
+	ret = __mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
+	if (check_rc && ret)
+		return ret;
 
 	/* Select the Function : DATA with no post increment */
-	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
-			devad | MII_MMD_CTRL_NOINCR);
+	ret = __mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
+			      devad | MII_MMD_CTRL_NOINCR);
+	if (check_rc && ret)
+		return ret;
+
+	return 0;
+}
+
+/**
+ * __phy_mmd_indirect - prepare an indirect C45 register access
+ *
+ * @bus: the target MII bus
+ * @phy_addr: PHY address on the MII bus
+ * @devad: The target MMD (0..31)
+ * @regnum: The target register on the MMD (0..65535)
+ *
+ * Prepare an indirect C45 read or write transfer using the MII_MMD_CTRL and
+ * MII_MMD_DATA registers in C22 space.
+ */
+int __phy_mmd_indirect(struct mii_bus *bus, int phy_addr, int devad,
+		       u16 regnum)
+{
+	return mmd_phy_indirect(bus, phy_addr, devad, regnum, true);
 }
+EXPORT_SYMBOL(__phy_mmd_indirect);
 
 /**
  * __phy_read_mmd - Convenience function for reading a register
@@ -563,7 +591,7 @@ int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
 		struct mii_bus *bus = phydev->mdio.bus;
 		int phy_addr = phydev->mdio.addr;
 
-		mmd_phy_indirect(bus, phy_addr, devad, regnum);
+		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
 
 		/* Read the content of the MMD's selected register */
 		val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
@@ -619,7 +647,7 @@ int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 		struct mii_bus *bus = phydev->mdio.bus;
 		int phy_addr = phydev->mdio.addr;
 
-		mmd_phy_indirect(bus, phy_addr, devad, regnum);
+		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
 
 		/* Write the data into MMD's selected register */
 		__mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fbeba4fee8d4..f7a5e110f95c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1228,6 +1228,8 @@ int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
 	__ret; \
 })
 
+int __phy_mmd_indirect(struct mii_bus *bus, int phy_addr, int devad,
+		       u16 regnum);
 /*
  * __phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
-- 
2.30.2

Re: [PATCH net-next 1/5] net: phy: add error checks in mmd_phy_indirect() and export it

[Andrew Lunn]

On Fri, Jan 20, 2023 at 11:40:07PM +0100, Michael Walle wrote:
> Add missing error checks in mmd_phy_indirect(). The error checks need to
> be disabled to retain the current behavior in phy_read_mmd() and
> phy_write_mmd(). Therefore, add a new parameter to enable the error
> checks. Add a thin wrapper __phy_mmd_indirect() which is then exported.

Do we need to retain the current behavior? Is there a good reason to
silently ignore errors? If there is, it would be good to state it here
in the commit message.

   Andrew

[PATCH 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 C45 accesses over the C22 registers.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9ba8f973f26f..9777a7fd180a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -698,6 +698,28 @@ 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;
+
+	if (bus->read_c45)
+		return mdiobus_c45_read(bus, prtad, devad, regnum);
+
+	mutex_lock(&bus->mdio_lock);
+
+	ret = __phy_mmd_indirect(bus, prtad, devad, regnum);
+	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
@@ -713,7 +735,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;
 
@@ -736,12 +758,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;
@@ -825,12 +847,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

[PATCH net-next 3/5] net: phy: add support for C45-over-C22 transfers

[Michael Walle]

If an MDIO bus is only capable of doing C22 transfers we can use
indirect accesses to C45 registers over C22 registers. This was already
the intention of the GPY215 driver. The author described their use case
as follows:

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

In its probe function phy_get_c45_ids() is called but this will always
do C45 accesses and thus will fail on a C22-only bus. With the current
code we only have the is_c45 property which is used to indicate a C45
PHY but also used to choose the transfer mode. With C45-over-C22 we need
to split these two properties.

Drop the is_c45 and instead introduce two new properties, has_c45 and
c45_over_c22. has_c45 is set to true if this is a C45 PHY and
c45_over_c22 is true if we need to do indirect accesses using the C22
registers.

c45_over_c22 will always be set by just looking at the bus capabilities.
It will be set to true if a bus is C22-only, regardless if the PHY would
support indirect access. Firstly, it is a reasonable assumption that C45
PHYs will support this access and secondly, there is really not much we
can do otherwise.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../net/ethernet/hisilicon/hns/hns_ethtool.c  |  4 +--
 drivers/net/phy/bcm84881.c                    |  2 +-
 drivers/net/phy/marvell10g.c                  |  2 +-
 drivers/net/phy/mxl-gpy.c                     |  2 +-
 drivers/net/phy/phy-core.c                    |  4 +--
 drivers/net/phy/phy.c                         |  6 ++---
 drivers/net/phy/phy_device.c                  | 25 ++++++++++++++++---
 drivers/net/phy/phylink.c                     |  8 +++---
 include/linux/phy.h                           |  8 +++---
 9 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index b54f3706fb97..e34e1510e6a6 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -916,7 +916,7 @@ static void hns_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 					hns_nic_test_strs[MAC_INTERNALLOOP_MAC]);
 		ethtool_sprintf(&buff,
 				hns_nic_test_strs[MAC_INTERNALLOOP_SERDES]);
-		if ((netdev->phydev) && (!netdev->phydev->is_c45))
+		if (netdev->phydev && !netdev->phydev->has_c45)
 			ethtool_sprintf(&buff,
 					hns_nic_test_strs[MAC_INTERNALLOOP_PHY]);
 
@@ -976,7 +976,7 @@ static int hns_get_sset_count(struct net_device *netdev, int stringset)
 		if (priv->ae_handle->phy_if == PHY_INTERFACE_MODE_XGMII)
 			cnt--;
 
-		if ((!netdev->phydev) || (netdev->phydev->is_c45))
+		if (!netdev->phydev || netdev->phydev->has_c45)
 			cnt--;
 
 		return cnt;
diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 9717a1626f3f..d9131d5284c1 100644
--- a/drivers/net/phy/bcm84881.c
+++ b/drivers/net/phy/bcm84881.c
@@ -47,7 +47,7 @@ static int bcm84881_probe(struct phy_device *phydev)
 	/* This driver requires PMAPMD and AN blocks */
 	const u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
 
-	if (!phydev->is_c45 ||
+	if (!phydev->has_c45 ||
 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
 		return -ENODEV;
 
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 383a9c9f36e5..27a52c11ad75 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -499,7 +499,7 @@ static int mv3310_probe(struct phy_device *phydev)
 	u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
 	int ret;
 
-	if (!phydev->is_c45 ||
+	if (!phydev->has_c45 ||
 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
 		return -ENODEV;
 
diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index e5972b4ef6e8..e86aea4381c2 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -281,7 +281,7 @@ static int gpy_probe(struct phy_device *phydev)
 	int fw_version;
 	int ret;
 
-	if (!phydev->is_c45) {
+	if (!phydev->has_c45) {
 		ret = phy_get_c45_ids(phydev);
 		if (ret < 0)
 			return ret;
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index c9c92b95ace2..5e4a54f0a3dc 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -584,7 +584,7 @@ int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
 
 	if (phydev->drv && phydev->drv->read_mmd) {
 		val = phydev->drv->read_mmd(phydev, devad, regnum);
-	} else if (phydev->is_c45) {
+	} else if (phydev->has_c45 && !phydev->c45_over_c22) {
 		val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
 					 devad, regnum);
 	} else {
@@ -640,7 +640,7 @@ int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 
 	if (phydev->drv && phydev->drv->write_mmd) {
 		ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
-	} else if (phydev->is_c45) {
+	} else if (phydev->has_c45 && !phydev->c45_over_c22) {
 		ret = __mdiobus_c45_write(phydev->mdio.bus, phydev->mdio.addr,
 					  devad, regnum, val);
 	} else {
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3378ca4f49b6..121aaae6b2f8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -169,7 +169,7 @@ int phy_restart_aneg(struct phy_device *phydev)
 {
 	int ret;
 
-	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
+	if (phydev->has_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
 		ret = genphy_c45_restart_aneg(phydev);
 	else
 		ret = genphy_restart_aneg(phydev);
@@ -190,7 +190,7 @@ int phy_aneg_done(struct phy_device *phydev)
 {
 	if (phydev->drv && phydev->drv->aneg_done)
 		return phydev->drv->aneg_done(phydev);
-	else if (phydev->is_c45)
+	else if (phydev->has_c45)
 		return genphy_c45_aneg_done(phydev);
 	else
 		return genphy_aneg_done(phydev);
@@ -883,7 +883,7 @@ int phy_config_aneg(struct phy_device *phydev)
 	/* Clause 45 PHYs that don't implement Clause 22 registers are not
 	 * allowed to call genphy_config_aneg()
 	 */
-	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
+	if (phydev->has_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
 		return genphy_c45_config_aneg(phydev);
 
 	return genphy_config_aneg(phydev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9777a7fd180a..d5ea034cde98 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -516,7 +516,7 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
 	if (phydrv->match_phy_device)
 		return phydrv->match_phy_device(phydev);
 
-	if (phydev->is_c45) {
+	if (phydev->has_c45) {
 		for (i = 1; i < num_ids; i++) {
 			if (phydev->c45_ids.device_ids[i] == 0xffffffff)
 				continue;
@@ -648,7 +648,24 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	dev->autoneg = AUTONEG_ENABLE;
 
 	dev->pma_extable = -ENODATA;
-	dev->is_c45 = is_c45;
+
+	/* Depending on the bus capabilities, we have to use C45-over-C22
+	 * register access. We have the following cases:
+	 *
+	 * 1) bus can only do C45.
+	 * 2) bus can only do C22.
+	 * 3) bus can do C22 and C45.
+	 *
+	 * 1) and 3) are easy, because we can just use C45 transfers. For 2) we
+	 * don't have any other choice but to use C22 transfers. Even if the
+	 * PHY wouldn't support it we cannot do any better.
+	 *
+	 * Set this for C22 PHYs, too, because the PHY driver might promote it
+	 * to C45.
+	 */
+	dev->c45_over_c22 = bus->read && !bus->read_c45;
+
+	dev->has_c45 = is_c45;
 	dev->phy_id = phy_id;
 	if (c45_ids)
 		dev->c45_ids = *c45_ids;
@@ -1456,7 +1473,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	 * exist, and we should use the genphy driver.
 	 */
 	if (!d->driver) {
-		if (phydev->is_c45)
+		if (phydev->has_c45)
 			d->driver = &genphy_c45_driver.mdiodrv.driver;
 		else
 			d->driver = &genphy_driver.mdiodrv.driver;
@@ -3115,7 +3132,7 @@ static int phy_probe(struct device *dev)
 		linkmode_copy(phydev->supported, phydrv->features);
 	else if (phydrv->get_features)
 		err = phydrv->get_features(phydev);
-	else if (phydev->is_c45)
+	else if (phydev->has_c45)
 		err = genphy_c45_pma_read_abilities(phydev);
 	else
 		err = genphy_read_abilities(phydev);
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 319790221d7f..63bec523a211 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1642,7 +1642,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 	 * against all interface modes, which may lead to more ethtool link
 	 * modes being advertised than are actually supported.
 	 */
-	if (phy->is_c45 && config.rate_matching == RATE_MATCH_NONE &&
+	if (phy->has_c45 && config.rate_matching == RATE_MATCH_NONE &&
 	    interface != PHY_INTERFACE_MODE_RXAUI &&
 	    interface != PHY_INTERFACE_MODE_XAUI &&
 	    interface != PHY_INTERFACE_MODE_USXGMII)
@@ -2584,7 +2584,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
 					reg);
 	}
 
-	if (phydev->is_c45) {
+	if (phydev->has_c45 && !phydev->c45_over_c22) {
 		switch (reg) {
 		case MII_BMCR:
 		case MII_BMSR:
@@ -2626,7 +2626,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
 					 reg, val);
 	}
 
-	if (phydev->is_c45) {
+	if (phydev->has_c45 && !phydev->c45_over_c22) {
 		switch (reg) {
 		case MII_BMCR:
 		case MII_BMSR:
@@ -3101,7 +3101,7 @@ static void phylink_sfp_link_up(void *upstream)
  */
 static bool phylink_phy_no_inband(struct phy_device *phy)
 {
-	return phy->is_c45 &&
+	return phy->has_c45 &&
 		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
 }
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f7a5e110f95c..4c02c468a24b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -532,8 +532,9 @@ struct macsec_ops;
  * @devlink: Create a link between phy dev and mac dev, if the external phy
  *           used by current mac interface is managed by another mac interface.
  * @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.
+ * @c45_ids: 802.3-c45 Device Identifiers if has_c45.
+ * @has_c45:  Set to true if this PHY has clause 45 address space.
+ * @c45_over_c22:  Set to true if c45-over-c22 addressing is used.
  * @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
@@ -625,7 +626,8 @@ struct phy_device {
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;
-	unsigned is_c45:1;
+	unsigned has_c45:1;
+	unsigned c45_over_c22:1;
 	unsigned is_internal:1;
 	unsigned is_pseudo_fixed_link:1;
 	unsigned is_gigabit_capable:1;
-- 
2.30.2

[PATCH net-next 4/5] phy: net: introduce phy_promote_to_c45()

[Michael Walle]

If not explitly asked to be probed as a C45 PHY, on a bus which is
capable of doing both C22 and C45 transfers, C45 PHYs are first tried to
be probed as C22 PHYs. To be able to promote the PHY to be a C45 one,
the driver can call phy_promote_to_c45() in its probe function.

This was already done in the mxl-gpy driver by the following snippet:

   if (!phydev->has_c45) {
           ret = phy_get_c45_ids(phydev);
           if (ret < 0)
                   return ret;
   }

Move that code into the core by creating a new function
phy_promote_to_c45(). If a PHY is promoted, C45-over-C22 access is used,
regardless if the bus supports C45 or not. That is because there might
be C22 PHYs on the bus which gets confused by C45 accesses.

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

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index e86aea4381c2..4dc1093dbdc1 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -281,11 +281,10 @@ static int gpy_probe(struct phy_device *phydev)
 	int fw_version;
 	int ret;
 
-	if (!phydev->has_c45) {
-		ret = phy_get_c45_ids(phydev);
-		if (ret < 0)
-			return ret;
-	}
+	/* This might have been probed as a C22 PHY, but this is a C45 PHY */
+	ret = phy_promote_to_c45(phydev);
+	if (ret)
+		return ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d5ea034cde98..ae24b62abfac 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1058,18 +1058,33 @@ 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.
- * @phydev: phy_device structure to read 802.3-c45 IDs
+ * phy_promote_to_c45 - Promote to a C45 PHY
+ * @phydev: phy_device structure
+ *
+ * If a PHY supports both C22 and C45 and it isn't specifically asked to probe
+ * as a C45 PHY it might be probed as a C22 PHY. The driver can call this
+ * function to promote a PHY from C22 to C45.
+ *
+ * Can also be called if a PHY is already a C45 one. In that case it does
+ * nothing.
+ *
+ * Promoted PHYs will always use C45-over-C22 accesses to prevent any C45
+ * transactions on the bus, which might confuse C22-only PHYs.
  *
  * 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_promote_to_c45(struct phy_device *phydev)
 {
+	if (phydev->has_c45)
+		return 0;
+
+	phydev->has_c45 = true;
+	phydev->c45_over_c22 = true;
 	return get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
 			       &phydev->c45_ids);
 }
-EXPORT_SYMBOL(phy_get_c45_ids);
+EXPORT_SYMBOL(phy_promote_to_c45);
 
 /**
  * phy_find_first - finds the first PHY device on the bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4c02c468a24b..9473e2ed8496 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1588,7 +1588,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_promote_to_c45(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 net-next 4/5] phy: net: introduce phy_promote_to_c45()

[Russell King (Oracle)]

On Fri, Jan 20, 2023 at 11:40:10PM +0100, Michael Walle wrote:
> If not explitly asked to be probed as a C45 PHY, on a bus which is
> capable of doing both C22 and C45 transfers, C45 PHYs are first tried to
> be probed as C22 PHYs. To be able to promote the PHY to be a C45 one,
> the driver can call phy_promote_to_c45() in its probe function.
> 
> This was already done in the mxl-gpy driver by the following snippet:
> 
>    if (!phydev->has_c45) {
>            ret = phy_get_c45_ids(phydev);
>            if (ret < 0)
>                    return ret;
>    }
> 
> Move that code into the core by creating a new function
> phy_promote_to_c45(). If a PHY is promoted, C45-over-C22 access is used,
> regardless if the bus supports C45 or not. That is because there might
> be C22 PHYs on the bus which gets confused by C45 accesses.

It is my understanding that C45 PHYs do not have to respond to C22
accesses. So, wouldn't this lead to some C45 PHYs not being detected?

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

Re: [PATCH net-next 4/5] phy: net: introduce phy_promote_to_c45()

[Michael Walle]

Am 2023-01-21 00:20, schrieb Russell King (Oracle):
> On Fri, Jan 20, 2023 at 11:40:10PM +0100, Michael Walle wrote:
>> If not explitly asked to be probed as a C45 PHY, on a bus which is
>> capable of doing both C22 and C45 transfers, C45 PHYs are first tried 
>> to
>> be probed as C22 PHYs. To be able to promote the PHY to be a C45 one,
>> the driver can call phy_promote_to_c45() in its probe function.
>> 
>> This was already done in the mxl-gpy driver by the following snippet:
>> 
>>    if (!phydev->has_c45) {
>>            ret = phy_get_c45_ids(phydev);
>>            if (ret < 0)
>>                    return ret;
>>    }
>> 
>> Move that code into the core by creating a new function
>> phy_promote_to_c45(). If a PHY is promoted, C45-over-C22 access is 
>> used,
>> regardless if the bus supports C45 or not. That is because there might
>> be C22 PHYs on the bus which gets confused by C45 accesses.
> 
> It is my understanding that C45 PHYs do not have to respond to C22
> accesses. So, wouldn't this lead to some C45 PHYs not being detected?

In that case, the PHY already has to be a C45 PHY, correct? Because
no access to c22 means, it can't be probed as a c22 phy; therefore,
it has the has_c45 set to true. Then phy_promote_to_c45() is a noop and
c45-over-c22 wont be set.

-michael

[PATCH net-next 5/5] net: phy: mxl-gpy: remove unneeded ops

[Michael Walle]

Now that we have proper c45-over-c22 support and the PHY driver promote
the PHY to a C45 device, we can drop the ops because the core will
already call them.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/mxl-gpy.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 4dc1093dbdc1..043c084a8a16 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -799,13 +799,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY2xx),
 		.name		= "Maxlinear Ethernet GPY2xx",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -817,13 +815,11 @@ static struct phy_driver gpy_drivers[] = {
 		.phy_id		= PHY_ID_GPY115B,
 		.phy_id_mask	= PHY_ID_GPYx15B_MASK,
 		.name		= "Maxlinear Ethernet GPY115B",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -834,13 +830,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY115C),
 		.name		= "Maxlinear Ethernet GPY115C",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -852,13 +846,11 @@ static struct phy_driver gpy_drivers[] = {
 		.phy_id		= PHY_ID_GPY211B,
 		.phy_id_mask	= PHY_ID_GPY21xB_MASK,
 		.name		= "Maxlinear Ethernet GPY211B",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -869,13 +861,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY211C),
 		.name		= "Maxlinear Ethernet GPY211C",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -887,13 +877,11 @@ static struct phy_driver gpy_drivers[] = {
 		.phy_id		= PHY_ID_GPY212B,
 		.phy_id_mask	= PHY_ID_GPY21xB_MASK,
 		.name		= "Maxlinear Ethernet GPY212B",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -904,13 +892,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY212C),
 		.name		= "Maxlinear Ethernet GPY212C",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -922,13 +908,11 @@ static struct phy_driver gpy_drivers[] = {
 		.phy_id		= PHY_ID_GPY215B,
 		.phy_id_mask	= PHY_ID_GPYx15B_MASK,
 		.name		= "Maxlinear Ethernet GPY215B",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -939,13 +923,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY215C),
 		.name		= "Maxlinear Ethernet GPY215C",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -956,13 +938,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY241B),
 		.name		= "Maxlinear Ethernet GPY241B",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -973,13 +953,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY241BM),
 		.name		= "Maxlinear Ethernet GPY241BM",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
@@ -990,13 +968,11 @@ static struct phy_driver gpy_drivers[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_GPY245B),
 		.name		= "Maxlinear Ethernet GPY245B",
-		.get_features	= genphy_c45_pma_read_abilities,
 		.config_init	= gpy_config_init,
 		.probe		= gpy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.config_aneg	= gpy_config_aneg,
-		.aneg_done	= genphy_c45_aneg_done,
 		.read_status	= gpy_read_status,
 		.config_intr	= gpy_config_intr,
 		.handle_interrupt = gpy_handle_interrupt,
-- 
2.30.2

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

[Andrew Lunn]

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

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

[Russell King (Oracle)]

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.

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

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

[Andrew Lunn]

> 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

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

[Michael Walle]

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

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

[Andrew Lunn]

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

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

[Michael Walle]

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

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

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

	Andrew

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

[Michael Walle]

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

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

[Andrew Lunn]

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