[PATCH v6 0/4] Exynos Thermal code improvement

[PATCH v6 0/4] Exynos Thermal code improvement

[Anand Moon]

Hi All,

This patch series is a rework of my previous patch series [1],
where the code changes were not adequately justified.

In this new series, I have improved the commit subject
and commit message to better explain the changes.

v6: Add new patch to use devm_clk_get_enabled
    and Fix few typo in subject as suggested by Daniel.
v5: Drop the guard mutex patch
v4: Tried to address Lukasz review comments.

Tested on Odroid U3 amd XU4 SoC boards.
Build with clang with W=1 enable.

[4] https://lore.kernel.org/all/20250410063754.5483-2-linux.amoon@gmail.com/
[3] https://lore.kernel.org/all/20250310143450.8276-2-linux.amoon@gmail.com/
[2] https://lore.kernel.org/all/20250216195850.5352-2-linux.amoon@gmail.com/
[1] https://lore.kernel.org/all/20220515064126.1424-1-linux.amoon@gmail.com/
[0] https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m77e57120d230d57f34c29e1422d7fc5f5587ac30

Thanks
-Anand

Anand Moon (4):
  thermal/drivers/exynos: Refactor clk_sec initialization inside
    SOC-specific case
  thermal/drivers/exynos: Use devm_clk_get_enabled() helpers
  thermal/drivers/exynos: Remove redundant IS_ERR() checks for clk_sec
    clock
  thermal/drivers/exynos: Fixed the efuse min max value for exynos5422

 drivers/thermal/samsung/exynos_tmu.c | 100 ++++++++++-----------------
 1 file changed, 35 insertions(+), 65 deletions(-)


base-commit: b6ea1680d0ac0e45157a819c41b46565f4616186
-- 
2.49.0

[PATCH v6 1/4] thermal/drivers/exynos: Refactor clk_sec initialization inside SOC-specific case

[Anand Moon]

Refactor the initialization of the clk_sec clock to be inside the
SOC_ARCH_EXYNOS5420_TRIMINFO case. It ensures that the clk_sec clock
is only initialized for the specified SOC and not for other SOCs,
thereby simplifying the code. The clk_sec clock is used by the TMU
for GPU on the Exynos 542x platform.

Removed redundant IS_ERR() checks for the clk_sec clock since error
handling is already managed internally by clk_unprepare() functions.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v6: Add Rb Lukasz and try to address Daniel review coments.
v5: None
v4: Fix the aligment of code clk for clk_prepare in proper if/else block.
    update the commit for clk_sec used.
    checked to goto clean up all the clks are proper.
    drop IS_ERR() check for clk_sec.
v3: improve the commit message.
---
 drivers/thermal/samsung/exynos_tmu.c | 36 +++++++++++++---------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 47a99b3c5395..04517d52afbd 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1040,26 +1040,26 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 	if (IS_ERR(data->clk))
 		return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n");
 
-	data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
-	if (IS_ERR(data->clk_sec)) {
-		if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO)
-			return dev_err_probe(dev, PTR_ERR(data->clk_sec),
-					     "Failed to get triminfo clock\n");
-	} else {
-		ret = clk_prepare(data->clk_sec);
-		if (ret) {
-			dev_err(dev, "Failed to get clock\n");
-			return ret;
-		}
-	}
-
 	ret = clk_prepare(data->clk);
 	if (ret) {
 		dev_err(dev, "Failed to get clock\n");
-		goto err_clk_sec;
+		return ret;
 	}
 
 	switch (data->soc) {
+	case SOC_ARCH_EXYNOS5420_TRIMINFO:
+		data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
+		if (IS_ERR(data->clk_sec)) {
+			ret = dev_err_probe(dev, PTR_ERR(data->clk_sec),
+					    "Failed to get clk_sec clock\n");
+			goto err_clk;
+		}
+		ret = clk_prepare(data->clk_sec);
+		if (ret) {
+			dev_err(dev, "Failed to prepare clk_sec clock\n");
+			goto err_clk_sec;
+		}
+		break;
 	case SOC_ARCH_EXYNOS5433:
 	case SOC_ARCH_EXYNOS7:
 		data->sclk = devm_clk_get(dev, "tmu_sclk");
@@ -1112,11 +1112,10 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 
 err_sclk:
 	clk_disable_unprepare(data->sclk);
+err_clk_sec:
+	clk_unprepare(data->clk_sec);
 err_clk:
 	clk_unprepare(data->clk);
-err_clk_sec:
-	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
 	return ret;
 }
 
