[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Fabio Estevam]

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) {
-- 
1.9.1

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Sinan Akman]

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

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Fabio Estevam]

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

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Sinan Akman]

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

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Fabio Estevam]

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

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Wolfgang Denk]

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

-- 
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
As far as the laws of mathematics refer to reality, they are not cer-
tain, and as far as they are certain, they do not refer  to  reality.
                                                   -- Albert Einstein

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Fabio Estevam]

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

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Wolfgang Denk]

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

-- 
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 human race has one really effective weapon, and that is laughter.
                                                         - Mark Twain

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Fabio Estevam]

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/drivers/watchdog/imx2_wdt.c?id=refs/tags/v4.2.2#n86

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] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Sinan Akman]

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/drivers/watchdog/imx2_wdt.c?id=refs/tags/v4.2.2#n86
>
> 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Wolfgang Denk]

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

-- 
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
"Ja, mach' nur einen Plan,    sei nur ein grosses Licht
und mach' dann noch 'nen zweiten Plan,    geh'n tun sie beide nicht."
                                     -- Bert Brecht, Dreigroschenoper

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Fabio Estevam]

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

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[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
-- 
1.9.1

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Sinan Akman]

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

[U-Boot] [PATCH] Revert "imx: wdog: correct wcr register settings"

[Fabio Estevam]

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.