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

[Alice Guo (OSS)]

> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Monday, August 22, 2022 10:04 PM
> To: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> Cc: Alice Guo (OSS) <alice.guo@xxxxxxxxxxx>; 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
> 
> 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 <m.felsch@xxxxxxxxxxxxxx>
> > > > Sent: Tuesday, August 16, 2022 2:24 PM
> > > > To: Alice Guo (OSS) <alice.guo@xxxxxxxxxxx>
> > > > Cc: wim@xxxxxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx;
> > > > 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
> > > >
> > > > On 22-08-16, Alice Guo (OSS) wrote:
> > > > > From: Jacky Bai <ping.bai@xxxxxxx>
> > > > >
> > > > > 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.

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.

Best Regards,
Alice Guo

> > > Best Regards,
> > > Alice Guo
> > >
> > > >
> > > > >
> > > > > Suggested-by: Ye Li <ye.li@xxxxxxx>
> > > > > Signed-off-by: Jacky Bai <ping.bai@xxxxxxx>
> > > > > Signed-off-by: Alice Guo <alice.guo@xxxxxxx>
> > > > > Reviewed-by: Ye Li <ye.li@xxxxxxx>
> > > > > ---
> > > > >  drivers/watchdog/imx7ulp_wdt.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/drivers/watchdog/imx7ulp_wdt.c
> > > > > b/drivers/watchdog/imx7ulp_wdt.c index
> > > > > 014f497ea0dc..b8ac0cb04d2f
> > > > > 100644
> > > > > --- a/drivers/watchdog/imx7ulp_wdt.c
> > > > > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > > > > @@ -179,9 +179,13 @@ static int imx7ulp_wdt_init(void __iomem
> > > > > *base,
> > > > unsigned int timeout)
> > > > >  	int ret;
> > > > >
> > > > >  	local_irq_disable();
> > > > > +
> > > > > +	mb();
> > > > >  	/* unlock the wdog for reconfiguration */
> > > > >  	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > > > >  	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > > > > +	mb();
> > > > > +
> > > > >  	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
> > > > >  	if (ret)
> > > > >  		goto init_out;
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > >
> > > > >
> > >