@@ -1128,8 +1127,7 @@ static void exynos_tmu_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(data->sclk);
 	clk_unprepare(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+	clk_unprepare(data->clk_sec);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.49.0

[PATCH v6 2/4] thermal/drivers/exynos: Use devm_clk_get_enabled() helpers

[Anand Moon]

Use devm_clk_get_enabled() helper instead of calling devm_clk_get() and
then clk_prepare_enable(). It simplifies the error handling and makes
the code more compact.

Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v6: New patch as per Daniel request.
---
 drivers/thermal/samsung/exynos_tmu.c | 77 ++++++++--------------------
 1 file changed, 20 insertions(+), 57 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 04517d52afbd..aa0726b33c84 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1036,65 +1036,41 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	data->clk = devm_clk_get(dev, "tmu_apbif");
+	data->clk = devm_clk_get_enabled(dev, "tmu_apbif");
 	if (IS_ERR(data->clk))
-		return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n");
-
-	ret = clk_prepare(data->clk);
-	if (ret) {
-		dev_err(dev, "Failed to get clock\n");
-		return ret;
-	}
-
-	switch (data->soc) {
-	case SOC_ARCH_EXYNOS5420_TRIMINFO:
-		data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
-		if (IS_ERR(data->clk_sec)) {
-			ret = dev_err_probe(dev, PTR_ERR(data->clk_sec),
-					    "Failed to get clk_sec clock\n");
-			goto err_clk;
-		}
-		ret = clk_prepare(data->clk_sec);
-		if (ret) {
-			dev_err(dev, "Failed to prepare clk_sec clock\n");
-			goto err_clk_sec;
-		}
-		break;
-	case SOC_ARCH_EXYNOS5433:
-	case SOC_ARCH_EXYNOS7:
-		data->sclk = devm_clk_get(dev, "tmu_sclk");
-		if (IS_ERR(data->sclk)) {
-			ret = dev_err_probe(dev, PTR_ERR(data->sclk), "Failed to get sclk\n");
-			goto err_clk;
-		} else {
-			ret = clk_prepare_enable(data->sclk);
-			if (ret) {
-				dev_err(dev, "Failed to enable sclk\n");
-				goto err_clk;
-			}
-		}
-		break;
-	default:
-		break;
+		return dev_err_probe(dev, PTR_ERR(data->clk),
+				     "Failed to get clock\n");
+
+	if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
+		data->clk_sec = devm_clk_get_enabled(dev, "tmu_triminfo_apbif");
+		if (IS_ERR(data->clk_sec))
+			return dev_err_probe(dev, PTR_ERR(data->clk_sec),
+					     "Failed to get clk_sec clock\n");
+	} else if (data->soc == SOC_ARCH_EXYNOS5433 ||
+		   data->soc == SOC_ARCH_EXYNOS7) {
+		data->sclk = devm_clk_get_enabled(dev, "tmu_sclk");
+		if (IS_ERR(data->sclk))
+			return dev_err_probe(dev, PTR_ERR(data->sclk),
+					     "Failed to get sclk\n");
 	}
 
 	ret = exynos_tmu_initialize(pdev);
 	if (ret) {
 		dev_err(dev, "Failed to initialize TMU\n");
-		goto err_sclk;
+		return ret;
 	}
 
 	data->tzd = devm_thermal_of_zone_register(dev, 0, data,
 						  &exynos_sensor_ops);
 	if (IS_ERR(data->tzd)) {
-		ret = dev_err_probe(dev, PTR_ERR(data->tzd), "Failed to register sensor\n");
-		goto err_sclk;
+		return dev_err_probe(dev, PTR_ERR(data->tzd),
+				     "Failed to register sensor\n");
 	}
 
 	ret = exynos_thermal_zone_configure(pdev);
 	if (ret) {
 		dev_err(dev, "Failed to configure the thermal zone\n");
-		goto err_sclk;
+		return ret;
 	}
 
 	ret = devm_request_threaded_irq(dev, data->irq, NULL,
@@ -1104,30 +1080,17 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 					dev_name(dev), data);
 	if (ret) {
 		dev_err(dev, "Failed to request irq: %d\n", data->irq);
-		goto err_sclk;
+		return ret;
 	}
 
 	exynos_tmu_control(pdev, true);
-	return 0;
 
-err_sclk:
-	clk_disable_unprepare(data->sclk);
-err_clk_sec:
-	clk_unprepare(data->clk_sec);
-err_clk:
-	clk_unprepare(data->clk);
 	return ret;
 }
 
 static void exynos_tmu_remove(struct platform_device *pdev)
 {
-	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
-
 	exynos_tmu_control(pdev, false);
-
-	clk_disable_unprepare(data->sclk);
-	clk_unprepare(data->clk);
-	clk_unprepare(data->clk_sec);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.49.0

[PATCH v6 3/4] thermal/drivers/exynos: Remove redundant IS_ERR() checks for clk_sec clock

[Anand Moon]

Remove unnecessary IS_ERR() checks for the clk_sec clock,
the clk_enable() and clk_disable() functions can handle NULL clock
pointers, so the additional checks are redundant and have been removed
to simplify the code.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v6: Add Rb - Lukasz and Fix the typo in the subject
v5: None
v4: drop IE_ERR() for clk_unprepare() as its handle in earlier code.
v3: improve the commit message.
---
 drivers/thermal/samsung/exynos_tmu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index aa0726b33c84..5f017a78f437 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -258,8 +258,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) {
@@ -269,8 +268,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 		data->tmu_clear_irqs(data);
 	}
 
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
+	clk_disable(data->clk_sec);
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
-- 
2.49.0

[PATCH v6 4/4] thermal/drivers/exynos: Fixed the efuse min max value for exynos5422

[Anand Moon]

As per Exynos5422 user manual 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. Refactors the efuse value
initialization logic within exynos_map_dt_data function by
replacing the nested if-else statements with a switch statement.
Ensures proper initialization of efuse values based on the SOC type.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v6: Add Rb Lukasz and fix typo in subject
v5: None
V4: None
---
 drivers/thermal/samsung/exynos_tmu.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 5f017a78f437..ef216aac13ee 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -899,12 +899,23 @@ 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)
+		data->max_efuse_value = 100;
+		switch (data->soc) {
+		case SOC_ARCH_EXYNOS3250:
+		case SOC_ARCH_EXYNOS4412:
+		case SOC_ARCH_EXYNOS5250:
+		case SOC_ARCH_EXYNOS5260:
 			data->min_efuse_value = 40;
-		else
+			break;
+		case SOC_ARCH_EXYNOS5420:
+		case SOC_ARCH_EXYNOS5420_TRIMINFO:
+			data->min_efuse_value = 16;
+			data->max_efuse_value = 76;
+			break;
+		default:
 			data->min_efuse_value = 0;
-		data->max_efuse_value = 100;
+			break;
+		}
 		break;
 	case SOC_ARCH_EXYNOS5433:
 		data->tmu_set_low_temp = exynos5433_tmu_set_low_temp;
-- 
2.49.0

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Anand Moon]

Hi All,

On Wed, 30 Apr 2025 at 18:03, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi All,
>
> This patch series is a rework of my previous patch series [1],
> where the code changes were not adequately justified.
>
> In this new series, I have improved the commit subject
> and commit message to better explain the changes.
>
> v6: Add new patch to use devm_clk_get_enabled
>     and Fix few typo in subject as suggested by Daniel.
> v5: Drop the guard mutex patch
> v4: Tried to address Lukasz review comments.
>
> Tested on Odroid U3 amd XU4 SoC boards.
> Build with clang with W=1 enable.
>

Genital Ping!!!

Thanks
-Anand

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Krzysztof Kozlowski]

On 08/05/2025 08:14, Anand Moon wrote:
> Hi All,
> 
> On Wed, 30 Apr 2025 at 18:03, Anand Moon <linux.amoon@gmail.com> wrote:
>>
>> Hi All,
>>
>> This patch series is a rework of my previous patch series [1],
>> where the code changes were not adequately justified.
>>
>> In this new series, I have improved the commit subject
>> and commit message to better explain the changes.
>>
>> v6: Add new patch to use devm_clk_get_enabled
>>     and Fix few typo in subject as suggested by Daniel.
>> v5: Drop the guard mutex patch
>> v4: Tried to address Lukasz review comments.
>>
>> Tested on Odroid U3 amd XU4 SoC boards.
>> Build with clang with W=1 enable.
>>
> 
> Genital Ping!!!


Huhu, nice. :)
I make typos as well, but some typos are better to avoid. :)

Anyway, !!! are exclamation marks and I think it is very difficult to
scream at someone gently. I think this is contradictory to itself, so it
does not feel gently at all.

Plus you sent it 7 days ago and you are known to send poor quality,
untested code, so just relax and wait.

Best regards,
Krzysztof

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Anand Moon]

