[PATCH net-next 1/3] net: phy: broadcom: add helper to write/read RDB registers

[PATCH net-next 1/3] net: phy: broadcom: add helper to write/read RDB registers

[Michael Walle]

RDB regsiters are used on newer Broadcom PHYs. Add helper to read, write
and modify these registers.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm-phy-lib.c | 80 +++++++++++++++++++++++++++++++++++
 drivers/net/phy/bcm-phy-lib.h |  9 ++++
 include/linux/brcmphy.h       |  3 ++
 3 files changed, 92 insertions(+)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index e77b274a09fd..d5f9a2701989 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -155,6 +155,86 @@ int bcm_phy_write_shadow(struct phy_device *phydev, u16 shadow,
 }
 EXPORT_SYMBOL_GPL(bcm_phy_write_shadow);
 
+int __bcm_phy_read_rdb(struct phy_device *phydev, u16 rdb)
+{
+	int val;
+
+	val = __phy_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	if (val < 0)
+		return val;
+
+	return __phy_read(phydev, MII_BCM54XX_RDB_DATA);
+}
+EXPORT_SYMBOL_GPL(__bcm_phy_read_rdb);
+
+int bcm_phy_read_rdb(struct phy_device *phydev, u16 rdb)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = __bcm_phy_read_rdb(phydev, rdb);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_read_rdb);
+
+int __bcm_phy_write_rdb(struct phy_device *phydev, u16 rdb, u16 val)
+{
+	int ret;
+
+	ret = __phy_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	if (ret < 0)
+		return ret;
+
+	return __phy_write(phydev, MII_BCM54XX_RDB_DATA, val);
+}
+EXPORT_SYMBOL_GPL(__bcm_phy_write_rdb);
+
+int bcm_phy_write_rdb(struct phy_device *phydev, u16 rdb, u16 val)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = __bcm_phy_write_rdb(phydev, rdb, val);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_write_rdb);
+
+int __bcm_phy_modify_rdb(struct phy_device *phydev, u16 rdb, u16 mask, u16 set)
+{
+	int new, ret;
+
+	ret = __phy_write(phydev, MII_BCM54XX_RDB_ADDR, rdb);
+	if (ret < 0)
+		return ret;
+
+	ret = __phy_read(phydev, MII_BCM54XX_RDB_DATA);
+	if (ret < 0)
+		return ret;
+
+	new = (ret & ~mask) | set;
+	if (new == ret)
+		return 0;
+
+	return __phy_write(phydev, MII_BCM54XX_RDB_DATA, new);
+}
+EXPORT_SYMBOL_GPL(__bcm_phy_modify_rdb);
+
+int bcm_phy_modify_rdb(struct phy_device *phydev, u16 rdb, u16 mask, u16 set)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = __bcm_phy_modify_rdb(phydev, rdb, mask, set);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_modify_rdb);
+
 int bcm_phy_enable_apd(struct phy_device *phydev, bool dll_pwr_down)
 {
 	int val;
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 129df819be8c..4d3de91cda6c 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -48,6 +48,15 @@ int bcm_phy_write_shadow(struct phy_device *phydev, u16 shadow,
 			 u16 val);
 int bcm_phy_read_shadow(struct phy_device *phydev, u16 shadow);
 
+int __bcm_phy_write_rdb(struct phy_device *phydev, u16 rdb, u16 val);
+int bcm_phy_write_rdb(struct phy_device *phydev, u16 rdb, u16 val);
+int __bcm_phy_read_rdb(struct phy_device *phydev, u16 rdb);
+int bcm_phy_read_rdb(struct phy_device *phydev, u16 rdb);
+int __bcm_phy_modify_rdb(struct phy_device *phydev, u16 rdb, u16 mask,
+			 u16 set);
+int bcm_phy_modify_rdb(struct phy_device *phydev, u16 rdb, u16 mask,
+		       u16 set);
+
 int bcm_phy_ack_intr(struct phy_device *phydev);
 int bcm_phy_config_intr(struct phy_device *phydev);
 
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 6462c5447872..e14f78b3c8a4 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -114,6 +114,9 @@
 #define MII_BCM54XX_SHD_VAL(x)	((x & 0x1f) << 10)
 #define MII_BCM54XX_SHD_DATA(x)	((x & 0x3ff) << 0)
 
+#define MII_BCM54XX_RDB_ADDR	0x1e
+#define MII_BCM54XX_RDB_DATA	0x1f
+
 /*
  * AUXILIARY CONTROL SHADOW ACCESS REGISTERS.  (PHY REG 0x18)
  */
-- 
2.20.1

[PATCH net-next 2/3] net: phy: add Broadcom BCM54140 support

[Michael Walle]

The Broadcom BCM54140 is a Quad SGMII/QSGMII Copper/Fiber Gigabit
Ethernet transceiver.

This also adds support for tunables to set and get downshift and
energy detect auto power-down.

The PHY has four ports and each port has its own PHY address.
There are per-port registers as well as global registers.
Unfortunately, the global registers can only be accessed by reading
and writing from/to the PHY address of the first port. Further,
there is no way to find out what port you actually are by just
reading the per-port registers. We therefore, have to scan the
bus on the PHY probe to determine the port and thus what address
we need to access the global registers.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/Kconfig    |  10 +
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/bcm54140.c | 478 +++++++++++++++++++++++++++++++++++++
 include/linux/brcmphy.h    |   1 +
 4 files changed, 490 insertions(+)
 create mode 100644 drivers/net/phy/bcm54140.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3fa33d27eeba..cb7936b577de 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -346,6 +346,16 @@ config BROADCOM_PHY
 	  Currently supports the BCM5411, BCM5421, BCM5461, BCM54616S, BCM5464,
 	  BCM5481, BCM54810 and BCM5482 PHYs.
 
+config BCM54140_PHY
+	tristate "Broadcom BCM54140 PHY"
+	depends on PHYLIB
+	select BCM_NET_PHYLIB
+	help
+	  Support the Broadcom BCM54140 Quad SGMII/QSGMII PHY.
+
+	  This driver also supports the hardware monitoring of this PHY and
+	  exposes voltage and temperature sensors.
+
 config BCM84881_PHY
 	tristate "Broadcom BCM84881 PHY"
 	depends on PHYLIB
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 2f5c7093a65b..cd345b75d127 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_BCM87XX_PHY)	+= bcm87xx.o
 obj-$(CONFIG_BCM_CYGNUS_PHY)	+= bcm-cygnus.o
 obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o
 obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
+obj-$(CONFIG_BCM54140_PHY)	+= bcm54140.o
 obj-$(CONFIG_BCM84881_PHY)	+= bcm84881.o
 obj-$(CONFIG_CICADA_PHY)	+= cicada.o
 obj-$(CONFIG_CORTINA_PHY)	+= cortina.o
diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
new file mode 100644
index 000000000000..97465491b41b
--- /dev/null
+++ b/drivers/net/phy/bcm54140.c
@@ -0,0 +1,478 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Broadcom BCM54140 Quad SGMII/QSGMII Copper/Fiber Gigabit PHY
+ *
+ * Copyright (c) 2020 Michael Walle <michael@walle.cc>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/brcmphy.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+
+#include "bcm-phy-lib.h"
+
+/* RDB per-port registers
+ */
+#define BCM54140_RDB_ISR		0x00a	/* interrupt status */
+#define BCM54140_RDB_IMR		0x00b	/* interrupt mask */
+#define  BCM54140_RDB_INT_LINK		BIT(1)	/* link status changed */
+#define  BCM54140_RDB_INT_SPEED		BIT(2)	/* link speed change */
+#define  BCM54140_RDB_INT_DUPLEX	BIT(3)	/* duplex mode changed */
+#define BCM54140_RDB_SPARE1		0x012	/* spare control 1 */
+#define  BCM54140_RDB_SPARE1_LSLM	BIT(2)	/* link speed LED mode */
+#define BCM54140_RDB_SPARE2		0x014	/* spare control 2 */
+#define  BCM54140_RDB_SPARE2_WS_RTRY_DIS BIT(8) /* wirespeed retry disable */
+#define  BCM54140_RDB_SPARE2_WS_RTRY_LIMIT GENMASK(4, 2) /* retry limit */
+#define BCM54140_RDB_SPARE3		0x015	/* spare control 3 */
+#define  BCM54140_RDB_SPARE3_BIT0	BIT(0)
+#define BCM54140_RDB_LED_CTRL		0x019	/* LED control */
+#define  BCM54140_RDB_LED_CTRL_ACTLINK0	BIT(4)
+#define  BCM54140_RDB_LED_CTRL_ACTLINK1	BIT(8)
+#define BCM54140_RDB_C_APWR		0x01a	/* auto power down control */
+#define  BCM54140_RDB_C_APWR_SINGLE_PULSE	BIT(8)	/* single pulse */
+#define  BCM54140_RDB_C_APWR_APD_MODE_DIS	0 /* ADP disable */
+#define  BCM54140_RDB_C_APWR_APD_MODE_EN	1 /* ADP enable */
+#define  BCM54140_RDB_C_APWR_APD_MODE_DIS2	2 /* ADP disable */
+#define  BCM54140_RDB_C_APWR_APD_MODE_EN_ANEG	3 /* ADP enable w/ aneg */
+#define  BCM54140_RDB_C_APWR_APD_MODE_MASK	GENMASK(6, 5)
+#define  BCM54140_RDB_C_APWR_SLP_TIM_MASK BIT(4)/* sleep timer */
+#define  BCM54140_RDB_C_APWR_SLP_TIM_2_7 0	/* 2.7s */
+#define  BCM54140_RDB_C_APWR_SLP_TIM_5_4 1	/* 5.4s */
+#define BCM54140_RDB_C_PWR		0x02a	/* copper power control */
+#define  BCM54140_RDB_C_PWR_ISOLATE	BIT(5)	/* super isolate mode */
+#define BCM54140_RDB_C_MISC_CTRL	0x02f	/* misc copper control */
+#define  BCM54140_RDB_C_MISC_CTRL_WS_EN BIT(4)	/* wirespeed enable */
+
+/* RDB global registers
+ */
+#define BCM54140_RDB_TOP_IMR		0x82d	/* interrupt mask */
+#define  BCM54140_RDB_TOP_IMR_PORT0	BIT(4)
+#define  BCM54140_RDB_TOP_IMR_PORT1	BIT(5)
+#define  BCM54140_RDB_TOP_IMR_PORT2	BIT(6)
+#define  BCM54140_RDB_TOP_IMR_PORT3	BIT(7)
+
+#define BCM54140_DEFAULT_DOWNSHIFT 5
+#define BCM54140_MAX_DOWNSHIFT 9
+
+struct bcm54140_phy_priv {
+	int port;
+	int base_addr;
+};
+
+static int bcm54140_phy_base_read_rdb(struct phy_device *phydev, u16 rdb)
+{
+	struct bcm54140_phy_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);
+	if (ret < 0)
+		goto out;
+
+	ret = __mdiobus_read(bus, priv->base_addr, MII_BCM54XX_RDB_DATA);
+
+out:
+	mutex_unlock(&bus->mdio_lock);
+	return ret;
+}
+
+static int bcm54140_phy_base_write_rdb(struct phy_device *phydev,
+				       u16 rdb, u16 val)
+{
+	struct bcm54140_phy_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);
+	if (ret < 0)
+		goto out;
+
+	ret = __mdiobus_write(bus, priv->base_addr, MII_BCM54XX_RDB_DATA, val);
+
+out:
+	mutex_unlock(&bus->mdio_lock);
+	return ret;
+}
+
+static int bcm54140_b0_workaround(struct phy_device *phydev)
+{
+	int spare3;
+	int ret;
+
+	spare3 = bcm_phy_read_rdb(phydev, BCM54140_RDB_SPARE3);
+	if (spare3 < 0)
+		return spare3;
+
+	spare3 &= ~BCM54140_RDB_SPARE3_BIT0;
+
+	ret = bcm_phy_write_rdb(phydev, BCM54140_RDB_SPARE3, spare3);
+	if (ret)
+		return ret;
+
+	ret = phy_modify(phydev, MII_BMCR, 0, BMCR_PDOWN);
+	if (ret)
+		return ret;
+
+	ret = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
+	if (ret)
+		return ret;
+
+	spare3 |= BCM54140_RDB_SPARE3_BIT0;
+
+	return bcm_phy_write_rdb(phydev, BCM54140_RDB_SPARE3, spare3);
+}
+
+/* The BCM54140 is a quad PHY where only the first port has access to the
+ * global register. Thus we need to find out its PHY address.
+ *
+ */
+static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
+{
+	struct bcm54140_phy_priv *priv = phydev->priv;
+	struct mii_bus *bus = phydev->mdio.bus;
+	int addr, min_addr, max_addr;
+	int step = 1;
+	u32 phy_id;
+	int tmp;
+
+	min_addr = phydev->mdio.addr;
+	max_addr = phydev->mdio.addr;
+	addr = phydev->mdio.addr;
+
+	/* We scan forward and backwards and look for PHYs which have the
+	 * same phy_id like we do. Step 1 will scan forward, step 2
+	 * backwards. Once we are finished, we have a min_addr and
+	 * max_addr which resembles the range of PHY addresses of the same
+	 * type of PHY. There is one caveat; there may be many PHYs of
+	 * the same type, but we know that each PHY takes exactly 4
+	 * consecutive addresses. Therefore we can deduce our offset
+	 * to the base address of this quad PHY.
+	 */
+
+	while (1) {
+		if (step == 3) {
+			break;
+		} else if (step == 1) {
+			max_addr = addr;
+			addr++;
+		} else {
+			min_addr = addr;
+			addr--;
+		}
+
+		if (addr < 0 || addr >= PHY_MAX_ADDR) {
+			addr = phydev->mdio.addr;
+			step++;
+			continue;
+		}
+
+		/* read the PHY id */
+		tmp = mdiobus_read(bus, addr, MII_PHYSID1);
+		if (tmp < 0)
+			return tmp;
+		phy_id = tmp << 16;
+		tmp = mdiobus_read(bus, addr, MII_PHYSID2);
+		if (tmp < 0)
+			return tmp;
+		phy_id |= tmp;
+
+		/* see if it is still the same PHY */
+		if ((phy_id & phydev->drv->phy_id_mask) !=
+		    (phydev->drv->phy_id & phydev->drv->phy_id_mask)) {
+			addr = phydev->mdio.addr;
+			step++;
+		}
+	}
+
+	/* The range we get should be a multiple of four. Please note that both
+	 * the min_addr and max_addr are inclusive. So we have to add one if we
+	 * subtract them.
+	 */
+	if ((max_addr - min_addr + 1) % 4) {
+		dev_err(&phydev->mdio.dev,
+			"Detected Quad PHY IDs %d..%d doesn't make sense.\n",
+			min_addr, max_addr);
+		return -EINVAL;
+	}
+
+	priv->port = (phydev->mdio.addr - min_addr) % 4;
+	priv->base_addr = phydev->mdio.addr - priv->port;
+
+	return 0;
+}
+
+static int bcm54140_phy_probe(struct phy_device *phydev)
+{
+	struct bcm54140_phy_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	ret = bcm54140_get_base_addr_and_port(phydev);
+	if (ret)
+		return ret;
+
+	dev_info(&phydev->mdio.dev,
+		 "probed (port %d, base PHY address %d)\n",
+		 priv->port, priv->base_addr);
+
+	return 0;
+}
+
+static int bcm54140_config_init(struct phy_device *phydev)
+{
+	u16 reg = 0xffff;
+	int ret;
+
+	/* Apply hardware errata */
+	ret = bcm54140_b0_workaround(phydev);
+	if (ret)
+		return ret;
+
+	/* unmask events we are interested in. */
+	reg &= ~(BCM54140_RDB_INT_DUPLEX |
+		 BCM54140_RDB_INT_SPEED |
+		 BCM54140_RDB_INT_LINK);
+	ret = bcm_phy_write_rdb(phydev, BCM54140_RDB_IMR, reg);
+	if (ret)
+		return ret;
+
+	/* LED1=LINKSPD[1], LED2=LINKSPD[2], LED3=ACTIVITY */
+	ret = bcm_phy_modify_rdb(phydev, BCM54140_RDB_SPARE1,
+				 0, BCM54140_RDB_SPARE1_LSLM);
+	if (ret)
+		return ret;
+
+	ret = bcm_phy_modify_rdb(phydev, BCM54140_RDB_LED_CTRL,
+				 0, BCM54140_RDB_LED_CTRL_ACTLINK0);
+	if (ret)
+		return ret;
+
+	/* disable super isolate mode */
+	return bcm_phy_modify_rdb(phydev, BCM54140_RDB_C_PWR,
+				  BCM54140_RDB_C_PWR_ISOLATE, 0);
+}
+
+int bcm54140_phy_did_interrupt(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = bcm_phy_read_rdb(phydev, BCM54140_RDB_ISR);
+
+	return (ret < 0) ? 0 : ret;
+}
+
+int bcm54140_phy_ack_intr(struct phy_device *phydev)
+{
+	int reg;
+
+	/* clear pending interrupts */
+	reg = bcm_phy_read_rdb(phydev, BCM54140_RDB_ISR);
+	if (reg < 0)
+		return reg;
+
+	return 0;
+}
+
+int bcm54140_phy_config_intr(struct phy_device *phydev)
+{
+	struct bcm54140_phy_priv *priv = phydev->priv;
+	static const u16 port_to_imr_bit[] = {
+		BCM54140_RDB_TOP_IMR_PORT0, BCM54140_RDB_TOP_IMR_PORT1,
+		BCM54140_RDB_TOP_IMR_PORT2, BCM54140_RDB_TOP_IMR_PORT3,
+	};
+	int reg;
+
+	if (priv->port >= ARRAY_SIZE(port_to_imr_bit))
+		return -EINVAL;
+
+	reg = bcm54140_phy_base_read_rdb(phydev, BCM54140_RDB_TOP_IMR);
+	if (reg < 0)
+		return reg;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		reg &= ~port_to_imr_bit[priv->port];
+	else
+		reg |= port_to_imr_bit[priv->port];
+
+	return bcm54140_phy_base_write_rdb(phydev, BCM54140_RDB_TOP_IMR, reg);
+}
+
+static int bcm54140_get_downshift(struct phy_device *phydev, u8 *data)
+{
+	int val;
+
+	val = bcm_phy_read_rdb(phydev, BCM54140_RDB_C_MISC_CTRL);
+	if (val < 0)
+		return val;
+
+	if (!(val & BCM54140_RDB_C_MISC_CTRL_WS_EN)) {
+		*data = DOWNSHIFT_DEV_DISABLE;
+		return 0;
+	}
+
+	val = bcm_phy_read_rdb(phydev, BCM54140_RDB_SPARE2);
+	if (val < 0)
+		return val;
+
+	if (val & BCM54140_RDB_SPARE2_WS_RTRY_DIS)
+		*data = 1;
+	else
+		*data = FIELD_GET(BCM54140_RDB_SPARE2_WS_RTRY_LIMIT, val) + 2;
+
+	return 0;
+}
+
+static int bcm54140_set_downshift(struct phy_device *phydev, u8 cnt)
+{
+	u16 mask, set;
+	int ret;
+
+	if (cnt > BCM54140_MAX_DOWNSHIFT && cnt != DOWNSHIFT_DEV_DEFAULT_COUNT)
+		return -EINVAL;
+
+	if (!cnt)
+		return bcm_phy_modify_rdb(phydev, BCM54140_RDB_C_MISC_CTRL,
+					  BCM54140_RDB_C_MISC_CTRL_WS_EN, 0);
+
+	if (cnt == DOWNSHIFT_DEV_DEFAULT_COUNT)
+		cnt = BCM54140_DEFAULT_DOWNSHIFT;
+
+	if (cnt == 1) {
+		mask = 0;
+		set = BCM54140_RDB_SPARE2_WS_RTRY_DIS;
+	} else {
+		mask = BCM54140_RDB_SPARE2_WS_RTRY_DIS;
+		mask |= BCM54140_RDB_SPARE2_WS_RTRY_LIMIT;
+		set = FIELD_PREP(BCM54140_RDB_SPARE2_WS_RTRY_LIMIT, cnt - 2);
+	}
+	ret = bcm_phy_modify_rdb(phydev, BCM54140_RDB_SPARE2,
+				 mask, set);
+	if (ret)
+		return ret;
+
+	return bcm_phy_modify_rdb(phydev, BCM54140_RDB_C_MISC_CTRL,
+				  0, BCM54140_RDB_C_MISC_CTRL_WS_EN);
+}
+
+static int bcm54140_get_edpd(struct phy_device *phydev, u16 *tx_interval)
+{
+	int val;
+
+	val = bcm_phy_read_rdb(phydev, BCM54140_RDB_C_APWR);
+	if (val < 0)
+		return val;
+
+	switch (FIELD_GET(BCM54140_RDB_C_APWR_APD_MODE_MASK, val)) {
+	case BCM54140_RDB_C_APWR_APD_MODE_DIS:
+	case BCM54140_RDB_C_APWR_APD_MODE_DIS2:
+		*tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
+		break;
+	case BCM54140_RDB_C_APWR_APD_MODE_EN:
+	case BCM54140_RDB_C_APWR_APD_MODE_EN_ANEG:
+		switch (FIELD_GET(BCM54140_RDB_C_APWR_SLP_TIM_MASK, val)) {
+		case BCM54140_RDB_C_APWR_SLP_TIM_2_7:
+			*tx_interval = 2700;
+			break;
+		case BCM54140_RDB_C_APWR_SLP_TIM_5_4:
+			*tx_interval = 5400;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int bcm54140_set_edpd(struct phy_device *phydev, u16 tx_interval)
+{
+	u16 mask, set;
+
+	mask = BCM54140_RDB_C_APWR_APD_MODE_MASK;
+	if (tx_interval == ETHTOOL_PHY_EDPD_DISABLE)
+		set = FIELD_PREP(BCM54140_RDB_C_APWR_APD_MODE_MASK,
+				 BCM54140_RDB_C_APWR_APD_MODE_DIS);
+	else
+		set = FIELD_PREP(BCM54140_RDB_C_APWR_APD_MODE_MASK,
+				 BCM54140_RDB_C_APWR_APD_MODE_EN_ANEG);
+
+	/* enable single pulse mode */
+	set |= BCM54140_RDB_C_APWR_SINGLE_PULSE;
+
+	/* set sleep timer */
+	mask |= BCM54140_RDB_C_APWR_SLP_TIM_MASK;
+	switch (tx_interval) {
+	case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
+	case ETHTOOL_PHY_EDPD_DISABLE:
+	case 2700:
+		set |= BCM54140_RDB_C_APWR_SLP_TIM_2_7;
+		break;
+	case 5400:
+		set |= BCM54140_RDB_C_APWR_SLP_TIM_5_4;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return bcm_phy_modify_rdb(phydev, BCM54140_RDB_C_APWR, mask, set);
+}
+
+static int bcm54140_get_tunable(struct phy_device *phydev,
+				struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return bcm54140_get_downshift(phydev, data);
+	case ETHTOOL_PHY_EDPD:
+		return bcm54140_get_edpd(phydev, data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int bcm54140_set_tunable(struct phy_device *phydev,
+				struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return bcm54140_set_downshift(phydev, *(const u8 *)data);
+	case ETHTOOL_PHY_EDPD:
+		return bcm54140_set_edpd(phydev, *(const u16 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static struct phy_driver bcm54140_drivers[] = {
+	{
+		.phy_id         = PHY_ID_BCM54140,
+		.phy_id_mask    = 0xfffffff0,
+		.name           = "Broadcom BCM54140",
+		.features       = PHY_GBIT_FEATURES,
+		.config_init    = bcm54140_config_init,
+		.did_interrupt	= bcm54140_phy_did_interrupt,
+		.ack_interrupt  = bcm54140_phy_ack_intr,
+		.config_intr    = bcm54140_phy_config_intr,
+		.probe		= bcm54140_phy_probe,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.get_tunable	= bcm54140_get_tunable,
+		.set_tunable	= bcm54140_set_tunable,
+	},
+};
+module_phy_driver(bcm54140_drivers);
+
+static struct mdio_device_id __maybe_unused bcm54140_tbl[] = {
+	{ PHY_ID_BCM54140, 0xfffffff0 },
+	{ }
+};
+
+MODULE_AUTHOR("Michael Walle");
+MODULE_DESCRIPTION("Broadcom BCM54140 PHY driver");
+MODULE_DEVICE_TABLE(mdio, bcm54140_tbl);
+MODULE_LICENSE("GPL");
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index e14f78b3c8a4..5a346a985a5b 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -24,6 +24,7 @@
 #define PHY_ID_BCM5461			0x002060c0
 #define PHY_ID_BCM54612E		0x03625e60
 #define PHY_ID_BCM54616S		0x03625d10
+#define PHY_ID_BCM54140			0xae025019
 #define PHY_ID_BCM57780			0x03625d90
 #define PHY_ID_BCM89610			0x03625cd0
 
-- 
2.20.1

[PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

The PHY supports monitoring its die temperature as well as two analog
voltages. Add support for it.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 Documentation/hwmon/bcm54140.rst |  45 ++++
 Documentation/hwmon/index.rst    |   1 +
 drivers/net/phy/bcm54140.c       | 377 +++++++++++++++++++++++++++++++
 3 files changed, 423 insertions(+)
 create mode 100644 Documentation/hwmon/bcm54140.rst

diff --git a/Documentation/hwmon/bcm54140.rst b/Documentation/hwmon/bcm54140.rst
new file mode 100644
index 000000000000..bc6ea4b45966
--- /dev/null
+++ b/Documentation/hwmon/bcm54140.rst
@@ -0,0 +1,45 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Broadcom BCM54140 Quad SGMII/QSGMII PHY
+=======================================
+
+Supported chips:
+
+   * Broadcom BCM54140
+
+     Datasheet: not public
+
+Author: Michael Walle <michael@walle.cc>
+
+Description
+-----------
+
+The Broadcom BCM54140 is a Quad SGMII/QSGMII PHY which supports monitoring
+its die temperature as well as two analog voltages.
+
+The AVDDL is a 1.0V analogue voltage, the AVDDH is a 3.3V analogue voltage.
+Both voltages and the temperature are measured in a round-robin fashion.
+
+Sysfs entries
+-------------
+
+The following attributes are supported.
+
+======================= ========================================================
+in0_label		"AVDDL"
+in0_input		Measured AVDDL voltage.
+in0_min			Minimum AVDDL voltage.
+in0_max			Maximum AVDDL voltage.
+in0_alarm		AVDDL voltage alarm.
+
+in1_label		"AVDDH"
+in1_input		Measured AVDDH voltage.
+in1_min			Minimum AVDDH voltage.
+in1_max			Maximum AVDDH voltage.
+in1_alarm		AVDDH voltage alarm.
+
+temp1_input		Die temperature.
+temp1_min		Minimum die temperature.
+temp1_max		Maximum die temperature.
+temp1_alarm		Die temperature alarm.
+======================= ========================================================
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index f022583f96f6..19ad0846736d 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -42,6 +42,7 @@ Hardware Monitoring Kernel Drivers
    asb100
    asc7621
    aspeed-pwm-tacho
+   bcm54140
    bel-pfe
    coretemp
    da9052
diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index 97465491b41b..97c364ce05e3 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -6,6 +6,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/brcmphy.h>
+#include <linux/hwmon.h>
 #include <linux/module.h>
 #include <linux/phy.h>
 
@@ -50,6 +51,54 @@
 #define  BCM54140_RDB_TOP_IMR_PORT1	BIT(5)
 #define  BCM54140_RDB_TOP_IMR_PORT2	BIT(6)
 #define  BCM54140_RDB_TOP_IMR_PORT3	BIT(7)
+#define BCM54140_RDB_MON_CTRL		0x831	/* monitor control */
+#define  BCM54140_RDB_MON_CTRL_V_MODE	BIT(3)	/* voltage mode */
+#define  BCM54140_RDB_MON_CTRL_SEL_MASK	GENMASK(2, 1)
+#define  BCM54140_RDB_MON_CTRL_SEL_TEMP	0	/* meassure temperature */
+#define  BCM54140_RDB_MON_CTRL_SEL_1V0	1	/* meassure AVDDL 1.0V */
+#define  BCM54140_RDB_MON_CTRL_SEL_3V3	2	/* meassure AVDDH 3.3V */
+#define  BCM54140_RDB_MON_CTRL_SEL_RR	3	/* meassure all round-robin */
+#define  BCM54140_RDB_MON_CTRL_PWR_DOWN	BIT(0)	/* power-down monitor */
+#define BCM54140_RDB_MON_TEMP_VAL	0x832	/* temperature value */
+#define BCM54140_RDB_MON_TEMP_MAX	0x833	/* temperature high thresh */
+#define BCM54140_RDB_MON_TEMP_MIN	0x834	/* temperature low thresh */
+#define  BCM54140_RDB_MON_TEMP_DATA_MASK GENMASK(9, 0)
+#define BCM54140_RDB_MON_1V0_VAL	0x835	/* AVDDL 1.0V value */
+#define BCM54140_RDB_MON_1V0_MAX	0x836	/* AVDDL 1.0V high thresh */
+#define BCM54140_RDB_MON_1V0_MIN	0x837	/* AVDDL 1.0V low thresh */
+#define  BCM54140_RDB_MON_1V0_DATA_MASK	GENMASK(10, 0)
+#define BCM54140_RDB_MON_3V3_VAL	0x838	/* AVDDH 3.3V value */
+#define BCM54140_RDB_MON_3V3_MAX	0x839	/* AVDDH 3.3V high thresh */
+#define BCM54140_RDB_MON_3V3_MIN	0x83a	/* AVDDH 3.3V low thresh */
+#define  BCM54140_RDB_MON_3V3_DATA_MASK	GENMASK(11, 0)
+#define BCM54140_RDB_MON_ISR		0x83b	/* interrupt status */
+#define  BCM54140_RDB_MON_ISR_3V3	BIT(2)	/* AVDDH 3.3V alarm */
+#define  BCM54140_RDB_MON_ISR_1V0	BIT(1)	/* AVDDL 1.0V alarm */
+#define  BCM54140_RDB_MON_ISR_TEMP	BIT(0)	/* temperature alarm */
+
+/* According to the datasheet the formula is:
+ *   T = 413.35 - (0.49055 * bits[9:0])
+ */
+#define BCM54140_HWMON_TO_TEMP(v) (413350L - (v) * 491)
+#define BCM54140_HWMON_FROM_TEMP(v) DIV_ROUND_CLOSEST_ULL(413350L - (v), 491)
+
+/* According to the datasheet the formula is:
+ *   U = bits[11:0] / 1024 * 220 / 0.2
+ *
+ * Normalized:
+ *   U = bits[11:0] / 4096 * 2514
+ */
+#define BCM54140_HWMON_TO_IN_1V0(v) ((v) * 2514 >> 11)
+#define BCM54140_HWMON_FROM_IN_1V0(v) DIV_ROUND_CLOSEST_ULL(((v) << 11), 2514)
+
+/* According to the datasheet the formula is:
+ *   U = bits[10:0] / 1024 * 880 / 0.7
+ *
+ * Normalized:
+ *   U = bits[10:0] / 2048 * 4400
+ */
+#define BCM54140_HWMON_TO_IN_3V3(v) ((v) * 4400 >> 12)
+#define BCM54140_HWMON_FROM_IN_3V3(v) DIV_ROUND_CLOSEST_ULL(((v) << 12), 4400)
 
 #define BCM54140_DEFAULT_DOWNSHIFT 5
 #define BCM54140_MAX_DOWNSHIFT 9
@@ -57,6 +106,258 @@
 struct bcm54140_phy_priv {
 	int port;
 	int base_addr;
+	bool pkg_init;
+	u16 alarm;
+};
+
+static umode_t bcm54140_hwmon_is_visible(const void *data,
+					 enum hwmon_sensor_types type,
+					 u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_min:
+		case hwmon_in_max:
+			return 0644;
+		case hwmon_in_label:
+		case hwmon_in_input:
+		case hwmon_in_alarm:
+			return 0444;
+		default:
+			return 0;
+		}
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+			return 0644;
+		case hwmon_temp_input:
+		case hwmon_temp_alarm:
+			return 0444;
+		default:
+			return 0;
+		}
+	default:
+		return 0;
+	}
+}
+
+static int bcm54140_hwmon_read_alarm(struct device *dev, unsigned int bit,
+				     long *val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	struct bcm54140_phy_priv *priv = phydev->priv;
+	u16 tmp;
+
+	/* latch any alarm bits */
+	tmp = bcm_phy_read_rdb(phydev, BCM54140_RDB_MON_ISR);
+	if (tmp < 0)
+		return tmp;
+	priv->alarm |= tmp;
+
+	*val = !!(priv->alarm & bit);
+	priv->alarm &= ~bit;
+
+	return 0;
+}
+
+static int bcm54140_hwmon_read_temp(struct device *dev, u32 attr,
+				    int channel, long *val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	u16 reg, tmp;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = BCM54140_RDB_MON_TEMP_VAL;
+		break;
+	case hwmon_temp_min:
+		reg = BCM54140_RDB_MON_TEMP_MIN;
+		break;
+	case hwmon_temp_max:
+		reg = BCM54140_RDB_MON_TEMP_MAX;
+		break;
+	case hwmon_temp_alarm:
+		return bcm54140_hwmon_read_alarm(dev,
+						 BCM54140_RDB_MON_ISR_TEMP,
+						 val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	tmp = bcm_phy_read_rdb(phydev, reg);
+	if (tmp < 0)
+		return tmp;
+
+	*val = BCM54140_HWMON_TO_TEMP(tmp & BCM54140_RDB_MON_TEMP_DATA_MASK);
+
+	return 0;
+}
+
+static int bcm54140_hwmon_read_in(struct device *dev, u32 attr,
+				  int channel, long *val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK
+			      : BCM54140_RDB_MON_3V3_DATA_MASK;
+	u16 bit, reg, tmp;
+
+	switch (attr) {
+	case hwmon_in_input:
+		reg = (!channel) ? BCM54140_RDB_MON_1V0_VAL
+				 : BCM54140_RDB_MON_3V3_VAL;
+		break;
+	case hwmon_in_min:
+		reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN
+				 : BCM54140_RDB_MON_3V3_MIN;
+		break;
+	case hwmon_in_max:
+		reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX
+				 : BCM54140_RDB_MON_3V3_MAX;
+		break;
+	case hwmon_in_alarm:
+		bit = (!channel) ? BCM54140_RDB_MON_ISR_1V0
+				 : BCM54140_RDB_MON_ISR_3V3;
+		return bcm54140_hwmon_read_alarm(dev, bit, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	tmp = bcm_phy_read_rdb(phydev, reg);
+	if (tmp < 0)
+		return tmp;
+
+	if (!channel)
+		*val = BCM54140_HWMON_TO_IN_1V0(tmp & mask);
+	else
+		*val = BCM54140_HWMON_TO_IN_3V3(tmp & mask);
+
+	return 0;
+}
+
+static int bcm54140_hwmon_read(struct device *dev,
+			       enum hwmon_sensor_types type, u32 attr,
+			       int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return bcm54140_hwmon_read_temp(dev, attr, channel, val);
+	case hwmon_in:
+		return bcm54140_hwmon_read_in(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const char *const bcm54140_hwmon_in_labels[] = {
+	"AVDDL",
+	"AVDDH",
+};
+
+static int bcm54140_hwmon_read_string(struct device *dev,
+				      enum hwmon_sensor_types type, u32 attr,
+				      int channel, const char **str)
+{
+	switch (type) {
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_label:
+			*str = bcm54140_hwmon_in_labels[channel];
+			return 0;
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int bcm54140_hwmon_write_temp(struct device *dev, u32 attr,
+				     int channel, long val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	u16 reg;
+
+	switch (attr) {
+	case hwmon_temp_min:
+		reg = BCM54140_RDB_MON_TEMP_MIN;
+		break;
+	case hwmon_temp_max:
+		reg = BCM54140_RDB_MON_TEMP_MAX;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return bcm_phy_modify_rdb(phydev, reg, BCM54140_RDB_MON_TEMP_DATA_MASK,
+				  BCM54140_HWMON_FROM_TEMP(val));
+}
+
+static int bcm54140_hwmon_write_in(struct device *dev, u32 attr,
+				   int channel, long val)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK
+			      : BCM54140_RDB_MON_3V3_DATA_MASK;
+	unsigned long raw = (!channel) ?  BCM54140_HWMON_FROM_IN_1V0(val)
+				       :  BCM54140_HWMON_FROM_IN_3V3(val);
+	u16 reg;
+
+	raw = clamp_val(raw, 0, mask);
+
+	switch (attr) {
+	case hwmon_in_min:
+		reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN
+				 : BCM54140_RDB_MON_3V3_MIN;
+		break;
+	case hwmon_in_max:
+		reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX
+				 : BCM54140_RDB_MON_3V3_MAX;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return bcm_phy_modify_rdb(phydev, reg, mask, raw);
+}
+
+static int bcm54140_hwmon_write(struct device *dev,
+				enum hwmon_sensor_types type, u32 attr,
+				int channel, long val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return bcm54140_hwmon_write_temp(dev, attr, channel, val);
+	case hwmon_in:
+		return bcm54140_hwmon_write_in(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_channel_info *bcm54140_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
+			   HWMON_T_ALARM),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
+			   HWMON_I_ALARM | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
+			   HWMON_I_ALARM | HWMON_I_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops bcm54140_hwmon_ops = {
+	.is_visible = bcm54140_hwmon_is_visible,
+	.read = bcm54140_hwmon_read,
+	.read_string = bcm54140_hwmon_read_string,
+	.write = bcm54140_hwmon_write,
+};
+
+static const struct hwmon_chip_info bcm54140_chip_info = {
+	.ops = &bcm54140_hwmon_ops,
+	.info = bcm54140_hwmon_info,
 };
 
 static int bcm54140_phy_base_read_rdb(struct phy_device *phydev, u16 rdb)
@@ -203,6 +504,74 @@ static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
 	return 0;
 }
 
+/* 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 mdio_device **map = phydev->mdio.bus->mdio_map;
+	struct bcm54140_phy_priv *priv;
+	struct phy_device *phy;
+	int i, addr;
+
+	/* Quad PHY */
+	for (i = 0; i < 4; i++) {
+		priv = phydev->priv;
+		addr = priv->base_addr + i;
+
+		if (!map[addr])
+			continue;
+
+		phy = container_of(map[addr], struct phy_device, mdio);
+
+		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_enable_monitoring(struct phy_device *phydev)
+{
+	u16 mask, set;
+
+	/* 3.3V voltage mode */
+	set = BCM54140_RDB_MON_CTRL_V_MODE;
+
+	/* select round-robin */
+	mask = BCM54140_RDB_MON_CTRL_SEL_MASK;
+	set |= FIELD_PREP(BCM54140_RDB_MON_CTRL_SEL_MASK,
+			  BCM54140_RDB_MON_CTRL_SEL_RR);
+
+	/* remove power-down bit */
+	mask |= BCM54140_RDB_MON_CTRL_PWR_DOWN;
+
+	return bcm_phy_modify_rdb(phydev, BCM54140_RDB_MON_CTRL, mask, set);
+}
+
+static int bcm54140_phy_probe_once(struct phy_device *phydev)
+{
+	struct device *hwmon;
+	int ret;
+
+	/* enable hardware monitoring */
+	ret = bcm54140_enable_monitoring(phydev);
+	if (ret)
+		return ret;
+
+	hwmon = devm_hwmon_device_register_with_info(&phydev->mdio.dev,
+						     "BCM54140", phydev,
+						     &bcm54140_chip_info,
+						     NULL);
+	return PTR_ERR_OR_ZERO(hwmon);
+}
+
 static int bcm54140_phy_probe(struct phy_device *phydev)
 {
 	struct bcm54140_phy_priv *priv;
@@ -218,6 +587,14 @@ static int bcm54140_phy_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	if (!bcm54140_is_pkg_init(phydev)) {
+		ret = bcm54140_phy_probe_once(phydev);
+		if (ret)
+			return ret;
+	}
+
+	priv->pkg_init = true;
+
 	dev_info(&phydev->mdio.dev,
 		 "probed (port %d, base PHY address %d)\n",
 		 priv->port, priv->base_addr);
-- 
2.20.1

Re: [PATCH net-next 1/3] net: phy: broadcom: add helper to write/read RDB registers

[Florian Fainelli]

On 4/17/2020 12:28 PM, Michael Walle wrote:
> RDB regsiters are used on newer Broadcom PHYs. Add helper to read, write
> and modify these registers.

Only if you have to respin: please correct the typo above: regsiters vs. 
registers.

> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/3] net: phy: add Broadcom BCM54140 support

[Andrew Lunn]

On Fri, Apr 17, 2020 at 09:28:57PM +0200, Michael Walle wrote:

> +static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
> +{
> +	struct bcm54140_phy_priv *priv = phydev->priv;
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int addr, min_addr, max_addr;
> +	int step = 1;
> +	u32 phy_id;
> +	int tmp;
> +
> +	min_addr = phydev->mdio.addr;
> +	max_addr = phydev->mdio.addr;
> +	addr = phydev->mdio.addr;
> +
> +	/* We scan forward and backwards and look for PHYs which have the
> +	 * same phy_id like we do. Step 1 will scan forward, step 2
> +	 * backwards. Once we are finished, we have a min_addr and
> +	 * max_addr which resembles the range of PHY addresses of the same
> +	 * type of PHY. There is one caveat; there may be many PHYs of
> +	 * the same type, but we know that each PHY takes exactly 4
> +	 * consecutive addresses. Therefore we can deduce our offset
> +	 * to the base address of this quad PHY.
> +	 */

Hi Michael

How much flexibility is there in setting the base address using
strapping etc? Is it limited to a multiple of 4?

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Andrew Lunn]

> +/* 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 mdio_device **map = phydev->mdio.bus->mdio_map;
> +	struct bcm54140_phy_priv *priv;
> +	struct phy_device *phy;
> +	int i, addr;
> +
> +	/* Quad PHY */
> +	for (i = 0; i < 4; i++) {
> +		priv = phydev->priv;
> +		addr = priv->base_addr + i;
> +
> +		if (!map[addr])
> +			continue;
> +
> +		phy = container_of(map[addr], struct phy_device, mdio);

I don't particularly like a PHY driver having knowledge of the mdio
bus core. Please add a helper in the core to get you the phydev for a
particular address.

There is also the question of locking. What happens if the PHY devices
is unbound while you have an instance of its phydev? What happens if
the base PHY is unbound? Are the three others then unusable?

I think we need to take a step back and look at how we handle quad
PHYs in general. The VSC8584 has many of the same issues.

    Andrew

Re: [PATCH net-next 2/3] net: phy: add Broadcom BCM54140 support

[Michael Walle]

Hi Andrew,

Am 2020-04-17 21:39, schrieb Andrew Lunn:
> On Fri, Apr 17, 2020 at 09:28:57PM +0200, Michael Walle wrote:
> 
>> +static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
>> +{
>> +	struct bcm54140_phy_priv *priv = phydev->priv;
>> +	struct mii_bus *bus = phydev->mdio.bus;
>> +	int addr, min_addr, max_addr;
>> +	int step = 1;
>> +	u32 phy_id;
>> +	int tmp;
>> +
>> +	min_addr = phydev->mdio.addr;
>> +	max_addr = phydev->mdio.addr;
>> +	addr = phydev->mdio.addr;
>> +
>> +	/* We scan forward and backwards and look for PHYs which have the
>> +	 * same phy_id like we do. Step 1 will scan forward, step 2
>> +	 * backwards. Once we are finished, we have a min_addr and
>> +	 * max_addr which resembles the range of PHY addresses of the same
>> +	 * type of PHY. There is one caveat; there may be many PHYs of
>> +	 * the same type, but we know that each PHY takes exactly 4
>> +	 * consecutive addresses. Therefore we can deduce our offset
>> +	 * to the base address of this quad PHY.
>> +	 */
> 
> Hi Michael
> 
> How much flexibility is there in setting the base address using
> strapping etc? Is it limited to a multiple of 4?

You can just set the base address to any address. Then the following
addresses are used:
   base, base + 1, base + 2, base + 3, (base + 4)*

It is not specified what happens if you set the base so that it would
overflow. I guess that is a invalid strapping.

* (base + 4) is some kind of special PHY address which maps some kind
of moving window to a QSGMII address space. It is enabled by default,
could be disabled in software, but it doesn't share the same PHY id
for which this scans.

So yes, if you look at the addresses and the phy ids, there are
always 4 of this.

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Am 2020-04-17 21:50, schrieb Andrew Lunn:
>> +/* 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 mdio_device **map = phydev->mdio.bus->mdio_map;
>> +	struct bcm54140_phy_priv *priv;
>> +	struct phy_device *phy;
>> +	int i, addr;
>> +
>> +	/* Quad PHY */
>> +	for (i = 0; i < 4; i++) {
>> +		priv = phydev->priv;
>> +		addr = priv->base_addr + i;
>> +
>> +		if (!map[addr])
>> +			continue;
>> +
>> +		phy = container_of(map[addr], struct phy_device, mdio);
> 
> I don't particularly like a PHY driver having knowledge of the mdio
> bus core. Please add a helper in the core to get you the phydev for a
> particular address.
> 
> There is also the question of locking. What happens if the PHY devices
> is unbound while you have an instance of its phydev? What happens if
> the base PHY is unbound? Are the three others then unusable?
> 
> I think we need to take a step back and look at how we handle quad
> PHYs in general. The VSC8584 has many of the same issues.

Correct, and this function was actually stolen from there ;) This was
actually stolen from the mscc PHY ;)

-michael

Re: [PATCH net-next 2/3] net: phy: add Broadcom BCM54140 support

[Vladimir Oltean]

Hi Michael,

On Fri, 17 Apr 2020 at 22:52, Michael Walle <michael@walle.cc> wrote:
>
> Hi Andrew,
>
> Am 2020-04-17 21:39, schrieb Andrew Lunn:
> > On Fri, Apr 17, 2020 at 09:28:57PM +0200, Michael Walle wrote:
> >
> >> +static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
> >> +{
> >> +    struct bcm54140_phy_priv *priv = phydev->priv;
> >> +    struct mii_bus *bus = phydev->mdio.bus;
> >> +    int addr, min_addr, max_addr;
> >> +    int step = 1;
> >> +    u32 phy_id;
> >> +    int tmp;
> >> +
> >> +    min_addr = phydev->mdio.addr;
> >> +    max_addr = phydev->mdio.addr;
> >> +    addr = phydev->mdio.addr;
> >> +
> >> +    /* We scan forward and backwards and look for PHYs which have the
> >> +     * same phy_id like we do. Step 1 will scan forward, step 2
> >> +     * backwards. Once we are finished, we have a min_addr and
> >> +     * max_addr which resembles the range of PHY addresses of the same
> >> +     * type of PHY. There is one caveat; there may be many PHYs of
> >> +     * the same type, but we know that each PHY takes exactly 4
> >> +     * consecutive addresses. Therefore we can deduce our offset
> >> +     * to the base address of this quad PHY.
> >> +     */
> >
> > Hi Michael
> >
> > How much flexibility is there in setting the base address using
> > strapping etc? Is it limited to a multiple of 4?
>
> You can just set the base address to any address. Then the following
> addresses are used:
>    base, base + 1, base + 2, base + 3, (base + 4)*
>
> It is not specified what happens if you set the base so that it would
> overflow. I guess that is a invalid strapping.
>
> * (base + 4) is some kind of special PHY address which maps some kind
> of moving window to a QSGMII address space. It is enabled by default,
> could be disabled in software, but it doesn't share the same PHY id
> for which this scans.
>
> So yes, if you look at the addresses and the phy ids, there are
> always 4 of this.
>
> -michael

What does the reading of the global register give you, when accessed
through the master PHY ID vs any other PHY ID? Could you use that as
an indication of this being the correct PHY ID, and scan only to the
left?

Regards,
-Vladimir

Re: [PATCH net-next 2/3] net: phy: add Broadcom BCM54140 support

[Andrew Lunn]

> > Hi Michael
> > 
> > How much flexibility is there in setting the base address using
> > strapping etc? Is it limited to a multiple of 4?
> 
> You can just set the base address to any address. Then the following
> addresses are used:
>   base, base + 1, base + 2, base + 3, (base + 4)*

O.K, nothing as nice as base = my MOD 4.

:-(

	Andrew

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Andrew Lunn]

> Correct, and this function was actually stolen from there ;) This was
> actually stolen from the mscc PHY ;)

Which in itself indicates it is time to make it a helper :-)

      Andrew

Re: [PATCH net-next 2/3] net: phy: add Broadcom BCM54140 support

[Michael Walle]

Hi Vladimir,

Am 2020-04-17 22:00, schrieb Vladimir Oltean:
> Hi Michael,
> 
> On Fri, 17 Apr 2020 at 22:52, Michael Walle <michael@walle.cc> wrote:
>> 
>> Hi Andrew,
>> 
>> Am 2020-04-17 21:39, schrieb Andrew Lunn:
>> > On Fri, Apr 17, 2020 at 09:28:57PM +0200, Michael Walle wrote:
>> >
>> >> +static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
>> >> +{
>> >> +    struct bcm54140_phy_priv *priv = phydev->priv;
>> >> +    struct mii_bus *bus = phydev->mdio.bus;
>> >> +    int addr, min_addr, max_addr;
>> >> +    int step = 1;
>> >> +    u32 phy_id;
>> >> +    int tmp;
>> >> +
>> >> +    min_addr = phydev->mdio.addr;
>> >> +    max_addr = phydev->mdio.addr;
>> >> +    addr = phydev->mdio.addr;
>> >> +
>> >> +    /* We scan forward and backwards and look for PHYs which have the
>> >> +     * same phy_id like we do. Step 1 will scan forward, step 2
>> >> +     * backwards. Once we are finished, we have a min_addr and
>> >> +     * max_addr which resembles the range of PHY addresses of the same
>> >> +     * type of PHY. There is one caveat; there may be many PHYs of
>> >> +     * the same type, but we know that each PHY takes exactly 4
>> >> +     * consecutive addresses. Therefore we can deduce our offset
>> >> +     * to the base address of this quad PHY.
>> >> +     */
>> >
>> > Hi Michael
>> >
>> > How much flexibility is there in setting the base address using
>> > strapping etc? Is it limited to a multiple of 4?
>> 
>> You can just set the base address to any address. Then the following
>> addresses are used:
>>    base, base + 1, base + 2, base + 3, (base + 4)*
>> 
>> It is not specified what happens if you set the base so that it would
>> overflow. I guess that is a invalid strapping.
>> 
>> * (base + 4) is some kind of special PHY address which maps some kind
>> of moving window to a QSGMII address space. It is enabled by default,
>> could be disabled in software, but it doesn't share the same PHY id
>> for which this scans.
>> 
>> So yes, if you look at the addresses and the phy ids, there are
>> always 4 of this.
>> 
>> -michael
> 
> What does the reading of the global register give you, when accessed
> through the master PHY ID vs any other PHY ID? Could you use that as
> an indication of this being the correct PHY ID, and scan only to the
> left?

That was my first try, I thought it reads zero if you access a global
register by a PHY address which is not the base one. So I've looked
at registers which have at least one read-only 1 bit in it and scanned
only backwards. Well it turns out, my assumption was wrong and it
returns an old value of a successful read/write before. So it can just
return anything. And yes, its likely that you could read another
register and then probe the global register. But in the end I preferred
scanning the (known) phy id registers over strange hacks. Broadcom
could have just added a per-port register to actually read the base
address, but well.. ;)

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Am 2020-04-17 22:13, schrieb Andrew Lunn:
>> Correct, and this function was actually stolen from there ;) This was
>> actually stolen from the mscc PHY ;)
> 
> Which in itself indicates it is time to make it a helper :-)

Sure, do you have any suggestions?

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Andrew Lunn]

On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote:
> Am 2020-04-17 22:13, schrieb Andrew Lunn:
> > > Correct, and this function was actually stolen from there ;) This was
> > > actually stolen from the mscc PHY ;)
> > 
> > Which in itself indicates it is time to make it a helper :-)
> 
> Sure, do you have any suggestions?

mdiobus_get_phy() does the bit i was complaining about, the mdiobus
internal knowledge.

	Andrew

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Guenter Roeck]

On 4/17/20 12:28 PM, Michael Walle wrote:
> The PHY supports monitoring its die temperature as well as two analog
> voltages. Add support for it.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

This will probably fail to compile if HWMON is not enabled or if it is built
as module and this driver is built into the kernel.

> ---
>  Documentation/hwmon/bcm54140.rst |  45 ++++
>  Documentation/hwmon/index.rst    |   1 +
>  drivers/net/phy/bcm54140.c       | 377 +++++++++++++++++++++++++++++++
>  3 files changed, 423 insertions(+)
>  create mode 100644 Documentation/hwmon/bcm54140.rst
> 
> diff --git a/Documentation/hwmon/bcm54140.rst b/Documentation/hwmon/bcm54140.rst
> new file mode 100644
> index 000000000000..bc6ea4b45966
> --- /dev/null
> +++ b/Documentation/hwmon/bcm54140.rst
> @@ -0,0 +1,45 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +Broadcom BCM54140 Quad SGMII/QSGMII PHY
> +=======================================
> +
> +Supported chips:
> +
> +   * Broadcom BCM54140
> +
> +     Datasheet: not public
> +
> +Author: Michael Walle <michael@walle.cc>
> +
> +Description
> +-----------
> +
> +The Broadcom BCM54140 is a Quad SGMII/QSGMII PHY which supports monitoring
> +its die temperature as well as two analog voltages.
> +
> +The AVDDL is a 1.0V analogue voltage, the AVDDH is a 3.3V analogue voltage.
> +Both voltages and the temperature are measured in a round-robin fashion.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported.
> +
> +======================= ========================================================
> +in0_label		"AVDDL"
> +in0_input		Measured AVDDL voltage.
> +in0_min			Minimum AVDDL voltage.
> +in0_max			Maximum AVDDL voltage.
> +in0_alarm		AVDDL voltage alarm.
> +
> +in1_label		"AVDDH"
> +in1_input		Measured AVDDH voltage.
> +in1_min			Minimum AVDDH voltage.
> +in1_max			Maximum AVDDH voltage.
> +in1_alarm		AVDDH voltage alarm.
> +
> +temp1_input		Die temperature.
> +temp1_min		Minimum die temperature.
> +temp1_max		Maximum die temperature.
> +temp1_alarm		Die temperature alarm.
> +======================= ========================================================
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index f022583f96f6..19ad0846736d 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -42,6 +42,7 @@ Hardware Monitoring Kernel Drivers
>     asb100
>     asc7621
>     aspeed-pwm-tacho
> +   bcm54140
>     bel-pfe
>     coretemp
>     da9052
> diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
> index 97465491b41b..97c364ce05e3 100644
> --- a/drivers/net/phy/bcm54140.c
> +++ b/drivers/net/phy/bcm54140.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/brcmphy.h>
> +#include <linux/hwmon.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
>  
> @@ -50,6 +51,54 @@
>  #define  BCM54140_RDB_TOP_IMR_PORT1	BIT(5)
>  #define  BCM54140_RDB_TOP_IMR_PORT2	BIT(6)
>  #define  BCM54140_RDB_TOP_IMR_PORT3	BIT(7)
> +#define BCM54140_RDB_MON_CTRL		0x831	/* monitor control */
> +#define  BCM54140_RDB_MON_CTRL_V_MODE	BIT(3)	/* voltage mode */
> +#define  BCM54140_RDB_MON_CTRL_SEL_MASK	GENMASK(2, 1)
> +#define  BCM54140_RDB_MON_CTRL_SEL_TEMP	0	/* meassure temperature */
> +#define  BCM54140_RDB_MON_CTRL_SEL_1V0	1	/* meassure AVDDL 1.0V */
> +#define  BCM54140_RDB_MON_CTRL_SEL_3V3	2	/* meassure AVDDH 3.3V */
> +#define  BCM54140_RDB_MON_CTRL_SEL_RR	3	/* meassure all round-robin */
> +#define  BCM54140_RDB_MON_CTRL_PWR_DOWN	BIT(0)	/* power-down monitor */
> +#define BCM54140_RDB_MON_TEMP_VAL	0x832	/* temperature value */
> +#define BCM54140_RDB_MON_TEMP_MAX	0x833	/* temperature high thresh */
> +#define BCM54140_RDB_MON_TEMP_MIN	0x834	/* temperature low thresh */
> +#define  BCM54140_RDB_MON_TEMP_DATA_MASK GENMASK(9, 0)
> +#define BCM54140_RDB_MON_1V0_VAL	0x835	/* AVDDL 1.0V value */
> +#define BCM54140_RDB_MON_1V0_MAX	0x836	/* AVDDL 1.0V high thresh */
> +#define BCM54140_RDB_MON_1V0_MIN	0x837	/* AVDDL 1.0V low thresh */
> +#define  BCM54140_RDB_MON_1V0_DATA_MASK	GENMASK(10, 0)
> +#define BCM54140_RDB_MON_3V3_VAL	0x838	/* AVDDH 3.3V value */
> +#define BCM54140_RDB_MON_3V3_MAX	0x839	/* AVDDH 3.3V high thresh */
> +#define BCM54140_RDB_MON_3V3_MIN	0x83a	/* AVDDH 3.3V low thresh */
> +#define  BCM54140_RDB_MON_3V3_DATA_MASK	GENMASK(11, 0)
> +#define BCM54140_RDB_MON_ISR		0x83b	/* interrupt status */
> +#define  BCM54140_RDB_MON_ISR_3V3	BIT(2)	/* AVDDH 3.3V alarm */
> +#define  BCM54140_RDB_MON_ISR_1V0	BIT(1)	/* AVDDL 1.0V alarm */
> +#define  BCM54140_RDB_MON_ISR_TEMP	BIT(0)	/* temperature alarm */
> +
> +/* According to the datasheet the formula is:
> + *   T = 413.35 - (0.49055 * bits[9:0])
> + */
> +#define BCM54140_HWMON_TO_TEMP(v) (413350L - (v) * 491)
> +#define BCM54140_HWMON_FROM_TEMP(v) DIV_ROUND_CLOSEST_ULL(413350L - (v), 491)
> +
> +/* According to the datasheet the formula is:
> + *   U = bits[11:0] / 1024 * 220 / 0.2
> + *
> + * Normalized:
> + *   U = bits[11:0] / 4096 * 2514
> + */
> +#define BCM54140_HWMON_TO_IN_1V0(v) ((v) * 2514 >> 11)
> +#define BCM54140_HWMON_FROM_IN_1V0(v) DIV_ROUND_CLOSEST_ULL(((v) << 11), 2514)
> +
> +/* According to the datasheet the formula is:
> + *   U = bits[10:0] / 1024 * 880 / 0.7
> + *
> + * Normalized:
> + *   U = bits[10:0] / 2048 * 4400
> + */
> +#define BCM54140_HWMON_TO_IN_3V3(v) ((v) * 4400 >> 12)
> +#define BCM54140_HWMON_FROM_IN_3V3(v) DIV_ROUND_CLOSEST_ULL(((v) << 12), 4400)
>  
>  #define BCM54140_DEFAULT_DOWNSHIFT 5
>  #define BCM54140_MAX_DOWNSHIFT 9
> @@ -57,6 +106,258 @@
>  struct bcm54140_phy_priv {
>  	int port;
>  	int base_addr;
> +	bool pkg_init;
> +	u16 alarm;
> +};
> +
> +static umode_t bcm54140_hwmon_is_visible(const void *data,
> +					 enum hwmon_sensor_types type,
> +					 u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_min:
> +		case hwmon_in_max:
> +			return 0644;
> +		case hwmon_in_label:
> +		case hwmon_in_input:
> +		case hwmon_in_alarm:
> +			return 0444;
> +		default:
> +			return 0;
> +		}
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_min:
> +		case hwmon_temp_max:
> +			return 0644;
> +		case hwmon_temp_input:
> +		case hwmon_temp_alarm:
> +			return 0444;
> +		default:
> +			return 0;
> +		}
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int bcm54140_hwmon_read_alarm(struct device *dev, unsigned int bit,
> +				     long *val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	struct bcm54140_phy_priv *priv = phydev->priv;
> +	u16 tmp;
> +
> +	/* latch any alarm bits */
> +	tmp = bcm_phy_read_rdb(phydev, BCM54140_RDB_MON_ISR);
> +	if (tmp < 0)
> +		return tmp;
> +	priv->alarm |= tmp;
> +
> +	*val = !!(priv->alarm & bit);
> +	priv->alarm &= ~bit;> +
> +	return 0;
> +}
> +
> +static int bcm54140_hwmon_read_temp(struct device *dev, u32 attr,
> +				    int channel, long *val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	u16 reg, tmp;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = BCM54140_RDB_MON_TEMP_VAL;
> +		break;
> +	case hwmon_temp_min:
> +		reg = BCM54140_RDB_MON_TEMP_MIN;
> +		break;
> +	case hwmon_temp_max:
> +		reg = BCM54140_RDB_MON_TEMP_MAX;
> +		break;
> +	case hwmon_temp_alarm:
> +		return bcm54140_hwmon_read_alarm(dev,
> +						 BCM54140_RDB_MON_ISR_TEMP,
> +						 val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	tmp = bcm_phy_read_rdb(phydev, reg);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	*val = BCM54140_HWMON_TO_TEMP(tmp & BCM54140_RDB_MON_TEMP_DATA_MASK);
> +
> +	return 0;
> +}
> +
> +static int bcm54140_hwmon_read_in(struct device *dev, u32 attr,
> +				  int channel, long *val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK
> +			      : BCM54140_RDB_MON_3V3_DATA_MASK;
> +	u16 bit, reg, tmp;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		reg = (!channel) ? BCM54140_RDB_MON_1V0_VAL
> +				 : BCM54140_RDB_MON_3V3_VAL;

I am personally neither a friend of unnecessary () nor of
unnecessary negations. Why not the following ?

		reg = channel ? BCM54140_RDB_MON_3V3_VAL :
				BCM54140_RDB_MON_1V0_VAL;

Another option would be to read all those values from a set of
defines, given the expressions are repeated several times.

> +		break;
> +	case hwmon_in_min:
> +		reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN
> +				 : BCM54140_RDB_MON_3V3_MIN;
> +		break;
> +	case hwmon_in_max:
> +		reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX
> +				 : BCM54140_RDB_MON_3V3_MAX;
> +		break;
> +	case hwmon_in_alarm:
> +		bit = (!channel) ? BCM54140_RDB_MON_ISR_1V0
> +				 : BCM54140_RDB_MON_ISR_3V3;
> +		return bcm54140_hwmon_read_alarm(dev, bit, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	tmp = bcm_phy_read_rdb(phydev, reg);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	if (!channel)
> +		*val = BCM54140_HWMON_TO_IN_1V0(tmp & mask);
> +	else
> +		*val = BCM54140_HWMON_TO_IN_3V3(tmp & mask);
> +
> +	return 0;
> +}
> +
> +static int bcm54140_hwmon_read(struct device *dev,
> +			       enum hwmon_sensor_types type, u32 attr,
> +			       int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return bcm54140_hwmon_read_temp(dev, attr, channel, val);
> +	case hwmon_in:
> +		return bcm54140_hwmon_read_in(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const char *const bcm54140_hwmon_in_labels[] = {
> +	"AVDDL",
> +	"AVDDH",
> +};
> +
> +static int bcm54140_hwmon_read_string(struct device *dev,
> +				      enum hwmon_sensor_types type, u32 attr,
> +				      int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_label:
> +			*str = bcm54140_hwmon_in_labels[channel];
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int bcm54140_hwmon_write_temp(struct device *dev, u32 attr,
> +				     int channel, long val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	u16 reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_min:
> +		reg = BCM54140_RDB_MON_TEMP_MIN;
> +		break;
> +	case hwmon_temp_max:
> +		reg = BCM54140_RDB_MON_TEMP_MAX;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return bcm_phy_modify_rdb(phydev, reg, BCM54140_RDB_MON_TEMP_DATA_MASK,
> +				  BCM54140_HWMON_FROM_TEMP(val));
> +}
> +
> +static int bcm54140_hwmon_write_in(struct device *dev, u32 attr,
> +				   int channel, long val)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK
> +			      : BCM54140_RDB_MON_3V3_DATA_MASK;
> +	unsigned long raw = (!channel) ?  BCM54140_HWMON_FROM_IN_1V0(val)
> +				       :  BCM54140_HWMON_FROM_IN_3V3(val);
> +	u16 reg;
> +
> +	raw = clamp_val(raw, 0, mask);
> +
> +	switch (attr) {
> +	case hwmon_in_min:
> +		reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN
> +				 : BCM54140_RDB_MON_3V3_MIN;
> +		break;
> +	case hwmon_in_max:
> +		reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX
> +				 : BCM54140_RDB_MON_3V3_MAX;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return bcm_phy_modify_rdb(phydev, reg, mask, raw);
> +}
> +
> +static int bcm54140_hwmon_write(struct device *dev,
> +				enum hwmon_sensor_types type, u32 attr,
> +				int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return bcm54140_hwmon_write_temp(dev, attr, channel, val);
> +	case hwmon_in:
> +		return bcm54140_hwmon_write_in(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_channel_info *bcm54140_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
> +			   HWMON_T_ALARM),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
> +			   HWMON_I_ALARM | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX |
> +			   HWMON_I_ALARM | HWMON_I_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_ops bcm54140_hwmon_ops = {
> +	.is_visible = bcm54140_hwmon_is_visible,
> +	.read = bcm54140_hwmon_read,
> +	.read_string = bcm54140_hwmon_read_string,
> +	.write = bcm54140_hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info bcm54140_chip_info = {
> +	.ops = &bcm54140_hwmon_ops,
> +	.info = bcm54140_hwmon_info,
>  };
>  
>  static int bcm54140_phy_base_read_rdb(struct phy_device *phydev, u16 rdb)
> @@ -203,6 +504,74 @@ static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/* 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 mdio_device **map = phydev->mdio.bus->mdio_map;
> +	struct bcm54140_phy_priv *priv;
> +	struct phy_device *phy;
> +	int i, addr;
> +
> +	/* Quad PHY */
> +	for (i = 0; i < 4; i++) {
> +		priv = phydev->priv;
> +		addr = priv->base_addr + i;
> +
> +		if (!map[addr])
> +			continue;
> +
> +		phy = container_of(map[addr], struct phy_device, mdio);
> +
> +		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_enable_monitoring(struct phy_device *phydev)
> +{
> +	u16 mask, set;
> +
> +	/* 3.3V voltage mode */
> +	set = BCM54140_RDB_MON_CTRL_V_MODE;
> +
> +	/* select round-robin */
> +	mask = BCM54140_RDB_MON_CTRL_SEL_MASK;
> +	set |= FIELD_PREP(BCM54140_RDB_MON_CTRL_SEL_MASK,
> +			  BCM54140_RDB_MON_CTRL_SEL_RR);
> +
> +	/* remove power-down bit */
> +	mask |= BCM54140_RDB_MON_CTRL_PWR_DOWN;
> +
> +	return bcm_phy_modify_rdb(phydev, BCM54140_RDB_MON_CTRL, mask, set);
> +}
> +
> +static int bcm54140_phy_probe_once(struct phy_device *phydev)
> +{
> +	struct device *hwmon;
> +	int ret;
> +
> +	/* enable hardware monitoring */
> +	ret = bcm54140_enable_monitoring(phydev);
> +	if (ret)
> +		return ret;
> +
> +	hwmon = devm_hwmon_device_register_with_info(&phydev->mdio.dev,
> +						     "BCM54140", phydev,
> +						     &bcm54140_chip_info,
> +						     NULL);
> +	return PTR_ERR_OR_ZERO(hwmon);
> +}
> +
>  static int bcm54140_phy_probe(struct phy_device *phydev)
>  {
>  	struct bcm54140_phy_priv *priv;
> @@ -218,6 +587,14 @@ static int bcm54140_phy_probe(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> +	if (!bcm54140_is_pkg_init(phydev)) {
> +		ret = bcm54140_phy_probe_once(phydev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	priv->pkg_init = true;
> +
>  	dev_info(&phydev->mdio.dev,
>  		 "probed (port %d, base PHY address %d)\n",
>  		 priv->port, priv->base_addr);
> 

Re: [PATCH net-next 1/3] net: phy: broadcom: add helper to write/read RDB registers

[Andrew Lunn]

On Fri, Apr 17, 2020 at 09:28:56PM +0200, Michael Walle wrote:
> RDB regsiters are used on newer Broadcom PHYs. Add helper to read, write
> and modify these registers.

It would be nice to give a hint what RDB means?

Thanks
	Andrew

Re: [PATCH net-next 1/3] net: phy: broadcom: add helper to write/read RDB registers

[Florian Fainelli]

On 4/18/2020 7:13 AM, Andrew Lunn wrote:
> On Fri, Apr 17, 2020 at 09:28:56PM +0200, Michael Walle wrote:
>> RDB regsiters are used on newer Broadcom PHYs. Add helper to read, write
>> and modify these registers.
> 
> It would be nice to give a hint what RDB means?

It means Register Data Base, it is meant to be a linear register offset 
as opposed to the more convulated shadow 18, 1c or other expansion 
registers.
-- 
Florian

Re: [PATCH net-next 1/3] net: phy: broadcom: add helper to write/read RDB registers

[Michael Walle]

Am 2020-04-18 17:55, schrieb Florian Fainelli:
> On 4/18/2020 7:13 AM, Andrew Lunn wrote:
>> On Fri, Apr 17, 2020 at 09:28:56PM +0200, Michael Walle wrote:
>>> RDB regsiters are used on newer Broadcom PHYs. Add helper to read, 
>>> write
>>> and modify these registers.
>> 
>> It would be nice to give a hint what RDB means?
> 
> It means Register Data Base, it is meant to be a linear register
> offset as opposed to the more convulated shadow 18, 1c or other
> expansion registers.

Oh I just found some comment to another linux patch explaining this.
Because there is no trace what RDB actually means in the datasheets
from Broadcom I've seen so far.

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Am 2020-04-17 23:28, schrieb Andrew Lunn:
> On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote:
>> Am 2020-04-17 22:13, schrieb Andrew Lunn:
>> > > Correct, and this function was actually stolen from there ;) This was
>> > > actually stolen from the mscc PHY ;)
>> >
>> > Which in itself indicates it is time to make it a helper :-)
>> 
>> Sure, do you have any suggestions?
> 
> mdiobus_get_phy() does the bit i was complaining about, the mdiobus
> internal knowledge.

But that doesn't address your other comment.

> There is also the question of locking. What happens if the PHY devices
> is unbound while you have an instance of its phydev?

Is there any lock one could take to avoid that?

> What happens if the base PHY is unbound? Are the three others then
> unusable?

In my case, this would mean the hwmon device is also removed. I don't
see any other way to do it right now. I guess it would be better to
have the hwmon device registered to some kind of parent device.

> I think we need to take a step back and look at how we handle quad
> PHYs in general. The VSC8584 has many of the same issues.

For the BCM54140 there are three different functions:
  (1) PHY functions accessible by the PHYs own address (ie PHY
      status/control)
  (2) PHY functions but only accessible by the global registers (ie
      interrupt enables per PHY of the shared interrupt pin)
  (3) global functions (like sensors, global configuration)

(1) is already supported in the current PHY framework. (2) and (3)
need the "hack" which uses mdiobus_read/write() with the base
address.

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Andrew Lunn]

On Sun, Apr 19, 2020 at 12:29:23PM +0200, Michael Walle wrote:
> Am 2020-04-17 23:28, schrieb Andrew Lunn:
> > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote:
> > > Am 2020-04-17 22:13, schrieb Andrew Lunn:
> > > > > Correct, and this function was actually stolen from there ;) This was
> > > > > actually stolen from the mscc PHY ;)
> > > >
> > > > Which in itself indicates it is time to make it a helper :-)
> > > 
> > > Sure, do you have any suggestions?
> > 
> > mdiobus_get_phy() does the bit i was complaining about, the mdiobus
> > internal knowledge.
> 
> But that doesn't address your other comment.

Yes, you are right. But i don't think you can easily generalize the
rest. It needs knowledge of the driver private structure to reference
pkg_init. You would have to move that into phy_device.

> 
> > There is also the question of locking. What happens if the PHY devices
> > is unbound while you have an instance of its phydev?
> 
> Is there any lock one could take to avoid that?

phy_attach_direct() does a get_device(). That at least means the
struct device will not go away. I don't know the code well enough to
know if that will also stop the phy_device structure from being freed.
We might need mdiobus_get_phy() to also do a get_device(), and add a
mdiobus_put_phy() which does a put_device().

> > What happens if the base PHY is unbound? Are the three others then
> > unusable?
> 
> In my case, this would mean the hwmon device is also removed. I don't
> see any other way to do it right now. I guess it would be better to
> have the hwmon device registered to some kind of parent device.

The phydev structure might go away. But the hardware is still
there. You can access it via address on the bus. What you have to be
careful of is using the phydev for a different phy.

> For the BCM54140 there are three different functions:
>  (1) PHY functions accessible by the PHYs own address (ie PHY
>      status/control)
>  (2) PHY functions but only accessible by the global registers (ie
>      interrupt enables per PHY of the shared interrupt pin)
>  (3) global functions (like sensors, global configuration)
> 
> (1) is already supported in the current PHY framework. (2) and (3)
> need the "hack" which uses mdiobus_read/write() with the base
> address.

Is the _is_pkg_init() function the only place you need to access some
other phy_device structure.

Maybe we need a phydev->shared structure, which all PHYs in one
package share? Get the core to do reference counting on the structure?
Add helpers phy_read_shared(), phy_write_shared(), etc, which does
MDIO accesses on the base device, taking care of the locking. pkg_init
is a member of this shared structure. And have a void * priv in shared
for shared driver private data?

Just "thinking out loud"

    Andrew

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Am 2020-04-19 18:29, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 12:29:23PM +0200, Michael Walle wrote:
>> Am 2020-04-17 23:28, schrieb Andrew Lunn:
>> > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote:
>> > > Am 2020-04-17 22:13, schrieb Andrew Lunn:
>> > > > > Correct, and this function was actually stolen from there ;) This was
>> > > > > actually stolen from the mscc PHY ;)
>> > > >
>> > > > Which in itself indicates it is time to make it a helper :-)
>> > >
>> > > Sure, do you have any suggestions?
>> >
>> > mdiobus_get_phy() does the bit i was complaining about, the mdiobus
>> > internal knowledge.
>> 
>> But that doesn't address your other comment.
> 
> Yes, you are right. But i don't think you can easily generalize the
> rest. It needs knowledge of the driver private structure to reference
> pkg_init. You would have to move that into phy_device.
> 
>> 
>> > There is also the question of locking. What happens if the PHY devices
>> > is unbound while you have an instance of its phydev?
>> 
>> Is there any lock one could take to avoid that?
> 
> phy_attach_direct() does a get_device(). That at least means the
> struct device will not go away. I don't know the code well enough to
> know if that will also stop the phy_device structure from being freed.
> We might need mdiobus_get_phy() to also do a get_device(), and add a
> mdiobus_put_phy() which does a put_device().
> 
>> > What happens if the base PHY is unbound? Are the three others then
>> > unusable?
>> 
>> In my case, this would mean the hwmon device is also removed. I don't
>> see any other way to do it right now. I guess it would be better to
>> have the hwmon device registered to some kind of parent device.
> 
> The phydev structure might go away. But the hardware is still
> there. You can access it via address on the bus. What you have to be
> careful of is using the phydev for a different phy.

But the hwmon is registered to the device of the PHY which might be
unbound. So it will also be removed, correct? FWIW I don't think that
is likely to happen in my case ;)

> 
>> For the BCM54140 there are three different functions:
>>  (1) PHY functions accessible by the PHYs own address (ie PHY
>>      status/control)
>>  (2) PHY functions but only accessible by the global registers (ie
>>      interrupt enables per PHY of the shared interrupt pin)
>>  (3) global functions (like sensors, global configuration)
>> 
>> (1) is already supported in the current PHY framework. (2) and (3)
>> need the "hack" which uses mdiobus_read/write() with the base
>> address.
> 
> Is the _is_pkg_init() function the only place you need to access some
> other phy_device structure.

yes.

> Maybe we need a phydev->shared structure, which all PHYs in one
> package share?

That came to my mind too. But how could the PHY core find out which
shared structure belongs to which phydev? I guess the phydev have to
find out, but then how does it tell the PHY core that it wants such
a shared structure. Have the (base) PHY address as an identifier?

> Get the core to do reference counting on the structure?
> Add helpers phy_read_shared(), phy_write_shared(), etc, which does
> MDIO accesses on the base device, taking care of the locking.

The "base" access is another thing, I guess, which has nothing to do
with the shared structure. Also I presume not every PHY has the base
address as some global register access. Eg. this PHY also have
"base + 4" (or depending on the configuration base + 3, that is the
last PHY of the four) as a special register access.

> pkg_init
> is a member of this shared structure. And have a void * priv in shared
> for shared driver private data?

if you have a void *priv, why would you need pkg_init, which is an
implementation detail of the phydev. I guess it is enough to just have
a void *shared (I don't know about the locking for now).

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Andrew Lunn]

> > Maybe we need a phydev->shared structure, which all PHYs in one
> > package share?
> 
> That came to my mind too. But how could the PHY core find out which
> shared structure belongs to which phydev? I guess the phydev have to
> find out, but then how does it tell the PHY core that it wants such
> a shared structure. Have the (base) PHY address as an identifier?

Yes. I was thinking along those lines.

phy_package_join(phydev, base)

If this is the first call with that value of base, allocate the
structure, set the ref count to 1, and set phydev->shared to point to
it. For subsequent calls, increment the reference count, and set
phydev->shared.

phy_package_leave(phydev)

Decrement the reference count, and set phydev->shared to NULL. If the
reference count goes to 0, free the structure.

> > Get the core to do reference counting on the structure?
> > Add helpers phy_read_shared(), phy_write_shared(), etc, which does
> > MDIO accesses on the base device, taking care of the locking.
> 
> The "base" access is another thing, I guess, which has nothing to do
> with the shared structure.

I'm making the assumption that all global addresses are at the base
address. If we don't want to make that assumption, we need the change
the API above so you pass a cookie, and all PHYs need to use the same
cookie to identify the package.

Maybe base is the wrong name, since MSCC can have the base as the high
address of the four, not the low?

Still just thinking aloud....

       Andrew

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Russell King - ARM Linux admin]

On Sun, Apr 19, 2020 at 06:29:28PM +0200, Andrew Lunn wrote:
> On Sun, Apr 19, 2020 at 12:29:23PM +0200, Michael Walle wrote:
> > Am 2020-04-17 23:28, schrieb Andrew Lunn:
> > > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote:
> > > > Am 2020-04-17 22:13, schrieb Andrew Lunn:
> > > > > > Correct, and this function was actually stolen from there ;) This was
> > > > > > actually stolen from the mscc PHY ;)
> > > > >
> > > > > Which in itself indicates it is time to make it a helper :-)
> > > > 
> > > > Sure, do you have any suggestions?
> > > 
> > > mdiobus_get_phy() does the bit i was complaining about, the mdiobus
> > > internal knowledge.
> > 
> > But that doesn't address your other comment.
> 
> Yes, you are right. But i don't think you can easily generalize the
> rest. It needs knowledge of the driver private structure to reference
> pkg_init. You would have to move that into phy_device.
> 
> > 
> > > There is also the question of locking. What happens if the PHY devices
> > > is unbound while you have an instance of its phydev?
> > 
> > Is there any lock one could take to avoid that?
> 
> phy_attach_direct() does a get_device(). That at least means the
> struct device will not go away. I don't know the code well enough to
> know if that will also stop the phy_device structure from being freed.

Well, struct device is embedded in struct mdio_device, which in turn
is embedded in struct phy_device. So, if struct device can't go away
because its refcount is held, the same is true of the structs
embedding it.

-- 
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

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Am 2020-04-19 19:05, schrieb Andrew Lunn:
>> > Maybe we need a phydev->shared structure, which all PHYs in one
>> > package share?
>> 
>> That came to my mind too. But how could the PHY core find out which
>> shared structure belongs to which phydev? I guess the phydev have to
>> find out, but then how does it tell the PHY core that it wants such
>> a shared structure. Have the (base) PHY address as an identifier?
> 
> Yes. I was thinking along those lines.
> 
> phy_package_join(phydev, base)
> 
> If this is the first call with that value of base, allocate the
> structure, set the ref count to 1, and set phydev->shared to point to
> it. For subsequent calls, increment the reference count, and set
> phydev->shared.
> 
> phy_package_leave(phydev)
> 
> Decrement the reference count, and set phydev->shared to NULL. If the
> reference count goes to 0, free the structure.
> 
>> > Get the core to do reference counting on the structure?
>> > Add helpers phy_read_shared(), phy_write_shared(), etc, which does
>> > MDIO accesses on the base device, taking care of the locking.
>> 
>> The "base" access is another thing, I guess, which has nothing to do
>> with the shared structure.
> 
> I'm making the assumption that all global addresses are at the base
> address.

But what does that have to do with the shared structure? I don't think
you have to "bundle" the shared structure with the "access the global
registers" method. The phy drivers just have to know some common key,
which can be anything arbitrary, correct? So we can say its the
lowest address, but it could also be any other address, as long as
each PHY driver instance can deduce the same key.

> If we don't want to make that assumption, we need the change
> the API above so you pass a cookie, and all PHYs need to use the same
> cookie to identify the package.

whats the difference between a PHY address and a cookie, given that the
phy core doesn't actually use the phy address for anything.

-michael

> Maybe base is the wrong name, since MSCC can have the base as the high
> address of the four, not the low?
> 
> Still just thinking aloud....
> 
>        Andrew

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Andrew Lunn]

> But what does that have to do with the shared structure? I don't think
> you have to "bundle" the shared structure with the "access the global
> registers" method.

We don't need to. But it would be a good way to clean up code which
locks the mdio bus, does a register access on some other device, and
then unlocks the bus.

As a general rule of thumb, it is better to have the core do the
locking, rather than the driver. Driver writers don't always think
about locking, so it is better to give driver writers safe APIs to
use.

	Andrew

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Hi Andrew,

Am 2020-04-19 23:55, schrieb Andrew Lunn:
>> But what does that have to do with the shared structure? I don't think
>> you have to "bundle" the shared structure with the "access the global
>> registers" method.
> 
> We don't need to. But it would be a good way to clean up code which
> locks the mdio bus, does a register access on some other device, and
> then unlocks the bus.

I'd like do an RFC for that. But how should I proceed with the original
patch series? Should I send an updated version; you didn't reply to the
LED stuff. That is the last remark for now.

> As a general rule of thumb, it is better to have the core do the
> locking, rather than the driver. Driver writers don't always think
> about locking, so it is better to give driver writers safe APIs to
> use.

Ok I see, but what locking do you have in mind? We could have something
like

__phy_package_write(struct phy_device *dev, u32 regnum, u16 val)
{
   return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr,
                          regnum, val);
}

and its phy_package_write() equivalent. But that would just be
convenience functions, nothing where you actually help the user with
locking. Am I missing something?

>>> Get the core to do reference counting on the structure?
>>> Add helpers phy_read_shared(), phy_write_shared(), etc, which does
>>> MDIO accesses on the base device, taking care of the locking.
>>> 
>> The "base" access is another thing, I guess, which has nothing to do
>> with the shared structure.
>> 
> I'm making the assumption that all global addresses are at the base
> address. If we don't want to make that assumption, we need the change
> the API above so you pass a cookie, and all PHYs need to use the same
> cookie to identify the package.

how would a phy driver deduce a common cookie? And how would that be a
difference to using a PHY address.

> Maybe base is the wrong name, since MSCC can have the base as the high
> address of the four, not the low?

I'd say it might be any of the four addresses as long as it is the same
across the PHYs in the same package. And in that case you can also have
the phy_package_read/write() functions.

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Andrew Lunn]

> Ok I see, but what locking do you have in mind? We could have something
> like
> 
> __phy_package_write(struct phy_device *dev, u32 regnum, u16 val)
> {
>   return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr,
>                          regnum, val);
> }
> 
> and its phy_package_write() equivalent. But that would just be
> convenience functions, nothing where you actually help the user with
> locking. Am I missing something?

In general, drivers should not be using __foo functions. We want
drivers to make use of phy_package_write() which would do the bus
locking. Look at a typical PHY driver. There is no locking what so
ever. Just lots of phy_read() and phy write(). The locking is done by
the core and so should be correct.

> > > > Get the core to do reference counting on the structure?
> > > > Add helpers phy_read_shared(), phy_write_shared(), etc, which does
> > > > MDIO accesses on the base device, taking care of the locking.
> > > > 
> > > The "base" access is another thing, I guess, which has nothing to do
> > > with the shared structure.
> > > 
> > I'm making the assumption that all global addresses are at the base
> > address. If we don't want to make that assumption, we need the change
> > the API above so you pass a cookie, and all PHYs need to use the same
> > cookie to identify the package.
> 
> how would a phy driver deduce a common cookie? And how would that be a
> difference to using a PHY address.

For a cookie, i don't care how the driver decides on the cookie. The
core never uses it, other than comparing cookies to combine individual
PHYs into a package. It could be a PHY address. It could be the PHY
address where the global registers are. Or it could be anything else.

