| 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
| ||
|
Message-ID: <CABjd4Yx0p0B=e00MjCpDDq8Z=0FtM0s9EN86WdvRimt-_+kh2w@mail.gmail.com>
Date: Fri, 24 Jan 2025 21:23:18 +0400
From: Alexey Charkov <alchark@...il.com>
To: Dragan Simic <dsimic@...jaro.org>
Cc: Alexander Shiyan <eagle.alexander923@...il.com>, Rob Herring <robh@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>, devicetree@...r.kernel.org,
Sebastian Reichel <sebastian.reichel@...labora.com>, stable@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
Krzysztof Kozlowski <krzk+dt@...nel.org>, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] arm64: dts: rockchip: Fix broken tsadc pinctrl binding
for rk3588
On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@...jaro.org> wrote:
>
> On 2025-01-24 11:25, Alexey Charkov wrote:
> > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@...jaro.org>
> > wrote:
> >> On 2025-01-24 09:33, Alexey Charkov wrote:
> >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> >> > <eagle.alexander923@...il.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);
> >
> > 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;
}
Powered by blists - more mailing lists