diff mbox series

[net-next,v6,16/16] arm: mvebu: dt: Add PHY LED support for 370-rd WAN port

Message ID 20230327141031.11904-17-ansuelsmth@gmail.com
State New
Headers show
Series net: Add basic LED support for switch/phy | expand

Commit Message

Christian Marangi March 27, 2023, 2:10 p.m. UTC 
From: Andrew Lunn <andrew@lunn.ch>

The WAN port of the 370-RD has a Marvell PHY, with one LED on
the front panel. List this LED in the device tree.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 arch/arm/boot/dts/armada-370-rd.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Rob Herring April 3, 2023, 6:46 p.m. UTC | #1 
On Mon, Mar 27, 2023 at 04:10:31PM +0200, Christian Marangi wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> The WAN port of the 370-RD has a Marvell PHY, with one LED on
> the front panel. List this LED in the device tree.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  arch/arm/boot/dts/armada-370-rd.dts | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
> index be005c9f42ef..15b36aa34ef4 100644
> --- a/arch/arm/boot/dts/armada-370-rd.dts
> +++ b/arch/arm/boot/dts/armada-370-rd.dts
> @@ -20,6 +20,7 @@
>  /dts-v1/;
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/leds/common.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include "armada-370.dtsi"
>  
> @@ -135,6 +136,19 @@ &mdio {
>  	pinctrl-names = "default";
>  	phy0: ethernet-phy@0 {
>  		reg = <0>;
> +		leds {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			led@0 {
> +				reg = <0>;
> +				label = "WAN";

WAN or

> +				color = <LED_COLOR_ID_WHITE>;
> +				function = LED_FUNCTION_LAN;

LAN?

> +				function-enumerator = <1>;
> +				linux,default-trigger = "netdev";
> +			};
> +		};
>  	};
>  
>  	switch: switch@10 {
> -- 
> 2.39.2
>
Andrew Lunn April 3, 2023, 7:28 p.m. UTC | #2 
> > +		leds {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			led@0 {
> > +				reg = <0>;
> > +				label = "WAN";
> 
> WAN or
> 
> > +				color = <LED_COLOR_ID_WHITE>;
> > +				function = LED_FUNCTION_LAN;
> 
> LAN?

Hi Rob

I did not know there was LED_FUNCTION_WAN. I just blindly copied it
from some other DT fragment.

I will change this, thanks.

	Andrew
Pavel Machek April 3, 2023, 7:54 p.m. UTC | #3 
Hi!

> > > The WAN port of the 370-RD has a Marvell PHY, with one LED on
> > > the front panel. List this LED in the device tree.
> > 
> > > @@ -135,6 +136,19 @@ &mdio {
> > >  	pinctrl-names = "default";
> > >  	phy0: ethernet-phy@0 {
> > >  		reg = <0>;
> > > +		leds {
> > > +			#address-cells = <1>;
> > > +			#size-cells = <0>;
> > > +
> > > +			led@0 {
> > > +				reg = <0>;
> > > +				label = "WAN";
> > > +				color = <LED_COLOR_ID_WHITE>;
> > > +				function = LED_FUNCTION_LAN;
> > > +				function-enumerator = <1>;
> > > +				linux,default-trigger = "netdev";
> > > +			};
> > 
> > /sys/class/leds/WAN is not acceptable.
> 
> As i said here, that is not what it gets called:
> 
> https://lore.kernel.org/netdev/aa2d0a8b-b98b-4821-9413-158be578e8e0@lunn.ch/T/#m6c72bd355df3fcf8babc0d01dd6bf2697d069407
> 
> > It can be found in /sys/class/leds/f1072004.mdio-mii:00:WAN. But when
> > we come to using it for ledtrig-netdev, the user is more likely to follow
> > /sys/class/net/eth0/phydev/leds/f1072004.mdio-mii\:00\:WAN/
> 
> Is that acceptable?
> 
> What are the acceptance criteria?

Acceptance criteria would be "consistent with documentation and with
other similar users". If the LED is really white, it should be
f1072004.mdio-mii\:white\:WAN, but you probably want
f1072004.mdio-mii\:white\:LAN (or :activity), as discussed elsewhere in the thread.

Documentation is in Documentation/leds/well-known-leds.txt , so you
should probably add a new section about networking, and explain naming
scheme for network activity LEDs. When next users appear, I'll point
them to the documentation.

Does that sound ok?

Best regards,
								Pavel
Andrew Lunn April 4, 2023, 7:52 p.m. UTC | #4 
> Acceptance criteria would be "consistent with documentation and with
> other similar users". If the LED is really white, it should be
> f1072004.mdio-mii\:white\:WAN, but you probably want
> f1072004.mdio-mii\:white\:LAN (or :activity), as discussed elsewhere in the thread.

Hi Pavel

What i ended up with is:

f1072004.mdio-mii:00:white:wan

The label on the box is WAN, since it is meant to be a WiFi routers,
and this port should connected to your WAN. And this is what the LED
code came up with, given my DT description for this device.

> Documentation is in Documentation/leds/well-known-leds.txt , so you
> should probably add a new section about networking, and explain naming
> scheme for network activity LEDs. When next users appear, I'll point
> them to the documentation.

I added a patch with the following text:

* Ethernet LEDs

Currently two types of Network LEDs are support, those controlled by
the PHY and those by the MAC. In theory both can be present at the
same time for one Linux netdev, hence the names need to differ between
MAC and PHY.

Do not use the netdev name, such as eth0, enp1s0. These are not stable
and are not unique. They also don't differentiate between MAC and PHY.

** MAC LEDs

Good: f1070000.ethernet:white:WAN
Good: mdio_mux-0.1:00:green:left
Good: 0000:02:00.0:yellow:top

The first part must uniquely name the MAC controller. Then follows the
colour.  WAN/LAN should be used for a single LED. If there are
multiple LEDs, use left/right, or top/bottom to indicate their
position on the RJ45 socket.

** PHY LEDs

Good: f1072004.mdio-mii:00: white:WAN
Good: !mdio-mux!mdio@2!switch@0!mdio:01:green:right
Good: r8169-0-200:00:yellow:bottom

The first part must uniquely name the PHY. This often means uniquely
identifying the MDIO bus controller, and the address on the bus.


	Andrew
Pavel Machek April 6, 2023, 9:18 a.m. UTC | #5 
Hi!

> > Acceptance criteria would be "consistent with documentation and with
> > other similar users". If the LED is really white, it should be
> > f1072004.mdio-mii\:white\:WAN, but you probably want
> > f1072004.mdio-mii\:white\:LAN (or :activity), as discussed elsewhere in the thread.
> 
> Hi Pavel
> 
> What i ended up with is:
> 
> f1072004.mdio-mii:00:white:wan
> 
> The label on the box is WAN, since it is meant to be a WiFi routers,
> and this port should connected to your WAN. And this is what the LED
> code came up with, given my DT description for this device.

Ok, thanks for explanation.

> > Documentation is in Documentation/leds/well-known-leds.txt , so you
> > should probably add a new section about networking, and explain naming
> > scheme for network activity LEDs. When next users appear, I'll point
> > them to the documentation.
> 
> I added a patch with the following text:
> 
> * Ethernet LEDs
> 
> Currently two types of Network LEDs are support, those controlled by
> the PHY and those by the MAC. In theory both can be present at the
> same time for one Linux netdev, hence the names need to differ between
> MAC and PHY.
> 
> Do not use the netdev name, such as eth0, enp1s0. These are not stable
> and are not unique. They also don't differentiate between MAC and PHY.
> 
> ** MAC LEDs
> 
> Good: f1070000.ethernet:white:WAN
> Good: mdio_mux-0.1:00:green:left
> Good: 0000:02:00.0:yellow:top

> The first part must uniquely name the MAC controller. Then follows the
> colour.  WAN/LAN should be used for a single LED. If there are
> multiple LEDs, use left/right, or top/bottom to indicate their
> position on the RJ45 socket.

I don't think basing stuff on position is reasonable. (And am not sure
if making difference between MAC and PHY leds is good idea).

Normally, there's ethernet port with two LEDs, one is usually green
and indicates link, second being yellow and indicates activity,
correct?

On devices like ADSL modems, there is one LED per port, typically on
with link and blinking with activity.  

Could we use that distinction instead? (id):green:link,
(id):yellow:activity, (id):?:linkact -- for combined LED as it seems.

Are there any other common leds? I seem to remember "100mbps" lights
from time where 100mbit was fast...?

Best regards,
								Pavel
Andrew Lunn April 6, 2023, 1:54 p.m. UTC | #6 
> I don't think basing stuff on position is reasonable. (And am not sure
> if making difference between MAC and PHY leds is good idea).
> 
> Normally, there's ethernet port with two LEDs, one is usually green
> and indicates link, second being yellow and indicates activity,
> correct?

Nope. I have machines with 1, 2 or 3 LEDs. I have green, yellow, white
and red LEDs.

Part of the problem is 802.3 says absolutely nothing about LEDs. So
every vendor is free to do whatever why want. There is no
standardisation at all. So we have to assume every vendor does
something different.

> On devices like ADSL modems, there is one LED per port, typically on
> with link and blinking with activity.  
> 
> Could we use that distinction instead? (id):green:link,
> (id):yellow:activity, (id):?:linkact -- for combined LED as it seems.
> 
> Are there any other common leds? I seem to remember "100mbps" lights
> from time where 100mbit was fast...?

But what about 2.5G, 5G, 10G, 40G... And 10Mbps for automotive. And
collision for 1/2 duplex, which is making a bit of a comeback in
automotive.

Plus, we are using ledtrig-netdev. A wifi device is a netdev. A CAN
bus devices is a netdev. Link speed has a totally different meaning
for 802.11 and CAN.

You are also assuming the LEDs have fixed meaning. But they are not
fixed, they mean whatever the ledtrig-netdev is configured to make
them blink.  I even have one of my boxes blinking heartbeat, because
if has a habit of crashing... And i think for Linux LEDs in general,
we should not really tie an LED to a meaning. Maybe tie it to a label
on the case, but the meaning of an LED is all about software, what
ledtrig- is controlling it.

As to differentiating MAC and PHY, we need to, because as i said, both
could offer LEDs. Generally, Ethernet switches have LED controllers
per MAC port. Most switches have internal PHYs, and those PHYs don't
have LED controllers. However, not all ports have internal PHYs, there
can be external PHYs with its own LED controller. So in that case,
both the MAC and the PHY could register an LED controller for the same
netdev. It comes down to DT to indicate what LED controllers are
actually wired to an LED.

	 Andrew
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/armada-370-rd.dts b/arch/arm/boot/dts/armada-370-rd.dts
index be005c9f42ef..15b36aa34ef4 100644
--- a/arch/arm/boot/dts/armada-370-rd.dts
+++ b/arch/arm/boot/dts/armada-370-rd.dts
@@ -20,6 +20,7 @@ 
 /dts-v1/;
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/leds/common.h>
 #include <dt-bindings/gpio/gpio.h>
 #include "armada-370.dtsi"
 
@@ -135,6 +136,19 @@  &mdio {
 	pinctrl-names = "default";
 	phy0: ethernet-phy@0 {
 		reg = <0>;
+		leds {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 {
+				reg = <0>;
+				label = "WAN";
+				color = <LED_COLOR_ID_WHITE>;
+				function = LED_FUNCTION_LAN;
+				function-enumerator = <1>;
+				linux,default-trigger = "netdev";
+			};
+		};
 	};
 
 	switch: switch@10 {