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

From: Marco Felsch <m.felsch@pengutronix.de>
To: "Alice Guo (OSS)" <alice.guo@oss.nxp.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	"wim@linux-watchdog.org" <wim@linux-watchdog.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>
Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for unlock sequence
Date: Wed, 24 Aug 2022 10:03:38 +0200	[thread overview]
Message-ID: <20220824080338.humjny4fabhmx3z7@pengutronix.de> (raw)
In-Reply-To: <AM6PR04MB60535EC5B774004AF996BDA5E2739@AM6PR04MB6053.eurprd04.prod.outlook.com>

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