[PATCH 0/2] watchdog: rti_wdt: Disable module when unused

[PATCH 0/2] watchdog: rti_wdt: Disable module when unused

[Vignesh Raghavendra]

This series adds couple of fixes to rti_wdt driver so that module is
disabled when unused. First patch is cleanup to use devm_* API and
second patch drops RPM count when unused.

On K3 SoCs, each RTI is paired to a CPU/GPU core. So disabling such
cores (CPU hotplug) would require corresponding RTIs to be off. This
series enables hotplug of the core, if the corresponding watchdog was
unused.


Vignesh Raghavendra (2):
  watchdog: rti_wdt: Use managed APIs to handle runtime PM
  watchdog: rti_wdt: Drop RPM watchdog when unused

 drivers/watchdog/rti_wdt.c | 39 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

-- 
2.42.0

[PATCH 1/2] watchdog: rti_wdt: Use managed APIs to handle runtime PM

[Vignesh Raghavendra]

Switch to devm_pm_runtime_enable() to simplify error handling in driver
probe.

Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---
 drivers/watchdog/rti_wdt.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index 8e1be7ba0103..163bdeb6929a 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -236,12 +236,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	if (wdt->freq < 32768)
 		wdt->freq = wdt->freq * 9 / 10;
 
-	pm_runtime_enable(dev);
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret < 0) {
-		pm_runtime_disable(&pdev->dev);
-		return dev_err_probe(dev, ret, "runtime pm failed\n");
-	}
+	devm_pm_runtime_enable(dev);
+	pm_runtime_get_noresume(dev);
 
 	platform_set_drvdata(pdev, wdt);
 
@@ -260,7 +256,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	wdt->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(wdt->base)) {
 		ret = PTR_ERR(wdt->base);
-		goto err_iomap;
+		return ret;
 	}
 
 	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
@@ -286,7 +282,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		ret = rti_wdt_setup_hw_hb(wdd, wsize);
 		if (ret) {
 			dev_err(dev, "bad window size.\n");
-			goto err_iomap;
+			return ret;
 		}
 
 		last_ping = heartbeat_ms - time_left_ms;
@@ -301,7 +297,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		ret = of_address_to_resource(node, 0, &res);
 		if (ret) {
 			dev_err(dev, "No memory address assigned to the region.\n");
-			goto err_iomap;
+			return ret;
 		}
 
 		/*
@@ -312,15 +308,13 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		reserved_mem_size = resource_size(&res);
 		if (reserved_mem_size < RESERVED_MEM_MIN_SIZE) {
 			dev_err(dev, "The size of reserved memory is too small.\n");
-			ret = -EINVAL;
-			goto err_iomap;
+			return -EINVAL;
 		}
 
 		vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
 		if (!vaddr) {
 			dev_err(dev, "Failed to map memory-region.\n");
-			ret = -ENOMEM;
-			goto err_iomap;
+			return -ENOMEM;
 		}
 
 		if (vaddr[0] == PON_REASON_SOF_NUM &&
@@ -337,19 +331,13 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	ret = watchdog_register_device(wdd);
 	if (ret) {
 		dev_err(dev, "cannot register watchdog device\n");
-		goto err_iomap;
+		return ret;
 	}
 
 	if (last_ping)
 		watchdog_set_last_hw_keepalive(wdd, last_ping);
 
 	return 0;
-
-err_iomap:
-	pm_runtime_put_sync(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
-
-	return ret;
 }
 
 static void rti_wdt_remove(struct platform_device *pdev)
@@ -357,8 +345,6 @@ static void rti_wdt_remove(struct platform_device *pdev)
 	struct rti_wdt_device *wdt = platform_get_drvdata(pdev);
 
 	watchdog_unregister_device(&wdt->wdd);
-	pm_runtime_put(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
 }
 
 static const struct of_device_id rti_wdt_of_match[] = {
-- 
2.42.0

Re: [PATCH 1/2] watchdog: rti_wdt: Use managed APIs to handle runtime PM

[Guenter Roeck]

On 11/10/23 02:07, Vignesh Raghavendra wrote:
> Switch to devm_pm_runtime_enable() to simplify error handling in driver
> probe.
> 

This also replaces the call to pm_runtime_resume_and_get() without explanation.
Worse, the next patch conditionally re-introduces pm_runtime_put_sync()
on the probe function.

> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
>   drivers/watchdog/rti_wdt.c | 30 ++++++++----------------------
>   1 file changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index 8e1be7ba0103..163bdeb6929a 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -236,12 +236,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   	if (wdt->freq < 32768)
>   		wdt->freq = wdt->freq * 9 / 10;
>   
> -	pm_runtime_enable(dev);
> -	ret = pm_runtime_resume_and_get(dev);
> -	if (ret < 0) {
> -		pm_runtime_disable(&pdev->dev);
> -		return dev_err_probe(dev, ret, "runtime pm failed\n");
> -	}
> +	devm_pm_runtime_enable(dev);

devm_pm_runtime_enable() returns an error code. I don't think ignoring it
is a good idea.

> +	pm_runtime_get_noresume(dev);

Is this functionally identical to pm_runtime_resume_and_get() ?
That would require further explanation. Why is it not necessary
to resume here ?

Also, doesn't the next patch fix or at least modify this ?

>   
>   	platform_set_drvdata(pdev, wdt);
>   
> @@ -260,7 +256,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   	wdt->base = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(wdt->base)) {
>   		ret = PTR_ERR(wdt->base);
> -		goto err_iomap;
> +		return ret;
>   	}
>   
>   	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
> @@ -286,7 +282,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   		ret = rti_wdt_setup_hw_hb(wdd, wsize);
>   		if (ret) {
>   			dev_err(dev, "bad window size.\n");
> -			goto err_iomap;
> +			return ret;
>   		}
>   
>   		last_ping = heartbeat_ms - time_left_ms;
> @@ -301,7 +297,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   		ret = of_address_to_resource(node, 0, &res);
>   		if (ret) {
>   			dev_err(dev, "No memory address assigned to the region.\n");
> -			goto err_iomap;
> +			return ret;
>   		}
>   
>   		/*
> @@ -312,15 +308,13 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   		reserved_mem_size = resource_size(&res);
>   		if (reserved_mem_size < RESERVED_MEM_MIN_SIZE) {
>   			dev_err(dev, "The size of reserved memory is too small.\n");
> -			ret = -EINVAL;
> -			goto err_iomap;
> +			return -EINVAL;
>   		}
>   
>   		vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
>   		if (!vaddr) {
>   			dev_err(dev, "Failed to map memory-region.\n");
> -			ret = -ENOMEM;
> -			goto err_iomap;
> +			return -ENOMEM;
>   		}
>   
>   		if (vaddr[0] == PON_REASON_SOF_NUM &&
> @@ -337,19 +331,13 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   	ret = watchdog_register_device(wdd);
>   	if (ret) {
>   		dev_err(dev, "cannot register watchdog device\n");
> -		goto err_iomap;
> +		return ret;
>   	}
>   
>   	if (last_ping)
>   		watchdog_set_last_hw_keepalive(wdd, last_ping);
>   
>   	return 0;
> -
> -err_iomap:
> -	pm_runtime_put_sync(&pdev->dev);
> -	pm_runtime_disable(&pdev->dev);
> -
> -	return ret;
>   }
>   
>   static void rti_wdt_remove(struct platform_device *pdev)
> @@ -357,8 +345,6 @@ static void rti_wdt_remove(struct platform_device *pdev)
>   	struct rti_wdt_device *wdt = platform_get_drvdata(pdev);
>   
>   	watchdog_unregister_device(&wdt->wdd);
> -	pm_runtime_put(&pdev->dev);
> -	pm_runtime_disable(&pdev->dev);
>   }
>   
>   static const struct of_device_id rti_wdt_of_match[] = {

Re: [PATCH 1/2] watchdog: rti_wdt: Use managed APIs to handle runtime PM

[Vignesh Raghavendra]

Hi,

On 10/11/23 20:33, Guenter Roeck wrote:
> On 11/10/23 02:07, Vignesh Raghavendra wrote:
>> Switch to devm_pm_runtime_enable() to simplify error handling in driver
>> probe.
>>
> 
> This also replaces the call to pm_runtime_resume_and_get() without
> explanation.
> Worse, the next patch conditionally re-introduces pm_runtime_put_sync()
> on the probe function.
> 
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>>   drivers/watchdog/rti_wdt.c | 30 ++++++++----------------------
>>   1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index 8e1be7ba0103..163bdeb6929a 100644
>> --- a/drivers/watchdog/rti_wdt.c
>> +++ b/drivers/watchdog/rti_wdt.c
>> @@ -236,12 +236,8 @@ static int rti_wdt_probe(struct platform_device
>> *pdev)
>>       if (wdt->freq < 32768)
>>           wdt->freq = wdt->freq * 9 / 10;
>>   -    pm_runtime_enable(dev);
>> -    ret = pm_runtime_resume_and_get(dev);
>> -    if (ret < 0) {
>> -        pm_runtime_disable(&pdev->dev);
>> -        return dev_err_probe(dev, ret, "runtime pm failed\n");
>> -    }
>> +    devm_pm_runtime_enable(dev);
> 
> devm_pm_runtime_enable() returns an error code. I don't think ignoring it
> is a good idea.
> 


Oops, yes...

>> +    pm_runtime_get_noresume(dev);
> 
> Is this functionally identical to pm_runtime_resume_and_get() ?
> That would require further explanation. Why is it not necessary
> to resume here ?

include/linux/pm_runtime.h ::
 pm_runtime_resume_and_get - Bump up usage counter of a device and
resume it.

vs

pm_runtime_get_noresume - Bump up runtime PM usage counter of a device.

During probe, device is already active. Hence, there is really no need
to call driver level runtime_resume() callback as there is really no
context to resume from. Driver currently doesnt have runtime_pm calls
which I intend to add as a later patch. I guess, its probably better to
move this patch to same series.

Also, missed a call to pm_runtime_put_noidle() in the error path.


So for now I will respin 2/2 as standalone fix and repost along with
runtime_pm support.

Apologies, for the delayed response!

-- 
Regards
Vignesh

[PATCH 2/2] watchdog: rti_wdt: Drop RPM watchdog when unused

[Vignesh Raghavendra]

Do a RPM put if watchdog is not already started during probe and re
enable it in watchdog start.

On K3 SoCs, watchdogs and their corresponding CPUs are under same PD, so
if the reference count of unused watchdogs aren't dropped, it will lead
to CPU hotplug failures as Device Management firmware won't allow to
turn off the PD due to dangling reference count.

Fixes: 2d63908bdbfb ("watchdog: Add K3 RTI watchdog support")
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
---
 drivers/watchdog/rti_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index 163bdeb6929a..87f2c333a41d 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -78,6 +78,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
 	u32 timer_margin;
 	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
 
+	if (pm_runtime_suspended(wdd->parent))
+		pm_runtime_get_sync(wdd->parent);
+
 	/* set timeout period */
 	timer_margin = (u64)wdd->timeout * wdt->freq;
 	timer_margin >>= WDT_PRELOAD_SHIFT;