Hi Krzysztof,
On Thu, 8 May 2025 at 11:57, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 08/05/2025 08:14, Anand Moon wrote:
> > Hi All,
> >
> > On Wed, 30 Apr 2025 at 18:03, Anand Moon <linux.amoon@gmail.com> wrote:
> >>
> >> Hi All,
> >>
> >> This patch series is a rework of my previous patch series [1],
> >> where the code changes were not adequately justified.
> >>
> >> In this new series, I have improved the commit subject
> >> and commit message to better explain the changes.
> >>
> >> v6: Add new patch to use devm_clk_get_enabled
> >>     and Fix few typo in subject as suggested by Daniel.
> >> v5: Drop the guard mutex patch
> >> v4: Tried to address Lukasz review comments.
> >>
> >> Tested on Odroid U3 amd XU4 SoC boards.
> >> Build with clang with W=1 enable.
> >>
> >
> > Genital Ping!!!
>
>
> Huhu, nice. :)
> I make typos as well, but some typos are better to avoid. :)
>
> Anyway, !!! are exclamation marks and I think it is very difficult to
> scream at someone gently. I think this is contradictory to itself, so it
> does not feel gently at all.
>
I did not mean anything harsh with these !!! marks.

> Plus you sent it 7 days ago and you are known to send poor quality,
> untested code, so just relax and wait.
>
NAK - Do not merge these changes. The original code is in better condition,
and the proposed modifications introduce issues.

> Best regards,
> Krzysztof

Thanks
-Anand

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Daniel Lezcano]

On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
> Hi All,

Hi Anand,

if the goal of the changes is to do cleanups, I recommend to rework
how the code is organized. Instead of having the data->soc check all
around the functions, write per platform functions and store them in
struct of_device_id data field instead of the soc version.

Basically get rid of exynos_map_dt_data by settings the different ops
in a per platform structure.

Then the initialization routine would be simpler to clean.

> This patch series is a rework of my previous patch series [1],
> where the code changes were not adequately justified.
> 
> In this new series, I have improved the commit subject
> and commit message to better explain the changes.
> 
> v6: Add new patch to use devm_clk_get_enabled
>     and Fix few typo in subject as suggested by Daniel.
> v5: Drop the guard mutex patch
> v4: Tried to address Lukasz review comments.
> 
> Tested on Odroid U3 amd XU4 SoC boards.
> Build with clang with W=1 enable.
> 
> [4] https://lore.kernel.org/all/20250410063754.5483-2-linux.amoon@gmail.com/
> [3] https://lore.kernel.org/all/20250310143450.8276-2-linux.amoon@gmail.com/
> [2] https://lore.kernel.org/all/20250216195850.5352-2-linux.amoon@gmail.com/
> [1] https://lore.kernel.org/all/20220515064126.1424-1-linux.amoon@gmail.com/
> [0] https://lore.kernel.org/lkml/CANAwSgS=08fVsqn95WHzSF71WTTyD2-=K2C6-BEz0tY0t6A1-g@mail.gmail.com/T/#m77e57120d230d57f34c29e1422d7fc5f5587ac30
> 
> Thanks
> -Anand
> 
> Anand Moon (4):
>   thermal/drivers/exynos: Refactor clk_sec initialization inside
>     SOC-specific case
>   thermal/drivers/exynos: Use devm_clk_get_enabled() helpers
>   thermal/drivers/exynos: Remove redundant IS_ERR() checks for clk_sec
>     clock
>   thermal/drivers/exynos: Fixed the efuse min max value for exynos5422
> 
>  drivers/thermal/samsung/exynos_tmu.c | 100 ++++++++++-----------------
>  1 file changed, 35 insertions(+), 65 deletions(-)
> 
> 
> base-commit: b6ea1680d0ac0e45157a819c41b46565f4616186
> -- 
> 2.49.0
> 

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Anand Moon]

Hi Daniel,

On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
> > Hi All,
>
> Hi Anand,
>
> if the goal of the changes is to do cleanups, I recommend to rework
> how the code is organized. Instead of having the data->soc check all
> around the functions, write per platform functions and store them in
> struct of_device_id data field instead of the soc version.
>
> Basically get rid of exynos_map_dt_data by settings the different ops
> in a per platform structure.
>
> Then the initialization routine would be simpler to clean.
>

Thanks, I had previously attempted this approach.
The goal is to split the exynos_tmu_data structure to accommodate
SoC-specific callbacks for initialization and configuration.

In my earlier attempt, I tried to refactor the code to achieve this.
However, the main challenge I encountered was that the
exynos_sensor_ops weren’t being correctly mapped for each SoC.

Some SoC have multiple sensor
exynos4x12
                    tmu: tmu@100c0000
