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

[PATCH 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>
---
 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 2/4] watchdog: rzg2l_wdt: Add error check for reset_control_{deassert/reset}

[Biju Das]

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

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>
---
 drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 96f2a018ab62..58fe4efd9a89 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -85,8 +85,14 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
 static int rzg2l_wdt_start(struct watchdog_device *wdev)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
+
+	ret = reset_control_deassert(priv->rstc);
+	if (ret) {
+		dev_err(wdev->parent, "failed to deassert");
+		return ret;
+	}
 
-	reset_control_deassert(priv->rstc);
 	pm_runtime_get_sync(wdev->parent);
 
 	/* Initialize time out */
@@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 			     unsigned long action, void *data)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
+	int ret;
 
 	/* Reset the module before we modify any register */
-	reset_control_reset(priv->rstc);
+	ret = reset_control_reset(priv->rstc);
+	if (ret) {
+		dev_err(wdev->parent, "failed to reset");
+		return ret;
+	}
+
 	pm_runtime_get_sync(wdev->parent);
 
 	/* smallest counter value to reboot soon */
@@ -151,12 +163,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);
 }
@@ -204,13 +215,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;
@@ -222,7 +231,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 +244,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 3/4] watchdog: rzg2l_wdt: Add set_timeout callback

[Biju Das]

Add support for set_timeout() callback.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index 58fe4efd9a89..c81b9dd05e63 100644
--- a/drivers/watchdog/rzg2l_wdt.c
+++ b/drivers/watchdog/rzg2l_wdt.c
@@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
 	return 0;
 }
 
+static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
+{
+	wdev->timeout = timeout;
+	rzg2l_wdt_stop(wdev);
+	rzg2l_wdt_start(wdev);
+
+	return 0;
+}
+
 static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 			     unsigned long action, void *data)
 {
@@ -160,6 +169,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

[PATCH 4/4] watchdog: rzg2l_wdt: Fix Reboot failed message

[Biju Das]

This patch fixes the message "Reboot failed -- System halted"
by triggering WDT reset by enabling force reset(WDTRSTB).

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/watchdog/rzg2l_wdt.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
index c81b9dd05e63..497c86129f20 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
 
@@ -130,22 +133,12 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
 			     unsigned long action, void *data)
 {
 	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
-	int ret;
 
-	/* Reset the module before we modify any register */
-	ret = reset_control_reset(priv->rstc);
-	if (ret) {
-		dev_err(wdev->parent, "failed to reset");
-		return ret;
-	}
+	/* Generate Reset (WDTRSTB) Signal */
+	rzg2l_wdt_write(priv, 0, PECR);
 
-	pm_runtime_get_sync(wdev->parent);
-
-	/* smallest counter value to reboot soon */
-	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
-
-	/* 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 3/4] watchdog: rzg2l_wdt: Add set_timeout callback

[Guenter Roeck]

On 12/11/21 1:26 PM, Biju Das wrote:
> Add support for set_timeout() callback.
> 
This needs an explanation. WDIOF_SETTIMEOUT is, after all,
already supported. I can see that 'count' is not recalculated,
so that is one of the reasons. However, it also needs to be explained
why it is necessary to stop and restart the watchdog.

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>   drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 58fe4efd9a89..c81b9dd05e63 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>   	return 0;
>   }
>   
> +static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
> +{
> +	wdev->timeout = timeout;
> +	rzg2l_wdt_stop(wdev);
> +	rzg2l_wdt_start(wdev);

Is it necessary to stop and restart the timeout, or would it be sufficient
to call rza_wdt_calc_timeout() ? If it is necessary, please add a comment
describing the reason.

Either case, calling rzg2l_wdt_start() unconditionally is wrong because
the watchdog might be stopped.

Guenter

> +
> +	return 0;
> +}
> +
>   static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>   			     unsigned long action, void *data)
>   {
> @@ -160,6 +169,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,
>   };
>   
> 

Re: [PATCH 4/4] watchdog: rzg2l_wdt: Fix Reboot failed message

[Guenter Roeck]

On 12/11/21 1:26 PM, Biju Das wrote:
> This patch fixes the message "Reboot failed -- System halted"
> by triggering WDT reset by enabling force reset(WDTRSTB).
> 

That is really misleading. The patch does not fix the message - it fixes
the reboot handler to make it actually execute the reboot.

Guenter

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>   drivers/watchdog/rzg2l_wdt.c | 21 +++++++--------------
>   1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index c81b9dd05e63..497c86129f20 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
>   
> @@ -130,22 +133,12 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>   			     unsigned long action, void *data)
>   {
>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> -	int ret;
>   
> -	/* Reset the module before we modify any register */
> -	ret = reset_control_reset(priv->rstc);
> -	if (ret) {
> -		dev_err(wdev->parent, "failed to reset");
> -		return ret;
> -	}
> +	/* Generate Reset (WDTRSTB) Signal */
> +	rzg2l_wdt_write(priv, 0, PECR);
>   
> -	pm_runtime_get_sync(wdev->parent);
> -
> -	/* smallest counter value to reboot soon */
> -	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
> -
> -	/* Enable watchdog timer*/
> -	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> +	/* Force reset (WDTRSTB) */
> +	rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN);
>   
>   	return 0;
>   }
> 

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

