diff mbox series

[net-next,v3,07/11] net: phy: introduce phy_mdiobus_read_mmd()

Message ID 20230620-feature-c45-over-c22-v3-7-9eb37edf7be0@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: phy: C45-over-C22 access | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1744 this patch: 1744
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1643 this patch: 1643
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1753 this patch: 1753
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 144 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Michael Walle July 12, 2023, 3:07 p.m. UTC 
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(-)

Comments

Andrew Lunn July 18, 2023, 11:54 p.m. UTC | #1 
> +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
Michael Walle July 19, 2023, 7:21 a.m. UTC | #2 
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
diff mbox series

Patch

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.