| Message ID | e132a32129e1c9c7a7de7c216f6e222340bb0c48.1654587762.git.sander@svanheule.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Sander Vanheule |
| Headers | show |
| Series | realtek: remove sys-led from setup.c | expand |
Hi,
has anyone tested that??? This does not make sense at all, there is no LED disable
in the LED_GLB_CTRL register. Instead one needs to use RTL9310_MAC_L2_GLOBAL_CTRL2
The following works nicely on the XS1930 and Edgecore:
pinmux: pinmux@1b001358 {
compatible = "pinctrl-single";
reg = <0x1b001358 0x4>;
pinctrl-single,bit-per-mux;
pinctrl-single,register-width = <32>;
pinctrl-single,function-mask = <0x1>;
#pinctrl-cells = <2>;
/* Enable GPIO6 and GPIO7, possibly unknown others */
pinmux_disable_jtag: disable_jtag {
pinctrl-single,bits = <0x0 0x0 0x8000>;
};
pinmux_disable_sys_led: disable_sys_led {
pinctrl-single,bits = <0x0 0x0 0x100>;
};
};
Cheers,
Birger
On 07.06.22 09:50, Sander Vanheule wrote:
> Like for RTL838x devices, add a pinctrl-single node to manage the
> sys-led/gpio0 mux, and allow using the pin as GPIO.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
> target/linux/realtek/dts-5.10/rtl931x.dtsi | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/target/linux/realtek/dts-5.10/rtl931x.dtsi b/target/linux/realtek/dts-5.10/rtl931x.dtsi
> index 29aee1f7b268..f4e2fd248f7e 100644
> --- a/target/linux/realtek/dts-5.10/rtl931x.dtsi
> +++ b/target/linux/realtek/dts-5.10/rtl931x.dtsi
> @@ -155,6 +155,20 @@
> };
> };
>
> + pinmux_led: pinmux@1b000600 {
> + compatible = "pinctrl-single";
> + reg = <0x1b000600 0x4>;
> +
> + pinctrl-single,bit-per-mux;
> + pinctrl-single,register-width = <32>;
> + pinctrl-single,function-mask = <0x1>;
> + #pinctrl-cells = <2>;
> +
> + /* enable GPIO 0 */
> + pinmux_disable_sys_led: disable_sys_led {
> + pinctrl-single,bits = <0x0 0x3000 0x3000>;
> + };
> + };
>
> ethernet0: ethernet@1b00a300 {
> status = "okay";
On Tue, 2022-06-07 at 10:15 +0200, Birger Koblitz wrote: > Hi, > > has anyone tested that??? I don't have any 931x hardware, but it is based on what you put into setup.c. > This does not make sense at all, there is no LED disable > in the LED_GLB_CTRL register. Instead one needs to use RTL9310_MAC_L2_GLOBAL_CTRL2 > > The following works nicely on the XS1930 and Edgecore: > > pinmux: pinmux@1b001358 { > compatible = "pinctrl-single"; > reg = <0x1b001358 0x4>; > > pinctrl-single,bit-per-mux; > pinctrl-single,register-width = <32>; > pinctrl-single,function-mask = <0x1>; > #pinctrl-cells = <2>; > > /* Enable GPIO6 and GPIO7, possibly unknown others */ > pinmux_disable_jtag: disable_jtag { > pinctrl-single,bits = <0x0 0x0 0x8000>; > }; > > pinmux_disable_sys_led: disable_sys_led { > pinctrl-single,bits = <0x0 0x0 0x100>; > }; Thanks, I wasn't aware of these fields. Will update in v2. > > > > + pinmux_led: pinmux@1b000600 { > > + compatible = "pinctrl-single"; > > + reg = <0x1b000600 0x4>; > > + > > + pinctrl-single,bit-per-mux; > > + pinctrl-single,register-width = <32>; > > + pinctrl-single,function-mask = <0x1>; > > + #pinctrl-cells = <2>; > > + > > + /* enable GPIO 0 */ > > + pinmux_disable_sys_led: disable_sys_led { > > + pinctrl-single,bits = <0x0 0x3000 0x3000>; > > + }; This field, I assume, controls the toggling rate of the system led then. Would explain why it has two bits and is called SYS_LED_MODE. Best, Sander
Hi, On 07.06.22 11:04, Sander Vanheule wrote: > On Tue, 2022-06-07 at 10:15 +0200, Birger Koblitz wrote: >> Hi, >> >> has anyone tested that??? > > I don't have any 931x hardware, but it is based on what you put into setup.c. What is in the setup.c makes the System LED solid on. It is the same for all the architectures: static void __init rtl838x_setup(void) { /* Setup System LED. Bit 15 then allows to toggle it */ sw_w32_mask(0, 3 << 16, RTL838X_LED_GLB_CTRL); } static void __init rtl839x_setup(void) { /* Setup System LED. Bit 14 of RTL839X_LED_GLB_CTRL then allows to toggle it */ sw_w32_mask(0, 3 << 15, RTL839X_LED_GLB_CTRL); } static void __init rtl931x_setup(void) { sw_w32_mask(0, 3 << 12, RTL931X_LED_GLB_CTRL); } You are not using that to disable the system led on the 838x/839x, so why would it do something different on the RTL931x? Note that at that time it was not know how to toggle the LED, because that is somewhere else. > >> This does not make sense at all, there is no LED disable >> in the LED_GLB_CTRL register. Instead one needs to use RTL9310_MAC_L2_GLOBAL_CTRL2 >> >> The following works nicely on the XS1930 and Edgecore: >> >> pinmux: pinmux@1b001358 { >> compatible = "pinctrl-single"; >> reg = <0x1b001358 0x4>; >> >> pinctrl-single,bit-per-mux; >> pinctrl-single,register-width = <32>; >> pinctrl-single,function-mask = <0x1>; >> #pinctrl-cells = <2>; >> >> /* Enable GPIO6 and GPIO7, possibly unknown others */ >> pinmux_disable_jtag: disable_jtag { >> pinctrl-single,bits = <0x0 0x0 0x8000>; >> }; >> >> pinmux_disable_sys_led: disable_sys_led { >> pinctrl-single,bits = <0x0 0x0 0x100>; >> }; > > Thanks, I wasn't aware of these fields. Will update in v2. Good. > >>> >>> + pinmux_led: pinmux@1b000600 { >>> + compatible = "pinctrl-single"; >>> + reg = <0x1b000600 0x4>; >>> + >>> + pinctrl-single,bit-per-mux; >>> + pinctrl-single,register-width = <32>; >>> + pinctrl-single,function-mask = <0x1>; >>> + #pinctrl-cells = <2>; >>> + >>> + /* enable GPIO 0 */ >>> + pinmux_disable_sys_led: disable_sys_led { >>> + pinctrl-single,bits = <0x0 0x3000 0x3000>; >>> + }; > > This field, I assume, controls the toggling rate of the system led then. Would explain why it has > two bits and is called SYS_LED_MODE. Yes, see above. Cheers, Birger
Hi Birger, On Tue, 2022-06-07 at 11:26 +0200, Birger Koblitz wrote: > Hi, > > On 07.06.22 11:04, Sander Vanheule wrote: > > On Tue, 2022-06-07 at 10:15 +0200, Birger Koblitz wrote: > > > Hi, > > > > > > has anyone tested that??? > > > > I don't have any 931x hardware, but it is based on what you put into setup.c. > What is in the setup.c makes the System LED solid on. It is the same for all > the architectures: > static void __init rtl838x_setup(void) > { > /* Setup System LED. Bit 15 then allows to toggle it */ > sw_w32_mask(0, 3 << 16, RTL838X_LED_GLB_CTRL); > } > > static void __init rtl839x_setup(void) > { > /* Setup System LED. Bit 14 of RTL839X_LED_GLB_CTRL then allows to toggle it */ > sw_w32_mask(0, 3 << 15, RTL839X_LED_GLB_CTRL); > } > static void __init rtl931x_setup(void) > { > sw_w32_mask(0, 3 << 12, RTL931X_LED_GLB_CTRL); > } > > You are not using that to disable the system led on the 838x/839x, so why would it do something > different on the RTL931x? Note that at that time it was not know how to toggle the LED, because > that is somewhere else. > OK, it's clear I misunderstood what it was doing. Still, I don't think we should modify this setting unconditionally, so I'll reword the commit message. Best, Sander > > > > > This does not make sense at all, there is no LED disable > > > in the LED_GLB_CTRL register. Instead one needs to use RTL9310_MAC_L2_GLOBAL_CTRL2 > > > > > > The following works nicely on the XS1930 and Edgecore: > > > > > > pinmux: pinmux@1b001358 { > > > compatible = "pinctrl-single"; > > > reg = <0x1b001358 0x4>; > > > > > > pinctrl-single,bit-per-mux; > > > pinctrl-single,register-width = <32>; > > > pinctrl-single,function-mask = <0x1>; > > > #pinctrl-cells = <2>; > > > > > > /* Enable GPIO6 and GPIO7, possibly unknown others */ > > > pinmux_disable_jtag: disable_jtag { > > > pinctrl-single,bits = <0x0 0x0 0x8000>; > > > }; > > > > > > pinmux_disable_sys_led: disable_sys_led { > > > pinctrl-single,bits = <0x0 0x0 0x100>; > > > }; > > > > Thanks, I wasn't aware of these fields. Will update in v2. > Good. > > > > > > > > > > > + pinmux_led: pinmux@1b000600 { > > > > + compatible = "pinctrl-single"; > > > > + reg = <0x1b000600 0x4>; > > > > + > > > > + pinctrl-single,bit-per-mux; > > > > + pinctrl-single,register-width = <32>; > > > > + pinctrl-single,function-mask = <0x1>; > > > > + #pinctrl-cells = <2>; > > > > + > > > > + /* enable GPIO 0 */ > > > > + pinmux_disable_sys_led: disable_sys_led { > > > > + pinctrl-single,bits = <0x0 0x3000 0x3000>; > > > > + }; > > > > This field, I assume, controls the toggling rate of the system led then. Would explain why it > > has > > two bits and is called SYS_LED_MODE. > Yes, see above. > > Cheers, > Birger
diff --git a/target/linux/realtek/dts-5.10/rtl931x.dtsi b/target/linux/realtek/dts-5.10/rtl931x.dtsi index 29aee1f7b268..f4e2fd248f7e 100644 --- a/target/linux/realtek/dts-5.10/rtl931x.dtsi +++ b/target/linux/realtek/dts-5.10/rtl931x.dtsi @@ -155,6 +155,20 @@ }; }; + pinmux_led: pinmux@1b000600 { + compatible = "pinctrl-single"; + reg = <0x1b000600 0x4>; + + pinctrl-single,bit-per-mux; + pinctrl-single,register-width = <32>; + pinctrl-single,function-mask = <0x1>; + #pinctrl-cells = <2>; + + /* enable GPIO 0 */ + pinmux_disable_sys_led: disable_sys_led { + pinctrl-single,bits = <0x0 0x3000 0x3000>; + }; + }; ethernet0: ethernet@1b00a300 { status = "okay";
Like for RTL838x devices, add a pinctrl-single node to manage the sys-led/gpio0 mux, and allow using the pin as GPIO. Signed-off-by: Sander Vanheule <sander@svanheule.net> --- target/linux/realtek/dts-5.10/rtl931x.dtsi | 14 ++++++++++++++ 1 file changed, 14 insertions(+)