diff mbox series

net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

Message ID 20250110-marvell-88q2xxx-leds-v1-1-22e7734941c2@gmail.com (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx | expand

Commit Message

Dimitri Fedrau Jan. 10, 2025, 3:10 p.m. UTC
Marvell 88Q2XXX devices support up to two configurable Light Emitting
Diode (LED). Add minimal LED controller driver supporting the most common
uses with the 'netdev' trigger.

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
 drivers/net/phy/marvell-88q2xxx.c | 161 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 161 insertions(+)


---
base-commit: 3409aa9a349cc1d911c08eff3b265a4db48865c7
change-id: 20241221-marvell-88q2xxx-leds-69a4037b5157

Best regards,

Comments

Stefan Eichenberger Jan. 10, 2025, 5:27 p.m. UTC | #1
Hi Dimitri ,

On Fri, Jan 10, 2025 at 04:10:04PM +0100, Dimitri Fedrau wrote:
> Marvell 88Q2XXX devices support up to two configurable Light Emitting
> Diode (LED). Add minimal LED controller driver supporting the most common
> uses with the 'netdev' trigger.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 161 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 161 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 5107f58338aff4ed6cfea4d91e37282d9bb60ba5..bef3357b9d279fca5d1f86ff0eaa0d45a699e3f9 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -8,6 +8,7 @@
>   */
>  #include <linux/ethtool_netlink.h>
>  #include <linux/marvell_phy.h>
> +#include <linux/of.h>
>  #include <linux/phy.h>
>  #include <linux/hwmon.h>
>  
> @@ -27,6 +28,9 @@
>  #define MDIO_MMD_AN_MV_STAT2_100BT1		0x2000
>  #define MDIO_MMD_AN_MV_STAT2_1000BT1		0x4000
>  
> +#define MDIO_MMD_PCS_MV_RESET_CTRL		32768
> +#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE	0x8
> +
>  #define MDIO_MMD_PCS_MV_INT_EN			32784
>  #define MDIO_MMD_PCS_MV_INT_EN_LINK_UP		0x0040
>  #define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN	0x0080
> @@ -40,6 +44,15 @@
>  #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
>  #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
>  
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL			32790
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK		GENMASK(7, 4)
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK	GENMASK(3, 0)
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK		0x0 /* Link established */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX	0x1 /* Link established, blink for rx or tx activity */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX		0x4 /* Receive or Transmit activity */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX		0x5 /* Transmit activity */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1	0x7 /* 1000BT1 link established */
> +
>  #define MDIO_MMD_PCS_MV_TEMP_SENSOR1			32833
>  #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT		0x0001
>  #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT		0x0040
> @@ -95,6 +108,9 @@
>  
>  #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
>  
> +#define MV88Q2XXX_LED_INDEX_TX_ENABLE	0
> +#define MV88Q2XXX_LED_INDEX_GPIO	1

Not sure if I understand this. TX_ENABLE would be LED0 and GPIO would be
LED1? In my datasheet the 88Q222x only has a GPIO pin (which is also
TX_ENABLE), is this a problem? Would we need a led_count variable per
chip? 

In the 88Q2110 I can see that there is a TX_ENABLE (0) and a GPIO (1)
pin. In the register description they just call it LED [0] Control and
LED [1] Control. Maybe calling it LED_0 and LED_1 would be easier to
understand? Same for MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK and
MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK.

> +
>  struct mmd_val {
>  	int devad;
>  	u32 regnum;
> @@ -741,8 +757,58 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_OF_MDIO)
> +static int mv88q2xxx_leds_probe(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct device_node *leds;
> +	int ret = 0;
> +	u32 index;
> +
> +	if (!node)
> +		return 0;
> +
> +	leds = of_get_child_by_name(node, "leds");
> +	if (!leds)
> +		return 0;
> +
> +	for_each_available_child_of_node_scoped(leds, led) {
> +		ret = of_property_read_u32(led, "reg", &index);
> +		if (ret)
> +			goto exit;
> +
> +		if (index > MV88Q2XXX_LED_INDEX_GPIO) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
> +		if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
> +			ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
> +						 MDIO_MMD_PCS_MV_RESET_CTRL,
> +						 MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE);

If I understand it correctly, this switches the function of the pin from
TX_DISABLE to GPIO? Can you maybe add a comment here?

Regards,
Stefan
Andrew Lunn Jan. 10, 2025, 5:52 p.m. UTC | #2
> > +		if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
> > +			ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
> > +						 MDIO_MMD_PCS_MV_RESET_CTRL,
> > +						 MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE);
> 
> If I understand it correctly, this switches the function of the pin from
> TX_DISABLE to GPIO? Can you maybe add a comment here?

What is TX_DISABLE used for? I know it from SFPs, where it will
disable the laser. But here we are talking about a T1 PHY.

Do we have to be careful of use cases where the TX_DISABLE pin is
being used for TX_DISABLE?

	Andrew
Andrew Lunn Jan. 10, 2025, 5:56 p.m. UTC | #3
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL			32790
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK		GENMASK(7, 4)
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK	GENMASK(3, 0)
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK		0x0 /* Link established */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX	0x1 /* Link established, blink for rx or tx activity */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX		0x4 /* Receive or Transmit activity */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX		0x5 /* Transmit activity */
> +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1	0x7 /* 1000BT1 link established */

Are 2, 3 and 6 defined? It might be nice to include them just for
documentation.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index 5107f58338aff4ed6cfea4d91e37282d9bb60ba5..bef3357b9d279fca5d1f86ff0eaa0d45a699e3f9 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -8,6 +8,7 @@ 
  */
 #include <linux/ethtool_netlink.h>
 #include <linux/marvell_phy.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/hwmon.h>
 
@@ -27,6 +28,9 @@ 
 #define MDIO_MMD_AN_MV_STAT2_100BT1		0x2000
 #define MDIO_MMD_AN_MV_STAT2_1000BT1		0x4000
 
+#define MDIO_MMD_PCS_MV_RESET_CTRL		32768
+#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE	0x8
+
 #define MDIO_MMD_PCS_MV_INT_EN			32784
 #define MDIO_MMD_PCS_MV_INT_EN_LINK_UP		0x0040
 #define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN	0x0080
@@ -40,6 +44,15 @@ 
 #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL			32787
 #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS		0x0800
 
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL			32790
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK		GENMASK(7, 4)
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK	GENMASK(3, 0)
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK		0x0 /* Link established */
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX	0x1 /* Link established, blink for rx or tx activity */
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX		0x4 /* Receive or Transmit activity */
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX		0x5 /* Transmit activity */
+#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1	0x7 /* 1000BT1 link established */
+
 #define MDIO_MMD_PCS_MV_TEMP_SENSOR1			32833
 #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT		0x0001
 #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT		0x0040
@@ -95,6 +108,9 @@ 
 
 #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF			65246
 
+#define MV88Q2XXX_LED_INDEX_TX_ENABLE	0
+#define MV88Q2XXX_LED_INDEX_GPIO	1
+
 struct mmd_val {
 	int devad;
 	u32 regnum;
@@ -741,8 +757,58 @@  static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_OF_MDIO)
+static int mv88q2xxx_leds_probe(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *leds;
+	int ret = 0;
+	u32 index;
+
+	if (!node)
+		return 0;
+
+	leds = of_get_child_by_name(node, "leds");
+	if (!leds)
+		return 0;
+
+	for_each_available_child_of_node_scoped(leds, led) {
+		ret = of_property_read_u32(led, "reg", &index);
+		if (ret)
+			goto exit;
+
+		if (index > MV88Q2XXX_LED_INDEX_GPIO) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
+			ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
+						 MDIO_MMD_PCS_MV_RESET_CTRL,
+						 MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE);
+	}
+
+exit:
+	of_node_put(leds);
+
+	return ret;
+}
+
+#else
+static int mv88q2xxx_leds_probe(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif
+
 static int mv88q2xxx_probe(struct phy_device *phydev)
 {
+	int ret;
+
+	ret = mv88q2xxx_leds_probe(phydev);
+	if (ret)
+		return ret;
+
 	return mv88q2xxx_hwmon_probe(phydev);
 }
 
@@ -899,6 +965,98 @@  static int mv88q222x_cable_test_get_status(struct phy_device *phydev,
 	return 0;
 }
 
+static int mv88q2xxx_led_mode(u8 index, unsigned long rules)
+{
+	switch (rules) {
+	case BIT(TRIGGER_NETDEV_LINK):
+		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK;
+	case BIT(TRIGGER_NETDEV_LINK_1000):
+		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1;
+	case BIT(TRIGGER_NETDEV_TX):
+		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX;
+	case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX):
+		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX;
+	case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX):
+		return MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int mv88q2xxx_led_hw_is_supported(struct phy_device *phydev, u8 index,
+					 unsigned long rules)
+{
+	int mode;
+
+	mode = mv88q2xxx_led_mode(index, rules);
+	if (mode < 0)
+		return mode;
+
+	return 0;
+}
+
+static int mv88q2xxx_led_hw_control_set(struct phy_device *phydev, u8 index,
+					unsigned long rules)
+{
+	int mode;
+
+	mode = mv88q2xxx_led_mode(index, rules);
+	if (mode < 0)
+		return mode;
+
+	if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
+		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
+				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL,
+				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK,
+				      FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK,
+						 mode));
+	else
+		return phy_modify_mmd(phydev, MDIO_MMD_PCS,
+				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL,
+				      MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK,
+				      FIELD_PREP(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK,
+						 mode));
+}
+
+static int mv88q2xxx_led_hw_control_get(struct phy_device *phydev, u8 index,
+					unsigned long *rules)
+{
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_LED_FUNC_CTRL);
+	if (val < 0)
+		return val;
+
+	if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
+		val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK, val);
+	else
+		val = FIELD_GET(MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK, val);
+
+	switch (val) {
+	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK:
+		*rules = BIT(TRIGGER_NETDEV_LINK);
+		break;
+	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1:
+		*rules = BIT(TRIGGER_NETDEV_LINK_1000);
+		break;
+	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX:
+		*rules = BIT(TRIGGER_NETDEV_TX);
+		break;
+	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX:
+		*rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
+		break;
+	case MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX:
+		*rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) |
+			 BIT(TRIGGER_NETDEV_RX);
+		break;
+	default:
+		*rules = 0;
+		break;
+	}
+
+	return 0;
+}
+
 static struct phy_driver mv88q2xxx_driver[] = {
 	{
 		.phy_id			= MARVELL_PHY_ID_88Q2110,
@@ -934,6 +1092,9 @@  static struct phy_driver mv88q2xxx_driver[] = {
 		.get_sqi_max		= mv88q2xxx_get_sqi_max,
 		.suspend		= mv88q2xxx_suspend,
 		.resume			= mv88q2xxx_resume,
+		.led_hw_is_supported	= mv88q2xxx_led_hw_is_supported,
+		.led_hw_control_set	= mv88q2xxx_led_hw_control_set,
+		.led_hw_control_get	= mv88q2xxx_led_hw_control_get,
 	},
 };