[PATCHv2 0/6] Exynos Thermal code inprovement

[PATCHv2 0/6] Exynos Thermal code inprovement

[Anand Moon]

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
-- 
2.36.1

[PATCHv2 0/6] Exynos Thermal code inprovement

[Anand Moon]

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
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Anand Moon]

Use clk_prepare_enable api to enable tmu internal hardware clock
flag on, use clk_disable_unprepare to disable the clock.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: split te changes and improve the commit message.
---
 drivers/thermal/samsung/exynos_tmu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f4ab4c5b4b62..75b3afadb5be 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1054,14 +1054,14 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 			goto err_sensor;
 		}
 	} else {
-		ret = clk_prepare(data->clk_sec);
+		ret = clk_prepare_enable(data->clk_sec);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to get clock\n");
 			goto err_sensor;
 		}
 	}
 
-	ret = clk_prepare(data->clk);
+	ret = clk_prepare_enable(data->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to get clock\n");
 		goto err_clk_sec;
@@ -1122,10 +1122,10 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_sclk:
 	clk_disable_unprepare(data->sclk);
 err_clk:
-	clk_unprepare(data->clk);
+	clk_disable_unprepare(data->clk);
 err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+		clk_disable_unprepare(data->clk_sec);
 err_sensor:
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
@@ -1142,9 +1142,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 	exynos_tmu_control(pdev, false);
 
 	clk_disable_unprepare(data->sclk);
-	clk_unprepare(data->clk);
+	clk_disable_unprepare(data->clk);
 	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+		clk_disable_unprepare(data->clk_sec);
 
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
-- 
2.36.1

[PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Anand Moon]

Use clk_prepare_enable api to enable tmu internal hardware clock
flag on, use clk_disable_unprepare to disable the clock.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: split te changes and improve the commit message.
---
 drivers/thermal/samsung/exynos_tmu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f4ab4c5b4b62..75b3afadb5be 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1054,14 +1054,14 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 			goto err_sensor;
 		}
 	} else {
-		ret = clk_prepare(data->clk_sec);
+		ret = clk_prepare_enable(data->clk_sec);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to get clock\n");
 			goto err_sensor;
 		}
 	}
 
-	ret = clk_prepare(data->clk);
+	ret = clk_prepare_enable(data->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to get clock\n");
 		goto err_clk_sec;
@@ -1122,10 +1122,10 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_sclk:
 	clk_disable_unprepare(data->sclk);
 err_clk:
-	clk_unprepare(data->clk);
+	clk_disable_unprepare(data->clk);
 err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+		clk_disable_unprepare(data->clk_sec);
 err_sensor:
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
@@ -1142,9 +1142,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 	exynos_tmu_control(pdev, false);
 
 	clk_disable_unprepare(data->sclk);
-	clk_unprepare(data->clk);
+	clk_disable_unprepare(data->clk);
 	if (!IS_ERR(data->clk_sec))
-		clk_unprepare(data->clk_sec);
+		clk_disable_unprepare(data->clk_sec);
 
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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(-)

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);
-- 
2.36.1

[PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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(-)

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);
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed

[Anand Moon]

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);
 
 	status = readb(data->base + EXYNOS_TMU_REG_STATUS);
 	if (!status) {
@@ -323,8 +322,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 err:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
+	clk_disable(data->clk_sec);
 out:
 	return ret;
 }
@@ -1119,8 +1117,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_thermal:
 	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
 err_clk_sec:
-	if (!IS_ERR(data->clk_sec))
-		clk_disable_unprepare(data->clk_sec);
+	clk_disable_unprepare(data->clk_sec);
 err_sclk:
 	clk_disable_unprepare(data->sclk);
 err_clk_apbif:
@@ -1142,8 +1139,7 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(data->sclk);
 	clk_disable_unprepare(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable_unprepare(data->clk_sec);
+	clk_disable_unprepare(data->clk_sec);
 
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
-- 
2.36.1

[PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed

[Anand Moon]

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);
 
 	status = readb(data->base + EXYNOS_TMU_REG_STATUS);
 	if (!status) {
@@ -323,8 +322,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 err:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
+	clk_disable(data->clk_sec);
 out:
 	return ret;
 }
@@ -1119,8 +1117,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_thermal:
 	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
 err_clk_sec:
-	if (!IS_ERR(data->clk_sec))
-		clk_disable_unprepare(data->clk_sec);
+	clk_disable_unprepare(data->clk_sec);
 err_sclk:
 	clk_disable_unprepare(data->sclk);
 err_clk_apbif:
@@ -1142,8 +1139,7 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(data->sclk);
 	clk_disable_unprepare(data->clk);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable_unprepare(data->clk_sec);
+	clk_disable_unprepare(data->clk_sec);
 
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Anand Moon]

