linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
@ 2020-04-20 23:26 Michael Walle
  2020-04-20 23:26 ` [RFC PATCH net-next 2/3] net: phy: bcm54140: use phy_package_shared Michael Walle
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Michael Walle @ 2020-04-20 23:26 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
	David S . Miller, Vladimir Oltean, Michael Walle

There are packages which contain multiple PHY devices, eg. a quad PHY
transceiver. Provide functions to allocate and free shared storage.

Usually, a quad PHY contains global registers, which don't belong to any
PHY. Provide convenience functions to access these registers.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/mdio_bus.c   |   1 +
 drivers/net/phy/phy_device.c | 112 +++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  86 +++++++++++++++++++++++++++
 3 files changed, 199 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 7a4eb3f2cb74..247cb6610a05 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -611,6 +611,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	}
 
 	mutex_init(&bus->mdio_lock);
+	mutex_init(&bus->shared_lock);
 
 	/* de-assert bus level PHY GPIO reset */
 	gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac2784192472..420af3f732c3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1457,6 +1457,118 @@ bool phy_driver_is_genphy_10g(struct phy_device *phydev)
 }
 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
+ *
+ * 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 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.
+ *
+ * This will set the shared pointer of the phydev to the shared storage.
+ * If this is the first call for a this cookie the share storage will be
+ * allocated.
+ */
+int phy_package_join(struct phy_device *phydev, int addr)
+{
+	struct mii_bus *bus = phydev->mdio.bus;
+	struct phy_package_shared *shared;
+
+	if (addr >= PHY_MAX_ADDR)
+		return -EINVAL;
+
+	mutex_lock(&bus->shared_lock);
+	shared = bus->shared[addr];
+	if (!shared) {
+		shared = kzalloc(sizeof(*shared), GFP_KERNEL);
+		shared->addr = addr;
+		refcount_set(&shared->refcnt, 1);
+		bus->shared[addr] = shared;
+	} else {
+		refcount_inc(&shared->refcnt);
+	}
+	mutex_unlock(&bus->shared_lock);
+
+	phydev->shared = shared;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(phy_package_join);
+
+/**
+ * phy_package_leave - leave a common PHY group
+ * @phydev: target phy_device struct
+ *
+ * This leaves a PHY group created by phy_package_join(). If this phydev
+ * was the last user of the shared data between the group, this data is
+ * freed. Resets the phydev->shared pointer to NULL.
+ */
+void phy_package_leave(struct phy_device *phydev)
+{
+	struct mii_bus *bus = phydev->mdio.bus;
+	struct phy_package_shared *shared = phydev->shared;
+
+	if (!shared)
+		return;
+
+	if (refcount_dec_and_mutex_lock(&shared->refcnt, &bus->shared_lock)) {
+		bus->shared[shared->addr] = NULL;
+		mutex_unlock(&bus->shared_lock);
+		kfree(shared);
+	}
+
+	phydev->shared = NULL;
+}
+EXPORT_SYMBOL_GPL(phy_package_leave);
+
+static void devm_phy_package_leave(struct device *dev, void *res)
+{
+	phy_package_leave(*(struct phy_device **)res);
+}
+
+/**
+ * devm_phy_package_join - resource managed phy_package_join()
+ * @dev: device that is registering this GPIO device
+ * @phydev: target phy_device struct
+ * @addr: cookie and PHY address for global register access
+ *
+ * Managed phy_package_join(). Shared storage fetched by this function,
+ * phy_package_leave() is automatically called on driver detach. See
+ * phy_package_join() for more information.
+ */
+int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
+			  int addr)
+{
+	struct phy_device **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_phy_package_leave, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = phy_package_join(phydev, addr);
+
+	if (!ret) {
+		*ptr = phydev;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_phy_package_join);
+
 /**
  * phy_detach - detach a PHY device from its network device
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2432ca463ddc..d0d85868af76 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -25,6 +25,7 @@
 #include <linux/u64_stats_sync.h>
 #include <linux/irqreturn.h>
 #include <linux/iopoll.h>
+#include <linux/refcount.h>
 
 #include <linux/atomic.h>
 
@@ -227,6 +228,25 @@ struct mdio_bus_stats {
 	struct u64_stats_sync syncp;
 };
 
+
+/* Represents a shared structure between different phydev's in the same
+ * package, for example a quad PHY. See phy_package_join() and
+ * phy_package_leave().
+ */
+struct phy_package_shared {
+	int addr;
+	refcount_t refcnt;
+	unsigned long flags;
+
+	/* private data pointer */
+	/* note that this pointer is shared between different phydevs and
+	 * the user has to take care of appropriate locking.
+	 */
+	void *priv;
+};
+
+#define PHY_SHARED_F_INIT_DONE 0
+
 /*
  * The Bus class for PHYs.  Devices which provide access to
  * PHYs should register using this structure
@@ -275,6 +295,12 @@ struct mii_bus {
 	int reset_delay_us;
 	/* RESET GPIO descriptor pointer */
 	struct gpio_desc *reset_gpiod;
+
+	/* protect access to the shared element */
+	struct mutex shared_lock;
+
+	/* shared state across different PHYs */
+	struct phy_package_shared *shared[PHY_MAX_ADDR];
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
 
@@ -461,6 +487,10 @@ struct phy_device {
 	/* For use by PHYs to maintain extra state */
 	void *priv;
 
+	/* shared data pointer */
+	/* For use by PHYs inside the same package and needs a shared state. */
+	struct phy_package_shared *shared;
+
 	/* Interrupt and Polling infrastructure */
 	struct delayed_work state_queue;
 
@@ -1341,6 +1371,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);
+void phy_package_leave(struct phy_device *phydev);
+int devm_phy_package_join(struct device *dev, struct phy_device *phydev,
+			  int addr);
 
 #if IS_ENABLED(CONFIG_PHYLIB)
 int __init mdio_bus_init(void);
@@ -1393,6 +1427,58 @@ static inline int phy_ethtool_get_stats(struct phy_device *phydev,
 	return 0;
 }
 
+static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
+{
+	struct phy_package_shared *shared = phydev->shared;
+
+	if (!shared)
+		return -EIO;
+
+	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+}
+
+static inline int __phy_package_read(struct phy_device *phydev, u32 regnum)
+{
+	struct phy_package_shared *shared = phydev->shared;
+
+	if (!shared)
+		return -EIO;
+
+	return __mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
+}
+
+static inline int phy_package_write(struct phy_device *phydev,
+				    u32 regnum, u16 val)
+{
+	struct phy_package_shared *shared = phydev->shared;
+
+	if (!shared)
+		return -EIO;
+
+	return mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+}
+
+static inline int __phy_package_write(struct phy_device *phydev,
+				      u32 regnum, u16 val)
+{
+	struct phy_package_shared *shared = phydev->shared;
+
+	if (!shared)
+		return -EIO;
+
+	return __mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val);
+}
+
+static inline bool phy_package_init_once(struct phy_device *phydev)
+{
+	struct phy_package_shared *shared = phydev->shared;
+
+	if (!shared)
+		return false;
+
+	return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags);
+}
+
 extern struct bus_type mdio_bus_type;
 
 struct mdio_board_info {
-- 
2.20.1


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

* [RFC PATCH net-next 2/3] net: phy: bcm54140: use phy_package_shared
  2020-04-20 23:26 [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Michael Walle
@ 2020-04-20 23:26 ` Michael Walle
  2020-04-20 23:26 ` [RFC PATCH net-next 3/3] net: phy: mscc: " Michael Walle
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2020-04-20 23:26 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
	David S . Miller, Vladimir Oltean, Michael Walle

Use the new phy_package_shared common storage to ease the package
initialization and to access the global registers.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm54140.c | 57 ++++++++------------------------------
 1 file changed, 11 insertions(+), 46 deletions(-)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index aa854477e06a..97b9e1c763da 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -122,7 +122,6 @@ struct bcm54140_priv {
 	int port;
 	int base_addr;
 #if IS_ENABLED(CONFIG_HWMON)
-	bool pkg_init;
 	/* protect the alarm bits */
 	struct mutex alarm_lock;
 	u16 alarm;
@@ -395,36 +394,6 @@ static int bcm54140_enable_monitoring(struct phy_device *phydev)
 	return bcm_phy_modify_rdb(phydev, BCM54140_RDB_MON_CTRL, mask, set);
 }
 
-/* Check if one PHY has already done the init of the parts common to all PHYs
- * in the Quad PHY package.
- */
-static bool bcm54140_is_pkg_init(struct phy_device *phydev)
-{
-	struct bcm54140_priv *priv = phydev->priv;
-	struct mii_bus *bus = phydev->mdio.bus;
-	int base_addr = priv->base_addr;
-	struct phy_device *phy;
-	int i;
-
-	/* Quad PHY */
-	for (i = 0; i < 4; i++) {
-		phy = mdiobus_get_phy(bus, base_addr + i);
-		if (!phy)
-			continue;
-
-		if ((phy->phy_id & phydev->drv->phy_id_mask) !=
-		    (phydev->drv->phy_id & phydev->drv->phy_id_mask))
-			continue;
-
-		priv = phy->priv;
-
-		if (priv && priv->pkg_init)
-			return true;
-	}
-
-	return false;
-}
-
 static int bcm54140_probe_once(struct phy_device *phydev)
 {
 	struct device *hwmon;
@@ -445,38 +414,34 @@ static int bcm54140_probe_once(struct phy_device *phydev)
 
 static int bcm54140_base_read_rdb(struct phy_device *phydev, u16 rdb)
 {
-	struct bcm54140_priv *priv = phydev->priv;
-	struct mii_bus *bus = phydev->mdio.bus;
 	int ret;
 
-	mutex_lock(&bus->mdio_lock);
-	ret = __mdiobus_write(bus, priv->base_addr, MII_BCM54XX_RDB_ADDR, rdb);
+	phy_lock_mdio_bus(phydev);
+	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __mdiobus_read(bus, priv->base_addr, MII_BCM54XX_RDB_DATA);
+	ret = __phy_package_read(phydev, MII_BCM54XX_RDB_DATA);
 
 out:
-	mutex_unlock(&bus->mdio_lock);
+	phy_unlock_mdio_bus(phydev);
 	return ret;
 }
 
 static int bcm54140_base_write_rdb(struct phy_device *phydev,
 				   u16 rdb, u16 val)
 {
-	struct bcm54140_priv *priv = phydev->priv;
-	struct mii_bus *bus = phydev->mdio.bus;
 	int ret;
 
-	mutex_lock(&bus->mdio_lock);
-	ret = __mdiobus_write(bus, priv->base_addr, MII_BCM54XX_RDB_ADDR, rdb);
+	phy_lock_mdio_bus(phydev);
+	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
 	if (ret < 0)
 		goto out;
 
-	ret = __mdiobus_write(bus, priv->base_addr, MII_BCM54XX_RDB_DATA, val);
+	ret = __phy_package_write(phydev, MII_BCM54XX_RDB_DATA, val);
 
 out:
-	mutex_unlock(&bus->mdio_lock);
+	phy_unlock_mdio_bus(phydev);
 	return ret;
 }
 
@@ -606,16 +571,16 @@ static int bcm54140_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	devm_phy_package_join(&phydev->mdio.dev, phydev, priv->base_addr);
+
 #if IS_ENABLED(CONFIG_HWMON)
 	mutex_init(&priv->alarm_lock);
 
-	if (!bcm54140_is_pkg_init(phydev)) {
+	if (phy_package_init_once(phydev)) {
 		ret = bcm54140_probe_once(phydev);
 		if (ret)
 			return ret;
 	}
-
-	priv->pkg_init = true;
 #endif
 
 	phydev_dbg(phydev, "probed (port %d, base PHY address %d)\n",
-- 
2.20.1


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

* [RFC PATCH net-next 3/3] net: phy: mscc: use phy_package_shared
  2020-04-20 23:26 [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Michael Walle
  2020-04-20 23:26 ` [RFC PATCH net-next 2/3] net: phy: bcm54140: use phy_package_shared Michael Walle
