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

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

[Michael Walle]

RDB (Register Data Base) registers are used on newer Broadcom PHYs. Add
helper to read, write and modify these registers.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
changes since v1:
 - corrected typo
 - added Reviewed-by

 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 v2 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>
---
changes since v1:
 none

 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 v2 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>
---
changes since v1:
 - add IS_ENABLED(CONFIG_HWMON) guards
 - clamp values before conversion
 - add mutex alarm_lock
 - use mdiobus_get_phy()
 - make CONFIG_BCM54140_PHY depend on HWMON (or disabled altogether)
 - add BCM54140_HWMON_IN_xx(ch) macros

Btw. is it possible to rely on the compiler to strip away unused
function calls. For exmaple, instead of using the
#if IS_ENABLED(CONFIG_HWMON) guards, one could use the following:

  if (IS_ENABLED(CONFIG_HWMON))
    if (!bcm54140_is_pkg_init(phydev)) {
      ret = bcm54140_phy_probe_once(phydev);
      if (ret)
        return ret;
    }
  }

This will then optimize away the devm_hwmon_device_register() call.

 Documentation/hwmon/bcm54140.rst |  45 ++++
 Documentation/hwmon/index.rst    |   1 +
 drivers/net/phy/Kconfig          |   1 +
 drivers/net/phy/bcm54140.c       | 397 +++++++++++++++++++++++++++++++
 4 files changed, 444 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/Kconfig b/drivers/net/phy/Kconfig
index cb7936b577de..bacfee41b564 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -349,6 +349,7 @@ config BROADCOM_PHY
 config BCM54140_PHY
 	tristate "Broadcom BCM54140 PHY"
 	depends on PHYLIB
+	depends on HWMON || HWMON=n
 	select BCM_NET_PHYLIB
 	help
 	  Support the Broadcom BCM54140 Quad SGMII/QSGMII PHY.
diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index 97465491b41b..cb0eb58eec76 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,69 @@
 #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_HWMON_TO_IN(ch, v) ((ch) ? BCM54140_HWMON_TO_IN_3V3(v) \
+					  : BCM54140_HWMON_TO_IN_1V0(v))
+#define BCM54140_HWMON_FROM_IN(ch, v) ((ch) ? BCM54140_HWMON_FROM_IN_3V3(v) \
+					    : BCM54140_HWMON_FROM_IN_1V0(v))
+#define BCM54140_HWMON_IN_MASK(ch) ((ch) ? BCM54140_RDB_MON_3V3_DATA_MASK \
+					 : BCM54140_RDB_MON_1V0_DATA_MASK)
+#define BCM54140_HWMON_IN_VAL_REG(ch) ((ch) ? BCM54140_RDB_MON_3V3_VAL \
+					    : BCM54140_RDB_MON_1V0_VAL)
+#define BCM54140_HWMON_IN_MIN_REG(ch) ((ch) ? BCM54140_RDB_MON_3V3_MIN \
+					    : BCM54140_RDB_MON_1V0_MIN)
+#define BCM54140_HWMON_IN_MAX_REG(ch) ((ch) ? BCM54140_RDB_MON_3V3_MAX \
+					    : BCM54140_RDB_MON_1V0_MAX)
+#define BCM54140_HWMON_IN_ALARM_BIT(ch) ((ch) ? BCM54140_RDB_MON_ISR_3V3 \
+					      : BCM54140_RDB_MON_ISR_1V0)
 
 #define BCM54140_DEFAULT_DOWNSHIFT 5
 #define BCM54140_MAX_DOWNSHIFT 9
