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

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

[Dimitri Fedrau]

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

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

Best regards,
-- 
Dimitri Fedrau <dima.fedrau@gmail.com>

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Stefan Eichenberger]

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

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Andrew Lunn]

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

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Andrew Lunn]

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

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Dimitri Fedrau]

Hi Stefan,

Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> 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? 
> 
Yes you understand it correctly.
Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
For which device GPIO pin and TX_ENABLE are the same ?

> 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.
> 
I named them just as the pin. Probably it would be easier to understand,
but the mapping between pin and index would be lost. What do you think ?

> > +
> >  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?
>
You are right, will add the comment.

Best regards,
Dimitri Fedrau

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Dimitri Fedrau]

Am Fri, Jan 10, 2025 at 06:52:15PM +0100 schrieb Andrew Lunn:
> > > +		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.
> 

If pin input is LOW, then Tx packets will be stopped after link up, but Rx
packets are still received normally. The link will stay up and only idles
will be sent out the Tx media side.

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

The pin can be used for feature described above(default) or as a LED. So
when the LED function should be used we have to setup DT otherwise we
are already fine.

Dimitri

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Dimitri Fedrau]

Am Fri, Jan 10, 2025 at 06:56:24PM +0100 schrieb Andrew Lunn:
> > +#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.
> 
Ok, will add them in the next version.

Dimitri

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Stefan Eichenberger]

Hi Dimitri,

On Fri, Jan 10, 2025 at 07:30:58PM +0100, Dimitri Fedrau wrote:
> Hi Stefan,
> 
> Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> > 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? 
> > 
> Yes you understand it correctly.
> Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
> TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
> For which device GPIO pin and TX_ENABLE are the same ?
> 

Hmm in my datasheet the bits 4:7 in Register 3:0x8016 are marked as
reserved. However, maybe my datasheet is outdated. I have the TD-000217
Rev. 9 from November 10, 2023. It is for the following chips:
88Q2220/88Q2220M/88Q2221/88Q2221M/88Q1200M/88Q1201M
If it is documented in your datasheet, then it is fine. Maybe they just
forgot to document them in the one I have.

> > 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.
> > 
> I named them just as the pin. Probably it would be easier to understand,
> but the mapping between pin and index would be lost. What do you think ?
> 
> > > +
> > >  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?
> >
> You are right, will add the comment.

Perfekt, thanks.

Regards,
Stefan

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Stefan Eichenberger]

On Fri, Jan 10, 2025 at 07:30:58PM +0100, Dimitri Fedrau wrote:
> Hi Stefan,
> 
> Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> > 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? 
> > 
> Yes you understand it correctly.
> Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
> TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
> For which device GPIO pin and TX_ENABLE are the same ?
> 
> > 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.
> > 
> I named them just as the pin. Probably it would be easier to understand,
> but the mapping between pin and index would be lost. What do you think ?

I missed this one in the previous mail, sorry. I personally would name
it LED_0_CONTROL_MASK and LED_1_CONTROL_MASK because the description of
the register is "3:0 LED [0] Control". As index I would probably also
call it LED_0_INDEX and LED_1_INDEX because it is not directly related
to the pin functionality. But that's just my personal preference not
sure if it is really better.

Regards,
Stefan

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Dimitri Fedrau]

Hi Stefan,

Am Mon, Jan 13, 2025 at 08:57:31AM +0100 schrieb Stefan Eichenberger:
> Hi Dimitri,
> 
> On Fri, Jan 10, 2025 at 07:30:58PM +0100, Dimitri Fedrau wrote:
> > Hi Stefan,
> > 
> > Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> > > 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? 
> > > 
> > Yes you understand it correctly.
> > Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
> > TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
> > For which device GPIO pin and TX_ENABLE are the same ?
> > 
> 

