Re: [PATCH 3/8] reboot-mode: add PSCI reboot-mode driver

Hi Balaji,

On 2026-08-11T05:18:35, Balaji Selvanathan
<[email protected]> wrote:
> reboot-mode: add PSCI reboot-mode driver
>
> Add a PSCI backend for the reboot-mode framework that triggers a
> SYSTEM_RESET2 vendor-specific reset described by a "mode-*" property under
> a "reboot-mode" subnode of the PSCI device tree node (for example
> "mode-edl = <0x80000000 0x00000001>" to enter Qualcomm EDL/download mode).
>
> All device tree parsing and name matching already live in the reboot-mode
> uclass; this driver only turns a decoded set of magic cells into a
> firmware call.
>
> U-Boot's psci_system_reset2() client takes only a 32-bit cookie and ORs in
> PSCI_RESET2_TYPE_VENDOR itself, so the trigger folds the low cells into a
> cookie and rejects a 3-cell mode whose cookie_hi is non-zero rather than
> silently truncating it.
>
> psci_bind() binds the driver to the "reboot-mode" subnode when
> CONFIG_REBOOT_MODE_PSCI is enabled, using device_bind_driver_to_node().
>
> Signed-off-by: Balaji Selvanathan <[email protected]>
>
> drivers/firmware/psci.c                | 16 ++++++++++
>  drivers/reboot-mode/Kconfig            | 11 +++++++
>  drivers/reboot-mode/Makefile           |  1 +
>  drivers/reboot-mode/reboot-mode-psci.c | 58 ++++++++++++++++++++++++++++++++++
>  4 files changed, 86 insertions(+)

> diff --git a/drivers/reboot-mode/Kconfig b/drivers/reboot-mode/Kconfig
> @@ -39,4 +39,15 @@ config REBOOT_MODE_NVMEM
> +config REBOOT_MODE_PSCI
> +     bool "Use PSCI SYSTEM_RESET2 vendor resets as reboot modes"
> +     depends on DM_REBOOT_MODE && ARM_SMCCC

psci_features() and psci_system_reset2() only exist for ARM64 -
fwcall.c lives under arch/arm/cpu/armv8, and the prototypes in
asm/system.h are inside the CONFIG_ARM64 block. ARM_SMCCC is available
on CPU_V7A too, so as written this can be enabled on a 32-bit ARM
build and then fail to link. Please add 'depends on ARM64' (or gate on
the appropriate ARMv8 PSCI symbol).

> diff --git a/drivers/reboot-mode/reboot-mode-psci.c b/drivers/reboot-mode/reboot-mode-psci.c
> @@ -0,0 +1,58 @@
> +     u32 reset_type = magic[0];
> +     u64 cookie = 0;
> +     int i;
> +
> +     if (count < 1 || count > REBOOT_MODE_MAX_MAGIC)
> +             return -EINVAL;
> +
> +     /*
> +      * U-Boot's psci_system_reset2() takes only a 32-bit cookie, whereas
> +      * the binding allows a 64-bit cookie in the 3-cell form (magic[1] is
> +      * cookie_hi, magic[2] is cookie_lo). Reject a non-zero high half
> +      * rather than silently truncating it.
> +      */
> +     if (count == REBOOT_MODE_MAX_MAGIC && magic[1]) {
> +             dev_err(dev, "64-bit reset cookie is not supported\n");
> +             return -EINVAL;
> +     }
> +
> +     for (i = 1; i < count; i++)
> +             cookie = (cookie << 32) | magic[i];

Given that magic[1] is guaranteed to be zero in the 3-cell case and
the value is immediately truncated to u32 at the call site, the u64
shift/OR loop is misleading - it looks like it is assembling a real
64-bit cookie when it never can. Please make cookie a u32 and pick it
directly, e.g.

    u32 cookie = 0;

    if (count >= 2)
        cookie = magic[count - 1];

That also removes the need for the loop variable and the cast on the call.

> diff --git a/drivers/reboot-mode/reboot-mode-psci.c b/drivers/reboot-mode/reboot-mode-psci.c
> @@ -0,0 +1,58 @@
> +     /* Does not return on success. */
> +     psci_system_reset2(reset_type, (u32)cookie);
> +
> +     return -EINPROGRESS;
> +}

psci_system_reset2() is declared __noreturn in asm/system.h and ends
in a while(1), so the return statement is unreachable. I doubt you can
remove this 'return' though?

> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> @@ -188,6 +188,22 @@ static int psci_bind(struct udevice *dev)
> +     if (CONFIG_IS_ENABLED(REBOOT_MODE_PSCI)) {
> +             ofnode rm_node = ofnode_find_subnode(dev_ofnode(dev),
> +                                                  "reboot-mode");
> +
> +             if (ofnode_valid(rm_node) &&
> +                 device_bind_driver_to_node(dev, "reboot-mode-psci",
> +                                            "reboot-mode-psci", rm_node,
> +                                            NULL))
> +                     pr_warn("PSCI reboot-mode was not bound.\n");
> +     }

Shouldn't we be checking errors here?

SYSTEM_RESET2 arrived in PSCI 1.1, but this bind runs for any PSCI
node (including the 0.2/1.0 block just above, which only binds the
sysreset driver conditionally). The runtime psci_features() check will
catch missing support, but consider gating the bind on 'arm,psci-1.1'
compat to avoid a warning on older PSCI DTs when the config happens to
be enabled. Worth a comment either way.

Regards,
Simon