[v2,4/4] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

  From: Frank Wunderlich <frank-w@public-files.de>

Add Support for Mediatek Filogic 880/MT7988 LVTS.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
v2:
- use 105°C for hw shutdown
- move constants to binding file
- change coeff.a to temp_factor and coeff.b to temp_offset
- change to lvts to lvts-ap (Application Processor)
- drop comments about efuse offsets
- change comment of mt8195 to be similar to mt7988
---
 drivers/thermal/mediatek/lvts_thermal.c | 46 +++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
  

Il 20/09/23 19:50, Frank Wunderlich ha scritto:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add Support for Mediatek Filogic 880/MT7988 LVTS.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v2:
> - use 105°C for hw shutdown
> - move constants to binding file
> - change coeff.a to temp_factor and coeff.b to temp_offset
> - change to lvts to lvts-ap (Application Processor)
> - drop comments about efuse offsets
> - change comment of mt8195 to be similar to mt7988
> ---
>   drivers/thermal/mediatek/lvts_thermal.c | 46 +++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index c2669f405a94..8fd1dc5adb16 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -82,6 +82,8 @@
>   #define LVTS_GOLDEN_TEMP_DEFAULT	50
>   #define LVTS_COEFF_A_MT8195			-250460
>   #define LVTS_COEFF_B_MT8195			250460
> +#define LVTS_COEFF_A_MT7988			-204650
> +#define LVTS_COEFF_B_MT7988			204650
>   
>   #define LVTS_MSR_IMMEDIATE_MODE		0
>   #define LVTS_MSR_FILTERED_MODE		1
> @@ -89,6 +91,7 @@
>   #define LVTS_MSR_READ_TIMEOUT_US	400
>   #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
>   
> +#define LVTS_HW_SHUTDOWN_MT7988		105000

I would simply reuse the definition of LVTS_HW_SHUTDOWN_MT8195....

>   #define LVTS_HW_SHUTDOWN_MT8195		105000
>   
>   #define LVTS_MINIMUM_THRESHOLD		20000
> @@ -1269,6 +1272,41 @@ static int lvts_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +/*
> + * LVTS MT7988
> + */
> +

Please remove this big comment block, that's not needed.

> +static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
> +	{
> +		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924

This 918,91c,etc comment is not necessary

> +		.lvts_sensor = {
> +			{ .dt_id = MT7988_CPU_0 }, // CPU 0,1

If you want to retain those comments, you shall use the right style.

{ .dt_id = MT7988_CPU_0 }, /* CPU 0,1 */
{ .. } /* CPU 2,3 */
{ .. } /* Internal 2.5G PHY 1 */

etc

> +			{ .dt_id = MT7988_CPU_1 }, // CPU 2,3
> +			{ .dt_id = MT7988_ETH2P5G_0 }, // internal 2.5G Phy 1
> +			{ .dt_id = MT7988_ETH2P5G_1 }  // internal 2.5G Phy 2
> +		},
> +		.num_lvts_sensor = 4,
> +		.offset = 0x0,
> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
> +	},
> +	{
> +		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938

comment not needed

> +		.lvts_sensor = {
> +			{ .dt_id = MT7988_TOPS_0}, // TOPS > +			{ .dt_id = MT7988_TOPS_1}, // TOPS

The dt_id definition already says "TOPS", this comment is not needed.

> +			{ .dt_id = MT7988_ETHWARP_0}, // WED 1
> +			{ .dt_id = MT7988_ETHWARP_1}  // WED 2

Same comment about the format; /* WED 1 */

> +		},
> +		.num_lvts_sensor = 4,
> +		.offset = 0x100,
> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
> +	}
> +};
> +
> +/*
> + * LVTS MT8195
> + */

Please also remove this big comment block, it's not needed.

Apart from that, this patch looks good; v3 will be the golden one :-)

Cheers,
Angelo