As per Exynos5422 user manaul 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.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: Fix the commit message
---
 drivers/thermal/samsung/exynos_tmu.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 58ff1b577c47..0faec0f16db6 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -926,12 +926,14 @@ 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)
+		if (data->soc == SOC_ARCH_EXYNOS5420 &&
+		    data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
+			data->min_efuse_value = 16;
+			data->max_efuse_value = 76;
+		} else {
 			data->min_efuse_value = 40;
-		else
-			data->min_efuse_value = 0;
-		data->max_efuse_value = 100;
+			data->max_efuse_value = 100;
+		}
 		break;
 	case SOC_ARCH_EXYNOS5433:
 		data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
-- 
2.36.1

[PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Anand Moon]

As per Exynos5422 user manaul 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.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: Fix the commit message
---
 drivers/thermal/samsung/exynos_tmu.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 58ff1b577c47..0faec0f16db6 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -926,12 +926,14 @@ 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)
+		if (data->soc == SOC_ARCH_EXYNOS5420 &&
+		    data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
+			data->min_efuse_value = 16;
+			data->max_efuse_value = 76;
+		} else {
 			data->min_efuse_value = 40;
-		else
-			data->min_efuse_value = 0;
-		data->max_efuse_value = 100;
+			data->max_efuse_value = 100;
+		}
 		break;
 	case SOC_ARCH_EXYNOS5433:
 		data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

Use the newlly introduced pm_sleep_ptr() macro, and mark the
functions __maybe_unused. These functions can then be moved outside the
CONFIG_PM_SUSPEND block, and the compiler can then process them and
detect build failures independently of the config. If unused, they will
simply be discarded by the compiler.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series.
---
 drivers/thermal/samsung/exynos_tmu.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 0faec0f16db6..f8a527f19383 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1149,15 +1149,14 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int exynos_tmu_suspend(struct device *dev)
+static int __maybe_unused exynos_tmu_suspend(struct device *dev)
 {
 	exynos_tmu_control(to_platform_device(dev), false);
 
 	return 0;
 }
 
-static int exynos_tmu_resume(struct device *dev)
+static int __maybe_unused exynos_tmu_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 
@@ -1169,15 +1168,11 @@ static int exynos_tmu_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(exynos_tmu_pm,
 			 exynos_tmu_suspend, exynos_tmu_resume);
-#define EXYNOS_TMU_PM	(&exynos_tmu_pm)
-#else
-#define EXYNOS_TMU_PM	NULL
-#endif
 
 static struct platform_driver exynos_tmu_driver = {
 	.driver = {
 		.name   = "exynos-tmu",
-		.pm     = EXYNOS_TMU_PM,
+		.pm     = pm_sleep_ptr(&exynos_tmu_pm),
 		.of_match_table = exynos_tmu_match,
 	},
 	.probe = exynos_tmu_probe,
-- 
2.36.1

[PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

Use the newlly introduced pm_sleep_ptr() macro, and mark the
functions __maybe_unused. These functions can then be moved outside the
CONFIG_PM_SUSPEND block, and the compiler can then process them and
detect build failures independently of the config. If unused, they will
simply be discarded by the compiler.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series.
---
 drivers/thermal/samsung/exynos_tmu.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 0faec0f16db6..f8a527f19383 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1149,15 +1149,14 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int exynos_tmu_suspend(struct device *dev)
+static int __maybe_unused exynos_tmu_suspend(struct device *dev)
 {
 	exynos_tmu_control(to_platform_device(dev), false);
 
 	return 0;
 }
 
-static int exynos_tmu_resume(struct device *dev)
+static int __maybe_unused exynos_tmu_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 
@@ -1169,15 +1168,11 @@ static int exynos_tmu_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(exynos_tmu_pm,
 			 exynos_tmu_suspend, exynos_tmu_resume);
-#define EXYNOS_TMU_PM	(&exynos_tmu_pm)
-#else
-#define EXYNOS_TMU_PM	NULL
-#endif
 
 static struct platform_driver exynos_tmu_driver = {
 	.driver = {
 		.name   = "exynos-tmu",
-		.pm     = EXYNOS_TMU_PM,
+		.pm     = pm_sleep_ptr(&exynos_tmu_pm),
 		.of_match_table = exynos_tmu_match,
 	},
 	.probe = exynos_tmu_probe,
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Anand Moon]

Add runtime power management for exynos thermal driver.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series.
---
 drivers/thermal/samsung/exynos_tmu.c | 29 ++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f8a527f19383..be9b98caf2ba 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
 
 #include <dt-bindings/thermal/thermal_exynos.h>
 
@@ -1106,6 +1107,15 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_thermal;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0)
+		goto disable_runtime_pm;
+
 	ret = devm_request_irq(&pdev->dev, data->irq, exynos_tmu_irq,
 		IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data);
 	if (ret) {
@@ -1113,11 +1123,16 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_thermal;
 	}
 
+	pm_runtime_put(&pdev->dev);
+
 	exynos_tmu_control(pdev, true);
 	return 0;
 
 err_thermal:
 	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
+disable_runtime_pm:
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 err_clk_sec:
 	clk_disable_unprepare(data->clk_sec);
 err_sclk:
@@ -1143,6 +1158,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 	clk_disable_unprepare(data->clk);
 	clk_disable_unprepare(data->clk_sec);
 
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
 
@@ -1151,18 +1169,25 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 
 static int __maybe_unused exynos_tmu_suspend(struct device *dev)
 {
-	exynos_tmu_control(to_platform_device(dev), false);
+	struct platform_device *pdev = to_platform_device(dev);
 
-	return 0;
+	exynos_tmu_control(pdev, false);
+
+	return pm_runtime_force_suspend(&pdev->dev);
 }
 
 static int __maybe_unused exynos_tmu_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	int ret;
 
 	exynos_tmu_initialize(pdev);
 	exynos_tmu_control(pdev, true);
 
+	ret = pm_runtime_force_resume(&pdev->dev);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.36.1

[PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Anand Moon]

Add runtime power management for exynos thermal driver.

Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v1: new patch in this series.
---
 drivers/thermal/samsung/exynos_tmu.c | 29 ++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f8a527f19383..be9b98caf2ba 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
 
 #include <dt-bindings/thermal/thermal_exynos.h>
 
@@ -1106,6 +1107,15 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_thermal;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0)
+		goto disable_runtime_pm;
+
 	ret = devm_request_irq(&pdev->dev, data->irq, exynos_tmu_irq,
 		IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data);
 	if (ret) {
@@ -1113,11 +1123,16 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_thermal;
 	}
 
