[PATCH 0/6] Add initial Exynos 850 support to the thermal driver

[PATCH 0/6] Add initial Exynos 850 support to the thermal driver

[Mateusz Majewski]

This series adds initial Exynos 850 support to the thermal driver
together with its requirements (tmu_temp_mask fix, making data->clk
optional, adding the new string to dt-bindings), while also cleaning up
a bit (using DEFINE_SIMPLE_DEV_PM_OPS and removing some outdated
information from dt-bindings).

Mateusz Majewski (6):
  drivers/thermal/exynos: use DEFINE_SIMPLE_DEV_PM_OPS
  drivers/thermal/exynos: use tmu_temp_mask consistently
  drivers/thermal/exynos: check IS_ERR(data->clk) consistently
  dt-bindings: thermal: samsung,exynos: add exynos850-tmu string
  drivers/thermal/exynos: add initial Exynos 850 support
  dt-bindings: thermal: samsung,exynos: remove outdated information on
    trip point count

 .../thermal/samsung,exynos-thermal.yaml       |  33 ++-
 drivers/thermal/samsung/exynos_tmu.c          | 279 +++++++++++++++---
 2 files changed, 270 insertions(+), 42 deletions(-)

-- 
2.45.1

[PATCH 1/6] drivers/thermal/exynos: use DEFINE_SIMPLE_DEV_PM_OPS

[Mateusz Majewski]

SIMPLE_DEV_PM_OPS is deprecated, as noted next to its definition.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 96cffb2c44ba..9b7ca93a72f1 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1150,8 +1150,8 @@ static int exynos_tmu_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(exynos_tmu_pm,
-			 exynos_tmu_suspend, exynos_tmu_resume);
+static DEFINE_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
-- 
2.45.1

[PATCH 2/6] drivers/thermal/exynos: use tmu_temp_mask consistently

[Mateusz Majewski]

Some of the usages in sanitize_temp_error were missed, probably because
the boards being used never actually exceeded 255 in their trimming
information. This is needed for Exynos 850 support, which uses 9-bit
temperature codes.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 9b7ca93a72f1..61606a9b9a00 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -237,17 +237,17 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
 
 	data->temp_error1 = trim_info & tmu_temp_mask;
 	data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
-				EXYNOS_TMU_TEMP_MASK);
+				tmu_temp_mask);
 
 	if (!data->temp_error1 ||
 	    (data->min_efuse_value > data->temp_error1) ||
 	    (data->temp_error1 > data->max_efuse_value))
-		data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;
+		data->temp_error1 = data->efuse_value & tmu_temp_mask;
 
 	if (!data->temp_error2)
 		data->temp_error2 =
 			(data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) &
-			EXYNOS_TMU_TEMP_MASK;
+			tmu_temp_mask;
 }
 
 static int exynos_tmu_initialize(struct platform_device *pdev)
-- 
2.45.1

[PATCH 3/6] drivers/thermal/exynos: check IS_ERR(data->clk) consistently

[Mateusz Majewski]

This will be needed for Exynos 850 support, which does not require this
clock.

It would also be possible to set data->clk to NULL instead, but doing it
like this is consistent with what we do with data->clk_sec.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 58 ++++++++++++++++++----------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 61606a9b9a00..f0de72a62fd7 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -257,7 +257,8 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 	int ret = 0;
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 	if (!IS_ERR(data->clk_sec))
 		clk_enable(data->clk_sec);
 
@@ -271,7 +272,8 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 
 	if (!IS_ERR(data->clk_sec))
 		clk_disable(data->clk_sec);
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return ret;
@@ -295,11 +297,13 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
 	}
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	data->tmu_set_crit_temp(data, temp / MCELSIUS);
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return 0;
@@ -328,10 +332,12 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 	data->tmu_control(pdev, on);
 	data->enabled = on;
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 }
 
@@ -648,7 +654,8 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 		return -EAGAIN;
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	value = data->tmu_read(data);
 	if (value < 0)
@@ -656,7 +663,8 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
 	else
 		*temp = code_to_temp(data, value) * MCELSIUS;
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return ret;
@@ -723,9 +731,11 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
 		goto out;
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 	data->tmu_set_emulation(data, temp);
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 	return 0;
 out:
@@ -763,12 +773,14 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
 	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	/* TODO: take action based on particular interrupt */
 	data->tmu_clear_irqs(data);
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return IRQ_HANDLED;
@@ -979,7 +991,8 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 	struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
 
 	mutex_lock(&data->lock);
-	clk_enable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_enable(data->clk);
 
 	if (low > INT_MIN)
 		data->tmu_set_low_temp(data, low / MCELSIUS);
@@ -990,7 +1003,8 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
 	else
 		data->tmu_disable_high(data);
 
-	clk_disable(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
 	return 0;
@@ -1053,10 +1067,12 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = clk_prepare(data->clk);
-	if (ret) {
-		dev_err(dev, "Failed to get clock\n");
-		goto err_clk_sec;
+	if (!IS_ERR(data->clk)) {
+		ret = clk_prepare(data->clk);
+		if (ret) {
+			dev_err(dev, "Failed to get clock\n");
+			goto err_clk_sec;
+		}
 	}
 
 	switch (data->soc) {
@@ -1113,7 +1129,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_sclk:
 	clk_disable_unprepare(data->sclk);
 err_clk:
-	clk_unprepare(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_unprepare(data->clk);
 err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
@@ -1127,7 +1144,8 @@ static void exynos_tmu_remove(struct platform_device *pdev)
 	exynos_tmu_control(pdev, false);
 
 	clk_disable_unprepare(data->sclk);
-	clk_unprepare(data->clk);
+	if (!IS_ERR(data->clk))
+		clk_unprepare(data->clk);
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
 }
-- 
2.45.1

[PATCH 4/6] dt-bindings: thermal: samsung,exynos: add exynos850-tmu string

[Mateusz Majewski]

Note that unlike others, Exynos 850 does not require clocks, hence we
have to be a little be more specific about when the related properties
are required.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 .../thermal/samsung,exynos-thermal.yaml       | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
index 29a08b0729ee..4363ee625339 100644
--- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
@@ -27,6 +27,7 @@ properties:
       - samsung,exynos5420-tmu-ext-triminfo
       - samsung,exynos5433-tmu
       - samsung,exynos7-tmu
+      - samsung,exynos850-tmu
 
   clocks:
     minItems: 1
@@ -69,8 +70,6 @@ properties:
 
 required:
   - compatible
-  - clocks
-  - clock-names
   - interrupts
   - reg
 
@@ -82,6 +81,9 @@ allOf:
           contains:
             const: samsung,exynos5420-tmu-ext-triminfo
     then:
+      required:
+        - clocks
+        - clock-names
       properties:
         clocks:
           items:
@@ -105,6 +107,9 @@ allOf:
               - samsung,exynos5433-tmu
               - samsung,exynos7-tmu
     then:
+      required:
+        - clocks
+        - clock-names
       properties:
         clocks:
           items:
@@ -132,6 +137,9 @@ allOf:
               - samsung,exynos5260-tmu
               - samsung,exynos5420-tmu
     then:
+      required:
+        - clocks
+        - clock-names
       properties:
         clocks:
           minItems: 1
@@ -140,6 +148,20 @@ allOf:
           minItems: 1
           maxItems: 1
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - samsung,exynos850-tmu
+    then:
+      properties:
+        clocks: false
+        clock-names: false
+        reg:
+          minItems: 1
+          maxItems: 1
+
 additionalProperties: false
 
 examples:
-- 
2.45.1

[PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Mateusz Majewski]

This is loosely adapted from an implementation available at
https://gitlab.com/Linaro/96boards/e850-96/kernel/-/blob/android-exynos-4.14-linaro/drivers/thermal/samsung/exynos_tmu.c
Some differences from that implementation:
- unlike that implementation, we do not use the ACPM mechanism, instead
  we just access the registers, like we do for other SoCs,
- the SoC is supposed to support multiple sensors inside one unit. The
  vendor implementation uses one kernel device per sensor, we would
  probably prefer to have one device for all sensors, have
  #thermal-sensor-cells = <1> and so on. We implemented this, but we
  could not get the extra sensors to work on our hardware so far. This
  might be due to a misconfiguration and we will probably come back to
  this, however our implementation only supports a single sensor for
  now,
- the vendor implementation supports disabling CPU cores as a cooling
  device. We did not attempt to port this, and this would not really fit
  this driver anyway.

Additionally, some differences from the other SoCs supported by this
driver:
- this SoC does not require a clock to work correctly, so we need an
  exception for data->clk,
- we do not really constrain the e-fuse information like the other SoCs
  do (data->{min,max}_efuse_value). In our tests, those values (as well
  as the raw sensor values) were much higher than in the other SoCs, to
  the degree that reusing the data->{min,max}_efuse_value from the other
  SoCs would cause instant critical temperature reset on boot,
- this SoC provides more information in the e-fuse data than other SoCs,
  so we read some values inside exynos850_tmu_initialize instead of
  hardcoding them in exynos_map_dt_data.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 214 +++++++++++++++++++++++++--
 1 file changed, 202 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index f0de72a62fd7..bd52663f1a5a 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -116,6 +116,43 @@
 #define EXYNOS7_EMUL_DATA_SHIFT			7
 #define EXYNOS7_EMUL_DATA_MASK			0x1ff
 
+/* Exynos850 specific registers */
+#define EXYNOS850_TMU_REG_AVG_CON		0x58
+#define EXYNOS850_TMU_REG_CONTROL1		0x24
+#define EXYNOS850_TMU_REG_COUNTER_VALUE0	0x30
+#define EXYNOS850_TMU_REG_COUNTER_VALUE1	0x34
+#define EXYNOS850_TMU_REG_CURRENT_TEMP1_0	0x40
+#define EXYNOS850_TMU_REG_THD_TEMP0_RISE	0x50
+#define EXYNOS850_TMU_REG_THD_TEMP0_FALL	0x60
+#define EXYNOS850_TMU_REG_TRIM0			0x3C
+
+#define EXYNOS850_TMU_AVG_CON_SHIFT		18
+#define EXYNOS850_TMU_AVG_MODE_MASK		0x7
+#define EXYNOS850_TMU_BGRI_TRIM_MASK		0xF
+#define EXYNOS850_TMU_BGRI_TRIM_SHIFT		20
+#define EXYNOS850_TMU_CLK_SENSE_ON_MASK		0xffff
+#define EXYNOS850_TMU_CLK_SENSE_ON_SHIFT	16
+#define EXYNOS850_TMU_DEM_ENABLE		1
+#define EXYNOS850_TMU_DEM_SHIFT			4
+#define EXYNOS850_TMU_EN_TEMP_SEN_OFF_MASK	0xffff
+#define EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT	0
+#define EXYNOS850_TMU_LPI_MODE_MASK		1
+#define EXYNOS850_TMU_LPI_MODE_SHIFT		10
+#define EXYNOS850_TMU_T_BUF_SLOPE_SEL_MASK	0xF
+#define EXYNOS850_TMU_T_BUF_SLOPE_SEL_SHIFT	18
+#define EXYNOS850_TMU_T_BUF_VREF_SEL_MASK	0x1F
+#define EXYNOS850_TMU_T_BUF_VREF_SEL_SHIFT	18
+#define EXYNOS850_TMU_TEM1051X_SENSE_VALUE	0x028A
+#define EXYNOS850_TMU_TEM1456X_SENSE_VALUE	0x0A28
+#define EXYNOS850_TMU_TEMP_SHIFT		9
+#define EXYNOS850_TMU_TRIMINFO_SHIFT		4
+#define EXYNOS850_TMU_T_TRIM0_MASK		0xF
+#define EXYNOS850_TMU_T_TRIM0_SHIFT		18
+#define EXYNOS850_TMU_VBEI_TRIM_MASK		0xF
+#define EXYNOS850_TMU_VBEI_TRIM_SHIFT		8
+#define EXYNOS850_TMU_VREF_TRIM_MASK		0xF
+#define EXYNOS850_TMU_VREF_TRIM_SHIFT		12
+
 #define EXYNOS_FIRST_POINT_TRIM			25
 #define EXYNOS_SECOND_POINT_TRIM		85
 
@@ -133,6 +170,7 @@ enum soc_type {
 	SOC_ARCH_EXYNOS5420_TRIMINFO,
 	SOC_ARCH_EXYNOS5433,
 	SOC_ARCH_EXYNOS7,
+	SOC_ARCH_EXYNOS850,
 };
 
 /**
@@ -231,13 +269,16 @@ static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
 
 static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
 {
-	u16 tmu_temp_mask =
-		(data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_MASK
-						: EXYNOS_TMU_TEMP_MASK;
+	u16 tmu_temp_mask = (data->soc == SOC_ARCH_EXYNOS7 ||
+			     data->soc == SOC_ARCH_EXYNOS850) ?
+				    EXYNOS7_TMU_TEMP_MASK :
+				    EXYNOS_TMU_TEMP_MASK;
+	int tmu_85_shift = (data->soc == SOC_ARCH_EXYNOS850) ?
+				   EXYNOS850_TMU_TEMP_SHIFT :
+				   EXYNOS_TRIMINFO_85_SHIFT;
 
 	data->temp_error1 = trim_info & tmu_temp_mask;
-	data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
-				tmu_temp_mask);
+	data->temp_error2 = ((trim_info >> tmu_85_shift) & tmu_temp_mask);
 
 	if (!data->temp_error1 ||
 	    (data->min_efuse_value > data->temp_error1) ||
@@ -245,9 +286,8 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
 		data->temp_error1 = data->efuse_value & tmu_temp_mask;
 
 	if (!data->temp_error2)
-		data->temp_error2 =
-			(data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) &
-			tmu_temp_mask;
+		data->temp_error2 = (data->efuse_value >> tmu_85_shift) &
+				    tmu_temp_mask;
 }
 
 static int exynos_tmu_initialize(struct platform_device *pdev)
@@ -588,6 +628,129 @@ static void exynos7_tmu_initialize(struct platform_device *pdev)
 	sanitize_temp_error(data, trim_info);
 }
 
+static void exynos850_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	exynos_tmu_update_temp(data, EXYNOS850_TMU_REG_THD_TEMP0_FALL + 12, 0,
+			       temp);
+	exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_FALL0_SHIFT + 0, true);
+}
+
+static void exynos850_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	exynos_tmu_update_temp(data, EXYNOS850_TMU_REG_THD_TEMP0_RISE + 12, 16,
+			       temp);
+	exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
+			      EXYNOS7_TMU_INTEN_RISE0_SHIFT + 1, true);
+}
+
+static void exynos850_tmu_disable_low(struct exynos_tmu_data *data)
+{
+	exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_FALL0_SHIFT + 0, false);
+}
+
+static void exynos850_tmu_disable_high(struct exynos_tmu_data *data)
+{
+	exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
+			      EXYNOS7_TMU_INTEN_RISE0_SHIFT + 1, false);
+}
+
+static void exynos850_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	exynos_tmu_update_temp(data, EXYNOS850_TMU_REG_THD_TEMP0_RISE + 0, 16,
+			       temp);
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
+			      EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
+	exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
+			      EXYNOS7_TMU_INTEN_RISE0_SHIFT + 7, true);
+}
+
+static void exynos850_tmu_initialize(struct platform_device *pdev)
+{
+	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+	int cal_type;
+	unsigned int avg_mode, buf, bgri, vref, vbei;
+
+	buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
+	cal_type = (buf & EXYNOS5433_TRIMINFO_CALIB_SEL_MASK) >>
+		   EXYNOS5433_TRIMINFO_CALIB_SEL_SHIFT;
+	data->reference_voltage = (buf >> EXYNOS850_TMU_T_BUF_VREF_SEL_SHIFT) &
+				  EXYNOS850_TMU_T_BUF_VREF_SEL_MASK;
+	buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
+		    EXYNOS850_TMU_TRIMINFO_SHIFT);
+	data->gain = (buf >> EXYNOS850_TMU_T_BUF_SLOPE_SEL_SHIFT) &
+		     EXYNOS850_TMU_T_BUF_SLOPE_SEL_MASK;
+	buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
+		    2 * EXYNOS850_TMU_TRIMINFO_SHIFT);
+	avg_mode = (buf >> EXYNOS850_TMU_AVG_CON_SHIFT) &
+		   EXYNOS850_TMU_AVG_MODE_MASK;
+	buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
+		    3 * EXYNOS850_TMU_TRIMINFO_SHIFT);
+	bgri = (buf >> EXYNOS850_TMU_T_TRIM0_SHIFT) &
+	       EXYNOS850_TMU_T_TRIM0_MASK;
+	buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
+		    4 * EXYNOS850_TMU_TRIMINFO_SHIFT);
+	vref = (buf >> EXYNOS850_TMU_T_TRIM0_SHIFT) &
+	       EXYNOS850_TMU_T_TRIM0_MASK;
+	buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
+		    5 * EXYNOS850_TMU_TRIMINFO_SHIFT);
+	vbei = (buf >> EXYNOS850_TMU_T_TRIM0_SHIFT) &
+	       EXYNOS850_TMU_T_TRIM0_MASK;
+
+	buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
+	sanitize_temp_error(data, buf);
+
+	switch (cal_type) {
+	case EXYNOS5433_TRIMINFO_TWO_POINT_TRIMMING:
+		data->cal_type = TYPE_TWO_POINT_TRIMMING;
+		break;
+	case EXYNOS5433_TRIMINFO_ONE_POINT_TRIMMING:
+	default:
+		data->cal_type = TYPE_ONE_POINT_TRIMMING;
+		break;
+	}
+
+	dev_info(&pdev->dev, "Calibration type is %d-point calibration\n",
+		 cal_type ? 2 : 1);
+
+	buf = readl(data->base + EXYNOS850_TMU_REG_AVG_CON);
+	buf &= ~(EXYNOS850_TMU_AVG_MODE_MASK);
+	buf &= ~(EXYNOS850_TMU_DEM_ENABLE << EXYNOS850_TMU_DEM_SHIFT);
+	if (avg_mode) {
+		buf |= avg_mode;
+		buf |= (EXYNOS850_TMU_DEM_ENABLE << EXYNOS850_TMU_DEM_SHIFT);
+	}
+	writel(buf, data->base + EXYNOS850_TMU_REG_AVG_CON);
+
+	buf = readl(data->base + EXYNOS850_TMU_REG_COUNTER_VALUE0);
+	buf &= ~(EXYNOS850_TMU_EN_TEMP_SEN_OFF_MASK
+		 << EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT);
+	buf |= EXYNOS850_TMU_TEM1051X_SENSE_VALUE
+	       << EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT;
+	writel(buf, data->base + EXYNOS850_TMU_REG_COUNTER_VALUE0);
+
+	buf = readl(data->base + EXYNOS850_TMU_REG_COUNTER_VALUE1);
+	buf &= ~(EXYNOS850_TMU_CLK_SENSE_ON_MASK
+		 << EXYNOS850_TMU_CLK_SENSE_ON_SHIFT);
+	buf |= EXYNOS850_TMU_TEM1051X_SENSE_VALUE
+	       << EXYNOS850_TMU_CLK_SENSE_ON_SHIFT;
+	writel(buf, data->base + EXYNOS850_TMU_REG_COUNTER_VALUE1);
+
+	buf = readl(data->base + EXYNOS850_TMU_REG_TRIM0);
+	buf &= ~(EXYNOS850_TMU_BGRI_TRIM_MASK << EXYNOS850_TMU_BGRI_TRIM_SHIFT);
+	buf &= ~(EXYNOS850_TMU_VREF_TRIM_MASK << EXYNOS850_TMU_VREF_TRIM_SHIFT);
+	buf &= ~(EXYNOS850_TMU_VBEI_TRIM_MASK << EXYNOS850_TMU_VBEI_TRIM_SHIFT);
+	buf |= (bgri << EXYNOS850_TMU_BGRI_TRIM_SHIFT);
+	buf |= (vref << EXYNOS850_TMU_VREF_TRIM_SHIFT);
+	buf |= (vbei << EXYNOS850_TMU_VBEI_TRIM_SHIFT);
+	writel(buf, data->base + EXYNOS850_TMU_REG_TRIM0);
+
+	buf = readl(data->base + EXYNOS850_TMU_REG_CONTROL1);
+	buf &= ~(EXYNOS850_TMU_LPI_MODE_MASK << EXYNOS850_TMU_LPI_MODE_SHIFT);
+	writel(buf, data->base + EXYNOS850_TMU_REG_CONTROL1);
+}
+
 static void exynos4210_tmu_control(struct platform_device *pdev, bool on)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
@@ -679,7 +842,8 @@ static u32 get_emul_con_reg(struct exynos_tmu_data *data, unsigned int val,
 
 		val &= ~(EXYNOS_EMUL_TIME_MASK << EXYNOS_EMUL_TIME_SHIFT);
 		val |= (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT);
-		if (data->soc == SOC_ARCH_EXYNOS7) {
+		if (data->soc == SOC_ARCH_EXYNOS7 ||
+		    data->soc == SOC_ARCH_EXYNOS850) {
 			val &= ~(EXYNOS7_EMUL_DATA_MASK <<
 				EXYNOS7_EMUL_DATA_SHIFT);
 			val |= (temp_to_code(data, temp) <<
@@ -709,7 +873,8 @@ static void exynos4412_tmu_set_emulation(struct exynos_tmu_data *data,
 		emul_con = EXYNOS5260_EMUL_CON;
 	else if (data->soc == SOC_ARCH_EXYNOS5433)
 		emul_con = EXYNOS5433_TMU_EMUL_CON;
-	else if (data->soc == SOC_ARCH_EXYNOS7)
+	else if (data->soc == SOC_ARCH_EXYNOS7 ||
+		 data->soc == SOC_ARCH_EXYNOS850)
 		emul_con = EXYNOS7_TMU_REG_EMUL_CON;
 	else
 		emul_con = EXYNOS_EMUL_CON;
@@ -766,6 +931,12 @@ static int exynos7_tmu_read(struct exynos_tmu_data *data)
 		EXYNOS7_TMU_TEMP_MASK;
 }
 
+static int exynos850_tmu_read(struct exynos_tmu_data *data)
+{
+	return readw(data->base + EXYNOS850_TMU_REG_CURRENT_TEMP1_0) &
+	       EXYNOS7_TMU_TEMP_MASK;
+}
+
 static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
 {
 	struct exynos_tmu_data *data = id;
@@ -794,7 +965,8 @@ static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
 	if (data->soc == SOC_ARCH_EXYNOS5260) {
 		tmu_intstat = EXYNOS5260_TMU_REG_INTSTAT;
 		tmu_intclear = EXYNOS5260_TMU_REG_INTCLEAR;
-	} else if (data->soc == SOC_ARCH_EXYNOS7) {
+	} else if (data->soc == SOC_ARCH_EXYNOS7 ||
+		   data->soc == SOC_ARCH_EXYNOS850) {
 		tmu_intstat = EXYNOS7_TMU_REG_INTPEND;
 		tmu_intclear = EXYNOS7_TMU_REG_INTPEND;
 	} else if (data->soc == SOC_ARCH_EXYNOS5433) {
@@ -845,6 +1017,9 @@ static const struct of_device_id exynos_tmu_match[] = {
 	}, {
 		.compatible = "samsung,exynos7-tmu",
 		.data = (const void *)SOC_ARCH_EXYNOS7,
+	}, {
+		.compatible = "samsung,exynos850-tmu",
+		.data = (const void *)SOC_ARCH_EXYNOS850,
 	},
 	{ },
 };
@@ -957,6 +1132,21 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 		data->min_efuse_value = 15;
 		data->max_efuse_value = 100;
 		break;
+	case SOC_ARCH_EXYNOS850:
+		data->tmu_set_low_temp = exynos850_tmu_set_low_temp;
+		data->tmu_set_high_temp = exynos850_tmu_set_high_temp;
+		data->tmu_disable_low = exynos850_tmu_disable_low;
+		data->tmu_disable_high = exynos850_tmu_disable_high;
+		data->tmu_set_crit_temp = exynos850_tmu_set_crit_temp;
+		data->tmu_initialize = exynos850_tmu_initialize;
+		data->tmu_control = exynos4210_tmu_control;
+		data->tmu_read = exynos850_tmu_read;
+		data->tmu_set_emulation = exynos4412_tmu_set_emulation;
+		data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
+		data->efuse_value = 55;
+		data->min_efuse_value = 0;
+		data->max_efuse_value = 511;
+		break;
 	default:
 		dev_err(&pdev->dev, "Platform not supported\n");
 		return -EINVAL;
@@ -1051,7 +1241,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		return ret;
 
 	data->clk = devm_clk_get(dev, "tmu_apbif");
-	if (IS_ERR(data->clk))
+	if (IS_ERR(data->clk) && data->soc != SOC_ARCH_EXYNOS850)
 		return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n");
 
 	data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
-- 
2.45.1

[PATCH 6/6] dt-bindings: thermal: samsung,exynos: remove outdated information on trip point count

[Mateusz Majewski]

This is not true as of commit 5314b1543787 ("thermal/drivers/exynos: Use
set_trips ops").

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 .../bindings/thermal/samsung,exynos-thermal.yaml           | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
index 4363ee625339..5a82764a4dbb 100644
--- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
@@ -40,11 +40,8 @@ properties:
   interrupts:
     description: |
       The Exynos TMU supports generating interrupts when reaching given
-      temperature thresholds. Number of supported thermal trip points depends
-      on the SoC (only first trip points defined in DT will be configured)::
-       - most of SoC: 4
-       - samsung,exynos5433-tmu: 8
-       - samsung,exynos7-tmu: 8
+      temperature thresholds. The trip points will be set dynamically in
+      runtime, which means there is no limit on the number of trip points.
     maxItems: 1
 
   reg:
-- 
2.45.1

Re: [PATCH 0/6] Add initial Exynos 850 support to the thermal driver

[Sam Protsenko]

Hi Mateusz,


On Fri, Jul 19, 2024 at 7:09 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> This series adds initial Exynos 850 support to the thermal driver
> together with its requirements (tmu_temp_mask fix, making data->clk
> optional, adding the new string to dt-bindings), while also cleaning up
> a bit (using DEFINE_SIMPLE_DEV_PM_OPS and removing some outdated
> information from dt-bindings).
>
> Mateusz Majewski (6):
>   drivers/thermal/exynos: use DEFINE_SIMPLE_DEV_PM_OPS
>   drivers/thermal/exynos: use tmu_temp_mask consistently
>   drivers/thermal/exynos: check IS_ERR(data->clk) consistently
>   dt-bindings: thermal: samsung,exynos: add exynos850-tmu string
>   drivers/thermal/exynos: add initial Exynos 850 support
>   dt-bindings: thermal: samsung,exynos: remove outdated information on
>     trip point count
>
>  .../thermal/samsung,exynos-thermal.yaml       |  33 ++-
>  drivers/thermal/samsung/exynos_tmu.c          | 279 +++++++++++++++---
>  2 files changed, 270 insertions(+), 42 deletions(-)
>
> --

Thank you for the contribution! Did you by chance test it on any
hardware, perhaps on E850-96 board? Just noticed there are no dts
changes in this series (or as separate patches). If no -- I'll be glad
to assist you on that, if you can share dts definitions for E850-96
and the testing instructions with me.

Thanks!

> 2.45.1
>
>

Re: [PATCH 1/6] drivers/thermal/exynos: use DEFINE_SIMPLE_DEV_PM_OPS

[Sam Protsenko]

On Fri, Jul 19, 2024 at 7:10 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> SIMPLE_DEV_PM_OPS is deprecated, as noted next to its definition.
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/thermal/samsung/exynos_tmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 96cffb2c44ba..9b7ca93a72f1 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -1150,8 +1150,8 @@ static int exynos_tmu_resume(struct device *dev)
>         return 0;
>  }
>
> -static SIMPLE_DEV_PM_OPS(exynos_tmu_pm,
> -                        exynos_tmu_suspend, exynos_tmu_resume);
> +static DEFINE_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
> --
> 2.45.1
>
>

Re: [PATCH 2/6] drivers/thermal/exynos: use tmu_temp_mask consistently

[Sam Protsenko]

On Fri, Jul 19, 2024 at 7:10 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> Some of the usages in sanitize_temp_error were missed, probably because
> the boards being used never actually exceeded 255 in their trimming
> information. This is needed for Exynos 850 support, which uses 9-bit
> temperature codes.
>

That looks like an actual fix to me, so maybe also add the
corresponding "Fixes:" tag here?

> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 9b7ca93a72f1..61606a9b9a00 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -237,17 +237,17 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
>
>         data->temp_error1 = trim_info & tmu_temp_mask;
>         data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &

EXYNOS_TRIMINFO_85_SHIFT=8 in the driver. Is that value actually
correct in case of Exynos850? I just checked the TRM and it says the
layout for TRIMINFO0 register is as follows:

  - RSVD: Bit [31:24]
  - CALIB_SEL: Bit [23]
  - T_BUF_VREF_SEL: Bit [22:18]
  - TRIMINFO_85_P0: Bit [17:9]
  - TRIMINFO_25_P0: Bit [8:0]

So maybe that shift value should be 9 instead of 8 for Exynos850? Not
sure about other platforms though, this might be also the case for
Exynos7 SoCs too (SOC_ARCH_EXYNOS7 in the driver).

> -                               EXYNOS_TMU_TEMP_MASK);
> +                               tmu_temp_mask);
>
>         if (!data->temp_error1 ||
>             (data->min_efuse_value > data->temp_error1) ||
>             (data->temp_error1 > data->max_efuse_value))
> -               data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;
> +               data->temp_error1 = data->efuse_value & tmu_temp_mask;
>
>         if (!data->temp_error2)
>                 data->temp_error2 =
>                         (data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) &
> -                       EXYNOS_TMU_TEMP_MASK;
> +                       tmu_temp_mask;
>  }
>
>  static int exynos_tmu_initialize(struct platform_device *pdev)
> --
> 2.45.1
>
>

Re: [PATCH 3/6] drivers/thermal/exynos: check IS_ERR(data->clk) consistently

[Sam Protsenko]

On Fri, Jul 19, 2024 at 7:10 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> This will be needed for Exynos 850 support, which does not require this
> clock.
>
> It would also be possible to set data->clk to NULL instead, but doing it
> like this is consistent with what we do with data->clk_sec.
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 58 ++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 61606a9b9a00..f0de72a62fd7 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -257,7 +257,8 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>         int ret = 0;
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);

Hmm, it looks like it's better to rework this clock obtaining using
devm_clk_get_optional() API, or even devm_clk_get_optional_prepared()
in this case. It'll simplify its further usage, i.e. it'll be possible
to just call clk_enable() without checking the clock with IS_ERR()
each time. You can check these drivers for pointers on optional API
usage:

  - drivers/i2c/busses/i2c-exynos5.c
  - drivers/watchdog/s3c2410_wdt.c

Also, if it's only optional for Exynos850 (and not optional for other
chips), maybe it would be a good idea to use *_optional_* API only for
Exynos850 case, so that the driver's behavior for those chips stays
unchanged.

Btw, from the downstream kernel code [1] I can see that the only TMU
clock present in Exynos850 is
GOUT_BLK_PERI_UID_BUSIF_TMU_IPCLKPORT_PCLK (which I was able to
confirm in TRM). But it's not enabled in clk-exynos850.c driver right
now. Do you want me by chance to send the patch adding it?

[1] https://gitlab.com/Linaro/96boards/e850-96/kernel/-/blob/android-exynos-5.10-linaro/drivers/soc/samsung/cal-if/s5e3830/cmucal-node.c?ref_type=heads#L1196

>         if (!IS_ERR(data->clk_sec))
>                 clk_enable(data->clk_sec);
>
> @@ -271,7 +272,8 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>
>         if (!IS_ERR(data->clk_sec))
>                 clk_disable(data->clk_sec);
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return ret;
> @@ -295,11 +297,13 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
>         }
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         data->tmu_set_crit_temp(data, temp / MCELSIUS);
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return 0;
> @@ -328,10 +332,12 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
>         struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>         data->tmu_control(pdev, on);
>         data->enabled = on;
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>  }
>
> @@ -648,7 +654,8 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>                 return -EAGAIN;
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         value = data->tmu_read(data);
>         if (value < 0)
> @@ -656,7 +663,8 @@ static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
>         else
>                 *temp = code_to_temp(data, value) * MCELSIUS;
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return ret;
> @@ -723,9 +731,11 @@ static int exynos_tmu_set_emulation(struct thermal_zone_device *tz, int temp)
>                 goto out;
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>         data->tmu_set_emulation(data, temp);
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>         return 0;
>  out:
> @@ -763,12 +773,14 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
>         thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         /* TODO: take action based on particular interrupt */
>         data->tmu_clear_irqs(data);
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return IRQ_HANDLED;
> @@ -979,7 +991,8 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>         struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
>
>         mutex_lock(&data->lock);
> -       clk_enable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_enable(data->clk);
>
>         if (low > INT_MIN)
>                 data->tmu_set_low_temp(data, low / MCELSIUS);
> @@ -990,7 +1003,8 @@ static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
>         else
>                 data->tmu_disable_high(data);
>
> -       clk_disable(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_disable(data->clk);
>         mutex_unlock(&data->lock);
>
>         return 0;
> @@ -1053,10 +1067,12 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>                 }
>         }
>
> -       ret = clk_prepare(data->clk);
> -       if (ret) {
> -               dev_err(dev, "Failed to get clock\n");
> -               goto err_clk_sec;
> +       if (!IS_ERR(data->clk)) {
> +               ret = clk_prepare(data->clk);
> +               if (ret) {
> +                       dev_err(dev, "Failed to get clock\n");
> +                       goto err_clk_sec;
> +               }
>         }
>
>         switch (data->soc) {
> @@ -1113,7 +1129,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>  err_sclk:
>         clk_disable_unprepare(data->sclk);
>  err_clk:
> -       clk_unprepare(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_unprepare(data->clk);
>  err_clk_sec:
>         if (!IS_ERR(data->clk_sec))
>                 clk_unprepare(data->clk_sec);
> @@ -1127,7 +1144,8 @@ static void exynos_tmu_remove(struct platform_device *pdev)
>         exynos_tmu_control(pdev, false);
>
>         clk_disable_unprepare(data->sclk);
> -       clk_unprepare(data->clk);
> +       if (!IS_ERR(data->clk))
> +               clk_unprepare(data->clk);
>         if (!IS_ERR(data->clk_sec))
>                 clk_unprepare(data->clk_sec);
>  }
> --
> 2.45.1
>
>

Re: [PATCH 4/6] dt-bindings: thermal: samsung,exynos: add exynos850-tmu string

[Sam Protsenko]

On Fri, Jul 19, 2024 at 7:10 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> Note that unlike others, Exynos 850 does not require clocks, hence we

From the TRM and from the downstream kernel [1] I can see that there
exists at least this clock:

    GOUT_BLK_PERI_UID_BUSIF_TMU_IPCLKPORT_PCLK

Isn't that the TMU bus clock (needed to interface the TMU registers)?
Of course, it's not present in the clock driver right now, but it can
be added.

[1] https://gitlab.com/Linaro/96boards/e850-96/kernel/-/blob/android-exynos-5.10-linaro/drivers/soc/samsung/cal-if/s5e3830/cmucal-node.c?ref_type=heads#L1196

> have to be a little be more specific about when the related properties
> are required.
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  .../thermal/samsung,exynos-thermal.yaml       | 26 +++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> index 29a08b0729ee..4363ee625339 100644
> --- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> @@ -27,6 +27,7 @@ properties:
>        - samsung,exynos5420-tmu-ext-triminfo
>        - samsung,exynos5433-tmu
>        - samsung,exynos7-tmu
> +      - samsung,exynos850-tmu
>
>    clocks:
>      minItems: 1
> @@ -69,8 +70,6 @@ properties:
>
>  required:
>    - compatible
> -  - clocks
> -  - clock-names
>    - interrupts
>    - reg
>
> @@ -82,6 +81,9 @@ allOf:
>            contains:
>              const: samsung,exynos5420-tmu-ext-triminfo
>      then:
> +      required:
> +        - clocks
> +        - clock-names
>        properties:
>          clocks:
>            items:
> @@ -105,6 +107,9 @@ allOf:
>                - samsung,exynos5433-tmu
>                - samsung,exynos7-tmu
>      then:
> +      required:
> +        - clocks
> +        - clock-names
>        properties:
>          clocks:
>            items:
> @@ -132,6 +137,9 @@ allOf:
>                - samsung,exynos5260-tmu
>                - samsung,exynos5420-tmu
>      then:
> +      required:
> +        - clocks
> +        - clock-names
>        properties:
>          clocks:
>            minItems: 1
> @@ -140,6 +148,20 @@ allOf:
>            minItems: 1
>            maxItems: 1
>
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - samsung,exynos850-tmu
> +    then:
> +      properties:
> +        clocks: false
> +        clock-names: false
> +        reg:
> +          minItems: 1
> +          maxItems: 1
> +
>  additionalProperties: false
>
>  examples:
> --
> 2.45.1
>
>

Re: [PATCH 6/6] dt-bindings: thermal: samsung,exynos: remove outdated information on trip point count

[Sam Protsenko]

On Fri, Jul 19, 2024 at 7:10 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> This is not true as of commit 5314b1543787 ("thermal/drivers/exynos: Use
> set_trips ops").
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  .../bindings/thermal/samsung,exynos-thermal.yaml           | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> index 4363ee625339..5a82764a4dbb 100644
> --- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> @@ -40,11 +40,8 @@ properties:
>    interrupts:
>      description: |
>        The Exynos TMU supports generating interrupts when reaching given
> -      temperature thresholds. Number of supported thermal trip points depends
> -      on the SoC (only first trip points defined in DT will be configured)::
> -       - most of SoC: 4
> -       - samsung,exynos5433-tmu: 8
> -       - samsung,exynos7-tmu: 8
> +      temperature thresholds. The trip points will be set dynamically in
> +      runtime, which means there is no limit on the number of trip points.
>      maxItems: 1
>
>    reg:
> --
> 2.45.1
>
>

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Sam Protsenko]

On Fri, Jul 19, 2024 at 7:10 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> This is loosely adapted from an implementation available at
> https://gitlab.com/Linaro/96boards/e850-96/kernel/-/blob/android-exynos-4.14-linaro/drivers/thermal/samsung/exynos_tmu.c

Not sure if it's going to be helpful to you, but we also uploaded the
downstream k5.10 a while back, and it features a bit different TMU
driver implementation [1].

[1] https://gitlab.com/Linaro/96boards/e850-96/kernel/-/tree/android-exynos-5.10-linaro/drivers/thermal/samsung?ref_type=heads

> Some differences from that implementation:
> - unlike that implementation, we do not use the ACPM mechanism, instead
>   we just access the registers, like we do for other SoCs,

Do you know what are the possible implications of not using ACPM? As I
understand, ACPM is a Samsung's downstream framework which uses APM
(Active Power Management) IP block internally to act as an IPC
mechanism, which makes it possible to offload any PM related
operations (which might get quite heavy, if we are to belive the TRM
description of APM) from CPU to APM. I'm not against the direct
registers access based implementation (in fact, I'm not sure how that
APM/ACPM thing can be implemented in upstreamable way and if it's
worth it at all). Just curious if we understand what we are
potentially missing out, and if at some point we'll be forced to
implement that ACPM thing anyway (for something else)?

> - the SoC is supposed to support multiple sensors inside one unit. The
>   vendor implementation uses one kernel device per sensor, we would
>   probably prefer to have one device for all sensors, have
>   #thermal-sensor-cells = <1> and so on. We implemented this, but we
>   could not get the extra sensors to work on our hardware so far. This
>   might be due to a misconfiguration and we will probably come back to
>   this, however our implementation only supports a single sensor for
>   now,
> - the vendor implementation supports disabling CPU cores as a cooling
>   device. We did not attempt to port this, and this would not really fit
>   this driver anyway.
>
> Additionally, some differences from the other SoCs supported by this
> driver:
> - this SoC does not require a clock to work correctly, so we need an
>   exception for data->clk,

Not sure if that's true, as already discussed in my comments for the
previous patches. Looks like one clock is still needed, which is the
PCLK bus clock (to interface registers) which might simultaneously act
as an operating (functional) clock.

> - we do not really constrain the e-fuse information like the other SoCs
>   do (data->{min,max}_efuse_value). In our tests, those values (as well
>   as the raw sensor values) were much higher than in the other SoCs, to
>   the degree that reusing the data->{min,max}_efuse_value from the other
>   SoCs would cause instant critical temperature reset on boot,
> - this SoC provides more information in the e-fuse data than other SoCs,
>   so we read some values inside exynos850_tmu_initialize instead of
>   hardcoding them in exynos_map_dt_data.
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 214 +++++++++++++++++++++++++--
>  1 file changed, 202 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index f0de72a62fd7..bd52663f1a5a 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -116,6 +116,43 @@
>  #define EXYNOS7_EMUL_DATA_SHIFT                        7
>  #define EXYNOS7_EMUL_DATA_MASK                 0x1ff
>
> +/* Exynos850 specific registers */
> +#define EXYNOS850_TMU_REG_AVG_CON              0x58

Exynos850 TRM says AVG_CONTROL offset is 0x38, and 0x58 is actually
for THRESHOLD0_TEMP_RISE3_2 register.

> +#define EXYNOS850_TMU_REG_CONTROL1             0x24
> +#define EXYNOS850_TMU_REG_COUNTER_VALUE0       0x30
> +#define EXYNOS850_TMU_REG_COUNTER_VALUE1       0x34
> +#define EXYNOS850_TMU_REG_CURRENT_TEMP1_0      0x40

In TRM, this register is called CURRENT_TEMP0_1. Maybe change 1_0 -> 0_1?

> +#define EXYNOS850_TMU_REG_THD_TEMP0_RISE       0x50
> +#define EXYNOS850_TMU_REG_THD_TEMP0_FALL       0x60
> +#define EXYNOS850_TMU_REG_TRIM0                        0x3C
> +
> +#define EXYNOS850_TMU_AVG_CON_SHIFT            18

Maybe rename it to something like EXYNOS850_TMU_T_AVG_MODE_SHIFT, to
avoid confusion with AVG_CONTROL register? That belongs to TRIMINFO2
register, if I understand it correctly, not to AVG_CONTROL.

> +#define EXYNOS850_TMU_AVG_MODE_MASK            0x7

I'd suggest to group all the definitions here as such:

#define REG1_OFFSET
#define REG1_FIELD1_OFFSET
#define REG1_FIELD2_OFFSET
...empty line...
#define REG2_OFFSET
#define REG2_FIELD1_OFFSET
#define REG2_FIELD2_OFFSET
...etc...

Or otherwise each shift/mask constant should contain its register name
as a prefix, to avoid confusion. But right now it's kinda hard to
understand what belongs to what :) But that's just a nitpick.

> +#define EXYNOS850_TMU_BGRI_TRIM_MASK           0xF

Suggest using GENMASK() macro whenever possible.

> +#define EXYNOS850_TMU_BGRI_TRIM_SHIFT          20
> +#define EXYNOS850_TMU_CLK_SENSE_ON_MASK                0xffff
> +#define EXYNOS850_TMU_CLK_SENSE_ON_SHIFT       16
> +#define EXYNOS850_TMU_DEM_ENABLE               1
> +#define EXYNOS850_TMU_DEM_SHIFT                        4

Instead of above two values, it could be just BIT(4) for
EXYNOS850_TMU_DEM_ENABLE?

> +#define EXYNOS850_TMU_EN_TEMP_SEN_OFF_MASK     0xffff
> +#define EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT    0
> +#define EXYNOS850_TMU_LPI_MODE_MASK            1
> +#define EXYNOS850_TMU_LPI_MODE_SHIFT           10
> +#define EXYNOS850_TMU_T_BUF_SLOPE_SEL_MASK     0xF
> +#define EXYNOS850_TMU_T_BUF_SLOPE_SEL_SHIFT    18
> +#define EXYNOS850_TMU_T_BUF_VREF_SEL_MASK      0x1F
> +#define EXYNOS850_TMU_T_BUF_VREF_SEL_SHIFT     18
> +#define EXYNOS850_TMU_TEM1051X_SENSE_VALUE     0x028A
> +#define EXYNOS850_TMU_TEM1456X_SENSE_VALUE     0x0A28

I'd pull those two values under shift/mask definitions. Also, please
use lowercase characters for hex values, here and in all other places.

> +#define EXYNOS850_TMU_TEMP_SHIFT               9
> +#define EXYNOS850_TMU_TRIMINFO_SHIFT           4
> +#define EXYNOS850_TMU_T_TRIM0_MASK             0xF
> +#define EXYNOS850_TMU_T_TRIM0_SHIFT            18
> +#define EXYNOS850_TMU_VBEI_TRIM_MASK           0xF
> +#define EXYNOS850_TMU_VBEI_TRIM_SHIFT          8
> +#define EXYNOS850_TMU_VREF_TRIM_MASK           0xF
> +#define EXYNOS850_TMU_VREF_TRIM_SHIFT          12
> +
>  #define EXYNOS_FIRST_POINT_TRIM                        25
>  #define EXYNOS_SECOND_POINT_TRIM               85
>
> @@ -133,6 +170,7 @@ enum soc_type {
>         SOC_ARCH_EXYNOS5420_TRIMINFO,
>         SOC_ARCH_EXYNOS5433,
>         SOC_ARCH_EXYNOS7,
> +       SOC_ARCH_EXYNOS850,
>  };
>
>  /**
> @@ -231,13 +269,16 @@ static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
>
>  static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
>  {
> -       u16 tmu_temp_mask =
> -               (data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_MASK
> -                                               : EXYNOS_TMU_TEMP_MASK;
> +       u16 tmu_temp_mask = (data->soc == SOC_ARCH_EXYNOS7 ||
> +                            data->soc == SOC_ARCH_EXYNOS850) ?
> +                                   EXYNOS7_TMU_TEMP_MASK :
> +                                   EXYNOS_TMU_TEMP_MASK;
> +       int tmu_85_shift = (data->soc == SOC_ARCH_EXYNOS850) ?
> +                                  EXYNOS850_TMU_TEMP_SHIFT :
> +                                  EXYNOS_TRIMINFO_85_SHIFT;

Something seems off to me here. How come the shift value for EXYNOS7
case is 8, but the mask is actually 9 bits long? Does it mean the
first error field is 8 bits long, and the second error field is 9 bits
long for EXYNOS7? I don't have the Exynos7 manual, so it's just a
hunch. But if it's true, maybe this shift value has to be added in
your [PATCH 2/6] to fix Exynos7 case?

Also, just an idea: those values (and other similar values) could be
pre-calculated somewhere during the probe, stored in some struct (e.g.
_variant or _chip) and then just used here. Stylistically, instead of
the ternary operator, maybe switch one would easier to read? Again,
those are very minor nitpicks.

>
>         data->temp_error1 = trim_info & tmu_temp_mask;
> -       data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
> -                               tmu_temp_mask);
> +       data->temp_error2 = ((trim_info >> tmu_85_shift) & tmu_temp_mask);
>

No need for the left-most and right-most brackets.

>         if (!data->temp_error1 ||
>             (data->min_efuse_value > data->temp_error1) ||
> @@ -245,9 +286,8 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
>                 data->temp_error1 = data->efuse_value & tmu_temp_mask;
>
>         if (!data->temp_error2)
> -               data->temp_error2 =
> -                       (data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) &
> -                       tmu_temp_mask;
> +               data->temp_error2 = (data->efuse_value >> tmu_85_shift) &
> +                                   tmu_temp_mask;
>  }
>
>  static int exynos_tmu_initialize(struct platform_device *pdev)
> @@ -588,6 +628,129 @@ static void exynos7_tmu_initialize(struct platform_device *pdev)
>         sanitize_temp_error(data, trim_info);
>  }
>
> +static void exynos850_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
> +{
> +       exynos_tmu_update_temp(data, EXYNOS850_TMU_REG_THD_TEMP0_FALL + 12, 0,
> +                              temp);
> +       exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
> +                             EXYNOS_TMU_INTEN_FALL0_SHIFT + 0, true);
> +}
> +
> +static void exynos850_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
> +{
> +       exynos_tmu_update_temp(data, EXYNOS850_TMU_REG_THD_TEMP0_RISE + 12, 16,
> +                              temp);
> +       exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
> +                             EXYNOS7_TMU_INTEN_RISE0_SHIFT + 1, true);
> +}
> +
> +static void exynos850_tmu_disable_low(struct exynos_tmu_data *data)
> +{
> +       exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
> +                             EXYNOS_TMU_INTEN_FALL0_SHIFT + 0, false);
> +}
> +
> +static void exynos850_tmu_disable_high(struct exynos_tmu_data *data)
> +{
> +       exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
> +                             EXYNOS7_TMU_INTEN_RISE0_SHIFT + 1, false);
> +}
> +
> +static void exynos850_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> +{
> +       exynos_tmu_update_temp(data, EXYNOS850_TMU_REG_THD_TEMP0_RISE + 0, 16,
> +                              temp);
> +       exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
> +                             EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
> +       exynos_tmu_update_bit(data, EXYNOS7_TMU_REG_INTEN,
> +                             EXYNOS7_TMU_INTEN_RISE0_SHIFT + 7, true);
> +}
> +
> +static void exynos850_tmu_initialize(struct platform_device *pdev)
> +{
> +       struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> +       int cal_type;

Please make it u32.

> +       unsigned int avg_mode, buf, bgri, vref, vbei;

Suggest renaming buf -> reg, and maybe make it u32.

> +
> +       buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> +       cal_type = (buf & EXYNOS5433_TRIMINFO_CALIB_SEL_MASK) >>
> +                  EXYNOS5433_TRIMINFO_CALIB_SEL_SHIFT;
> +       data->reference_voltage = (buf >> EXYNOS850_TMU_T_BUF_VREF_SEL_SHIFT) &
> +                                 EXYNOS850_TMU_T_BUF_VREF_SEL_MASK;
> +       buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
> +                   EXYNOS850_TMU_TRIMINFO_SHIFT);
> +       data->gain = (buf >> EXYNOS850_TMU_T_BUF_SLOPE_SEL_SHIFT) &
> +                    EXYNOS850_TMU_T_BUF_SLOPE_SEL_MASK;
> +       buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
> +                   2 * EXYNOS850_TMU_TRIMINFO_SHIFT);
> +       avg_mode = (buf >> EXYNOS850_TMU_AVG_CON_SHIFT) &
> +                  EXYNOS850_TMU_AVG_MODE_MASK;
> +       buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
> +                   3 * EXYNOS850_TMU_TRIMINFO_SHIFT);
> +       bgri = (buf >> EXYNOS850_TMU_T_TRIM0_SHIFT) &
> +              EXYNOS850_TMU_T_TRIM0_MASK;
> +       buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
> +                   4 * EXYNOS850_TMU_TRIMINFO_SHIFT);
> +       vref = (buf >> EXYNOS850_TMU_T_TRIM0_SHIFT) &
> +              EXYNOS850_TMU_T_TRIM0_MASK;
> +       buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO +
> +                   5 * EXYNOS850_TMU_TRIMINFO_SHIFT);

For cases like that, maybe introduce some macro like:

    #define EXYNOS850_TRIMINFO_OFFSET(n)    (EXYNOS_TMU_REG_TRIMINFO +
(n) * EXYNOS850_TMU_TRIMINFO_SHIFT)

and use it everywhere?

> +       vbei = (buf >> EXYNOS850_TMU_T_TRIM0_SHIFT) &
> +              EXYNOS850_TMU_T_TRIM0_MASK;
> +
> +       buf = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> +       sanitize_temp_error(data, buf);
> +
> +       switch (cal_type) {
> +       case EXYNOS5433_TRIMINFO_TWO_POINT_TRIMMING:
> +               data->cal_type = TYPE_TWO_POINT_TRIMMING;
> +               break;
> +       case EXYNOS5433_TRIMINFO_ONE_POINT_TRIMMING:

Add "fallthrough;" here? Or maybe just remove above line at all?

> +       default:
> +               data->cal_type = TYPE_ONE_POINT_TRIMMING;
> +               break;
> +       }
> +
> +       dev_info(&pdev->dev, "Calibration type is %d-point calibration\n",
> +                cal_type ? 2 : 1);
> +
> +       buf = readl(data->base + EXYNOS850_TMU_REG_AVG_CON);
> +       buf &= ~(EXYNOS850_TMU_AVG_MODE_MASK);

No need for brackets.

> +       buf &= ~(EXYNOS850_TMU_DEM_ENABLE << EXYNOS850_TMU_DEM_SHIFT);
> +       if (avg_mode) {
> +               buf |= avg_mode;
> +               buf |= (EXYNOS850_TMU_DEM_ENABLE << EXYNOS850_TMU_DEM_SHIFT);
> +       }
> +       writel(buf, data->base + EXYNOS850_TMU_REG_AVG_CON);
> +
> +       buf = readl(data->base + EXYNOS850_TMU_REG_COUNTER_VALUE0);
> +       buf &= ~(EXYNOS850_TMU_EN_TEMP_SEN_OFF_MASK
> +                << EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT);
> +       buf |= EXYNOS850_TMU_TEM1051X_SENSE_VALUE
> +              << EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT;
> +       writel(buf, data->base + EXYNOS850_TMU_REG_COUNTER_VALUE0);
> +
> +       buf = readl(data->base + EXYNOS850_TMU_REG_COUNTER_VALUE1);
> +       buf &= ~(EXYNOS850_TMU_CLK_SENSE_ON_MASK
> +                << EXYNOS850_TMU_CLK_SENSE_ON_SHIFT);
> +       buf |= EXYNOS850_TMU_TEM1051X_SENSE_VALUE
> +              << EXYNOS850_TMU_CLK_SENSE_ON_SHIFT;
> +       writel(buf, data->base + EXYNOS850_TMU_REG_COUNTER_VALUE1);
> +
> +       buf = readl(data->base + EXYNOS850_TMU_REG_TRIM0);
> +       buf &= ~(EXYNOS850_TMU_BGRI_TRIM_MASK << EXYNOS850_TMU_BGRI_TRIM_SHIFT);
> +       buf &= ~(EXYNOS850_TMU_VREF_TRIM_MASK << EXYNOS850_TMU_VREF_TRIM_SHIFT);
> +       buf &= ~(EXYNOS850_TMU_VBEI_TRIM_MASK << EXYNOS850_TMU_VBEI_TRIM_SHIFT);

Why not define this mask value like this instead:

    #define EXYNOS850_TMU_VBEI_TRIM_MASK        GENMASK(11,8)

And then you'll be able to do just:

    buf &= ~EXYNOS850_TMU_VBEI_TRIM_MASK;

The same goes for all similar cases.

> +       buf |= (bgri << EXYNOS850_TMU_BGRI_TRIM_SHIFT);
> +       buf |= (vref << EXYNOS850_TMU_VREF_TRIM_SHIFT);
> +       buf |= (vbei << EXYNOS850_TMU_VBEI_TRIM_SHIFT);

Brackets are not needed.

> +       writel(buf, data->base + EXYNOS850_TMU_REG_TRIM0);
> +
> +       buf = readl(data->base + EXYNOS850_TMU_REG_CONTROL1);
> +       buf &= ~(EXYNOS850_TMU_LPI_MODE_MASK << EXYNOS850_TMU_LPI_MODE_SHIFT);
> +       writel(buf, data->base + EXYNOS850_TMU_REG_CONTROL1);
> +}
> +
>  static void exynos4210_tmu_control(struct platform_device *pdev, bool on)
>  {
>         struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> @@ -679,7 +842,8 @@ static u32 get_emul_con_reg(struct exynos_tmu_data *data, unsigned int val,
>
>                 val &= ~(EXYNOS_EMUL_TIME_MASK << EXYNOS_EMUL_TIME_SHIFT);
>                 val |= (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT);
> -               if (data->soc == SOC_ARCH_EXYNOS7) {
> +               if (data->soc == SOC_ARCH_EXYNOS7 ||
> +                   data->soc == SOC_ARCH_EXYNOS850) {
>                         val &= ~(EXYNOS7_EMUL_DATA_MASK <<
>                                 EXYNOS7_EMUL_DATA_SHIFT);
>                         val |= (temp_to_code(data, temp) <<
> @@ -709,7 +873,8 @@ static void exynos4412_tmu_set_emulation(struct exynos_tmu_data *data,
>                 emul_con = EXYNOS5260_EMUL_CON;
>         else if (data->soc == SOC_ARCH_EXYNOS5433)
>                 emul_con = EXYNOS5433_TMU_EMUL_CON;
> -       else if (data->soc == SOC_ARCH_EXYNOS7)
> +       else if (data->soc == SOC_ARCH_EXYNOS7 ||
> +                data->soc == SOC_ARCH_EXYNOS850)
>                 emul_con = EXYNOS7_TMU_REG_EMUL_CON;
>         else
>                 emul_con = EXYNOS_EMUL_CON;
> @@ -766,6 +931,12 @@ static int exynos7_tmu_read(struct exynos_tmu_data *data)
>                 EXYNOS7_TMU_TEMP_MASK;
>  }
>
> +static int exynos850_tmu_read(struct exynos_tmu_data *data)
> +{
> +       return readw(data->base + EXYNOS850_TMU_REG_CURRENT_TEMP1_0) &
> +              EXYNOS7_TMU_TEMP_MASK;
> +}
> +
>  static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
>  {
>         struct exynos_tmu_data *data = id;
> @@ -794,7 +965,8 @@ static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
>         if (data->soc == SOC_ARCH_EXYNOS5260) {
>                 tmu_intstat = EXYNOS5260_TMU_REG_INTSTAT;
>                 tmu_intclear = EXYNOS5260_TMU_REG_INTCLEAR;
> -       } else if (data->soc == SOC_ARCH_EXYNOS7) {
> +       } else if (data->soc == SOC_ARCH_EXYNOS7 ||
> +                  data->soc == SOC_ARCH_EXYNOS850) {
>                 tmu_intstat = EXYNOS7_TMU_REG_INTPEND;
>                 tmu_intclear = EXYNOS7_TMU_REG_INTPEND;
>         } else if (data->soc == SOC_ARCH_EXYNOS5433) {
> @@ -845,6 +1017,9 @@ static const struct of_device_id exynos_tmu_match[] = {
>         }, {
>                 .compatible = "samsung,exynos7-tmu",
>                 .data = (const void *)SOC_ARCH_EXYNOS7,
> +       }, {
> +               .compatible = "samsung,exynos850-tmu",
> +               .data = (const void *)SOC_ARCH_EXYNOS850,
>         },
>         { },
>  };
> @@ -957,6 +1132,21 @@ static int exynos_map_dt_data(struct platform_device *pdev)
>                 data->min_efuse_value = 15;
>                 data->max_efuse_value = 100;
>                 break;
> +       case SOC_ARCH_EXYNOS850:
> +               data->tmu_set_low_temp = exynos850_tmu_set_low_temp;
> +               data->tmu_set_high_temp = exynos850_tmu_set_high_temp;
> +               data->tmu_disable_low = exynos850_tmu_disable_low;
> +               data->tmu_disable_high = exynos850_tmu_disable_high;
> +               data->tmu_set_crit_temp = exynos850_tmu_set_crit_temp;
> +               data->tmu_initialize = exynos850_tmu_initialize;
> +               data->tmu_control = exynos4210_tmu_control;
> +               data->tmu_read = exynos850_tmu_read;
> +               data->tmu_set_emulation = exynos4412_tmu_set_emulation;
> +               data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
> +               data->efuse_value = 55;
> +               data->min_efuse_value = 0;
> +               data->max_efuse_value = 511;
> +               break;
>         default:
>                 dev_err(&pdev->dev, "Platform not supported\n");
>                 return -EINVAL;
> @@ -1051,7 +1241,7 @@ static int exynos_tmu_probe(struct platform_device *pdev)
>                 return ret;
>
>         data->clk = devm_clk_get(dev, "tmu_apbif");
> -       if (IS_ERR(data->clk))
> +       if (IS_ERR(data->clk) && data->soc != SOC_ARCH_EXYNOS850)
>                 return dev_err_probe(dev, PTR_ERR(data->clk), "Failed to get clock\n");
>
>         data->clk_sec = devm_clk_get(dev, "tmu_triminfo_apbif");
> --
> 2.45.1
>
>

Re: [PATCH 6/6] dt-bindings: thermal: samsung,exynos: remove outdated information on trip point count

[Rob Herring]

On Fri, Jul 19, 2024 at 02:08:50PM +0200, Mateusz Majewski wrote:
> This is not true as of commit 5314b1543787 ("thermal/drivers/exynos: Use
> set_trips ops").
> 
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  .../bindings/thermal/samsung,exynos-thermal.yaml           | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> index 4363ee625339..5a82764a4dbb 100644
> --- a/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/samsung,exynos-thermal.yaml
> @@ -40,11 +40,8 @@ properties:
>    interrupts:
>      description: |
>        The Exynos TMU supports generating interrupts when reaching given
> -      temperature thresholds. Number of supported thermal trip points depends
> -      on the SoC (only first trip points defined in DT will be configured)::
> -       - most of SoC: 4
> -       - samsung,exynos5433-tmu: 8
> -       - samsung,exynos7-tmu: 8
> +      temperature thresholds. The trip points will be set dynamically in
> +      runtime, which means there is no limit on the number of trip points.

How can the hardware change how many trip points it supports?

>      maxItems: 1
>  
>    reg:
> -- 
> 2.45.1
> 

Re: [PATCH 0/6] Add initial Exynos 850 support to the thermal driver

[Mateusz Majewski]

Hi :)

> Thank you for the contribution! Did you by chance test it on any
> hardware, perhaps on E850-96 board? Just noticed there are no dts
> changes in this series (or as separate patches). If no -- I'll be glad
> to assist you on that, if you can share dts definitions for E850-96
> and the testing instructions with me.

I did test it on our copy of E850-96. I used this for testing:

diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
index f1c8b4613cbc..cffc173fd059 100644
--- a/arch/arm64/boot/dts/exynos/exynos850.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
@@ -617,6 +617,53 @@ sysreg_cmgp: syscon@11c20000 {
 			clocks = <&cmu_cmgp CLK_GOUT_SYSREG_CMGP_PCLK>;
 		};
 
+		tmuctrl_0: tmu@10070000{
+			compatible = "samsung,exynos850-tmu";
+			reg = <0x10070000 0x800>;
+			interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>;
+			#thermal-sensor-cells = <0>;
+		};
+
+		thermal-zones {
+			test_thermal: test-thermal{
+				polling-delay-passive = <0>;
+				polling-delay = <0>;
+				thermal-sensors = <&tmuctrl_0>;
+				trips {
+					test_40000: test-40000 {
+						temperature = <40000>;
+						hysteresis = <1000>;
+						type = "passive";
+					};
+					test_50000: test-50000 {
+						temperature = <50000>;
+						hysteresis = <1000>;
+						type = "passive";
+					};
+					test_60000: test-60000 {
+						temperature = <60000>;
+						hysteresis = <1000>;
+						type = "passive";
+					};
+					test_70000: test-70000 {
+						temperature = <70000>;
+						hysteresis = <1000>;
+						type = "passive";
+					};
+					test_80000: test-80000 {
+						temperature = <80000>;
+						hysteresis = <1000>;
+						type = "passive";
+					};
+					test_crit: test-crit {
+						temperature = <90000>;
+						hysteresis = <1000>;
+						type = "critical";
+					};
+				};
+			};
+		};
+
 		usbdrd: usb@13600000 {
 			compatible = "samsung,exynos850-dwusb3";
 			ranges = <0x0 0x13600000 0x10000>;

I did not post this because it would probably need some more thought,
and it probably wouldn't get merged until this series does anyway I
think? During testing I couldn't get readings higher than 35C, but the
values reacted to CPU load as expected. Also, Marek had physical access
to the board while I was testing it and has confirmed that the values
are realistic. Some more testing was done with some combination of
lowering the trip points, emul_temp and those temporary changes:

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index bd52663f1a5a..8db3f9039e7a 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -941,6 +941,7 @@ static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
 {
 	struct exynos_tmu_data *data = id;
 
+	pr_info("interrupt\n");
 	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
 
 	mutex_lock(&data->lock);
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 72b302bf914e..0864179526e2 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -232,13 +232,11 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
 
 	mutex_lock(&tz->lock);
 
-	if (!tz->ops.set_emul_temp)
+	if (!tz->ops.set_emul_temp) {
 		tz->emul_temperature = temperature;
-	else
-		ret = tz->ops.set_emul_temp(tz, temperature);
-
-	if (!ret)
 		__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
+	} else
+		ret = tz->ops.set_emul_temp(tz, temperature);
 
 	mutex_unlock(&tz->lock);
 

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Mateusz Majewski]

> Do you know what are the possible implications of not using ACPM? As I
> understand, ACPM is a Samsung's downstream framework which uses APM
> (Active Power Management) IP block internally to act as an IPC
> mechanism, which makes it possible to offload any PM related
> operations (which might get quite heavy, if we are to belive the TRM
> description of APM) from CPU to APM. I'm not against the direct
> registers access based implementation (in fact, I'm not sure how that
> APM/ACPM thing can be implemented in upstreamable way and if it's
> worth it at all). Just curious if we understand what we are
> potentially missing out, and if at some point we'll be forced to
> implement that ACPM thing anyway (for something else)?

Not sure honestly. The downstream v4.10 driver does many operations on
registers anyway...?

> Not sure if that's true, as already discussed in my comments for the
> previous patches. Looks like one clock is still needed, which is the
> PCLK bus clock (to interface registers) which might simultaneously act
> as an operating (functional) clock.

The code seems to be working correctly without this clock, both register
reads and writes. Maybe the support for extra sensors, which I couldn't
get to work, would require this clock?

> Exynos850 TRM says AVG_CONTROL offset is 0x38, and 0x58 is actually
> for THRESHOLD0_TEMP_RISE3_2 register.

Thank you so much! Will fix in v2. Though writing to the right place
doesn't seem to change much in practice, probably just means that the
correct mode is being used.

> Something seems off to me here. How come the shift value for EXYNOS7
> case is 8, but the mask is actually 9 bits long? Does it mean the
> first error field is 8 bits long, and the second error field is 9 bits
> long for EXYNOS7? I don't have the Exynos7 manual, so it's just a
> hunch. But if it's true, maybe this shift value has to be added in
> your [PATCH 2/6] to fix Exynos7 case?

I did not really want to mess with Exynos7 code, as we don't have an
Exynos7 board sadly. Honestly I feel like I should drop the 2/6 patch
completely and only modify the code to run on 850 correctly.

> Also, just an idea: those values (and other similar values) could be
> pre-calculated somewhere during the probe, stored in some struct (e.g.
> _variant or _chip) and then just used here.

sanitize_temp_error is only called one per probe and once per resume, so
probably little to gain?

Will also do all other.

Re: [PATCH 3/6] drivers/thermal/exynos: check IS_ERR(data->clk) consistently

[Mateusz Majewski]

> Also, if it's only optional for Exynos850 (and not optional for other
> chips), maybe it would be a good idea to use *_optional_* API only for
> Exynos850 case, so that the driver's behavior for those chips stays
> unchanged.

Probably should just set the clock to NULL in case of 850 then?

> Btw, from the downstream kernel code [1] I can see that the only TMU
> clock present in Exynos850 is
> GOUT_BLK_PERI_UID_BUSIF_TMU_IPCLKPORT_PCLK (which I was able to
> confirm in TRM). But it's not enabled in clk-exynos850.c driver right
> now. Do you want me by chance to send the patch adding it?

Would be very grateful :) If nothing else, it would be useful for
testing.

Re: [PATCH 6/6] dt-bindings: thermal: samsung,exynos: remove outdated information on trip point count

[Mateusz Majewski]

Hi :)

> > +      temperature thresholds. The trip points will be set dynamically in
> > +      runtime, which means there is no limit on the number of trip points.
> 
> How can the hardware change how many trip points it supports?

Would just removing the whole "The trip points..." sentence be ok? I see
how it is more confusing than helpful.

Re: [PATCH 0/6] Add initial Exynos 850 support to the thermal driver

[Sam Protsenko]

On Tue, Jul 23, 2024 at 9:16 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> Hi :)
>
> > Thank you for the contribution! Did you by chance test it on any
> > hardware, perhaps on E850-96 board? Just noticed there are no dts
> > changes in this series (or as separate patches). If no -- I'll be glad
> > to assist you on that, if you can share dts definitions for E850-96
> > and the testing instructions with me.
>
> I did test it on our copy of E850-96. I used this for testing:
>

Good to know, thanks for the detailed info, Mateusz! Just wanted to be
sure it was tested properly and my help is not needed. Btw, I'm
curious what is the reason for implementing TMU? Do you have some use
cases where it's needed?

Re: [PATCH 3/6] drivers/thermal/exynos: check IS_ERR(data->clk) consistently

[Sam Protsenko]

On Tue, Jul 23, 2024 at 9:17 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> > Also, if it's only optional for Exynos850 (and not optional for other
> > chips), maybe it would be a good idea to use *_optional_* API only for
> > Exynos850 case, so that the driver's behavior for those chips stays
> > unchanged.
>
> Probably should just set the clock to NULL in case of 850 then?
>

Ah, you are right, there is not much sense in doing both. I guess the
canonical way to do that (please check the drivers I referenced) -- is
not to check the chip, but just run devm_clk_get_optional(), which
sets the clock to NULL in case it's missing in dts. Less code this
way. And while at it, maybe consider reducing the code even more by
using devm_clk_get_optional_prepared().

> > Btw, from the downstream kernel code [1] I can see that the only TMU
> > clock present in Exynos850 is
> > GOUT_BLK_PERI_UID_BUSIF_TMU_IPCLKPORT_PCLK (which I was able to
> > confirm in TRM). But it's not enabled in clk-exynos850.c driver right
> > now. Do you want me by chance to send the patch adding it?
>
> Would be very grateful :) If nothing else, it would be useful for
> testing.

Cool, will try to do that soon!

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Sam Protsenko]

On Tue, Jul 23, 2024 at 9:17 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> > Do you know what are the possible implications of not using ACPM? As I
> > understand, ACPM is a Samsung's downstream framework which uses APM
> > (Active Power Management) IP block internally to act as an IPC
> > mechanism, which makes it possible to offload any PM related
> > operations (which might get quite heavy, if we are to belive the TRM
> > description of APM) from CPU to APM. I'm not against the direct
> > registers access based implementation (in fact, I'm not sure how that
> > APM/ACPM thing can be implemented in upstreamable way and if it's
> > worth it at all). Just curious if we understand what we are
> > potentially missing out, and if at some point we'll be forced to
> > implement that ACPM thing anyway (for something else)?
>
> Not sure honestly. The downstream v4.10 driver does many operations on
> registers anyway...?
>
> > Not sure if that's true, as already discussed in my comments for the
> > previous patches. Looks like one clock is still needed, which is the
> > PCLK bus clock (to interface registers) which might simultaneously act
> > as an operating (functional) clock.
>
> The code seems to be working correctly without this clock, both register
> reads and writes. Maybe the support for extra sensors, which I couldn't
> get to work, would require this clock?
>

Chances are that clock was enabled by the bootloader for us (or it's
just enabled by default) and it just keeps running. If that's so, I'd
say it must be described in dts and controlled by the driver. Because
otherwise it might get disabled at any point in future, e.g. kernel
may disable it during startup as an unused clock (when it's added to
the clock driver), etc. Let me enable that clock for you, and then you
can use /sys/kernel/debug/clk/ files to disable it manually and see if
it actually affects TMU driver.

> > Exynos850 TRM says AVG_CONTROL offset is 0x38, and 0x58 is actually
> > for THRESHOLD0_TEMP_RISE3_2 register.
>
> Thank you so much! Will fix in v2. Though writing to the right place
> doesn't seem to change much in practice, probably just means that the
> correct mode is being used.
>
> > Something seems off to me here. How come the shift value for EXYNOS7
> > case is 8, but the mask is actually 9 bits long? Does it mean the
> > first error field is 8 bits long, and the second error field is 9 bits
> > long for EXYNOS7? I don't have the Exynos7 manual, so it's just a
> > hunch. But if it's true, maybe this shift value has to be added in
> > your [PATCH 2/6] to fix Exynos7 case?
>
> I did not really want to mess with Exynos7 code, as we don't have an
> Exynos7 board sadly. Honestly I feel like I should drop the 2/6 patch
> completely and only modify the code to run on 850 correctly.
>

It feels like there is an error for Exynos7 case there. Take a look at
this commit:

    aef27b658b43 ("thermal: exynos: use sanitize_temp_error() in
exynos7_tmu_initialize()")

I think that commit just forgets to update the shift value for Exynos7
properly. This code:

    data->temp_error1 = trim_info & tmu_temp_mask;
    data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
                EXYNOS_TMU_TEMP_MASK);

in case of Exynos7 becomes:

    data->temp_error1 = trim_info & 0x1ff;    // mask = 9 bits
    data->temp_error2 = (trim_info >> 8) & 0xff;

it contradicts itself, because it takes 9 rightmost bits for error1,
and then uses 1 of those bits for error2 too. It's obvious that if 9
bits are already used for error1, then for error2 it has to be shifted
by 9 bits, not 8.

That's why I think your patch 2/6 is legit and useful on its own, and
it's actually a good catch on your part! But the shift value has to be
fixed as well (for Exynos7). It's not ideal you don't have the
hardware to test it, but it just screams *bug* to me :) Also, maybe we
can ask someone who has Exynos7 hardware to test it for us?

> > Also, just an idea: those values (and other similar values) could be
> > pre-calculated somewhere during the probe, stored in some struct (e.g.
> > _variant or _chip) and then just used here.
>
> sanitize_temp_error is only called one per probe and once per resume, so
> probably little to gain?
>

Sure, it was just a minor suggestion to make the code look more linear
so to speak. It can be totally skipped.

> Will also do all other.

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Sam Protsenko]

On Tue, Jul 23, 2024 at 10:23 AM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> On Tue, Jul 23, 2024 at 9:17 AM Mateusz Majewski
> <m.majewski2@samsung.com> wrote:
> >
> > > Do you know what are the possible implications of not using ACPM? As I
> > > understand, ACPM is a Samsung's downstream framework which uses APM
> > > (Active Power Management) IP block internally to act as an IPC
> > > mechanism, which makes it possible to offload any PM related
> > > operations (which might get quite heavy, if we are to belive the TRM
> > > description of APM) from CPU to APM. I'm not against the direct
> > > registers access based implementation (in fact, I'm not sure how that
> > > APM/ACPM thing can be implemented in upstreamable way and if it's
> > > worth it at all). Just curious if we understand what we are
> > > potentially missing out, and if at some point we'll be forced to
> > > implement that ACPM thing anyway (for something else)?
> >
> > Not sure honestly. The downstream v4.10 driver does many operations on
> > registers anyway...?
> >
> > > Not sure if that's true, as already discussed in my comments for the
> > > previous patches. Looks like one clock is still needed, which is the
> > > PCLK bus clock (to interface registers) which might simultaneously act
> > > as an operating (functional) clock.
> >
> > The code seems to be working correctly without this clock, both register
> > reads and writes. Maybe the support for extra sensors, which I couldn't
> > get to work, would require this clock?
> >
>
> Chances are that clock was enabled by the bootloader for us (or it's
> just enabled by default) and it just keeps running. If that's so, I'd
> say it must be described in dts and controlled by the driver. Because
> otherwise it might get disabled at any point in future, e.g. kernel
> may disable it during startup as an unused clock (when it's added to
> the clock driver), etc. Let me enable that clock for you, and then you
> can use /sys/kernel/debug/clk/ files to disable it manually and see if
> it actually affects TMU driver.
>

Yeah, that clock is definitely needed. Just submitted the series [1]
adding it, which makes the kernel stuck on startup when your series is
applied. To fix that I added next lines to the TMU node in dts:

8<--------------------------------------------------------------------------->8
     tmuctrl_0: tmu@10070000 {
         compatible = "samsung,exynos850-tmu";
         reg = <0x10070000 0x800>;
         interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>;
+        clock-names = "tmu_apbif";
+        clocks = <&cmu_peri CLK_GOUT_BUSIF_TMU_PCLK>;
         #thermal-sensor-cells = <0>;
    };
8<--------------------------------------------------------------------------->8

Please rework your patches to account for that required clock. Alas
the TMU dts changes can't be submitted until my series [1] is applied.
But you can still apply my series locally, and I think the driver and
bindings changes don't depend on that clock, so it should be ok to
send those

Thanks!

[1] https://lore.kernel.org/linux-samsung-soc/20240723163311.28654-2-semen.protsenko@linaro.org/T/#mf28e4aab0111b95479ef632bc1979dff93d28cc7

> > > Exynos850 TRM says AVG_CONTROL offset is 0x38, and 0x58 is actually
> > > for THRESHOLD0_TEMP_RISE3_2 register.
> >
> > Thank you so much! Will fix in v2. Though writing to the right place
> > doesn't seem to change much in practice, probably just means that the
> > correct mode is being used.
> >
> > > Something seems off to me here. How come the shift value for EXYNOS7
> > > case is 8, but the mask is actually 9 bits long? Does it mean the
> > > first error field is 8 bits long, and the second error field is 9 bits
> > > long for EXYNOS7? I don't have the Exynos7 manual, so it's just a
> > > hunch. But if it's true, maybe this shift value has to be added in
> > > your [PATCH 2/6] to fix Exynos7 case?
> >
> > I did not really want to mess with Exynos7 code, as we don't have an
> > Exynos7 board sadly. Honestly I feel like I should drop the 2/6 patch
> > completely and only modify the code to run on 850 correctly.
> >
>
> It feels like there is an error for Exynos7 case there. Take a look at
> this commit:
>
>     aef27b658b43 ("thermal: exynos: use sanitize_temp_error() in
> exynos7_tmu_initialize()")
>
> I think that commit just forgets to update the shift value for Exynos7
> properly. This code:
>
>     data->temp_error1 = trim_info & tmu_temp_mask;
>     data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
>                 EXYNOS_TMU_TEMP_MASK);
>
> in case of Exynos7 becomes:
>
>     data->temp_error1 = trim_info & 0x1ff;    // mask = 9 bits
>     data->temp_error2 = (trim_info >> 8) & 0xff;
>
> it contradicts itself, because it takes 9 rightmost bits for error1,
> and then uses 1 of those bits for error2 too. It's obvious that if 9
> bits are already used for error1, then for error2 it has to be shifted
> by 9 bits, not 8.
>
> That's why I think your patch 2/6 is legit and useful on its own, and
> it's actually a good catch on your part! But the shift value has to be
> fixed as well (for Exynos7). It's not ideal you don't have the
> hardware to test it, but it just screams *bug* to me :) Also, maybe we
> can ask someone who has Exynos7 hardware to test it for us?
>
> > > Also, just an idea: those values (and other similar values) could be
> > > pre-calculated somewhere during the probe, stored in some struct (e.g.
> > > _variant or _chip) and then just used here.
> >
> > sanitize_temp_error is only called one per probe and once per resume, so
> > probably little to gain?
> >
>
> Sure, it was just a minor suggestion to make the code look more linear
> so to speak. It can be totally skipped.
>
> > Will also do all other.

Re: [PATCH 6/6] dt-bindings: thermal: samsung,exynos: remove outdated information on trip point count

[Rob Herring]

On Tue, Jul 23, 2024 at 04:17:14PM +0200, Mateusz Majewski wrote:
> Hi :)
> 
> > > +      temperature thresholds. The trip points will be set dynamically in
> > > +      runtime, which means there is no limit on the number of trip points.
> > 
> > How can the hardware change how many trip points it supports?
> 
> Would just removing the whole "The trip points..." sentence be ok? I see
> how it is more confusing than helpful.

If the old text had nothing to do with the h/w, then I suppose so. I 
would have assumed the h/w supports some number of thresholds causing 
some action whether an interrupt or some sort of shutdown.

Rob

Re: [PATCH 1/6] drivers/thermal/exynos: use DEFINE_SIMPLE_DEV_PM_OPS

[Anand Moon]

Hi Mateusz,

On Fri, 19 Jul 2024 at 17:40, Mateusz Majewski <m.majewski2@samsung.com> wrote:
>
> SIMPLE_DEV_PM_OPS is deprecated, as noted next to its definition.
>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 96cffb2c44ba..9b7ca93a72f1 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -1150,8 +1150,8 @@ static int exynos_tmu_resume(struct device *dev)
>         return 0;
>  }
>
> -static SIMPLE_DEV_PM_OPS(exynos_tmu_pm,
> -                        exynos_tmu_suspend, exynos_tmu_resume);
> +static DEFINE_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

You can drop the CONFIG_PM_SLEEP guard and use pm_sleep_ptr macro for
exynos_tmu_pm.

Thanks
-Anand

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Mateusz Majewski]

> It feels like there is an error for Exynos7 case there. Take a look at
> this commit:
> 
>     aef27b658b43 ("thermal: exynos: use sanitize_temp_error() in
> exynos7_tmu_initialize()")
> 
> I think that commit just forgets to update the shift value for Exynos7
> properly. This code:
> 
>     data->temp_error1 = trim_info & tmu_temp_mask;
>     data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
>                 EXYNOS_TMU_TEMP_MASK);
> 
> in case of Exynos7 becomes:
> 
>     data->temp_error1 = trim_info & 0x1ff;    // mask = 9 bits
>     data->temp_error2 = (trim_info >> 8) & 0xff;
> 
> it contradicts itself, because it takes 9 rightmost bits for error1,
> and then uses 1 of those bits for error2 too. It's obvious that if 9
> bits are already used for error1, then for error2 it has to be shifted
> by 9 bits, not 8.
> 
> That's why I think your patch 2/6 is legit and useful on its own, and
> it's actually a good catch on your part! But the shift value has to be
> fixed as well (for Exynos7). It's not ideal you don't have the
> hardware to test it, but it just screams *bug* to me :) Also, maybe we
> can ask someone who has Exynos7 hardware to test it for us?

I thought about it for a bit and finally realized that Exynos7 only
supports one point trimming. That is why in that commit, the original
exynos7_tmu_initialize did not do anything with temp_error2. So
temp_error2 will never be used in Exynos7. The real "fix" I guess is to
only calculate temp_error2 if two point trimming is used, which is
possible with a very small reordering of exynos7_tmu_initialize. But
then the shift value will never be reachable in Exynos7 anyway. What do
you think about this? I feel like it's worth it to change the shift
value just because the code is a bit confusing anyway.

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Mateusz Majewski]

> I'd suggest to group all the definitions here as such:
> 
> #define REG1_OFFSET
> #define REG1_FIELD1_OFFSET
> #define REG1_FIELD2_OFFSET
> ...empty line...
> #define REG2_OFFSET
> #define REG2_FIELD1_OFFSET
> #define REG2_FIELD2_OFFSET
> ...etc...
> 
> Or otherwise each shift/mask constant should contain its register name
> as a prefix, to avoid confusion. But right now it's kinda hard to
> understand what belongs to what :) But that's just a nitpick.

I came up with this:

/* Exynos850 specific registers */
#define EXYNOS850_TMU_REG_CURRENT_TEMP0_1	0x40
#define EXYNOS850_TMU_REG_THD_TEMP0_RISE	0x50
#define EXYNOS850_TMU_REG_THD_TEMP0_FALL	0x60
#define EXYNOS850_TMU_TEMP_SHIFT		9

#define EXYNOS850_TMU_TRIMINFO_SHIFT		4
#define EXYNOS850_TMU_TRIMINFO_OFFSET(n) \
	(EXYNOS_TMU_REG_TRIMINFO + (n) * EXYNOS850_TMU_TRIMINFO_SHIFT)
#define EXYNOS850_TMU_T_TRIM0_SHIFT		18

#define EXYNOS850_TMU_REG_CONTROL1		0x24
#define EXYNOS850_TMU_LPI_MODE_MASK		1
#define EXYNOS850_TMU_LPI_MODE_SHIFT		10

#define EXYNOS850_TMU_REG_COUNTER_VALUE0	0x30
#define EXYNOS850_TMU_EN_TEMP_SEN_OFF_MASK	0xffff
#define EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT	0

#define EXYNOS850_TMU_REG_COUNTER_VALUE1	0x34
#define EXYNOS850_TMU_CLK_SENSE_ON_MASK		0xffff
#define EXYNOS850_TMU_CLK_SENSE_ON_SHIFT	16

#define EXYNOS850_TMU_REG_AVG_CON		0x38
#define EXYNOS850_TMU_AVG_MODE_MASK		0x7
#define EXYNOS850_TMU_DEM_ENABLE		BIT(4)

#define EXYNOS850_TMU_REG_TRIM0			0x3c
#define EXYNOS850_TMU_TRIM0_MASK		0xf
#define EXYNOS850_TMU_VBEI_TRIM_SHIFT		8
#define EXYNOS850_TMU_VREF_TRIM_SHIFT		12
#define EXYNOS850_TMU_BGRI_TRIM_SHIFT		20

#define EXYNOS850_TMU_TEM1051X_SENSE_VALUE	0x028a
#define EXYNOS850_TMU_TEM1456X_SENSE_VALUE	0x0a28

This also omits some definitions that were in v1, as they had the same
value and they were the same thing anyway. For instance, I dropped
EXYNOS850_TMU_T_BUF_VREF_SEL_MASK in favor of
EXYNOS_TMU_REF_VOLTAGE_MASK, and have a single EXYNOS850_TMU_TRIM0_MASK
instead of EXYNOS850_TMU_BGRI_TRIM_MASK, EXYNOS850_TMU_VREF_TRIM_MASK,
EXYNOS850_TMU_VBEI_TRIM_MASK and EXYNOS850_TMU_T_TRIM0_MASK. Also,

> Suggest using GENMASK() macro whenever possible.

This would make me have a separate mask for each of these again. Maybe
if this driver gets refactored in the future to use u32_get_bits() and
so on this would make more sense?

Re: [PATCH 6/6] dt-bindings: thermal: samsung,exynos: remove outdated information on trip point count

[Mateusz Majewski]

> If the old text had nothing to do with the h/w, then I suppose so. I 
> would have assumed the h/w supports some number of thresholds causing 
> some action whether an interrupt or some sort of shutdown.

That is true, but after the mentioned change only 3 are used at the
time: one lower than current temperature, one higher than current
temperature, and one for the critical temperature. So, from the
perspective of somebody writing the devicetree source file this limit is
probably irrelevant?

Re: [PATCH 0/6] Add initial Exynos 850 support to the thermal driver

[Mateusz Majewski]

> Btw, I'm
> curious what is the reason for implementing TMU? Do you have some use
> cases where it's needed?

Not really AFAIK? Mostly because we have the hardware, are familiar with
this driver, and have some time to do this :)

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Sam Protsenko]

On Wed, Jul 24, 2024 at 10:30 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> > It feels like there is an error for Exynos7 case there. Take a look at
> > this commit:
> >
> >     aef27b658b43 ("thermal: exynos: use sanitize_temp_error() in
> > exynos7_tmu_initialize()")
> >
> > I think that commit just forgets to update the shift value for Exynos7
> > properly. This code:
> >
> >     data->temp_error1 = trim_info & tmu_temp_mask;
> >     data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
> >                 EXYNOS_TMU_TEMP_MASK);
> >
> > in case of Exynos7 becomes:
> >
> >     data->temp_error1 = trim_info & 0x1ff;    // mask = 9 bits
> >     data->temp_error2 = (trim_info >> 8) & 0xff;
> >
> > it contradicts itself, because it takes 9 rightmost bits for error1,
> > and then uses 1 of those bits for error2 too. It's obvious that if 9
> > bits are already used for error1, then for error2 it has to be shifted
> > by 9 bits, not 8.
> >
> > That's why I think your patch 2/6 is legit and useful on its own, and
> > it's actually a good catch on your part! But the shift value has to be
> > fixed as well (for Exynos7). It's not ideal you don't have the
> > hardware to test it, but it just screams *bug* to me :) Also, maybe we
> > can ask someone who has Exynos7 hardware to test it for us?
>
> I thought about it for a bit and finally realized that Exynos7 only
> supports one point trimming. That is why in that commit, the original
> exynos7_tmu_initialize did not do anything with temp_error2. So
> temp_error2 will never be used in Exynos7. The real "fix" I guess is to
> only calculate temp_error2 if two point trimming is used, which is
> possible with a very small reordering of exynos7_tmu_initialize. But
> then the shift value will never be reachable in Exynos7 anyway. What do
> you think about this? I feel like it's worth it to change the shift
> value just because the code is a bit confusing anyway.

