[PATCH v3 1/4] watchdog: rzg2l_wdt: Fix 32bit overflow issue

[PATCH v3 1/4] watchdog: rzg2l_wdt: Fix 32bit overflow issue

[Biju Das]

The value of timer_cycle_us can be 0 due to 32bit overflow.
For eg:- If we assign the counter value "0xfff" for computing
maxval.

This patch fixes this issue by appending ULL to 1024, so that
it is promoted to 64bit.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2->v3:
 * No change
V1->V2:
 * Added Rb tag from Guenter and Geert.
---
 drivers/watchdog/rzg2l_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 6b426df34fd6..96f2a018ab62 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -53,7 +53,7 @@ static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
 
 static u32 rzg2l_wdt_get_cycle_usec(unsigned long cycle, u32 wdttime)
 {
-	u64 timer_cycle_us = 1024 * 1024 * (wdttime + 1) * MICRO;
+	u64 timer_cycle_us = 1024 * 1024ULL * (wdttime + 1) * MICRO;
 
 	return div64_ul(timer_cycle_us, cycle);
 }
-- 
2.17.1

[PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Biju Das]

This patch uses the force reset(WDTRSTB) for triggering WDT reset for
restart callback. This method is faster compared to the overflow method
for triggering watchdog reset.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
V2->v3:
 * Patch reordering from patch 4->patch 2
 * Updated the commit description.
V1->V2:
 * Updated the commit description.
---
 drivers/watchdog/rzg2l_wdt.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 96f2a018ab62..b02ecfff5299 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -21,8 +21,11 @@
 #define WDTSET		0x04
 #define WDTTIM		0x08
 #define WDTINT		0x0C
+#define PECR		0x10
+#define PEEN		0x14
 #define WDTCNT_WDTEN	BIT(0)
 #define WDTINT_INTDISP	BIT(0)
+#define PEEN_FORCE_RST	BIT(0)
 
 #define WDT_DEFAULT_TIMEOUT		60U
 
@@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
-	/* Reset the module before we modify any register */
-	reset_control_reset(priv->rstc);
-	pm_runtime_get_sync(wdev->parent);
-
-	/* smallest counter value to reboot soon */
-	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
+	/* Generate Reset (WDTRSTB) Signal */
+	rzg2l_wdt_write(priv, 0, PECR);
 
-	/* Enable watchdog timer*/
-	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
+	/* Force reset (WDTRSTB) */
+	rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Geert Uytterhoeven]

Hi Biju,

On Tue, Jan 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> This patch uses the force reset(WDTRSTB) for triggering WDT reset for
> restart callback. This method is faster compared to the overflow method
> for triggering watchdog reset.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -21,8 +21,11 @@
>  #define WDTSET         0x04
>  #define WDTTIM         0x08
>  #define WDTINT         0x0C
> +#define PECR           0x10
> +#define PEEN           0x14
>  #define WDTCNT_WDTEN   BIT(0)
>  #define WDTINT_INTDISP BIT(0)
> +#define PEEN_FORCE_RST BIT(0)

PEEN_FORCE, as this can trigger either a reset or interrupt?

>
>  #define WDT_DEFAULT_TIMEOUT            60U
>
> @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>  {
>         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>
> -       /* Reset the module before we modify any register */
> -       reset_control_reset(priv->rstc);
> -       pm_runtime_get_sync(wdev->parent);

Why are these no longer needed?
Because .probe() takes care of that? Then why do .start() and .stop()
have reset and Runtime PM handling, too?

> -
> -       /* smallest counter value to reboot soon */
> -       rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
> +       /* Generate Reset (WDTRSTB) Signal */

... on parity error

> +       rzg2l_wdt_write(priv, 0, PECR);
>
> -       /* Enable watchdog timer*/
> -       rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> +       /* Force reset (WDTRSTB) */

s/reset/parity error/

> +       rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN);
>
>         return 0;
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT
> reset
> 
> Hi Biju,
> 
> On Tue, Jan 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > This patch uses the force reset(WDTRSTB) for triggering WDT reset for
> > restart callback. This method is faster compared to the overflow
> > method for triggering watchdog reset.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/watchdog/rzg2l_wdt.c
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -21,8 +21,11 @@
> >  #define WDTSET         0x04
> >  #define WDTTIM         0x08
> >  #define WDTINT         0x0C
> > +#define PECR           0x10
> > +#define PEEN           0x14
> >  #define WDTCNT_WDTEN   BIT(0)
> >  #define WDTINT_INTDISP BIT(0)
> > +#define PEEN_FORCE_RST BIT(0)
> 
> PEEN_FORCE, as this can trigger either a reset or interrupt?