exynos5420
                tmu_cpu0: tmu@10060000
                tmu_cpu1: tmu@10064000
                tmu_cpu2: tmu@10068000
                tmu_cpu3: tmu@1006c000
                tmu_gpu: tmu@100a0000
 exynos5433
                tmu_atlas0: tmu@10060000
                tmu_atlas1: tmu@10068000
                tmu_g3d: tmu@10070000
exynos7
                tmu@10060000

It could be a design issue of the structure.or some DTS issue.
So what I found in debugging it is not working correctly.

static const struct thermal_zone_device_ops exynos_sensor_ops = {
        .get_temp = exynos_get_temp,
        .set_emul_temp = exynos_tmu_set_emulation,
        .set_trips = exynos_set_trips,
};

The sensor callback will not return a valid pointer and soc id for the get_temp.

Here is my earlier version of local changes.
[1] https://pastebin.com/bbEP04Zh exynos_tmu.c
[2] https://pastebin.com/PzNz5yve Odroid U3 dmesg.log
[3] https://pastebin.com/4Yjt2d2u    Odroid Xu4 dmesg.log

I want to re-model the structure to improve the code.
Once Its working condition I will send this for review.

If you have some suggestions please let me know.

Thanks
-Anand

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Daniel Lezcano]

On 5/15/25 13:10, Anand Moon wrote:
> Hi Daniel,
> 
> On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
>>> Hi All,
>>
>> Hi Anand,
>>
>> if the goal of the changes is to do cleanups, I recommend to rework
>> how the code is organized. Instead of having the data->soc check all
>> around the functions, write per platform functions and store them in
>> struct of_device_id data field instead of the soc version.
>>
>> Basically get rid of exynos_map_dt_data by settings the different ops
>> in a per platform structure.
>>
>> Then the initialization routine would be simpler to clean.
>>
> 
> Thanks, I had previously attempted this approach.
> The goal is to split the exynos_tmu_data structure to accommodate
> SoC-specific callbacks for initialization and configuration.
> 
> In my earlier attempt, I tried to refactor the code to achieve this.
> However, the main challenge I encountered was that the
> exynos_sensor_ops weren’t being correctly mapped for each SoC.
> 
> Some SoC have multiple sensor
> exynos4x12
>                      tmu: tmu@100c0000
> exynos5420
>                  tmu_cpu0: tmu@10060000
>                  tmu_cpu1: tmu@10064000
>                  tmu_cpu2: tmu@10068000
>                  tmu_cpu3: tmu@1006c000
>                  tmu_gpu: tmu@100a0000
>   exynos5433
>                  tmu_atlas0: tmu@10060000
>                  tmu_atlas1: tmu@10068000
>                  tmu_g3d: tmu@10070000
> exynos7
>                  tmu@10060000
> 
> It could be a design issue of the structure.or some DTS issue.
> So what I found in debugging it is not working correctly.
> 
> static const struct thermal_zone_device_ops exynos_sensor_ops = {
>          .get_temp = exynos_get_temp,
>          .set_emul_temp = exynos_tmu_set_emulation,
>          .set_trips = exynos_set_trips,
> };
> 
> The sensor callback will not return a valid pointer and soc id for the get_temp.
> 
> Here is my earlier version of local changes.
> [1] https://pastebin.com/bbEP04Zh exynos_tmu.c
> [2] https://pastebin.com/PzNz5yve Odroid U3 dmesg.log
> [3] https://pastebin.com/4Yjt2d2u    Odroid Xu4 dmesg.log
> 
> I want to re-model the structure to improve the code.
> Once Its working condition I will send this for review.
> 
> If you have some suggestions please let me know.

I suggest to do the conversion step by step beginning by 
exynos4210_tmu_clear_irqs, then by exynos_map_dt_data as the first 
cleanup iteration


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Anand Moon]

Hi Daniel,