+	pm_runtime_put(&pdev->dev);
+
 	exynos_tmu_control(pdev, true);
 	return 0;
 
 err_thermal:
 	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
+disable_runtime_pm:
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 err_clk_sec:
 	clk_disable_unprepare(data->clk_sec);
 err_sclk:
@@ -1143,6 +1158,9 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 	clk_disable_unprepare(data->clk);
 	clk_disable_unprepare(data->clk_sec);
 
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
 	if (!IS_ERR(data->regulator))
 		regulator_disable(data->regulator);
 
@@ -1151,18 +1169,25 @@ static int exynos_tmu_remove(struct platform_device *pdev)
 
 static int __maybe_unused exynos_tmu_suspend(struct device *dev)
 {
-	exynos_tmu_control(to_platform_device(dev), false);
+	struct platform_device *pdev = to_platform_device(dev);
 
-	return 0;
+	exynos_tmu_control(pdev, false);
+
+	return pm_runtime_force_suspend(&pdev->dev);
 }
 
 static int __maybe_unused exynos_tmu_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	int ret;
 
 	exynos_tmu_initialize(pdev);
 	exynos_tmu_control(pdev, true);
 
+	ret = pm_runtime_force_resume(&pdev->dev);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed

[Krzysztof Kozlowski]

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

Re: [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Krzysztof Kozlowski]

On 15/05/2022 08:41, Anand Moon wrote:
> As per Exynos5422 user manaul 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.
> 
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Krzysztof Kozlowski]

On 15/05/2022 08:41, Anand Moon wrote:
> As per Exynos5422 user manaul 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.
> 
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Krzysztof Kozlowski]

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

Re: [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Krzysztof Kozlowski]

On 15/05/2022 08:41, Anand Moon wrote:
> Add runtime power management for exynos thermal driver.

First of all - why? Second, I do not see it being added. Where are the
runtime callbacks?


Best regards,
Krzysztof

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Krzysztof Kozlowski]

On 15/05/2022 08:41, Anand Moon wrote:
> Add runtime power management for exynos thermal driver.

First of all - why? Second, I do not see it being added. Where are the
runtime callbacks?


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[kernel test robot]

