| Message ID | 20240122204650.344794-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] net: phy: broadcom: Add DT LED configuration support | expand |
On 1/22/24 12:45, Marek Vasut wrote: > The BCM54213E and similar PHYs have extensive LED configuration > capabilities -- the PHY has two LEDs, either of the two LEDs can > be configured to 1 of 16 functions (speed, TX, RX, activity, on, > off, quality, ... multi-color) used to drive single-color LED. > The multi-color mode is special, it provides 16 more sub-modes > used to drive multi-color LED. > > The current configuration -- both LEDs configured as multi-color, > with both LEDs multi-color sub-mode set to link activity indicator, > is not suitable for all systems in which this PHY is used. > > Attempt to implement a way to describe the LED configuration in DT. > > Use Documentation/devicetree/bindings/net/ethernet-phy.yaml leds {} > subnode of the PHY DT node, describe both LEDs present on this PHY > as single LEDs within the leds {} subnode. Each described LED is a > subnode of its own, the description uses standard LED subsystem > bindings from Documentation/devicetree/bindings/leds/common.yaml . > > The DT description of the LED configuration can look for example > like this: > > " > ethernet-phy@1 { > ... > leds { > #address-cells = <1>; > #size-cells = <0>; > > led@0 { > reg = <0>; > function = LED_FUNCTION_ACTIVITY; > }; > > led@1 { > reg = <1>; > function = LED_FUNCTION_SPEED_2; > }; > }; > }; > " > > Implement parsing code in the broadcom PHY driver to detemine desired > LED configuration from DT. In case the leds {} subnode is present, the > parser code iterates over its subnodes and for each led@N subnode it > parses the following properties: > > - reg - LED ID, either 0 or 1, used to identify the LED on the PHY > - function - LED single-color function (speed, TX, RX, multi-color...), > uses LED subsystem LED_FUNCTION_* string. The parser in > the driver maps this to register setting. > - function-enumerator - In case function is set to "multi-color", > the multi-color function number. The parser > in the driver uses this value directly for > the multi-color configuration register. > > Once the properties are parsed, the LED configuration registers of the > PHY are programmed. > > The current list of LED subsystem LED_FUNCTION_* does not cover the > entire list of possible single-color LED functions of this PHY, add > example extension for "link speed 1" and "link speed 2" setting into > the leds/common.h header file. > > The function-enumerator should probably not be a number, but maybe > some sort of macro specific to this PHY ? I would like to avoid new > broadcom PHY specific DT properties. The parsing should definitively not be in the driver code, the driver should only be providing a mapping between the function and enumerator and a method to set those. Christian has been working on Ethernet PHY LEDs for a while now, so he would be in a better position to comment about how to about that. The LED functions and register interface is actually quite stable across Ethernet PHYs from Broadcom so this code, however it looks like in the future should be moved to bcm-phy-lib.[ch]. If and where they are differences we can account for them in the library or by having each PHY driver entry provide a bcm54xx_* wrapper function that provides a table with the appropriate mapping. Thanks!
On 1/22/24 23:34, Florian Fainelli wrote: > On 1/22/24 12:45, Marek Vasut wrote: >> The BCM54213E and similar PHYs have extensive LED configuration >> capabilities -- the PHY has two LEDs, either of the two LEDs can >> be configured to 1 of 16 functions (speed, TX, RX, activity, on, >> off, quality, ... multi-color) used to drive single-color LED. >> The multi-color mode is special, it provides 16 more sub-modes >> used to drive multi-color LED. >> >> The current configuration -- both LEDs configured as multi-color, >> with both LEDs multi-color sub-mode set to link activity indicator, >> is not suitable for all systems in which this PHY is used. >> >> Attempt to implement a way to describe the LED configuration in DT. >> >> Use Documentation/devicetree/bindings/net/ethernet-phy.yaml leds {} >> subnode of the PHY DT node, describe both LEDs present on this PHY >> as single LEDs within the leds {} subnode. Each described LED is a >> subnode of its own, the description uses standard LED subsystem >> bindings from Documentation/devicetree/bindings/leds/common.yaml . >> >> The DT description of the LED configuration can look for example >> like this: >> >> " >> ethernet-phy@1 { >> ... >> leds { >> #address-cells = <1>; >> #size-cells = <0>; >> >> led@0 { >> reg = <0>; >> function = LED_FUNCTION_ACTIVITY; >> }; >> >> led@1 { >> reg = <1>; >> function = LED_FUNCTION_SPEED_2; >> }; >> }; >> }; >> " >> >> Implement parsing code in the broadcom PHY driver to detemine desired >> LED configuration from DT. In case the leds {} subnode is present, the >> parser code iterates over its subnodes and for each led@N subnode it >> parses the following properties: >> >> - reg - LED ID, either 0 or 1, used to identify the LED on the PHY >> - function - LED single-color function (speed, TX, RX, multi-color...), >> uses LED subsystem LED_FUNCTION_* string. The parser in >> the driver maps this to register setting. >> - function-enumerator - In case function is set to "multi-color", >> the multi-color function number. The parser >> in the driver uses this value directly for >> the multi-color configuration register. >> >> Once the properties are parsed, the LED configuration registers of the >> PHY are programmed. >> >> The current list of LED subsystem LED_FUNCTION_* does not cover the >> entire list of possible single-color LED functions of this PHY, add >> example extension for "link speed 1" and "link speed 2" setting into >> the leds/common.h header file. >> >> The function-enumerator should probably not be a number, but maybe >> some sort of macro specific to this PHY ? I would like to avoid new >> broadcom PHY specific DT properties. > > The parsing should definitively not be in the driver code, the driver > should only be providing a mapping between the function and enumerator > and a method to set those. Christian has been working on Ethernet PHY > LEDs for a while now, so he would be in a better position to comment > about how to about that. > > The LED functions and register interface is actually quite stable across > Ethernet PHYs from Broadcom so this code, however it looks like in the > future should be moved to bcm-phy-lib.[ch]. If and where they are > differences we can account for them in the library or by having each PHY > driver entry provide a bcm54xx_* wrapper function that provides a table > with the appropriate mapping. I very much agree. I also hope Rafal can chime in, I saw some openwrt LED patches floating around recently.
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 312a8bb35d780..9250cd45b0b24 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -407,20 +407,64 @@ static int bcm54xx_config_init(struct phy_device *phydev) /* For non-SFP setups, encode link speed into LED1 and LED3 pair * (green/amber). - * Also flash these two LEDs on activity. This means configuring - * them for MULTICOLOR and encoding link/activity into them. + * By default, flash these two LEDs on activity. This means + * configuring them for MULTICOLOR and encoding link/activity + * into them, but let user reconfigure this via DT. * Don't do this for devices on an SFP module, since some of these * use the LED outputs to control the SFP LOS signal, and changing * these settings will cause LOS to malfunction. */ if (!phy_on_sfp(phydev)) { - val = BCM54XX_SHD_LEDS1_LED1(BCM_LED_SRC_MULTICOLOR1) | - BCM54XX_SHD_LEDS1_LED3(BCM_LED_SRC_MULTICOLOR1); + struct device_node *np = phydev->mdio.dev.of_node; + struct device_node *leds, *led = NULL; + u8 mode[2] = { BCM_LED_SRC_MULTICOLOR1, BCM_LED_SRC_MULTICOLOR1 }; + u8 mcmode[2] = { BCM_LED_MULTICOLOR_LINK_ACT, BCM_LED_MULTICOLOR_LINK_ACT }; + const char *func; + u32 val, enumerator; + int ret; + + leds = of_find_node_by_name(np, "leds"); + if (leds) { + for_each_available_child_of_node(leds, led) { + ret = of_property_read_u32(led, "reg", &val); + if (ret < 0 || val >= 2) + continue; + + ret = of_property_read_string(led, "function", &func); + if (ret) + continue; + + if (!strcmp(func, LED_FUNCTION_TX)) + mode[val] = BCM_LED_SRC_XMITLED; + else if (!strcmp(func, LED_FUNCTION_RX)) + mode[val] = BCM_LED_SRC_RCVLED; + else if (!strcmp(func, LED_FUNCTION_ACTIVITY)) + mode[val] = BCM_LED_SRC_ACTIVITYLED; + else if (!strcmp(func, LED_FUNCTION_SPEED_1)) + mode[val] = BCM_LED_SRC_LINKSPD1; + else if (!strcmp(func, LED_FUNCTION_SPEED_2)) + mode[val] = BCM_LED_SRC_LINKSPD2; + /* Add other LED settings here */ + + ret = of_property_read_string(led, "function", &func); + if (ret) + continue; + + ret = of_property_read_u32(led, "function-enumerator", &enumerator); + if (ret || enumerator >= 16) + continue; + + mcmode[val] = enumerator; + } + } + + val = BCM54XX_SHD_LEDS1_LED1(mode[0]) | + BCM54XX_SHD_LEDS1_LED3(mode[1]); bcm_phy_write_shadow(phydev, BCM54XX_SHD_LEDS1, val); val = BCM_LED_MULTICOLOR_IN_PHASE | - BCM54XX_SHD_LEDS1_LED1(BCM_LED_MULTICOLOR_LINK_ACT) | - BCM54XX_SHD_LEDS1_LED3(BCM_LED_MULTICOLOR_LINK_ACT); + BCM54XX_SHD_LEDS1_LED1(mcmode[0]) | + BCM54XX_SHD_LEDS1_LED3(mcmode[1]); bcm_phy_write_exp(phydev, BCM_EXP_MULTICOLOR, val); } diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h index 9a0d33d027fff..83d09508841b6 100644 --- a/include/dt-bindings/leds/common.h +++ b/include/dt-bindings/leds/common.h @@ -102,5 +102,7 @@ #define LED_FUNCTION_WAN "wan" #define LED_FUNCTION_WLAN "wlan" #define LED_FUNCTION_WPS "wps" +#define LED_FUNCTION_SPEED_1 "speed-1" +#define LED_FUNCTION_SPEED_2 "speed-2" #endif /* __DT_BINDINGS_LEDS_H */
The BCM54213E and similar PHYs have extensive LED configuration capabilities -- the PHY has two LEDs, either of the two LEDs can be configured to 1 of 16 functions (speed, TX, RX, activity, on, off, quality, ... multi-color) used to drive single-color LED. The multi-color mode is special, it provides 16 more sub-modes used to drive multi-color LED. The current configuration -- both LEDs configured as multi-color, with both LEDs multi-color sub-mode set to link activity indicator, is not suitable for all systems in which this PHY is used. Attempt to implement a way to describe the LED configuration in DT. Use Documentation/devicetree/bindings/net/ethernet-phy.yaml leds {} subnode of the PHY DT node, describe both LEDs present on this PHY as single LEDs within the leds {} subnode. Each described LED is a subnode of its own, the description uses standard LED subsystem bindings from Documentation/devicetree/bindings/leds/common.yaml . The DT description of the LED configuration can look for example like this: " ethernet-phy@1 { ... leds { #address-cells = <1>; #size-cells = <0>; led@0 { reg = <0>; function = LED_FUNCTION_ACTIVITY; }; led@1 { reg = <1>; function = LED_FUNCTION_SPEED_2; }; }; }; " Implement parsing code in the broadcom PHY driver to detemine desired LED configuration from DT. In case the leds {} subnode is present, the parser code iterates over its subnodes and for each led@N subnode it parses the following properties: - reg - LED ID, either 0 or 1, used to identify the LED on the PHY - function - LED single-color function (speed, TX, RX, multi-color...), uses LED subsystem LED_FUNCTION_* string. The parser in the driver maps this to register setting. - function-enumerator - In case function is set to "multi-color", the multi-color function number. The parser in the driver uses this value directly for the multi-color configuration register. Once the properties are parsed, the LED configuration registers of the PHY are programmed. The current list of LED subsystem LED_FUNCTION_* does not cover the entire list of possible single-color LED functions of this PHY, add example extension for "link speed 1" and "link speed 2" setting into the leds/common.h header file. The function-enumerator should probably not be a number, but maybe some sort of macro specific to this PHY ? I would like to avoid new broadcom PHY specific DT properties. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: "David S. Miller" <davem@davemloft.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> Cc: Conor Dooley <conor+dt@kernel.org> Cc: Eric Dumazet <edumazet@google.com> Cc: Florian Fainelli <florian.fainelli@broadcom.com> Cc: Heiner Kallweit <hkallweit1@gmail.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> Cc: Lee Jones <lee@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: Rafał Miłecki <rafal@milecki.pl> Cc: Rob Herring <robh+dt@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: devicetree@vger.kernel.org Cc: linux-leds@vger.kernel.org Cc: netdev@vger.kernel.org --- drivers/net/phy/broadcom.c | 56 +++++++++++++++++++++++++++---- include/dt-bindings/leds/common.h | 2 ++ 2 files changed, 52 insertions(+), 6 deletions(-)