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

[Anson Huang]

Hi, Guenter


> Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence
> for wdog operations
> 
> On 7/28/20 10:02 PM, Anson Huang wrote:
> > Hi, Guenter
> >
> >
> >> Subject: RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the
> >> sequence for wdog operations
> >>
> >> 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.
> >
> > After a further look, some of the imx7ulp_wdt_wait () callers are void
> > function, so if want to handle the return value, all those functions
> > return type need to be changed. And, when the return value is
> > -ETIMEDOUT, the ONLY action is to print out some error message for
> > these void function, need to use pr_err() due to no dev pointer available, so
> do you think it is acceptable to just replace the WARN_ON with pr_err() as
> below?
> >
> First, the point here is that the callers can't do their work if the function times
> out. So, if the return value isn't necessary, and callers don't need to check it,
> the function would not be necessary to start with. If it is necessary, and if there
> is a concern that it can fail, callers should make sure that it actually succeeded.
> With that in mind, yes, imx7ulp_wdt_init() should fail and return an error,
> because presumably that is what happened. The same is true for
> imx7ulp_wdt_enable().
> Really, what is the point of detecting a problem just to ignore it ?

Understood, then I will change those void caller functions to int type, and return error
if timeout. And maybe some 'goto' will be added, since whenever error happen, it
has to go to enable local irq then return.

> 
> Second, the wait function is also called _after_ a register was set. In many
> cases that won't do any good or bad. While it is ok to ignore the error in that
> case (when nothing else is done), the error message is pointless in that
> situation.

OK, for the RCS bit check, I will just ignore the return value check, similar with the
clk_disable_unprepare(), nothing can be done for such scenario.

Thanks,
Anson