diff mbox

[U-Boot] ls102xa: Fix reset hang

Message ID 1443792315-18997-1-git-send-email-festevam@gmail.com
State Superseded
Delegated to: Stefano Babic
Headers show

Commit Message

Fabio Estevam Oct. 2, 2015, 1:25 p.m. UTC 
From: Fabio Estevam <fabio.estevam@freescale.com>

Since commit 623d96e89aca6("imx: wdog: correct wcr register settings")
issuing a 'reset' command causes the system to hang.

Unlike i.MX and Vybrid, the watchdog controller on LS102x is big-endian.

This means that the watchdog on LS1021 has been working by accident as
it does not use the big-endian accessors in drivers/watchdog/imx_watchdog.c.
Commit 623d96e89aca6("imx: wdog: correct wcr register settings") only
revelead the endianness problem on LS102x.

In order to fix the reset hang, introduce a reset_cpu() implementation that 
is specific for ls102x, which accesses the watchdog WCR register in big-endian
format. All that is required to reset LS102x is to clear the SRS bit.

Reported-by: Sinan Akman <sinan@writeme.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/cpu/armv7/ls102xa/cpu.c | 21 +++++++++++++++++++++
 drivers/watchdog/Makefile        |  2 +-
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Sinan Akman Oct. 2, 2015, 2:21 p.m. UTC | #1 
Hi Fabio

On 02/10/15 09:25 AM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> Since commit 623d96e89aca6("imx: wdog: correct wcr register settings")
> issuing a 'reset' command causes the system to hang.
>
> Unlike i.MX and Vybrid, the watchdog controller on LS102x is big-endian.
>
> This means that the watchdog on LS1021 has been working by accident as
> it does not use the big-endian accessors in drivers/watchdog/imx_watchdog.c.
> Commit 623d96e89aca6("imx: wdog: correct wcr register settings") only
> revelead the endianness problem on LS102x.
>
> In order to fix the reset hang, introduce a reset_cpu() implementation that
> is specific for ls102x, which accesses the watchdog WCR register in big-endian
> format. All that is required to reset LS102x is to clear the SRS bit.
>
> Reported-by: Sinan Akman <sinan@writeme.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>   arch/arm/cpu/armv7/ls102xa/cpu.c | 21 +++++++++++++++++++++
>   drivers/watchdog/Makefile        |  2 +-
>   2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/cpu/armv7/ls102xa/cpu.c b/arch/arm/cpu/armv7/ls102xa/cpu.c
> index 8dd95d9..0de6f19 100644
> --- a/arch/arm/cpu/armv7/ls102xa/cpu.c
> +++ b/arch/arm/cpu/armv7/ls102xa/cpu.c
> @@ -13,6 +13,7 @@
>   #include <tsec.h>
>   #include <netdev.h>
>   #include <fsl_esdhc.h>
> +#include <config.h>
>   
>   #include "fsl_epu.h"
>   
> @@ -354,3 +355,23 @@ void smp_kick_all_cpus(void)
>   	asm volatile("sev");
>   }
>   #endif
> +
> +struct watchdog_regs {
> +	u16	wcr;	/* Control */
> +	u16	wsr;	/* Service */
> +	u16	wrsr;	/* Reset Status */
> +};
> +
> +#define WCR_SRS		(1 << 4)
> +void reset_cpu(ulong addr)
> +{
> +	struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
> +
> +	clrbits_be16(&wdog->wcr, WCR_SRS);
> +
> +	while (1) {
> +		/*
> +		 * Let the watchdog trigger
> +		 */
> +	}
> +}
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 9e9cb55..a007ae8 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -7,7 +7,7 @@
>   
>   obj-$(CONFIG_AT91SAM9_WATCHDOG) += at91sam9_wdt.o
>   obj-$(CONFIG_FTWDT010_WATCHDOG) += ftwdt010_wdt.o
> -ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610 ls102xa))
> +ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610))
>   obj-y += imx_watchdog.o
>   endif
>   obj-$(CONFIG_S5P)               += s5p_wdt.o

    Tested-by: Sinan Akman <sinan@writeme.com>

    Regards
    Sinan Akman
Fabio Estevam Oct. 2, 2015, 2:34 p.m. UTC | #2 
On Fri, Oct 2, 2015 at 11:21 AM, Sinan Akman <sinan@writeme.com> wrote:

>    Tested-by: Sinan Akman <sinan@writeme.com>

Thanks a lot for your help, Sinan!
Wolfgang Denk Oct. 2, 2015, 2:42 p.m. UTC | #3 
Dear Fabio,

