All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v3 00/11] net: phy: C45-over-C22 access
@ 2023-07-12 15:07 Michael Walle
  2023-07-12 15:07 ` [PATCH net-next v3 01/11] net: phy: get rid of redundant is_c45 information Michael Walle
                   ` (10 more replies)
  0 siblings, 11 replies; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

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 in the OF case. We must not use C45-over-C22 if the device being
probed isn't a PHY because it involes register writes to standard PHY
registers, which might not be supported in generic PHY devices.
Therefore, we cannot use it in the generic PHY probing code. In the DT
case, we know we are probing a PHY device and thus we can fall back to
C45-over-C22.

The first patches convert the is_c45 property to a new "enum
phy_access_mode". The former property is then derived from the
access mode.

To support the probing for DT, export the prevent_c45_scan logic and
make it a property of the mii bus. Unfortunately, you'd need to stick to
the following flow in every scanning code:
 (1) add any c22 phys
 (2) scan for broken phys
 (3) add any c45 phys using either c45 or c45-over-c22 access

I couldn't find a way to make that generic and move the flow into the
phy core.

Then, a new access method c45-over-c22 is added for get_phy_device() and
phy_{read,write}_mmd(). It is the callers responsibilty to choose the
correct mode. Esp. the generic probing code isn't using c45-over-c22.

c45-over-c22 is then added for the MaxLinear PHYs if it is probed as a
C22 device, which is always the case if there is no compatible =
"ethernet-phy-ieee802.3-c45" in the device tree. The driver will
automatically "promote" the PHY to a C45 one if probed as C22.

The last two patches will then add c45-over-c22 fallback to the DT PHY
registration code. As described above, the probing is split into the
three phases.

FWIW, this now triggers a bug in mscc-miim. Haven't figured out what's
wrong yet, somehow the status register doesn't return busy/pending.
Currently, working around that with a sleep.

net: phy: add error checks in mmd_phy_indirect() and export it
net: phy: get rid of redundant is_c45 information
net: phy: introduce phy_is_c45()
net: phy: replace is_c45 with phy_accces_mode
net: phy: make the "prevent_c45_scan" a property of the MII bus
net: phy: print an info if a broken C45 bus is found
net: phy: add support for C45-over-C22 transfers
net: phy: introduce phy_promote_to_c45()
net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy()
net: mdio: scan for broken C22 PHYs when probed via OF

Changes since v2:
 - addressed the review comments, see individual patches
 - more importantly, introduce a new helper phy_mdiobus_read_mmd() which
   takes an enum access_mode.
 - Link: https://lore.kernel.org/r/20230620-feature-c45-over-c22-v2-0-def0ab9ccee2@kernel.org

Changes since v1:
 - major rework to address the problem to not use c45-over-c22 on
   bus scanning, see the description above. Except from some preparation
   patches there is little left of the original series. The major
   difference is that there is now a new transport mode argument and the
   c45-over-c22 is not automatically used anymore.
 - Link: https://lore.kernel.org/netdev/20230120224011.796097-1-michael@walle.cc/

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
 - Link: https://lore.kernel.org/netdev/20220325213518.2668832-1-michael@walle.cc/

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
 - Link: https://lore.kernel.org/netdev/20220323183419.2278676-1-michael@walle.cc/

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
Michael Walle (11):
      net: phy: get rid of redundant is_c45 information
      net: phy: introduce phy_has_c45_registers()
      net: phy: replace is_c45 with phy_accces_mode
      net: phy: make the "prevent_c45_scan" a property of the MII bus
      net: phy: print an info if a broken C45 bus is found
      net: phy: add error checks in mmd_phy_indirect()
      net: phy: introduce phy_mdiobus_read_mmd()
      net: phy: add support for C45-over-C22 transfers
      net: phy: introduce phy_promote_to_c45()
      net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy()
      net: mdio: support C45-over-C22 when probed via OF

 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c |   8 +-
 drivers/net/ethernet/hisilicon/hns/hns_ethtool.c  |   4 +-
 drivers/net/mdio/fwnode_mdio.c                    |  19 ++-
 drivers/net/mdio/of_mdio.c                        |  63 +++++++---
 drivers/net/phy/bcm84881.c                        |   2 +-
 drivers/net/phy/marvell10g.c                      |   2 +-
 drivers/net/phy/mdio_bus.c                        |  34 ++---
 drivers/net/phy/mxl-gpy.c                         |   9 +-
 drivers/net/phy/nxp-tja11xx.c                     |   3 +-
 drivers/net/phy/phy-core.c                        | 144 ++++++++++++++++------
 drivers/net/phy/phy.c                             |   8 +-
 drivers/net/phy/phy_device.c                      | 107 +++++++++++-----
 drivers/net/phy/phylink.c                         |  12 +-
 drivers/net/phy/sfp.c                             |  12 +-
 include/linux/phy.h                               |  48 ++++++--
 15 files changed, 335 insertions(+), 140 deletions(-)
---
base-commit: ed2f1f76bed5a7ace3c4fa90bce3457d0fe2035d
change-id: 20230620-feature-c45-over-c22-0a2181babd56


^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 01/11] net: phy: get rid of redundant is_c45 information
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-18 17:25   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers() Michael Walle
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

phy_device_create() will be called with is_c45 and c45_ids. If c45_ids
are set, is_c45 is (usually) true. Change the only caller which do
things differently, then drop the is_c45 check in phy_device_create().

This is a preparation patch to replace the is_c45 boolean with an enum
which will indicate how the PHY is accessed (by c22, c45 or
c45-over-c22).

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/net/phy/phy_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0c2014accba7..226d5507c865 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -689,7 +689,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	 * driver will get bored and give up as soon as it finds that
 	 * there's no driver _already_ loaded.
 	 */
-	if (is_c45 && c45_ids) {
+	if (c45_ids) {
 		const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
 		int i;
 
@@ -970,7 +970,8 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 						 true, &c45_ids);
 	}
 
