Hey Casey, Varadarajan,
On Tuesday, April 29th, 2025 at 12:22 AM, Casey Connolly
<casey.conno...@linaro.org> 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).
I've started on the v2 of the SCM driver patch, but I'm behind on
everything atm, and not expecting to resend that series for another
couple of weeks.
If someone wants to pick up the SCM part please be my guest :D
Thanks,
-Sam
>
> 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.
>
> Otherwise, I'm looking forward to v2.
>
> 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)
