Message-ID: <b92c164f-c6df-a2c0-1416-67652a01b179@oss.qualcomm.com>
Date: Wed, 30 Jul 2025 18:33:31 +0530
From: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
To: André Draszik <andre.draszik@...aro.org>,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Souvik Chakravarty <Souvik.Chakravarty@....com>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
 <conor+dt@...nel.org>,
        Andy Yan <andy.yan@...k-chips.com>,
        Mark Rutland <mark.rutland@....com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, Konrad Dybcio <konradybcio@...nel.org>,
        cros-qcom-dts-watchers@...omium.org, Vinod Koul <vkoul@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Florian Fainelli <florian.fainelli@...adcom.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        Mukesh Ojha <mukesh.ojha@....qualcomm.com>,
        Stephen Boyd <swboyd@...omium.org>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
        Elliot Berman <quic_eberman@...cinc.com>,
        Srinivas Kandagatla <srini@...nel.org>
Subject: Re: [PATCH v13 07/10] firmware: psci: Implement vendor-specific
 resets as reboot-mode



On 7/30/2025 2:14 PM, André Draszik wrote:
> On Sun, 2025-07-27 at 21:54 +0530, Shivendra Pratap wrote:
>> SoC vendors have different types of resets which are controlled
>> through various hardware registers. For instance, Qualcomm SoC
>> may have a requirement that reboot with “bootloader” command
>> should reboot the device to bootloader flashing mode and reboot
>> with “edl” should reboot the device into Emergency flashing mode.
>> Setting up such reboots on Qualcomm devices can be inconsistent
>> across SoC platforms and may require setting different HW
>> registers, where some of these registers may not be accessible to
>> HLOS. These knobs evolve over product generations and require
>> more drivers. PSCI spec defines, SYSTEM_RESET2, vendor-specific
>> reset which can help align this requirement. Add support for PSCI
>> SYSTEM_RESET2, vendor-specific resets and align the implementation
>> to allow user-space initiated reboots to trigger these resets.
>>
>> Introduce a late_initcall to register PSCI vendor-specific resets
>> as reboot modes. Implement a reboot-mode write function that sets
>> reset_type and cookie values during the reboot notifier callback.
>> Introduce a firmware-based call for SYSTEM_RESET2 vendor-specific
>> reset in the psci_sys_reset path, using reset_type and cookie if
>> supported by secure firmware.
>>
>> By using the above implementation, userspace will be able to issue
>> such resets using the reboot() system call with the "*arg"
>> parameter as a string based command. The commands can be defined
>> in PSCI device tree node as “reset-types” and are based on the
>> reboot-mode based commands.
>>
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
>> ---
>>  drivers/firmware/psci/Kconfig |  2 ++
>>  drivers/firmware/psci/psci.c  | 57 ++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 58 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/psci/Kconfig b/drivers/firmware/psci/Kconfig
>> index 97944168b5e66aea1e38a7eb2d4ced8348fce64b..93ff7b071a0c364a376699733e6bc5654d56a17f 100644
>> --- a/drivers/firmware/psci/Kconfig
>> +++ b/drivers/firmware/psci/Kconfig
>> @@ -1,6 +1,8 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  config ARM_PSCI_FW
>>  	bool
>> +	select POWER_RESET
>> +	select REBOOT_MODE
>>  
>>  config ARM_PSCI_CHECKER
>>  	bool "ARM PSCI checker"
>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>> index 38ca190d4a22d6e7e0f06420e8478a2b0ec2fe6f..e14bcdbec1750db8aa9297c8bcdb242f58cc420e 100644
>> --- a/drivers/firmware/psci/psci.c
>> +++ b/drivers/firmware/psci/psci.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/printk.h>
>>  #include <linux/psci.h>
>>  #include <linux/reboot.h>
>> +#include <linux/reboot-mode.h>
>>  #include <linux/slab.h>
>>  #include <linux/suspend.h>
>>  
>> @@ -51,6 +52,14 @@ static int resident_cpu = -1;
>>  struct psci_operations psci_ops;
>>  static enum arm_smccc_conduit psci_conduit = SMCCC_CONDUIT_NONE;
>>  
>> +struct psci_vendor_sysreset2 {
>> +	u32 reset_type;
>> +	u32 cookie;
>> +	bool valid;
>> +};
>> +
>> +static struct psci_vendor_sysreset2 vendor_reset;
>> +
>>  bool psci_tos_resident_on(int cpu)
>>  {
>>  	return cpu == resident_cpu;
>> @@ -309,7 +318,10 @@ 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) &&
>> +	if (vendor_reset.valid && psci_system_reset2_supported) {
>> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), vendor_reset.reset_type,
>> +			       vendor_reset.cookie, 0);
>> +	} else if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>>  	    psci_system_reset2_supported) {
>>  		/*
>>  		 * reset_type[31] = 0 (architectural)
> 
> I don't know the PSCI spec, but it looks like with this code it's not
> possible to set a reboot mode (in DT) and at the same time instruct
> the firmware whether a warm or a cold reboot was requested.

The code added in this patch is kind of dead, until vendor_reset.valid is set to true.
It can be true, only when both below conditions are true.
 1. A SoC DT defines a psci->reboot-mode command say - "bootloader".
 2. reboot sys call is issued using LINUX_REBOOT_CMD_RESTART2 and the arg* as "bootloader".
      reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, LINUX_REBOOT_CMD_RESTART2, "bootloader");

With that in place, warm and cold reboot just work same as before until above conditions are true.
There is no effect on regular reboots or the reboots with a "command" which is not defined in
psci->reboot-mode DT.

Now lets take a case below, where a SoC vendor wants a combination of psci->reboo-mode command and
warm/cold to work in combination. For ex. a requirement below:
 - reboot command say - "bootlaoder" should do a cold reboot along with some extra HW reg writes.
 - reboot command say - "edl" should do a warm reboot along with some extra HW reg writes.

1. For this, both commands will be defined in the psci->reboot-mode DT Node with the arguments that
   are defined and supported by the firmware.
2. Further, such requirement will now be taken care by the underlying firmware that supports
   PSCI vendor-specific reset. When we call into firmware with vendor specific reset arguments,
   firmware will take care of the defined HW writes and warm/cold reset as per the mapping of the
   defined arguments. Firmware and the Linux kernel here are in agreement for executing the
   vendor-specific resets.

> 
> Doing warm reboot is useful if e.g. RAM contents needs to be retained
> for crash recovery handling, or other reasons, while in normal cases
> doing a more secure cold reboot.
> 
> On the other hand, of course it's useful to be able to specify the
> reboot target for normal reboots.
> 
> Is this a problem with the PSCI spec or with this specific change
> geared at the Qcom implementation?

SoC vendor should define a vendor-specific reset in psci DT only when they support them in their
firmware. 

Do we still think we are breaking anything in the spec or in the warm or the cold
reset path? If so can we discuss such use-cases?

> 
> 
>> @@ -547,6 +559,49 @@ static const struct platform_suspend_ops psci_suspend_ops = {
>>  	.enter          = psci_system_suspend_enter,
>>  };
>>  
>> +static int psci_set_vendor_sys_reset2(struct reboot_mode_driver *reboot, u64 magic)
>> +{
>> +	u32 magic_32;
>> +
>> +	if (psci_system_reset2_supported) {
>> +		magic_32 = magic & 0xFFFFFFFF;
> 
> I believe usual kernel style is to use lower case for
> hex values.

Sure, will make it lower case.

> 
>> +		vendor_reset.reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic_32;
>> +		vendor_reset.cookie = (magic >> 32) & 0xFFFFFFFF;
> 
> dito.

Ack.

thanks.

> 
> Cheers,
> Andre'
> 
>> +		vendor_reset.valid = true;
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static int __init psci_init_vendor_reset(void)
>> +{
>> +	struct reboot_mode_driver *reboot;
>> +	struct device_node *np;
>> +	int ret;
>> +
>> +	np = of_find_node_by_path("/psci/reboot-mode");
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	reboot = kzalloc(sizeof(*reboot), GFP_KERNEL);
>> +	if (!reboot) {
>> +		of_node_put(np);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	reboot->write = psci_set_vendor_sys_reset2;
>> +
>> +	ret = reboot_mode_register(reboot, np, "psci");
>> +	if (ret) {
>> +		of_node_put(np);
>> +		kfree(reboot);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +late_initcall(psci_init_vendor_reset)
>> +
>>  static void __init psci_init_system_reset2(void)
>>  {
>>  	int ret;

Powered by blists - more mailing lists