Message-ID: <c6fac95c-dd7a-4fdc-a93f-8ebac731d499@rock-chips.com>
Date: Thu, 8 Jan 2026 18:41:46 +0800
From: Chaoyi Chen <chaoyi.chen@...k-chips.com>
To: Quentin Schulz <quentin.schulz@...rry.de>, 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 Quentin,

On 1/8/2026 6:07 PM, Quentin Schulz wrote:
> 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.
>

Oh, it make sense.

>>>> +    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 :) ).
>

Yes, this seems unnecessary, which is why I'm asking if you've
encountered similar designs before.

>>>> +    };
>>>> +
>>>> +    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.
>

Thank you for provide these info. I will try to add them in v3.

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

Oh! I did overlook that point! I will try to do it in v3.

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

No, the "vcc5v0_device" serves as the vin-supply for both
"VCC5V0_USB3_HOST0" and "VCC5V0_USB3_HOST1". 
And "VCC5V0_USB3_HOSTx" is supply for USB VBUS.

All the names marked here are those shown in the schematic. As you
suggested, it would be better to set regulator-name to the schematic
name "VCC5V0_USB3_HOSTx" and use "usbx_vbus" for the label.

-- 
Best, 
Chaoyi

Powered by blists - more mailing lists