[net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

[net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

[Christian Marangi]

Current API for PHY package are limited to single address to configure
global settings for the PHY package.

It was found that some PHY package (for example the qca807x, a PHY
package that is shipped with a bundle of 5 PHY) requires multiple PHY
address to configure global settings. An example scenario is a PHY that
have a dedicated PHY for PSGMII/serdes calibrarion and have a specific
PHY in the package where the global PHY mode is set and affects every
other PHY in the package.

Change the API in the following way:
- Change phy_package_join() to take the base addr of the PHY package
  instead of the global PHY addr.
- Make __/phy_package_write/read() require an additional arg that
  select what global PHY address to use by passing the offset from the
  base addr passed on phy_package_join().

Each user of this API is updated to follow this new implementation
following a pattern where an enum is defined to declare the offset of the
addr.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/bcm54140.c       | 16 +++++++++---
 drivers/net/phy/mscc/mscc.h      |  5 ++++
 drivers/net/phy/mscc/mscc_main.c |  4 +--
 drivers/net/phy/phy_device.c     | 40 ++++++++++++++++------------
 include/linux/phy.h              | 45 ++++++++++++++++++++------------
 5 files changed, 72 insertions(+), 38 deletions(-)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index d43076592f81..2eea3d09b1e6 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -128,6 +128,10 @@
 #define BCM54140_DEFAULT_DOWNSHIFT 5
 #define BCM54140_MAX_DOWNSHIFT 9
 
+enum bcm54140_global_phy {
+	BCM54140_BASE_ADDR = 0,
+};
+
 struct bcm54140_priv {
 	int port;
 	int base_addr;
@@ -429,11 +433,13 @@ static int bcm54140_base_read_rdb(struct phy_device *phydev, u16 rdb)
 	int ret;
 
 	phy_lock_mdio_bus(phydev);
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __phy_package_read(phydev, MII_BCM54XX_RDB_DATA);
+	ret = __phy_package_read(phydev, BCM54140_BASE_ADDR,
+				 MII_BCM54XX_RDB_DATA);
 
 out:
 	phy_unlock_mdio_bus(phydev);
@@ -446,11 +452,13 @@ static int bcm54140_base_write_rdb(struct phy_device *phydev,
 	int ret;
 
 	phy_lock_mdio_bus(phydev);
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_DATA, val);
+	ret = __phy_package_write(phydev, BCM54140_BASE_ADDR,
+				  MII_BCM54XX_RDB_DATA, val);
 
 out:
 	phy_unlock_mdio_bus(phydev);
diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 7a962050a4d4..6a3d8a754eb8 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -416,6 +416,11 @@ struct vsc8531_private {
  * gpio_lock: used for PHC operations. Common for all PHYs as the load/save GPIO
  * is shared.
  */
+
+enum vsc85xx_global_phy {
+	VSC88XX_BASE_ADDR = 0,
+};
+
 struct vsc85xx_shared_private {
 	struct mutex gpio_lock;
 };
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 4171f01d34e5..6f74ce0ab1aa 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -711,7 +711,7 @@ int phy_base_write(struct phy_device *phydev, u32 regnum, u16 val)
 		dump_stack();
 	}
 
-	return __phy_package_write(phydev, regnum, val);
+	return __phy_package_write(phydev, VSC88XX_BASE_ADDR, regnum, val);
 }
 
 /* phydev->bus->mdio_lock should be locked when using this function */
@@ -722,7 +722,7 @@ int phy_base_read(struct phy_device *phydev, u32 regnum)
 		dump_stack();
 	}
 
-	return __phy_package_read(phydev, regnum);
+	return __phy_package_read(phydev, VSC88XX_BASE_ADDR, regnum);
 }
 
 u32 vsc85xx_csr_read(struct phy_device *phydev,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 478126f6b5bc..823b25bb3e3e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1648,20 +1648,27 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
 /**
  * phy_package_join - join a common PHY group
  * @phydev: target phy_device struct
- * @addr: cookie and PHY address for global register access
+ * @base_addr: cookie and base PHY address of PHY package for offset
+ *   calculation of global register access
  * @priv_size: if non-zero allocate this amount of bytes for private data
  *
  * This joins a PHY group and provides a shared storage for all phydevs in
  * this group. This is intended to be used for packages which contain
  * more than one PHY, for example a quad PHY transceiver.
  *
- * The addr parameter serves as a cookie which has to have the same value
- * for all members of one group and as a PHY address to access generic
- * registers of a PHY package. Usually, one of the PHY addresses of the
- * different PHYs in the package provides access to these global registers.
+ * The addr parameter serves as cookie which has to have the same values
+ * for all members of one group and as the base PHY address of the PHY package
+ * for offset calculation to access generic registers of a PHY package.
+ * Usually, one of the PHY addresses of the different PHYs in the package
+ * provides access to these global registers.
  * The address which is given here, will be used in the phy_package_read()
- * and phy_package_write() convenience functions. If your PHY doesn't have
- * global registers you can just pick any of the PHY addresses.
+ * and phy_package_write() convenience functions as base and added to the
+ * passed offset in those functions. If your PHY doesn't have global registers
+ * you can just pick any of the PHY addresses.
+ * In some special PHY package, multiple PHY are used for global init of
+ * the entire PHY package. In this the scenario, phy_package_read() and
+ * phy_package_write() offset is used to communicate which PHY to use for
+ * global init on read/write.
  *
  * This will set the shared pointer of the phydev to the shared storage.
  * If this is the first call for a this cookie the shared storage will be
@@ -1671,17 +1678,17 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
  * Returns < 1 on error, 0 on success. Esp. calling phy_package_join()
  * with the same cookie but a different priv_size is an error.
  */
-int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size)
+int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size)
 {
 	struct mii_bus *bus = phydev->mdio.bus;
 	struct phy_package_shared *shared;
 	int ret;
 
-	if (addr < 0 || addr >= PHY_MAX_ADDR)
+	if (base_addr < 0 || base_addr >= PHY_MAX_ADDR)
 		return -EINVAL;
 
 	mutex_lock(&bus->shared_lock);
-	shared = bus->shared[addr];
+	shared = bus->shared[base_addr];
 	if (!shared) {
 		ret = -ENOMEM;
 		shared = kzalloc(sizeof(*shared), GFP_KERNEL);
@@ -1693,9 +1700,9 @@ int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size)
 				goto err_free;
 			shared->priv_size = priv_size;
 		}
