On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@xxxxxxxxxxx> wrote: > > On 2025-01-24 11:25, Alexey Charkov wrote: > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@xxxxxxxxxxx> > > wrote: > >> On 2025-01-24 09:33, Alexey Charkov wrote: > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan > >> > <eagle.alexander923@xxxxxxxxx> 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); > > > > 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/edb3eeeaa4643ecac6f4185d6d391c574735fca1/drivers/thermal/rockchip_thermal.c#L2564 > 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; }