@ 2020-04-20 23:26 ` Michael Walle
  2020-04-23 12:27   ` Vladimir Oltean
  2020-04-21 14:34 ` [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Andrew Lunn
  2020-04-21 15:25 ` Michael Walle
  3 siblings, 1 reply; 14+ messages in thread
From: Michael Walle @ 2020-04-20 23:26 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
	David S . Miller, Vladimir Oltean, Michael Walle

Use the new phy_package_shared common storage to ease the package
initialization and to access the global registers.

This was only compile-time tested!

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/mscc/mscc.h      |  1 -
 drivers/net/phy/mscc/mscc_main.c | 99 ++++++++++----------------------
 2 files changed, 29 insertions(+), 71 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 030bf8b600df..acdd8ee61a39 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -353,7 +353,6 @@ struct vsc8531_private {
 	const struct vsc85xx_hw_stat *hw_stats;
 	u64 *stats;
 	int nstats;
-	bool pkg_init;
 	/* For multiple port PHYs; the MDIO address of the base PHY in the
 	 * package.
 	 */
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 5391acdece05..382b56064de9 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -691,27 +691,23 @@ static int vsc85xx_eee_init_seq_set(struct phy_device *phydev)
 /* phydev->bus->mdio_lock should be locked when using this function */
 static int phy_base_write(struct phy_device *phydev, u32 regnum, u16 val)
 {
-	struct vsc8531_private *priv = phydev->priv;
-
 	if (unlikely(!mutex_is_locked(&phydev->mdio.bus->mdio_lock))) {
 		dev_err(&phydev->mdio.dev, "MDIO bus lock not held!\n");
 		dump_stack();
 	}
 
-	return __mdiobus_write(phydev->mdio.bus, priv->base_addr, regnum, val);
+	return __phy_package_write(phydev, regnum, val);
 }
 
 /* phydev->bus->mdio_lock should be locked when using this function */
 static int phy_base_read(struct phy_device *phydev, u32 regnum)
 {
-	struct vsc8531_private *priv = phydev->priv;
-
 	if (unlikely(!mutex_is_locked(&phydev->mdio.bus->mdio_lock))) {
 		dev_err(&phydev->mdio.dev, "MDIO bus lock not held!\n");
 		dump_stack();
 	}
 
-	return __mdiobus_read(phydev->mdio.bus, priv->base_addr, regnum);
+	return __phy_package_read(phydev, regnum);
 }
 
 /* bus->mdio_lock should be locked when using this function */
@@ -1287,65 +1283,36 @@ static int vsc8584_config_pre_init(struct phy_device *phydev)
 	return ret;
 }
 
