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

quoted

-----Original Message-----
From: Guenter Roeck <redacted> On Behalf Of Guenter Roeck
Sent: Tuesday, August 23, 2022 8:02 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 Tue, Aug 23, 2022 at 11:10:27AM +0200, Marco Felsch wrote:
On 22-08-23, Alice Guo (OSS) wrote:

-----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:
On 22-08-22, Alice Guo (OSS) wrote:
-----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:
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.
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.
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

Regards,
  Marco

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

Best Regards,
Alice Guo

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help