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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 8056 bytes --]

On Thu, Jul 02, 2026 at 07:43:06PM +0200, Quentin Schulz wrote:
> On 6/23/26 6:50 PM, Tom Rini wrote:
> > On Mon, Jun 08, 2026 at 04:53:23PM +0200, Quentin Schulz wrote:
> > > 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@linaro.org>
> > > > > Reviewed-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > > > > Signed-off-by: Varadarajan Narayanan
> > > > > <varadarajan.narayanan@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@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?
> > 
> > Following up here.
> > 
> 
> Do we end up reverting just before the release then?

Please send it out, cc everyone, and see if that gets some response?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]