Date:   Wed, 18 May 2022 09:25:27 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Anand Moon <linux.amoon@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amitk@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Linux PM list <linux-pm@...r.kernel.org>,
        linux-samsung-soc@...r.kernel.org,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel <linux-kernel@...r.kernel.org>
Subject: 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
> <krzysztof.kozlowski@...aro.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@...il.com>
>>> Signed-off-by: Anand Moon <linux.amoon@...il.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)?"
>>
> Ok, I fall short to understand the clock framework.
> 
>> 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

Powered by blists - more mailing lists