mbox series

[PATCHv2,0/6] Exynos Thermal code inprovement

Message ID 20220515064126.1424-1-linux.amoon@gmail.com
Headers show
Series Exynos Thermal code inprovement | expand

Message

Anand Moon May 15, 2022, 6:41 a.m. UTC
Hi All 

This patch series is bit of rework on my previous
patch series [0], where I failed to justify the code changes.

With this new series I have tried to improve the commit subject
and commit message.

Added new patchs are added to improve the PM suspend/resume and 
added runtime power management support to exynos tmu driver.
  
[0] https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m77e57120d230d57f34c29e1422d7fc5f5587ac30

Best Regards
-Anand

Anand Moon (6):
  thermal: exynos: Enable core tmu hardware clk flag on exynos platform
  thermal: exynos: Reorder the gpu clock initialization for exynos5420
    SoC
  thermal: exynos: Check before clk_disable_unprepare() not needed
  thermal: exynos: fixed the efuse min/max value for exynos5422
  thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
  thermal: exynos: Add runtime power management for tmu

 drivers/thermal/samsung/exynos_tmu.c | 107 ++++++++++++++++-----------
 1 file changed, 62 insertions(+), 45 deletions(-)


base-commit: ec7f49619d8ee13e108740c82f942cd401b989e9

Comments

Krzysztof Kozlowski May 15, 2022, 9:39 a.m. UTC | #1
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.

Please explain why this is needed. Each change needs explanation why you
are doing it.


Best regards,
Krzysztof
Krzysztof Kozlowski May 15, 2022, 9:43 a.m. UTC | #2
On 15/05/2022 08:41, Anand Moon wrote:
> All code in clk_disable_unprepare() already checks the clk ptr using
> IS_ERR_OR_NULL so there is no need to check it again before calling it.
> A lot of other drivers already rely on this behaviour, so it's safe
> to do so here.
> 
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v1: improve the commit message
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 1ef90dc52c08..58ff1b577c47 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -289,8 +289,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>  
>  	mutex_lock(&data->lock);
>  	clk_enable(data->clk);
> -	if (!IS_ERR(data->clk_sec))
> -		clk_enable(data->clk_sec);
> +	clk_enable(data->clk_sec);

You say that clk_enable() checks for IS_ERR_OR_NULL. Where? I see only
check for non-null case and then immediately taking clk prepare lock.

This looks buggy... did you test it?

Best regards,
Krzysztof
Krzysztof Kozlowski May 15, 2022, 9:47 a.m. UTC | #3
On 15/05/2022 08:41, Anand Moon wrote:
> Use the newlly introduced pm_sleep_ptr() macro, and mark the

s/newlly/newly/

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Krzysztof Kozlowski May 15, 2022, 9:52 a.m. UTC | #4
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 not addressed, you just resent same code...


Best regards,
Krzysztof
Anand Moon May 17, 2022, 6:42 p.m. UTC | #5
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
Anand Moon May 17, 2022, 6:44 p.m. UTC | #6
Hi Krzysztof,

On Sun, 15 May 2022 at 15:13, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > All code in clk_disable_unprepare() already checks the clk ptr using
> > IS_ERR_OR_NULL so there is no need to check it again before calling it.
> > A lot of other drivers already rely on this behaviour, so it's safe
> > to do so here.
> >
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v1: improve the commit message
> > ---
> >  drivers/thermal/samsung/exynos_tmu.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index 1ef90dc52c08..58ff1b577c47 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -289,8 +289,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >
> >       mutex_lock(&data->lock);
> >       clk_enable(data->clk);
> > -     if (!IS_ERR(data->clk_sec))
> > -             clk_enable(data->clk_sec);
> > +     clk_enable(data->clk_sec);
>
> You say that clk_enable() checks for IS_ERR_OR_NULL. Where? I see only
> check for non-null case and then immediately taking clk prepare lock.
>
> This looks buggy... did you test it?

Thanks for your review comments
Yes have tested the changes, this was last-minute changes
will drop this in the next version.

>
> Best regards,
> Krzysztof

Thanks & Regards


-Anand
Anand Moon May 17, 2022, 6:44 p.m. UTC | #7
Hi Krzysztof,

On Sun, 15 May 2022 at 15:17, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > Use the newlly introduced pm_sleep_ptr() macro, and mark the
>
> s/newlly/newly/
>
Ok, I will fix this next version.

> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Best regards,
> Krzysztof

Thanks & Regards

-Anand
Krzysztof Kozlowski May 18, 2022, 7:25 a.m. UTC | #8
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)?"
>>
> 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
Anand Moon May 21, 2022, 9:50 a.m. UTC | #9
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
Krzysztof Kozlowski May 21, 2022, 2:15 p.m. UTC | #10
On 21/05/2022 11:50, Anand Moon wrote:
> 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.

Correct and this is done on purpose, to not have the clock enabled all
the time.
> 
> But if we call *clk_prepare_enable* in  exynos_tmu_probe we enable the
> clk correctly.

It was enabled correctly. clk_prepare followed by clk_enabled is correct
way.

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

This proves your patch is incorrect, because you enabled clock for times
when it is not needed. Original code looks ok.

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

I don't understand this. Again, where is the non-optimal way?

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

No, you don't understand the clocks. Enabling and disabling the clock is
optimal.

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

This could be better, what is this CLK_SENSE and which clocks are affected?

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

I don't understand this sentence. Why do you want to drop entire
functions? How is exynos_get_temp related to clocks?


Best regards,
Krzysztof