There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
Fixes: 32641b8ab1a5 ("arm64: dts: rockchip: add rk3588 thermal sensor") Cc: stable@vger.kernel.org Signed-off-by: Alexander Shiyan eagle.alexander923@gmail.com --- arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi index a337f3fb8377..f141065eb69d 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi @@ -2667,9 +2667,9 @@ tsadc: tsadc@fec00000 { rockchip,hw-tshut-temp = <120000>; rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU 1:GPIO */ rockchip,hw-tshut-polarity = <0>; /* tshut polarity 0:LOW 1:HIGH */ - pinctrl-0 = <&tsadc_gpio_func>; - pinctrl-1 = <&tsadc_shut>; - pinctrl-names = "gpio", "otpout"; + pinctrl-0 = <&tsadc_shut>; + pinctrl-1 = <&tsadc_gpio_func>; + pinctrl-names = "default", "sleep"; #thermal-sensor-cells = <1>; status = "disabled"; };
Hi Alexander,
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
Best regards, Alexey
Hello Alexey,
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
+ if (thermal->tshut_mode == TSHUT_MODE_GPIO) + pinctrl_select_default_state(dev); + else + pinctrl_select_sleep_state(dev); + for (i = 0; i < thermal->chip->chn_num; i++) { error = rockchip_thermal_register_sensor(pdev, thermal, &thermal->sensors[i],
If you could test it, please, it would be great, and I'd prepare the proper patch tomorrow or so.
On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic dsimic@manjaro.org wrote:
Hello Alexey,
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
if (thermal->tshut_mode == TSHUT_MODE_GPIO)pinctrl_select_default_state(dev);elsepinctrl_select_sleep_state(dev);
I believe no 'else' block is needed here, because if tshut_mode is not TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's no reason for the driver to mess with its pinctrl state. I'd rather put a mirroring block to put the pin back to its 'sleep' state in the removal function for the TSHUT_MODE_GPIO case.
Will try and revert.
P.S. Just looked at the downstream driver, and it actually calls TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was not a typo in the first place. So maybe the right approach here is not to change the device tree but rather fix the "gpio" / "otpout" pinctrl state handling in the driver.
Best, Alexey
On 2025-01-24 11:25, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
if (thermal->tshut_mode == TSHUT_MODE_GPIO)pinctrl_select_default_state(dev);elsepinctrl_select_sleep_state(dev);I believe no 'else' block is needed here, because if tshut_mode is not TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's no reason for the driver to mess with its pinctrl state. I'd rather put a mirroring block to put the pin back to its 'sleep' state in the removal function for the TSHUT_MODE_GPIO case.
You're right, but the "else block" is what the downstream driver does, so I think it's better to simply stay on the safe side and follow that logic in the upstream driver. Is it really needed? Perhaps not, but it also shouldn't hurt.
Will try and revert.
Awesome, thanks!
P.S. Just looked at the downstream driver, and it actually calls TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was not a typo in the first place. So maybe the right approach here is not to change the device tree but rather fix the "gpio" / "otpout" pinctrl state handling in the driver.
Indeed, "otpout" wasn't a typo, and I've already addressed that in my comments to Alexander's patch. Will send that response a bit later.
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
On 2025-01-24 11:37, Dragan Simic wrote:
On 2025-01-24 11:25, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
if (thermal->tshut_mode == TSHUT_MODE_GPIO)pinctrl_select_default_state(dev);elsepinctrl_select_sleep_state(dev);I believe no 'else' block is needed here, because if tshut_mode is not TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's no reason for the driver to mess with its pinctrl state. I'd rather put a mirroring block to put the pin back to its 'sleep' state in the removal function for the TSHUT_MODE_GPIO case.
You're right, but the "else block" is what the downstream driver does, so I think it's better to simply stay on the safe side and follow that logic in the upstream driver. Is it really needed? Perhaps not, but it also shouldn't hurt.
Will try and revert.
Awesome, thanks!
Actually... Revert or report? :)
P.S. Just looked at the downstream driver, and it actually calls TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was not a typo in the first place. So maybe the right approach here is not to change the device tree but rather fix the "gpio" / "otpout" pinctrl state handling in the driver.
Indeed, "otpout" wasn't a typo, and I've already addressed that in my comments to Alexander's patch. Will send that response a bit later.
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 11:25, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
if (thermal->tshut_mode == TSHUT_MODE_GPIO)pinctrl_select_default_state(dev);elsepinctrl_select_sleep_state(dev);I believe no 'else' block is needed here, because if tshut_mode is not TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's no reason for the driver to mess with its pinctrl state. I'd rather put a mirroring block to put the pin back to its 'sleep' state in the removal function for the TSHUT_MODE_GPIO case.
You're right, but the "else block" is what the downstream driver does,
Does it though? It only handles the TSHUT_MODE_GPIO case as far as I can tell (or TSHUT_MODE_OTP in downstream driver lingo) [1]
[1] https://github.com/radxa/kernel/blob/edb3eeeaa4643ecac6f4185d6d391c574735fca...
so I think it's better to simply stay on the safe side and follow that logic in the upstream driver. Is it really needed? Perhaps not, but it also shouldn't hurt.
Will try and revert.
Awesome, thanks!
P.S. Just looked at the downstream driver, and it actually calls TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was not a typo in the first place. So maybe the right approach here is not to change the device tree but rather fix the "gpio" / "otpout" pinctrl state handling in the driver.
Indeed, "otpout" wasn't a typo, and I've already addressed that in my comments to Alexander's patch. Will send that response a bit later.
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
Anyway, I've just tried it after including the changes below, and while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected pinctrls under tsadc, the driver still doesn't seem to be triggering a PMIC reset. Weird. Any thoughts welcome.
Best regards, Alexey
rock-5c ~ # cat /sys/kernel/debug/pinctrl/pinctrl-handles Requested pin control handlers their pinmux maps: [...] device: fec00000.tsadc current state: default state: default type: MUX_GROUP controller rockchip-pinctrl group: tsadc-shut (84) function: tsadc (84) type: CONFIGS_PIN controller rockchip-pinctrl pin gpio0-1 (1)config 00000001 state: sleep type: MUX_GROUP controller rockchip-pinctrl group: tsadc-gpio-func (95) function: gpio-func (97) type: CONFIGS_PIN controller rockchip-pinctrl pin gpio0-1 (1)config 00000001 [...]
rock-5c ~ # gpioinfo 0 gpiochip0 - 32 lines: line 0: unnamed "regulator-vdd-3v3" output active-high [used] line 1: unnamed unused input active-high line 2: unnamed unused input active-high line 3: unnamed unused input active-high line 4: unnamed unused input active-high line 5: unnamed unused input active-high line 6: unnamed unused input active-high line 7: unnamed unused input active-high line 8: unnamed unused input active-high line 9: unnamed unused input active-high line 10: unnamed unused input active-high line 11: unnamed unused input active-high line 12: unnamed unused input active-high line 13: unnamed unused input active-high line 14: unnamed unused input active-high line 15: unnamed unused input active-high line 16: unnamed unused input active-high line 17: unnamed unused input active-high line 18: unnamed unused input active-high line 19: unnamed unused input active-high line 20: unnamed unused input active-high line 21: unnamed "regulator-pcie2x1l2-3v3" output active-high [used] line 22: unnamed unused input active-high line 23: unnamed unused input active-high line 24: unnamed unused input active-high line 25: unnamed unused input active-high line 26: unnamed unused input active-high line 27: unnamed unused input active-high line 28: unnamed "regulator-vcc5v0-usb-otg0" output active-high [used] line 29: unnamed unused input active-high line 30: unnamed unused input active-high line 31: unnamed unused input active-high
(note line 1: unused above)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts index 6e56d7704cbe..e8c4d9b3f828 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts @@ -873,6 +873,9 @@ regulator-state-mem { };
&tsadc { + rockchip,hw-tshut-temp = <65000>; + rockchip,hw-tshut-mode = <1>; /* tshut mode 0:CRU 1:GPIO */ + rockchip,hw-tshut-polarity = <0>; /* tshut polarity 0:LOW 1:HIGH */ status = "okay"; };
diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c index f551df48eef9..4f474906b2b0 100644 --- a/drivers/thermal/rockchip_thermal.c +++ b/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,9 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
+ if (thermal->tshut_mode == TSHUT_MODE_GPIO) + pinctrl_select_default_state(&pdev->dev); + for (i = 0; i < thermal->chip->chn_num; i++) { error = rockchip_thermal_register_sensor(pdev, thermal, &thermal->sensors[i], @@ -1614,6 +1617,9 @@ static void rockchip_thermal_remove(struct platform_device *pdev) }
thermal->chip->control(thermal->regs, false); + + if (thermal->tshut_mode == TSHUT_MODE_GPIO) + pinctrl_pm_select_sleep_state(&pdev->dev); }
static int __maybe_unused rockchip_thermal_suspend(struct device *dev) @@ -1629,7 +1635,8 @@ static int __maybe_unused rockchip_thermal_suspend(struct device *dev) clk_disable(thermal->pclk); clk_disable(thermal->clk);
- pinctrl_pm_select_sleep_state(dev); + if (thermal->tshut_mode == TSHUT_MODE_GPIO) + pinctrl_pm_select_sleep_state(dev);
return 0; } @@ -1674,7 +1681,8 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev) for (i = 0; i < thermal->chip->chn_num; i++) rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
- pinctrl_pm_select_default_state(dev); + if (thermal->tshut_mode == TSHUT_MODE_GPIO) + pinctrl_pm_select_default_state(dev);
return 0; }
On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov alchark@gmail.com wrote:
On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 11:25, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
if (thermal->tshut_mode == TSHUT_MODE_GPIO)pinctrl_select_default_state(dev);elsepinctrl_select_sleep_state(dev);I believe no 'else' block is needed here, because if tshut_mode is not TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's no reason for the driver to mess with its pinctrl state. I'd rather put a mirroring block to put the pin back to its 'sleep' state in the removal function for the TSHUT_MODE_GPIO case.
You're right, but the "else block" is what the downstream driver does,
Does it though? It only handles the TSHUT_MODE_GPIO case as far as I can tell (or TSHUT_MODE_OTP in downstream driver lingo) [1]
[1] https://github.com/radxa/kernel/blob/edb3eeeaa4643ecac6f4185d6d391c574735fca...
so I think it's better to simply stay on the safe side and follow that logic in the upstream driver. Is it really needed? Perhaps not, but it also shouldn't hurt.
Will try and revert.
Awesome, thanks!
P.S. Just looked at the downstream driver, and it actually calls TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was not a typo in the first place. So maybe the right approach here is not to change the device tree but rather fix the "gpio" / "otpout" pinctrl state handling in the driver.
Indeed, "otpout" wasn't a typo, and I've already addressed that in my comments to Alexander's patch. Will send that response a bit later.
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
Anyway, I've just tried it after including the changes below, and while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected pinctrls under tsadc, the driver still doesn't seem to be triggering a PMIC reset. Weird. Any thoughts welcome.
I found the culprit. "otpout" (or "default" if we follow Alexander's suggested approach) pinctrl state should refer to the &tsadc_shut_org config instead of &tsadc_shut - then the PMIC reset works.
Best, Alexey
On 2025-01-24 20:44, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov alchark@gmail.com wrote:
On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 11:25, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote: > > There is no pinctrl "gpio" and "otpout" (probably designed as > "output") > handling in the tsadc driver. > Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
if (thermal->tshut_mode == TSHUT_MODE_GPIO)pinctrl_select_default_state(dev);elsepinctrl_select_sleep_state(dev);I believe no 'else' block is needed here, because if tshut_mode is not TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's no reason for the driver to mess with its pinctrl state. I'd rather put a mirroring block to put the pin back to its 'sleep' state in the removal function for the TSHUT_MODE_GPIO case.
You're right, but the "else block" is what the downstream driver does,
Does it though? It only handles the TSHUT_MODE_GPIO case as far as I can tell (or TSHUT_MODE_OTP in downstream driver lingo) [1]
[1] https://github.com/radxa/kernel/blob/edb3eeeaa4643ecac6f4185d6d391c574735fca...
so I think it's better to simply stay on the safe side and follow that logic in the upstream driver. Is it really needed? Perhaps not, but it also shouldn't hurt.
Will try and revert.
Awesome, thanks!
P.S. Just looked at the downstream driver, and it actually calls TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was not a typo in the first place. So maybe the right approach here is not to change the device tree but rather fix the "gpio" / "otpout" pinctrl state handling in the driver.
Indeed, "otpout" wasn't a typo, and I've already addressed that in my comments to Alexander's patch. Will send that response a bit later.
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
Anyway, I've just tried it after including the changes below, and while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected pinctrls under tsadc, the driver still doesn't seem to be triggering a PMIC reset. Weird. Any thoughts welcome.
I found the culprit. "otpout" (or "default" if we follow Alexander's suggested approach) pinctrl state should refer to the &tsadc_shut_org config instead of &tsadc_shut - then the PMIC reset works.
Huh, thanks for debugging, but this is quite confusing. Let me dig through everything again later today.
Hello.
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
Anyway, I've just tried it after including the changes below, and while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected pinctrls under tsadc, the driver still doesn't seem to be triggering a PMIC reset. Weird. Any thoughts welcome.
I found the culprit. "otpout" (or "default" if we follow Alexander's suggested approach) pinctrl state should refer to the &tsadc_shut_org config instead of &tsadc_shut - then the PMIC reset works.
Great, I'll use this in v2.
Thanks!
Hello Alexander,
On 2025-01-26 15:25, Alexander Shiyan wrote:
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
Anyway, I've just tried it after including the changes below, and while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected pinctrls under tsadc, the driver still doesn't seem to be triggering a PMIC reset. Weird. Any thoughts welcome.
I found the culprit. "otpout" (or "default" if we follow Alexander's suggested approach) pinctrl state should refer to the &tsadc_shut_org config instead of &tsadc_shut - then the PMIC reset works.
Great, I'll use this in v2.
Please, let's wait with the v2 until I go through the whole thing again, which I expected to have done already, but had some other "IRL stuff" that introduced a delay.
On Tue, Jan 28, 2025 at 1:24 PM Dragan Simic dsimic@manjaro.org wrote:
Hello Alexander,
On 2025-01-26 15:25, Alexander Shiyan wrote:
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
Anyway, I've just tried it after including the changes below, and while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected pinctrls under tsadc, the driver still doesn't seem to be triggering a PMIC reset. Weird. Any thoughts welcome.
I found the culprit. "otpout" (or "default" if we follow Alexander's suggested approach) pinctrl state should refer to the &tsadc_shut_org config instead of &tsadc_shut - then the PMIC reset works.
Great, I'll use this in v2.
Please, let's wait with the v2 until I go through the whole thing again
I, for one, would welcome a v2 that could be tested and confirmed working with and without driver changes. Especially given that: - the changes are pretty small - hardware docs say nothing about the difference between TSADC_SHUT vs. TSADC_SHUT_ORG, except that one is config #2 and the other is config #1 - none of the source trees I looked at seem to enable PMIC based resets on any RK3588-based boards, so these pinctrl configs appear to have never been tested in the wild for RK3588*
So trying and testing seems to be the only way to understand the best way forward. Unless, of course, someone from Rockchip can comment on how the hardware works with TSADC_SHUT vs. TSADC_SHUT_ORG.
Best regards, Alexey
which I expected to have done already, but had some other "IRL stuff" that introduced a delay.
Hello Alexey,
On 2025-01-28 11:30, Alexey Charkov wrote:
On Tue, Jan 28, 2025 at 1:24 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-26 15:25, Alexander Shiyan wrote:
I think it's actually better to accept the approach in Alexander's patch, because the whole thing applies to other Rockchip SoCs as well, not just to the RK3588(S).
Anyway, I've just tried it after including the changes below, and while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected pinctrls under tsadc, the driver still doesn't seem to be triggering a PMIC reset. Weird. Any thoughts welcome.
I found the culprit. "otpout" (or "default" if we follow Alexander's suggested approach) pinctrl state should refer to the &tsadc_shut_org config instead of &tsadc_shut - then the PMIC reset works.
Great, I'll use this in v2.
Please, let's wait with the v2 until I go through the whole thing again
I, for one, would welcome a v2 that could be tested and confirmed working with and without driver changes. Especially given that:
- the changes are pretty small
- hardware docs say nothing about the difference between TSADC_SHUT
vs. TSADC_SHUT_ORG, except that one is config #2 and the other is config #1
- none of the source trees I looked at seem to enable PMIC based
resets on any RK3588-based boards, so these pinctrl configs appear to have never been tested in the wild for RK3588*
So trying and testing seems to be the only way to understand the best way forward. Unless, of course, someone from Rockchip can comment on how the hardware works with TSADC_SHUT vs. TSADC_SHUT_ORG.
Perhaps the best approach would be to have the v2 that works without the driver changes, so it can be propagated into the stable kernels.
Then, a separate series, which I'll volunteer for, :) would introduce the cleanups and any needed driver changes, which wouldn't be propagated into the stable kernels. That way, we'd have the minimal bugfixes in stable kernels, and the nice cleanups in the latest kernel version.
Of course, detailed testing is mandatory.
which I expected to have done already, but had some other "IRL stuff" that introduced a delay.
Hello Alexey,
On 2025-01-24 18:23, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 11:25, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic dsimic@manjaro.org wrote:
On 2025-01-24 09:33, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan eagle.alexander923@gmail.com wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
This looks reasonable, however I've tried it on my Radxa Rock 5C and the driver still doesn't claim GPIO0 RK_PA1 even with this change. As a result, a simulated thermal runaway condition (I've changed the tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC reset, even though a direct `gpioset 0 1=0` does.
Are any additional changes needed to the driver itself?
I've been digging through this patch the whole TSADC/OTP thing in the last couple of hours, and AFAIK some parts of the upstream driver are still missing, in comparison with the downstream driver.
I've got some small suggestions for the patch itself, but the issue you observed is obviously of higher priority, and I've singled it out as well while digging through the code.
Could you, please, try the patch below quickly, to see is it going to fix the issue you observed? I've got some "IRL stuff" to take care of today, so I can't test it myself, and it would be great to know is it the right path to the proper fix.
diff --git i/drivers/thermal/rockchip_thermal.c w/drivers/thermal/rockchip_thermal.c index f551df48eef9..62f0e14a8d98 100644 --- i/drivers/thermal/rockchip_thermal.c +++ w/drivers/thermal/rockchip_thermal.c @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->initialize(thermal->grf, thermal->regs, thermal->tshut_polarity);
if (thermal->tshut_mode == TSHUT_MODE_GPIO)pinctrl_select_default_state(dev);elsepinctrl_select_sleep_state(dev);I believe no 'else' block is needed here, because if tshut_mode is not TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's no reason for the driver to mess with its pinctrl state. I'd rather put a mirroring block to put the pin back to its 'sleep' state in the removal function for the TSHUT_MODE_GPIO case.
You're right, but the "else block" is what the downstream driver does,
Does it though? It only handles the TSHUT_MODE_GPIO case as far as I can tell (or TSHUT_MODE_OTP in downstream driver lingo) [1]
[1] https://github.com/radxa/kernel/blob/edb3eeeaa4643ecac6f4185d6d391c574735fca...
Ah, you're right. Somehow I saw something that actually wasn't there, so the else block would indeed be redundant.
so I think it's better to simply stay on the safe side and follow that logic in the upstream driver. Is it really needed? Perhaps not, but it also shouldn't hurt.
Hello Alexander,
On 2025-01-24 06:26, Alexander Shiyan wrote:
There is no pinctrl "gpio" and "otpout" (probably designed as "output") handling in the tsadc driver. Let's use proper binding "default" and "sleep".
Fixes: 32641b8ab1a5 ("arm64: dts: rockchip: add rk3588 thermal sensor") Cc: stable@vger.kernel.org Signed-off-by: Alexander Shiyan eagle.alexander923@gmail.com
arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi index a337f3fb8377..f141065eb69d 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi @@ -2667,9 +2667,9 @@ tsadc: tsadc@fec00000 { rockchip,hw-tshut-temp = <120000>; rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU 1:GPIO */ rockchip,hw-tshut-polarity = <0>; /* tshut polarity 0:LOW 1:HIGH */
pinctrl-0 = <&tsadc_gpio_func>;pinctrl-1 = <&tsadc_shut>;pinctrl-names = "gpio", "otpout";
pinctrl-0 = <&tsadc_shut>;pinctrl-1 = <&tsadc_gpio_func>;pinctrl-names = "default", "sleep";
Thanks for the patch, it's looking good to me. The old values for the pinctrl names are leftovers back from the import of the downstream kernel code, while the new values follow the expected pinctrl naming scheme. The resulting behavior follows, almost entirely, the behavior found in the downstream kernel code.
Actually, there's some rather critical discrepancy between the upstream TSADC driver and it's downstream cousin, as already described in earlier responses from Alexey and me. However, those issues have to be addressed in a separate patch, while this patch, to me, remains fine on its own.
My only suggestions would be to adjust both the patch summary and the description not to use word "binding", because that technically isn't fixed here, but to use "pinctrl names" instead. Also, please note that the downstream kernel uses "otpout" as a pinctrl name, [1] so the assumption about "output" in the patch description should be removed.
With the suggestions from above addressed in the v2, please feel free to include my
Reviewed-by: Dragan Simic dsimic@manjaro.org
[1] https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-5...
linux-stable-mirror@lists.linaro.org
-
Alexander Shiyan -
Alexey Charkov -
Dragan Simic