> > Maybe base is the wrong name, since MSCC can have the base as the high
> > address of the four, not the low?
> 
> I'd say it might be any of the four addresses as long as it is the same
> across the PHYs in the same package. And in that case you can also have
> the phy_package_read/write() functions.

Yes. That is the semantics which is think is most useful. But then we
don't have a cookie, the value has real significance, and we need to
document what is should mean.

     Andrew

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Am 2020-04-20 17:36, schrieb Andrew Lunn:
>> Ok I see, but what locking do you have in mind? We could have 
>> something
>> like
>> 
>> __phy_package_write(struct phy_device *dev, u32 regnum, u16 val)
>> {
>>   return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr,
>>                          regnum, val);
>> }
>> 
>> and its phy_package_write() equivalent. But that would just be
>> convenience functions, nothing where you actually help the user with
>> locking. Am I missing something?
> 
> In general, drivers should not be using __foo functions. We want
> drivers to make use of phy_package_write() which would do the bus
> locking. Look at a typical PHY driver. There is no locking what so
> ever. Just lots of phy_read() and phy write(). The locking is done by
> the core and so should be correct.

Ok, but for example the BCM54140 uses indirect register access and thus
need to lock the mdio bus itself in which case I need the __funcs.

>> > > > Get the core to do reference counting on the structure?
>> > > > Add helpers phy_read_shared(), phy_write_shared(), etc, which does
>> > > > MDIO accesses on the base device, taking care of the locking.
>> > > >
>> > > The "base" access is another thing, I guess, which has nothing to do
>> > > with the shared structure.
>> > >
>> > I'm making the assumption that all global addresses are at the base
>> > address. If we don't want to make that assumption, we need the change
>> > the API above so you pass a cookie, and all PHYs need to use the same
>> > cookie to identify the package.
>> 
>> how would a phy driver deduce a common cookie? And how would that be a
>> difference to using a PHY address.
> 
> For a cookie, i don't care how the driver decides on the cookie. The
> core never uses it, other than comparing cookies to combine individual
> PHYs into a package. It could be a PHY address. It could be the PHY
> address where the global registers are. Or it could be anything else.
> 
>> > Maybe base is the wrong name, since MSCC can have the base as the high
>> > address of the four, not the low?
>> 
>> I'd say it might be any of the four addresses as long as it is the 
>> same
>> across the PHYs in the same package. And in that case you can also 
>> have
>> the phy_package_read/write() functions.
> 
> Yes. That is the semantics which is think is most useful. But then we
> don't have a cookie, the value has real significance, and we need to
> document what is should mean.

Agreed.

I will post a RFC shortly.

-michael

Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

[Russell King - ARM Linux admin]

On Mon, Apr 20, 2020 at 05:10:19PM +0200, Michael Walle wrote:
> Hi Andrew,
> 
> Am 2020-04-19 23:55, schrieb Andrew Lunn:
> > > But what does that have to do with the shared structure? I don't think
> > > you have to "bundle" the shared structure with the "access the global
> > > registers" method.
> > 
> > We don't need to. But it would be a good way to clean up code which
> > locks the mdio bus, does a register access on some other device, and
> > then unlocks the bus.
> 
> I'd like do an RFC for that. But how should I proceed with the original
> patch series? Should I send an updated version; you didn't reply to the
> LED stuff. That is the last remark for now.

The LED stuff is something that there isn't a solution for at the
moment.  There's been talk about coming up with some generic way
to describe the PHY LED configuration in DT, but given that almost
every PHY has quite different ways to configure LEDs, I fear such
a task is virtually impossible.

Very few PHYs under Linux have their LEDs operating "correctly" or
in a meaningful or sensible way because of this, and it's been this
way for years.

-- 
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