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

On Tue, Aug 23, 2022 at 11:10:27AM +0200, Marco Felsch wrote:

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

quoted

quoted

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

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.

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

quoted

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.

As written above, writel() as well as writel_relaxed() are not 16-bit
access functions.

So to me it looks as you need first to ensure that 32-bit access mode is
enabled. After that you can write drop the to writel_relaxed() functions
and instead just write:

   writel(UNLOCK, wdt->base + WDOG_CNT);

Also why do we need to unlock the watchdog during imx7ulp_wdt_init()?
This is handled just fine during imx7ulp_wdt_enable() and during
imx7ulp_wdt_set_timeout(). So just drop those relaxed writes and
everything should be fine.

Regards,
  Marco

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