On Thu, 15 May 2025 at 18:59, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 5/15/25 13:10, Anand Moon wrote:
> > Hi Daniel,
> >
> > On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
> >>> Hi All,
> >>
> >> Hi Anand,
> >>
> >> if the goal of the changes is to do cleanups, I recommend to rework
> >> how the code is organized. Instead of having the data->soc check all
> >> around the functions, write per platform functions and store them in
> >> struct of_device_id data field instead of the soc version.
> >>
> >> Basically get rid of exynos_map_dt_data by settings the different ops
> >> in a per platform structure.
> >>
> >> Then the initialization routine would be simpler to clean.
> >>
> >
> > Thanks, I had previously attempted this approach.
> > The goal is to split the exynos_tmu_data structure to accommodate
> > SoC-specific callbacks for initialization and configuration.
> >
> > In my earlier attempt, I tried to refactor the code to achieve this.
> > However, the main challenge I encountered was that the
> > exynos_sensor_ops weren’t being correctly mapped for each SoC.
> >
> > Some SoC have multiple sensor
> > exynos4x12
> >                      tmu: tmu@100c0000
> > exynos5420
> >                  tmu_cpu0: tmu@10060000
> >                  tmu_cpu1: tmu@10064000
> >                  tmu_cpu2: tmu@10068000
> >                  tmu_cpu3: tmu@1006c000
> >                  tmu_gpu: tmu@100a0000
> >   exynos5433
> >                  tmu_atlas0: tmu@10060000
> >                  tmu_atlas1: tmu@10068000
> >                  tmu_g3d: tmu@10070000
> > exynos7
> >                  tmu@10060000
> >
> > It could be a design issue of the structure.or some DTS issue.
> > So what I found in debugging it is not working correctly.
> >
> > static const struct thermal_zone_device_ops exynos_sensor_ops = {
> >          .get_temp = exynos_get_temp,
> >          .set_emul_temp = exynos_tmu_set_emulation,
> >          .set_trips = exynos_set_trips,
> > };
> >
> > The sensor callback will not return a valid pointer and soc id for the get_temp.
> >
> > Here is my earlier version of local changes.
> > [1] https://pastebin.com/bbEP04Zh exynos_tmu.c
> > [2] https://pastebin.com/PzNz5yve Odroid U3 dmesg.log
> > [3] https://pastebin.com/4Yjt2d2u    Odroid Xu4 dmesg.log
> >
> > I want to re-model the structure to improve the code.
> > Once Its working condition I will send this for review.
> >
> > If you have some suggestions please let me know.
>
> I suggest to do the conversion step by step beginning by
> exynos4210_tmu_clear_irqs, then by exynos_map_dt_data as the first
> cleanup iteration
>
Ok you want IRQ handle per SoC call back functions?
so on all the changes depending on SoC id should be moved to
respective callback functions to reduce the code.

Thank  you for having a look into my changes

Thanks
-Anand
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Daniel Lezcano]

On 5/15/25 20:01, Anand Moon wrote:
> Hi Daniel,
> 
> On Thu, 15 May 2025 at 18:59, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 5/15/25 13:10, Anand Moon wrote:
>>> Hi Daniel,
>>>
>>> On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
>>>>> Hi All,
>>>>
>>>> Hi Anand,
>>>>
>>>> if the goal of the changes is to do cleanups, I recommend to rework
>>>> how the code is organized. Instead of having the data->soc check all
>>>> around the functions, write per platform functions and store them in
>>>> struct of_device_id data field instead of the soc version.
>>>>
>>>> Basically get rid of exynos_map_dt_data by settings the different ops
>>>> in a per platform structure.
>>>>
>>>> Then the initialization routine would be simpler to clean.
>>>>
>>>
>>> Thanks, I had previously attempted this approach.
>>> The goal is to split the exynos_tmu_data structure to accommodate
>>> SoC-specific callbacks for initialization and configuration.
>>>
>>> In my earlier attempt, I tried to refactor the code to achieve this.
>>> However, the main challenge I encountered was that the
>>> exynos_sensor_ops weren’t being correctly mapped for each SoC.
>>>
>>> Some SoC have multiple sensor
>>> exynos4x12
>>>                       tmu: tmu@100c0000
>>> exynos5420
>>>                   tmu_cpu0: tmu@10060000
>>>                   tmu_cpu1: tmu@10064000
>>>                   tmu_cpu2: tmu@10068000
>>>                   tmu_cpu3: tmu@1006c000
>>>                   tmu_gpu: tmu@100a0000
>>>    exynos5433
>>>                   tmu_atlas0: tmu@10060000
>>>                   tmu_atlas1: tmu@10068000
>>>                   tmu_g3d: tmu@10070000
>>> exynos7
>>>                   tmu@10060000
>>>
>>> It could be a design issue of the structure.or some DTS issue.
>>> So what I found in debugging it is not working correctly.
>>>
>>> static const struct thermal_zone_device_ops exynos_sensor_ops = {
>>>           .get_temp = exynos_get_temp,
>>>           .set_emul_temp = exynos_tmu_set_emulation,
>>>           .set_trips = exynos_set_trips,
>>> };
>>>
>>> The sensor callback will not return a valid pointer and soc id for the get_temp.
>>>
>>> Here is my earlier version of local changes.
>>> [1] https://pastebin.com/bbEP04Zh exynos_tmu.c
>>> [2] https://pastebin.com/PzNz5yve Odroid U3 dmesg.log
>>> [3] https://pastebin.com/4Yjt2d2u    Odroid Xu4 dmesg.log
>>>
>>> I want to re-model the structure to improve the code.
>>> Once Its working condition I will send this for review.
>>>
>>> If you have some suggestions please let me know.
>>
>> I suggest to do the conversion step by step beginning by
>> exynos4210_tmu_clear_irqs, then by exynos_map_dt_data as the first
>> cleanup iteration
>>
> Ok you want IRQ handle per SoC call back functions?
> so on all the changes depending on SoC id should be moved to
> respective callback functions to reduce the code.

