* [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform
2022-05-15 6:41 [PATCHv2 0/6] Exynos Thermal code inprovement Anand Moon
@ 2022-05-15 6:41 ` Anand Moon
2022-05-15 9:39 ` Krzysztof Kozlowski
2022-05-15 9:52 ` Krzysztof Kozlowski
2022-05-15 6:41 ` [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC Anand Moon
` (4 subsequent siblings)
5 siblings, 2 replies; 33+ messages in thread
From: Anand Moon @ 2022-05-15 6:41 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: 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 <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: split te changes and improve the commit message.
---
drivers/thermal/samsung/exynos_tmu.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f4ab4c5b4b62..75b3afadb5be 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1054,14 +1054,14 @@ static int exynos_tmu_probe(struct platform_device *pdev)
goto err_sensor;
}
} else {
- ret = clk_prepare(data->clk_sec);
+ ret = clk_prepare_enable(data->clk_sec);
if (ret) {
dev_err(&pdev->dev, "Failed to get clock\n");
goto err_sensor;
}
}
- ret = clk_prepare(data->clk);
+ ret = clk_prepare_enable(data->clk);
if (ret) {
dev_err(&pdev->dev, "Failed to get clock\n");
goto err_clk_sec;
@@ -1122,10 +1122,10 @@ static int exynos_tmu_probe(struct platform_device *pdev)
err_sclk:
clk_disable_unprepare(data->sclk);
err_clk:
- clk_unprepare(data->clk);
+ clk_disable_unprepare(data->clk);
err_clk_sec:
if (!IS_ERR(data->clk_sec))
- clk_unprepare(data->clk_sec);
+ clk_disable_unprepare(data->clk_sec);
err_sensor:
if (!IS_ERR(data->regulator))
regulator_disable(data->regulator);
@@ -1142,9 +1142,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
exynos_tmu_control(pdev, false);
clk_disable_unprepare(data->sclk);
- clk_unprepare(data->clk);
+ clk_disable_unprepare(data->clk);
if (!IS_ERR(data->clk_sec))
- clk_unprepare(data->clk_sec);
+ clk_disable_unprepare(data->clk_sec);
if (!IS_ERR(data->regulator))
regulator_disable(data->regulator);
--
2.36.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform
2022-05-15 6:41 ` [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform Anand Moon
@ 2022-05-15 9:39 ` Krzysztof Kozlowski
2022-05-15 9:52 ` Krzysztof Kozlowski
1 sibling, 0 replies; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:39 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform
2022-05-15 6:41 ` [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform Anand Moon
2022-05-15 9:39 ` Krzysztof Kozlowski
@ 2022-05-15 9:52 ` Krzysztof Kozlowski
2022-05-17 18:42 ` Anand Moon
1 sibling, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:52 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform
2022-05-15 9:52 ` Krzysztof Kozlowski
@ 2022-05-17 18:42 ` Anand Moon
2022-05-18 7:25 ` Krzysztof Kozlowski
0 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-17 18:42 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform
2022-05-17 18:42 ` Anand Moon
@ 2022-05-18 7:25 ` Krzysztof Kozlowski
2022-05-21 9:50 ` Anand Moon
0 siblings, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-18 7:25 UTC (permalink / raw)
To: Anand Moon, Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform
2022-05-18 7:25 ` Krzysztof Kozlowski
@ 2022-05-21 9:50 ` Anand Moon
2022-05-21 14:15 ` Krzysztof Kozlowski
0 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-21 9:50 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform
2022-05-21 9:50 ` Anand Moon
@ 2022-05-21 14:15 ` Krzysztof Kozlowski
0 siblings, 0 replies; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-21 14:15 UTC (permalink / raw)
To: Anand Moon
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-15 6:41 [PATCHv2 0/6] Exynos Thermal code inprovement Anand Moon
2022-05-15 6:41 ` [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform Anand Moon
@ 2022-05-15 6:41 ` Anand Moon
2022-05-15 9:41 ` Krzysztof Kozlowski
2022-05-15 9:50 ` Krzysztof Kozlowski
2022-05-15 6:41 ` [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed Anand Moon
` (3 subsequent siblings)
5 siblings, 2 replies; 33+ messages in thread
From: Anand Moon @ 2022-05-15 6:41 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: Anand Moon
Reorder the tmu_gpu clock initialization for exynos5422 SoC.
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: split the changes and improve the commit messages
---
drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 75b3afadb5be..1ef90dc52c08 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -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);
- goto err_sensor;
- }
} else {
- ret = clk_prepare_enable(data->clk_sec);
+ ret = clk_prepare_enable(data->clk);
if (ret) {
dev_err(&pdev->dev, "Failed to get clock\n");
goto err_sensor;
}
}
- ret = clk_prepare_enable(data->clk);
- if (ret) {
- dev_err(&pdev->dev, "Failed to get clock\n");
- goto err_clk_sec;
- }
-
switch (data->soc) {
+ case SOC_ARCH_EXYNOS5420_TRIMINFO:
+ data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
+ if (IS_ERR(data->clk_sec)) {
+ dev_err(&pdev->dev, "Failed to get triminfo clock\n");
+ ret = PTR_ERR(data->clk_sec);
+ goto err_clk_apbif;
+ } else {
+ ret = clk_prepare_enable(data->clk_sec);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to get clock\n");
+ goto err_clk_apbif;
+ }
+ }
+ break;
case SOC_ARCH_EXYNOS5433:
case SOC_ARCH_EXYNOS7:
data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk");
if (IS_ERR(data->sclk)) {
dev_err(&pdev->dev, "Failed to get sclk\n");
ret = PTR_ERR(data->sclk);
- goto err_clk;
+ goto err_clk_sec;
} else {
ret = clk_prepare_enable(data->sclk);
if (ret) {
dev_err(&pdev->dev, "Failed to enable sclk\n");
- goto err_clk;
+ goto err_clk_sec;
}
}
break;
@@ -1119,13 +1118,13 @@ static int exynos_tmu_probe(struct platform_device *pdev)
err_thermal:
thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
-err_sclk:
- clk_disable_unprepare(data->sclk);
-err_clk:
- clk_disable_unprepare(data->clk);
err_clk_sec:
if (!IS_ERR(data->clk_sec))
clk_disable_unprepare(data->clk_sec);
+err_sclk:
+ clk_disable_unprepare(data->sclk);
+err_clk_apbif:
+ clk_disable_unprepare(data->clk);
err_sensor:
if (!IS_ERR(data->regulator))
regulator_disable(data->regulator);
--
2.36.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-15 6:41 ` [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC Anand Moon
@ 2022-05-15 9:41 ` Krzysztof Kozlowski
2022-05-17 18:43 ` Anand Moon
2022-05-15 9:50 ` Krzysztof Kozlowski
1 sibling, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:41 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
On 15/05/2022 08:41, Anand Moon wrote:
> Reorder the tmu_gpu clock initialization for exynos5422 SoC.
Why?
>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v1: split the changes and improve the commit messages
> ---
> drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
> 1 file changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 75b3afadb5be..1ef90dc52c08 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -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);
> - 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-15 9:41 ` Krzysztof Kozlowski
@ 2022-05-17 18:43 ` Anand Moon
2022-05-18 7:28 ` Krzysztof Kozlowski
0 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-17 18:43 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
Hi Krzysztof,
On Sun, 15 May 2022 at 15:11, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > Reorder the tmu_gpu clock initialization for exynos5422 SoC.
>
> Why?
It just code reorder
>
> >
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v1: split the changes and improve the commit messages
> > ---
> > drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
> > 1 file changed, 21 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index 75b3afadb5be..1ef90dc52c08 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -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);
> > - 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.
>
>
> Best regards,
> Krzysztof
Thanks & Regards
-Anand
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-17 18:43 ` Anand Moon
@ 2022-05-18 7:28 ` Krzysztof Kozlowski
2022-05-21 9:51 ` Anand Moon
0 siblings, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-18 7:28 UTC (permalink / raw)
To: Anand Moon, Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
On 17/05/2022 20:43, Anand Moon wrote:
> Hi Krzysztof,
>
> On Sun, 15 May 2022 at 15:11, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 15/05/2022 08:41, Anand Moon wrote:
>>> Reorder the tmu_gpu clock initialization for exynos5422 SoC.
>>
>> 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.
>>
>>>
>>> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> v1: split the changes and improve the commit messages
>>> ---
>>> drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
>>> 1 file changed, 21 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>> index 75b3afadb5be..1ef90dc52c08 100644
>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>> @@ -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);
>>> - 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.
This is not the answer. Why are you preparing data->clk within else{}
branch?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-18 7:28 ` Krzysztof Kozlowski
@ 2022-05-21 9:51 ` Anand Moon
2022-05-21 14:20 ` Krzysztof Kozlowski
0 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-21 9:51 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
Hi Krzysztof,
On Wed, 18 May 2022 at 12:58, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/05/2022 20:43, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Sun, 15 May 2022 at 15:11, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 15/05/2022 08:41, Anand Moon wrote:
> >>> Reorder the tmu_gpu clock initialization for exynos5422 SoC.
> >>
> >> 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.
> 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.
>
> >>
> >>>
> >>> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>> ---
> >>> v1: split the changes and improve the commit messages
> >>> ---
> >>> drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
> >>> 1 file changed, 21 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> >>> index 75b3afadb5be..1ef90dc52c08 100644
> >>> --- a/drivers/thermal/samsung/exynos_tmu.c
> >>> +++ b/drivers/thermal/samsung/exynos_tmu.c
> >>> @@ -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);
> >>> - 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.
>
> 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);
if (ret) {
dev_err(&pdev->dev, "Failed to get clock\n");
goto err_sensor;
}
}
switch (data->soc) {
case SOC_ARCH_EXYNOS5420_TRIMINFO:
data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
if (IS_ERR(data->clk_sec)) {
dev_err(&pdev->dev, "Failed to get triminfo clock\n");
ret = PTR_ERR(data->clk_sec);
goto err_clk_apbif;
} else {
ret = clk_prepare_enable(data->clk_sec);
if (ret) {
dev_err(&pdev->dev, "Failed to get clock\n");
goto err_clk_apbif;
}
}
break;
case SOC_ARCH_EXYNOS5433:
case SOC_ARCH_EXYNOS7:
data->sclk = devm_clk_get(&pdev->dev, "tmu_sclk");
if (IS_ERR(data->sclk)) {
dev_err(&pdev->dev, "Failed to get sclk\n");
ret = PTR_ERR(data->sclk);
goto err_clk_sec;
} else {
ret = clk_prepare_enable(data->sclk);
if (ret) {
dev_err(&pdev->dev, "Failed to enable sclk\n");
goto err_clk_sec;
}
}
break;
default:
break;
}
>
> Best regards,
> Krzysztof
Thanks and Regards
-Anand
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-21 9:51 ` Anand Moon
@ 2022-05-21 14:20 ` Krzysztof Kozlowski
0 siblings, 0 replies; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-21 14:20 UTC (permalink / raw)
To: Anand Moon
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
On 21/05/2022 11:51, Anand Moon wrote:
> Hi Krzysztof,
>
> On Wed, 18 May 2022 at 12:58, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/05/2022 20:43, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On Sun, 15 May 2022 at 15:11, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 15/05/2022 08:41, Anand Moon wrote:
>>>>> Reorder the tmu_gpu clock initialization for exynos5422 SoC.
>>>>
>>>> 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.
What was wrong with original order? You still did not explain 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.
>>
>>>>
>>>>>
>>>>> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>> ---
>>>>> v1: split the changes and improve the commit messages
>>>>> ---
>>>>> drivers/thermal/samsung/exynos_tmu.c | 43 ++++++++++++++--------------
>>>>> 1 file changed, 21 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>>> index 75b3afadb5be..1ef90dc52c08 100644
>>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>>> @@ -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);
>>>>> - 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.
>>
>> 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);
Which is wrong and does not make any sense. This is third question - why
the main clock is prepared within 'else' branch?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-15 6:41 ` [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC Anand Moon
2022-05-15 9:41 ` Krzysztof Kozlowski
@ 2022-05-15 9:50 ` Krzysztof Kozlowski
2022-05-17 18:43 ` Anand Moon
1 sibling, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:50 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
On 15/05/2022 08:41, Anand Moon wrote:
> 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC
2022-05-15 9:50 ` Krzysztof Kozlowski
@ 2022-05-17 18:43 ` Anand Moon
0 siblings, 0 replies; 33+ messages in thread
From: Anand Moon @ 2022-05-17 18:43 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
Hi Krzysztof,
On Sun, 15 May 2022 at 15:20, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > 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.
>
As per the Exynos 5422 clock driver,
CLK_TMU_GPU_APBIF represents the clock for TMU_GPU
CLK_TMU_APBIF represents the clock for TMU
hence the subject is GPU-related.
> 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.
>
OK, thanks, I have not touched that part of the code in this series
but now I have a better understanding of the TMU drivers.
> Best regards,
> Krzysztof
Thanks & Regards
-Anand
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed
2022-05-15 6:41 [PATCHv2 0/6] Exynos Thermal code inprovement Anand Moon
2022-05-15 6:41 ` [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform Anand Moon
2022-05-15 6:41 ` [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC Anand Moon
@ 2022-05-15 6:41 ` Anand Moon
2022-05-15 9:43 ` Krzysztof Kozlowski
2022-05-15 6:41 ` [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422 Anand Moon
` (2 subsequent siblings)
5 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-15 6:41 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: 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 <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);
status = readb(data->base + EXYNOS_TMU_REG_STATUS);
if (!status) {
@@ -323,8 +322,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
err:
clk_disable(data->clk);
mutex_unlock(&data->lock);
- if (!IS_ERR(data->clk_sec))
- clk_disable(data->clk_sec);
+ clk_disable(data->clk_sec);
out:
return ret;
}
@@ -1119,8 +1117,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
err_thermal:
thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
err_clk_sec:
- if (!IS_ERR(data->clk_sec))
- clk_disable_unprepare(data->clk_sec);
+ clk_disable_unprepare(data->clk_sec);
err_sclk:
clk_disable_unprepare(data->sclk);
err_clk_apbif:
@@ -1142,8 +1139,7 @@ static int exynos_tmu_remove(struct platform_device *pdev)
clk_disable_unprepare(data->sclk);
clk_disable_unprepare(data->clk);
- if (!IS_ERR(data->clk_sec))
- clk_disable_unprepare(data->clk_sec);
+ clk_disable_unprepare(data->clk_sec);
if (!IS_ERR(data->regulator))
regulator_disable(data->regulator);
--
2.36.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed
2022-05-15 6:41 ` [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed Anand Moon
@ 2022-05-15 9:43 ` Krzysztof Kozlowski
2022-05-17 18:44 ` Anand Moon
0 siblings, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:43 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed
2022-05-15 9:43 ` Krzysztof Kozlowski
@ 2022-05-17 18:44 ` Anand Moon
0 siblings, 0 replies; 33+ messages in thread
From: Anand Moon @ 2022-05-17 18:44 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422
2022-05-15 6:41 [PATCHv2 0/6] Exynos Thermal code inprovement Anand Moon
` (2 preceding siblings ...)
2022-05-15 6:41 ` [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed Anand Moon
@ 2022-05-15 6:41 ` Anand Moon
2022-05-15 9:45 ` Krzysztof Kozlowski
2022-05-16 10:42 ` kernel test robot
2022-05-15 6:41 ` [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr() Anand Moon
2022-05-15 6:41 ` [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu Anand Moon
5 siblings, 2 replies; 33+ messages in thread
From: Anand Moon @ 2022-05-15 6:41 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: Anand Moon
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 <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: Fix the commit message
---
drivers/thermal/samsung/exynos_tmu.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 58ff1b577c47..0faec0f16db6 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -926,12 +926,14 @@ static int exynos_map_dt_data(struct platform_device *pdev)
data->gain = 8;
data->reference_voltage = 16;
data->efuse_value = 55;
- if (data->soc != SOC_ARCH_EXYNOS5420 &&
- data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
+ if (data->soc == SOC_ARCH_EXYNOS5420 &&
+ data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
+ data->min_efuse_value = 16;
+ data->max_efuse_value = 76;
+ } else {
data->min_efuse_value = 40;
- else
- data->min_efuse_value = 0;
- data->max_efuse_value = 100;
+ data->max_efuse_value = 100;
+ }
break;
case SOC_ARCH_EXYNOS5433:
data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
--
2.36.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422
2022-05-15 6:41 ` [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422 Anand Moon
@ 2022-05-15 9:45 ` Krzysztof Kozlowski
2022-05-16 10:42 ` kernel test robot
1 sibling, 0 replies; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:45 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
On 15/05/2022 08:41, Anand Moon wrote:
> 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 <bzolnier@gmail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422
2022-05-15 6:41 ` [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422 Anand Moon
2022-05-15 9:45 ` Krzysztof Kozlowski
@ 2022-05-16 10:42 ` kernel test robot
2022-05-16 10:44 ` Krzysztof Kozlowski
1 sibling, 1 reply; 33+ messages in thread
From: kernel test robot @ 2022-05-16 10:42 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: llvm, kbuild-all, Anand Moon
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@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 <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> 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.
vim +929 drivers/thermal/samsung/exynos_tmu.c
865
866 static int exynos_map_dt_data(struct platform_device *pdev)
867 {
868 struct exynos_tmu_data *data = platform_get_drvdata(pdev);
869 struct resource res;
870
871 if (!data || !pdev->dev.of_node)
872 return -ENODEV;
873
874 data->id = of_alias_get_id(pdev->dev.of_node, "tmuctrl");
875 if (data->id < 0)
876 data->id = 0;
877
878 data->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
879 if (data->irq <= 0) {
880 dev_err(&pdev->dev, "failed to get IRQ\n");
881 return -ENODEV;
882 }
883
884 if (of_address_to_resource(pdev->dev.of_node, 0, &res)) {
885 dev_err(&pdev->dev, "failed to get Resource 0\n");
886 return -ENODEV;
887 }
888
889 data->base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
890 if (!data->base) {
891 dev_err(&pdev->dev, "Failed to ioremap memory\n");
892 return -EADDRNOTAVAIL;
893 }
894
895 data->soc = (enum soc_type)of_device_get_match_data(&pdev->dev);
896
897 switch (data->soc) {
898 case SOC_ARCH_EXYNOS4210:
899 data->tmu_set_trip_temp = exynos4210_tmu_set_trip_temp;
900 data->tmu_set_trip_hyst = exynos4210_tmu_set_trip_hyst;
901 data->tmu_initialize = exynos4210_tmu_initialize;
902 data->tmu_control = exynos4210_tmu_control;
903 data->tmu_read = exynos4210_tmu_read;
904 data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
905 data->ntrip = 4;
906 data->gain = 15;
907 data->reference_voltage = 7;
908 data->efuse_value = 55;
909 data->min_efuse_value = 40;
910 data->max_efuse_value = 100;
911 break;
912 case SOC_ARCH_EXYNOS3250:
913 case SOC_ARCH_EXYNOS4412:
914 case SOC_ARCH_EXYNOS5250:
915 case SOC_ARCH_EXYNOS5260:
916 case SOC_ARCH_EXYNOS5420:
917 case SOC_ARCH_EXYNOS5420_TRIMINFO:
918 data->tmu_set_trip_temp = exynos4412_tmu_set_trip_temp;
919 data->tmu_set_trip_hyst = exynos4412_tmu_set_trip_hyst;
920 data->tmu_initialize = exynos4412_tmu_initialize;
921 data->tmu_control = exynos4210_tmu_control;
922 data->tmu_read = exynos4412_tmu_read;
923 data->tmu_set_emulation = exynos4412_tmu_set_emulation;
924 data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
925 data->ntrip = 4;
926 data->gain = 8;
927 data->reference_voltage = 16;
928 data->efuse_value = 55;
> 929 if (data->soc == SOC_ARCH_EXYNOS5420 &&
930 data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
931 data->min_efuse_value = 16;
932 data->max_efuse_value = 76;
933 } else {
934 data->min_efuse_value = 40;
935 data->max_efuse_value = 100;
936 }
937 break;
938 case SOC_ARCH_EXYNOS5433:
939 data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
940 data->tmu_set_trip_hyst = exynos5433_tmu_set_trip_hyst;
941 data->tmu_initialize = exynos5433_tmu_initialize;
942 data->tmu_control = exynos5433_tmu_control;
943 data->tmu_read = exynos4412_tmu_read;
944 data->tmu_set_emulation = exynos4412_tmu_set_emulation;
945 data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
946 data->ntrip = 8;
947 data->gain = 8;
948 if (res.start == EXYNOS5433_G3D_BASE)
949 data->reference_voltage = 23;
950 else
951 data->reference_voltage = 16;
952 data->efuse_value = 75;
953 data->min_efuse_value = 40;
954 data->max_efuse_value = 150;
955 break;
956 case SOC_ARCH_EXYNOS7:
957 data->tmu_set_trip_temp = exynos7_tmu_set_trip_temp;
958 data->tmu_set_trip_hyst = exynos7_tmu_set_trip_hyst;
959 data->tmu_initialize = exynos7_tmu_initialize;
960 data->tmu_control = exynos7_tmu_control;
961 data->tmu_read = exynos7_tmu_read;
962 data->tmu_set_emulation = exynos4412_tmu_set_emulation;
963 data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
964 data->ntrip = 8;
965 data->gain = 9;
966 data->reference_voltage = 17;
967 data->efuse_value = 75;
968 data->min_efuse_value = 15;
969 data->max_efuse_value = 100;
970 break;
971 default:
972 dev_err(&pdev->dev, "Platform not supported\n");
973 return -EINVAL;
974 }
975
976 data->cal_type = TYPE_ONE_POINT_TRIMMING;
977
978 /*
979 * Check if the TMU shares some registers and then try to map the
980 * memory of common registers.
981 */
982 if (data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
983 return 0;
984
985 if (of_address_to_resource(pdev->dev.of_node, 1, &res)) {
986 dev_err(&pdev->dev, "failed to get Resource 1\n");
987 return -ENODEV;
988 }
989
990 data->base_second = devm_ioremap(&pdev->dev, res.start,
991 resource_size(&res));
992 if (!data->base_second) {
993 dev_err(&pdev->dev, "Failed to ioremap memory\n");
994 return -ENOMEM;
995 }
996
997 return 0;
998 }
999
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422
2022-05-16 10:42 ` kernel test robot
@ 2022-05-16 10:44 ` Krzysztof Kozlowski
2022-05-17 18:44 ` Anand Moon
0 siblings, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-16 10:44 UTC (permalink / raw)
To: kernel test robot, Anand Moon, Bartlomiej Zolnierkiewicz,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: llvm, kbuild-all
On 16/05/2022 12:42, kernel test robot wrote:
> 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@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 <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
>>> 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422
2022-05-16 10:44 ` Krzysztof Kozlowski
@ 2022-05-17 18:44 ` Anand Moon
0 siblings, 0 replies; 33+ messages in thread
From: Anand Moon @ 2022-05-17 18:44 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: kernel test robot, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar,
Linux PM list, linux-samsung-soc, linux-arm-kernel, Linux Kernel,
llvm, kbuild-all
Hi Krzysztof,
On Mon, 16 May 2022 at 16:14, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/05/2022 12:42, kernel test robot wrote:
> > 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@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 <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> >>> 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...
>
Ok I will try to resolve this warning in the next version.
>
> Best regards,
> Krzysztof
Thanks & Regards
-Anand
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
2022-05-15 6:41 [PATCHv2 0/6] Exynos Thermal code inprovement Anand Moon
` (3 preceding siblings ...)
2022-05-15 6:41 ` [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422 Anand Moon
@ 2022-05-15 6:41 ` Anand Moon
2022-05-15 9:47 ` Krzysztof Kozlowski
2022-05-15 6:41 ` [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu Anand Moon
5 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-15 6:41 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: Anand Moon
Use the newlly introduced pm_sleep_ptr() macro, and mark the
functions __maybe_unused. These functions can then be moved outside the
CONFIG_PM_SUSPEND block, and the compiler can then process them and
detect build failures independently of the config. If unused, they will
simply be discarded by the compiler.
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series.
---
drivers/thermal/samsung/exynos_tmu.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 0faec0f16db6..f8a527f19383 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1149,15 +1149,14 @@ static int exynos_tmu_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
-static int exynos_tmu_suspend(struct device *dev)
+static int __maybe_unused exynos_tmu_suspend(struct device *dev)
{
exynos_tmu_control(to_platform_device(dev), false);
return 0;
}
-static int exynos_tmu_resume(struct device *dev)
+static int __maybe_unused exynos_tmu_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -1169,15 +1168,11 @@ static int exynos_tmu_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(exynos_tmu_pm,
exynos_tmu_suspend, exynos_tmu_resume);
-#define EXYNOS_TMU_PM (&exynos_tmu_pm)
-#else
-#define EXYNOS_TMU_PM NULL
-#endif
static struct platform_driver exynos_tmu_driver = {
.driver = {
.name = "exynos-tmu",
- .pm = EXYNOS_TMU_PM,
+ .pm = pm_sleep_ptr(&exynos_tmu_pm),
.of_match_table = exynos_tmu_match,
},
.probe = exynos_tmu_probe,
--
2.36.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
2022-05-15 6:41 ` [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr() Anand Moon
@ 2022-05-15 9:47 ` Krzysztof Kozlowski
2022-05-17 18:44 ` Anand Moon
0 siblings, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:47 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()
2022-05-15 9:47 ` Krzysztof Kozlowski
@ 2022-05-17 18:44 ` Anand Moon
0 siblings, 0 replies; 33+ messages in thread
From: Anand Moon @ 2022-05-17 18:44 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu
2022-05-15 6:41 [PATCHv2 0/6] Exynos Thermal code inprovement Anand Moon
` (4 preceding siblings ...)
2022-05-15 6:41 ` [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr() Anand Moon
@ 2022-05-15 6:41 ` Anand Moon
2022-05-15 9:48 ` Krzysztof Kozlowski
5 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-15 6:41 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski,
Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Alim Akhtar, linux-pm, linux-samsung-soc, linux-arm-kernel,
linux-kernel
Cc: Anand Moon
Add runtime power management for exynos thermal driver.
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series.
---
drivers/thermal/samsung/exynos_tmu.c | 29 ++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f8a527f19383..be9b98caf2ba 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -20,6 +20,7 @@
#include <linux/of_irq.h>
#include <linux/platform_device.h>
#include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
#include <dt-bindings/thermal/thermal_exynos.h>
@@ -1106,6 +1107,15 @@ static int exynos_tmu_probe(struct platform_device *pdev)
goto err_thermal;
}
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+
+ ret = pm_runtime_resume_and_get(&pdev->dev);
+ if (ret < 0)
+ goto disable_runtime_pm;
+
ret = devm_request_irq(&pdev->dev, data->irq, exynos_tmu_irq,
IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data);
if (ret) {
@@ -1113,11 +1123,16 @@ static int exynos_tmu_probe(struct platform_device *pdev)
goto err_thermal;
}
+ pm_runtime_put(&pdev->dev);
+
exynos_tmu_control(pdev, true);
return 0;
err_thermal:
thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
+disable_runtime_pm:
+ pm_runtime_put_noidle(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
err_clk_sec:
clk_disable_unprepare(data->clk_sec);
err_sclk:
@@ -1143,6 +1158,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
clk_disable_unprepare(data->clk);
clk_disable_unprepare(data->clk_sec);
+ pm_runtime_put_noidle(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
if (!IS_ERR(data->regulator))
regulator_disable(data->regulator);
@@ -1151,18 +1169,25 @@ static int exynos_tmu_remove(struct platform_device *pdev)
static int __maybe_unused exynos_tmu_suspend(struct device *dev)
{
- exynos_tmu_control(to_platform_device(dev), false);
+ struct platform_device *pdev = to_platform_device(dev);
- return 0;
+ exynos_tmu_control(pdev, false);
+
+ return pm_runtime_force_suspend(&pdev->dev);
}
static int __maybe_unused exynos_tmu_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
+ int ret;
exynos_tmu_initialize(pdev);
exynos_tmu_control(pdev, true);
+ ret = pm_runtime_force_resume(&pdev->dev);
+ if (ret)
+ return ret;
+
return 0;
}
--
2.36.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu
2022-05-15 6:41 ` [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu Anand Moon
@ 2022-05-15 9:48 ` Krzysztof Kozlowski
2022-05-17 18:45 ` Anand Moon
0 siblings, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-15 9:48 UTC (permalink / raw)
To: Anand Moon, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki,
Daniel Lezcano, Amit Kucheria, Zhang Rui, Alim Akhtar, linux-pm,
linux-samsung-soc, linux-arm-kernel, linux-kernel
On 15/05/2022 08:41, Anand Moon wrote:
> 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu
2022-05-15 9:48 ` Krzysztof Kozlowski
@ 2022-05-17 18:45 ` Anand Moon
2022-05-18 7:19 ` Krzysztof Kozlowski
0 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-17 18:45 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
Hi Krzysztof,
On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > 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.
>
> Best regards,
> Krzysztof
Thanks & Regards
-Anand
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu
2022-05-17 18:45 ` Anand Moon
@ 2022-05-18 7:19 ` Krzysztof Kozlowski
2022-05-21 9:52 ` Anand Moon
0 siblings, 1 reply; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-18 7:19 UTC (permalink / raw)
To: Anand Moon, Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
On 17/05/2022 20:45, Anand Moon wrote:
> Hi Krzysztof,
>
> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 15/05/2022 08:41, Anand Moon wrote:
>>> 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?
Controlling runtime PM by itself is not a goal. What does it change if
it is enabled?
> 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu
2022-05-18 7:19 ` Krzysztof Kozlowski
@ 2022-05-21 9:52 ` Anand Moon
2022-05-21 14:11 ` Krzysztof Kozlowski
0 siblings, 1 reply; 33+ messages in thread
From: Anand Moon @ 2022-05-21 9:52 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
Hi Krzysztof,
On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/05/2022 20:45, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 15/05/2022 08:41, Anand Moon wrote:
> >>> 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?
>
> 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
> > 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?
>
> Best regards,
> Krzysztof
Thanks and Regards
-Anand
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu
2022-05-21 9:52 ` Anand Moon
@ 2022-05-21 14:11 ` Krzysztof Kozlowski
0 siblings, 0 replies; 33+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-21 14:11 UTC (permalink / raw)
To: Anand Moon
Cc: Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Alim Akhtar, Linux PM list,
linux-samsung-soc, linux-arm-kernel, Linux Kernel
On 21/05/2022 11:52, Anand Moon wrote:
> Hi Krzysztof,
>
> On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/05/2022 20:45, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 15/05/2022 08:41, Anand Moon wrote:
>>>>> 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?
>>
>> 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
How? I asked - what is being changed after enabling PM - and you
answered without any specifics. Where exactly is the power saving?
Please be specific, very specific.
>
>>> 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
And does nothing else, right? No benefits?
> So I have made sure that suspend resume feature works correctly
> with these changes on SBC Odroid U3 and XU4.
How is suspend/resume related to runtime PM? Are you talking about
system suspend? What do you mean now?
>
> 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?
Why looking at them? You avoid giving any specific answer, so we are
repeating the same and the same. Just to be sure - maybe I don't see the
obvious stuff, so please explain also this obvious things.
Please come with specifics, because otherwise I see it as a waste of our
time.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 33+ messages in thread