[PATCH 0/2] Reset related fixes for rzg2l_wdt

[PATCH 0/2] Reset related fixes for rzg2l_wdt

[Fabrizio Castro]

Dear All,

this series addresses a couple of issues found with the rzg2l_wdt
driver:
1) on the RZ/Five SoC it was observed that changing the timeout
   wouldn't reset the system as expected
2) on the RZ/V2M SoC it was observed that calling into the restart
   callback from the reboot command wouldn't reboot the system
   straight away

As it turns out, the RZ/Five watchdog IP requires the module clock
to be supplied during a reset.
On RZ/V2M, a specific procedure has to be followed to reset the
watchdog IP.

This series comes with two patches, one to fix the problem affecting
RZ/Five, and one to fix the problem affecting RZ/V2M.

Thanks,
Fab

Fabrizio Castro (1):
  watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

Lad Prabhakar (1):
  watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks

 drivers/watchdog/rzg2l_wdt.c | 45 +++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks

[Fabrizio Castro]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't
reset the system.

The procedure described in the HW manual (Procedure for Activating Modules)
for activating the target module states we need to start supply of the
clock module before applying the reset signal. This patch makes sure we
follow the same procedure to clear the registers of the WDT module, fixing
the issues seen on RZ/Five SoC.

While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as it has
the same function calls.

Fixes: 4055ee81009e ("watchdog: rzg2l_wdt: Add set_timeout callback")
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 974a4194a8fd..ceca42db0837 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -115,25 +115,23 @@ 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_reset(priv->rstc);
+	pm_runtime_put(wdev->parent);
 
 	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.
+	 * register by calling rzg2l_wdt_stop() (which internally calls reset_control_reset()
+	 * to reset the module) 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_stop(wdev);
 		rzg2l_wdt_start(wdev);
 	}
 
-- 
2.34.1

Re: [PATCH 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks

[Guenter Roeck]

On 11/17/22 03:49, Fabrizio Castro wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't
> reset the system.
> 
> The procedure described in the HW manual (Procedure for Activating Modules)
> for activating the target module states we need to start supply of the
> clock module before applying the reset signal. This patch makes sure we
> follow the same procedure to clear the registers of the WDT module, fixing
> the issues seen on RZ/Five SoC.
> 
> While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as it has
> the same function calls.
> 
> Fixes: 4055ee81009e ("watchdog: rzg2l_wdt: Add set_timeout callback")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/rzg2l_wdt.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 974a4194a8fd..ceca42db0837 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -115,25 +115,23 @@ 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_reset(priv->rstc);
> +	pm_runtime_put(wdev->parent);
>   
>   	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.
> +	 * register by calling rzg2l_wdt_stop() (which internally calls reset_control_reset()
> +	 * to reset the module) 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_stop(wdev);
>   		rzg2l_wdt_start(wdev);
>   	}
>   

Re: [PATCH 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks

[Geert Uytterhoeven]

On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't
> reset the system.
>
> The procedure described in the HW manual (Procedure for Activating Modules)
> for activating the target module states we need to start supply of the
> clock module before applying the reset signal. This patch makes sure we
> follow the same procedure to clear the registers of the WDT module, fixing
> the issues seen on RZ/Five SoC.
>
> While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as it has
> the same function calls.
>
> Fixes: 4055ee81009e ("watchdog: rzg2l_wdt: Add set_timeout callback")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@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 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks

[Fabrizio Castro]

Hi All,

It looks like this patch has been reviewed by both Guenter Roeck
and Geert Uytterhoeven, any chance that it can be taken for v6.3?

Thanks,
Fab

> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On RZ/Five SoC it was observed that setting timeout (to say 1 sec)
> wouldn't
> reset the system.
> 
> The procedure described in the HW manual (Procedure for Activating
> Modules)
> for activating the target module states we need to start supply of the
> clock module before applying the reset signal. This patch makes sure we
> follow the same procedure to clear the registers of the WDT module, fixing
> the issues seen on RZ/Five SoC.
> 
> While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as it has
> the same function calls.
> 
> Fixes: 4055ee81009e ("watchdog: rzg2l_wdt: Add set_timeout callback")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/watchdog/rzg2l_wdt.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 974a4194a8fd..ceca42db0837 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -115,25 +115,23 @@ 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_reset(priv->rstc);
> +	pm_runtime_put(wdev->parent);
> 
>  	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.
> +	 * register by calling rzg2l_wdt_stop() (which internally calls
> reset_control_reset()
> +	 * to reset the module) 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_stop(wdev);
>  		rzg2l_wdt_start(wdev);
>  	}
> 
> --
> 2.34.1