> +
>   static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = {
>   	{
>   		.cal_offset = { 0x04, 0x07 },
> @@ -1348,6 +1386,13 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = {
>   	}
>   };
>   
> +static const struct lvts_data mt7988_lvts_ap_data = {
> +	.lvts_ctrl	= mt7988_lvts_ap_data_ctrl,
> +	.num_lvts_ctrl	= ARRAY_SIZE(mt7988_lvts_ap_data_ctrl),
> +	.temp_factor	= LVTS_COEFF_A_MT7988,
> +	.temp_offset	= LVTS_COEFF_B_MT7988,
> +};
> +
>   static const struct lvts_data mt8195_lvts_mcu_data = {
>   	.lvts_ctrl	= mt8195_lvts_mcu_data_ctrl,
>   	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl),
> @@ -1363,6 +1408,7 @@ static const struct lvts_data mt8195_lvts_ap_data = {
>   };
>   
>   static const struct of_device_id lvts_of_match[] = {
> +	{ .compatible = "mediatek,mt7988-lvts-ap", .data = &mt7988_lvts_ap_data },
>   	{ .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data },
>   	{ .compatible = "mediatek,mt8195-lvts-ap", .data = &mt8195_lvts_ap_data },
>   	{},
  

Am 21. September 2023 09:54:35 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>Il 20/09/23 19:50, Frank Wunderlich ha scritto:
>> From: Frank Wunderlich <frank-w@public-files.de>
 
>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>> index c2669f405a94..8fd1dc5adb16 100644
>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>> @@ -82,6 +82,8 @@
>>   #define LVTS_GOLDEN_TEMP_DEFAULT	50
>>   #define LVTS_COEFF_A_MT8195			-250460
>>   #define LVTS_COEFF_B_MT8195			250460
>> +#define LVTS_COEFF_A_MT7988			-204650
>> +#define LVTS_COEFF_B_MT7988			204650
>>     #define LVTS_MSR_IMMEDIATE_MODE		0
>>   #define LVTS_MSR_FILTERED_MODE		1
>> @@ -89,6 +91,7 @@
>>   #define LVTS_MSR_READ_TIMEOUT_US	400
>>   #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
>>   +#define LVTS_HW_SHUTDOWN_MT7988		105000
>
>I would simply reuse the definition of LVTS_HW_SHUTDOWN_MT8195....

Hi angelo,
thanks for review.

Imho it should be separated...if someone thinks it needs to be changed later it will be changed not only for mt8195...a generic name can also cause problems if the next soc has different value.

>>   #define LVTS_HW_SHUTDOWN_MT8195		105000
>>     #define LVTS_MINIMUM_THRESHOLD		20000
>> @@ -1269,6 +1272,41 @@ static int lvts_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   +/*
>> + * LVTS MT7988
>> + */
>> +
>
>Please remove this big comment block, that's not needed.

Ok,i drop the comments (maybe except the wed one where the name in technical document (i used for constants) does not point to wed function.

>> +static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
>> +	{
>> +		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924
>
>This 918,91c,etc comment is not necessary
>
>> +		.lvts_sensor = {
>> +			{ .dt_id = MT7988_CPU_0 }, // CPU 0,1
>
>If you want to retain those comments, you shall use the right style.
>
>{ .dt_id = MT7988_CPU_0 }, /* CPU 0,1 */
>{ .. } /* CPU 2,3 */
>{ .. } /* Internal 2.5G PHY 1 */
>
>etc
>
>> +			{ .dt_id = MT7988_CPU_1 }, // CPU 2,3
>> +			{ .dt_id = MT7988_ETH2P5G_0 }, // internal 2.5G Phy 1
>> +			{ .dt_id = MT7988_ETH2P5G_1 }  // internal 2.5G Phy 2
>> +		},
>> +		.num_lvts_sensor = 4,
>> +		.offset = 0x0,
>> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
>> +	},
>> +	{
>> +		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938
>
>comment not needed
>
>> +		.lvts_sensor = {
>> +			{ .dt_id = MT7988_TOPS_0}, // TOPS > +			{ .dt_id = MT7988_TOPS_1}, // TOPS
>
>The dt_id definition already says "TOPS", this comment is not needed.
>
>> +			{ .dt_id = MT7988_ETHWARP_0}, // WED 1
>> +			{ .dt_id = MT7988_ETHWARP_1}  // WED 2
>
>Same comment about the format; /* WED 1 */
>
>> +		},
>> +		.num_lvts_sensor = 4,
>> +		.offset = 0x100,
>> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
>> +	}
>> +};
>> +
>> +/*
>> + * LVTS MT8195
>> + */
>
>Please also remove this big comment block, it's not needed.
>
>Apart from that, this patch looks good; v3 will be the golden one :-)
>
>Cheers,
>Angelo
>

regards Frank
  

Il 21/09/23 12:39, Frank Wunderlich ha scritto:
> Am 21. September 2023 09:54:35 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>> Il 20/09/23 19:50, Frank Wunderlich ha scritto:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>   
>>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>>> index c2669f405a94..8fd1dc5adb16 100644
>>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>>> @@ -82,6 +82,8 @@
>>>    #define LVTS_GOLDEN_TEMP_DEFAULT	50
>>>    #define LVTS_COEFF_A_MT8195			-250460
>>>    #define LVTS_COEFF_B_MT8195			250460
>>> +#define LVTS_COEFF_A_MT7988			-204650
>>> +#define LVTS_COEFF_B_MT7988			204650
>>>      #define LVTS_MSR_IMMEDIATE_MODE		0
>>>    #define LVTS_MSR_FILTERED_MODE		1
>>> @@ -89,6 +91,7 @@
>>>    #define LVTS_MSR_READ_TIMEOUT_US	400
>>>    #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
>>>    +#define LVTS_HW_SHUTDOWN_MT7988		105000
>>
>> I would simply reuse the definition of LVTS_HW_SHUTDOWN_MT8195....
> 
> Hi angelo,
> thanks for review.
> 
> Imho it should be separated...if someone thinks it needs to be changed later it will be changed not only for mt8195...a generic name can also cause problems if the next soc has different value.
> 

Okay, I don't really have strong opinions against that anyway.

Cheers
Angelo

>>>    #define LVTS_HW_SHUTDOWN_MT8195		105000
>>>      #define LVTS_MINIMUM_THRESHOLD		20000
>>> @@ -1269,6 +1272,41 @@ static int lvts_remove(struct platform_device *pdev)
>>>    	return 0;
>>>    }
>>>    +/*
>>> + * LVTS MT7988
>>> + */
>>> +
>>
>> Please remove this big comment block, that's not needed.
> 
> Ok,i drop the comments (maybe except the wed one where the name in technical document (i used for constants) does not point to wed function.
> 
>>> +static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
>>> +	{
>>> +		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924
>>
>> This 918,91c,etc comment is not necessary
>>
>>> +		.lvts_sensor = {
>>> +			{ .dt_id = MT7988_CPU_0 }, // CPU 0,1
>>
>> If you want to retain those comments, you shall use the right style.
>>
>> { .dt_id = MT7988_CPU_0 }, /* CPU 0,1 */
>> { .. } /* CPU 2,3 */
>> { .. } /* Internal 2.5G PHY 1 */
>>
>> etc
>>
>>> +			{ .dt_id = MT7988_CPU_1 }, // CPU 2,3
>>> +			{ .dt_id = MT7988_ETH2P5G_0 }, // internal 2.5G Phy 1
>>> +			{ .dt_id = MT7988_ETH2P5G_1 }  // internal 2.5G Phy 2
>>> +		},
>>> +		.num_lvts_sensor = 4,
>>> +		.offset = 0x0,
>>> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
>>> +	},
>>> +	{
>>> +		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938
>>
>> comment not needed
>>
>>> +		.lvts_sensor = {
>>> +			{ .dt_id = MT7988_TOPS_0}, // TOPS > +			{ .dt_id = MT7988_TOPS_1}, // TOPS
>>
>> The dt_id definition already says "TOPS", this comment is not needed.
>>
>>> +			{ .dt_id = MT7988_ETHWARP_0}, // WED 1
>>> +			{ .dt_id = MT7988_ETHWARP_1}  // WED 2
>>
>> Same comment about the format; /* WED 1 */
>>
>>> +		},
>>> +		.num_lvts_sensor = 4,
>>> +		.offset = 0x100,
>>> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
>>> +	}
>>> +};
>>> +
>>> +/*
>>> + * LVTS MT8195
>>> + */
>>
>> Please also remove this big comment block, it's not needed.
>>
>> Apart from that, this patch looks good; v3 will be the golden one :-)
>>
>> Cheers,
>> Angelo
>>
> 
> regards Frank