[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"
From: Fabio Estevam fabio.estevam@freescale.com
This reverts commit 623d96e89aca64c2762150087f4e872c55481f13.
commit 623d96e89aca6("imx: wdog: correct wcr register settings") introduced the usage of clrsetbits_le16(), which causes a regression on LS1021 systems.
Unlike i.MX, LS1021 uses big-endian ordering for the watchdog controller, which means we cannot use the little endian accessors.
Reported-by: Sinan Akman sinan@writeme.com Signed-off-by: Fabio Estevam fabio.estevam@freescale.com --- drivers/watchdog/imx_watchdog.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/watchdog/imx_watchdog.c b/drivers/watchdog/imx_watchdog.c index 9a77a54..1d18d4b 100644 --- a/drivers/watchdog/imx_watchdog.c +++ b/drivers/watchdog/imx_watchdog.c @@ -55,8 +55,7 @@ void reset_cpu(ulong addr) { struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
- clrsetbits_le16(&wdog->wcr, 0, WCR_WDE); - + writew(WCR_WDE, &wdog->wcr); writew(0x5555, &wdog->wsr); writew(0xaaaa, &wdog->wsr); /* load minimum 1/2 second timeout */ while (1) {
On 01/10/15 03:32 PM, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
This reverts commit 623d96e89aca64c2762150087f4e872c55481f13.
commit 623d96e89aca6("imx: wdog: correct wcr register settings") introduced the usage of clrsetbits_le16(), which causes a regression on LS1021 systems.
Unlike i.MX, LS1021 uses big-endian ordering for the watchdog controller, which means we cannot use the little endian accessors.
Reported-by: Sinan Akman sinan@writeme.com Signed-off-by: Fabio Estevam fabio.estevam@freescale.com
drivers/watchdog/imx_watchdog.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/watchdog/imx_watchdog.c b/drivers/watchdog/imx_watchdog.c index 9a77a54..1d18d4b 100644 --- a/drivers/watchdog/imx_watchdog.c +++ b/drivers/watchdog/imx_watchdog.c @@ -55,8 +55,7 @@ void reset_cpu(ulong addr) { struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
- clrsetbits_le16(&wdog->wcr, 0, WCR_WDE);
- writew(WCR_WDE, &wdog->wcr); writew(0x5555, &wdog->wsr); writew(0xaaaa, &wdog->wsr); /* load minimum 1/2 second timeout */ while (1) {
Hi Fabio, I just wanted to point out that with this revert we don't only break imx again (whatever the initial bug was for this commit) but also we are having ls1021atwr working accidentally, just because writew(WCR_WDE, &wdog->wcr) clears SRS bit which is the only requirement for reset if watchdog is not running.
I don't have any strong opinion on this but i just wanted to make it clear that we are leaving both imx6 and ls1021atwr not properly implemented.
Regards Sinan Akman
Hi Sinan,
On Thu, Oct 1, 2015 at 4:39 PM, Sinan Akman sinan@writeme.com wrote:
Hi Fabio, I just wanted to point out that with this revert we don't only break imx again
We are not breaking imx by doing the revert. The reset still works. 623d96e89aca64c2 appeared only in 2015.10-rc4.
(whatever the initial bug was for this commit) but also we are having ls1021atwr working accidentally, just because writew(WCR_WDE, &wdog->wcr) clears SRS bit which is the only requirement for reset if watchdog is not running.
I don't have any strong opinion on this but i just wanted to make it clear that we are leaving both imx6 and ls1021atwr not properly implemented.
I think it is the best/safest we can do at -rc4 to avoid the reset regression on ls1021.
Then a proper implementation can be done for 2016.01.
Do you agree?
Regards,
Fabio Estevam
On 01/10/15 03:45 PM, Fabio Estevam wrote:
Hi Sinan,
On Thu, Oct 1, 2015 at 4:39 PM, Sinan Akman sinan@writeme.com wrote:
Hi Fabio, I just wanted to point out that with this revert we don't only break imx again
We are not breaking imx by doing the revert. The reset still works. 623d96e89aca64c2 appeared only in 2015.10-rc4.
(whatever the initial bug was for this commit) but also we are having ls1021atwr working accidentally, just because writew(WCR_WDE, &wdog->wcr) clears SRS bit which is the only requirement for reset if watchdog is not running.
I don't have any strong opinion on this but i just wanted to make it clear that we are leaving both imx6 and ls1021atwr not properly implemented.
I think it is the best/safest we can do at -rc4 to avoid the reset regression on ls1021.
Then a proper implementation can be done for 2016.01.
Do you agree?
Hi Fabio, yes this seems to be the best thing to do for now. Let's implement then this thing properly soon after.
Thanks Sinan Akman
On Thu, Oct 1, 2015 at 4:50 PM, Sinan Akman sinan@writeme.com wrote:
Hi Fabio, yes this seems to be the best thing to do for now. Let's implement then this thing properly soon after.
Could you please reply with a Tested-by tag?
Thanks
Dear Fabio,
In message 1443727970-10347-1-git-send-email-festevam@gmail.com you wrote:
Unlike i.MX, LS1021 uses big-endian ordering for the watchdog controller, which means we cannot use the little endian accessors.
...
- clrsetbits_le16(&wdog->wcr, 0, WCR_WDE);
- writew(WCR_WDE, &wdog->wcr);
I'm sorry, but I fail to understand how writew() can be better than another I/O accessor. Neither of these has the capability to detect the endianess of this specific register interface ?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Thu, Oct 1, 2015 at 5:11 PM, Wolfgang Denk wd@denx.de wrote:
I'm sorry, but I fail to understand how writew() can be better than another I/O accessor. Neither of these has the capability to detect the endianess of this specific register interface ?
It's not that writew() is better. The problem is that we cannot use clrsetbits_le16() for a big endian controller.
Regards,
Fabio Estevam
Dear Fabio,
In message CAOMZO5BEfS10XVztnigMejMVJYLvv+jqDLZYom9K8-G+Zi1TXA@mail.gmail.com you wrote:
I'm sorry, but I fail to understand how writew() can be better than another I/O accessor. Neither of these has the capability to detect the endianess of this specific register interface ?
It's not that writew() is better. The problem is that we cannot use clrsetbits_le16() for a big endian controller.
On ARM (a LE architecture), clrsetbits_le16() maps down into:
clrsetbits_le16 -> out_le16 / in_le16 -> out_arch, w,le16 / in_arch, w,le16 -> __raw_writew(cpu_to_le16()) / le16__to_cpu(__raw_readw()) -> __raw_writew() / __raw_readw()
while
writew() -> __raw_writew(cpu_to_le16(v),__mem_pci(c)) __raw_writew()
Both map into __raw_writew() [which then boild down into __arch_putw() which is just a volatile unsigned short write access.
So both clrsetbits_le16() and writew() are little endian accessors. In which way would one write other data to the device than the other?
Best regards,
Wolfgang Denk
On Thu, Oct 1, 2015 at 5:50 PM, Wolfgang Denk wd@denx.de wrote:
On ARM (a LE architecture), clrsetbits_le16() maps down into:
clrsetbits_le16 -> out_le16 / in_le16 -> out_arch, w,le16 / in_arch, w,le16 -> __raw_writew(cpu_to_le16()) / le16__to_cpu(__raw_readw()) -> __raw_writew() / __raw_readw()while
writew() -> __raw_writew(cpu_to_le16(v),__mem_pci(c)) __raw_writew()Both map into __raw_writew() [which then boild down into __arch_putw() which is just a volatile unsigned short write access.
So both clrsetbits_le16() and writew() are little endian accessors. In which way would one write other data to the device than the other?
Yes, you are right.
The issue seems to be related to the effect of writing doing 'writew(WCR_WDE, &wdog->wcr)', which writes 0x40 to the register WCR versus 'clrsetbits_le16(&wdog->wcr, 0, WCR_WDE)' which only enables the WDE bit.
The kernel driver also does the full write to the WCR register(like we used to have prior to 623d96e89aca6) https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/dr...
This has also the effect of clearing the SRS bit.
By the way we need to implement erratum ERR004346 (WDOG: WDOG SRS bit requires to be written twice) in U-boot, but this is an unrelated issue.
So the revert seems to be appropriate. The commit log should be adjusted though.
Regards,
Fabio Estevam
On 01/10/15 07:11 PM, Fabio Estevam wrote:
On Thu, Oct 1, 2015 at 5:50 PM, Wolfgang Denk wd@denx.de wrote:
On ARM (a LE architecture), clrsetbits_le16() maps down into:
clrsetbits_le16 -> out_le16 / in_le16 -> out_arch, w,le16 / in_arch, w,le16 -> __raw_writew(cpu_to_le16()) / le16__to_cpu(__raw_readw()) -> __raw_writew() / __raw_readw()while
writew() -> __raw_writew(cpu_to_le16(v),__mem_pci(c)) __raw_writew()Both map into __raw_writew() [which then boild down into __arch_putw() which is just a volatile unsigned short write access.
So both clrsetbits_le16() and writew() are little endian accessors. In which way would one write other data to the device than the other?
Yes, you are right.
The issue seems to be related to the effect of writing doing 'writew(WCR_WDE, &wdog->wcr)', which writes 0x40 to the register WCR versus 'clrsetbits_le16(&wdog->wcr, 0, WCR_WDE)' which only enables the WDE bit.
Unfortunately, I believe this is not exactly true. After the revert with writew(WCR_WDE, &wdog->wcr) we are not really writing 0x4 or setting WDE but we are writing 0x0400 and setting the WT bits (wcr[8:15] to 0x04 and this is accidentally also clearing the SRS bit. In addition, even if it was setting the WDE correctly it wouldn't be relevant to ls1021atwr as we are not setting the timeout.
Bottom line is the code is broken for ls1021 both before and after the revert and it just happens that the broken code after the revert (with writew) clears a bit we didn't intend to and that generates a wdog_rst so it hides the real bug. The correct implementation would be clearing the SRS bit via an _be16 operation.
Anyhow, let's move on with this revert if you all agree with this.
Fabio, I will send you the test-by to your commit but we will have to clean up this mini mess soon after :)
Thanks Sinan Akman
The kernel driver also does the full write to the WCR register(like we used to have prior to 623d96e89aca6) https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/dr...
This has also the effect of clearing the SRS bit.
By the way we need to implement erratum ERR004346 (WDOG: WDOG SRS bit requires to be written twice) in U-boot, but this is an unrelated issue.
So the revert seems to be appropriate. The commit log should be adjusted though.
Regards,
Fabio Estevam _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear Fabio,
In message CAOMZO5CAQXVzpX9mW7uHKB98JnMBeuT3ZjiUe4-V4=adjRk0zg@mail.gmail.com you wrote:
So both clrsetbits_le16() and writew() are little endian accessors. In which way would one write other data to the device than the other?
Yes, you are right.
The issue seems to be related to the effect of writing doing 'writew(WCR_WDE, &wdog->wcr)', which writes 0x40 to the register WCR versus 'clrsetbits_le16(&wdog->wcr, 0, WCR_WDE)' which only enables the WDE bit.
But if we agree that both are LE accessors, and that the register is BE, then how does it work at all - we would be writing the wrong bit?
Best regards,
Wolfgang Denk
On Fri, Oct 2, 2015 at 1:30 AM, Wolfgang Denk wd@denx.de wrote:
In message CAOMZO5CAQXVzpX9mW7uHKB98JnMBeuT3ZjiUe4-V4=adjRk0zg@mail.gmail.com you But if we agree that both are LE accessors, and that the register is BE, then how does it work at all - we would be writing the wrong bit?
Watchdog on LS1021 works by accident rather than by design.
What we are trying to do is to avoid the regression on LS1021 for the 2015.10 release.
Then a proper watchdog driver implementation is needed for 2016.01 so that it takes care of the endianness.
Is this approach acceptable?
Regards,
Fabio Estevam
On Fri, Oct 2, 2015 at 8:10 AM, Fabio Estevam festevam@gmail.com wrote:
On Fri, Oct 2, 2015 at 1:30 AM, Wolfgang Denk wd@denx.de wrote:
In message CAOMZO5CAQXVzpX9mW7uHKB98JnMBeuT3ZjiUe4-V4=adjRk0zg@mail.gmail.com you But if we agree that both are LE accessors, and that the register is BE, then how does it work at all - we would be writing the wrong bit?
Watchdog on LS1021 works by accident rather than by design.
What we are trying to do is to avoid the regression on LS1021 for the 2015.10 release.
Then a proper watchdog driver implementation is needed for 2016.01 so that it takes care of the endianness.
Is this approach acceptable?
Or what about providing a reset_cpu() for LS102x that uses the proper endianness? Would this be a better approach?
Sinan, does it work?
--- 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,25 @@ 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_WDE 0x04 /* WDOG enable */ +void reset_cpu(ulong addr) +{ + struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR; + + out_be16(&wdog->wcr, WCR_WDE); + + out_be16(&wdog->wsr, 0x5555); + out_be16(&wdog->wsr, 0xaaaa); /* load minimum 1/2 second timeout */ + while (1) { + /* + * spin for .5 seconds before reset + */ + } +} 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
On 02/10/15 07:39 AM, Fabio Estevam wrote:
On Fri, Oct 2, 2015 at 8:10 AM, Fabio Estevam festevam@gmail.com wrote:
On Fri, Oct 2, 2015 at 1:30 AM, Wolfgang Denk wd@denx.de wrote:
In message CAOMZO5CAQXVzpX9mW7uHKB98JnMBeuT3ZjiUe4-V4=adjRk0zg@mail.gmail.com you But if we agree that both are LE accessors, and that the register is BE, then how does it work at all - we would be writing the wrong bit?
Watchdog on LS1021 works by accident rather than by design.
What we are trying to do is to avoid the regression on LS1021 for the 2015.10 release.
Then a proper watchdog driver implementation is needed for 2016.01 so that it takes care of the endianness.
Is this approach acceptable?
Or what about providing a reset_cpu() for LS102x that uses the proper endianness? Would this be a better approach?
Sinan, does it work?
--- 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,25 @@ 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_WDE 0x04 /* WDOG enable */ +void reset_cpu(ulong addr) +{
- struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR;
- out_be16(&wdog->wcr, WCR_WDE);
I'll test this little later on when I am in the lab, but why are we setting WCR_WDE anyways. We are not re-setting a new time out value so this should be irrelevant.
All we need is to clear the SRS bit, no need to set WCR_WDE and no 5555/aaaa service sequence. I tested this earlier I know it works. So a correct patch for reset_cpu() for LS102x could be a single line SRS bit clear via _be32 which is all what we are intending to.
Regards Sinan Akman
- out_be16(&wdog->wsr, 0x5555);
- out_be16(&wdog->wsr, 0xaaaa); /* load minimum 1/2 second timeout */
- while (1) {
/** spin for .5 seconds before reset*/- }
+} 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
On Fri, Oct 2, 2015 at 10:04 AM, Sinan Akman sinan@writeme.com wrote:
I'll test this little later on when I am in the lab, but why are we setting WCR_WDE anyways. We are not re-setting a new time out value so this should be irrelevant.
All we need is to clear the SRS bit, no need to set WCR_WDE and no 5555/aaaa service sequence. I tested this earlier I know it works. So a correct patch for reset_cpu() for LS102x could be a single line SRS bit clear via _be32 which is all what we are intending to.
Thanks, just sent a patch using this method.
participants (3)
-
Fabio Estevam -
Sinan Akman -
Wolfgang Denk