| Message ID | 20230319191814.22067-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | net: Add basic LED support for switch/phy | expand |
On Sun, Mar 19, 2023 at 08:18:00PM +0100, Christian Marangi wrote: > Move qca8k_port_to_phy() to qca8k header as it's useful for future > reference in Switch LEDs module since the same logic is applied to get > the right index of the switch port. > Make it inline as it's simple function that just decrease the port. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > --- Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>
On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote: > Document support for LEDs node in ethernet-controller. > Ethernet Controller may support different LEDs that can be configured > for different operation like blinking on traffic event or port link. > > Also add some Documentation to describe the difference of these nodes > compared to PHY LEDs, since ethernet-controller LEDs are controllable > by the ethernet controller regs and the possible intergated PHY doesn't > have control on them. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../bindings/net/ethernet-controller.yaml | 21 +++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml > index 00be387984ac..a93673592314 100644 > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml > @@ -222,6 +222,27 @@ properties: > required: > - speed > > + leds: > + type: object > + description: > + Describes the LEDs associated by Ethernet Controller. > + These LEDs are not integrated in the PHY and PHY doesn't have any > + control on them. Ethernet Controller regs are used to control > + these defined LEDs. > + > + properties: > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + patternProperties: > + '^led(@[a-f0-9]+)?$': > + $ref: /schemas/leds/common.yaml# Are specific ethernet controllers allowed to add their own properties in led nodes? If so, this doesn't work. As-is, this allows any other properties. You need 'unevaluatedProperties: false' here to prevent that. But then no one can add properties. If you want to support that, then you need this to be a separate schema that devices can optionally include if they don't extend the properties, and then devices that extend the binding would essentially have the above with: $ref: /schemas/leds/common.yaml# unevaluatedProperties: false properties: a-custom-device-prop: ... If you wanted to define both common ethernet LED properties and device specific properties, then you'd need to replace leds/common.yaml above with the ethernet one. This is all the same reasons the DSA/switch stuff and graph bindings are structured the way they are. Rob
On Sun, Mar 19, 2023 at 08:18:10PM +0100, Christian Marangi 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>; Once 'unevaluatedProperties' is properly implemented in the schema, this will be a warning. You didn't define 'reg' in the schema. > + 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 { > -- > 2.39.2 >
> > Are specific ethernet controllers allowed to add their own properties in > > led nodes? If so, this doesn't work. As-is, this allows any other > > properties. You need 'unevaluatedProperties: false' here to prevent > > that. But then no one can add properties. If you want to support that, > > then you need this to be a separate schema that devices can optionally > > include if they don't extend the properties, and then devices that > > extend the binding would essentially have the above with: > > > > $ref: /schemas/leds/common.yaml# > > unevaluatedProperties: false > > properties: > > a-custom-device-prop: ... > > > > > > If you wanted to define both common ethernet LED properties and > > device specific properties, then you'd need to replace leds/common.yaml > > above with the ethernet one. > > > > This is all the same reasons the DSA/switch stuff and graph bindings are > > structured the way they are. > > > > Hi Rob, thanks for the review/questions. > > The idea of all of this is to keep leds node as standard as possible. > It was asked to add unevaluatedProperties: False but I didn't understood > it was needed also for the led nodes. > > leds/common.yaml have additionalProperties set to true but I guess that > is not OK for the final schema and we need something more specific. > > Looking at the common.yaml schema reg binding is missing so an > additional schema is needed. > > Reg is needed for ethernet LEDs and PHY but I think we should also permit > to skip that if the device actually have just one LED. (if this wouldn't > complicate the implementation. Maybe some hints from Andrew about this > decision?) I would make reg mandatory. We should not encourage additional properties, but i also think we cannot block it. The problem we have is that there is absolutely no standardisation here. Vendors are free to do whatever they want, and they do. So i would not be too surprised if some vendor properties are needed eventually. Andrew
Hi! > Add LEDs blink_set() support to qca8k Switch Family. > These LEDs support hw accellerated blinking at a fixed rate > of 4Hz. > > Reject any other value since not supported by the LEDs switch. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Acked-by: Pavel Machek <pavel@ucw.cz>
On Sun 2023-03-19 20:18:06, Christian Marangi wrote: > From: Andrew Lunn <andrew@lunn.ch> > > Add a brightness function, so the LEDs can be controlled from > software using the standard Linux LED infrastructure. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Acked-by: Pavel Machek <pavel@ucw.cz>
On Sun 2023-03-19 20:18:12, Christian Marangi wrote: > Add Switch LED for each port for MikroTik RB3011UiAS-RM. > > MikroTik RB3011UiAS-RM is a 10 port device with 2 qca8337 switch chips > connected. > > It was discovered that in the hardware design all 3 Switch LED trace of > the related port is connected to the same LED. This was discovered by > setting to 'always on' the related led in the switch regs and noticing > that all 3 LED for the specific port (for example for port 1) cause the > connected LED for port 1 to turn on. As an extra test we tried enabling > 2 different LED for the port resulting in the LED turned off only if > every led in the reg was off. > > Aside from this funny and strange hardware implementation, the device > itself have one green LED for each port, resulting in 10 green LED one > for each of the 10 supported port. > > Cc: Jonathan McDowell <noodles@earth.li> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Reviewed-by: Pavel Machek <pavel@ucw.cz>
Hi! > 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> > @@ -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"; > + }; > + }; > }; > How will this end up looking in sysfs? Should documentation be added to Documentation/leds/leds-blinkm.rst ? BR, Pavel
On Thu, Mar 23, 2023 at 01:04:53PM +0100, Pavel Machek wrote: > Hi! > > > 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> > > > @@ -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"; > > + }; > > + }; > > }; > > > > How will this end up looking in sysfs? Hi Pavel It is just a plain boring LED, so it will look like all other LEDs. There is nothing special here. > Should documentation be added to Documentation/leds/leds-blinkm.rst > ? This has nothing to do with blinkm, which appears to be an i2c LED driver. Andrew
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. > > > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > > > @@ -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"; > > > + }; > > > + }; > > > }; > > > > > > > How will this end up looking in sysfs? > > Hi Pavel > > It is just a plain boring LED, so it will look like all other LEDs. > There is nothing special here. Well, AFAICT it will end up as /sys/class/leds/WAN, which is really not what we want. (Plus the netdev trigger should be tested; we'll need some kind of link to the ethernet device if we want this to work on multi-ethernet systems). > > Should documentation be added to Documentation/leds/leds-blinkm.rst > > ? > > This has nothing to do with blinkm, which appears to be an i2c LED > driver. Sorry, I meant Should documentation be added to Documentation/leds/well-known-leds.txt ? Best regards, Pavel
> > Hi Pavel > > > > It is just a plain boring LED, so it will look like all other LEDs. > > There is nothing special here. > > Well, AFAICT it will end up as /sys/class/leds/WAN, which is really > not what we want. Why not? It is just a plain boring LED. It can be used for anything, heartbeat, panic SOS in Morse code, shift lock, disk activity. Any of the triggers can be applied to it. 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/ > (Plus the netdev trigger should be tested; we'll > need some kind of link to the ethernet device if we want this to work > on multi-ethernet systems). Since this is a plain boring LED, it could actually blink for any netdev. When we get to offloading blinking to hardware, then things change, we need to check the netdev which is configured in the ledtrig-netdev is the same one the PHY is associated to. But i have a patchset for that which will appear later. > Should documentation be added to Documentation/leds/well-known-leds.txt ? Saying what. That there might be LEDs in your RJ45 connector, which can be used for anything which is supported by an Linux LED trigger? Andrew
On Wed, Mar 22, 2023 at 12:39:48AM +0100, Christian Marangi wrote: > On Wed, Mar 22, 2023 at 12:23:59AM +0100, Andrew Lunn wrote: > > > > Are specific ethernet controllers allowed to add their own properties in > > > > led nodes? If so, this doesn't work. As-is, this allows any other > > > > properties. You need 'unevaluatedProperties: false' here to prevent > > > > that. But then no one can add properties. If you want to support that, > > > > then you need this to be a separate schema that devices can optionally > > > > include if they don't extend the properties, and then devices that > > > > extend the binding would essentially have the above with: > > > > > > > > $ref: /schemas/leds/common.yaml# > > > > unevaluatedProperties: false > > > > properties: > > > > a-custom-device-prop: ... > > > > > > > > > > > > If you wanted to define both common ethernet LED properties and > > > > device specific properties, then you'd need to replace leds/common.yaml > > > > above with the ethernet one. > > > > > > > > This is all the same reasons the DSA/switch stuff and graph bindings are > > > > structured the way they are. > > > > > > > > > > Hi Rob, thanks for the review/questions. > > > > > > The idea of all of this is to keep leds node as standard as possible. > > > It was asked to add unevaluatedProperties: False but I didn't understood > > > it was needed also for the led nodes. > > > > > > leds/common.yaml have additionalProperties set to true but I guess that > > > is not OK for the final schema and we need something more specific. > > > > > > Looking at the common.yaml schema reg binding is missing so an > > > additional schema is needed. > > > > > > Reg is needed for ethernet LEDs and PHY but I think we should also permit > > > to skip that if the device actually have just one LED. (if this wouldn't > > > complicate the implementation. Maybe some hints from Andrew about this > > > decision?) > > > > I would make reg mandatory. > > > > Ok will add a new schema and change the regex. > > > We should not encourage additional properties, but i also think we > > cannot block it. > > > > The problem we have is that there is absolutely no standardisation > > here. Vendors are free to do whatever they want, and they do. So i > > would not be too surprised if some vendor properties are needed > > eventually. > > > > Think that will come later with defining a more specific schema. But I > honestly think most of the special implementation will be handled to the > driver internally and not with special binding in DT. Then encourage no additional properties by letting whomever wants to add them to restructure the schema. ;) Rob
On Tue, Mar 21, 2023 at 11:54:46PM +0100, Christian Marangi wrote: > On Tue, Mar 21, 2023 at 04:19:53PM -0500, Rob Herring wrote: > > On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote: > > > Document support for LEDs node in ethernet-controller. > > > Ethernet Controller may support different LEDs that can be configured > > > for different operation like blinking on traffic event or port link. > > > > > > Also add some Documentation to describe the difference of these nodes > > > compared to PHY LEDs, since ethernet-controller LEDs are controllable > > > by the ethernet controller regs and the possible intergated PHY doesn't > > > have control on them. > > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > --- > > > .../bindings/net/ethernet-controller.yaml | 21 +++++++++++++++++++ > > > 1 file changed, 21 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml > > > index 00be387984ac..a93673592314 100644 > > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml > > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml > > > @@ -222,6 +222,27 @@ properties: > > > required: > > > - speed > > > > > > + leds: > > > + type: object > > > + description: > > > + Describes the LEDs associated by Ethernet Controller. > > > + These LEDs are not integrated in the PHY and PHY doesn't have any > > > + control on them. Ethernet Controller regs are used to control > > > + these defined LEDs. > > > + > > > + properties: > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > + > > > + patternProperties: > > > + '^led(@[a-f0-9]+)?$': > > > + $ref: /schemas/leds/common.yaml# > > > > Are specific ethernet controllers allowed to add their own properties in > > led nodes? If so, this doesn't work. As-is, this allows any other > > properties. You need 'unevaluatedProperties: false' here to prevent > > that. But then no one can add properties. If you want to support that, > > then you need this to be a separate schema that devices can optionally > > include if they don't extend the properties, and then devices that > > extend the binding would essentially have the above with: > > > > $ref: /schemas/leds/common.yaml# > > unevaluatedProperties: false > > properties: > > a-custom-device-prop: ... > > > > > > If you wanted to define both common ethernet LED properties and > > device specific properties, then you'd need to replace leds/common.yaml > > above with the ethernet one. > > > > This is all the same reasons the DSA/switch stuff and graph bindings are > > structured the way they are. > > > > Hi Rob, thanks for the review/questions. > > The idea of all of this is to keep leds node as standard as possible. > It was asked to add unevaluatedProperties: False but I didn't understood > it was needed also for the led nodes. > > leds/common.yaml have additionalProperties set to true but I guess that > is not OK for the final schema and we need something more specific. Yes, every node needs a schema with all possible properties and then 'unevaluatedProperties: false' to not allow any other properties. > Looking at the common.yaml schema reg binding is missing so an > additional schema is needed. > > Reg is needed for ethernet LEDs and PHY but I think we should also permit > to skip that if the device actually have just one LED. (if this wouldn't > complicate the implementation. Maybe some hints from Andrew about this > decision?) > > If we decide that reg is a must, if I understood it correctly we should > create something like leds-ethernet.yaml that would reference common and > add reg binding? Is it correct? This schema should be laded in leds > directory and not in the net/ethernet. You need 'reg' in properties, but whether it is required or not just depends on putting it in 'required'. I don't have a strong opinion on that, but generally it's only use 'reg' when there's more than 1. Rob
This is a continue of [1]. It was decided to take a more gradual approach to implement LEDs support for switch and phy starting with basic support and then implementing the hw control part when we have all the prereq done. This series implements only the brightness_set() and blink_set() ops. An example of switch implementation is done with qca8k. For PHY a more generic approach is used with implementing the LED support in PHY core and with the user (in this case marvell) adding all the required functions. Currently we set the default-state as "keep" to not change the default configuration of the declared LEDs since almost every switch have a default configuration. [1] https://lore.kernel.org/lkml/20230216013230.22978-1-ansuelsmth@gmail.com/ Changes in new series v5: - Rebase everything on top of net-next/main - Add more info on LED probe fail for qca8k - Drop some additional raw number and move to define in qca8k header - Add additional info on LED mapping on qca8k regs - Checks port number in qca8k switch port parse - Changes in Andrew patch: - Add additional patch for stubs when CLASS_LED disabled - Drop CLASS_LED dependency for PHYLIB (to fix kbot errors reported) Changes in new series v4: - Changes in Andrew patch: - net: phy: Add a binding for PHY LEDs: - Rename phy_led: led_list to list - Rename phy_device: led_list to leds - Remove phy_leds_remove() since devm_ should do what is needed - Fixup documentation for struct phy_led - Fail probe on LED errors - net: phy: phy_device: Call into the PHY driver to set LED brightness - Moved phy_led::phydev from previous patch to here since it is first used here. - net: phy: marvell: Implement led_blink_set() - Use int instead of unsigned - net: phy: marvell: Add software control of the LEDs - Use int instead of unsigned - Add depends on LED_CLASS for qca8k Kconfig - Fix Makefile for qca8k as suggested - Move qca8k_setup_led_ctrl to separate header - Move Documentation from dsa-port to ethernet-controller - Drop trailing . from Andrew patch fro consistency Changes in new series v3: - Move QCA8K_LEDS Kconfig option from tristate to bool - Use new helper led_init_default_state_get for default-state in qca8k - Drop cled_qca8k_brightness_get() as there isn't a good way to describe the mode the led is currently in - Rework qca8k_led_brightness_get() to return true only when LED is set to always ON Changes in new series v2: - Add LEDs node for rb3011 - Fix rb3011 switch node unevaluated properties while running make dtbs_check - Fix a copypaste error in qca8k-leds.c for port 4 required shift - Drop phy-handle usage for qca8k and use qca8k_port_to_phy() - Add review tag from Andrew - Add Christian Marangi SOB in each Andrew patch - Add extra description for dsa-port stressing that PHY have no access and LED are controlled by the related MAC - Add missing additionalProperties for dsa-port.yaml and ethernet-phy.yaml Changes from the old v8 series: - Drop linux,default-trigger set to netdev. - Dropped every hw control related patch and implement only blink_set and brightness_set - Add default-state to "keep" for each LED node example Andrew Lunn (7): leds: Provide stubs for when CLASS_LED is disabled net: phy: Add a binding for PHY LEDs net: phy: phy_device: Call into the PHY driver to set LED brightness net: phy: marvell: Add software control of the LEDs net: phy: phy_device: Call into the PHY driver to set LED blinking net: phy: marvell: Implement led_blink_set() arm: mvebu: dt: Add PHY LED support for 370-rd WAN port Christian Marangi (8): net: dsa: qca8k: move qca8k_port_to_phy() to header net: dsa: qca8k: add LEDs basic support net: dsa: qca8k: add LEDs blink_set() support dt-bindings: net: ethernet-controller: Document support for LEDs node dt-bindings: net: dsa: qca8k: add LEDs definition example arm: qcom: dt: Drop unevaluated properties in switch nodes for rb3011 arm: qcom: dt: Add Switch LED for each port for rb3011 dt-bindings: net: phy: Document support for LEDs node .../devicetree/bindings/net/dsa/qca8k.yaml | 24 ++ .../bindings/net/ethernet-controller.yaml | 21 ++ .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++ arch/arm/boot/dts/armada-370-rd.dts | 14 + arch/arm/boot/dts/qcom-ipq8064-rb3011.dts | 124 +++++++- drivers/net/dsa/qca/Kconfig | 8 + drivers/net/dsa/qca/Makefile | 3 + drivers/net/dsa/qca/qca8k-8xxx.c | 20 +- drivers/net/dsa/qca/qca8k-leds.c | 270 ++++++++++++++++++ drivers/net/dsa/qca/qca8k.h | 74 +++++ drivers/net/dsa/qca/qca8k_leds.h | 16 ++ drivers/net/phy/marvell.c | 81 +++++- drivers/net/phy/phy_device.c | 102 +++++++ include/linux/leds.h | 18 ++ include/linux/phy.h | 35 +++ 15 files changed, 817 insertions(+), 24 deletions(-) create mode 100644 drivers/net/dsa/qca/qca8k-leds.c create mode 100644 drivers/net/dsa/qca/qca8k_leds.h