-/* Check if one PHY has already done the init of the parts common to all PHYs
- * in the Quad PHY package.
- */
-static bool vsc8584_is_pkg_init(struct phy_device *phydev, bool reversed)
+static void vsc8584_get_base_addr(struct phy_device *phydev)
 {
-	struct mii_bus *bus = phydev->mdio.bus;
-	struct vsc8531_private *vsc8531;
-	struct phy_device *phy;
-	int i, addr;
-
-	/* VSC8584 is a Quad PHY */
-	for (i = 0; i < 4; i++) {
-		vsc8531 = phydev->priv;
-
-		if (reversed)
-			addr = vsc8531->base_addr - i;
-		else
-			addr = vsc8531->base_addr + i;
-
-		phy = mdiobus_get_phy(bus, addr);
-		if (!phy)
-			continue;
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	u16 val, addr;
 
-		if ((phy->phy_id & phydev->drv->phy_id_mask) !=
-		    (phydev->drv->phy_id & phydev->drv->phy_id_mask))
-			continue;
+	mutex_lock(&phydev->mdio.bus->mdio_lock);
+	__phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
 
-		vsc8531 = phy->priv;
+	addr = __phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_4);
+	addr >>= PHY_CNTL_4_ADDR_POS;
 
-		if (vsc8531 && vsc8531->pkg_init)
-			return true;
-	}
+	val = __phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
 
-	return false;
+	if (val & PHY_ADDR_REVERSED)
+		vsc8531->base_addr = phydev->mdio.addr + addr;
+	else
+		vsc8531->base_addr = phydev->mdio.addr - addr;
+	mutex_unlock(&phydev->mdio.bus->mdio_lock);
 }
 
 static int vsc8584_config_init(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531 = phydev->priv;
-	u16 addr, val;
 	int ret, i;
+	u16 val;
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 
 	mutex_lock(&phydev->mdio.bus->mdio_lock);
 
-	__mdiobus_write(phydev->mdio.bus, phydev->mdio.addr,
-			MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
-	addr = __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr,
-			      MSCC_PHY_EXT_PHY_CNTL_4);
-	addr >>= PHY_CNTL_4_ADDR_POS;
-
-	val = __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr,
-			     MSCC_PHY_ACTIPHY_CNTL);
-	if (val & PHY_ADDR_REVERSED)
-		vsc8531->base_addr = phydev->mdio.addr + addr;
-	else
-		vsc8531->base_addr = phydev->mdio.addr - addr;
-
 	/* Some parts of the init sequence are identical for every PHY in the
 	 * package. Some parts are modifying the GPIO register bank which is a
 	 * set of registers that are affecting all PHYs, a few resetting the
@@ -1359,7 +1326,7 @@ static int vsc8584_config_init(struct phy_device *phydev)
 	 * do the correct init sequence for all PHYs that are package-critical
 	 * in this pre-init function.
 	 */