-		shared->addr = addr;
+		shared->base_addr = base_addr;
 		refcount_set(&shared->refcnt, 1);
-		bus->shared[addr] = shared;
+		bus->shared[base_addr] = shared;
 	} else {
 		ret = -EINVAL;
 		if (priv_size && priv_size != shared->priv_size)
@@ -1733,7 +1740,7 @@ void phy_package_leave(struct phy_device *phydev)
 		return;
 
 	if (refcount_dec_and_mutex_lock(&shared->refcnt, &bus->shared_lock)) {
-		bus->shared[shared->addr] = NULL;
+		bus->shared[shared->base_addr] = NULL;
 		mutex_unlock(&bus->shared_lock);
 		kfree(shared->priv);
 		kfree(shared);
@@ -1752,7 +1759,8 @@ static void devm_phy_package_leave(struct device *dev, void *res)
  * devm_phy_package_join - resource managed phy_package_join()
  * @dev: device that is registering this PHY package
  * @phydev: target phy_device struct
- * @addr: cookie and PHY address for global register access
+ * @base_addr: cookie and base PHY address of PHY package for offset
+ *   calculation of global register access
  * @priv_size: if non-zero allocate this amount of bytes for private data
  *
  * Managed phy_package_join(). Shared storage fetched by this function,
@@ -1760,7 +1768,7 @@ static void devm_phy_package_leave(struct device *dev, void *res)
  * phy_package_join() for more information.
  */
 int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
-			  int addr, size_t priv_size)
+			  int base_addr, size_t priv_size)
 {
 	struct phy_device **ptr;
 	int ret;
@@ -1770,7 +1778,7 @@ int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
 	if (!ptr)
 		return -ENOMEM;
 
-	ret = phy_package_join(phydev, addr, priv_size);
+	ret = phy_package_join(phydev, base_addr, priv_size);
 
 	if (!ret) {
 		*ptr = phydev;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e5f1f41e399c..8253c7871d68 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -327,7 +327,8 @@ struct mdio_bus_stats {
 
 /**
  * struct phy_package_shared - Shared information in PHY packages
- * @addr: Common PHY address used to combine PHYs in one package
+ * @base_addr: Base PHY address of PHY package used to combine PHYs
+ *   in one package and for offset calculation of phy_package_read/write
  * @refcnt: Number of PHYs connected to this shared data
  * @flags: Initialization of PHY package
  * @priv_size: Size of the shared private data @priv
@@ -338,7 +339,7 @@ struct mdio_bus_stats {
  * phy_package_leave().
  */
 struct phy_package_shared {
-	int addr;
+	int base_addr;
 	refcount_t refcnt;
 	unsigned long flags;
 	size_t priv_size;
@@ -1972,10 +1973,10 @@ int phy_ethtool_get_link_ksettings(struct net_device *ndev,
 int phy_ethtool_set_link_ksettings(struct net_device *ndev,
 				   const struct ethtool_link_ksettings *cmd);
 int phy_ethtool_nway_reset(struct net_device *ndev);
-int phy_package_join(struct phy_device *phydev, int addr, size_t priv_size);
+int phy_package_join(struct phy_device *phydev, int base_addr, size_t priv_size);
 void phy_package_leave(struct phy_device *phydev);
 int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
-			  int addr, size_t priv_size);
+			  int base_addr, size_t priv_size);
 
 int __init mdio_bus_init(void);
 void mdio_bus_exit(void);
@@ -1998,46 +1999,58 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config,
 		       struct netlink_ext_ack *extack);
 
-static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
+static inline int phy_package_read(struct phy_device *phydev,
+				   unsigned int addr_offset, u32 regnum)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr;
 
-	if (!shared)
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
 		return -EIO;
 
-	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+	addr = shared->base_addr + addr_offset;
+	return mdiobus_read(phydev->mdio.bus, addr, regnum);
 }
 
-static inline int __phy_package_read(struct phy_device *phydev, u32 regnum)
+static inline int __phy_package_read(struct phy_device *phydev,
+				     unsigned int addr_offset, u32 regnum)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr;
 
-	if (!shared)
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
 		return -EIO;
 
-	return __mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+	addr = shared->base_addr + addr_offset;
+	return __mdiobus_read(phydev->mdio.bus, addr, regnum);
 }
 
 static inline int phy_package_write(struct phy_device *phydev,
-				    u32 regnum, u16 val)
+				    unsigned int addr_offset, u32 regnum,
+				    u16 val)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr;
 
