Re: [PATCH v20 04/10] firmware: psci: Introduce command-based reset in psci_sys_reset

Shivendra Pratap
2026-03-31T17:39:02Z

On 27-03-2026 19:37, Lorenzo Pieralisi wrote:
> On Wed, Mar 04, 2026 at 11:33:04PM +0530, Shivendra Pratap wrote:
>> PSCI currently supports only COLD reset and ARCH WARM reset based on the
>> Linux reboot_mode variable. The PSCI specification now includes
>> SYSTEM_RESET2 for vendor-specific resets, but there's no mechanism to
>> issue these through psci_sys_reset.
>>
>> Add a command-based reset mechanism that allows external drivers to set
>> the psci reset command via a new psci_set_reset_cmd() function.
>>
>> The psci command-based reset is disabled by default and the
>> psci_sys_reset follows its original flow until a psci_reset command is
>> set. In kernel panic path, psci_reset command is ignored.
>
> If it is function calls you should add parenthesis (eg psci_sys_reset ->
> psci_sys_reset()).
>
> You must explain why the kernel panic path requires separate handling
> here AND in the code - think about looking at this years down the line
> and figure out why kernel panics are special here.

Ack.

>
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
>> drivers/firmware/psci/psci.c | 45 ++++++++++++++++++++++++++++++++++++++++++--
>> include/linux/psci.h | 2 ++
>> 2 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>> index 38ca190d4a22d6e7e0f06420e8478a2b0ec2fe6f..ae6f7a0aead913d740070080d4b2a3da15b29485 100644
>> --- a/drivers/firmware/psci/psci.c
>> +++ b/drivers/firmware/psci/psci.c
>> @@ -51,6 +51,15 @@ static int resident_cpu = -1;
>> struct psci_operations psci_ops;
>> static enum arm_smccc_conduit psci_conduit = SMCCC_CONDUIT_NONE;
>>
>> +struct psci_sys_reset_params {
>> + u32 system_reset;
>> + u32 reset_type;
>> + u32 cookie;
>> + bool cmd;
>> +};
>> +
>> +static struct psci_sys_reset_params psci_reset;
>> +
>> bool psci_tos_resident_on(int cpu)
>> {
>> return cpu == resident_cpu;
>> @@ -80,6 +89,28 @@ static u32 psci_cpu_suspend_feature;
>> static bool psci_system_reset2_supported;
>> static bool psci_system_off2_hibernate_supported;
>>
>> +/**
>> + * psci_set_reset_cmd - Sets the psci_reset_cmd for command-based
>> + * reset which will be used in psci_sys_reset call.
>> + *
>> + * @cmd_sys_rst2: Set to true for SYSTEM_RESET2 based resets.
>> + * @cmd_reset_type: Set the reset_type argument for psci_sys_reset.
>> + * @cmd_cookie: Set the cookie argument for psci_sys_reset.
>> + */
>> +void psci_set_reset_cmd(bool cmd_sys_rst2, u32 cmd_reset_type, u32 cmd_cookie)
>> +{
>
> I don't think cmd_sys_rst2 is needed, as a replied in a different thread.

Need bit more clarification. The issue is that at some point we need to
decide - sys_rst2 or the reboot_mode based reset. Are you suggesting
that this check should be in psci driver instead of a pre-check in
psci_reboot_mode driver?

>
>> + if (cmd_sys_rst2 && psci_system_reset2_supported) {
>> + psci_reset.system_reset = PSCI_FN_NATIVE(1_1, SYSTEM_RESET2);
>> + psci_reset.reset_type = cmd_reset_type;
>> + psci_reset.cookie = cmd_cookie;
>> + } else {
>> + psci_reset.system_reset = PSCI_0_2_FN_SYSTEM_RESET;
>> + psci_reset.reset_type = 0;
>> + psci_reset.cookie = 0;
>> + }
>> + psci_reset.cmd = true;
>> +}
>> +
>> static inline bool psci_has_ext_power_state(void)
>> {
>> return psci_cpu_suspend_feature &
>> @@ -309,14 +340,24 @@ static int get_set_conduit_method(const struct device_node *np)
>> static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>> void *data)
>> {
>> - if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>> - psci_system_reset2_supported) {
>> + if (((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>> + psci_system_reset2_supported) && (panic_in_progress() || !psci_reset.cmd)) {
>> /*
>> * reset_type[31] = 0 (architectural)
>> * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
>> * cookie = 0 (ignored by the implementation)
>> */
>> invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);
>> + } else if (!panic_in_progress() && psci_reset.cmd) {
>> + /*
>> + * Commands are being set in psci_set_reset_cmd
>> + * This issues, SYSTEM_RESET2 arch warm reset or
>> + * SYSTEM_RESET2 vendor-specific reset or
>> + * a SYSTEM_RESET cold reset in accordance with
>> + * the reboot-mode command.
>> + */
>> + invoke_psci_fn(psci_reset.system_reset, psci_reset.reset_type,
>> + psci_reset.cookie, 0);
>> } else {
>> invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
>
> This is very hard to parse. IMO, what you should do is:
>
> - Split this into two different paths: reboot_mode vs psci_reset.cmd == true.
> - Document very clearly why a panic needs separate handling.
>
> Something like:
>
> if (psci_reset.cmd)
> handle_reset_cmd();
> else
> handle_reboot_mode();
>
> I don't think we are far from converging but I want to be able to maintain
> this code going forward.

Ack. Will restructure it as suggested.

thanks,
Shivendra