diff mbox series

[PATCHv2,6/6] thermal: exynos: Add runtime power management for tmu

Message ID 20220515064126.1424-7-linux.amoon@gmail.com (mailing list archive)
State New, archived
Headers show
Series Exynos Thermal code inprovement | expand

Commit Message

Anand Moon May 15, 2022, 6:41 a.m. UTC
Add runtime power management for exynos thermal driver.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series.
---
 drivers/thermal/samsung/exynos_tmu.c | 29 ++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski May 15, 2022, 9:48 a.m. UTC | #1
On 15/05/2022 08:41, Anand Moon wrote:
> Add runtime power management for exynos thermal driver.

First of all - why? Second, I do not see it being added. Where are the
runtime callbacks?


Best regards,
Krzysztof
Anand Moon May 17, 2022, 6:45 p.m. UTC | #2
Hi Krzysztof,

On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > Add runtime power management for exynos thermal driver.
>
> First of all - why? Second, I do not see it being added. Where are the
> runtime callbacks?
>

To control runtime control PMU, did I miss something?
I looked into imx thermal driver # drivers/thermal/imx_thermal.c
to enable run-time power management for exynos driver.

>
> Best regards,
> Krzysztof

Thanks & Regards


-Anand
Krzysztof Kozlowski May 18, 2022, 7:19 a.m. UTC | #3
On 17/05/2022 20:45, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 15/05/2022 08:41, Anand Moon wrote:
>>> Add runtime power management for exynos thermal driver.
>>
>> First of all - why? Second, I do not see it being added. Where are the
>> runtime callbacks?
>>
> 
> To control runtime control PMU, did I miss something?

Controlling runtime PM by itself is not a goal. What does it change if
it is enabled?

> I looked into imx thermal driver # drivers/thermal/imx_thermal.c
> to enable run-time power management for exynos driver.

So you have runtime PM enabled and then what happens? Where is the power
saving? Since you did not implement the callbacks, all this should be
explained in commit msg.


Best regards,
Krzysztof
Anand Moon May 21, 2022, 9:52 a.m. UTC | #4
Hi Krzysztof,

On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/05/2022 20:45, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 15/05/2022 08:41, Anand Moon wrote:
> >>> Add runtime power management for exynos thermal driver.
> >>
> >> First of all - why? Second, I do not see it being added. Where are the
> >> runtime callbacks?
> >>
> >
> > To control runtime control PMU, did I miss something?
>
> Controlling runtime PM by itself is not a goal. What does it change if
> it is enabled?
>
It means we could have efficient power management for this driver.
as per my understanding, it controls runtime sleep and improves power efficiency

> > I looked into imx thermal driver # drivers/thermal/imx_thermal.c
> > to enable run-time power management for exynos driver.
>
> So you have runtime PM enabled and then what happens? Where is the power
> saving? Since you did not implement the callbacks, all this should be
> explained in commit msg.
>
Ok, As per the original code, it just registers the SIMPLE_DEV_PM_OPS
with .pm = &exynos_tmu_pm
So I have made sure that suspend resume feature works correctly
 with these changes on SBC Odroid U3 and XU4.

I will try to look into setting RUNTIME_PM_OPS
or use UNIVERSAL_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS
any thought on this?

>
> Best regards,
> Krzysztof

Thanks and Regards

-Anand
Krzysztof Kozlowski May 21, 2022, 2:11 p.m. UTC | #5
On 21/05/2022 11:52, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/05/2022 20:45, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 15/05/2022 08:41, Anand Moon wrote:
>>>>> Add runtime power management for exynos thermal driver.
>>>>
>>>> First of all - why? Second, I do not see it being added. Where are the
>>>> runtime callbacks?
>>>>
>>>
>>> To control runtime control PMU, did I miss something?
>>
>> Controlling runtime PM by itself is not a goal. What does it change if
>> it is enabled?
>>
> It means we could have efficient power management for this driver.
> as per my understanding, it controls runtime sleep and improves power efficiency

How? I asked - what is being changed after enabling PM - and you
answered without any specifics. Where exactly is the power saving?
Please be specific, very specific.

> 
>>> I looked into imx thermal driver # drivers/thermal/imx_thermal.c
>>> to enable run-time power management for exynos driver.
>>
>> So you have runtime PM enabled and then what happens? Where is the power
>> saving? Since you did not implement the callbacks, all this should be
>> explained in commit msg.
>>
> Ok, As per the original code, it just registers the SIMPLE_DEV_PM_OPS
> with .pm = &exynos_tmu_pm

And does nothing else, right? No benefits?

> So I have made sure that suspend resume feature works correctly
>  with these changes on SBC Odroid U3 and XU4.

How is suspend/resume related to runtime PM? Are you talking about
system suspend? What do you mean now?

> 
> I will try to look into setting RUNTIME_PM_OPS
> or use UNIVERSAL_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS
> any thought on this?

Why looking at them? You avoid giving any specific answer, so we are
repeating the same and the same. Just to be sure - maybe I don't see the
obvious stuff, so please explain also this obvious things.

Please come with specifics, because otherwise I see it as a waste of our
time.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f8a527f19383..be9b98caf2ba 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -20,6 +20,7 @@ 
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
 
 #include <dt-bindings/thermal/thermal_exynos.h>
 
@@ -1106,6 +1107,15 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_thermal;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0)
+		goto disable_runtime_pm;
+
 	ret = devm_request_irq(&pdev->dev, data->irq, exynos_tmu_irq,
 		IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data);
 	if (ret) {
@@ -1113,11 +1123,16 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_thermal;
 	}
 
+	pm_runtime_put(&pdev->dev);
+
 	exynos_tmu_control(pdev, true);
 	return 0;
 
 err_thermal:
 	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
+disable_runtime_pm:
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 err_clk_sec:
 	clk_disable_unprepare(data->clk_sec);
 err_sclk:
@@ -1143,6 +1158,9 @@  static int exynos_tmu_remove(struct platform_device *pdev)
 	clk_disable_unprepare(data->clk);
 	clk_disable_unprepare(data->clk_sec);
 
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
 
@@ -1151,18 +1169,25 @@  static int exynos_tmu_remove(struct platform_device *pdev)
 
 static int __maybe_unused exynos_tmu_suspend(struct device *dev)
 {
-	exynos_tmu_control(to_platform_device(dev), false);
+	struct platform_device *pdev = to_platform_device(dev);
 
-	return 0;
+	exynos_tmu_control(pdev, false);
+
+	return pm_runtime_force_suspend(&pdev->dev);
 }
 
 static int __maybe_unused exynos_tmu_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	int ret;
 
 	exynos_tmu_initialize(pdev);
 	exynos_tmu_control(pdev, true);
 
+	ret = pm_runtime_force_resume(&pdev->dev);
+	if (ret)
+		return ret;
+
 	return 0;
 }