| Message ID | 1531822342-4293-1-git-send-email-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
Hi Anand, Thanks for patch. On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: > clk_summary do not show tmu_apbif clk enable, so replace > the clk_prepare with clk_prepare_enables to enable tmu clk. 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)? > simplify the enable of tmu_triminfo_apbif clk, also fixed > the order of goto error for failed cases. This has to be split into separate change. Best regards, Krzysztof
Hi Krzysztof On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote: > Hi Anand, > > Thanks for patch. > > On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: >> clk_summary do not show tmu_apbif clk enable, so replace >> the clk_prepare with clk_prepare_enables to enable tmu clk. > > 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)? > Opps I could not explain some more in my commit message. Actually TMU sensor for Exynos process are controlled by so external clk Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu. Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which control the CLK_SENSE tmu. So as per my understanding tmu is clk driver which control the flow PMIC. clk_prepare_enable combine clk_prepare and clk_enable and clk_disable_unprepare combine clk_disable and clk_unprepare. most of the driver prefer clk_prepare_enable and clk_disable_unprepare. clk_summary is just a reference looking point where we could check the clk is enable/disable. what is broken ? I still few more parameter need to tuned to configure the tmu driver. >> simplify the enable of tmu_triminfo_apbif clk, also fixed >> the order of goto error for failed cases. > > This has to be split into separate change. > > Best regards, > Krzysztof Best Regards -Anand
On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote: > Hi Krzysztof > > On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> Hi Anand, >> >> Thanks for patch. >> >> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: >>> clk_summary do not show tmu_apbif clk enable, so replace >>> the clk_prepare with clk_prepare_enables to enable tmu clk. >> >> 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)? >> > > Opps I could not explain some more in my commit message. > > Actually TMU sensor for Exynos process are controlled by so external clk > > Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu. > Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which > control the CLK_SENSE tmu. > > So as per my understanding tmu is clk driver which control the flow PMIC. > > clk_prepare_enable combine clk_prepare and clk_enable > and clk_disable_unprepare combine clk_disable and clk_unprepare. > > most of the driver prefer clk_prepare_enable and clk_disable_unprepare. > > clk_summary is just a reference looking point where we could check the > clk is enable/disable. > > what is broken ? > I still few more parameter need to tuned to configure the tmu driver. I am sorry but I am still unable to see what is broken and what are you trying to fix. I asked what is broken and you replied that there is a sensor, there is a clock, drivers use clk_prepare_enable and some more parameter need to be tuned... None of these are answers to question - what is broken. How can I reproduce the problem? Best regards, Krzysztof
Hi Krzysztof On 18 July 2018 at 11:47, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote: >> Hi Krzysztof >> >> On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> Hi Anand, >>> >>> Thanks for patch. >>> >>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: >>>> clk_summary do not show tmu_apbif clk enable, so replace >>>> the clk_prepare with clk_prepare_enables to enable tmu clk. >>> >>> 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)? >>> >> >> Opps I could not explain some more in my commit message. >> >> Actually TMU sensor for Exynos process are controlled by so external clk >> >> Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu. >> Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which >> control the CLK_SENSE tmu. >> >> So as per my understanding tmu is clk driver which control the flow PMIC. >> >> clk_prepare_enable combine clk_prepare and clk_enable >> and clk_disable_unprepare combine clk_disable and clk_unprepare. >> >> most of the driver prefer clk_prepare_enable and clk_disable_unprepare. >> >> clk_summary is just a reference looking point where we could check the >> clk is enable/disable. >> >> what is broken ? >> I still few more parameter need to tuned to configure the tmu driver. > > I am sorry but I am still unable to see what is broken and what are > you trying to fix. I asked what is broken and you replied that there > is a sensor, there is a clock, drivers use clk_prepare_enable and some > more parameter need to be tuned... None of these are answers to > question - what is broken. How can I reproduce the problem? > > Best regards, > Krzysztof Basically I use thermal testing. # git clone https://git.linaro.org/power/pm-qa.git # cd pm-qa # make -C thermal check most of the testcase failed on Exynos5422 but some pass on Exynos4412. Attach is the software overview from Exynos5422 user manual. I am not able to explain in deep technically, but I have studied other thermal driver to draw into conclusion that tmu clk's need to be enabled. If you feel the we should not enable these clk, them I will drop the clk_prepare_enable check and resubmit the changes with better commit message. Best Regards -Anand The example of software sequence as follows: The parameter may be changed according to an application. /* Read the measured data from e-fuse */ Triminfo_85 = TRIMINFO[15:8] Triminfo_25 = TRIMINFO[7:0] /* Calibrated threshold temperature is written into THRES_TEMP_RISE and THRES_TEMP_FALL */ /* Refer to 1.6.1 */ THRES_TEMP_RISE0 = 0x40; THRES_TEMP_RISE1 = 0x50; THRES_TEMP_RISE2 = 0x60; THRES_TEMP_RISE3 = 0x70; THRES_TEMP_FALL0 = 0x3A; THRES_TEMP_FALL1 = 0x4A; THRES_TEMP_FALL2 = 0x5A; /* Parameter for sampling interval is set */ SAMPLING_INTERVAL = 0x1; /* Interrupt enable */ INTEN[24] =0x1; // for INTEN_FALL2 INTEN[20] =0x1; // for INTEN_FALL1 INTEN[16] =0x1; // for INTEN_FALL0 INTEN[8] =0x1; // for INTEN_RISE2 INTEN[4] =0x1; // for INTEN_RISE1 INTEN[0] =0x1; // for INTEN_RISE0 /* Thermal tripping mode selection */ THERM_TRIP_MODE = 0x4; /* Thermal tripping enable */ THERM_TRIP_EN = 0x1; /* Check sensing operation is idle */ tmu_idle = 0; while(tmu_idle&1) { tmu_idle = TMU_STATUS[0]; } /* Start sensing operation */ TMU_CONTROL |= 1; ISR_INTREQ_TMU () { /* Read interrupt status register */ int_status = INTSTAT; if(int_status[24]) { ISR_INT_FALL2(); } else if(int_status[20]) { ISR_INT_FALL1(); } else if(int_status[16]) { ISR_INT_FALL0(); } Else if(int_status[8]) { ISR_INT_RISE2(); } else if(int_status[4]) { ISR_INT_RISE1(); } else if(int_status[0]) { ISR_INT_RISE0(); } else { $display("Some error occurred..!"); } ISR_INT0 () { /* Perform proper task for decrease temperature */ INTCLEAR[0] = 0x1; }
On 18 July 2018 at 11:24, Anand Moon <linux.amoon@gmail.com> wrote: > Hi Krzysztof > > On 18 July 2018 at 11:47, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote: >>> Hi Krzysztof >>> >>> On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>>> Hi Anand, >>>> >>>> Thanks for patch. >>>> >>>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: >>>>> clk_summary do not show tmu_apbif clk enable, so replace >>>>> the clk_prepare with clk_prepare_enables to enable tmu clk. >>>> >>>> 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)? >>>> >>> >>> Opps I could not explain some more in my commit message. >>> >>> Actually TMU sensor for Exynos process are controlled by so external clk >>> >>> Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu. >>> Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which >>> control the CLK_SENSE tmu. >>> >>> So as per my understanding tmu is clk driver which control the flow PMIC. >>> >>> clk_prepare_enable combine clk_prepare and clk_enable >>> and clk_disable_unprepare combine clk_disable and clk_unprepare. >>> >>> most of the driver prefer clk_prepare_enable and clk_disable_unprepare. >>> >>> clk_summary is just a reference looking point where we could check the >>> clk is enable/disable. >>> >>> what is broken ? >>> I still few more parameter need to tuned to configure the tmu driver. >> >> I am sorry but I am still unable to see what is broken and what are >> you trying to fix. I asked what is broken and you replied that there >> is a sensor, there is a clock, drivers use clk_prepare_enable and some >> more parameter need to be tuned... None of these are answers to >> question - what is broken. How can I reproduce the problem? >> >> Best regards, >> Krzysztof > > Basically I use thermal testing. > > # git clone https://git.linaro.org/power/pm-qa.git > # cd pm-qa > # make -C thermal check > > most of the testcase failed on Exynos5422 but some pass on Exynos4412. > > Attach is the software overview from Exynos5422 user manual. > > I am not able to explain in deep technically, but I have studied other thermal > driver to draw into conclusion that tmu clk's need to be enabled. That is true in general - the clk has to be enabled in certain cases. However you did not say at all when you want this clock to be enabled... and the your patch enables it for entire lifetime of device. > If you feel the we should not enable these clk, them I will drop the > clk_prepare_enable check > and resubmit the changes with better commit message. I don't know. This was fifth email in this thread and it is the first time some real problem is mentioned. Still the issue is not described entirely so I really do not have a clue whether this patch fixes something or not. What is more, you mentioned falling pm-qa tests here (not in commit msg) but did not say whether this patch fixes anything or not. So let me summarize it: 1. You did not describe the problem you want to fix. 2. The patch looks incorrect because it enables the clock for entire lifetime of device which we do not want. 3. The patch might or not might fix some problem. We even do not know what... 4. The clock not being enabled when not needed... is obviously not a problem. Please start from beginning. Find the problem, tell us how it can be reproduced and deliver a single patch which fixes the problem. This pattern of your code - fixing something without describing the problem - happened many time before. I repeated this some times before as well. I would prefer not to repeat to many times. Therefore I would be happy if you follow the path mentioned in paragraph before always: find the problem, tell how it can be reproduced, deliver single patch which fixes the problem. Best regards, Krzysztof
Hi Krzysztof, On 18 July 2018 at 15:36, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 18 July 2018 at 11:24, Anand Moon <linux.amoon@gmail.com> wrote: >> Hi Krzysztof >> >> On 18 July 2018 at 11:47, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> On 17 July 2018 at 22:23, Anand Moon <linux.amoon@gmail.com> wrote: >>>> Hi Krzysztof >>>> >>>> On 17 July 2018 at 17:50, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>>>> Hi Anand, >>>>> >>>>> Thanks for patch. >>>>> >>>>> On 17 July 2018 at 12:12, Anand Moon <linux.amoon@gmail.com> wrote: >>>>>> clk_summary do not show tmu_apbif clk enable, so replace >>>>>> the clk_prepare with clk_prepare_enables to enable tmu clk. >>>>> >>>>> 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)? >>>>> >>>> >>>> Opps I could not explain some more in my commit message. >>>> >>>> Actually TMU sensor for Exynos process are controlled by so external clk >>>> >>>> Exynos4412 have VDD18_TS sensor which controls the CLK_SENSE tmu. >>>> Exynos5422 have VDD18_TS01 / VDD18_TS23 / VDD18_TS4 sensor which >>>> control the CLK_SENSE tmu. >>>> >>>> So as per my understanding tmu is clk driver which control the flow PMIC. >>>> >>>> clk_prepare_enable combine clk_prepare and clk_enable >>>> and clk_disable_unprepare combine clk_disable and clk_unprepare. >>>> >>>> most of the driver prefer clk_prepare_enable and clk_disable_unprepare. >>>> >>>> clk_summary is just a reference looking point where we could check the >>>> clk is enable/disable. >>>> >>>> what is broken ? >>>> I still few more parameter need to tuned to configure the tmu driver. >>> >>> I am sorry but I am still unable to see what is broken and what are >>> you trying to fix. I asked what is broken and you replied that there >>> is a sensor, there is a clock, drivers use clk_prepare_enable and some >>> more parameter need to be tuned... None of these are answers to >>> question - what is broken. How can I reproduce the problem? >>> >>> Best regards, >>> Krzysztof >> >> Basically I use thermal testing. >> >> # git clone https://git.linaro.org/power/pm-qa.git >> # cd pm-qa >> # make -C thermal check >> >> most of the testcase failed on Exynos5422 but some pass on Exynos4412. >> >> Attach is the software overview from Exynos5422 user manual. >> >> I am not able to explain in deep technically, but I have studied other thermal >> driver to draw into conclusion that tmu clk's need to be enabled. > > That is true in general - the clk has to be enabled in certain cases. > However you did not say at all when you want this clock to be > enabled... and the your patch enables it for entire lifetime of > device. > >> If you feel the we should not enable these clk, them I will drop the >> clk_prepare_enable check >> and resubmit the changes with better commit message. > > I don't know. This was fifth email in this thread and it is the first > time some real problem is mentioned. Still the issue is not described > entirely so I really do not have a clue whether this patch fixes > something or not. > What is more, you mentioned falling pm-qa tests here (not in commit > msg) but did not say whether this patch fixes anything or not. > > So let me summarize it: > 1. You did not describe the problem you want to fix. > 2. The patch looks incorrect because it enables the clock for entire > lifetime of device which we do not want. > 3. The patch might or not might fix some problem. We even do not know what... > 4. The clock not being enabled when not needed... is obviously not a problem. > > Please start from beginning. Find the problem, tell us how it can be > reproduced and deliver a single patch which fixes the problem. > > This pattern of your code - fixing something without describing the > problem - happened many time before. I repeated this some times before > as well. I would prefer not to repeat to many times. Therefore I would > be happy if you follow the path mentioned in paragraph before always: > find the problem, tell how it can be reproduced, deliver single patch > which fixes the problem. > > Best regards, > Krzysztof Yes I will try to improve my self to the point commit message and code as per the documentation. Let me clear my thoughts on clk's turned on for by driver. Please correct me If I am wrong. Exynos support Power domain to control the dynamic power consumption via clk. Operating voltage Operating frequency Toggling ratios of the logic gate PMU generates power control signal to regulator or PMIC. To reduce the dynamic power consumption, Exynos SCP uses clock gating and frequency scaling. Exynos process support tmu power domain Power Domain Power Source Internal Power Gating Method Included Modules TEMP VDD18_TEMP None TEMP SENSOR Exynos TMU is directly co-related to CPU frequency scaling module. as their is increase in CPU clk speed with increase in temperature TMU will control the flow of cpu speed via clk freq scaling. It might be a good option to implement pm_runtime for tmu driver. If you are not satisfied with this series of change lets drop them. I will just re-submit err-clean up and e-Fuse changes. Best Regards -Anand
On Thu, Jul 19, 2018 at 03:22:21PM +0530, Anand Moon wrote: > Hi Krzysztof, > Please send a new version of the series with applied fixes and also get reviews from the samsung devs. Thanks. > > Best Regards > -Anand
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index a992e51..0164c9e 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -1060,41 +1060,40 @@ 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(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(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; + } else { + ret = clk_prepare_enable(data->clk_sec); + if (ret) { + dev_err(&pdev->dev, "Failed to get clock\n"); + goto err_clk; + } + } + 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"); - 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; @@ -1134,11 +1133,11 @@ static int exynos_tmu_probe(struct platform_device *pdev) thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd); err_sclk: clk_disable_unprepare(data->sclk); -err_clk: - clk_unprepare(data->clk); err_clk_sec: if (!IS_ERR(data->clk_sec)) - clk_unprepare(data->clk_sec); + clk_disable_unprepare(data->clk_sec); +err_clk: + clk_disable_unprepare(data->clk); err_sensor: if (!IS_ERR(data->regulator)) regulator_disable(data->regulator); @@ -1155,9 +1154,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);
clk_summary do not show tmu_apbif clk enable, so replace the clk_prepare with clk_prepare_enables to enable tmu clk. simplify the enable of tmu_triminfo_apbif clk, also fixed the order of goto error for failed cases. CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- before: cat /sys/kernel/debug/clk/clk_summary | grep tmu tmu_gpu 0 2 66600000 0 0 tmu 0 6 66600000 0 0 after: cat /sys/kernel/debug/clk/clk_summary | grep tmu tmu_gpu 2 2 0 66600000 0 0 tmu 6 6 0 66600000 0 0 --- drivers/thermal/samsung/exynos_tmu.c | 45 ++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 23 deletions(-)