RE: [PATCH 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM clocks

[Biju Das]

Hi Fabrizio,

Thanks for the patch.

> Subject: [PATCH 1/2] watchdog: rzg2l_wdt: Issue a reset before we put the PM
> clocks
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On RZ/Five SoC it was observed that setting timeout (to say 1 sec) wouldn't
> reset the system.
> 
> The procedure described in the HW manual (Procedure for Activating Modules)
> for activating the target module states we need to start supply of the clock
> module before applying the reset signal. This patch makes sure we follow the
> same procedure to clear the registers of the WDT module, fixing the issues
> seen on RZ/Five SoC.
> 
> While at it re-used rzg2l_wdt_stop() in rzg2l_wdt_set_timeout() as it has
> the same function calls.
> 
> Fixes: 4055ee81009e ("watchdog: rzg2l_wdt: Add set_timeout callback")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Cheers,
Biju

> ---
>  drivers/watchdog/rzg2l_wdt.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 974a4194a8fd..ceca42db0837 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -115,25 +115,23 @@ 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_reset(priv->rstc);
> +	pm_runtime_put(wdev->parent);
> 
>  	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.
> +	 * register by calling rzg2l_wdt_stop() (which internally calls
> reset_control_reset()
> +	 * to reset the module) 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_stop(wdev);
>  		rzg2l_wdt_start(wdev);
>  	}
> 
> --
> 2.34.1

[PATCH 2/2] watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

[Fabrizio Castro]

As per section 48.4 of the HW User Manual, IPs in the RZ/V2M
SoC need either a TYPE-A reset sequence or a TYPE-B reset
sequence. More specifically, the watchdog IP needs a TYPE-B
reset sequence.

If the proper reset sequence isn't implemented, then resetting
IPs may lead to undesired behaviour. In the restart callback of
the watchdog driver the reset has basically no effect on the
desired funcionality, as the register writes following the reset
happen before the IP manages to come out of reset.

Implement the TYPE-B reset sequence in the watchdog driver to
address the issues with the restart callback on RZ/V2M.

Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support")
Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index ceca42db0837..d404953d0e0f 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -8,6 +8,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -35,6 +36,8 @@
 
 #define F2CYCLE_NSEC(f)			(1000000000 / (f))
 
+#define RZV2M_A_NSEC			730
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
@@ -51,11 +54,35 @@ struct rzg2l_wdt_priv {
 	struct reset_control *rstc;
 	unsigned long osc_clk_rate;
 	unsigned long delay;
+	unsigned long minimum_assertion_period;
 	struct clk *pclk;
 	struct clk *osc_clk;
 	enum rz_wdt_type devtype;
 };
 
