| Message ID | 20220515064126.1424-1-linux.amoon@gmail.com |
|---|---|
| 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. Please explain why this is needed. Each change needs explanation why you are doing it. Best regards, Krzysztof
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
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
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
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
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
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
Hi All This patch series is bit of rework on my previous patch series [0], where I failed to justify the code changes. With this new series I have tried to improve the commit subject and commit message. Added new patchs are added to improve the PM suspend/resume and added runtime power management support to exynos tmu driver. [0] https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m77e57120d230d57f34c29e1422d7fc5f5587ac30 Best Regards -Anand Anand Moon (6): thermal: exynos: Enable core tmu hardware clk flag on exynos platform thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC thermal: exynos: Check before clk_disable_unprepare() not needed thermal: exynos: fixed the efuse min/max value for exynos5422 thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr() thermal: exynos: Add runtime power management for tmu drivers/thermal/samsung/exynos_tmu.c | 107 ++++++++++++++++----------- 1 file changed, 62 insertions(+), 45 deletions(-) base-commit: ec7f49619d8ee13e108740c82f942cd401b989e9