On Mon, Apr 28, 2025 at 04:22:47PM +0200, Casey Connolly wrote:
>
>
> On 4/10/25 14:02, Varadarajan Narayanan wrote:
> > Add SYSRESET_EDL to sysreset_t and handle the different SYSRESET_xxx
> > requests in sysreset_qcom-psci.c.
>
> To be honest, I'm not really excited about this. Copying the entire PSCI
> reset code and polluting the global sysreset reason enum with entirely
> platform specific symbols is not good practise.
>
> Additionally, while this specific EDL implementation works on your platform,
> it does not work on (for example) sdm845 and sm8250 which are platforms we
> still very much care about. I'm also not sure if it works on SC7280.
>
> I think this should be handled in an/the SCM driver like it is in Linux, not
> sure if Sam is working on that or if I can send a barebones version of the
> one I have in my tree. It will need to be based on the compatible string for
> the scm node in DT (and checking for the qcom,dload-mode property too I
> think).
>
> As I proposed before on IRC, I think it makes sense to pass the "reset"
> arguments into sysreset and allow the sysreset driver to implement a
> callback/handler for them, this way the "edl" argument makes it all the way
> to our platform-specific driver which can then call "psci_system_reset2()"
>
> I would implement this by adding a new op to sysreset_ops, something like
> .request_args, then have a second for loop which calls this, since we want
> any reset handler which /can/ parse the args to do so before we call the
> normal .request callback (otherwise some other handler might do a normal
> reset before we have parsed the arguments).
>
> For now I'm fine with this new sysreset-qcom.c driver being specific to
> qcs9100, but please include a comment that the psci reset2 call is only
> supported on some newer qualcomm platforms and that others can be supported
> via SCM.
>
> On that note, how does this new driver actually get bound? It doesn't have
> any compatible strings associated? Or am I missing something here... Please
> explain this.
Apologies, my bad for not mentioning it. Presently it is not getting bound.
Wanted to get feedback about the approach.
> Otherwise, I'm looking forward to v2.
Will post v2, with device_bind_driver("qcom_psci-sysreset") in psci_bind().
Thanks
Varada
> Kind regards,
>
> >
> > Signed-off-by: Varadarajan Narayanan <quic_var...@quicinc.com>
> > ---
> > drivers/sysreset/Kconfig | 5 +++
> > drivers/sysreset/Makefile | 1 +
> > drivers/sysreset/sysreset-uclass.c | 7 ++--
> > drivers/sysreset/sysreset_qcom-psci.c | 47 +++++++++++++++++++++++++++
> > include/sysreset.h | 2 ++
> > 5 files changed, 60 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/sysreset/sysreset_qcom-psci.c
> >
> > diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig
> > index 121194e4418..a6a1e57b603 100644
> > --- a/drivers/sysreset/Kconfig
> > +++ b/drivers/sysreset/Kconfig
> > @@ -240,6 +240,11 @@ config SYSRESET_RAA215300
> > help
> > Add support for the system reboot via the Renesas RAA215300 PMIC.
> > +config SYSRESET_QCOM_PSCI
> > + bool "Support sysreset for Qualcomm SoCs via PSCI"
> > + help
> > + Add support for the system reboot on Qualcomm SoCs via PSCI.
> > +
> > config SYSRESET_QCOM_PSHOLD
> > bool "Support sysreset for Qualcomm SoCs via PSHOLD"
> > depends on ARCH_IPQ40XX
> > diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile
> > index 796fc9effa5..12dbad5254a 100644
> > --- a/drivers/sysreset/Makefile
> > +++ b/drivers/sysreset/Makefile
> > @@ -29,5 +29,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-uclass.c
> > b/drivers/sysreset/sysreset-uclass.c
> > index 536ac727142..d06c0a6908a 100644
> > --- a/drivers/sysreset/sysreset-uclass.c
> > +++ b/drivers/sysreset/sysreset-uclass.c
> > @@ -125,8 +125,11 @@ int do_reset(struct cmd_tbl *cmdtp, int flag, int
> > argc, char *const argv[])
> > if (argc > 2)
> > return CMD_RET_USAGE;
> > - if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w') {
> > - reset_type = SYSRESET_WARM;
> > + if (argc == 2) {
> > + if (argv[1][0] == '-' && argv[1][1] == 'w')
> > + reset_type = SYSRESET_WARM;
> > + else if (!strncmp("edl", argv[1], 3))
> > + reset_type = SYSRESET_EDL;
> > }
> > printf("resetting ...\n");
> > diff --git a/drivers/sysreset/sysreset_qcom-psci.c
> > b/drivers/sysreset/sysreset_qcom-psci.c
> > new file mode 100644
> > index 00000000000..3afb79e2d2e
> > --- /dev/null
> > +++ b/drivers/sysreset/sysreset_qcom-psci.c
> > @@ -0,0 +1,47 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2017 Masahiro Yamada <yamada.masah...@socionext.com>
> > + */
> > +
> > +#include <dm.h>
> > +#include <sysreset.h>
> > +#include <asm/system.h>
> > +#include <linux/errno.h>
> > +#include <linux/psci.h>
> > +
> > +__weak int qcom_psci_sysreset_get_status(struct udevice *dev, char *buf,
> > int size)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static int qcom_psci_sysreset_request(struct udevice *dev, enum sysreset_t
> > type)
> > +{
> > + switch (type) {
> > + case SYSRESET_WARM:
> > + case SYSRESET_COLD:
> > + psci_sys_reset(type);
> > + break;
> > + case SYSRESET_POWER_OFF:
> > + psci_sys_poweroff();
> > + break;
> > + case SYSRESET_EDL:
> > + psci_system_reset2(0, 1);
> > + break;
> > + default:
> > + return -EPROTONOSUPPORT;
> > + }
> > +
> > + return -EINPROGRESS;
> > +}
> > +
> > +static struct sysreset_ops qcom_psci_sysreset_ops = {
> > + .request = qcom_psci_sysreset_request,
> > + .get_status = qcom_psci_sysreset_get_status,
> > +};
> > +
> > +U_BOOT_DRIVER(qcom_psci_sysreset) = {
> > + .name = "qcom_psci-sysreset",
> > + .id = UCLASS_SYSRESET,
> > + .ops = &qcom_psci_sysreset_ops,
> > + .flags = DM_FLAG_PRE_RELOC,
> > +};
> > diff --git a/include/sysreset.h b/include/sysreset.h
> > index ff20abdeed3..8bda9703cd9 100644
> > --- a/include/sysreset.h
> > +++ b/include/sysreset.h
> > @@ -21,6 +21,8 @@ enum sysreset_t {
> > SYSRESET_POWER,
> > /** @SYSRESET_POWER_OFF: turn off power */
> > SYSRESET_POWER_OFF,
> > + /** @SYSRESET_EDL: reset and boot into Emergency DownLoader */
> > + SYSRESET_EDL,
> > /** @SYSRESET_COUNT: number of available reset types */
> > SYSRESET_COUNT,
> > };
> --
> Casey (she/they)
>
