RE: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
Hi, Guenter
> Subject: RE: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for
> wdog operations
>
> Hi, Guenter
>
>
> > Subject: Re: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the
> > sequence for wdog operations
> >
> > On 7/27/20 11:42 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
> > > ---
> > > 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..b414ecf 100644
> > > --- a/drivers/watchdog/imx7ulp_wdt.c
> > > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > > @@ -4,6 +4,7 @@
> > > */
> > >
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -48,17 +49,32 @@ struct imx7ulp_wdt_device {
> > > struct clk *clk;
> > > };
> > >
> > > +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask) {
> > > + int retries = 100;
> > > +
> > > + do {
> > > + if (readl_relaxed(base + WDOG_CS) & mask)
> > > + return;
> > > + usleep_range(200, 1000);
> > > + } while (retries--);
> >
> > Sleep with interrupts disabled ? I can not imagine that this works
> > well in a single CPU system. On top of that, it seems quite pointless.
> > Either you don't want to be interrupted or you do, but sleeping with
> > interrupts disabled really doesn't make sense. And does it really take
> > 200-1000 uS for the watchdog subsystem to react, and sometimes up to
> > 200 * 100 = 20 mS ? That seems highly unlikely. If such a delay loop
> > is indeed needed, it should be limited by a time, not by number of
> repetitions.
> >
> > Unless there is evidence that there is a problem that needs to be
> > solved, I am not going to accept this code.
> >
>
> Oops, this is a mistake of using sleep with interrupt disabled, sorry for
> that.
> The best option is to use readl_relaxed_poll_timeout_atomic() to poll the
> status bit, however, the i.MX7ULP watchdog is very special that the unlock
> window ONLY open for several cycles, that means the unlock status bit will be
> set and then clear automatically after those cycles, using
> readl_relaxed_poll_timeout_atomic() will fail since there are many timeout
> handle code in it and the unlock window is open and close during this timeout
> handle interval, so it fail to catch the unlock bit.
>
> The ideal option is using atomic polling without any other timeout check to
> make sure the unlock window is NOT missed, but I think Linux kernel will NOT
> accept a while loop without timeout, and that is why I tried to use
> usleep_ranges(), but obviously I made a mistake of using it with IRQ disabled.
>
> Do you have any suggestion of how to handle such case? If the hardware ONLY
> unlock the register for a small window, how to poll the status bit with
> timeout
> handle and also make sure the timeout handle code as quick as possible to
> NOT miss the window?
>
I did more experiment and found that below readl_poll_timeout_atomic() is
actually
working, so I sent a V2 with it, please help review, thank you.
+ 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));
Thanks,
Anson
RE: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
Hi, Guenter
> Subject: Re: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for
> wdog operations
>
> On 7/27/20 11:42 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
> > ---
> > 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..b414ecf 100644
> > --- a/drivers/watchdog/imx7ulp_wdt.c
> > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > @@ -4,6 +4,7 @@
> > */
> >
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -48,17 +49,32 @@ struct imx7ulp_wdt_device {
> > struct clk *clk;
> > };
> >
> > +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask) {
> > + int retries = 100;
> > +
> > + do {
> > + if (readl_relaxed(base + WDOG_CS) & mask)
> > + return;
> > + usleep_range(200, 1000);
> > + } while (retries--);
>
> Sleep with interrupts disabled ? I can not imagine that this works well in a
> single CPU system. On top of that, it seems quite pointless.
> Either you don't want to be interrupted or you do, but sleeping with
> interrupts
> disabled really doesn't make sense. And does it really take 200-1000 uS for
> the
> watchdog subsystem to react, and sometimes up to 200 * 100 = 20 mS ? That
> seems highly unlikely. If such a delay loop is indeed needed, it should be
> limited by a time, not by number of repetitions.
>
> Unless there is evidence that there is a problem that needs to be solved, I am
> not going to accept this code.
>
Oops, this is a mistake of using sleep with interrupt disabled, sorry for that.
The best option is to use readl_relaxed_poll_timeout_atomic() to poll the status
bit, however, the i.MX7ULP watchdog is very special that the unlock window ONLY
open for several cycles, that means the unlock status bit will be set and then
clear
automatically after those cycles, using readl_relaxed_poll_timeout_atomic() will
fail since there are many timeout handle code in it and the unlock window is
open
and close during this timeout handle interval, so it fail to catch the unlock
bit.
The ideal option is using atomic polling without any other timeout check to make
sure the unlock window is NOT missed, but I think Linux kernel will NOT accept
a while loop without timeout, and that is why I tried to use usleep_ranges(),
but
obviously I made a mistake of using it with IRQ disabled.
Do you have any suggestion of how to handle such case? If the hardware ONLY
unlock the register for a small window, how to poll the status bit with timeout
handle and also make sure the timeout handle code as quick as possible to NOT
miss the window?
Thanks,
Anson
Re: [PATCH 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
On 7/27/20 11:42 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
> ---
> 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..b414ecf 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -4,6 +4,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -48,17 +49,32 @@ struct imx7ulp_wdt_device {
> struct clk *clk;
> };
>
> +static inline void imx7ulp_wdt_wait(void __iomem *base, u32 mask)
> +{
> + int retries = 100;
> +
> + do {
> + if (readl_relaxed(base + WDOG_CS) & mask)
> + return;
> + usleep_range(200, 1000);
> + } while (retries--);
Sleep with interrupts disabled ? I can not imagine that this works well
in a single CPU system. On top of that, it seems quite pointless.
Either you don't want to be interrupted or you do, but sleeping
with interrupts disabled really doesn't make sense. And does it really
take 200-1000 uS for the watchdog subsystem to react, and sometimes up
to 200 * 100 = 20 mS ? That seems highly unlikely. If such a delay loop
is indeed needed, it should be limited by a time, not by number of
repetitions.
Unless there is evidence that there is a problem that needs to be solved,
I am not going to accept this code.
Thanks,
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)
>