@@ -57,6 +121,261 @@
 struct bcm54140_phy_priv {
 	int port;
 	int base_addr;
+#if IS_ENABLED(CONFIG_HWMON)
+	bool pkg_init;
+	/* protect the alarm bits */
+	struct mutex alarm_lock;
+	u16 alarm;
+#endif
+};
+
+#if IS_ENABLED(CONFIG_HWMON)
+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;
+	int tmp, ret = 0;
+
+	mutex_lock(&priv->alarm_lock);
+
+	/* latch any alarm bits */
+	tmp = bcm_phy_read_rdb(phydev, BCM54140_RDB_MON_ISR);
+	if (tmp < 0) {
+		ret = tmp;
+		goto out;
+	}
+	priv->alarm |= tmp;
+
+	*val = !!(priv->alarm & bit);
+	priv->alarm &= ~bit;
+
+out:
+	mutex_unlock(&priv->alarm_lock);
+	return ret;
+}
+
+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 bit, reg, tmp;
+
+	switch (attr) {
+	case hwmon_in_input:
+		reg = BCM54140_HWMON_IN_VAL_REG(channel);
+		break;
+	case hwmon_in_min:
+		reg = BCM54140_HWMON_IN_MIN_REG(channel);
+		break;
+	case hwmon_in_max:
+		reg = BCM54140_HWMON_IN_MAX_REG(channel);
+		break;
+	case hwmon_in_alarm:
+		bit = BCM54140_HWMON_IN_ALARM_BIT(channel);
+		return bcm54140_hwmon_read_alarm(dev, bit, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	tmp = bcm_phy_read_rdb(phydev, reg);
+	if (tmp < 0)
+		return tmp;
+
+	tmp &= BCM54140_HWMON_IN_MASK(channel);
+	*val = BCM54140_HWMON_TO_IN(channel, tmp);
+
+	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 mask = BCM54140_RDB_MON_TEMP_DATA_MASK;
+	u16 reg;
+
+	val = clamp_val(val, BCM54140_HWMON_TO_TEMP(mask),
+			BCM54140_HWMON_TO_TEMP(0));
+
+	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, 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 = BCM54140_HWMON_IN_MASK(channel);
+	u16 reg;
+
+	val = clamp_val(val, 0, BCM54140_HWMON_TO_IN(channel, mask));
+
+	switch (attr) {
+	case hwmon_in_min:
+		reg = BCM54140_HWMON_IN_MIN_REG(channel);
+		break;
+	case hwmon_in_max:
+		reg = BCM54140_HWMON_IN_MAX_REG(channel);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return bcm_phy_modify_rdb(phydev, reg, mask,
+				  BCM54140_HWMON_FROM_IN(channel, val));
+}
+
+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 +522,72 @@ 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 bcm54140_phy_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_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);
+}
+#endif
+
 static int bcm54140_phy_probe(struct phy_device *phydev)
 {
 	struct bcm54140_phy_priv *priv;
@@ -218,6 +603,18 @@ static int bcm54140_phy_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+#if IS_ENABLED(CONFIG_HWMON)
+	mutex_init(&priv->alarm_lock);
+
+	if (!bcm54140_is_pkg_init(phydev)) {
+		ret = bcm54140_phy_probe_once(phydev);
+		if (ret)
+			return ret;
+	}
+
+	priv->pkg_init = true;
+#endif
+
 	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 v2 2/3] net: phy: add Broadcom BCM54140 support

[Andrew Lunn]

On Sun, Apr 19, 2020 at 12:12:48PM +0200, Michael Walle wrote:

Hi Michael

> +static int bcm54140_b0_workaround(struct phy_device *phydev)
> +{
> +	int spare3;
> +	int ret;

Could you add a comment about what this is working around?

> +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);

phydev_dbg() ? Do we need to see this message four times?

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

What are the reset default for LEDs? Can the LEDs be configured via
strapping pins? There is currently no good solution for this. Whatever
you pick will be wrong for somebody else. At minimum, strapping pins,
if they exist, should not be overridden.

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

[Andrew Lunn]

On Sun, Apr 19, 2020 at 12:12:49PM +0200, Michael Walle wrote:

Hi Michael

You have an #if here...

> +#if IS_ENABLED(CONFIG_HWMON)
> +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 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 +522,72 @@ static int bcm54140_get_base_addr_and_port(struct phy_device *phydev)
>  	return 0;
>  }


Still inside the #if. Some original code is now inside the #if/#endif.
Is this correct? Hard to see from just the patch.