Hi Anand,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
base:   ec7f49619d8ee13e108740c82f942cd401b989e9
config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
        git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
                   if (data->soc == SOC_ARCH_EXYNOS5420 &&
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
   1 warning generated.


vim +929 drivers/thermal/samsung/exynos_tmu.c

   865	
   866	static int exynos_map_dt_data(struct platform_device *pdev)
   867	{
   868		struct exynos_tmu_data *data = platform_get_drvdata(pdev);
   869		struct resource res;
   870	
   871		if (!data || !pdev->dev.of_node)
   872			return -ENODEV;
   873	
   874		data->id = of_alias_get_id(pdev->dev.of_node, "tmuctrl");
   875		if (data->id < 0)
   876			data->id = 0;
   877	
   878		data->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
   879		if (data->irq <= 0) {
   880			dev_err(&pdev->dev, "failed to get IRQ\n");
   881			return -ENODEV;
   882		}
   883	
   884		if (of_address_to_resource(pdev->dev.of_node, 0, &res)) {
   885			dev_err(&pdev->dev, "failed to get Resource 0\n");
   886			return -ENODEV;
   887		}
   888	
   889		data->base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
   890		if (!data->base) {
   891			dev_err(&pdev->dev, "Failed to ioremap memory\n");
   892			return -EADDRNOTAVAIL;
   893		}
   894	
   895		data->soc = (enum soc_type)of_device_get_match_data(&pdev->dev);
   896	
   897		switch (data->soc) {
   898		case SOC_ARCH_EXYNOS4210:
   899			data->tmu_set_trip_temp = exynos4210_tmu_set_trip_temp;
   900			data->tmu_set_trip_hyst = exynos4210_tmu_set_trip_hyst;
   901			data->tmu_initialize = exynos4210_tmu_initialize;
   902			data->tmu_control = exynos4210_tmu_control;
   903			data->tmu_read = exynos4210_tmu_read;
   904			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   905			data->ntrip = 4;
   906			data->gain = 15;
   907			data->reference_voltage = 7;
   908			data->efuse_value = 55;
   909			data->min_efuse_value = 40;
   910			data->max_efuse_value = 100;
   911			break;
   912		case SOC_ARCH_EXYNOS3250:
   913		case SOC_ARCH_EXYNOS4412:
   914		case SOC_ARCH_EXYNOS5250:
   915		case SOC_ARCH_EXYNOS5260:
   916		case SOC_ARCH_EXYNOS5420:
   917		case SOC_ARCH_EXYNOS5420_TRIMINFO:
   918			data->tmu_set_trip_temp = exynos4412_tmu_set_trip_temp;
   919			data->tmu_set_trip_hyst = exynos4412_tmu_set_trip_hyst;
   920			data->tmu_initialize = exynos4412_tmu_initialize;
   921			data->tmu_control = exynos4210_tmu_control;
   922			data->tmu_read = exynos4412_tmu_read;
   923			data->tmu_set_emulation = exynos4412_tmu_set_emulation;
   924			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   925			data->ntrip = 4;
   926			data->gain = 8;
   927			data->reference_voltage = 16;
   928			data->efuse_value = 55;
 > 929			if (data->soc == SOC_ARCH_EXYNOS5420 &&
   930			    data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
   931				data->min_efuse_value = 16;
   932				data->max_efuse_value = 76;
   933			} else {
   934				data->min_efuse_value = 40;
   935				data->max_efuse_value = 100;
   936			}
   937			break;
   938		case SOC_ARCH_EXYNOS5433:
   939			data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
   940			data->tmu_set_trip_hyst = exynos5433_tmu_set_trip_hyst;
   941			data->tmu_initialize = exynos5433_tmu_initialize;
   942			data->tmu_control = exynos5433_tmu_control;
   943			data->tmu_read = exynos4412_tmu_read;
   944			data->tmu_set_emulation = exynos4412_tmu_set_emulation;
   945			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   946			data->ntrip = 8;
   947			data->gain = 8;
   948			if (res.start == EXYNOS5433_G3D_BASE)
   949				data->reference_voltage = 23;
   950			else
   951				data->reference_voltage = 16;
   952			data->efuse_value = 75;
   953			data->min_efuse_value = 40;
   954			data->max_efuse_value = 150;
   955			break;
   956		case SOC_ARCH_EXYNOS7:
   957			data->tmu_set_trip_temp = exynos7_tmu_set_trip_temp;
   958			data->tmu_set_trip_hyst = exynos7_tmu_set_trip_hyst;
   959			data->tmu_initialize = exynos7_tmu_initialize;
   960			data->tmu_control = exynos7_tmu_control;
   961			data->tmu_read = exynos7_tmu_read;
   962			data->tmu_set_emulation = exynos4412_tmu_set_emulation;
   963			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   964			data->ntrip = 8;
   965			data->gain = 9;
   966			data->reference_voltage = 17;
   967			data->efuse_value = 75;
   968			data->min_efuse_value = 15;
   969			data->max_efuse_value = 100;
   970			break;
   971		default:
   972			dev_err(&pdev->dev, "Platform not supported\n");
   973			return -EINVAL;
   974		}
   975	
   976		data->cal_type = TYPE_ONE_POINT_TRIMMING;
   977	
   978		/*
   979		 * Check if the TMU shares some registers and then try to map the
   980		 * memory of common registers.
   981		 */
   982		if (data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
   983			return 0;
   984	
   985		if (of_address_to_resource(pdev->dev.of_node, 1, &res)) {
   986			dev_err(&pdev->dev, "failed to get Resource 1\n");
   987			return -ENODEV;
   988		}
   989	
   990		data->base_second = devm_ioremap(&pdev->dev, res.start,
   991						resource_size(&res));
   992		if (!data->base_second) {
   993			dev_err(&pdev->dev, "Failed to ioremap memory\n");
   994			return -ENOMEM;
   995		}
   996	
   997		return 0;
   998	}
   999	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[kernel test robot]

Hi Anand,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
base:   ec7f49619d8ee13e108740c82f942cd401b989e9
config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
        git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
                   if (data->soc == SOC_ARCH_EXYNOS5420 &&
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
   1 warning generated.


vim +929 drivers/thermal/samsung/exynos_tmu.c

   865	
   866	static int exynos_map_dt_data(struct platform_device *pdev)
   867	{
   868		struct exynos_tmu_data *data = platform_get_drvdata(pdev);
   869		struct resource res;
   870	
   871		if (!data || !pdev->dev.of_node)
   872			return -ENODEV;
   873	
   874		data->id = of_alias_get_id(pdev->dev.of_node, "tmuctrl");
   875		if (data->id < 0)
   876			data->id = 0;
   877	
   878		data->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
   879		if (data->irq <= 0) {
   880			dev_err(&pdev->dev, "failed to get IRQ\n");
   881			return -ENODEV;
   882		}
   883	
   884		if (of_address_to_resource(pdev->dev.of_node, 0, &res)) {
   885			dev_err(&pdev->dev, "failed to get Resource 0\n");
   886			return -ENODEV;
   887		}
   888	
   889		data->base = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
   890		if (!data->base) {
   891			dev_err(&pdev->dev, "Failed to ioremap memory\n");
   892			return -EADDRNOTAVAIL;
   893		}
   894	
   895		data->soc = (enum soc_type)of_device_get_match_data(&pdev->dev);
   896	
   897		switch (data->soc) {
   898		case SOC_ARCH_EXYNOS4210:
   899			data->tmu_set_trip_temp = exynos4210_tmu_set_trip_temp;
   900			data->tmu_set_trip_hyst = exynos4210_tmu_set_trip_hyst;
   901			data->tmu_initialize = exynos4210_tmu_initialize;
   902			data->tmu_control = exynos4210_tmu_control;
   903			data->tmu_read = exynos4210_tmu_read;
   904			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   905			data->ntrip = 4;
   906			data->gain = 15;
   907			data->reference_voltage = 7;
   908			data->efuse_value = 55;
   909			data->min_efuse_value = 40;
   910			data->max_efuse_value = 100;
   911			break;
   912		case SOC_ARCH_EXYNOS3250:
   913		case SOC_ARCH_EXYNOS4412:
   914		case SOC_ARCH_EXYNOS5250:
   915		case SOC_ARCH_EXYNOS5260:
   916		case SOC_ARCH_EXYNOS5420:
   917		case SOC_ARCH_EXYNOS5420_TRIMINFO:
   918			data->tmu_set_trip_temp = exynos4412_tmu_set_trip_temp;
   919			data->tmu_set_trip_hyst = exynos4412_tmu_set_trip_hyst;
   920			data->tmu_initialize = exynos4412_tmu_initialize;
   921			data->tmu_control = exynos4210_tmu_control;
   922			data->tmu_read = exynos4412_tmu_read;
   923			data->tmu_set_emulation = exynos4412_tmu_set_emulation;
   924			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   925			data->ntrip = 4;
   926			data->gain = 8;
   927			data->reference_voltage = 16;
   928			data->efuse_value = 55;
 > 929			if (data->soc == SOC_ARCH_EXYNOS5420 &&
   930			    data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
   931				data->min_efuse_value = 16;
   932				data->max_efuse_value = 76;
   933			} else {
   934				data->min_efuse_value = 40;
   935				data->max_efuse_value = 100;
   936			}
   937			break;
   938		case SOC_ARCH_EXYNOS5433:
   939			data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
   940			data->tmu_set_trip_hyst = exynos5433_tmu_set_trip_hyst;
   941			data->tmu_initialize = exynos5433_tmu_initialize;
   942			data->tmu_control = exynos5433_tmu_control;
   943			data->tmu_read = exynos4412_tmu_read;
   944			data->tmu_set_emulation = exynos4412_tmu_set_emulation;
   945			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   946			data->ntrip = 8;
   947			data->gain = 8;
   948			if (res.start == EXYNOS5433_G3D_BASE)
   949				data->reference_voltage = 23;
   950			else
   951				data->reference_voltage = 16;
   952			data->efuse_value = 75;
   953			data->min_efuse_value = 40;
   954			data->max_efuse_value = 150;
   955			break;
   956		case SOC_ARCH_EXYNOS7:
   957			data->tmu_set_trip_temp = exynos7_tmu_set_trip_temp;
   958			data->tmu_set_trip_hyst = exynos7_tmu_set_trip_hyst;
   959			data->tmu_initialize = exynos7_tmu_initialize;
   960			data->tmu_control = exynos7_tmu_control;
   961			data->tmu_read = exynos7_tmu_read;
   962			data->tmu_set_emulation = exynos4412_tmu_set_emulation;
   963			data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
   964			data->ntrip = 8;
   965			data->gain = 9;
   966			data->reference_voltage = 17;
   967			data->efuse_value = 75;
   968			data->min_efuse_value = 15;
   969			data->max_efuse_value = 100;
   970			break;
   971		default:
   972			dev_err(&pdev->dev, "Platform not supported\n");
   973			return -EINVAL;
   974		}
   975	
   976		data->cal_type = TYPE_ONE_POINT_TRIMMING;
   977	
   978		/*
   979		 * Check if the TMU shares some registers and then try to map the
   980		 * memory of common registers.
   981		 */
   982		if (data->soc != SOC_ARCH_EXYNOS5420_TRIMINFO)
   983			return 0;
   984	
   985		if (of_address_to_resource(pdev->dev.of_node, 1, &res)) {
   986			dev_err(&pdev->dev, "failed to get Resource 1\n");
   987			return -ENODEV;
   988		}
   989	
   990		data->base_second = devm_ioremap(&pdev->dev, res.start,
   991						resource_size(&res));
   992		if (!data->base_second) {
   993			dev_err(&pdev->dev, "Failed to ioremap memory\n");
   994			return -ENOMEM;
   995		}
   996	
   997		return 0;
   998	}
   999	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Krzysztof Kozlowski]

On 16/05/2022 12:42, kernel test robot wrote:
> Hi Anand,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> base:   ec7f49619d8ee13e108740c82f942cd401b989e9
> config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
>         git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
>                    if (data->soc == SOC_ARCH_EXYNOS5420 &&
>                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
>    1 warning generated.

Ah, I did not notice it and it seems code was not compile-tested with W=1.

Anand, please be sure you compile your code with W=1...


Best regards,
Krzysztof

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Krzysztof Kozlowski]

On 16/05/2022 12:42, kernel test robot wrote:
> Hi Anand,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> base:   ec7f49619d8ee13e108740c82f942cd401b989e9
> config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
>         git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
>                    if (data->soc == SOC_ARCH_EXYNOS5420 &&
>                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
>    1 warning generated.

Ah, I did not notice it and it seems code was not compile-tested with W=1.

Anand, please be sure you compile your code with W=1...


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Krzysztof Kozlowski]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

On 16/05/2022 12:42, kernel test robot wrote:
> Hi Anand,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> base:   ec7f49619d8ee13e108740c82f942cd401b989e9
> config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp(a)intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
>         git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
>                    if (data->soc == SOC_ARCH_EXYNOS5420 &&
>                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
>    1 warning generated.

Ah, I did not notice it and it seems code was not compile-tested with W=1.

Anand, please be sure you compile your code with W=1...


Best regards,
Krzysztof

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Anand Moon]

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

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed

[Anand Moon]

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

Re: [PATCHv2 3/6] thermal: exynos: Check before clk_disable_unprepare() not needed

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Anand Moon]

Hi Krzysztof,

On Mon, 16 May 2022 at 16:14, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/05/2022 12:42, kernel test robot wrote:
> > Hi Anand,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> > base:   ec7f49619d8ee13e108740c82f942cd401b989e9
> > config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp@intel.com/config)
> > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> >         git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> >>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
> >                    if (data->soc == SOC_ARCH_EXYNOS5420 &&
> >                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
> >    1 warning generated.
>
> Ah, I did not notice it and it seems code was not compile-tested with W=1.
>
> Anand, please be sure you compile your code with W=1...
>
Ok I will try to resolve this warning in the next version.
>
> Best regards,
> Krzysztof

Thanks & Regards

-Anand

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Anand Moon]

