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

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

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)?"

Ok, I fall short to understand the clock framework.

quoted

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.

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?

2. If incorrectly, how is the incorrectness visible? How can we trigger
and see the issue?

3. If it operates correctly, maybe it is operating in nonoptimal way?

4. If it is not optimal, then what states are not optimal and when?

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

Always, always please explain why this change is needed.

Best regards,
Krzysztof

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