[PATCH] realtek: ZyXEL GS1900-48: drop gpio-restart

[Daniel Golle]

On Mon, Feb 21, 2022 at 10:04:13AM +0100, Sander Vanheule wrote:
> On Sun, 2022-02-20 at 21:13 +0100, Birger Koblitz wrote:
> > Hi,
> > 
> > during development I came across situations where the RTL839x
> > SoC's own reset did not always completely reset its state.
> > U-Boot was no longer able to boot via tftp afterwards.
> > This is the same situation we see on the RTL838x.
> > While users will hopefully never mess up the SoC as during
> > development, I would prefer a solution that reliably
> > resets the device over a solution that avoids a warning.
> 
> If you did not see all three warnings on your device, that would mean it's
> behaving differently. As Daniel noted, the fact that the third warning is even
> emitted, means that gpio-restart failed to restart the device and is returning
> from its handler. That either means the GPIO is incorrect (although it matches
> the data from the stock firmware), or the RTL8231's output cannot be set to the
> intended value.
> 
> At least for my device it is not just about avoiding an ugly warning. gpio-
> restart effectively does nothing, so there's no point in setting it up.
> 

Thinking about it another night I think you are both right:
The GPIO does trigger a reliable reset, it's just **not fast enough**,
hence the warning about that it can sleep.


> > At least we should have a comment in, that states that
> > once this is fixed in libgpiod this should be the way
> > to reset the device. And we should look into fixing libgpiod.
> 
> There will be a good reason that gpio-restart is using gpiod_set_value() instead
> of gpiod_set_value_cansleep(). I don't know that much about the kernel, but

The reason is simple:
The restart code doesn't have any timeout, it expects an immediate
action and if it even reaches the next instruction, that would mean
the previous reset handler has failed.

> since the system is being torn down, it is likely certain things cannot be used
> anymore. Even if it would work with the current driver, once the RTL8231 is
> controlled through an MDIO bus (from a kernel perspective), things might change.

Yes, it may not even be possible to have it use *_cansleep() and then
wait a defined amount of time, because that would mean spawning another
thread/worker/whatever for the timeout...

Not using bit-banging to control the RTL8231 could solve that.
MDIO access code typically just actively loop-waits until the busy-bit
of the bus control register has cleared -- no sleep()'ing involved
there.


> 
> > 
> > Cheers,
> >    Birger
> > 
> > 
> > On 20/02/2022 19:50, Sander Vanheule wrote:
> > > GPIO 5 on the RTL8231 can be used to reset the system, but gpio-restart
> > > uses gpiod_set_value. This triggers a kernel warning and backtrace for
> > > GPIO pins that can sleep, such as the RTL8231's. Two warnings are
> > > emitted by libgpiod, and a third warning by gpio-restart itself after it
> > > fails to restart the system:
> > > 
> > > [  106.654008] ------------[ cut here ]------------
> > > [  106.659240] WARNING: CPU: 0 PID: 4279 at drivers/gpio/gpiolib.c:3098
> > > gpiod_set_value+0x7c/0x108
> > >                 [ Stack dump and call trace ]
> > > [  106.826218] ---[ end trace d1de50b401f5a153 ]---
> > > [  106.962992] ------------[ cut here ]------------
> > > [  106.968208] WARNING: CPU: 0 PID: 4279 at drivers/gpio/gpiolib.c:3098
> > > gpiod_set_value+0x7c/0x108
> > >                 [ Stack dump and call trace ]
> > > [  107.136718] ---[ end trace d1de50b401f5a154 ]---
> > > [  111.087092] ------------[ cut here ]------------
> > > [  111.092271] WARNING: CPU: 0 PID: 4279 at drivers/power/reset/gpio-
> > > restart.c:46 gpio_restart_notify+0xc0/0xdc
> > >                 [ Stack dump and call trace ]
> > > [  111.256629] ---[ end trace d1de50b401f5a155 ]---
> > > 
> > > By removing gpio-restart from this device, we skip the restart-by-GPIO
> > > attempt and rely only on the watchdog for restarts, which is already the
> > > de facto behaviour.
> > > 
> > > Signed-off-by: Sander Vanheule <sander at svanheule.net>
> > > ---
> > >   target/linux/realtek/dts-5.10/rtl8393_zyxel_gs1900-48.dts | 5 -----
> > >   1 file changed, 5 deletions(-)
> > > 
> > > diff --git a/target/linux/realtek/dts-5.10/rtl8393_zyxel_gs1900-48.dts
> > > b/target/linux/realtek/dts-5.10/rtl8393_zyxel_gs1900-48.dts
> > > index dd392c5a9beb..e0e79068d2bc 100644
> > > --- a/target/linux/realtek/dts-5.10/rtl8393_zyxel_gs1900-48.dts
> > > +++ b/target/linux/realtek/dts-5.10/rtl8393_zyxel_gs1900-48.dts
> > > @@ -39,11 +39,6 @@
> > >                 gpio-controller;
> > >         };
> > >   
> > > -       gpio-restart {
> > > -               compatible = "gpio-restart";
> > > -               gpios = <&gpio1 5 GPIO_ACTIVE_LOW>;
> > > -       };
> > > -
> > >         keys {
> > >                 compatible = "gpio-keys-polled";
> > >                 poll-interval = <20>;