-	if (!shared)
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
 		return -EIO;
 
-	return mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+	addr = shared->base_addr + addr_offset;
+	return mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
 static inline int __phy_package_write(struct phy_device *phydev,
-				      u32 regnum, u16 val)
+				      unsigned int addr_offset, u32 regnum,
+				      u16 val)
 {
 	struct phy_package_shared *shared = phydev->shared;
+	int addr;
 
-	if (!shared)
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
 		return -EIO;
 
-	return __mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+	addr = shared->base_addr + addr_offset;
+	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
 static inline bool __phy_package_set_once(struct phy_device *phydev,
-- 
2.40.1

[net-next PATCH 2/3] net: phy: move mmd_phy_indirect to generic header

[Christian Marangi]

Move mmd_phy_indirect function from phy-core to generic phy.h to permit
future usage for PHY package read/write_mmd.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/phy-core.c | 14 --------------
 include/linux/phy.h        | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 966c93cbe616..b4f80847eefd 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -526,20 +526,6 @@ 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)
-{
-	/* Write the desired MMD Devad */
-	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
-
-	/* Write the desired MMD register address */
-	__mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
-
-	/* Select the Function : DATA with no post increment */
-	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
-			devad | MII_MMD_CTRL_NOINCR);
-}
-
 /**
  * __phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 8253c7871d68..984bca9a82f4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1316,6 +1316,20 @@ static inline int __phy_modify_changed(struct phy_device *phydev, u32 regnum,
 					regnum, mask, set);
 }
 
+static inline void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
+				    u16 regnum)
+{
+	/* Write the desired MMD Devad */
+	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
+
+	/* Write the desired MMD register address */
+	__mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
+
+	/* Select the Function : DATA with no post increment */
+	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
+			devad | MII_MMD_CTRL_NOINCR);
+}
+
 /*
  * phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
-- 
2.40.1

[net-next PATCH 3/3] net: phy: add support for PHY package MMD read/write

[Christian Marangi]

Some PHY in PHY package may require to read/write MMD regs to correctly
configure the PHY package.

Add support for these additional required function in both lock and no
lock variant.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 984bca9a82f4..1799133c8387 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev,
 	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
 }
 
+static inline int phy_package_read_mmd(struct phy_device *phydev,
+				       unsigned int addr_offset, int devad,
+				       u32 regnum)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	struct mii_bus *bus = phydev->mdio.bus;
+	int addr, val;
+
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
+		return -EIO;
+
+	addr = shared->base_addr + addr_offset;
+
+	phy_lock_mdio_bus(phydev);
+	mmd_phy_indirect(bus, addr, devad, regnum);
+	val = __mdiobus_read(bus, addr, MII_MMD_DATA);
+	phy_unlock_mdio_bus(phydev);
+
+	return val;
+}
+
+static inline int __phy_package_read_mmd(struct phy_device *phydev,
+					 unsigned int addr_offset, int devad,
+					 u32 regnum)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	struct mii_bus *bus = phydev->mdio.bus;
+	int addr;
+
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
+		return -EIO;
+
+	addr = shared->base_addr + addr_offset;
+	mmd_phy_indirect(bus, addr, devad, regnum);
+	return __mdiobus_read(bus, addr, MII_MMD_DATA);
+}
+
+static inline int phy_package_write_mmd(struct phy_device *phydev,
+					unsigned int addr_offset, int devad,
+					u32 regnum, u16 val)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	struct mii_bus *bus = phydev->mdio.bus;
+	int addr, ret;
+
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
+		return -EIO;
+
+	addr = shared->base_addr + addr_offset;
+
+	phy_lock_mdio_bus(phydev);
+	mmd_phy_indirect(bus, addr, devad, regnum);
+	ret = __mdiobus_write(bus, addr, MII_MMD_DATA, val);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+
+static inline int __phy_package_write_mmd(struct phy_device *phydev,
+					  unsigned int addr_offset, int devad,
+					  u32 regnum, u16 val)
+{
+	struct phy_package_shared *shared = phydev->shared;
+	struct mii_bus *bus = phydev->mdio.bus;
+	int addr;
+
+	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
+		return -EIO;
+
+	addr = shared->base_addr + addr_offset;
+	mmd_phy_indirect(bus, addr, devad, regnum);
+	return __mdiobus_write(bus, addr, MII_MMD_DATA, val);
+}
+
 static inline bool __phy_package_set_once(struct phy_device *phydev,
 					  unsigned int b)
 {
-- 
2.40.1

Re: [net-next PATCH 3/3] net: phy: add support for PHY package MMD read/write

[Florian Fainelli]

On 11/25/2023 4:37 PM, Christian Marangi wrote:
> Some PHY in PHY package may require to read/write MMD regs to correctly
> configure the PHY package.
> 
> Add support for these additional required function in both lock and no
> lock variant.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>   include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 74 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 984bca9a82f4..1799133c8387 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev,
>   	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
>   }
>   
> +static inline int phy_package_read_mmd(struct phy_device *phydev,
> +				       unsigned int addr_offset, int devad,
> +				       u32 regnum)
> +{
> +	struct phy_package_shared *shared = phydev->shared;
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int addr, val;
> +
> +	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
> +		return -EIO;

You might be off by one here, should not that >= PHY_MAX_ADDR here and 
below?
-- 
Florian

Re: [net-next PATCH 3/3] net: phy: add support for PHY package MMD read/write

[Christian Marangi]

On Sat, Nov 25, 2023 at 04:52:19PM -0800, Florian Fainelli wrote:
> 
> 
> On 11/25/2023 4:37 PM, Christian Marangi wrote:
> > Some PHY in PHY package may require to read/write MMD regs to correctly
> > configure the PHY package.
> > 
> > Add support for these additional required function in both lock and no
> > lock variant.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 74 insertions(+)
> > 
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 984bca9a82f4..1799133c8387 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev,
> >   	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
> >   }
> > +static inline int phy_package_read_mmd(struct phy_device *phydev,
> > +				       unsigned int addr_offset, int devad,
> > +				       u32 regnum)
> > +{
> > +	struct phy_package_shared *shared = phydev->shared;
> > +	struct mii_bus *bus = phydev->mdio.bus;
> > +	int addr, val;
> > +
> > +	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
> > +		return -EIO;
> 
> You might be off by one here, should not that >= PHY_MAX_ADDR here and
> below?

Thanks for the review. Yes PHY_MAX_ADDR is 32 so I should use >=.

(interesting choice to use 32 instead of 31 as MAX, guess an old mistake)

-- 
	Ansuel

Re: [net-next PATCH 3/3] net: phy: add support for PHY package MMD read/write

[Florian Fainelli]

On 11/25/2023 4:54 PM, Christian Marangi wrote:
> On Sat, Nov 25, 2023 at 04:52:19PM -0800, Florian Fainelli wrote:
>>
>>
>> On 11/25/2023 4:37 PM, Christian Marangi wrote:
>>> Some PHY in PHY package may require to read/write MMD regs to correctly
>>> configure the PHY package.
>>>
>>> Add support for these additional required function in both lock and no
>>> lock variant.
>>>
>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>> ---
>>>    include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 74 insertions(+)
>>>
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index 984bca9a82f4..1799133c8387 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev,
>>>    	return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
>>>    }
>>> +static inline int phy_package_read_mmd(struct phy_device *phydev,
>>> +				       unsigned int addr_offset, int devad,
>>> +				       u32 regnum)
>>> +{
>>> +	struct phy_package_shared *shared = phydev->shared;
>>> +	struct mii_bus *bus = phydev->mdio.bus;
>>> +	int addr, val;
>>> +
>>> +	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
>>> +		return -EIO;
>>
>> You might be off by one here, should not that >= PHY_MAX_ADDR here and
>> below?
> 
> Thanks for the review. Yes PHY_MAX_ADDR is 32 so I should use >=.
> 
> (interesting choice to use 32 instead of 31 as MAX, guess an old mistake)

It has historically been used as an iterator upper bound, hence the 32.
-- 
Florian

Re: [net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

[Andrew Lunn]

> @@ -1648,20 +1648,27 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
>  /**
>   * phy_package_join - join a common PHY group
>   * @phydev: target phy_device struct
> - * @addr: cookie and PHY address for global register access
> + * @base_addr: cookie and base PHY address of PHY package for offset
> + *   calculation of global register access
>   * @priv_size: if non-zero allocate this amount of bytes for private data
>   *
>   * This joins a PHY group and provides a shared storage for all phydevs in
>   * this group. This is intended to be used for packages which contain
>   * more than one PHY, for example a quad PHY transceiver.
>   *
> - * The addr parameter serves as a cookie which has to have the same value
> - * for all members of one group and as a PHY address to access generic
> - * registers of a PHY package. Usually, one of the PHY addresses of the
> - * different PHYs in the package provides access to these global registers.
> + * The addr parameter serves as cookie which has to have the same values

addr has been renamed base_addr.

> + * for all members of one group and as the base PHY address of the PHY package
> + * for offset calculation to access generic registers of a PHY package.
> + * Usually, one of the PHY addresses of the different PHYs in the package
> + * provides access to these global registers.
>   * The address which is given here, will be used in the phy_package_read()
> - * and phy_package_write() convenience functions. If your PHY doesn't have
> - * global registers you can just pick any of the PHY addresses.
> + * and phy_package_write() convenience functions as base and added to the
> + * passed offset in those functions. If your PHY doesn't have global registers
> + * you can just pick any of the PHY addresses.


I would not add this last sentence. We want a clearly defined meaning
of base_addr. Its the lowest address in the package. It does not
matter if its not used, it should still be the lowest address in the
package.

> + * In some special PHY package, multiple PHY are used for global init of

I don't see why they are special.

> -static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
> +static inline int phy_package_read(struct phy_device *phydev,
> +				   unsigned int addr_offset, u32 regnum)
>  {
>  	struct phy_package_shared *shared = phydev->shared;
> +	int addr;
>  
> -	if (!shared)
> +	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
>  		return -EIO;
>  
> -	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
> +	addr = shared->base_addr + addr_offset;
> +	return mdiobus_read(phydev->mdio.bus, addr, regnum);

This might be a little bit more readable:

static inline int phy_package_read(struct phy_device *phydev,
				   unsigned int addr_offset, u32 regnum)
{
	struct phy_package_shared *shared = phydev->shared;
	int addr = shared->base_addr + addr_offset;

	if (!shared)
	if (!shared || addr > PHY_MAX_ADDR)
		return -EIO;

	return mdiobus_read(phydev->mdio.bus, addr, regnum);
}


    Andrew

---
pw-bot: cr

Re: [net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

[Christian Marangi]

On Sun, Nov 26, 2023 at 07:04:26PM +0100, Andrew Lunn wrote:
> > @@ -1648,20 +1648,27 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
> >  /**
> >   * phy_package_join - join a common PHY group
> >   * @phydev: target phy_device struct
> > - * @addr: cookie and PHY address for global register access
> > + * @base_addr: cookie and base PHY address of PHY package for offset
> > + *   calculation of global register access
> >   * @priv_size: if non-zero allocate this amount of bytes for private data
> >   *
> >   * This joins a PHY group and provides a shared storage for all phydevs in
> >   * this group. This is intended to be used for packages which contain
> >   * more than one PHY, for example a quad PHY transceiver.
> >   *
> > - * The addr parameter serves as a cookie which has to have the same value
> > - * for all members of one group and as a PHY address to access generic
> > - * registers of a PHY package. Usually, one of the PHY addresses of the
> > - * different PHYs in the package provides access to these global registers.
> > + * The addr parameter serves as cookie which has to have the same values
> 
> addr has been renamed base_addr.
> 
> > + * for all members of one group and as the base PHY address of the PHY package
> > + * for offset calculation to access generic registers of a PHY package.
> > + * Usually, one of the PHY addresses of the different PHYs in the package
> > + * provides access to these global registers.
> >   * The address which is given here, will be used in the phy_package_read()
> > - * and phy_package_write() convenience functions. If your PHY doesn't have
> > - * global registers you can just pick any of the PHY addresses.
> > + * and phy_package_write() convenience functions as base and added to the
> > + * passed offset in those functions. If your PHY doesn't have global registers
> > + * you can just pick any of the PHY addresses.
> 
> 
> I would not add this last sentence. We want a clearly defined meaning
> of base_addr. Its the lowest address in the package. It does not
> matter if its not used, it should still be the lowest address in the
> package.
> 
> > + * In some special PHY package, multiple PHY are used for global init of
> 
> I don't see why they are special.
> 
> > -static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
> > +static inline int phy_package_read(struct phy_device *phydev,
> > +				   unsigned int addr_offset, u32 regnum)
> >  {
> >  	struct phy_package_shared *shared = phydev->shared;
> > +	int addr;
> >  
> > -	if (!shared)
> > +	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
> >  		return -EIO;
> >  
> > -	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
> > +	addr = shared->base_addr + addr_offset;
> > +	return mdiobus_read(phydev->mdio.bus, addr, regnum);
> 
> This might be a little bit more readable:
> 
> static inline int phy_package_read(struct phy_device *phydev,
> 				   unsigned int addr_offset, u32 regnum)
> {
> 	struct phy_package_shared *shared = phydev->shared;
> 	int addr = shared->base_addr + addr_offset;

Isn't this problematic if shared is NULL?

I can add 2 if condition and set addr in between only after shared not
NULL check is done?

> 
> 	if (!shared)
> 	if (!shared || addr > PHY_MAX_ADDR)
> 		return -EIO;
> 
> 	return mdiobus_read(phydev->mdio.bus, addr, regnum);
> }
> 
>

-- 
	Ansuel

Re: [net-next PATCH 3/3] net: phy: add support for PHY package MMD read/write

[Andrew Lunn]

On Sun, Nov 26, 2023 at 01:37:48AM +0100, Christian Marangi wrote:
> Some PHY in PHY package may require to read/write MMD regs to correctly
> configure the PHY package.
> 
> Add support for these additional required function in both lock and no
> lock variant.

You are assuming the PHY only supports C45 over C22. But what about
those PHYs which have native C45? And maybe don't have C22 at all?

You should refactor the code of __phy_read_mmd() into a helper and use
it here.

   Andrew

Re: [net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

[Andrew Lunn]

> > static inline int phy_package_read(struct phy_device *phydev,
> > 				   unsigned int addr_offset, u32 regnum)
> > {
> > 	struct phy_package_shared *shared = phydev->shared;
> > 	int addr = shared->base_addr + addr_offset;
> 
> Isn't this problematic if shared is NULL?

Duh! Yes, it is. But why should shared be NULL? The driver is doing a
read on the package before the package is created. That is a bug. So
an Opps is O.K, it helps find the bug. So i would drop the test for
!shared.

	Andrew

Re: [net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

[Christian Marangi]

On Sun, Nov 26, 2023 at 07:19:16PM +0100, Andrew Lunn wrote:
> > > static inline int phy_package_read(struct phy_device *phydev,
> > > 				   unsigned int addr_offset, u32 regnum)
> > > {
> > > 	struct phy_package_shared *shared = phydev->shared;
> > > 	int addr = shared->base_addr + addr_offset;
> > 
> > Isn't this problematic if shared is NULL?
> 
> Duh! Yes, it is. But why should shared be NULL? The driver is doing a
> read on the package before the package is created. That is a bug. So
> an Opps is O.K, it helps find the bug. So i would drop the test for
> !shared.

Well yes I think we should assume those API to be called only in
config_once context or in package context. But is it Panic ok? I would
at least use something like BUG() to give descriptive warning instead of
NULL pointer exception. What do you think?

-- 
	Ansuel

Re: [net-next PATCH 3/3] net: phy: add support for PHY package MMD read/write

[Christian Marangi]

On Sun, Nov 26, 2023 at 07:14:52PM +0100, Andrew Lunn wrote:
> On Sun, Nov 26, 2023 at 01:37:48AM +0100, Christian Marangi wrote:
> > Some PHY in PHY package may require to read/write MMD regs to correctly
> > configure the PHY package.
> > 
> > Add support for these additional required function in both lock and no
> > lock variant.
> 
> You are assuming the PHY only supports C45 over C22. But what about
> those PHYs which have native C45? And maybe don't have C22 at all?
> 
> You should refactor the code of __phy_read_mmd() into a helper and use
> it here.
>

Have to be honest here and chose the ""easy"" way. Was a little scared
by all the complexity with mmd with C45 and C22... The idea was to add
C22 support for now and if used by others extend the function. But I
guess with the generic approach we are taking it's better to handle it
now. Will update this in v2.

-- 
	Ansuel

Re: [net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

[Andrew Lunn]

> Well yes I think we should assume those API to be called only in
> config_once context or in package context. But is it Panic ok? I would
> at least use something like BUG() to give descriptive warning instead of
> NULL pointer exception. What do you think?

BUG() is way too strong. It causes an immediate stop of everything,
and probably file system data loss and a reboot. WARN_ON() is
generally no better.

An Opps gives a stack trace, which is what you need to find the bug,
and kills the process. Generally, you can do a controlled shutdown,
without any losses.

	Andrew