ls102xa: Fix reset hang

PATCH New Fabio Estevam 5 participants 11 messages Oct 2, 2015 – Oct 7, 2015

Subsystems (2)

Arm (32-bit) Watchdog device drivers

lore

Share & tools

lore

Fabio Estevam

author

2015-10-02 13:25 UTC #0 lore raw

PATCH 1/1 ls102xa: Fix reset hang

From: Fabio Estevam <[email protected]>

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 <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

---
 arch/arm/cpu/armv7/ls102xa/cpu.c | 21 +++++++++++++++++++++
 drivers/watchdog/Makefile        |  2 +-
 2 files changed, 22 insertions(+), 1 deletion(-)

arch/arm/cpu/armv7/ls102xa/cpu.c +21 -0

index 8dd95d9..0de6f19 100644
@@ -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
+		 */
+	}
+}

drivers/watchdog/Makefile +1 -1

index 9e9cb55..a007ae8 100644
@@ -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

Signature · 1 line

1.9.1

Sinan Akman

2015-10-02 14:21 UTC #1 lore raw

PATCH 1/1 ls102xa: Fix reset hang

Hi Fabio

Quoted 64 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
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 <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>
---
  arch/arm/cpu/armv7/ls102xa/cpu.c | 21 +++++++++++++++++++++
  drivers/watchdog/Makefile        |  2 +-
  2 files changed, 22 insertions(+), 1 deletion(-)
arch/arm/cpu/armv7/ls102xa/cpu.c +21 -0
index 8dd95d9..0de6f19 100644
@@ -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
+		 */
+	}
+}
drivers/watchdog/Makefile +0 -0
index 9e9cb55..a007ae8 100644

drivers/watchdog/Makefile line 13

Quoted 8 lines

drivers/watchdog/Makefile +1 -1

@@ -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 <[email protected]>

Regards
Sinan Akman

Fabio Estevam

author

2015-10-02 14:34 UTC #2 lore raw

PATCH 1/1 ls102xa: Fix reset hang

Quoted 1 line · Sinan Akman

Tested-by Sinan Akman <[email protected]>

Thanks a lot for your help, Sinan!

Sinan Akman

2015-10-02 14:55 UTC #3 lore raw

PATCH 1/1 ls102xa: Fix reset hang

Quoted 4 lines · Fabio Estevam

Quoted 1 line · Sinan Akman
Tested-by Sinan Akman <[email protected]>
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

Wolfgang Denk

2015-10-02 14:42 UTC #4 lore raw

PATCH 1/1 ls102xa: Fix reset hang

In reply to Fabio Estevam

Dear Fabio,

Quoted 1 line · In message <[email protected]> you wrote:

...

Quoted 1 line

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

...

arch/arm/cpu/armv7/ls102xa/cpu.c void smp_kick_all_cpus(void) · line 365

Quoted 7 lines

arch/arm/cpu/armv7/ls102xa/cpu.c +7 -0

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

drivers/watchdog/Makefile line 10

Quoted 10 lines

drivers/watchdog/Makefile +1 -1

index 9e9cb55..a007ae8 100644
@@ -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

Signature · 5 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The only person who always got his work done by Friday
                                                 was Robinson Crusoe.

Sinan Akman

2015-10-02 14:51 UTC #5 lore raw

PATCH 1/1 ls102xa: Fix reset hang

Hi Wolfgang

Quoted 24 lines · Wolfgang Denk

Dear Fabio,
In message <[email protected]> you wrote:
...
Quoted 1 line
Unlike i.MX and Vybrid, the watchdog controller on LS102x is big-endian.
...
arch/arm/cpu/armv7/ls102xa/cpu.c void smp_kick_all_cpus(void) · line 365
Quoted 7 lines
arch/arm/cpu/armv7/ls102xa/cpu.c +7 -0
+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.
Quoted 6 lines
drivers/watchdog/Makefile +0 -0
index 9e9cb55..a007ae8 100644
@@ -7,7 +7,7 @@
 

Quoted 8 lines

drivers/watchdog/Makefile line 10
Quoted 4 lines
drivers/watchdog/Makefile +1 -1
  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

Quoted 5 lines

Best regards,
Wolfgang Denk

Fabio Estevam

author

2015-10-02 15:00 UTC #6 lore raw

PATCH 1/1 ls102xa: Fix reset hang

In reply to Wolfgang Denk

Quoted 4 lines · Wolfgang Denk

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

2015-10-03 15:18 UTC #7 lore raw

PATCH 1/1 ls102xa: Fix reset hang

In reply to Wolfgang Denk

Quoted 26 lines · Wolfgang Denk

Dear Fabio,
Quoted 1 line · In message <[email protected]> you wrote:
...
Quoted 1 line
Unlike i.MX and Vybrid, the watchdog controller on LS102x is big-endian.
...
arch/arm/cpu/armv7/ls102xa/cpu.c void smp_kick_all_cpus(void) · line 365
Quoted 7 lines
arch/arm/cpu/armv7/ls102xa/cpu.c +7 -0
+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.
Quoted 6 lines
drivers/watchdog/Makefile +0 -0
index 9e9cb55..a007ae8 100644
@@ -7,7 +7,7 @@
 

Quoted 9 lines

drivers/watchdog/Makefile line 10
Quoted 4 lines
drivers/watchdog/Makefile +1 -1
 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!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151003/8d2a2e20/attachment-0001.sig>

Fabio Estevam

author

2015-10-07 21:43 UTC #8 lore raw

PATCH 1/1 ls102xa: Fix reset hang

Quoted 7 lines · Tom Rini

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

2015-10-07 21:45 UTC #9 lore raw

PATCH 1/1 ls102xa: Fix reset hang

Quoted 10 lines · Fabio Estevam

Quoted 7 lines · Tom Rini
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!

Quoted 4 lines

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

author

2015-10-07 21:47 UTC #10 lore raw

PATCH 1/1 ls102xa: Fix reset hang

Quoted 2 lines · York Sun

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.