[U-Boot] [PATCH] ls102xa: Fix reset hang
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
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
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!
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
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
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
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
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!
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?
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
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.
participants (5)
-
Fabio Estevam -
Sinan Akman -
Tom Rini -
Wolfgang Denk -
York Sun