Re: [PATCH v2] arm64: dts: rockchip: Turn all LEDs on at boot for Radxa boards

["Diederik de Haas" <diederik@cknow-tech.com>]

Hi,

On Wed Nov 12, 2025 at 12:36 PM CET, FUKAUMI Naoki wrote:
> My goal is to minimize the DTS fragments 
> (u-boot/arch/arm/dts/*-u-boot.dtsi) in U-Boot, consolidate them 
> upstream, and improve clarity/visibility.

Long story short: I was wrong.

It was mostly Quentin's argument/reminder that the DT is about
describing the hardware.
I think Quentin's point about 'initial state' was right on point,
certainly from my perspective and where my logic error came from.

Cheers,
  Diederik

> On 11/12/25 19:34, Quentin Schulz wrote:
>> On 11/12/25 10:40 AM, Diederik de Haas wrote:
>>> [You don't often get email from diederik@cknow-tech.com. Learn why 
>>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> On Wed Nov 12, 2025 at 12:42 AM CET, FUKAUMI Naoki wrote:
>>>> On 11/12/25 03:32, Quentin Schulz wrote:
>>>>> On 11/11/25 5:14 PM, Dragan Simic wrote:
>>>>>> On Tuesday, November 11, 2025 16:32 CET, FUKAUMI Naoki
>>>>>> <naoki@radxa.com> wrote:
>>>>>>> On 11/11/25 23:46, Dragan Simic wrote:
>>>>>>>> On Tuesday, November 11, 2025 14:07 CET, "Diederik de Haas"
>>>>>>>> <diederik@cknow-tech.com> wrote:
>>>>>>>>> On Tue Nov 11, 2025 at 6:41 AM CET, FUKAUMI Naoki wrote:
>>>>>>>>>> Radxa's boards turn all LEDs on at boot(loader), but some boards
>>>>>>>>>> don't have `default-state` property in Linux kernel tree but
>>>>>>>>>> have it in U-Boot tree instead[1].
>>>>>>>>>>
>>>>>>>>>> This patch adds `default-state = "on"` for (almost) all LEDs 
>>>>>>>>>> (with a
>>>>>>>>>> few exceptions which should be "off" such as RGB LEDs on E25 
>>>>>>>>>> and LAN/
>>>>>>>>>> WAN LEDs on E20C/E52C).
>>>>>>>>>
>>>>>>>>> I'm missing the *why* these changes would be an improvement.
>>>>>>>>>
>>>>>>>>> Personally, for both 'heartbeat' and 'netdev' triggers, I want 
>>>>>>>>> them to
>>>>>>>>> be off by default and once it gets a 'heartbeat' or a 'netdev'
>>>>>>>>> trigger, THEN I want the LED to be on/blinking.
>>>>>>>>
>>>>>>>> That's a good question for Naoki.  My own preference would also
>>>>>>>> be to have the device's power LED turned on by U-Boot as quickly
>>>>>>>> as possible after supplying power to the board or turning it on
>>>>>>>> by pressing the power button.  I'm actually not a big fan of
>>>>>>>> having all the LEDs shining for a couple of seconds or so, which
>>>>>>>> may actually look like some error condition to me.
>>>>>>>>
>>>>>>>> Having all that in mind, I may suggest that just the U-Boot's
>>>>>>>> behavior is changed to turn the power LEDs on only.
>>>>>>>
>>>>>>> I can't quite explain it, but...
>>>>>>>
>>>>>>> - 1st (Power) LED
>>>>>>>
>>>>>>> The 1st (power) LED turns on automatically/immediately without 
>>>>>>> software
>>>>>>> intervention. (On some boards, this LED cannot be controlled by 
>>>>>>> software
>>>>>>> at all.)
>
> I'm not saying the DTS has anything about LEDs that can't be controlled 
> by software, nor am I trying to add such a thing to the DTS.
>
> I'm just pointing out that the power LED is always on right after 
> power-up. This makes it useless for determining if the software is running.
>
>>>>>>> In DTS, this should be described using `default-state = "on"`. The 
>>>>>>> use
>>>>>>> of the Linux-specific property `linux,default-trigger = "default- 
>>>>>>> on"` is
>>>>>>> unsuitable for non-Linux environments.
>>>>>>>
>> 
>> I think the wording in the binding can be understood two ways.
>> 
>> The binding says the following about the default-state property:
>> 
>> """
>>        The initial state of the LED. If the LED is already on or off and 
>> the
>>        default-state property is set the to same value, then no glitch 
>> should be
>>        produced where the LED momentarily turns off (or on). The "keep" 
>> setting
>>        will keep the LED at whatever its current state is, without 
>> producing a
>>        glitch.
>> """
>> 
>> I think the issue here is around the meaning of "initial state". I 
>> believe Naoki is probably thinking about the **HW** initial state of the 
>> LED, which is whatever is the state of the LED without SW control. I 
>> think Diederik is thinking about this being the state of the LED right 
>> when the SW takes over and configures the LED before the trigger is setup.
>> 
>> In the first interpretation, there's no need for an "improvement" for 
>> the patches as they would just fix correctness of the DT wrt HW state at 
>> boot.
>> 
>> In the second interpretation, a change of this value must be justified 
>> as people will simply disagree forever and we could end up with people 
>> reverting other people's patches after each release. If it's just a 
>> matter of taste, I believe the typical answer is keeping the status quo.
>> 
>> We should find a way to make this binding not up to interpretation.
>> 
>> Additionally, if the LED cannot be controlled on some boards, I don't 
>> think it should be part of the DT.
>> 
>>>>>>> - 2nd (Heartbeat) LED
>>>>>>>
>>>>>>> The 2nd (heartbeat) LED can be controlled by software. It should 
>>>>>>> be lit
>>>>>>> up as quickly as possible to indicate that the very first software
>>>>>>> (e.g., the bootloader) is running.
>>>>>>>
>> 
>> My understanding is Naoki wants to use default-state = on, for the 
>> bootloader to turn it on as soon as it takes over control of the LEDs.
>> 
>>>>>>> On Linux, usually this is used as `linux,default-trigger = 
>>>>>>> "heartbeat"`.
>>>>>>> It indicates that kernel is running (regardless of the `default- 
>>>>>>> state`
>>>>>>> setting), and its behavior can be modified in user space.
>>>>>>
>>>>>> As discussed already in the #linux-rockchip IRC channel, [1] perhaps
>>>>>> the best option would be to have the power LEDs turned on as quickly
>>>>>> upon powering on the board as possible, and to have U-Boot pulsate
>>>>>> the heartbeat LEDs using the LED_BOOT feature.  In such a scenario,
>>>>>> no other LEDs would be turned on early, and the LED-related DT parts
>>>>>> specific to U-Boot would be migrated to the kernel DTs.
>>>>>>
>>>>>> [1] https://libera.catirclogs.org/linux-rockchip/2025-11-11#38997824;
>>>>>
>>>>> The LED_BOOT feature (guarded by the Kconfig symbol of the same 
>>>>> name) in
>>>>> U-Boot only applies if /options/u-boot/boot-led property is set.
>>>>
>>>> For the default state of the heartbeat LED, I'm thinking of using
>>>> LED_BOOT (/options/u-boot/boot-led), but I'm concerned that this is
>>>> U-Boot-specific.
>>>
>>> If U-Boot wants to use the heartbeat LED to signal the *bootloader* is
>>> running, I guess that's fine. And if you want to make it solid or
>>> blinking, that seems best discussed on the U-Boot ML.
>>>
>> 
>> The solution may still involve configuring the Device Tree, and we're 
>> trying to have U-Boot-specific changes to the Device Tree in U-Boot 
>> source tree to a minimum.
>> 
>>> I still consider the bootloader and the kernel stages separate.
>> 
>> They do however share most of their Device Tree (for Rockchip at least) 
>> and the least (ideally no) changes we can have in U-Boot the better.
>> 
>>> And I haven't seen an argument why I should change *my* opinion on the
>>> heartbeat and netdev triggers (default-state) wrt the kernel.
>>>
>> 
>> Device Tree is not kernel specific as you said already.
>> 
>>> I don't think that what U-Boot does or doesn't do, should determine what
>>> the Linux kernel does or doesn't do.
>> 
>> It shouldn't, but (most of) the Device Tree is shared, so you cannot 
>> just dismiss U-Boot behavior when talking about Linux behavior based on 
>> Device Tree interpretation. We may have a need for a bootloader-specific 
>> property. We have a Linux-specific one after all (linux,default- 
>> trigger). Though... that does seem to be on the edge of what the DT is 
>> made for (description of the HW, not logic/policy).
>> 
>>> I have no plans to use another bootloader then U-Boot, but it's possible
>>> that people do, so what the Linux kernel does should be independent from
>>> what the/a specific bootloader does.
>
> Each software should be independent, but hardware (state) cannot be 
> independent.
>
>> Barebox also uses upstream DT as far as I know and supports some Radxa 
>> products (Rock 5B/5T/..., CM3, Rock (RK3188), Rock 3A from the arch/arm/ 
>> boards/radxa-* directories). Zephyr has support for RK3568, RK3588, and 
>> other SoCs, and uses upstream DT as well.
>> 
>> Again, we're talking about modifications of the Device Tree here, so 
>> typically I would expect all consumers of that DT to be interpreting the 
>> properties the same way, except if you have OS-specific properties/nodes 
>> (think u-boot,config-compatible nodes, linux, prefixed properties, 
>> bootph- properties, ...).
>> 
>>> And as I said before, *I* want LEDs with netdev and heartbeat triggers,
>>> to be off (at the start, which is indeed the default value).
>
> If you are using U-Boot, heartbeat LED is already on by U-Boot,
> e.g.
>   https://source.denx.de/u-boot/u-boot/-/blob/v2025.10/arch/arm/dts/rk3588s-rock-5a-u-boot.dtsi#L10-12
>
> But it's not visible in DTS in Linux,
> e.g.
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts?h=v6.17#n55
>
> I think this situation should be fixed.
>
> Best regards,
>
> --
> FUKAUMI Naoki
> Radxa Computer (Shenzhen) Co., Ltd.
>
>>> I use the heartbeat trigger to:
>>> 1) See the kernel has started (and has gotten to the point the heartbeat
>>>     'infrastructure' has been set up
>>> 2) Wait for the blinking to slow down as that (generally) means it's
>>>     pretty much done with the boot process and the SSH server should
>>>     probably be running then, so I can login
>>> 3) When the heartbeat LED is solid, that means the system has crashed
>>>     (f.e. due to overheating ...)
>>>
>> 
>> If the *HW* default state of the LED is off and the default-state 
>> property is off, then you won't be able to tell apart a completely 
>> bricked board and one that is stuck somewhere between U-Boot proper and 
>> the Linux kernel taking over that LED.
>> 
>>> And also, if you're going to change/override other people's choices, a
>>> motivation as to why would be 'nice'.
>>>
>>>>> <more discussion about LED functionality in U-Boot ...>
>>>>
>>>> As you know, default "default-state" is "off".
>>>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/ 
>>>> linux.git/tree/Documentation/devicetree/bindings/leds/common.yaml? 
>>>> h=v6.17#n74
>>>>
>>>> As far as I understand, there should not be any workarounds for specific
>>>> implementations.
>>>>    https://lore.kernel.org/linux-rockchip/3389401.44csPzL39Z@phil/
>>>>
>>>> So removing `default-state = "off"` is acceptable, right?
>>>
>>> I don't see/understand the connection with 'workarounds for specific
>>> implementations' with removing ``default-state = "off"``.
>>>
>>> IMO it's perfectly fine to remove ``default-state = "off"``, although
>>> having it explicitly may be useful, especially if the commit that set
>>> that property specified *why* it should be "off".
>>>
>> 
>> The status property defaults to okay, and we do not want them to be 
>> listed explicitly. Not sure if there's consensus on applying this to all 
>> properties which have defaults, across all subsystems.
>> 
>>> Relatedly, when a node does not have the 'default-state' property, I
>>> would _assume_ the author wanted/intended it to be "off". Ideally it
>>> would be described in the commit message, but that is optional.
>> 
>> The lack of a property doesn't necessarily mean it was forgotten, agreed.
>> 
>>> But if that is changed, then it should be motivated *why*.
>>>
>> 
>> Agreed.
>> 
>> Cheers,
>> Quentin
>> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip