Hi Krzysztof, On Sun, 15 May 2022 at 15:22, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 15/05/2022 08:41, Anand Moon wrote: > > Use clk_prepare_enable api to enable tmu internal hardware clock > > flag on, use clk_disable_unprepare to disable the clock. > > > > Cc: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> > > Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx> > > Here as well you ignored my first comment: > https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@xxxxxxxxxxxxxx/T/#mbfc57b40a7ed043dd4d4890bedb6bad8240058cd > > "This is not valid reason to do a change. What is clk_summary does not > really matter. Your change has negative impact on power consumption as > the clock stays enabled all the time. This is not what we want... so > please explain it more - why you need the clock to be enabled all the > time? What is broken (clk_summary is not broken in this case)?" > Ok, I fall short to understand the clock framework. > This was not addressed, you just resent same code... > Thanks for the review comment. Here is the Exynos4412 user manual I am referring to get a better understanding of TMU drivers [0] https://chasinglulu.github.io/downloads/SEC_Exynos4412_Users%20Manual_Ver.1.00.00.pdf Exynos Thermal is controlled by CLK_SENSE field is toggled on/off by the TMU for rising and falling temperatures which control the interrupt. TMU monitors temperature variation in a chip by measuring on-chip temperature and generates an interrupt to CPU when the temperature exceeds or goes below pre-defined threshold. Instead of using an interrupt generation scheme, CPU can obtain on-chip temperature information by reading the related register field, that is, by using a polling scheme. tmu clk control the CPU freq with various power management like DVFS and MFC so this clk needs to be *enabled on*, If we use clk_prepare_enable API we enable the clk and the clk counters, I check the call trace of the *clk_prepare_enable* [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/clk.h?h=v5.18-rc7#n945 it first calls *clk_prepare* and then *clk_enable* functions to enable the clock and then the hardware flag gets enabled for the clock I also check the call trace of the *clk_prepare* below [2} https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk.c?h=v5.18-rc7#n943 it does not enable the clk explicitly but prepares the clock to be used. "clk_prepare can be used instead of clk_enable to ungate a clk if the operation may sleep. One example is a clk which is accessed over I2c. In the complex case a clk ungate operation may require a fast and a slow part. It is this reason that clk_prepare and clk_enable are not mutually exclusive. In fact clk_prepare must be called before clk_enable. Returns 0 on success, -EERROR otherwise." What it means is we still need to call *clk_enable* to enable clk in the drivers and *clk_disable* to disable within the driver. In exynos tmu driver uses many clk_enable and clk_disable to toggle the clock which we can avoid if we used the switch to used *clk_prepare_enable* function in the probe function. [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/samsung/exynos_tmu.c?h=v5.18-rc7#n259 [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/samsung/exynos_tmu.c?h=v5.18-rc7#n350 [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/samsung/exynos_tmu.c?h=v5.18-rc7#n653 [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/samsung/exynos_tmu.c?h=v5.18-rc7#n731 Locally I remove these function calls of clk_enable/ clk_disable function calls in the driver with these changes, stress-tested did not find any lockup. Please correct me if I am wrong. > > Best regards, > Krzysztof Thanks & Regards -Anand
