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

[Marco Felsch]

Hi Alice,

On 22-08-24, Alice Guo (OSS) wrote:
> > -----Original Message-----
> > From: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> > Sent: Wednesday, August 24, 2022 4:04 PM
> > To: Alice Guo (OSS) <alice.guo@xxxxxxxxxxx>
> > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx;
> > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > dl-linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> > linux-watchdog@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for
> > unlock sequence
> > 
> > Hi Alice,
> > 
> > On 22-08-24, Alice Guo (OSS) wrote:
> > 
> > ...
> > 
> > > > > > 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.
> > 
> > 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.

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

> 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

> Best Regards,
> Alice Guo
> 
> 
> 
>