Re: [PATCH v2 1/2] thermal: mediatek: add support for MT7986 and MT7981

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 30/11/22 14:19, Daniel Golle ha scritto:
> Add support for V3 generation thermal found in MT7986 and MT7981 SoCs.
> Brings code to assign values from efuse as well as new function to
> convert raw temperature to millidegree celsius, as found in MediaTek's
> SDK sources (but cleaned up and de-duplicated)
> 
> [1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/baf36c7eef477aae1f8f2653b6c29e2caf48475b
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Reviewed-by: Henry Yen <henry.yen@mediatek.com>
> ---
> Changes since v1: Drop use of adc_oe field in efuse, Henry Yen confirmed
> its use has been dropped intentionally in MTK SDK as well.
> 
>   drivers/thermal/mtk_thermal.c | 122 +++++++++++++++++++++++++++++++++-
>   1 file changed, 119 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thermal/mtk_thermal.c b/drivers/thermal/mtk_thermal.c
> index 8440692e3890..6a69419f8960 100644
> --- a/drivers/thermal/mtk_thermal.c
> +++ b/drivers/thermal/mtk_thermal.c
> @@ -150,6 +150,21 @@
>   #define CALIB_BUF1_VALID_V2(x)		(((x) >> 4) & 0x1)
>   #define CALIB_BUF1_O_SLOPE_SIGN_V2(x)	(((x) >> 3) & 0x1)
>   
> +/*
> + * Layout of the fuses providing the calibration data
> + * These macros can be used for MT7981 and MT7986.
> + */
> +#define CALIB_BUF0_ADC_GE_V3(x)		(((x) >> 0) & 0x3ff)
> +#define CALIB_BUF0_ADC_OE_V3(x)		(((x) >> 10) & 0x3ff)
> +#define CALIB_BUF0_DEGC_CALI_V3(x)	(((x) >> 20) & 0x3f)
> +#define CALIB_BUF0_O_SLOPE_V3(x)	(((x) >> 26) & 0x3f)
> +#define CALIB_BUF1_VTS_TS1_V3(x)	(((x) >> 0) & 0x1ff)
> +#define CALIB_BUF1_VTS_TS2_V3(x)	(((x) >> 21) & 0x1ff)
> +#define CALIB_BUF1_VTS_TSABB_V3(x)	(((x) >> 9) & 0x1ff)
> +#define CALIB_BUF1_VALID_V3(x)		(((x) >> 18) & 0x1)
> +#define CALIB_BUF1_O_SLOPE_SIGN_V3(x)	(((x) >> 19) & 0x1)
> +#define CALIB_BUF1_ID_V3(x)		(((x) >> 20) & 0x1)
> +
>   enum {
>   	VTS1,
>   	VTS2,
> @@ -163,6 +178,7 @@ enum {
>   enum mtk_thermal_version {
>   	MTK_THERMAL_V1 = 1,
>   	MTK_THERMAL_V2,
> +	MTK_THERMAL_V3,
>   };
>   
>   /* MT2701 thermal sensors */
> @@ -245,6 +261,27 @@ enum mtk_thermal_version {
>   /* The calibration coefficient of sensor  */
>   #define MT8183_CALIBRATION	153
>   
> +/* AUXADC channel 11 is used for the temperature sensors */
> +#define MT7986_TEMP_AUXADC_CHANNEL	11
> +
> +/* The total number of temperature sensors in the MT7986 */
> +#define MT7986_NUM_SENSORS		1
> +
> +/* The number of banks in the MT7986 */
> +#define MT7986_NUM_ZONES		1
> +
> +/* The number of sensing points per bank */
> +#define MT7986_NUM_SENSORS_PER_ZONE	1
> +
> +/* MT7986 thermal sensors */
> +#define MT7986_TS1			0
> +
> +/* The number of controller in the MT7986 */
> +#define MT7986_NUM_CONTROLLER		1
> +
> +/* The calibration coefficient of sensor  */
> +#define MT7986_CALIBRATION		165
> +
>   struct mtk_thermal;
>   
>   struct thermal_bank_cfg {
> @@ -386,6 +423,14 @@ static const int mt7622_mux_values[MT7622_NUM_SENSORS] = { 0, };
>   static const int mt7622_vts_index[MT7622_NUM_SENSORS] = { VTS1 };
>   static const int mt7622_tc_offset[MT7622_NUM_CONTROLLER] = { 0x0, };
>   
> +/* MT7986 thermal sensor data */
> +static const int mt7986_bank_data[MT7986_NUM_SENSORS] = { MT7986_TS1, };
> +static const int mt7986_msr[MT7986_NUM_SENSORS_PER_ZONE] = { TEMP_MSR0, };
> +static const int mt7986_adcpnp[MT7986_NUM_SENSORS_PER_ZONE] = { TEMP_ADCPNP0, };
> +static const int mt7986_mux_values[MT7986_NUM_SENSORS] = { 0, };
> +static const int mt7986_vts_index[MT7986_NUM_SENSORS] = { VTS1 };
> +static const int mt7986_tc_offset[MT7986_NUM_CONTROLLER] = { 0x0, };
> +
>   /*
>    * The MT8173 thermal controller has four banks. Each bank can read up to
>    * four temperature sensors simultaneously. The MT8173 has a total of 5
> @@ -549,6 +594,30 @@ static const struct mtk_thermal_data mt8183_thermal_data = {
>   	.version = MTK_THERMAL_V1,
>   };
>   
> +/*
> + * MT7986 uses AUXADC Channel 11 for raw data access.
> + */
> +static const struct mtk_thermal_data mt7986_thermal_data = {
> +	.auxadc_channel = MT7986_TEMP_AUXADC_CHANNEL,
> +	.num_banks = MT7986_NUM_ZONES,
> +	.num_sensors = MT7986_NUM_SENSORS,
> +	.vts_index = mt7986_vts_index,
> +	.cali_val = MT7986_CALIBRATION,
> +	.num_controller = MT7986_NUM_CONTROLLER,
> +	.controller_offset = mt7986_tc_offset,
> +	.need_switch_bank = true,
> +	.bank_data = {
> +		{
> +			.num_sensors = 1,
> +			.sensors = mt7986_bank_data,
> +		},
> +	},
> +	.msr = mt7986_msr,
> +	.adcpnp = mt7986_adcpnp,
> +	.sensor_mux_values = mt7986_mux_values,
> +	.version = MTK_THERMAL_V3,
> +};
> +
>   /**
>    * raw_to_mcelsius - convert a raw ADC value to mcelsius
>    * @mt:	The thermal controller
> @@ -603,6 +672,22 @@ static int raw_to_mcelsius_v2(struct mtk_thermal *mt, int sensno, s32 raw)
>   	return (format_2 - tmp) * 100;
>   }
>   
> +static int raw_to_mcelsius_v3(struct mtk_thermal *mt, int sensno, s32 raw)
> +{
> +	s32 tmp;
> +
> +	if (raw == 0)
> +		return 0;
> +
> +	raw &= 0xfff;
> +	tmp = 100000 * 15 / 16 * 10000;
> +	tmp /= 4096 - 512 + mt->adc_ge;
> +	tmp /= 1490;
> +	tmp *= raw - mt->vts[sensno] - 2900;
> +
> +	return mt->degc_cali * 500 - tmp;
> +}
> +
>   /**
>    * mtk_thermal_get_bank - get bank
>    * @bank:	The bank
> @@ -659,9 +744,12 @@ static int mtk_thermal_bank_temperature(struct mtk_thermal_bank *bank)
>   		if (mt->conf->version == MTK_THERMAL_V1) {
>   			temp = raw_to_mcelsius_v1(
>   				mt, conf->bank_data[bank->id].sensors[i], raw);
> -		} else {
> +		} else if (mt->conf->version == MTK_THERMAL_V2) {
>   			temp = raw_to_mcelsius_v2(
>   				mt, conf->bank_data[bank->id].sensors[i], raw);
> +		} else {
> +			temp = raw_to_mcelsius_v3(
> +				mt, conf->bank_data[bank->id].sensors[i], raw);
>   		}

What about optimizing this with assigning a function pointer?
Like that, we wouldn't check any version in there... as in that case we'd
simply do something like

temp = conf->raw_to_mcelsius(mt, conf->bank...blahblah...);

...and this would also mean that the snippet saying "the first read of a sensor
often contains very high bogus temperature value [...]" would get merged in the v2
of raw_to_mcelsius (as that function is used only in mtk_thermal_bank_temperature).

>   
>   		/*
> @@ -887,6 +975,26 @@ static int mtk_thermal_extract_efuse_v2(struct mtk_thermal *mt, u32 *buf)
>   	return 0;
>   }
>   
> +static int mtk_thermal_extract_efuse_v3(struct mtk_thermal *mt, u32 *buf)
> +{
> +	if (!CALIB_BUF1_VALID_V3(buf[1]))
> +		return -EINVAL;
> +
> +	mt->adc_oe = CALIB_BUF0_ADC_OE_V3(buf[0]);
> +	mt->adc_ge = CALIB_BUF0_ADC_GE_V3(buf[0]);
> +	mt->degc_cali = CALIB_BUF0_DEGC_CALI_V3(buf[0]);
> +	mt->o_slope = CALIB_BUF0_O_SLOPE_V3(buf[0]);
> +	mt->vts[VTS1] = CALIB_BUF1_VTS_TS1_V3(buf[1]);
> +	mt->vts[VTS2] = CALIB_BUF1_VTS_TS2_V3(buf[1]);
> +	mt->vts[VTSABB] = CALIB_BUF1_VTS_TSABB_V3(buf[1]);
> +	mt->o_slope_sign = CALIB_BUF1_O_SLOPE_SIGN_V3(buf[1]);
> +
> +	if (CALIB_BUF1_ID_V3(buf[1]) == 0)
> +		mt->o_slope = 0;
> +
> +	return 0;
> +}
> +
>   static int mtk_thermal_get_calibration_data(struct device *dev,
>   					    struct mtk_thermal *mt)
>   {
> @@ -897,6 +1005,7 @@ static int mtk_thermal_get_calibration_data(struct device *dev,
>   
>   	/* Start with default values */
>   	mt->adc_ge = 512;
> +	mt->adc_oe = 512;
>   	for (i = 0; i < mt->conf->num_sensors; i++)
>   		mt->vts[i] = 260;
>   	mt->degc_cali = 40;
> @@ -924,8 +1033,10 @@ static int mtk_thermal_get_calibration_data(struct device *dev,
>   
>   	if (mt->conf->version == MTK_THERMAL_V1)
>   		ret = mtk_thermal_extract_efuse_v1(mt, buf);
> -	else
> +	else if (mt->conf->version == MTK_THERMAL_V2)
>   		ret = mtk_thermal_extract_efuse_v2(mt, buf);
> +	else
> +		ret = mtk_thermal_extract_efuse_v3(mt, buf);

I propose to use a switch here instead.

	switch(mt->conf->version) {
	case MTK_THERMAL_V1:
		....
	case MTK_THERMAL_V2:
		....
	case MTK_THERMAL_V3:
		....
	default:
		ret = -EINVAL;
		break;
	};

This would also prevent a potential issue with getting an invalid calibration
due to us calling the wrong version of the get_calibration() function, in which
case, using the default calibration values would be at that point preferred.

Regards,
Angelo