Good catch! Yes, makes total sense to me. I think it's like you said,
would be better to do both:

1. For 1-point trimming architectures: don't calculate error2, to
avoid confusion
2. For 9-bit temp length architectures: always set the shift variable
to 9, again, to avoid confusion and possible bugs

As I see it, the actual reason why that confusion happened in the
first place, is that the data is not really separated from the code in
this driver. Right now exynos_tmu_match[] table contains SOC_ARCH_*
constants for each compatible, and actual features for each platform
are devised in run-time (e.g. in exynos_map_data_data() switch, and
all other places where data->soc is checked). Because of all those
"ifs" the code looks very non-linear, hard to read, bug prone, and may
even reduce the performance. A better approach would be to extract all
possible data into some const structure containing all features for
each platform, and assign that const structure to corresponding .data
for each compatible. Maybe also add .temp_length field containing 8 or
9 accordingly. Having all that done, all the platform features will be
known at compile time and collected in one place, simplifying the
actual driver's code (most of all those ifs and switches will go
away). One example of such approach is drivers/watchdog/s3c2410_wdt.c.
This way the sanitize function could look something like this:

8<------------------------------------------------------------------------------->8
static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
{
    data->temp_error1 = trim_info & data->tmu_temp_mask;
    if (!data->temp_error1 ||
         data->temp_error1 < data->min_efuse_value  ||
         data->temp_error1 > data->max_efuse_value)
             data->temp_error1 = data->efuse_value & data->tmu_temp_mask;

    if (data->cal_type == ONE_POINT_TRIMMING)
        return;

    data->temp_error2 = (trim_info >> data->tmu_temp_shift) &
data->tmu_temp_mask;
    if (!data->temp_error2)
        data->temp_error2 = (data->efuse_value >>
data->tmu_temp_shift) & data->tmu_temp_mask;
}
8<------------------------------------------------------------------------------->8

