| Message ID | 20230216013230.22978-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | Adds support for PHY LEDs with offload triggers | expand |
On Fri, Feb 17, 2023 at 03:30:13PM +0100, Andrew Lunn wrote: > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > This is another attempt on adding this feature on LEDs, hoping this is > > the right time and someone finally notice this. > > Hi Christian > > Thanks for keeping working on this. > > I want to review it, and maybe implement LED support in a PHY > driver. But i'm busy with reworking EEE at the moment. > > The merge window is about to open, so patches are not going to be > accepted for the next two weeks. So i will take a look within that > time and give you feedback. > Sure take your time happy to discuss any improvement to this.
On Fri, Feb 17, 2023 at 05:03:46PM -0600, Rob Herring wrote: > On Thu, Feb 16, 2023 at 02:32:28AM +0100, Christian Marangi wrote: > > Document the netdev trigger that makes the LED blink or turn on based on > > switch/phy events or an attached network interface. > > NAK. What is netdev? But netdev is a trigger, nothing new. Actually it was never documented. Is the linux,default-trigger getting deprecated? > > Don't add new linux,default-trigger entries either. We have better ways > to define trigger sources, namely 'trigger-sources'. > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > Documentation/devicetree/bindings/leds/common.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml > > index d34bb58c0037..6e016415a4d8 100644 > > --- a/Documentation/devicetree/bindings/leds/common.yaml > > +++ b/Documentation/devicetree/bindings/leds/common.yaml > > @@ -98,6 +98,8 @@ properties: > > # LED alters the brightness for the specified duration with one software > > # timer (requires "led-pattern" property) > > - pattern > > + # LED blink and turns on based on netdev events > > + - netdev > > - pattern: "^cpu[0-9]*$" > > - pattern: "^hci[0-9]+-power$" > > # LED is triggered by Bluetooth activity > > -- > > 2.38.1 > >
On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > This is another attempt on adding this feature on LEDs, hoping this is > the right time and someone finally notice this. Hi Christian Thanks for keeping working on this. I want to review it, and maybe implement LED support in a PHY driver. But i'm busy with reworking EEE at the moment. The merge window is about to open, so patches are not going to be accepted for the next two weeks. So i will take a look within that time and give you feedback. Andrew
On Thu, Feb 16, 2023 at 02:32:28AM +0100, Christian Marangi wrote: > Document the netdev trigger that makes the LED blink or turn on based on > switch/phy events or an attached network interface. NAK. What is netdev? Don't add new linux,default-trigger entries either. We have better ways to define trigger sources, namely 'trigger-sources'. > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > Documentation/devicetree/bindings/leds/common.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml > index d34bb58c0037..6e016415a4d8 100644 > --- a/Documentation/devicetree/bindings/leds/common.yaml > +++ b/Documentation/devicetree/bindings/leds/common.yaml > @@ -98,6 +98,8 @@ properties: > # LED alters the brightness for the specified duration with one software > # timer (requires "led-pattern" property) > - pattern > + # LED blink and turns on based on netdev events > + - netdev > - pattern: "^cpu[0-9]*$" > - pattern: "^hci[0-9]+-power$" > # LED is triggered by Bluetooth activity > -- > 2.38.1 >
On Fri, Feb 17, 2023 at 06:58:39AM +0100, Christian Marangi wrote: > On Fri, Feb 17, 2023 at 05:03:46PM -0600, Rob Herring wrote: > > On Thu, Feb 16, 2023 at 02:32:28AM +0100, Christian Marangi wrote: > > > Document the netdev trigger that makes the LED blink or turn on based on > > > switch/phy events or an attached network interface. > > > > NAK. What is netdev? > > But netdev is a trigger, nothing new. Actually it was never documented. Okay, please state that in the commit message. > Is the linux,default-trigger getting deprecated? Not quite, but it shouldn't be used for anything tied to a device IMO. Rob
On Fri, 17 Feb 2023, Andrew Lunn wrote: > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > This is another attempt on adding this feature on LEDs, hoping this is > > the right time and someone finally notice this. > > Hi Christian > > Thanks for keeping working on this. > > I want to review it, and maybe implement LED support in a PHY > driver. But i'm busy with reworking EEE at the moment. > > The merge window is about to open, so patches are not going to be > accepted for the next two weeks. So i will take a look within that > time and give you feedback. Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm going to need all the help I can get with complex submissions such as these.
On Wed, Feb 22, 2023 at 03:02:04PM +0000, Lee Jones wrote: > On Fri, 17 Feb 2023, Andrew Lunn wrote: > > > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > > This is another attempt on adding this feature on LEDs, hoping this is > > > the right time and someone finally notice this. > > > > Hi Christian > > > > Thanks for keeping working on this. > > > > I want to review it, and maybe implement LED support in a PHY > > driver. But i'm busy with reworking EEE at the moment. > > > > The merge window is about to open, so patches are not going to be > > accepted for the next two weeks. So i will take a look within that > > time and give you feedback. > > Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm > going to need all the help I can get with complex submissions such as > these. > Hi Lee, thanks for stepping in. Just wanted to tell you I got some message with Andrew to make this thing less problematic and to dry/make it more review friendly. We decided on pushing this in 3 step: 1. Propose most basic things for some switch and some PHY. (brightness and blink_set support only, already supported by LED core) 2. A small series that should be just a cleanup for the netdev trigger 3. Support for hw_control in the most possible clean and way with small patch to they are not hard to track and understand the concept of this feature. I'm starting with the step 1 and sending some of my patch and Andrew patch to add basic support and I will add you and LED mailing list in Cc. Again thanks for starting checking this and feel free to ask any question about this to me also privately, I'm very open to any help.
On Mon, 06 Mar 2023, Christian Marangi wrote: > On Wed, Feb 22, 2023 at 03:02:04PM +0000, Lee Jones wrote: > > On Fri, 17 Feb 2023, Andrew Lunn wrote: > > > > > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > > > This is another attempt on adding this feature on LEDs, hoping this is > > > > the right time and someone finally notice this. > > > > > > Hi Christian > > > > > > Thanks for keeping working on this. > > > > > > I want to review it, and maybe implement LED support in a PHY > > > driver. But i'm busy with reworking EEE at the moment. > > > > > > The merge window is about to open, so patches are not going to be > > > accepted for the next two weeks. So i will take a look within that > > > time and give you feedback. > > > > Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm > > going to need all the help I can get with complex submissions such as > > these. > > > > Hi Lee, > thanks for stepping in. Just wanted to tell you I got some message with > Andrew to make this thing less problematic and to dry/make it more > review friendly. > > We decided on pushing this in 3 step: > 1. Propose most basic things for some switch and some PHY. (brightness > and blink_set support only, already supported by LED core) > 2. A small series that should be just a cleanup for the netdev trigger > 3. Support for hw_control in the most possible clean and way with small > patch to they are not hard to track and understand the concept of this > feature. > > I'm starting with the step 1 and sending some of my patch and Andrew > patch to add basic support and I will add you and LED mailing list in > Cc. Sounds like a plan. Thank you both. > Again thanks for starting checking this and feel free to ask any > question about this to me also privately, I'm very open to any help. -- Lee Jones [李琼斯]
Hi Christian, thanks for your patch! On Thu, Feb 16, 2023 at 2:36 AM Christian Marangi <ansuelsmth@gmail.com> wrote: > The current idea is: > - LED driver implement 3 API (hw_control_status/start/stop). > They are used to put the LED in hardware mode and to configure the > various trigger. > - We have hardware triggers that are used to expose to userspace the > supported hardware mode and set the hardware mode on trigger > activation. > - We can also have triggers that both support hardware and software mode. > - The LED driver will declare each supported hardware blink mode and > communicate with the trigger all the supported blink modes that will > be available by sysfs. > - A trigger will use blink_set to configure the blink mode to active > in hardware mode. > - On hardware trigger activation, only the hardware mode is enabled but > the blink modes are not configured. The LED driver should reset any > link mode active by default. The series looks good as a start. There are some drivers and HW definitions etc for switch-controlled LEDs, which is great. I am a bit reluctant on the ambition to rely on configuration from sysfs for the triggers, and I am also puzzled to how a certain trigger on a certain LED is going to associate itself with, say, a certain port. I want to draw your attention to this recently merged patch series from Hans de Goede: https://lore.kernel.org/linux-leds/20230120114524.408368-1-hdegoede@redhat.com/ This adds the devm_led_get() API which works similar to getting regulators, clocks, GPIOs or any other resources. It is not yet (I think) hooked into the device tree framework, but it supports software nodes so adding DT handling should be sort of trivial. I think the ambition should be something like this (conjured example) for a DSA switch: platform { switch { compatible = "foo"; leds { #address-cells = <1>; #size-cells = <0>; led0: led@0 { reg = <0>; color =... function = ... function-enumerator = ... default-state = ... }; led1: led@1 { reg = <1>; color =... function = ... function-enumerator = ... default-state = ... }; }; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "lan0"; phy-handle = <&phy0>; leds = <&led0>; }; port@1 { reg = <1>; label = "lan1"; phy-handle = <&phy1>; leds = <&led0>; }; }; mdio { compatible = "foo-mdio"; #address-cells = <1>; #size-cells = <0>; phy0: ethernet-phy@0 { reg = <0>; }; phy1: ethernet-phy@1 { reg = <1>; }; }; }; }; I am not the man to tell whether the leds = <&led0>; phandle should be on the port or actually on the phy, it may even vary. You guys know the answer to this. But certainly something like this resource phandle will be necessary to assign the right LED to the right port or phy, I hope you were not going to rely on strings and naming conventions? Yours, Linus Walleij
Hi, On 3/9/23 10:09, Linus Walleij wrote: > Hi Christian, > > thanks for your patch! > > On Thu, Feb 16, 2023 at 2:36 AM Christian Marangi <ansuelsmth@gmail.com> wrote: > >> The current idea is: >> - LED driver implement 3 API (hw_control_status/start/stop). >> They are used to put the LED in hardware mode and to configure the >> various trigger. >> - We have hardware triggers that are used to expose to userspace the >> supported hardware mode and set the hardware mode on trigger >> activation. >> - We can also have triggers that both support hardware and software mode. >> - The LED driver will declare each supported hardware blink mode and >> communicate with the trigger all the supported blink modes that will >> be available by sysfs. >> - A trigger will use blink_set to configure the blink mode to active >> in hardware mode. >> - On hardware trigger activation, only the hardware mode is enabled but >> the blink modes are not configured. The LED driver should reset any >> link mode active by default. > > The series looks good as a start. > There are some drivers and HW definitions etc for switch-controlled > LEDs, which is great. > > I am a bit reluctant on the ambition to rely on configuration from sysfs > for the triggers, and I am also puzzled to how a certain trigger on a > certain LED is going to associate itself with, say, a certain port. > > I want to draw your attention to this recently merged patch series > from Hans de Goede: > https://lore.kernel.org/linux-leds/20230120114524.408368-1-hdegoede@redhat.com/ > > This adds the devm_led_get() API which works similar to getting > regulators, clocks, GPIOs or any other resources. > > It is not yet (I think) hooked into the device tree framework, but it > supports software nodes so adding DT handling should be sort of > trivial. That series contains this (unmerged) patch to hookup DT handling: https://lore.kernel.org/linux-leds/20230120114524.408368-6-hdegoede@redhat.com/ this was not merged because there are no current users, but adding support is as easy as picking up that patch :) Note there also already is a devicetree *only*: struct led_classdev *of_led_get(struct device_node *np, int index); Since I was working on a x86/ACPI platform I needed something more generic though and ideally new code would use the generic approach. Regards, Hans > > I think the ambition should be something like this (conjured example) > for a DSA switch: > > platform { > switch { > compatible = "foo"; > > leds { > #address-cells = <1>; > #size-cells = <0>; > led0: led@0 { > reg = <0>; > color =... > function = ... > function-enumerator = ... > default-state = ... > }; > led1: led@1 { > reg = <1>; > color =... > function = ... > function-enumerator = ... > default-state = ... > }; > }; > > ports { > #address-cells = <1>; > #size-cells = <0>; > port@0 { > reg = <0>; > label = "lan0"; > phy-handle = <&phy0>; > leds = <&led0>; > }; > port@1 { > reg = <1>; > label = "lan1"; > phy-handle = <&phy1>; > leds = <&led0>; > }; > }; > > mdio { > compatible = "foo-mdio"; > #address-cells = <1>; > #size-cells = <0>; > > phy0: ethernet-phy@0 { > reg = <0>; > }; > phy1: ethernet-phy@1 { > reg = <1>; > }; > }; > }; > }; > > I am not the man to tell whether the leds = <&led0>; phandle should be on > the port or actually on the phy, it may even vary. You guys know the answer > to this. > > But certainly something like this resource phandle will be necessary to > assign the right LED to the right port or phy, I hope you were not going > to rely on strings and naming conventions? > > Yours, > Linus Walleij >
> I am a bit reluctant on the ambition to rely on configuration from sysfs > for the triggers, and I am also puzzled to how a certain trigger on a > certain LED is going to associate itself with, say, a certain port. Hi Linus There will need to be a device tree binding for the default trigger. That is what nearly all the rejected hacks so far have been about, write registers in the PHYs LEDs registers to put it into a specific mode. I don't see that part of the overall problem too problematic, apart from the old issue, is it describing configuration, not hardware. As to 'how a certain trigger on a certain LED is going to associate itself with, say, a certain port' is clearly a property of the hardware, when offloading is supported. I've not seen a switch you can arbitrarily assign LEDs to ports. The Marvell switches have the LED registers within the port registers, for example, two LEDs per port. > > I want to draw your attention to this recently merged patch series > from Hans de Goede: > https://lore.kernel.org/linux-leds/20230120114524.408368-1-hdegoede@redhat.com/ > > This adds the devm_led_get() API which works similar to getting > regulators, clocks, GPIOs or any other resources. Interesting. Thanks for pointing this out. But i don't think it is of interest in our use case, which is hardware offload. For a purely software controlled LED, where the LED could be anywhere, devm_led_get() makes sense. But in our case, the LED is in a well defined place, either the MAC or the PHY, where it has access to the RX and TX packets, link status etc. So we don't have the problem of finding it in an arbitrary location. There is also one additional problem we have, both for MAC and PHY LEDs. The trigger is ledtrig-netdev. All trigger state revolves around a netdev. A DSA port is not a netdev. A PHY is not a netdev. Each of these three things have there own life cycle. Often, a PHY does not know what netdev it is associated to until the interface is opened, ie. ip link set eth0 up. But once it is associated, phylib knows this information, so can return it, without any additional configuration data in DT. A DSA switch port can be created before the netdev associated to it is created. But again, the DSA core does know the mapping between a netdev and a port. Using devm_led_get() does not gain us anything. Andrew
On Thu, Mar 9, 2023 at 4:22 PM Andrew Lunn <andrew@lunn.ch> wrote: > As to 'how a certain trigger on a certain LED is going to associate > itself with, say, a certain port' is clearly a property of the > hardware, when offloading is supported. I've not seen a switch you can > arbitrarily assign LEDs to ports. The Marvell switches have the LED > registers within the port registers, for example, two LEDs per port. Aha so there is an implicit HW dependency between the port and the LED, that we just cannot see in the device tree. Okay, it makes sense. I think there will be a day when a switch without LED controller appears, but the system has a few LEDs for the ports connected to an arbitrary GPIO controller, and then we will need this. But we have not seen that yet :) Yours, Linus Walleij
On Fri, Mar 10, 2023 at 10:37:25AM +0100, Linus Walleij wrote: > On Thu, Mar 9, 2023 at 4:22 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > As to 'how a certain trigger on a certain LED is going to associate > > itself with, say, a certain port' is clearly a property of the > > hardware, when offloading is supported. I've not seen a switch you can > > arbitrarily assign LEDs to ports. The Marvell switches have the LED > > registers within the port registers, for example, two LEDs per port. > > Aha so there is an implicit HW dependency between the port and the > LED, that we just cannot see in the device tree. Okay, it makes sense. Well, i would say the dependency is in the device tree, in that the LEDs are described in the ports, not as a block of their own at a higher level within the switch. And in some switches, they might actually be a block of registers in there own space, rather than in the port registers. But i still expect there is a fixed mapping between LED and port. > I think there will be a day when a switch without LED controller appears, > but the system has a few LEDs for the ports connected to an > arbitrary GPIO controller, and then we will need this. But we have > not seen that yet :) The microchip sparx5 might be going in that direction. It has what looks like a reasonably generic sgpio controller: drivers/pinctrl/pinctrl-microchip-sgpio.c But this not just about switches. It is also plain NICs. And using ledtrig-netdev, you could make your keyboard LED blink based on network traffic etc. So yes, using a phandle to an LED could very well be useful in the future. Andrew
> > I think there will be a day when a switch without LED controller appears, > > but the system has a few LEDs for the ports connected to an > > arbitrary GPIO controller, and then we will need this. But we have > > not seen that yet :) > > The microchip sparx5 might be going in that direction. It has what > looks like a reasonably generic sgpio controller: > drivers/pinctrl/pinctrl-microchip-sgpio.c That gpio controller supports both, some kind of hardware controlled and pure software controlled mode. AFAIK the driver only supports software controlled mode (yet?). In any case, our board (arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt.dtsi) is broken in a way that we are forced to use the software controlled mode anyway. Therefore, there is already such a board ;) -michael