Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 21/05/2022 11:50, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Wed, 18 May 2022 at 12:55, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/05/2022 20:42, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On Sun, 15 May 2022 at 15:22, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> 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@gmail.com>
>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>
>>>> Here as well you ignored my first comment:
>>>> https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/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