The Exynos tmu driver's private data structure has been extended
to support the exynosautov920 hardware, which requires per-sensor interrupt
enablement and multiple-zone handling:
- Add 'slope_comp' : compensation parameter below 25 degrees.
- Add 'calib_temp' : stores the fused calibaration temperature.
- Add 'sensor_count' : reflects the maximum sensor numbers.
- Rename 'tzd' -> 'tzd_array' to register multiple thermal zones.
Since splitting this patch causes runtime errors during temperature
emulation or problems where the read temperature feature fails to
retrieve values, I have submitted it as a single commit. To add support
for the exynosautov920 to the exisiting TMU interface, the following
changes are included:
1. Simplify "temp_to_code" and "code_to_temp" to one computation path
by normalizing calib_temp.
2. Loop over 'sensor_count' in critical-point setup.
3. Introduce 'update_con_reg' for exynosautov920 control-register updates.
4. Add exynosautov920-specific branch in 'exynos_tmu_update_temp' function.
5. Skip high & low temperature threshold setup in exynosautov920.
6. Enable interrupts via sensor_count in exynosautov920.
7. Initialize all new members during 'exynosautov920_tmu_initialize'.
8. Clear IRQs by iterating the sensor_count in exynosautov920.
9. Register each zone with 'devm_thermal_of_zone_register()'
based on 'sensor_count'.
Reviewed-by: Henrik Grimler <henrik@grimler.se>
Signed-off-by: Shin Son <shin.son@samsung.com>
---
drivers/thermal/samsung/exynos_tmu.c | 322 ++++++++++++++++++++++++---
1 file changed, 285 insertions(+), 37 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 47a99b3c5395..8fa188928b79 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -121,8 +121,51 @@
#define EXYNOS_NOISE_CANCEL_MODE 4
+/* ExynosAutov920 specific registers */
+#define EXYNOSAUTOV920_SLOPE_COMP 25
+#define EXYNOSAUTOV920_SLOPE_COMP_MASK 0xf
+#define EXYNOSAUTOV920_CALIB_SEL_TEMP 30
+#define EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK 0x2
+
+#define EXYNOSAUTOV920_SENSOR0_TRIM_INFO 0x10
+#define EXYNOSAUTOV920_TRIM_MASK 0x1ff
+#define EXYNOSAUTOV920_TRIMINFO_25_SHIFT 0
+#define EXYNOSAUTOV920_TRIMINFO_85_SHIFT 9
+
+#define EXYNOSAUTOV920_TMU_REG_TRIMINFO2 0x04
+
+#define EXYNOSAUTOV920_TMU_REG_THRESHOLD(p) (((p)) * 0x50 + 0x00d0)
+#define EXYNOSAUTOV920_TMU_REG_INTEN(p) (((p)) * 0x50 + 0x00f0)
+#define EXYNOSAUTOV920_TMU_REG_INT_PEND(p) (((p)) * 0x50 + 0x00f8)
+
+#define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0 0x084
+#define EXYNOSAUTOV920_TMU_REG_EMUL_CON 0x0b0
+
+#define EXYNOSAUTOV920_TMU_REG_CONTROL 0x50
+#define EXYNOSAUTOV920_TMU_REG_CONTROL1 0x54
+#define EXYNOSAUTOV920_TMU_REG_AVG_CONTROL 0x58
+#define EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL 0x70
+#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0 0x74
+#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1 0x78
+
+#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT 8
+#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK 0x1f
+#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT 3
+#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK 0xf
+#define EXYNOSAUTOV920_TMU_NUM_PROBE_MASK 0xf
+#define EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT 16
+#define EXYNOSAUTOV920_TMU_LPI_MODE_MASK 1
+#define EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT 10
+
+#define EXYNOSAUTOV920_TMU_AVG_CON_UPDATE 0x0008011a
+#define EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE 0x030003c0
+#define EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE 0x03c0004d
+
#define MCELSIUS 1000
+#define EXYNOS_DEFAULT_SENSOR_COUNT 1
+#define EXYNOS_MAX_SENSOR_COUNT 15
+
enum soc_type {
SOC_ARCH_EXYNOS3250 = 1,
SOC_ARCH_EXYNOS4210,
@@ -133,6 +176,7 @@ enum soc_type {
SOC_ARCH_EXYNOS5420_TRIMINFO,
SOC_ARCH_EXYNOS5433,
SOC_ARCH_EXYNOS7,
+ SOC_ARCH_EXYNOSAUTOV920,
};
/**
@@ -150,6 +194,8 @@ enum soc_type {
* @efuse_value: SoC defined fuse value
* @min_efuse_value: minimum valid trimming data
* @max_efuse_value: maximum valid trimming data
+ * @slope_comp: allocated value of the slope compensation.
+ * @calib_temp: calibration temperature of the TMU.
* @temp_error1: fused value of the first point trim.
* @temp_error2: fused value of the second point trim.
* @gain: gain of amplifier in the positive-TC generator block
@@ -157,7 +203,8 @@ enum soc_type {
* @reference_voltage: reference voltage of amplifier
* in the positive-TC generator block
* 0 < reference_voltage <= 31
- * @tzd: pointer to thermal_zone_device structure
+ * @sensor_count: The maximum number of the sensors
+ * @tzd_array: pointer array of thermal_zone_device structure
* @enabled: current status of TMU device
* @tmu_set_low_temp: SoC specific method to set trip (falling threshold)
* @tmu_set_high_temp: SoC specific method to set trip (rising threshold)
@@ -174,6 +221,7 @@ struct exynos_tmu_data {
void __iomem *base;
void __iomem *base_second;
int irq;
+ int sensor_count;
enum soc_type soc;
struct mutex lock;
struct clk *clk, *clk_sec, *sclk;
@@ -181,10 +229,12 @@ struct exynos_tmu_data {
u32 efuse_value;
u32 min_efuse_value;
u32 max_efuse_value;
+ u16 slope_comp;
+ u16 calib_temp;
u16 temp_error1, temp_error2;
u8 gain;
u8 reference_voltage;
- struct thermal_zone_device *tzd;
+ struct thermal_zone_device *tzd_array[EXYNOS_MAX_SENSOR_COUNT];
bool enabled;
void (*tmu_set_low_temp)(struct exynos_tmu_data *data, u8 temp);
@@ -205,13 +255,20 @@ struct exynos_tmu_data {
*/
static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
{
+ s32 temp_diff, code;
+
if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
return temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
- return (temp - EXYNOS_FIRST_POINT_TRIM) *
- (data->temp_error2 - data->temp_error1) /
- (EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
- data->temp_error1;
+ temp_diff = temp - EXYNOS_FIRST_POINT_TRIM;
+
+ code = temp_diff * (data->temp_error2 - data->temp_error1) * MCELSIUS /
+ (data->calib_temp - EXYNOS_FIRST_POINT_TRIM);
+
+ if (data->soc == SOC_ARCH_EXYNOSAUTOV920 && temp_diff < 0)
+ code = code * (57 + data->slope_comp) / 65;
+
+ return code / MCELSIUS + data->temp_error1;
}
/*
@@ -220,13 +277,20 @@ static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
*/
static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
{
+ s32 code_diff, temp;
+
if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
return temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
- return (temp_code - data->temp_error1) *
- (EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
- (data->temp_error2 - data->temp_error1) +
- EXYNOS_FIRST_POINT_TRIM;
+ code_diff = temp_code - data->temp_error1;
+
+ temp = code_diff * (data->calib_temp - EXYNOS_FIRST_POINT_TRIM) * MCELSIUS /
+ (data->temp_error2 - data->temp_error1);
+
+ if (data->soc == SOC_ARCH_EXYNOSAUTOV920 && code_diff < 0)
+ temp = temp * 65 / (57 + data->slope_comp);
+
+ return temp / MCELSIUS + EXYNOS_FIRST_POINT_TRIM;
}
static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
@@ -262,6 +326,9 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
clk_enable(data->clk_sec);
status = readb(data->base + EXYNOS_TMU_REG_STATUS);
+ if (data->soc == SOC_ARCH_EXYNOSAUTOV920)
+ status = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
+
if (!status) {
ret = -EBUSY;
} else {
@@ -280,27 +347,34 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
static int exynos_thermal_zone_configure(struct platform_device *pdev)
{
struct exynos_tmu_data *data = platform_get_drvdata(pdev);
- struct thermal_zone_device *tzd = data->tzd;
- int ret, temp;
+ struct thermal_zone_device *tzd;
+ int ret, temp, idx;
- ret = thermal_zone_get_crit_temp(tzd, &temp);
- if (ret) {
- /* FIXME: Remove this special case */
- if (data->soc == SOC_ARCH_EXYNOS5433)
- return 0;
+ for (idx = 0; idx < data->sensor_count; idx++) {
+ tzd = data->tzd_array[idx];
- dev_err(&pdev->dev,
- "No CRITICAL trip point defined in device tree!\n");
- return ret;
- }
+ if (!tzd)
+ continue;
- mutex_lock(&data->lock);
- clk_enable(data->clk);
+ ret = thermal_zone_get_crit_temp(tzd, &temp);
+ if (ret) {
+ /* FIXME: Remove this special case */
+ if (data->soc == SOC_ARCH_EXYNOS5433)
+ return 0;
- data->tmu_set_crit_temp(data, temp / MCELSIUS);
+ dev_err(&pdev->dev,
+ "No CRITICAL trip point defined in device tree!\n");
+ return ret;
+ }
- clk_disable(data->clk);
- mutex_unlock(&data->lock);
+ mutex_lock(&data->lock);
+ clk_enable(data->clk);
+
+ data->tmu_set_crit_temp(data, temp / MCELSIUS);
+
+ clk_disable(data->clk);
+ mutex_unlock(&data->lock);
+ }
return 0;
}
@@ -323,6 +397,37 @@ static u32 get_con_reg(struct exynos_tmu_data *data, u32 con)
return con;
}
+static void update_con_reg(struct exynos_tmu_data *data)
+{
+ u32 val, t_buf_vref_sel, t_buf_slope_sel;
+
+ val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
+ t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
+ & EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
+ t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
+ & EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
+
+ val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
+ val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
+ val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
+ val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
+ val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
+ writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
+
+ val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
+ val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
+ val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK << EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
+ val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
+ writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
+
+ writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
+ writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base + EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
+ writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
+ data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
+ writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
+ data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
+}
+
static void exynos_tmu_control(struct platform_device *pdev, bool on)
{
struct exynos_tmu_data *data = platform_get_drvdata(pdev);
@@ -354,9 +459,8 @@ static void exynos_tmu_update_temp(struct exynos_tmu_data *data, int reg_off,
u16 tmu_temp_mask;
u32 th;
- tmu_temp_mask =
- (data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_MASK
- : EXYNOS_TMU_TEMP_MASK;
+ tmu_temp_mask = (data->soc == SOC_ARCH_EXYNOS7 || data->soc == SOC_ARCH_EXYNOSAUTOV920)
+ ? EXYNOS7_TMU_TEMP_MASK : EXYNOS_TMU_TEMP_MASK;
th = readl(data->base + reg_off);
th &= ~(tmu_temp_mask << bit_off);
@@ -582,6 +686,68 @@ static void exynos7_tmu_initialize(struct platform_device *pdev)
sanitize_temp_error(data, trim_info);
}
+static void exynosautov920_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
+{
+ /*
+ * Failing thresholds are not supported on Exynosautov920.
+ * We use polling instead.
+ */
+}
+
+static void exynosautov920_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
+{
+ /*
+ * Rising thresholds are not supported on Exynosautov920.
+ * We use polling instead.
+ */
+}
+
+static void exynosautov920_tmu_disable_low(struct exynos_tmu_data *data)
+{
+ /* Again, this is handled by polling. */
+}
+
+static void exynosautov920_tmu_disable_high(struct exynos_tmu_data *data)
+{
+ /* Again, this is handled by polling. */
+}
+
+static void exynosautov920_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
+{
+ unsigned int idx;
+
+ for (idx = 0; idx < data->sensor_count; idx++) {
+ if (!data->tzd_array[idx])
+ continue;
+
+ exynos_tmu_update_temp(data, EXYNOSAUTOV920_TMU_REG_THRESHOLD(idx), 16, temp);
+ exynos_tmu_update_bit(data, EXYNOSAUTOV920_TMU_REG_INTEN(idx), 7, true);
+ }
+}
+
+static void exynosautov920_tmu_initialize(struct platform_device *pdev)
+{
+ struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+ unsigned int val;
+
+ data->tmu_control(pdev, false);
+
+ update_con_reg(data);
+
+ val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
+ data->cal_type = TYPE_TWO_POINT_TRIMMING;
+ data->slope_comp = (val >> EXYNOSAUTOV920_SLOPE_COMP) & EXYNOSAUTOV920_SLOPE_COMP_MASK;
+
+ val = readl(data->base + EXYNOSAUTOV920_SENSOR0_TRIM_INFO);
+ data->temp_error1 = (val >> EXYNOSAUTOV920_TRIMINFO_25_SHIFT) & EXYNOSAUTOV920_TRIM_MASK;
+ data->temp_error2 = (val >> EXYNOSAUTOV920_TRIMINFO_85_SHIFT) & EXYNOSAUTOV920_TRIM_MASK;
+
+ val = readl(data->base + EXYNOSAUTOV920_TMU_REG_TRIMINFO2);
+ val = (val >> EXYNOSAUTOV920_CALIB_SEL_TEMP) & EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK;
+
+ data->calib_temp = (EXYNOS_SECOND_POINT_TRIM + (20 * val));
+}
+
static void exynos4210_tmu_control(struct platform_device *pdev, bool on)
{
struct exynos_tmu_data *data = platform_get_drvdata(pdev);
@@ -633,6 +799,24 @@ static void exynos7_tmu_control(struct platform_device *pdev, bool on)
writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
}
+static void exynosautov920_tmu_control(struct platform_device *pdev, bool on)
+{
+ struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+ unsigned int con;
+
+ con = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
+
+ if (on) {
+ con |= BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
+ con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT);
+ } else {
+ con &= ~BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
+ con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT);
+ }
+
+ writel(con, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
+}
+
static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
{
struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
@@ -671,7 +855,7 @@ 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_EXYNOSAUTOV920) {
val &= ~(EXYNOS7_EMUL_DATA_MASK <<
EXYNOS7_EMUL_DATA_SHIFT);
val |= (temp_to_code(data, temp) <<
@@ -703,6 +887,8 @@ static void exynos4412_tmu_set_emulation(struct exynos_tmu_data *data,
emul_con = EXYNOS5433_TMU_EMUL_CON;
else if (data->soc == SOC_ARCH_EXYNOS7)
emul_con = EXYNOS7_TMU_REG_EMUL_CON;
+ else if (data->soc == SOC_ARCH_EXYNOSAUTOV920)
+ emul_con = EXYNOSAUTOV920_TMU_REG_EMUL_CON;
else
emul_con = EXYNOS_EMUL_CON;
@@ -756,11 +942,23 @@ static int exynos7_tmu_read(struct exynos_tmu_data *data)
EXYNOS7_TMU_TEMP_MASK;
}
+static int exynosautov920_tmu_read(struct exynos_tmu_data *data)
+{
+ return readw(data->base + EXYNOSAUTOV920_CURRENT_TEMP_P1_P0) &
+ EXYNOS7_TMU_TEMP_MASK;
+}
+
static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
{
struct exynos_tmu_data *data = id;
+ int idx;
- thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
+ for (idx = 0; idx < data->sensor_count; idx++) {
+ if (!data->tzd_array[idx])
+ continue;
+
+ thermal_zone_device_update(data->tzd_array[idx], THERMAL_EVENT_UNSPECIFIED);
+ }
mutex_lock(&data->lock);
clk_enable(data->clk);
@@ -805,6 +1003,19 @@ static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
writel(val_irq, data->base + tmu_intclear);
}
+static void exynosautov920_tmu_clear_irqs(struct exynos_tmu_data *data)
+{
+ unsigned int idx, val_irq;
+
+ for (idx = 0; idx < data->sensor_count; idx++) {
+ if (!data->tzd_array[idx])
+ continue;
+
+ val_irq = readl(data->base + EXYNOSAUTOV920_TMU_REG_INT_PEND(idx));
+ writel(val_irq, data->base + EXYNOSAUTOV920_TMU_REG_INT_PEND(idx));
+ }
+}
+
static const struct of_device_id exynos_tmu_match[] = {
{
.compatible = "samsung,exynos3250-tmu",
@@ -833,6 +1044,9 @@ static const struct of_device_id exynos_tmu_match[] = {
}, {
.compatible = "samsung,exynos7-tmu",
.data = (const void *)SOC_ARCH_EXYNOS7,
+ }, {
+ .compatible = "samsung,exynosautov920-tmu",
+ .data = (const void *)SOC_ARCH_EXYNOSAUTOV920,
},
{ },
};
@@ -865,6 +1079,10 @@ static int exynos_map_dt_data(struct platform_device *pdev)
data->soc = (uintptr_t)of_device_get_match_data(&pdev->dev);
+ data->sensor_count = EXYNOS_DEFAULT_SENSOR_COUNT;
+
+ data->calib_temp = EXYNOS_SECOND_POINT_TRIM;
+
switch (data->soc) {
case SOC_ARCH_EXYNOS4210:
data->tmu_set_low_temp = exynos4210_tmu_set_low_temp;
@@ -945,6 +1163,19 @@ static int exynos_map_dt_data(struct platform_device *pdev)
data->min_efuse_value = 15;
data->max_efuse_value = 100;
break;
+ case SOC_ARCH_EXYNOSAUTOV920:
+ data->tmu_set_low_temp = exynosautov920_tmu_set_low_temp;
+ data->tmu_set_high_temp = exynosautov920_tmu_set_high_temp;
+ data->tmu_disable_low = exynosautov920_tmu_disable_low;
+ data->tmu_disable_high = exynosautov920_tmu_disable_high;
+ data->tmu_set_crit_temp = exynosautov920_tmu_set_crit_temp;
+ data->tmu_initialize = exynosautov920_tmu_initialize;
+ data->tmu_control = exynosautov920_tmu_control;
+ data->tmu_read = exynosautov920_tmu_read;
+ data->tmu_set_emulation = exynos4412_tmu_set_emulation;
+ data->tmu_clear_irqs = exynosautov920_tmu_clear_irqs;
+ data->sensor_count = EXYNOS_MAX_SENSOR_COUNT;
+ break;
default:
dev_err(&pdev->dev, "Platform not supported\n");
return -EINVAL;
@@ -952,6 +1183,14 @@ static int exynos_map_dt_data(struct platform_device *pdev)
data->cal_type = TYPE_ONE_POINT_TRIMMING;
+ if (data->soc == SOC_ARCH_EXYNOSAUTOV920) {
+ if (of_property_read_u32(pdev->dev.of_node, "samsung,sensors",
+ &data->sensor_count)) {
+ dev_err(&pdev->dev, "failed to get sensor count\n");
+ return -ENODEV;
+ }
+ }
+
/*
* Check if the TMU shares some registers and then try to map the
* memory of common registers.
@@ -1006,7 +1245,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct exynos_tmu_data *data;
- int ret;
+ struct thermal_zone_device *tzd;
+ int ret, idx;
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -1084,11 +1324,19 @@ static int exynos_tmu_probe(struct platform_device *pdev)
goto err_sclk;
}
- data->tzd = devm_thermal_of_zone_register(dev, 0, data,
- &exynos_sensor_ops);
- if (IS_ERR(data->tzd)) {
- ret = dev_err_probe(dev, PTR_ERR(data->tzd), "Failed to register sensor\n");
- goto err_sclk;
+ for (idx = 0; idx < data->sensor_count; idx++) {
+ tzd = devm_thermal_of_zone_register(dev, idx, data, &exynos_sensor_ops);
+
+ if (IS_ERR(tzd)) {
+ if (PTR_ERR(tzd) == -ENODEV)
+ continue;
+
+ ret = dev_err_probe(dev, PTR_ERR(data->tzd_array[idx]),
+ "Failed to register sensor\n");
+ goto err_sclk;
+ }
+
+ data->tzd_array[idx] = tzd;
}
ret = exynos_thermal_zone_configure(pdev);
--
2.50.1Hi, Shin Son, Just trivial notes on registers description for now. On 11/13/25 8:40 AM, Shin Son wrote: > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/ > thermal/ samsung/exynos_tmu.c index 47a99b3c5395..8fa188928b79 > 100644 --- a/ drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/ > thermal/samsung/ exynos_tmu.c @@ -121,8 +121,51 @@ > > #define EXYNOS_NOISE_CANCEL_MODE 4 > > +/* ExynosAutov920 specific registers */ +#define > EXYNOSAUTOV920_SLOPE_COMP 25 +#define > EXYNOSAUTOV920_SLOPE_COMP_MASK 0xf Register fields shall be named SOC_REG_NAME_FIELD_NAME If you include <linux/bits.h> you can substitute the above 2 definitions with just one: EXYNOSAUTOV920_TRIMINFO_SLOPE_COMP GENMASK(28, 25) and later on in the code, instead of doing the shift and the mask, you can include <linux/bitfield.h> and do: data->slope_comp = FIELD_GET(EXYNOSAUTOV920_TRIMINFO_SLOPE_COMP, val); btw, above matches the GS101 definitions. > +#define EXYNOSAUTOV920_CALIB_SEL_TEMP 30 +#define > EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK 0x2 + is this BIT(31)? EXYNOSAUTOV920_TRIMINFO2_CALIB_SEL_TEMP BIT(31) GS101 differs, it has this field defined at: GS101_TRIMINFO_CALIB_SEL_TEMP BIT(0) where TRIMINFO is at Base Address + 0x0, not at Base Address + 0x4 as in your case. > +#define EXYNOSAUTOV920_SENSOR0_TRIM_INFO 0x10 GS101 does not have any SENSOR0 in the reg name, so maybe rename it to: #define EXYNOSAUTOV920_TRIMINFO0 0x10 > +#define EXYNOSAUTOV920_TRIM_MASK 0x1ff +#define > EXYNOSAUTOV920_TRIMINFO_25_SHIFT 0 +#define > EXYNOSAUTOV920_TRIMINFO_85_SHIFT 9 #define EXYNOSAUTOV920_TRIMINFO_85_P0 GENMASK(17, 9) #define EXYNOSAUTOV920_TRIMINFO_25_P0 GENMASK(8, 0) > + +#define EXYNOSAUTOV920_TMU_REG_TRIMINFO2 0x04 Is this a TRIMINFO_CONFIG2 register? I don't have such thing on GS101. > + +#define EXYNOSAUTOV920_TMU_REG_THRESHOLD(p) (((p)) * 0x50 + > 0x00d0) #define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6(p) (((p)) * 0x50 + 0xd0) and then: #define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6_RISE7 GENMASK(24, 16) #define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6_RISE6 GENMASK(8, 0) you'll stop passing the shift and mask as function arguments :) > +#define EXYNOSAUTOV920_TMU_REG_INTEN(p) (((p)) * 0x50 + 0x00f0) #define EXYNOSAUTOV920_INTEN(p) (((p)) * 0x50 + 0xf0) I see you use just BIT(7) from this register. Let's define it and stop passing the bit offset as function argument: #define EXYNOSAUTOV920_INTEN_RISE7 BIT(7) > +#define EXYNOSAUTOV920_TMU_REG_INT_PEND(p) (((p)) * 0x50 + 0x00f8) #define EXYNOSAUTOV920_PEND(p) (((p)) * 0x50 + 0xf8) Are you using GENMASK(15, 0) for this register? On GS101 GENMASK(15, 9) is reserved. Here's how the bits are defined for GS101: #define EXYNOSAUTOV920_PEND_FALL(i) BIT(16 + (i)) #define EXYNOSAUTOV920_PEND_RISE_MASK GENMASK(23, 16) #define EXYNOSAUTOV920_PEND_RISE(i) BIT(i) #define EXYNOSAUTOV920_PEND_RISE_MASK GENMASK(8, 0) Would you please verify and let me know if EXYNOSAUTOV920 differs or not? > +#define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0 0x084 no leading 0 #define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0 0x84 then define the fields: #define EXYNOSAUTOV920_CURRENT_TEMP_P1 GENMASK(24, 16) #define EXYNOSAUTOV920_CURRENT_TEMP_P0 GENMASK(8, 0) > +#define EXYNOSAUTOV920_TMU_REG_EMUL_CON 0x0b0 no TMU_REG in the name, no leading 0, define the fields as GENMASK #define EXYNOSAUTOV920_EMUL_CON 0xb0 #define EXYNOSAUTOV920_EMUL_CON_EMUL_NEXT_TIME GENMASK(31, 16) #define EXYNOSAUTOV920_EMUL_CON_EMUL_NEXT_DATA GENMASK(15, 7) #define EXYNOSAUTOV920_EMUL_CON_EMUL_EN BIT(0) > + > +#define EXYNOSAUTOV920_TMU_REG_CONTROL 0x50 no reg in the name, control0 #define EXYNOSAUTOV920_TMU_CONTROL0 0x50 define fields as GENMASK and BIT > +#define EXYNOSAUTOV920_TMU_REG_CONTROL1 0x54 ditto > +#define EXYNOSAUTOV920_TMU_REG_AVG_CONTROL 0x58 ditto > +#define EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL 0x70 no TMU in the name, respect the registers name from the datasheet please. define the full genmask #define EXYNOSAUTOV920_SAMPLING_INTERVAL_MASK GENMASK(31, 0) > +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0 0x74 > +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1 0x78 no TMU_REG in the name, define fields> + > +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT 8 > +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK 0x1f > +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT 3 > +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK 0xf > +#define EXYNOSAUTOV920_TMU_NUM_PROBE_MASK 0xf > +#define EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT 16 > +#define EXYNOSAUTOV920_TMU_LPI_MODE_MASK 1 > +#define EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT 10 you won't need these if you define the register fields, isn't it?> + > +#define EXYNOSAUTOV920_TMU_AVG_CON_UPDATE 0x0008011a no leading zeros. You better construct the fields dynamically, by using bitfields, no full register magic number, humans don't understand this. > +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE 0x030003c0 > +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE 0x03c0004d same for both > + > #define MCELSIUS 1000 > > +#define EXYNOS_DEFAULT_SENSOR_COUNT 1 > +#define EXYNOS_MAX_SENSOR_COUNT would it make sense to have the tzd_array to fit just the sensor count that we're using so that we don't waste memory? i.e. allocate tzd_array dynamically. Looking at the exynosautov9 registers that you described and comparing them with gs101 I see just 2 differences: 1/ exnosautov2 has a TRIMINFO_CONFIG2 register, while gs101 doesn't 2/ EXYNOSAUTOV920_PEND register fields differ from GS101 Given the similarities, and considering the EXYNOS9_ registers rename from: https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-5-youngmin.nam@samsung.com/ would it make sense to use the SoC-era name instead of specific SoC, i.e. s/EXYNOSAUTOV920_/EXYNOS9_ and use the latter for both exynosautov9 and gs101? Cheers, ta
Hello Tudor Ambarus > -----Original Message----- > From: Tudor Ambarus [mailto:tudor.ambarus@linaro.org] > Sent: Tuesday, November 25, 2025 6:15 PM > To: Shin Son <shin.son@samsung.com>; Bartlomiej Zolnierkiewicz > <bzolnier@gmail.com>; Krzysztof Kozlowski <krzk@kernel.org>; Rafael J . > Wysocki <rafael@kernel.org>; Daniel Lezcano <daniel.lezcano@linaro.org>; > Zhang Rui <rui.zhang@intel.com>; Lukasz Luba <lukasz.luba@arm.com>; Rob > Herring <robh@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Alim Akhtar > <alim.akhtar@samsung.com> > Cc: Henrik Grimler <henrik@grimler.se>; linux-pm@vger.kernel.org; linux- > samsung-soc@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Peter Griffin > <peter.griffin@linaro.org>; André Draszik <andre.draszik@linaro.org>; > William McVicker <willmcvicker@google.com>; jyescas@google.com > Subject: Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new > hardware and update TMU interface > > Hi, Shin Son, > > Just trivial notes on registers description for now. > > On 11/13/25 8:40 AM, Shin Son wrote: > > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/ thermal/ > > samsung/exynos_tmu.c index 47a99b3c5395..8fa188928b79 > > 100644 --- a/ drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/ > > thermal/samsung/ exynos_tmu.c @@ -121,8 +121,51 @@ > > > > #define EXYNOS_NOISE_CANCEL_MODE 4 > > > > +/* ExynosAutov920 specific registers */ +#define > > EXYNOSAUTOV920_SLOPE_COMP 25 +#define > > EXYNOSAUTOV920_SLOPE_COMP_MASK 0xf > > Register fields shall be named > SOC_REG_NAME_FIELD_NAME > > If you include <linux/bits.h> you can substitute the above 2 definitions > with just one: > EXYNOSAUTOV920_TRIMINFO_SLOPE_COMP GENMASK(28, 25) > > and later on in the code, instead of doing the shift and the mask, you can > include <linux/bitfield.h> and do: > > data->slope_comp = FIELD_GET(EXYNOSAUTOV920_TRIMINFO_SLOPE_COMP, val); > > btw, above matches the GS101 definitions. I understand your point and will apply there bit operations in the new patch series. > > +#define EXYNOSAUTOV920_CALIB_SEL_TEMP 30 +#define > > EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK 0x2 + > > is this BIT(31)? > EXYNOSAUTOV920_TRIMINFO2_CALIB_SEL_TEMP BIT(31) > This field actually uses both bit 30 and bit 31, so the mask should be updated from 0x2 to 0x3. Sorry for the confusion. > GS101 differs, it has this field defined at: > GS101_TRIMINFO_CALIB_SEL_TEMP BIT(0) > where TRIMINFO is at Base Address + 0x0, not at Base Address + 0x4 as in > your case. At base address + 0x4, the register actually contains CALIB_TEMP, not CALIB_SEL in my case. So the naming was incorrect, and I'll fix it in the next patch series. > > +#define EXYNOSAUTOV920_SENSOR0_TRIM_INFO 0x10 > > GS101 does not have any SENSOR0 in the reg name, so maybe rename it to: > #define EXYNOSAUTOV920_TRIMINFO0 0x10 That's correct. I'll rename it in the next patch series. > > +#define EXYNOSAUTOV920_TRIM_MASK 0x1ff +#define > > EXYNOSAUTOV920_TRIMINFO_25_SHIFT 0 +#define > > EXYNOSAUTOV920_TRIMINFO_85_SHIFT 9 > > #define EXYNOSAUTOV920_TRIMINFO_85_P0 GENMASK(17, 9) > #define EXYNOSAUTOV920_TRIMINFO_25_P0 GENMASK(8, 0) Understood. Thanks. > > + +#define EXYNOSAUTOV920_TMU_REG_TRIMINFO2 0x04 > > Is this a TRIMINFO_CONFIG2 register? I don't have such thing on GS101. No, that name is incorrect. I'll rename it to TRIMINFO_CONFIG1. > > + +#define EXYNOSAUTOV920_TMU_REG_THRESHOLD(p) (((p)) * 0x50 + > > 0x00d0) > > #define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6(p) (((p)) * 0x50 + 0xd0) > and then: > #define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6_RISE7 GENMASK(24, 16) > #define EXYNOSAUTOV920_THRESHOLD_TEMP_RISE7_6_RISE6 GENMASK(8, 0) > you'll stop passing the shift and mask as function arguments :) Got it. Thanks :). > > +#define EXYNOSAUTOV920_TMU_REG_INTEN(p) (((p)) * 0x50 + > 0x00f0) > > #define EXYNOSAUTOV920_INTEN(p) (((p)) * 0x50 + 0xf0) > > I see you use just BIT(7) from this register. Let's define it and stop > passing the bit offset as function argument: > #define EXYNOSAUTOV920_INTEN_RISE7 BIT(7) Ok, Thanks. I'll define the BIT(7) field as suggested. > > +#define EXYNOSAUTOV920_TMU_REG_INT_PEND(p) (((p)) * 0x50 + 0x00f8) > > #define EXYNOSAUTOV920_PEND(p) (((p)) * 0x50 + 0xf8) Understood. > Are you using GENMASK(15, 0) for this register? > > On GS101 GENMASK(15, 9) is reserved. Here's how the bits are defined for > GS101: > > #define EXYNOSAUTOV920_PEND_FALL(i) BIT(16 + (i)) > #define EXYNOSAUTOV920_PEND_RISE_MASK GENMASK(23, 16) > #define EXYNOSAUTOV920_PEND_RISE(i) BIT(i) > #define EXYNOSAUTOV920_PEND_RISE_MASK GENMASK(8, 0) > > Would you please verify and let me know if EXYNOSAUTOV920 differs or not? It differs, On Exynosautov920m bits 8 - 15 are reserved, so only bits 0-7 are used. > > +#define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0 0x084 > > no leading 0 > #define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0 0x84 Got it. > then define the fields: > #define EXYNOSAUTOV920_CURRENT_TEMP_P1 GENMASK(24, 16) > #define EXYNOSAUTOV920_CURRENT_TEMP_P0 GENMASK(8, 0) Thank you. > > +#define EXYNOSAUTOV920_TMU_REG_EMUL_CON 0x0b0 > > no TMU_REG in the name, no leading 0, define the fields as GENMASK > #define EXYNOSAUTOV920_EMUL_CON 0xb0 > #define EXYNOSAUTOV920_EMUL_CON_EMUL_NEXT_TIME GENMASK(31, 16) > #define EXYNOSAUTOV920_EMUL_CON_EMUL_NEXT_DATA GENMASK(15, 7) > #define EXYNOSAUTOV920_EMUL_CON_EMUL_EN BIT(0) That's right. Thanks. > > + > > +#define EXYNOSAUTOV920_TMU_REG_CONTROL 0x50 > > no reg in the name, control0 > #define EXYNOSAUTOV920_TMU_CONTROL0 0x50 > > define fields as GENMASK and BIT Got it. > > +#define EXYNOSAUTOV920_TMU_REG_CONTROL1 0x54 > > ditto Got it. > > +#define EXYNOSAUTOV920_TMU_REG_AVG_CONTROL 0x58 > > ditto Got it. > > +#define EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL 0x70 > > no TMU in the name, respect the registers name from the datasheet please. > define the full genmask > #define EXYNOSAUTOV920_SAMPLING_INTERVAL_MASK GENMASK(31, 0) Ok, I'll follow the datasheet naming update it accordingly. > > +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0 0x74 > > +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1 0x78 > > no TMU_REG in the name, define fields Got it. > > +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT 8 > > +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK 0x1f > > +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT 3 > > +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK 0xf > > +#define EXYNOSAUTOV920_TMU_NUM_PROBE_MASK 0xf > > +#define EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT 16 > > +#define EXYNOSAUTOV920_TMU_LPI_MODE_MASK 1 > > +#define EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT 10 > > you won't need these if you define the register fields, isn't it? Got it. I'll refactor this part accordingly. > > +#define EXYNOSAUTOV920_TMU_AVG_CON_UPDATE 0x0008011a > > no leading zeros. You better construct the fields dynamically, by using > bitfields, no full register magic number, humans don't understand this. Understood. I'll replace the magic number with proper bit field constructions. > > +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE 0x030003c0 > > +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE 0x03c0004d > > same for both Got it. > > + > > #define MCELSIUS 1000 > > > > +#define EXYNOS_DEFAULT_SENSOR_COUNT 1 > > +#define EXYNOS_MAX_SENSOR_COUNT > would it make sense to have the tzd_array to fit just the sensor count > that we're using so that we don't waste memory? i.e. allocate tzd_array > dynamically. Ok, I may need to prepare a separate patch for the thermal driver on eav920 and allocate the tzd_array dynamically there. > Looking at the exynosautov9 registers that you described and comparing > them with > gs101 I see just 2 differences: > 1/ exnosautov2 has a TRIMINFO_CONFIG2 register, while gs101 doesn't 2/ > EXYNOSAUTOV920_PEND register fields differ from GS101 TRIMINFO_CONFIG2 doesn't exist on eav920 either; I simply misnamed it. However, the PEND register indeed differs from GS101. > Given the similarities, and considering the EXYNOS9_ registers rename from: > https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-5- > youngmin.nam@samsung.com/ > would it make sense to use the SoC-era name instead of specific SoC, i.e. > s/EXYNOSAUTOV920_/EXYNOS9_ and use the latter for both exynosautov9 and > gs101? > > Cheers, > ta First of all, as far as I know, EXYNOS9 is not the same as exynosautov9, and exynosautov920 also differs from exynosautov9. So while sharing a common prefix is a good suggestion in general, I believe it's not appropriate here Because the register definitions are not fully compatible across these SoCs. Using a common name array may introduce confusion later. I'll prepare a new patch series accordingly and make sure to CC you. Best regards, Shin
Hi, Shin Son, On 11/26/25 9:19 AM, 손신 wrote: >> Looking at the exynosautov9 registers that you described and comparing >> them with >> gs101 I see just 2 differences: >> 1/ exnosautov2 has a TRIMINFO_CONFIG2 register, while gs101 doesn't 2/ >> EXYNOSAUTOV920_PEND register fields differ from GS101 > TRIMINFO_CONFIG2 doesn't exist on eav920 either; I simply misnamed it. > However, the PEND register indeed differs from GS101. > >> Given the similarities, and considering the EXYNOS9_ registers rename from: >> https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-5- >> youngmin.nam@samsung.com/ >> would it make sense to use the SoC-era name instead of specific SoC, i.e. >> s/EXYNOSAUTOV920_/EXYNOS9_ and use the latter for both exynosautov9 and >> gs101? >> > First of all, as far as I know, EXYNOS9 is not the same as exynosautov9, and exynosautov920 also differs from exynosautov9. See also see this patch, or maybe the entire patch set: https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-2-youngmin.nam@samsung.com/ It's not just autov9 and gs101 that have similar TMU registers (with the two exceptions AFAICT), it's also exynos850 that seems identical with autov9. All seem to be part of the same "Exynos9-era" SoCs. Let's think about how gs101/exynos850 TMU addition will follow. Shall one use the EXYNOSAUTOV920 registers? That seems misleading. Shall one redefine the entire register set? That won't fly because of the code duplication. Thus I propose to use the EXYNOS9 prefix for the register definitions, and if there are SoCs with slight differences, that can be handled with compatible match data and specific SoC definitions, but only where things differ. > So while sharing a common prefix is a good suggestion in general, I believe it's not appropriate here > Because the register definitions are not fully compatible across these SoCs. Using a common name array may introduce confusion later. Please reconsider this. Maybe Youngmin Nam or others can intervene. Thanks! ta
On 11/26/25 18:21, Tudor Ambarus wrote: > Hi, Shin Son, > > On 11/26/25 9:19 AM, 손신 wrote: >>> Looking at the exynosautov9 registers that you described and comparing >>> them with >>> gs101 I see just 2 differences: >>> 1/ exnosautov2 has a TRIMINFO_CONFIG2 register, while gs101 doesn't 2/ >>> EXYNOSAUTOV920_PEND register fields differ from GS101 >> TRIMINFO_CONFIG2 doesn't exist on eav920 either; I simply misnamed it. >> However, the PEND register indeed differs from GS101. >> >>> Given the similarities, and considering the EXYNOS9_ registers rename from: >>> https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-5- >>> youngmin.nam@samsung.com/ >>> would it make sense to use the SoC-era name instead of specific SoC, i.e. >>> s/EXYNOSAUTOV920_/EXYNOS9_ and use the latter for both exynosautov9 and >>> gs101? >>> >> First of all, as far as I know, EXYNOS9 is not the same as exynosautov9, and exynosautov920 also differs from exynosautov9. > > See also see this patch, or maybe the entire patch set: > https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-2-youngmin.nam@samsung.com/ > > It's not just autov9 and gs101 that have similar TMU registers (with the two > exceptions AFAICT), it's also exynos850 that seems identical with autov9. > > All seem to be part of the same "Exynos9-era" SoCs. Let's think about how > gs101/exynos850 TMU addition will follow. Shall one use the EXYNOSAUTOV920 > registers? That seems misleading. Shall one redefine the entire register set? > That won't fly because of the code duplication. > > Thus I propose to use the EXYNOS9 prefix for the register definitions, and if > there are SoCs with slight differences, that can be handled with compatible > match data and specific SoC definitions, but only where things differ. > >> So while sharing a common prefix is a good suggestion in general, I believe it's not appropriate here >> Because the register definitions are not fully compatible across these SoCs. Using a common name array may introduce confusion later. > > Please reconsider this. Maybe Youngmin Nam or others can intervene. > > Thanks! > ta > Hi Tudor, First, thank you for referencing my pinctrl patch and asking for my input. Yes. That's exactly my intent. Use the EXYNOS9 prefix to minimize fragmentation. Even if some registers aren't fully compatible, I'd prefer to maximize the common EXYNOS9 definitions and handle SoC-specific differences via match data or small deltas, rather than introducing SoC-specific register names. Hi Shin Son, if possible, please consider this approach as well. Thanks, Youngmin
Hello, Youngmin Nam. > Hi Shin Son, > if possible, please consider this approach as well. > > Thanks, > Youngmin Ok, Understood. When I resume this task, I'll CC you. Thank you for your opinion. Best regards, Shin Son
Hello, Tudor Ambarus > -----Original Message----- > From: Tudor Ambarus [mailto:tudor.ambarus@linaro.org] > Sent: Wednesday, November 26, 2025 6:22 PM > To: 손신 <shin.son@samsung.com>; 'Bartlomiej Zolnierkiewicz' > <bzolnier@gmail.com>; 'Krzysztof Kozlowski' <krzk@kernel.org>; 'Rafael J . > Wysocki' <rafael@kernel.org>; 'Daniel Lezcano' <daniel.lezcano@linaro.org>; > 'Zhang Rui' <rui.zhang@intel.com>; 'Lukasz Luba' <lukasz.luba@arm.com>; > 'Rob Herring' <robh@kernel.org>; 'Conor Dooley' <conor+dt@kernel.org>; > 'Alim Akhtar' <alim.akhtar@samsung.com>; youngmin.nam@samsung.com > Cc: 'Henrik Grimler' <henrik@grimler.se>; linux-pm@vger.kernel.org; linux- > samsung-soc@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; 'Peter Griffin' > <peter.griffin@linaro.org>; 'André Draszik' <andre.draszik@linaro.org>; > 'William McVicker' <willmcvicker@google.com>; jyescas@google.com > Subject: Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new > hardware and update TMU interface > > Hi, Shin Son, > > On 11/26/25 9:19 AM, 손신 wrote: > >> Looking at the exynosautov9 registers that you described and > >> comparing them with > >> gs101 I see just 2 differences: > >> 1/ exnosautov2 has a TRIMINFO_CONFIG2 register, while gs101 doesn't > >> 2/ EXYNOSAUTOV920_PEND register fields differ from GS101 > > TRIMINFO_CONFIG2 doesn't exist on eav920 either; I simply misnamed it. > > However, the PEND register indeed differs from GS101. > > > >> Given the similarities, and considering the EXYNOS9_ registers rename > from: > >> https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-5- > >> youngmin.nam@samsung.com/ > >> would it make sense to use the SoC-era name instead of specific SoC, > i.e. > >> s/EXYNOSAUTOV920_/EXYNOS9_ and use the latter for both exynosautov9 > >> and gs101? > >> > > First of all, as far as I know, EXYNOS9 is not the same as exynosautov9, > and exynosautov920 also differs from exynosautov9. > > See also see this patch, or maybe the entire patch set: > https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-2- > youngmin.nam@samsung.com/ > > It's not just autov9 and gs101 that have similar TMU registers (with the > two exceptions AFAICT), it's also exynos850 that seems identical with > autov9. Yes, Do you have any plans to upstream the GS101 TMU code? From what I understand, Autov9 and exynos850 are unlikely to be upstreamed in their current form. > All seem to be part of the same "Exynos9-era" SoCs. Let's think about how > gs101/exynos850 TMU addition will follow. Shall one use the EXYNOSAUTOV920 > registers? That seems misleading. Shall one redefine the entire register > set? > That won't fly because of the code duplication. I kind of admit that. > Thus I propose to use the EXYNOS9 prefix for the register definitions, and > if there are SoCs with slight differences, that can be handled with > compatible match data and specific SoC definitions, but only where things > differ. However, I am not sure whether Exynos2200, 7885, 990, 9810, 8890, 8895, or FSD share the same TMU hardware layout as exynosautov920. So I’m wondering whether the EXYNOS9 prefix should be limited to GS101 and eav920, or if we should consider a different prefix that better reflects the grouping. > > So while sharing a common prefix is a good suggestion in general, I > > believe it's not appropriate here Because the register definitions are > not fully compatible across these SoCs. Using a common name array may > introduce confusion later. > > Please reconsider this. Maybe Youngmin Nam or others can intervene. Ok, I'll reconsider this based on your clarification. Thank you for the detailed feedback. > > Thanks! > ta Best regards, Shin
On 11/27/25 5:07 AM, 손신 wrote: > Hello, Tudor Ambarus Hi! > >> -----Original Message----- >> From: Tudor Ambarus [mailto:tudor.ambarus@linaro.org] >> Sent: Wednesday, November 26, 2025 6:22 PM >> To: 손신 <shin.son@samsung.com>; 'Bartlomiej Zolnierkiewicz' >> <bzolnier@gmail.com>; 'Krzysztof Kozlowski' <krzk@kernel.org>; 'Rafael J . >> Wysocki' <rafael@kernel.org>; 'Daniel Lezcano' <daniel.lezcano@linaro.org>; >> 'Zhang Rui' <rui.zhang@intel.com>; 'Lukasz Luba' <lukasz.luba@arm.com>; >> 'Rob Herring' <robh@kernel.org>; 'Conor Dooley' <conor+dt@kernel.org>; >> 'Alim Akhtar' <alim.akhtar@samsung.com>; youngmin.nam@samsung.com >> Cc: 'Henrik Grimler' <henrik@grimler.se>; linux-pm@vger.kernel.org; linux- >> samsung-soc@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; 'Peter Griffin' >> <peter.griffin@linaro.org>; 'André Draszik' <andre.draszik@linaro.org>; >> 'William McVicker' <willmcvicker@google.com>; jyescas@google.com >> Subject: Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new >> hardware and update TMU interface >> >> Hi, Shin Son, >> >> On 11/26/25 9:19 AM, 손신 wrote: >>>> Looking at the exynosautov9 registers that you described and >>>> comparing them with >>>> gs101 I see just 2 differences: >>>> 1/ exnosautov2 has a TRIMINFO_CONFIG2 register, while gs101 doesn't >>>> 2/ EXYNOSAUTOV920_PEND register fields differ from GS101 >>> TRIMINFO_CONFIG2 doesn't exist on eav920 either; I simply misnamed it. >>> However, the PEND register indeed differs from GS101. >>> >>>> Given the similarities, and considering the EXYNOS9_ registers rename >> from: >>>> https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-5- >>>> youngmin.nam@samsung.com/ >>>> would it make sense to use the SoC-era name instead of specific SoC, >> i.e. >>>> s/EXYNOSAUTOV920_/EXYNOS9_ and use the latter for both exynosautov9 >>>> and gs101? >>>> >>> First of all, as far as I know, EXYNOS9 is not the same as exynosautov9, >> and exynosautov920 also differs from exynosautov9. >> >> See also see this patch, or maybe the entire patch set: >> https://lore.kernel.org/linux-samsung-soc/20251117074140.4090939-2- >> youngmin.nam@samsung.com/ >> >> It's not just autov9 and gs101 that have similar TMU registers (with the >> two exceptions AFAICT), it's also exynos850 that seems identical with >> autov9. > > Yes, Do you have any plans to upstream the GS101 TMU code? From what I understand, Yes, I'm currently working on upstreaming the GS101 TMU code. My plan is to do the acpm tmu helpers and then integrate the gs101 TMU support with what will be the new exynos TMU driver. > Autov9 and exynos850 are unlikely to be upstreamed in their current form. From what I understand from your email exchanges with Daniel, you're going to propose a new driver. Is my understanding correct? Do you have a timeline for it? I'll then follow with the gs101 support. > >> All seem to be part of the same "Exynos9-era" SoCs. Let's think about how >> gs101/exynos850 TMU addition will follow. Shall one use the EXYNOSAUTOV920 >> registers? That seems misleading. Shall one redefine the entire register >> set? >> That won't fly because of the code duplication. > > I kind of admit that. > >> Thus I propose to use the EXYNOS9 prefix for the register definitions, and >> if there are SoCs with slight differences, that can be handled with >> compatible match data and specific SoC definitions, but only where things >> differ. > > However, I am not sure whether Exynos2200, 7885, 990, 9810, 8890, 8895, or FSD share the same TMU hardware layout as exynosautov920. > So I’m wondering whether the EXYNOS9 prefix should be limited to GS101 and eav920, or if we should consider a different prefix that better reflects the grouping. exynos850 has the same reg layout as eav920 and gs101 too. If Exynos9-era is a common terminology used inside Samsung then we should be good to go with Exynos9 prefix I think. Especially since we have a predecessor, the renaming tried in pinctrl. But if you're not sure about it, use just EXYNOS_ then. Cheers, ta
Hello, Tudor Ambarus. > > Yes, Do you have any plans to upstream the GS101 TMU code? From what I > > understand, > > Yes, I'm currently working on upstreaming the GS101 TMU code. My plan is > to do the acpm tmu helpers and then integrate the gs101 TMU support with > what will be the new exynos TMU driver. Ok, Understood. > > Autov9 and exynos850 are unlikely to be upstreamed in their current form. > > From what I understand from your email exchanges with Daniel, you're going > to propose a new driver. Is my understanding correct? Do you have a > timeline for it? > I'll then follow with the gs101 support. We don't have a timeline yet. Separating the driver was not our initial plan, So this work will likely be postponed to next year. When we resume it, I will CC you. > exynos850 has the same reg layout as eav920 and gs101 too. If Exynos9-era > is a common terminology used inside Samsung then we should be good to go > with > Exynos9 prefix I think. Especially since we have a predecessor, the > renaming tried in pinctrl. But if you're not sure about it, use just > EXYNOS_ then. > > Cheers, > ta Got it. Thank you for your clarification. Best regards, Shin
On 11/13/25 07:40, Shin Son wrote:
> The Exynos tmu driver's private data structure has been extended
> to support the exynosautov920 hardware, which requires per-sensor interrupt
> enablement and multiple-zone handling:
>
> - Add 'slope_comp' : compensation parameter below 25 degrees.
> - Add 'calib_temp' : stores the fused calibaration temperature.
> - Add 'sensor_count' : reflects the maximum sensor numbers.
> - Rename 'tzd' -> 'tzd_array' to register multiple thermal zones.
>
> Since splitting this patch causes runtime errors during temperature
> emulation or problems where the read temperature feature fails to
> retrieve values, I have submitted it as a single commit. To add support
> for the exynosautov920 to the exisiting TMU interface, the following
> changes are included:
>
> 1. Simplify "temp_to_code" and "code_to_temp" to one computation path
> by normalizing calib_temp.
> 2. Loop over 'sensor_count' in critical-point setup.
> 3. Introduce 'update_con_reg' for exynosautov920 control-register updates.
> 4. Add exynosautov920-specific branch in 'exynos_tmu_update_temp' function.
> 5. Skip high & low temperature threshold setup in exynosautov920.
> 6. Enable interrupts via sensor_count in exynosautov920.
> 7. Initialize all new members during 'exynosautov920_tmu_initialize'.
> 8. Clear IRQs by iterating the sensor_count in exynosautov920.
> 9. Register each zone with 'devm_thermal_of_zone_register()'
> based on 'sensor_count'.
>
> Reviewed-by: Henrik Grimler <henrik@grimler.se>
> Signed-off-by: Shin Son <shin.son@samsung.com>
> ---
> drivers/thermal/samsung/exynos_tmu.c | 322 ++++++++++++++++++++++++---
> 1 file changed, 285 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 47a99b3c5395..8fa188928b79 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -121,8 +121,51 @@
>
> #define EXYNOS_NOISE_CANCEL_MODE 4
>
> +/* ExynosAutov920 specific registers */
> +#define EXYNOSAUTOV920_SLOPE_COMP 25
> +#define EXYNOSAUTOV920_SLOPE_COMP_MASK 0xf
> +#define EXYNOSAUTOV920_CALIB_SEL_TEMP 30
> +#define EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK 0x2
> +
> +#define EXYNOSAUTOV920_SENSOR0_TRIM_INFO 0x10
> +#define EXYNOSAUTOV920_TRIM_MASK 0x1ff
> +#define EXYNOSAUTOV920_TRIMINFO_25_SHIFT 0
> +#define EXYNOSAUTOV920_TRIMINFO_85_SHIFT 9
> +
> +#define EXYNOSAUTOV920_TMU_REG_TRIMINFO2 0x04
> +
> +#define EXYNOSAUTOV920_TMU_REG_THRESHOLD(p) (((p)) * 0x50 + 0x00d0)
> +#define EXYNOSAUTOV920_TMU_REG_INTEN(p) (((p)) * 0x50 + 0x00f0)
> +#define EXYNOSAUTOV920_TMU_REG_INT_PEND(p) (((p)) * 0x50 + 0x00f8)
> +
> +#define EXYNOSAUTOV920_CURRENT_TEMP_P1_P0 0x084
> +#define EXYNOSAUTOV920_TMU_REG_EMUL_CON 0x0b0
> +
> +#define EXYNOSAUTOV920_TMU_REG_CONTROL 0x50
> +#define EXYNOSAUTOV920_TMU_REG_CONTROL1 0x54
> +#define EXYNOSAUTOV920_TMU_REG_AVG_CONTROL 0x58
> +#define EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL 0x70
> +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0 0x74
> +#define EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1 0x78
> +
> +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT 8
> +#define EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK 0x1f
> +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT 3
> +#define EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK 0xf
> +#define EXYNOSAUTOV920_TMU_NUM_PROBE_MASK 0xf
> +#define EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT 16
> +#define EXYNOSAUTOV920_TMU_LPI_MODE_MASK 1
> +#define EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT 10
> +
> +#define EXYNOSAUTOV920_TMU_AVG_CON_UPDATE 0x0008011a
> +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE 0x030003c0
> +#define EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE 0x03c0004d
> +
> #define MCELSIUS 1000
>
> +#define EXYNOS_DEFAULT_SENSOR_COUNT 1
> +#define EXYNOS_MAX_SENSOR_COUNT 15
> +
> enum soc_type {
> SOC_ARCH_EXYNOS3250 = 1,
> SOC_ARCH_EXYNOS4210,
> @@ -133,6 +176,7 @@ enum soc_type {
> SOC_ARCH_EXYNOS5420_TRIMINFO,
> SOC_ARCH_EXYNOS5433,
> SOC_ARCH_EXYNOS7,
> + SOC_ARCH_EXYNOSAUTOV920,
> };
>
> /**
> @@ -150,6 +194,8 @@ enum soc_type {
> * @efuse_value: SoC defined fuse value
> * @min_efuse_value: minimum valid trimming data
> * @max_efuse_value: maximum valid trimming data
> + * @slope_comp: allocated value of the slope compensation.
> + * @calib_temp: calibration temperature of the TMU.
> * @temp_error1: fused value of the first point trim.
> * @temp_error2: fused value of the second point trim.
> * @gain: gain of amplifier in the positive-TC generator block
> @@ -157,7 +203,8 @@ enum soc_type {
> * @reference_voltage: reference voltage of amplifier
> * in the positive-TC generator block
> * 0 < reference_voltage <= 31
> - * @tzd: pointer to thermal_zone_device structure
> + * @sensor_count: The maximum number of the sensors
> + * @tzd_array: pointer array of thermal_zone_device structure
> * @enabled: current status of TMU device
> * @tmu_set_low_temp: SoC specific method to set trip (falling threshold)
> * @tmu_set_high_temp: SoC specific method to set trip (rising threshold)
> @@ -174,6 +221,7 @@ struct exynos_tmu_data {
> void __iomem *base;
> void __iomem *base_second;
> int irq;
> + int sensor_count;
> enum soc_type soc;
> struct mutex lock;
> struct clk *clk, *clk_sec, *sclk;
> @@ -181,10 +229,12 @@ struct exynos_tmu_data {
> u32 efuse_value;
> u32 min_efuse_value;
> u32 max_efuse_value;
> + u16 slope_comp;
> + u16 calib_temp;
> u16 temp_error1, temp_error2;
> u8 gain;
> u8 reference_voltage;
> - struct thermal_zone_device *tzd;
> + struct thermal_zone_device *tzd_array[EXYNOS_MAX_SENSOR_COUNT];
> bool enabled;
>
> void (*tmu_set_low_temp)(struct exynos_tmu_data *data, u8 temp);
> @@ -205,13 +255,20 @@ struct exynos_tmu_data {
> */
> static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
> {
> + s32 temp_diff, code;
> +
> if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
> return temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
>
> - return (temp - EXYNOS_FIRST_POINT_TRIM) *
> - (data->temp_error2 - data->temp_error1) /
> - (EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
> - data->temp_error1;
> + temp_diff = temp - EXYNOS_FIRST_POINT_TRIM;
> +
> + code = temp_diff * (data->temp_error2 - data->temp_error1) * MCELSIUS /
> + (data->calib_temp - EXYNOS_FIRST_POINT_TRIM);
> +
> + if (data->soc == SOC_ARCH_EXYNOSAUTOV920 && temp_diff < 0)
> + code = code * (57 + data->slope_comp) / 65;
> +
> + return code / MCELSIUS + data->temp_error1;
> }
>
> /*
> @@ -220,13 +277,20 @@ static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
> */
> static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
> {
> + s32 code_diff, temp;
> +
> if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
> return temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
>
> - return (temp_code - data->temp_error1) *
> - (EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
> - (data->temp_error2 - data->temp_error1) +
> - EXYNOS_FIRST_POINT_TRIM;
> + code_diff = temp_code - data->temp_error1;
> +
> + temp = code_diff * (data->calib_temp - EXYNOS_FIRST_POINT_TRIM) * MCELSIUS /
> + (data->temp_error2 - data->temp_error1);
> +
> + if (data->soc == SOC_ARCH_EXYNOSAUTOV920 && code_diff < 0)
> + temp = temp * 65 / (57 + data->slope_comp);
No litterals, comments, etc ...
> +
> + return temp / MCELSIUS + EXYNOS_FIRST_POINT_TRIM;
> }
>
> static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
> @@ -262,6 +326,9 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> clk_enable(data->clk_sec);
>
> status = readb(data->base + EXYNOS_TMU_REG_STATUS);
> + if (data->soc == SOC_ARCH_EXYNOSAUTOV920)
> + status = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> +
> if (!status) {
> ret = -EBUSY;
> } else {
> @@ -280,27 +347,34 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> static int exynos_thermal_zone_configure(struct platform_device *pdev)
> {
> struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> - struct thermal_zone_device *tzd = data->tzd;
> - int ret, temp;
> + struct thermal_zone_device *tzd;
> + int ret, temp, idx;
>
> - ret = thermal_zone_get_crit_temp(tzd, &temp);
> - if (ret) {
> - /* FIXME: Remove this special case */
> - if (data->soc == SOC_ARCH_EXYNOS5433)
> - return 0;
> + for (idx = 0; idx < data->sensor_count; idx++) {
> + tzd = data->tzd_array[idx];
>
> - dev_err(&pdev->dev,
> - "No CRITICAL trip point defined in device tree!\n");
> - return ret;
> - }
> + if (!tzd)
> + continue;
>
> - mutex_lock(&data->lock);
> - clk_enable(data->clk);
> + ret = thermal_zone_get_crit_temp(tzd, &temp);
> + if (ret) {
> + /* FIXME: Remove this special case */
> + if (data->soc == SOC_ARCH_EXYNOS5433)
> + return 0;
>
> - data->tmu_set_crit_temp(data, temp / MCELSIUS);
> + dev_err(&pdev->dev,
> + "No CRITICAL trip point defined in device tree!\n");
> + return ret;
> + }
>
> - clk_disable(data->clk);
> - mutex_unlock(&data->lock);
> + mutex_lock(&data->lock);
> + clk_enable(data->clk);
> +
> + data->tmu_set_crit_temp(data, temp / MCELSIUS);
> +
> + clk_disable(data->clk);
> + mutex_unlock(&data->lock);
> + }
>
> return 0;
> }
> @@ -323,6 +397,37 @@ static u32 get_con_reg(struct exynos_tmu_data *data, u32 con)
> return con;
> }
>
> +static void update_con_reg(struct exynos_tmu_data *data)
> +{
> + u32 val, t_buf_vref_sel, t_buf_slope_sel;
> +
> + val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> + t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
> + & EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
> + t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
> + & EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
> +
> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> + val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> + val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> + val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> + val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> +
> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> + val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> + val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK << EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
> + val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> +
> + writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
> + writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base + EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
> +}
> +
This is unreadable; please make it understandable for those who don’t
have the documentation (explicit static inline functions, comments, etc
...).
> static void exynos_tmu_control(struct platform_device *pdev, bool on)
> {
> struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> @@ -354,9 +459,8 @@ static void exynos_tmu_update_temp(struct exynos_tmu_data *data, int reg_off,
> u16 tmu_temp_mask;
> u32 th;
>
> - tmu_temp_mask =
> - (data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_MASK
> - : EXYNOS_TMU_TEMP_MASK;
> + tmu_temp_mask = (data->soc == SOC_ARCH_EXYNOS7 || data->soc == SOC_ARCH_EXYNOSAUTOV920)
> + ? EXYNOS7_TMU_TEMP_MASK : EXYNOS_TMU_TEMP_MASK;
>
> th = readl(data->base + reg_off);
> th &= ~(tmu_temp_mask << bit_off);
> @@ -582,6 +686,68 @@ static void exynos7_tmu_initialize(struct platform_device *pdev)
> sanitize_temp_error(data, trim_info);
> }
>
> +static void exynosautov920_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
> +{
> + /*
> + * Failing thresholds are not supported on Exynosautov920.
> + * We use polling instead.
> + */
> +}
> +
> +static void exynosautov920_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
> +{
> + /*
> + * Rising thresholds are not supported on Exynosautov920.
> + * We use polling instead.
> + */
> +}
> +
> +static void exynosautov920_tmu_disable_low(struct exynos_tmu_data *data)
> +{
> + /* Again, this is handled by polling. */
> +}
> +
> +static void exynosautov920_tmu_disable_high(struct exynos_tmu_data *data)
> +{
> + /* Again, this is handled by polling. */
> +}
The driver would deserve some cleanups. Instead of having empty
callbacks, check in exynos_set_trips() if the ops is !NULL. Then remove
all no-op ops.
> +static void exynosautov920_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> +{
> + unsigned int idx;
> +
> + for (idx = 0; idx < data->sensor_count; idx++) {
> + if (!data->tzd_array[idx])
> + continue;
> +
> + exynos_tmu_update_temp(data, EXYNOSAUTOV920_TMU_REG_THRESHOLD(idx), 16, temp);
> + exynos_tmu_update_bit(data, EXYNOSAUTOV920_TMU_REG_INTEN(idx), 7, true);
> + }
> +}
There is something wrong in the driver design.
exynosautov920_tmu_set_crit_temp() is called from
exynos_thermal_zone_configure() and the routine above sets the
temperature on all the thermal zone while this one is retrieved from one
thermal zone.
Which results in:
for all tz do;
for all tz do;
if !tz then continue;
set_crit_temp(tz)
No, this driver needs to be revisited and cleanup before sending changes
for multiple sensors support.
What percentage of code sharing is there with the existing driver ?
> +static void exynosautov920_tmu_initialize(struct platform_device *pdev)
> +{
> + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> + unsigned int val;
> +
> + data->tmu_control(pdev, false);
> +
> + update_con_reg(data);
> +
> + val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> + data->cal_type = TYPE_TWO_POINT_TRIMMING;
> + data->slope_comp = (val >> EXYNOSAUTOV920_SLOPE_COMP) & EXYNOSAUTOV920_SLOPE_COMP_MASK;
> +
> + val = readl(data->base + EXYNOSAUTOV920_SENSOR0_TRIM_INFO);
> + data->temp_error1 = (val >> EXYNOSAUTOV920_TRIMINFO_25_SHIFT) & EXYNOSAUTOV920_TRIM_MASK;
> + data->temp_error2 = (val >> EXYNOSAUTOV920_TRIMINFO_85_SHIFT) & EXYNOSAUTOV920_TRIM_MASK;
> +
> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_TRIMINFO2);
> + val = (val >> EXYNOSAUTOV920_CALIB_SEL_TEMP) & EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK;
> +
> + data->calib_temp = (EXYNOS_SECOND_POINT_TRIM + (20 * val));
> +}
> +
This is unreadable; please make it understandable for those who don’t
have the documentation (explicit static inline functions, comments, etc
...).
> static void exynos4210_tmu_control(struct platform_device *pdev, bool on)
> {
> struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> @@ -633,6 +799,24 @@ static void exynos7_tmu_control(struct platform_device *pdev, bool on)
> writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
> }
>
> +static void exynosautov920_tmu_control(struct platform_device *pdev, bool on)
> +{
> + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> + unsigned int con;
> +
> + con = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> +
> + if (on) {
> + con |= BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
> + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT);
> + } else {
> + con &= ~BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
> + con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT);
> + }
> +
> + writel(con, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> +}
Document a bit the code please.
> static int exynos_get_temp(struct thermal_zone_device *tz, int *temp)
> {
> struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
> @@ -671,7 +855,7 @@ 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_EXYNOSAUTOV920) {
> val &= ~(EXYNOS7_EMUL_DATA_MASK <<
> EXYNOS7_EMUL_DATA_SHIFT);
> val |= (temp_to_code(data, temp) <<
> @@ -703,6 +887,8 @@ static void exynos4412_tmu_set_emulation(struct exynos_tmu_data *data,
> emul_con = EXYNOS5433_TMU_EMUL_CON;
> else if (data->soc == SOC_ARCH_EXYNOS7)
> emul_con = EXYNOS7_TMU_REG_EMUL_CON;
> + else if (data->soc == SOC_ARCH_EXYNOSAUTOV920)
> + emul_con = EXYNOSAUTOV920_TMU_REG_EMUL_CON;
> else
> emul_con = EXYNOS_EMUL_CON;
>
> @@ -756,11 +942,23 @@ static int exynos7_tmu_read(struct exynos_tmu_data *data)
> EXYNOS7_TMU_TEMP_MASK;
> }
>
> +static int exynosautov920_tmu_read(struct exynos_tmu_data *data)
> +{
> + return readw(data->base + EXYNOSAUTOV920_CURRENT_TEMP_P1_P0) &
> + EXYNOS7_TMU_TEMP_MASK;
> +}
> +
> static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> {
> struct exynos_tmu_data *data = id;
> + int idx;
>
> - thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
> + for (idx = 0; idx < data->sensor_count; idx++) {
> + if (!data->tzd_array[idx])
> + continue;
> +
> + thermal_zone_device_update(data->tzd_array[idx], THERMAL_EVENT_UNSPECIFIED);
I understand the main reason is to keep a common isr but you should
*not* update all the thermal zones. There is an amount of processing
behind this function adding a significant overhead.
So somehow readl(data->base + EXYNOSAUTOV920_TMU_REG_INT_PEND(idx));
should be used here to know if the thermal zone has to be updated or not.
> + }
>
> mutex_lock(&data->lock);
> clk_enable(data->clk);
> @@ -805,6 +1003,19 @@ static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
> writel(val_irq, data->base + tmu_intclear);
> }
>
> +static void exynosautov920_tmu_clear_irqs(struct exynos_tmu_data *data)
> +{
> + unsigned int idx, val_irq;
> +
> + for (idx = 0; idx < data->sensor_count; idx++) {
> + if (!data->tzd_array[idx])
> + continue;
> +
> + val_irq = readl(data->base + EXYNOSAUTOV920_TMU_REG_INT_PEND(idx));
> + writel(val_irq, data->base + EXYNOSAUTOV920_TMU_REG_INT_PEND(idx));
> + }
> +}
> +
> static const struct of_device_id exynos_tmu_match[] = {
> {
> .compatible = "samsung,exynos3250-tmu",
> @@ -833,6 +1044,9 @@ static const struct of_device_id exynos_tmu_match[] = {
> }, {
> .compatible = "samsung,exynos7-tmu",
> .data = (const void *)SOC_ARCH_EXYNOS7,
> + }, {
> + .compatible = "samsung,exynosautov920-tmu",
> + .data = (const void *)SOC_ARCH_EXYNOSAUTOV920,
Time to do cleanups in the driver. Use at your advantage the .data to
store the relevant info instead of a awful else-if in the different
functions above.
> },
> { },
> };
> @@ -865,6 +1079,10 @@ static int exynos_map_dt_data(struct platform_device *pdev)
>
> data->soc = (uintptr_t)of_device_get_match_data(&pdev->dev);
>
> + data->sensor_count = EXYNOS_DEFAULT_SENSOR_COUNT;
> +
> + data->calib_temp = EXYNOS_SECOND_POINT_TRIM;
> +
> switch (data->soc) {
> case SOC_ARCH_EXYNOS4210:
> data->tmu_set_low_temp = exynos4210_tmu_set_low_temp;
> @@ -945,6 +1163,19 @@ static int exynos_map_dt_data(struct platform_device *pdev)
> data->min_efuse_value = 15;
> data->max_efuse_value = 100;
> break;
> + case SOC_ARCH_EXYNOSAUTOV920:
> + data->tmu_set_low_temp = exynosautov920_tmu_set_low_temp;
> + data->tmu_set_high_temp = exynosautov920_tmu_set_high_temp;
> + data->tmu_disable_low = exynosautov920_tmu_disable_low;
> + data->tmu_disable_high = exynosautov920_tmu_disable_high;
> + data->tmu_set_crit_temp = exynosautov920_tmu_set_crit_temp;
> + data->tmu_initialize = exynosautov920_tmu_initialize;
> + data->tmu_control = exynosautov920_tmu_control;
> + data->tmu_read = exynosautov920_tmu_read;
> + data->tmu_set_emulation = exynos4412_tmu_set_emulation;
> + data->tmu_clear_irqs = exynosautov920_tmu_clear_irqs;
> + data->sensor_count = EXYNOS_MAX_SENSOR_COUNT;
> + break;
Same comment as above.
> default:
> dev_err(&pdev->dev, "Platform not supported\n");
> return -EINVAL;
> @@ -952,6 +1183,14 @@ static int exynos_map_dt_data(struct platform_device *pdev)
>
> data->cal_type = TYPE_ONE_POINT_TRIMMING;
>
> + if (data->soc == SOC_ARCH_EXYNOSAUTOV920) {
> + if (of_property_read_u32(pdev->dev.of_node, "samsung,sensors",
> + &data->sensor_count)) {
> + dev_err(&pdev->dev, "failed to get sensor count\n");
> + return -ENODEV;
> + }
> + }
> +
> /*
> * Check if the TMU shares some registers and then try to map the
> * memory of common registers.
> @@ -1006,7 +1245,8 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct exynos_tmu_data *data;
> - int ret;
> + struct thermal_zone_device *tzd;
> + int ret, idx;
>
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> @@ -1084,11 +1324,19 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> goto err_sclk;
> }
>
> - data->tzd = devm_thermal_of_zone_register(dev, 0, data,
> - &exynos_sensor_ops);
> - if (IS_ERR(data->tzd)) {
> - ret = dev_err_probe(dev, PTR_ERR(data->tzd), "Failed to register sensor\n");
> - goto err_sclk;
> + for (idx = 0; idx < data->sensor_count; idx++) {
> + tzd = devm_thermal_of_zone_register(dev, idx, data, &exynos_sensor_ops);
> +
> + if (IS_ERR(tzd)) {
> + if (PTR_ERR(tzd) == -ENODEV)
> + continue;
> +
> + ret = dev_err_probe(dev, PTR_ERR(data->tzd_array[idx]),
> + "Failed to register sensor\n");
> + goto err_sclk;
> + }
> +
> + data->tzd_array[idx] = tzd;
> }
>
> ret = exynos_thermal_zone_configure(pdev);
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Hello, Daniel Lezcano
> On 11/13/25 07:40, Shin Son wrote:
> > + if (data->soc == SOC_ARCH_EXYNOSAUTOV920 && code_diff < 0)
> > + temp = temp * 65 / (57 + data->slope_comp);
>
> No litterals, comments, etc ...
I'll move those fomulas into the variant data via the .data field in the of_device_id match table.
> > +static void update_con_reg(struct exynos_tmu_data *data) {
> > + u32 val, t_buf_vref_sel, t_buf_slope_sel;
> > +
> > + val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> > + t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
> > + & EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
> > + t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
> > + & EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
> > +
> > + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> > + val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK <<
> EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> > + val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> > + val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK <<
> EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> > + val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> > + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> > +
> > + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> > + val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK <<
> EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> > + val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK <<
> EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
> > + val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> > + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> > +
> > + writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
> > + writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base +
> EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
> > + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
> > + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
> > + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
> > + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
> > +}
> > +
>
> This is unreadable; please make it understandable for those who don’t have
> the documentation (explicit static inline functions, comments, etc ...).
I'll restructure this code by introducing explicit static inline helper functions and proper comments to improve readability.
> > +static void exynosautov920_tmu_disable_high(struct exynos_tmu_data
> > +*data) {
> > + /* Again, this is handled by polling. */ }
>
> The driver would deserve some cleanups. Instead of having empty callbacks,
> check in exynos_set_trips() if the ops is !NULL. Then remove all no-op ops.
Ok, I'll update exynos_set_trips() to check for NULL ops and remove the no-op callbacks accordingly.
> > +static void exynosautov920_tmu_set_crit_temp(struct exynos_tmu_data
> > +*data, u8 temp) {
> > + unsigned int idx;
> > +
> > + for (idx = 0; idx < data->sensor_count; idx++) {
> > + if (!data->tzd_array[idx])
> > + continue;
> > +
> > + exynos_tmu_update_temp(data,
> EXYNOSAUTOV920_TMU_REG_THRESHOLD(idx), 16, temp);
> > + exynos_tmu_update_bit(data,
> EXYNOSAUTOV920_TMU_REG_INTEN(idx), 7, true);
> > + }
> > +}
>
> There is something wrong in the driver design.
>
> exynosautov920_tmu_set_crit_temp() is called from
> exynos_thermal_zone_configure() and the routine above sets the temperature
> on all the thermal zone while this one is retrieved from one thermal zone.
>
> Which results in:
>
> for all tz do;
> for all tz do;
> if !tz then continue;
> set_crit_temp(tz)
>
> No, this driver needs to be revisited and cleanup before sending changes
> for multiple sensors support.
>
> What percentage of code sharing is there with the existing driver ?
Overall, I would say that roughly 60% of the logic can be shared.
The temperature reading and emulation paths are similar, but the initialization sequence differs significantly.
Given this level of divergence, would introducing a separate driver
(instead of extending the current one with many special-case paths) be acceptable?
> > +static void exynosautov920_tmu_initialize(struct platform_device
> > +*pdev) {
> > + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > + unsigned int val;
> > +
> > + data->tmu_control(pdev, false);
> > +
> > + update_con_reg(data);
> > +
> > + val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> > + data->cal_type = TYPE_TWO_POINT_TRIMMING;
> > + data->slope_comp = (val >> EXYNOSAUTOV920_SLOPE_COMP) &
> > +EXYNOSAUTOV920_SLOPE_COMP_MASK;
> > +
> > + val = readl(data->base + EXYNOSAUTOV920_SENSOR0_TRIM_INFO);
> > + data->temp_error1 = (val >> EXYNOSAUTOV920_TRIMINFO_25_SHIFT) &
> EXYNOSAUTOV920_TRIM_MASK;
> > + data->temp_error2 = (val >> EXYNOSAUTOV920_TRIMINFO_85_SHIFT) &
> > +EXYNOSAUTOV920_TRIM_MASK;
> > +
> > + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_TRIMINFO2);
> > + val = (val >> EXYNOSAUTOV920_CALIB_SEL_TEMP) &
> > +EXYNOSAUTOV920_CALIB_SEL_TEMP_MASK;
> > +
> > + data->calib_temp = (EXYNOS_SECOND_POINT_TRIM + (20 * val)); }
> > +
>
> This is unreadable; please make it understandable for those who don’t have
> the documentation (explicit static inline functions, comments, etc ...).
Ok, I'll refactor this code using explicit static inline helpers and comments.
> > +static void exynosautov920_tmu_control(struct platform_device *pdev,
> > +bool on) {
> > + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > + unsigned int con;
> > +
> > + con = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> > +
> > + if (on) {
> > + con |= BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
> > + con |= BIT(EXYNOS_TMU_CORE_EN_SHIFT);
> > + } else {
> > + con &= ~BIT(EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
> > + con &= ~BIT(EXYNOS_TMU_CORE_EN_SHIFT);
> > + }
> > +
> > + writel(con, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL); }
>
> Document a bit the code please.
Sure, I’ll document this part properly by adding clear comments and splitting the register options into explicit helper functions.
> > static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
> > {
> > struct exynos_tmu_data *data = id;
> > + int idx;
> >
> > - thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
> > + for (idx = 0; idx < data->sensor_count; idx++) {
> > + if (!data->tzd_array[idx])
> > + continue;
> > +
> > + thermal_zone_device_update(data->tzd_array[idx],
> > +THERMAL_EVENT_UNSPECIFIED);
> I understand the main reason is to keep a common isr but you should
> *not* update all the thermal zones. There is an amount of processing
> behind this function adding a significant overhead.
>
> So somehow readl(data->base + EXYNOSAUTOV920_TMU_REG_INT_PEND(idx));
> should be used here to know if the thermal zone has to be updated or not.
OK, I'll update the ISR so that it checks the pending register before calling 'thermal_zone_device_update()',
And only update the relevant thermal zones.
> > static const struct of_device_id exynos_tmu_match[] = {
> > {
> > .compatible = "samsung,exynos3250-tmu", @@ -833,6 +1044,9 @@
> > static const struct of_device_id exynos_tmu_match[] = {
> > }, {
> > .compatible = "samsung,exynos7-tmu",
> > .data = (const void *)SOC_ARCH_EXYNOS7,
> > + }, {
> > + .compatible = "samsung,exynosautov920-tmu",
> > + .data = (const void *)SOC_ARCH_EXYNOSAUTOV920,
>
> Time to do cleanups in the driver. Use at your advantage the .data to
> store the relevant info instead of a awful else-if in the different
> functions above.
OK, I'll refactor this by using the .data field.
However, since ExynosAutov920 diverges significantly from the existing driver,
Would introducing a separate driver instead of unifying everything be acceptable?
> > },
> > { },
> > };
> > @@ -865,6 +1079,10 @@ static int exynos_map_dt_data(struct
> > platform_device *pdev)
> >
> > data->soc = (uintptr_t)of_device_get_match_data(&pdev->dev);
> >
> > + data->sensor_count = EXYNOS_DEFAULT_SENSOR_COUNT;
> > +
> > + data->calib_temp = EXYNOS_SECOND_POINT_TRIM;
> > +
> > switch (data->soc) {
> > case SOC_ARCH_EXYNOS4210:
> > data->tmu_set_low_temp = exynos4210_tmu_set_low_temp; @@ -
> 945,6
> > +1163,19 @@ static int exynos_map_dt_data(struct platform_device *pdev)
> > data->min_efuse_value = 15;
> > data->max_efuse_value = 100;
> > break;
> > + case SOC_ARCH_EXYNOSAUTOV920:
> > + data->tmu_set_low_temp = exynosautov920_tmu_set_low_temp;
> > + data->tmu_set_high_temp = exynosautov920_tmu_set_high_temp;
> > + data->tmu_disable_low = exynosautov920_tmu_disable_low;
> > + data->tmu_disable_high = exynosautov920_tmu_disable_high;
> > + data->tmu_set_crit_temp = exynosautov920_tmu_set_crit_temp;
> > + data->tmu_initialize = exynosautov920_tmu_initialize;
> > + data->tmu_control = exynosautov920_tmu_control;
> > + data->tmu_read = exynosautov920_tmu_read;
> > + data->tmu_set_emulation = exynos4412_tmu_set_emulation;
> > + data->tmu_clear_irqs = exynosautov920_tmu_clear_irqs;
> > + data->sensor_count = EXYNOS_MAX_SENSOR_COUNT;
> > + break;
>
> Same comment as above.
Ok, I'll refactor this by using the .data field to move the SoC-specific callbacks into a proper
variant structure.
> --
Thank you for your detailed feedback. I appreciate it.
Hi, Shin,
On 11/24/25 12:06 PM, 손신 wrote:
>>> +static void update_con_reg(struct exynos_tmu_data *data) {
>>> + u32 val, t_buf_vref_sel, t_buf_slope_sel;
>>> +
>>> + val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
>>> + t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
>>> + & EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
>>> + t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
>>> + & EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
>>> +
>>> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
>>> + val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK <<
>> EXYNOS_TMU_REF_VOLTAGE_SHIFT);
>>> + val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
>>> + val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK <<
>> EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
>>> + val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
>>> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
>>> +
>>> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
>>> + val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK <<
>> EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
>>> + val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK <<
>> EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
>>> + val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
>>> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
>>> +
>>> + writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
>>> + writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base +
>> EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
>>> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
>>> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
>>> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
>>> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
>>> +}
>>> +
>> This is unreadable; please make it understandable for those who don’t have
>> the documentation (explicit static inline functions, comments, etc ...).
> I'll restructure this code by introducing explicit static inline helper functions and proper comments to improve readability.
We likely shouldn't use inlines here, see Linus's reply from 2006:
https://lore.kernel.org/all/Pine.LNX.4.64.0601021105000.3668@g5.osdl.org/T/#u
I guess you can make this easier to read if you use FIELD_GET/SET from
bitfield.h. Other improvement would be using the regmap api.
Shin, a bit unrelated with the patch, but I wanted to let you know that
I started looking at the GS101 TMU. I assume it's very similar with the
TMU on exynosautov920. Do you know if they share the same IP version?
I noticed GS101 uses ACPM calls to communicate with the TMU. Why did you
choose to not use ACPM for exynosautov920 TMU?
Thanks!
ta
Hello, Tudor Ambarus.
> -----Original Message-----
> From: Tudor Ambarus [mailto:tudor.ambarus@linaro.org]
> Sent: Monday, November 24, 2025 7:43 PM
> To: 손신 <shin.son@samsung.com>; 'Daniel Lezcano'
> <daniel.lezcano@linaro.org>; 'Bartlomiej Zolnierkiewicz'
> <bzolnier@gmail.com>; 'Krzysztof Kozlowski' <krzk@kernel.org>; 'Rafael J .
> Wysocki' <rafael@kernel.org>; 'Zhang Rui' <rui.zhang@intel.com>; 'Lukasz
> Luba' <lukasz.luba@arm.com>; 'Rob Herring' <robh@kernel.org>; 'Conor
> Dooley' <conor+dt@kernel.org>; 'Alim Akhtar' <alim.akhtar@samsung.com>
> Cc: 'Henrik Grimler' <henrik@grimler.se>; linux-pm@vger.kernel.org; linux-
> samsung-soc@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Peter Griffin
> <peter.griffin@linaro.org>; André Draszik <andre.draszik@linaro.org>;
> William McVicker <willmcvicker@google.com>; jyescas@google.com
> Subject: Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new
> hardware and update TMU interface
>
> Hi, Shin,
>
> On 11/24/25 12:06 PM, 손신 wrote:
> >>> +static void update_con_reg(struct exynos_tmu_data *data) {
> >>> + u32 val, t_buf_vref_sel, t_buf_slope_sel;
> >>> +
> >>> + val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> >>> + t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
> >>> + & EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
> >>> + t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
> >>> + & EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
> >>> +
> >>> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> >>> + val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK <<
> >> EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> >>> + val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> >>> + val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK <<
> >> EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> >>> + val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
> >>> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
> >>> +
> >>> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> >>> + val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK <<
> >> EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> >>> + val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK <<
> >> EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
> >>> + val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
> >>> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
> >>> +
> >>> + writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
> >>> + writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base +
> >> EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
> >>> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
> >>> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
> >>> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
> >>> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
> >>> +}
> >>> +
> >> This is unreadable; please make it understandable for those who don’t
> >> have the documentation (explicit static inline functions, comments,
> etc ...).
> > I'll restructure this code by introducing explicit static inline helper
> functions and proper comments to improve readability.
>
> We likely shouldn't use inlines here, see Linus's reply from 2006:
> https://lore.kernel.org/all/Pine.LNX.4.64.0601021105000.3668@g5.osdl.org/T
> /#u
>
> I guess you can make this easier to read if you use FIELD_GET/SET from
> bitfield.h. Other improvement would be using the regmap api.
>
> Shin, a bit unrelated with the patch, but I wanted to let you know that I
> started looking at the GS101 TMU. I assume it's very similar with the TMU
> on exynosautov920. Do you know if they share the same IP version?
>
> I noticed GS101 uses ACPM calls to communicate with the TMU. Why did you
> choose to not use ACPM for exynosautov920 TMU?
>
> Thanks!
> ta
I will adopt FIELD_GET/FIELD_PREP to simplify the bitfield handling, and I will avoid using inline functions excessively as suggested.
Regarding ACPM, I did not introduce it earlier because I was trying to align the implementation with the existing framework.
However, if we move toward a separate driver, I will reconsider whether ACPM integration makes sense there.
Would it be possible to get your feedback again when I prepare the next revision of the driver?
Plus, the GS101 TMU driver isn't upstream yet, right?
Could you share where I can find the example code you mentioned? Thank you in advance.
Best regards,
Shin
Hi, Shin Son, On 11/24/25 1:41 PM, 손신 wrote: >> Shin, a bit unrelated with the patch, but I wanted to let you know that I >> started looking at the GS101 TMU. I assume it's very similar with the TMU >> on exynosautov920. Do you know if they share the same IP version? I guess you omitted this question. >> >> I noticed GS101 uses ACPM calls to communicate with the TMU. Why did you >> choose to not use ACPM for exynosautov920 TMU? cut > Regarding ACPM, I did not introduce it earlier because I was trying to align the implementation with the existing framework. > However, if we move toward a separate driver, I will reconsider whether ACPM integration makes sense there. > Would it be possible to get your feedback again when I prepare the next revision of the driver? Yes, I'll try to review it. Add me to cc please. > > Plus, the GS101 TMU driver isn't upstream yet, right? It isn't. I started getting familiar with it, and will try to upstream it. Given exynosautov9 and gs101 already share lots of IPs, I assume TMU is similar. I will likely follow the ACPM route because that's what the downstream code does. > Could you share where I can find the example code you mentioned? Thank you in advance. Are you referring to the GS101 TMU driver code? Here it is: https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/drivers/thermal/samsung/gs_tmu_v2.c DT at: https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101.dtsi#1453 Is the downstream exynosautov9 code publicly available? Can you provide some links? Thanks! ta
Hello, Tudor Ambarus. > -----Original Message----- > From: Tudor Ambarus [mailto:tudor.ambarus@linaro.org] > Sent: Monday, November 24, 2025 9:24 PM > To: 손신 <shin.son@samsung.com>; 'Daniel Lezcano' > <daniel.lezcano@linaro.org>; 'Bartlomiej Zolnierkiewicz' > <bzolnier@gmail.com>; 'Krzysztof Kozlowski' <krzk@kernel.org>; 'Rafael J . > Wysocki' <rafael@kernel.org>; 'Zhang Rui' <rui.zhang@intel.com>; 'Lukasz > Luba' <lukasz.luba@arm.com>; 'Rob Herring' <robh@kernel.org>; 'Conor > Dooley' <conor+dt@kernel.org>; 'Alim Akhtar' <alim.akhtar@samsung.com> > Cc: 'Henrik Grimler' <henrik@grimler.se>; linux-pm@vger.kernel.org; linux- > samsung-soc@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; 'Peter Griffin' > <peter.griffin@linaro.org>; 'André Draszik' <andre.draszik@linaro.org>; > 'William McVicker' <willmcvicker@google.com>; jyescas@google.com > Subject: Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new > hardware and update TMU interface > > Hi, Shin Son, > > On 11/24/25 1:41 PM, 손신 wrote: > >> Shin, a bit unrelated with the patch, but I wanted to let you know > >> that I started looking at the GS101 TMU. I assume it's very similar > >> with the TMU on exynosautov920. Do you know if they share the same IP > version? > > I guess you omitted this question. > Oh, Sorry - I missed that question earlier. I do see many similarities, But I'm not sure whether Exynosautov920 actually share the same IP version as GS101. > > Regarding ACPM, I did not introduce it earlier because I was trying to > align the implementation with the existing framework. > > However, if we move toward a separate driver, I will reconsider whether > ACPM integration makes sense there. > > Would it be possible to get your feedback again when I prepare the next > revision of the driver? > > Yes, I'll try to review it. Add me to cc please. Thanks a lot! > Is the downstream exynosautov9 code publicly available? Can you provide > some links? > > Thanks! > ta Thank you for sharing the links. As far as I know, the downstream eav9 code is not publicly available. In the next patch series, I'll make sure to add you to CC. I appreciate your help and feedback. Thanks, Shin
Hi Tudor,
On 11/24/25 11:43, Tudor Ambarus wrote:
> Hi, Shin,
>
> On 11/24/25 12:06 PM, 손신 wrote:
>>>> +static void update_con_reg(struct exynos_tmu_data *data) {
>>>> + u32 val, t_buf_vref_sel, t_buf_slope_sel;
>>>> +
>>>> + val = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
>>>> + t_buf_vref_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_SHIFT)
>>>> + & EXYNOSAUTOV920_TMU_T_BUF_VREF_SEL_MASK;
>>>> + t_buf_slope_sel = (val >> EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_SHIFT)
>>>> + & EXYNOSAUTOV920_TMU_T_BUF_SLOPE_SEL_MASK;
>>>> +
>>>> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
>>>> + val &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK <<
>>> EXYNOS_TMU_REF_VOLTAGE_SHIFT);
>>>> + val |= (t_buf_vref_sel << EXYNOS_TMU_REF_VOLTAGE_SHIFT);
>>>> + val &= ~(EXYNOS_TMU_BUF_SLOPE_SEL_MASK <<
>>> EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
>>>> + val |= (t_buf_slope_sel << EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT);
>>>> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL);
>>>> +
>>>> + val = readl(data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
>>>> + val &= ~(EXYNOSAUTOV920_TMU_NUM_PROBE_MASK <<
>>> EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
>>>> + val &= ~(EXYNOSAUTOV920_TMU_LPI_MODE_MASK <<
>>> EXYNOSAUTOV920_TMU_LPI_MODE_SHIFT);
>>>> + val |= (data->sensor_count << EXYNOSAUTOV920_TMU_NUM_PROBE_SHIFT);
>>>> + writel(val, data->base + EXYNOSAUTOV920_TMU_REG_CONTROL1);
>>>> +
>>>> + writel(1, data->base + EXYNOSAUTOV920_TMU_SAMPLING_INTERVAL);
>>>> + writel(EXYNOSAUTOV920_TMU_AVG_CON_UPDATE, data->base +
>>> EXYNOSAUTOV920_TMU_REG_AVG_CONTROL);
>>>> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE0_UPDATE,
>>>> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE0);
>>>> + writel(EXYNOSAUTOV920_TMU_COUNTER_VALUE1_UPDATE,
>>>> + data->base + EXYNOSAUTOV920_TMU_REG_COUNTER_VALUE1);
>>>> +}
>>>> +
>>> This is unreadable; please make it understandable for those who don’t have
>>> the documentation (explicit static inline functions, comments, etc ...).
>> I'll restructure this code by introducing explicit static inline helper functions and proper comments to improve readability.
>
> We likely shouldn't use inlines here, see Linus's reply from 2006:
> https://lore.kernel.org/all/Pine.LNX.4.64.0601021105000.3668@g5.osdl.org/T/#u
We should not use inline functions when they can be called from
different places and if they contain a significant amount of code inside.
But for one line functions, inside a driver it may help for the
readability when the function name is self-explanatory.
> I guess you can make this easier to read if you use FIELD_GET/SET from
> bitfield.h. Other improvement would be using the regmap api.
regmap would be too overkill to write unshared registers.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 11/24/25 11:06, 손신 wrote: [ ... ] > However, since ExynosAutov920 diverges significantly from the existing driver, > Would introducing a separate driver instead of unifying everything be acceptable? So this driver is one controller for multiple sensors while the others drivers are one controller for one sensor, right ? -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
Hello, Daniel Lezcano. > On 11/24/25 11:06, 손신 wrote: > [ ... ] > > > However, since ExynosAutov920 diverges significantly from the existing > > driver, Would introducing a separate driver instead of unifying > everything be acceptable? > > So this driver is one controller for multiple sensors while the others > drivers are one controller for one sensor, right ? > Yes. As far as I understand, the previous Exynos variants used one TMU controller per sensor, while on ExynosAutoV920 the hardware has multiple TMU instances and each instance contains multiple sensors. Therefore, this new automotive SoC requires supporting multiple sensors behind a single TMU controller. Best regards, Shin
On 11/24/25 12:04, 손신 wrote: > Hello, Daniel Lezcano. > >> On 11/24/25 11:06, 손신 wrote: >> [ ... ] >> >>> However, since ExynosAutov920 diverges significantly from the existing >>> driver, Would introducing a separate driver instead of unifying >> everything be acceptable? >> >> So this driver is one controller for multiple sensors while the others >> drivers are one controller for one sensor, right ? >> > > Yes. As far as I understand, the previous Exynos variants used one TMU controller per sensor, > while on ExynosAutoV920 the hardware has multiple TMU instances and each instance contains multiple sensors. > Therefore, this new automotive SoC requires supporting multiple sensors behind a single TMU controller. Ok thanks. It makes sense to create a separate driver. -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
Hello, Daniel Lezcano. > -----Original Message----- > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] > Sent: Monday, November 24, 2025 8:09 PM > To: 손신 <shin.son@samsung.com>; 'Bartlomiej Zolnierkiewicz' > <bzolnier@gmail.com>; 'Krzysztof Kozlowski' <krzk@kernel.org>; 'Rafael J . > Wysocki' <rafael@kernel.org>; 'Zhang Rui' <rui.zhang@intel.com>; 'Lukasz > Luba' <lukasz.luba@arm.com>; 'Rob Herring' <robh@kernel.org>; 'Conor > Dooley' <conor+dt@kernel.org>; 'Alim Akhtar' <alim.akhtar@samsung.com> > Cc: 'Henrik Grimler' <henrik@grimler.se>; linux-pm@vger.kernel.org; linux- > samsung-soc@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new > hardware and update TMU interface > > On 11/24/25 12:04, 손신 wrote: > > Hello, Daniel Lezcano. > > > >> On 11/24/25 11:06, 손신 wrote: > >> [ ... ] > >> > >>> However, since ExynosAutov920 diverges significantly from the > >>> existing driver, Would introducing a separate driver instead of > >>> unifying > >> everything be acceptable? > >> > >> So this driver is one controller for multiple sensors while the > >> others drivers are one controller for one sensor, right ? > >> > > > > Yes. As far as I understand, the previous Exynos variants used one TMU > > controller per sensor, while on ExynosAutoV920 the hardware has multiple > TMU instances and each instance contains multiple sensors. > > Therefore, this new automotive SoC requires supporting multiple sensors > behind a single TMU controller. > > Ok thanks. It makes sense to create a separate driver. Then, I'll prepare a new patch series. Thank you for your feedback Best regards, Shin
© 2016 - 2025 Red Hat, Inc.