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 <[email protected]>
---
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/[email protected]/
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
