| Message ID | 20230829091853.626011-1-m.majewski2@samsung.com |
|---|---|
| Headers | show |
| Series | Improve Exynos thermal driver | expand |
On 29/08/2023 11:18, Mateusz Majewski wrote: > This field is not used in code, and seems to not have any meaning; in my > tests, the value was always 0. > > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- Please drop also remaining alias from DTS. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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:18, Mateusz Majewski wrote: > After having switched to dynamic trip points, we no longer have a > hardware limit for trip point count and can support as many as we want > without polling. > > Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com> > --- > .../dts/samsung/exynos5422-odroidxu3-common.dtsi | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > This will need to wait for release after driver changes get accepted. You sent your patchset just after merge window started, so unfortunately this means it will get in 2 releases. 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
This work improves Exynos thermal driver in various ways. This is related to the discussion in https://lore.kernel.org/all/97201878-3bb8-eac5-7fac-a690322ac43a@linaro.org/ The primary issue being fixed is a lockdep warning, which is fixed by the thermal: exynos: use set_trips patch. We also handle Exynos 4210 not supporting falling temperature thresholds by enabling polling for this SoC, and simplify the code in general. Mateusz Majewski (11): ARM: dts: exynos: enable polling in Exynos 4210 thermal: exynos: drop id field thermal: exynos: switch from workqueue-driven interrupt handling to threaded interrupts thermal: exynos: remove fine-grained clk management thermal: exynos: simplify sclk (de)initialization thermal: exynos: simplify regulator (de)initialization thermal: exynos: simplify clk_sec (de)initialization thermal: exynos: stop using the threshold mechanism on Exynos 4210 thermal: exynos: split initialization of TMU and the thermal zone thermal: exynos: use set_trips ARM: dts: exynos: disable polling in Odroid XU3-related devices arch/arm/boot/dts/samsung/exynos4210.dtsi | 10 +- .../samsung/exynos5422-odroidxu3-common.dtsi | 16 +- drivers/thermal/samsung/exynos_tmu.c | 581 ++++++++---------- 3 files changed, 284 insertions(+), 323 deletions(-)