linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
@ 2020-07-29  2:20 Anson Huang
  2020-07-29  2:20 ` [PATCH V2 2/2] watchdog: imx7ulp: Watchdog should continue running for wait/stop mode Anson Huang
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Anson Huang @ 2020-07-29  2:20 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, kernel, festevam, linux-watchdog,
	linux-arm-kernel, linux-kernel
  Cc: Linux-imx

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);
+	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)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH V2 2/2] watchdog: imx7ulp: Watchdog should continue running for wait/stop mode
  2020-07-29  2:20 [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Anson Huang
@ 2020-07-29  2:20 ` Anson Huang
  2020-07-29  4:15 ` [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Guenter Roeck
  2020-07-29 15:15 ` Guenter Roeck
  2 siblings, 0 replies; 13+ messages in thread
From: Anson Huang @ 2020-07-29  2:20 UTC (permalink / raw)
  To: wim, linux, shawnguo, s.hauer, kernel, festevam, linux-watchdog,
	linux-arm-kernel, linux-kernel
  Cc: Linux-imx

When kernel idle, system will enter wait/stop mode, wdog should continue
running in this scenario, and the refresh thread can wake up system from
wait/stop mode.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
no change.
---
 drivers/watchdog/imx7ulp_wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index 7d2b12e..2b4ff43 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -22,6 +22,8 @@
 #define WDOG_CS_CLK		(LPO_CLK << LPO_CLK_SHIFT)
 #define WDOG_CS_EN		BIT(7)
 #define WDOG_CS_UPDATE		BIT(5)
+#define WDOG_CS_WAIT		BIT(1)
+#define WDOG_CS_STOP		BIT(0)
 
 #define WDOG_CNT	0x4
 #define WDOG_TOVAL	0x8
@@ -174,7 +176,8 @@ static void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
 	/* 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;
+	val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE |
+	      WDOG_CS_WAIT | WDOG_CS_STOP;
 	writel(val, base + WDOG_CS);
 	imx7ulp_wdt_wait(base, WDOG_CS_RCS);
 	local_irq_enable();
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29  2:20 [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Anson Huang
  2020-07-29  2:20 ` [PATCH V2 2/2] watchdog: imx7ulp: Watchdog should continue running for wait/stop mode Anson Huang
@ 2020-07-29  4:15 ` Guenter Roeck
  2020-07-29  4:50   ` Anson Huang
  2020-07-29 15:15 ` Guenter Roeck
  2 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-07-29  4:15 UTC (permalink / raw)
  To: Anson Huang, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

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)
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29  4:15 ` [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Guenter Roeck
@ 2020-07-29  4:50   ` Anson Huang
  2020-07-29  5:02     ` Anson Huang
  2020-07-29 14:13     ` Guenter Roeck
  0 siblings, 2 replies; 13+ messages in thread
From: Anson Huang @ 2020-07-29  4:50 UTC (permalink / raw)
  To: Guenter Roeck, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29  4:50   ` Anson Huang
@ 2020-07-29  5:02     ` Anson Huang
  2020-07-29 14:55       ` Guenter Roeck
  2020-07-29 14:13     ` Guenter Roeck
  1 sibling, 1 reply; 13+ messages in thread
From: Anson Huang @ 2020-07-29  5:02 UTC (permalink / raw)
  To: Guenter Roeck, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29  4:50   ` Anson Huang
  2020-07-29  5:02     ` Anson Huang
@ 2020-07-29 14:13     ` Guenter Roeck
  2020-07-29 14:17       ` Anson Huang
  1 sibling, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-07-29 14:13 UTC (permalink / raw)
  To: Anson Huang, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29 14:13     ` Guenter Roeck
@ 2020-07-29 14:17       ` Anson Huang
  0 siblings, 0 replies; 13+ messages in thread
From: Anson Huang @ 2020-07-29 14:17 UTC (permalink / raw)
  To: Guenter Roeck, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29  5:02     ` Anson Huang
@ 2020-07-29 14:55       ` Guenter Roeck
  2020-07-29 15:13         ` Anson Huang
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-07-29 14:55 UTC (permalink / raw)
  To: Anson Huang, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29 14:55       ` Guenter Roeck
@ 2020-07-29 15:13         ` Anson Huang
  0 siblings, 0 replies; 13+ messages in thread
From: Anson Huang @ 2020-07-29 15:13 UTC (permalink / raw)
  To: Guenter Roeck, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29  2:20 [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Anson Huang
  2020-07-29  2:20 ` [PATCH V2 2/2] watchdog: imx7ulp: Watchdog should continue running for wait/stop mode Anson Huang
  2020-07-29  4:15 ` [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Guenter Roeck
@ 2020-07-29 15:15 ` Guenter Roeck
  2020-07-29 15:32   ` Anson Huang
  2 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-07-29 15:15 UTC (permalink / raw)
  To: Anson Huang, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29 15:15 ` Guenter Roeck
@ 2020-07-29 15:32   ` Anson Huang
  2020-07-29 15:49     ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Anson Huang @ 2020-07-29 15:32 UTC (permalink / raw)
  To: Guenter Roeck, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29 15:32   ` Anson Huang
@ 2020-07-29 15:49     ` Guenter Roeck
  2020-07-30  2:04       ` Anson Huang
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2020-07-29 15:49 UTC (permalink / raw)
  To: Anson Huang, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations
  2020-07-29 15:49     ` Guenter Roeck
@ 2020-07-30  2:04       ` Anson Huang
  0 siblings, 0 replies; 13+ messages in thread
From: Anson Huang @ 2020-07-30  2:04 UTC (permalink / raw)
  To: Guenter Roeck, wim, shawnguo, s.hauer, kernel, festevam,
	linux-watchdog, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-07-30  2:04 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29  2:20 [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Anson Huang
2020-07-29  2:20 ` [PATCH V2 2/2] watchdog: imx7ulp: Watchdog should continue running for wait/stop mode Anson Huang
2020-07-29  4:15 ` [PATCH V2 1/2] watchdog: imx7ulp: Strictly follow the sequence for wdog operations Guenter Roeck
2020-07-29  4:50   ` Anson Huang
2020-07-29  5:02     ` Anson Huang
2020-07-29 14:55       ` Guenter Roeck
2020-07-29 15:13         ` Anson Huang
2020-07-29 14:13     ` Guenter Roeck
2020-07-29 14:17       ` Anson Huang
2020-07-29 15:15 ` Guenter Roeck
2020-07-29 15:32   ` Anson Huang
2020-07-29 15:49     ` Guenter Roeck
2020-07-30  2:04       ` Anson Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).