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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 18 May 2022 00:12:27 +0530
From:   Anand Moon <linux.amoon@...il.com>
To:     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

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.

Please correct me if I am wrong.

>
> Best regards,
> Krzysztof

Thanks & Regards

-Anand