> Hmm in my datasheet the bits 4:7 in Register 3:0x8016 are marked as
> reserved. However, maybe my datasheet is outdated. I have the TD-000217
> Rev. 9 from November 10, 2023. It is for the following chips:
> 88Q2220/88Q2220M/88Q2221/88Q2221M/88Q1200M/88Q1201M
> If it is documented in your datasheet, then it is fine. Maybe they just
> forgot to document them in the one I have.
> 
I think this is a mistake in the datasheet. I have a newer datasheet
from October 30, 2024 and it is still wrong in the register description.
When looking at 2.15 LED the missing bits are described. In datasheets for
88Q212x and 88Q211x it is correct. I think the datasheet is a bit messy.

Pin names also slightly differ in pinout and pin description in chapter
1. Maybe this was what you meant with having LED and GPIO pin for
88Q222x devices ? In the description the LED pins is referred as
LED/TX_ENABLE pin. Sorry for the misunderstanding, but I think naming it
TX_ENABLE is still right.

[...]

Best regards,
Dimitri

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Dimitri Fedrau]

Hi Stefan,

Am Mon, Jan 13, 2025 at 09:05:21AM +0100 schrieb Stefan Eichenberger:
> On Fri, Jan 10, 2025 at 07:30:58PM +0100, Dimitri Fedrau wrote:
> > Hi Stefan,
> > 
> > Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> > > 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? 
> > > 
> > Yes you understand it correctly.
> > Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
> > TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
> > For which device GPIO pin and TX_ENABLE are the same ?
> > 
> > > 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.
> > > 
> > I named them just as the pin. Probably it would be easier to understand,
> > but the mapping between pin and index would be lost. What do you think ?
> 
> I missed this one in the previous mail, sorry. I personally would name
> it LED_0_CONTROL_MASK and LED_1_CONTROL_MASK because the description of
> the register is "3:0 LED [0] Control". As index I would probably also
> call it LED_0_INDEX and LED_1_INDEX because it is not directly related
> to the pin functionality. But that's just my personal preference not
> sure if it is really better.
>
I think you are right, since the datasheet for 88Q222x is a bit messy
and datasheets for 88Q212x and 88Q211x name them "LED [0] Control" and
"LED [1] Control" I will do as you suggest. Thanks for pointing out.

I would stick to the index because you assign functionality when you
configure DT to do so. How should one know which pins belongs to which led
index, there is no documentation on this.

Best regards,
Dimitri

Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx

[Stefan Eichenberger]

Hi Dimitri,

On Mon, Jan 13, 2025 at 10:25:24AM +0100, Dimitri Fedrau wrote:
> Hi Stefan,
> 
> Am Mon, Jan 13, 2025 at 09:05:21AM +0100 schrieb Stefan Eichenberger:
> > On Fri, Jan 10, 2025 at 07:30:58PM +0100, Dimitri Fedrau wrote:
> > > Hi Stefan,
> > > 
> > > Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> > > > 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? 
> > > > 
> > > Yes you understand it correctly.
> > > Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
> > > TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
> > > For which device GPIO pin and TX_ENABLE are the same ?
> > > 
> > > > 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.
> > > > 
> > > I named them just as the pin. Probably it would be easier to understand,
> > > but the mapping between pin and index would be lost. What do you think ?
> > 
> > I missed this one in the previous mail, sorry. I personally would name
> > it LED_0_CONTROL_MASK and LED_1_CONTROL_MASK because the description of
> > the register is "3:0 LED [0] Control". As index I would probably also
> > call it LED_0_INDEX and LED_1_INDEX because it is not directly related
> > to the pin functionality. But that's just my personal preference not
> > sure if it is really better.
> >
> I think you are right, since the datasheet for 88Q222x is a bit messy
> and datasheets for 88Q212x and 88Q211x name them "LED [0] Control" and
> "LED [1] Control" I will do as you suggest. Thanks for pointing out.
> 
> I would stick to the index because you assign functionality when you
> configure DT to do so. How should one know which pins belongs to which led
> index, there is no documentation on this.

Perfect thanks, I agree with you on the index. If you can rename the
mask and add the comment as written in the previous mail, I'm happy with
the change.

Regards,
Stefan