Re: [PATCH] rockchip: boot_mode: fix download key detection

[Tianling Shen <cnsztl@gmail.com>]

Hi Quentin,

On 2026/6/26 19:52, Quentin Schulz wrote:
> Hi Tianling, Jonas,
> 
> On 6/11/26 9:45 AM, Tianling Shen wrote:
>> Hi Jonas,
>>
>> On 2026/6/10 21:25, Jonas Karlman wrote:
>>> Hi Tianling,
>>>
>>> On 6/10/2026 5:01 AM, Tianling Shen wrote:
>>>> rockchip_dnl_key_pressed() looks for the ADC device by checking
>>>> whether the device name starts with "saradc".
>>>>
>>>> On RK3328, RK3576, RK3588 etc., the SARADC node is named "adc@...",
>>>> so the device name no longer has the "saradc" prefix. As a result,
>>>> U-Boot fails to find the SARADC device and does not sample the
>>>> download key state.
>>>>
>>>> Do not rely on the DT node name. Match the bound Rockchip SARADC
>>>> driver instead, which works for both "saradc@..." and "adc@..."
>>>> node names.
>>>>
>>>> Signed-off-by: Tianling Shen <cnsztl@gmail.com>
>>>> ---
>>>>   arch/arm/mach-rockchip/boot_mode.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/mach-rockchip/boot_mode.c b/arch/arm/mach- 
>>>> rockchip/boot_mode.c
>>>> index 55e9456668ae..363fad523cbd 100644
>>>> --- a/arch/arm/mach-rockchip/boot_mode.c
>>>> +++ b/arch/arm/mach-rockchip/boot_mode.c
>>>> @@ -51,7 +51,7 @@ __weak int rockchip_dnl_key_pressed(void)
>>>>       ret = -ENODEV;
>>>>       uclass_foreach_dev(dev, uc) {
>>>> -        if (!strncmp(dev->name, "saradc", 6)) {
>>>> +        if (!strcmp(dev->driver->name, "rockchip_saradc")) {
>>>
>>> The adc channel used below may not fit all boards/SoCs, so this change
>>> may have unintended consequences.
>>>
>>> I have previously avoided "fixing"/enable this code path for RK35xx
>>> because the download key selection should really be improved when this
>>> is fixed/expanded to more boards/SoCs.
>>>
>>> E.g. maybe add support to declare a recovery button in u-boot,config
>>> node and use UCLASS_BUTTON to check if such button was pressed or
>>> something similar?
>>
>> Yes this is much better than evaluating raw ADC values, but BUTTON is 
>> not available in SPL so we cannot have this functionality in SPL 
>> (though this idea was already rejected :P).
>>
> 
> I'm sorry my memory is failing me, do you have a link maybe to that 
> discussion?

Please refer 
https://lore.kernel.org/u-boot/fd15b6e4-5525-4ce5-a145-ceb47d44c40e@rock-chips.com/

> 
>> I have made an initial version that use UCLASS_BUTTON:
>>
>> ```
>> __weak int rockchip_dnl_key_pressed(void)
>> {
>> #if CONFIG_IS_ENABLED(BUTTON)
>>      const char *path;
>>      struct udevice *button;
>>      ofnode node;
>>      int ret;
>>
>>      path = ofnode_options_read_str("recovery-key");
>>      if (!path)
>>          path = ofnode_conf_read_str("u-boot,recovery-key");
>>      if (!path)
>>          return false;
>>
>>      if (path[0] == '/')
>>          node = ofnode_path(path);
>>      else
>>          node = ofnode_get_aliases_node(path);
>>
>>      if (!ofnode_valid(node)) {
>>          pr_err("%s: invalid recovery key '%s'\n", __func__, path);
>>          return false;
>>      }
>>
>>      ret = uclass_get_device_by_ofnode(UCLASS_BUTTON, node, &button);
>>      if (ret) {
>>          pr_err("%s: recovery key is not a button: %d\n", __func__, ret);
>>          return false;
>>      }
>>
>>      ret = button_get_state(button);
>>      if (ret < 0) {
>>          pr_err("%s: failed to read recovery key: %d\n", __func__, ret);
>>          return false;
>>      }
>>
>>      return ret == BUTTON_ON;
>> #else
>>      return false;
>> #endif
>> }
>> ```
>>
>> ... so it can be configured in u-boot.dtsi like this:
>> ```
>> / {
>>      options {
>>          u-boot {
>>              recovery-key = "/keys-1/button-recovery";
> 
> This should absolutely be a phandle and not a path.

This prop also works with phandle. However most dts from upstream kernel 
does not have aliases for button, I have no idea how to refer them by 
phandle without modifying original copy.

> 
>>          };
>>      };
>> };
>> ```
>>
>> I have tested this on my DshanPi A1 board and it seems to work okay.
>>
>> Is this an acceptable way to configure recovery key? I'm also wondering 
> 
> It could be. Typically, when a new property makes it to the device tree, 
> it must be documented in the binding. Here it would be part of a u- 
> boot,config node, but I couldn't find a binding in U-Boot, Linux kernel 
> or the Device Tree spec, so I don't know what we're supposed to do here. 
> The button itself will need to be part of the Linux kernel upstream DTS, 
> the difficult part will be which event code it needs to return and will 
> be board-specific.

So recovery-key prop is added to detect if that button is pressed only. 
We don't care the event code.

> 
>> if it's necessary to keep compatibility with old 
>> adc_channel_single_shot implementation.
>>
> 
> Not necessarily, but we absolutely need all boards using this old 
> mechanism to be supported by the new mechanism, which may be difficult 
> since we don't necessarily have access to their schematics.

I'm not sure how many users are there. This code has been broken for a 
long time. I can see a few boards implement their own download key 
detection by similar (old) code.

> 
> I think we can maybe compromise for now on a Kconfig symbol for the 
> channel to use? What do you think? We would still need to check the 
> schematics of all boards that exercise this path and check if fixing the 
> condition won't cause side effects (because it now is reading the ADC 
> channel while it wasn't before due to the typo). The other option is to 
> explicitly disable it for all devices that could never meet the 
> condition with the typo (so even at SoC level since SARADC is an SoC- 
> specific IP) such that it can only be enabled if someone tests it on 
> their board.

Honestly I don't want to "improve" this old path if we decide to use the 
new config prop. KEY_DOWN_MAX_VAL=30 is also too strict for some boards 
(for example the ADC value on DshanPi A1 can be up to 45~47 when the 
button is pressed).

Thanks,
Tianling.

> 
> Cheers,
> Quentin