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