| Message ID | 20220515064126.1424-2-linux.amoon@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Exynos Thermal code inprovement | expand |
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
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
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
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
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
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);
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(-)