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.hu...@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 ?
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.
Thanks,
Guenter