Hi Krzysztof,

On Mon, 16 May 2022 at 16:14, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/05/2022 12:42, kernel test robot wrote:
> > Hi Anand,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> > base:   ec7f49619d8ee13e108740c82f942cd401b989e9
> > config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp@intel.com/config)
> > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> >         git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> >>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
> >                    if (data->soc == SOC_ARCH_EXYNOS5420 &&
> >                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
> >    1 warning generated.
>
> Ah, I did not notice it and it seems code was not compile-tested with W=1.
>
> Anand, please be sure you compile your code with W=1...
>
Ok I will try to resolve this warning in the next version.
>
> Best regards,
> Krzysztof

Thanks & Regards

-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 4/6] thermal: exynos: fixed the efuse min/max value for exynos5422

[Anand Moon]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

Hi Krzysztof,

On Mon, 16 May 2022 at 16:14, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/05/2022 12:42, kernel test robot wrote:
> > Hi Anand,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on ec7f49619d8ee13e108740c82f942cd401b989e9]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> > base:   ec7f49619d8ee13e108740c82f942cd401b989e9
> > config: hexagon-randconfig-r033-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161820.8rHIcsvI-lkp(a)intel.com/config)
> > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/intel-lab-lkp/linux/commit/eb50b0c2100fabd6d09b87abd11f52c5295512e8
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review Anand-Moon/Exynos-Thermal-code-inprovement/20220515-144336
> >         git checkout eb50b0c2100fabd6d09b87abd11f52c5295512e8
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/thermal/samsung/
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> >>> drivers/thermal/samsung/exynos_tmu.c:929:40: warning: overlapping comparisons always evaluate to false [-Wtautological-overlap-compare]
> >                    if (data->soc == SOC_ARCH_EXYNOS5420 &&
> >                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
> >    1 warning generated.
>
> Ah, I did not notice it and it seems code was not compile-tested with W=1.
>
> Anand, please be sure you compile your code with W=1...
>
Ok I will try to resolve this warning in the next version.
>
> Best regards,
> Krzysztof

Thanks & Regards

-Anand

Re: [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

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

Re: [PATCHv2 5/6] thermal: exynos: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Anand Moon]

Hi Krzysztof,

On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > Add runtime power management for exynos thermal driver.
>
> First of all - why? Second, I do not see it being added. Where are the
> runtime callbacks?
>

To control runtime control PMU, did I miss something?
I looked into imx thermal driver # drivers/thermal/imx_thermal.c
to enable run-time power management for exynos driver.

>
> Best regards,
> Krzysztof

Thanks & Regards


-Anand

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Anand Moon]

