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

[Sander Vanheule]

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