[PATCH] ARM: i.MX: When SRSR shows wdog then lookup reset source in wdog

[PATCH] ARM: i.MX: When SRSR shows wdog then lookup reset source in wdog

[Sascha Hauer]

On i.MX6 when the watchdog has resetted the system then the SRSR
register correctly shows that the watchdog has resetted the system.
This is not the desired result though, a "reset" in barebox or "reboot"
in Linux should result in "RST" as reset source. So instead of making
the SRSR register value overwrite the reset source read from the
watchdog registers, interpret the SRSR value corresponding to watchdog
reset as "lookup details in the watchdog registers".

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/mach-imx/imx.c  |  6 +++---
 drivers/watchdog/imxwd.c | 10 +++++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-imx/imx.c b/arch/arm/mach-imx/imx.c
index ad227663dd..f87dd76c75 100644
--- a/arch/arm/mach-imx/imx.c
+++ b/arch/arm/mach-imx/imx.c
@@ -204,9 +204,9 @@ void imx_set_reset_reason(void __iomem *srsr,
 	 * sure we'll always override info from watchdog driver.
 	 */
 	reset_source_set_priority(type,
-				  RESET_SOURCE_DEFAULT_PRIORITY + 1);
+				  RESET_SOURCE_DEFAULT_PRIORITY);
 	reset_source_set_instance(type, instance);
 
-	pr_info("i.MX reset reason %s (SRSR: 0x%08x)\n",
-		reset_source_name(), reg);
+	pr_debug("i.MX reset reason %s (SRSR: 0x%08x)\n",
+		 reset_source_name(), reg);
 }
diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
index a66fae400c..8dba662392 100644
--- a/drivers/watchdog/imxwd.c
+++ b/drivers/watchdog/imxwd.c
@@ -162,19 +162,23 @@ static void __noreturn imxwd_force_soc_reset(struct restart_handler *rst)
 static void imx_watchdog_detect_reset_source(struct imx_wd *priv)
 {
 	u16 val = readw(priv->base + IMX21_WDOG_WSTR);
+	int priority = RESET_SOURCE_DEFAULT_PRIORITY;
+
+	if (reset_source_get() == RESET_WDG)
+		priority++;
 
 	if (val & WSTR_COLDSTART) {
-		reset_source_set(RESET_POR);
+		reset_source_set_priority(RESET_POR, priority);
 		return;
 	}
 
 	if (val & (WSTR_HARDRESET | WSTR_WARMSTART)) {
-		reset_source_set(RESET_RST);
+		reset_source_set_priority(RESET_RST, priority);
 		return;
 	}
 
 	if (val & WSTR_WDOG) {
-		reset_source_set(RESET_WDG);
+		reset_source_set_priority(RESET_WDG, priority);
 		return;
 	}
 
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: i.MX: When SRSR shows wdog then lookup reset source in wdog

[Andrey Smirnov]

On Thu, Nov 15, 2018 at 6:15 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On i.MX6 when the watchdog has resetted the system then the SRSR
> register correctly shows that the watchdog has resetted the system.
> This is not the desired result though, a "reset" in barebox or "reboot"
> in Linux should result in "RST" as reset source. So instead of making
> the SRSR register value overwrite the reset source read from the
> watchdog registers, interpret the SRSR value corresponding to watchdog
> reset as "lookup details in the watchdog registers".
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/mach-imx/imx.c  |  6 +++---
>  drivers/watchdog/imxwd.c | 10 +++++++---
>  2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-imx/imx.c b/arch/arm/mach-imx/imx.c
> index ad227663dd..f87dd76c75 100644
> --- a/arch/arm/mach-imx/imx.c
> +++ b/arch/arm/mach-imx/imx.c
> @@ -204,9 +204,9 @@ void imx_set_reset_reason(void __iomem *srsr,
>          * sure we'll always override info from watchdog driver.
>          */
>         reset_source_set_priority(type,
> -                                 RESET_SOURCE_DEFAULT_PRIORITY + 1);
> +                                 RESET_SOURCE_DEFAULT_PRIORITY);
>         reset_source_set_instance(type, instance);
>
> -       pr_info("i.MX reset reason %s (SRSR: 0x%08x)\n",
> -               reset_source_name(), reg);
> +       pr_debug("i.MX reset reason %s (SRSR: 0x%08x)\n",
> +                reset_source_name(), reg);

Can we keep this at pr_info level?

>  }
> diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
> index a66fae400c..8dba662392 100644
> --- a/drivers/watchdog/imxwd.c
> +++ b/drivers/watchdog/imxwd.c
> @@ -162,19 +162,23 @@ static void __noreturn imxwd_force_soc_reset(struct restart_handler *rst)
>  static void imx_watchdog_detect_reset_source(struct imx_wd *priv)
>  {
>         u16 val = readw(priv->base + IMX21_WDOG_WSTR);
> +       int priority = RESET_SOURCE_DEFAULT_PRIORITY;
> +
> +       if (reset_source_get() == RESET_WDG)
> +               priority++;
>
>         if (val & WSTR_COLDSTART) {
> -               reset_source_set(RESET_POR);
> +               reset_source_set_priority(RESET_POR, priority);
>                 return;
>         }
>
>         if (val & (WSTR_HARDRESET | WSTR_WARMSTART)) {
> -               reset_source_set(RESET_RST);
> +               reset_source_set_priority(RESET_RST, priority);
>                 return;
>         }
>
>         if (val & WSTR_WDOG) {
> -               reset_source_set(RESET_WDG);
> +               reset_source_set_priority(RESET_WDG, priority);
>                 return;
>         }
>
> --
> 2.19.1
>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: i.MX: When SRSR shows wdog then lookup reset source in wdog

[Sascha Hauer]

On Thu, Nov 15, 2018 at 08:16:09AM -0800, Andrey Smirnov wrote:
> On Thu, Nov 15, 2018 at 6:15 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On i.MX6 when the watchdog has resetted the system then the SRSR
> > register correctly shows that the watchdog has resetted the system.
> > This is not the desired result though, a "reset" in barebox or "reboot"
> > in Linux should result in "RST" as reset source. So instead of making
> > the SRSR register value overwrite the reset source read from the
> > watchdog registers, interpret the SRSR value corresponding to watchdog
> > reset as "lookup details in the watchdog registers".
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  arch/arm/mach-imx/imx.c  |  6 +++---
> >  drivers/watchdog/imxwd.c | 10 +++++++---
> >  2 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/imx.c b/arch/arm/mach-imx/imx.c
> > index ad227663dd..f87dd76c75 100644
> > --- a/arch/arm/mach-imx/imx.c
> > +++ b/arch/arm/mach-imx/imx.c
> > @@ -204,9 +204,9 @@ void imx_set_reset_reason(void __iomem *srsr,
> >          * sure we'll always override info from watchdog driver.
> >          */
> >         reset_source_set_priority(type,
> > -                                 RESET_SOURCE_DEFAULT_PRIORITY + 1);
> > +                                 RESET_SOURCE_DEFAULT_PRIORITY);
> >         reset_source_set_instance(type, instance);
> >
> > -       pr_info("i.MX reset reason %s (SRSR: 0x%08x)\n",
> > -               reset_source_name(), reg);
> > +       pr_debug("i.MX reset reason %s (SRSR: 0x%08x)\n",
> > +                reset_source_name(), reg);
> 
> Can we keep this at pr_info level?

There are other places setting the reset source like the watchdog
introduced here and the da9053 driver which is used on some Phytec
boards. I think it's confusing that we get the message the reset reason
is something and when you look at $global.system.reset later it's
something else.

I'll leave the pr_info in for now, but I think this leaves room for
improvements.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: i.MX: When SRSR shows wdog then lookup reset source in wdog

[Marc Kleine-Budde]

[-- Attachment #1.1.1: Type: text/plain, Size: 2807 bytes --]

On 11/15/18 3:15 PM, Sascha Hauer wrote:
> On i.MX6 when the watchdog has resetted the system then the SRSR
> register correctly shows that the watchdog has resetted the system.
> This is not the desired result though, a "reset" in barebox or "reboot"
> in Linux should result in "RST" as reset source. So instead of making
> the SRSR register value overwrite the reset source read from the
> watchdog registers, interpret the SRSR value corresponding to watchdog
> reset as "lookup details in the watchdog registers".
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/mach-imx/imx.c  |  6 +++---
>  drivers/watchdog/imxwd.c | 10 +++++++---
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/imx.c b/arch/arm/mach-imx/imx.c
> index ad227663dd..f87dd76c75 100644
> --- a/arch/arm/mach-imx/imx.c
> +++ b/arch/arm/mach-imx/imx.c
> @@ -204,9 +204,9 @@ void imx_set_reset_reason(void __iomem *srsr,
>  	 * sure we'll always override info from watchdog driver.
>  	 */

With this change, the above comment doesn't describe the reality anymore.

>  	reset_source_set_priority(type,
> -				  RESET_SOURCE_DEFAULT_PRIORITY + 1);
> +				  RESET_SOURCE_DEFAULT_PRIORITY);
>  	reset_source_set_instance(type, instance);
>  
> -	pr_info("i.MX reset reason %s (SRSR: 0x%08x)\n",
> -		reset_source_name(), reg);
> +	pr_debug("i.MX reset reason %s (SRSR: 0x%08x)\n",
> +		 reset_source_name(), reg);
>  }
> diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
> index a66fae400c..8dba662392 100644
> --- a/drivers/watchdog/imxwd.c
> +++ b/drivers/watchdog/imxwd.c
> @@ -162,19 +162,23 @@ static void __noreturn imxwd_force_soc_reset(struct restart_handler *rst)
>  static void imx_watchdog_detect_reset_source(struct imx_wd *priv)
>  {
>  	u16 val = readw(priv->base + IMX21_WDOG_WSTR);
> +	int priority = RESET_SOURCE_DEFAULT_PRIORITY;
> +
> +	if (reset_source_get() == RESET_WDG)
> +		priority++;
>  
>  	if (val & WSTR_COLDSTART) {
> -		reset_source_set(RESET_POR);
> +		reset_source_set_priority(RESET_POR, priority);
>  		return;
>  	}
>  
>  	if (val & (WSTR_HARDRESET | WSTR_WARMSTART)) {
> -		reset_source_set(RESET_RST);
> +		reset_source_set_priority(RESET_RST, priority);
>  		return;
>  	}
>  
>  	if (val & WSTR_WDOG) {
> -		reset_source_set(RESET_WDG);
> +		reset_source_set_priority(RESET_WDG, priority);
>  		return;
>  	}
>  
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: i.MX: When SRSR shows wdog then lookup reset source in wdog

[Sascha Hauer]

On Fri, Nov 16, 2018 at 03:22:59PM +0100, Marc Kleine-Budde wrote:
> On 11/15/18 3:15 PM, Sascha Hauer wrote:
> > On i.MX6 when the watchdog has resetted the system then the SRSR
> > register correctly shows that the watchdog has resetted the system.
> > This is not the desired result though, a "reset" in barebox or "reboot"
> > in Linux should result in "RST" as reset source. So instead of making
> > the SRSR register value overwrite the reset source read from the
> > watchdog registers, interpret the SRSR value corresponding to watchdog
> > reset as "lookup details in the watchdog registers".
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  arch/arm/mach-imx/imx.c  |  6 +++---
> >  drivers/watchdog/imxwd.c | 10 +++++++---
> >  2 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/imx.c b/arch/arm/mach-imx/imx.c
> > index ad227663dd..f87dd76c75 100644
> > --- a/arch/arm/mach-imx/imx.c
> > +++ b/arch/arm/mach-imx/imx.c
> > @@ -204,9 +204,9 @@ void imx_set_reset_reason(void __iomem *srsr,
> >  	 * sure we'll always override info from watchdog driver.
> >  	 */
> 
> With this change, the above comment doesn't describe the reality anymore.

Right, thanks. I removed it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox