[PATCH v4 0/6] RZG2L_WDT Fixes and Improvements

[PATCH v4 0/6] RZG2L_WDT Fixes and Improvements

[Biju Das]

The first 3 patch in this series fixes the below issues
1) 32 bit overflow issue
2) Runtime PM usage count issue
3) BUG: Invalid context during reset.

The later 3 patches are enhancements to the WDT driver.
1) Adding error check for reset_control_deassert()
2) Generate Parity error for WDT reset
3) Add support for set_timeout callback

Biju Das (6):
  watchdog: rzg2l_wdt: Fix 32bit overflow issue
  watchdog: rzg2l_wdt: Fix Runtime PM usage
  watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'
  watchdog: rzg2l_wdt: Add error check for reset_control_deassert
  watchdog: rzg2l_wdt: Use force reset for WDT reset
  watchdog: rzg2l_wdt: Add set_timeout callback

 drivers/watchdog/rzg2l_wdt.c | 83 ++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 36 deletions(-)

-- 
2.17.1

[PATCH v4 1/6] 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.

This patch also fixes the warning message, 'watchdog: Invalid min and
max timeout values, resetting to 0!"

Fixes: 2cbc5cd0b55fa2 ("watchdog: Add Watchdog Timer driver for RZ/G2L")
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>
---
v3->v4:
 * Updated commit description. Added Fixes tag
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 v4 2/6] watchdog: rzg2l_wdt: Fix Runtime PM usage

[Biju Das]

Both rzg2l_wdt_probe() and rzg2l_wdt_start() calls pm_runtime_get() which
results in a usage counter imbalance. This patch fixes this issue by
removing pm_runtime_get() call from probe.

Fixes: 2cbc5cd0b55fa2 ("watchdog: Add Watchdog Timer driver for RZ/G2L")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
V4:
 * New patch
---
 drivers/watchdog/rzg2l_wdt.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 96f2a018ab62..0fc73b8a9567 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -151,12 +151,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);
 }
@@ -206,11 +205,6 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	reset_control_deassert(priv->rstc);
 	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;
@@ -222,7 +216,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;
@@ -235,12 +229,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 v4 3/6] watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'

[Biju Das]

This patch fixes the issue 'BUG: Invalid wait context' during restart()
callback by using clk_prepare_enable() instead of pm_runtime_get_sync()
for turning on the clocks during restart.

This issue is noticed when testing with renesas_defconfig.

[   42.213802] reboot: Restarting system
[   42.217860]
[   42.219364] =============================
[   42.223368] [ BUG: Invalid wait context ]
[   42.227372] 5.17.0-rc5-arm64-renesas-00002-g10393723e35e #522 Not tainted
[   42.234153] -----------------------------
[   42.238155] systemd-shutdow/1 is trying to lock:
[   42.242766] ffff00000a650828 (&genpd->mlock){+.+.}-{3:3}, at: genpd_lock_mtx+0x14/0x20
[   42.250709] other info that might help us debug this:
[   42.255753] context-{4:4}
[   42.258368] 2 locks held by systemd-shutdow/1:
[   42.262806]  #0: ffff80000944e1c8 (system_transition_mutex#2){+.+.}-{3:3}, at: __do_sys_reboot+0xd0/0x250
[   42.272388]  #1: ffff8000094c4e40 (rcu_read_lock){....}-{1:2}, at: atomic_notifier_call_chain+0x0/0x150
[   42.281795] stack backtrace:
[   42.284672] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 5.17.0-rc5-arm64-renesas-00002-g10393723e35e #522
[   42.294577] Hardware name: Renesas SMARC EVK based on r9a07g044c2 (DT)
[   42.301096] Call trace:
[   42.303538]  dump_backtrace+0xcc/0xd8
[   42.307203]  show_stack+0x14/0x30
[   42.310517]  dump_stack_lvl+0x88/0xb0
[   42.314180]  dump_stack+0x14/0x2c
[   42.317492]  __lock_acquire+0x1b24/0x1b50
[   42.321502]  lock_acquire+0x120/0x3a8
[   42.325162]  __mutex_lock+0x84/0x8f8
[   42.328737]  mutex_lock_nested+0x30/0x58
[   42.332658]  genpd_lock_mtx+0x14/0x20
[   42.336319]  genpd_runtime_resume+0xc4/0x228
[   42.340587]  __rpm_callback+0x44/0x170
[   42.344337]  rpm_callback+0x64/0x70
[   42.347824]  rpm_resume+0x4e0/0x6b8
[   42.351310]  __pm_runtime_resume+0x50/0x78
[   42.355404]  rzg2l_wdt_restart+0x28/0x68
[   42.359329]  watchdog_restart_notifier+0x1c/0x30
[   42.363943]  atomic_notifier_call_chain+0x94/0x150
[   42.368732]  do_kernel_restart+0x24/0x30
[   42.372652]  machine_restart+0x44/0x70
[   42.376399]  kernel_restart+0x3c/0x60
[   42.380058]  __do_sys_reboot+0x228/0x250
[   42.383977]  __arm64_sys_reboot+0x20/0x28
[   42.387983]  invoke_syscall+0x40/0xf8

