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

[Guenter Roeck]

On 7/28/20 9:50 PM, Anson Huang wrote:
> 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?
> 

That would be even worse. You say yourself that the window is only open for a few
microseconds. Now you are suggesting to hold the entire system hostage for up to
100 mS if the code misses that window for some reason. Based on what you said,
100 uS might be barely acceptable. 10-20 uS would be reasonable. But not 100 mS.

Guenter