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

-----Original Message-----
From: Guenter Roeck <redacted> On Behalf Of Guenter Roeck
Sent: Monday, August 22, 2022 10:04 PM
To: Marco Felsch <redacted>
Cc: Alice Guo (OSS) <redacted>; 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

On Mon, Aug 22, 2022 at 10:00:10AM +0200, Marco Felsch wrote:

quoted

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

quoted

quoted

-----Original Message-----
From: Marco Felsch <redacted>
Sent: Tuesday, August 16, 2022 2:24 PM
To: Alice Guo (OSS) <redacted>
Cc: wim@linux-watchdog.org; linux@roeck-us.net;
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

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

quoted

From: Jacky Bai <ping.bai@nxp.com>

Add explict memory barrier for the wdog unlock sequence.

Did you inspected any failures? It's not enough to say what you
did, you need to specify the why as well.

Regards,
  Marco

Hi,

Two 16-bit writes of unlocking the Watchdog should be completed within a

certain time. The first mb() is used to ensure that previous instructions are
completed.

quoted

quoted

The second mb() is used to ensure that the unlock sequence cannot be

affected by subsequent instructions. The reason will be added in the commit
log of v2.

quoted

Hi,

I know what memory barriers are. My question was, did you see any
issues? Since the driver is used mainline and no one reported issues.

Also just don't use the *_relaxed() versions is more common, than
adding
mb() calls around *_relaxed() versions.

Agreed with both. The series is a bit short in explaining _why_ the changes are
made.

Guenter

quoted

Regards,
  Marco

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.

2. Also just don't use the *_relaxed() versions is more common, than adding mb() calls around *_relaxed() versions.
Memory barriers cannot be added between two 16-bit writes. I do not know the reason.

Best Regards,
Alice Guo

quoted

quoted

Best Regards,
Alice Guo

quoted

quoted

Suggested-by: Ye Li <redacted>
Signed-off-by: Jacky Bai <ping.bai@nxp.com>
Signed-off-by: Alice Guo <redacted>
Reviewed-by: Ye Li <redacted>
---
 drivers/watchdog/imx7ulp_wdt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/watchdog/imx7ulp_wdt.c

b/drivers/watchdog/imx7ulp_wdt.c index
014f497ea0dc..b8ac0cb04d2f
100644

--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c

@@ -179,9 +179,13 @@ static int imx7ulp_wdt_init(void __iomem

*base,

unsigned int timeout)

quoted

 	int ret;

 	local_irq_disable();
+
+	mb();
 	/* unlock the wdog for reconfiguration */
 	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
 	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+	mb();
+
 	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
 	if (ret)
 		goto init_out;
--
2.17.1

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