Hi Krzysztof,

On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/05/2022 08:41, Anand Moon wrote:
> > Add runtime power management for exynos thermal driver.
>
> First of all - why? Second, I do not see it being added. Where are the
> runtime callbacks?
>

To control runtime control PMU, did I miss something?
I looked into imx thermal driver # drivers/thermal/imx_thermal.c
to enable run-time power management for exynos driver.

>
> Best regards,
> Krzysztof

Thanks & Regards


-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Krzysztof Kozlowski]

On 17/05/2022 20:45, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 15/05/2022 08:41, Anand Moon wrote:
>>> Add runtime power management for exynos thermal driver.
>>
>> First of all - why? Second, I do not see it being added. Where are the
>> runtime callbacks?
>>
> 
> To control runtime control PMU, did I miss something?

Controlling runtime PM by itself is not a goal. What does it change if
it is enabled?

> I looked into imx thermal driver # drivers/thermal/imx_thermal.c
> to enable run-time power management for exynos driver.

So you have runtime PM enabled and then what happens? Where is the power
saving? Since you did not implement the callbacks, all this should be
explained in commit msg.


Best regards,
Krzysztof

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Krzysztof Kozlowski]

On 17/05/2022 20:45, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 15/05/2022 08:41, Anand Moon wrote:
>>> Add runtime power management for exynos thermal driver.
>>
>> First of all - why? Second, I do not see it being added. Where are the
>> runtime callbacks?
>>
> 
> To control runtime control PMU, did I miss something?

