Exynos Thermal code inprovement

Use clk_prepare_enable api to enable tmu internal hardware clock flag on, use clk_disable_unprepare to disable the clock.

Use clk_prepare_enable api to enable tmu internal hardware clock flag on, use clk_disable_unprepare to disable the clock.

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

Quoted 5 lines · Anand Moon

Use clk_prepare_enable api to enable tmu internal hardware clock flag on, use clk_disable_unprepare to disable the clock.

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

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

Hi Krzysztof,

Quoted 8 lines · Krzysztof Kozlowski

Quoted 5 lines · Anand Moon

Use clk_prepare_enable api to enable tmu internal hardware clock flag on, use clk_disable_unprepare to disable the clock.

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

Quoted 9 lines

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.

Quoted 2 lines

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.

Quoted 15 lines · Anand Moon

Hi Krzysztof,

Quoted 11 lines · Krzysztof Kozlowski

Quoted 5 lines · Anand Moon

Use clk_prepare_enable api to enable tmu internal hardware clock flag on, use clk_disable_unprepare to disable the clock.

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

Here as well you ignored my first comment:
https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#mbfc57b40a7ed043dd4d4890bedb6bad8240058cd

Quoted 6 lines

Quoted 6 lines

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

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

Hi Krzysztof,

Quoted 17 lines · Krzysztof Kozlowski

Quoted 15 lines · Anand Moon

Hi Krzysztof,

Quoted 11 lines · Krzysztof Kozlowski

Quoted 5 lines · Anand Moon

Use clk_prepare_enable api to enable tmu internal hardware clock flag on, use clk_disable_unprepare to disable the clock.

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

Here as well you ignored my first comment:
https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#mbfc57b40a7ed043dd4d4890bedb6bad8240058cd

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 1 line

Quoted 2 lines

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

Quoted 2 lines

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.

Reorder the tmu_gpu clock initialization for exynos5422 SoC.

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
v1: split the changes and improve the commit messages

---
 drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
 1 file changed, 21 insertions(+), 22 deletions(-)

drivers/thermal/samsung/exynos_tmu.c +0 -2

index 75b3afadb5be..1ef90dc52c08 100644
@@ -1044,42 +1044,41 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to get clock\n");
 		ret = PTR_ERR(data->clk);
 		goto err_sensor;
-	}
-

Quoted 1 line · Anand Moon

Reorder the tmu_gpu clock initialization for exynos5422 SoC.

Why?

Quoted 25 lines

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
v1: split the changes and improve the commit messages

---
 drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
 1 file changed, 21 insertions(+), 22 deletions(-)

drivers/thermal/samsung/exynos_tmu.c +0 -7

index 75b3afadb5be..1ef90dc52c08 100644
@@ -1044,42 +1044,41 @@ static int exynos_tmu_probe(struct platform_device *pdev)
              dev_err(&pdev->dev, "Failed to get clock\n");
              ret = PTR_ERR(data->clk);
              goto err_sensor;