-	if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0)) {
+	if (phy_package_init_once(phydev)) {
 		/* The following switch statement assumes that the lowest
 		 * nibble of the phy_id_mask is always 0. This works because
 		 * the lowest nibble of the PHY_ID's below are also 0.
@@ -1388,8 +1355,6 @@ static int vsc8584_config_init(struct phy_device *phydev)
 			goto err;
 	}
 
-	vsc8531->pkg_init = true;
-
 	phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
 		       MSCC_PHY_PAGE_EXTENDED_GPIO);
 
@@ -1427,7 +1392,8 @@ static int vsc8584_config_init(struct phy_device *phydev)
 
 	/* Disable SerDes for 100Base-FX */
 	ret = vsc8584_cmd(phydev, PROC_CMD_FIBER_MEDIA_CONF |
-			  PROC_CMD_FIBER_PORT(addr) | PROC_CMD_FIBER_DISABLE |
+			  PROC_CMD_FIBER_PORT(vsc8531->base_addr) |
+			  PROC_CMD_FIBER_DISABLE |
 			  PROC_CMD_READ_MOD_WRITE_PORT |
 			  PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_100BASE_FX);
 	if (ret)
@@ -1435,7 +1401,8 @@ static int vsc8584_config_init(struct phy_device *phydev)
 
 	/* Disable SerDes for 1000Base-X */
 	ret = vsc8584_cmd(phydev, PROC_CMD_FIBER_MEDIA_CONF |
-			  PROC_CMD_FIBER_PORT(addr) | PROC_CMD_FIBER_DISABLE |
+			  PROC_CMD_FIBER_PORT(vsc8531->base_addr) |
+			  PROC_CMD_FIBER_DISABLE |
 			  PROC_CMD_READ_MOD_WRITE_PORT |
 			  PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_1000BASE_X);
 	if (ret)
@@ -1750,26 +1717,14 @@ static int vsc8514_config_init(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531 = phydev->priv;
 	unsigned long deadline;
-	u16 val, addr;
 	int ret, i;
+	u16 val;
 	u32 reg;
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 
 	mutex_lock(&phydev->mdio.bus->mdio_lock);
 
-	__phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
-
-	addr = __phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_4);
-	addr >>= PHY_CNTL_4_ADDR_POS;
-
-	val = __phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
-
-	if (val & PHY_ADDR_REVERSED)
-		vsc8531->base_addr = phydev->mdio.addr + addr;
-	else
-		vsc8531->base_addr = phydev->mdio.addr - addr;
-
 	/* Some parts of the init sequence are identical for every PHY in the
 	 * package. Some parts are modifying the GPIO register bank which is a
 	 * set of registers that are affecting all PHYs, a few resetting the
@@ -1781,11 +1736,9 @@ static int vsc8514_config_init(struct phy_device *phydev)
 	 * do the correct init sequence for all PHYs that are package-critical
 	 * in this pre-init function.
 	 */
-	if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0))
+	if (phy_package_init_once(phydev))
 		vsc8514_config_pre_init(phydev);
 
-	vsc8531->pkg_init = true;
-
 	phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
 		       MSCC_PHY_PAGE_EXTENDED_GPIO);
 
@@ -1991,6 +1944,9 @@ static int vsc8514_probe(struct phy_device *phydev)
 
 	phydev->priv = vsc8531;
 
+	vsc8584_get_base_addr(phydev);
+	devm_phy_package_join(&phydev->mdio.dev, phydev, vsc8531->base_addr);
+
 	vsc8531->nleds = 4;
 	vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
 	vsc8531->hw_stats = vsc85xx_hw_stats;
@@ -2046,6 +2002,9 @@ static int vsc8584_probe(struct phy_device *phydev)
 
 	phydev->priv = vsc8531;
 
+	vsc8584_get_base_addr(phydev);
+	devm_phy_package_join(&phydev->mdio.dev, phydev, vsc8531->base_addr);
+
 	vsc8531->nleds = 4;
 	vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
 	vsc8531->hw_stats = vsc8584_hw_stats;
-- 
2.20.1


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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-20 23:26 [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Michael Walle
  2020-04-20 23:26 ` [RFC PATCH net-next 2/3] net: phy: bcm54140: use phy_package_shared Michael Walle
  2020-04-20 23:26 ` [RFC PATCH net-next 3/3] net: phy: mscc: " Michael Walle
@ 2020-04-21 14:34 ` Andrew Lunn
  2020-04-21 14:43   ` Russell King - ARM Linux admin
  2020-04-21 15:25 ` Michael Walle
  3 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2020-04-21 14:34 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-kernel, netdev, Florian Fainelli, Heiner Kallweit,
	Russell King, David S . Miller, Vladimir Oltean

On Tue, Apr 21, 2020 at 01:26:22AM +0200, Michael Walle wrote:
> There are packages which contain multiple PHY devices, eg. a quad PHY
> transceiver. Provide functions to allocate and free shared storage.
> 
> Usually, a quad PHY contains global registers, which don't belong to any
> PHY. Provide convenience functions to access these registers.

Hi Michael

Please provide a patch 0/3 cover note. DaveM will uses it for the
merge commit, etc.

> +void phy_package_leave(struct phy_device *phydev)
> +{
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	struct phy_package_shared *shared = phydev->shared;

Reverse Christmas tree.

> +static inline bool phy_package_init_once(struct phy_device *phydev)
> +{
> +	struct phy_package_shared *shared = phydev->shared;
> +
> +	if (!shared)
> +		return false;
> +
> +	return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags);
> +}

I need to look at how you actually use this, but i wonder if this is
sufficient. Can two PHYs probe at the same time? Could we have one PHY
be busy setting up the global init, and the other thinks the global
setup is complete? Do we want a comment like: 'Returns true when the
global package initialization is either under way or complete'?

       Andrew

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 14:34 ` [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Andrew Lunn
@ 2020-04-21 14:43   ` Russell King - ARM Linux admin
  2020-04-21 14:52     ` Andrew Lunn
  0 siblings, 1 reply; 14+ messages in thread
From: Russell King - ARM Linux admin @ 2020-04-21 14:43 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michael Walle, linux-kernel, netdev, Florian Fainelli,
	Heiner Kallweit, David S . Miller, Vladimir Oltean

On Tue, Apr 21, 2020 at 04:34:55PM +0200, Andrew Lunn wrote:
> > +static inline bool phy_package_init_once(struct phy_device *phydev)
> > +{
> > +	struct phy_package_shared *shared = phydev->shared;
> > +
> > +	if (!shared)
> > +		return false;
> > +
> > +	return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags);
> > +}
> 
> I need to look at how you actually use this, but i wonder if this is
> sufficient. Can two PHYs probe at the same time? Could we have one PHY
> be busy setting up the global init, and the other thinks the global
> setup is complete? Do we want a comment like: 'Returns true when the
> global package initialization is either under way or complete'?

IIRC, probe locking in the driver model is by per-driver locks, so
any particular driver won't probe more than one device at a time.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 14:43   ` Russell King - ARM Linux admin
@ 2020-04-21 14:52     ` Andrew Lunn
  2020-04-21 15:20       ` Michael Walle
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2020-04-21 14:52 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Michael Walle, linux-kernel, netdev, Florian Fainelli,
	Heiner Kallweit, David S . Miller, Vladimir Oltean

On Tue, Apr 21, 2020 at 03:43:02PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Apr 21, 2020 at 04:34:55PM +0200, Andrew Lunn wrote:
> > > +static inline bool phy_package_init_once(struct phy_device *phydev)
> > > +{
> > > +	struct phy_package_shared *shared = phydev->shared;
> > > +
> > > +	if (!shared)
> > > +		return false;
> > > +
> > > +	return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags);
> > > +}
> > 
> > I need to look at how you actually use this, but i wonder if this is
> > sufficient. Can two PHYs probe at the same time? Could we have one PHY
> > be busy setting up the global init, and the other thinks the global
> > setup is complete? Do we want a comment like: 'Returns true when the
> > global package initialization is either under way or complete'?
> 
> IIRC, probe locking in the driver model is by per-driver locks, so
> any particular driver won't probe more than one device at a time.

