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

On 21/05/2022 11:50, Anand Moon wrote:

Hi Krzysztof,

On Wed, 18 May 2022 at 12:55, Krzysztof Kozlowski
[off-list ref] wrote:

quoted

On 17/05/2022 20:42, Anand Moon wrote:

quoted

Hi Krzysztof,

On Sun, 15 May 2022 at 15:22, Krzysztof Kozlowski
[off-list ref] wrote:

quoted

On 15/05/2022 08:41, Anand Moon wrote:

quoted

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 <redacted>

Here as well you ignored my first comment:
https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#mbfc57b40a7ed043dd4d4890bedb6bad8240058cd (local)

"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.

quoted

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.

quoted

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.

quoted

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.

quoted

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?

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel