Message-ID: <a0202e76-4931-4061-9915-5fae50d3abdf@cherry.de>
Date: Thu, 5 Jun 2025 11:05:53 +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

On 6/5/25 10:57 AM, Quentin Schulz wrote:
> 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?).
> 

I have been told by you in private that I mixed 
regulator-enable-ramp-delay and regulator-ramp-delay for that, so just 
acknowledging it publicly :) You can ignore the above paragraph :)

>> 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) 

Based on the above revelation, we should add this enable-ramp-delay to 
that DT too I believe.

> 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?
> 

Read regulator-enable-ramp-delay here instead but still applicable.

Quentin

Powered by blists - more mailing lists