Revert "imx: wdog: correct wcr register settings"

From: Fabio Estevam <[email protected]>

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

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

---
  drivers/watchdog/imx_watchdog.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

drivers/watchdog/imx_watchdog.c +0 -0

index 9a77a54..1d18d4b 100644

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.

Hi Sinan,

Quoted 2 lines · Sinan Akman

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.

Quoted 8 lines

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

Unlike i.MX, LS1021 uses big-endian ordering for the watchdog controller, which means we cannot use the little endian accessors.

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 ?

Quoted 4 lines

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?

Quoted 17 lines · Wolfgang Denk

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.

Quoted 2 lines

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 mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 3 lines

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.

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?

Quoted 3 lines · Wolfgang Denk

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?

Quoted 7 lines · Fabio Estevam

Quoted 3 lines · Wolfgang Denk

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.

Quoted 7 lines

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?

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

@@ -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 */

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

+
+    out_be16(&wdog->wsr, 0x5555);
+    out_be16(&wdog->wsr, 0xaaaa);    /* load minimum 1/2 second timeout */
+    while (1) {
+        /*
+         * spin for .5 seconds before reset
+         */
+    }
+}

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

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.