Yes you are correct, it should be PEEN_FORCE.
1 --> Force reset
0 --> Based on operation of parity error register it can trigger a reset or interrupt.

> 
> >
> >  #define WDT_DEFAULT_TIMEOUT            60U
> >
> > @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct
> > watchdog_device *wdev,  {
> >         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >
> > -       /* Reset the module before we modify any register */
> > -       reset_control_reset(priv->rstc);
> > -       pm_runtime_get_sync(wdev->parent);
> 
> Why are these no longer needed? Because .probe() takes care of that?

This code is added to modify WDTSET register. 
Once watchdog is started, On the fly, we won't be able to
update WDTSET register. By resetting(assert/deassert) the module
we can update the WDTSET register. It takes 34 millisec to trigger reset.

But with PEEN_FORCE, on the fly we can update register and it immediately triggers reset.

Then why do .start() and .stop() have
> reset and Runtime PM handling, too?

.start-> turns on the Clocks using Runtime PM.

We cannot Update WDTSET/WDTEN registers, once watchdog is started.
Adding reset and Runtime PM handling in .stop, allows to stop the watchdog.

.stop-> turns off the clock using Runtime PM and does the reset(assert/deassert) of the module
        in order to modify WDTSET/WDTEN registers after stop operation.

May be I should keep pm_runtime_get_sync(wdev->parent) in restart callback,
To turn on the clocks, If someone calls stop followed by restart 

Regards,
Biju



> 
> > -
> > -       /* smallest counter value to reboot soon */
> > -       rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
> > +       /* Generate Reset (WDTRSTB) Signal */
> 
> ... on parity error

You are correct, Force parity error causes reset generation.
OK will update the comment.

> 
> > +       rzg2l_wdt_write(priv, 0, PECR);
> >
> > -       /* Enable watchdog timer*/
> > -       rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> > +       /* Force reset (WDTRSTB) */
> 
> s/reset/parity error/

OK. Will update the comment.


Regards,
Biju

> 
> > +       rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN);
> >
> >         return 0;
> >  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds

Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Geert Uytterhoeven]

Hi Biju,

On Mon, Jan 10, 2022 at 11:51 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT
> > reset
> > On Tue, Jan 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > This patch uses the force reset(WDTRSTB) for triggering WDT reset for
> > > restart callback. This method is faster compared to the overflow
> > > method for triggering watchdog reset.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> > > --- a/drivers/watchdog/rzg2l_wdt.c
> > > +++ b/drivers/watchdog/rzg2l_wdt.c

> > >  #define WDT_DEFAULT_TIMEOUT            60U
> > >
> > > @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct
> > > watchdog_device *wdev,  {
> > >         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > >
> > > -       /* Reset the module before we modify any register */
> > > -       reset_control_reset(priv->rstc);
> > > -       pm_runtime_get_sync(wdev->parent);
> >
> > Why are these no longer needed? Because .probe() takes care of that?
>
> This code is added to modify WDTSET register.
> Once watchdog is started, On the fly, we won't be able to
> update WDTSET register. By resetting(assert/deassert) the module
> we can update the WDTSET register. It takes 34 millisec to trigger reset.
>
> But with PEEN_FORCE, on the fly we can update register and it immediately triggers reset.
>
> Then why do .start() and .stop() have
> > reset and Runtime PM handling, too?
>
> .start-> turns on the Clocks using Runtime PM.
>
> We cannot Update WDTSET/WDTEN registers, once watchdog is started.
> Adding reset and Runtime PM handling in .stop, allows to stop the watchdog.
>
> .stop-> turns off the clock using Runtime PM and does the reset(assert/deassert) of the module
>         in order to modify WDTSET/WDTEN registers after stop operation.
>
> May be I should keep pm_runtime_get_sync(wdev->parent) in restart callback,
> To turn on the clocks, If someone calls stop followed by restart

I'm still confused: .probe() turns the clock on through Runtime PM,
so it's always running.  Calling .start() merely increases the usage
count, and a subsequent .stop() will decrease it again.  But the
clock keeps on running? What am I missing?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Biju Das]