Fixes: 2cbc5cd0b55fa2 ("watchdog: Add Watchdog Timer driver for RZ/G2L")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
V4:
 * New patch
---
 drivers/watchdog/rzg2l_wdt.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 0fc73b8a9567..48dfe6e5e64f 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -43,6 +43,8 @@ struct rzg2l_wdt_priv {
 	struct reset_control *rstc;
 	unsigned long osc_clk_rate;
 	unsigned long delay;
+	struct clk *pclk;
+	struct clk *osc_clk;
 };
 
 static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
@@ -118,7 +120,9 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 
 	/* Reset the module before we modify any register */
 	reset_control_reset(priv->rstc);
-	pm_runtime_get_sync(wdev->parent);
+
+	clk_prepare_enable(priv->pclk);
+	clk_prepare_enable(priv->osc_clk);
 
 	/* smallest counter value to reboot soon */
 	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
@@ -165,7 +169,6 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rzg2l_wdt_priv *priv;
 	unsigned long pclk_rate;
-	struct clk *wdt_clk;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -177,22 +180,20 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->base);
 
 	/* Get watchdog main clock */
-	wdt_clk = clk_get(&pdev->dev, "oscclk");
-	if (IS_ERR(wdt_clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no oscclk");
+	priv->osc_clk = devm_clk_get(&pdev->dev, "oscclk");
+	if (IS_ERR(priv->osc_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->osc_clk), "no oscclk");
 
-	priv->osc_clk_rate = clk_get_rate(wdt_clk);
-	clk_put(wdt_clk);
+	priv->osc_clk_rate = clk_get_rate(priv->osc_clk);
 	if (!priv->osc_clk_rate)
 		return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
 
 	/* Get Peripheral clock */
-	wdt_clk = clk_get(&pdev->dev, "pclk");
-	if (IS_ERR(wdt_clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
+	priv->pclk = devm_clk_get(&pdev->dev, "pclk");
+	if (IS_ERR(priv->pclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->pclk), "no pclk");
 
-	pclk_rate = clk_get_rate(wdt_clk);
-	clk_put(wdt_clk);
+	pclk_rate = clk_get_rate(priv->pclk);
 	if (!pclk_rate)
 		return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
 
-- 
2.17.1

[PATCH v4 4/6] 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 bailout in case of error.

While at it change reset_control_assert->reset_control_reset in
rzg2l_wdt_stop() and remove unnecessary reset_control_deassert()
from rzg2l_wdt_start().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Made reset usage counter balanced
 * Updated commit description
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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 48dfe6e5e64f..73b667ed3e99 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -88,7 +88,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 */
@@ -108,7 +107,7 @@ 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);
+	reset_control_reset(priv->rstc);
 
 	return 0;
 }
@@ -204,7 +203,10 @@ 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);
 
 	priv->wdev.info = &rzg2l_wdt_ident;
-- 
2.17.1

[PATCH v4 5/6] 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(ie, Generate Reset (WDTRSTB) Signal on
parity error)is faster compared to the overflow method for triggering
watchdog reset.

Overflow method:
	reboot: Restarting system
	Reboot failed -- System halted
	NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c

Parity error method:
	reboot: Restarting system
	NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
V3->V4:
 * Renamed PEEN_FORCE_RST->PEEN_FORCE
 * Updated comments with parity error description
 * Updated commit description
V2->v3:
 * Patch reordering from patch 4->patch 2
 * Updated the commit description.
V1->V2:
 * Updated the commit description.
---
 drivers/watchdog/rzg2l_wdt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 73b667ed3e99..4e7107655cc2 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	BIT(0)
 
 #define WDT_DEFAULT_TIMEOUT		60U
 
@@ -117,17 +120,14 @@ 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);
-
 	clk_prepare_enable(priv->pclk);
 	clk_prepare_enable(priv->osc_clk);
 
-	/* 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 parity error */
+	rzg2l_wdt_write(priv, PEEN_FORCE, PEEN);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v4 6/6] watchdog: rzg2l_wdt: Add set_timeout callback

[Biju Das]

This patch adds support for set_timeout callback.

Once WDT is started, the WDT cycle setting register(WDTSET) can be updated
only after issuing a module reset. Otherwise, it will ignore the writes
and will hold the previous value. This patch updates the WDTSET register
if it is active.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
V3->v4:
 * Updated commit description
 * Simplified the logic for updating timeout register, if wdt is active.
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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 4e7107655cc2..6eea0ee4af49 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -115,6 +115,25 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 	return 0;
 }
 
+static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
+{
+	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+	wdev->timeout = timeout;
+
+	/*
+	 * If the watchdog is active, reset the module for updating the WDTSET
+	 * register so that it is updated with new timeout values.
+	 */
+	if (watchdog_active(wdev)) {
+		pm_runtime_put(wdev->parent);
+		reset_control_reset(priv->rstc);
+		rzg2l_wdt_start(wdev);
+	}
+
+	return 0;
+}
+
 static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 			     unsigned long action, void *data)
 {
@@ -151,6 +170,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

Re: [PATCH v4 2/6] watchdog: rzg2l_wdt: Fix Runtime PM usage

[Geert Uytterhoeven]

On Wed, Feb 23, 2022 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Both rzg2l_wdt_probe() and rzg2l_wdt_start() calls pm_runtime_get() which
> results in a usage counter imbalance. This patch fixes this issue by
> removing pm_runtime_get() call from probe.
>
> Fixes: 2cbc5cd0b55fa2 ("watchdog: Add Watchdog Timer driver for RZ/G2L")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v4 4/6] watchdog: rzg2l_wdt: Add error check for reset_control_deassert

[Geert Uytterhoeven]

Hi Biju,

On Wed, Feb 23, 2022 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 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 bailout in case of error.
>
> While at it change reset_control_assert->reset_control_reset in
> rzg2l_wdt_stop() and remove unnecessary reset_control_deassert()
> from rzg2l_wdt_start().
>
> 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
> @@ -88,7 +88,6 @@ static int rzg2l_wdt_start(struct watchdog_device *wdev)
>  {
>         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>
> -       reset_control_deassert(priv->rstc);

So before, we had a reset control imbalance?
  - After probe, reset was deasserted.
  - After start, reset was deasserted twice. Oops.
You probably want to mention this in the commit description, too.

>         pm_runtime_get_sync(wdev->parent);
>
>         /* Initialize time out */
> @@ -108,7 +107,7 @@ 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);
> +       reset_control_reset(priv->rstc);

As the reset is now deasserted after probe(), stop(), and start(),
I think the reset_control_reset() in .restart() can now be removed?

>
>         return 0;
>  }
> @@ -204,7 +203,10 @@ 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);
>
>         priv->wdev.info = &rzg2l_wdt_ident;

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 v4 5/6] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Geert Uytterhoeven]

Hi Biju,

On Wed, Feb 23, 2022 at 5:01 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(ie, Generate Reset (WDTRSTB) Signal on
> parity error)is faster compared to the overflow method for triggering
> watchdog reset.
>
> Overflow method:
>         reboot: Restarting system
>         Reboot failed -- System halted
>         NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c
>
> Parity error method:
>         reboot: Restarting system
>         NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c
>
> 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

> @@ -117,17 +120,14 @@ 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);
> -

I think this part belongs in patch 4/6.

>         clk_prepare_enable(priv->pclk);
>         clk_prepare_enable(priv->osc_clk);
>
> -       /* 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 parity error */
> +       rzg2l_wdt_write(priv, PEEN_FORCE, PEEN);
>
>         return 0;

The rest LGTM, to
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v4 6/6] watchdog: rzg2l_wdt: Add set_timeout callback

[Geert Uytterhoeven]

On Wed, Feb 23, 2022 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> This patch adds support for set_timeout callback.
>
> Once WDT is started, the WDT cycle setting register(WDTSET) can be updated
> only after issuing a module reset. Otherwise, it will ignore the writes
> and will hold the previous value. This patch updates the WDTSET register
> if it is active.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v4 3/6] watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'

[Geert Uytterhoeven]

Hi Biju,

CC linux-pm

On Wed, Feb 23, 2022 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> This patch fixes the issue 'BUG: Invalid wait context' during restart()
> callback by using clk_prepare_enable() instead of pm_runtime_get_sync()
> for turning on the clocks during restart.
>
> This issue is noticed when testing with renesas_defconfig.
>
> [   42.213802] reboot: Restarting system
> [   42.217860]
> [   42.219364] =============================
> [   42.223368] [ BUG: Invalid wait context ]

> Fixes: 2cbc5cd0b55fa2 ("watchdog: Add Watchdog Timer driver for RZ/G2L")
> 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

> @@ -118,7 +120,9 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>
>         /* Reset the module before we modify any register */
>         reset_control_reset(priv->rstc);
> -       pm_runtime_get_sync(wdev->parent);
> +
> +       clk_prepare_enable(priv->pclk);
> +       clk_prepare_enable(priv->osc_clk);

I'm not super-happy with this circumvention of Runtime PM, but I
don't see a better option, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

PM: Is there a better option?

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 v4 5/6] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 5/6] watchdog: rzg2l_wdt: Use force reset for WDT
> reset
> 
> Hi Biju,
> 
> On Wed, Feb 23, 2022 at 5:01 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(ie, Generate Reset (WDTRSTB) Signal on
> > parity error)is faster compared to the overflow method for triggering
> > watchdog reset.
> >
> > Overflow method:
> >         reboot: Restarting system
> >         Reboot failed -- System halted
> >         NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c
> >
> > Parity error method:
> >         reboot: Restarting system
> >         NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c
> >
> > 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
> 
> > @@ -117,17 +120,14 @@ 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);
> > -
> 
> I think this part belongs in patch 4/6.

For Overflow method we need to reset the module, so that we can
update WDTSET register to change the timeout value from 60sec to 43.69 msec, 
so that reboot can occur after 43.69 msec which corresponds to counter value of 1.

Where as on parity error case, Generating parity error 
immediately trigger the reboot. Here we don't need to reset the module,
for Generating parity error, that is the reason it got removed in this patch.

Regards,
Biju

> 
> >         clk_prepare_enable(priv->pclk);
> >         clk_prepare_enable(priv->osc_clk);
> >
> > -       /* 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 parity error */
> > +       rzg2l_wdt_write(priv, PEEN_FORCE, PEEN);
> >
> >         return 0;
> 
> The rest LGTM, to
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> 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 v4 5/6] watchdog: rzg2l_wdt: Use force reset for WDT reset

[Geert Uytterhoeven]

Hi Biju,

On Thu, Feb 24, 2022 at 10:51 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v4 5/6] watchdog: rzg2l_wdt: Use force reset for WDT
> > reset
> > On Wed, Feb 23, 2022 at 5:01 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(ie, Generate Reset (WDTRSTB) Signal on
> > > parity error)is faster compared to the overflow method for triggering
> > > watchdog reset.
> > >
> > > Overflow method:
> > >         reboot: Restarting system
> > >         Reboot failed -- System halted
> > >         NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c
> > >
> > > Parity error method:
> > >         reboot: Restarting system
> > >         NOTICE:  BL2: v2.5(release):v2.5/rzg2l-1.00-27-gf48f1440c
> > >
> > > 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
> >
> > > @@ -117,17 +120,14 @@ 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);
> > > -
> >
> > I think this part belongs in patch 4/6.
>
> For Overflow method we need to reset the module, so that we can
> update WDTSET register to change the timeout value from 60sec to 43.69 msec,
> so that reboot can occur after 43.69 msec which corresponds to counter value of 1.
>
> Where as on parity error case, Generating parity error
> immediately trigger the reboot. Here we don't need to reset the module,
> for Generating parity error, that is the reason it got removed in this patch.

Right, so please ignore my comment.

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 v4 4/6] watchdog: rzg2l_wdt: Add error check for reset_control_deassert

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 4/6] watchdog: rzg2l_wdt: Add error check for
> reset_control_deassert
> 
> Hi Biju,
> 
> On Wed, Feb 23, 2022 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > 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 bailout in case of error.
> >
> > While at it change reset_control_assert->reset_control_reset in
> > rzg2l_wdt_stop() and remove unnecessary reset_control_deassert() from
> > rzg2l_wdt_start().
> >
> > 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
> > @@ -88,7 +88,6 @@ static int rzg2l_wdt_start(struct watchdog_device
> > *wdev)  {
> >         struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >
> > -       reset_control_deassert(priv->rstc);
> 
> So before, we had a reset control imbalance?
>   - After probe, reset was deasserted.
>   - After start, reset was deasserted twice. Oops.
> You probably want to mention this in the commit description, too.

OK, will add this in the commit description.

> 
> >         pm_runtime_get_sync(wdev->parent);
> >
> >         /* Initialize time out */
> > @@ -108,7 +107,7 @@ 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);
> > +       reset_control_reset(priv->rstc);
> 
> As the reset is now deasserted after probe(), stop(), and start(), I think
> the reset_control_reset() in .restart() can now be removed?

For Overflow method still we need to reset the module, so that we can update 
WDTSET register to change the timeout value from 60sec to 43.69 msec, 
so that reboot can occur after 43.69 msec which corresponds to counter value of 1.

Yes, it is removed in  patch#5, which use Force reset by parity error
instead of WDT overflow method.

Regards,
Biju


> 
> >
> >         return 0;
> >  }
> > @@ -204,7 +203,10 @@ 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);
> >
> >         priv->wdev.info = &rzg2l_wdt_ident;
> 
> 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