-     }
-
-     data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
-     if (IS_ERR(data->clk_sec)) {
-             if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
-                     dev_err(&pdev->dev, "Failed to get triminfo clock\n");
-                     ret = PTR_ERR(data->clk_sec);

drivers/thermal/samsung/exynos_tmu.c static int exynos_tmu_probe(struct platform_device *pdev) · line 1048

Quoted 5 lines

drivers/thermal/samsung/exynos_tmu.c +1 -3

-                     goto err_sensor;
-             }
      } else {
-             ret = clk_prepare_enable(data->clk_sec);
+             ret = clk_prepare_enable(data->clk);

This looks a bit odd. The clock was before taken unconditionally, not within "else" branch...

Best regards,
Krzysztof

Hi Krzysztof,

Quoted 5 lines · Krzysztof Kozlowski

Quoted 1 line · Anand Moon

Reorder the tmu_gpu clock initialization for exynos5422 SoC.

Why?

It just code reorder

Quoted 25 lines

Quoted 25 lines

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
v1: split the changes and improve the commit messages

---
 drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
 1 file changed, 21 insertions(+), 22 deletions(-)

drivers/thermal/samsung/exynos_tmu.c +0 -7

index 75b3afadb5be..1ef90dc52c08 100644
@@ -1044,42 +1044,41 @@ static int exynos_tmu_probe(struct platform_device *pdev)
              dev_err(&pdev->dev, "Failed to get clock\n");
              ret = PTR_ERR(data->clk);
              goto err_sensor;
-     }
-
-     data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
-     if (IS_ERR(data->clk_sec)) {
-             if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
-                     dev_err(&pdev->dev, "Failed to get triminfo clock\n");
-                     ret = PTR_ERR(data->clk_sec);

Quoted 8 lines

drivers/thermal/samsung/exynos_tmu.c static int exynos_tmu_probe(struct platform_device *pdev) · line 1048

Quoted 5 lines

drivers/thermal/samsung/exynos_tmu.c +1 -3

-                     goto err_sensor;
-             }
      } else {
-             ret = clk_prepare_enable(data->clk_sec);
+             ret = clk_prepare_enable(data->clk);

This looks a bit odd. The clock was before taken unconditionally, not within "else" branch...

The whole *clk_sec* ie tmu_triminfo_apbif clock enable is being moved down to the switch case.
tmu_triminfo_apbif clock is not used by Exynos4412 and Exynos5433 and Exynos7 SoC.

Quoted 8 lines · Anand Moon

Hi Krzysztof,

Quoted 4 lines · Krzysztof Kozlowski

Quoted 1 line · Anand Moon

Reorder the tmu_gpu clock initialization for exynos5422 SoC.

Quoted 2 lines

Quoted 1 line

Why?

It just code reorder

I know what it is. I asked why. The answer in English to question "Why" is starting with "Because".

You repeated again the argument what are you doing to my question "Why are you doing it".

It was the same before, many, many times. It's a waste of reviewers time, because you receive a review and you do not implement the feedback.

Quoted 34 lines

Quoted 33 lines

Quoted 20 lines

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
v1: split the changes and improve the commit messages

---
 drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
 1 file changed, 21 insertions(+), 22 deletions(-)

drivers/thermal/samsung/exynos_tmu.c +0 -2

index 75b3afadb5be..1ef90dc52c08 100644
@@ -1044,42 +1044,41 @@ static int exynos_tmu_probe(struct platform_device *pdev)
              dev_err(&pdev->dev, "Failed to get clock\n");
              ret = PTR_ERR(data->clk);
              goto err_sensor;
-     }
-

drivers/thermal/samsung/exynos_tmu.c static int exynos_tmu_probe(struct platform_device *pdev) · line 1048

Quoted 10 lines

drivers/thermal/samsung/exynos_tmu.c +1 -8