Hi Geert,

> Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for WDT
> reset
> 
> Hi Biju,
> 
> On Mon, Jan 10, 2022 at 11:51 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v3 2/4] watchdog: rzg2l_wdt: Use force reset for
> > > WDT reset On Tue, Jan 4, 2022 at 7:12 PM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > This patch uses the force reset(WDTRSTB) for triggering WDT reset
> > > > for restart callback. This method is faster compared to the
> > > > overflow method for triggering watchdog reset.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > > --- a/drivers/watchdog/rzg2l_wdt.c
> > > > +++ b/drivers/watchdog/rzg2l_wdt.c
> 
> > > >  #define WDT_DEFAULT_TIMEOUT            60U
> > > >
> > > > @@ -116,15 +119,11 @@ static int rzg2l_wdt_restart(struct
> > > > watchdog_device *wdev,  {
> > > >         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > > >
> > > > -       /* Reset the module before we modify any register */
> > > > -       reset_control_reset(priv->rstc);
> > > > -       pm_runtime_get_sync(wdev->parent);
> > >
> > > Why are these no longer needed? Because .probe() takes care of that?
> >
> > This code is added to modify WDTSET register.
> > Once watchdog is started, On the fly, we won't be able to update
> > WDTSET register. By resetting(assert/deassert) the module we can
> > update the WDTSET register. It takes 34 millisec to trigger reset.
> >
> > But with PEEN_FORCE, on the fly we can update register and it
> immediately triggers reset.
> >
> > Then why do .start() and .stop() have
> > > reset and Runtime PM handling, too?
> >
> > .start-> turns on the Clocks using Runtime PM.
> >
> > We cannot Update WDTSET/WDTEN registers, once watchdog is started.
> > Adding reset and Runtime PM handling in .stop, allows to stop the
> watchdog.
> >
> > .stop-> turns off the clock using Runtime PM and does the
> reset(assert/deassert) of the module
> >         in order to modify WDTSET/WDTEN registers after stop operation.
> >
> > May be I should keep pm_runtime_get_sync(wdev->parent) in restart
> > callback, To turn on the clocks, If someone calls stop followed by
> > restart
> 
> I'm still confused: .probe() turns the clock on through Runtime PM, so
> it's always running.  Calling .start() merely increases the usage count,
> and a subsequent .stop() will decrease it again.  But the clock keeps on
> running? What am I missing?

Sorry for the confusion. On the next patch #3, I am removing clock on through Runtime PM from probe,
In order to get balanced PM usage count with start/stop operation.

So after patch#3, .start() turns on the clock and .stop() turns off the clock.

Regards,
Biju

[PATCH v3 3/4] watchdog: rzg2l_wdt: Add error check for reset_control_deassert

[Biju Das]

If reset_control_deassert() fails, then we won't be able to
access the device registers. Therefore check the return code of
reset_control_deassert() and bail out in case of error.

Remove the reset_control_deassert() from rzg2l_wdt_start() as it
already deasserted in probe() and replace reset_control_assert()->
reset_control_reset() in rzg2l_wdt_stop().

While at it remove the unnecessary pm_runtime_resume_and_get()
from probe(), as it turns on the clocks.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Patch reordering from Patch 2 -> Patch 3
 * Updated commit description
v1->v2:
 * Updated commit description and removed Rb tag from Guenter,
   since there is code change
 * Replaced reset_control_assert with reset_control_reset in stop
   and removed reset_control_deassert() from start.
---
 drivers/watchdog/rzg2l_wdt.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index b02ecfff5299..024e7bf796c6 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -89,7 +89,6 @@ static int rzg2l_wdt_start(struct watchdog_device *wdev)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
-	reset_control_deassert(priv->rstc);
 	pm_runtime_get_sync(wdev->parent);
 
 	/* Initialize time out */
@@ -109,9 +108,9 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
 	pm_runtime_put(wdev->parent);
-	reset_control_assert(priv->rstc);
 
-	return 0;
+	/* Reset the module for stopping watchdog */
+	return reset_control_reset(priv->rstc);
 }
 
 static int rzg2l_wdt_restart(struct watchdog_device *wdev,
@@ -150,12 +149,11 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
 	.restart = rzg2l_wdt_restart,
 };
 
-static void rzg2l_wdt_reset_assert_pm_disable_put(void *data)
+static void rzg2l_wdt_reset_assert_pm_disable(void *data)
 {
 	struct watchdog_device *wdev = data;
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
-	pm_runtime_put(wdev->parent);
 	pm_runtime_disable(wdev->parent);
 	reset_control_assert(priv->rstc);
 }
@@ -203,13 +201,11 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
 				     "failed to get cpg reset");
 
