[PATCH] realtek: ZyXEL GS1900-48: drop gpio-restart
Daniel Golle
daniel at makrotopia.org
Mon Feb 21 12:16:45 PST 2022
On Mon, Feb 21, 2022 at 08:33:06PM +0100, Sander Vanheule wrote:
> 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.
Absolutely. 3s should be much more than enough.
Thank you for investigating this in such great depth!
>
> >
> > > 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.
Ah ok, so it's more of a RTL83xx+RTL8231 driver at this point ;)
Again, thank you for enlightening me!
Cheers
Daniel
>
> 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