diff mbox series

[net-next,v3,01/11] net: mdio: Add dedicated C45 API to MDIO bus drivers

Message ID 20221227-v6-2-rc1-c45-seperation-v3-1-ade1deb438da@walle.cc (mailing list archive)
State Accepted
Delegated to: Netdev Maintainers
Headers show
Series net: mdio: Start separating C22 and C45 | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 571 this patch: 571
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang success Errors and warnings before: 294 this patch: 294
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
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: 539 this patch: 539
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc fail Errors and warnings before: 0 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Michael Walle Jan. 9, 2023, 3:30 p.m. UTC 
From: Andrew Lunn <andrew@lunn.ch>

Currently C22 and C45 transactions are mixed over a combined API calls
which make use of a special bit in the reg address to indicate if a
C45 transaction should be performed. This makes it impossible to know
if the bus driver actually supports C45. Additionally, many C22 only
drivers don't return -EOPNOTSUPP when asked to perform a C45
transaction, they mistaking perform a C22 transaction.

This is the first step to cleanly separate C22 from C45. To maintain
backwards compatibility until all drivers which are capable of
performing C45 are converted to this new API, the helper functions
will fall back to the older API if the new API is not
supported. Eventually this fallback will be removed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/mdio_bus.c | 189 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h       |  39 +++++-----
 include/linux/phy.h        |   5 ++
 3 files changed, 214 insertions(+), 19 deletions(-)

Comments