So this data driven approach doesn't leave much space for mistakes.
Anyways, I'm not asking you to do such rework, it's just my
understanding on the cause of such issues.

Thanks!

Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Sam Protsenko]

On Wed, Jul 24, 2024 at 10:31 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> > I'd suggest to group all the definitions here as such:
> >
> > #define REG1_OFFSET
> > #define REG1_FIELD1_OFFSET
> > #define REG1_FIELD2_OFFSET
> > ...empty line...
> > #define REG2_OFFSET
> > #define REG2_FIELD1_OFFSET
> > #define REG2_FIELD2_OFFSET
> > ...etc...
> >
> > Or otherwise each shift/mask constant should contain its register name
> > as a prefix, to avoid confusion. But right now it's kinda hard to
> > understand what belongs to what :) But that's just a nitpick.
>
> I came up with this:
>
> /* Exynos850 specific registers */
> #define EXYNOS850_TMU_REG_CURRENT_TEMP0_1       0x40
> #define EXYNOS850_TMU_REG_THD_TEMP0_RISE        0x50
> #define EXYNOS850_TMU_REG_THD_TEMP0_FALL        0x60
> #define EXYNOS850_TMU_TEMP_SHIFT                9
>
> #define EXYNOS850_TMU_TRIMINFO_SHIFT            4
> #define EXYNOS850_TMU_TRIMINFO_OFFSET(n) \
>         (EXYNOS_TMU_REG_TRIMINFO + (n) * EXYNOS850_TMU_TRIMINFO_SHIFT)
> #define EXYNOS850_TMU_T_TRIM0_SHIFT             18
>
> #define EXYNOS850_TMU_REG_CONTROL1              0x24
> #define EXYNOS850_TMU_LPI_MODE_MASK             1
> #define EXYNOS850_TMU_LPI_MODE_SHIFT            10
>
> #define EXYNOS850_TMU_REG_COUNTER_VALUE0        0x30
> #define EXYNOS850_TMU_EN_TEMP_SEN_OFF_MASK      0xffff
> #define EXYNOS850_TMU_EN_TEMP_SEN_OFF_SHIFT     0
>
> #define EXYNOS850_TMU_REG_COUNTER_VALUE1        0x34
> #define EXYNOS850_TMU_CLK_SENSE_ON_MASK         0xffff
> #define EXYNOS850_TMU_CLK_SENSE_ON_SHIFT        16
>
> #define EXYNOS850_TMU_REG_AVG_CON               0x38
> #define EXYNOS850_TMU_AVG_MODE_MASK             0x7
> #define EXYNOS850_TMU_DEM_ENABLE                BIT(4)
>
> #define EXYNOS850_TMU_REG_TRIM0                 0x3c
> #define EXYNOS850_TMU_TRIM0_MASK                0xf
> #define EXYNOS850_TMU_VBEI_TRIM_SHIFT           8
> #define EXYNOS850_TMU_VREF_TRIM_SHIFT           12
> #define EXYNOS850_TMU_BGRI_TRIM_SHIFT           20
>
> #define EXYNOS850_TMU_TEM1051X_SENSE_VALUE      0x028a
> #define EXYNOS850_TMU_TEM1456X_SENSE_VALUE      0x0a28
>