-	reset_control_deassert(priv->rstc);
+	ret = reset_control_deassert(priv->rstc);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to deassert");
+
 	pm_runtime_enable(&pdev->dev);
-	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0) {
-		dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe", ERR_PTR(ret));
-		goto out_pm_get;
-	}
 
 	priv->wdev.info = &rzg2l_wdt_ident;
 	priv->wdev.ops = &rzg2l_wdt_ops;
@@ -221,7 +217,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(&priv->wdev, priv);
 	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_wdt_reset_assert_pm_disable_put,
+				       rzg2l_wdt_reset_assert_pm_disable,
 				       &priv->wdev);
 	if (ret < 0)
 		return ret;
@@ -234,12 +230,6 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 		dev_warn(dev, "Specified timeout invalid, using default");
 
 	return devm_watchdog_register_device(&pdev->dev, &priv->wdev);
-
-out_pm_get:
-	pm_runtime_disable(dev);
-	reset_control_assert(priv->rstc);
-
-	return ret;
 }
 
 static const struct of_device_id rzg2l_wdt_ids[] = {
-- 
2.17.1

[PATCH v3 4/4] watchdog: rzg2l_wdt: Add set_timeout callback

[Biju Das]

Once watchdog is started, WDT cycle setting register(WDTSET) cannot be
changed. However we can reconfigure the new value for WDTSET, after a
module reset. Otherwise it will ignore the writes and will hold the
previous value instead of the updated one.

This patch add support for set_timeout callback. Based on the
watchdog timer state, it may restart WDT with the modified values.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Patch reodering Patch 3 -> patch 4
 * Updated commit description.
V1->V2:
 * Updated commit description
 * Removed stop/start and started using reset() instead.
 * After reset, Start WDT based on watchdog timer state.
---
 drivers/watchdog/rzg2l_wdt.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 024e7bf796c6..94f98825ab8d 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -113,6 +113,33 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 	return reset_control_reset(priv->rstc);
 }
 
+static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
+
+	wdev->timeout = timeout;
+
+	/*
+	 * We need to reset the module for updating WDTSET register
+	 * If watchdog is active, then decrement the PM counter to make
+	 * it balanced, after reset operation.
+	 */
+	if (watchdog_active(wdev))
+		pm_runtime_put(wdev->parent);
+
+	/* Reset the module for updating WDTSET register */
+	ret = reset_control_reset(priv->rstc);
+	if (watchdog_active(wdev)) {
+		if (ret)
+			pm_runtime_get_sync(wdev->parent);
+		else
+			rzg2l_wdt_start(wdev);
+	}
+
+	return ret;
+}
+
 static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 			     unsigned long action, void *data)
 {
@@ -146,6 +173,7 @@ static const struct watchdog_ops rzg2l_wdt_ops = {
 	.start = rzg2l_wdt_start,
 	.stop = rzg2l_wdt_stop,
 	.ping = rzg2l_wdt_ping,
+	.set_timeout = rzg2l_wdt_set_timeout,
 	.restart = rzg2l_wdt_restart,
 };
 
-- 
2.17.1