Re: [PATCH 1/2] thermal: rockchip: Support RK3588 SoC in the thermal driver

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

On Freitag, 21. Oktober 2022 21:59:56 CEST Daniel Lezcano wrote:
> On 21/10/2022 19:47, Sebastian Reichel wrote:
> > From: Finley Xiao <finley.xiao@rock-chips.com>
> > 
> > The RK3588 SoC has seven channels TS-ADC(TOP, BIG_CORE0, BIG_CORE1,
> > LITTEL_CORE, CENTER, GPU, and NPU).
> 
> Is possible to give a more elaborate description of those sensors?
> 
> What is TOP and CENTER ?
> 
> There are 4 Bigs on this platform but two sensors ?

As far as I know, the four big cores in the SoC are arranged in two
clusters of two cores each, so one temperature sensor for each
cluster. As far as I can tell each CPU in a cluster shares its voltage
with its partner CPU core in its cluster.

If you have access to the TRM, it contains the following line in
part 1 on page 1372:

	Support to 7 channel TS-ADC (near chip center, A76_0/1, A76_2/3,
	DSU and A55_0/1/2/3, PD_CENTER, NPU, GPU)

I assume one of "TOP" and "CENTER" is "near chip center", the other is
PD_CENTER, whatever that means (PD = power domain maybe?)

I agree these could be named more descriptively.

> 
> 
> > Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> > [rebase, squash fixes]
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >   drivers/thermal/rockchip_thermal.c | 182 ++++++++++++++++++++++++++++-
> >   1 file changed, 179 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> > index 819e059cde71..82f475a6448f 100644
> > --- a/drivers/thermal/rockchip_thermal.c
> > +++ b/drivers/thermal/rockchip_thermal.c
> > @@ -61,10 +61,9 @@ enum adc_sort_mode {
> >   #include "thermal_hwmon.h"
> >   
> >   /**
> > - * The max sensors is two in rockchip SoCs.
> > - * Two sensors: CPU and GPU sensor.
> > + * The max sensors is seven in rockchip SoCs.
> >    */
> > -#define SOC_MAX_SENSORS	2
> > +#define SOC_MAX_SENSORS	7
> 
> You may get rid of this and replace the sensors array to an dyn 
> allocation based on chn_num
> 
> [ ... ]
> 
> > +static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
> > +	/* top, big_core0, big_core1, little_core, center, gpu, npu */
> > +	.chn_id = {0, 1, 2, 3, 4, 5, 6},
> 
> You may want to revisit that. Actually, chn_id is not useful and can be 
> removed everywhere as there is no hole. Otherwise a bit mask could be 
> used. By removing it, SENSOR_CPU and SENSOR_GPU can be removed too.
> 
> > +	.chn_num = 7, /* seven 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_tsadcv3_tshut_mode,
> > +	.table = {
> > +		.id = rk3588_code_table,
> > +		.length = ARRAY_SIZE(rk3588_code_table),
> > +		.data_mask = TSADCV4_DATA_MASK,
> > +		.mode = ADC_INCREMENT,
> > +	},
> > +};
> > +
> >   static const struct of_device_id of_rockchip_thermal_match[] = {
> >   	{	.compatible = "rockchip,px30-tsadc",
> >   		.data = (void *)&px30_tsadc_data,
> > @@ -1180,6 +1352,10 @@ static const struct of_device_id of_rockchip_thermal_match[] = {
> >   		.compatible = "rockchip,rk3568-tsadc",
> >   		.data = (void *)&rk3568_tsadc_data,
> >   	},
> > +	{
> > +		.compatible = "rockchip,rk3588-tsadc",
> > +		.data = (void *)&rk3588_tsadc_data,
> > +	},
> >   	{ /* end */ },
> >   };
> >   MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
> 
> 
>