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@xxxxxxxxxxxxxx>
>>>
>>> The RK3576 SoC has six TS-ADC channels: TOP, BIG_CORE, LITTLE_CORE,
>>> DDR, NPU and GPU.
>>>
>>> Signed-off-by: Ye Zhang <ye.zhang@xxxxxxxxxxxxxx>
>>> [ported to mainline, reworded commit message]
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@xxxxxxxxxxxxx>
>>> ---
>>>
>>> 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,
>
>
>
>