>  
> +/* 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_phy_priv *priv = phydev->priv;
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int base_addr = priv->base_addr;
> +	struct phy_device *phy;
> +	int i;
> +

...

> +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);
> +}
> +#endif


Thanks
  Andrew

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

[Michael Walle]

Am 2020-04-19 17:49, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 12:12:48PM +0200, Michael Walle wrote:
> 
> Hi Michael
> 
>> +static int bcm54140_b0_workaround(struct phy_device *phydev)
>> +{
>> +	int spare3;
>> +	int ret;
> 
> Could you add a comment about what this is working around?

sure

> 
>> +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);
> 
> phydev_dbg() ? Do we need to see this message four times?

ok. every phy will have a different port. And keep in mind,
that you might have less than four ports/PHYs here. So I'd
like to keep that as a phydev_dbg() if you agree.

> 
>> +
>> +	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;
> 
> What are the reset default for LEDs? Can the LEDs be configured via
> strapping pins? There is currently no good solution for this. Whatever
> you pick will be wrong for somebody else. At minimum, strapping pins,
> if they exist, should not be overridden.

Fair enough. There are no strapping options, just the "default 
behaviour",
where LED1/2 indicates the speed, and LED3 just activity (no link
indication). And I just noticed that in this case the comment above is
wrong, because it is actually link/activity. Further, there are myriad
configuration options which I didn't want to encode altogether. So I've
just chosen the typical one (which actually matches our hardware), ie.
to have the "activity/led mode". The application note mentions some 
other
concrete modes, but I don't know if its worth implementing them. Maybe 
we
can have a enumeration of some distinct modes? Ie.

    broadcom,led-mode = <BCM54140_NO_CHANGE>;
    broadcom,led-mode = <BCM54140_ACT_LINK_MODE>;

-michael

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

[Andrew Lunn]

On Sun, Apr 19, 2020 at 06:33:40PM +0200, Michael Walle wrote:
> > > +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);
> > 
> > phydev_dbg() ? Do we need to see this message four times?
> 
> ok. every phy will have a different port. And keep in mind,
> that you might have less than four ports/PHYs here. So I'd
> like to keep that as a phydev_dbg() if you agree.

phydev_dbg() is fine.


> 
> > 
> > > +
> > > +	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;
> > 
> > What are the reset default for LEDs? Can the LEDs be configured via
> > strapping pins? There is currently no good solution for this. Whatever
> > you pick will be wrong for somebody else. At minimum, strapping pins,
> > if they exist, should not be overridden.
> 
> Fair enough. There are no strapping options, just the "default behaviour",
> where LED1/2 indicates the speed, and LED3 just activity (no link
> indication). And I just noticed that in this case the comment above is
> wrong, because it is actually link/activity. Further, there are myriad
> configuration options which I didn't want to encode altogether. So I've
> just chosen the typical one (which actually matches our hardware), ie.
> to have the "activity/led mode". The application note mentions some other
> concrete modes, but I don't know if its worth implementing them. Maybe we
> can have a enumeration of some distinct modes? Ie.
> 
>    broadcom,led-mode = <BCM54140_NO_CHANGE>;
>    broadcom,led-mode = <BCM54140_ACT_LINK_MODE>;

Configuring LEDs is a mess at the moment. No two PHYs do it the
same. For a long time i've had a TODO item to make PHY LEDs work just
like every other LED in linux, and you can set trigger actions which
are then implemented in hardware.

We have been pushing back on adding DT properties, it just makes the
problem worse. If reset defaults are good enough for you, please leave
it at that.

Thanks
   Andrew

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

[Michael Walle]

Am 2020-04-19 17:56, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 12:12:49PM +0200, Michael Walle wrote:
> 
> Hi Michael
> 
> You have an #if here...
> 
>> +#if IS_ENABLED(CONFIG_HWMON)
>> +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 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 +522,72 @@ static int bcm54140_get_base_addr_and_port(struct 
>> phy_device *phydev)
>>  	return 0;
>>  }
> 
> 
> Still inside the #if. Some original code is now inside the #if/#endif.
> Is this correct? Hard to see from just the patch.

Whoops you're correct, something got messed up here. Will
fix that in the next version.

-michael


> 
>> 
>> +/* 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_phy_priv *priv = phydev->priv;
>> +	struct mii_bus *bus = phydev->mdio.bus;
>> +	int base_addr = priv->base_addr;
>> +	struct phy_device *phy;
>> +	int i;
>> +
> 
> ...
> 
>> +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);
>> +}
>> +#endif
> 
> 
> Thanks
>   Andrew

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

[Michael Walle]

Am 2020-04-19 18:49, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 06:33:40PM +0200, Michael Walle wrote:
>> >
>> > > +
>> > > +	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;
>> >
>> > What are the reset default for LEDs? Can the LEDs be configured via
>> > strapping pins? There is currently no good solution for this. Whatever
>> > you pick will be wrong for somebody else. At minimum, strapping pins,
>> > if they exist, should not be overridden.
>> 
>> Fair enough. There are no strapping options, just the "default 
>> behaviour",
>> where LED1/2 indicates the speed, and LED3 just activity (no link
>> indication). And I just noticed that in this case the comment above is
>> wrong, because it is actually link/activity. Further, there are myriad
>> configuration options which I didn't want to encode altogether. So 
>> I've
>> just chosen the typical one (which actually matches our hardware), ie.
>> to have the "activity/led mode". The application note mentions some 
>> other
>> concrete modes, but I don't know if its worth implementing them. Maybe 
>> we
>> can have a enumeration of some distinct modes? Ie.
>> 
>>    broadcom,led-mode = <BCM54140_NO_CHANGE>;
>>    broadcom,led-mode = <BCM54140_ACT_LINK_MODE>;
> 
> Configuring LEDs is a mess at the moment. No two PHYs do it the
> same. For a long time i've had a TODO item to make PHY LEDs work just
> like every other LED in linux, and you can set trigger actions which
> are then implemented in hardware.
> 
> We have been pushing back on adding DT properties, it just makes the
> problem worse. If reset defaults are good enough for you, please leave
> it at that.

Unfortunately not. We need the link/act, which I presume will also be
used by most other users, thus the driver enables this setting. I don't
know any board which just have an activity led, that is just off and
blinks for a short time if there is RX or TX, which is the reset default
setting of this PHY.

-michael