Hi Russel

Cool, thanks for the info.

      Andrew

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 14:52     ` Andrew Lunn
@ 2020-04-21 15:20       ` Michael Walle
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Walle @ 2020-04-21 15:20 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Russell King - ARM Linux admin, linux-kernel, netdev,
	Florian Fainelli, Heiner Kallweit, David S . Miller,
	Vladimir Oltean

Am 2020-04-21 16:52, schrieb Andrew Lunn:
> On Tue, Apr 21, 2020 at 03:43:02PM +0100, Russell King - ARM Linux 
> admin wrote:
>> On Tue, Apr 21, 2020 at 04:34:55PM +0200, Andrew Lunn wrote:
>> > > +static inline bool phy_package_init_once(struct phy_device *phydev)
>> > > +{
>> > > +	struct phy_package_shared *shared = phydev->shared;
>> > > +
>> > > +	if (!shared)
>> > > +		return false;
>> > > +
>> > > +	return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags);
>> > > +}
>> >
>> > I need to look at how you actually use this, but i wonder if this is
>> > sufficient. Can two PHYs probe at the same time? Could we have one PHY
>> > be busy setting up the global init, and the other thinks the global
>> > setup is complete?

So with Russells answer below, this should be clarified and the
test_and_set_bit() is enough correct?

>> > Do we want a comment like: 'Returns true when the
>> > global package initialization is either under way or complete'?

I've forgot the whole annotation here.

>> IIRC, probe locking in the driver model is by per-driver locks, so
>> any particular driver won't probe more than one device at a time.

