lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ba057235-85d4-4635-87ad-741e369904f3@kwiboo.se>
Date: Mon, 17 Mar 2025 12:47:52 +0100
From: Jonas Karlman <jonas@...boo.se>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
 Daniel Lezcano <daniel.lezcano@...aro.org>, Zhang Rui <rui.zhang@...el.com>,
 Lukasz Luba <lukasz.luba@....com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Ye Zhang <ye.zhang@...k-chips.com>, devicetree@...r.kernel.org,
 linux-pm@...r.kernel.org, Sebastian Reichel
 <sebastian.reichel@...labora.com>, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, kernel@...labora.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 3/6] thermal: rockchip: Support RK3576 SoC in the
 thermal driver

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@...k-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@...k-chips.com>
>>> [ported to mainline, reworded commit message]
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.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,
> 
> 
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