| Message ID | 1595989227-24700-1-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2,1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations | expand |
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@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. Also, I do not believe that a 10 milli-second timeout is warranted. This will need to be backed up by the datasheet. Guenter > +} > + > static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable) > { > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > > u32 val = readl(wdt->base + WDOG_CS); > > + local_irq_disable(); > writel(UNLOCK, wdt->base + WDOG_CNT); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > if (enable) > writel(val | WDOG_CS_EN, wdt->base + WDOG_CS); > else > writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > + local_irq_enable(); > } > > static bool imx7ulp_wdt_is_enabled(void __iomem *base) > @@ -72,7 +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog) > { > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > > + local_irq_disable(); > + writel(UNLOCK, wdt->base + WDOG_CNT); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > writel(REFRESH, wdt->base + WDOG_CNT); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > + local_irq_enable(); > > return 0; > } > @@ -98,8 +119,12 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog, > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > u32 val = WDOG_CLOCK_RATE * timeout; > > + local_irq_disable(); > writel(UNLOCK, wdt->base + WDOG_CNT); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > writel(val, wdt->base + WDOG_TOVAL); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > + local_irq_enable(); > > wdog->timeout = timeout; > > @@ -140,15 +165,19 @@ static void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout) > { > u32 val; > > + local_irq_disable(); > /* unlock the wdog for reconfiguration */ > writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT); > writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT); > + imx7ulp_wdt_wait(base, WDOG_CS_ULK); > > /* set an initial timeout value in TOVAL */ > writel(timeout, base + WDOG_TOVAL); > /* enable 32bit command sequence and reconfigure */ > val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE; > writel(val, base + WDOG_CS); > + imx7ulp_wdt_wait(base, WDOG_CS_RCS); > + local_irq_enable(); > } > > static void imx7ulp_wdt_action(void *data) >
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@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
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@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? + if (!(val & mask)) { + if (readl_poll_timeout_atomic(base + WDOG_CS, val, + val & mask, 0, + WDOG_WAIT_TIMEOUT)) + pr_err("wdog wait timeout, mask 0x%x\n", mask); + } Thanks, Anson
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@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
Hi, Guenter > Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence > for wdog operations > > 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@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. OK, I will use 20us. Thanks, Anson
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@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
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@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
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@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)); > +} > + > static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable) > { > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > > u32 val = readl(wdt->base + WDOG_CS); > > + local_irq_disable(); > writel(UNLOCK, wdt->base + WDOG_CNT); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > if (enable) > writel(val | WDOG_CS_EN, wdt->base + WDOG_CS); > else > writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > + local_irq_enable(); > } > > static bool imx7ulp_wdt_is_enabled(void __iomem *base) > @@ -72,7 +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog) > { > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > > + local_irq_disable(); > + writel(UNLOCK, wdt->base + WDOG_CNT); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > writel(REFRESH, wdt->base + WDOG_CNT); > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); Per reference manual (section 59.5.4), the waits are not required here, and neither is the unlock. For practical purposes, disabling interrupts is useless as well since the refresh write operation is just a single register write. Guenter
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@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)); > > +} > > + > > static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool > > enable) { > > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > > > > u32 val = readl(wdt->base + WDOG_CS); > > > > + local_irq_disable(); > > writel(UNLOCK, wdt->base + WDOG_CNT); > > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > > if (enable) > > writel(val | WDOG_CS_EN, wdt->base + WDOG_CS); > > else > > writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS); > > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > > + local_irq_enable(); > > } > > > > static bool imx7ulp_wdt_is_enabled(void __iomem *base) @@ -72,7 > > +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog) { > > struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > > > > + local_irq_disable(); > > + writel(UNLOCK, wdt->base + WDOG_CNT); > > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > > writel(REFRESH, wdt->base + WDOG_CNT); > > + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > > Per reference manual (section 59.5.4), the waits are not required here, and > neither is the unlock. For practical purposes, disabling interrupts is useless as > well since the refresh write operation is just a single register write. Correct, the example in reference manual does NOT have this flow for refresh, but I checked with our design team yestoday, their validation code indeed has this flow, that is why I added it for refresh operation as well. I will do a test on our EVK board, and if it works without this flow, I will remove them In V3. Thanks, Anson
On 7/29/20 8:32 AM, 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@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)); >>> +} >>> + >>> static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool >>> enable) { >>> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); >>> >>> u32 val = readl(wdt->base + WDOG_CS); >>> >>> + local_irq_disable(); >>> writel(UNLOCK, wdt->base + WDOG_CNT); >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); >>> if (enable) >>> writel(val | WDOG_CS_EN, wdt->base + WDOG_CS); >>> else >>> writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS); >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); >>> + local_irq_enable(); >>> } >>> >>> static bool imx7ulp_wdt_is_enabled(void __iomem *base) @@ -72,7 >>> +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog) { >>> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); >>> >>> + local_irq_disable(); >>> + writel(UNLOCK, wdt->base + WDOG_CNT); >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); >>> writel(REFRESH, wdt->base + WDOG_CNT); >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); >> >> Per reference manual (section 59.5.4), the waits are not required here, and >> neither is the unlock. For practical purposes, disabling interrupts is useless as >> well since the refresh write operation is just a single register write. > > Correct, the example in reference manual does NOT have this flow for refresh, but > I checked with our design team yestoday, their validation code indeed has this flow, > that is why I added it for refresh operation as well. If it is needed, they'll need to update the manual. Urgently. Really, missing the information that the watchdog must be unlocked in order to refresh the timer would not just be be a minor flaw in the manual. Either it is needed and must be mentioned (because the watchdog would otherwise simply not work), or it isn't needed and should not be done. Thanks, Guenter > I will do a test on our EVK board, and if it works without this flow, I will remove them > In V3. > > Thanks, > Anson > > > >
Hi, Guenter > Subject: Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence > for wdog operations > > On 7/29/20 8:32 AM, 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@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)); > >>> +} > >>> + > >>> static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool > >>> enable) { > >>> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > >>> > >>> u32 val = readl(wdt->base + WDOG_CS); > >>> > >>> + local_irq_disable(); > >>> writel(UNLOCK, wdt->base + WDOG_CNT); > >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > >>> if (enable) > >>> writel(val | WDOG_CS_EN, wdt->base + WDOG_CS); > >>> else > >>> writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS); > >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > >>> + local_irq_enable(); > >>> } > >>> > >>> static bool imx7ulp_wdt_is_enabled(void __iomem *base) @@ -72,7 > >>> +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog) > >>> +{ > >>> struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); > >>> > >>> + local_irq_disable(); > >>> + writel(UNLOCK, wdt->base + WDOG_CNT); > >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); > >>> writel(REFRESH, wdt->base + WDOG_CNT); > >>> + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); > >> > >> Per reference manual (section 59.5.4), the waits are not required > >> here, and neither is the unlock. For practical purposes, disabling > >> interrupts is useless as well since the refresh write operation is just a single > register write. > > > > Correct, the example in reference manual does NOT have this flow for > > refresh, but I checked with our design team yestoday, their validation > > code indeed has this flow, that is why I added it for refresh operation as well. > > If it is needed, they'll need to update the manual. Urgently. > Really, missing the information that the watchdog must be unlocked in order > to refresh the timer would not just be be a minor flaw in the manual. Either it > is needed and must be mentioned (because the watchdog would otherwise > simply not work), or it isn't needed and should not be done. > Previously, the guy I checked the refresh flow is validation guy and looks like his answer is NOT accurate, and I just checked with the SoC design owner, and he confirmed that the refresh does NOT need unlock operation. I will drop the sequence for refresh operation in V3. " Hi Anson, As we talked in IM, wdg unlock and refresh are two different operations, and they don’t impact each other. So you can refresh wdg without unlocking it. " Thanks, Anson
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)); +} + static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable) { struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); u32 val = readl(wdt->base + WDOG_CS); + local_irq_disable(); writel(UNLOCK, wdt->base + WDOG_CNT); + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); if (enable) writel(val | WDOG_CS_EN, wdt->base + WDOG_CS); else writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS); + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); + local_irq_enable(); } static bool imx7ulp_wdt_is_enabled(void __iomem *base) @@ -72,7 +88,12 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog) { struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); + local_irq_disable(); + writel(UNLOCK, wdt->base + WDOG_CNT); + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); writel(REFRESH, wdt->base + WDOG_CNT); + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); + local_irq_enable(); return 0; } @@ -98,8 +119,12 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog, struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog); u32 val = WDOG_CLOCK_RATE * timeout; + local_irq_disable(); writel(UNLOCK, wdt->base + WDOG_CNT); + imx7ulp_wdt_wait(wdt->base, WDOG_CS_ULK); writel(val, wdt->base + WDOG_TOVAL); + imx7ulp_wdt_wait(wdt->base, WDOG_CS_RCS); + local_irq_enable(); wdog->timeout = timeout; @@ -140,15 +165,19 @@ static void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout) { u32 val; + local_irq_disable(); /* unlock the wdog for reconfiguration */ writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT); writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT); + imx7ulp_wdt_wait(base, WDOG_CS_ULK); /* set an initial timeout value in TOVAL */ writel(timeout, base + WDOG_TOVAL); /* enable 32bit command sequence and reconfigure */ val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE; writel(val, base + WDOG_CS); + imx7ulp_wdt_wait(base, WDOG_CS_RCS); + local_irq_enable(); } static void imx7ulp_wdt_action(void *data)
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@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(+)