IMX: Rearm watchdog after loading value for restart
diff mbox

Message ID 20170427131434.17085-1-Frederik.juul@3shape.com
State New
Headers show

Commit Message

frederikj@gmail.com April 27, 2017, 1:14 p.m. UTC
When calling the restart function the watchdog is activated and the WDOG
timeout field is set to 0. This gives a reset time of 500 ms, which is
consistent with the following delay. However this new time is not loaded
into the WDOG until the WDOG service routine is run (see IMX6SXRM section
70.5.1.1 "Servicing WDOG to reload the counter").

Not reloading the counter could result in a random delay up to 128 seconds
before the system restarts, depending on the previous value of the WDOG
timeout field and when it has last been serviced. That bug is fixed with
this patch.

Signed-off-by: Frederik Juul <Frederik.juul@3shape.com>

Comments

Baruch Siach April 27, 2017, 7:57 p.m. UTC | #1
Hi Frederik,

On Thu, Apr 27, 2017 at 03:14:34PM +0200, frederikj@gmail.com wrote:
> When calling the restart function the watchdog is activated and the WDOG
> timeout field is set to 0. This gives a reset time of 500 ms, which is
> consistent with the following delay. However this new time is not loaded
> into the WDOG until the WDOG service routine is run (see IMX6SXRM section
> 70.5.1.1 "Servicing WDOG to reload the counter").
> 
> Not reloading the counter could result in a random delay up to 128 seconds
> before the system restarts, depending on the previous value of the WDOG
> timeout field and when it has last been serviced. That bug is fixed with
> this patch.

Which platform are you using? This code seems to only be used on legacy 
platforms that were not converted to DT. For DT based systems, including all 
i.MX6, the equivalent version is at 
drivers/watchdog/imx2_wdt.c:imx2_wdt_restart().

baruch

> Signed-off-by: Frederik Juul <Frederik.juul@3shape.com>
> 
> --- linux/arch/arm/mach-imx/system.c.orig	2017-04-20 10:41:33.207558297 +0200
> +++ linux/arch/arm/mach-imx/system.c	2017-04-27 14:27:19.225474885 +0200
> @@ -32,6 +32,8 @@
>  #include "common.h"
>  #include "hardware.h"
>  
> +#define IMX_WATCHDOG_SERVICE_REGISTER 2
> +
>  static void __iomem *wdog_base;
>  static struct clk *wdog_clk;
>  static int wcr_enable = (1 << 2);
> @@ -59,6 +61,10 @@ void mxc_restart(enum reboot_mode mode,
>  	imx_writew(wcr_enable, wdog_base);
>  	imx_writew(wcr_enable, wdog_base);
>  
> +	/* Rearm WDOG to load 500ms value */
> +	imx_writew(0x5555, wdog_base+IMX_WATCHDOG_SERVICE_REGISTER);
> +	imx_writew(0xaaaa, wdog_base+IMX_WATCHDOG_SERVICE_REGISTER);
> +
>  	/* wait for reset to assert... */
>  	mdelay(500);
Lothar Waßmann April 28, 2017, 10:03 a.m. UTC | #2
Hi,

On Thu, 27 Apr 2017 15:14:34 +0200 frederikj@gmail.com wrote:
> When calling the restart function the watchdog is activated and the WDOG
> timeout field is set to 0. This gives a reset time of 500 ms, which is
> consistent with the following delay. However this new time is not loaded
> into the WDOG until the WDOG service routine is run (see IMX6SXRM section
> 70.5.1.1 "Servicing WDOG to reload the counter").
> 
> Not reloading the counter could result in a random delay up to 128 seconds
> before the system restarts, depending on the previous value of the WDOG
> timeout field and when it has last been serviced. That bug is fixed with
> this patch.
>
The routine is actually initiating a softreset by clearing the SRS bit
in WCR. Thus the watchdog timeout should only have any effect, if the
softreset fails in some way. Under normal circumstances the reset
should happen immediately no matter what watchdog timeout is programmed.


Lothar Waßmann
Frederik Juul April 28, 2017, 12:45 p.m. UTC | #3
Disregard this patch then :) I had seen some strange behavior when toying with watchdog where it didn’t reset properly and wrote the "something went wrong" message. This patch fixed it for me, but I guess I may have misunderstood the issue.

Frederik Juul

-----Original Message-----
From: Lothar Waßmann [mailto:LW@KARO-electronics.de] 

Sent: 28. april 2017 12:03
To: frederikj@gmail.com
Cc: linux-arm-kernel@lists.infradead.org; shawnguo@kernel.org; fabio.estevam@nxp.com; kernel@pengutronix.de; Frederik Juul <Frederik.Juul@3shape.com>
Subject: Re: [PATCH] IMX: Rearm watchdog after loading value for restart

Hi,

On Thu, 27 Apr 2017 15:14:34 +0200 frederikj@gmail.com wrote:
> When calling the restart function the watchdog is activated and the 

> WDOG timeout field is set to 0. This gives a reset time of 500 ms, 

> which is consistent with the following delay. However this new time is 

> not loaded into the WDOG until the WDOG service routine is run (see 

> IMX6SXRM section

> 70.5.1.1 "Servicing WDOG to reload the counter").

> 

> Not reloading the counter could result in a random delay up to 128 

> seconds before the system restarts, depending on the previous value of 

> the WDOG timeout field and when it has last been serviced. That bug is 

> fixed with this patch.

>

The routine is actually initiating a softreset by clearing the SRS bit in WCR. Thus the watchdog timeout should only have any effect, if the softreset fails in some way. Under normal circumstances the reset should happen immediately no matter what watchdog timeout is programmed.


Lothar Waßmann
--
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de ___________________________________________________________

Patch
diff mbox

--- linux/arch/arm/mach-imx/system.c.orig	2017-04-20 10:41:33.207558297 +0200
+++ linux/arch/arm/mach-imx/system.c	2017-04-27 14:27:19.225474885 +0200
@@ -32,6 +32,8 @@ 
 #include "common.h"
 #include "hardware.h"
 
+#define IMX_WATCHDOG_SERVICE_REGISTER 2
+
 static void __iomem *wdog_base;
 static struct clk *wdog_clk;
 static int wcr_enable = (1 << 2);
@@ -59,6 +61,10 @@  void mxc_restart(enum reboot_mode mode,
 	imx_writew(wcr_enable, wdog_base);
 	imx_writew(wcr_enable, wdog_base);
 
+	/* Rearm WDOG to load 500ms value */
+	imx_writew(0x5555, wdog_base+IMX_WATCHDOG_SERVICE_REGISTER);
+	imx_writew(0xaaaa, wdog_base+IMX_WATCHDOG_SERVICE_REGISTER);
+
 	/* wait for reset to assert... */
 	mdelay(500);