Paolo Abeni Jan. 10, 2023, 11:16 a.m. UTC | #1 
On Mon, 2023-01-09 at 16:30 +0100, Michael Walle wrote:
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 6378c997ded5..65844f0a7fb3 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -364,6 +364,11 @@ struct mii_bus {
>  	int (*read)(struct mii_bus *bus, int addr, int regnum);
>  	/** @write: Perform a write transfer on the bus */
>  	int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
> +	/** @read: Perform a C45 read transfer on the bus */

Minor nit: s/@read/@read_c45/ 

> +	int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
> +	/** @write: Perform a C45 write transfer on the bus */

Mutatis mutandis, same thing here.

Thanks!

Paolo
Paolo Abeni Jan. 10, 2023, 12:49 p.m. UTC | #2 
On Tue, 2023-01-10 at 12:16 +0100, Paolo Abeni wrote:
> On Mon, 2023-01-09 at 16:30 +0100, Michael Walle wrote:
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 6378c997ded5..65844f0a7fb3 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -364,6 +364,11 @@ struct mii_bus {
> >  	int (*read)(struct mii_bus *bus, int addr, int regnum);
> >  	/** @write: Perform a write transfer on the bus */
> >  	int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
> > +	/** @read: Perform a C45 read transfer on the bus */
> 
> Minor nit: s/@read/@read_c45/ 
> 
> > +	int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
> > +	/** @write: Perform a C45 write transfer on the bus */
> 
> Mutatis mutandis, same thing here.

Otherwise the series LGTM. I think it would be preferable avoiding
reposting a largish series like this for such a minor change. I think
the series can be merged as-is and the above can be addressed with a
follow-up patch.

Cheers,

Paolo
Jakub Kicinski Jan. 10, 2023, 11:59 p.m. UTC | #3 
On Tue, 10 Jan 2023 13:49:28 +0100 Paolo Abeni wrote:
> On Tue, 2023-01-10 at 12:16 +0100, Paolo Abeni wrote:
> > On Mon, 2023-01-09 at 16:30 +0100, Michael Walle wrote:  
> > > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > > index 6378c997ded5..65844f0a7fb3 100644
> > > --- a/include/linux/phy.h
> > > +++ b/include/linux/phy.h
> > > @@ -364,6 +364,11 @@ struct mii_bus {
> > >  	int (*read)(struct mii_bus *bus, int addr, int regnum);
> > >  	/** @write: Perform a write transfer on the bus */
> > >  	int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
> > > +	/** @read: Perform a C45 read transfer on the bus */  
> > 
> > Minor nit: s/@read/@read_c45/ 
> >   
> > > +	int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
> > > +	/** @write: Perform a C45 write transfer on the bus */  
> > 
> > Mutatis mutandis, same thing here.  
> 
> Otherwise the series LGTM. I think it would be preferable avoiding
> reposting a largish series like this for such a minor change. I think
> the series can be merged as-is and the above can be addressed with a
> follow-up patch.

Hmmm if we're doing special treatment I guess can as well fix this 
when applying. Save people who report warnings potential work.
diff mbox series

Patch

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 1cd604cd1fa1..bde195864c17 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -826,6 +826,100 @@  int __mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum,
 }
 EXPORT_SYMBOL_GPL(__mdiobus_modify_changed);
 
+/**
+ * __mdiobus_c45_read - Unlocked version of the mdiobus_c45_read function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: device address to read
+ * @regnum: register number to read
+ *
+ * Read a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum)
+{
+	int retval;
+
+	lockdep_assert_held_once(&bus->mdio_lock);
+
+	if (bus->read_c45)
+		retval = bus->read_c45(bus, addr, devad, regnum);
+	else
+		retval = bus->read(bus, addr, mdiobus_c45_addr(devad, regnum));
+
+	trace_mdio_access(bus, 1, addr, regnum, retval, retval);
+	mdiobus_stats_acct(&bus->stats[addr], true, retval);
+
+	return retval;
+}
+EXPORT_SYMBOL(__mdiobus_c45_read);
+
+/**
+ * __mdiobus_c45_write - Unlocked version of the mdiobus_write function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: device address to read
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * Write a MDIO bus register. Caller must hold the mdio bus lock.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+int __mdiobus_c45_write(struct mii_bus *bus, int addr, int devad, u32 regnum,
+			u16 val)
+{
+	int err;
+
+	lockdep_assert_held_once(&bus->mdio_lock);
+
+	if (bus->write_c45)
+		err = bus->write_c45(bus, addr, devad, regnum, val);
+	else
+		err = bus->write(bus, addr, mdiobus_c45_addr(devad, regnum),
+				 val);
+
+	trace_mdio_access(bus, 0, addr, regnum, val, err);
+	mdiobus_stats_acct(&bus->stats[addr], false, err);
+
+	return err;
+}
+EXPORT_SYMBOL(__mdiobus_c45_write);
+
+/**
+ * __mdiobus_c45_modify_changed - Unlocked version of the mdiobus_modify function
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: device address to read
+ * @regnum: register number to modify
+ * @mask: bit mask of bits to clear
+ * @set: bit mask of bits to set
+ *
+ * Read, modify, and if any change, write the register value back to the
+ * device. Any error returns a negative number.
+ *
+ * NOTE: MUST NOT be called from interrupt context.
+ */
+static int __mdiobus_c45_modify_changed(struct mii_bus *bus, int addr,
+					int devad, u32 regnum, u16 mask,
+					u16 set)
+{
+	int new, ret;
+
+	ret = __mdiobus_c45_read(bus, addr, devad, regnum);
+	if (ret < 0)
+		return ret;
+
+	new = (ret & ~mask) | set;
+	if (new == ret)
+		return 0;
+
+	ret = __mdiobus_c45_write(bus, addr, devad, regnum, new);
+
+	return ret < 0 ? ret : 1;
+}
+
 /**
  * mdiobus_read_nested - Nested version of the mdiobus_read function
  * @bus: the mii_bus struct
@@ -873,6 +967,29 @@  int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
 }
 EXPORT_SYMBOL(mdiobus_read);
 
+/**
+ * mdiobus_c45_read - Convenience function for reading a given MII mgmt register
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: device address to read
+ * @regnum: register number to read
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum)
+{
+	int retval;
+
+	mutex_lock(&bus->mdio_lock);
+	retval = __mdiobus_c45_read(bus, addr, devad, regnum);
+	mutex_unlock(&bus->mdio_lock);
+
+	return retval;
+}
+EXPORT_SYMBOL(mdiobus_c45_read);
+
 /**
  * mdiobus_write_nested - Nested version of the mdiobus_write function
  * @bus: the mii_bus struct
@@ -922,6 +1039,31 @@  int mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 }
 EXPORT_SYMBOL(mdiobus_write);
 
+/**
+ * mdiobus_c45_write - Convenience function for writing a given MII mgmt register
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: device address to read
+ * @regnum: register number to write
+ * @val: value to write to @regnum
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+int mdiobus_c45_write(struct mii_bus *bus, int addr, int devad, u32 regnum,
+		      u16 val)
+{
+	int err;
+
+	mutex_lock(&bus->mdio_lock);
+	err = __mdiobus_c45_write(bus, addr, devad, regnum, val);
+	mutex_unlock(&bus->mdio_lock);
+
+	return err;
+}
+EXPORT_SYMBOL(mdiobus_c45_write);
+
 /**
  * mdiobus_modify - Convenience function for modifying a given mdio device
  *	register
@@ -943,6 +1085,30 @@  int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask, u16 set)
 }
 EXPORT_SYMBOL_GPL(mdiobus_modify);
 
+/**
+ * mdiobus_c45_modify - Convenience function for modifying a given mdio device
+ *	register
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: device address to read
+ * @regnum: register number to write
+ * @mask: bit mask of bits to clear
+ * @set: bit mask of bits to set
+ */
+int mdiobus_c45_modify(struct mii_bus *bus, int addr, int devad, u32 regnum,
+		       u16 mask, u16 set)
+{
+	int err;
+
+	mutex_lock(&bus->mdio_lock);
+	err = __mdiobus_c45_modify_changed(bus, addr, devad, regnum,
+					   mask, set);
+	mutex_unlock(&bus->mdio_lock);
+
+	return err < 0 ? err : 0;
+}
+EXPORT_SYMBOL_GPL(mdiobus_c45_modify);
+
 /**
  * mdiobus_modify_changed - Convenience function for modifying a given mdio
  *	device register and returning if it changed
@@ -965,6 +1131,29 @@  int mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum,
 }
 EXPORT_SYMBOL_GPL(mdiobus_modify_changed);
 
+/**
+ * mdiobus_c45_modify_changed - Convenience function for modifying a given mdio
+ *	device register and returning if it changed
+ * @bus: the mii_bus struct
+ * @addr: the phy address
+ * @devad: device address to read
+ * @regnum: register number to write
+ * @mask: bit mask of bits to clear
+ * @set: bit mask of bits to set
+ */
+int mdiobus_c45_modify_changed(struct mii_bus *bus, int devad, int addr,
+			       u32 regnum, u16 mask, u16 set)
+{
+	int err;
+
+	mutex_lock(&bus->mdio_lock);
+	err = __mdiobus_c45_modify_changed(bus, addr, devad, regnum, mask, set);
+	mutex_unlock(&bus->mdio_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(mdiobus_c45_modify_changed);
+
 /**
  * mdio_bus_match - determine if given MDIO driver supports the given
  *		    MDIO device
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index f7fbbf3069e7..1e78c8410b21 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -423,6 +423,17 @@  int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask,
 		   u16 set);
 int mdiobus_modify_changed(struct mii_bus *bus, int addr, u32 regnum,
 			   u16 mask, u16 set);
+int __mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum);
+int mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum);
+int __mdiobus_c45_write(struct mii_bus *bus, int addr,  int devad, u32 regnum,
+			u16 val);
+int mdiobus_c45_write(struct mii_bus *bus, int addr,  int devad, u32 regnum,
+		      u16 val);
+int mdiobus_c45_modify(struct mii_bus *bus, int addr, int devad, u32 regnum,
+		       u16 mask, u16 set);
+
+int mdiobus_c45_modify_changed(struct mii_bus *bus, int addr, int devad,
+			       u32 regnum, u16 mask, u16 set);
 
 static inline int mdiodev_read(struct mdio_device *mdiodev, u32 regnum)
 {
@@ -463,29 +474,19 @@  static inline u16 mdiobus_c45_devad(u32 regnum)
 	return FIELD_GET(MII_DEVADDR_C45_MASK, regnum);
 }
 
-static inline int __mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
-				     u16 regnum)
+static inline int mdiodev_c45_modify(struct mdio_device *mdiodev, int devad,
+				     u32 regnum, u16 mask, u16 set)
 {
-	return __mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
+	return mdiobus_c45_modify(mdiodev->bus, mdiodev->addr, devad, regnum,
+				  mask, set);
 }
 
-static inline int __mdiobus_c45_write(struct mii_bus *bus, int prtad, int devad,
-				      u16 regnum, u16 val)
+static inline int mdiodev_c45_modify_changed(struct mdio_device *mdiodev,
+					     int devad, u32 regnum, u16 mask,
+					     u16 set)
 {
-	return __mdiobus_write(bus, prtad, mdiobus_c45_addr(devad, regnum),
-			       val);
-}
-
-static inline int mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
-				   u16 regnum)
-{
-	return mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
-}
-
-static inline int mdiobus_c45_write(struct mii_bus *bus, int prtad, int devad,
-				    u16 regnum, u16 val)
-{
-	return mdiobus_write(bus, prtad, mdiobus_c45_addr(devad, regnum), val);
+	return mdiobus_c45_modify_changed(mdiodev->bus, mdiodev->addr, devad,
+					  regnum, mask, set);
 }
 
 static inline int mdiodev_c45_read(struct mdio_device *mdiodev, int devad,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6378c997ded5..65844f0a7fb3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -364,6 +364,11 @@  struct mii_bus {
 	int (*read)(struct mii_bus *bus, int addr, int regnum);
 	/** @write: Perform a write transfer on the bus */
 	int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
+	/** @read: Perform a C45 read transfer on the bus */
+	int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
+	/** @write: Perform a C45 write transfer on the bus */
+	int (*write_c45)(struct mii_bus *bus, int addr, int devnum,
+			 int regnum, u16 val);
 	/** @reset: Perform a reset of the bus */
 	int (*reset)(struct mii_bus *bus);