Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

realtek: Refactor rtl930x leds #11898

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+426 −122
Open

realtek: Refactor rtl930x leds #11898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
91e9d4b
realtek: Add some generic register helpers
oliv3r Jan 24, 2023
ea3eb0c
realtek: led: Honor the vendor default behavior
oliv3r Jan 25, 2023
725d6a4
realtek: Store number of LEDs in devicetree
oliv3r Jan 24, 2023
2325341
realtek: led: Rewrite RTL93xx LED init functions
oliv3r Jan 24, 2023
f92c01e
realtek: dts: Remove unused '0xffff' value
oliv3r Jan 25, 2023
8d05caa
realtek: led: Use LED defines for LED sets
oliv3r Jan 22, 2023
cfac8b4
realtek: dts: Drop led-set alias
oliv3r Jan 24, 2023
82cc7b2
realtek: dts: Prepare led sets for future expantion
oliv3r Feb 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
141 changes: 130 additions & 11 deletions target/linux/realtek/dts-5.15/rtl9302_zyxel_xgs1250-12.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <dt-bindings/input/input.h>
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/leds/common.h>
#include <dt-bindings/leds/leds-realtek.h>

/ {
compatible = "zyxel,xgs1250-12", "realtek,rtl838x-soc";
Expand Down Expand Up @@ -62,14 +63,78 @@
tx-disable-gpio = <&gpio0 15 GPIO_ACTIVE_HIGH>;
};

led_set: led_set@0 {
compatible = "realtek,rtl9300-leds";
active-low;
led_sets {
compatible = "realtek,rtl9302-leds", "realtek,rtl930x-leds";

led_set0 = <0x0000 0xffff 0x0a20 0x0b80>; // LED set 0: 1000Mbps, 10/100Mbps
led_set1 = <0x0a0b 0x0a28 0x0a82 0x0a0b>; // LED set 1: (10G, 5G, 2.5G) (2.5G, 1G)
// (5G, 10/100) (10G, 5G, 2.5G)
led_set2 = <0x0000 0xffff 0x0a20 0x0a01>; // LED set 2: 1000MBit, 10GBit
led_set0 = /bits/ 16 <
/* LED0 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_100M_LINK |
RTL93XX_LED_SET_10M_LINK
)
/* LED1 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_1000M_LINK
)
/* LED2 */
RTL93XX_LED_SET_NONE
/* LED3 */
RTL93XX_LED_SET_NONE
>;
led_set2 = /bits/ 16 <
/* LED0 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_10G_LINK |
RTL93XX_LED_SET_5G_LINK |
RTL93XX_LED_SET_2G5_LINK
)
/* LED1 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_5G_LINK |
RTL93XX_LED_SET_100M_LINK
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I'd recommend also adding the 10M here, by default, we get no light all, and officially 10M isn't supported, but of course it works just fine, and amber is 100/10 on GE ports ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Officially not supported by Zyxel? Because the LED controller hardware does appear to support it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10M? it works fine on my RTL8226 ports, but you have your aquantica MAC's there, so they may not support it.

I thus only recommended it, if you can confirm (with ethtool -s 10M on the host) if the port actually works. It does for me, and the leds indicate it all properly. Btw, the 10/100M support on those ports I added later in my patch series only, though I don't think it affects the aquantia? I can't test that, but if you start testing this stuff, take my WIP or DEV branches's tip to get all those patches and fixes.

zyxel ignores 10M, but having a 'black' led, when 10M is connected is super weird too, even if it works. And disabling it in openwrt/software feels wrong too. If the hardware fully works with it, why not?

)
/* LED2 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_2G5_LINK |
RTL93XX_LED_SET_1000M_LINK
)
/* LED3 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_10G_LINK |
RTL93XX_LED_SET_5G_LINK |
RTL93XX_LED_SET_2G5_LINK
)
>;
led_set3 = /bits/ 16 <
/* LED0 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_10G_LINK
)
/* LED1 */
(
RTL93XX_LED_SET_LINK_ACTIVITY |
RTL93XX_LED_SET_LINK_UP_SOLID |
RTL93XX_LED_SET_1000M_LINK
)
/* LED2 */
RTL93XX_LED_SET_NONE
/* LED3 */
RTL93XX_LED_SET_NONE
>;
};
};

Expand Down Expand Up @@ -224,77 +289,127 @@
phy-handle = <&phy0>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

led-set and led-num aren't upstreamable. This is also very RTL93xx-specific, and doesn't allow a user to reference an LED (for standard OpenWrt software control), which is needed on some devices (e.g. Cisco SG220). But good shouldn't the evil of perfect, and I'm willing to accept this as a stop-gap solution to improve device description.

My proposed led controller driver handled this by requiring every used LED to be defined individually. The driver would then determine how to configure the hardware in the most appropriate way. So in a sense what you're adding here is similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