-michael

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-20 23:26 [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Michael Walle
                   ` (2 preceding siblings ...)
  2020-04-21 14:34 ` [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Andrew Lunn
@ 2020-04-21 15:25 ` Michael Walle
  2020-04-21 15:50   ` Andrew Lunn
  3 siblings, 1 reply; 14+ messages in thread
From: Michael Walle @ 2020-04-21 15:25 UTC (permalink / raw)
  To: linux-kernel, netdev
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Russell King,
	David S . Miller, Vladimir Oltean

Am 2020-04-21 01:26, schrieb Michael Walle:
> +
> +/* Represents a shared structure between different phydev's in the 
> same
> + * package, for example a quad PHY. See phy_package_join() and
> + * phy_package_leave().
> + */
> +struct phy_package_shared {
> +	int addr;
> +	refcount_t refcnt;
> +	unsigned long flags;
> +
> +	/* private data pointer */
> +	/* note that this pointer is shared between different phydevs and
> +	 * the user has to take care of appropriate locking.
> +	 */
> +	void *priv;

btw. how should a driver actually use this? I mean, it can allocate
memory if its still NULL but when will it be freed again. Do we need
a callback? Is there something better than a callback?

-michael

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 15:25 ` Michael Walle
@ 2020-04-21 15:50   ` Andrew Lunn
  2020-04-21 19:08     ` Michael Walle
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2020-04-21 15:50 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-kernel, netdev, Florian Fainelli, Heiner Kallweit,
	Russell King, David S . Miller, Vladimir Oltean

On Tue, Apr 21, 2020 at 05:25:19PM +0200, Michael Walle wrote:
> Am 2020-04-21 01:26, schrieb Michael Walle:
> > +
> > +/* Represents a shared structure between different phydev's in the same
> > + * package, for example a quad PHY. See phy_package_join() and
> > + * phy_package_leave().
> > + */
> > +struct phy_package_shared {
> > +	int addr;
> > +	refcount_t refcnt;
> > +	unsigned long flags;
> > +
> > +	/* private data pointer */
> > +	/* note that this pointer is shared between different phydevs and
> > +	 * the user has to take care of appropriate locking.
> > +	 */
> > +	void *priv;
> 
> btw. how should a driver actually use this? I mean, it can allocate
> memory if its still NULL but when will it be freed again. Do we need
> a callback? Is there something better than a callback?

Good point. phy_package_join() should take a size_t and do the
allocation. phy_package_leave() would then free it.

But since we don't have a user at the moment, maybe leave it out.

    Andrew

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 15:50   ` Andrew Lunn
@ 2020-04-21 19:08     ` Michael Walle
  2020-04-21 19:30       ` Andrew Lunn
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Walle @ 2020-04-21 19:08 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: linux-kernel, netdev, Florian Fainelli, Heiner Kallweit,
	Russell King, David S . Miller, Vladimir Oltean

Am 2020-04-21 17:50, schrieb Andrew Lunn:
> On Tue, Apr 21, 2020 at 05:25:19PM +0200, Michael Walle wrote:
>> Am 2020-04-21 01:26, schrieb Michael Walle:
>> > +
>> > +/* Represents a shared structure between different phydev's in the same
>> > + * package, for example a quad PHY. See phy_package_join() and
>> > + * phy_package_leave().
>> > + */
>> > +struct phy_package_shared {
>> > +	int addr;
>> > +	refcount_t refcnt;
>> > +	unsigned long flags;
>> > +
>> > +	/* private data pointer */
>> > +	/* note that this pointer is shared between different phydevs and
>> > +	 * the user has to take care of appropriate locking.
>> > +	 */
>> > +	void *priv;
>> 
>> btw. how should a driver actually use this? I mean, it can allocate
>> memory if its still NULL but when will it be freed again. Do we need
>> a callback? Is there something better than a callback?
> 
> Good point. phy_package_join() should take a size_t and do the
> allocation. phy_package_leave() would then free it.
> 
> But since we don't have a user at the moment, maybe leave it out.

Speaking of it. Does anyone have an idea how I could create the hwmon
device without the PHY device? At the moment it is attached to the
first PHY device and is removed when the PHY is removed, although
there might be still other PHYs in this package. Its unlikely to
happen though, but if someone has a good idea how to handle that,
I'd give it a try.

-michael

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 19:08     ` Michael Walle
@ 2020-04-21 19:30       ` Andrew Lunn
  2020-04-21 19:38         ` Russell King - ARM Linux admin
  2020-04-21 21:19         ` Michael Walle
  0 siblings, 2 replies; 14+ messages in thread
From: Andrew Lunn @ 2020-04-21 19:30 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-kernel, netdev, Florian Fainelli, Heiner Kallweit,
	Russell King, David S . Miller, Vladimir Oltean

> Speaking of it. Does anyone have an idea how I could create the hwmon
> device without the PHY device? At the moment it is attached to the
> first PHY device and is removed when the PHY is removed, although
> there might be still other PHYs in this package. Its unlikely to
> happen though, but if someone has a good idea how to handle that,
> I'd give it a try.

There is a somewhat similar problem with Marvell Ethernet switches and
their internal PHYs. The PHYs are the same as the discrete PHYs, and
the usual Marvell PHY driver is used. But there is only one
temperature sensor for the whole switch, and it is mapped into all the
PHYs. So we end up creating multiple hwmon devices for the one
temperature sensor, one per PHY.

You could take the same approach here. Each PHY exposes a hwmon
device?

Looking at
static struct device *
__hwmon_device_register(struct device *dev, const char *name, void *drvdata,
                        const struct hwmon_chip_info *chip,
                        const struct attribute_group **groups)

I think it is O.K. to pass dev as NULL. You don't have to associate it
to a device. So you could create the hwmon device as part of package
initialisation and put it into shared->priv.

	Andrew

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 19:30       ` Andrew Lunn
@ 2020-04-21 19:38         ` Russell King - ARM Linux admin
  2020-04-21 21:19         ` Michael Walle
  1 sibling, 0 replies; 14+ messages in thread
From: Russell King - ARM Linux admin @ 2020-04-21 19:38 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michael Walle, linux-kernel, netdev, Florian Fainelli,
	Heiner Kallweit, David S . Miller, Vladimir Oltean

On Tue, Apr 21, 2020 at 09:30:55PM +0200, Andrew Lunn wrote:
> > Speaking of it. Does anyone have an idea how I could create the hwmon
> > device without the PHY device? At the moment it is attached to the
> > first PHY device and is removed when the PHY is removed, although
> > there might be still other PHYs in this package. Its unlikely to
> > happen though, but if someone has a good idea how to handle that,
> > I'd give it a try.
> 
> There is a somewhat similar problem with Marvell Ethernet switches and
> their internal PHYs. The PHYs are the same as the discrete PHYs, and
> the usual Marvell PHY driver is used. But there is only one
> temperature sensor for the whole switch, and it is mapped into all the
> PHYs. So we end up creating multiple hwmon devices for the one
> temperature sensor, one per PHY.

And sometimes we really mess it up - like on the 88e6141:

cp1configspacef4000000mdio12a200switch04mdio14-mdio-e
Adapter: MDIO adapter
temp1:        -75.0°C

because DSA forces the 6390 PHY ID for this PHY, and the marvell
driver tries to drive the PHY as if it's a different switch, so
we end up reading a register that isn't meaningful.

So, imho, the current approach isn't as good as you think it is.

That aside from wasting memory allocating multiple sensors when
there's really only one.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

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

* Re: [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs
  2020-04-21 19:30       ` Andrew Lunn
  2020-04-21 19:38         ` Russell King - ARM Linux admin
@ 2020-04-21 21:19         ` Michael Walle
  1 sibling, 0 replies; 14+ messages in thread
From: Michael Walle @ 2020-04-21 21:19 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: linux-kernel, netdev, Florian Fainelli, Heiner Kallweit,
	Russell King, David S . Miller, Vladimir Oltean

Am 2020-04-21 21:30, schrieb Andrew Lunn:
>> Speaking of it. Does anyone have an idea how I could create the hwmon
>> device without the PHY device? At the moment it is attached to the
>> first PHY device and is removed when the PHY is removed, although
>> there might be still other PHYs in this package. Its unlikely to
>> happen though, but if someone has a good idea how to handle that,
>> I'd give it a try.
> 
> There is a somewhat similar problem with Marvell Ethernet switches and
> their internal PHYs. The PHYs are the same as the discrete PHYs, and
> the usual Marvell PHY driver is used. But there is only one
> temperature sensor for the whole switch, and it is mapped into all the
> PHYs. So we end up creating multiple hwmon devices for the one
> temperature sensor, one per PHY.
> 
> You could take the same approach here. Each PHY exposes a hwmon
> device?
> 
> Looking at
> static struct device *
> __hwmon_device_register(struct device *dev, const char *name, void 
> *drvdata,
>                         const struct hwmon_chip_info *chip,
>                         const struct attribute_group **groups)
> 
> I think it is O.K. to pass dev as NULL. You don't have to associate it
> to a device. So you could create the hwmon device as part of package
> initialisation and put it into shared->priv.

I actually tried that before writing my mail. Have a look at commit
59df4f4e8e0b ("hwmon: (core) check parent dev != NULL when chip != 
NULL")

and the corresponding discussion here:
   https://patchwork.kernel.org/patch/10381759/

And if I'd had to choose, I'd prefer having one hwmon device on the
first PHY (with its drawback) rather than having it four times.

-michael

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

* Re: [RFC PATCH net-next 3/3] net: phy: mscc: use phy_package_shared
  2020-04-20 23:26 ` [RFC PATCH net-next 3/3] net: phy: mscc: " Michael Walle
@ 2020-04-23 12:27   ` Vladimir Oltean
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2020-04-23 12:27 UTC (permalink / raw)
  To: Michael Walle
  Cc: lkml, netdev, Andrew Lunn, Florian Fainelli, Heiner Kallweit,
	Russell King, David S . Miller, Vladimir Oltean

Hi Michael,

On Tue, 21 Apr 2020 at 02:28, Michael Walle <michael@walle.cc> wrote:
>
> Use the new phy_package_shared common storage to ease the package
> initialization and to access the global registers.
>
> This was only compile-time tested!
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---

I happen to have a board with a VSC8514 PHY. For this driver, only the "Fast
Link Failure Indication" feature seems to be configured via
phy_base_write/__phy_package_write. Functionally I can't test it, because the
GPIO9 pin is not wired on my board. But I could at least check that the value
that should be written can be read back from all PHYs in the package:

Microsemi GE VSC8514 SyncE 0000:00:00.3:10: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
swp0: PHY [0000:00:00.3:10] driver [Microsemi GE VSC8514 SyncE] (irq=POLL)
Microsemi GE VSC8514 SyncE 0000:00:00.3:11: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
0000:00:00.5 swp1: PHY [0000:00:00.3:11] driver [Microsemi GE VSC8514
SyncE] (irq=POLL)
Microsemi GE VSC8514 SyncE 0000:00:00.3:12: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
0000:00:00.5 swp2: PHY [0000:00:00.3:12] driver [Microsemi GE VSC8514
SyncE] (irq=POLL)
Microsemi GE VSC8514 SyncE 0000:00:00.3:13: reading 0x400f from
MSCC_PHY_MAC_CFG_FASTLINK
0000:00:00.5 swp3: PHY [0000:00:00.3:13] driver [Microsemi GE VSC8514
SyncE] (irq=POLL)

So assuming that the feature worked before, it looks like there is no
regression. Of course, the PHY still passes traffic just fine.

So you can add my:

Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

>  drivers/net/phy/mscc/mscc.h      |  1 -
>  drivers/net/phy/mscc/mscc_main.c | 99 ++++++++++----------------------
>  2 files changed, 29 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index 030bf8b600df..acdd8ee61a39 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -353,7 +353,6 @@ struct vsc8531_private {
>         const struct vsc85xx_hw_stat *hw_stats;
>         u64 *stats;
>         int nstats;
> -       bool pkg_init;
>         /* For multiple port PHYs; the MDIO address of the base PHY in the
>          * package.
>          */
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 5391acdece05..382b56064de9 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -691,27 +691,23 @@ static int vsc85xx_eee_init_seq_set(struct phy_device *phydev)
>  /* phydev->bus->mdio_lock should be locked when using this function */
>  static int phy_base_write(struct phy_device *phydev, u32 regnum, u16 val)
>  {
> -       struct vsc8531_private *priv = phydev->priv;
> -
>         if (unlikely(!mutex_is_locked(&phydev->mdio.bus->mdio_lock))) {
>                 dev_err(&phydev->mdio.dev, "MDIO bus lock not held!\n");
>                 dump_stack();
>         }
>
> -       return __mdiobus_write(phydev->mdio.bus, priv->base_addr, regnum, val);
> +       return __phy_package_write(phydev, regnum, val);
>  }
>
>  /* phydev->bus->mdio_lock should be locked when using this function */
>  static int phy_base_read(struct phy_device *phydev, u32 regnum)
>  {
> -       struct vsc8531_private *priv = phydev->priv;
> -
>         if (unlikely(!mutex_is_locked(&phydev->mdio.bus->mdio_lock))) {
>                 dev_err(&phydev->mdio.dev, "MDIO bus lock not held!\n");
>                 dump_stack();
>         }
>
> -       return __mdiobus_read(phydev->mdio.bus, priv->base_addr, regnum);
> +       return __phy_package_read(phydev, regnum);
>  }
>
>  /* bus->mdio_lock should be locked when using this function */
> @@ -1287,65 +1283,36 @@ static int vsc8584_config_pre_init(struct phy_device *phydev)
>         return ret;
>  }
>
> -/* Check if one PHY has already done the init of the parts common to all PHYs
> - * in the Quad PHY package.
> - */
> -static bool vsc8584_is_pkg_init(struct phy_device *phydev, bool reversed)
> +static void vsc8584_get_base_addr(struct phy_device *phydev)
>  {
> -       struct mii_bus *bus = phydev->mdio.bus;
> -       struct vsc8531_private *vsc8531;
> -       struct phy_device *phy;
> -       int i, addr;
> -
> -       /* VSC8584 is a Quad PHY */
> -       for (i = 0; i < 4; i++) {
> -               vsc8531 = phydev->priv;
> -
> -               if (reversed)
> -                       addr = vsc8531->base_addr - i;
> -               else
> -                       addr = vsc8531->base_addr + i;
> -
> -               phy = mdiobus_get_phy(bus, addr);
> -               if (!phy)
> -                       continue;
> +       struct vsc8531_private *vsc8531 = phydev->priv;
> +       u16 val, addr;
>
> -               if ((phy->phy_id & phydev->drv->phy_id_mask) !=
> -                   (phydev->drv->phy_id & phydev->drv->phy_id_mask))
> -                       continue;
> +       mutex_lock(&phydev->mdio.bus->mdio_lock);
> +       __phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
>
> -               vsc8531 = phy->priv;
> +       addr = __phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_4);
> +       addr >>= PHY_CNTL_4_ADDR_POS;
>
> -               if (vsc8531 && vsc8531->pkg_init)
> -                       return true;
> -       }
> +       val = __phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
>
> -       return false;
> +       if (val & PHY_ADDR_REVERSED)
> +               vsc8531->base_addr = phydev->mdio.addr + addr;
> +       else
> +               vsc8531->base_addr = phydev->mdio.addr - addr;
> +       mutex_unlock(&phydev->mdio.bus->mdio_lock);
>  }
>
>  static int vsc8584_config_init(struct phy_device *phydev)
>  {
>         struct vsc8531_private *vsc8531 = phydev->priv;
> -       u16 addr, val;
>         int ret, i;
> +       u16 val;
>
>         phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>
>         mutex_lock(&phydev->mdio.bus->mdio_lock);
>
> -       __mdiobus_write(phydev->mdio.bus, phydev->mdio.addr,
> -                       MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
> -       addr = __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr,
> -                             MSCC_PHY_EXT_PHY_CNTL_4);
> -       addr >>= PHY_CNTL_4_ADDR_POS;
> -
> -       val = __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr,
> -                            MSCC_PHY_ACTIPHY_CNTL);
> -       if (val & PHY_ADDR_REVERSED)
> -               vsc8531->base_addr = phydev->mdio.addr + addr;
> -       else
> -               vsc8531->base_addr = phydev->mdio.addr - addr;
> -
>         /* Some parts of the init sequence are identical for every PHY in the
>          * package. Some parts are modifying the GPIO register bank which is a
>          * set of registers that are affecting all PHYs, a few resetting the
> @@ -1359,7 +1326,7 @@ static int vsc8584_config_init(struct phy_device *phydev)
>          * do the correct init sequence for all PHYs that are package-critical
>          * in this pre-init function.
>          */
> -       if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0)) {
> +       if (phy_package_init_once(phydev)) {
>                 /* The following switch statement assumes that the lowest
>                  * nibble of the phy_id_mask is always 0. This works because
>                  * the lowest nibble of the PHY_ID's below are also 0.
> @@ -1388,8 +1355,6 @@ static int vsc8584_config_init(struct phy_device *phydev)
>                         goto err;
>         }
>
> -       vsc8531->pkg_init = true;
> -
>         phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
>                        MSCC_PHY_PAGE_EXTENDED_GPIO);
>
> @@ -1427,7 +1392,8 @@ static int vsc8584_config_init(struct phy_device *phydev)
>
>         /* Disable SerDes for 100Base-FX */
>         ret = vsc8584_cmd(phydev, PROC_CMD_FIBER_MEDIA_CONF |
> -                         PROC_CMD_FIBER_PORT(addr) | PROC_CMD_FIBER_DISABLE |
> +                         PROC_CMD_FIBER_PORT(vsc8531->base_addr) |
> +                         PROC_CMD_FIBER_DISABLE |
>                           PROC_CMD_READ_MOD_WRITE_PORT |
>                           PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_100BASE_FX);
>         if (ret)
> @@ -1435,7 +1401,8 @@ static int vsc8584_config_init(struct phy_device *phydev)
>
>         /* Disable SerDes for 1000Base-X */
>         ret = vsc8584_cmd(phydev, PROC_CMD_FIBER_MEDIA_CONF |
> -                         PROC_CMD_FIBER_PORT(addr) | PROC_CMD_FIBER_DISABLE |
> +                         PROC_CMD_FIBER_PORT(vsc8531->base_addr) |
> +                         PROC_CMD_FIBER_DISABLE |
>                           PROC_CMD_READ_MOD_WRITE_PORT |
>                           PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_1000BASE_X);
>         if (ret)
> @@ -1750,26 +1717,14 @@ static int vsc8514_config_init(struct phy_device *phydev)
>  {
>         struct vsc8531_private *vsc8531 = phydev->priv;
>         unsigned long deadline;
> -       u16 val, addr;
>         int ret, i;
> +       u16 val;
>         u32 reg;
>
>         phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>
>         mutex_lock(&phydev->mdio.bus->mdio_lock);
>
> -       __phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_EXTENDED);
> -
> -       addr = __phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_4);
> -       addr >>= PHY_CNTL_4_ADDR_POS;
> -
> -       val = __phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
> -
> -       if (val & PHY_ADDR_REVERSED)
> -               vsc8531->base_addr = phydev->mdio.addr + addr;
> -       else
> -               vsc8531->base_addr = phydev->mdio.addr - addr;
> -
>         /* Some parts of the init sequence are identical for every PHY in the
>          * package. Some parts are modifying the GPIO register bank which is a
>          * set of registers that are affecting all PHYs, a few resetting the
> @@ -1781,11 +1736,9 @@ static int vsc8514_config_init(struct phy_device *phydev)
>          * do the correct init sequence for all PHYs that are package-critical
>          * in this pre-init function.
>          */
> -       if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0))
> +       if (phy_package_init_once(phydev))
>                 vsc8514_config_pre_init(phydev);
>
> -       vsc8531->pkg_init = true;
> -
>         phy_base_write(phydev, MSCC_EXT_PAGE_ACCESS,
>                        MSCC_PHY_PAGE_EXTENDED_GPIO);
>
> @@ -1991,6 +1944,9 @@ static int vsc8514_probe(struct phy_device *phydev)
>
>         phydev->priv = vsc8531;
>
> +       vsc8584_get_base_addr(phydev);
> +       devm_phy_package_join(&phydev->mdio.dev, phydev, vsc8531->base_addr);
> +
>         vsc8531->nleds = 4;
>         vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
>         vsc8531->hw_stats = vsc85xx_hw_stats;
> @@ -2046,6 +2002,9 @@ static int vsc8584_probe(struct phy_device *phydev)
>
>         phydev->priv = vsc8531;
>
> +       vsc8584_get_base_addr(phydev);
> +       devm_phy_package_join(&phydev->mdio.dev, phydev, vsc8531->base_addr);
> +
>         vsc8531->nleds = 4;
>         vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
>         vsc8531->hw_stats = vsc8584_hw_stats;
> --
> 2.20.1
>

Thanks,
-Vladimir

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

end of thread, other threads:[~2020-04-23 12:27 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-20 23:26 [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Michael Walle
2020-04-20 23:26 ` [RFC PATCH net-next 2/3] net: phy: bcm54140: use phy_package_shared Michael Walle
2020-04-20 23:26 ` [RFC PATCH net-next 3/3] net: phy: mscc: " Michael Walle
2020-04-23 12:27   ` Vladimir Oltean
2020-04-21 14:34 ` [RFC PATCH net-next 1/3] net: phy: add concept of shared storage for PHYs Andrew Lunn
2020-04-21 14:43   ` Russell King - ARM Linux admin
2020-04-21 14:52     ` Andrew Lunn
2020-04-21 15:20       ` Michael Walle
2020-04-21 15:25 ` Michael Walle
2020-04-21 15:50   ` Andrew Lunn
2020-04-21 19:08     ` Michael Walle
2020-04-21 19:30       ` Andrew Lunn
2020-04-21 19:38         ` Russell King - ARM Linux admin
2020-04-21 21:19         ` Michael Walle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).