Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220515064126.1424-1-linux.amoon@gmail.com> <20220515064126.1424-2-linux.amoon@gmail.com>
 <6a6ed76a-50fa-2b05-896e-8936d3c3f597@linaro.org>
In-Reply-To: <6a6ed76a-50fa-2b05-896e-8936d3c3f597@linaro.org>
From:   Anand Moon <linux.amoon@gmail.com>
Date:   Wed, 18 May 2022 00:12:27 +0530
Message-ID: <CANAwSgTMwKhn6QDcoOXcSVgUqLGq1W6X4QvcSRHT-JSVfOe0rA@mail.gmail.com>
Subject: Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag
 on exynos platform
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Linux PM list <linux-pm@vger.kernel.org>,
        linux-samsung-soc@vger.kernel.org,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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