led-set and led-num aren't upstreamable. This is also very RTL93xx-specific, and doesn't allow a user to reference an LED (for standard OpenWrt software control), which is needed on some devices (e.g. Cisco SG220). But good shouldn't the evil of perfect, and I'm willing to accept this as a stop-gap solution to improve device description.

Well we already had led-set, there's nothing new here, I only added led-num. And yes, they would have to be 'realtek,rtl93xx-led-set' at the least :p but I also do not intend to upstream either of those (hopefully). These are temporary, until we can go to a better driver. And then, it'll need to cover all our cases obviously.

My proposed led controller driver handled this by requiring every used LED to be defined individually. The driver would then determine how to configure the hardware in the most appropriate way. So in a sense what you're adding here is similar.

Yeah, I saw that, but then, I don't like that either. While this is certainly the wrong place to discuss future design :p why I didn't like it, is because that allows you to have an arbitrary led setup, which sounds perfect, except you are restricted to the 4 (for rtl93xx), 3 afaik for rtl839x? So now you have some very deep complexity in figuring this out. Ideally, you'd refer a led here via a phandle and which 'mode', but the mode bit doesn't exist yet (which you struggled with too). But again, wrong time/place. And something we should deff. have a brainstorming session over.

};
port@1 {
reg = <1>;
label = "lan2";
phy-handle = <&phy1>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
};
port@2 {
reg = <2>;
label = "lan3";
phy-handle = <&phy2>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
};
port@3 {
reg = <3>;
label = "lan4";
phy-handle = <&phy3>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
};
port@4 {
reg = <4>;
label = "lan5";
phy-handle = <&phy4>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
};
port@5 {
reg = <5>;
label = "lan6";
phy-handle = <&phy5>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
};
port@6 {
reg = <6>;
label = "lan7";
phy-handle = <&phy6>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
};
port@7 {
reg = <7>;
label = "lan8";
phy-handle = <&phy7>;
phy-mode = "xgmii";
led-set = <0>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber */
RTL93XX_LED_NUM1 /* Lime */
)>;
};

port@24 {
reg = <24>;
label = "lan9";
phy-mode = "usxgmii";
phy-handle = <&phy24>;
led-set = <1>;
led-set = <2>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber/Lime | RGB */
RTL93XX_LED_NUM1 | /* Amber | Red */
RTL93XX_LED_NUM2 | /* Lime | Green */
RTL93XX_LED_NUM3 /* off | Blue */
)>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still can't believe they wired the LEDs this way... I think it makes it pretty much impossible to allow all five LED components to be configured to userspace control (because one pin is essentially a mux now).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

five? thought it was 4? anyway, yeah, and that's exactly my point above, it's not so black/white trivial. So (ignoring the warts and uglyness what this patch propses) it is atleast 'somewhat' logical, but even that falls on its ass (when you look at the led configuration, 1G_ACTIVE etc) where you have to keep in mind that it's a mux you are setting up ...

It's just very nasty, not just how they hooked up the leds, more how this is what the hardware allows (an in documentation encourages to a point).

};
port@25 {
reg = <25>;
label = "lan10";
phy-mode = "usxgmii";
phy-handle = <&phy25>;
led-set = <1>;
led-set = <2>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber/Lime | RGB */
RTL93XX_LED_NUM1 | /* Amber | Red */
RTL93XX_LED_NUM2 | /* Lime | Green */
RTL93XX_LED_NUM3 /* off | Blue */
)>;
};
port@26 {
reg = <26>;
label = "lan11";
phy-mode = "usxgmii";
phy-handle = <&phy26>;
led-set = <1>;
led-set = <2>;
led-num = <(
RTL93XX_LED_NUM0 | /* Amber/Lime | RGB */
RTL93XX_LED_NUM1 | /* Amber | Red */
RTL93XX_LED_NUM2 | /* Lime | Green */
RTL93XX_LED_NUM3 /* off | Blue */
)>;
};

