| Message ID | 20230327141031.11904-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | net: Add basic LED support for switch/phy | expand |
On Mon, 27 Mar 2023 16:10:15 +0200 Christian Marangi wrote: > 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. Please run ./scripts/kernel-doc -none on the headers, the new ops added in patches 6 and 8 need to be kdoc'ed.
On Mon 2023-03-27 16:10:31, 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. > @@ -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. Best regards, Pavel
On Tue, Mar 28, 2023 at 10:31:16AM +0200, Pavel Machek wrote: > On Mon 2023-03-27 16:10:31, 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. > > > @@ -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? 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. > > > > > @@ -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
> 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
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
> 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
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 v6: - Add leds-ethernet.yaml to document reg in led node - Update ethernet-controller and ethernet-phy to follow new leds-ethernet schema - Fix comments in qca8k-leds.c (at least -> at most) (wrong GENMASK for led phy 0 and 4) - Add review and ack tag from Pavel Machek - Changes in Andrew patch: - leds: Provide stubs for when CLASS_LED & NEW_LEDS are disabled - Change LED_CLASS to NEW_LEDS for led_init_default_state_get() - net: phy: Add a binding for PHY LEDs - Add dependency on LED_CLASS - Drop review tag from Michal Kubiak (patch modified) 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 & NEW_LEDS are 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 (9): 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: leds: Document support for generic ethernet LEDs dt-bindings: net: ethernet-controller: Document support for LEDs node dt-bindings: net: dsa: qca8k: add LEDs definition example ARM: dts: qcom: ipq8064-rb3011: Drop unevaluated properties in switch nodes ARM: dts: qcom: ipq8064-rb3011: Add Switch LED for each port dt-bindings: net: phy: Document support for LEDs node .../bindings/leds/leds-ethernet.yaml | 76 +++++ .../devicetree/bindings/net/dsa/qca8k.yaml | 24 ++ .../bindings/net/ethernet-controller.yaml | 10 + .../devicetree/bindings/net/ethernet-phy.yaml | 19 ++ 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/Kconfig | 1 + drivers/net/phy/marvell.c | 81 +++++- drivers/net/phy/phy_device.c | 102 +++++++ include/linux/leds.h | 18 ++ include/linux/phy.h | 35 +++ 17 files changed, 871 insertions(+), 24 deletions(-) create mode 100644 Documentation/devicetree/bindings/leds/leds-ethernet.yaml create mode 100644 drivers/net/dsa/qca/qca8k-leds.c create mode 100644 drivers/net/dsa/qca/qca8k_leds.h