diff mbox series

[3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog init

Message ID 20220816043643.26569-4-alice.guo@oss.nxp.com
State Superseded
Headers show
Series watchdog: imx7ulp_wdt: update i.MX7ULP WDOG timer driver | expand

Commit Message

Alice Guo (OSS) Aug. 16, 2022, 4:36 a.m. UTC 
From: Ye Li <ye.li@nxp.com>

When bootloader has enabled the CMD32EN bit, switch to use 32bits
unlock command to unlock the CS register. Using 32bits command will
help on avoiding 16 bus cycle window violation for two 16 bits
commands.

Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Alice Guo <alice.guo@nxp.com>
Reviewed-by: Jacky Bai <ping.bai@nxp.com>
Acked-by: Jason Liu <jason.hui.liu@nxp.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Guenter Roeck Aug. 22, 2022, 2:05 p.m. UTC | #1 
On Tue, Aug 16, 2022 at 12:36:39PM +0800, Alice Guo (OSS) wrote:
> From: Ye Li <ye.li@nxp.com>
> 
> When bootloader has enabled the CMD32EN bit, switch to use 32bits
> unlock command to unlock the CS register. Using 32bits command will
> help on avoiding 16 bus cycle window violation for two 16 bits
> commands.
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> ---
>  drivers/watchdog/imx7ulp_wdt.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index b8ac0cb04d2f..a0f6b8cea78f 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -180,11 +180,16 @@ static int imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
>  
>  	local_irq_disable();
>  
> -	mb();
> -	/* unlock the wdog for reconfiguration */
> -	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> -	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> -	mb();
> +	val = readl(base + WDOG_CS);
> +	if (val & WDOG_CS_CMD32EN) {
> +		writel(UNLOCK, base + WDOG_CNT);
> +	} else {
> +		mb();
> +		/* unlock the wdog for reconfiguration */
> +		writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> +		writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> +		mb();

Now this is intermixing writel() with writel_relaxed(), making
the code all but impossible to understand.

Guenter

> +	}
>  
>  	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
>  	if (ret)
> -- 
> 2.17.1
>
Alice Guo (OSS) Aug. 23, 2022, 5:46 a.m. UTC | #2 
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, August 22, 2022 10:06 PM
> To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog
> init
> 
> On Tue, Aug 16, 2022 at 12:36:39PM +0800, Alice Guo (OSS) wrote:
> > From: Ye Li <ye.li@nxp.com>
> >
> > When bootloader has enabled the CMD32EN bit, switch to use 32bits
> > unlock command to unlock the CS register. Using 32bits command will
> > help on avoiding 16 bus cycle window violation for two 16 bits
> > commands.
> >
> > Signed-off-by: Ye Li <ye.li@nxp.com>
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> > Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> > ---
> >  drivers/watchdog/imx7ulp_wdt.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/watchdog/imx7ulp_wdt.c
> > b/drivers/watchdog/imx7ulp_wdt.c index b8ac0cb04d2f..a0f6b8cea78f
> > 100644
> > --- a/drivers/watchdog/imx7ulp_wdt.c
> > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > @@ -180,11 +180,16 @@ static int imx7ulp_wdt_init(void __iomem *base,
> > unsigned int timeout)
> >
> >  	local_irq_disable();
> >
> > -	mb();
> > -	/* unlock the wdog for reconfiguration */
> > -	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > -	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > -	mb();
> > +	val = readl(base + WDOG_CS);
> > +	if (val & WDOG_CS_CMD32EN) {
> > +		writel(UNLOCK, base + WDOG_CNT);
> > +	} else {
> > +		mb();
> > +		/* unlock the wdog for reconfiguration */
> > +		writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > +		writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > +		mb();
> 
> Now this is intermixing writel() with writel_relaxed(), making the code all but
> impossible to understand.
> 
> Guenter

Hi Guenter,

Intermixing writel() with writel_relaxed() is unavoidable here. Because there cannot be a memory barrier between writing UNLOCK_SEQ0 and writing UNLOCK_SEQ1. This may be determined by hardware design.

Best Regards,
Alice Guo
 
> > +	}
> >
> >  	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
> >  	if (ret)
> > --
> > 2.17.1
> >
Guenter Roeck Aug. 23, 2022, 12:05 p.m. UTC | #3 
On Tue, Aug 23, 2022 at 05:46:55AM +0000, Alice Guo (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Monday, August 22, 2022 10:06 PM
> > To: Alice Guo (OSS) <alice.guo@oss.nxp.com>
> > Cc: wim@linux-watchdog.org; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; festevam@gmail.com; kernel@pengutronix.de;
> > dl-linux-imx <linux-imx@nxp.com>; linux-watchdog@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 3/7] watchdog: imx7ulp_wdt: Check CMD32EN in wdog
> > init
> > 
> > On Tue, Aug 16, 2022 at 12:36:39PM +0800, Alice Guo (OSS) wrote:
> > > From: Ye Li <ye.li@nxp.com>
> > >
> > > When bootloader has enabled the CMD32EN bit, switch to use 32bits
> > > unlock command to unlock the CS register. Using 32bits command will
> > > help on avoiding 16 bus cycle window violation for two 16 bits
> > > commands.
> > >
> > > Signed-off-by: Ye Li <ye.li@nxp.com>
> > > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > > Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> > > Acked-by: Jason Liu <jason.hui.liu@nxp.com>
> > > ---
> > >  drivers/watchdog/imx7ulp_wdt.c | 15 ++++++++++-----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/watchdog/imx7ulp_wdt.c
> > > b/drivers/watchdog/imx7ulp_wdt.c index b8ac0cb04d2f..a0f6b8cea78f
> > > 100644
> > > --- a/drivers/watchdog/imx7ulp_wdt.c
> > > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > > @@ -180,11 +180,16 @@ static int imx7ulp_wdt_init(void __iomem *base,
> > > unsigned int timeout)
> > >
> > >  	local_irq_disable();
> > >
> > > -	mb();
> > > -	/* unlock the wdog for reconfiguration */
> > > -	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > > -	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > > -	mb();
> > > +	val = readl(base + WDOG_CS);
> > > +	if (val & WDOG_CS_CMD32EN) {
> > > +		writel(UNLOCK, base + WDOG_CNT);
> > > +	} else {
> > > +		mb();
> > > +		/* unlock the wdog for reconfiguration */
> > > +		writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > > +		writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > > +		mb();
> > 
> > Now this is intermixing writel() with writel_relaxed(), making the code all but
> > impossible to understand.
> > 
> > Guenter
> 
> Hi Guenter,
> 
> Intermixing writel() with writel_relaxed() is unavoidable here. Because there cannot be a memory barrier between writing UNLOCK_SEQ0 and writing UNLOCK_SEQ1. This may be determined by hardware design.
> 

If it is indeed impossible to configure the watchdog for 32-bit
access mode, that needs to be explained in the code and backed up,
for example with a reference to the documentation. Similar,
it needs to be documented in the code why writel() does not work
here and why mb() is needed.

Thanks,
Guenter

> Best Regards,
> Alice Guo
>  
> > > +	}
> > >
> > >  	ret = imx7ulp_wdt_wait(base, WDOG_CS_ULK);
> > >  	if (ret)
> > > --
> > > 2.17.1
> > >
diff mbox series

Patch

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index b8ac0cb04d2f..a0f6b8cea78f 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -180,11 +180,16 @@  static int imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
 
 	local_irq_disable();
 
-	mb();
-	/* unlock the wdog for reconfiguration */
-	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
-	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
-	mb();
+	val = readl(base + WDOG_CS);
+	if (val & WDOG_CS_CMD32EN) {
+		writel(UNLOCK, base + WDOG_CNT);
+	} else {
+		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)