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> <CANAwSgTMwKhn6QDcoOXcSVgUqLGq1W6X4QvcSRHT-JSVfOe0rA@mail.gmail.com>
 <39336391-8882-b2cb-058f-18f59dcc90d3@linaro.org>
In-Reply-To: <39336391-8882-b2cb-058f-18f59dcc90d3@linaro.org>
From:   Anand Moon <linux.amoon@gmail.com>
Date:   Sat, 21 May 2022 15:20:51 +0530
Message-ID: <CANAwSgT0Vmq4D9nRNc9h=xXxjmN_wXjbhwjj4btX+4SkXePkrw@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 Wed, 18 May 2022 at 12:55, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/05/2022 20:42, Anand Moon wrote:
> > 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)?"
> >>

This was just to update my knowledge on what is missing in the driver.

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

But if we call *clk_prepare_enable* in  exynos_tmu_probe we enable the
clk correctly.

*Note:* This current patch is missing the clean-up in
exynos_tmu_initialize function.

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

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

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

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

Since CLK_SENSE internally has a timer to on/off and control the PMU operations.

Look at following functions we could drop this
exynos_get_temp , exynos_tmu_control and exynos_tmu_set_emulation.

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

Some time is a bit hard for me to explain the feature changes in a
crisp clean way.
I will try to correct myself on this. Please try to understand this I am
just trying to improve the code.

> Always, always please explain why this change is needed.
Ok.

>
> Best regards,
> Krzysztof

Thanks & Regards


-Anand