diff mbox series

[v3,3/6] thermal: rockchip: Support RK3576 SoC in the thermal driver

Message ID 20250228-rk3576-tsadc-upstream-v3-3-4bfbb3b699b9@collabora.com (mailing list archive)
State New
Headers show
Series RK3576 thermal sensor support, including OTP trim adjustments | expand

Commit Message

Nicolas Frattaroli Feb. 28, 2025, 8:06 p.m. UTC 
From: Ye Zhang <ye.zhang@rock-chips.com>

The RK3576 SoC has six TS-ADC channels: TOP, BIG_CORE, LITTLE_CORE,
DDR, NPU and GPU.

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
[ported to mainline, reworded commit message]
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/thermal/rockchip_thermal.c | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Jonas Karlman March 15, 2025, 8:20 a.m. UTC | #1 
Hi Nicolas,

On 2025-02-28 21:06, Nicolas Frattaroli wrote:
> From: Ye Zhang <ye.zhang@rock-chips.com>
> 
> The RK3576 SoC has six TS-ADC channels: TOP, BIG_CORE, LITTLE_CORE,
> DDR, NPU and GPU.
> 
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> [ported to mainline, reworded commit message]
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/thermal/rockchip_thermal.c | 42 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index a8ad85feb68fbb7ec8d79602b16c47838ecb3c00..bec1930bebd87859a7e519cfc9f05e10b1c31e87 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -1061,6 +1061,22 @@ static void rk_tsadcv3_tshut_mode(int chn, void __iomem *regs,
>  	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
>  }
>  
> +static void rk_tsadcv4_tshut_mode(int chn, void __iomem *regs,
> +				  enum tshut_mode mode)
> +{
> +	u32 val_gpio, val_cru;
> +
> +	if (mode == TSHUT_MODE_GPIO) {
> +		val_gpio = TSADCV2_INT_SRC_EN(chn) | TSADCV2_INT_SRC_EN_MASK(chn);
> +		val_cru = TSADCV2_INT_SRC_EN_MASK(chn);
> +	} else {
> +		val_cru = TSADCV2_INT_SRC_EN(chn) | TSADCV2_INT_SRC_EN_MASK(chn);
> +		val_gpio = TSADCV2_INT_SRC_EN_MASK(chn);
> +	}
> +	writel_relaxed(val_gpio, regs + TSADCV3_HSHUT_GPIO_INT_EN);
> +	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
> +}

This function is identical to rk_tsadcv3_tshut_mode() in mainline.

Should the v3 function be renamed to v4 in mainline to match vendor
kernel to avoid confusion?

> +
>  static const struct rockchip_tsadc_chip px30_tsadc_data = {
>  	/* cpu, gpu */
>  	.chn_offset = 0,
> @@ -1284,6 +1300,28 @@ static const struct rockchip_tsadc_chip rk3568_tsadc_data = {
>  	},
>  };
>  
> +static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
> +	/* top, big_core, little_core, ddr, npu, gpu */
> +	.chn_offset = 0,
> +	.chn_num = 6, /* six channels for tsadc */
> +	.tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> +	.tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> +	.tshut_temp = 95000,

Here the default is GPIO and 95 deg, in DT node the default is override
to CRU and 120 deg.

Any reason that is not the default here?

Regards,
Jonas

