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

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

quoted

-----Original Message-----
From: Marco Felsch <redacted>
Sent: Wednesday, August 24, 2022 5:06 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

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

quoted

quoted

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.

You don't have to according the RM:
8<----------------------------------------------------------------------
59.5.2 Disable Watchdog after Reset

All of watchdog registers are unlocked by reset. Therefore, unlock sequence is
unnecessary, but it needs to write all of watchdog registers to make the new
configuration take effect. The code snippet below shows an example of
disabling watchdog after reset.
8<----------------------------------------------------------------------

also the RM tells us:
8<----------------------------------------------------------------------
59.4.3.1 Configuring the Watchdog Once

The new configuration takes effect only after all registers except CNT are
written after reset. Otherwise, the WDOG uses the reset values by default. If
window mode is not used (CS[WIN] is 0), writing to WIN is not required to
make the new configuration take effect.
8<----------------------------------------------------------------------

quoted

Support for 32-bit unlock command write words in enabled in
imx7ulp_wdt_init now.

So.. after reading the IMX7ULP RM, which was not my intention, I found out
that most of the WDOG_CS regiter bits are write-once bits. This means if you
didn't set it within the bootloader you still in case "59.4.3.1".

So the imx7ulp_wdt_init() function just needs to check if the
WDOG_CS_UPDATE bit was set. If it is not the case, then you need to write the
WDOG_CS register as currently done. If the bit is set, than you need know that
the bootloader did the job for you and you can exit
imx7ulp_wdt_init() early. In both cases the unlock is not required.

Can you please check/test if this is working for you?

Regards,
  Marco

Hi Marco,

Rom code has already configured the WDOG once, so we cannot use "
Configuring the Watchdog Once".

What? How does the ROM code configure the WDOG? Also this would be worth a
comment within the code. Also still assume that this "16bit unlock" seq.
is useless since you writing 32bit anyway.

Regards,
  Marco

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