Message-ID: <e4a1bf0a-0fc5-4c15-96dc-7e9c7ae3ccd5@cherry.de>
Date: Thu, 5 Jun 2025 10:57:13 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Heiko Stuebner <heiko@...ech.de>
Cc: jonas@...boo.se, dsimic@...jaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: rockchip: add regulator-enable-ramp-delay to
 RK860-2/-3 regulators

Hi Heiko,

On 6/4/25 10:24 PM, Heiko Stuebner wrote:
> The RK860-2/-3 regulators are used on rk3588 boards to supply components
> like the big cpu-clusters and npu individually.
> 
> Most of these things will be, and in fact most regulator nodes right now are,
> always-on - probably nobody has tried completely turning of the big clusters
> for example.
> 

This is a bit of a confusing wording here. If most things will be (and 
are) always-on, then the ramp-up typically shouldn't be an issue I 
assume? I'm not too familiar with the regulator subsystem but I guess 
for the first initial enable this could be an issue?

> This changed with the new NPU driver, which does combined runtime-suspends
> with the regulator being tied to the power-domain (it supplies the pd).
> 
> If the NPU runtime-suspends while running a load and then starts again
> hangs can be observed with messages like
> 
>    rockchip-pm-domain fd8d8000.power-management:power-controller: failed to set domain 'nputop' on, val=0
> 
> Removing the regulator from the domain and instead setting it to always-on
> "fixes" the issue, which suggests that the domain is trying to get current
> from the regulator before it is ready when it gets turned back on.
> 

If I'm not mistaken, this is also misleading as nothing currently uses 
that (NPU support not merged yet and most if not all NPU regulators 
currently are always-on so typically not impacted by this issue).

I assume this will be an issue the moment we add support for suspending 
the NPU regulator, which would anyway require modification of the 
various DT. Is that correct?

> And in fact the datasheet for the regulator defines an "Internal soft-start
> time". For a target output voltage of 1.0V the _typical_ time to reach at
> least 92% of the output is given as 260uS.
> 

Indeed. Now looking at the existing Device Trees, it seems some set the 
ramp-up delay already, but to 2300 and not 500 like suggested here. 
Maybe it'd be safer to go for 2300 by default then? This could also be 
the typical "this Device Tree got merged, we use the same node, so we 
simply copy it" case and not really backed by anything (though I would 
hope the Toybrick and Rockchip evaluation boards values are based on 
*something* since Rockchip would (could) have done measurements for those?).

> So that value is dependent on the target voltage (up to 1.5V for the type)
> and also the rest of the board design.
> 
> So add a regulator-enable-ramp-delay to all rk860-2/-3 nodes we have in

Maybe mention that there's currently no rk8601 node upstream, and the 
only rk8600 (arch/arm64/boot/dts/rockchip/rk3566-radxa-zero-3.dtsi) 
already has a ramp-up delay configured. Otherwise reading this I'm 
wondering why the rk860-0 and rk860-1 while being handled by the same 
datasheet are implicitly excluded from this change.

> mainline right now. I've chosen 500uS to be on the safe side, as
> 260uS is the "typical" value for 1.0V and sadly no max value is given
> in the datasheet.
> 

Reading the rk808 regulator driver... maybe we should also set the 
typical delay as default in the fan53555.c driver? See rk805_reg which 
sets the enable_time for some (typically the LDO with 400 and the DCDC 
at 0). I assume those can be overridden from the DT anyway, but at least 
we would have some decently safe defaults?

If we do not do this, then we should probably force the presence of 
regulator-ramp-delay property for the rk860x DT binding so we don't 
forget for future Device Trees?

Cheers,
Quentin

Powered by blists - more mailing lists