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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

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,