lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250904083745.GA33254@l14.localdomain>
Date: Thu, 4 Sep 2025 10:37:45 +0200
From: Henrik Grimler <henrik@...mler.se>
To: Shin Son <shin.son@...sung.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
	Krzysztof Kozlowski <krzk@...nel.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Alim Akhtar <alim.akhtar@...sung.com>, linux-pm@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] thermal: exynos_tmu: Support new hardware and
 update TMU interface

Hi Shin,

On Wed, Sep 03, 2025 at 04:36:33PM +0900, 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 dual-zone handling:
> 
> - Add 'slope_comp' : compensation parameter below 25 degrees.
> - Add 'calib_temp' : stores the fused calibaration temperature.
> - Add 'tz_count' : reflects the new 1:2 hardware-to-thermal-zone ratio.
> - Add 'valid_sensor_bitmap' : bitmap to enable interrupts
> 			      for each valid sensor.
> - 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. Branch 'code_to_temp' and 'temp_to_code' for exynosautov920 SoC variant.
> 2. Loop over 'tz_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 bitmap in 'exynosautov920_tmu_set_crit_temp'.
> 7. Initialize all new members during 'exynosautov920_tmu_initialize'.
> 8. Clear IRQs by iterating the bitamp in exynosautov920.
> 9. Register each zone with 'devm_thermal_of_zone_register()'
>    based on 'tz_count'.
> 
> Signed-off-by: Shin Son <shin.son@...sung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 340 ++++++++++++++++++++++++---
>  1 file changed, 303 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 47a99b3c5395..60d5ab33c593 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c

[ ... ]

> +#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_THERM_TRIP_EN_SHIFT	12

There already is a EXYNOS_TMU_THERM_TRIP_EN_SHIFT constant with the
same value. Is there some fundamental difference between
EXYNOSAUTOV920_TMU_THERM_TRIP_EN_SHIFT and
EXYNOS_TMU_THERM_TRIP_EN_SHIFT?

> +#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

If I am not mistaken lowercase letters is preferred in defines. The
file already has a mix, but let's not make it worse. Please change to
0x03c0004d and so on in constants above.

>  #define MCELSIUS	1000
>  
> +#define EXYNOS_DEFAULT_TZ_COUNT		1
> +#define EXYNOS_MAX_TZ_COUNT		2
> +
>  enum soc_type {
>  	SOC_ARCH_EXYNOS3250 = 1,
>  	SOC_ARCH_EXYNOS4210,
> @@ -133,6 +179,7 @@ enum soc_type {
>  	SOC_ARCH_EXYNOS5420_TRIMINFO,
>  	SOC_ARCH_EXYNOS5433,
>  	SOC_ARCH_EXYNOS7,
> +	SOC_ARCH_EXYNOSAUTOV920,
>  };
>  
>  /**
> @@ -150,6 +197,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 +206,9 @@ 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
> + * @tz_count: The allocated number of the thermal zone
> + * @tzd_array: pointer array of thermal_zone_device structure
> + * @valid_sensor_bitmap: The enabled sensor of the TMU device
>   * @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)
> @@ -181,10 +232,14 @@ 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;
> +	u8 tz_count;
> +	unsigned long valid_sensor_bitmap;
> +	struct thermal_zone_device *tzd_array[EXYNOS_MAX_TZ_COUNT];
>  	bool enabled;
>  
>  	void (*tmu_set_low_temp)(struct exynos_tmu_data *data, u8 temp);
> @@ -208,10 +263,25 @@ static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
>  	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;
> +	if (data->soc == SOC_ARCH_EXYNOSAUTOV920) {
> +		if ((temp - EXYNOS_FIRST_POINT_TRIM) >= 0) {
> +			return (temp - EXYNOS_FIRST_POINT_TRIM) *
> +				(data->temp_error2 - data->temp_error1) /
> +				(data->calib_temp - EXYNOS_FIRST_POINT_TRIM) +
> +				data->temp_error1;
> +		} else {
> +			return ((temp - EXYNOS_FIRST_POINT_TRIM) *
> +				(data->temp_error2 - data->temp_error1) /
> +				(data->calib_temp - EXYNOS_FIRST_POINT_TRIM) *
> +				((57 + data->slope_comp) * 1000 / 65)) / 1000 +
> +				data->temp_error1;
> +		}
> +	} else {
> +		return (temp - EXYNOS_FIRST_POINT_TRIM) *
> +			(data->temp_error2 - data->temp_error1) /
> +			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
> +			data->temp_error1;

This is essentially the same as the first return in the
SOC_ARCH_EXYNOSAUTOV920 path. How about putting
EXYNOS_SECOND_POINT_TRIM in the calib_temp field for the non autov920
SoCs, then we can simplify temp_to_code and code_to_temp to something
more readable like:

static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
{
	if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
		return temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;

	int coeff = (data->temp_error2 - data->temp_error1) /
			(data->calib_temp - EXYNOS_FIRST_POINT_TRIM);

	if (data->soc == SOC_ARCH_EXYNOSAUTOV920 &&
	    temp < EXYNOS_FIRST_POINT_TRIM)
		coeff *= (57 + data->slope_comp) * 1000 / 65)) / 1000;

	return (temp - EXYNOS_FIRST_POINT_TRIM) * coeff + data->temp_error1;
}

>  }
>  
>  /*
> @@ -223,10 +293,25 @@ static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
>  	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;
> +	if (data->soc == SOC_ARCH_EXYNOSAUTOV920) {
> +		if ((temp_code - data->temp_error1) >= 0) {
> +			return (temp_code - data->temp_error1) *
> +				(data->calib_temp - EXYNOS_FIRST_POINT_TRIM) /
> +				(data->temp_error2 - data->temp_error1) +
> +				EXYNOS_FIRST_POINT_TRIM;
> +		} else {
> +			return ((temp_code - data->temp_error1) *
> +				(data->calib_temp - EXYNOS_FIRST_POINT_TRIM) /
> +				(data->temp_error2 - data->temp_error1) *
> +				(65 * 1000 / (57 + data->slope_comp))) / 1000 +
> +				EXYNOS_FIRST_POINT_TRIM;
> +		}
> +	} else {
> +		return (temp_code - data->temp_error1) *
> +			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
> +			(data->temp_error2 - data->temp_error1) +
> +			EXYNOS_FIRST_POINT_TRIM;
> +	}

Similar suggestion as for temp_to_code applies here as well.

Best regards,
Henrik Grimler

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