[PATCH v6 3/5] sysreset: Implement PSCI based reset to EDL mode for QCOM SoCs

[Quentin Schulz]

Hi all,

Gentle ping. It'd be nice if we can fix this new API before we have a 
release made with it?

On 5/26/26 3:32 PM, Quentin Schulz wrote:
> Hi Varadarajan, Casey,
> 
> On 1/21/26 7:39 AM, Varadarajan Narayanan wrote:
>> Implement request_arg() sysreset_op for QCOM SoCs that use
>> PSCI to reset to EDL (Emergency Download) mode.
>>
>> Reviewed-by: Casey Connolly <casey.connolly at linaro.org>
>> Reviewed-by: Sumit Garg <sumit.garg at oss.qualcomm.com>
>> Signed-off-by: Varadarajan Narayanan 
>> <varadarajan.narayanan at oss.qualcomm.com>
>> ---
>> v4: * Check if ARM_PSCI_1_1_FN64_SYSTEM_RESET2 is supported before
>>        issuing it
>>
>> v3: * Move argument handling to a separate function and pass the
>>        arguments to the actual handler to process
>>      * Drop Qcom specific SYSRESET_EDL from the common enum
>>
>> v2: * Update commit message
>>      * Elaborate Kconfig help text
>>      * Use '-edl' instead of 'edl' for consistency with other 
>> arguments of reset
>>        command
>>      * Remove 'weak' for qcom_psci_sysreset_get_status() and make it 
>> static
>>      * Mention 'SYSRESET_EDL' is Qcom specific in the enum's comments
>> ---
>>   drivers/firmware/psci.c               |  4 +++
>>   drivers/sysreset/Kconfig              |  6 ++++
>>   drivers/sysreset/Makefile             |  1 +
>>   drivers/sysreset/sysreset_qcom-psci.c | 45 +++++++++++++++++++++++++++
>>   4 files changed, 56 insertions(+)
>>   create mode 100644 drivers/sysreset/sysreset_qcom-psci.c
>>
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index 2e3223e1c32..b6838a244d2 100644
>> --- a/drivers/firmware/psci.c
>> +++ b/drivers/firmware/psci.c
>> @@ -186,6 +186,10 @@ static int psci_bind(struct udevice *dev)
>>                        NULL);
>>           if (ret)
>>               pr_debug("PSCI System Reset was not bound.\n");
>> +        if (IS_ENABLED(CONFIG_SYSRESET_QCOM_PSCI) &&
>> +            device_bind_driver(dev, "qcom_psci-sysreset",
>> +                       "qcom_psci-sysreset", NULL))
>> +            pr_debug("QCOM PSCI System Reset was not bound.\n");
>>       }
>>       /* From PSCI v1.0 onward we can discover services through 
>> ARM_SMCCC_FEATURE */
>> diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig
>> index 6fb0ca81dc6..905ad2a7769 100644
>> --- a/drivers/sysreset/Kconfig
>> +++ b/drivers/sysreset/Kconfig
>> @@ -301,6 +301,12 @@ config SYSRESET_RAA215300
>>       help
>>         Add support for the system reboot via the Renesas RAA215300 PMIC.
>> +config SYSRESET_QCOM_PSCI
>> +    bool "Support reset to EDL for Qualcomm SoCs via PSCI"
>> +    help
>> +      Add support for the reset to EDL (Emergency Download) on Qualcomm
>> +      SoCs via PSCI.
>> +
>>   config SYSRESET_QCOM_PSHOLD
>>       bool "Support sysreset for Qualcomm SoCs via PSHOLD"
>>       help
>> diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile
>> index f5c78b25896..8bb1eabd48e 100644
>> --- a/drivers/sysreset/Makefile
>> +++ b/drivers/sysreset/Makefile
>> @@ -30,5 +30,6 @@ obj-$(CONFIG_SYSRESET_RESETCTL) += sysreset_resetctl.o
>>   obj-$(CONFIG_$(PHASE_)SYSRESET_AT91) += sysreset_at91.o
>>   obj-$(CONFIG_$(PHASE_)SYSRESET_X86) += sysreset_x86.o
>>   obj-$(CONFIG_SYSRESET_RAA215300) += sysreset_raa215300.o
>> +obj-$(CONFIG_SYSRESET_QCOM_PSCI) += sysreset_qcom-psci.o
>>   obj-$(CONFIG_SYSRESET_QCOM_PSHOLD) += sysreset_qcom-pshold.o
>>   obj-$(CONFIG_TARGET_XTFPGA) += sysreset_xtfpga.o
>> diff --git a/drivers/sysreset/sysreset_qcom-psci.c b/drivers/sysreset/ 
>> sysreset_qcom-psci.c
>> new file mode 100644
>> index 00000000000..3627bbf5c82
>> --- /dev/null
>> +++ b/drivers/sysreset/sysreset_qcom-psci.c
>> @@ -0,0 +1,45 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2017 Masahiro Yamada <yamada.masahiro at socionext.com>
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <dm.h>
>> +#include <sysreset.h>
>> +#include <asm/system.h>
>> +#include <linux/errno.h>
>> +#include <linux/psci.h>
>> +#include <asm/psci.h>
>> +
>> +static int qcom_psci_sysreset_get_status(struct udevice *dev, char 
>> *buf, int size)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static int qcom_psci_sysreset_request_arg(struct udevice *dev, int argc,
>> +                      char * const argv[])
>> +{
>> +    if (!strncasecmp(argv[1], "-edl", 4)) {
>> +        /* Supported in qcs9100, qcs8300, sc7280, qcs615 */
>> +        if (psci_features(ARM_PSCI_1_1_FN64_SYSTEM_RESET2) ==
>> +                            ARM_PSCI_RET_SUCCESS) {
>> +            psci_system_reset2(0, 1);
>> +            return -EINPROGRESS;
>> +        }
>> +        printf("PSCI SYSTEM_RESET2 not supported\n");
>> +    }
>> +
>> +    return -EPROTONOSUPPORT;
> 
> I think this is a logic bug.
> 
> For reference, what calls this function:
> 
>    int sysreset_walk_arg(int argc, char * const argv[])
>    {
>            struct udevice *dev;
>            int ret = -ENOSYS;
> 
>            while (ret != -EINPROGRESS && ret != -EPROTONOSUPPORT) {
>                    for (uclass_first_device(UCLASS_SYSRESET, &dev);
>                         dev;
>                         uclass_next_device(&dev)) {
>                            ret = sysreset_request_arg(dev, argc, argv);
>                            if (ret == -EINPROGRESS || ret == - 
> EPROTONOSUPPORT)
>                                    break;
>                    }
>            }
> 
>            return ret;
>    }
> 
> The issue is that the first sysreset device implementing the request_arg 
> callback will consume the args and return EPROTONOSUPPORT which will 
> stop the loop.
> 
> Think about multiple sysreset devices, and let's say we have a new '- 
> dummy' argument to the reset CLI command, then it'll depend on the 
> registration order of the sysreset driver whether the dummy argument 
> will be handled by the qcom driver which will return -
> EPROTONOSUPPORT since it isn't '-edl', or by the dummy sysreset driver.
> 
> I think we should have a fourth possible return code which is "I don't 
> know how to handle this argument", e.g. à-la IRQ_NONE in Linux kernel 
> for interrupts that aren't for the device this IRQ handler was triggered 
> for. Or maybe we should really be using -EPROTONOSUPPORT for that 
> meaning and not exit the loop if that's the return code?
> 
> Or maybe I misread or misunderstood the code?
> 
> Cheers,
> Quentin