Controlling runtime PM by itself is not a goal. What does it change if
it is enabled?

> I looked into imx thermal driver # drivers/thermal/imx_thermal.c
> to enable run-time power management for exynos driver.

So you have runtime PM enabled and then what happens? Where is the power
saving? Since you did not implement the callbacks, all this should be
explained in commit msg.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Anand Moon]

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

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Anand Moon]

Hi Krzysztof,

On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/05/2022 20:45, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 15/05/2022 08:41, Anand Moon wrote:
> >>> Add runtime power management for exynos thermal driver.
> >>
> >> First of all - why? Second, I do not see it being added. Where are the
> >> runtime callbacks?
> >>
> >
> > To control runtime control PMU, did I miss something?
>
> Controlling runtime PM by itself is not a goal. What does it change if
> it is enabled?
>
It means we could have efficient power management for this driver.
as per my understanding, it controls runtime sleep and improves power efficiency

> > I looked into imx thermal driver # drivers/thermal/imx_thermal.c
> > to enable run-time power management for exynos driver.
>
> So you have runtime PM enabled and then what happens? Where is the power
> saving? Since you did not implement the callbacks, all this should be
> explained in commit msg.
>
Ok, As per the original code, it just registers the SIMPLE_DEV_PM_OPS
with .pm = &exynos_tmu_pm
So I have made sure that suspend resume feature works correctly
 with these changes on SBC Odroid U3 and XU4.

I will try to look into setting RUNTIME_PM_OPS
or use UNIVERSAL_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS
any thought on this?

>
> Best regards,
> Krzysztof

Thanks and Regards

-Anand

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Anand Moon]

Hi Krzysztof,

On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/05/2022 20:45, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 15/05/2022 08:41, Anand Moon wrote:
> >>> Add runtime power management for exynos thermal driver.
> >>
> >> First of all - why? Second, I do not see it being added. Where are the
> >> runtime callbacks?
> >>
> >
> > To control runtime control PMU, did I miss something?
>
> Controlling runtime PM by itself is not a goal. What does it change if
> it is enabled?
>
It means we could have efficient power management for this driver.
as per my understanding, it controls runtime sleep and improves power efficiency

> > I looked into imx thermal driver # drivers/thermal/imx_thermal.c
> > to enable run-time power management for exynos driver.
>
> So you have runtime PM enabled and then what happens? Where is the power
> saving? Since you did not implement the callbacks, all this should be
> explained in commit msg.
>
Ok, As per the original code, it just registers the SIMPLE_DEV_PM_OPS
with .pm = &exynos_tmu_pm
So I have made sure that suspend resume feature works correctly
 with these changes on SBC Odroid U3 and XU4.

I will try to look into setting RUNTIME_PM_OPS
or use UNIVERSAL_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS
any thought on this?

>
> Best regards,
> Krzysztof

Thanks and Regards

-Anand

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Krzysztof Kozlowski]

On 21/05/2022 11:52, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/05/2022 20:45, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 15/05/2022 08:41, Anand Moon wrote:
>>>>> Add runtime power management for exynos thermal driver.
>>>>
>>>> First of all - why? Second, I do not see it being added. Where are the
>>>> runtime callbacks?
>>>>
>>>
>>> To control runtime control PMU, did I miss something?
>>
>> Controlling runtime PM by itself is not a goal. What does it change if
>> it is enabled?
>>
> It means we could have efficient power management for this driver.
> as per my understanding, it controls runtime sleep and improves power efficiency

How? I asked - what is being changed after enabling PM - and you
answered without any specifics. Where exactly is the power saving?
Please be specific, very specific.

> 
>>> I looked into imx thermal driver # drivers/thermal/imx_thermal.c
>>> to enable run-time power management for exynos driver.
>>
>> So you have runtime PM enabled and then what happens? Where is the power
>> saving? Since you did not implement the callbacks, all this should be
>> explained in commit msg.
>>
> Ok, As per the original code, it just registers the SIMPLE_DEV_PM_OPS
> with .pm = &exynos_tmu_pm

And does nothing else, right? No benefits?

> So I have made sure that suspend resume feature works correctly
>  with these changes on SBC Odroid U3 and XU4.

How is suspend/resume related to runtime PM? Are you talking about
system suspend? What do you mean now?

> 
> I will try to look into setting RUNTIME_PM_OPS
> or use UNIVERSAL_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS
> any thought on this?

Why looking at them? You avoid giving any specific answer, so we are
repeating the same and the same. Just to be sure - maybe I don't see the
obvious stuff, so please explain also this obvious things.

Please come with specifics, because otherwise I see it as a waste of our
time.

Best regards,
Krzysztof

Re: [PATCHv2 6/6] thermal: exynos: Add runtime power management for tmu

[Krzysztof Kozlowski]

On 21/05/2022 11:52, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Wed, 18 May 2022 at 12:49, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/05/2022 20:45, Anand Moon wrote:
>>> Hi Krzysztof,
>>>
>>> On Sun, 15 May 2022 at 15:18, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 15/05/2022 08:41, Anand Moon wrote:
>>>>> Add runtime power management for exynos thermal driver.
>>>>
>>>> First of all - why? Second, I do not see it being added. Where are the
>>>> runtime callbacks?
>>>>
>>>
>>> To control runtime control PMU, did I miss something?
>>
>> Controlling runtime PM by itself is not a goal. What does it change if
>> it is enabled?
>>
> It means we could have efficient power management for this driver.
> as per my understanding, it controls runtime sleep and improves power efficiency

How? I asked - what is being changed after enabling PM - and you
answered without any specifics. Where exactly is the power saving?
Please be specific, very specific.

> 
>>> I looked into imx thermal driver # drivers/thermal/imx_thermal.c
>>> to enable run-time power management for exynos driver.
>>
>> So you have runtime PM enabled and then what happens? Where is the power
>> saving? Since you did not implement the callbacks, all this should be
>> explained in commit msg.
>>
> Ok, As per the original code, it just registers the SIMPLE_DEV_PM_OPS
> with .pm = &exynos_tmu_pm

And does nothing else, right? No benefits?

> So I have made sure that suspend resume feature works correctly
>  with these changes on SBC Odroid U3 and XU4.

How is suspend/resume related to runtime PM? Are you talking about
system suspend? What do you mean now?

> 
> I will try to look into setting RUNTIME_PM_OPS
> or use UNIVERSAL_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS
> any thought on this?

Why looking at them? You avoid giving any specific answer, so we are
repeating the same and the same. Just to be sure - maybe I don't see the
obvious stuff, so please explain also this obvious things.

Please come with specifics, because otherwise I see it as a waste of our
time.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

Re: [PATCHv2 1/6] thermal: exynos: Enable core tmu hardware clk flag on exynos platform

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

Re: [PATCHv2 2/6] thermal: exynos: Reorder the gpu clock initialization for exynos5420 SoC

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel