Re: [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.conno...@linaro.org>
Reviewed-by: Sumit Garg <sumit.g...@oss.qualcomm.com>
Signed-off-by: Varadarajan Narayanan 
<varadarajan.naraya...@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.masah...@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