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. But if we call *clk_prepare_enable* in exynos_tmu_probe we enable the clk correctly. *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 > 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 > 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. Since CLK_SENSE internally has a timer to on/off and control the PMU operations. Look at following functions we could drop this exynos_get_temp , exynos_tmu_control and exynos_tmu_set_emulation. > > In any case you commit fails to explain WHY you are doing it. I > explained you this over the years several times and after these several > times you still do not like to answer that simple question. This is > pointless. You receive feedback and keep it ignored... > Some time is a bit hard for me to explain the feature changes in a crisp clean way. I will try to correct myself on this. Please try to understand this I am just trying to improve the code. > Always, always please explain why this change is needed. Ok. > > Best regards, > Krzysztof Thanks & Regards -Anand