[Guenter Roeck]

On 12/11/21 1:26 PM, Biju Das wrote:
> 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>

> ---
>   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);
>   }
> 

Re: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for reset_control_{deassert/reset}

[Guenter Roeck]

On 12/11/21 1:26 PM, Biju Das wrote:
> If reset_control_deassert()/reset_control_reset() fails, then we
> won't be able to access the device registers. Therefore check the
> return code of reset_control_deassert()/reset_control_reset() and
> return the error code to caller in case of error.
> 
> 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>

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

See note below, though.

> ---
>   drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++-----------------
>   1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 96f2a018ab62..58fe4efd9a89 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -85,8 +85,14 @@ static void rzg2l_wdt_init_timeout(struct watchdog_device *wdev)
>   static int rzg2l_wdt_start(struct watchdog_device *wdev)
>   {
>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
> +
> +	ret = reset_control_deassert(priv->rstc);
> +	if (ret) {
> +		dev_err(wdev->parent, "failed to deassert");
> +		return ret;
> +	}
>   
This patch introduces an error return into rzg2l_wdt_start().
If it is indeed necessary to call this function when setting
the timeout, the error return needs to be checked there.

Guenter

> -	reset_control_deassert(priv->rstc);
>   	pm_runtime_get_sync(wdev->parent);
>   
>   	/* Initialize time out */
> @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>   			     unsigned long action, void *data)
>   {
>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
>   
>   	/* Reset the module before we modify any register */
> -	reset_control_reset(priv->rstc);
> +	ret = reset_control_reset(priv->rstc);
> +	if (ret) {
> +		dev_err(wdev->parent, "failed to reset");
> +		return ret;
> +	}
> +
>   	pm_runtime_get_sync(wdev->parent);
>   
>   	/* smallest counter value to reboot soon */
> @@ -151,12 +163,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);
>   }
> @@ -204,13 +215,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;
> @@ -222,7 +231,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 +244,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[] = {
> 

Re: [PATCH 3/4] watchdog: rzg2l_wdt: Add set_timeout callback

[Guenter Roeck]

On 12/11/21 1:38 PM, Guenter Roeck wrote:
> On 12/11/21 1:26 PM, Biju Das wrote:
>> Add support for set_timeout() callback.
>>
> This needs an explanation. WDIOF_SETTIMEOUT is, after all,
> already supported. I can see that 'count' is not recalculated,
> so that is one of the reasons. However, it also needs to be explained
> why it is necessary to stop and restart the watchdog.
> 
>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>> ---
>>   drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>> index 58fe4efd9a89..c81b9dd05e63 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>>       return 0;
>>   }
>> +static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev, unsigned int timeout)
>> +{
>> +    wdev->timeout = timeout;
>> +    rzg2l_wdt_stop(wdev);
>> +    rzg2l_wdt_start(wdev);
> 
> Is it necessary to stop and restart the timeout, or would it be sufficient
> to call rza_wdt_calc_timeout() ? If it is necessary, please add a comment

That should have been rzg2l_wdt_init_timeout(). Also, as mentioned in
the second patch of the series, the return value of rzg2l_wdt_start()
needs to be checked if the watchdog needs to be stopped and restarted.

Thanks,
Guenter

> describing the reason.
> 
> Either case, calling rzg2l_wdt_start() unconditionally is wrong because
> the watchdog might be stopped.
> 
> Guenter
> 
>> +
>> +    return 0;
>> +}
>> +
>>   static int rzg2l_wdt_restart(struct watchdog_device *wdev,
>>                    unsigned long action, void *data)
>>   {
>> @@ -160,6 +169,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,
>>   };
>>
> 