-     data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
-     if (IS_ERR(data->clk_sec)) {
-             if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
-                     dev_err(&pdev->dev, "Failed to get triminfo clock\n");
-                     ret = PTR_ERR(data->clk_sec);
-                     goto err_sensor;
-             }
      } else {
-             ret = clk_prepare_enable(data->clk_sec);
+             ret = clk_prepare_enable(data->clk);

This looks a bit odd. The clock was before taken unconditionally, not within "else" branch...

Quoted 4 lines

The whole *clk_sec* ie tmu_triminfo_apbif clock enable is being moved down to the switch case.
tmu_triminfo_apbif clock is not used by Exynos4412 and Exynos5433 and Exynos7 SoC.

This is not the answer. Why are you preparing data->clk within else{} branch?

Best regards,
Krzysztof

Hi Krzysztof,

Quoted 10 lines · Krzysztof Kozlowski

Quoted 8 lines · Anand Moon

Hi Krzysztof,

Quoted 4 lines · Krzysztof Kozlowski

Quoted 1 line · Anand Moon

Reorder the tmu_gpu clock initialization for exynos5422 SoC.

Quoted 9 lines

Quoted 2 lines

Quoted 1 line

Why?

It just code reorder

I know what it is. I asked why. The answer in English to question "Why" is starting with "Because".

You repeated again the argument what are you doing to my question "Why are you doing it".

tmu_triminfo_apbif is not a core driver to all the Exynos SOC board it is only used by the Exynos542x SOC family

If we look into the original code its place in between
the devm_clk_get(data->clk) and clk_prepare(data->clk) after this change, the code is in the correct order of initialization of the clock.

Quoted 38 lines

It was the same before, many, many times. It's a waste of reviewers time, because you receive a review and you do not implement the feedback.

Quoted 34 lines

Quoted 33 lines

Quoted 20 lines

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
v1: split the changes and improve the commit messages

---
 drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
 1 file changed, 21 insertions(+), 22 deletions(-)

drivers/thermal/samsung/exynos_tmu.c +0 -2

index 75b3afadb5be..1ef90dc52c08 100644
@@ -1044,42 +1044,41 @@ static int exynos_tmu_probe(struct platform_device *pdev)
              dev_err(&pdev->dev, "Failed to get clock\n");
              ret = PTR_ERR(data->clk);
              goto err_sensor;
-     }
-

drivers/thermal/samsung/exynos_tmu.c static int exynos_tmu_probe(struct platform_device *pdev) · line 1048

Quoted 10 lines

drivers/thermal/samsung/exynos_tmu.c +1 -8

