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

Anson Huang anson.huang at nxp.com
Wed Jul 29 00:50:06 EDT 2020


Hi, Guenter


> Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence
> for wdog operations
> 
> On 7/28/20 7:20 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 at nxp.com>
> > ---
> > Changes since V1:
> > 	- use readl_poll_timeout_atomic() instead of usleep_ranges() since IRQ is
> disabled.
> > ---
> >  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..7d2b12e 100644
> > --- a/drivers/watchdog/imx7ulp_wdt.c
> > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > @@ -5,6 +5,7 @@
> >
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > @@ -36,6 +37,7 @@
> >  #define DEFAULT_TIMEOUT	60
> >  #define MAX_TIMEOUT	128
> >  #define WDOG_CLOCK_RATE	1000
> > +#define WDOG_WAIT_TIMEOUT	10000
> >
> >  static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout,
> > bool, 0000); @@ -48,17 +50,31 @@ struct imx7ulp_wdt_device {
> >  	struct clk *clk;
> >  };
> >
> > +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask) {
> > +	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));
> 
> I am not a friend of WARN_ON, especially in situations like this.
> Please explain why this is needed, and why a return of -ETIMEDOUT is not
> feasible.

OK, I will use return value of -ETIMEOUT and handle it in the caller.

> 
> Also, I do not believe that a 10 milli-second timeout is warranted.
> This will need to be backed up by the datasheet.
> 

There is no such info provided in reference manual or datasheet, but I just did
an experiment, the unlock window is open in less than 1us after sending unlock command,
and ONLY last for ONLY 2~3 us then close, the reconfiguration status bit will be set in less than
1us after register write. So what do you recommend for this timeout value? 100mS for safe?

Thanks,
Anson


More information about the linux-arm-kernel mailing list