@@ -337,6 +340,9 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	if (last_ping)
 		watchdog_set_last_hw_keepalive(wdd, last_ping);
 
+	if (!watchdog_hw_running(wdd))
+		pm_runtime_put_sync(&pdev->dev);
+
 	return 0;
 }
 
@@ -345,6 +351,9 @@ static void rti_wdt_remove(struct platform_device *pdev)
 	struct rti_wdt_device *wdt = platform_get_drvdata(pdev);
 
 	watchdog_unregister_device(&wdt->wdd);
+
+	if (!pm_runtime_suspended(&pdev->dev))
+		pm_runtime_put_sync(&pdev->dev);
 }
 
 static const struct of_device_id rti_wdt_of_match[] = {
-- 
2.42.0

Re: [PATCH 2/2] watchdog: rti_wdt: Drop RPM watchdog when unused

[Guenter Roeck]

On 11/10/23 02:07, Vignesh Raghavendra wrote:
> Do a RPM put if watchdog is not already started during probe and re
> enable it in watchdog start.
> 
> On K3 SoCs, watchdogs and their corresponding CPUs are under same PD, so
> if the reference count of unused watchdogs aren't dropped, it will lead
> to CPU hotplug failures as Device Management firmware won't allow to
> turn off the PD due to dangling reference count.
> 
> Fixes: 2d63908bdbfb ("watchdog: Add K3 RTI watchdog support")
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
>   drivers/watchdog/rti_wdt.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index 163bdeb6929a..87f2c333a41d 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -78,6 +78,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
>   	u32 timer_margin;
>   	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
>   
> +	if (pm_runtime_suspended(wdd->parent))
> +		pm_runtime_get_sync(wdd->parent);
> +
>   	/* set timeout period */
>   	timer_margin = (u64)wdd->timeout * wdt->freq;
>   	timer_margin >>= WDT_PRELOAD_SHIFT;
> @@ -337,6 +340,9 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   	if (last_ping)
>   		watchdog_set_last_hw_keepalive(wdd, last_ping);
>   
> +	if (!watchdog_hw_running(wdd))
> +		pm_runtime_put_sync(&pdev->dev);
> +

You will have to explain why this is needed here, but not as error handling,
and why it is added in this patch after the changes in patch 1 of the series
which essentially declared that no error cleanup would be needed under
any circumstances.

Guenter

>   	return 0;
>   }
>   
> @@ -345,6 +351,9 @@ static void rti_wdt_remove(struct platform_device *pdev)
>   	struct rti_wdt_device *wdt = platform_get_drvdata(pdev);
>   
>   	watchdog_unregister_device(&wdt->wdd);
> +
> +	if (!pm_runtime_suspended(&pdev->dev))
> +		pm_runtime_put_sync(&pdev->dev);
>   }
>   
>   static const struct of_device_id rti_wdt_of_match[] = {