In message <1443792315-18997-1-git-send-email-festevam@gmail.com> you wrote:
> 
...
> Unlike i.MX and Vybrid, the watchdog controller on LS102x is big-endian.
...
> +struct watchdog_regs {
> +	u16	wcr;	/* Control */
> +	u16	wsr;	/* Service */
> +	u16	wrsr;	/* Reset Status */
> +};
> +
> +#define WCR_SRS		(1 << 4)

This belongs to some watchdog (or processor) related header file.

As is, it duplicates code from drivers/watchdog/imx_watchdog.c which
is something we should not do.

> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 9e9cb55..a007ae8 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -7,7 +7,7 @@
>  
>  obj-$(CONFIG_AT91SAM9_WATCHDOG) += at91sam9_wdt.o
>  obj-$(CONFIG_FTWDT010_WATCHDOG) += ftwdt010_wdt.o
> -ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610 ls102xa))
> +ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610))

So this fixes the reset problem for now - but what happens when
someone wants to use the watchdog for real?  Will we create a copy of
drivers/watchdog/imx_watchdog.c  using big-endian accessors?  This
cannot be right?

Best regards,

Wolfgang Denk
Sinan Akman Oct. 2, 2015, 2:51 p.m. UTC | #4 
Hi Wolfgang

On 02/10/15 10:42 AM, Wolfgang Denk wrote:
> Dear Fabio,
>
> In message <1443792315-18997-1-git-send-email-festevam@gmail.com> you wrote:
> ...
>> Unlike i.MX and Vybrid, the watchdog controller on LS102x is big-endian.
> ...
>> +struct watchdog_regs {
>> +	u16	wcr;	/* Control */
>> +	u16	wsr;	/* Service */
>> +	u16	wrsr;	/* Reset Status */
>> +};
>> +
>> +#define WCR_SRS		(1 << 4)
> This belongs to some watchdog (or processor) related header file.
>
> As is, it duplicates code from drivers/watchdog/imx_watchdog.c which
> is something we should not do.
>
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 9e9cb55..a007ae8 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -7,7 +7,7 @@
>>   
>>   obj-$(CONFIG_AT91SAM9_WATCHDOG) += at91sam9_wdt.o
>>   obj-$(CONFIG_FTWDT010_WATCHDOG) += ftwdt010_wdt.o
>> -ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610 ls102xa))
>> +ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610))
> So this fixes the reset problem for now - but what happens when
> someone wants to use the watchdog for real?  Will we create a copy of
> drivers/watchdog/imx_watchdog.c  using big-endian accessors?  This
> cannot be right?

   I don't know if you've seen my earlier e-mail on this but I was 
suggesting
to bring watchdog to DM and then consider endian type from dts to implement
it properly. Would this not ultimately be the right solution ?

   Regards
   Sinan Akman
>
> Best regards,
>
> Wolfgang Denk
>
Sinan Akman Oct. 2, 2015, 2:55 p.m. UTC | #5 
On 02/10/15 10:34 AM, Fabio Estevam wrote:
> On Fri, Oct 2, 2015 at 11:21 AM, Sinan Akman <sinan@writeme.com> wrote:
>
>>     Tested-by: Sinan Akman <sinan@writeme.com>
> Thanks a lot for your help, Sinan!

   You are very welcome Fabio. Let's take a cleaner look
at this for 2016.01 to consider all the mixed endian type
peripheral blocks.

   Regards
   Sinan Akman
Fabio Estevam Oct. 2, 2015, 3 p.m. UTC | #6 
On Fri, Oct 2, 2015 at 11:42 AM, Wolfgang Denk <wd@denx.de> wrote:

> So this fixes the reset problem for now - but what happens when
> someone wants to use the watchdog for real?  Will we create a copy of
> drivers/watchdog/imx_watchdog.c  using big-endian accessors?  This
> cannot be right?

If someone wants to use the watchdog for real on LS102x then this is a
new feature that needs to be properly implemented.

In the kernel we have a common watchdog driver for i.MX, Vybrid and
LS, which uses regmap and take the endianness into consideration. We
should make drivers/watchdog/imx_watchdog.c endianness-aware so that
it can work for all these different SoCs. I was not suggesting to
create a new copy of drivers/watchdog/imx_watchdog.c.

Regards,