+static int rzg2l_wdt_reset(struct rzg2l_wdt_priv *priv)
+{
+	int err, status;
+
+	if (priv->devtype == WDT_RZV2M) {
+		/* WDT needs TYPE-B reset control */
+		err = reset_control_assert(priv->rstc);
+		if (err)
+			return err;
+		ndelay(priv->minimum_assertion_period);
+		err = reset_control_deassert(priv->rstc);
+		if (err)
+			return err;
+		err = read_poll_timeout(reset_control_status, status,
+					status != 1, 0, 1000, false,
+					priv->rstc);
+	} else {
+		err = reset_control_reset(priv->rstc);
+	}
+
+	return err;
+}
+
 static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
 {
 	/* delay timer when change the setting register */
@@ -115,7 +142,7 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
 
-	reset_control_reset(priv->rstc);
+	rzg2l_wdt_reset(priv);
 	pm_runtime_put(wdev->parent);
 
 	return 0;
@@ -154,6 +181,7 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 		rzg2l_wdt_write(priv, PEEN_FORCE, PEEN);
 	} else {
 		/* RZ/V2M doesn't have parity error registers */
+		rzg2l_wdt_reset(priv);
 
 		wdev->timeout = 0;
 
@@ -251,6 +279,13 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
 
 	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
 
+	if (priv->devtype == WDT_RZV2M) {
+		priv->minimum_assertion_period = RZV2M_A_NSEC +
+			3 * F2CYCLE_NSEC(pclk_rate) + 5 *
+			max(F2CYCLE_NSEC(priv->osc_clk_rate),
+			    F2CYCLE_NSEC(pclk_rate));
+	}
+
 	pm_runtime_enable(&pdev->dev);
 
 	priv->wdev.info = &rzg2l_wdt_ident;
-- 
2.34.1

Re: [PATCH 2/2] watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

[Guenter Roeck]

On 11/17/22 03:49, Fabrizio Castro wrote:
> As per section 48.4 of the HW User Manual, IPs in the RZ/V2M
> SoC need either a TYPE-A reset sequence or a TYPE-B reset
> sequence. More specifically, the watchdog IP needs a TYPE-B
> reset sequence.
> 
> If the proper reset sequence isn't implemented, then resetting
> IPs may lead to undesired behaviour. In the restart callback of
> the watchdog driver the reset has basically no effect on the
> desired funcionality, as the register writes following the reset
> happen before the IP manages to come out of reset.
> 
> Implement the TYPE-B reset sequence in the watchdog driver to
> address the issues with the restart callback on RZ/V2M.
> 
> Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support")
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index ceca42db0837..d404953d0e0f 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -8,6 +8,7 @@
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/io.h>
> +#include <linux/iopoll.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of_device.h>
> @@ -35,6 +36,8 @@
>   
>   #define F2CYCLE_NSEC(f)			(1000000000 / (f))
>   
> +#define RZV2M_A_NSEC			730
> +
>   static bool nowayout = WATCHDOG_NOWAYOUT;
>   module_param(nowayout, bool, 0);
>   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> @@ -51,11 +54,35 @@ struct rzg2l_wdt_priv {
>   	struct reset_control *rstc;
>   	unsigned long osc_clk_rate;
>   	unsigned long delay;
> +	unsigned long minimum_assertion_period;
>   	struct clk *pclk;
>   	struct clk *osc_clk;
>   	enum rz_wdt_type devtype;
>   };
>   
> +static int rzg2l_wdt_reset(struct rzg2l_wdt_priv *priv)
> +{
> +	int err, status;
> +
> +	if (priv->devtype == WDT_RZV2M) {
> +		/* WDT needs TYPE-B reset control */
> +		err = reset_control_assert(priv->rstc);
> +		if (err)
> +			return err;
> +		ndelay(priv->minimum_assertion_period);
> +		err = reset_control_deassert(priv->rstc);
> +		if (err)
> +			return err;
> +		err = read_poll_timeout(reset_control_status, status,
> +					status != 1, 0, 1000, false,
> +					priv->rstc);
> +	} else {
> +		err = reset_control_reset(priv->rstc);
> +	}
> +
> +	return err;
> +}
> +
>   static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
>   {
>   	/* delay timer when change the setting register */
> @@ -115,7 +142,7 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>   {
>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>   
> -	reset_control_reset(priv->rstc);
> +	rzg2l_wdt_reset(priv);
>   	pm_runtime_put(wdev->parent);
>   
>   	return 0;
> @@ -154,6 +181,7 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>   		rzg2l_wdt_write(priv, PEEN_FORCE, PEEN);
>   	} else {
>   		/* RZ/V2M doesn't have parity error registers */
> +		rzg2l_wdt_reset(priv);
>   
>   		wdev->timeout = 0;
>   
> @@ -251,6 +279,13 @@ static int rzg2l_wdt_probe(struct platform_device *pdev)
>   
>   	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
>   
> +	if (priv->devtype == WDT_RZV2M) {
> +		priv->minimum_assertion_period = RZV2M_A_NSEC +
> +			3 * F2CYCLE_NSEC(pclk_rate) + 5 *
> +			max(F2CYCLE_NSEC(priv->osc_clk_rate),
> +			    F2CYCLE_NSEC(pclk_rate));
> +	}
> +
>   	pm_runtime_enable(&pdev->dev);
>   
>   	priv->wdev.info = &rzg2l_wdt_ident;

RE: [PATCH 2/2] watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

[Fabrizio Castro]

Hi Geert,

Have you had the chance to look into this patch?
I was hoping it could get taken for v6.3?

Thanks,
Fab

> 
> On 11/17/22 03:49, Fabrizio Castro wrote:
> > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M
> > SoC need either a TYPE-A reset sequence or a TYPE-B reset
> > sequence. More specifically, the watchdog IP needs a TYPE-B
> > reset sequence.
> >
> > If the proper reset sequence isn't implemented, then resetting
> > IPs may lead to undesired behaviour. In the restart callback of
> > the watchdog driver the reset has basically no effect on the
> > desired funcionality, as the register writes following the reset
> > happen before the IP manages to come out of reset.
> >
> > Implement the TYPE-B reset sequence in the watchdog driver to
> > address the issues with the restart callback on RZ/V2M.
> >
> > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support")
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> > ---
> >   drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++++++++++++++++++-
> >   1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> > index ceca42db0837..d404953d0e0f 100644
> > --- a/drivers/watchdog/rzg2l_wdt.c
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -8,6 +8,7 @@
> >   #include <linux/clk.h>
> >   #include <linux/delay.h>
> >   #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >   #include <linux/kernel.h>
> >   #include <linux/module.h>
> >   #include <linux/of_device.h>
> > @@ -35,6 +36,8 @@
> >
> >   #define F2CYCLE_NSEC(f)			(1000000000 / (f))
> >
> > +#define RZV2M_A_NSEC			730
> > +
> >   static bool nowayout = WATCHDOG_NOWAYOUT;
> >   module_param(nowayout, bool, 0);
> >   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
> (default="
> > @@ -51,11 +54,35 @@ struct rzg2l_wdt_priv {
> >   	struct reset_control *rstc;
> >   	unsigned long osc_clk_rate;
> >   	unsigned long delay;
> > +	unsigned long minimum_assertion_period;
> >   	struct clk *pclk;
> >   	struct clk *osc_clk;
> >   	enum rz_wdt_type devtype;
> >   };
> >
> > +static int rzg2l_wdt_reset(struct rzg2l_wdt_priv *priv)
> > +{
> > +	int err, status;
> > +
> > +	if (priv->devtype == WDT_RZV2M) {
> > +		/* WDT needs TYPE-B reset control */
> > +		err = reset_control_assert(priv->rstc);
> > +		if (err)
> > +			return err;
> > +		ndelay(priv->minimum_assertion_period);
> > +		err = reset_control_deassert(priv->rstc);
> > +		if (err)
> > +			return err;
> > +		err = read_poll_timeout(reset_control_status, status,
> > +					status != 1, 0, 1000, false,
> > +					priv->rstc);
> > +	} else {
> > +		err = reset_control_reset(priv->rstc);
> > +	}
> > +
> > +	return err;
> > +}
> > +
> >   static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv)
> >   {
> >   	/* delay timer when change the setting register */
> > @@ -115,7 +142,7 @@ static int rzg2l_wdt_stop(struct watchdog_device
> *wdev)
> >   {
> >   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >
> > -	reset_control_reset(priv->rstc);
> > +	rzg2l_wdt_reset(priv);
> >   	pm_runtime_put(wdev->parent);
> >
> >   	return 0;
> > @@ -154,6 +181,7 @@ static int rzg2l_wdt_restart(struct watchdog_device
> *wdev,
> >   		rzg2l_wdt_write(priv, PEEN_FORCE, PEEN);
> >   	} else {
> >   		/* RZ/V2M doesn't have parity error registers */
> > +		rzg2l_wdt_reset(priv);
> >
> >   		wdev->timeout = 0;
> >
> > @@ -251,6 +279,13 @@ static int rzg2l_wdt_probe(struct platform_device
> *pdev)
> >
> >   	priv->devtype = (uintptr_t)of_device_get_match_data(dev);
> >
> > +	if (priv->devtype == WDT_RZV2M) {
> > +		priv->minimum_assertion_period = RZV2M_A_NSEC +
> > +			3 * F2CYCLE_NSEC(pclk_rate) + 5 *
> > +			max(F2CYCLE_NSEC(priv->osc_clk_rate),
> > +			    F2CYCLE_NSEC(pclk_rate));
> > +	}
> > +
> >   	pm_runtime_enable(&pdev->dev);
> >
> >   	priv->wdev.info = &rzg2l_wdt_ident;

Re: [PATCH 2/2] watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

[Geert Uytterhoeven]

Hi Fabrizio,

On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> As per section 48.4 of the HW User Manual, IPs in the RZ/V2M
> SoC need either a TYPE-A reset sequence or a TYPE-B reset
> sequence. More specifically, the watchdog IP needs a TYPE-B
> reset sequence.
>
> If the proper reset sequence isn't implemented, then resetting
> IPs may lead to undesired behaviour. In the restart callback of
> the watchdog driver the reset has basically no effect on the
> desired funcionality, as the register writes following the reset
> happen before the IP manages to come out of reset.
>
> Implement the TYPE-B reset sequence in the watchdog driver to
> address the issues with the restart callback on RZ/V2M.
>
> Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support")
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

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

Perhaps this logic can be incorporated into the RZ/V2M reset controller
driver later, so reset consumers don't have to care about TYPE-A and
TYPE-B reset, but can just call reset_control_reset()?
I understand that's not gonna be easy, as it needs to know about the
relation between resets and clocks, and how to handle both cases (clock
(not) switched off) for TYPE-B resets.

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 2/2] watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

[Fabrizio Castro]

Hi Geert,

> 
> Hi Fabrizio,
> 
> On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M
> > SoC need either a TYPE-A reset sequence or a TYPE-B reset
> > sequence. More specifically, the watchdog IP needs a TYPE-B
> > reset sequence.
> >
> > If the proper reset sequence isn't implemented, then resetting
> > IPs may lead to undesired behaviour. In the restart callback of
> > the watchdog driver the reset has basically no effect on the
> > desired funcionality, as the register writes following the reset
> > happen before the IP manages to come out of reset.
> >
> > Implement the TYPE-B reset sequence in the watchdog driver to
> > address the issues with the restart callback on RZ/V2M.
> >
> > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support")
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Perhaps this logic can be incorporated into the RZ/V2M reset controller
> driver later, so reset consumers don't have to care about TYPE-A and
> TYPE-B reset, but can just call reset_control_reset()?
> I understand that's not gonna be easy, as it needs to know about the
> relation between resets and clocks, and how to handle both cases (clock
> (not) switched off) for TYPE-B resets.

Yeah, we have been thinking about dealing with this in the reset controller
driver, but as you pointed out it's not going to be simple, and therefore
it'll take some time. This change will guarantee the correct behaviour of
the watchdog for now, we'll tackle the larger issue later on, if that's okay
with you.

Thanks,
Fab

> 
> 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 2/2] watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

[Geert Uytterhoeven]

Hi Fabrizio,

On Mon, Jan 16, 2023 at 5:18 PM Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> > On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro
> > <fabrizio.castro.jz@renesas.com> wrote:
> > > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M
> > > SoC need either a TYPE-A reset sequence or a TYPE-B reset
> > > sequence. More specifically, the watchdog IP needs a TYPE-B
> > > reset sequence.
> > >
> > > If the proper reset sequence isn't implemented, then resetting
> > > IPs may lead to undesired behaviour. In the restart callback of
> > > the watchdog driver the reset has basically no effect on the
> > > desired funcionality, as the register writes following the reset
> > > happen before the IP manages to come out of reset.
> > >
> > > Implement the TYPE-B reset sequence in the watchdog driver to
> > > address the issues with the restart callback on RZ/V2M.
> > >
> > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support")
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > Perhaps this logic can be incorporated into the RZ/V2M reset controller
> > driver later, so reset consumers don't have to care about TYPE-A and
> > TYPE-B reset, but can just call reset_control_reset()?
> > I understand that's not gonna be easy, as it needs to know about the
> > relation between resets and clocks, and how to handle both cases (clock
> > (not) switched off) for TYPE-B resets.
>
> Yeah, we have been thinking about dealing with this in the reset controller
> driver, but as you pointed out it's not going to be simple, and therefore
> it'll take some time. This change will guarantee the correct behaviour of
> the watchdog for now, we'll tackle the larger issue later on, if that's okay
> with you.

Fine for me.

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 2/2] watchdog: rzg2l_wdt: Handle TYPE-B reset for RZ/V2M

[Fabrizio Castro]

Hi Geert,

Thanks for your reply!

> 
> Hi Fabrizio,
> 
> On Mon, Jan 16, 2023 at 5:18 PM Fabrizio Castro
> <fabrizio.castro.jz@renesas.com> wrote:
> > > On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro
> > > <fabrizio.castro.jz@renesas.com> wrote:
> > > > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M
> > > > SoC need either a TYPE-A reset sequence or a TYPE-B reset
> > > > sequence. More specifically, the watchdog IP needs a TYPE-B
> > > > reset sequence.
> > > >
> > > > If the proper reset sequence isn't implemented, then resetting
> > > > IPs may lead to undesired behaviour. In the restart callback of
> > > > the watchdog driver the reset has basically no effect on the
> > > > desired funcionality, as the register writes following the reset
> > > > happen before the IP manages to come out of reset.
> > > >
> > > > Implement the TYPE-B reset sequence in the watchdog driver to
> > > > address the issues with the restart callback on RZ/V2M.
> > > >
> > > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support")
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > Perhaps this logic can be incorporated into the RZ/V2M reset
> controller
> > > driver later, so reset consumers don't have to care about TYPE-A
> and
> > > TYPE-B reset, but can just call reset_control_reset()?
> > > I understand that's not gonna be easy, as it needs to know about
> the
> > > relation between resets and clocks, and how to handle both cases
> (clock
> > > (not) switched off) for TYPE-B resets.
> >
> > Yeah, we have been thinking about dealing with this in the reset
> controller
> > driver, but as you pointed out it's not going to be simple, and
> therefore
> > it'll take some time. This change will guarantee the correct
> behaviour of
> > the watchdog for now, we'll tackle the larger issue later on, if
> that's okay
> > with you.
> 
> Fine for me.

Awesome, thanks for that.

Cheers,
Fab

> 
> 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