[PATCH] realtek: ZyXEL GS1900-48: drop gpio-restart
Sander Vanheule
sander at svanheule.net
Mon Feb 21 11:33:06 PST 2022
On Mon, 2022-02-21 at 12:09 +0000, Daniel Golle wrote:
> 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.
>
I just checked with my multimeter, and while the GPIO5 on the RTL8231 does go high/low
when I set the output high/low from Linux, my device certainly doesn't reset. The other
GPIO lines on the chip do work, since SFP modules are correctly detected.
Birger, just to be sure, can you confirm that your device does reset with GPIO5 on the
RTL8231?
>
> > > 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.
Not sure what you mean here, gpio-restart does have a few built-in delays. With the used
default values this should give the following waveform (voltages inverted with
GPIO_ACTIVE_LOW):
|< 100ms >|< 100 ms >|< 3000 ms >|< Restart failed
_____|_________| |_______________|__ [ active ]
_____X \__________/ [inactive]
^ Restart should already occur here
There is some debouncing circuitry between the hard reset button and the trace leading to
the SoC, so maybe 100ms wouldn't be enough. Even if setting the GPIO is slow (or happens
asynchronously), I think 3s should be enough to allow the correct level to be asserted.
>
> > 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.
The current driver for the RTL82131 doesn't use bit-banging, but talks to the AUX-MDIO
control registers directly to perform register reads and writes. To wait for MDIO
transfers to finish, udelay() is used, so there's already no sleep()'ing at the moment.
Best,
Sander
>
> >
> > >
> > > 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>;
More information about the openwrt-devel
mailing list