| Message ID | 20230317023125.486-11-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: Add basic LED support for switch/phy | expand |
Hello Christian, also Rob Herring, Andrew Lunn and Pavel Machek, On Fri, 17 Mar 2023 03:31:21 +0100 Christian Marangi <ansuelsmth@gmail.com> wrote: > Add LEDs definition example for qca8k Switch Family to describe how they > should be defined for a correct usage. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > index 389892592aac..2e9c14af0223 100644 > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > @@ -18,6 +18,8 @@ description: > PHY it is connected to. In this config, an internal mdio-bus is registered and > the MDIO master is used for communication. Mixed external and internal > mdio-bus configurations are not supported by the hardware. > + Each phy has at least 3 LEDs connected and can be declared > + using the standard LEDs structure. > > properties: > compatible: > @@ -117,6 +119,7 @@ unevaluatedProperties: false > examples: > - | > #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/leds/common.h> > > mdio { > #address-cells = <1>; > @@ -226,6 +229,27 @@ examples: > label = "lan1"; > phy-mode = "internal"; > phy-handle = <&internal_phy_port1>; > + > + leds { > + #address-cells = <1>; > + #size-cells = <0>; > + > + led@0 { > + reg = <0>; > + color = <LED_COLOR_ID_WHITE>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <1>; > + default-state = "keep"; > + }; > + > + led@1 { > + reg = <1>; > + color = <LED_COLOR_ID_AMBER>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <1>; > + default-state = "keep"; > + }; > + }; > }; I have nothing against this, but I would like to point out the existence of the trigger-sources DT property, and I would like to discuss how this property should be used by the LED subsystem to choose default behaviour of a LED. Consider that we want to specify in device-tree that a PHY LED (or any other LED) should blink on network activity of the network device connected to this PHY (let's say the attached network device is eth0). (Why would we want to specify this in devicetree? Because currently the drivers either keep the behaviour from boot or change it to something specific that is not configurable.) We could specify in DT something like: eth0: ethernet-controller { ... } ethernet-phy { leds { led@0 { reg = <0>; color = <LED_COLOR_ID_GREEN>; trigger-sources = <ð0>; function = LED_FUNCTION_ ?????? ; } } } The above example specifies that the LED has a trigger source (eth0), but we still need to specify the trigger itself (for example that the LED should blink on activity, or the different kinds of link). In my opinion, this should be specified by the function property, but this property is currently used in other way: it is filled in with something like "wan" or "lan" or "wlan", an information which, IMO, should instead come from the devicename part of the LED, not the function part. Recall that the LED names are of the form devicename:color:function where the devicename part is supposed to be something like mmc0 or sda1. With LEDs that are associated with network devices I think the corresponding name should be the name of the network device (like eth0), but there is the problem of network namespaces and also that network devices can be renamed :(. So one option how to specify the behaviour of the LED to blink on activity would be to set function = LED_FUNCTION_ACTIVITY; but this would conflict with how currently some devicetrees use "lan", "wlan" or "wan" as the function (which is IMO incorrect, as I said above). Another option would be to ignore the function and instead use additional argument in the trigger-source property, something like trigger-sources = <ð0 TRIGGER_SOURCE_ACTIVITY>; I would like to start a discussion on this and hear about your opinions, because I think that the trigger-sources and function properties were proposed in good faith, but currently the implementation and usage is a mess. Marek
On Fri, Mar 17, 2023 at 09:14:10AM +0100, Marek Behún wrote: > Hello Christian, also Rob Herring, Andrew Lunn and Pavel Machek, > > On Fri, 17 Mar 2023 03:31:21 +0100 > Christian Marangi <ansuelsmth@gmail.com> wrote: > > > Add LEDs definition example for qca8k Switch Family to describe how they > > should be defined for a correct usage. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > > index 389892592aac..2e9c14af0223 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > > @@ -18,6 +18,8 @@ description: > > PHY it is connected to. In this config, an internal mdio-bus is registered and > > the MDIO master is used for communication. Mixed external and internal > > mdio-bus configurations are not supported by the hardware. > > + Each phy has at least 3 LEDs connected and can be declared > > + using the standard LEDs structure. > > > > properties: > > compatible: > > @@ -117,6 +119,7 @@ unevaluatedProperties: false > > examples: > > - | > > #include <dt-bindings/gpio/gpio.h> > > + #include <dt-bindings/leds/common.h> > > > > mdio { > > #address-cells = <1>; > > @@ -226,6 +229,27 @@ examples: > > label = "lan1"; > > phy-mode = "internal"; > > phy-handle = <&internal_phy_port1>; > > + > > + leds { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + led@0 { > > + reg = <0>; > > + color = <LED_COLOR_ID_WHITE>; > > + function = LED_FUNCTION_LAN; > > + function-enumerator = <1>; > > + default-state = "keep"; > > + }; > > + > > + led@1 { > > + reg = <1>; > > + color = <LED_COLOR_ID_AMBER>; > > + function = LED_FUNCTION_LAN; > > + function-enumerator = <1>; > > + default-state = "keep"; > > + }; > > + }; > > }; > > I have nothing against this, but I would like to point out the > existence of the trigger-sources DT property, and I would like to > discuss how this property should be used by the LED subsystem to choose > default behaviour of a LED. > > Consider that we want to specify in device-tree that a PHY LED (or any > other LED) should blink on network activity of the network device > connected to this PHY (let's say the attached network device is eth0). > (Why would we want to specify this in devicetree? Because currently the > drivers either keep the behaviour from boot or change it to something > specific that is not configurable.) > > We could specify in DT something like: > eth0: ethernet-controller { > ... > } > > ethernet-phy { > leds { > led@0 { > reg = <0>; > color = <LED_COLOR_ID_GREEN>; > trigger-sources = <ð0>; > function = LED_FUNCTION_ ?????? ; > } > } > } > > The above example specifies that the LED has a trigger source (eth0), > but we still need to specify the trigger itself (for example that > the LED should blink on activity, or the different kinds of link). In my > opinion, this should be specified by the function property, but this > property is currently used in other way: it is filled in with something > like "wan" or "lan" or "wlan", an information which, IMO, > should instead come from the devicename part of the LED, not the > function part. > > Recall that the LED names are of the form > devicename:color:function > where the devicename part is supposed to be something like mmc0 or > sda1. With LEDs that are associated with network devices I think the > corresponding name should be the name of the network device (like eth0), > but there is the problem of network namespaces and also that network > devices can be renamed :(. > > So one option how to specify the behaviour of the LED to blink on > activity would be to set > function = LED_FUNCTION_ACTIVITY; > but this would conflict with how currently some devicetrees use "lan", > "wlan" or "wan" as the function (which is IMO incorrect, as I said > above). > > Another option would be to ignore the function and instead use > additional argument in the trigger-source property, something like > trigger-sources = <ð0 TRIGGER_SOURCE_ACTIVITY>; > > I would like to start a discussion on this and hear about your opinions, > because I think that the trigger-sources and function properties were > proposed in good faith, but currently the implementation and usage is a > mess. > I think we should continue and make this discussion when we start implementing the hw contro for these LEDs to configure them in DT. Currently we are implementing very basic support so everything will be in sw. Anyway just to give some ideas. Yes it sound a good idea to use the trigger-sources binding. My idea would be that trigger needs to have specific support for them. If this in mind netdev can be configured in DT and setup hw control to offload blink with the required interface passed. The current implementation still didn't include a way to configure the blink in DT as the series are already a bit big... (currently we have 3: - This series that already grow from 10 patch to 14 - A cleanup series for netdev trigger that is already 7 patch - hw control that is another big boy with 12 patch ) So our idea was to first implement the minor things and then polish and improve it. (to make it easier to review) But agree with you that it would be a nice idea to have a correct and good implementation for trigger-sources.
> We could specify in DT something like: > eth0: ethernet-controller { > ... > } > > ethernet-phy { > leds { > led@0 { > reg = <0>; > color = <LED_COLOR_ID_GREEN>; > trigger-sources = <ð0>; > function = LED_FUNCTION_ ?????? ; > } > } > } For generic case, where you just have an LED on some random bus, you need to know what netdev it should represent. And in effect, this patch series gives you just that. However, when we get to offload, which is the ultimate goal, things are different. We cannot expect an LED inside the PHY connected to eth42 to offload blinking for eth24. There is a clear hardware relationship between the LED and the netdev. And in the more general case, there will always be a hardware relationship, be it wifi activity, or hard disk activity. phylib knows this relationship, and the DSA core also knows this relationship. Probably the SATA core will know the relationship. So i have a patchset which adds an op to the cdev to ask it, what struct device do you HW blink for? trigger-sources then becomes optional. And in fact, if it is used, you need to verify if it fits to this relationship, and if not, refuse to offload blinking, so software blinking only. Anyway, that is an aside to your main question. But the Day Job is calling. I will address your question later today. Andrew
> I would like to start a discussion on this and hear about your opinions, > because I think that the trigger-sources and function properties were > proposed in good faith, but currently the implementation and usage is a > mess. Hi Marek We are pushing the boundaries of the LED code here, doing things which have not been done before, as far as i know. So i expect some discussion about this. However, that discussion should not really affect this patchset, which is just adding plain boring software controlled LEDs. A quick recap about ledtrig-netdev. If you have a plain boring LED, you have: root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls brightness device max_brightness power subsystem trigger uevent You can turn the LED on with root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 1 > brightness and turn it off with: root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 0 > brightness You select the trigger via the trigger sysfs file: root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# cat trigger [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 timer heartbeat netdev mmc0 root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo netdev > trigger root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls activity brightness device_name half_duplex link link_100 max_brightness rx trigger uevent available_mode device full_duplex interval link_10 link_1000 power subsystem tx When you select a trigger, that trigger can add additional sysfs files. For the netdev trigger we gain link, link_10, link_100, link_1000, rx & tx. Nothing special here, if you selected the timer trigger you get delay_off delay_on. The oneshot trigger has invert, delay_on, delay_off etc. You then configure the trigger via setting values in the sysfs files. If you want the LED to indicate if there is link, you would do: echo 1 > link The LED would then light up if the netdev has carrier. If you want link plus RX packets echo 1 > link echo 1 > rx The LED will then be on if there is link, and additionally blink if the netdev stats indicate received frames. For the netdev trigger, all the configuration values are boolean. So a simple way to represent this in DT would be boolean properties: netdev-link = <1>; netdev->rx = <1>; We probably want these properties name spaced, because we have oneshot delay_on and timer delay_on for example. The same sysfs name could have different types, bool vs milliseconds, etc. I would make it, that when the trigger is activated, the values are read from DT and used. There is currently no persistent state for triggers. If you where to swap to the timer trigger and then return to the netdev trigger, all state is lost, so i would re-read DT. Offloading to hardware should not make an difference here. All we are going to do is pass the current configuration to the LED and ask it, can you do this? If it says no, we keep blinking in software. If yes, we leave the blinking to the hardware. There is the open question of if DT should be used like this. It is not describing hardware, it is describing configuration of hardware. So it could well get rejected. You then need to configure it in software. Andrew
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml index 389892592aac..2e9c14af0223 100644 --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml @@ -18,6 +18,8 @@ description: PHY it is connected to. In this config, an internal mdio-bus is registered and the MDIO master is used for communication. Mixed external and internal mdio-bus configurations are not supported by the hardware. + Each phy has at least 3 LEDs connected and can be declared + using the standard LEDs structure. properties: compatible: @@ -117,6 +119,7 @@ unevaluatedProperties: false examples: - | #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/leds/common.h> mdio { #address-cells = <1>; @@ -226,6 +229,27 @@ examples: label = "lan1"; phy-mode = "internal"; phy-handle = <&internal_phy_port1>; + + leds { + #address-cells = <1>; + #size-cells = <0>; + + led@0 { + reg = <0>; + color = <LED_COLOR_ID_WHITE>; + function = LED_FUNCTION_LAN; + function-enumerator = <1>; + default-state = "keep"; + }; + + led@1 { + reg = <1>; + color = <LED_COLOR_ID_AMBER>; + function = LED_FUNCTION_LAN; + function-enumerator = <1>; + default-state = "keep"; + }; + }; }; port@2 {
Add LEDs definition example for qca8k Switch Family to describe how they should be defined for a correct usage. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- .../devicetree/bindings/net/dsa/qca8k.yaml | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+)