RE: [PATCH v7 RESEND 2/3] thermal: exynos_tmu: Support new hardware and update TMU interface

[손신 <shin.son@samsung.com>]

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