Message-ID: <CABjd4Yx9rt2W=MhCSyO5vaxndD1jvGHNWsz7J=HnvnJcgOvQHw@mail.gmail.com>
Date: Sat, 4 Oct 2025 15:41:41 +0400
From: Alexey Charkov <alchark@...il.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Dragan Simic <dsimic@...jaro.org>, 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 Sat, Oct 4, 2025 at 3:29 AM Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Alexey,
>
> On Fri, Oct 03, 2025 at 06:55:26PM +0400, Alexey Charkov wrote:
> > On Fri, Oct 3, 2025 at 6:13 PM Alexey Charkov wrote:
> > > On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart wrote:
> > > > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@...il.com> wrote:
> > > > > > 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.
> > > > >
> > > > > 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.
> > > >
> > > > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > > > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > > > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > > > of the CM5 to GND (see page 3 of the schematics in [3]).
> > > >
> > > > With &tsadc_shut_org the reset button has absolutely no effect. With
> > > > &tsadc_shut it resets the board as expected.
> > >
> > > Interesting. The TSADC shouldn't affect the physical button operation
> > > at all, if it's really wired to the PMIC as the signal name implies.
> > > There isn't even any default pull value associated with the TSHUT pin
> > > config.
> >
> > On a second thought, I've got another hypothesis. Your baseboard only
> > pulls the reset line through  a 100 Ohm resistor when the button is
> > pressed. So if the TSHUT pin is in its default push-pull mode and
> > stays high when no thermal runaway reset is requested, the reset
> > button won't pull the line fully to zero, as the TSHUT line pulls it
> > high at the same time.
>
> That's the most likely cause, I agree.
>
> > If you switch it from &tsadc_shut_org to &tsadc_shut, then it stops
> > working properly as the thermal protection reset, and GPIO0_A1 remains
> > high-impendance, thus allowing the reset button to function even
> > though its pull is too weak.
>
> By the way, what is the difference between tsadc_shut_org and tsadc_shut
> ? I haven't seen it being clearly documented in the TRM.

No idea frankly. Looks like a half-finished design change to me, which
left the non-"org" version unconnected internally.

> > So maybe change the pin configuration of &tsadc_shut_org in
> > rk3588-base-pinctrl.dtsi to open drain and retry?
>
> That's a good idea, but... how ? The pinctrl-rockchip driver doesn't
> seem to support generic open-drain configuration.

I thought I saw open-drain configurations here, but after reviewing
the TRM, bindings and the driver it turns out I must have been
daydreaming :( Sorry.

Looks like the best we can try is a lower drive strength while keeping
the push-pull mode, but I'm afraid this 100 Ohm pulldown is too weak,
because the lowest TSHUT drive strength Rockchip offers is 100 Ohm,
while the PMIC would only count anything below 30% reference voltage
as logical low. Maybe adding a pulldown to the pin config can help,
but most likely this board will require switching the pin to GPIO
input for high-z, and switching the TSHUT mode to CRU.

So how about something like this first:

&tsadc_shut_org {
        rockchip,pins = <0 RK_PA1 1 &pcfg_pull_down_drv_level_0>;
};

Best regards,
Alexey

Powered by blists - more mailing lists