[RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[Wolfram Sang]

The Renesas RZ/N1-extension board also connects only two out of three
LED outputs from the Marvell PHY to the actual LEDs. The already
existing setting MARVELL_PHY_LED0_LINK_LED1_ACTIVE fits this scenario,
but a device flag cannot be used because the PHYs use a generic MDIO bus
on which also PHYs from other vendors reside. So, the driver is updated
to count the number of LED nodes in DT. If the number is 2, the
alternative LED configuration is used, otherwise the default one.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Is this a proper approach? FYI I double checked that
of_get_child_count() is NULL safe.

 drivers/net/phy/marvell.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 623292948fa7..b967b4fcd25a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -843,7 +843,8 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 static void marvell_config_led(struct phy_device *phydev)
 {
 	u16 def_config;
-	int err;
+	int num_leds, err;
+	struct device_node *np_leds;
 
 	switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
 	/* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
@@ -857,7 +858,9 @@ static void marvell_config_led(struct phy_device *phydev)
 	 * LED[2] .. Blink, Activity
 	 */
 	case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
-		if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
+		np_leds = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
+		num_leds = of_get_child_count(np_leds);
+		if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE || num_leds == 2)
 			def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
 		else
 			def_config = MII_88E1510_PHY_LED_DEF;
-- 
2.47.2

Re: [RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[Andrew Lunn]

On Tue, Apr 08, 2025 at 08:30:56AM +0200, Wolfram Sang wrote:
> The Renesas RZ/N1-extension board also connects only two out of three
> LED outputs from the Marvell PHY to the actual LEDs. The already
> existing setting MARVELL_PHY_LED0_LINK_LED1_ACTIVE fits this scenario,
> but a device flag cannot be used because the PHYs use a generic MDIO bus
> on which also PHYs from other vendors reside. So, the driver is updated
> to count the number of LED nodes in DT. If the number is 2, the
> alternative LED configuration is used, otherwise the default one.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Hi Wolfram

Please make use of the LED binding:

&mdio {
        pinctrl-0 = <&mdio_pins>;
        pinctrl-names = "default";
        phy0: ethernet-phy@0 {
                reg = <0>;
                leds {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        led@0 {
                                reg = <0>;
                                color = <LED_COLOR_ID_WHITE>;
                                function = LED_FUNCTION_WAN;
                                default-state = "keep";
                        };
                };
        };

Just list the two LEDs you have connected.

	Andrew

Re: [RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

Hi Andrew,

> Please make use of the LED binding:
> 
> &mdio {
>         pinctrl-0 = <&mdio_pins>;
>         pinctrl-names = "default";
>         phy0: ethernet-phy@0 {
>                 reg = <0>;
>                 leds {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         led@0 {
>                                 reg = <0>;
>                                 color = <LED_COLOR_ID_WHITE>;
>                                 function = LED_FUNCTION_WAN;
>                                 default-state = "keep";
>                         };
>                 };
>         };
> 
> Just list the two LEDs you have connected.

Been there, didn't work. This is what I had:

	mdio {
		#address-cells = <1>;
		#size-cells = <0>;
		compatible = "snps,dwmac-mdio";

		phy_mii0: ethernet-phy@8 {
			reg = <8>;
			leds {
				#address-cells = <1>;
				#size-cells = <0>;
				led@0 {
					reg = <0>;
					color = <LED_COLOR_ID_GREEN>;
					function = LED_FUNCTION_LAN;
					default-state = "keep";
				};

				led@1 {
					reg = <1>;
					color = <LED_COLOR_ID_AMBER>;
					function = LED_FUNCTION_ACTIVITY;
					default-state = "keep";
				};
			};
		};
	};

I played around with LED_FUNCTION_* values. I looked at other
devicetrees but I only could find one-LED setups. I tried going to one
LED, too, with LED_COLOR_ID_MULTI. No success. Then, I looked at the
driver code and did not see a path that would enable
'MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE' via any DT configuration. Thus,
the above patch. If you have any further pointers how to do this
properly, I'd love to hear about them.

Thank you,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[Andrew Lunn]

On Wed, Apr 09, 2025 at 08:55:24AM +0200, Wolfram Sang wrote:
> Hi Andrew,
> 
> > Please make use of the LED binding:
> > 
> > &mdio {
> >         pinctrl-0 = <&mdio_pins>;
> >         pinctrl-names = "default";
> >         phy0: ethernet-phy@0 {
> >                 reg = <0>;
> >                 leds {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> > 
> >                         led@0 {
> >                                 reg = <0>;
> >                                 color = <LED_COLOR_ID_WHITE>;
> >                                 function = LED_FUNCTION_WAN;
> >                                 default-state = "keep";
> >                         };
> >                 };
> >         };
> > 
> > Just list the two LEDs you have connected.
> 
> Been there, didn't work. This is what I had:
> 
> 	mdio {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		compatible = "snps,dwmac-mdio";
> 
> 		phy_mii0: ethernet-phy@8 {
> 			reg = <8>;
> 			leds {
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				led@0 {
> 					reg = <0>;
> 					color = <LED_COLOR_ID_GREEN>;
> 					function = LED_FUNCTION_LAN;
> 					default-state = "keep";
> 				};
> 
> 				led@1 {
> 					reg = <1>;
> 					color = <LED_COLOR_ID_AMBER>;
> 					function = LED_FUNCTION_ACTIVITY;
> 					default-state = "keep";
> 				};
> 			};
> 		};
> 	};
> 
> I played around with LED_FUNCTION_* values.


Function is just to do with naming. What you want is to add

linux,default-trigger = "netdev";

and ensure you have drivers/leds/trigger/ledtrig-netdev.c in your
kernel.

You should then find that you gain an LED directory per LED in sysfs,
trigger has [netdev] and there are additional files you can use to
configure when the LED lights/blinks for different link speeds, RX and
TX etc.

	Andrew

Re: [RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4222 bytes --]


> You should then find that you gain an LED directory per LED in sysfs,
> trigger has [netdev] and there are additional files you can use to
> configure when the LED lights/blinks for different link speeds, RX and
> TX etc.

Again thanks for the pointer, yet I get weird results. After booting,
with the interface up:

===
# cd /sys/class/leds/stmmac-0:08:green:lan/
# ls -l
total 0
-rw-r--r--    1 root     root          4096 May  5 10:13 brightness
lrwxrwxrwx    1 root     root             0 May  5 10:13 device -> ../../../stmmac-0:08
-rw-r--r--    1 root     root          4096 May  5 10:13 device_name
-rw-r--r--    1 root     root          4096 May  5 10:13 full_duplex
-rw-r--r--    1 root     root          4096 May  5 10:13 half_duplex
-rw-r--r--    1 root     root          4096 May  5 10:13 interval
-rw-r--r--    1 root     root          4096 May  5 10:13 link
-r--r--r--    1 root     root          4096 May  5 10:13 max_brightness
-r--r--r--    1 root     root          4096 May  5 10:13 offloaded
drwxr-xr-x    2 root     root             0 May  5 10:13 power
-rw-r--r--    1 root     root          4096 May  5 10:13 rx
-rw-r--r--    1 root     root          4096 May  5 10:13 rx_err
lrwxrwxrwx    1 root     root             0 May  5 10:13 subsystem -> ../../../../../../../../../class/leds
-rw-r--r--    1 root     root             0 May  5 10:13 trigger
-rw-r--r--    1 root     root          4096 May  5 10:13 tx
-rw-r--r--    1 root     root          4096 May  5 10:13 tx_err
-rw-r--r--    1 root     root          4096 May  5 10:13 uevent
# cat trigger device_name offloaded 
none kbd-scrolllock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock [netdev] mmc0

0
===

This shows that 'netdev' trigger is selected, alas the device name is
empty and offloading is disabled despite the driver using those
callbacks. The only thing that works is setting 'brightness' manually.

If I now select the 'netdev' trigger _again_, things change:

===
# echo netdev > trigger
# ls -l
total 0
-rw-r--r--    1 root     root          4096 May  5 10:13 brightness
lrwxrwxrwx    1 root     root             0 May  5 10:13 device -> ../../../stmmac-0:08
-rw-r--r--    1 root     root          4096 May  5 10:17 device_name
-rw-r--r--    1 root     root          4096 May  5 10:17 full_duplex
-rw-r--r--    1 root     root          4096 May  5 10:17 half_duplex
-rw-r--r--    1 root     root          4096 May  5 10:17 interval
-rw-r--r--    1 root     root          4096 May  5 10:17 link
-rw-r--r--    1 root     root          4096 May  5 10:17 link_10
-rw-r--r--    1 root     root          4096 May  5 10:17 link_100
-rw-r--r--    1 root     root          4096 May  5 10:17 link_1000
-r--r--r--    1 root     root          4096 May  5 10:13 max_brightness
-r--r--r--    1 root     root          4096 May  5 10:17 offloaded
drwxr-xr-x    2 root     root             0 May  5 10:13 power
-rw-r--r--    1 root     root          4096 May  5 10:17 rx
-rw-r--r--    1 root     root          4096 May  5 10:17 rx_err
lrwxrwxrwx    1 root     root             0 May  5 10:13 subsystem -> ../../../../../../../../../class/leds
-rw-r--r--    1 root     root             0 May  5 10:17 trigger
-rw-r--r--    1 root     root          4096 May  5 10:17 tx
-rw-r--r--    1 root     root          4096 May  5 10:17 tx_err
-rw-r--r--    1 root     root          4096 May  5 10:13 uevent
# cat trigger device_name offloaded 
none kbd-scrolllock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock [netdev] mmc0
eth1
1
===

The 'link_*' files appeared, 'device_name' and 'offloaded' have the
expected values. But now the LED is blinking like crazy despite all the
rx/tx/whatnot triggers still set to 0.

At this point, I have to stop it because I currently have not the
bandwidth to go further. I will live with the default 'link only' setup.
I hope I will have some time in the future to add the activity led
properly.

Thank you for your assistance!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[Andrew Lunn]

On Thu, Apr 10, 2025 at 09:40:58AM +0200, Wolfram Sang wrote:
> 
> > You should then find that you gain an LED directory per LED in sysfs,
> > trigger has [netdev] and there are additional files you can use to
> > configure when the LED lights/blinks for different link speeds, RX and
> > TX etc.
> 
> Again thanks for the pointer, yet I get weird results. After booting,
> with the interface up:
> 
> ===
> # cd /sys/class/leds/stmmac-0:08:green:lan/
> # ls -l
> total 0
> -rw-r--r--    1 root     root          4096 May  5 10:13 brightness
> lrwxrwxrwx    1 root     root             0 May  5 10:13 device -> ../../../stmmac-0:08
> -rw-r--r--    1 root     root          4096 May  5 10:13 device_name
> -rw-r--r--    1 root     root          4096 May  5 10:13 full_duplex
> -rw-r--r--    1 root     root          4096 May  5 10:13 half_duplex
> -rw-r--r--    1 root     root          4096 May  5 10:13 interval
> -rw-r--r--    1 root     root          4096 May  5 10:13 link
> -r--r--r--    1 root     root          4096 May  5 10:13 max_brightness
> -r--r--r--    1 root     root          4096 May  5 10:13 offloaded
> drwxr-xr-x    2 root     root             0 May  5 10:13 power
> -rw-r--r--    1 root     root          4096 May  5 10:13 rx
> -rw-r--r--    1 root     root          4096 May  5 10:13 rx_err
> lrwxrwxrwx    1 root     root             0 May  5 10:13 subsystem -> ../../../../../../../../../class/leds
> -rw-r--r--    1 root     root             0 May  5 10:13 trigger
> -rw-r--r--    1 root     root          4096 May  5 10:13 tx
> -rw-r--r--    1 root     root          4096 May  5 10:13 tx_err
> -rw-r--r--    1 root     root          4096 May  5 10:13 uevent
> # cat trigger device_name offloaded 
> none kbd-scrolllock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock [netdev] mmc0
> 
> 0
> ===
> 
> This shows that 'netdev' trigger is selected, alas the device name is
> empty and offloading is disabled despite the driver using those
> callbacks. The only thing that works is setting 'brightness' manually.

There is a weak relationship between the MAC, in this case, stmmac,
and the PHY. They get created at different times, and have different
life cycles. The LEDs get created when the PHY is probed. This is
generally before the MAC is created. At that point, you can use the
LED as just another LED. However, due to the default trigger, the
netdev trigger will be assigned to the LED. But at this stage it is
useless.

Sometime later the MAC will get created. Generally, at this point, the
MAC and PHY are still not linked together.

When you open the device, i.e. configure it admin up, then the MAC
driver goes and finds its PHY and connects to it. It is only at this
point can the LED get the MAC device name, know what speeds are
supported etc, which is the subset of what the MAC and PHY support
etc.

> If I now select the 'netdev' trigger _again_, things change:

That was how the code was initially developed, and the most tested
scenario. Using DT to set the trigger came a lot later.

Due to the weak link between the MAC and the PHY, the LED trigger
firsts asks the PHY what MAC are you connected to when the trigger is
activated. This can return indicating it is not connected, and this is
likely with DT configuration.

The trigger also links into the netdev notifier chain. It gets called
when the MAC registers, changes its name, unregisters, or is
configured up. The admin up notifier is the one which normally links
the LED to the MAC. So if you have time to debug this further, i would
start from netdev_trig_notify().

> The 'link_*' files appeared, 'device_name' and 'offloaded' have the
> expected values. But now the LED is blinking like crazy despite all the
> rx/tx/whatnot triggers still set to 0.

So that is odd. If offloaded indicates the hardware is doing the
blinking, that means we have a problem with the PHY configuration.
What model of Marvell PHY is it? There are some differences between
the models.

	Andrew

Re: [RFC PATCH net-next] net: phy: marvell: support DT configurations with only two LEDs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]


> > The 'link_*' files appeared, 'device_name' and 'offloaded' have the
> > expected values. But now the LED is blinking like crazy despite all the
> > rx/tx/whatnot triggers still set to 0.
> 
> So that is odd. If offloaded indicates the hardware is doing the
> blinking, that means we have a problem with the PHY configuration.
> What model of Marvell PHY is it? There are some differences between
> the models.

Thank you for the detailed explanations. I think they could be a basis
for a documentation file.

My PHY is a M88E1510. This is why changing its init to
MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE helps.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]