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