On 21/05/2022 11:50, Anand Moon wrote: > Hi Krzysztof, > > On Wed, 18 May 2022 at 12:55, Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> >> On 17/05/2022 20:42, Anand Moon wrote: >>> 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)?" >>>> > > This was just to update my knowledge on what is missing in the driver. > >> I don't understand how all this is relevant to the Exynos TMU driver. >> You paste some COMMON_CLK framework links, but this is just a framework. >> It has nothing to do with Exynos TMU. >> >> Since we are making circles, let's make it clearer. Answer these simple >> questions: >> 1. Is Exynos TMU driver operating correctly or not correctly? > > Yes Exynos TMU clk is getting initialized, but not incorrect order. > within the exynos tmu driver we call > exynos_tmu_probe > ---> clk_prepare > exynos_tmu_initialize > ---> clk_enable > which is seem to work but it does not enable the clk in total. Correct and this is done on purpose, to not have the clock enabled all the time. > > But if we call *clk_prepare_enable* in exynos_tmu_probe we enable the > clk correctly. It was enabled correctly. clk_prepare followed by clk_enabled is correct way. > > *Note:* This current patch is missing the clean-up in > exynos_tmu_initialize function. > >> >> 2. If incorrectly, how is the incorrectness visible? > > See before the change in Exynos 5422 > $ sudo cat /sys/kernel/debug/clk/clk_summary | grep tmu > tmu_gpu 0 2 0 66600000 > 0 0 50000 N > tmu 0 6 0 66600000 > 0 0 50000 N > > $ sudo cat /sys/kernel/debug/clk/clk_summary | grep tmu > tmu_gpu 2 2 0 66600000 > 0 0 50000 Y > tmu 6 6 0 66600000 > 0 0 50000 Y > > After the changes, the internal tmu clk internal hardware flag is set to 'Y' > * hence I mention this in the commit message.* > > Before the patch > # cat /sys/kernel/debug/clk/tmu/clk_enable_count > 0 > # cat /sys/kernel/debug/clk/tmu_gpu/clk_enable_count > 0 > > After the patch > # cat /sys/kernel/debug/clk/tmu/clk_enable_count > 6 > # cat /sys/kernel/debug/clk/tmu_gpu/clk_enable_count > 2 This proves your patch is incorrect, because you enabled clock for times when it is not needed. Original code looks ok. > >> How can we trigger and see the issue? > > We can trigger or see the issue but enable clk trace feature, > for example trace clk_enable, clk_prepare clk_enable_complete > > I don't know how to trace clk during clk initialization > but I will try to find out more about this. > >> >> 3. If it operates correctly, maybe it is operating in nonoptimal way? >> > Few new things we could set in this TMU driver which control the internal timing > > SAMPLING_INTERVAL - sample interval > COUNTER_VALUE0 - Timing control of T_EN_TEMP_SEN on/off timing > COUNTER_VALUE1 - Timing control of CLK_SENSE on/off timing I don't understand this. Again, where is the non-optimal way? > >> 4. If it is not optimal, then what states are not optimal and when? > > We could drop the unnecessary clk_enable and clk_disable as we don't check > the return value of the function and it just toggles the clock which > does not look optimal. No, you don't understand the clocks. Enabling and disabling the clock is optimal. > > Since CLK_SENSE internally has a timer to on/off and control the PMU operations. This could be better, what is this CLK_SENSE and which clocks are affected? > Look at following functions we could drop this > exynos_get_temp , exynos_tmu_control and exynos_tmu_set_emulation. I don't understand this sentence. Why do you want to drop entire functions? How is exynos_get_temp related to clocks? Best regards, Krzysztof