RE: [PATCH 4/4] watchdog: rzg2l_wdt: Fix Reboot failed message

[Biju Das]

Hi Guenter,

Thanks for the feedback.

> Subject: Re: [PATCH 4/4] watchdog: rzg2l_wdt: Fix Reboot failed message
> 
> On 12/11/21 1:26 PM, Biju Das wrote:
> > This patch fixes the message "Reboot failed -- System halted"
> > by triggering WDT reset by enabling force reset(WDTRSTB).
> >
> 
> That is really misleading. The patch does not fix the message - it fixes
> the reboot handler to make it actually execute the reboot.

There is some delay(~34 msec)in reboot handler, make this noisy message to appear in the kernel logs,
before the reboot handler to actually executing the reboot due to watchdog reset.

This issue is vanished by using new force reset method.

May be I will update the commit header and description as Use force reset method
for WDT reset.

Regards,
Biju


> 
> Guenter
> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >   drivers/watchdog/rzg2l_wdt.c | 21 +++++++--------------
> >   1 file changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/watchdog/rzg2l_wdt.c
> > b/drivers/watchdog/rzg2l_wdt.c index c81b9dd05e63..497c86129f20 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
> >
> > @@ -130,22 +133,12 @@ static int rzg2l_wdt_restart(struct
> watchdog_device *wdev,
> >   			     unsigned long action, void *data)
> >   {
> >   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > -	int ret;
> >
> > -	/* Reset the module before we modify any register */
> > -	ret = reset_control_reset(priv->rstc);
> > -	if (ret) {
> > -		dev_err(wdev->parent, "failed to reset");
> > -		return ret;
> > -	}
> > +	/* Generate Reset (WDTRSTB) Signal */
> > +	rzg2l_wdt_write(priv, 0, PECR);
> >
> > -	pm_runtime_get_sync(wdev->parent);
> > -
> > -	/* smallest counter value to reboot soon */
> > -	rzg2l_wdt_write(priv, WDTSET_COUNTER_VAL(1), WDTSET);
> > -
> > -	/* Enable watchdog timer*/
> > -	rzg2l_wdt_write(priv, WDTCNT_WDTEN, WDTCNT);
> > +	/* Force reset (WDTRSTB) */
> > +	rzg2l_wdt_write(priv, PEEN_FORCE_RST, PEEN);
> >
> >   	return 0;
> >   }
> >

RE: [PATCH 3/4] watchdog: rzg2l_wdt: Add set_timeout callback

[Biju Das]

Hi Guenter Roeck,

> Subject: Re: [PATCH 3/4] watchdog: rzg2l_wdt: Add set_timeout callback
> 
> On 12/11/21 1:38 PM, Guenter Roeck wrote:
> > On 12/11/21 1:26 PM, Biju Das wrote:
> >> Add support for set_timeout() callback.
> >>
> > This needs an explanation. WDIOF_SETTIMEOUT is, after all, already
> > supported. I can see that 'count' is not recalculated, so that is one
> > of the reasons. However, it also needs to be explained why it is
> > necessary to stop and restart the watchdog.