I think you can keep the same irq handler function but move the 
tmu_intstat, tmu_intclear in the persoc structure and remove the 
exynos4210_tmu_clear_irqs function.

You should end up with something like:

static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
{
	struct exynos_tmu_data *data = id;
	unsigned int val_irq;

	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);

	mutex_lock(&data->lock);
	clk_enable(data->clk);

	val_irq = readl(data->base + data->tmu_intstat);
         writel(val_irq, data->base + data->tmu_intclear);

	clk_disable(data->clk);
	mutex_unlock(&data->lock);

	return IRQ_HANDLED;
}

But if the irq handler has some soc specific code, then it should be a 
separate function

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Anand Moon]

Hi Daniel,

On Fri, 16 May 2025 at 20:15, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 5/15/25 20:01, Anand Moon wrote:
> > Hi Daniel,
> >
> > On Thu, 15 May 2025 at 18:59, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 5/15/25 13:10, Anand Moon wrote:
> >>> Hi Daniel,
> >>>
> >>> On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
> >>>>> Hi All,
> >>>>
> >>>> Hi Anand,
> >>>>
> >>>> if the goal of the changes is to do cleanups, I recommend to rework
> >>>> how the code is organized. Instead of having the data->soc check all
> >>>> around the functions, write per platform functions and store them in
> >>>> struct of_device_id data field instead of the soc version.
> >>>>
> >>>> Basically get rid of exynos_map_dt_data by settings the different ops
> >>>> in a per platform structure.
> >>>>
> >>>> Then the initialization routine would be simpler to clean.
> >>>>
> >>>
> >>> Thanks, I had previously attempted this approach.
> >>> The goal is to split the exynos_tmu_data structure to accommodate
> >>> SoC-specific callbacks for initialization and configuration.
> >>>
> >>> In my earlier attempt, I tried to refactor the code to achieve this.
> >>> However, the main challenge I encountered was that the
> >>> exynos_sensor_ops weren’t being correctly mapped for each SoC.
> >>>
> >>> Some SoC have multiple sensor
> >>> exynos4x12
> >>>                       tmu: tmu@100c0000
> >>> exynos5420
> >>>                   tmu_cpu0: tmu@10060000
> >>>                   tmu_cpu1: tmu@10064000
> >>>                   tmu_cpu2: tmu@10068000
> >>>                   tmu_cpu3: tmu@1006c000
> >>>                   tmu_gpu: tmu@100a0000
> >>>    exynos5433
> >>>                   tmu_atlas0: tmu@10060000
> >>>                   tmu_atlas1: tmu@10068000
> >>>                   tmu_g3d: tmu@10070000
> >>> exynos7
> >>>                   tmu@10060000
> >>>
> >>> It could be a design issue of the structure.or some DTS issue.
> >>> So what I found in debugging it is not working correctly.
> >>>
> >>> static const struct thermal_zone_device_ops exynos_sensor_ops = {
> >>>           .get_temp = exynos_get_temp,
> >>>           .set_emul_temp = exynos_tmu_set_emulation,
> >>>           .set_trips = exynos_set_trips,
> >>> };
> >>>
> >>> The sensor callback will not return a valid pointer and soc id for the get_temp.
> >>>
> >>> Here is my earlier version of local changes.
> >>> [1] https://pastebin.com/bbEP04Zh exynos_tmu.c
> >>> [2] https://pastebin.com/PzNz5yve Odroid U3 dmesg.log
> >>> [3] https://pastebin.com/4Yjt2d2u    Odroid Xu4 dmesg.log
> >>>
> >>> I want to re-model the structure to improve the code.
> >>> Once Its working condition I will send this for review.
> >>>
> >>> If you have some suggestions please let me know.
> >>
> >> I suggest to do the conversion step by step beginning by
> >> exynos4210_tmu_clear_irqs, then by exynos_map_dt_data as the first
> >> cleanup iteration
> >>
> > Ok you want IRQ handle per SoC call back functions?
> > so on all the changes depending on SoC id should be moved to
> > respective callback functions to reduce the code.
>
> I think you can keep the same irq handler function but move the
> tmu_intstat, tmu_intclear in the persoc structure and remove the
> exynos4210_tmu_clear_irqs function.
>
> You should end up with something like:
>
> static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> {
>         struct exynos_tmu_data *data = id;
>         unsigned int val_irq;
>
>         thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>
>         mutex_lock(&data->lock);
>         clk_enable(data->clk);
>
>         val_irq = readl(data->base + data->tmu_intstat);
>          writel(val_irq, data->base + data->tmu_intclear);
>
>         clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return IRQ_HANDLED;
> }