-     data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
-     if (IS_ERR(data->clk_sec)) {
-             if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
-                     dev_err(&pdev->dev, "Failed to get triminfo clock\n");
-                     ret = PTR_ERR(data->clk_sec);
-                     goto err_sensor;
-             }
      } else {
-             ret = clk_prepare_enable(data->clk_sec);
+             ret = clk_prepare_enable(data->clk);

This looks a bit odd. The clock was before taken unconditionally, not within "else" branch...

Quoted 8 lines

Quoted 4 lines

The whole *clk_sec* ie tmu_triminfo_apbif clock enable is being moved down to the switch case.
tmu_triminfo_apbif clock is not used by Exynos4412 and Exynos5433 and Exynos7 SoC.

This is not the answer. Why are you preparing data->clk within else{} branch?

After cleanly applying the patches I see the below changes. if you want me to remove the else part below and keep the original code I am ok.

        data->clk = devm_clk_get(&pdev->dev, "tmu_apbif");
        if (IS_ERR(data->clk)) {
                dev_err(&pdev->dev, "Failed to get clock\n");
                ret = PTR_ERR(data->clk);
                goto err_sensor;
        } else {
                ret = clk_prepare_enable(data->clk);

Reorder the tmu_gpu clock initialization for exynos5422 SoC.

Quoted 1 line · Anand Moon

Reorder the tmu_gpu clock initialization for exynos5422 SoC.

Some more thoughts: where is the GPU here? This is a TMU driver... In subject you also describe GPU.

My comments about unusual code order from [1] were not answered.

https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m3edd1fa357eb3e921e51eb09e2e32d68496332eb

Please respond/address to all comments before resending.

Best regards,
Krzysztof

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 <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
v1: improve the commit message

---
 drivers/thermal/samsung/exynos_tmu.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/thermal/samsung/exynos_tmu.c +0 -0

index 1ef90dc52c08..58ff1b577c47 100644

Quoted 18 lines · Anand Moon

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 <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
v1: improve the commit message

---
 drivers/thermal/samsung/exynos_tmu.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/thermal/samsung/exynos_tmu.c +0 -0

index 1ef90dc52c08..58ff1b577c47 100644

drivers/thermal/samsung/exynos_tmu.c static int exynos_tmu_initialize(struct platform_device *pdev) · line 292

Quoted 6 lines

drivers/thermal/samsung/exynos_tmu.c +1 -2

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

As per Exynos5422 user manaul e-Fuse range min~max range is 16~76. if e-Fuse value is out of this range, then thermal sensor may not sense thermal data properly.

Cc Bartlomiej Zolnierkiewicz <[email protected]>

Signed-off-by Anand Moon <[email protected]>

Hi Anand,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
base:   ec7f49619d8ee13e108740c82f942cd401b989e9
config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 1 line

Quoted 1 line

drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]

                   if (data->soc == SOC_ARCH_EXYNOS5420 &&
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

1 warning generated.

Quoted 26 lines · kernel test robot

Hi Anand,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
base:   ec7f49619d8ee13e108740c82f942cd401b989e9
config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp(a)intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 4 lines

Quoted 1 line

Quoted 1 line

drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]

                   if (data->soc == SOC_ARCH_EXYNOS5420 &&
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

1 warning generated.

Ah, I did not notice it and it seems code was not compile-tested with W=1.

Anand, please be sure you compile your code with W=1...

Best regards,
Krzysztof

Use the newlly introduced pm_sleep_ptr() macro, and mark the

Quoted 1 line · Anand Moon

Use the newlly introduced pm_sleep_ptr() macro, and mark the

s/newlly/newly/

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

Add runtime power management for exynos thermal driver.

Quoted 1 line · Anand Moon

Add runtime power management for exynos thermal driver.

First of all - why? Second, I do not see it being added. Where are the runtime callbacks?

Best regards,
Krzysztof

Hi Krzysztof,

Quoted 1 line · Krzysztof Kozlowski

Quoted 6 lines · Anand Moon

Quoted 1 line

Add runtime power management for exynos thermal driver.

First of all - why? Second, I do not see it being added. Where are the runtime callbacks?

To control runtime control PMU, did I miss something?

I looked into imx thermal driver # drivers/thermal/imx_thermal.c to enable run-time power management for exynos driver.

Quoted 8 lines · Anand Moon

Hi Krzysztof,

Quoted 4 lines · Krzysztof Kozlowski

Quoted 1 line · Anand Moon

Add runtime power management for exynos thermal driver.

Quoted 5 lines

Quoted 3 lines

First of all - why? Second, I do not see it being added. Where are the runtime callbacks?

To control runtime control PMU, did I miss something?

Controlling runtime PM by itself is not a goal. What does it change if it is enabled?

Quoted 2 lines

I looked into imx thermal driver # drivers/thermal/imx_thermal.c to enable run-time power management for exynos driver.

So you have runtime PM enabled and then what happens? Where is the power saving? Since you did not implement the callbacks, all this should be explained in commit msg.

Best regards,
Krzysztof

Hi Krzysztof,

Quoted 14 lines · Krzysztof Kozlowski

Quoted 12 lines · Anand Moon

Hi Krzysztof,

Quoted 7 lines · Krzysztof Kozlowski

Quoted 1 line · Anand Moon

Add runtime power management for exynos thermal driver.

First of all - why? Second, I do not see it being added. Where are the runtime callbacks?

Quoted 5 lines

Quoted 1 line

To control runtime control PMU, did I miss something?

Controlling runtime PM by itself is not a goal. What does it change if it is enabled?

It means we could have efficient power management for this driver. as per my understanding, it controls runtime sleep and improves power efficiency

Quoted 8 lines

Quoted 3 lines

I looked into imx thermal driver # drivers/thermal/imx_thermal.c to enable run-time power management for exynos driver.

So you have runtime PM enabled and then what happens? Where is the power saving? Since you did not implement the callbacks, all this should be explained in commit msg.

Ok, As per the original code, it just registers the SIMPLE_DEV_PM_OPS with .pm = &exynos_tmu_pm

So I have made sure that suspend resume feature works correctly
with these changes on SBC Odroid U3 and XU4.

I will try to look into setting RUNTIME_PM_OPS or use UNIVERSAL_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS any thought on this?