Re: [PATCH 1/3] thermal: exynos: Add the support for Exynos5433 TMU

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Lukasz,

On 03/04/2015 07:38 PM, Lukasz Majewski wrote:
> Hi Chanwoo,
> 
>> This patch adds the support for Exynos5433's TMU (Thermal Management
>> Unit). Exynos5433 has a little different register bit fields as
>> following description:
>> - Support the eight trip points for rising/falling interrupt by using
>> two registers
>> - Read the calibration type (1-point or 2-point) and sensor id from
>> TRIMINFO register
>> - Use a little different register address
>>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/thermal/samsung/exynos_tmu.c | 161
>> +++++++++++++++++++++++++++++++++--
>> drivers/thermal/samsung/exynos_tmu.h |   1 + 2 files changed, 157
>> insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/thermal/samsung/exynos_tmu.c
>> b/drivers/thermal/samsung/exynos_tmu.c index 1d30b09..1bb2fb7 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.c
>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>> @@ -97,6 +97,32 @@
>>  #define EXYNOS4412_MUX_ADDR_VALUE          6
>>  #define EXYNOS4412_MUX_ADDR_SHIFT          20
>>  
>> +/* Exynos5433 specific registers */
>> +#define EXYNOS5433_TMU_REG_CONTROL1		0x024
>> +#define EXYNOS5433_TMU_SAMPLING_INTERVAL	0x02c
>> +#define EXYNOS5433_TMU_COUNTER_VALUE0		0x030
>> +#define EXYNOS5433_TMU_COUNTER_VALUE1		0x034
>> +#define EXYNOS5433_TMU_REG_CURRENT_TEMP1	0x044
>> +#define EXYNOS5433_THD_TEMP_RISE3_0		0x050
>> +#define EXYNOS5433_THD_TEMP_RISE7_4		0x054
>> +#define EXYNOS5433_THD_TEMP_FALL3_0		0x060
>> +#define EXYNOS5433_THD_TEMP_FALL7_4		0x064
>> +#define EXYNOS5433_TMU_REG_INTEN		0x0c0
>> +#define EXYNOS5433_TMU_REG_INTPEND		0x0c8
>> +#define EXYNOS5433_TMU_EMUL_CON			0x110
>> +#define EXYNOS5433_TMU_PD_DET_EN		0x130
>> +
>> +#define EXYNOS5433_TRIMINFO_SENSOR_ID_SHIFT	16
>> +#define EXYNOS5433_TRIMINFO_CALIB_SEL_SHIFT	23
>> +#define EXYNOS5433_TRIMINFO_SENSOR_ID_MASK	\
>> +			(0xf << EXYNOS5433_TRIMINFO_SENSOR_ID_SHIFT)
>> +#define EXYNOS5433_TRIMINFO_CALIB_SEL_MASK	BIT(23)
>> +
>> +#define EXYNOS5433_TRIMINFO_ONE_POINT_TRIMMING	0
>> +#define EXYNOS5433_TRIMINFO_TWO_POINT_TRIMMING	1
>> +
>> +#define EXYNOS5433_PD_DET_EN			1
>> +
>>  /*exynos5440 specific registers*/
>>  #define EXYNOS5440_TMU_S0_7_TRIM		0x000
>>  #define EXYNOS5440_TMU_S0_7_CTRL		0x020
>> @@ -484,6 +510,101 @@ out:
>>  	return ret;
>>  }
>>  
>> +static int exynos5433_tmu_initialize(struct platform_device *pdev)
>> +{
>> +	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>> +	struct exynos_tmu_platform_data *pdata = data->pdata;
>> +	struct thermal_zone_device *tz = data->tzd;
>> +	unsigned int status, trim_info;
>> +	unsigned int rising_threshold = 0, falling_threshold = 0;
>> +	unsigned long temp, temp_hist;
>> +	int ret = 0, threshold_code, i, sensor_id, cal_type;
>> +
>> +	status = readb(data->base + EXYNOS_TMU_REG_STATUS);
>> +	if (!status) {
>> +		ret = -EBUSY;
>> +		goto out;
>> +	}
>> +
>> +	trim_info = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
>> +	sanitize_temp_error(data, trim_info);
>> +
>> +	/* Read the temperature sensor id */
>> +	sensor_id = (trim_info & EXYNOS5433_TRIMINFO_SENSOR_ID_MASK)
>> +				>>
>> EXYNOS5433_TRIMINFO_SENSOR_ID_SHIFT;
>> +	dev_info(&pdev->dev, "Temperature sensor ID: 0x%x\n",
>> sensor_id); +
> 
> 	Isn't dev_info a bit too noisy? IMHO, dev_dbg would be enough
> 	here.
> 
> 	Please consider this globally.

OK, I'll use dev_dbg.

> 
>> +	/* Read the calibration mode */
>> +	writel(trim_info, data->base + EXYNOS_TMU_REG_TRIMINFO);
>> +	cal_type = (trim_info & EXYNOS5433_TRIMINFO_CALIB_SEL_MASK)
>> +				>>
>> EXYNOS5433_TRIMINFO_CALIB_SEL_SHIFT; +
>> +	switch (cal_type) {
>> +	case EXYNOS5433_TRIMINFO_ONE_POINT_TRIMMING:
>> +		pdata->cal_type = TYPE_ONE_POINT_TRIMMING;
>> +		break;
>> +	case EXYNOS5433_TRIMINFO_TWO_POINT_TRIMMING:
>> +		pdata->cal_type = TYPE_TWO_POINT_TRIMMING;
>> +		break;
>> +	default:
>> +		pdata->cal_type = TYPE_ONE_POINT_TRIMMING;
>> +		break;
>> +	};
>> +
>> +	dev_info(&pdev->dev, "Calibration type is %d-point
>> calibration\n",
>> +			cal_type ?  2 : 1);
>> +
>> +	/* Write temperature code for rising and falling threshold */
>> +	for (i = 0; i < of_thermal_get_ntrips(tz); i++) {
>> +		int rising_reg_offset, falling_reg_offset;
>> +		int j = 0;
>> +
>> +		switch (i) {
>> +		case 0:
>> +		case 1:
>> +		case 2:
>> +		case 3:
>> +			rising_reg_offset =
>> EXYNOS5433_THD_TEMP_RISE3_0;
>> +			falling_reg_offset =
>> EXYNOS5433_THD_TEMP_FALL3_0;
>> +			j = i;
>> +			break;
>> +		case 4:
>> +		case 5:
>> +		case 6:
>> +		case 7:
>> +			rising_reg_offset =
>> EXYNOS5433_THD_TEMP_RISE7_4;
>> +			falling_reg_offset =
>> EXYNOS5433_THD_TEMP_FALL7_4;
>> +			j = i - 4;
>> +			break;
>> +		default:
>> +			continue;
>> +		}
>> +
>> +		/* Write temperature code for rising threshold */
>> +		tz->ops->get_trip_temp(tz, i, &temp);
>> +		temp /= MCELSIUS;
>> +		threshold_code = temp_to_code(data, temp);
>> +
>> +		rising_threshold = readl(data->base +
>> rising_reg_offset);
>> +		rising_threshold |= (threshold_code << j * 8);
>> +		writel(rising_threshold, data->base +
>> rising_reg_offset); +
>> +		/* Write temperature code for falling threshold */
>> +		tz->ops->get_trip_hyst(tz, i, &temp_hist);
>> +		temp_hist = temp - (temp_hist / MCELSIUS);
>> +		threshold_code = temp_to_code(data, temp_hist);
>> +
>> +		falling_threshold = readl(data->base +
>> falling_reg_offset);
>> +		falling_threshold &= ~(0xff << j * 8);
>> +		falling_threshold |= (threshold_code << j * 8);
>> +		writel(falling_threshold, data->base +
>> falling_reg_offset);
>> +	}
>> +
>> +	data->tmu_clear_irqs(data);
>> +out:
>> +	return ret;
>> +}
>> +
>>  static int exynos5440_tmu_initialize(struct platform_device *pdev)
>>  {
>>  	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>> @@ -682,7 +803,8 @@ static void exynos7_tmu_control(struct
>> platform_device *pdev, bool on) 
>>  	if (on) {
>>  		con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
>> -		con |= (1 << EXYNOS7_PD_DET_EN_SHIFT);
>> +		if (data->soc == SOC_ARCH_EXYNOS7)
>> +			con |= (1 << EXYNOS7_PD_DET_EN_SHIFT);
> 
> 	Isn't exynos7 implying that we already have SOC_ARCH_EXYNOS7?
> 	Why do we need this extra check?

On this patch, Exynos5433 TMU use the exynos7_tmu_control function.
But, as below your comment, I'll add the separate function for Exynos5433.

> 
>>  		interrupt_en =
>>  			(of_thermal_is_trip_valid(tz, 7)
>>  			<< EXYNOS7_TMU_INTEN_RISE7_SHIFT) |
>> @@ -705,11 +827,20 @@ static void exynos7_tmu_control(struct
>> platform_device *pdev, bool on) interrupt_en <<
>> EXYNOS_TMU_INTEN_FALL0_SHIFT; } else {
>>  		con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
>> -		con &= ~(1 << EXYNOS7_PD_DET_EN_SHIFT);
>> +		if (data->soc == SOC_ARCH_EXYNOS7)
>> +			con &= ~(1 << EXYNOS7_PD_DET_EN_SHIFT);
>>  		interrupt_en = 0; /* Disable all interrupts */
>>  	}
>>  
>> -	writel(interrupt_en, data->base + EXYNOS7_TMU_REG_INTEN);
>> +	if (data->soc == SOC_ARCH_EXYNOS5433) {
>> +		int pd_det_en = on ? EXYNOS5433_PD_DET_EN : 0;
>> +
>> +		writel(pd_det_en, data->base +
>> EXYNOS5433_TMU_PD_DET_EN);
>> +		writel(interrupt_en, data->base +
>> EXYNOS5433_TMU_REG_INTEN);
>> +	} else {
>> +		writel(interrupt_en, data->base +
>> EXYNOS7_TMU_REG_INTEN);
>> +	}
>> +
>>  	writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
>>  }
>>  
>> @@ -770,6 +901,8 @@ static void exynos4412_tmu_set_emulation(struct
>> exynos_tmu_data *data, 
>>  	if (data->soc == SOC_ARCH_EXYNOS5260)
>>  		emul_con = EXYNOS5260_EMUL_CON;
>> +	if (data->soc == SOC_ARCH_EXYNOS5433)
>> +		emul_con = EXYNOS5433_TMU_EMUL_CON;
>>  	else if (data->soc == SOC_ARCH_EXYNOS7)
>>  		emul_con = EXYNOS7_TMU_REG_EMUL_CON;
>>  	else
>> @@ -882,6 +1015,9 @@ static void exynos4210_tmu_clear_irqs(struct
>> exynos_tmu_data *data) } else if (data->soc == SOC_ARCH_EXYNOS7) {
>>  		tmu_intstat = EXYNOS7_TMU_REG_INTPEND;
>>  		tmu_intclear = EXYNOS7_TMU_REG_INTPEND;
>> +	} else if (data->soc == SOC_ARCH_EXYNOS5433) {
>> +		tmu_intstat = EXYNOS5433_TMU_REG_INTPEND;
>> +		tmu_intclear = EXYNOS5433_TMU_REG_INTPEND;
>>  	} else {
>>  		tmu_intstat = EXYNOS_TMU_REG_INTSTAT;
>>  		tmu_intclear = EXYNOS_TMU_REG_INTCLEAR;
>> @@ -926,6 +1062,7 @@ static const struct of_device_id
>> exynos_tmu_match[] = { { .compatible = "samsung,exynos5260-tmu", },
>>  	{ .compatible = "samsung,exynos5420-tmu", },
>>  	{ .compatible = "samsung,exynos5420-tmu-ext-triminfo", },
>> +	{ .compatible = "samsung,exynos5433-tmu", },
>>  	{ .compatible = "samsung,exynos5440-tmu", },
>>  	{ .compatible = "samsung,exynos7-tmu", },
>>  	{ /* sentinel */ },
>> @@ -949,6 +1086,8 @@ static int exynos_of_get_soc_type(struct
>> device_node *np) else if (of_device_is_compatible(np,
>>  					 "samsung,exynos5420-tmu-ext-triminfo"))
>>  		return SOC_ARCH_EXYNOS5420_TRIMINFO;
>> +	else if (of_device_is_compatible(np,
>> "samsung,exynos5433-tmu"))
>> +		return SOC_ARCH_EXYNOS5433;
>>  	else if (of_device_is_compatible(np,
>> "samsung,exynos5440-tmu")) return SOC_ARCH_EXYNOS5440;
>>  	else if (of_device_is_compatible(np, "samsung,exynos7-tmu"))
>> @@ -1069,6 +1208,13 @@ static int exynos_map_dt_data(struct
>> platform_device *pdev) data->tmu_set_emulation =
>> exynos4412_tmu_set_emulation; data->tmu_clear_irqs =
>> exynos4210_tmu_clear_irqs; break;
>> +	case SOC_ARCH_EXYNOS5433:
>> +		data->tmu_initialize = exynos5433_tmu_initialize;
>> +		data->tmu_control = exynos7_tmu_control;
> 
> I must frankly admit that I'm a bit confused.
> 
> I'm curious why we didn't either define totally separate set of
> exynos5433_tmu_* functions or reusing existing exynos7_tmu_* ?
> 
> Are exynos7 and exynos5433 so much different?

Exynos5444 TMU has a bit different register map from Exynos7 TMU.
To remove some confusion between Exynos7 and Exnynos5433,
I'll add seprate function for Exynos5433 TMU.

[snip]


Thanks,
Chanwoo Choi