Message-ID: <0d99140e-8f57-4a7a-a492-92efa30292d8@cherry.de>
Date: Thu, 8 Jan 2026 11:07:45 +0100
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Chaoyi Chen <chaoyi.chen@...k-chips.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>, Jonas Karlman <jonas@...boo.se>,
 John Clark <inindev@...il.com>, FUKAUMI Naoki <naoki@...xa.com>,
 Jimmy Hon <honyuenkwun@...il.com>, Dragan Simic <dsimic@...jaro.org>,
 Michael Riesch <michael.riesch@...labora.com>,
 Peter Robinson <pbrobinson@...il.com>, Alexey Charkov <alchark@...il.com>,
 Shawn Lin <shawn.lin@...k-chips.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Andy Yan <andy.yan@...k-chips.com>, Chaoyi Chen <kernel@...kyi.com>
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] arm64: dts: rockchip: Add rk3576 evb2 board

Hi Chaoyi,

On 1/8/26 7:27 AM, Chaoyi Chen wrote:
> Hi Quentin,
> 
> Thank you for your patient review.
> 
> On 1/7/2026 11:46 PM, Quentin Schulz wrote:
>> Hi Chaoyi,
>>
>> On 1/7/26 8:03 AM, Chaoyi Chen wrote:

[...]

>>> +        stdout-path = "serial0:1500000n8";
>>> +    };
>>> +
>>> +    adc_keys: adc-keys {
>>
>> Are we expecting to extend this node from another DT? Why the label?
>>
>> Won't comment on all other labeled-but-no-phandle-use instances, please check.
>>
> 
> I think one exception is the regulator labels. Even though their
> phandles are unused, they match the names on the schematic, so it
> seems meaningful to keep them.
> 

Can't you use the regulator-name property for that? Or a comment if you 
realllly want it to be somewhere in the DTS? If I remember correctly, 
labels make it to the DTB when building it with symbols (-@, enabled 
whenever there's a DTSO for the board; see __symbols__ node when 
decompiling), this will unnecessarily bloat the final DTB.

>>> +    vcc3v3_rtc_s5: regulator-vcc3v3-rtc-s5 {
>>> +        compatible = "regulator-fixed";
>>> +        regulator-name = "vcc3v3_rtc_s5";
>>> +        regulator-boot-on;
>>> +        regulator-always-on;
>>> +        regulator-min-microvolt = <3300000>;
>>> +        regulator-max-microvolt = <3300000>;
>>> +        vin-supply = <&vcc_sys>;
>>
>> If this is for the rtc, shouldn't we declare this dependency in the RTC device node and not have it always-on?
>>
> 
> I checked other boards that use the hym8563 device and couldn't find
> a similar approach. Could you give an example?
> 

If this is truly always on by hardware design, then I guess it's "fine". 
That other boards aren't doing isn't much of an argument, as they may 
not need it. Typically, you could need to update the driver (and its 
binding) to accept and control power supplies so that you can link the 
two together in the device tree. I'm assuming for an RTC it doesn't make 
much sense to have its power supply controllable as it likely needs to 
be powered even when the device is turned off (otherwise the RTC stops 
counting :) ).

>>> +    };
>>> +
>>> +    vcc3v3_sata_pwren: vcc3v3-sata-pwren {
>>> +        compatible = "regulator-fixed";
>>> +        regulator-name = "vcc3v3_sata_pwren";
>>> +        enable-active-high;
>>> +        regulator-boot-on;
>>> +        regulator-always-on;
>>
>> Why do we have this always-on? Seems like we're missing a dependency on this regulator in the SATA controller?
>>
> 
> In v1 we set the pinctrl inside the SATA node. To keep the pins from
> being reused by mistake we added this regulator in v2.
> 

This is a controllable regulator, which seems to be dedicated to SATA 
(from the name of it). Why isn't it something that the SATA controller 
controls?

You have ahci-supply, target-supply and phy-supply in ahci-common.yaml 
DT binding that may be appropriate for this regulator.

The issue is if this regulator is probed *after* your SATA device, SATA 
probably won't work. Imagine REGULATOR_FIXED_VOLTAGE=m (or even =n, but 
then you likely have other issues) and your SATA controller driver is 
built-in, this will be broken.

Remove the always-on and set the proper link in the SATA controller 
device node so that this is handled properly.

>>> +        gpio = <&gpio4 RK_PC7 GPIO_ACTIVE_HIGH>;
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&satapm_pwren>;
>>> +    };
>>> +
>>> +    vcc5v0_device: regulator-vcc5v0-device {
>>> +        compatible = "regulator-fixed";
>>> +        regulator-name = "vcc5v0_device";
>>> +        regulator-always-on;
>>> +        regulator-boot-on;
>>> +        regulator-min-microvolt = <5000000>;
>>> +        regulator-max-microvolt = <5000000>;
>>> +        vin-supply = <&vcc12v_dcin>;
>>> +    };
>>> +
>>> +    vcc5v0_host: regulator-vcc5v0-host {
>>> +        compatible = "regulator-fixed";
>>> +        regulator-name = "vcc5v0_host";
>>> +        regulator-boot-on;
>>> +        regulator-always-on;
>>> +        regulator-min-microvolt = <5000000>;
>>> +        regulator-max-microvolt = <5000000>;
>>> +        enable-active-high;
>>> +        gpio = <&gpio0 RK_PC3 GPIO_ACTIVE_HIGH>;
>>> +        vin-supply = <&vcc5v0_device>;
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&usb_host_pwren>;
>>> +    };
>>> +
>>
>> I assume both of the above are related to USB operating in host or device mode? Maybe there's a way to have something more useful to the user in regulator-name (and possibly the regulator node name) so that they have an idea what this pertains to?
> 
> It's a good idea. Actually, we have two regulators here, one for USB0
> and another for USB1. I'll try to rename them in v2.
> 

Are you sure? vcc5v0_device is a supply for vcc5v0_host, so it'd be odd 
that in order for USB1 to work, you need USB0 powered?

[...]

Cheers,
Quentin

Powered by blists - more mailing lists