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 <[email protected]> > > > Reviewed-by: Sumit Garg <[email protected]> > > > Signed-off-by: Varadarajan Narayanan > > > <[email protected]> > > > --- > > > 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 <[email protected]> > > > + * 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. -- Tom
signature.asc
Description: PGP signature