> +	.initialize = rk_tsadcv8_initialize,
> +	.irq_ack = rk_tsadcv4_irq_ack,
> +	.control = rk_tsadcv4_control,
> +	.get_temp = rk_tsadcv4_get_temp,
> +	.set_alarm_temp = rk_tsadcv3_alarm_temp,
> +	.set_tshut_temp = rk_tsadcv3_tshut_temp,
> +	.set_tshut_mode = rk_tsadcv4_tshut_mode,
> +	.table = {
> +		.id = rk3588_code_table,
> +		.length = ARRAY_SIZE(rk3588_code_table),
> +		.data_mask = TSADCV4_DATA_MASK,
> +		.mode = ADC_INCREMENT,
> +	},
> +};
> +
>  static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
>  	/* top, big_core0, big_core1, little_core, center, gpu, npu */
>  	.chn_offset = 0,
> @@ -1342,6 +1380,10 @@ static const struct of_device_id of_rockchip_thermal_match[] = {
>  		.compatible = "rockchip,rk3568-tsadc",
>  		.data = (void *)&rk3568_tsadc_data,
>  	},
> +	{
> +		.compatible = "rockchip,rk3576-tsadc",
> +		.data = (void *)&rk3576_tsadc_data,
> +	},
>  	{
>  		.compatible = "rockchip,rk3588-tsadc",
>  		.data = (void *)&rk3588_tsadc_data,
>
Nicolas Frattaroli March 17, 2025, 8:28 a.m. UTC | #2 
On Saturday, 15 March 2025 09:20:07 Central European Standard Time Jonas 
Karlman wrote:
> Hi Nicolas,

Hi Jonas,

> 
> On 2025-02-28 21:06, Nicolas Frattaroli wrote:
> > From: Ye Zhang <ye.zhang@rock-chips.com>
> > 
> > The RK3576 SoC has six TS-ADC channels: TOP, BIG_CORE, LITTLE_CORE,
> > DDR, NPU and GPU.
> > 
> > Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> > [ported to mainline, reworded commit message]
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> > 
> >  drivers/thermal/rockchip_thermal.c | 42
> >  ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
> > 
> > diff --git a/drivers/thermal/rockchip_thermal.c
> > b/drivers/thermal/rockchip_thermal.c index
> > a8ad85feb68fbb7ec8d79602b16c47838ecb3c00..bec1930bebd87859a7e519cfc9f05e1
> > 0b1c31e87 100644 --- a/drivers/thermal/rockchip_thermal.c
> > +++ b/drivers/thermal/rockchip_thermal.c
> > @@ -1061,6 +1061,22 @@ static void rk_tsadcv3_tshut_mode(int chn, void
> > __iomem *regs,> 
> >  	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
> >  
> >  }
> > 
> > +static void rk_tsadcv4_tshut_mode(int chn, void __iomem *regs,
> > +				  enum tshut_mode mode)
> > +{
> > +	u32 val_gpio, val_cru;
> > +
> > +	if (mode == TSHUT_MODE_GPIO) {
> > +		val_gpio = TSADCV2_INT_SRC_EN(chn) | 
TSADCV2_INT_SRC_EN_MASK(chn);
> > +		val_cru = TSADCV2_INT_SRC_EN_MASK(chn);
> > +	} else {
> > +		val_cru = TSADCV2_INT_SRC_EN(chn) | 
TSADCV2_INT_SRC_EN_MASK(chn);
> > +		val_gpio = TSADCV2_INT_SRC_EN_MASK(chn);
> > +	}
> > +	writel_relaxed(val_gpio, regs + TSADCV3_HSHUT_GPIO_INT_EN);
> > +	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
> > +}
> 
> This function is identical to rk_tsadcv3_tshut_mode() in mainline.
> 
> Should the v3 function be renamed to v4 in mainline to match vendor
> kernel to avoid confusion?

Good catch. Yes, I'll add a patch to rename the function before introducing 
new changes in v4, and get rid of the duplicate function.

> 
> > +
> > 
> >  static const struct rockchip_tsadc_chip px30_tsadc_data = {
> >  
> >  	/* cpu, gpu */
> >  	.chn_offset = 0,
> > 
> > @@ -1284,6 +1300,28 @@ static const struct rockchip_tsadc_chip
> > rk3568_tsadc_data = {> 
> >  	},
> >  
> >  };
> > 
> > +static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
> > +	/* top, big_core, little_core, ddr, npu, gpu */
> > +	.chn_offset = 0,
> > +	.chn_num = 6, /* six channels for tsadc */
> > +	.tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC 
*/
> > +	.tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> > +	.tshut_temp = 95000,
> 
> Here the default is GPIO and 95 deg, in DT node the default is override
> to CRU and 120 deg.
> 
> Any reason that is not the default here?

No reason, other than that this is what most Rockchip SoCs seem to do. RK3588 
does the same thing. The hardware power-on-reset state is to not have any 
tshut, so whatever the "default" should be is entirely made up by the driver 
in either case.

For the sake of being consistent, I'll keep it the same in v4, as RK3588 does. 
Otherwise, we'll have RK3576 and RK3588 do different things. If someone wants 
to change the default, then ideally this would be done in a follow-up series 
to make it consistent for all SoCs.

If that's alright with you, then I'll send out a v4.

> 
> Regards,
> Jonas
> 

Regards,
Nicolas Frattaroli

> > +	.initialize = rk_tsadcv8_initialize,
> > +	.irq_ack = rk_tsadcv4_irq_ack,
> > +	.control = rk_tsadcv4_control,
> > +	.get_temp = rk_tsadcv4_get_temp,
> > +	.set_alarm_temp = rk_tsadcv3_alarm_temp,
> > +	.set_tshut_temp = rk_tsadcv3_tshut_temp,
> > +	.set_tshut_mode = rk_tsadcv4_tshut_mode,
> > +	.table = {
> > +		.id = rk3588_code_table,
> > +		.length = ARRAY_SIZE(rk3588_code_table),
> > +		.data_mask = TSADCV4_DATA_MASK,
> > +		.mode = ADC_INCREMENT,
> > +	},
> > +};
> > +
> > 
> >  static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
> >  
> >  	/* top, big_core0, big_core1, little_core, center, gpu, npu */
> >  	.chn_offset = 0,
> > 
> > @@ -1342,6 +1380,10 @@ static const struct of_device_id
> > of_rockchip_thermal_match[] = {> 
> >  		.compatible = "rockchip,rk3568-tsadc",
> >  		.data = (void *)&rk3568_tsadc_data,
> >  	
> >  	},
> > 
> > +	{
> > +		.compatible = "rockchip,rk3576-tsadc",
> > +		.data = (void *)&rk3576_tsadc_data,
> > +	},
> > 
> >  	{
> >  	
> >  		.compatible = "rockchip,rk3588-tsadc",
> >  		.data = (void *)&rk3588_tsadc_data,
Jonas Karlman March 17, 2025, 11:47 a.m. UTC | #3 
Hi Nicolas,

On 2025-03-17 09:28, Nicolas Frattaroli wrote:
> On Saturday, 15 March 2025 09:20:07 Central European Standard Time Jonas 
> Karlman wrote:
>> Hi Nicolas,
> 
> Hi Jonas,
> 
>>
>> On 2025-02-28 21:06, Nicolas Frattaroli wrote:
>>> From: Ye Zhang <ye.zhang@rock-chips.com>
>>>
>>> The RK3576 SoC has six TS-ADC channels: TOP, BIG_CORE, LITTLE_CORE,
>>> DDR, NPU and GPU.
>>>
>>> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
>>> [ported to mainline, reworded commit message]
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>>> ---
>>>
>>>  drivers/thermal/rockchip_thermal.c | 42
>>>  ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/thermal/rockchip_thermal.c
>>> b/drivers/thermal/rockchip_thermal.c index
>>> a8ad85feb68fbb7ec8d79602b16c47838ecb3c00..bec1930bebd87859a7e519cfc9f05e1
>>> 0b1c31e87 100644 --- a/drivers/thermal/rockchip_thermal.c
>>> +++ b/drivers/thermal/rockchip_thermal.c
>>> @@ -1061,6 +1061,22 @@ static void rk_tsadcv3_tshut_mode(int chn, void
>>> __iomem *regs,> 
>>>  	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
>>>  
>>>  }
>>>
>>> +static void rk_tsadcv4_tshut_mode(int chn, void __iomem *regs,
>>> +				  enum tshut_mode mode)
>>> +{
>>> +	u32 val_gpio, val_cru;
>>> +
>>> +	if (mode == TSHUT_MODE_GPIO) {
>>> +		val_gpio = TSADCV2_INT_SRC_EN(chn) | 
> TSADCV2_INT_SRC_EN_MASK(chn);
>>> +		val_cru = TSADCV2_INT_SRC_EN_MASK(chn);
>>> +	} else {
>>> +		val_cru = TSADCV2_INT_SRC_EN(chn) | 
> TSADCV2_INT_SRC_EN_MASK(chn);
>>> +		val_gpio = TSADCV2_INT_SRC_EN_MASK(chn);
>>> +	}
>>> +	writel_relaxed(val_gpio, regs + TSADCV3_HSHUT_GPIO_INT_EN);
>>> +	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
>>> +}
>>
>> This function is identical to rk_tsadcv3_tshut_mode() in mainline.
>>
>> Should the v3 function be renamed to v4 in mainline to match vendor
>> kernel to avoid confusion?
> 
> Good catch. Yes, I'll add a patch to rename the function before introducing 
> new changes in v4, and get rid of the duplicate function.

Great.

> 
>>
>>> +
>>>
>>>  static const struct rockchip_tsadc_chip px30_tsadc_data = {
>>>  
>>>  	/* cpu, gpu */
>>>  	.chn_offset = 0,
>>>
>>> @@ -1284,6 +1300,28 @@ static const struct rockchip_tsadc_chip
>>> rk3568_tsadc_data = {> 
>>>  	},
>>>  
>>>  };
>>>
>>> +static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
>>> +	/* top, big_core, little_core, ddr, npu, gpu */
>>> +	.chn_offset = 0,
>>> +	.chn_num = 6, /* six channels for tsadc */
>>> +	.tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC 
> */
>>> +	.tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
>>> +	.tshut_temp = 95000,
>>
>> Here the default is GPIO and 95 deg, in DT node the default is override
>> to CRU and 120 deg.
>>
>> Any reason that is not the default here?
> 
> No reason, other than that this is what most Rockchip SoCs seem to do. RK3588 
> does the same thing. The hardware power-on-reset state is to not have any 
> tshut, so whatever the "default" should be is entirely made up by the driver 
> in either case.
> 
> For the sake of being consistent, I'll keep it the same in v4, as RK3588 does. 
> Otherwise, we'll have RK3576 and RK3588 do different things. If someone wants 
> to change the default, then ideally this would be done in a follow-up series 
> to make it consistent for all SoCs.
> 
> If that's alright with you, then I'll send out a v4.

Sounds good, I was mostly wondering because I am preparing patches for
RK3528 and it felt strange to define one default in driver and directly
override that default in DT to something different.

Also noticed that this driver log a warning when the DT props are
missing, something that probably should be downgraded as the props are
optional in DT schema. Something for another series.

Regards,
Jonas

> 
>>
>> Regards,
>> Jonas
>>
> 
> Regards,
> Nicolas Frattaroli
> 
>>> +	.initialize = rk_tsadcv8_initialize,
>>> +	.irq_ack = rk_tsadcv4_irq_ack,
>>> +	.control = rk_tsadcv4_control,
>>> +	.get_temp = rk_tsadcv4_get_temp,
>>> +	.set_alarm_temp = rk_tsadcv3_alarm_temp,
>>> +	.set_tshut_temp = rk_tsadcv3_tshut_temp,
>>> +	.set_tshut_mode = rk_tsadcv4_tshut_mode,
>>> +	.table = {
>>> +		.id = rk3588_code_table,
>>> +		.length = ARRAY_SIZE(rk3588_code_table),
>>> +		.data_mask = TSADCV4_DATA_MASK,
>>> +		.mode = ADC_INCREMENT,
>>> +	},
>>> +};
>>> +
>>>
>>>  static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
>>>  
>>>  	/* top, big_core0, big_core1, little_core, center, gpu, npu */
>>>  	.chn_offset = 0,
>>>
>>> @@ -1342,6 +1380,10 @@ static const struct of_device_id
>>> of_rockchip_thermal_match[] = {> 
>>>  		.compatible = "rockchip,rk3568-tsadc",
>>>  		.data = (void *)&rk3568_tsadc_data,
>>>  	
>>>  	},
>>>
>>> +	{
>>> +		.compatible = "rockchip,rk3576-tsadc",
>>> +		.data = (void *)&rk3576_tsadc_data,
>>> +	},
>>>
>>>  	{
>>>  	
>>>  		.compatible = "rockchip,rk3588-tsadc",
>>>  		.data = (void *)&rk3588_tsadc_data,
> 
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index a8ad85feb68fbb7ec8d79602b16c47838ecb3c00..bec1930bebd87859a7e519cfc9f05e10b1c31e87 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -1061,6 +1061,22 @@  static void rk_tsadcv3_tshut_mode(int chn, void __iomem *regs,
 	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
 }
 
+static void rk_tsadcv4_tshut_mode(int chn, void __iomem *regs,
+				  enum tshut_mode mode)
+{
+	u32 val_gpio, val_cru;
+
+	if (mode == TSHUT_MODE_GPIO) {
+		val_gpio = TSADCV2_INT_SRC_EN(chn) | TSADCV2_INT_SRC_EN_MASK(chn);
+		val_cru = TSADCV2_INT_SRC_EN_MASK(chn);
+	} else {
+		val_cru = TSADCV2_INT_SRC_EN(chn) | TSADCV2_INT_SRC_EN_MASK(chn);
+		val_gpio = TSADCV2_INT_SRC_EN_MASK(chn);
+	}
+	writel_relaxed(val_gpio, regs + TSADCV3_HSHUT_GPIO_INT_EN);
+	writel_relaxed(val_cru, regs + TSADCV3_HSHUT_CRU_INT_EN);
+}
+
 static const struct rockchip_tsadc_chip px30_tsadc_data = {
 	/* cpu, gpu */
 	.chn_offset = 0,
@@ -1284,6 +1300,28 @@  static const struct rockchip_tsadc_chip rk3568_tsadc_data = {
 	},
 };
 
+static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
+	/* top, big_core, little_core, ddr, npu, gpu */
+	.chn_offset = 0,
+	.chn_num = 6, /* six channels for tsadc */
+	.tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
+	.tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
+	.tshut_temp = 95000,
+	.initialize = rk_tsadcv8_initialize,
+	.irq_ack = rk_tsadcv4_irq_ack,
+	.control = rk_tsadcv4_control,
+	.get_temp = rk_tsadcv4_get_temp,
+	.set_alarm_temp = rk_tsadcv3_alarm_temp,
+	.set_tshut_temp = rk_tsadcv3_tshut_temp,
+	.set_tshut_mode = rk_tsadcv4_tshut_mode,
+	.table = {
+		.id = rk3588_code_table,
+		.length = ARRAY_SIZE(rk3588_code_table),
+		.data_mask = TSADCV4_DATA_MASK,
+		.mode = ADC_INCREMENT,
+	},
+};
+
 static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
 	/* top, big_core0, big_core1, little_core, center, gpu, npu */
 	.chn_offset = 0,
@@ -1342,6 +1380,10 @@  static const struct of_device_id of_rockchip_thermal_match[] = {
 		.compatible = "rockchip,rk3568-tsadc",
 		.data = (void *)&rk3568_tsadc_data,
 	},
+	{
+		.compatible = "rockchip,rk3576-tsadc",
+		.data = (void *)&rk3576_tsadc_data,
+	},
 	{
 		.compatible = "rockchip,rk3588-tsadc",
 		.data = (void *)&rk3588_tsadc_data,