[PATCH v1 3/5] realtek: add sys-led disable pinctrl for rtl931x

[Birger Koblitz]

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 at 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 at 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