Fabio Estevam
Tom Rini Oct. 3, 2015, 3:18 p.m. UTC | #7 
On Fri, Oct 02, 2015 at 04:42:14PM +0200, Wolfgang Denk wrote:
> Dear Fabio,
> 
> In message <1443792315-18997-1-git-send-email-festevam@gmail.com> you wrote:
> > 
> ...
> > Unlike i.MX and Vybrid, the watchdog controller on LS102x is big-endian.
> ...
> > +struct watchdog_regs {
> > +	u16	wcr;	/* Control */
> > +	u16	wsr;	/* Service */
> > +	u16	wrsr;	/* Reset Status */
> > +};
> > +
> > +#define WCR_SRS		(1 << 4)
> 
> This belongs to some watchdog (or processor) related header file.
> 
> As is, it duplicates code from drivers/watchdog/imx_watchdog.c which
> is something we should not do.
> 
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 9e9cb55..a007ae8 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -7,7 +7,7 @@
> >  
> >  obj-$(CONFIG_AT91SAM9_WATCHDOG) += at91sam9_wdt.o
> >  obj-$(CONFIG_FTWDT010_WATCHDOG) += ftwdt010_wdt.o
> > -ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610 ls102xa))
> > +ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610))
> 
> So this fixes the reset problem for now - but what happens when
> someone wants to use the watchdog for real?  Will we create a copy of
> drivers/watchdog/imx_watchdog.c  using big-endian accessors?  This
> cannot be right?

Fabio, can you do a v2 that makes the commit message a bit clearer that
this is a temporary work-around and that a proper solution to the
underlying problem is coming?  I think everyone that's reading this
thread knows this but we should make it clear to someone that just picks
up the code / commit (so maybe a comment block too) that we're making
things non-broken for the release but that's not the same thing as
making it correct for the long term.  Thanks!
Fabio Estevam Oct. 7, 2015, 9:43 p.m. UTC | #8 
On Sat, Oct 3, 2015 at 12:18 PM, Tom Rini <trini@konsulko.com> wrote:

> Fabio, can you do a v2 that makes the commit message a bit clearer that
> this is a temporary work-around and that a proper solution to the
> underlying problem is coming?  I think everyone that's reading this
> thread knows this but we should make it clear to someone that just picks
> up the code / commit (so maybe a comment block too) that we're making
> things non-broken for the release but that's not the same thing as
> making it correct for the long term.  Thanks!

Did as suggested in v3.

Into which tree will the patch go in: Yours or York's?
York Sun Oct. 7, 2015, 9:45 p.m. UTC | #9 
On 10/07/2015 02:43 PM, Fabio Estevam wrote:
> On Sat, Oct 3, 2015 at 12:18 PM, Tom Rini <trini@konsulko.com> wrote:
> 
>> Fabio, can you do a v2 that makes the commit message a bit clearer that
>> this is a temporary work-around and that a proper solution to the
>> underlying problem is coming?  I think everyone that's reading this
>> thread knows this but we should make it clear to someone that just picks
>> up the code / commit (so maybe a comment block too) that we're making
>> things non-broken for the release but that's not the same thing as
>> making it correct for the long term.  Thanks!
> 
> Did as suggested in v3.
> 
> Into which tree will the patch go in: Yours or York's?
> 

Please use master, or Tom's tree. My tree is behind and I only update before
requesting a pull.

York
Fabio Estevam Oct. 7, 2015, 9:47 p.m. UTC | #10 
On Wed, Oct 7, 2015 at 6:45 PM, York Sun <yorksun@freescale.com> wrote:

> Please use master, or Tom's tree. My tree is behind and I only update before
> requesting a pull.

I generated the patches against master, so Tom should be able to apply
them cleanly.
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/ls102xa/cpu.c b/arch/arm/cpu/armv7/ls102xa/cpu.c
index 8dd95d9..0de6f19 100644
--- a/arch/arm/cpu/armv7/ls102xa/cpu.c
+++ b/arch/arm/cpu/armv7/ls102xa/cpu.c
@@ -13,6 +13,7 @@ 
 #include <tsec.h>
 #include <netdev.h>
 #include <fsl_esdhc.h>
+#include <config.h>
 
 #include "fsl_epu.h"
 
@@ -354,3 +355,23 @@  void smp_kick_all_cpus(void)
 	asm volatile("sev");
 }
 #endif
+
+struct watchdog_regs {
+	u16	wcr;	/* Control */
+	u16	wsr;	/* Service */
+	u16	wrsr;	/* Reset Status */
+};
+
+#define WCR_SRS		(1 << 4)
+void reset_cpu(ulong addr)
+{
+	struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
+
+	clrbits_be16(&wdog->wcr, WCR_SRS);
+
+	while (1) {
+		/*
+		 * Let the watchdog trigger
+		 */
+	}
+}
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9e9cb55..a007ae8 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -7,7 +7,7 @@ 
 
 obj-$(CONFIG_AT91SAM9_WATCHDOG) += at91sam9_wdt.o
 obj-$(CONFIG_FTWDT010_WATCHDOG) += ftwdt010_wdt.o
-ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610 ls102xa))
+ifneq (,$(filter $(SOC), mx31 mx35 mx5 mx6 mx7 vf610))
 obj-y += imx_watchdog.o
 endif
 obj-$(CONFIG_S5P)               += s5p_wdt.o