Will add explanation in next revision. Basically once watchdog is started,
WDT cycle setting register(WDTSET) cannot be changed. However we can stop WDT by
issuing a reset and then reconfigure the new value for WDSET. So module reset is needed here.

As you stated below, restarting watchdog unconditionally is not correct.
May be after module reset, if watchdog timer is active(test_bit(WDOG_ACTIVE)), then 
will call start function. If it is in stopped state, when it calls start next time, 
it will pick new values. So both conditions are taken care here.

> >
> >> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >> ---
> >>   drivers/watchdog/rzg2l_wdt.c | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/watchdog/rzg2l_wdt.c
> >> b/drivers/watchdog/rzg2l_wdt.c index 58fe4efd9a89..c81b9dd05e63
> >> 100644
> >> --- a/drivers/watchdog/rzg2l_wdt.c
> >> +++ b/drivers/watchdog/rzg2l_wdt.c
> >> @@ -117,6 +117,15 @@ static int rzg2l_wdt_stop(struct watchdog_device
> >> *wdev)
> >>       return 0;
> >>   }
> >> +static int rzg2l_wdt_set_timeout(struct watchdog_device *wdev,
> >> +unsigned int timeout) {
> >> +    wdev->timeout = timeout;
> >> +    rzg2l_wdt_stop(wdev);
> >> +    rzg2l_wdt_start(wdev);
> >
> > Is it necessary to stop and restart the timeout, or would it be
> > sufficient to call rza_wdt_calc_timeout() ? If it is necessary, please
> > add a comment

OK, will do.

Basically we need to do a module reset. Then only we can change the WDTSET register.
On the next version, instead of stop/start, will issue a module reset, and
If wdt is active then will call start.

> 
> That should have been rzg2l_wdt_init_timeout(). Also, as mentioned in the
> second patch of the series, the return value of rzg2l_wdt_start() needs to
> be checked if the watchdog needs to be stopped and restarted.

On the second patch, I will add the changes as you suggested for rzg2l_wdt_init_timeout().
and rzg2l_wdt_start will check return value of rzg2l_wdt_init_timeout.

Regards,
Biju

> 
> Thanks,
> Guenter
> 
> > describing the reason.
> >
> > Either case, calling rzg2l_wdt_start() unconditionally is wrong
> > because the watchdog might be stopped.
> >
> > Guenter
> >
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>   static int rzg2l_wdt_restart(struct watchdog_device *wdev,
> >>                    unsigned long action, void *data)
> >>   {
> >> @@ -160,6 +169,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,
> >>   };
> >>
> >

RE: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for reset_control_{deassert/reset}

[Biju Das]

Hi Guenter Roeck,

> Subject: Re: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for
> reset_control_{deassert/reset}
> 
> On 12/11/21 1:26 PM, Biju Das wrote:
> > If reset_control_deassert()/reset_control_reset() fails, then we won't
> > be able to access the device registers. Therefore check the return
> > code of reset_control_deassert()/reset_control_reset() and return the
> > error code to caller in case of error.
> >
> > 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>
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> See note below, though.
OK.

> 
> > ---
> >   drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++-----------------
> >   1 file changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/watchdog/rzg2l_wdt.c
> > b/drivers/watchdog/rzg2l_wdt.c index 96f2a018ab62..58fe4efd9a89 100644
> > --- a/drivers/watchdog/rzg2l_wdt.c
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -85,8 +85,14 @@ static void rzg2l_wdt_init_timeout(struct
> watchdog_device *wdev)
> >   static int rzg2l_wdt_start(struct watchdog_device *wdev)
> >   {
> >   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	int ret;
> > +
> > +	ret = reset_control_deassert(priv->rstc);
> > +	if (ret) {
> > +		dev_err(wdev->parent, "failed to deassert");
> > +		return ret;
> > +	}
> >
> This patch introduces an error return into rzg2l_wdt_start().
> If it is indeed necessary to call this function when setting the timeout,
> the error return needs to be checked there.

OK will do.

Basically once watchdog is started it can be stopped only by reset.
So for stopping WDT, we need to assert the reset.

Also once WDT is started, Updation of WDT cycle setting register(WDTSET)
is possible only by resetting WDT,  that is assert/deassert and reconfigure
WDTSET with new values.

Regards,
Biju

> 
> Guenter
> 
> > -	reset_control_deassert(priv->rstc);
> >   	pm_runtime_get_sync(wdev->parent);
> >
> >   	/* Initialize time out */
> > @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device
> *wdev,
> >   			     unsigned long action, void *data)
> >   {
> >   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	int ret;
> >
> >   	/* Reset the module before we modify any register */
> > -	reset_control_reset(priv->rstc);
> > +	ret = reset_control_reset(priv->rstc);
> > +	if (ret) {
> > +		dev_err(wdev->parent, "failed to reset");
> > +		return ret;
> > +	}
> > +
> >   	pm_runtime_get_sync(wdev->parent);
> >
> >   	/* smallest counter value to reboot soon */ @@ -151,12 +163,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);
> >   }
> > @@ -204,13 +215,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;
> > @@ -222,7 +231,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 +244,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[] = {
> >

RE: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for reset_control_{deassert/reset}

[Biju Das]

Hi Guenter Roeck,

> Subject: Re: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for
> reset_control_{deassert/reset}
> 
> On 12/11/21 1:26 PM, Biju Das wrote:
> > If reset_control_deassert()/reset_control_reset() fails, then we won't
> > be able to access the device registers. Therefore check the return
> > code of reset_control_deassert()/reset_control_reset() and return the
> > error code to caller in case of error.
> >
> > 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>
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> See note below, though.
> 
> > ---
> >   drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++-----------------
> >   1 file changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/watchdog/rzg2l_wdt.c
> > b/drivers/watchdog/rzg2l_wdt.c index 96f2a018ab62..58fe4efd9a89 100644
> > --- a/drivers/watchdog/rzg2l_wdt.c
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -85,8 +85,14 @@ static void rzg2l_wdt_init_timeout(struct
> watchdog_device *wdev)
> >   static int rzg2l_wdt_start(struct watchdog_device *wdev)
> >   {
> >   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	int ret;
> > +
> > +	ret = reset_control_deassert(priv->rstc);
> > +	if (ret) {
> > +		dev_err(wdev->parent, "failed to deassert");
> > +		return ret;
> > +	}
> >
> This patch introduces an error return into rzg2l_wdt_start().
> If it is indeed necessary to call this function when setting the timeout,
> the error return needs to be checked there.

Good point. After rechecking, this call is not at all needed. 

For WDT stop/ settime we need to reset the module. So we should use reset_control_reset() instead.
From platform point, it just asserts and then de-assert the module[1].

Will add checks for this call in stop/settime and return error code to caller.

I will send V2 with changes.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/renesas/rzg2l-cpg.c?h=v5.16-rc4#n685

Regards,
Biju

> 
> Guenter
> 
> > -	reset_control_deassert(priv->rstc);
> >   	pm_runtime_get_sync(wdev->parent);
> >
> >   	/* Initialize time out */
> > @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device
> *wdev,
> >   			     unsigned long action, void *data)
> >   {
> >   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	int ret;
> >
> >   	/* Reset the module before we modify any register */
> > -	reset_control_reset(priv->rstc);
> > +	ret = reset_control_reset(priv->rstc);
> > +	if (ret) {
> > +		dev_err(wdev->parent, "failed to reset");
> > +		return ret;
> > +	}
> > +
> >   	pm_runtime_get_sync(wdev->parent);
> >
> >   	/* smallest counter value to reboot soon */ @@ -151,12 +163,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);
> >   }
> > @@ -204,13 +215,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;
> > @@ -222,7 +231,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 +244,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[] = {
> >

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

[Geert Uytterhoeven]

On Sat, Dec 11, 2021 at 10:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 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: 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