Re: [PATCH v20 06/10] power: reset: Add psci-reboot-mode driver

[Shivendra Pratap]

On 03-04-2026 21:20, Lorenzo Pieralisi wrote:
> On Fri, Apr 03, 2026 at 12:05:27AM +0530, Shivendra Pratap wrote:
> > On 01-04-2026 20:07, Lorenzo Pieralisi wrote:
> > > On Tue, Mar 31, 2026 at 11:30:09PM +0530, Shivendra Pratap wrote:
> > > > On 27-03-2026 19:25, Lorenzo Pieralisi wrote:
> > > > > On Wed, Mar 04, 2026 at 11:33:06PM +0530, Shivendra Pratap wrote:
> > > > > > PSCI supports different types of resets like COLD reset, ARCH WARM
> >
> > [snip..]
> >
> > > > > > + * Predefined reboot-modes are defined as per the values
> > > > > > + * of enum reboot_mode defined in the kernel: reboot.c.
> > > > > > + */
> > > > > > +static struct mode_info psci_resets[] = {
> > > > > > +	{ .mode = "warm", .magic = REBOOT_WARM},
> > > > > > +	{ .mode = "soft", .magic = REBOOT_SOFT},
> > > > > > +	{ .mode = "cold", .magic = REBOOT_COLD},
> > >
> > > These strings match the command userspace issue right ? I think that we
> > > should make them match the corresponding PSCI reset types, the list above
> > > maps command to reboot_mode values and those can belong to any reboot
> > > mode driver to be honest they don't make much sense in a PSCI reboot
> > > mode driver only.
> > >
> > > It is a question for everyone here: would it make sense to make these
> > > predefined resets a set of strings, eg:
> > >
> > > psci-system-reset
> > > psci-system-reset2-arch-warm-reset
> > >
> > > and then vendor resets:
> > >
> > > psci-system-reset2-vendor-reset
> >
> > Can you share bit more details on this? We are already defining the string
> > from userspace in the struct - eg: ".mode = "warm".
>
> "warm","soft","cold" are not strictly speaking PSCI concepts and mean nothing
> well defined to user space and even if they did, they would not belong in
> the PSCI reboot mode driver but in generic code.
>
> Spelling out what a reset is might help instead, again, this is just my
> opinion, I don't know how the semantics of resets have been handled thus
> far.
>
> If userspace issues a LINUX_REBOOT_CMD_RESTART2 with arg, say,
> "psci-system-reset2-arch-warm-reset" it is pretty clear what it wants
> to do in PSCI.

ok. got it.

so it predef-modes.

reboot psci-system-reset2-arch-warm-reset =>goes for => ARCH WARM RESET.
etc..

> Again, it is a suggestion, comments welcome.
>
> > yes we can move away from enum reboot_mode and use custom psci defines one -
> > Ack.
> >
> > >
> >
> > [snip ..]
> >
> > > > > > +
> > > > > > +/*
> > > > > > + * arg1 is reset_type(Low 32 bit of magic).
> > > > > > + * arg2 is cookie(High 32 bit of magic).
> > > > > > + * If reset_type is 0, cookie will be used to decide the reset command.
> > > > > > + */
> > > > > > +static int psci_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
> > > > > > +{
> > > > > > +	u32 reset_type = REBOOT_MODE_ARG1(magic);
> > > > > > +	u32 cookie = REBOOT_MODE_ARG2(magic);
> > > > > > +
> > > > > > +	if (reset_type == 0) {
> > > > > > +		if (cookie == REBOOT_WARM || cookie == REBOOT_SOFT)
> > > > > > +			psci_set_reset_cmd(true, 0, 0);
> > > > > > +		else
> > > > > > +			psci_set_reset_cmd(false, 0, 0);
> > > > > > +	} else {
> > > > > > +		psci_set_reset_cmd(true, reset_type, cookie);
> > > > > > +	}
> > > > >
> > > > > I don't think that psci_set_reset_cmd() has the right interface (and this
> > > > > nested if is too complicated for my taste). All we need to pass is reset-type
> > > > > and cookie (and if the reset is one of the predefined ones, reset-type is 0
> > > > > and cookie is the REBOOT_* cookie).
> > > > >
> > > > > Then the PSCI firmware driver will take the action according to what
> > > > > resets are available.
> > > > >
> > > > > How does it sound ?
> > > >
> > > > So we mean these checks will move to the psci driver? Sorry for re-iterating
> > > > the question.
> > >
> > > Given what I say above, I believe that something we can do is mapping the magic
> > > to an enum like:
> > >
> > > PSCI_SYSTEM_RESET
> > > PSCI_SYSTEM_RESET2_ARCH_SYSTEM_WARM_RESET
> > > PSCI_SYSTEM_RESET2_VENDOR_RESET
> > >
> > > and can add a probe function into PSCI driver similar to psci_has_osi_support() but
> > > to probe for SYSTEM_RESET2 and initialize the predefined strings accordingly,
> > > depending on its presence.
> >
> > Not able to get it cleanly.
> >
> > 1. Will move away from reboot_mode enum for pre-defined modes and define new
> > enum defining these modes- fine.
> > 2. get SYSTEM_RESET2 is supported from psci exported function -- fine, but
> > how we use it here now, as we do not want to send the reset_cmd from
> > psci_set_reset_cmd now?
>
> You do keep psci_set_reset_cmd() but all it is used for is setting a struct
> shared with the PSCI driver where you initialize the enum above, possibly
> with a cookie if it is a vendor reset.
>
> > 3. For pre-defined modes, warm/soft or cold - reset_type and cookie, both
> > are zero, sys_reset2 or sys_reset2 decides the ARCH reset vs cold reset.
> > 4. For vendor-rest , we use sys_reset2 with reset_type and cookie.
>
> Yes.

Ack.


> > All above is done in reboot_notifier call at psci-reboot-mode.
> > --
> >
> > Now in the final restart_notifier->psci_sys_reset --
> >
> > If panic is in progress, we do not use any of the cmd based reset params and
> > go with the legacy reset. So we need to preserve the values that were set
> > from psci-reboot-mode.
> >
> > Did not understand the proposed suggestion in above usecase. Need more input
> > on this.
>
> I explained above. The reboot mode driver sets the command to carry out
> depending on the string coming from user space and whether PSCI supports
> SYSTEM_RESET2 or not.

got it. working on it. thanks.


> > --
> >
> > One other option is to have a restart_notifier in psci-reboot-mode, with
> > lesser priority than psci_sys_rest and then handle all the case including
> > panic and sys_reset2.
>
> No.

Ack.

thanks,
Shivendra