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

-----Original Message-----
From: Marco Felsch <redacted>
Sent: Wednesday, August 24, 2022 4:04 PM
To: Alice Guo (OSS) <redacted>
Cc: Guenter Roeck <linux@roeck-us.net>; wim@linux-watchdog.org;
shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
dl-linux-imx [off-list ref]; kernel@pengutronix.de;
linux-watchdog@vger.kernel.org
Subject: 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

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.

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

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

Hi Marco,

Sorry, I did not tell you that all watchdog control bits, timeout value, and window value cannot be set until the watchdog is unlocked. Support for 32-bit unlock command write words in enabled in imx7ulp_wdt_init now.

Best Regards,
Alice Guo




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