Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence

Hi Alice,

On 22-08-24, Alice Guo (OSS) wrote:

...

quoted

quoted

quoted

Hi Guenter and Marco,

1. did you see any issues?
This WDOG Timer first appeared in i.MX7ULP, no one report issues
probably because few people use i.MX7ULP. This issue was found when
we did a stress test on it. When we reconfigure the WDOG Timer,
there is a certain probability that it reset. The reason for the
error is that when WDOG_CS[CMD32EN] is 0, the unlock sequence is two
16-bit writes (0xC520, 0xD928) to the CNT register within 16 bus
clocks, and improper unlock sequence causes the WDOG to reset.
Adding mb() is to guarantee that two 16-bit writes are finished within 16

bus clocks.

quoted

After this explanation the whole imx7ulp_wdt_init() seems a bit buggy
because writel_relaxed() as well as writel() are 32bit access functions.
So the very first thing to do is to enable the 32-bit mode.

Agreed. This is much better than having extra code to deal with both 16-bit
and 32-bit access.

quoted

Also this is a explanation worth to be added to the commit message ;)

Definitely. Also, the use of mb(), if it should indeed be needed, would have to
be explained in a code comment.

Thanks,
Guenter

Hi Marco and Guenter,

Thank you for your comments. I plan to enable support for 32-bit
unlock command write words in bootloader. In this way, there is no
need to distinguish whether the unlock command is a 32-bit command or
a 16-bit command in driver.

Please don't move this into the bootloader, enabling it within the init
seq. is just fine. If you move it into the bootloader then you can't
ensure that the bit is set since there are plenty of bootloaders out
there.

As I said, just drop the "16bit" unlock sequence from the init function
because the unlock is handled just fine in all the watchdog_ops.

Regards,
  Marco

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel