[PATCH v17 00/12] Implement vendor resets for PSCI SYSTEM_RESET2

[PATCH v17 00/12] Implement vendor resets for PSCI SYSTEM_RESET2

[Shivendra Pratap]

The PSCI SYSTEM_RESET2 call allows vendor firmware to define
additional reset types which could be mapped to the reboot
argument.

User-space should be able to reboot a device into different
operational boot-states supported by underlying bootloader and
firmware. Generally, some HW registers need to be written, based
on which the bootloader and firmware decide the next boot state
of device, after the reset. For example, a requirement on
Qualcomm platforms may state that reboot with "bootloader"
command, should reboot the device into bootloader flashing mode
and reboot with “edl” command, should reboot the device into an
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 defines a
vendor-specific reset in SYSTEM_RESET2 spec, which enables the
firmware to take care of underlying setting for any such
supported vendor-specific reboot. Qualcomm firmwares are
beginning to support and expose PSCI SYSTEM_RESET2
vendor-specific reset types to simplify driver requirements from
Linux. With such support added in the firmware, we now need a
Linux interface which can make use of the firmware calls for PSCI
vendor-specific resets. This will align such reboot requirement
across platforms and vendors.

The current psci driver supports two types of resets –
SYSTEM_RESET2 Arch warm-reset and SYSTEM_RESET cold-reset. The
patchset introduces the PSCI SYSTEM_RESET2 vendor-specific reset
into the reset path of the psci driver and aligns it to work with
reboot system call - LINUX_REBOOT_CMD_RESTART2, when used along
with a supported string-based command in “*arg”.

The patchset uses reboot-mode based commands, to define the
supported vendor reset-types commands in psci device tree node
and registers these commands with the reboot-mode framework.

The PSCI vendor-specific reset takes two arguments, being,
reset_type and cookie as defined by the spec. To accommodate this
requirement, enhance the reboot-mode framework to support two
32-bit arguments by switching to 64-bit magic values.

Along this line, the patchset also extends the reboot-mode
framework to add a non-device-based registration function, which
will allow drivers to register using firmware node, while
keeping backward compatibility for existing users of reboot-mode.
This will enable psci driver to register for reboot-mode and
implement a write function, which will save the magic and then
use it in psci reset path to make a vendor-specific reset call
into the firmware. In addition, the patchset will expose a sysfs
entry interface within reboot-mode which can be used by userspace
to view the supported reboot-mode commands.

The list of vendor-specific reset commands remains open due to
divergent requirements across vendors, but this can be
streamlined and standardized through dedicated device tree
bindings.

The patchset removes devres-based allocations from reboot-mode
as a fix. Its placed as first change in the series as a prerequisite
for addition of firmware node–based registration.

Currently three drivers register with reboot-mode framework -
syscon-reboot-mode, nvmem-reboot-mode and qcom-pon. Consolidated
list of commands currently added across various vendor DTs:
 mode-loader
 mode-normal
 mode-bootloader
 mode-charge
 mode-fastboot
 mode-reboot-ab-update
 mode-recovery
 mode-rescue
 mode-shutdown-thermal
 mode-shutdown-thermal-battery

On gs101 we also pass kernel-generated modes from kernel_restart()
or panic(), specifically DM verity's 'dm-verity device corrupted':
	mode-dm-verity-device-corrupted = <0x50>;

- thanks Andre' for providing this.

Detailed list of commands being used by syscon-reboot-mode:
    arm64/boot/dts/exynos/exynosautov9.dtsi:
	mode-bootloader = <EXYNOSAUTOV9_BOOT_BOOTLOADER>;
	mode-fastboot = <EXYNOSAUTOV9_BOOT_FASTBOOT>;
	mode-recovery = <EXYNOSAUTOV9_BOOT_RECOVERY>;

    arm64/boot/dts/exynos/google/gs101.dtsi:
    	mode-bootloader = <0xfc>;
    	mode-charge = <0x0a>;
    	mode-fastboot = <0xfa>;
    	mode-reboot-ab-update = <0x52>;
    	mode-recovery = <0xff>;
    	mode-rescue = <0xf9>;
    	mode-shutdown-thermal = <0x51>;
    	mode-shutdown-thermal-battery = <0x51>;

    arm64/boot/dts/hisilicon/hi3660-hikey960.dts:
    	mode-normal = <0x77665501>;
    	mode-bootloader = <0x77665500>;
    	mode-recovery = <0x77665502>;

    arm64/boot/dts/hisilicon/hi6220-hikey.dts:
    	mode-normal = <0x77665501>;
    	mode-bootloader = <0x77665500>;
    	mode-recovery = <0x77665502>;

    arm64/boot/dts/rockchip/px30.dtsi:
    	mode-bootloader = <BOOT_BL_DOWNLOAD>;
    	mode-fastboot = <BOOT_FASTBOOT>;
    	mode-loader = <BOOT_BL_DOWNLOAD>;
    	mode-normal = <BOOT_NORMAL>;
    	mode-recovery = <BOOT_RECOVERY>;

    arm64/boot/dts/rockchip/rk3308.dtsi:
    	mode-bootloader = <BOOT_BL_DOWNLOAD>;
    	mode-loader = <BOOT_BL_DOWNLOAD>;
    	mode-normal = <BOOT_NORMAL>;
    	mode-recovery = <BOOT_RECOVERY>;
    	mode-fastboot = <BOOT_FASTBOOT>;

    arm64/boot/dts/rockchip/rk3566-lckfb-tspi.dts:
    	mode-normal = <BOOT_NORMAL>;
    	mode-loader = <BOOT_BL_DOWNLOAD>;
			mode-recovery = <BOOT_RECOVERY>;
			mode-bootloader = <BOOT_FASTBOOT>;

Detailed list of commands being used by nvmem-reboot-mode:
    arm64/boot/dts/qcom/pmXXXX.dtsi:(multiple qcom DTs)
			mode-recovery = <0x01>;
			mode-bootloader = <0x02>;

The patch is tested on rb3Gen2, lemans-ride, lemans-evk, monaco-ride,
qcs615-ride.

Previous discussions around SYSTEM_RESET2:
- https://lore.kernel.org/lkml/20230724223057.1208122-2-quic_eberman@quicinc.com/T/
- https://lore.kernel.org/all/4a679542-b48d-7e11-f33a-63535a5c68cb@quicinc.com/

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

Tested-by: Florian Fainelli <florian.fainelli@broadcom.com> # On ARCH_BRCMSTB
Tested-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com> # IPQ5424-RDP466
Tested-by: Xin Liu <xin.liu@oss.qualcomm.com> # On qcs615-ride 

Changes in v17:
 Remove the patch to synchronize list traversal - Bjorn
 power: reset: reboot-mode: Remove devres based allocations
   - Remove devres based allocations as a fix   - Bartosz
 power: reset: reboot-mode: Expose sysfs for registered reboot_modes
   - remove devres based allocations in create_reboot_mode_device
     and manually free resources on failure.    - Bartosz
   - Add driver data while creating reboot device and 
     retrive the same in reboot_mode_show.      - Bartosz
   - Remove the mutex lock.
 firmware: psci: Implement vendor-specific resets as reboot-mode
   - Call put_device(np) once processing is complete - Pavan Kondeti 
 Move reboot-mode to SOC DT wherever applicable - Mukesh Ojha
- Link to v16: https://lore.kernel.org/r/20251015-arm-psci-system_reset2-vendor-reboots-v16-0-b98aedaa23ee@oss.qualcomm.com

Changes in v16:
 firmware: psci: Implement vendor-specific resets as reboot-mode
  - Use GENMASK(31, 0) instead of 0xffffffff - by Kathiravan
- Link to v15: https://lore.kernel.org/r/20250922-arm-psci-system_reset2-vendor-reboots-v15-0-7ce3a08878f1@oss.qualcomm.com

Changes in v15:
By Sebastian:
  power: reset: reboot-mode: Synchronize list traversal
   - Change mutex locking to scoped_guard() and a Fixes: tag
  power: reset: reboot-mode: Add device tree node-based registration
   - Change reboot_mode_register external call to use fwnode
  power: reset: reboot-mode: Expose sysfs for registered reboot_modes
   - Use sysfs_emit_at for printing sysfs entries
   - Add driver_name to struct reboot_mode_driver instead of passing
     as argument
   - Update reboot_mode_register, devm_reboot_mode_register and
     create_reboot_mode_device for same.
  firmware: psci: Implement vendor-specific resets as reboot-mode
   - Update psci to use updated reboot_mode_register and store driver_name
     to struct reboot_mode_driver
- Add DT nodes for PSCI SYSTEM_RESET2 types for lemans-evk, qcs8300-ride,
  monaco-evk and qcs615-ride boards.
- Link to v14: https://lore.kernel.org/r/20250815-arm-psci-system_reset2-vendor-reboots-v14-0-37d29f59ac9a@oss.qualcomm.com

Changes in v14:
- mode-dm-verity-device-corrupted documented in cover letter -by André
 ABI Documentation:
- Updated KernelVersion in ABI documentation to reflect base commit
  version. – by André
- Revised ABI documentation to clarify space-separated format for
  supported reboot-mode commands. – by André
 power: reset: reboot-mode: Expose sysfs patch
- Modified `show_modes` to output a space-separated list of supported
  reboot modes – by André
- Added error handling in `create_reboot_mode_device()` to ensure
  proper cleanup on failure.
 firmware: psci:
- Locate psci/reboot-mode node using psci compatible. - by Krzysztof,
  Dmitry, Sudeep.
- Added error handling for additional code for compatible.
- Converted hex values to lowercase for consistency. – by André
- Introduced panic notifier to disable valid vendor-reset flag in
  panic path. – by André
- Added check for `psci_system_reset2` before registering vendor reset
  commands.
- Updated Commit text.
 dts: sa8775p:
- DT file name changed from sa8775p to lemans and commit text updated
  accordingly. – for dt renaming in base commit (sa8775p to lemans).
- Link to v13: https://lore.kernel.org/r/20250727-arm-psci-system_reset2-vendor-reboots-v13-0-6b8d23315898@oss.qualcomm.com

Changes in v13:
- Split patch1 into two (Synchronize list traversal and DT node-based
  registration) - by Dmitry.
- Move mutex lock inside get_reboot_mode_magic - by Dmitry.
- Reorder the patches – pull patch8 for exposing reboot-mode sysfs before
  psci patch - to align the change in reboot-mode sysfs patch.
- Update patch- reboot-mode: Expose sysfs for registered reboot_modes
     - Introduce a driver_name in reboot_mode_register. This will be used
       in sysfs creation  -  by Arnd.
     - Update documentation and commit text for above.
     - Fix release function to properly call delete attr file.
     - Fix sparse warning for devres_find.
     - Add error handling for devres_find.
- Split ABI documentation as a separate patch and update ABI documentation
  for usage of driver-name in sysfs - by Arnd
- Update patch - psci: Implement vendor-specific resets as reboot-mode
     - Fix Kconfig for CONFIG related warning.
     - Add driver_name as "psci" in register call to reboot-mode - by Arnd
- Link to v12: https://lore.kernel.org/r/20250721-arm-psci-system_reset2-vendor-reboots-v12-0-87bac3ec422e@oss.qualcomm.com

Changes in v12:
- Added lock for list traversals in reboot-mode - by Dmitry.
- Added proper handling for BE and LE cases in reboot-mode - by Dmitry.
- Removed type casting for u64 to u32 conversions. Added limit checks
  and used bitwise operations for same - by Andrew.
- Link to v11: https://lore.kernel.org/r/20250717-arm-psci-system_reset2-vendor-reboots-v11-0-df3e2b2183c3@oss.qualcomm.com

Changes in v11:
- Remove reference of cookie in reboot-mode – Arnd/Rob
- Introduce 64-bit magic in reboot-mode to accommodate two 32-bit
  arguments – Arnd
- Change reset-type to reboot-mode in psci device tree binding – Arnd
	- binding no more mandates two arguments as in v10.
	- dt changes done to support this binding.
- Remove obvious comments in psci reset path – Konrad
- Merge sysfs and ABI doc into single patch.
- Fix compilation issue on X86 configs.
- Fix warnings for pr_fmt.
- Link to v10: https://lore.kernel.org/all/569f154d-c714-1714-b898-83a42a38771c@oss.qualcomm.com/

Changes in V10:
- Change in reset-type binding to make cookie as a mandatory
  argument.
- Change reboot-mode binding to support additional argument
  "cookie".
 From Lorenzo:
- Use reboot-mode framework for implementing vendor-resets.
- Modify reboot-mode framework to support two arguments
  (magic and cookie).
- Expose sysfs for supported reboot-modes commands.
- List out all existing reboot-mode commands and their users.
   - Added this to cover letter.
 From Dmitry:
- Modify reboot-mode to support non-device based registration.
- Modify reboot-mode to create a class and device to expose
  sysfs interface.
- Link to v9: https://lore.kernel.org/all/20250303-arm-psci-system_reset2-vendor-reboots-v9-0-b2cf4a20feda@oss.qualcomm.com/

Changes in v9:
- Don't fallback to architecturally defined resets from Lorenzo.
- Link to v8: https://lore.kernel.org/r/20241107-arm-psci-system_reset2-vendor-reboots-v8-0-e8715fa65cb5@quicinc.com

Changes in v8:
- Code style nits from Stephen
- Add rb3gen2
- Link to v7: https://lore.kernel.org/r/20241028-arm-psci-system_reset2-vendor-reboots-v7-0-a4c40b0ebc54@quicinc.com

Changes in v7:
- Code style nits from Stephen
- Dropped unnecessary hunk from the sa8775p-ride patch
- Link to v6: https://lore.kernel.org/r/20241018-arm-psci-system_reset2-vendor-reboots-v6-0-50cbe88b0a24@quicinc.com

Changes in v6:
- Rebase to v6.11 and fix trivial conflicts in qcm6490-idp
- Add sa8775p-ride support (same as qcm6490-idp)
- Link to v5: https://lore.kernel.org/r/20240617-arm-psci-system_reset2-vendor-reboots-v5-0-086950f650c8@quicinc.com

Changes in v5:
- Drop the nested "items" in prep for future dtschema tools
- Link to v4: https://lore.kernel.org/r/20240611-arm-psci-system_reset2-vendor-reboots-v4-0-98f55aa74ae8@quicinc.com

Changes in v4:
- Change mode- properties from uint32-matrix to uint32-array
- Restructure the reset-types node so only the restriction is in the
  if/then schemas and not the entire definition
- Link to v3: https://lore.kernel.org/r/20240515-arm-psci-system_reset2-vendor-reboots-v3-0-16dd4f9c0ab4@quicinc.com

Changes in v3:
- Limit outer number of items to 1 for mode-* properties
- Move the reboot-mode for psci under a subnode "reset-types"
- Fix the DT node in qcm6490-idp so it doesn't overwrite the one from
  sc7820.dtsi
- Link to v2: https://lore.kernel.org/r/20240414-arm-psci-system_reset2-vendor-reboots-v2-0-da9a055a648f@quicinc.com

Changes in v2:
- Fixes to schema as suggested by Rob and Krzysztof
- Add qcm6490 idp as first Qualcomm device to support
- Link to v1: https://lore.kernel.org/r/20231117-arm-psci-system_reset2-vendor-reboots-v1-0-03c4612153e2@quicinc.com

Changes in v1:
- Reference reboot-mode bindings as suggeted by Rob.
- Link to RFC: https://lore.kernel.org/r/20231030-arm-psci-system_reset2-vendor-reboots-v1-0-dcdd63352ad1@quicinc.com

---
Elliot Berman (3):
      dt-bindings: arm: Document reboot mode magic
      arm64: dts: qcom: qcm6490-idp: Add PSCI SYSTEM_RESET2 types
      arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

Shivendra Pratap (9):
      power: reset: reboot-mode: Remove devres based allocations
      power: reset: reboot-mode: Add firmware node based registration
      power: reset: reboot-mode: Add support for 64 bit magic
      Documentation: ABI: Add sysfs-class-reboot-mode-reboot_modes
      power: reset: reboot-mode: Expose sysfs for registered reboot_modes
      firmware: psci: Implement vendor-specific resets as reboot-mode
      arm64: dts: qcom: lemans: Add PSCI SYSTEM_RESET2 types
      arm64: dts: qcom: monaco: Add PSCI SYSTEM_RESET2 types
      arm64: dts: qcom: talos: Add PSCI SYSTEM_RESET2 types

 .../testing/sysfs-class-reboot-mode-reboot_modes   |  39 +++++++
 Documentation/devicetree/bindings/arm/psci.yaml    |  43 +++++++
 arch/arm64/boot/dts/qcom/lemans.dtsi               |   5 +
 arch/arm64/boot/dts/qcom/monaco.dtsi               |   5 +
 arch/arm64/boot/dts/qcom/qcm6490-idp.dts           |   7 ++
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |   7 ++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               |   2 +-
 arch/arm64/boot/dts/qcom/talos.dtsi                |   5 +
 drivers/firmware/psci/Kconfig                      |   2 +
 drivers/firmware/psci/psci.c                       |  92 ++++++++++++++-
 drivers/power/reset/nvmem-reboot-mode.c            |  13 ++-
 drivers/power/reset/qcom-pon.c                     |  11 +-
 drivers/power/reset/reboot-mode.c                  | 126 ++++++++++++++++++---
 drivers/power/reset/syscon-reboot-mode.c           |  11 +-
 include/linux/reboot-mode.h                        |   9 +-
 15 files changed, 345 insertions(+), 32 deletions(-)
---
base-commit: f9ba12abc5282bf992f9a9ae87ad814fd03a0270
change-id: 20250709-arm-psci-system_reset2-vendor-reboots-46c80044afcf

Best regards,
-- 
Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

[PATCH v17 01/12] power: reset: reboot-mode: Remove devres based allocations

[Shivendra Pratap]

Devres APIs are intended for use in drivers, and they should be
avoided in shared subsystem code which is being used by multiple
drivers. Avoid using devres based allocations in the reboot-mode
subsystem and manually free the resources.

Replace devm_kzalloc with kzalloc and handle memory cleanup
explicitly.

Fixes: 4fcd504edbf7 ("power: reset: add reboot mode driver")
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index fba53f638da04655e756b5f8b7d2d666d1379535..ac4223794083f36960b2bd37a601b7e1f1872de5 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
  */
 
+#define pr_fmt(fmt)	"reboot-mode: " fmt
+
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -71,6 +73,7 @@ static int reboot_mode_notify(struct notifier_block *this,
 int reboot_mode_register(struct reboot_mode_driver *reboot)
 {
 	struct mode_info *info;
+	struct mode_info *next;
 	struct property *prop;
 	struct device_node *np = reboot->dev->of_node;
 	size_t len = strlen(PREFIX);
@@ -82,29 +85,27 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
 		if (strncmp(prop->name, PREFIX, len))
 			continue;
 
-		info = devm_kzalloc(reboot->dev, sizeof(*info), GFP_KERNEL);
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
 		if (!info) {
 			ret = -ENOMEM;
 			goto error;
 		}
 
 		if (of_property_read_u32(np, prop->name, &info->magic)) {
-			dev_err(reboot->dev, "reboot mode %s without magic number\n",
-				info->mode);
-			devm_kfree(reboot->dev, info);
+			pr_err("reboot mode %s without magic number\n", info->mode);
+			kfree(info);
 			continue;
 		}
 
 		info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
 		if (!info->mode) {
 			ret =  -ENOMEM;
-			goto error;
+			goto err_info;
 		} else if (info->mode[0] == '\0') {
 			kfree_const(info->mode);
 			ret = -EINVAL;
-			dev_err(reboot->dev, "invalid mode name(%s): too short!\n",
-				prop->name);
-			goto error;
+			pr_err("invalid mode name(%s): too short!\n", prop->name);
+			goto err_info;
 		}
 
 		list_add_tail(&info->list, &reboot->head);
@@ -115,9 +116,14 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
 
 	return 0;
 
+err_info:
+	kfree(info);
 error:
-	list_for_each_entry(info, &reboot->head, list)
+	list_for_each_entry_safe(info, next, &reboot->head, list) {
+		list_del(&info->list);
 		kfree_const(info->mode);
+		kfree(info);
+	}
 
 	return ret;
 }
@@ -130,11 +136,15 @@ EXPORT_SYMBOL_GPL(reboot_mode_register);
 int reboot_mode_unregister(struct reboot_mode_driver *reboot)
 {
 	struct mode_info *info;
+	struct mode_info *next;
 
 	unregister_reboot_notifier(&reboot->reboot_notifier);
 
-	list_for_each_entry(info, &reboot->head, list)
+	list_for_each_entry_safe(info, next, &reboot->head, list) {
+		list_del(&info->list);
 		kfree_const(info->mode);
+		kfree(info);
+	}
 
 	return 0;
 }

-- 
2.34.1

[PATCH v17 02/12] power: reset: reboot-mode: Add firmware node based registration

[Shivendra Pratap]

The reboot-mode driver does not have a strict requirement for
device-based registration. It primarily uses the device's of_node
to read mode-<cmd> properties.

Remove the dependency on struct device and introduce support for
firmware node (fwnode) based registration. This enables drivers
that are not associated with a struct device to leverage the
reboot-mode framework.

Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 drivers/power/reset/reboot-mode.c | 19 ++++++++++++++++---
 include/linux/reboot-mode.h       |  4 +++-
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index ac4223794083f36960b2bd37a601b7e1f1872de5..eff60d6e04df2cb84ba59d38512654336f272f8a 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -8,10 +8,12 @@
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/reboot.h>
 #include <linux/reboot-mode.h>
+#include <linux/slab.h>
 
 #define PREFIX "mode-"
 
@@ -67,18 +69,26 @@ static int reboot_mode_notify(struct notifier_block *this,
 /**
  * reboot_mode_register - register a reboot mode driver
  * @reboot: reboot mode driver
+ * @fwnode: Firmware node with reboot-mode configuration
  *
  * Returns: 0 on success or a negative error code on failure.
  */
-int reboot_mode_register(struct reboot_mode_driver *reboot)
+int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle *fwnode)
 {
 	struct mode_info *info;
 	struct mode_info *next;
+	struct device_node *np;
 	struct property *prop;
-	struct device_node *np = reboot->dev->of_node;
 	size_t len = strlen(PREFIX);
 	int ret;
 
+	if (!fwnode)
+		return -EINVAL;
+
+	np = to_of_node(fwnode);
+	if (!np)
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&reboot->head);
 
 	for_each_property_of_node(np, prop) {
@@ -168,11 +178,14 @@ int devm_reboot_mode_register(struct device *dev,
 	struct reboot_mode_driver **dr;
 	int rc;
 
+	if (!reboot->dev || !reboot->dev->of_node)
+		return -EINVAL;
+
 	dr = devres_alloc(devm_reboot_mode_release, sizeof(*dr), GFP_KERNEL);
 	if (!dr)
 		return -ENOMEM;
 
-	rc = reboot_mode_register(reboot);
+	rc = reboot_mode_register(reboot, of_fwnode_handle(reboot->dev->of_node));
 	if (rc) {
 		devres_free(dr);
 		return rc;
diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
index 4a2abb38d1d612ec0fdf05eb18c98b210f631b7f..22f707ade4ba93592823ea8718d467dbfc5ffd7c 100644
--- a/include/linux/reboot-mode.h
+++ b/include/linux/reboot-mode.h
@@ -2,6 +2,8 @@
 #ifndef __REBOOT_MODE_H__
 #define __REBOOT_MODE_H__
 
+#include <linux/fwnode.h>
+
 struct reboot_mode_driver {
 	struct device *dev;
 	struct list_head head;
@@ -9,7 +11,7 @@ struct reboot_mode_driver {
 	struct notifier_block reboot_notifier;
 };
 
-int reboot_mode_register(struct reboot_mode_driver *reboot);
+int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle *fwnode);
 int reboot_mode_unregister(struct reboot_mode_driver *reboot);
 int devm_reboot_mode_register(struct device *dev,
 			      struct reboot_mode_driver *reboot);

-- 
2.34.1

[PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Shivendra Pratap]

Current reboot-mode supports a single 32-bit argument for any
supported mode. Some reboot-mode based drivers may require
passing two independent 32-bit arguments during a reboot
sequence, for uses-cases, where a mode requires an additional
argument. Such drivers may not be able to use the reboot-mode
driver. For example, ARM PSCI vendor-specific resets, need two
arguments for its operation – reset_type and cookie, to complete
the reset operation. If a driver wants to implement this
firmware-based reset, it cannot use reboot-mode framework.

Introduce 64-bit magic values in reboot-mode driver to
accommodate dual 32-bit arguments when specified via device tree.
In cases, where no second argument is passed from device tree,
keep the upper 32-bit of magic un-changed(0) to maintain backward
compatibility.

Update the current drivers using reboot-mode for a 64-bit magic
value.

Reviewed-by: Umang Chheda <umang.chheda@oss.qualcomm.com>
Reviewed-by: Nirmesh Kumar Singh <nirmesh.singh@oss.qualcomm.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 drivers/power/reset/nvmem-reboot-mode.c  | 13 +++++++++----
 drivers/power/reset/qcom-pon.c           | 11 ++++++++---
 drivers/power/reset/reboot-mode.c        | 19 +++++++++++++------
 drivers/power/reset/syscon-reboot-mode.c | 11 ++++++++---
 include/linux/reboot-mode.h              |  3 ++-
 5 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/power/reset/nvmem-reboot-mode.c b/drivers/power/reset/nvmem-reboot-mode.c
index 41530b70cfc48c2a83fbbd96f523d5816960a0d1..5d73dde585b1fd438b1847f884feb37cd9e4dd5c 100644
--- a/drivers/power/reset/nvmem-reboot-mode.c
+++ b/drivers/power/reset/nvmem-reboot-mode.c
@@ -16,15 +16,20 @@ struct nvmem_reboot_mode {
 	struct nvmem_cell *cell;
 };
 
-static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
-				    unsigned int magic)
+static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
 {
-	int ret;
 	struct nvmem_reboot_mode *nvmem_rbm;
+	u32 magic_32;
+	int ret;
+
+	if (magic > U32_MAX)
+		return -EINVAL;
+
+	magic_32 = magic;
 
 	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
 
-	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
+	ret = nvmem_cell_write(nvmem_rbm->cell, &magic_32, sizeof(magic_32));
 	if (ret < 0)
 		dev_err(reboot->dev, "update reboot mode bits failed\n");
 
diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
index 7e108982a582e8243c5c806bd4a793646b87189f..d0ed9431a02313a7bbaa93743c16fa1ae713ddfe 100644
--- a/drivers/power/reset/qcom-pon.c
+++ b/drivers/power/reset/qcom-pon.c
@@ -27,17 +27,22 @@ struct qcom_pon {
 	long reason_shift;
 };
 
-static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
-				    unsigned int magic)
+static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
 {
 	struct qcom_pon *pon = container_of
 			(reboot, struct qcom_pon, reboot_mode);
+	u32 magic_32;
 	int ret;
 
+	if (magic > U32_MAX || (magic << pon->reason_shift) > U32_MAX)
+		return -EINVAL;
+
+	magic_32 = magic << pon->reason_shift;
+
 	ret = regmap_update_bits(pon->regmap,
 				 pon->baseaddr + PON_SOFT_RB_SPARE,
 				 GENMASK(7, pon->reason_shift),
-				 magic << pon->reason_shift);
+				 magic_32);
 	if (ret < 0)
 		dev_err(pon->dev, "update reboot mode bits failed\n");
 
diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index eff60d6e04df2cb84ba59d38512654336f272f8a..873ac45cd7659b214b7c21958f580ca381e0a63d 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -19,12 +19,11 @@
 
 struct mode_info {
 	const char *mode;
-	u32 magic;
+	u64 magic;
 	struct list_head list;
 };
 
-static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
-					  const char *cmd)
+static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
 {
 	const char *normal = "normal";
 	struct mode_info *info;
@@ -56,7 +55,7 @@ static int reboot_mode_notify(struct notifier_block *this,
 			      unsigned long mode, void *cmd)
 {
 	struct reboot_mode_driver *reboot;
-	unsigned int magic;
+	u64 magic;
 
 	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
 	magic = get_reboot_mode_magic(reboot, cmd);
@@ -80,6 +79,8 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
 	struct device_node *np;
 	struct property *prop;
 	size_t len = strlen(PREFIX);
+	u32 magic_arg1;
+	u32 magic_arg2;
 	int ret;
 
 	if (!fwnode)
@@ -101,12 +102,18 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
 			goto error;
 		}
 
-		if (of_property_read_u32(np, prop->name, &info->magic)) {
-			pr_err("reboot mode %s without magic number\n", info->mode);
+		if (of_property_read_u32(np, prop->name, &magic_arg1)) {
+			pr_err("reboot mode without magic number\n");
 			kfree(info);
 			continue;
 		}
 
+		if (of_property_read_u32_index(np, prop->name, 1, &magic_arg2))
+			magic_arg2 = 0;
+
+		info->magic = magic_arg2;
+		info->magic = (info->magic << 32) | magic_arg1;
+
 		info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
 		if (!info->mode) {
 			ret =  -ENOMEM;
diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
index e0772c9f70f7a19cd8ec8a0b7fdbbaa7ba44afd0..3cbd000c512239b12ec51987e900d260540a9dea 100644
--- a/drivers/power/reset/syscon-reboot-mode.c
+++ b/drivers/power/reset/syscon-reboot-mode.c
@@ -20,16 +20,21 @@ struct syscon_reboot_mode {
 	u32 mask;
 };
 
-static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot,
-				    unsigned int magic)
+static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
 {
 	struct syscon_reboot_mode *syscon_rbm;
+	u32 magic_32;
 	int ret;
 
+	if (magic > U32_MAX)
+		return -EINVAL;
+
+	magic_32 = magic;
+
 	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
 
 	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
-				 syscon_rbm->mask, magic);
+				 syscon_rbm->mask, magic_32);
 	if (ret < 0)
 		dev_err(reboot->dev, "update reboot mode bits failed\n");
 
diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
index 22f707ade4ba93592823ea8718d467dbfc5ffd7c..e0d3e8a54050a76f26846f456120b4c7e371d284 100644
--- a/include/linux/reboot-mode.h
+++ b/include/linux/reboot-mode.h
@@ -3,11 +3,12 @@
 #define __REBOOT_MODE_H__
 
 #include <linux/fwnode.h>
+#include <linux/types.h>
 
 struct reboot_mode_driver {
 	struct device *dev;
 	struct list_head head;
-	int (*write)(struct reboot_mode_driver *reboot, unsigned int magic);
+	int (*write)(struct reboot_mode_driver *reboot, u64 magic);
 	struct notifier_block reboot_notifier;
 };
 

-- 
2.34.1

[PATCH v17 04/12] Documentation: ABI: Add sysfs-class-reboot-mode-reboot_modes

[Shivendra Pratap]

Add ABI documentation for /sys/class/reboot-mode/*/reboot_modes,
a read-only sysfs attribute exposing the list of supported
reboot-mode arguments. This file is created by reboot-mode
framework and provides a user-readable interface to query
available reboot-mode arguments.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 .../testing/sysfs-class-reboot-mode-reboot_modes   | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-reboot-mode-reboot_modes b/Documentation/ABI/testing/sysfs-class-reboot-mode-reboot_modes
new file mode 100644
index 0000000000000000000000000000000000000000..6a3fc379afae3a6caf56ad0b73b1c06c43a9fee7
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-reboot-mode-reboot_modes
@@ -0,0 +1,39 @@
+What:		/sys/class/reboot-mode/<driver>/reboot_modes
+Date:		August 2025
+KernelVersion:	6.17.0-rc1
+Contact:	linux-pm@vger.kernel.org
+		Description:
+		This interface exposes the reboot-mode arguments
+		registered with the reboot-mode framework. It is
+		a read-only interface and provides a space
+		separated list of reboot-mode arguments supported
+		on the current platform.
+		Example:
+		 recovery fastboot bootloader
+
+		The exact sysfs path may vary depending on the
+		name of the driver that registers the arguments.
+		Example:
+		 /sys/class/reboot-mode/nvmem-reboot-mode/reboot_modes
+		 /sys/class/reboot-mode/syscon-reboot-mode/reboot_modes
+		 /sys/class/reboot-mode/qcom-pon/reboot_modes
+
+		The supported arguments can be used by userspace
+		to invoke device reset using the reboot() system
+		call, with the "argument" as string to "*arg"
+		parameter along with LINUX_REBOOT_CMD_RESTART2.
+		Example:
+		 reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2,
+		        LINUX_REBOOT_CMD_RESTART2, "bootloader");
+
+		A driver can expose the supported arguments by
+		registering them with the reboot-mode framework
+		using the property names that follow the
+		mode-<argument> format.
+		Example:
+		 mode-bootloader, mode-recovery.
+
+		This attribute is useful for scripts or initramfs
+		logic that need to programmatically determine
+		which reboot-mode arguments are valid before
+		triggering a reboot.

-- 
2.34.1

[PATCH v17 05/12] power: reset: reboot-mode: Expose sysfs for registered reboot_modes

[Shivendra Pratap]

Currently, there is no standardized mechanism for userspace to
discover which reboot-modes are supported on a given platform.
This limitation forces tools and scripts to rely on hardcoded
assumptions about the supported reboot-modes.

Create a class 'reboot-mode' and a device under it to expose a
sysfs interface to show the available reboot mode arguments to
userspace. Use the driver_name field of the struct
reboot_mode_driver to create the device. For device-based
drivers, configure the device driver name as driver_name.

This results in the creation of:
  /sys/class/reboot-mode/<driver>/reboot_modes

This read-only sysfs file will exposes the list of supported
reboot modes arguments provided by the driver, enabling userspace
to query the list of arguments.

Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 drivers/power/reset/reboot-mode.c | 62 ++++++++++++++++++++++++++++++++++++++-
 include/linux/reboot-mode.h       |  2 ++
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index 873ac45cd7659b214b7c21958f580ca381e0a63d..582aa7f8ed7fa485c5a67877558c9b15d3600ef4 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -6,6 +6,7 @@
 #define pr_fmt(fmt)	"reboot-mode: " fmt
 
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -23,6 +24,8 @@ struct mode_info {
 	struct list_head list;
 };
 
+static struct class *rb_class;
+
 static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
 {
 	const char *normal = "normal";
@@ -65,6 +68,51 @@ static int reboot_mode_notify(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
+static ssize_t reboot_modes_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct reboot_mode_driver *reboot;
+	struct mode_info *info;
+	ssize_t size = 0;
+
+	reboot = (struct reboot_mode_driver *)dev_get_drvdata(dev);
+	if (!reboot)
+		return -ENODATA;
+
+	list_for_each_entry(info, &reboot->head, list)
+		size += sysfs_emit_at(buf, size, "%s ", info->mode);
+
+	if (size) {
+		size += sysfs_emit_at(buf, size - 1, "\n");
+		return size;
+	}
+
+	return -ENODATA;
+}
+static DEVICE_ATTR_RO(reboot_modes);
+
+static int create_reboot_mode_device(struct reboot_mode_driver *reboot)
+{
+	int ret = 0;
+
+	if (!rb_class) {
+		rb_class = class_create("reboot-mode");
+		if (IS_ERR(rb_class))
+			return PTR_ERR(rb_class);
+	}
+
+	reboot->reboot_dev = device_create(rb_class, NULL, 0, (void *)reboot, reboot->driver_name);
+	if (IS_ERR(reboot->reboot_dev))
+		return PTR_ERR(reboot->reboot_dev);
+
+	ret = device_create_file(reboot->reboot_dev, &dev_attr_reboot_modes);
+	if (ret) {
+		device_unregister(reboot->reboot_dev);
+		return ret;
+	}
+
+	return ret;
+}
+
 /**
  * reboot_mode_register - register a reboot mode driver
  * @reboot: reboot mode driver
@@ -83,13 +131,17 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
 	u32 magic_arg2;
 	int ret;
 
-	if (!fwnode)
+	if (!fwnode || !reboot->driver_name)
 		return -EINVAL;
 
 	np = to_of_node(fwnode);
 	if (!np)
 		return -EINVAL;
 
+	ret = create_reboot_mode_device(reboot);
+	if (ret)
+		return ret;
+
 	INIT_LIST_HEAD(&reboot->head);
 
 	for_each_property_of_node(np, prop) {
@@ -142,6 +194,8 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
 		kfree(info);
 	}
 
+	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
+	device_unregister(reboot->reboot_dev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(reboot_mode_register);
@@ -155,6 +209,9 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
 	struct mode_info *info;
 	struct mode_info *next;
 
+	if (!reboot->reboot_dev)
+		return -EINVAL;
+
 	unregister_reboot_notifier(&reboot->reboot_notifier);
 
 	list_for_each_entry_safe(info, next, &reboot->head, list) {
@@ -163,6 +220,8 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
 		kfree(info);
 	}
 
+	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
+	device_unregister(reboot->reboot_dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(reboot_mode_unregister);
@@ -192,6 +251,7 @@ int devm_reboot_mode_register(struct device *dev,
 	if (!dr)
 		return -ENOMEM;
 
+	reboot->driver_name = reboot->dev->driver->name;
 	rc = reboot_mode_register(reboot, of_fwnode_handle(reboot->dev->of_node));
 	if (rc) {
 		devres_free(dr);
diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
index e0d3e8a54050a76f26846f456120b4c7e371d284..81c149edf40fbcf0d3427c2e12eb415199cb153b 100644
--- a/include/linux/reboot-mode.h
+++ b/include/linux/reboot-mode.h
@@ -7,6 +7,8 @@
 
 struct reboot_mode_driver {
 	struct device *dev;
+	struct device *reboot_dev;
+	const char *driver_name;
 	struct list_head head;
 	int (*write)(struct reboot_mode_driver *reboot, u64 magic);
 	struct notifier_block reboot_notifier;

-- 
2.34.1

[PATCH v17 06/12] dt-bindings: arm: Document reboot mode magic

[Shivendra Pratap]

From: Elliot Berman <elliot.berman@oss.qualcomm.com>

Add bindings to describe vendor-specific reboot modes. Values here
correspond to valid parameters to vendor-specific reset types in PSCI
SYSTEM_RESET2 call.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/arm/psci.yaml | 43 +++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
index 6e2e0c551841111fbb0aa8c0951dca411b94035c..5d4368d77d07e3340bb380df93aae31df40e2779 100644
--- a/Documentation/devicetree/bindings/arm/psci.yaml
+++ b/Documentation/devicetree/bindings/arm/psci.yaml
@@ -98,6 +98,27 @@ properties:
       [1] Kernel documentation - ARM idle states bindings
         Documentation/devicetree/bindings/cpu/idle-states.yaml
 
+  reboot-mode:
+    type: object
+    $ref: /schemas/power/reset/reboot-mode.yaml#
+    unevaluatedProperties: false
+    properties:
+      # "mode-normal" is just SYSTEM_RESET
+      mode-normal: false
+    patternProperties:
+      "^mode-.*$":
+        minItems: 1
+        maxItems: 2
+        description: |
+          Describes a vendor-specific reset type. The string after "mode-"
+          maps a reboot mode to the parameters in the PSCI SYSTEM_RESET2 call.
+
+          Parameters are named mode-xxx = <type[, cookie]>, where xxx
+          is the name of the magic reboot mode, type is the lower 31 bits
+          of the reset_type, and, optionally, the cookie value. If the cookie
+          is not provided, it is defaulted to zero.
+          The 31st bit (vendor-resets) will be implicitly set by the driver.
+
 patternProperties:
   "^power-domain-":
     $ref: /schemas/power/power-domain.yaml#
@@ -137,6 +158,15 @@ allOf:
       required:
         - cpu_off
         - cpu_on
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: arm,psci-1.0
+    then:
+      properties:
+        reboot-mode: false
 
 additionalProperties: false
 
@@ -260,4 +290,17 @@ examples:
         domain-idle-states = <&cluster_ret>, <&cluster_pwrdn>;
       };
     };
+
+  - |+
+
+    // Case 5: SYSTEM_RESET2 vendor resets
+    psci {
+      compatible = "arm,psci-1.0";
+      method = "smc";
+
+      reboot-mode {
+        mode-edl = <0>;
+        mode-bootloader = <1 2>;
+      };
+    };
 ...

-- 
2.34.1

[PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Shivendra Pratap]

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.

Implement the PSCI vendor-specific resets by registering to the
reboot-mode framework. As psci init is done at early kernel init,
reboot-mode registration cannot be done at the time of psci init.
This is because reboot-mode creates a “reboot-mode” class for
exposing sysfs, which can fail at early kernel init. To overcome
this, 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. Register a
panic notifier and clear vendor_reset valid status during panic.
This is needed for any kernel panic that occurs post
reboot_notifiers.

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 under “reboot-mode” and are based on the
reboot-mode based commands.

Reviewed-by: Umang Chheda <umang.chheda@oss.qualcomm.com>
Reviewed-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
Reviewed-by: Nirmesh Kumar Singh <nirmesh.singh@oss.qualcomm.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 drivers/firmware/psci/Kconfig |  2 +
 drivers/firmware/psci/psci.c  | 92 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 93 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..ff82e7f4c27d1609a75cedc3a9790affaf839801 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -8,15 +8,18 @@
 
 #include <linux/acpi.h>
 #include <linux/arm-smccc.h>
+#include <linux/bitops.h>
 #include <linux/cpuidle.h>
 #include <linux/debugfs.h>
 #include <linux/errno.h>
 #include <linux/linkage.h>
 #include <linux/of.h>
+#include <linux/panic_notifier.h>
 #include <linux/pm.h>
 #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 +54,24 @@ 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;
+
+static int psci_panic_event(struct notifier_block *nb, unsigned long v, void *p)
+{
+	vendor_reset.valid = false;
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block psci_panic_block = {
+	.notifier_call = psci_panic_event
+};
+
 bool psci_tos_resident_on(int cpu)
 {
 	return cpu == resident_cpu;
@@ -309,7 +330,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)
@@ -547,6 +571,72 @@ 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 & GENMASK(31, 0);
+		vendor_reset.reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic_32;
+		vendor_reset.cookie = (magic >> 32) & GENMASK(31, 0);
+		vendor_reset.valid = true;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int __init psci_init_vendor_reset(void)
+{
+	struct reboot_mode_driver *reboot;
+	struct device_node *psci_np;
+	struct device_node *np;
+	int ret;
+
+	if (!psci_system_reset2_supported)
+		return -EINVAL;
+
+	psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
+	if (!psci_np)
+		return -ENODEV;
+
+	np = of_find_node_by_name(psci_np, "reboot-mode");
+	if (!np) {
+		of_node_put(psci_np);
+		return -ENODEV;
+	}
+
+	ret = atomic_notifier_chain_register(&panic_notifier_list, &psci_panic_block);
+	if (ret)
+		goto err_notifier;
+
+	reboot = kzalloc(sizeof(*reboot), GFP_KERNEL);
+	if (!reboot) {
+		ret = -ENOMEM;
+		goto err_kzalloc;
+	}
+
+	reboot->write = psci_set_vendor_sys_reset2;
+	reboot->driver_name = "psci";
+
+	ret = reboot_mode_register(reboot, of_fwnode_handle(np));
+	if (ret)
+		goto err_register;
+
+	of_node_put(psci_np);
+	of_node_put(np);
+	return 0;
+
+err_register:
+	kfree(reboot);
+err_kzalloc:
+	atomic_notifier_chain_unregister(&panic_notifier_list, &psci_panic_block);
+err_notifier:
+	of_node_put(psci_np);
+	of_node_put(np);
+	return ret;
+}
+late_initcall(psci_init_vendor_reset)
+
 static void __init psci_init_system_reset2(void)
 {
 	int ret;

-- 
2.34.1

[PATCH v17 08/12] arm64: dts: qcom: qcm6490-idp: Add PSCI SYSTEM_RESET2 types

[Shivendra Pratap]

From: Elliot Berman <elliot.berman@oss.qualcomm.com>

Add support for SYSTEM_RESET2 vendor-specific resets in
qcm6490-idp as reboot-modes.  Describe the resets: "bootloader"
will cause device to reboot and stop in the bootloader's fastboot
mode. "edl" will cause device to reboot into "emergency download
mode", which permits loading images via the Firehose protocol.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 7 +++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi     | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
index 73fce639370cd356687f14a3091848b8f422e36c..84322b74917f3a70adce5a4182adfa5d787ab11c 100644
--- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
+++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
@@ -695,6 +695,13 @@ &pon_resin {
 	status = "okay";
 };
 
+&psci {
+	reboot-mode {
+		mode-bootloader = <0x10001 0x2>;
+		mode-edl = <0 0x1>;
+	};
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 3ef61af2ed8a87d03e82131fcd8848f0a1bc509c..5fe7a2220e1dec3b5cffdf151e09553d54e27960 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -858,7 +858,7 @@ pmu-a78 {
 		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
 	};
 
-	psci {
+	psci: psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
 

-- 
2.34.1

[PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Shivendra Pratap]

From: Elliot Berman <elliot.berman@oss.qualcomm.com>

Add support for SYSTEM_RESET2 vendor-specific resets in
qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
"bootloader" will cause device to reboot and stop in the
bootloader's fastboot mode. "edl" will cause device to reboot
into "emergency download mode", which permits loading images via
the Firehose protocol.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -935,6 +935,13 @@ &pon_resin {
 	status = "okay";
 };
 
+&psci {
+	reboot-mode {
+		mode-bootloader = <0x10001 0x2>;
+		mode-edl = <0 0x1>;
+	};
+};
+
 &qup_uart7_cts {
 	/*
 	 * Configure a bias-bus-hold on CTS to lower power

-- 
2.34.1

[PATCH v17 10/12] arm64: dts: qcom: lemans: Add PSCI SYSTEM_RESET2 types

[Shivendra Pratap]

Add support for SYSTEM_RESET2 vendor-specific resets
as reboot-modes in the psci node.  Describe the resets:
"bootloader" will cause device to reboot and stop in the
bootloader's fastboot mode.  "edl" will cause device to reboot
into "emergency download mode", which permits loading images via
the Firehose protocol.

Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/lemans.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/lemans.dtsi b/arch/arm64/boot/dts/qcom/lemans.dtsi
index 0b154d57ba24e69a9d900f06bbb22baa2781cc3f..8b594a6535199b38ab6ca432673c4a9432c0df39 100644
--- a/arch/arm64/boot/dts/qcom/lemans.dtsi
+++ b/arch/arm64/boot/dts/qcom/lemans.dtsi
@@ -698,6 +698,11 @@ system_pd: power-domain-system {
 			#power-domain-cells = <0>;
 			domain-idle-states = <&cluster_sleep_apss_rsc_pc>;
 		};
+
+		reboot-mode {
+			mode-bootloader = <0x10001 0x2>;
+			mode-edl = <0 0x1>;
+		};
 	};
 
 	reserved-memory {

-- 
2.34.1

[PATCH v17 11/12] arm64: dts: qcom: monaco: Add PSCI SYSTEM_RESET2 types

[Shivendra Pratap]

Add support for SYSTEM_RESET2 vendor-specific resets as
reboot-modes in the psci node.  Describe the resets: "bootloader"
will cause device to reboot and stop in the bootloader's fastboot
mode.  "edl" will cause device to reboot into "emergency download
mode", which permits loading images via the Firehose protocol.

Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/monaco.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
index 816fa2af8a9a663b8ad176f93d2f18284a08c3d1..e690ba62d9099e270e14510325dd1422b152584b 100644
--- a/arch/arm64/boot/dts/qcom/monaco.dtsi
+++ b/arch/arm64/boot/dts/qcom/monaco.dtsi
@@ -732,6 +732,11 @@ system_pd: power-domain-system {
 			#power-domain-cells = <0>;
 			domain-idle-states = <&system_sleep>;
 		};
+
+		reboot-mode {
+			mode-bootloader = <0x10001 0x2>;
+			mode-edl = <0 0x1>;
+		};
 	};
 
 	reserved-memory {

-- 
2.34.1

[PATCH v17 12/12] arm64: dts: qcom: talos: Add PSCI SYSTEM_RESET2 types

[Shivendra Pratap]

Add support for SYSTEM_RESET2 vendor-specific resets as
reboot-modes in the psci node.  Describe the resets: "bootloader"
will cause device to reboot and stop in the bootloader's fastboot
mode.  "edl" will cause device to reboot into "emergency download
mode", which permits loading images via the Firehose protocol.

Signed-off-by: Song Xue <quic_songxue@quicinc.com>
Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/talos.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/talos.dtsi b/arch/arm64/boot/dts/qcom/talos.dtsi
index eb6f69be4a82ea47486f5c8e39519d0952b146cb..c946d07c540f5ae40f147ccb06539ecf0de18df5 100644
--- a/arch/arm64/boot/dts/qcom/talos.dtsi
+++ b/arch/arm64/boot/dts/qcom/talos.dtsi
@@ -612,6 +612,11 @@ cluster_pd: power-domain-cluster {
 					      &cluster_sleep_1
 					      &cluster_sleep_2>;
 		};
+
+		reboot-mode {
+			mode-bootloader = <0x10001 0x2>;
+			mode-edl = <0 0x1>;
+		};
 	};
 
 	reserved-memory {

-- 
2.34.1

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Kathiravan Thirumoorthy]

On 11/9/2025 8:07 PM, Shivendra Pratap wrote:
> +static int __init psci_init_vendor_reset(void)
> +{
> +	struct reboot_mode_driver *reboot;
> +	struct device_node *psci_np;
> +	struct device_node *np;


We can take advantage of __cleanup() attribute to simply the code paths.

Declare the variables like below

     struct device_node *psci_np __free(device_node) = NULL;
     struct device_node *np __free(device_node) = NULL;

and get rid of the explicit of_node_put().

I think, we can take up this an improvement once this series landed. But 
if you happen to respin to address other issues, please take care of 
this as well.


> +	int ret;
> +
> +	if (!psci_system_reset2_supported)
> +		return -EINVAL;
> +
> +	psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
> +	if (!psci_np)
> +		return -ENODEV;
> +
> +	np = of_find_node_by_name(psci_np, "reboot-mode");
> +	if (!np) {
> +		of_node_put(psci_np);
> +		return -ENODEV;
> +	}
> +
> +	ret = atomic_notifier_chain_register(&panic_notifier_list, &psci_panic_block);
> +	if (ret)
> +		goto err_notifier;
> +
> +	reboot = kzalloc(sizeof(*reboot), GFP_KERNEL);
> +	if (!reboot) {
> +		ret = -ENOMEM;
> +		goto err_kzalloc;
> +	}
> +
> +	reboot->write = psci_set_vendor_sys_reset2;
> +	reboot->driver_name = "psci";
> +
> +	ret = reboot_mode_register(reboot, of_fwnode_handle(np));
> +	if (ret)
> +		goto err_register;
> +
> +	of_node_put(psci_np);
> +	of_node_put(np);
> +	return 0;
> +
> +err_register:
> +	kfree(reboot);
> +err_kzalloc:
> +	atomic_notifier_chain_unregister(&panic_notifier_list, &psci_panic_block);
> +err_notifier:
> +	of_node_put(psci_np);
> +	of_node_put(np);
> +	return ret;
> +}
> +late_initcall(psci_init_vendor_reset)

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Mukesh Ojha]

On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
> From: Elliot Berman <elliot.berman@oss.qualcomm.com>
> 
> Add support for SYSTEM_RESET2 vendor-specific resets in
> qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
> "bootloader" will cause device to reboot and stop in the
> bootloader's fastboot mode. "edl" will cause device to reboot
> into "emergency download mode", which permits loading images via
> the Firehose protocol.
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -935,6 +935,13 @@ &pon_resin {
>  	status = "okay";
>  };
>  
> +&psci {
> +	reboot-mode {
> +		mode-bootloader = <0x10001 0x2>;
> +		mode-edl = <0 0x1>;
> +	};
> +};
> +

Make sense for this as it leverages sc7280 and adding it there would not
have made sense.

Acked-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

>  &qup_uart7_cts {
>  	/*
>  	 * Configure a bias-bus-hold on CTS to lower power
> 
> -- 
> 2.34.1
> 

-- 
-Mukesh Ojha

Re: [PATCH v17 12/12] arm64: dts: qcom: talos: Add PSCI SYSTEM_RESET2 types

[Mukesh Ojha]

On Sun, Nov 09, 2025 at 08:07:25PM +0530, Shivendra Pratap wrote:
> Add support for SYSTEM_RESET2 vendor-specific resets as
> reboot-modes in the psci node.  Describe the resets: "bootloader"
> will cause device to reboot and stop in the bootloader's fastboot
> mode.  "edl" will cause device to reboot into "emergency download
> mode", which permits loading images via the Firehose protocol.
> 
> Signed-off-by: Song Xue <quic_songxue@quicinc.com>
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/talos.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/talos.dtsi b/arch/arm64/boot/dts/qcom/talos.dtsi
> index eb6f69be4a82ea47486f5c8e39519d0952b146cb..c946d07c540f5ae40f147ccb06539ecf0de18df5 100644
> --- a/arch/arm64/boot/dts/qcom/talos.dtsi
> +++ b/arch/arm64/boot/dts/qcom/talos.dtsi
> @@ -612,6 +612,11 @@ cluster_pd: power-domain-cluster {
>  					      &cluster_sleep_1
>  					      &cluster_sleep_2>;
>  		};
> +
> +		reboot-mode {
> +			mode-bootloader = <0x10001 0x2>;
> +			mode-edl = <0 0x1>;
> +		};
>  	};

Feel free to add

Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

for Lemans, Monaco, Talos ..

>  
>  	reserved-memory {
> 
> -- 
> 2.34.1
> 

-- 
-Mukesh Ojha

Re: [PATCH v17 01/12] power: reset: reboot-mode: Remove devres based allocations

[Mukesh Ojha]

On Sun, Nov 09, 2025 at 08:07:14PM +0530, Shivendra Pratap wrote:
> Devres APIs are intended for use in drivers, and they should be
> avoided in shared subsystem code which is being used by multiple
> drivers. Avoid using devres based allocations in the reboot-mode
> subsystem and manually free the resources.
> 
> Replace devm_kzalloc with kzalloc and handle memory cleanup
> explicitly.
> 
> Fixes: 4fcd504edbf7 ("power: reset: add reboot mode driver")
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index fba53f638da04655e756b5f8b7d2d666d1379535..ac4223794083f36960b2bd37a601b7e1f1872de5 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -3,6 +3,8 @@
>   * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>   */
>  
> +#define pr_fmt(fmt)	"reboot-mode: " fmt
> +
>  #include <linux/device.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -71,6 +73,7 @@ static int reboot_mode_notify(struct notifier_block *this,
>  int reboot_mode_register(struct reboot_mode_driver *reboot)
>  {
>  	struct mode_info *info;
> +	struct mode_info *next;
>  	struct property *prop;
>  	struct device_node *np = reboot->dev->of_node;
>  	size_t len = strlen(PREFIX);
> @@ -82,29 +85,27 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>  		if (strncmp(prop->name, PREFIX, len))
>  			continue;
>  
> -		info = devm_kzalloc(reboot->dev, sizeof(*info), GFP_KERNEL);
> +		info = kzalloc(sizeof(*info), GFP_KERNEL);
>  		if (!info) {
>  			ret = -ENOMEM;
>  			goto error;
>  		}
>  
>  		if (of_property_read_u32(np, prop->name, &info->magic)) {
> -			dev_err(reboot->dev, "reboot mode %s without magic number\n",
> -				info->mode);
> -			devm_kfree(reboot->dev, info);
> +			pr_err("reboot mode %s without magic number\n", info->mode);
> +			kfree(info);

This as well could be avoided if we move the above memory allocation
after of_property_read_u32()

>  			continue;
>  		}
>  
>  		info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
>  		if (!info->mode) {
>  			ret =  -ENOMEM;
> -			goto error;
> +			goto err_info;
>  		} else if (info->mode[0] == '\0') {
>  			kfree_const(info->mode);
>  			ret = -EINVAL;
> -			dev_err(reboot->dev, "invalid mode name(%s): too short!\n",
> -				prop->name);
> -			goto error;
> +			pr_err("invalid mode name(%s): too short!\n", prop->name);
> +			goto err_info;
>  		}
>  
>  		list_add_tail(&info->list, &reboot->head);
> @@ -115,9 +116,14 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>  
>  	return 0;
>  
> +err_info:

free_info ?

> +	kfree(info);
>  error:
> -	list_for_each_entry(info, &reboot->head, list)
> +	list_for_each_entry_safe(info, next, &reboot->head, list) {
> +		list_del(&info->list);
>  		kfree_const(info->mode);
> +		kfree(info);
> +	}
>  
>  	return ret;
>  }
> @@ -130,11 +136,15 @@ EXPORT_SYMBOL_GPL(reboot_mode_register);
>  int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>  {
>  	struct mode_info *info;
> +	struct mode_info *next;
>  
>  	unregister_reboot_notifier(&reboot->reboot_notifier);
>  
> -	list_for_each_entry(info, &reboot->head, list)
> +	list_for_each_entry_safe(info, next, &reboot->head, list) {
> +		list_del(&info->list);
>  		kfree_const(info->mode);
> +		kfree(info);
> +	}
>  
>  	return 0;
>  }
> 
> -- 
> 2.34.1
> 

-- 
-Mukesh Ojha

Re: [PATCH v17 01/12] power: reset: reboot-mode: Remove devres based allocations

[Bartosz Golaszewski]

On Sun, 9 Nov 2025 at 15:38, Shivendra Pratap
<shivendra.pratap@oss.qualcomm.com> wrote:
>
> Devres APIs are intended for use in drivers, and they should be
> avoided in shared subsystem code which is being used by multiple
> drivers. Avoid using devres based allocations in the reboot-mode
> subsystem and manually free the resources.
>

You're making it sound as if there's some race condition going on.
That's not the reason. They should be avoided in subsystem code
because you have no guarantee that the function will be called after
the driver is attached to the device nor that it will not be
referenced after the managed resources were released after a driver
detach. It's about life-times not synchronization.

Bart

Re: [PATCH v17 02/12] power: reset: reboot-mode: Add firmware node based registration

[Mukesh Ojha]

On Sun, Nov 09, 2025 at 08:07:15PM +0530, Shivendra Pratap wrote:
> The reboot-mode driver does not have a strict requirement for
> device-based registration. It primarily uses the device's of_node
> to read mode-<cmd> properties.
> 
> Remove the dependency on struct device and introduce support for
> firmware node (fwnode) based registration. This enables drivers
> that are not associated with a struct device to leverage the
> reboot-mode framework.
> 
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  drivers/power/reset/reboot-mode.c | 19 ++++++++++++++++---
>  include/linux/reboot-mode.h       |  4 +++-
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index ac4223794083f36960b2bd37a601b7e1f1872de5..eff60d6e04df2cb84ba59d38512654336f272f8a 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -8,10 +8,12 @@
>  #include <linux/device.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>

nit: Looks like stray change and belongs to other patch

>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/reboot.h>
>  #include <linux/reboot-mode.h>
> +#include <linux/slab.h>
>  
>  #define PREFIX "mode-"
>  
> @@ -67,18 +69,26 @@ static int reboot_mode_notify(struct notifier_block *this,
>  /**
>   * reboot_mode_register - register a reboot mode driver
>   * @reboot: reboot mode driver
> + * @fwnode: Firmware node with reboot-mode configuration
>   *
>   * Returns: 0 on success or a negative error code on failure.
>   */
> -int reboot_mode_register(struct reboot_mode_driver *reboot)
> +int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle *fwnode)
>  {
>  	struct mode_info *info;
>  	struct mode_info *next;
> +	struct device_node *np;
>  	struct property *prop;
> -	struct device_node *np = reboot->dev->of_node;
>  	size_t len = strlen(PREFIX);
>  	int ret;
>  
> +	if (!fwnode)
> +		return -EINVAL;
> +
> +	np = to_of_node(fwnode);
> +	if (!np)
> +		return -EINVAL;
> +
>  	INIT_LIST_HEAD(&reboot->head);
>  
>  	for_each_property_of_node(np, prop) {
> @@ -168,11 +178,14 @@ int devm_reboot_mode_register(struct device *dev,
>  	struct reboot_mode_driver **dr;
>  	int rc;
>  
> +	if (!reboot->dev || !reboot->dev->of_node)
> +		return -EINVAL;
> +
>  	dr = devres_alloc(devm_reboot_mode_release, sizeof(*dr), GFP_KERNEL);
>  	if (!dr)
>  		return -ENOMEM;
>  
> -	rc = reboot_mode_register(reboot);
> +	rc = reboot_mode_register(reboot, of_fwnode_handle(reboot->dev->of_node));
>  	if (rc) {
>  		devres_free(dr);
>  		return rc;
> diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
> index 4a2abb38d1d612ec0fdf05eb18c98b210f631b7f..22f707ade4ba93592823ea8718d467dbfc5ffd7c 100644
> --- a/include/linux/reboot-mode.h
> +++ b/include/linux/reboot-mode.h
> @@ -2,6 +2,8 @@
>  #ifndef __REBOOT_MODE_H__
>  #define __REBOOT_MODE_H__
>  
> +#include <linux/fwnode.h>
> +
>  struct reboot_mode_driver {
>  	struct device *dev;
>  	struct list_head head;
> @@ -9,7 +11,7 @@ struct reboot_mode_driver {
>  	struct notifier_block reboot_notifier;
>  };
>  
> -int reboot_mode_register(struct reboot_mode_driver *reboot);
> +int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle *fwnode);
>  int reboot_mode_unregister(struct reboot_mode_driver *reboot);
>  int devm_reboot_mode_register(struct device *dev,
>  			      struct reboot_mode_driver *reboot);
> 
> -- 
> 2.34.1
> 

-- 
-Mukesh Ojha

Re: [PATCH v17 01/12] power: reset: reboot-mode: Remove devres based allocations

[Shivendra Pratap]

On 11/10/2025 6:40 PM, Bartosz Golaszewski wrote:
> You're making it sound as if there's some race condition going on.
> That's not the reason. They should be avoided in subsystem code
> because you have no guarantee that the function will be called after
> the driver is attached to the device nor that it will not be
> referenced after the managed resources were released after a driver
> detach. It's about life-times not synchronization.

sure. Will correct the language and make it more clear in the commit
message.

thanks,
Shivendra

Re: [PATCH v17 01/12] power: reset: reboot-mode: Remove devres based allocations

[Shivendra Pratap]

On 11/10/2025 6:31 PM, Mukesh Ojha wrote:
> On Sun, Nov 09, 2025 at 08:07:14PM +0530, Shivendra Pratap wrote:
>> Devres APIs are intended for use in drivers, and they should be
>> avoided in shared subsystem code which is being used by multiple
>> drivers. Avoid using devres based allocations in the reboot-mode
>> subsystem and manually free the resources.
>>
>> Replace devm_kzalloc with kzalloc and handle memory cleanup
>> explicitly.
>>
>> Fixes: 4fcd504edbf7 ("power: reset: add reboot mode driver")
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
>>  drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++----------
>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
>> index fba53f638da04655e756b5f8b7d2d666d1379535..ac4223794083f36960b2bd37a601b7e1f1872de5 100644
>> --- a/drivers/power/reset/reboot-mode.c
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -3,6 +3,8 @@
>>   * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>>   */
>>  
>> +#define pr_fmt(fmt)	"reboot-mode: " fmt
>> +
>>  #include <linux/device.h>
>>  #include <linux/init.h>
>>  #include <linux/kernel.h>
>> @@ -71,6 +73,7 @@ static int reboot_mode_notify(struct notifier_block *this,
>>  int reboot_mode_register(struct reboot_mode_driver *reboot)
>>  {
>>  	struct mode_info *info;
>> +	struct mode_info *next;
>>  	struct property *prop;
>>  	struct device_node *np = reboot->dev->of_node;
>>  	size_t len = strlen(PREFIX);
>> @@ -82,29 +85,27 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>  		if (strncmp(prop->name, PREFIX, len))
>>  			continue;
>>  
>> -		info = devm_kzalloc(reboot->dev, sizeof(*info), GFP_KERNEL);
>> +		info = kzalloc(sizeof(*info), GFP_KERNEL);
>>  		if (!info) {
>>  			ret = -ENOMEM;
>>  			goto error;
>>  		}
>>  
>>  		if (of_property_read_u32(np, prop->name, &info->magic)) {
>> -			dev_err(reboot->dev, "reboot mode %s without magic number\n",
>> -				info->mode);
>> -			devm_kfree(reboot->dev, info);
>> +			pr_err("reboot mode %s without magic number\n", info->mode);
>> +			kfree(info);
> 
> This as well could be avoided if we move the above memory allocation
> after of_property_read_u32()

ok. Will re-order the code to avoid the kfree(info) here.

> 
>>  			continue;
>>  		}
>>  
>>  		info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
>>  		if (!info->mode) {
>>  			ret =  -ENOMEM;
>> -			goto error;
>> +			goto err_info;
>>  		} else if (info->mode[0] == '\0') {
>>  			kfree_const(info->mode);
>>  			ret = -EINVAL;
>> -			dev_err(reboot->dev, "invalid mode name(%s): too short!\n",
>> -				prop->name);
>> -			goto error;
>> +			pr_err("invalid mode name(%s): too short!\n", prop->name);
>> +			goto err_info;
>>  		}
>>  
>>  		list_add_tail(&info->list, &reboot->head);
>> @@ -115,9 +116,14 @@ int reboot_mode_register(struct reboot_mode_driver *reboot)
>>  
>>  	return 0;
>>  
>> +err_info:
> 
> free_info ?

Ack.

thanks,
Shivendra

Re: [PATCH v17 02/12] power: reset: reboot-mode: Add firmware node based registration

[Shivendra Pratap]

On 11/10/2025 6:43 PM, Mukesh Ojha wrote:
> On Sun, Nov 09, 2025 at 08:07:15PM +0530, Shivendra Pratap wrote:
>> The reboot-mode driver does not have a strict requirement for
>> device-based registration. It primarily uses the device's of_node
>> to read mode-<cmd> properties.
>>
>> Remove the dependency on struct device and introduce support for
>> firmware node (fwnode) based registration. This enables drivers
>> that are not associated with a struct device to leverage the
>> reboot-mode framework.
>>
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
>>  drivers/power/reset/reboot-mode.c | 19 ++++++++++++++++---
>>  include/linux/reboot-mode.h       |  4 +++-
>>  2 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
>> index ac4223794083f36960b2bd37a601b7e1f1872de5..eff60d6e04df2cb84ba59d38512654336f272f8a 100644
>> --- a/drivers/power/reset/reboot-mode.c
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -8,10 +8,12 @@
>>  #include <linux/device.h>
>>  #include <linux/init.h>
>>  #include <linux/kernel.h>
>> +#include <linux/list.h>
> 
> nit: Looks like stray change and belongs to other patch

Ack.

thanks,
Shivendra

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Mukesh Ojha]

On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
> Current reboot-mode supports a single 32-bit argument for any
> supported mode. Some reboot-mode based drivers may require
> passing two independent 32-bit arguments during a reboot
> sequence, for uses-cases, where a mode requires an additional
> argument. Such drivers may not be able to use the reboot-mode
> driver. For example, ARM PSCI vendor-specific resets, need two
> arguments for its operation – reset_type and cookie, to complete
> the reset operation. If a driver wants to implement this
> firmware-based reset, it cannot use reboot-mode framework.
> 
> Introduce 64-bit magic values in reboot-mode driver to
> accommodate dual 32-bit arguments when specified via device tree.
> In cases, where no second argument is passed from device tree,
> keep the upper 32-bit of magic un-changed(0) to maintain backward
> compatibility.
> 
> Update the current drivers using reboot-mode for a 64-bit magic
> value.
> 
> Reviewed-by: Umang Chheda <umang.chheda@oss.qualcomm.com>
> Reviewed-by: Nirmesh Kumar Singh <nirmesh.singh@oss.qualcomm.com>
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  drivers/power/reset/nvmem-reboot-mode.c  | 13 +++++++++----
>  drivers/power/reset/qcom-pon.c           | 11 ++++++++---
>  drivers/power/reset/reboot-mode.c        | 19 +++++++++++++------
>  drivers/power/reset/syscon-reboot-mode.c | 11 ++++++++---
>  include/linux/reboot-mode.h              |  3 ++-
>  5 files changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/power/reset/nvmem-reboot-mode.c b/drivers/power/reset/nvmem-reboot-mode.c
> index 41530b70cfc48c2a83fbbd96f523d5816960a0d1..5d73dde585b1fd438b1847f884feb37cd9e4dd5c 100644
> --- a/drivers/power/reset/nvmem-reboot-mode.c
> +++ b/drivers/power/reset/nvmem-reboot-mode.c
> @@ -16,15 +16,20 @@ struct nvmem_reboot_mode {
>  	struct nvmem_cell *cell;
>  };
>  
> -static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
> -				    unsigned int magic)
> +static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
>  {
> -	int ret;
>  	struct nvmem_reboot_mode *nvmem_rbm;
> +	u32 magic_32;
> +	int ret;
> +
> +	if (magic > U32_MAX)
> +		return -EINVAL;


I believe, we need a comment in all the client driver.. 

> +
> +	magic_32 = magic;
>  
>  	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
>  
> -	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
> +	ret = nvmem_cell_write(nvmem_rbm->cell, &magic_32, sizeof(magic_32));
>  	if (ret < 0)
>  		dev_err(reboot->dev, "update reboot mode bits failed\n");
>  
> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
> index 7e108982a582e8243c5c806bd4a793646b87189f..d0ed9431a02313a7bbaa93743c16fa1ae713ddfe 100644
> --- a/drivers/power/reset/qcom-pon.c
> +++ b/drivers/power/reset/qcom-pon.c
> @@ -27,17 +27,22 @@ struct qcom_pon {
>  	long reason_shift;
>  };
>  
> -static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
> -				    unsigned int magic)
> +static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
>  {
>  	struct qcom_pon *pon = container_of
>  			(reboot, struct qcom_pon, reboot_mode);
> +	u32 magic_32;
>  	int ret;
> 

Since we are doing this change in reboot framework, client driver should know about
it too about this new check because of framework.

> +	if (magic > U32_MAX || (magic << pon->reason_shift) > U32_MAX)


is this (magic << pon->reason_shift) > U32_MAX really needed ..?

> +		return -EINVAL;
> +
> +	magic_32 = magic << pon->reason_shift;
> +
>  	ret = regmap_update_bits(pon->regmap,
>  				 pon->baseaddr + PON_SOFT_RB_SPARE,
>  				 GENMASK(7, pon->reason_shift),
> -				 magic << pon->reason_shift);
> +				 magic_32);
>  	if (ret < 0)
>  		dev_err(pon->dev, "update reboot mode bits failed\n");
>  
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index eff60d6e04df2cb84ba59d38512654336f272f8a..873ac45cd7659b214b7c21958f580ca381e0a63d 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -19,12 +19,11 @@
>  
>  struct mode_info {
>  	const char *mode;
> -	u32 magic;
> +	u64 magic;
>  	struct list_head list;
>  };
>  
> -static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> -					  const char *cmd)
> +static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
>  {
>  	const char *normal = "normal";
>  	struct mode_info *info;
> @@ -56,7 +55,7 @@ static int reboot_mode_notify(struct notifier_block *this,
>  			      unsigned long mode, void *cmd)
>  {
>  	struct reboot_mode_driver *reboot;
> -	unsigned int magic;
> +	u64 magic;
>  
>  	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
>  	magic = get_reboot_mode_magic(reboot, cmd);
> @@ -80,6 +79,8 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>  	struct device_node *np;
>  	struct property *prop;
>  	size_t len = strlen(PREFIX);
> +	u32 magic_arg1;
> +	u32 magic_arg2;
>  	int ret;
>  
>  	if (!fwnode)
> @@ -101,12 +102,18 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>  			goto error;
>  		}
>  
> -		if (of_property_read_u32(np, prop->name, &info->magic)) {
> -			pr_err("reboot mode %s without magic number\n", info->mode);
> +		if (of_property_read_u32(np, prop->name, &magic_arg1)) {
> +			pr_err("reboot mode without magic number\n");
>  			kfree(info);
>  			continue;
>  		}
>  
> +		if (of_property_read_u32_index(np, prop->name, 1, &magic_arg2))
> +			magic_arg2 = 0;
> +
> +		info->magic = magic_arg2;
> +		info->magic = (info->magic << 32) | magic_arg1;
> +
>  		info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
>  		if (!info->mode) {
>  			ret =  -ENOMEM;
> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
> index e0772c9f70f7a19cd8ec8a0b7fdbbaa7ba44afd0..3cbd000c512239b12ec51987e900d260540a9dea 100644
> --- a/drivers/power/reset/syscon-reboot-mode.c
> +++ b/drivers/power/reset/syscon-reboot-mode.c
> @@ -20,16 +20,21 @@ struct syscon_reboot_mode {
>  	u32 mask;
>  };
>  
> -static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot,
> -				    unsigned int magic)
> +static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
>  {
>  	struct syscon_reboot_mode *syscon_rbm;
> +	u32 magic_32;
>  	int ret;
>

same here

> +	if (magic > U32_MAX)
> +		return -EINVAL;
> +
> +	magic_32 = magic;
> +
>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
>  
>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
> -				 syscon_rbm->mask, magic);
> +				 syscon_rbm->mask, magic_32);
>  	if (ret < 0)
>  		dev_err(reboot->dev, "update reboot mode bits failed\n");
>  
> diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
> index 22f707ade4ba93592823ea8718d467dbfc5ffd7c..e0d3e8a54050a76f26846f456120b4c7e371d284 100644
> --- a/include/linux/reboot-mode.h
> +++ b/include/linux/reboot-mode.h
> @@ -3,11 +3,12 @@
>  #define __REBOOT_MODE_H__
>  
>  #include <linux/fwnode.h>
> +#include <linux/types.h>
>  
>  struct reboot_mode_driver {
>  	struct device *dev;
>  	struct list_head head;
> -	int (*write)(struct reboot_mode_driver *reboot, unsigned int magic);
> +	int (*write)(struct reboot_mode_driver *reboot, u64 magic);
>  	struct notifier_block reboot_notifier;
>  };
>  
> 
> -- 
> 2.34.1
> 

-- 
-Mukesh Ojha

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Shivendra Pratap]

On 11/10/2025 7:15 PM, Mukesh Ojha wrote:
> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
>> Current reboot-mode supports a single 32-bit argument for any

[SNIP..]

>>  
>> -static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
>> -				    unsigned int magic)
>> +static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
>>  {
>> -	int ret;
>>  	struct nvmem_reboot_mode *nvmem_rbm;
>> +	u32 magic_32;
>> +	int ret;
>> +
>> +	if (magic > U32_MAX)
>> +		return -EINVAL;
> 
> 
> I believe, we need a comment in all the client driver.. 

Ack. Will add a comment.

> 
>> +
>> +	magic_32 = magic;
>>  
>>  	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
>>  
>> -	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
>> +	ret = nvmem_cell_write(nvmem_rbm->cell, &magic_32, sizeof(magic_32));
>>  	if (ret < 0)
>>  		dev_err(reboot->dev, "update reboot mode bits failed\n");
>>  
>> diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
>> index 7e108982a582e8243c5c806bd4a793646b87189f..d0ed9431a02313a7bbaa93743c16fa1ae713ddfe 100644
>> --- a/drivers/power/reset/qcom-pon.c
>> +++ b/drivers/power/reset/qcom-pon.c
>> @@ -27,17 +27,22 @@ struct qcom_pon {
>>  	long reason_shift;
>>  };
>>  
>> -static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
>> -				    unsigned int magic)
>> +static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
>>  {
>>  	struct qcom_pon *pon = container_of
>>  			(reboot, struct qcom_pon, reboot_mode);
>> +	u32 magic_32;
>>  	int ret;
>>
> 
> Since we are doing this change in reboot framework, client driver should know about
> it too about this new check because of framework.

ok.

> 
>> +	if (magic > U32_MAX || (magic << pon->reason_shift) > U32_MAX)
> 
> 
> is this (magic << pon->reason_shift) > U32_MAX really needed ..?

Can be omitted as we already checked magic > U32_MAX. Will
remove it.

> 

[SNIP..]

>> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
>> index e0772c9f70f7a19cd8ec8a0b7fdbbaa7ba44afd0..3cbd000c512239b12ec51987e900d260540a9dea 100644
>> --- a/drivers/power/reset/syscon-reboot-mode.c
>> +++ b/drivers/power/reset/syscon-reboot-mode.c
>> @@ -20,16 +20,21 @@ struct syscon_reboot_mode {
>>  	u32 mask;
>>  };
>>  
>> -static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot,
>> -				    unsigned int magic)
>> +static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
>>  {
>>  	struct syscon_reboot_mode *syscon_rbm;
>> +	u32 magic_32;
>>  	int ret;
>>
> 
> same here

will add comment here.

thanks,
Shivendra

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Shivendra Pratap]

On 11/10/2025 10:10 AM, Kathiravan Thirumoorthy wrote:
> 
> On 11/9/2025 8:07 PM, Shivendra Pratap wrote:
>> +static int __init psci_init_vendor_reset(void)
>> +{
>> +    struct reboot_mode_driver *reboot;
>> +    struct device_node *psci_np;
>> +    struct device_node *np;
> 
> 
> We can take advantage of __cleanup() attribute to simply the code paths.
> 
> Declare the variables like below
> 
>     struct device_node *psci_np __free(device_node) = NULL;
>     struct device_node *np __free(device_node) = NULL;
> 
> and get rid of the explicit of_node_put().
> 
> I think, we can take up this an improvement once this series landed. But if you happen to respin to address other issues, please take care of this as well.

Let me evaluate this for next respin.

thanks,
Shivendra

Re: [PATCH v17 05/12] power: reset: reboot-mode: Expose sysfs for registered reboot_modes

[Bartosz Golaszewski]

On Sun, 9 Nov 2025 at 15:38, Shivendra Pratap
<shivendra.pratap@oss.qualcomm.com> wrote:
>
> Currently, there is no standardized mechanism for userspace to
> discover which reboot-modes are supported on a given platform.
> This limitation forces tools and scripts to rely on hardcoded
> assumptions about the supported reboot-modes.
>
> Create a class 'reboot-mode' and a device under it to expose a
> sysfs interface to show the available reboot mode arguments to
> userspace. Use the driver_name field of the struct
> reboot_mode_driver to create the device. For device-based
> drivers, configure the device driver name as driver_name.
>
> This results in the creation of:
>   /sys/class/reboot-mode/<driver>/reboot_modes
>
> This read-only sysfs file will exposes the list of supported
> reboot modes arguments provided by the driver, enabling userspace
> to query the list of arguments.
>
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  drivers/power/reset/reboot-mode.c | 62 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/reboot-mode.h       |  2 ++
>  2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index 873ac45cd7659b214b7c21958f580ca381e0a63d..582aa7f8ed7fa485c5a67877558c9b15d3600ef4 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -6,6 +6,7 @@
>  #define pr_fmt(fmt)    "reboot-mode: " fmt
>
>  #include <linux/device.h>
> +#include <linux/err.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -23,6 +24,8 @@ struct mode_info {
>         struct list_head list;
>  };
>
> +static struct class *rb_class;
> +

I know C is a spartan language but the rb_ prefix makes me think of
the red-black tree. Please call it reboot_mode_class.

>  static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
>  {
>         const char *normal = "normal";
> @@ -65,6 +68,51 @@ static int reboot_mode_notify(struct notifier_block *this,
>         return NOTIFY_DONE;
>  }
>
> +static ssize_t reboot_modes_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +       struct reboot_mode_driver *reboot;
> +       struct mode_info *info;
> +       ssize_t size = 0;
> +
> +       reboot = (struct reboot_mode_driver *)dev_get_drvdata(dev);

No need for the cast.

> +       if (!reboot)
> +               return -ENODATA;
> +
> +       list_for_each_entry(info, &reboot->head, list)
> +               size += sysfs_emit_at(buf, size, "%s ", info->mode);
> +
> +       if (size) {
> +               size += sysfs_emit_at(buf, size - 1, "\n");
> +               return size;
> +       }

This is a weird logic inversion. Just do:

if (!size)
    return -ENODATA;

return size + sysfs_emit_at(buf, size - 1, "\n");

> +
> +       return -ENODATA;
> +}
> +static DEVICE_ATTR_RO(reboot_modes);
> +
> +static int create_reboot_mode_device(struct reboot_mode_driver *reboot)
> +{
> +       int ret = 0;
> +
> +       if (!rb_class) {
> +               rb_class = class_create("reboot-mode");
> +               if (IS_ERR(rb_class))
> +                       return PTR_ERR(rb_class);
> +       }

Why the lazy initialization here? Is there any reason you can't
statically define the class? Don't you need synchronization here if
multiple drivers try to do this?

Bart

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Bjorn Andersson]

On Mon, Nov 10, 2025 at 05:58:24PM +0530, Mukesh Ojha wrote:
> On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
> > From: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > 
> > Add support for SYSTEM_RESET2 vendor-specific resets in
> > qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
> > "bootloader" will cause device to reboot and stop in the
> > bootloader's fastboot mode. "edl" will cause device to reboot
> > into "emergency download mode", which permits loading images via
> > the Firehose protocol.
> > 
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> > ---
> >  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > @@ -935,6 +935,13 @@ &pon_resin {
> >  	status = "okay";
> >  };
> >  
> > +&psci {
> > +	reboot-mode {
> > +		mode-bootloader = <0x10001 0x2>;
> > +		mode-edl = <0 0x1>;
> > +	};
> > +};
> > +
> 
> Make sense for this as it leverages sc7280 and adding it there would not
> have made sense.
> 

Why wouldn't it make sense?

> Acked-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> 

Please read submitting-patches.rst about Acked-by, and use Reviewed-by
going forward.

Regards,
Bjorn

> >  &qup_uart7_cts {
> >  	/*
> >  	 * Configure a bias-bus-hold on CTS to lower power
> > 
> > -- 
> > 2.34.1
> > 
> 
> -- 
> -Mukesh Ojha
> 

Re: [PATCH v17 05/12] power: reset: reboot-mode: Expose sysfs for registered reboot_modes

[Bjorn Andersson]

On Sun, Nov 09, 2025 at 08:07:18PM +0530, Shivendra Pratap wrote:
> Currently, there is no standardized mechanism for userspace to
> discover which reboot-modes are supported on a given platform.
> This limitation forces tools and scripts to rely on hardcoded
> assumptions about the supported reboot-modes.
> 
> Create a class 'reboot-mode' and a device under it to expose a
> sysfs interface to show the available reboot mode arguments to
> userspace. Use the driver_name field of the struct
> reboot_mode_driver to create the device. For device-based
> drivers, configure the device driver name as driver_name.
> 
> This results in the creation of:
>   /sys/class/reboot-mode/<driver>/reboot_modes
> 
> This read-only sysfs file will exposes the list of supported
> reboot modes arguments provided by the driver, enabling userspace
> to query the list of arguments.
> 

I like this addition, and your commit message reasoning about this
addition. But, while touching upon the same subject, you've made this
series add two separate things.

So now this part can't be merged unless there's agreement on the PSCI
SYSTEM_RESET2, and the PSCI SYSTEM_RESET2 can't be merged unless this
sysfs interface is agreed upon.

Unless I'm missing some clear dependency here, it would have been better
to keep these two topics in separate series, and drive them to
conclusion independently.

> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  drivers/power/reset/reboot-mode.c | 62 ++++++++++++++++++++++++++++++++++++++-
>  include/linux/reboot-mode.h       |  2 ++
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index 873ac45cd7659b214b7c21958f580ca381e0a63d..582aa7f8ed7fa485c5a67877558c9b15d3600ef4 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -6,6 +6,7 @@
>  #define pr_fmt(fmt)	"reboot-mode: " fmt
>  
>  #include <linux/device.h>
> +#include <linux/err.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -23,6 +24,8 @@ struct mode_info {
>  	struct list_head list;
>  };
>  
> +static struct class *rb_class;

Why not "static const struct class reboot_mode_class" and then a
class_register() call? Why do you need the class dynamically allocated
on the heap?

> +
>  static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
>  {
>  	const char *normal = "normal";
> @@ -65,6 +68,51 @@ static int reboot_mode_notify(struct notifier_block *this,
>  	return NOTIFY_DONE;
>  }
>  
> +static ssize_t reboot_modes_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct reboot_mode_driver *reboot;
> +	struct mode_info *info;
> +	ssize_t size = 0;
> +
> +	reboot = (struct reboot_mode_driver *)dev_get_drvdata(dev);
> +	if (!reboot)
> +		return -ENODATA;
> +
> +	list_for_each_entry(info, &reboot->head, list)
> +		size += sysfs_emit_at(buf, size, "%s ", info->mode);
> +
> +	if (size) {
> +		size += sysfs_emit_at(buf, size - 1, "\n");
> +		return size;
> +	}
> +
> +	return -ENODATA;
> +}
> +static DEVICE_ATTR_RO(reboot_modes);
> +
> +static int create_reboot_mode_device(struct reboot_mode_driver *reboot)

Note how (almost) all other function names in this file start with
a "reboot_mode_" prefix.

> +{
> +	int ret = 0;

First use is an assignment, no need for you to zero-initialize it here.

> +
> +	if (!rb_class) {
> +		rb_class = class_create("reboot-mode");
> +		if (IS_ERR(rb_class))
> +			return PTR_ERR(rb_class);
> +	}
> +
> +	reboot->reboot_dev = device_create(rb_class, NULL, 0, (void *)reboot, reboot->driver_name);

Every struct reboot_mode_driver is going to end up having one of these,
so why not incorporate it into the reboot_mode_driver in the first
place. It avoids the extra heap allocation, and you can use
container_of() instead of drv_data to find your reboot_mode_driver in
the reboot_modes_show() above.


Just:
  reboot->reboot_dev.class = &reboot_mode_class;
  dev_set_name(&reboot->reboot_dev, reboot->driver_name);
  ret = device_register(&reboot->reboot_dev);

> +	if (IS_ERR(reboot->reboot_dev))
> +		return PTR_ERR(reboot->reboot_dev);
> +
> +	ret = device_create_file(reboot->reboot_dev, &dev_attr_reboot_modes);

Manually creating sysfs attributes is both error prone and racy, so if
you can you should avoid it.

Here you have the opportunity to just statically assign
reboot_mode_class->dev_groups to an ATTRIBUTE_GROUP() with your
attribute and it will all be handled for you.

> +	if (ret) {
> +		device_unregister(reboot->reboot_dev);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * reboot_mode_register - register a reboot mode driver
>   * @reboot: reboot mode driver
> @@ -83,13 +131,17 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>  	u32 magic_arg2;
>  	int ret;
>  
> -	if (!fwnode)
> +	if (!fwnode || !reboot->driver_name)
>  		return -EINVAL;
>  
>  	np = to_of_node(fwnode);
>  	if (!np)
>  		return -EINVAL;
>  
> +	ret = create_reboot_mode_device(reboot);
> +	if (ret)
> +		return ret;
> +
>  	INIT_LIST_HEAD(&reboot->head);
>  
>  	for_each_property_of_node(np, prop) {
> @@ -142,6 +194,8 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>  		kfree(info);
>  	}
>  
> +	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
> +	device_unregister(reboot->reboot_dev);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(reboot_mode_register);
> @@ -155,6 +209,9 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>  	struct mode_info *info;
>  	struct mode_info *next;
>  
> +	if (!reboot->reboot_dev)
> +		return -EINVAL;
> +
>  	unregister_reboot_notifier(&reboot->reboot_notifier);
>  
>  	list_for_each_entry_safe(info, next, &reboot->head, list) {
> @@ -163,6 +220,8 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>  		kfree(info);
>  	}
>  
> +	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
> +	device_unregister(reboot->reboot_dev);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(reboot_mode_unregister);
> @@ -192,6 +251,7 @@ int devm_reboot_mode_register(struct device *dev,
>  	if (!dr)
>  		return -ENOMEM;
>  
> +	reboot->driver_name = reboot->dev->driver->name;

It seems unlikely that we will have multiple instances of the same
driver influencing the actual reboot mode, but we could very well have
multiple instances of the same driver calling
devm_reboot_mode_register(). E.g. on a board two PMICs, both with PON
blocks (but only one considered as the source for boot mode).

In that case you will end up trying to create multiple devices with the
name "qcom-pon", presumably that will fail and per your error handling
you have now disabled the reboot-mechanism for all but the first pon
instance that was registered.

It also creates some asymmetry between devm_reboot_mode_register() and
reboot_mode_register(), in that the one API the client driver decides
the name, in other it's hard coded to the driver name (and if the client
did specify a name - which they should if they use the non-devm one- it
will be overwritten).



On that note, I would argue that aborting the registration of
reboot-modes, just because we failed to create the convenient "debug"
interface, doesn't make sense. I think it would be better to just
continue even when create_reboot_mode_device() returns an error.

>  	rc = reboot_mode_register(reboot, of_fwnode_handle(reboot->dev->of_node));
>  	if (rc) {
>  		devres_free(dr);
> diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
> index e0d3e8a54050a76f26846f456120b4c7e371d284..81c149edf40fbcf0d3427c2e12eb415199cb153b 100644
> --- a/include/linux/reboot-mode.h
> +++ b/include/linux/reboot-mode.h
> @@ -7,6 +7,8 @@
>  
>  struct reboot_mode_driver {
>  	struct device *dev;
> +	struct device *reboot_dev;

As suggested above:

struct device reboot_dev;

Regards,
Bjorn

> +	const char *driver_name;
>  	struct list_head head;
>  	int (*write)(struct reboot_mode_driver *reboot, u64 magic);
>  	struct notifier_block reboot_notifier;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Mukesh Ojha]

On Mon, Nov 10, 2025 at 09:30:26AM -0600, Bjorn Andersson wrote:
> On Mon, Nov 10, 2025 at 05:58:24PM +0530, Mukesh Ojha wrote:
> > On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
> > > From: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > > 
> > > Add support for SYSTEM_RESET2 vendor-specific resets in
> > > qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
> > > "bootloader" will cause device to reboot and stop in the
> > > bootloader's fastboot mode. "edl" will cause device to reboot
> > > into "emergency download mode", which permits loading images via
> > > the Firehose protocol.
> > > 
> > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > > Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> > > ---
> > >  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > @@ -935,6 +935,13 @@ &pon_resin {
> > >  	status = "okay";
> > >  };
> > >  
> > > +&psci {
> > > +	reboot-mode {
> > > +		mode-bootloader = <0x10001 0x2>;
> > > +		mode-edl = <0 0x1>;
> > > +	};
> > > +};
> > > +
> > 
> > Make sense for this as it leverages sc7280 and adding it there would not
> > have made sense.
> > 
> 
> Why wouldn't it make sense?

It is better to add for platforms we know their firmware support this
from day1 and not add for something like chrome or any other variant of
sc7280 where this support would never come or not tested.
> 
> > Acked-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > 
> 
> Please read submitting-patches.rst about Acked-by, and use Reviewed-by
> going forward.

I was acking the idea of this particular change in platform file compared to
other patches, if above reason looks fine, can be converted to R-by.

> 
> Regards,
> Bjorn
> 
> > >  &qup_uart7_cts {
> > >  	/*
> > >  	 * Configure a bias-bus-hold on CTS to lower power
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > -Mukesh Ojha
> > 

-- 
-Mukesh Ojha

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Bjorn Andersson]

On Mon, Nov 10, 2025 at 07:15:29PM +0530, Mukesh Ojha wrote:
> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
> > Current reboot-mode supports a single 32-bit argument for any
> > supported mode. Some reboot-mode based drivers may require
> > passing two independent 32-bit arguments during a reboot
> > sequence, for uses-cases, where a mode requires an additional
> > argument. Such drivers may not be able to use the reboot-mode
> > driver. For example, ARM PSCI vendor-specific resets, need two
> > arguments for its operation – reset_type and cookie, to complete
> > the reset operation. If a driver wants to implement this
> > firmware-based reset, it cannot use reboot-mode framework.
> > 
> > Introduce 64-bit magic values in reboot-mode driver to
> > accommodate dual 32-bit arguments when specified via device tree.
> > In cases, where no second argument is passed from device tree,
> > keep the upper 32-bit of magic un-changed(0) to maintain backward
> > compatibility.
> > 
> > Update the current drivers using reboot-mode for a 64-bit magic
> > value.
> > 
> > Reviewed-by: Umang Chheda <umang.chheda@oss.qualcomm.com>
> > Reviewed-by: Nirmesh Kumar Singh <nirmesh.singh@oss.qualcomm.com>
> > Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> > ---
> >  drivers/power/reset/nvmem-reboot-mode.c  | 13 +++++++++----
> >  drivers/power/reset/qcom-pon.c           | 11 ++++++++---
> >  drivers/power/reset/reboot-mode.c        | 19 +++++++++++++------
> >  drivers/power/reset/syscon-reboot-mode.c | 11 ++++++++---
> >  include/linux/reboot-mode.h              |  3 ++-
> >  5 files changed, 40 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/power/reset/nvmem-reboot-mode.c b/drivers/power/reset/nvmem-reboot-mode.c
> > index 41530b70cfc48c2a83fbbd96f523d5816960a0d1..5d73dde585b1fd438b1847f884feb37cd9e4dd5c 100644
> > --- a/drivers/power/reset/nvmem-reboot-mode.c
> > +++ b/drivers/power/reset/nvmem-reboot-mode.c
> > @@ -16,15 +16,20 @@ struct nvmem_reboot_mode {
> >  	struct nvmem_cell *cell;
> >  };
> >  
> > -static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
> > -				    unsigned int magic)
> > +static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
> >  {
> > -	int ret;
> >  	struct nvmem_reboot_mode *nvmem_rbm;
> > +	u32 magic_32;
> > +	int ret;
> > +
> > +	if (magic > U32_MAX)
> > +		return -EINVAL;
> 
> 
> I believe, we need a comment in all the client driver.. 
> 
> > +
> > +	magic_32 = magic;
> >  
> >  	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
> >  
> > -	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
> > +	ret = nvmem_cell_write(nvmem_rbm->cell, &magic_32, sizeof(magic_32));
> >  	if (ret < 0)
> >  		dev_err(reboot->dev, "update reboot mode bits failed\n");
> >  
> > diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
> > index 7e108982a582e8243c5c806bd4a793646b87189f..d0ed9431a02313a7bbaa93743c16fa1ae713ddfe 100644
> > --- a/drivers/power/reset/qcom-pon.c
> > +++ b/drivers/power/reset/qcom-pon.c
> > @@ -27,17 +27,22 @@ struct qcom_pon {
> >  	long reason_shift;
> >  };
> >  
> > -static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
> > -				    unsigned int magic)
> > +static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
> >  {
> >  	struct qcom_pon *pon = container_of
> >  			(reboot, struct qcom_pon, reboot_mode);
> > +	u32 magic_32;
> >  	int ret;
> > 
> 
> Since we are doing this change in reboot framework, client driver should know about
> it too about this new check because of framework.
> 
> > +	if (magic > U32_MAX || (magic << pon->reason_shift) > U32_MAX)
> 
> 
> is this (magic << pon->reason_shift) > U32_MAX really needed ..?
> 

In my eyes the introduction of a magic_32 variable seems a bit
copy-paste, and this check does tell a similar story...

We have two possible bit patterns:
  GENMASK(7, 2)
  GENMASK(7, 1)

So this means that we have either 5 or 6 bits of magic, not 32.

So far we've just trusted that the mode provided in DeviceTree would
fit, and we've silently discarded the other bits.

But if we're introducing checks here, we should do it properly, if
nothing else for the sake of making the code self-documented.


There's also no need for a new local variable for the magic, check that
we have a valid range and then just typecast it into the argument.

> > +		return -EINVAL;
> > +
> > +	magic_32 = magic << pon->reason_shift;
> > +
> >  	ret = regmap_update_bits(pon->regmap,
> >  				 pon->baseaddr + PON_SOFT_RB_SPARE,
> >  				 GENMASK(7, pon->reason_shift),
> > -				 magic << pon->reason_shift);
> > +				 magic_32);
> >  	if (ret < 0)
> >  		dev_err(pon->dev, "update reboot mode bits failed\n");
> >  
> > diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> > index eff60d6e04df2cb84ba59d38512654336f272f8a..873ac45cd7659b214b7c21958f580ca381e0a63d 100644
> > --- a/drivers/power/reset/reboot-mode.c
> > +++ b/drivers/power/reset/reboot-mode.c
> > @@ -19,12 +19,11 @@
> >  
> >  struct mode_info {
> >  	const char *mode;
> > -	u32 magic;
> > +	u64 magic;
> >  	struct list_head list;
> >  };
> >  
> > -static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> > -					  const char *cmd)
> > +static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
> >  {
> >  	const char *normal = "normal";
> >  	struct mode_info *info;
> > @@ -56,7 +55,7 @@ static int reboot_mode_notify(struct notifier_block *this,
> >  			      unsigned long mode, void *cmd)
> >  {
> >  	struct reboot_mode_driver *reboot;
> > -	unsigned int magic;
> > +	u64 magic;
> >  
> >  	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
> >  	magic = get_reboot_mode_magic(reboot, cmd);
> > @@ -80,6 +79,8 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
> >  	struct device_node *np;
> >  	struct property *prop;
> >  	size_t len = strlen(PREFIX);
> > +	u32 magic_arg1;
> > +	u32 magic_arg2;
> >  	int ret;
> >  
> >  	if (!fwnode)
> > @@ -101,12 +102,18 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
> >  			goto error;
> >  		}
> >  
> > -		if (of_property_read_u32(np, prop->name, &info->magic)) {
> > -			pr_err("reboot mode %s without magic number\n", info->mode);
> > +		if (of_property_read_u32(np, prop->name, &magic_arg1)) {
> > +			pr_err("reboot mode without magic number\n");
> >  			kfree(info);
> >  			continue;
> >  		}
> >  
> > +		if (of_property_read_u32_index(np, prop->name, 1, &magic_arg2))
> > +			magic_arg2 = 0;
> > +
> > +		info->magic = magic_arg2;
> > +		info->magic = (info->magic << 32) | magic_arg1;

There's no reason to assign the value and then reassign it, it just
makes the code harder to read.

If you mean "info->magic = (magic_arg2 << 32) | magic_arg1" then write
that...

> > +
> >  		info->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
> >  		if (!info->mode) {
> >  			ret =  -ENOMEM;
> > diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
> > index e0772c9f70f7a19cd8ec8a0b7fdbbaa7ba44afd0..3cbd000c512239b12ec51987e900d260540a9dea 100644
> > --- a/drivers/power/reset/syscon-reboot-mode.c
> > +++ b/drivers/power/reset/syscon-reboot-mode.c
> > @@ -20,16 +20,21 @@ struct syscon_reboot_mode {
> >  	u32 mask;
> >  };
> >  
> > -static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot,
> > -				    unsigned int magic)
> > +static int syscon_reboot_mode_write(struct reboot_mode_driver *reboot, u64 magic)
> >  {
> >  	struct syscon_reboot_mode *syscon_rbm;
> > +	u32 magic_32;
> >  	int ret;
> >
> 
> same here
> 
> > +	if (magic > U32_MAX)
> > +		return -EINVAL;
> > +
> > +	magic_32 = magic;
> > +
> >  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
> >  
> >  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
> > -				 syscon_rbm->mask, magic);
> > +				 syscon_rbm->mask, magic_32);

As above, if we're no longer silently discarding bits, I think we should
compare the magic against syscon_rbm->mask.

No need for a local variable, just type cast after checking the validity.

Regards,
Bjorn

> >  	if (ret < 0)
> >  		dev_err(reboot->dev, "update reboot mode bits failed\n");
> >  
> > diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
> > index 22f707ade4ba93592823ea8718d467dbfc5ffd7c..e0d3e8a54050a76f26846f456120b4c7e371d284 100644
> > --- a/include/linux/reboot-mode.h
> > +++ b/include/linux/reboot-mode.h
> > @@ -3,11 +3,12 @@
> >  #define __REBOOT_MODE_H__
> >  
> >  #include <linux/fwnode.h>
> > +#include <linux/types.h>
> >  
> >  struct reboot_mode_driver {
> >  	struct device *dev;
> >  	struct list_head head;
> > -	int (*write)(struct reboot_mode_driver *reboot, unsigned int magic);
> > +	int (*write)(struct reboot_mode_driver *reboot, u64 magic);
> >  	struct notifier_block reboot_notifier;
> >  };
> >  
> > 
> > -- 
> > 2.34.1
> > 
> 
> -- 
> -Mukesh Ojha

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Lorenzo Pieralisi]

On Sun, Nov 09, 2025 at 08:07:20PM +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.
> 
> Implement the PSCI vendor-specific resets by registering to the
> reboot-mode framework.

I think that we should expose to user space _all_ PSCI reset types,
cold, warm + vendor specific - as a departure from using the reboot_mode
variable (and possibly deprecate it - or at least stop using it).

> As psci init is done at early kernel init, reboot-mode registration cannot
> be done at the time of psci init.  This is because reboot-mode creates a
> “reboot-mode” class for exposing sysfs, which can fail at early kernel init.
> To overcome this, 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. Register a panic notifier and clear vendor_reset valid status
> during panic.  This is needed for any kernel panic that occurs post
> reboot_notifiers.

Is it because panic uses reboot_mode to determine the reset to issue ?

> 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 under “reboot-mode” and are based on the
> reboot-mode based commands.

IMHO - it would be nice if could add mode-cold (or mode-normal in reboot mode
speak) and mode-warm by default (if PSCI supports them) so that userspace
could issue those resets too without having to set the reboot_mode variable.

Reason is, since we are doing this it is worth going the whole nine
yards and try to decouple the reboot_mode variable from the RESTART2
syscall argument.

Reworded: just use the new userspace interface you are adding for
all PSCI reset types.

Thoughts very much welcome - I understand this is controversial.

> Reviewed-by: Umang Chheda <umang.chheda@oss.qualcomm.com>
> Reviewed-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> Reviewed-by: Nirmesh Kumar Singh <nirmesh.singh@oss.qualcomm.com>
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  drivers/firmware/psci/Kconfig |  2 +
>  drivers/firmware/psci/psci.c  | 92 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 93 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..ff82e7f4c27d1609a75cedc3a9790affaf839801 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -8,15 +8,18 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/arm-smccc.h>
> +#include <linux/bitops.h>
>  #include <linux/cpuidle.h>
>  #include <linux/debugfs.h>
>  #include <linux/errno.h>
>  #include <linux/linkage.h>
>  #include <linux/of.h>
> +#include <linux/panic_notifier.h>
>  #include <linux/pm.h>
>  #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 +54,24 @@ 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;

I think this should represent all possible PSCI reset types, not vendor only
and its value is set by the reboot mode framework.

> +
> +static int psci_panic_event(struct notifier_block *nb, unsigned long v, void *p)
> +{
> +	vendor_reset.valid = false;

I don't like this. Basically all you want this for is to make sure that
we don't override the reboot_mode variable.

One (hack) would consist in checking the reboot_mode variable here and
set the struct I mentioned above to the value represented in reboot_mode.

Good luck if reboot_mode == REBOOT_GPIO :-)

> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block psci_panic_block = {
> +	.notifier_call = psci_panic_event
> +};
> +
>  bool psci_tos_resident_on(int cpu)
>  {
>  	return cpu == resident_cpu;
> @@ -309,7 +330,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);

See above. Two calls here: one for resets issued using the new userspace
interface you are adding and legacy below - no vendor vs reboot_mode, this
is a mess.

> +	} else if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>  	    psci_system_reset2_supported) {
>  		/*
>  		 * reset_type[31] = 0 (architectural)
> @@ -547,6 +571,72 @@ 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 & GENMASK(31, 0);
> +		vendor_reset.reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic_32;
> +		vendor_reset.cookie = (magic >> 32) & GENMASK(31, 0);

Use FIELD_PREP/GET() please (but as mentioned above the vendor reset type
bit[31] should be part of the reboot mode magic value, see above).

> +		vendor_reset.valid = true;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int __init psci_init_vendor_reset(void)
> +{
> +	struct reboot_mode_driver *reboot;
> +	struct device_node *psci_np;
> +	struct device_node *np;
> +	int ret;
> +
> +	if (!psci_system_reset2_supported)
> +		return -EINVAL;
> +
> +	psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
> +	if (!psci_np)
> +		return -ENODEV;
> +
> +	np = of_find_node_by_name(psci_np, "reboot-mode");
> +	if (!np) {
> +		of_node_put(psci_np);
> +		return -ENODEV;
> +	}
> +
> +	ret = atomic_notifier_chain_register(&panic_notifier_list, &psci_panic_block);
> +	if (ret)
> +		goto err_notifier;
> +
> +	reboot = kzalloc(sizeof(*reboot), GFP_KERNEL);
> +	if (!reboot) {
> +		ret = -ENOMEM;
> +		goto err_kzalloc;
> +	}
> +
> +	reboot->write = psci_set_vendor_sys_reset2;
> +	reboot->driver_name = "psci";
> +
> +	ret = reboot_mode_register(reboot, of_fwnode_handle(np));
> +	if (ret)
> +		goto err_register;
> +
> +	of_node_put(psci_np);
> +	of_node_put(np);
> +	return 0;
> +
> +err_register:
> +	kfree(reboot);
> +err_kzalloc:
> +	atomic_notifier_chain_unregister(&panic_notifier_list, &psci_panic_block);
> +err_notifier:
> +	of_node_put(psci_np);
> +	of_node_put(np);
> +	return ret;
> +}
> +late_initcall(psci_init_vendor_reset)

I don't like adding another initcall here.

I wonder whether this code belongs in a PSCI reboot mode driver, possibly a
faux device in a way similar to what we did for cpuidle-psci (that after all
is a consumer of PSCI_CPU_SUSPEND in a similar way as this code is a
PSCI_SYSTEM_RESET{2} consumer), that communicates with
drivers/firmware/psci/psci.c with the struct mentioned above.

Thanks,
Lorenzo

> +
>  static void __init psci_init_system_reset2(void)
>  {
>  	int ret;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Shivendra Pratap]

On 11/10/2025 10:00 PM, Bjorn Andersson wrote:
> On Mon, Nov 10, 2025 at 07:15:29PM +0530, Mukesh Ojha wrote:
>> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
>>> Current reboot-mode supports a single 32-bit argument for any
>>> supported mode. Some reboot-mode based drivers may require
>>> passing two independent 32-bit arguments during a reboot
>>> sequence, for uses-cases, where a mode requires an additional
>>> argument. Such drivers may not be able to use the reboot-mode
>>> driver. For example, ARM PSCI vendor-specific resets, need two
>>> arguments for its operation – reset_type and cookie, to complete
>>> the reset operation. If a driver wants to implement this
>>> firmware-based reset, it cannot use reboot-mode framework.
>>>
>>> Introduce 64-bit magic values in reboot-mode driver to
>>> accommodate dual 32-bit arguments when specified via device tree.
>>> In cases, where no second argument is passed from device tree,
>>> keep the upper 32-bit of magic un-changed(0) to maintain backward
>>> compatibility.
>>>
>>> Update the current drivers using reboot-mode for a 64-bit magic
>>> value.

[SNIP..]

>>> +	if (magic > U32_MAX)
>>> +		return -EINVAL;
>>> +
>>> +	magic_32 = magic;
>>> +
>>>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
>>>  
>>>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
>>> -				 syscon_rbm->mask, magic);
>>> +				 syscon_rbm->mask, magic_32);
> 
> As above, if we're no longer silently discarding bits, I think we should
> compare the magic against syscon_rbm->mask.
> 
> No need for a local variable, just type cast after checking the validity.

Trying to summarize below why we added these check-

the patch in v11 used typecasting and did not have any of these checks(link below):
https://lore.kernel.org/all/20250717-arm-psci-system_reset2-vendor-reboots-v11-2-df3e2b2183c3@oss.qualcomm.com/

As per below upstream review, type cast was removed and bound checks were added all-over patchset:
"As a general rule of thumb, code with casts is poor quality code. Try
to write the code without casts." - 
https://lore.kernel.org/all/8d4a42b6-657f-4c30-8e25-4213d8d53a89@lunn.ch/

We can revert to the typecast way. Please suggest.

thanks,
Shivendra

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Bjorn Andersson]

On Mon, Nov 10, 2025 at 11:22:40PM +0530, Shivendra Pratap wrote:
> 
> 
> On 11/10/2025 10:00 PM, Bjorn Andersson wrote:
> > On Mon, Nov 10, 2025 at 07:15:29PM +0530, Mukesh Ojha wrote:
> >> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
> >>> Current reboot-mode supports a single 32-bit argument for any
> >>> supported mode. Some reboot-mode based drivers may require
> >>> passing two independent 32-bit arguments during a reboot
> >>> sequence, for uses-cases, where a mode requires an additional
> >>> argument. Such drivers may not be able to use the reboot-mode
> >>> driver. For example, ARM PSCI vendor-specific resets, need two
> >>> arguments for its operation – reset_type and cookie, to complete
> >>> the reset operation. If a driver wants to implement this
> >>> firmware-based reset, it cannot use reboot-mode framework.
> >>>
> >>> Introduce 64-bit magic values in reboot-mode driver to
> >>> accommodate dual 32-bit arguments when specified via device tree.
> >>> In cases, where no second argument is passed from device tree,
> >>> keep the upper 32-bit of magic un-changed(0) to maintain backward
> >>> compatibility.
> >>>
> >>> Update the current drivers using reboot-mode for a 64-bit magic
> >>> value.
> 
> [SNIP..]
> 
> >>> +	if (magic > U32_MAX)
> >>> +		return -EINVAL;
> >>> +
> >>> +	magic_32 = magic;
> >>> +
> >>>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
> >>>  
> >>>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
> >>> -				 syscon_rbm->mask, magic);
> >>> +				 syscon_rbm->mask, magic_32);
> > 
> > As above, if we're no longer silently discarding bits, I think we should
> > compare the magic against syscon_rbm->mask.
> > 
> > No need for a local variable, just type cast after checking the validity.
> 
> Trying to summarize below why we added these check-
> 
> the patch in v11 used typecasting and did not have any of these checks(link below):
> https://lore.kernel.org/all/20250717-arm-psci-system_reset2-vendor-reboots-v11-2-df3e2b2183c3@oss.qualcomm.com/
> 
> As per below upstream review, type cast was removed and bound checks were added all-over patchset:
> "As a general rule of thumb, code with casts is poor quality code. Try
> to write the code without casts." - 
> https://lore.kernel.org/all/8d4a42b6-657f-4c30-8e25-4213d8d53a89@lunn.ch/
> 
> We can revert to the typecast way. Please suggest.
> 

Okay, I'm okay with Andrew's original request, stick to that for the
nvmem case. Although I don't fancy the name "magic_32", and would prefer
that you just call it "value" or something.


For pon and syscon however, I'm wondering why you have ignored Andrew's
other request from that same email:

"""
You might be able to go further, and validate that magic actually fits
into the field when you consider the << pon->reason_shift.
"""

Writing "if (magic > U32_MAX)" in a snippet of code where magic isn't
allowed to be more than either 32 or 64 is misleading.

For syscon, it's true that the parameter is an unsigned long, but the
actual limit better be based on syscon_rbm->mask.

Regards,
Bjorn

> thanks,
> Shivendra

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Shivendra Pratap]

On 11/11/2025 12:03 AM, Bjorn Andersson wrote:
> On Mon, Nov 10, 2025 at 11:22:40PM +0530, Shivendra Pratap wrote:
>>
>>
>> On 11/10/2025 10:00 PM, Bjorn Andersson wrote:
>>> On Mon, Nov 10, 2025 at 07:15:29PM +0530, Mukesh Ojha wrote:
>>>> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
>>>>> Current reboot-mode supports a single 32-bit argument for any
>>>>> supported mode. Some reboot-mode based drivers may require
>>>>> passing two independent 32-bit arguments during a reboot
>>>>> sequence, for uses-cases, where a mode requires an additional
>>>>> argument. Such drivers may not be able to use the reboot-mode
>>>>> driver. For example, ARM PSCI vendor-specific resets, need two
>>>>> arguments for its operation – reset_type and cookie, to complete
>>>>> the reset operation. If a driver wants to implement this
>>>>> firmware-based reset, it cannot use reboot-mode framework.
>>>>>
>>>>> Introduce 64-bit magic values in reboot-mode driver to
>>>>> accommodate dual 32-bit arguments when specified via device tree.
>>>>> In cases, where no second argument is passed from device tree,
>>>>> keep the upper 32-bit of magic un-changed(0) to maintain backward
>>>>> compatibility.
>>>>>
>>>>> Update the current drivers using reboot-mode for a 64-bit magic
>>>>> value.
>>
>> [SNIP..]
>>
>>>>> +	if (magic > U32_MAX)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	magic_32 = magic;
>>>>> +
>>>>>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
>>>>>  
>>>>>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
>>>>> -				 syscon_rbm->mask, magic);
>>>>> +				 syscon_rbm->mask, magic_32);
>>>
>>> As above, if we're no longer silently discarding bits, I think we should
>>> compare the magic against syscon_rbm->mask.
>>>
>>> No need for a local variable, just type cast after checking the validity.
>>
>> Trying to summarize below why we added these check-
>>
>> the patch in v11 used typecasting and did not have any of these checks(link below):
>> https://lore.kernel.org/all/20250717-arm-psci-system_reset2-vendor-reboots-v11-2-df3e2b2183c3@oss.qualcomm.com/
>>
>> As per below upstream review, type cast was removed and bound checks were added all-over patchset:
>> "As a general rule of thumb, code with casts is poor quality code. Try
>> to write the code without casts." - 
>> https://lore.kernel.org/all/8d4a42b6-657f-4c30-8e25-4213d8d53a89@lunn.ch/
>>
>> We can revert to the typecast way. Please suggest.
>>
> 
> Okay, I'm okay with Andrew's original request, stick to that for the
> nvmem case. Although I don't fancy the name "magic_32", and would prefer
> that you just call it "value" or something.

sure will make it proper wherever applicable. 

> 
> 
> For pon and syscon however, I'm wondering why you have ignored Andrew's
> other request from that same email:
> 
> """
> You might be able to go further, and validate that magic actually fits
> into the field when you consider the << pon->reason_shift.
> """
> 
> Writing "if (magic > U32_MAX)" in a snippet of code where magic isn't
> allowed to be more than either 32 or 64 is misleading.
> 
> For syscon, it's true that the parameter is an unsigned long, but the
> actual limit better be based on syscon_rbm->mask.

May be i was not able to interpret it correctly. Basically tried to
make sure that magic that now coming in a "u64 magic" should be passed
ahead only it its a 32 bit value.

So should i get rid of the checks done here for syscon and pon?

thanks,
Shivendra

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Bjorn Andersson]

On Tue, Nov 11, 2025 at 08:20:43PM +0530, Shivendra Pratap wrote:
> 
> 
> On 11/11/2025 12:03 AM, Bjorn Andersson wrote:
> > On Mon, Nov 10, 2025 at 11:22:40PM +0530, Shivendra Pratap wrote:
> >>
> >>
> >> On 11/10/2025 10:00 PM, Bjorn Andersson wrote:
> >>> On Mon, Nov 10, 2025 at 07:15:29PM +0530, Mukesh Ojha wrote:
> >>>> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
> >>>>> Current reboot-mode supports a single 32-bit argument for any
> >>>>> supported mode. Some reboot-mode based drivers may require
> >>>>> passing two independent 32-bit arguments during a reboot
> >>>>> sequence, for uses-cases, where a mode requires an additional
> >>>>> argument. Such drivers may not be able to use the reboot-mode
> >>>>> driver. For example, ARM PSCI vendor-specific resets, need two
> >>>>> arguments for its operation – reset_type and cookie, to complete
> >>>>> the reset operation. If a driver wants to implement this
> >>>>> firmware-based reset, it cannot use reboot-mode framework.
> >>>>>
> >>>>> Introduce 64-bit magic values in reboot-mode driver to
> >>>>> accommodate dual 32-bit arguments when specified via device tree.
> >>>>> In cases, where no second argument is passed from device tree,
> >>>>> keep the upper 32-bit of magic un-changed(0) to maintain backward
> >>>>> compatibility.
> >>>>>
> >>>>> Update the current drivers using reboot-mode for a 64-bit magic
> >>>>> value.
> >>
> >> [SNIP..]
> >>
> >>>>> +	if (magic > U32_MAX)
> >>>>> +		return -EINVAL;
> >>>>> +
> >>>>> +	magic_32 = magic;
> >>>>> +
> >>>>>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
> >>>>>  
> >>>>>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
> >>>>> -				 syscon_rbm->mask, magic);
> >>>>> +				 syscon_rbm->mask, magic_32);
> >>>
> >>> As above, if we're no longer silently discarding bits, I think we should
> >>> compare the magic against syscon_rbm->mask.
> >>>
> >>> No need for a local variable, just type cast after checking the validity.
> >>
> >> Trying to summarize below why we added these check-
> >>
> >> the patch in v11 used typecasting and did not have any of these checks(link below):
> >> https://lore.kernel.org/all/20250717-arm-psci-system_reset2-vendor-reboots-v11-2-df3e2b2183c3@oss.qualcomm.com/
> >>
> >> As per below upstream review, type cast was removed and bound checks were added all-over patchset:
> >> "As a general rule of thumb, code with casts is poor quality code. Try
> >> to write the code without casts." - 
> >> https://lore.kernel.org/all/8d4a42b6-657f-4c30-8e25-4213d8d53a89@lunn.ch/
> >>
> >> We can revert to the typecast way. Please suggest.
> >>
> > 
> > Okay, I'm okay with Andrew's original request, stick to that for the
> > nvmem case. Although I don't fancy the name "magic_32", and would prefer
> > that you just call it "value" or something.
> 
> sure will make it proper wherever applicable. 
> 
> > 
> > 
> > For pon and syscon however, I'm wondering why you have ignored Andrew's
> > other request from that same email:
> > 
> > """
> > You might be able to go further, and validate that magic actually fits
> > into the field when you consider the << pon->reason_shift.
> > """
> > 
> > Writing "if (magic > U32_MAX)" in a snippet of code where magic isn't
> > allowed to be more than either 32 or 64 is misleading.
> > 
> > For syscon, it's true that the parameter is an unsigned long, but the
> > actual limit better be based on syscon_rbm->mask.
> 
> May be i was not able to interpret it correctly. Basically tried to
> make sure that magic that now coming in a "u64 magic" should be passed
> ahead only it its a 32 bit value.
> 

That is the correct interpretation of the original ask. But what I'm
saying is that if you write:

if (magic > U32_MAX)

then that means "check that magic isn't larger than 32 bits". So the
reader will see that and understand that magic can only be 32 bits.

But the actual PON magic value is 5 or 6 bits, not 32 - so you should
check that the value fits in 5 or 6 bits.

> So should i get rid of the checks done here for syscon and pon?
> 

Continuing to silently ignoring other bits would be one option, but
you've been asked to sanity check the values, so please do that. You
have the code, just compare with the correct value.

Regards,
Bjorn

> thanks,
> Shivendra

Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

[Shivendra Pratap]

On 11/11/2025 9:55 PM, Bjorn Andersson wrote:
> On Tue, Nov 11, 2025 at 08:20:43PM +0530, Shivendra Pratap wrote:
>>
>>
>> On 11/11/2025 12:03 AM, Bjorn Andersson wrote:
>>> On Mon, Nov 10, 2025 at 11:22:40PM +0530, Shivendra Pratap wrote:
>>>>
>>>>
>>>> On 11/10/2025 10:00 PM, Bjorn Andersson wrote:
>>>>> On Mon, Nov 10, 2025 at 07:15:29PM +0530, Mukesh Ojha wrote:
>>>>>> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
>>>>>>> Current reboot-mode supports a single 32-bit argument for any
>>>>>>> supported mode. Some reboot-mode based drivers may require
>>>>>>> passing two independent 32-bit arguments during a reboot
>>>>>>> sequence, for uses-cases, where a mode requires an additional
>>>>>>> argument. Such drivers may not be able to use the reboot-mode
>>>>>>> driver. For example, ARM PSCI vendor-specific resets, need two
>>>>>>> arguments for its operation – reset_type and cookie, to complete
>>>>>>> the reset operation. If a driver wants to implement this
>>>>>>> firmware-based reset, it cannot use reboot-mode framework.
>>>>>>>
>>>>>>> Introduce 64-bit magic values in reboot-mode driver to
>>>>>>> accommodate dual 32-bit arguments when specified via device tree.
>>>>>>> In cases, where no second argument is passed from device tree,
>>>>>>> keep the upper 32-bit of magic un-changed(0) to maintain backward
>>>>>>> compatibility.
>>>>>>>
>>>>>>> Update the current drivers using reboot-mode for a 64-bit magic
>>>>>>> value.
>>>>
>>>> [SNIP..]
>>>>
>>>>>>> +	if (magic > U32_MAX)
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>> +	magic_32 = magic;
>>>>>>> +
>>>>>>>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
>>>>>>>  
>>>>>>>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
>>>>>>> -				 syscon_rbm->mask, magic);
>>>>>>> +				 syscon_rbm->mask, magic_32);
>>>>>
>>>>> As above, if we're no longer silently discarding bits, I think we should
>>>>> compare the magic against syscon_rbm->mask.
>>>>>
>>>>> No need for a local variable, just type cast after checking the validity.
>>>>
>>>> Trying to summarize below why we added these check-
>>>>
>>>> the patch in v11 used typecasting and did not have any of these checks(link below):
>>>> https://lore.kernel.org/all/20250717-arm-psci-system_reset2-vendor-reboots-v11-2-df3e2b2183c3@oss.qualcomm.com/
>>>>
>>>> As per below upstream review, type cast was removed and bound checks were added all-over patchset:
>>>> "As a general rule of thumb, code with casts is poor quality code. Try
>>>> to write the code without casts." - 
>>>> https://lore.kernel.org/all/8d4a42b6-657f-4c30-8e25-4213d8d53a89@lunn.ch/
>>>>
>>>> We can revert to the typecast way. Please suggest.
>>>>
>>>
>>> Okay, I'm okay with Andrew's original request, stick to that for the
>>> nvmem case. Although I don't fancy the name "magic_32", and would prefer
>>> that you just call it "value" or something.
>>
>> sure will make it proper wherever applicable. 
>>
>>>
>>>
>>> For pon and syscon however, I'm wondering why you have ignored Andrew's
>>> other request from that same email:
>>>
>>> """
>>> You might be able to go further, and validate that magic actually fits
>>> into the field when you consider the << pon->reason_shift.
>>> """
>>>
>>> Writing "if (magic > U32_MAX)" in a snippet of code where magic isn't
>>> allowed to be more than either 32 or 64 is misleading.
>>>
>>> For syscon, it's true that the parameter is an unsigned long, but the
>>> actual limit better be based on syscon_rbm->mask.
>>
>> May be i was not able to interpret it correctly. Basically tried to
>> make sure that magic that now coming in a "u64 magic" should be passed
>> ahead only it its a 32 bit value.
>>
> 
> That is the correct interpretation of the original ask. But what I'm
> saying is that if you write:
> 
> if (magic > U32_MAX)
> 
> then that means "check that magic isn't larger than 32 bits". So the
> reader will see that and understand that magic can only be 32 bits.
> 
> But the actual PON magic value is 5 or 6 bits, not 32 - so you should
> check that the value fits in 5 or 6 bits.

sure. thanks.

> 
>> So should i get rid of the checks done here for syscon and pon?
>>
> 
> Continuing to silently ignoring other bits would be one option, but
> you've been asked to sanity check the values, so please do that. You
> have the code, just compare with the correct value.

ok. got it. thanks.

thanks,
Shivendra

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Bjorn Andersson]

On Mon, Nov 10, 2025 at 09:49:50PM +0530, Mukesh Ojha wrote:
> On Mon, Nov 10, 2025 at 09:30:26AM -0600, Bjorn Andersson wrote:
> > On Mon, Nov 10, 2025 at 05:58:24PM +0530, Mukesh Ojha wrote:
> > > On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
> > > > From: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > > > 
> > > > Add support for SYSTEM_RESET2 vendor-specific resets in
> > > > qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
> > > > "bootloader" will cause device to reboot and stop in the
> > > > bootloader's fastboot mode. "edl" will cause device to reboot
> > > > into "emergency download mode", which permits loading images via
> > > > the Firehose protocol.
> > > > 
> > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > > > Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
> > > > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > @@ -935,6 +935,13 @@ &pon_resin {
> > > >  	status = "okay";
> > > >  };
> > > >  
> > > > +&psci {
> > > > +	reboot-mode {
> > > > +		mode-bootloader = <0x10001 0x2>;
> > > > +		mode-edl = <0 0x1>;
> > > > +	};
> > > > +};
> > > > +
> > > 
> > > Make sense for this as it leverages sc7280 and adding it there would not
> > > have made sense.
> > > 
> > 
> > Why wouldn't it make sense?
> 
> It is better to add for platforms we know their firmware support this
> from day1 and not add for something like chrome or any other variant of
> sc7280 where this support would never come or not tested.

So SYSTEM_RESET2 only exist in newer firmware versions and hence this
isn't (and won't be) broadly available in SC7280 devices.

That would be excellent information to put in the commit message, so
others writing Kodiak dts doesn't feel the urge to copy this and debug
why it doesn't work.

> > 
> > > Acked-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > > 
> > 
> > Please read submitting-patches.rst about Acked-by, and use Reviewed-by
> > going forward.
> 
> I was acking the idea of this particular change in platform file compared to
> other patches, if above reason looks fine, can be converted to R-by.
> 

That's appreciated, but per the documentation, the meaning of acked-by
is different.

Regards,
Bjorn

> > 
> > Regards,
> > Bjorn
> > 
> > > >  &qup_uart7_cts {
> > > >  	/*
> > > >  	 * Configure a bias-bus-hold on CTS to lower power
> > > > 
> > > > -- 
> > > > 2.34.1
> > > > 
> > > 
> > > -- 
> > > -Mukesh Ojha
> > > 
> 
> -- 
> -Mukesh Ojha

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Bjorn Andersson]

On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
> From: Elliot Berman <elliot.berman@oss.qualcomm.com>
> 
> Add support for SYSTEM_RESET2 vendor-specific resets in

Please rewrite this to start with the purpose of the patch, introduce
the fact that we can use SYSTEM_RESET2 to affect the reboot mode.

Make sure you include the information that this isn't broadly available
on all sc7280 devices.

Please line break at 72 characters, not less.

> qcs6490-rb3gen2 as reboot-modes.  Describe the resets:

This looks like the header for a list, that documents "bootloader" and
"edl", but it's just one lump of text. Please improve the formatting. If
it is a list, then make it look like a list.

Regards,
Bjorn

> "bootloader" will cause device to reboot and stop in the
> bootloader's fastboot mode. "edl" will cause device to reboot
> into "emergency download mode", which permits loading images via
> the Firehose protocol.
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -935,6 +935,13 @@ &pon_resin {
>  	status = "okay";
>  };
>  
> +&psci {
> +	reboot-mode {
> +		mode-bootloader = <0x10001 0x2>;
> +		mode-edl = <0 0x1>;
> +	};
> +};
> +
>  &qup_uart7_cts {
>  	/*
>  	 * Configure a bias-bus-hold on CTS to lower power
> 
> -- 
> 2.34.1
> 

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Mukesh Ojha]

On Tue, Nov 11, 2025 at 10:52:34AM -0600, Bjorn Andersson wrote:
> On Mon, Nov 10, 2025 at 09:49:50PM +0530, Mukesh Ojha wrote:
> > On Mon, Nov 10, 2025 at 09:30:26AM -0600, Bjorn Andersson wrote:
> > > On Mon, Nov 10, 2025 at 05:58:24PM +0530, Mukesh Ojha wrote:
> > > > On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
> > > > > From: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > > > > 
> > > > > Add support for SYSTEM_RESET2 vendor-specific resets in
> > > > > qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
> > > > > "bootloader" will cause device to reboot and stop in the
> > > > > bootloader's fastboot mode. "edl" will cause device to reboot
> > > > > into "emergency download mode", which permits loading images via
> > > > > the Firehose protocol.
> > > > > 
> > > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > > Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
> > > > > Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> > > > > ---
> > > > >  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > > > > @@ -935,6 +935,13 @@ &pon_resin {
> > > > >  	status = "okay";
> > > > >  };
> > > > >  
> > > > > +&psci {
> > > > > +	reboot-mode {
> > > > > +		mode-bootloader = <0x10001 0x2>;
> > > > > +		mode-edl = <0 0x1>;
> > > > > +	};
> > > > > +};
> > > > > +
> > > > 
> > > > Make sense for this as it leverages sc7280 and adding it there would not
> > > > have made sense.
> > > > 
> > > 
> > > Why wouldn't it make sense?
> > 
> > It is better to add for platforms we know their firmware support this
> > from day1 and not add for something like chrome or any other variant of
> > sc7280 where this support would never come or not tested.
> 
> So SYSTEM_RESET2 only exist in newer firmware versions and hence this
> isn't (and won't be) broadly available in SC7280 devices.
> 
> That would be excellent information to put in the commit message, so
> others writing Kodiak dts doesn't feel the urge to copy this and debug
> why it doesn't work.

Sure, @shivendra please add this information in commit text.

> 
> > > 
> > > > Acked-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> > > > 
> > > 
> > > Please read submitting-patches.rst about Acked-by, and use Reviewed-by
> > > going forward.
> > 
> > I was acking the idea of this particular change in platform file compared to
> > other patches, if above reason looks fine, can be converted to R-by.
> > 
> 
> That's appreciated, but per the documentation, the meaning of acked-by
> is different.

Sure..

> 
> Regards,
> Bjorn
> 
> > > 
> > > Regards,
> > > Bjorn
> > > 
> > > > >  &qup_uart7_cts {
> > > > >  	/*
> > > > >  	 * Configure a bias-bus-hold on CTS to lower power
> > > > > 
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > > > 
> > > > -- 
> > > > -Mukesh Ojha
> > > > 
> > 
> > -- 
> > -Mukesh Ojha

-- 
-Mukesh Ojha

Re: [PATCH v17 05/12] power: reset: reboot-mode: Expose sysfs for registered reboot_modes

[Shivendra Pratap]

On 11/10/2025 8:44 PM, Bartosz Golaszewski wrote:
> On Sun, 9 Nov 2025 at 15:38, Shivendra Pratap
> <shivendra.pratap@oss.qualcomm.com> wrote:
>>
>> Currently, there is no standardized mechanism for userspace to
>> discover which reboot-modes are supported on a given platform.
>> This limitation forces tools and scripts to rely on hardcoded
>> assumptions about the supported reboot-modes.
>>
>> Create a class 'reboot-mode' and a device under it to expose a
>> sysfs interface to show the available reboot mode arguments to
>> userspace. Use the driver_name field of the struct
>> reboot_mode_driver to create the device. For device-based
>> drivers, configure the device driver name as driver_name.
>>
>> This results in the creation of:
>>   /sys/class/reboot-mode/<driver>/reboot_modes
>>
>> This read-only sysfs file will exposes the list of supported
>> reboot modes arguments provided by the driver, enabling userspace
>> to query the list of arguments.
>>
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
>>  drivers/power/reset/reboot-mode.c | 62 ++++++++++++++++++++++++++++++++++++++-
>>  include/linux/reboot-mode.h       |  2 ++
>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
>> index 873ac45cd7659b214b7c21958f580ca381e0a63d..582aa7f8ed7fa485c5a67877558c9b15d3600ef4 100644
>> --- a/drivers/power/reset/reboot-mode.c
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -6,6 +6,7 @@
>>  #define pr_fmt(fmt)    "reboot-mode: " fmt
>>
>>  #include <linux/device.h>
>> +#include <linux/err.h>
>>  #include <linux/init.h>
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>> @@ -23,6 +24,8 @@ struct mode_info {
>>         struct list_head list;
>>  };
>>
>> +static struct class *rb_class;
>> +
> 
> I know C is a spartan language but the rb_ prefix makes me think of
> the red-black tree. Please call it reboot_mode_class.

sure will make it reboot_mode_class.

> 
>>  static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
>>  {
>>         const char *normal = "normal";
>> @@ -65,6 +68,51 @@ static int reboot_mode_notify(struct notifier_block *this,
>>         return NOTIFY_DONE;
>>  }
>>
>> +static ssize_t reboot_modes_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +       struct reboot_mode_driver *reboot;
>> +       struct mode_info *info;
>> +       ssize_t size = 0;
>> +
>> +       reboot = (struct reboot_mode_driver *)dev_get_drvdata(dev);
> 
> No need for the cast.

Ack.

>> +       if (!reboot)
>> +               return -ENODATA;
>> +
>> +       list_for_each_entry(info, &reboot->head, list)
>> +               size += sysfs_emit_at(buf, size, "%s ", info->mode);
>> +
>> +       if (size) {
>> +               size += sysfs_emit_at(buf, size - 1, "\n");
>> +               return size;
>> +       }
> 
> This is a weird logic inversion. Just do:
> 
> if (!size)
>     return -ENODATA;
> 
> return size + sysfs_emit_at(buf, size - 1, "\n");

Ack.

>> +
>> +       return -ENODATA;
>> +}
>> +static DEVICE_ATTR_RO(reboot_modes);
>> +
>> +static int create_reboot_mode_device(struct reboot_mode_driver *reboot)
>> +{
>> +       int ret = 0;
>> +
>> +       if (!rb_class) {
>> +               rb_class = class_create("reboot-mode");
>> +               if (IS_ERR(rb_class))
>> +                       return PTR_ERR(rb_class);
>> +       }
> 
> Why the lazy initialization here? Is there any reason you can't
> statically define the class? Don't you need synchronization here if
> multiple drivers try to do this?

reboot-mode framework does not has a init. Should i add a inti_call
here and create a class? Can you suggest a bit more.

For if we at-all continue this lazy init, will add a lock.

thanks,
Shivendra

Re: [PATCH v17 05/12] power: reset: reboot-mode: Expose sysfs for registered reboot_modes

[Shivendra Pratap]

On 11/10/2025 9:45 PM, Bjorn Andersson wrote:
> On Sun, Nov 09, 2025 at 08:07:18PM +0530, Shivendra Pratap wrote:
>> Currently, there is no standardized mechanism for userspace to
>> discover which reboot-modes are supported on a given platform.
>> This limitation forces tools and scripts to rely on hardcoded
>> assumptions about the supported reboot-modes.
>>
>> Create a class 'reboot-mode' and a device under it to expose a
>> sysfs interface to show the available reboot mode arguments to
>> userspace. Use the driver_name field of the struct
>> reboot_mode_driver to create the device. For device-based
>> drivers, configure the device driver name as driver_name.
>>
>> This results in the creation of:
>>   /sys/class/reboot-mode/<driver>/reboot_modes
>>
>> This read-only sysfs file will exposes the list of supported
>> reboot modes arguments provided by the driver, enabling userspace
>> to query the list of arguments.
>>
> 
> I like this addition, and your commit message reasoning about this
> addition. But, while touching upon the same subject, you've made this
> series add two separate things.
> 
> So now this part can't be merged unless there's agreement on the PSCI
> SYSTEM_RESET2, and the PSCI SYSTEM_RESET2 can't be merged unless this
> sysfs interface is agreed upon.
> 
> Unless I'm missing some clear dependency here, it would have been better
> to keep these two topics in separate series, and drive them to
> conclusion independently.

sure. Will split this series based on dependencies.

the psci patch does has a dependency on the fwnode based registration
and the u64 bit magic registration. Let me see how can i split the series
to 2 independent patchsets. Any suggestions will be helpful.

potentially it may be.
1 - devres removal + expose sysfs
2 - u64 bit registration = fw node + any extras + psci. 

> 
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
>>  drivers/power/reset/reboot-mode.c | 62 ++++++++++++++++++++++++++++++++++++++-
>>  include/linux/reboot-mode.h       |  2 ++
>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
>> index 873ac45cd7659b214b7c21958f580ca381e0a63d..582aa7f8ed7fa485c5a67877558c9b15d3600ef4 100644
>> --- a/drivers/power/reset/reboot-mode.c
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -6,6 +6,7 @@
>>  #define pr_fmt(fmt)	"reboot-mode: " fmt
>>  
>>  #include <linux/device.h>
>> +#include <linux/err.h>
>>  #include <linux/init.h>
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>> @@ -23,6 +24,8 @@ struct mode_info {
>>  	struct list_head list;
>>  };
>>  
>> +static struct class *rb_class;
> 
> Why not "static const struct class reboot_mode_class" and then a
> class_register() call? Why do you need the class dynamically allocated
> on the heap?

Ack. will update this.

> 
>> +
>>  static u64 get_reboot_mode_magic(struct reboot_mode_driver *reboot, const char *cmd)
>>  {
>>  	const char *normal = "normal";
>> @@ -65,6 +68,51 @@ static int reboot_mode_notify(struct notifier_block *this,
>>  	return NOTIFY_DONE;
>>  }
>>  
>> +static ssize_t reboot_modes_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct reboot_mode_driver *reboot;
>> +	struct mode_info *info;
>> +	ssize_t size = 0;
>> +
>> +	reboot = (struct reboot_mode_driver *)dev_get_drvdata(dev);
>> +	if (!reboot)
>> +		return -ENODATA;
>> +
>> +	list_for_each_entry(info, &reboot->head, list)
>> +		size += sysfs_emit_at(buf, size, "%s ", info->mode);
>> +
>> +	if (size) {
>> +		size += sysfs_emit_at(buf, size - 1, "\n");
>> +		return size;
>> +	}
>> +
>> +	return -ENODATA;
>> +}
>> +static DEVICE_ATTR_RO(reboot_modes);
>> +
>> +static int create_reboot_mode_device(struct reboot_mode_driver *reboot)
> 
> Note how (almost) all other function names in this file start with
> a "reboot_mode_" prefix.

Ack. will update.

> 
>> +{
>> +	int ret = 0;
> 
> First use is an assignment, no need for you to zero-initialize it here.
> 
>> +
>> +	if (!rb_class) {
>> +		rb_class = class_create("reboot-mode");
>> +		if (IS_ERR(rb_class))
>> +			return PTR_ERR(rb_class);
>> +	}
>> +
>> +	reboot->reboot_dev = device_create(rb_class, NULL, 0, (void *)reboot, reboot->driver_name);
> 
> Every struct reboot_mode_driver is going to end up having one of these,
> so why not incorporate it into the reboot_mode_driver in the first
> place. It avoids the extra heap allocation, and you can use
> container_of() instead of drv_data to find your reboot_mode_driver in
> the reboot_modes_show() above.
> 
> 
> Just:
>   reboot->reboot_dev.class = &reboot_mode_class;
>   dev_set_name(&reboot->reboot_dev, reboot->driver_name);
>   ret = device_register(&reboot->reboot_dev);
> 

Ack. thanks.

>> +	if (IS_ERR(reboot->reboot_dev))
>> +		return PTR_ERR(reboot->reboot_dev);
>> +
>> +	ret = device_create_file(reboot->reboot_dev, &dev_attr_reboot_modes);
> 
> Manually creating sysfs attributes is both error prone and racy, so if
> you can you should avoid it.
> 
> Here you have the opportunity to just statically assign
> reboot_mode_class->dev_groups to an ATTRIBUTE_GROUP() with your
> attribute and it will all be handled for you.
> 

Ack. will update this.

>> +	if (ret) {
>> +		device_unregister(reboot->reboot_dev);
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  /**
>>   * reboot_mode_register - register a reboot mode driver
>>   * @reboot: reboot mode driver
>> @@ -83,13 +131,17 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>>  	u32 magic_arg2;
>>  	int ret;
>>  
>> -	if (!fwnode)
>> +	if (!fwnode || !reboot->driver_name)
>>  		return -EINVAL;
>>  
>>  	np = to_of_node(fwnode);
>>  	if (!np)
>>  		return -EINVAL;
>>  
>> +	ret = create_reboot_mode_device(reboot);
>> +	if (ret)
>> +		return ret;
>> +
>>  	INIT_LIST_HEAD(&reboot->head);
>>  
>>  	for_each_property_of_node(np, prop) {
>> @@ -142,6 +194,8 @@ int reboot_mode_register(struct reboot_mode_driver *reboot, struct fwnode_handle
>>  		kfree(info);
>>  	}
>>  
>> +	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
>> +	device_unregister(reboot->reboot_dev);
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(reboot_mode_register);
>> @@ -155,6 +209,9 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>>  	struct mode_info *info;
>>  	struct mode_info *next;
>>  
>> +	if (!reboot->reboot_dev)
>> +		return -EINVAL;
>> +
>>  	unregister_reboot_notifier(&reboot->reboot_notifier);
>>  
>>  	list_for_each_entry_safe(info, next, &reboot->head, list) {
>> @@ -163,6 +220,8 @@ int reboot_mode_unregister(struct reboot_mode_driver *reboot)
>>  		kfree(info);
>>  	}
>>  
>> +	device_remove_file(reboot->reboot_dev, &dev_attr_reboot_modes);
>> +	device_unregister(reboot->reboot_dev);
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(reboot_mode_unregister);
>> @@ -192,6 +251,7 @@ int devm_reboot_mode_register(struct device *dev,
>>  	if (!dr)
>>  		return -ENOMEM;
>>  
>> +	reboot->driver_name = reboot->dev->driver->name;
> 
> It seems unlikely that we will have multiple instances of the same
> driver influencing the actual reboot mode, but we could very well have
> multiple instances of the same driver calling
> devm_reboot_mode_register(). E.g. on a board two PMICs, both with PON
> blocks (but only one considered as the source for boot mode).
> 
> In that case you will end up trying to create multiple devices with the
> name "qcom-pon", presumably that will fail and per your error handling
> you have now disabled the reboot-mechanism for all but the first pon
> instance that was registered.
> 
> It also creates some asymmetry between devm_reboot_mode_register() and
> reboot_mode_register(), in that the one API the client driver decides
> the name, in other it's hard coded to the driver name (and if the client
> did specify a name - which they should if they use the non-devm one- it
> will be overwritten).
> 
> 
> 
> On that note, I would argue that aborting the registration of
> reboot-modes, just because we failed to create the convenient "debug"
> interface, doesn't make sense. I think it would be better to just
> continue even when create_reboot_mode_device() returns an error.
>

sure i can modify this to continue on error.
 
>>  	rc = reboot_mode_register(reboot, of_fwnode_handle(reboot->dev->of_node));
>>  	if (rc) {
>>  		devres_free(dr);
>> diff --git a/include/linux/reboot-mode.h b/include/linux/reboot-mode.h
>> index e0d3e8a54050a76f26846f456120b4c7e371d284..81c149edf40fbcf0d3427c2e12eb415199cb153b 100644
>> --- a/include/linux/reboot-mode.h
>> +++ b/include/linux/reboot-mode.h
>> @@ -7,6 +7,8 @@
>>  
>>  struct reboot_mode_driver {
>>  	struct device *dev;
>> +	struct device *reboot_dev;
> 
> As suggested above:
> 
> struct device reboot_dev;

Ack.

thanks,
Shivendra

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Shivendra Pratap]

On 11/12/2025 4:45 PM, Mukesh Ojha wrote:
> On Tue, Nov 11, 2025 at 10:52:34AM -0600, Bjorn Andersson wrote:
>> On Mon, Nov 10, 2025 at 09:49:50PM +0530, Mukesh Ojha wrote:
>>> On Mon, Nov 10, 2025 at 09:30:26AM -0600, Bjorn Andersson wrote:
>>>> On Mon, Nov 10, 2025 at 05:58:24PM +0530, Mukesh Ojha wrote:
>>>>> On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
>>>>>> From: Elliot Berman <elliot.berman@oss.qualcomm.com>
>>>>>>
>>>>>> Add support for SYSTEM_RESET2 vendor-specific resets in
>>>>>> qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
>>>>>> "bootloader" will cause device to reboot and stop in the
>>>>>> bootloader's fastboot mode. "edl" will cause device to reboot
>>>>>> into "emergency download mode", which permits loading images via
>>>>>> the Firehose protocol.
>>>>>>
>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>> Signed-off-by: Elliot Berman <elliot.berman@oss.qualcomm.com>
>>>>>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>>>>>> ---
>>>>>>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 +++++++
>>>>>>  1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>>>>>> index 721a26d49ccaeb1429e2cc1c3a5c8d9517da3be6..cebdedd5d614b9efb6dfbee91dd67f3c3e322a38 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>>>>>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>>>>>> @@ -935,6 +935,13 @@ &pon_resin {
>>>>>>  	status = "okay";
>>>>>>  };
>>>>>>  
>>>>>> +&psci {
>>>>>> +	reboot-mode {
>>>>>> +		mode-bootloader = <0x10001 0x2>;
>>>>>> +		mode-edl = <0 0x1>;
>>>>>> +	};
>>>>>> +};
>>>>>> +
>>>>>
>>>>> Make sense for this as it leverages sc7280 and adding it there would not
>>>>> have made sense.
>>>>>
>>>>
>>>> Why wouldn't it make sense?
>>>
>>> It is better to add for platforms we know their firmware support this
>>> from day1 and not add for something like chrome or any other variant of
>>> sc7280 where this support would never come or not tested.
>>
>> So SYSTEM_RESET2 only exist in newer firmware versions and hence this
>> isn't (and won't be) broadly available in SC7280 devices.
>>
>> That would be excellent information to put in the commit message, so
>> others writing Kodiak dts doesn't feel the urge to copy this and debug
>> why it doesn't work.
> 
> Sure, @shivendra please add this information in commit text.

Ack.

> 
>>
>>>>
>>>>> Acked-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
>>>>>
>>>>
>>>> Please read submitting-patches.rst about Acked-by, and use Reviewed-by
>>>> going forward.
>>>
>>> I was acking the idea of this particular change in platform file compared to
>>> other patches, if above reason looks fine, can be converted to R-by.
>>>
>>
>> That's appreciated, but per the documentation, the meaning of acked-by
>> is different.
> 
> Sure..
> 
>>
>> Regards,
>> Bjorn
>>
>>>>
>>>> Regards,
>>>> Bjorn
>>>>
>>>>>>  &qup_uart7_cts {
>>>>>>  	/*
>>>>>>  	 * Configure a bias-bus-hold on CTS to lower power
>>>>>>
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>>>
>>>>> -- 
>>>>> -Mukesh Ojha
>>>>>
>>>
>>> -- 
>>> -Mukesh Ojha
> 

Re: [PATCH v17 09/12] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Shivendra Pratap]

On 11/11/2025 10:29 PM, Bjorn Andersson wrote:
> On Sun, Nov 09, 2025 at 08:07:22PM +0530, Shivendra Pratap wrote:
>> From: Elliot Berman <elliot.berman@oss.qualcomm.com>
>>
>> Add support for SYSTEM_RESET2 vendor-specific resets in
> 
> Please rewrite this to start with the purpose of the patch, introduce
> the fact that we can use SYSTEM_RESET2 to affect the reboot mode.
>
> Make sure you include the information that this isn't broadly available
> on all sc7280 devices.

ok.

> 
> Please line break at 72 characters, not less.

Ack. will keep note. thanks.

> 
>> qcs6490-rb3gen2 as reboot-modes.  Describe the resets:
> 
> This looks like the header for a list, that documents "bootloader" and
> "edl", but it's just one lump of text. Please improve the formatting. If
> it is a list, then make it look like a list.

Ack. will update this commit message.

thanks,
Shivendra

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Shivendra Pratap]

On 11/10/2025 10:52 PM, Lorenzo Pieralisi wrote:
> On Sun, Nov 09, 2025 at 08:07:20PM +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.
>>
>> Implement the PSCI vendor-specific resets by registering to the
>> reboot-mode framework.
> 
> I think that we should expose to user space _all_ PSCI reset types,
> cold, warm + vendor specific - as a departure from using the reboot_mode
> variable (and possibly deprecate it - or at least stop using it).

sure. We can try that. Have tried to compile it all at the end of this thread.

> 
>> As psci init is done at early kernel init, reboot-mode registration cannot
>> be done at the time of psci init.  This is because reboot-mode creates a
>> “reboot-mode” class for exposing sysfs, which can fail at early kernel init.
>> To overcome this, 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. Register a panic notifier and clear vendor_reset valid status
>> during panic.  This is needed for any kernel panic that occurs post
>> reboot_notifiers.
> 
> Is it because panic uses reboot_mode to determine the reset to issue ?

Yes. As we know, currently psci supports only two resets,
psci_sys_reset2 (ARCH warm reset) and psci_sys_reset(COLD RESET). And kernel
panic path should take the path set by reboot_mode to maintain backward
compatibility. 

> 
>> 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 under “reboot-mode” and are based on the
>> reboot-mode based commands.
> 
> IMHO - it would be nice if could add mode-cold (or mode-normal in reboot mode
> speak) and mode-warm by default (if PSCI supports them) so that userspace

Default mode in current kernel is cold, until explicitly set to warm.
So should it be defaulted to cold?

> could issue those resets too without having to set the reboot_mode variable.
> 
> Reason is, since we are doing this it is worth going the whole nine
> yards and try to decouple the reboot_mode variable from the RESTART2
> syscall argument.
> 
> Reworded: just use the new userspace interface you are adding for
> all PSCI reset types.
> 
> Thoughts very much welcome - I understand this is controversial.

We can remove the dependency on reboot_mode and include all supported
modes in a new psci_reset driver with some default modes (warm, cold)
and other vendor-specific resets which may be picked from device tree.
And then, reset based on the command being passed from userspace.

But yes some platforms that already reply on reboot_mode may break here
and will need to adjust to the new design being proposed.

Have summarized at the end of the thread.

> 
>> Reviewed-by: Umang Chheda <umang.chheda@oss.qualcomm.com>
>> Reviewed-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> Reviewed-by: Nirmesh Kumar Singh <nirmesh.singh@oss.qualcomm.com>
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
>>  drivers/firmware/psci/Kconfig |  2 +
>>  drivers/firmware/psci/psci.c  | 92 ++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 93 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..ff82e7f4c27d1609a75cedc3a9790affaf839801 100644
>> --- a/drivers/firmware/psci/psci.c
>> +++ b/drivers/firmware/psci/psci.c
>> @@ -8,15 +8,18 @@
>>  
>>  #include <linux/acpi.h>
>>  #include <linux/arm-smccc.h>
>> +#include <linux/bitops.h>
>>  #include <linux/cpuidle.h>
>>  #include <linux/debugfs.h>
>>  #include <linux/errno.h>
>>  #include <linux/linkage.h>
>>  #include <linux/of.h>
>> +#include <linux/panic_notifier.h>
>>  #include <linux/pm.h>
>>  #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 +54,24 @@ 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;
> 
> I think this should represent all possible PSCI reset types, not vendor only
> and its value is set by the reboot mode framework.
> 
>> +
>> +static int psci_panic_event(struct notifier_block *nb, unsigned long v, void *p)
>> +{
>> +	vendor_reset.valid = false;
> 
> I don't like this. Basically all you want this for is to make sure that
> we don't override the reboot_mode variable.

Yes, it does not look good but as we planned to use reboot-mode framework earlier, which
sets the modes at the at reboot_notifiers. This needs to be taken care for any panic
that occurs between reboot_notifier and restart_notifier.

> 
> One (hack) would consist in checking the reboot_mode variable here and
> set the struct I mentioned above to the value represented in reboot_mode.
> 
> Good luck if reboot_mode == REBOOT_GPIO :-)

psci supports only two modes, ARCH_WARM and cold, so anything else except WARM/SOFT
should default to cold? So even if REBOOT_GPIO is set in reboot_mode, we should default
it to cold reset.

> 
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block psci_panic_block = {
>> +	.notifier_call = psci_panic_event
>> +};
>> +
>>  bool psci_tos_resident_on(int cpu)
>>  {
>>  	return cpu == resident_cpu;
>> @@ -309,7 +330,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);
> 
> See above. Two calls here: one for resets issued using the new userspace
> interface you are adding and legacy below - no vendor vs reboot_mode, this
> is a mess.

Are we suggesting to completely remove the reboot_mode check from here in the new
design and base it on reboot <CMD> param?

> 
>> +	} else if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>>  	    psci_system_reset2_supported) {
>>  		/*
>>  		 * reset_type[31] = 0 (architectural)
>> @@ -547,6 +571,72 @@ 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 & GENMASK(31, 0);
>> +		vendor_reset.reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic_32;
>> +		vendor_reset.cookie = (magic >> 32) & GENMASK(31, 0);
> 
> Use FIELD_PREP/GET() please (but as mentioned above the vendor reset type
> bit[31] should be part of the reboot mode magic value, see above).

sure. Will align this. thanks.

> 
>> +		vendor_reset.valid = true;
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static int __init psci_init_vendor_reset(void)
>> +{
>> +	struct reboot_mode_driver *reboot;
>> +	struct device_node *psci_np;
>> +	struct device_node *np;
>> +	int ret;
>> +
>> +	if (!psci_system_reset2_supported)
>> +		return -EINVAL;
>> +
>> +	psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
>> +	if (!psci_np)
>> +		return -ENODEV;
>> +
>> +	np = of_find_node_by_name(psci_np, "reboot-mode");
>> +	if (!np) {
>> +		of_node_put(psci_np);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = atomic_notifier_chain_register(&panic_notifier_list, &psci_panic_block);
>> +	if (ret)
>> +		goto err_notifier;
>> +
>> +	reboot = kzalloc(sizeof(*reboot), GFP_KERNEL);
>> +	if (!reboot) {
>> +		ret = -ENOMEM;
>> +		goto err_kzalloc;
>> +	}
>> +
>> +	reboot->write = psci_set_vendor_sys_reset2;
>> +	reboot->driver_name = "psci";
>> +
>> +	ret = reboot_mode_register(reboot, of_fwnode_handle(np));
>> +	if (ret)
>> +		goto err_register;
>> +
>> +	of_node_put(psci_np);
>> +	of_node_put(np);
>> +	return 0;
>> +
>> +err_register:
>> +	kfree(reboot);
>> +err_kzalloc:
>> +	atomic_notifier_chain_unregister(&panic_notifier_list, &psci_panic_block);
>> +err_notifier:
>> +	of_node_put(psci_np);
>> +	of_node_put(np);
>> +	return ret;
>> +}
>> +late_initcall(psci_init_vendor_reset)
> 
> I don't like adding another initcall here.
> 
> I wonder whether this code belongs in a PSCI reboot mode driver, possibly a
> faux device in a way similar to what we did for cpuidle-psci (that after all
> is a consumer of PSCI_CPU_SUSPEND in a similar way as this code is a
> PSCI_SYSTEM_RESET{2} consumer), that communicates with
> drivers/firmware/psci/psci.c with the struct mentioned above.

sure. we can create a new driver and try it as in cpuidle: cpuidle-psci.
Can you suggest a bit more on the overall approach we want to take here?
Have tried to summarize the potential changes and few questions below.

- new driver registers a faux device - say - power: reset: psci_reset.
- struct with pre-built psci reset_types - (warm, soft, cold). Currently
  only two modes supported, anything other than warm/soft defaults to cold.
- vendor resets to be added as per vendor choice, inside psci device tree(SOC specific).
- psci_reset registers with reboot-mode for registering  vendor resets. Here, we
  have a problem, the pre-built psci reset_types - (warm, soft, cold) cannot be added via
  reboot-mode framework. Should the new psci_reset driver, move away from reboot-mode
  framework as-well? And define its own parsing logic for psci_reset_types, and have
  its own restart_notifier instead of reboot_notifier?
- If new psci_reset driver move away from reboot-mode, we can get rid of the panic_notifier
  added in the psci code. Else, we may still need the panic_notifier for any kernel panic
  that occurs between reboot_notifier and restart_notifier?
- psci driver will export a function which will be called externally to set the current
  psci reset_type.
- psci_sys_reset in psci driver should remove the check on reboot_mode. It will default to
  cold reset (for the reason the current kernel defaults to cold reset in psci.)
  example change in psci_sys_reset:
    if(psci_system_reset2_supported && <psci_reset_new_struct_var> != cold)
       psci_sys_reset2(AS PER PARAMS FROM new psci_reset driver)
    else
       psci_sys_reset(COLD RESET)

thanks,
Shivendra

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Lorenzo Pieralisi]

On Mon, Nov 17, 2025 at 11:14:48PM +0530, Shivendra Pratap wrote:
> 
> 
> On 11/10/2025 10:52 PM, Lorenzo Pieralisi wrote:
> > On Sun, Nov 09, 2025 at 08:07:20PM +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.
> >>
> >> Implement the PSCI vendor-specific resets by registering to the
> >> reboot-mode framework.
> > 
> > I think that we should expose to user space _all_ PSCI reset types,
> > cold, warm + vendor specific - as a departure from using the reboot_mode
> > variable (and possibly deprecate it - or at least stop using it).
> 
> sure. We can try that. Have tried to compile it all at the end of this thread.
> 
> > 
> >> As psci init is done at early kernel init, reboot-mode registration cannot
> >> be done at the time of psci init.  This is because reboot-mode creates a
> >> “reboot-mode” class for exposing sysfs, which can fail at early kernel init.
> >> To overcome this, 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. Register a panic notifier and clear vendor_reset valid status
> >> during panic.  This is needed for any kernel panic that occurs post
> >> reboot_notifiers.
> > 
> > Is it because panic uses reboot_mode to determine the reset to issue ?
> 
> Yes. As we know, currently psci supports only two resets,
> psci_sys_reset2 (ARCH warm reset) and psci_sys_reset(COLD RESET). And kernel
> panic path should take the path set by reboot_mode to maintain backward
> compatibility. 
> 
> > 
> >> 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 under “reboot-mode” and are based on the
> >> reboot-mode based commands.
> > 
> > IMHO - it would be nice if could add mode-cold (or mode-normal in reboot mode
> > speak) and mode-warm by default (if PSCI supports them) so that userspace
> 
> Default mode in current kernel is cold, until explicitly set to warm.
> So should it be defaulted to cold?

I managed to confuse you sorry. What I wanted to say is that user space
should be able to issue _all_ PSCI resets (inclusive of cold and warm if
supported - ie if SYSTEM_RESET2 is supported) not just vendor resets.

I misused "by default" - I meant cold and warm PSCI resets should be part
of the reboot-mode list.

[...]

> >>  
> >> +struct psci_vendor_sysreset2 {
> >> +	u32 reset_type;
> >> +	u32 cookie;
> >> +	bool valid;
> >> +};
> >> +
> >> +static struct psci_vendor_sysreset2 vendor_reset;
> > 
> > I think this should represent all possible PSCI reset types, not vendor only
> > and its value is set by the reboot mode framework.
> > 
> >> +
> >> +static int psci_panic_event(struct notifier_block *nb, unsigned long v, void *p)
> >> +{
> >> +	vendor_reset.valid = false;
> > 
> > I don't like this. Basically all you want this for is to make sure that
> > we don't override the reboot_mode variable.
> 
> Yes, it does not look good but as we planned to use reboot-mode framework earlier, which
> sets the modes at the at reboot_notifiers. This needs to be taken care for any panic
> that occurs between reboot_notifier and restart_notifier.

Isn't there a simpler way to detect whether we are in panic mode and
consequently we just issue a reset based on reboot_mode ?

panic_in_progress() ?

> > One (hack) would consist in checking the reboot_mode variable here and
> > set the struct I mentioned above to the value represented in reboot_mode.
> > 
> > Good luck if reboot_mode == REBOOT_GPIO :-)
> 
> psci supports only two modes, ARCH_WARM and cold, so anything else except WARM/SOFT
> should default to cold? So even if REBOOT_GPIO is set in reboot_mode, we should default
> it to cold reset.
> 
> > 
> >> +	return NOTIFY_DONE;
> >> +}
> >> +
> >> +static struct notifier_block psci_panic_block = {
> >> +	.notifier_call = psci_panic_event
> >> +};
> >> +
> >>  bool psci_tos_resident_on(int cpu)
> >>  {
> >>  	return cpu == resident_cpu;
> >> @@ -309,7 +330,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);
> > 
> > See above. Two calls here: one for resets issued using the new userspace
> > interface you are adding and legacy below - no vendor vs reboot_mode, this
> > is a mess.
> 
> Are we suggesting to completely remove the reboot_mode check from here in the new
> design and base it on reboot <CMD> param?

I am suggesting that there must be two reset options:

- based on reboot mode set by user space
- based on reboot_mode variable (as a fallback and while panic is in progress)

> > 
> >> +	} else if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
> >>  	    psci_system_reset2_supported) {
> >>  		/*
> >>  		 * reset_type[31] = 0 (architectural)
> >> @@ -547,6 +571,72 @@ 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 & GENMASK(31, 0);
> >> +		vendor_reset.reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic_32;
> >> +		vendor_reset.cookie = (magic >> 32) & GENMASK(31, 0);
> > 
> > Use FIELD_PREP/GET() please (but as mentioned above the vendor reset type
> > bit[31] should be part of the reboot mode magic value, see above).
> 
> sure. Will align this. thanks.
> 
> > 
> >> +		vendor_reset.valid = true;
> >> +	}
> >> +
> >> +	return NOTIFY_DONE;
> >> +}
> >> +
> >> +static int __init psci_init_vendor_reset(void)
> >> +{
> >> +	struct reboot_mode_driver *reboot;
> >> +	struct device_node *psci_np;
> >> +	struct device_node *np;
> >> +	int ret;
> >> +
> >> +	if (!psci_system_reset2_supported)
> >> +		return -EINVAL;
> >> +
> >> +	psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
> >> +	if (!psci_np)
> >> +		return -ENODEV;
> >> +
> >> +	np = of_find_node_by_name(psci_np, "reboot-mode");
> >> +	if (!np) {
> >> +		of_node_put(psci_np);
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	ret = atomic_notifier_chain_register(&panic_notifier_list, &psci_panic_block);
> >> +	if (ret)
> >> +		goto err_notifier;
> >> +
> >> +	reboot = kzalloc(sizeof(*reboot), GFP_KERNEL);
> >> +	if (!reboot) {
> >> +		ret = -ENOMEM;
> >> +		goto err_kzalloc;
> >> +	}
> >> +
> >> +	reboot->write = psci_set_vendor_sys_reset2;
> >> +	reboot->driver_name = "psci";
> >> +
> >> +	ret = reboot_mode_register(reboot, of_fwnode_handle(np));
> >> +	if (ret)
> >> +		goto err_register;
> >> +
> >> +	of_node_put(psci_np);
> >> +	of_node_put(np);
> >> +	return 0;
> >> +
> >> +err_register:
> >> +	kfree(reboot);
> >> +err_kzalloc:
> >> +	atomic_notifier_chain_unregister(&panic_notifier_list, &psci_panic_block);
> >> +err_notifier:
> >> +	of_node_put(psci_np);
> >> +	of_node_put(np);
> >> +	return ret;
> >> +}
> >> +late_initcall(psci_init_vendor_reset)
> > 
> > I don't like adding another initcall here.
> > 
> > I wonder whether this code belongs in a PSCI reboot mode driver, possibly a
> > faux device in a way similar to what we did for cpuidle-psci (that after all
> > is a consumer of PSCI_CPU_SUSPEND in a similar way as this code is a
> > PSCI_SYSTEM_RESET{2} consumer), that communicates with
> > drivers/firmware/psci/psci.c with the struct mentioned above.
> 
> sure. we can create a new driver and try it as in cpuidle: cpuidle-psci.
> Can you suggest a bit more on the overall approach we want to take here?
> Have tried to summarize the potential changes and few questions below.
> 
> - new driver registers a faux device - say - power: reset: psci_reset.

Yes this could be a potential way forward but that's decoupled from the
options below. If we take this route PSCI maintainers should be added
as maintainers for this reboot mode driver.

> - struct with pre-built psci reset_types - (warm, soft, cold). Currently
>   only two modes supported, anything other than warm/soft defaults to cold.
> - vendor resets to be added as per vendor choice, inside psci device tree(SOC specific).
> - psci_reset registers with reboot-mode for registering  vendor resets. Here, we
>   have a problem, the pre-built psci reset_types - (warm, soft, cold) cannot be added via
>   reboot-mode framework.

Why ?

>   Should the new psci_reset driver, move away from reboot-mode
>   framework as-well? And define its own parsing logic for psci_reset_types,
>   and have its own restart_notifier instead of reboot_notifier?

No. As I said earlier, I think it makes sense to allow user space to
select _all_ PSCI reset types - architected and vendor specific in
a single reboot mode driver.

I believe that we must be able to have two well defined ways for
issuing resets:

- one based on reboot mode driver
- one based on reboot_mode variable interface

Does this make sense everyone ? I don't know the history behind
reboot_mode and the reboot mode driver framework I am just stating
what I think makes sense to do for PSCI.

Thanks,
Lorenzo

> - If new psci_reset driver move away from reboot-mode, we can get rid of the panic_notifier
>   added in the psci code. Else, we may still need the panic_notifier for any kernel panic
>   that occurs between reboot_notifier and restart_notifier?
> - psci driver will export a function which will be called externally to set the current
>   psci reset_type.
> - psci_sys_reset in psci driver should remove the check on reboot_mode. It will default to
>   cold reset (for the reason the current kernel defaults to cold reset in psci.)
>   example change in psci_sys_reset:
>     if(psci_system_reset2_supported && <psci_reset_new_struct_var> != cold)
>        psci_sys_reset2(AS PER PARAMS FROM new psci_reset driver)
>     else
>        psci_sys_reset(COLD RESET)
> 
> thanks,
> Shivendra

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Shivendra Pratap]

On 11/18/2025 5:58 PM, Lorenzo Pieralisi wrote:
> On Mon, Nov 17, 2025 at 11:14:48PM +0530, Shivendra Pratap wrote:
>>
>>
>> On 11/10/2025 10:52 PM, Lorenzo Pieralisi wrote:
>>> On Sun, Nov 09, 2025 at 08:07:20PM +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.
>>>>
>>>> Implement the PSCI vendor-specific resets by registering to the
>>>> reboot-mode framework.
>>>
>>> I think that we should expose to user space _all_ PSCI reset types,
>>> cold, warm + vendor specific - as a departure from using the reboot_mode
>>> variable (and possibly deprecate it - or at least stop using it).
>>
>> sure. We can try that. Have tried to compile it all at the end of this thread.
>>
>>>
>>>> As psci init is done at early kernel init, reboot-mode registration cannot
>>>> be done at the time of psci init.  This is because reboot-mode creates a
>>>> “reboot-mode” class for exposing sysfs, which can fail at early kernel init.
>>>> To overcome this, 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. Register a panic notifier and clear vendor_reset valid status
>>>> during panic.  This is needed for any kernel panic that occurs post
>>>> reboot_notifiers.
>>>
>>> Is it because panic uses reboot_mode to determine the reset to issue ?
>>
>> Yes. As we know, currently psci supports only two resets,
>> psci_sys_reset2 (ARCH warm reset) and psci_sys_reset(COLD RESET). And kernel
>> panic path should take the path set by reboot_mode to maintain backward
>> compatibility. 
>>
>>>
>>>> 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 under “reboot-mode” and are based on the
>>>> reboot-mode based commands.
>>>
>>> IMHO - it would be nice if could add mode-cold (or mode-normal in reboot mode
>>> speak) and mode-warm by default (if PSCI supports them) so that userspace
>>
>> Default mode in current kernel is cold, until explicitly set to warm.
>> So should it be defaulted to cold?
> 
> I managed to confuse you sorry. What I wanted to say is that user space
> should be able to issue _all_ PSCI resets (inclusive of cold and warm if
> supported - ie if SYSTEM_RESET2 is supported) not just vendor resets.
> 
> I misused "by default" - I meant cold and warm PSCI resets should be part
> of the reboot-mode list.
> 
> [...]
> 
>>>>  
>>>> +struct psci_vendor_sysreset2 {
>>>> +	u32 reset_type;
>>>> +	u32 cookie;
>>>> +	bool valid;
>>>> +};
>>>> +
>>>> +static struct psci_vendor_sysreset2 vendor_reset;
>>>
>>> I think this should represent all possible PSCI reset types, not vendor only
>>> and its value is set by the reboot mode framework.
>>>
>>>> +
>>>> +static int psci_panic_event(struct notifier_block *nb, unsigned long v, void *p)
>>>> +{
>>>> +	vendor_reset.valid = false;
>>>
>>> I don't like this. Basically all you want this for is to make sure that
>>> we don't override the reboot_mode variable.
>>
>> Yes, it does not look good but as we planned to use reboot-mode framework earlier, which
>> sets the modes at the at reboot_notifiers. This needs to be taken care for any panic
>> that occurs between reboot_notifier and restart_notifier.
> 
> Isn't there a simpler way to detect whether we are in panic mode and
> consequently we just issue a reset based on reboot_mode ?
> 
> panic_in_progress() ?

Yes. panic_in_progress is added in 6.18.rc1. We can use that. Thanks. Seems i was
outdated.

> 
>>> One (hack) would consist in checking the reboot_mode variable here and
>>> set the struct I mentioned above to the value represented in reboot_mode.
>>>
>>> Good luck if reboot_mode == REBOOT_GPIO :-)
>>
>> psci supports only two modes, ARCH_WARM and cold, so anything else except WARM/SOFT
>> should default to cold? So even if REBOOT_GPIO is set in reboot_mode, we should default
>> it to cold reset.
>>
>>>
>>>> +	return NOTIFY_DONE;
>>>> +}
>>>> +
>>>> +static struct notifier_block psci_panic_block = {
>>>> +	.notifier_call = psci_panic_event
>>>> +};
>>>> +
>>>>  bool psci_tos_resident_on(int cpu)
>>>>  {
>>>>  	return cpu == resident_cpu;
>>>> @@ -309,7 +330,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);
>>>
>>> See above. Two calls here: one for resets issued using the new userspace
>>> interface you are adding and legacy below - no vendor vs reboot_mode, this
>>> is a mess.
>>
>> Are we suggesting to completely remove the reboot_mode check from here in the new
>> design and base it on reboot <CMD> param?
> 
> I am suggesting that there must be two reset options:
> 
> - based on reboot mode set by user space
> - based on reboot_mode variable (as a fallback and while panic is in progress)
> 
>>>
>>>> +	} else if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>>>>  	    psci_system_reset2_supported) {
>>>>  		/*
>>>>  		 * reset_type[31] = 0 (architectural)
>>>> @@ -547,6 +571,72 @@ 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 & GENMASK(31, 0);
>>>> +		vendor_reset.reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic_32;
>>>> +		vendor_reset.cookie = (magic >> 32) & GENMASK(31, 0);
>>>
>>> Use FIELD_PREP/GET() please (but as mentioned above the vendor reset type
>>> bit[31] should be part of the reboot mode magic value, see above).
>>
>> sure. Will align this. thanks.
>>
>>>
>>>> +		vendor_reset.valid = true;
>>>> +	}
>>>> +
>>>> +	return NOTIFY_DONE;
>>>> +}
>>>> +
>>>> +static int __init psci_init_vendor_reset(void)
>>>> +{
>>>> +	struct reboot_mode_driver *reboot;
>>>> +	struct device_node *psci_np;
>>>> +	struct device_node *np;
>>>> +	int ret;
>>>> +
>>>> +	if (!psci_system_reset2_supported)
>>>> +		return -EINVAL;
>>>> +
>>>> +	psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
>>>> +	if (!psci_np)
>>>> +		return -ENODEV;
>>>> +
>>>> +	np = of_find_node_by_name(psci_np, "reboot-mode");
>>>> +	if (!np) {
>>>> +		of_node_put(psci_np);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	ret = atomic_notifier_chain_register(&panic_notifier_list, &psci_panic_block);
>>>> +	if (ret)
>>>> +		goto err_notifier;
>>>> +
>>>> +	reboot = kzalloc(sizeof(*reboot), GFP_KERNEL);
>>>> +	if (!reboot) {
>>>> +		ret = -ENOMEM;
>>>> +		goto err_kzalloc;
>>>> +	}
>>>> +
>>>> +	reboot->write = psci_set_vendor_sys_reset2;
>>>> +	reboot->driver_name = "psci";
>>>> +
>>>> +	ret = reboot_mode_register(reboot, of_fwnode_handle(np));
>>>> +	if (ret)
>>>> +		goto err_register;
>>>> +
>>>> +	of_node_put(psci_np);
>>>> +	of_node_put(np);
>>>> +	return 0;
>>>> +
>>>> +err_register:
>>>> +	kfree(reboot);
>>>> +err_kzalloc:
>>>> +	atomic_notifier_chain_unregister(&panic_notifier_list, &psci_panic_block);
>>>> +err_notifier:
>>>> +	of_node_put(psci_np);
>>>> +	of_node_put(np);
>>>> +	return ret;
>>>> +}
>>>> +late_initcall(psci_init_vendor_reset)
>>>
>>> I don't like adding another initcall here.
>>>
>>> I wonder whether this code belongs in a PSCI reboot mode driver, possibly a
>>> faux device in a way similar to what we did for cpuidle-psci (that after all
>>> is a consumer of PSCI_CPU_SUSPEND in a similar way as this code is a
>>> PSCI_SYSTEM_RESET{2} consumer), that communicates with
>>> drivers/firmware/psci/psci.c with the struct mentioned above.
>>
>> sure. we can create a new driver and try it as in cpuidle: cpuidle-psci.
>> Can you suggest a bit more on the overall approach we want to take here?
>> Have tried to summarize the potential changes and few questions below.
>>
>> - new driver registers a faux device - say - power: reset: psci_reset.
> 
> Yes this could be a potential way forward but that's decoupled from the
> options below. If we take this route PSCI maintainers should be added
> as maintainers for this reboot mode driver.

you mean the new psci_reset driver? yes. Maintainer would be PSCI maintainer,
if we create a new  psci_reset reboot mode driver.

> 
>> - struct with pre-built psci reset_types - (warm, soft, cold). Currently
>>   only two modes supported, anything other than warm/soft defaults to cold.
>> - vendor resets to be added as per vendor choice, inside psci device tree(SOC specific).
>> - psci_reset registers with reboot-mode for registering  vendor resets. Here, we
>>   have a problem, the pre-built psci reset_types - (warm, soft, cold) cannot be added via
>>   reboot-mode framework.
> 
> Why ?

If we want the new psci_reset to take the reboot-mode framework route, is it ok to
add default modes (warm, cold) in the device tree?
If not, then the design of reboot-mode framework(power:reset:reboot-mode.c) needs to be
further changed to equip this new feature. 

If new psci_reset driver move away from reboot-mode framework(power:reset:reboot-mode.c), the driver
can have its own design, its own sysfs interface and maintained under psci Maintainer.

> 
>>   Should the new psci_reset driver, move away from reboot-mode
>>   framework as-well? And define its own parsing logic for psci_reset_types,
>>   and have its own restart_notifier instead of reboot_notifier?
> 
> No. As I said earlier, I think it makes sense to allow user space to
> select _all_ PSCI reset types - architected and vendor specific in
> a single reboot mode driver.
> 
> I believe that we must be able to have two well defined ways for
> issuing resets:
> 
> - one based on reboot mode driver
> - one based on reboot_mode variable interface

So may be in more details-
user space issues - reboot cold
   -> go for psci_reset (as psci_sysrest2 does not has cold reset?)
user space issues - reboot warm or a vendor_reset
   -> if psci_sysreset2 is supported - call psci_sysreset2 with required params.
   ->   else
   ->  go for psci_reset COLD

user space issues - reboot (no commands) or a panic_in_progress
   -> fallback to reboot_mode 
   ->  if (reboot_mode == WARM and psci_sysreset2 is supported )
   ->     call psci_sysreset2 (ARCH WARM RESET)
   ->  else
   ->     go for psci_reset COLD


And we want to do this in two conditional statements in firmware:psci: psci_sys_reset
function?
Or am i not getting the point here?

thanks,
Shivendra

> 
> Does this make sense everyone ? I don't know the history behind
> reboot_mode and the reboot mode driver framework I am just stating
> what I think makes sense to do for PSCI.
> 
> Thanks,
> Lorenzo
> 
>> - If new psci_reset driver move away from reboot-mode, we can get rid of the panic_notifier
>>   added in the psci code. Else, we may still need the panic_notifier for any kernel panic
>>   that occurs between reboot_notifier and restart_notifier?
>> - psci driver will export a function which will be called externally to set the current
>>   psci reset_type.
>> - psci_sys_reset in psci driver should remove the check on reboot_mode. It will default to
>>   cold reset (for the reason the current kernel defaults to cold reset in psci.)
>>   example change in psci_sys_reset:
>>     if(psci_system_reset2_supported && <psci_reset_new_struct_var> != cold)
>>        psci_sys_reset2(AS PER PARAMS FROM new psci_reset driver)
>>     else
>>        psci_sys_reset(COLD RESET)
>>
>> thanks,
>> Shivendra

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Lorenzo Pieralisi]

On Tue, Nov 18, 2025 at 11:11:33PM +0530, Shivendra Pratap wrote:

[...]

> > Yes this could be a potential way forward but that's decoupled from the
> > options below. If we take this route PSCI maintainers should be added
> > as maintainers for this reboot mode driver.
> 
> you mean the new psci_reset driver? yes. Maintainer would be PSCI maintainer,
> if we create a new  psci_reset reboot mode driver.

Yes.

> >> - struct with pre-built psci reset_types - (warm, soft, cold). Currently
> >>   only two modes supported, anything other than warm/soft defaults to cold.
> >> - vendor resets to be added as per vendor choice, inside psci device tree(SOC specific).
> >> - psci_reset registers with reboot-mode for registering  vendor resets. Here, we
> >>   have a problem, the pre-built psci reset_types - (warm, soft, cold) cannot be added via
> >>   reboot-mode framework.
> > 
> > Why ?
> 
> If we want the new psci_reset to take the reboot-mode framework route, is it ok to
> add default modes (warm, cold) in the device tree?
> If not, then the design of reboot-mode framework(power:reset:reboot-mode.c) needs to be
> further changed to equip this new feature. 

Well, yes, all it needs to do is allowing prepopulated reboot modes on top
of which DT based ones are added.

I don't see any point in adding properties to the DT node to provide
information we can already probe.

> If new psci_reset driver move away from reboot-mode framework(power:reset:reboot-mode.c), the driver
> can have its own design, its own sysfs interface and maintained under psci Maintainer.
> 
> > 
> >>   Should the new psci_reset driver, move away from reboot-mode
> >>   framework as-well? And define its own parsing logic for psci_reset_types,
> >>   and have its own restart_notifier instead of reboot_notifier?
> > 
> > No. As I said earlier, I think it makes sense to allow user space to
> > select _all_ PSCI reset types - architected and vendor specific in
> > a single reboot mode driver.
> > 
> > I believe that we must be able to have two well defined ways for
> > issuing resets:
> > 
> > - one based on reboot mode driver
> > - one based on reboot_mode variable interface
> 
> So may be in more details-
> user space issues - reboot cold
>    -> go for psci_reset (as psci_sysrest2 does not has cold reset?)
> user space issues - reboot warm or a vendor_reset
>    -> if psci_sysreset2 is supported - call psci_sysreset2 with required params.
>    ->   else
>    ->  go for psci_reset COLD
> 
> user space issues - reboot (no commands) or a panic_in_progress
>    -> fallback to reboot_mode 
>    ->  if (reboot_mode == WARM and psci_sysreset2 is supported )
>    ->     call psci_sysreset2 (ARCH WARM RESET)
>    ->  else
>    ->     go for psci_reset COLD
> 
> 
> And we want to do this in two conditional statements in firmware:psci: psci_sys_reset
> function?
> Or am i not getting the point here?

You are getting the point.

Thanks,
Lorenzo

> thanks,
> Shivendra
> 
> > 
> > Does this make sense everyone ? I don't know the history behind
> > reboot_mode and the reboot mode driver framework I am just stating
> > what I think makes sense to do for PSCI.
> > 
> > Thanks,
> > Lorenzo
> > 
> >> - If new psci_reset driver move away from reboot-mode, we can get rid of the panic_notifier
> >>   added in the psci code. Else, we may still need the panic_notifier for any kernel panic
> >>   that occurs between reboot_notifier and restart_notifier?
> >> - psci driver will export a function which will be called externally to set the current
> >>   psci reset_type.
> >> - psci_sys_reset in psci driver should remove the check on reboot_mode. It will default to
> >>   cold reset (for the reason the current kernel defaults to cold reset in psci.)
> >>   example change in psci_sys_reset:
> >>     if(psci_system_reset2_supported && <psci_reset_new_struct_var> != cold)
> >>        psci_sys_reset2(AS PER PARAMS FROM new psci_reset driver)
> >>     else
> >>        psci_sys_reset(COLD RESET)
> >>
> >> thanks,
> >> Shivendra

Re: [PATCH v17 07/12] firmware: psci: Implement vendor-specific resets as reboot-mode

[Shivendra Pratap]

On 11/19/2025 3:07 PM, Lorenzo Pieralisi wrote:
> On Tue, Nov 18, 2025 at 11:11:33PM +0530, Shivendra Pratap wrote:
> 
> [...]
> 
>>> Yes this could be a potential way forward but that's decoupled from the
>>> options below. If we take this route PSCI maintainers should be added
>>> as maintainers for this reboot mode driver.
>>
>> you mean the new psci_reset driver? yes. Maintainer would be PSCI maintainer,
>> if we create a new  psci_reset reboot mode driver.
> 
> Yes.
> 
>>>> - struct with pre-built psci reset_types - (warm, soft, cold). Currently
>>>>   only two modes supported, anything other than warm/soft defaults to cold.
>>>> - vendor resets to be added as per vendor choice, inside psci device tree(SOC specific).
>>>> - psci_reset registers with reboot-mode for registering  vendor resets. Here, we
>>>>   have a problem, the pre-built psci reset_types - (warm, soft, cold) cannot be added via
>>>>   reboot-mode framework.
>>>
>>> Why ?
>>
>> If we want the new psci_reset to take the reboot-mode framework route, is it ok to
>> add default modes (warm, cold) in the device tree?
>> If not, then the design of reboot-mode framework(power:reset:reboot-mode.c) needs to be
>> further changed to equip this new feature. 
> 
> Well, yes, all it needs to do is allowing prepopulated reboot modes on top
> of which DT based ones are added.

The mode-cold , adds a third variable to reboot-modes as the first parameter for 
invoke_psci_fn is different for cold vs warm/vendor.

cold reset call       : invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
vendor/warm reset call: invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), vendor, cookiee, 0);

Each mode will have 3 argument - like:
_ _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _ _ _ 
MODE   , cold reset, reset_type, cookie
_ _ _ _ _ _ _ _ _ _ _ _ _  _ _ _ _ _ _ - 
COLD   ,   1       ,    0      ,     0
WARM   ,   0       ,    0      ,     0
vendor1,   0       ,0x80000000 ,     1
vendor2,   0       ,0x80000010 ,     0

So reboot-mode framework will now define and support upto three 32 bit arguments for each mode?

thanks,
Shivendra