| Message ID | 20230829091853.626011-5-m.majewski2@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve Exynos thermal driver | expand |
On 29/08/2023 11:18, Mateusz Majewski wrote: > This clock only controls the register operations. The gain in power > efficiency is therefore quite dubious, while there is price of added > complexity that is important to get right (as a register operation might > outright hang the CPU if the clock is not enabled). So once it is done right, this stops being argument. The benefit is to keep this clock disabled most of the time, which now we lost. I don't find this patch correct approach. Best regards, Krzysztof
On 29.08.2023 11:56, Krzysztof Kozlowski wrote: > On 29/08/2023 11:18, Mateusz Majewski wrote: >> This clock only controls the register operations. The gain in power >> efficiency is therefore quite dubious, while there is price of added >> complexity that is important to get right (as a register operation might >> outright hang the CPU if the clock is not enabled). > So once it is done right, this stops being argument. The benefit is to > keep this clock disabled most of the time, which now we lost. > > I don't find this patch correct approach. I've suggested this change while playing with this driver. For me turning AHB clock on/off during normal driver operation seems to be over-engineering and really gives no real power saving benefits, especially if thermal driver is the only one that does such fine-grained clock management (none of the Exynos supported in mainline does that). Removing it simplifies code and makes it easier to understand or read, as the current code already was somehow problematic to understand and unintuitive: https://lore.kernel.org/all/c3258cb2-9a56-d048-5738-1132331a157d@linaro.org/ Taking into account that the driver is not really maintained, making it simpler without noticeable feature loss counts as a benefit for me. Best regards
On 01/09/2023 10:40, Marek Szyprowski wrote: > On 29.08.2023 11:56, Krzysztof Kozlowski wrote: >> On 29/08/2023 11:18, Mateusz Majewski wrote: >>> This clock only controls the register operations. The gain in power >>> efficiency is therefore quite dubious, while there is price of added >>> complexity that is important to get right (as a register operation might >>> outright hang the CPU if the clock is not enabled). >> So once it is done right, this stops being argument. The benefit is to >> keep this clock disabled most of the time, which now we lost. >> >> I don't find this patch correct approach. > > I've suggested this change while playing with this driver. > > For me turning AHB clock on/off during normal driver operation seems to > be over-engineering and really gives no real power saving benefits, > especially if thermal driver is the only one that does such fine-grained > clock management (none of the Exynos supported in mainline does that). > Removing it simplifies code and makes it easier to understand or read, > as the current code already was somehow problematic to understand and > unintuitive: > > https://lore.kernel.org/all/c3258cb2-9a56-d048-5738-1132331a157d@linaro.org/ > > Taking into account that the driver is not really maintained, making it > simpler without noticeable feature loss counts as a benefit for me. Hm, ok, let it be, although I bet once someone will come and start adding runtime PM for clock handling... Best regards, Krzysztof
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index 35b0a55017ad..2c5501704911 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -275,7 +275,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev) } mutex_lock(&data->lock); - clk_enable(data->clk); if (!IS_ERR(data->clk_sec)) clk_enable(data->clk_sec); @@ -305,7 +304,6 @@ static int exynos_tmu_initialize(struct platform_device *pdev) data->tmu_clear_irqs(data); } err: - clk_disable(data->clk); mutex_unlock(&data->lock); if (!IS_ERR(data->clk_sec)) clk_disable(data->clk_sec); @@ -336,10 +334,8 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on) struct exynos_tmu_data *data = platform_get_drvdata(pdev); mutex_lock(&data->lock); - clk_enable(data->clk); data->tmu_control(pdev, on); data->enabled = on; - clk_disable(data->clk); mutex_unlock(&data->lock); } @@ -654,7 +650,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp) return -EAGAIN; mutex_lock(&data->lock); - clk_enable(data->clk); value = data->tmu_read(data); if (value < 0) @@ -662,7 +657,6 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp) else *temp = code_to_temp(data, value) * MCELSIUS; - clk_disable(data->clk); mutex_unlock(&data->lock); return ret; @@ -729,9 +723,7 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp) goto out; mutex_lock(&data->lock); - clk_enable(data->clk); data->tmu_set_emulation(data, temp); - clk_disable(data->clk); mutex_unlock(&data->lock); return 0; out: @@ -769,12 +761,10 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id) thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED); mutex_lock(&data->lock); - clk_enable(data->clk); /* TODO: take action based on particular interrupt */ data->tmu_clear_irqs(data); - clk_disable(data->clk); mutex_unlock(&data->lock); return IRQ_HANDLED; @@ -1012,7 +1002,7 @@ static int exynos_tmu_probe(struct platform_device *pdev) if (ret) goto err_sensor; - data->clk = devm_clk_get(&pdev->dev, "tmu_apbif"); + data->clk = devm_clk_get_enabled(&pdev->dev, "tmu_apbif"); if (IS_ERR(data->clk)) { dev_err(&pdev->dev, "Failed to get clock\n"); ret = PTR_ERR(data->clk); @@ -1034,12 +1024,6 @@ static int exynos_tmu_probe(struct platform_device *pdev) } } - ret = clk_prepare(data->clk); - if (ret) { - dev_err(&pdev->dev, "Failed to get clock\n"); - goto err_clk_sec; - } - switch (data->soc) { case SOC_ARCH_EXYNOS5433: case SOC_ARCH_EXYNOS7: @@ -1047,12 +1031,12 @@ static int exynos_tmu_probe(struct platform_device *pdev) if (IS_ERR(data->sclk)) { dev_err(&pdev->dev, "Failed to get sclk\n"); ret = PTR_ERR(data->sclk); - goto err_clk; + goto err_clk_sec; } else { ret = clk_prepare_enable(data->sclk); if (ret) { dev_err(&pdev->dev, "Failed to enable sclk\n"); - goto err_clk; + goto err_clk_sec; } } break; @@ -1094,8 +1078,6 @@ static int exynos_tmu_probe(struct platform_device *pdev) err_sclk: clk_disable_unprepare(data->sclk); -err_clk: - clk_unprepare(data->clk); err_clk_sec: if (!IS_ERR(data->clk_sec)) clk_unprepare(data->clk_sec); @@ -1113,7 +1095,6 @@ static int exynos_tmu_remove(struct platform_device *pdev) exynos_tmu_control(pdev, false); clk_disable_unprepare(data->sclk); - clk_unprepare(data->clk); if (!IS_ERR(data->clk_sec)) clk_unprepare(data->clk_sec);
This clock only controls the register operations. The gain in power efficiency is therefore quite dubious, while there is price of added complexity that is important to get right (as a register operation might outright hang the CPU if the clock is not enabled). Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> --- drivers/thermal/samsung/exynos_tmu.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-)