No this will not work,
>
> But if the irq handler has some soc specific code, then it should be a
> separate function
>

INTSTAT interrupt status register holds the pending status of
rising and falling edge of IRQ

#define INTSTAT_FALL2   BIT(24)
#define INTSTAT_FALL1   BIT(20)
#define INTSTAT_FALL0   BIT(16)
#define INTSTAT_RISE2   BIT(8)
#define INTSTAT_RISE1   BIT(4)
#define INTSTAT_RISE0   BIT(0)

#define INTCLEAR_FALL2  BIT(24)
#define INTCLEAR_FALL1  BIT(20)
#define INTCLEAR_FALL0  BIT(16)
#define INTCLEAR_RISE2  BIT(8)
#define INTCLEAR_RISE1  BIT(4)
#define INTCLEAR_RISE0  BIT(0)

static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
{
        u32 tmu_intstat, tmu_intclear;
        u32 val_irq = 0, clear_mask = 0;

        if (data->soc == SOC_ARCH_EXYNOS5260) {
                tmu_intstat = EXYNOS5260_TMU_REG_INTSTAT;
                tmu_intclear = EXYNOS5260_TMU_REG_INTCLEAR;
        } else if (data->soc == SOC_ARCH_EXYNOS7) {
                tmu_intstat = EXYNOS7_TMU_REG_INTPEND;
                tmu_intclear = EXYNOS7_TMU_REG_INTPEND;
        } else if (data->soc == SOC_ARCH_EXYNOS5433) {
                tmu_intstat = EXYNOS5433_TMU_REG_INTPEND;
                tmu_intclear = EXYNOS5433_TMU_REG_INTPEND;
        } else {
                tmu_intstat = EXYNOS_TMU_REG_INTSTAT;
                tmu_intclear = EXYNOS_TMU_REG_INTCLEAR;
        }

        val_irq = readl(data->base + tmu_intstat);
        /*
         * Clear the interrupts.  Please note that the documentation for
         * Exynos3250, Exynos4412, Exynos5250 and Exynos5260 incorrectly
         * states that INTCLEAR register has a different placing of bits
         * responsible for FALL IRQs than INTSTAT register.  Exynos5420
         * and Exynos5440 documentation is correct (Exynos4210 doesn't
         * support FALL IRQs at all).
         */

        /* Map INTSTAT bits to INTCLEAR bits */
        if (val_irq & BIT(24))
                clear_mask |= BIT(24);
        else if (val_irq & BIT(20))
                clear_mask |= BIT(20);
        else if (val_irq & BIT(16))
                clear_mask |= BIT(16);
        else if (val_irq & BIT(8))
                clear_mask |= BIT(8);
        else if (val_irq & BIT(4))
                clear_mask |= BIT(4);
        else if (val_irq & BIT(0))
                clear_mask |= BIT(0);

        /* Perform proper task for decrease temperature */
        if (clear_mask)
                writel(clear_mask, data->base + tmu_intclear);
}

TMU clears the rising and falling interrupt according to the user manual.

Thanks
-Anand
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v6 2/4] thermal/drivers/exynos: Use devm_clk_get_enabled() helpers

[Mateusz Majewski]

Hello :)

These:

> +	data->clk = devm_clk_get_enabled(dev, "tmu_apbif");

> +		data->clk_sec = devm_clk_get_enabled(dev, "tmu_triminfo_apbif");

should probably call devm_clk_get_prepared instead, as they are only
prepared inside current code:

> -	ret = clk_prepare(data->clk);

> -		ret = clk_prepare(data->clk_sec);

as elsewhere they are only enabled on use and then disabled. Only
data->sclk is enabled immediately:

> -			ret = clk_prepare_enable(data->sclk);

Thank you,
Mateusz Majewski