RE: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations

[Anson Huang]

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