Looks better, thanks!

> This also omits some definitions that were in v1, as they had the same
> value and they were the same thing anyway. For instance, I dropped
> EXYNOS850_TMU_T_BUF_VREF_SEL_MASK in favor of
> EXYNOS_TMU_REF_VOLTAGE_MASK, and have a single EXYNOS850_TMU_TRIM0_MASK
> instead of EXYNOS850_TMU_BGRI_TRIM_MASK, EXYNOS850_TMU_VREF_TRIM_MASK,
> EXYNOS850_TMU_VBEI_TRIM_MASK and EXYNOS850_TMU_T_TRIM0_MASK. Also,
>
> > Suggest using GENMASK() macro whenever possible.
>
> This would make me have a separate mask for each of these again. Maybe
> if this driver gets refactored in the future to use u32_get_bits() and
> so on this would make more sense?

Sure, that was just a suggestion, don't have a strong opinion on that
one. If you don't like it, feel free to skip it for now.

Re: [PATCH 0/6] Add initial Exynos 850 support to the thermal driver

[Sam Protsenko]

On Wed, Jul 24, 2024 at 10:31 AM Mateusz Majewski
<m.majewski2@samsung.com> wrote:
>
> > Btw, I'm
> > curious what is the reason for implementing TMU? Do you have some use
> > cases where it's needed?
>
> Not really AFAIK? Mostly because we have the hardware, are familiar with
> this driver, and have some time to do this :)

No complaints from my side! Wish more folks were working on this
platform :) Please let me know if I can assist you in any way. Hope in
v2 you can account for the TMU clock I enabled in [1], and test it.

Thanks!

[1] https://lore.kernel.org/linux-samsung-soc/20240723163311.28654-1-semen.protsenko@linaro.org/