-	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
+	return phy_device_create(bus, addr, phy_id, is_c45,
+				 !is_c45 ? NULL : &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);
 

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
  2023-07-12 15:07 ` [PATCH net-next v3 01/11] net: phy: get rid of redundant is_c45 information Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-18 17:26   ` Andrew Lunn
  2023-07-18 20:07   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode Michael Walle
                   ` (8 subsequent siblings)
  10 siblings, 2 replies; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

Provide a helper to determine if the PHY has a C45 register space. This
is a preparation patch to remove the is_c45 field. No functional change.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
 - rename to phy_has_c45_registers()
---
 drivers/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                            |  8 +++++---
 drivers/net/phy/phy_device.c                     |  6 +++---
 drivers/net/phy/phylink.c                        | 12 +++++++-----
 include/linux/phy.h                              |  5 +++++
 9 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index b54f3706fb97..387d24f195aa 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 && !phy_has_c45_registers(netdev->phydev))
 			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 || phy_has_c45_registers(netdev->phydev))
 			cnt--;
 
 		return cnt;
diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 9717a1626f3f..857344260230 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 (!phy_has_c45_registers(phydev) ||
 	    (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 55d9d7acc32e..d1c12843462f 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 (!phy_has_c45_registers(phydev) ||
 	    (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 ea1073adc5a1..b4ddb9a003d9 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 (!phy_has_c45_registers(phydev)) {
 		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 a64186dc53f8..686a57d56885 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -556,7 +556,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 (phy_has_c45_registers(phydev)) {
 		val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
 					 devad, regnum);
 	} else {
@@ -612,7 +612,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 (phy_has_c45_registers(phydev)) {
 		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 bdf00b2b2c1d..c6059ba8b9e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -182,7 +182,8 @@ int phy_restart_aneg(struct phy_device *phydev)
 {
 	int ret;
 
-	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
+	if (phy_has_c45_registers(phydev) &&
+	    !(phydev->c45_ids.devices_in_package & BIT(0)))
 		ret = genphy_c45_restart_aneg(phydev);
 	else
 		ret = genphy_restart_aneg(phydev);
@@ -203,7 +204,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 (phy_has_c45_registers(phydev))
 		return genphy_c45_aneg_done(phydev);
 	else
 		return genphy_aneg_done(phydev);
@@ -896,7 +897,8 @@ 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 (phy_has_c45_registers(phydev) &&
+	    !(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 226d5507c865..44968ea447fc 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -533,7 +533,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 (phy_has_c45_registers(phydev)) {
 		for (i = 1; i < num_ids; i++) {
 			if (phydev->c45_ids.device_ids[i] == 0xffffffff)
 				continue;
@@ -1452,7 +1452,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 (phy_has_c45_registers(phydev))
 			d->driver = &genphy_c45_driver.mdiodrv.driver;
 		else
 			d->driver = &genphy_driver.mdiodrv.driver;
@@ -3227,7 +3227,7 @@ static int phy_probe(struct device *dev)
 	}
 	else if (phydrv->get_features)
 		err = phydrv->get_features(phydev);
-	else if (phydev->is_c45)
+	else if (phy_has_c45_registers(phydev))
 		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 d0aaa5cad853..54fde8252079 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1726,7 +1726,8 @@ 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_registers(phy) &&
+	    config.rate_matching == RATE_MATCH_NONE &&
 	    interface != PHY_INTERFACE_MODE_RXAUI &&
 	    interface != PHY_INTERFACE_MODE_XAUI &&
 	    interface != PHY_INTERFACE_MODE_USXGMII)
@@ -2673,7 +2674,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
 					reg);
 	}
 
-	if (phydev->is_c45) {
+	if (phy_has_c45_registers(phydev)) {
 		switch (reg) {
 		case MII_BMCR:
 		case MII_BMSR:
@@ -2715,7 +2716,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
 					 reg, val);
 	}
 
-	if (phydev->is_c45) {
+	if (phy_has_c45_registers(phydev)) {
 		switch (reg) {
 		case MII_BMCR:
 		case MII_BMSR:
@@ -3188,8 +3189,9 @@ static void phylink_sfp_link_up(void *upstream)
  */
 static bool phylink_phy_no_inband(struct phy_device *phy)
 {
-	return phy->is_c45 && phy_id_compare(phy->c45_ids.device_ids[1],
-					     0xae025150, 0xfffffff0);
+	return phy_has_c45_registers(phy) &&
+	       phy_id_compare(phy->c45_ids.device_ids[1],
+			      0xae025150, 0xfffffff0);
 }
 
 static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 11c1e91563d4..fdb3774e99fc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -766,6 +766,11 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
 	return container_of(to_mdio_device(dev), struct phy_device, mdio);
 }
 
+static inline bool phy_has_c45_registers(struct phy_device *phydev)
+{
+	return phydev->is_c45;
+}
+
 /**
  * struct phy_tdr_config - Configuration of a TDR raw test
  *

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
  2023-07-12 15:07 ` [PATCH net-next v3 01/11] net: phy: get rid of redundant is_c45 information Michael Walle
  2023-07-12 15:07 ` [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers() Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-18 17:40   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 04/11] net: phy: make the "prevent_c45_scan" a property of the MII bus Michael Walle
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

Instead of tracing whether the PHY is a C45 one, use the method how the
PHY is accessed. For now, that is either by C22 or by C45 transactions.
There is no functional change, just a semantical difference.

This is a preparation patch to add a third access method C45-over-C22.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
 - use 22 and 45 as constants for the enum to catch errors
 - move enum in struct next to enum phy_state
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c |  8 +++----
 drivers/net/mdio/fwnode_mdio.c                    |  6 +++--
 drivers/net/phy/mdio_bus.c                        |  9 +++----
 drivers/net/phy/nxp-tja11xx.c                     |  3 ++-
 drivers/net/phy/phy_device.c                      | 29 ++++++++++++++---------
 drivers/net/phy/sfp.c                             | 12 ++++++----
 include/linux/phy.h                               | 27 +++++++++++++++------
 7 files changed, 60 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
index 928d934cb21a..74cd6197735b 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -687,9 +687,9 @@ static int
 hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
 			u32 addr)
 {
+	enum phy_access_mode mode;
 	struct phy_device *phy;
 	const char *phy_type;
-	bool is_c45;
 	int rc;
 
 	rc = fwnode_property_read_string(mac_cb->fw_port,
@@ -698,13 +698,13 @@ hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
 		return rc;
 
 	if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_XGMII)))
-		is_c45 = true;
+		mode = PHY_ACCESS_C45;
 	else if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_SGMII)))
-		is_c45 = false;
+		mode = PHY_ACCESS_C22;
 	else
 		return -ENODATA;
 
-	phy = get_phy_device(mdio, addr, is_c45);
+	phy = get_phy_device(mdio, addr, mode);
 	if (!phy || IS_ERR(phy))
 		return -EIO;
 
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..972c8932c2fe 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -131,9 +131,11 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 
 	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
 	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
-		phy = get_phy_device(bus, addr, is_c45);
+		phy = get_phy_device(bus, addr,
+				     is_c45 ? PHY_ACCESS_C45 : PHY_ACCESS_C22);
 	else
-		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+		phy = phy_device_create(bus, addr, phy_id, PHY_ACCESS_C22,
+					NULL);
 	if (IS_ERR(phy)) {
 		rc = PTR_ERR(phy);
 		goto clean_mii_ts;
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 8b3618d3da4a..a31eb1204f63 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -513,12 +513,13 @@ static int mdiobus_create_device(struct mii_bus *bus,
 	return ret;
 }
 
-static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
+static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr,
+				       enum phy_access_mode mode)
 {
 	struct phy_device *phydev = ERR_PTR(-ENODEV);
 	int err;
 
-	phydev = get_phy_device(bus, addr, c45);
+	phydev = get_phy_device(bus, addr, mode);
 	if (IS_ERR(phydev))
 		return phydev;
 
@@ -550,7 +551,7 @@ static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
  */
 struct phy_device *mdiobus_scan_c22(struct mii_bus *bus, int addr)
 {
-	return mdiobus_scan(bus, addr, false);
+	return mdiobus_scan(bus, addr, PHY_ACCESS_C22);
 }
 EXPORT_SYMBOL(mdiobus_scan_c22);
 
@@ -568,7 +569,7 @@ EXPORT_SYMBOL(mdiobus_scan_c22);
  */
 static struct phy_device *mdiobus_scan_c45(struct mii_bus *bus, int addr)
 {
-	return mdiobus_scan(bus, addr, true);
+	return mdiobus_scan(bus, addr, PHY_ACCESS_C45);
 }
 
 static int mdiobus_scan_bus_c22(struct mii_bus *bus)
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index b13e15310feb..1c6c1523540e 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -580,7 +580,8 @@ static void tja1102_p1_register(struct work_struct *work)
 		}
 
 		/* Real PHY ID of Port 1 is 0 */
-		phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false, NULL);
+		phy = phy_device_create(bus, addr, PHY_ID_TJA1102,
+					PHY_ACCESS_C22, NULL);
 		if (IS_ERR(phy)) {
 			dev_err(dev, "Can't create PHY device for Port 1: %i\n",
 				addr);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 44968ea447fc..6254212d65a5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -630,7 +630,7 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
 }
 
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
-				     bool is_c45,
+				     enum phy_access_mode mode,
 				     struct phy_c45_device_ids *c45_ids)
 {
 	struct phy_device *dev;
@@ -664,7 +664,7 @@ 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;
+	dev->access_mode = mode;
 	dev->phy_id = phy_id;
 	if (c45_ids)
 		dev->c45_ids = *c45_ids;
@@ -926,7 +926,7 @@ EXPORT_SYMBOL(fwnode_get_phy_id);
  *		    struct
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
- * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
+ * @mode: Access mode of the PHY
  *
  * Probe for a PHY at @addr on @bus.
  *
@@ -940,7 +940,8 @@ EXPORT_SYMBOL(fwnode_get_phy_id);
  * Returns an allocated &struct phy_device on success, %-ENODEV if there is
  * no PHY present, or %-EIO on bus access error.
  */
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
+struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
+				  enum phy_access_mode mode)
 {
 	struct phy_c45_device_ids c45_ids;
 	u32 phy_id = 0;
@@ -950,10 +951,16 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	c45_ids.mmds_present = 0;
 	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
 
-	if (is_c45)
-		r = get_phy_c45_ids(bus, addr, &c45_ids);
-	else
+	switch (mode) {
+	case PHY_ACCESS_C22:
 		r = get_phy_c22_id(bus, addr, &phy_id);
+		break;
+	case PHY_ACCESS_C45:
+		r = get_phy_c45_ids(bus, addr, &c45_ids);
+		break;
+	default:
+		return ERR_PTR(-EIO);
+	}
 
 	if (r)
 		return ERR_PTR(r);
@@ -963,15 +970,15 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	 * probe with C45 to see if we're able to get a valid PHY ID in the C45
 	 * space, if successful, create the C45 PHY device.
 	 */
-	if (!is_c45 && phy_id == 0 && bus->read_c45) {
+	if (mode == PHY_ACCESS_C22 && phy_id == 0 && bus->read_c45) {
 		r = get_phy_c45_ids(bus, addr, &c45_ids);
 		if (!r)
 			return phy_device_create(bus, addr, phy_id,
-						 true, &c45_ids);
+						 PHY_ACCESS_C45, &c45_ids);
 	}
 
-	return phy_device_create(bus, addr, phy_id, is_c45,
-				 !is_c45 ? NULL : &c45_ids);
+	return phy_device_create(bus, addr, phy_id, mode,
+				 mode == PHY_ACCESS_C22 ? NULL : &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);
 
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index d855a18308d7..e7f8decaf3ff 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1750,12 +1750,13 @@ static void sfp_sm_phy_detach(struct sfp *sfp)
 	sfp->mod_phy = NULL;
 }
 
-static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
+static int sfp_sm_probe_phy(struct sfp *sfp, int addr,
+			    enum phy_access_mode mode)
 {
 	struct phy_device *phy;
 	int err;
 
-	phy = get_phy_device(sfp->i2c_mii, addr, is_c45);
+	phy = get_phy_device(sfp->i2c_mii, addr, mode);
 	if (phy == ERR_PTR(-ENODEV))
 		return PTR_ERR(phy);
 	if (IS_ERR(phy)) {
@@ -1879,15 +1880,16 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)
 		break;
 
 	case MDIO_I2C_MARVELL_C22:
-		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, false);
+		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, PHY_ACCESS_C22);
 		break;
 
 	case MDIO_I2C_C45:
-		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, true);
+		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR, PHY_ACCESS_C45);
 		break;
 
 	case MDIO_I2C_ROLLBALL:
-		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL, true);
+		err = sfp_sm_probe_phy(sfp, SFP_PHY_ADDR_ROLLBALL,
+				       PHY_ACCESS_C45);
 		break;
 	}
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fdb3774e99fc..fb7481715c3b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -531,6 +531,17 @@ struct phy_c45_device_ids {
 struct macsec_context;
 struct macsec_ops;
 
+/**
+ * enum phy_access_mode - PHY register access mode definitions
+ *
+ * @PHY_ACCESS_C22: use 802.3 c22 MDIO transactions
+ * @PHY_ACCESS_C45: use 802.3 c45 MDIO transactions
+ */
+enum phy_access_mode {
+	PHY_ACCESS_C22 = 22,
+	PHY_ACCESS_C45 = 45,
+};
+
 /**
  * struct phy_device - An instance of a PHY
  *
@@ -539,8 +550,7 @@ 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 it's an C45 PHY.
  * @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
@@ -555,6 +565,7 @@ struct macsec_ops;
  * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
  * 		 enabled.
  * @state: State of the PHY for management purposes
+ * @access_mode:  MDIO access mode of the PHY.
  * @dev_flags: Device-specific flags used by the PHY driver.
  *
  *      - Bits [15:0] are free to use by the PHY driver to communicate
@@ -638,7 +649,6 @@ struct phy_device {
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;
-	unsigned is_c45:1;
 	unsigned is_internal:1;
 	unsigned is_pseudo_fixed_link:1;
 	unsigned is_gigabit_capable:1;
@@ -665,6 +675,7 @@ struct phy_device {
 	int rate_matching;
 
 	enum phy_state state;
+	enum phy_access_mode access_mode;
 
 	u32 dev_flags;
 
@@ -768,7 +779,7 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
 
 static inline bool phy_has_c45_registers(struct phy_device *phydev)
 {
-	return phydev->is_c45;
+	return phydev->access_mode != PHY_ACCESS_C22;
 }
 
 /**
@@ -1624,7 +1635,7 @@ int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum,
 		     u16 mask, u16 set);
 
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
-				     bool is_c45,
+				     enum phy_access_mode mode,
 				     struct phy_c45_device_ids *c45_ids);
 #if IS_ENABLED(CONFIG_PHYLIB)
 int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
@@ -1632,7 +1643,8 @@ struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
 struct phy_device *device_phy_find_device(struct device *dev);
 struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode);
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
+struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
+				  enum phy_access_mode mode);
 int phy_device_register(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 #else
@@ -1664,7 +1676,8 @@ struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
 }
 
 static inline
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
+struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
+				  enum phy_access_mode mode)
 {
 	return NULL;
 }

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 04/11] net: phy: make the "prevent_c45_scan" a property of the MII bus
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (2 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-18 23:31   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 05/11] net: phy: print an info if a broken C45 bus is found Michael Walle
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

The blacklist will also be used elsewhere in the kernel, e.g. in the
DT scanning code. Make it a property of mii_bus and export the function.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
 - add missing EXPORT_SYMBOL_GPL() for
   mdiobus_scan_for_broken_c45_access()
---
 drivers/net/phy/mdio_bus.c | 18 +++++++++---------
 include/linux/phy.h        |  5 +++++
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index a31eb1204f63..29ad9302fe11 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -613,9 +613,9 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus)
  * stomping over the true devices reply, to performing a write to
  * themselves which was intended for another device. Now that C22
  * devices have been found, see if any of them are bad for C45, and if we
- * should skip the C45 scan.
+ * should prohibit any C45 transactions.
  */
-static bool mdiobus_prevent_c45_scan(struct mii_bus *bus)
+void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus)
 {
 	int i;
 
@@ -628,11 +628,13 @@ static bool mdiobus_prevent_c45_scan(struct mii_bus *bus)
 			continue;
 		oui = phydev->phy_id >> 10;
 
-		if (oui == MICREL_OUI)
-			return true;
+		if (oui == MICREL_OUI) {
+			bus->prevent_c45_access = true;
+			break;
+		}
 	}
-	return false;
 }
+EXPORT_SYMBOL_GPL(mdiobus_scan_for_broken_c45_access);
 
 /**
  * __mdiobus_register - bring up all the PHYs on a given bus and attach them to bus
@@ -652,7 +654,6 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 {
 	struct mdio_device *mdiodev;
 	struct gpio_desc *gpiod;
-	bool prevent_c45_scan;
 	int i, err;
 
 	if (!bus || !bus->name)
@@ -724,9 +725,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 			goto error;
 	}
 
-	prevent_c45_scan = mdiobus_prevent_c45_scan(bus);
-
-	if (!prevent_c45_scan && bus->read_c45) {
+	mdiobus_scan_for_broken_c45_access(bus);
+	if (!bus->prevent_c45_access && bus->read_c45) {
 		err = mdiobus_scan_bus_c45(bus);
 		if (err)
 			goto error;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fb7481715c3b..cd67887a7289 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -410,6 +410,9 @@ struct mii_bus {
 	/** @phy_ignore_ta_mask: PHY addresses to ignore the TA/read failure */
 	u32 phy_ignore_ta_mask;
 
+	/** @prevent_c45_access: Don't do any C45 transactions on the bus */
+	unsigned prevent_c45_access:1;
+
 	/**
 	 * @irq: An array of interrupts, each PHY's interrupt at the index
 	 * matching its address
@@ -462,6 +465,8 @@ static inline struct mii_bus *devm_mdiobus_alloc(struct device *dev)
 struct mii_bus *mdio_find_bus(const char *mdio_name);
 struct phy_device *mdiobus_scan_c22(struct mii_bus *bus, int addr);
 
+void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus);
+
 #define PHY_INTERRUPT_DISABLED	false
 #define PHY_INTERRUPT_ENABLED	true
 

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 05/11] net: phy: print an info if a broken C45 bus is found
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (3 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 04/11] net: phy: make the "prevent_c45_scan" a property of the MII bus Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-18 23:32   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 06/11] net: phy: add error checks in mmd_phy_indirect() Michael Walle
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

If there is an PHY which gets confused by C45 transactions on the MDIO
bus, print an info together with the PHY identifier of the offending
one.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
 - make it a dev_dbg()
 - check is phydev is not null
---
 drivers/net/phy/mdio_bus.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 29ad9302fe11..bdc178671234 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -617,10 +617,10 @@ static int mdiobus_scan_bus_c45(struct mii_bus *bus)
  */
 void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus)
 {
+	struct phy_device *phydev;
 	int i;
 
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
-		struct phy_device *phydev;
 		u32 oui;
 
 		phydev = mdiobus_get_phy(bus, i);
@@ -633,6 +633,11 @@ void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus)
 			break;
 		}
 	}
+
+	if (phydev && bus->prevent_c45_access)
+		dev_dbg(&bus->dev,
+			"Detected broken PHY (ID %08lx). Disabling C45 bus transactions.\n",
+			(unsigned long)phydev->phy_id);
 }
 EXPORT_SYMBOL_GPL(mdiobus_scan_for_broken_c45_access);
 

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 06/11] net: phy: add error checks in mmd_phy_indirect()
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (4 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 05/11] net: phy: print an info if a broken C45 bus is found Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-18 23:34   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd() Michael Walle
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, 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.

This is a preparation patch to introduce a new C45-over-C22 access
method which will make use of the new error checking.

Regarding the legacy handling, Russell states:

| The reason for that goes back to commit a59a4d192166 ("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.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
 - don't export it anymore, instead there will be a dedicated helper
---
 drivers/net/phy/phy-core.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 686a57d56885..598023610ee5 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -524,18 +524,26 @@ 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);
+
+	return check_rc ? ret : 0;
 }
 
 /**
@@ -563,7 +571,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 +627,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);

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd()
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (5 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 06/11] net: phy: add error checks in mmd_phy_indirect() Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-18 23:54   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers Michael Walle
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

Factor out the low-level MDIO bus access code in
__phy_{read,write}_mmd() into individual helper functions. These can
then be used without a struct phy_device, which is needed in the PHY
probing code.

To decide which access - direct or indirect - is used, move away from
phy_has_c45_registers(). That function only indicates whether the PHY
has C45 registers, but not how they are accessed. Instead look at the
access_mode property.

Export a locked variant of the read for the PHY probing code.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
 - new patch
---
 drivers/net/phy/phy-core.c | 109 +++++++++++++++++++++++++++++++--------------
 include/linux/phy.h        |   3 ++
 2 files changed, 78 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 598023610ee5..4863a85f8385 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -546,6 +546,73 @@ static int mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
 	return check_rc ? ret : 0;
 }
 
+static int __phy_mdiobus_read_mmd(struct mii_bus *bus, int phy_addr,
+				  enum phy_access_mode access_mode,
+				  int devad, u32 regnum)
+{
+	switch (access_mode) {
+	case PHY_ACCESS_C45:
+		return __mdiobus_c45_read(bus, phy_addr, devad, regnum);
+	case PHY_ACCESS_C22:
+		/* ignore return value for legacy reasons */
+		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
+
+		/* Read the content of the MMD's selected register */
+		return __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+/**
+ * phy_mdiobus_read_mmd - low-level function for reading a register
+ *
+ * @bus: the target MII bus
+ * @phy_addr: PHY address on the MII bus
+ * @mode: Access mode of the PHY
+ * @devad: The target MMD (0..31)
+ * @regnum: The target register on the MMD (0..65535)
+ *
+ * Similar to phy_read_mmd() except that it can be used without a phydev and
+ * operates on the MII bus.
+ */
+int phy_mdiobus_read_mmd(struct mii_bus *bus, int phy_addr,
+			 enum phy_access_mode mode,
+			 int devad, u32 regnum)
+{
+	int ret;
+
+	if (regnum > (u16)~0 || devad > 32)
+		return -EINVAL;
+
+	mutex_lock(&bus->mdio_lock);
+	ret = __phy_mdiobus_read_mmd(bus, phy_addr, mode, devad, regnum);
+	mutex_unlock(&bus->mdio_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(phy_mdiobus_read_mmd);
+
+static int __phy_mdiobus_write_mmd(struct mii_bus *bus, int phy_addr,
+				   enum phy_access_mode mode,
+				   int devad, u32 regnum, u16 val)
+{
+	switch (mode) {
+	case PHY_ACCESS_C45:
+		return __mdiobus_c45_write(bus, phy_addr, devad, regnum, val);
+	case PHY_ACCESS_C22:
+		/* ignore return value for legacy reasons */
+		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);
+
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 /**
  * __phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
@@ -557,26 +624,14 @@ static int mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
  */
 int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
 {
-	int val;
-
 	if (regnum > (u16)~0 || devad > 32)
 		return -EINVAL;
 
-	if (phydev->drv && phydev->drv->read_mmd) {
-		val = phydev->drv->read_mmd(phydev, devad, regnum);
-	} else if (phy_has_c45_registers(phydev)) {
-		val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
-					 devad, regnum);
-	} else {
-		struct mii_bus *bus = phydev->mdio.bus;
-		int phy_addr = phydev->mdio.addr;
-
-		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
+	if (phydev->drv && phydev->drv->read_mmd)
+		return phydev->drv->read_mmd(phydev, devad, regnum);
 
-		/* Read the content of the MMD's selected register */
-		val = __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
-	}
-	return val;
+	return __phy_mdiobus_read_mmd(phydev->mdio.bus, phydev->mdio.addr,
+				      phydev->access_mode, devad, regnum);
 }
 EXPORT_SYMBOL(__phy_read_mmd);
 
@@ -613,28 +668,14 @@ EXPORT_SYMBOL(phy_read_mmd);
  */
 int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 {
-	int ret;
-
 	if (regnum > (u16)~0 || devad > 32)
 		return -EINVAL;
 
-	if (phydev->drv && phydev->drv->write_mmd) {
-		ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
-	} else if (phy_has_c45_registers(phydev)) {
-		ret = __mdiobus_c45_write(phydev->mdio.bus, phydev->mdio.addr,
-					  devad, regnum, val);
-	} else {
-		struct mii_bus *bus = phydev->mdio.bus;
-		int phy_addr = phydev->mdio.addr;
-
-		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);
+	if (phydev->drv && phydev->drv->write_mmd)
+		return phydev->drv->write_mmd(phydev, devad, regnum, val);
 
-		ret = 0;
-	}
-	return ret;
+	return __phy_mdiobus_write_mmd(phydev->mdio.bus, phydev->mdio.addr,
+				       phydev->access_mode, devad, regnum, val);
 }
 EXPORT_SYMBOL(__phy_write_mmd);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index cd67887a7289..a482696a17a6 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1337,6 +1337,9 @@ int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
 	__ret; \
 })
 
+int phy_mdiobus_read_mmd(struct mii_bus *bus, int phy_addr,
+			 enum phy_access_mode mode,
+			 int devad, u32 regnum);
 /*
  * __phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (6 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd() Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-13  8:56   ` Simon Horman
  2023-07-12 15:07 ` [PATCH net-next v3 09/11] net: phy: introduce phy_promote_to_c45() Michael Walle
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, 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.

Add a new transfer mode C45-over-C22. Care has to be taken for
get_phy_device() which is called by either the bus scanning code or the
network device drivers. In the former case, we must not do any
C45-over-C22 accesses because it invokes register writes and we cannot
be sure if the accessed device is a PHY. C45-over-C22 is just defined
for network PHYs, not generic MDIO devices. Therefore, the it is the
callers responsibility to choose the access mode for get_phy_device().

Due to the reasons above, the current scanning code the PHY core cannot
make use of the new transfer mode because we cannot be sure what device
is a PHY. The code will be used for the device tree scanning where we
know which device is a PHY and therefore, C45-over-C22 is safe to use.

A word on the error checking in the MMD access methods: due to legacy
reasons, we cannot check for errors if we do "normal" MMD access to a
C22 PHY without introducing possible regressions. Although, C45-over-C22
is doing essentially the same access, we can do better now and check for
any errors while doing the indirect access.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
v3:
 - instead of the boolean c45_over_c22 flag pass an enum access_mode
 - use new low-level helper phy_mdiobus_read_mmd()
---
 drivers/net/phy/phy-core.c   | 25 ++++++++++++++++++++-----
 drivers/net/phy/phy_device.c | 41 +++++++++++++++++++++++++++--------------
 include/linux/phy.h          |  3 +++
 3 files changed, 50 insertions(+), 19 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 4863a85f8385..17ad118edbf1 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -550,12 +550,20 @@ static int __phy_mdiobus_read_mmd(struct mii_bus *bus, int phy_addr,
 				  enum phy_access_mode access_mode,
 				  int devad, u32 regnum)
 {
+	bool check_rc = true;
+	int ret;
+
 	switch (access_mode) {
 	case PHY_ACCESS_C45:
 		return __mdiobus_c45_read(bus, phy_addr, devad, regnum);
 	case PHY_ACCESS_C22:
 		/* ignore return value for legacy reasons */
-		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
+		check_rc = false;
+		fallthrough;
+	case PHY_ACCESS_C45_OVER_C22:
+		ret = mmd_phy_indirect(bus, phy_addr, devad, regnum, check_rc);
+		if (check_rc && ret)
+			return ret;
 
 		/* Read the content of the MMD's selected register */
 		return __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
@@ -597,17 +605,24 @@ static int __phy_mdiobus_write_mmd(struct mii_bus *bus, int phy_addr,
 				   enum phy_access_mode mode,
 				   int devad, u32 regnum, u16 val)
 {
+	bool check_rc = true;
+	int ret;
+
 	switch (mode) {
 	case PHY_ACCESS_C45:
 		return __mdiobus_c45_write(bus, phy_addr, devad, regnum, val);
 	case PHY_ACCESS_C22:
-		/* ignore return value for legacy reasons */
-		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
+		check_rc = false;
+		fallthrough;
+	case PHY_ACCESS_C45_OVER_C22:
+		ret = mmd_phy_indirect(bus, phy_addr, devad, regnum, check_rc);
+		if (check_rc && ret)
+			return ret;
 
 		/* Write the data into MMD's selected register */
-		__mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
+		ret = __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
 
-		return 0;
+		return check_rc ? ret : 0;
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6254212d65a5..01c41ecb63a7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -726,11 +726,12 @@ EXPORT_SYMBOL(phy_device_create);
  * Returns: negative error number on bus access error, zero if no device
  * is responding, or positive if a device is present.
  */
-static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
+static int phy_c45_probe_present(struct mii_bus *bus, int prtad,
+				 enum phy_access_mode mode, int devad)
 {
 	int stat2;
 
-	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
+	stat2 = phy_mdiobus_read_mmd(bus, prtad, mode, devad, MDIO_STAT2);
 	if (stat2 < 0)
 		return stat2;
 
@@ -748,17 +749,18 @@ static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
  *
  * Returns: 0 on success, -EIO on failure.
  */
-static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
+static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr,
+				   enum phy_access_mode mode, int dev_addr,
 				   u32 *devices_in_package)
 {
 	int phy_reg;
 
-	phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS2);
+	phy_reg = phy_mdiobus_read_mmd(bus, addr, mode, 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 = phy_mdiobus_read_mmd(bus, addr, mode, dev_addr, MDIO_DEVS1);
 	if (phy_reg < 0)
 		return -EIO;
 	*devices_in_package |= phy_reg;
@@ -780,6 +782,7 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
  * the "devices in package" is invalid.
  */
 static int get_phy_c45_ids(struct mii_bus *bus, int addr,
+			   enum phy_access_mode mode,
 			   struct phy_c45_device_ids *c45_ids)
 {
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
@@ -798,14 +801,15 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 			 * Some PHYs (88x3310) vendor space is not IEEE802.3
 			 * compliant.
 			 */
-			ret = phy_c45_probe_present(bus, addr, i);
+			ret = phy_c45_probe_present(bus, addr, mode, i);
 			if (ret < 0)
 				return -EIO;
 
 			if (!ret)
 				continue;
 		}
-		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, mode, i,
+						  &devs_in_pkg);
 		if (phy_reg < 0)
 			return -EIO;
 	}
@@ -815,7 +819,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 		 * MMD 0, as some 10G PHYs have zero Devices In package,
 		 * e.g. Cortina CS4315/CS4340 PHY.
 		 */
-		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, mode, 0,
+						  &devs_in_pkg);
 		if (phy_reg < 0)
 			return -EIO;
 
@@ -834,7 +839,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 			 * to ignore these if they do not contain IEEE 802.3
 			 * registers.
 			 */
-			ret = phy_c45_probe_present(bus, addr, i);
+			ret = phy_c45_probe_present(bus, addr, mode, i);
 			if (ret < 0)
 				return ret;
 
@@ -842,12 +847,14 @@ 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 = phy_mdiobus_read_mmd(bus, addr, mode, 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 = phy_mdiobus_read_mmd(bus, addr, mode, i,
+					       MII_PHYSID2);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] |= phy_reg;
@@ -937,6 +944,11 @@ EXPORT_SYMBOL(fwnode_get_phy_id);
  * If the "devices in package" appears valid, read the ID registers for each
  * MMD, allocate and return a &struct phy_device.
  *
+ * When using %PHY_ACCESS_C45_OVER_C22 as @mode care have to be taken to not
+ * access a non-PHY device as C45-over-C22 is a property of a PHY and not a
+ * generic MDIO device. As the access involves register writes, it may be
+ * destructive on non-PHY devices. IOW, it cannot be used for bus scanning.
+ *
  * Returns an allocated &struct phy_device on success, %-ENODEV if there is
  * no PHY present, or %-EIO on bus access error.
  */
@@ -956,7 +968,8 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
 		r = get_phy_c22_id(bus, addr, &phy_id);
 		break;
 	case PHY_ACCESS_C45:
-		r = get_phy_c45_ids(bus, addr, &c45_ids);
+	case PHY_ACCESS_C45_OVER_C22:
+		r = get_phy_c45_ids(bus, addr, mode, &c45_ids);
 		break;
 	default:
 		return ERR_PTR(-EIO);
@@ -971,7 +984,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr,
 	 * space, if successful, create the C45 PHY device.
 	 */
 	if (mode == PHY_ACCESS_C22 && phy_id == 0 && bus->read_c45) {
-		r = get_phy_c45_ids(bus, addr, &c45_ids);
+		r = get_phy_c45_ids(bus, addr, PHY_ACCESS_C45, &c45_ids);
 		if (!r)
 			return phy_device_create(bus, addr, phy_id,
 						 PHY_ACCESS_C45, &c45_ids);
@@ -1053,7 +1066,7 @@ EXPORT_SYMBOL(phy_device_remove);
 int phy_get_c45_ids(struct phy_device *phydev)
 {
 	return get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
-			       &phydev->c45_ids);
+			       PHY_ACCESS_C45, &phydev->c45_ids);
 }
 EXPORT_SYMBOL(phy_get_c45_ids);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a482696a17a6..e4014972b799 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -541,10 +541,13 @@ struct macsec_ops;
  *
  * @PHY_ACCESS_C22: use 802.3 c22 MDIO transactions
  * @PHY_ACCESS_C45: use 802.3 c45 MDIO transactions
+ * @PHY_ACCESS_C45_OVER_C22: indirectly access C45 registers by using by 802.3
+ *			     c22 MDIO transactions and registers 13 and 14.
  */
 enum phy_access_mode {
 	PHY_ACCESS_C22 = 22,
 	PHY_ACCESS_C45 = 45,
+	PHY_ACCESS_C45_OVER_C22 = 4522,
 };
 
 /**

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 09/11] net: phy: introduce phy_promote_to_c45()
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (7 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-13  8:56   ` Simon Horman
  2023-07-12 15:07 ` [PATCH net-next v3 10/11] net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy() Michael Walle
  2023-07-12 15:07 ` [PATCH net-next v3 11/11] net: mdio: support C45-over-C22 when probed via OF Michael Walle
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, 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 (!phy_is_c45(phydev)) {
           ret = phy_get_c45_ids(phydev);
           if (ret < 0)
                   return ret;
   }

Move that code into the core as it could be used by other drivers, too.
If a PHY is promoted C45-over-C22 access is automatically used as a
fallback if the bus doesn't support C45 transactions.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/net/phy/mxl-gpy.c    |  9 ++++-----
 drivers/net/phy/phy_device.c | 36 ++++++++++++++++++++++++++++++------
 include/linux/phy.h          |  7 ++++++-
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index b4ddb9a003d9..b7fca1aae1c3 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 (!phy_has_c45_registers(phydev)) {
-		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 01c41ecb63a7..97fb3cb8b54c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1057,18 +1057,42 @@ 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 register access 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 the PHY from C22 to C45.
+ *
+ * Can also be called if a PHY is already a C45 one. In that case it does
+ * nothing.
+ *
+ * If the bus isn't capable of doing C45 transfers, C45-over-C22 access will be
+ * used as a fallback.
  *
  * 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)
 {
-	return get_phy_c45_ids(phydev->mdio.bus, phydev->mdio.addr,
-			       PHY_ACCESS_C45, &phydev->c45_ids);
+	struct mii_bus *bus = phydev->mdio.bus;
+
+	if (phydev->access_mode != PHY_ACCESS_C22)
+		return 0;
+
+	if (dev_of_node(&phydev->mdio.dev))
+		phydev_info(phydev,
+			    "Promoting PHY to a C45 one. Please consider using compatible=\"ethernet-phy-ieee802.3-c45\".");
+
+	if (mdiobus_supports_c45(bus))
+		phydev->access_mode = PHY_ACCESS_C45;
+	else
+		phydev->access_mode = PHY_ACCESS_C45_OVER_C22;
+
+	return get_phy_c45_ids(bus, phydev->mdio.addr, phydev->access_mode,
+			       &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 e4014972b799..180fa5ac0b29 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -467,6 +467,11 @@ struct phy_device *mdiobus_scan_c22(struct mii_bus *bus, int addr);
 
 void mdiobus_scan_for_broken_c45_access(struct mii_bus *bus);
 
+static inline bool mdiobus_supports_c45(struct mii_bus *bus)
+{
+	return bus->read_c45 && !bus->prevent_c45_access;
+}
+
 #define PHY_INTERRUPT_DISABLED	false
 #define PHY_INTERRUPT_ENABLED	true
 
@@ -1701,7 +1706,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.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 10/11] net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy()
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (8 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 09/11] net: phy: introduce phy_promote_to_c45() Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  2023-07-19  0:03   ` Andrew Lunn
  2023-07-12 15:07 ` [PATCH net-next v3 11/11] net: mdio: support C45-over-C22 when probed via OF Michael Walle
  10 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

If trying to register a C45 PHY on an MDIO bus which isn't capable of
C45 (either because the MDIO controller doesn't support it or because
C45 accesses are prohibited due to faulty C22 PHYs) we can fall back to
the new C45-over-C22 access method.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
Please note, that both with the old and the new code compatible =
"ethernet-phy-idNNNN.NNNN" only works for the C22 case. I'm wondering if
compatible = "ethernet-phy-idNNNN.NNNN", "ethernet-phy-ieee802.3-c45
even makes sense because there might be multiple C45 ids. At least it is
an allowed pattern according to the device tree bindings. But with the
current code, the ethernet-phy-idNNNN.NNNN is ignored in the c45 case.
---
 drivers/net/mdio/fwnode_mdio.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 972c8932c2fe..fed056d82b4e 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -115,7 +115,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 	struct mii_timestamper *mii_ts = NULL;
 	struct pse_control *psec = NULL;
 	struct phy_device *phy;
-	bool is_c45;
 	u32 phy_id;
 	int rc;
 
@@ -129,13 +128,19 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 		goto clean_pse;
 	}
 
-	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
-	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
-		phy = get_phy_device(bus, addr,
-				     is_c45 ? PHY_ACCESS_C45 : PHY_ACCESS_C22);
-	else
+	if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45")) {
+		if (mdiobus_supports_c45(bus))
+			phy = get_phy_device(bus, addr, PHY_ACCESS_C45);
+		else
+			phy = get_phy_device(bus, addr,
+					     PHY_ACCESS_C45_OVER_C22);
+	} else if (fwnode_get_phy_id(child, &phy_id) == 0) {
 		phy = phy_device_create(bus, addr, phy_id, PHY_ACCESS_C22,
 					NULL);
+	} else {
+		phy = get_phy_device(bus, addr, PHY_ACCESS_C22);
+	}
+
 	if (IS_ERR(phy)) {
 		rc = PTR_ERR(phy);
 		goto clean_mii_ts;

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH net-next v3 11/11] net: mdio: support C45-over-C22 when probed via OF
  2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
                   ` (9 preceding siblings ...)
  2023-07-12 15:07 ` [PATCH net-next v3 10/11] net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy() Michael Walle
@ 2023-07-12 15:07 ` Michael Walle
  10 siblings, 0 replies; 47+ messages in thread
From: Michael Walle @ 2023-07-12 15:07 UTC (permalink / raw)
  To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang
  Cc: netdev, linux-kernel, Simon Horman, Michael Walle

Fall back to C45-over-C22 when the MDIO bus isn't capable of doing C45
transfers. This might be the case if there are broken PHYs on the bus or
if the MDIO controller cannot do C45 transactions at all.

For this to work, split the PHY registration into three steps, as done
in the generic PHY probing code:
 (1) add C22 PHYs
 (2) scan for broken C22 PHYs
 (3) add C45 PHYs

If step (2) detects a broken PHY, any PHYs will be added with
C45-over-C22 access in step (3). Step (3) also ensures, that
C45-over-C22 is used if C45 access is not supported at all on the bus.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/net/mdio/of_mdio.c | 63 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 15 deletions(-)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 7eb32ebb846d..e9d3cf6b68ee 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -100,6 +100,11 @@ static const struct of_device_id whitelist_phys[] = {
 	{}
 };
 
+static bool of_mdiobus_child_is_c45_phy(struct device_node *child)
+{
+	return of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
+}
+
 /*
  * Return true if the child node is for a phy. It must either:
  * o Compatible string of "ethernet-phy-idX.X"
@@ -118,7 +123,7 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
 	if (of_get_phy_id(child, &phy_id) != -EINVAL)
 		return true;
 
-	if (of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
+	if (of_mdiobus_child_is_c45_phy(child))
 		return true;
 
 	if (of_device_is_compatible(child, "ethernet-phy-ieee802.3-c22"))
@@ -138,6 +143,32 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
 }
 EXPORT_SYMBOL(of_mdiobus_child_is_phy);
 
+static int of_mdiobus_register_child(struct mii_bus *mdio,
+				     struct device_node *child, bool *scanphys)
+{
+	int addr, rc;
+
+	addr = of_mdio_parse_addr(&mdio->dev, child);
+	if (addr < 0) {
+		*scanphys = true;
+		return 0;
+	}
+
+	if (mdiobus_is_registered_device(mdio, addr))
+		return 0;
+
+	if (of_mdiobus_child_is_phy(child))
+		rc = of_mdiobus_register_phy(mdio, child, addr);
+	else
+		rc = of_mdiobus_register_device(mdio, child, addr);
+
+	if (rc == -ENODEV)
+		dev_err(&mdio->dev, "MDIO device at address %d is missing.\n",
+			addr);
+
+	return rc;
+}
+
 /**
  * __of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -178,24 +209,26 @@ int __of_mdiobus_register(struct mii_bus *mdio, struct device_node *np,
 	if (rc)
 		return rc;
 
-	/* Loop over the child nodes and register a phy_device for each phy */
+	/* Loop over the child nodes, skipping C45 PHYs so we can scan for
+	 * broken C22 PHYs. The C45 PHYs will be registered afterwards.
+	 */
 	for_each_available_child_of_node(np, child) {
-		addr = of_mdio_parse_addr(&mdio->dev, child);
-		if (addr < 0) {
-			scanphys = true;
+		if (of_mdiobus_child_is_c45_phy(child))
 			continue;
-		}
+		rc = of_mdiobus_register_child(mdio, child, &scanphys);
+		if (rc)
+			goto unregister;
+	}
 
-		if (of_mdiobus_child_is_phy(child))
-			rc = of_mdiobus_register_phy(mdio, child, addr);
-		else
-			rc = of_mdiobus_register_device(mdio, child, addr);
+	/* Some C22 PHYs are broken with C45 transactions. */
+	mdiobus_scan_for_broken_c45_access(mdio);
 
-		if (rc == -ENODEV)
-			dev_err(&mdio->dev,
-				"MDIO device at address %d is missing.\n",
-				addr);
-		else if (rc)
+	/* Now add any missing C45 PHYs. If C45 access is not allowed, they
+	 * will be registered with C45-over-C22 access.
+	 */
+	for_each_available_child_of_node(np, child) {
+		rc = of_mdiobus_register_child(mdio, child, &scanphys);
+		if (rc)
 			goto unregister;
 	}
 

-- 
2.39.2


^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers
  2023-07-12 15:07 ` [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers Michael Walle
@ 2023-07-13  8:56   ` Simon Horman
  2023-07-13  9:00     ` Michael Walle
  0 siblings, 1 reply; 47+ messages in thread
From: Simon Horman @ 2023-07-13  8:56 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang,
	netdev, linux-kernel

On Wed, Jul 12, 2023 at 05:07:08PM +0200, Michael Walle wrote:

...

> @@ -780,6 +782,7 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
>   * the "devices in package" is invalid.
>   */
>  static int get_phy_c45_ids(struct mii_bus *bus, int addr,
> +			   enum phy_access_mode mode,

Hi Michael,

Please add the new mode parameter to the kernel doc for this function,
which is in the lines preceding this hunk.

...

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 09/11] net: phy: introduce phy_promote_to_c45()
  2023-07-12 15:07 ` [PATCH net-next v3 09/11] net: phy: introduce phy_promote_to_c45() Michael Walle
@ 2023-07-13  8:56   ` Simon Horman
  0 siblings, 0 replies; 47+ messages in thread
From: Simon Horman @ 2023-07-13  8:56 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang,
	netdev, linux-kernel

On Wed, Jul 12, 2023 at 05:07:09PM +0200, Michael Walle wrote:
> If not explitly asked to be probed as a C45 PHY, on a bus which is

nit: explitly -> explicitly

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers
  2023-07-13  8:56   ` Simon Horman
@ 2023-07-13  9:00     ` Michael Walle
  2023-07-13  9:19       ` Simon Horman
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-13  9:00 UTC (permalink / raw)
  To: Simon Horman
  Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang,
	netdev, linux-kernel

Hi Simon,

Am 2023-07-13 10:56, schrieb Simon Horman:
> On Wed, Jul 12, 2023 at 05:07:08PM +0200, Michael Walle wrote:
> 
> ...
> 
>> @@ -780,6 +782,7 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus 
>> *bus, int addr, int dev_addr,
>>   * the "devices in package" is invalid.
>>   */
>>  static int get_phy_c45_ids(struct mii_bus *bus, int addr,
>> +			   enum phy_access_mode mode,
> 
> Please add the new mode parameter to the kernel doc for this function,
> which is in the lines preceding this hunk.

Ahh. Sorry. You've already mentioned that last time :/

Also true for get_phy_c45_devs_in_pkg().

-michael

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers
  2023-07-13  9:00     ` Michael Walle
@ 2023-07-13  9:19       ` Simon Horman
  0 siblings, 0 replies; 47+ messages in thread
From: Simon Horman @ 2023-07-13  9:19 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang,
	netdev, linux-kernel

On Thu, Jul 13, 2023 at 11:00:07AM +0200, Michael Walle wrote:
> Hi Simon,
> 
> Am 2023-07-13 10:56, schrieb Simon Horman:
> > On Wed, Jul 12, 2023 at 05:07:08PM +0200, Michael Walle wrote:
> > 
> > ...
> > 
> > > @@ -780,6 +782,7 @@ static int get_phy_c45_devs_in_pkg(struct
> > > mii_bus *bus, int addr, int dev_addr,
> > >   * the "devices in package" is invalid.
> > >   */
> > >  static int get_phy_c45_ids(struct mii_bus *bus, int addr,
> > > +			   enum phy_access_mode mode,
> > 
> > Please add the new mode parameter to the kernel doc for this function,
> > which is in the lines preceding this hunk.
> 
> Ahh. Sorry. You've already mentioned that last time :/
> 
> Also true for get_phy_c45_devs_in_pkg().

Thanks, I missed that one today.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 01/11] net: phy: get rid of redundant is_c45 information
  2023-07-12 15:07 ` [PATCH net-next v3 01/11] net: phy: get rid of redundant is_c45 information Michael Walle
@ 2023-07-18 17:25   ` Andrew Lunn
  0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 17:25 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Jul 12, 2023 at 05:07:01PM +0200, Michael Walle wrote:
> phy_device_create() will be called with is_c45 and c45_ids. If c45_ids
> are set, is_c45 is (usually) true. Change the only caller which do
> things differently, then drop the is_c45 check in phy_device_create().
> 
> This is a preparation patch to replace the is_c45 boolean with an enum
> which will indicate how the PHY is accessed (by c22, c45 or
> c45-over-c22).
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-07-12 15:07 ` [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers() Michael Walle
@ 2023-07-18 17:26   ` Andrew Lunn
  2023-07-18 20:07   ` Andrew Lunn
  1 sibling, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 17:26 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Jul 12, 2023 at 05:07:02PM +0200, Michael Walle wrote:
> Provide a helper to determine if the PHY has a C45 register space. This
> is a preparation patch to remove the is_c45 field. No functional change.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-12 15:07 ` [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode Michael Walle
@ 2023-07-18 17:40   ` Andrew Lunn
  2023-07-18 17:52     ` Russell King (Oracle)
  2023-07-18 19:53     ` Michael Walle
  0 siblings, 2 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 17:40 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

>  static inline bool phy_has_c45_registers(struct phy_device *phydev)
>  {
> -	return phydev->is_c45;
> +	return phydev->access_mode != PHY_ACCESS_C22;
>  }

So this is making me wounder if we have a clean separation between
register spaces and access methods.

Should there be a phy_has_c22_registers() ?

A PHY can have both C22 registers and C45 registers. It is up to the
driver to decide which it wants to access when.

Should phydev->access_mode really be phydev->access_mode_c45_registers
to indicate how to access the C45 registers if phy_has_c45_registers()
is true?

Has there been a review of all uses of phydev->is_c45 to determine if
the user wants to know if C45 registers exist,
a.k.a. phy_has_c45_registers(), or if C45 bus transactions can be
performed, and then later in this series, additionally if C45 over C22
can be performed. These are different things.

I need to keep reading the patches...

  Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-18 17:40   ` Andrew Lunn
@ 2023-07-18 17:52     ` Russell King (Oracle)
  2023-07-18 19:18       ` Andrew Lunn
  2023-07-18 19:53     ` Michael Walle
  1 sibling, 1 reply; 47+ messages in thread
From: Russell King (Oracle) @ 2023-07-18 17:52 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michael Walle, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Tue, Jul 18, 2023 at 07:40:49PM +0200, Andrew Lunn wrote:
> >  static inline bool phy_has_c45_registers(struct phy_device *phydev)
> >  {
> > -	return phydev->is_c45;
> > +	return phydev->access_mode != PHY_ACCESS_C22;
> >  }
> 
> So this is making me wounder if we have a clean separation between
> register spaces and access methods.
> 
> Should there be a phy_has_c22_registers() ?

Yes, I've been wondering that. I've recently heard about a Realtek PHY
which is supported by our realtek driver, but appears on a SFP that
can only do C45 accesses. However, the realtek driver is written to
use C22 accesses to this PHY - and the PHY supports both. So currently
it doesn't work.

That's just an additional data point for thinking about this, I haven't
formulated a solution to it yet.

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-18 17:52     ` Russell King (Oracle)
@ 2023-07-18 19:18       ` Andrew Lunn
  2023-07-18 21:46         ` Russell King (Oracle)
  0 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 19:18 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Michael Walle, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Tue, Jul 18, 2023 at 06:52:12PM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 18, 2023 at 07:40:49PM +0200, Andrew Lunn wrote:
> > >  static inline bool phy_has_c45_registers(struct phy_device *phydev)
> > >  {
> > > -	return phydev->is_c45;
> > > +	return phydev->access_mode != PHY_ACCESS_C22;
> > >  }
> > 
> > So this is making me wounder if we have a clean separation between
> > register spaces and access methods.
> > 
> > Should there be a phy_has_c22_registers() ?
> 
> Yes, I've been wondering that. I've recently heard about a Realtek PHY
> which is supported by our realtek driver, but appears on a SFP that
> can only do C45 accesses. However, the realtek driver is written to
> use C22 accesses to this PHY - and the PHY supports both. So currently
> it doesn't work.
>
> That's just an additional data point for thinking about this, I haven't
> formulated a solution to it yet.

That kind of sounds like two drivers. Or two drivers in one .c
file. Do you know what C45 IDs it has? Same as the C22? If it is
different, each could have its own struct phy_driver.

	   Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-18 17:40   ` Andrew Lunn
  2023-07-18 17:52     ` Russell King (Oracle)
@ 2023-07-18 19:53     ` Michael Walle
  2023-07-18 20:16       ` Andrew Lunn
  1 sibling, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-18 19:53 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

Am 2023-07-18 19:40, schrieb Andrew Lunn:
>>  static inline bool phy_has_c45_registers(struct phy_device *phydev)
>>  {
>> -	return phydev->is_c45;
>> +	return phydev->access_mode != PHY_ACCESS_C22;
>>  }
> 
> So this is making me wounder if we have a clean separation between
> register spaces and access methods.

The idea is to at least have it behind a helper which can be changed
if we get that information from somewhere else.

But right now, a PHY is considered to have c45 registers if it is
probed via c45 (accesses).

Instead of checking the access mode, I could also introduce a
bitmask(?)/flags has_c22/has_c45_registers. But how would you tell
if a PHY as c22 registers? Probe both c22 and c45? What if the bus
can't do c45?

> Should there be a phy_has_c22_registers() ?
> 
> A PHY can have both C22 registers and C45 registers. It is up to the
> driver to decide which it wants to access when.

But isn't it also the driver which has the ultimate information whether
a PHY has c22 register space and/or c45 one?

Maybe we need to clarify what "has c22/c45 registers space" actually
means. Responds to MII c22/c45 access?

-michael

> Should phydev->access_mode really be phydev->access_mode_c45_registers
> to indicate how to access the C45 registers if phy_has_c45_registers()
> is true?
> 
> Has there been a review of all uses of phydev->is_c45 to determine if
> the user wants to know if C45 registers exist,
> a.k.a. phy_has_c45_registers(), or if C45 bus transactions can be
> performed, and then later in this series, additionally if C45 over C22
> can be performed. These are different things.
> 
> I need to keep reading the patches...
> 
>   Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-07-12 15:07 ` [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers() Michael Walle
  2023-07-18 17:26   ` Andrew Lunn
@ 2023-07-18 20:07   ` Andrew Lunn
  2023-07-19  7:11     ` Michael Walle
  1 sibling, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 20:07 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Jul 12, 2023 at 05:07:02PM +0200, Michael Walle wrote:
> Provide a helper to determine if the PHY has a C45 register space. This
> is a preparation patch to remove the is_c45 field. No functional change.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> v3:
>  - rename to phy_has_c45_registers()
> ---
>  drivers/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                            |  8 +++++---
>  drivers/net/phy/phy_device.c                     |  6 +++---
>  drivers/net/phy/phylink.c                        | 12 +++++++-----
>  include/linux/phy.h                              |  5 +++++
>  9 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> index b54f3706fb97..387d24f195aa 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 && !phy_has_c45_registers(netdev->phydev))
>  			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 || phy_has_c45_registers(netdev->phydev))
>  			cnt--;

These two look to be phy_has_c22_registers(). Probably. It calls
phy_loopback() and it seems to assume that only works for C22
registers, because of the genphy_loopback() call.

>  
>  		return cnt;
> diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
> index 9717a1626f3f..857344260230 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 (!phy_has_c45_registers(phydev) ||
>  	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
>  		return -ENODEV;

The comment indicates it. phy_has_c45_registers() is correct, and how
you access them does not matter.

> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 55d9d7acc32e..d1c12843462f 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 (!phy_has_c45_registers(phydev) ||
>  	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
>  		return -ENODEV;

Correct, we need the C45 registers. How we access them does not
matter.

> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index ea1073adc5a1..b4ddb9a003d9 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 (!phy_has_c45_registers(phydev)) {
>  		ret = phy_get_c45_ids(phydev);
>  		if (ret < 0)
>  			return ret;

This is i think the oddball case which gits a patch of its own
later. This is about, did we discover the PHY via C22. If so, we need
to promote to C45. So i actually think this should be expressed
something like

	  if (phy_has_c22_registers(phydev) &&
	      !phy_has_c45_registers(phydev))

> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index a64186dc53f8..686a57d56885 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -556,7 +556,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 (phy_has_c45_registers(phydev)) {

This i would say should be

phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
on the bus, and if not, fall back to C45 over C22.

>  		val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
>  					 devad, regnum);
>  	} else {
> @@ -612,7 +612,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 (phy_has_c45_registers(phydev)) {

same as above.

>  		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 bdf00b2b2c1d..c6059ba8b9e6 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -182,7 +182,8 @@ int phy_restart_aneg(struct phy_device *phydev)
>  {
>  	int ret;
>  
> -	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
> +	if (phy_has_c45_registers(phydev) &&
> +	    !(phydev->c45_ids.devices_in_package & BIT(0)))
>  		ret = genphy_c45_restart_aneg(phydev);
>  	else
>  		ret = genphy_restart_aneg(phydev);

Correct.

> @@ -203,7 +204,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 (phy_has_c45_registers(phydev))
>  		return genphy_c45_aneg_done(phydev);
>  	else
>  		return genphy_aneg_done(phydev);

Correct.

> @@ -896,7 +897,8 @@ 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 (phy_has_c45_registers(phydev) &&
> +	    !(phydev->c45_ids.devices_in_package & BIT(0)))
>  		return genphy_c45_config_aneg(phydev);

Correct.

>  	return genphy_config_aneg(phydev);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 226d5507c865..44968ea447fc 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -533,7 +533,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 (phy_has_c45_registers(phydev)) {
>  		for (i = 1; i < num_ids; i++) {
>  			if (phydev->c45_ids.device_ids[i] == 0xffffffff)
>  				continue;

Correct, do we have C45 ID registers, and does not matter how we got
them.

> @@ -1452,7 +1452,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 (phy_has_c45_registers(phydev))
>  			d->driver = &genphy_c45_driver.mdiodrv.driver;
>  		else
>  			d->driver = &genphy_driver.mdiodrv.driver;

Correct.

> @@ -3227,7 +3227,7 @@ static int phy_probe(struct device *dev)
>  	}
>  	else if (phydrv->get_features)
>  		err = phydrv->get_features(phydev);
> -	else if (phydev->is_c45)
> +	else if (phy_has_c45_registers(phydev))
>  		err = genphy_c45_pma_read_abilities(phydev);
>  	else
>  		err = genphy_read_abilities(phydev);

Correct.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index d0aaa5cad853..54fde8252079 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1726,7 +1726,8 @@ 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_registers(phy) &&
> +	    config.rate_matching == RATE_MATCH_NONE &&
>  	    interface != PHY_INTERFACE_MODE_RXAUI &&
>  	    interface != PHY_INTERFACE_MODE_XAUI &&
>  	    interface != PHY_INTERFACE_MODE_USXGMII)

I would say correct. The comment above this is:

	/* Clause 45 PHYs may switch their Serdes lane between, e.g. 10GBASE-R,
	 * 5GBASE-R, 2500BASE-X and SGMII if they are not using rate matching.

I suppose a C22 PHY could as well, but none currently do, and actually
implementing anything faster than 1G without C45 is pretty difficult.

> @@ -2673,7 +2674,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
>  					reg);
>  	}
>  
> -	if (phydev->is_c45) {
> +	if (phy_has_c45_registers(phydev)) {
>  		switch (reg) {
>  		case MII_BMCR:
>  		case MII_BMSR:

This one is less clear. It ends up doing:

		return mdiobus_c45_read(pl->phydev->mdio.bus, prtad, devad,
					reg);

which is a C45 bus transaction. So i would actually say this should be
phy_has_c45_transfers(phydev). And maybe at some point in the future
this gets changed to use a higher level function which can do either
C45 or C45 over C22, and then the condition changed to
phy_has_c45_registers(phydev).

> @@ -2715,7 +2716,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
>  					 reg, val);
>  	}
>  
> -	if (phydev->is_c45) {
> +	if (phy_has_c45_registers(phydev)) {
>  		switch (reg) {
>  		case MII_BMCR:
>  		case MII_BMSR:

This is the same as the previous.

> @@ -3188,8 +3189,9 @@ static void phylink_sfp_link_up(void *upstream)
>   */
>  static bool phylink_phy_no_inband(struct phy_device *phy)
>  {
> -	return phy->is_c45 && phy_id_compare(phy->c45_ids.device_ids[1],
> -					     0xae025150, 0xfffffff0);
> +	return phy_has_c45_registers(phy) &&
> +	       phy_id_compare(phy->c45_ids.device_ids[1],
> +			      0xae025150, 0xfffffff0);
>  }

Correct, we are looking at a C45 ID.

>  static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 11c1e91563d4..fdb3774e99fc 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -766,6 +766,11 @@ static inline struct phy_device *to_phy_device(const struct device *dev)
>  	return container_of(to_mdio_device(dev), struct phy_device, mdio);
>  }
>  
> +static inline bool phy_has_c45_registers(struct phy_device *phydev)
> +{
> +	return phydev->is_c45;
> +}

And this is where it gets interesting. I think as a first step, you
should implement the four functions:

phy_has_c22_registers()
phy_has_c45_registers()
phy_has_c22_transfers()
phy_has_c45_transfers()

based on this. So there is initially no functional change.

You can then change the implementation of _transfers() based on what
the MDIO bus can do, plus the quirk for if a FUBAR microchip PHY has
been found.

Then change the implementation of _registers() based on the results of
probing for the ID registers.

That should give us a basis for a clean separation between register
spaces and bus transaction, and then adding C45 over C22 should be
more obviously correct.

	Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-18 19:53     ` Michael Walle
@ 2023-07-18 20:16       ` Andrew Lunn
  0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 20:16 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

> Maybe we need to clarify what "has c22/c45 registers space" actually
> means. Responds to MII c22/c45 access?

No. That is not what it means to me. Bus transactions and register
spaces are different.

There are C22 registers, and there are C45 registers.

The bus can do C22 transfers, and it can do C45 transfers.

C22 registers can be access using C22 bus transfers.

C45 registers can be accessed using either C45 bus transfers, or
indirectly via C45 over C22 transfers.

Currently, there is no C22 over C45, but given the oddball Realtek PHY
Russell is talking about in an SFP, it might have a proprietary C22
over C45?

     Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-18 19:18       ` Andrew Lunn
@ 2023-07-18 21:46         ` Russell King (Oracle)
  2023-07-18 23:30           ` Andrew Lunn
  0 siblings, 1 reply; 47+ messages in thread
From: Russell King (Oracle) @ 2023-07-18 21:46 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michael Walle, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Tue, Jul 18, 2023 at 09:18:51PM +0200, Andrew Lunn wrote:
> On Tue, Jul 18, 2023 at 06:52:12PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jul 18, 2023 at 07:40:49PM +0200, Andrew Lunn wrote:
> > > >  static inline bool phy_has_c45_registers(struct phy_device *phydev)
> > > >  {
> > > > -	return phydev->is_c45;
> > > > +	return phydev->access_mode != PHY_ACCESS_C22;
> > > >  }
> > > 
> > > So this is making me wounder if we have a clean separation between
> > > register spaces and access methods.
> > > 
> > > Should there be a phy_has_c22_registers() ?
> > 
> > Yes, I've been wondering that. I've recently heard about a Realtek PHY
> > which is supported by our realtek driver, but appears on a SFP that
> > can only do C45 accesses. However, the realtek driver is written to
> > use C22 accesses to this PHY - and the PHY supports both. So currently
> > it doesn't work.
> >
> > That's just an additional data point for thinking about this, I haven't
> > formulated a solution to it yet.
> 
> That kind of sounds like two drivers. Or two drivers in one .c
> file. Do you know what C45 IDs it has? Same as the C22? If it is
> different, each could have its own struct phy_driver.

Why would it be two drivers?

The PHY in question is RTL8221B-VB-CG, and it has the same ID in both
the C22 and C45 registers, since the realtek driver attempts to drive
the device, but fails because __phy_read() errors out (because there's
no C22 bus access.)

If one looks at:

https://www.realtek.com/en/products/connected-media-ics/item/rtl8221b-i-vb-cg

it states "Supports clause 22 and Clause 45 MDC/MDIO management
interface".

which to me suggests it can be driven via clause 22 MDIO cycles on
the bus as a clause 22 PHY and using indirect access to clause 45
registers, or only via clause 45 MDIO cycles.

In the case of this particular SFP, it apparently uses the Rollball
access method which is C45-only.

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode
  2023-07-18 21:46         ` Russell King (Oracle)
@ 2023-07-18 23:30           ` Andrew Lunn
  0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 23:30 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Michael Walle, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

> Why would it be two drivers?

I'm assuming there is no C22 over C45. So phy_read() is not going to
work. I'm also assuming the current driver is using the C22 register
space.

So either the driver needs re-writing to use only the C45 register
space, maybe using C45 over C22, or it needs a second parallel driver
using just the C45 register space when only C45 bus transfers are
available.

	Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 04/11] net: phy: make the "prevent_c45_scan" a property of the MII bus
  2023-07-12 15:07 ` [PATCH net-next v3 04/11] net: phy: make the "prevent_c45_scan" a property of the MII bus Michael Walle
@ 2023-07-18 23:31   ` Andrew Lunn
  0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 23:31 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Jul 12, 2023 at 05:07:04PM +0200, Michael Walle wrote:
> The blacklist will also be used elsewhere in the kernel, e.g. in the
> DT scanning code. Make it a property of mii_bus and export the function.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 05/11] net: phy: print an info if a broken C45 bus is found
  2023-07-12 15:07 ` [PATCH net-next v3 05/11] net: phy: print an info if a broken C45 bus is found Michael Walle
@ 2023-07-18 23:32   ` Andrew Lunn
  0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 23:32 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Jul 12, 2023 at 05:07:05PM +0200, Michael Walle wrote:
> If there is an PHY which gets confused by C45 transactions on the MDIO
> bus, print an info together with the PHY identifier of the offending
> one.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 06/11] net: phy: add error checks in mmd_phy_indirect()
  2023-07-12 15:07 ` [PATCH net-next v3 06/11] net: phy: add error checks in mmd_phy_indirect() Michael Walle
@ 2023-07-18 23:34   ` Andrew Lunn
  0 siblings, 0 replies; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 23:34 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Jul 12, 2023 at 05:07:06PM +0200, 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.
> 
> This is a preparation patch to introduce a new C45-over-C22 access
> method which will make use of the new error checking.
> 
> Regarding the legacy handling, Russell states:
> 
> | The reason for that goes back to commit a59a4d192166 ("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.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd()
  2023-07-12 15:07 ` [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd() Michael Walle
@ 2023-07-18 23:54   ` Andrew Lunn
  2023-07-19  7:21     ` Michael Walle
  0 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-07-18 23:54 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

> +static int __phy_mdiobus_read_mmd(struct mii_bus *bus, int phy_addr,
> +				  enum phy_access_mode access_mode,
> +				  int devad, u32 regnum)
> +{
> +	switch (access_mode) {
> +	case PHY_ACCESS_C45:
> +		return __mdiobus_c45_read(bus, phy_addr, devad, regnum);
> +	case PHY_ACCESS_C22:
> +		/* ignore return value for legacy reasons */
> +		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
> +
> +		/* Read the content of the MMD's selected register */
> +		return __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
> +	default:
> +		return -EOPNOTSUPP;
> +	}

So this is reading a C45 register space register, otherwise it would
not be called _mmd and have a devad. So access_mode should really be
transfer mode. Until now, only transfer mode C45 can be used to access
C45 register space. The point of this patchset is to add a new
C45_OVER_C22 transfer mode.

And C22 would should give -EINVAL, since you cannot use plain C22 bus
transactions to access C45 register space.

	Andrew 

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 10/11] net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy()
  2023-07-12 15:07 ` [PATCH net-next v3 10/11] net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy() Michael Walle
@ 2023-07-19  0:03   ` Andrew Lunn
  2023-07-19  7:32     ` Michael Walle
  0 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-07-19  0:03 UTC (permalink / raw)
  To: Michael Walle
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

> Please note, that both with the old and the new code compatible =
> "ethernet-phy-idNNNN.NNNN" only works for the C22 case. I'm wondering if
> compatible = "ethernet-phy-idNNNN.NNNN", "ethernet-phy-ieee802.3-c45
> even makes sense because there might be multiple C45 ids. At least it is
> an allowed pattern according to the device tree bindings. But with the
> current code, the ethernet-phy-idNNNN.NNNN is ignored in the c45 case.

I think these two should be orthogonal.

ethernet-phy-idNNNN.NNNN should be used to load the driver. The driver
has a list of IDs it can drive, and we want the module loading
machinery to load a module which can driver this ID.

"ethernet-phy-ieee802.3-c45" should be about how to read the ID
registers, if ethernet-phy-idNNNN.NNNN is not present.

	Andrew


^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-07-18 20:07   ` Andrew Lunn
@ 2023-07-19  7:11     ` Michael Walle
  2023-08-01 14:47       ` Michael Walle
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-07-19  7:11 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman


>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index a64186dc53f8..686a57d56885 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -556,7 +556,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 (phy_has_c45_registers(phydev)) {
> 
> This i would say should be
> 
> phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
> on the bus, and if not, fall back to C45 over C22.

Shouldn't this then be a bus property? I.e. mdiobus_has_c45_transfers().
I've have a similar helper introduced in 9/11:

static inline bool mdiobus_supports_c45(struct mii_bus *bus)
{
     return bus->read_c45 && !bus->prevent_c45_access;
}

>>  static int phylink_sfp_connect_phy(void *upstream, struct phy_device 
>> *phy)
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 11c1e91563d4..fdb3774e99fc 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -766,6 +766,11 @@ static inline struct phy_device 
>> *to_phy_device(const struct device *dev)
>>  	return container_of(to_mdio_device(dev), struct phy_device, mdio);
>>  }
>> 
>> +static inline bool phy_has_c45_registers(struct phy_device *phydev)
>> +{
>> +	return phydev->is_c45;
>> +}
> 
> And this is where it gets interesting. I think as a first step, you
> should implement the four functions:
> 
> phy_has_c22_registers()
> phy_has_c45_registers()
> phy_has_c22_transfers()
> phy_has_c45_transfers()
> 
> based on this. So there is initially no functional change.
> 
> 
> You can then change the implementation of _transfers() based on what
> the MDIO bus can do, plus the quirk for if a FUBAR microchip PHY has
> been found.

See above. Shouldn't it be mdiobus_...() then?

> Then change the implementation of _registers() based on the results of
> probing for the ID registers.

So this is where I cannot follow. Right now there is
(1) probing via bus scan
(2) probing via DT (or maybe also ACPI)

With (1) you we have scan_c22(), so if successful, 
phy_has_c22_registers()
will return true, right? But it's not that clear for
phy_has_c45_registers(), because sometimes we prevent that scan. So
the PHY might have c45 but we don't know.

For (2) we don't even do a c22 scan if we know if its a C45 PHY (or the
other way around). I'm not sure we can reliably tell (at the end of this
series) if a phy has c22 register, c45 registers or both.

-michael

> That should give us a basis for a clean separation between register
> spaces and bus transaction, and then adding C45 over C22 should be
> more obviously correct.
> 
> 	Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd()
  2023-07-18 23:54   ` Andrew Lunn
@ 2023-07-19  7:21     ` Michael Walle
  0 siblings, 0 replies; 47+ messages in thread
From: Michael Walle @ 2023-07-19  7:21 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

Am 2023-07-19 01:54, schrieb Andrew Lunn:
>> +static int __phy_mdiobus_read_mmd(struct mii_bus *bus, int phy_addr,
>> +				  enum phy_access_mode access_mode,
>> +				  int devad, u32 regnum)
>> +{
>> +	switch (access_mode) {
>> +	case PHY_ACCESS_C45:
>> +		return __mdiobus_c45_read(bus, phy_addr, devad, regnum);
>> +	case PHY_ACCESS_C22:
>> +		/* ignore return value for legacy reasons */
>> +		mmd_phy_indirect(bus, phy_addr, devad, regnum, false);
>> +
>> +		/* Read the content of the MMD's selected register */
>> +		return __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
> 
> So this is reading a C45 register space register, otherwise it would
> not be called _mmd and have a devad. So access_mode should really be
> transfer mode. Until now, only transfer mode C45 can be used to access
> C45 register space. The point of this patchset is to add a new
> C45_OVER_C22 transfer mode.

So you suggest to rename access_mode to transfer_mode, right?

> And C22 would should give -EINVAL, since you cannot use plain C22 bus
> transactions to access C45 register space.

We had indirect mmd access before with c22 PHYs before, we could
theoretically fold that into c45-over-c22, but the old indirect
mmd access wasn't checking for error codes and according to Russell
we cannot change that. Honestly, I'd just duplicate the code and
leave the old non-error checking code in __phy_read_mmd() while
__phy_mdiobus_read_mmd() will do error checking and return -EINVAL
with PHY_ACCESS_C22. I had that in one of the first versions but
you suggested to not copy the code :), then this ugly check_rc
thing came. Or __phy_mdiobus_(read,write)_mmd() will have a
check_rc parameter.

-michael

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 10/11] net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy()
  2023-07-19  0:03   ` Andrew Lunn
@ 2023-07-19  7:32     ` Michael Walle
  0 siblings, 0 replies; 47+ messages in thread
From: Michael Walle @ 2023-07-19  7:32 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

Am 2023-07-19 02:03, schrieb Andrew Lunn:
>> Please note, that both with the old and the new code compatible =
>> "ethernet-phy-idNNNN.NNNN" only works for the C22 case. I'm wondering 
>> if
>> compatible = "ethernet-phy-idNNNN.NNNN", "ethernet-phy-ieee802.3-c45
>> even makes sense because there might be multiple C45 ids. At least it 
>> is
>> an allowed pattern according to the device tree bindings. But with the
>> current code, the ethernet-phy-idNNNN.NNNN is ignored in the c45 case.
> 
> I think these two should be orthogonal.
> 
> ethernet-phy-idNNNN.NNNN should be used to load the driver. The driver
> has a list of IDs it can drive, and we want the module loading
> machinery to load a module which can driver this ID.

See [1]. It is used to overwrite the PHY ID. Which I think works
in the c22 case.

> "ethernet-phy-ieee802.3-c45" should be about how to read the ID
> registers, if ethernet-phy-idNNNN.NNNN is not present.

And if it's present? See [2].

-michael

[1] 
https://elixir.bootlin.com/linux/v6.4.3/source/Documentation/devicetree/bindings/net/ethernet-phy.yaml#L38
[2] 
https://elixir.bootlin.com/linux/v6.4.3/source/Documentation/devicetree/bindings/net/ethernet-phy.yaml#L50

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-07-19  7:11     ` Michael Walle
@ 2023-08-01 14:47       ` Michael Walle
  2023-08-01 14:57         ` Russell King (Oracle)
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-08-01 14:47 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Yisen Zhuang,
	Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang,
	netdev, linux-kernel, Simon Horman

Hi Andrew,

Am 2023-07-19 09:11, schrieb Michael Walle:
>>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>>> index a64186dc53f8..686a57d56885 100644
>>> --- a/drivers/net/phy/phy-core.c
>>> +++ b/drivers/net/phy/phy-core.c
>>> @@ -556,7 +556,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 (phy_has_c45_registers(phydev)) {
>> 
>> This i would say should be
>> 
>> phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
>> on the bus, and if not, fall back to C45 over C22.
> 
> Shouldn't this then be a bus property? I.e. 
> mdiobus_has_c45_transfers().
> I've have a similar helper introduced in 9/11:
> 
> static inline bool mdiobus_supports_c45(struct mii_bus *bus)
> {
>     return bus->read_c45 && !bus->prevent_c45_access;
> }
> 
>>>  static int phylink_sfp_connect_phy(void *upstream, struct phy_device 
>>> *phy)
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index 11c1e91563d4..fdb3774e99fc 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -766,6 +766,11 @@ static inline struct phy_device 
>>> *to_phy_device(const struct device *dev)
>>>  	return container_of(to_mdio_device(dev), struct phy_device, mdio);
>>>  }
>>> 
>>> +static inline bool phy_has_c45_registers(struct phy_device *phydev)
>>> +{
>>> +	return phydev->is_c45;
>>> +}
>> 
>> And this is where it gets interesting. I think as a first step, you
>> should implement the four functions:
>> 
>> phy_has_c22_registers()
>> phy_has_c45_registers()
>> phy_has_c22_transfers()
>> phy_has_c45_transfers()
>> 
>> based on this. So there is initially no functional change.
>> 
>> 
>> You can then change the implementation of _transfers() based on what
>> the MDIO bus can do, plus the quirk for if a FUBAR microchip PHY has
>> been found.
> 
> See above. Shouldn't it be mdiobus_...() then?
> 
>> Then change the implementation of _registers() based on the results of
>> probing for the ID registers.
> 
> So this is where I cannot follow. Right now there is
> (1) probing via bus scan
> (2) probing via DT (or maybe also ACPI)
> 
> With (1) you we have scan_c22(), so if successful, 
> phy_has_c22_registers()
> will return true, right? But it's not that clear for
> phy_has_c45_registers(), because sometimes we prevent that scan. So
> the PHY might have c45 but we don't know.
> 
> For (2) we don't even do a c22 scan if we know if its a C45 PHY (or the
> other way around). I'm not sure we can reliably tell (at the end of 
> this
> series) if a phy has c22 register, c45 registers or both.

Any news here?

-michael

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-01 14:47       ` Michael Walle
@ 2023-08-01 14:57         ` Russell King (Oracle)
  2023-08-01 15:20           ` Michael Walle
  0 siblings, 1 reply; 47+ messages in thread
From: Russell King (Oracle) @ 2023-08-01 14:57 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Tue, Aug 01, 2023 at 04:47:04PM +0200, Michael Walle wrote:
> Hi Andrew,
> 
> Am 2023-07-19 09:11, schrieb Michael Walle:
> > > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> > > > index a64186dc53f8..686a57d56885 100644
> > > > --- a/drivers/net/phy/phy-core.c
> > > > +++ b/drivers/net/phy/phy-core.c
> > > > @@ -556,7 +556,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 (phy_has_c45_registers(phydev)) {
> > > 
> > > This i would say should be
> > > 
> > > phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
> > > on the bus, and if not, fall back to C45 over C22.
> > 
> > Shouldn't this then be a bus property? I.e. mdiobus_has_c45_transfers().
> > I've have a similar helper introduced in 9/11:
> > 
> > static inline bool mdiobus_supports_c45(struct mii_bus *bus)
> > {
> >     return bus->read_c45 && !bus->prevent_c45_access;
> > }

In the case of the above (the code in __phy_read_mmd()), I wouldn't
at least initially change the test there.

phydev->is_c45 will only be true if we probed the PHY using clause
45 accesses. Thus, it will be set if "the bus supports clause 45
accesses" _and_ "the PHY responds to those accesses".

Changing that to only "the bus supports clause 45 accesses" means
that a PHY supporting only clause 22 access with indirect clause
45 access then fails if it's used with a bus that supports both
clause 22 and clause 45 accesses.

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-01 14:57         ` Russell King (Oracle)
@ 2023-08-01 15:20           ` Michael Walle
  2023-08-01 15:57             ` Russell King (Oracle)
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-08-01 15:20 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

Hi,

>> > > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> > > > index a64186dc53f8..686a57d56885 100644
>> > > > --- a/drivers/net/phy/phy-core.c
>> > > > +++ b/drivers/net/phy/phy-core.c
>> > > > @@ -556,7 +556,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 (phy_has_c45_registers(phydev)) {
>> > >
>> > > This i would say should be
>> > >
>> > > phy_has_c45_transfers(phydev). This is about, can we do C45 transfers
>> > > on the bus, and if not, fall back to C45 over C22.
>> >
>> > Shouldn't this then be a bus property? I.e. mdiobus_has_c45_transfers().
>> > I've have a similar helper introduced in 9/11:
>> >
>> > static inline bool mdiobus_supports_c45(struct mii_bus *bus)
>> > {
>> >     return bus->read_c45 && !bus->prevent_c45_access;
>> > }
> 
> In the case of the above (the code in __phy_read_mmd()), I wouldn't
> at least initially change the test there.
> 
> phydev->is_c45 will only be true if we probed the PHY using clause
> 45 accesses. Thus, it will be set if "the bus supports clause 45
> accesses" _and_ "the PHY responds to those accesses".
> 
> Changing that to only "the bus supports clause 45 accesses" means
> that a PHY supporting only clause 22 access with indirect clause
> 45 access then fails if it's used with a bus that supports both
> clause 22 and clause 45 accesses.

Yeah of course. It was more about the naming, but I just realized
that with mdiobus_supports_c45() you can't access the original
"is_c45" property of the PHY. So maybe this patch needs to be split
into two to get rid of .is_c45:

First a mechanical one:
phy_has_c45_registers() {
    return phydev->is_c45;
}

phy_has_c22_registers() {
   return !phydev->is_c45;
}

For all the places Andrew said it's correct. Leave all the
other uses of .is_c45 as is for now and rework them in a
later patch to use mdiobus_supports_{c22,c45}().

-michael

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-01 15:20           ` Michael Walle
@ 2023-08-01 15:57             ` Russell King (Oracle)
  2023-08-02 15:33               ` Michael Walle
  0 siblings, 1 reply; 47+ messages in thread
From: Russell King (Oracle) @ 2023-08-01 15:57 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
> > In the case of the above (the code in __phy_read_mmd()), I wouldn't
> > at least initially change the test there.
> > 
> > phydev->is_c45 will only be true if we probed the PHY using clause
> > 45 accesses. Thus, it will be set if "the bus supports clause 45
> > accesses" _and_ "the PHY responds to those accesses".
> > 
> > Changing that to only "the bus supports clause 45 accesses" means
> > that a PHY supporting only clause 22 access with indirect clause
> > 45 access then fails if it's used with a bus that supports both
> > clause 22 and clause 45 accesses.
> 
> Yeah of course. It was more about the naming, but I just realized
> that with mdiobus_supports_c45() you can't access the original
> "is_c45" property of the PHY. So maybe this patch needs to be split
> into two to get rid of .is_c45:
> 
> First a mechanical one:
> phy_has_c45_registers() {
>    return phydev->is_c45;
> }

Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
suggested "_transfers" instead - because a PHY can have C45 registers
that are accessible via the indirect registers in C22 space.

I'd go one further:

static bool phy_supports_c45_transfers(struct phy_device *phydev)
{
	return phydev->is_c45;
}

Since that covers that (a) the bus needs to support C45 transfers and
(b) the PHY also needs to respond to C45 transfers.

If we want to truly know whether a clause 22 PHY has clause 45
registers, that's difficult to answer, because then you're into the
realms of "does this PHY implement the indirect access method" and
we haven't been keeping track of that for the PHYs we have drivers
for - many will do, but it's optional in clause 22. The problem is
that when it's not implemented, the registers could be serving some
other function.

> phy_has_c22_registers() {
>   return !phydev->is_c45;
> }

The reverse is not true, as clause 45 PHYs can also support clause 22
registers - from 802.3:

 "For cases where a single entity combines Clause 45 MMDs with  Clause 22
 registers, then the Clause 22 registers may be accessed using the Clause
 45 electrical interface and the Clause 22 management frame structure."

 "Bit 5.0 is used to indicate that Clause 22 functionality has been
 implemented within a Clause 45 electrical interface device."

Therefore, this would more accurately describe when Clause 22 registers
are present for a PHY:

static bool phy_has_c22_registers(struct phy_device *phydev)
{
	/* If we probed the PHY without clause 45 accesses, then by
	 * definition, clause 22 registers must be present.
	 */
	if (!phydev->is_c45)
		return true;

	/* If we probed the PHY with clause 45 accesses, clause 22
	 * registers may be present if bit 0 in the Devices-in-pacakge
	 * register pair is set.
	 */
	return phydev->c45_ids.devices_in_package & BIT(0);
}

Note that this doesn't take account of whether the bus supports clause
22 register access - there are a number of MDIO buses that do not
support such accesses, and they may be coupled with a PHY that does
support clause 22 registers.

I'm aware of a SFP with a Realtek PHY on that falls into this exact
case, and getting that working is progressing at the moment.

> For all the places Andrew said it's correct. Leave all the
> other uses of .is_c45 as is for now and rework them in a
> later patch to use mdiobus_supports_{c22,c45}().

For the two cases in marvell10g and bcm84881, the test there for
is_c45 is purely to determine "was this ID found on a PHY supporting
clause 45 access" - however, in both cases, a check is made for MMDs
present in devices_in_package which will fail if the PHY wasn't
discovered in clause 45 mode.

Note that 88x3310 does not support clause 22 access. I forget whether
bcm84881 does or not.

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-01 15:57             ` Russell King (Oracle)
@ 2023-08-02 15:33               ` Michael Walle
  2023-08-02 16:06                 ` Russell King (Oracle)
  2023-08-02 16:15                 ` Andrew Lunn
  0 siblings, 2 replies; 47+ messages in thread
From: Michael Walle @ 2023-08-02 15:33 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

Am 2023-08-01 17:57, schrieb Russell King (Oracle):
> On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
>> > In the case of the above (the code in __phy_read_mmd()), I wouldn't
>> > at least initially change the test there.
>> >
>> > phydev->is_c45 will only be true if we probed the PHY using clause
>> > 45 accesses. Thus, it will be set if "the bus supports clause 45
>> > accesses" _and_ "the PHY responds to those accesses".
>> >
>> > Changing that to only "the bus supports clause 45 accesses" means
>> > that a PHY supporting only clause 22 access with indirect clause
>> > 45 access then fails if it's used with a bus that supports both
>> > clause 22 and clause 45 accesses.
>> 
>> Yeah of course. It was more about the naming, but I just realized
>> that with mdiobus_supports_c45() you can't access the original
>> "is_c45" property of the PHY. So maybe this patch needs to be split
>> into two to get rid of .is_c45:
>> 
>> First a mechanical one:
>> phy_has_c45_registers() {
>>    return phydev->is_c45;
>> }
> 
> Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
> suggested "_transfers" instead - because a PHY can have C45 registers
> that are accessible via the indirect registers in C22 space.

I'm confused now. Andrew suggested to split it into four different
functions:

phy_has_c22_registers()
phy_has_c45_registers()
phy_has_c22_transfers()
phy_has_c45_transfers()

Without a functional change. That is, either return phydev->is_c45
or the inverse.

You seem to suggest to use either
phy_supports_c45_transfers() or
phy_has_c22_registers()

I'm not sure how to continue now.

> I'd go one further:
> 
> static bool phy_supports_c45_transfers(struct phy_device *phydev)
> {
> 	return phydev->is_c45;
> }
> 
> Since that covers that (a) the bus needs to support C45 transfers and
> (b) the PHY also needs to respond to C45 transfers.
> 
> If we want to truly know whether a clause 22 PHY has clause 45
> registers, that's difficult to answer, because then you're into the
> realms of "does this PHY implement the indirect access method" and
> we haven't been keeping track of that for the PHYs we have drivers
> for - many will do, but it's optional in clause 22. The problem is
> that when it's not implemented, the registers could be serving some
> other function.
> 
>> phy_has_c22_registers() {
>>   return !phydev->is_c45;
>> }
> 
> The reverse is not true, as clause 45 PHYs can also support clause 22
> registers - from 802.3:
> 
>  "For cases where a single entity combines Clause 45 MMDs with  Clause 
> 22
>  registers, then the Clause 22 registers may be accessed using the 
> Clause
>  45 electrical interface and the Clause 22 management frame structure."
> 
>  "Bit 5.0 is used to indicate that Clause 22 functionality has been
>  implemented within a Clause 45 electrical interface device."
> 
> Therefore, this would more accurately describe when Clause 22 registers
> are present for a PHY:
> 
> static bool phy_has_c22_registers(struct phy_device *phydev)
> {
> 	/* If we probed the PHY without clause 45 accesses, then by
> 	 * definition, clause 22 registers must be present.
> 	 */
> 	if (!phydev->is_c45)
> 		return true;
> 
> 	/* If we probed the PHY with clause 45 accesses, clause 22
> 	 * registers may be present if bit 0 in the Devices-in-pacakge
> 	 * register pair is set.
> 	 */
> 	return phydev->c45_ids.devices_in_package & BIT(0);
> }
> 
> Note that this doesn't take account of whether the bus supports clause
> 22 register access - there are a number of MDIO buses that do not
> support such accesses, and they may be coupled with a PHY that does
> support clause 22 registers.
> 
> I'm aware of a SFP with a Realtek PHY on that falls into this exact
> case, and getting that working is progressing at the moment.
> 
>> For all the places Andrew said it's correct. Leave all the
>> other uses of .is_c45 as is for now and rework them in a
>> later patch to use mdiobus_supports_{c22,c45}().
> 
> For the two cases in marvell10g and bcm84881, the test there for
> is_c45 is purely to determine "was this ID found on a PHY supporting
> clause 45 access" - however, in both cases, a check is made for MMDs
> present in devices_in_package which will fail if the PHY wasn't
> discovered in clause 45 mode.
> 
> Note that 88x3310 does not support clause 22 access. I forget whether
> bcm84881 does or not.

So a simple "phydev->is_c45" should be enough? Why do you test
for the MMD presence bits?

-michael

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 15:33               ` Michael Walle
@ 2023-08-02 16:06                 ` Russell King (Oracle)
  2023-08-02 17:11                   ` Michael Walle
  2023-08-02 16:15                 ` Andrew Lunn
  1 sibling, 1 reply; 47+ messages in thread
From: Russell King (Oracle) @ 2023-08-02 16:06 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Aug 02, 2023 at 05:33:20PM +0200, Michael Walle wrote:
> Am 2023-08-01 17:57, schrieb Russell King (Oracle):
> > On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
> > > > In the case of the above (the code in __phy_read_mmd()), I wouldn't
> > > > at least initially change the test there.
> > > >
> > > > phydev->is_c45 will only be true if we probed the PHY using clause
> > > > 45 accesses. Thus, it will be set if "the bus supports clause 45
> > > > accesses" _and_ "the PHY responds to those accesses".
> > > >
> > > > Changing that to only "the bus supports clause 45 accesses" means
> > > > that a PHY supporting only clause 22 access with indirect clause
> > > > 45 access then fails if it's used with a bus that supports both
> > > > clause 22 and clause 45 accesses.
> > > 
> > > Yeah of course. It was more about the naming, but I just realized
> > > that with mdiobus_supports_c45() you can't access the original
> > > "is_c45" property of the PHY. So maybe this patch needs to be split
> > > into two to get rid of .is_c45:
> > > 
> > > First a mechanical one:
> > > phy_has_c45_registers() {
> > >    return phydev->is_c45;
> > > }
> > 
> > Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
> > suggested "_transfers" instead - because a PHY can have C45 registers
> > that are accessible via the indirect registers in C22 space.
> 
> I'm confused now. Andrew suggested to split it into four different
> functions:
> 
> phy_has_c22_registers()
> phy_has_c45_registers()
> phy_has_c22_transfers()
> phy_has_c45_transfers()

Honestly, I don't think we can come up with tests that satisfy all of
these. Particularly the question whether a PHY has c45 registers or
not is a difficult one, as there is no sane way to determine that with
a clause 22 PHY.

I'm also not sure what use the c22 transfers one would be, since if a
PHY doesn't have c22 registers, then that's probably all we need to
know.

> Without a functional change. That is, either return phydev->is_c45
> or the inverse.

I think I've already explained why !phydev->is_c45 can't be interpeted
as a PHY having C22 registers, but let me restate. It is _entirely_
possible for a PHY to have C45 registers _and_ C22 registers, and
that is indicated by bit 0 of the devices-in-package field.

> 
> You seem to suggest to use either
> phy_supports_c45_transfers() or
> phy_has_c22_registers()
> 
> I'm not sure how to continue now.
> 
> > I'd go one further:
> > 
> > static bool phy_supports_c45_transfers(struct phy_device *phydev)
> > {
> > 	return phydev->is_c45;
> > }
> > 
> > Since that covers that (a) the bus needs to support C45 transfers and
> > (b) the PHY also needs to respond to C45 transfers.
> > 
> > If we want to truly know whether a clause 22 PHY has clause 45
> > registers, that's difficult to answer, because then you're into the
> > realms of "does this PHY implement the indirect access method" and
> > we haven't been keeping track of that for the PHYs we have drivers
> > for - many will do, but it's optional in clause 22. The problem is
> > that when it's not implemented, the registers could be serving some
> > other function.
> > 
> > > phy_has_c22_registers() {
> > >   return !phydev->is_c45;
> > > }
> > 
> > The reverse is not true, as clause 45 PHYs can also support clause 22
> > registers - from 802.3:
> > 
> >  "For cases where a single entity combines Clause 45 MMDs with  Clause
> > 22
> >  registers, then the Clause 22 registers may be accessed using the
> > Clause
> >  45 electrical interface and the Clause 22 management frame structure."
> > 
> >  "Bit 5.0 is used to indicate that Clause 22 functionality has been
> >  implemented within a Clause 45 electrical interface device."
> > 
> > Therefore, this would more accurately describe when Clause 22 registers
> > are present for a PHY:
> > 
> > static bool phy_has_c22_registers(struct phy_device *phydev)
> > {
> > 	/* If we probed the PHY without clause 45 accesses, then by
> > 	 * definition, clause 22 registers must be present.
> > 	 */
> > 	if (!phydev->is_c45)
> > 		return true;
> > 
> > 	/* If we probed the PHY with clause 45 accesses, clause 22
> > 	 * registers may be present if bit 0 in the Devices-in-pacakge
> > 	 * register pair is set.
> > 	 */
> > 	return phydev->c45_ids.devices_in_package & BIT(0);
> > }
> > 
> > Note that this doesn't take account of whether the bus supports clause
> > 22 register access - there are a number of MDIO buses that do not
> > support such accesses, and they may be coupled with a PHY that does
> > support clause 22 registers.
> > 
> > I'm aware of a SFP with a Realtek PHY on that falls into this exact
> > case, and getting that working is progressing at the moment.
> > 
> > > For all the places Andrew said it's correct. Leave all the
> > > other uses of .is_c45 as is for now and rework them in a
> > > later patch to use mdiobus_supports_{c22,c45}().
> > 
> > For the two cases in marvell10g and bcm84881, the test there for
> > is_c45 is purely to determine "was this ID found on a PHY supporting
> > clause 45 access" - however, in both cases, a check is made for MMDs
> > present in devices_in_package which will fail if the PHY wasn't
> > discovered in clause 45 mode.
> > 
> > Note that 88x3310 does not support clause 22 access. I forget whether
> > bcm84881 does or not.
> 
> So a simple "phydev->is_c45" should be enough? Why do you test
> for the MMD presence bits?

Okay, so if quoting the bits from IEEE 802.3 doesn't provide sufficient
explanation, I'm at a loss what would...

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 15:33               ` Michael Walle
  2023-08-02 16:06                 ` Russell King (Oracle)
@ 2023-08-02 16:15                 ` Andrew Lunn
  2023-08-02 17:10                   ` Russell King (Oracle)
  1 sibling, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-08-02 16:15 UTC (permalink / raw)
  To: Michael Walle
  Cc: Russell King (Oracle),
	Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Yisen Zhuang, Salil Mehta, Florian Fainelli,
	Broadcom internal kernel review list, Marek Behún, Xu Liang,
	netdev, linux-kernel, Simon Horman

> I'm confused now. Andrew suggested to split it into four different
> functions:
> 
> phy_has_c22_registers()
> phy_has_c45_registers()
> phy_has_c22_transfers()
> phy_has_c45_transfers()
> 
> Without a functional change. That is, either return phydev->is_c45
> or the inverse.

Without a functional change at this step of introducing the four
functions. Then later really implement them to do what the name
implies. Doing it in steps helps with bisect when it breaks something.

It could also be that not all four are needed, or not all four are
possible. But the four express the full combinations of transfers and
registers.

	Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 16:15                 ` Andrew Lunn
@ 2023-08-02 17:10                   ` Russell King (Oracle)
  2023-08-02 22:21                     ` Andrew Lunn
  0 siblings, 1 reply; 47+ messages in thread
From: Russell King (Oracle) @ 2023-08-02 17:10 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michael Walle, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote:
> > I'm confused now. Andrew suggested to split it into four different
> > functions:
> > 
> > phy_has_c22_registers()
> > phy_has_c45_registers()
> > phy_has_c22_transfers()
> > phy_has_c45_transfers()
> > 
> > Without a functional change. That is, either return phydev->is_c45
> > or the inverse.
> 
> Without a functional change at this step of introducing the four
> functions. Then later really implement them to do what the name
> implies. Doing it in steps helps with bisect when it breaks something.
> 
> It could also be that not all four are needed, or not all four are
> possible. But the four express the full combinations of transfers and
> registers.

I'm left wondering how you think that phy_has_c45_registers() can
be implemented in a useful way, because I'm at a total loss...

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 16:06                 ` Russell King (Oracle)
@ 2023-08-02 17:11                   ` Michael Walle
  2023-08-02 23:00                     ` Russell King (Oracle)
  0 siblings, 1 reply; 47+ messages in thread
From: Michael Walle @ 2023-08-02 17:11 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

Am 2023-08-02 18:06, schrieb Russell King (Oracle):
> On Wed, Aug 02, 2023 at 05:33:20PM +0200, Michael Walle wrote:
>> Am 2023-08-01 17:57, schrieb Russell King (Oracle):
>> > On Tue, Aug 01, 2023 at 05:20:22PM +0200, Michael Walle wrote:
>> > > > In the case of the above (the code in __phy_read_mmd()), I wouldn't
>> > > > at least initially change the test there.
>> > > >
>> > > > phydev->is_c45 will only be true if we probed the PHY using clause
>> > > > 45 accesses. Thus, it will be set if "the bus supports clause 45
>> > > > accesses" _and_ "the PHY responds to those accesses".
>> > > >
>> > > > Changing that to only "the bus supports clause 45 accesses" means
>> > > > that a PHY supporting only clause 22 access with indirect clause
>> > > > 45 access then fails if it's used with a bus that supports both
>> > > > clause 22 and clause 45 accesses.
>> > >
>> > > Yeah of course. It was more about the naming, but I just realized
>> > > that with mdiobus_supports_c45() you can't access the original
>> > > "is_c45" property of the PHY. So maybe this patch needs to be split
>> > > into two to get rid of .is_c45:
>> > >
>> > > First a mechanical one:
>> > > phy_has_c45_registers() {
>> > >    return phydev->is_c45;
>> > > }
>> >
>> > Andrew's objection was that "phy_has_c45_registers" is a misnomer, and
>> > suggested "_transfers" instead - because a PHY can have C45 registers
>> > that are accessible via the indirect registers in C22 space.
>> 
>> I'm confused now. Andrew suggested to split it into four different
>> functions:
>> 
>> phy_has_c22_registers()
>> phy_has_c45_registers()
>> phy_has_c22_transfers()
>> phy_has_c45_transfers()
> 
> Honestly, I don't think we can come up with tests that satisfy all of
> these. Particularly the question whether a PHY has c45 registers or
> not is a difficult one, as there is no sane way to determine that with
> a clause 22 PHY.

And that is exactly the one which Andrew suggested to use for almost all
cases. It doesn't make sense to me to introduce that function if it 
can't
really be made to return a correct value (in a later patch).

> I'm also not sure what use the c22 transfers one would be, since if a
> PHY doesn't have c22 registers, then that's probably all we need to
> know.
> 
>> Without a functional change. That is, either return phydev->is_c45
>> or the inverse.
> 
> I think I've already explained why !phydev->is_c45 can't be interpeted
> as a PHY having C22 registers, but let me restate. It is _entirely_
> possible for a PHY to have C45 registers _and_ C22 registers, and
> that is indicated by bit 0 of the devices-in-package field.
> 
>> You seem to suggest to use either
>> phy_supports_c45_transfers() or
>> phy_has_c22_registers()
>> 
>> I'm not sure how to continue now.
>> 
>> > I'd go one further:
>> >
>> > static bool phy_supports_c45_transfers(struct phy_device *phydev)
>> > {
>> > 	return phydev->is_c45;
>> > }
>> >
>> > Since that covers that (a) the bus needs to support C45 transfers and
>> > (b) the PHY also needs to respond to C45 transfers.
>> >
>> > If we want to truly know whether a clause 22 PHY has clause 45
>> > registers, that's difficult to answer, because then you're into the
>> > realms of "does this PHY implement the indirect access method" and
>> > we haven't been keeping track of that for the PHYs we have drivers
>> > for - many will do, but it's optional in clause 22. The problem is
>> > that when it's not implemented, the registers could be serving some
>> > other function.
>> >
>> > > phy_has_c22_registers() {
>> > >   return !phydev->is_c45;
>> > > }
>> >
>> > The reverse is not true, as clause 45 PHYs can also support clause 22
>> > registers - from 802.3:
>> >
>> >  "For cases where a single entity combines Clause 45 MMDs with  Clause
>> > 22
>> >  registers, then the Clause 22 registers may be accessed using the
>> > Clause
>> >  45 electrical interface and the Clause 22 management frame structure."
>> >
>> >  "Bit 5.0 is used to indicate that Clause 22 functionality has been
>> >  implemented within a Clause 45 electrical interface device."
>> >
>> > Therefore, this would more accurately describe when Clause 22 registers
>> > are present for a PHY:
>> >
>> > static bool phy_has_c22_registers(struct phy_device *phydev)
>> > {
>> > 	/* If we probed the PHY without clause 45 accesses, then by
>> > 	 * definition, clause 22 registers must be present.
>> > 	 */
>> > 	if (!phydev->is_c45)
>> > 		return true;
>> >
>> > 	/* If we probed the PHY with clause 45 accesses, clause 22
>> > 	 * registers may be present if bit 0 in the Devices-in-pacakge
>> > 	 * register pair is set.
>> > 	 */
>> > 	return phydev->c45_ids.devices_in_package & BIT(0);
>> > }
>> >
>> > Note that this doesn't take account of whether the bus supports clause
>> > 22 register access - there are a number of MDIO buses that do not
>> > support such accesses, and they may be coupled with a PHY that does
>> > support clause 22 registers.
>> >
>> > I'm aware of a SFP with a Realtek PHY on that falls into this exact
>> > case, and getting that working is progressing at the moment.
>> >
>> > > For all the places Andrew said it's correct. Leave all the
>> > > other uses of .is_c45 as is for now and rework them in a
>> > > later patch to use mdiobus_supports_{c22,c45}().
>> >
>> > For the two cases in marvell10g and bcm84881, the test there for
>> > is_c45 is purely to determine "was this ID found on a PHY supporting
>> > clause 45 access" - however, in both cases, a check is made for MMDs
>> > present in devices_in_package which will fail if the PHY wasn't
>> > discovered in clause 45 mode.
>> >
>> > Note that 88x3310 does not support clause 22 access. I forget whether
>> > bcm84881 does or not.
>> 
>> So a simple "phydev->is_c45" should be enough? Why do you test
>> for the MMD presence bits?
> 
> Okay, so if quoting the bits from IEEE 802.3 doesn't provide sufficient
> explanation, I'm at a loss what would...

I'm talking about

	u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
	if (!phydev->is_c45 ||
	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
		return -ENODEV;

How should that look like after this series?

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 17:10                   ` Russell King (Oracle)
@ 2023-08-02 22:21                     ` Andrew Lunn
  2023-08-02 22:28                       ` Russell King (Oracle)
  0 siblings, 1 reply; 47+ messages in thread
From: Andrew Lunn @ 2023-08-02 22:21 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Michael Walle, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Aug 02, 2023 at 06:10:27PM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote:
> > > I'm confused now. Andrew suggested to split it into four different
> > > functions:
> > > 
> > > phy_has_c22_registers()
> > > phy_has_c45_registers()
> > > phy_has_c22_transfers()
> > > phy_has_c45_transfers()
> > > 
> > > Without a functional change. That is, either return phydev->is_c45
> > > or the inverse.
> > 
> > Without a functional change at this step of introducing the four
> > functions. Then later really implement them to do what the name
> > implies. Doing it in steps helps with bisect when it breaks something.
> > 
> > It could also be that not all four are needed, or not all four are
> > possible. But the four express the full combinations of transfers and
> > registers.
> 
> I'm left wondering how you think that phy_has_c45_registers() can
> be implemented in a useful way, because I'm at a total loss...

get_phy_c22_id() looks at the ID registers, and see if its mostly
Fs. If so, it says -ENODEV. So phy_has_c22_registers() could be
get_phy_c22_id() != -ENODEV. And you can extend that with the logic
you quoted from the standard. get_phy_c45_ids() also returns -ENODEV
if there are no devices in the package. So phy_has_c45_registers()
could be get_phy_c45_id() != -ENODEV.

This is heuristics, not something explicitly from the standard, but
the basic idea of looking for not mostly Fs has been used for a long
time.

    Andrew

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 22:21                     ` Andrew Lunn
@ 2023-08-02 22:28                       ` Russell King (Oracle)
  2023-09-05  8:22                         ` Michael Walle
  0 siblings, 1 reply; 47+ messages in thread
From: Russell King (Oracle) @ 2023-08-02 22:28 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michael Walle, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Thu, Aug 03, 2023 at 12:21:03AM +0200, Andrew Lunn wrote:
> On Wed, Aug 02, 2023 at 06:10:27PM +0100, Russell King (Oracle) wrote:
> > On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote:
> > > > I'm confused now. Andrew suggested to split it into four different
> > > > functions:
> > > > 
> > > > phy_has_c22_registers()
> > > > phy_has_c45_registers()
> > > > phy_has_c22_transfers()
> > > > phy_has_c45_transfers()
> > > > 
> > > > Without a functional change. That is, either return phydev->is_c45
> > > > or the inverse.
> > > 
> > > Without a functional change at this step of introducing the four
> > > functions. Then later really implement them to do what the name
> > > implies. Doing it in steps helps with bisect when it breaks something.
> > > 
> > > It could also be that not all four are needed, or not all four are
> > > possible. But the four express the full combinations of transfers and
> > > registers.
> > 
> > I'm left wondering how you think that phy_has_c45_registers() can
> > be implemented in a useful way, because I'm at a total loss...
> 
> get_phy_c22_id() looks at the ID registers, and see if its mostly
> Fs. If so, it says -ENODEV. So phy_has_c22_registers() could be
> get_phy_c22_id() != -ENODEV. And you can extend that with the logic
> you quoted from the standard. get_phy_c45_ids() also returns -ENODEV
> if there are no devices in the package. So phy_has_c45_registers()
> could be get_phy_c45_id() != -ENODEV.

No, this is not correct, because get_phy_c45_id() uses mdiobus c45
reads - if the bus doesn't support c45 transfers, then get_phy_c45_id()
fails.

This doesn't tell us whether the PHY has clause 45 registers at all,
it just tells us that we probed the PHY successfully using clause 45
accesses.

The fact of the matter is that there is no way in the current phylib
code (and probably no way to probe it either) to determine whether a
PHY has clause 45 registers or not. To see why, take for example the
AR8035. This clearly has clause 45 registers, because it has MMDs 3
and 7 implemented. However, these are only accessible through the
indirect access method via clause 22 cycles.

So, does it make sense for AR8035 to return false from
phy_has_c45_registers() ? No it doesn't, because it clearly does have
clause 45 registers!

The same can be said about Marvell 88e151x PHYs. And probably many
other clause 22 PHYs.

Hence why I disagree with your suggestion.

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 17:11                   ` Michael Walle
@ 2023-08-02 23:00                     ` Russell King (Oracle)
  0 siblings, 0 replies; 47+ messages in thread
From: Russell King (Oracle) @ 2023-08-02 23:00 UTC (permalink / raw)
  To: Michael Walle
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

On Wed, Aug 02, 2023 at 07:11:27PM +0200, Michael Walle wrote:
> I'm talking about
> 
> 	u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
> 	if (!phydev->is_c45 ||
> 	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
> 		return -ENODEV;
> 
> How should that look like after this series?

In the case of the marvell10g driver, at least the 88x3310 can be
accessed via clause 45 bus cycles _or_ the clause 45 indirect
register access via clause 22. I'm not sure what the clause 22
registers would contain, whether they contain valid values with
the exception of the clause 45 indirect access registers because
there's not enough information in the documentation, and I can't
access the clause 22 registers on real hardware.

Another issue is this PHY has two different ID values. One is
0x002b 0x09aa, the other is 0x0141 0x0daa. One or other of these
values appears in the MMDs - in other words, they are not
consistently used. This means it's impossible to guess what value
may be in the clause 22 ID registers for this PHY.

However, given the way phylib works, the above effectively ensures
that we detected the PHY using clause 45 accesses, and then goes on
to verify that we do indeed have the PMAPMD MMD and the AN MMD.
Effectively, it ensures that get_phy_c45_ids() was used to detect
the device.

So, in effect, this code is ensuring that we discovered the PHY
using clause 45 accesses, and that at a minimum the PHY also
indicates that the PMAPMD and AN MMDs are implemented.

For the bcm84881 driver, it's a similar situation - that's only
ever been used with a bus that supports _only_ clause 45 accesses.
Even less idea whether the PHY could respond to clause 22 accesses
as there is no information available on the PHY.

So, I'd suggest both of these are the same - returns -ENODEV if
the bus doesn't support clause 45 transfers or if the two MMDs
are not indicated.

That said, it _can_ be simplified down to just testing the
devices_in_package member, because that will only be non-zero if
we successfully probed the PHY via some accessible way to the
clause 45 registers. So:

	if ((phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
		return -ENODEV;

is probably entirely sufficient in both cases.

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

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers()
  2023-08-02 22:28                       ` Russell King (Oracle)
@ 2023-09-05  8:22                         ` Michael Walle
  0 siblings, 0 replies; 47+ messages in thread
From: Michael Walle @ 2023-09-05  8:22 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Yisen Zhuang, Salil Mehta,
	Florian Fainelli, Broadcom internal kernel review list,
	Marek Behún, Xu Liang, netdev, linux-kernel, Simon Horman

Hi,

> Hence why I disagree with your suggestion.

So how can we make progress here? I tried Russells suggestion using
phy_supports_c45_transfers() and phy_has_c22_registers(). You can find
a possible v4 of this series on my github [1].

Some uses of .is_c45 seem to be to test whether the c45_ids are valid. 
E.g.
from the phy.h:
* @c45_ids: 802.3-c45 Device Identifiers if is_c45.

But if you test if a bit is set, you can skip that. For the opposite 
test
you'd need to have some kind of indication whether c45_ids was populated
in the first place.

If you convert all these uses of is_c45, there will only be a handful
of uses left. And replacing these by phy_supports_c45_transfers() sounds
sane to me except for for these two:
(1) drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
(2) drivers/net/phy/mxl-gpy.c

(2) will eventually replaced by my phy_promote_to_c45(). I might reorder
the patches, so I'll get rid of the .is_c45 use there before the
conversion to phy_supports_c45_transfers().

But honestly, for (1) I don't have any idea whats going on. If I look
at the very first commit, it seems that the is_c45 property is used
to distinguish loopback handling between a gigabit and a 10g PHY (?).

Btw, Russell, I've noticed that phy_restart_aneg() and phy_config_aneg()
will test for c22 registers, but phy_aneg_done() tests just for .is_c45.
Is that correct?

-michael

[1] https://github.com/mwalle/linux/tree/feature-c45-over-c22-v4
[2] 
https://git.kernel.org/torvalds/c/b5996f11ea5496d0445078f47d22c987888ed467

^ permalink raw reply	[flat|nested] 47+ messages in thread

end of thread, other threads:[~2023-09-05 17:07 UTC | newest]

Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-12 15:07 [PATCH net-next v3 00/11] net: phy: C45-over-C22 access Michael Walle
2023-07-12 15:07 ` [PATCH net-next v3 01/11] net: phy: get rid of redundant is_c45 information Michael Walle
2023-07-18 17:25   ` Andrew Lunn
2023-07-12 15:07 ` [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers() Michael Walle
2023-07-18 17:26   ` Andrew Lunn
2023-07-18 20:07   ` Andrew Lunn
2023-07-19  7:11     ` Michael Walle
2023-08-01 14:47       ` Michael Walle
2023-08-01 14:57         ` Russell King (Oracle)
2023-08-01 15:20           ` Michael Walle
2023-08-01 15:57             ` Russell King (Oracle)
2023-08-02 15:33               ` Michael Walle
2023-08-02 16:06                 ` Russell King (Oracle)
2023-08-02 17:11                   ` Michael Walle
2023-08-02 23:00                     ` Russell King (Oracle)
2023-08-02 16:15                 ` Andrew Lunn
2023-08-02 17:10                   ` Russell King (Oracle)
2023-08-02 22:21                     ` Andrew Lunn
2023-08-02 22:28                       ` Russell King (Oracle)
2023-09-05  8:22                         ` Michael Walle
2023-07-12 15:07 ` [PATCH net-next v3 03/11] net: phy: replace is_c45 with phy_accces_mode Michael Walle
2023-07-18 17:40   ` Andrew Lunn
2023-07-18 17:52     ` Russell King (Oracle)
2023-07-18 19:18       ` Andrew Lunn
2023-07-18 21:46         ` Russell King (Oracle)
2023-07-18 23:30           ` Andrew Lunn
2023-07-18 19:53     ` Michael Walle
2023-07-18 20:16       ` Andrew Lunn
2023-07-12 15:07 ` [PATCH net-next v3 04/11] net: phy: make the "prevent_c45_scan" a property of the MII bus Michael Walle
2023-07-18 23:31   ` Andrew Lunn
2023-07-12 15:07 ` [PATCH net-next v3 05/11] net: phy: print an info if a broken C45 bus is found Michael Walle
2023-07-18 23:32   ` Andrew Lunn
2023-07-12 15:07 ` [PATCH net-next v3 06/11] net: phy: add error checks in mmd_phy_indirect() Michael Walle
2023-07-18 23:34   ` Andrew Lunn
2023-07-12 15:07 ` [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd() Michael Walle
2023-07-18 23:54   ` Andrew Lunn
2023-07-19  7:21     ` Michael Walle
2023-07-12 15:07 ` [PATCH net-next v3 08/11] net: phy: add support for C45-over-C22 transfers Michael Walle
2023-07-13  8:56   ` Simon Horman
2023-07-13  9:00     ` Michael Walle
2023-07-13  9:19       ` Simon Horman
2023-07-12 15:07 ` [PATCH net-next v3 09/11] net: phy: introduce phy_promote_to_c45() Michael Walle
2023-07-13  8:56   ` Simon Horman
2023-07-12 15:07 ` [PATCH net-next v3 10/11] net: mdio: add C45-over-C22 fallback to fwnode_mdiobus_register_phy() Michael Walle
2023-07-19  0:03   ` Andrew Lunn
2023-07-19  7:32     ` Michael Walle
2023-07-12 15:07 ` [PATCH net-next v3 11/11] net: mdio: support C45-over-C22 when probed via OF Michael Walle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.