port@27 {
Expand All @@ -303,7 +418,11 @@
phy-mode = "10gbase-r";
phy-handle = <&phy27>;
sfp = <&sfp0>;
led-set = <2>;
led-set = <3>;
led-num = <(
RTL93XX_LED_NUM0 | /* Blue */
RTL93XX_LED_NUM1 /* Lime */
)>;

fixed-link {
speed = <10000>;
Expand Down
1 change: 1 addition & 0 deletions target/linux/realtek/files-5.15/arch/mips/include/asm/mach-rtl838x/mach-rtl83xx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#define _MACH_RTL838X_H_

#include <asm/types.h>

oliv3r marked this conversation as resolved.
Show resolved Hide resolved
/*
* Register access macros
*/
Expand Down
3 changes: 3 additions & 0 deletions target/linux/realtek/files-5.15/drivers/net/dsa/rtl83xx/common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,9 @@ static int __init rtl83xx_mdio_probe(struct rtl838x_switch_priv *priv)
if (interface == PHY_INTERFACE_MODE_10GBASER)
priv->ports[pn].is10G = true;

if (of_property_read_u32(dn, "led-num", &priv->ports[pn].led_num))
priv->ports[pn].led_num = 0;

if (of_property_read_u32(dn, "led-set", &led_set))
led_set = 0;
priv->ports[pn].led_set = led_set;
Expand Down
19 changes: 2 additions & 17 deletions target/linux/realtek/files-5.15/drivers/net/dsa/rtl83xx/rtl838x.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#ifndef _RTL838X_H
#define _RTL838X_H

#include <linux/types.h>
#include <net/dsa.h>

/* Register definition */
Expand Down Expand Up @@ -553,23 +554,6 @@ typedef enum {
#define RTL930X_L3_HW_LU_CTRL (0xACC0)
#define RTL930X_L3_IP_ROUTE_CTRL 0xab44

/* Port LED Control */
#define RTL930X_LED_PORT_NUM_CTRL(p) (0xCC04 + (((p >> 4) << 2)))
#define RTL930X_LED_SET0_0_CTRL (0xCC28)
#define RTL930X_LED_PORT_COPR_SET_SEL_CTRL(p) (0xCC2C + (((p >> 4) << 2)))
#define RTL930X_LED_PORT_FIB_SET_SEL_CTRL(p) (0xCC34 + (((p >> 4) << 2)))
#define RTL930X_LED_PORT_COPR_MASK_CTRL (0xCC3C)
#define RTL930X_LED_PORT_FIB_MASK_CTRL (0xCC40)
#define RTL930X_LED_PORT_COMBO_MASK_CTRL (0xCC44)

#define RTL931X_LED_PORT_NUM_CTRL(p) (0x0604 + (((p >> 4) << 2)))
#define RTL931X_LED_SET0_0_CTRL (0x0630)
#define RTL931X_LED_PORT_COPR_SET_SEL_CTRL(p) (0x0634 + (((p >> 4) << 2)))
#define RTL931X_LED_PORT_FIB_SET_SEL_CTRL(p) (0x0644 + (((p >> 4) << 2)))
#define RTL931X_LED_PORT_COPR_MASK_CTRL (0x0654)
#define RTL931X_LED_PORT_FIB_MASK_CTRL (0x065c)
#define RTL931X_LED_PORT_COMBO_MASK_CTRL (0x0664)

#define MAX_VLANS 4096
#define MAX_LAGS 16
#define MAX_PRIOS 8
Expand Down Expand Up @@ -622,6 +606,7 @@ struct rtl838x_port {
bool is2G5;
int sds_num;
int led_set;
u32 led_num;
const struct dsa_port *dp;
};

Expand Down
23 changes: 23 additions & 0 deletions target/linux/realtek/files-5.15/drivers/net/dsa/rtl83xx/rtl83xx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,32 @@
#ifndef _NET_DSA_RTL83XX_H
#define _NET_DSA_RTL83XX_H

#include <linux/bitops.h>
#include <net/dsa.h>
#include "rtl838x.h"

/* The size of a register for the otto platform */
#define REALTEK_REG_SIZE BITS_PER_TYPE(u32)

/* Register port offset. e.g. 16 config bits per port -> 2 ports per reg */
#define REALTEK_REG_PORT_OFFSET(port, bits_per_port, offset) \
(((port) / (REALTEK_REG_SIZE / (bits_per_port))) * offset)

/* Register port index. e.g. 3rd port is the first set of bits of the reg */
#define REALTEK_REG_PORT_INDEX(port, bits_per_port) \
(((port) % (REALTEK_REG_SIZE / (bits_per_port))) * (bits_per_port))

/* Size of array to hold all port config bits. E.g. 4 bits per port needs 2 elements (if port num < 32) */
#define REALTEK_PORT_ARRAY_SIZE(port, bits_per_port) \
((((port) / REALTEK_REG_SIZE) + 1) * bits_per_port)

/* Index to element in array holding config bits. E.g. port 9 with 4 bits per port, yields index 1 */
#define REALTEK_PORT_ARRAY_INDEX(port, bits_per_port) \
(((port) * (bits_per_port)) / REALTEK_REG_SIZE)

/* Get (first) port matching an index. E.g. with 2 bits per port and array index 1 -> port 16 */
#define REALTEK_INDEX_ARRAY_PORT(index, bits_per_port) \
((index) * (REALTEK_REG_SIZE / (bits_per_port)))

#define RTL8380_VERSION_A 'A'
#define RTL8390_VERSION_A 'A'
Expand Down