[PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE
@ 2011-09-19 18:23 Fabio Estevam
  2011-09-19 18:24 ` [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes Fabio Estevam
  2011-09-21 13:25 ` [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE Wolfram Sang
  0 siblings, 2 replies; 6+ messages in thread
From: Fabio Estevam @ 2011-09-19 18:23 UTC (permalink / raw)
  To: linux-watchdog; +Cc: kernel, wim, Fabio Estevam

i.MX reference manual states that software can only write "1" into the WDE bit.

Remove the line that sets 0 to this bit and while at it remove an unneeded extra 
write to register WCR.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/watchdog/imx2_wdt.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index b8ef2c6..19df82d 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -89,13 +89,10 @@ static inline void imx2_wdt_setup(void)
 	val &= ~IMX2_WDT_WCR_WT;
 	/* Generate reset if WDOG times out */
 	val &= ~IMX2_WDT_WCR_WRE;
-	/* Keep Watchdog Disabled */
-	val &= ~IMX2_WDT_WCR_WDE;
+
 	/* Set the watchdog's Time-Out value */
 	val |= WDOG_SEC_TO_COUNT(imx2_wdt.timeout);
 
-	__raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
-
 	/* enable the watchdog */
 	val |= IMX2_WDT_WCR_WDE;
 	__raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
-- 
1.6.0.4



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

* [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes
  2011-09-19 18:23 [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE Fabio Estevam
@ 2011-09-19 18:24 ` Fabio Estevam
  2011-09-21 13:27   ` Wolfram Sang
  2011-09-21 13:25 ` [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE Wolfram Sang
  1 sibling, 1 reply; 6+ messages in thread
From: Fabio Estevam @ 2011-09-19 18:24 UTC (permalink / raw)
  To: linux-watchdog; +Cc: kernel, wim, Fabio Estevam

Activate WDZST so that the watchdog timer does not count during low power modes.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/watchdog/imx2_wdt.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 19df82d..e1a9abb 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -41,6 +41,7 @@
 #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
 #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
 #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
+#define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog Low Power */
 
 #define IMX2_WDT_WSR		0x02		/* Service Register */
 #define IMX2_WDT_SEQ1		0x5555		/* -> service sequence 1 */
@@ -93,6 +94,9 @@ static inline void imx2_wdt_setup(void)
 	/* Set the watchdog's Time-Out value */
 	val |= WDOG_SEC_TO_COUNT(imx2_wdt.timeout);
 
+	/* Disable watchdog suring low power modes */
+	val |= IMX2_WDT_WCR_WDZST;
+
 	/* enable the watchdog */
 	val |= IMX2_WDT_WCR_WDE;
 	__raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
-- 
1.6.0.4



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

* Re: [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes
  2011-09-19 18:24 ` [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes Fabio Estevam
@ 2011-09-21 13:27   ` Wolfram Sang
  2011-11-09 16:58     ` Fabio Estevam
  0 siblings, 1 reply; 6+ messages in thread
From: Wolfram Sang @ 2011-09-21 13:27 UTC (permalink / raw)
  To: Fabio Estevam; +Cc: linux-watchdog, kernel, wim

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

On Mon, Sep 19, 2011 at 03:24:00PM -0300, Fabio Estevam wrote:
> Activate WDZST so that the watchdog timer does not count during low power modes.

Should be a user-selectable option IMHO. Changing the default behaviour
may cause regressions.

> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/watchdog/imx2_wdt.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 19df82d..e1a9abb 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -41,6 +41,7 @@
>  #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
>  #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
>  #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
> +#define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog Low Power */
>  
>  #define IMX2_WDT_WSR		0x02		/* Service Register */
>  #define IMX2_WDT_SEQ1		0x5555		/* -> service sequence 1 */
> @@ -93,6 +94,9 @@ static inline void imx2_wdt_setup(void)
>  	/* Set the watchdog's Time-Out value */
>  	val |= WDOG_SEC_TO_COUNT(imx2_wdt.timeout);
>  
> +	/* Disable watchdog suring low power modes */
> +	val |= IMX2_WDT_WCR_WDZST;
> +
>  	/* enable the watchdog */
>  	val |= IMX2_WDT_WCR_WDE;
>  	__raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
> -- 
> 1.6.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

* Re: [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes
  2011-09-21 13:27   ` Wolfram Sang
@ 2011-11-09 16:58     ` Fabio Estevam
  2011-11-09 22:04       ` Wolfram Sang
  0 siblings, 1 reply; 6+ messages in thread
From: Fabio Estevam @ 2011-11-09 16:58 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-watchdog, kernel, wim

Wolfram,

On 9/21/2011 10:27 AM, Wolfram Sang wrote:
> On Mon, Sep 19, 2011 at 03:24:00PM -0300, Fabio Estevam wrote:
>> Activate WDZST so that the watchdog timer does not count during low power modes.
> 
> Should be a user-selectable option IMHO. Changing the default behaviour
> may cause regressions.

I can´t visualize a real system usage where the watchdog needs to be active in low power mode.

Which regressions exactly are you concerned about? Which platforms?

Regards,

Fabio Estevam



--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes
  2011-11-09 16:58     ` Fabio Estevam
@ 2011-11-09 22:04       ` Wolfram Sang
  0 siblings, 0 replies; 6+ messages in thread
From: Wolfram Sang @ 2011-11-09 22:04 UTC (permalink / raw)
  To: Fabio Estevam; +Cc: linux-watchdog, kernel, wim

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]


> >> Activate WDZST so that the watchdog timer does not count during low power modes.
> > 
> > Should be a user-selectable option IMHO. Changing the default behaviour
> > may cause regressions.
> 
> I can´t visualize a real system usage where the watchdog needs to be active in low power mode.
> 
> Which regressions exactly are you concerned about? Which platforms?

I don't have any concrete example, yet I see things I could not visualize
before daily ;)

My concern was that changing the default behaviour of a crucial device like a
watchdog driver should be done _very_ carefully. And here it could be avoided
by making this feature optional per platform_data, so I thought. If others
think this is over-engineered, so be it. I just didn't want this concern go
unnoticed.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

* Re: [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE
  2011-09-19 18:23 [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE Fabio Estevam
  2011-09-19 18:24 ` [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes Fabio Estevam
@ 2011-09-21 13:25 ` Wolfram Sang
  1 sibling, 0 replies; 6+ messages in thread
From: Wolfram Sang @ 2011-09-21 13:25 UTC (permalink / raw)
  To: Fabio Estevam; +Cc: linux-watchdog, kernel, wim

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

On Mon, Sep 19, 2011 at 03:23:59PM -0300, Fabio Estevam wrote:
> i.MX reference manual states that software can only write "1" into the WDE bit.

But it also says "Note: This bit can be set/reset in debug mode
(exception)."

> Remove the line that sets 0 to this bit and while at it remove an unneeded extra 
> write to register WCR.

The MX31 manual says in 37.5.3.1:

"The Watchdog Control Register (WCR) field WT[7:0] must have a time-out
value written to it before the Watchdog can be enabled. The WDOG is
enabled by setting the Watchdog Enable (WDE) bit in the WCR."

I seem to recall that on some imx that two-folded write was indeed
necessary.

I'd say better safe than sorry and keep things as they are?

> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/watchdog/imx2_wdt.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index b8ef2c6..19df82d 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -89,13 +89,10 @@ static inline void imx2_wdt_setup(void)
>  	val &= ~IMX2_WDT_WCR_WT;
>  	/* Generate reset if WDOG times out */
>  	val &= ~IMX2_WDT_WCR_WRE;
> -	/* Keep Watchdog Disabled */
> -	val &= ~IMX2_WDT_WCR_WDE;
> +
>  	/* Set the watchdog's Time-Out value */
>  	val |= WDOG_SEC_TO_COUNT(imx2_wdt.timeout);
>  
> -	__raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
> -
>  	/* enable the watchdog */
>  	val |= IMX2_WDT_WCR_WDE;
>  	__raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
> -- 
> 1.6.0.4
> 
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

end of thread, other threads:[~2011-11-09 22:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-19 18:23 [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE Fabio Estevam
2011-09-19 18:24 ` [PATCH 2/2] watchdog: imx2_wdt: Disable watchdog during low-power modes Fabio Estevam
2011-09-21 13:27   ` Wolfram Sang
2011-11-09 16:58     ` Fabio Estevam
2011-11-09 22:04       ` Wolfram Sang
2011-09-21 13:25 ` [PATCH 1/2] watchdog: imx2_wdt: Do not clear IMX2_WDT_WCR_WDE Wolfram Sang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.