Hi, Guenter
> Subject: RE: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for
> wdog operations
>
> Hi, Guenter
>
>
> > Subject: Re: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the
> > sequence for wdog operations
> >
> > On 7/27/20 11:42 PM, Anson Huang wrote:
> > > According to reference manual, the i.MX7ULP WDOG's operations should
> > > follow below sequence:
> > >
> > > 1. disable global interrupts;
> > > 2. unlock the wdog and wait unlock bit set; 3. reconfigure the wdog
> > > and wait for reconfiguration bit set; 4. enabel global interrupts.
> > >
> > > Strictly follow the recommended sequence can make it more robust.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > > ---
> > > drivers/watchdog/imx7ulp_wdt.c | 29 +++++++++++++++++++++++++++++
> > > 1 file changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/watchdog/imx7ulp_wdt.c
> > > b/drivers/watchdog/imx7ulp_wdt.c index 7993c8c..b414ecf 100644
> > > --- a/drivers/watchdog/imx7ulp_wdt.c
> > > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > > @@ -4,6 +4,7 @@
> > > */
> > >
> > > #include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > #include <linux/io.h>
> > > #include <linux/kernel.h>
> > > #include <linux/module.h>
> > > @@ -48,17 +49,32 @@ struct imx7ulp_wdt_device {
> > > struct clk *clk;
> > > };
> > >
> > > +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask) {
> > > + int retries = 100;
> > > +
> > > + do {
> > > + if (readl_relaxed(base + WDOG_CS) & mask)
> > > + return;
> > > + usleep_range(200, 1000);
> > > + } while (retries--);
> >
> > Sleep with interrupts disabled ? I can not imagine that this works
> > well in a single CPU system. On top of that, it seems quite pointless.
> > Either you don't want to be interrupted or you do, but sleeping with
> > interrupts disabled really doesn't make sense. And does it really take
> > 200-1000 uS for the watchdog subsystem to react, and sometimes up to
> > 200 * 100 = 20 mS ? That seems highly unlikely. If such a delay loop
> > is indeed needed, it should be limited by a time, not by number of
> repetitions.
> >
> > Unless there is evidence that there is a problem that needs to be
> > solved, I am not going to accept this code.
> >
>
> Oops, this is a mistake of using sleep with interrupt disabled, sorry for that.
> The best option is to use readl_relaxed_poll_timeout_atomic() to poll the
> status bit, however, the i.MX7ULP watchdog is very special that the unlock
> window ONLY open for several cycles, that means the unlock status bit will be
> set and then clear automatically after those cycles, using
> readl_relaxed_poll_timeout_atomic() will fail since there are many timeout
> handle code in it and the unlock window is open and close during this timeout
> handle interval, so it fail to catch the unlock bit.
>
> The ideal option is using atomic polling without any other timeout check to
> make sure the unlock window is NOT missed, but I think Linux kernel will NOT
> accept a while loop without timeout, and that is why I tried to use
> usleep_ranges(), but obviously I made a mistake of using it with IRQ disabled.
>
> Do you have any suggestion of how to handle such case? If the hardware ONLY
> unlock the register for a small window, how to poll the status bit with timeout
> handle and also make sure the timeout handle code as quick as possible to
> NOT miss the window?
>
I did more experiment and found that below readl_poll_timeout_atomic() is actually
working, so I sent a V2 with it, please help review, thank you.
+ u32 val = readl(base + WDOG_CS);
+
+ if (!(val & mask))
+ WARN_ON(readl_poll_timeout_atomic(base + WDOG_CS, val,
+ val & mask, 0,
+ WDOG_WAIT_TIMEOUT));
Thanks,
Anson
