[PATCH v8 0/6] Implement vendor resets for PSCI SYSTEM_RESET2

[PATCH v8 0/6] Implement vendor resets for PSCI SYSTEM_RESET2

[Elliot Berman]

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

Setting up reboot on Qualcomm devices can be inconsistent from chipset
to chipset. Generally, there is a PMIC register that gets written to
decide the reboot type. There is also sometimes a cookie that can be
written to indicate that the bootloader should behave differently than a
regular boot. These knobs evolve over product generations and require 
more drivers. Qualcomm firmwares are beginning to expose vendor
SYSTEM_RESET2 types to simplify driver requirements from Linux.

Add support in PSCI to statically wire reboot mode commands from
userspace to a vendor reset and cookie value using the device tree. The
DT bindings are similar to reboot mode framework except that 2
integers are accepted (the type and cookie). Also, reboot mode framework
is intended to program the cookies, but not actually reboot the host.
PSCI SYSTEM_RESET2 does both. I've not added support for reading ACPI
tables since I don't have any device which provides them + firmware that
supports vendor SYSTEM_RESET2 types.

Lorenzo and I are also looking for some feedback on whether it is safe
to perform a vendor SYSTEM_RESET2 irrespective of the enum reboot_mode:

https://lore.kernel.org/all/Zw5ffeYW5uRpsaG3@lpieralisi/

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>

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 (6):
      dt-bindings: power: reset: Convert mode-.* properties to array
      dt-bindings: arm: Document reboot mode magic
      firmware: psci: Read and use vendor reset types
      arm64: dts: qcom: qcm6490-idp: Add PSCI SYSTEM_RESET2 types
      arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types
      arm64: dts: qcom: sa8775p-ride: Add PSCI SYSTEM_RESET2 types

 Documentation/devicetree/bindings/arm/psci.yaml    |  43 +++++++++
 .../bindings/power/reset/nvmem-reboot-mode.yaml    |   4 +
 .../devicetree/bindings/power/reset/qcom,pon.yaml  |   7 ++
 .../bindings/power/reset/reboot-mode.yaml          |   4 +-
 .../bindings/power/reset/syscon-reboot-mode.yaml   |   4 +
 arch/arm64/boot/dts/qcom/qcm6490-idp.dts           |   7 ++
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |   7 ++
 arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi         |   7 ++
 arch/arm64/boot/dts/qcom/sa8775p.dtsi              |   2 +-
 arch/arm64/boot/dts/qcom/sc7280.dtsi               |   2 +-
 drivers/firmware/psci/psci.c                       | 104 +++++++++++++++++++++
 11 files changed, 187 insertions(+), 4 deletions(-)
---
base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
change-id: 20231016-arm-psci-system_reset2-vendor-reboots-cc3ad456c070

Best regards,
-- 
Elliot Berman <quic_eberman@quicinc.com>

[PATCH v8 1/6] dt-bindings: power: reset: Convert mode-.* properties to array

[Elliot Berman]

PSCI reboot mode will map a mode name to multiple magic values instead
of just one. Convert the mode-.* property to an array. Users of the
reboot-mode schema will need to specify the maxItems of the mode-.*
properties. Existing users will all be 1.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 .../devicetree/bindings/power/reset/nvmem-reboot-mode.yaml         | 4 ++++
 Documentation/devicetree/bindings/power/reset/qcom,pon.yaml        | 7 +++++++
 Documentation/devicetree/bindings/power/reset/reboot-mode.yaml     | 4 ++--
 .../devicetree/bindings/power/reset/syscon-reboot-mode.yaml        | 4 ++++
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
index 627f8a6078c299e32e4bc7597509a6ef52d119d7..7f5f94673e9c248302bbd70378a19f0d25906b7c 100644
--- a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
+++ b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
@@ -31,6 +31,10 @@ properties:
 allOf:
   - $ref: reboot-mode.yaml#
 
+patternProperties:
+  "^mode-.*$":
+    maxItems: 1
+
 required:
   - compatible
   - nvmem-cells
diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
index fc8105a7b9b268df5cb08ad32cde26c50ea955ce..3da3d02a669089af037f9492c64e0304c714b523 100644
--- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
+++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
@@ -54,6 +54,10 @@ required:
   - compatible
   - reg
 
+patternProperties:
+  "^mode-.*$":
+    maxItems: 1
+
 unevaluatedProperties: false
 
 allOf:
@@ -75,6 +79,9 @@ allOf:
         reg-names:
           items:
             - const: pon
+    else:
+      patternProperties:
+        "^mode-.*$": false
 
     # Special case for pm8941, which doesn't store reset mode
   - if:
diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml
index ad0a0b95cec1267c8e479556aebf99cca653a2d9..3ddac06cec7277789b066d8426ea77d293298fac 100644
--- a/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml
+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml
@@ -28,13 +28,13 @@ description: |
 
 properties:
   mode-normal:
-    $ref: /schemas/types.yaml#/definitions/uint32
+    $ref: /schemas/types.yaml#/definitions/uint32-array
     description:
       Default value to set on a reboot if no command was provided.
 
 patternProperties:
   "^mode-.*$":
-    $ref: /schemas/types.yaml#/definitions/uint32
+    $ref: /schemas/types.yaml#/definitions/uint32-array
 
 additionalProperties: true
 
diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
index b6acff199cdecea08c1243ed5e8ad71240d65e9a..79ffc78b23eaf913dcb43964d083b78befe76890 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
@@ -32,6 +32,10 @@ properties:
 allOf:
   - $ref: reboot-mode.yaml#
 
+patternProperties:
+  "^mode-.*$":
+    maxItems: 1
+
 unevaluatedProperties: false
 
 required:

-- 
2.34.1

[PATCH v8 2/6] dt-bindings: arm: Document reboot mode magic

[Elliot Berman]

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 <quic_eberman@quicinc.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 cbb012e217ab80c1ca88e611e7acc06c6d56fad0..5e07c62fe5d74a8d001f6b8a8c9e777f43f3113f 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
 
+  reset-types:
+    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:
+        reset-types: false
 
 additionalProperties: false
 
@@ -261,4 +291,17 @@ examples:
         domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
       };
     };
+
+  - |+
+
+    // Case 5: SYSTEM_RESET2 vendor resets
+    psci {
+      compatible = "arm,psci-1.0";
+      method = "smc";
+
+      reset-types {
+        mode-edl = <0>;
+        mode-bootloader = <1 2>;
+      };
+    };
 ...

-- 
2.34.1

[PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Elliot Berman]

SoC vendors have different types of resets and are controlled through
various registers. For instance, Qualcomm chipsets can reboot to a
"download mode" that allows a RAM dump to be collected. Another example
is they also support writing a cookie that can be read by bootloader
during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
vendor reset types to be implemented without requiring drivers for every
register/cookie.

Add support in PSCI to statically map reboot mode commands from
userspace to a vendor reset and cookie value using the device tree.

A separate initcall is needed to parse the devicetree, instead of using
psci_dt_init because mm isn't sufficiently set up to allocate memory.

Reboot mode framework is close but doesn't quite fit with the
design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
be solved but doesn't seem reasonable in sum:
 1. reboot mode registers against the reboot_notifier_list, which is too
    early to call SYSTEM_RESET2. PSCI would need to remember the reset
    type from the reboot-mode framework callback and use it
    psci_sys_reset.
 2. reboot mode assumes only one cookie/parameter is described in the
    device tree. SYSTEM_RESET2 uses 2: one for the type and one for
    cookie.
 3. psci cpuidle driver already registers a driver against the
    arm,psci-1.0 compatible. Refactoring would be needed to have both a
    cpuidle and reboot-mode driver.

Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
 static u32 psci_cpu_suspend_feature;
 static bool psci_system_reset2_supported;
 
+struct psci_reset_param {
+	const char *mode;
+	u32 reset_type;
+	u32 cookie;
+};
+static struct psci_reset_param *psci_reset_params __ro_after_init;
+static size_t num_psci_reset_params __ro_after_init;
+
 static inline bool psci_has_ext_power_state(void)
 {
 	return psci_cpu_suspend_feature &
@@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np)
 	return 0;
 }
 
+static void psci_vendor_system_reset2(const char *cmd)
+{
+	unsigned long ret;
+	size_t i;
+
+	for (i = 0; i < num_psci_reset_params; i++) {
+		if (!strcmp(psci_reset_params[i].mode, cmd)) {
+			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
+					     psci_reset_params[i].reset_type,
+					     psci_reset_params[i].cookie, 0);
+			/*
+			 * if vendor reset fails, log it and fall back to
+			 * architecture reset types
+			 */
+			pr_err("failed to perform reset \"%s\": %ld\n", cmd,
+			       (long)ret);
+			return;
+		}
+	}
+}
+
 static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
 			  void *data)
 {
+	/*
+	 * try to do the vendor system_reset2
+	 * If the reset fails or there wasn't a match on the command,
+	 * fall back to architectural resets
+	 */
+	if (data && num_psci_reset_params)
+		psci_vendor_system_reset2(data);
+
 	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
 	    psci_system_reset2_supported) {
 		/*
@@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = {
 	{},
 };
 
+#define REBOOT_PREFIX "mode-"
+
+static int __init psci_init_system_reset2_modes(void)
+{
+	const size_t len = strlen(REBOOT_PREFIX);
+	struct psci_reset_param *param;
+	struct device_node *psci_np __free(device_node) = NULL;
+	struct device_node *np __free(device_node) = NULL;
+	struct property *prop;
+	size_t count = 0;
+	u32 magic[2];
+	int num;
+
+	if (!psci_system_reset2_supported)
+		return 0;
+
+	psci_np = of_find_matching_node(NULL, psci_of_match);
+	if (!psci_np)
+		return 0;
+
+	np = of_find_node_by_name(psci_np, "reset-types");
+	if (!np)
+		return 0;
+
+	for_each_property_of_node(np, prop) {
+		if (strncmp(prop->name, REBOOT_PREFIX, len))
+			continue;
+		num = of_property_count_u32_elems(np, prop->name);
+		if (num != 1 && num != 2)
+			continue;
+
+		count++;
+	}
+
+	param = psci_reset_params =
+		kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL);
+	if (!psci_reset_params)
+		return -ENOMEM;
+
+	for_each_property_of_node(np, prop) {
+		if (strncmp(prop->name, REBOOT_PREFIX, len))
+			continue;
+
+		param->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
+		if (!param->mode)
+			continue;
+
+		num = of_property_read_variable_u32_array(np, prop->name, magic,
+							  1, ARRAY_SIZE(magic));
+		if (num < 0) {
+			pr_warn("Failed to parse vendor reboot mode %s\n",
+				param->mode);
+			kfree_const(param->mode);
+			continue;
+		}
+
+		/* Force reset type to be in vendor space */
+		param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0];
+		param->cookie = num > 1 ? magic[1] : 0;
+		param++;
+		num_psci_reset_params++;
+	}
+
+	return 0;
+}
+arch_initcall(psci_init_system_reset2_modes);
+
 int __init psci_dt_init(void)
 {
 	struct device_node *np;

-- 
2.34.1

[PATCH v8 4/6] arm64: dts: qcom: qcm6490-idp: Add PSCI SYSTEM_RESET2 types

[Elliot Berman]

qcm6490-idp firmware supports vendor-defined SYSTEM_RESET2 types.
Describe the reset types: "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.

Co-developed-by: Shivendra Pratap <quic_spratap@quicinc.com>
Signed-off-by: Shivendra Pratap <quic_spratap@quicinc.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.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 a0668f767e4bf9c8a749adf180dc65f785eb389e..9c141244a7b23cff4c1e85b952c28208b2ad40b7 100644
--- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
+++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
@@ -617,6 +617,13 @@ &pon_resin {
 	status = "okay";
 };
 
+&psci {
+	reset-types {
+		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 3d8410683402fd4c03c5c2951721938fff20fc77..5360d0e51a65874be86c0a6ad7bed612bcb44e81 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -850,7 +850,7 @@ pmu {
 		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
 	};
 
-	psci {
+	psci: psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
 

-- 
2.34.1

[PATCH v8 5/6] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Elliot Berman]

qcs6490-rb3gen2 firmware supports vendor-defined SYSTEM_RESET2 types.
Describe the reset types: "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 <quic_eberman@quicinc.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 0d45662b8028bff475024cff37c33e01d2ee251b..67d453c5b089aadb7b6f965e6ff7a29290963079 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -688,6 +688,13 @@ &pmk8350_rtc {
 	status = "okay";
 };
 
+&psci {
+	reset-types {
+		mode-bootloader = <0x10001 0x2>;
+		mode-edl = <0 0x1>;
+	};
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };

-- 
2.34.1

[PATCH v8 6/6] arm64: dts: qcom: sa8775p-ride: Add PSCI SYSTEM_RESET2 types

[Elliot Berman]

sa8775p-ride firmware supports vendor-defined SYSTEM_RESET2 types.
Describe the reset types: "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.

Co-developed-by: Shivendra Pratap <quic_spratap@quicinc.com>
Signed-off-by: Shivendra Pratap <quic_spratap@quicinc.com>
Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 7 +++++++
 arch/arm64/boot/dts/qcom/sa8775p.dtsi      | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
index 2a6170623ea95ad34625b7eb3b729a3e1018f99a..9e8cc21873338f5aaf289df0acde6576d425d6e5 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
@@ -498,6 +498,13 @@ &pmm8654au_3_gpios {
 			  "GNSS_BOOT_MODE";
 };
 
+&psci {
+	reset-types {
+		mode-bootloader = <0x10001 0x2>;
+		mode-edl = <0 0x1>;
+	};
+};
+
 &qupv3_id_1 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 23f1b2e5e62471396d8dd5eaf5ecb23e01a5e458..dd36eea80f7c5ca39ae7bd5dec7f469d4f69775f 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -329,7 +329,7 @@ pmu {
 		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
-	psci {
+	psci: psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
 	};

-- 
2.34.1

Re: [PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Stephen Boyd]

Quoting Elliot Berman (2024-11-07 15:38:27)
> SoC vendors have different types of resets and are controlled through
> various registers. For instance, Qualcomm chipsets can reboot to a
> "download mode" that allows a RAM dump to be collected. Another example
> is they also support writing a cookie that can be read by bootloader
> during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> vendor reset types to be implemented without requiring drivers for every
> register/cookie.
>
> Add support in PSCI to statically map reboot mode commands from
> userspace to a vendor reset and cookie value using the device tree.
>
> A separate initcall is needed to parse the devicetree, instead of using
> psci_dt_init because mm isn't sufficiently set up to allocate memory.
>
> Reboot mode framework is close but doesn't quite fit with the
> design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> be solved but doesn't seem reasonable in sum:
>  1. reboot mode registers against the reboot_notifier_list, which is too
>     early to call SYSTEM_RESET2. PSCI would need to remember the reset
>     type from the reboot-mode framework callback and use it
>     psci_sys_reset.
>  2. reboot mode assumes only one cookie/parameter is described in the
>     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
>     cookie.
>  3. psci cpuidle driver already registers a driver against the
>     arm,psci-1.0 compatible. Refactoring would be needed to have both a
>     cpuidle and reboot-mode driver.
>
> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v8 5/6] arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types

[Konrad Dybcio]

On 8.11.2024 12:38 AM, Elliot Berman wrote:
> qcs6490-rb3gen2 firmware supports vendor-defined SYSTEM_RESET2 types.
> Describe the reset types: "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 <quic_eberman@quicinc.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v8 6/6] arm64: dts: qcom: sa8775p-ride: Add PSCI SYSTEM_RESET2 types

[Konrad Dybcio]

On 8.11.2024 12:38 AM, Elliot Berman wrote:
> sa8775p-ride firmware supports vendor-defined SYSTEM_RESET2 types.
> Describe the reset types: "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.
> 
> Co-developed-by: Shivendra Pratap <quic_spratap@quicinc.com>
> Signed-off-by: Shivendra Pratap <quic_spratap@quicinc.com>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: (subset) [PATCH v8 0/6] Implement vendor resets for PSCI SYSTEM_RESET2

[Sebastian Reichel]

On Thu, 07 Nov 2024 15:38:24 -0800, Elliot Berman wrote:
> The PSCI SYSTEM_RESET2 call allows vendor firmware to define additional
> reset types which could be mapped to the reboot argument.
> 
> Setting up reboot on Qualcomm devices can be inconsistent from chipset
> to chipset. Generally, there is a PMIC register that gets written to
> decide the reboot type. There is also sometimes a cookie that can be
> written to indicate that the bootloader should behave differently than a
> regular boot. These knobs evolve over product generations and require
> more drivers. Qualcomm firmwares are beginning to expose vendor
> SYSTEM_RESET2 types to simplify driver requirements from Linux.
> 
> [...]

Applied, thanks!

[1/6] dt-bindings: power: reset: Convert mode-.* properties to array
      commit: 05d9044177c3e910921522e0209640d3b825a6ae

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: [PATCH v8 0/6] Implement vendor resets for PSCI SYSTEM_RESET2

[Elliot Berman]

Hi Lorenzo,

I haven't seen response on our question about how reboot_mode enum and
reboot command string are supposed to interact. Let's make a call
ourselves and merge?

Thanks,
Elliot

On Thu, Nov 07, 2024 at 03:38:24PM -0800, Elliot Berman wrote:
> The PSCI SYSTEM_RESET2 call allows vendor firmware to define additional
> reset types which could be mapped to the reboot argument.
> 
> Setting up reboot on Qualcomm devices can be inconsistent from chipset
> to chipset. Generally, there is a PMIC register that gets written to
> decide the reboot type. There is also sometimes a cookie that can be
> written to indicate that the bootloader should behave differently than a
> regular boot. These knobs evolve over product generations and require 
> more drivers. Qualcomm firmwares are beginning to expose vendor
> SYSTEM_RESET2 types to simplify driver requirements from Linux.
> 
> Add support in PSCI to statically wire reboot mode commands from
> userspace to a vendor reset and cookie value using the device tree. The
> DT bindings are similar to reboot mode framework except that 2
> integers are accepted (the type and cookie). Also, reboot mode framework
> is intended to program the cookies, but not actually reboot the host.
> PSCI SYSTEM_RESET2 does both. I've not added support for reading ACPI
> tables since I don't have any device which provides them + firmware that
> supports vendor SYSTEM_RESET2 types.
> 
> Lorenzo and I are also looking for some feedback on whether it is safe
> to perform a vendor SYSTEM_RESET2 irrespective of the enum reboot_mode:
> 
> https://lore.kernel.org/all/Zw5ffeYW5uRpsaG3@lpieralisi/
> 
> 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>
> 
> 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 (6):
>       dt-bindings: power: reset: Convert mode-.* properties to array
>       dt-bindings: arm: Document reboot mode magic
>       firmware: psci: Read and use vendor reset types
>       arm64: dts: qcom: qcm6490-idp: Add PSCI SYSTEM_RESET2 types
>       arm64: dts: qcom: qcs6490-rb3gen2: Add PSCI SYSTEM_RESET2 types
>       arm64: dts: qcom: sa8775p-ride: Add PSCI SYSTEM_RESET2 types
> 
>  Documentation/devicetree/bindings/arm/psci.yaml    |  43 +++++++++
>  .../bindings/power/reset/nvmem-reboot-mode.yaml    |   4 +
>  .../devicetree/bindings/power/reset/qcom,pon.yaml  |   7 ++
>  .../bindings/power/reset/reboot-mode.yaml          |   4 +-
>  .../bindings/power/reset/syscon-reboot-mode.yaml   |   4 +
>  arch/arm64/boot/dts/qcom/qcm6490-idp.dts           |   7 ++
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |   7 ++
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi         |   7 ++
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi              |   2 +-
>  arch/arm64/boot/dts/qcom/sc7280.dtsi               |   2 +-
>  drivers/firmware/psci/psci.c                       | 104 +++++++++++++++++++++
>  11 files changed, 187 insertions(+), 4 deletions(-)
> ---
> base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
> change-id: 20231016-arm-psci-system_reset2-vendor-reboots-cc3ad456c070
> 
> Best regards,
> -- 
> Elliot Berman <quic_eberman@quicinc.com>
> 

Re: [PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Lorenzo Pieralisi]

On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote:
> SoC vendors have different types of resets and are controlled through
> various registers. For instance, Qualcomm chipsets can reboot to a
> "download mode" that allows a RAM dump to be collected. Another example
> is they also support writing a cookie that can be read by bootloader
> during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> vendor reset types to be implemented without requiring drivers for every
> register/cookie.
> 
> Add support in PSCI to statically map reboot mode commands from
> userspace to a vendor reset and cookie value using the device tree.
> 
> A separate initcall is needed to parse the devicetree, instead of using
> psci_dt_init because mm isn't sufficiently set up to allocate memory.

Nit: information below this point is more a cover letter than for the
commit log.

> Reboot mode framework is close but doesn't quite fit with the
> design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> be solved but doesn't seem reasonable in sum:
>  1. reboot mode registers against the reboot_notifier_list, which is too
>     early to call SYSTEM_RESET2. PSCI would need to remember the reset
>     type from the reboot-mode framework callback and use it
>     psci_sys_reset.
>  2. reboot mode assumes only one cookie/parameter is described in the
>     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
>     cookie.
>  3. psci cpuidle driver already registers a driver against the
>     arm,psci-1.0 compatible. Refactoring would be needed to have both a
>     cpuidle and reboot-mode driver.
> 
> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
>  static u32 psci_cpu_suspend_feature;
>  static bool psci_system_reset2_supported;
>  
> +struct psci_reset_param {
> +	const char *mode;
> +	u32 reset_type;
> +	u32 cookie;
> +};
> +static struct psci_reset_param *psci_reset_params __ro_after_init;
> +static size_t num_psci_reset_params __ro_after_init;
> +
>  static inline bool psci_has_ext_power_state(void)
>  {
>  	return psci_cpu_suspend_feature &
> @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np)
>  	return 0;
>  }
>  
> +static void psci_vendor_system_reset2(const char *cmd)
> +{
> +	unsigned long ret;
> +	size_t i;
> +
> +	for (i = 0; i < num_psci_reset_params; i++) {
> +		if (!strcmp(psci_reset_params[i].mode, cmd)) {
> +			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> +					     psci_reset_params[i].reset_type,
> +					     psci_reset_params[i].cookie, 0);
> +			/*
> +			 * if vendor reset fails, log it and fall back to
> +			 * architecture reset types
> +			 */
> +			pr_err("failed to perform reset \"%s\": %ld\n", cmd,
> +			       (long)ret);
> +			return;
> +		}
> +	}
> +}
> +
>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>  			  void *data)
>  {
> +	/*
> +	 * try to do the vendor system_reset2
> +	 * If the reset fails or there wasn't a match on the command,
> +	 * fall back to architectural resets
> +	 */
> +	if (data && num_psci_reset_params)
> +		psci_vendor_system_reset2(data);
> +
>  	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>  	    psci_system_reset2_supported) {

This is a mess. To issue architectural warm reset we check reboot_mode,
for vendor resets we ignore it - there is no rationale, that's the point
I am making.

Also see my question on the other thread re: user space and reset
"modes".

I appreciate we are not making progress but I don't want to pick up
the pieces later after merging this code - it is unclear to me what's
the best path forward - I would like to understand how other
platforms/arches behave in this respect.

>  		/*
> @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = {
>  	{},
>  };
>  
> +#define REBOOT_PREFIX "mode-"
> +
> +static int __init psci_init_system_reset2_modes(void)
> +{
> +	const size_t len = strlen(REBOOT_PREFIX);
> +	struct psci_reset_param *param;
> +	struct device_node *psci_np __free(device_node) = NULL;
> +	struct device_node *np __free(device_node) = NULL;
> +	struct property *prop;
> +	size_t count = 0;
> +	u32 magic[2];
> +	int num;
> +
> +	if (!psci_system_reset2_supported)
> +		return 0;
> +
> +	psci_np = of_find_matching_node(NULL, psci_of_match);
> +	if (!psci_np)
> +		return 0;
> +
> +	np = of_find_node_by_name(psci_np, "reset-types");
> +	if (!np)
> +		return 0;
> +
> +	for_each_property_of_node(np, prop) {
> +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> +			continue;
> +		num = of_property_count_u32_elems(np, prop->name);
> +		if (num != 1 && num != 2)
> +			continue;
> +
> +		count++;
> +	}
> +
> +	param = psci_reset_params =
> +		kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL);
> +	if (!psci_reset_params)
> +		return -ENOMEM;
> +
> +	for_each_property_of_node(np, prop) {
> +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> +			continue;
> +
> +		param->mode = kstrdup_const(prop->name + len, GFP_KERNEL);

FWIW - I think you need to keep the logic in the previous loop into account
because that's what is used to allocate param, it is not a given that
param is valid at this stage if I am not mistaken - the previous loop
checked:

	num = of_property_count_u32_elems(np, prop->name);
	if (num != 1 && num != 2)
		continue;

Lorenzo

> +		if (!param->mode)
> +			continue;
> +
> +		num = of_property_read_variable_u32_array(np, prop->name, magic,
> +							  1, ARRAY_SIZE(magic));
> +		if (num < 0) {
> +			pr_warn("Failed to parse vendor reboot mode %s\n",
> +				param->mode);
> +			kfree_const(param->mode);
> +			continue;
> +		}
> +
> +		/* Force reset type to be in vendor space */
> +		param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0];
> +		param->cookie = num > 1 ? magic[1] : 0;
> +		param++;
> +		num_psci_reset_params++;
> +	}
> +
> +	return 0;
> +}
> +arch_initcall(psci_init_system_reset2_modes);
> +
>  int __init psci_dt_init(void)
>  {
>  	struct device_node *np;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Elliot Berman]

On Fri, Nov 15, 2024 at 02:51:16PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote:
> > SoC vendors have different types of resets and are controlled through
> > various registers. For instance, Qualcomm chipsets can reboot to a
> > "download mode" that allows a RAM dump to be collected. Another example
> > is they also support writing a cookie that can be read by bootloader
> > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> > vendor reset types to be implemented without requiring drivers for every
> > register/cookie.
> > 
> > Add support in PSCI to statically map reboot mode commands from
> > userspace to a vendor reset and cookie value using the device tree.
> > 
> > A separate initcall is needed to parse the devicetree, instead of using
> > psci_dt_init because mm isn't sufficiently set up to allocate memory.
> 
> Nit: information below this point is more a cover letter than for the
> commit log.
> 
> > Reboot mode framework is close but doesn't quite fit with the
> > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> > be solved but doesn't seem reasonable in sum:
> >  1. reboot mode registers against the reboot_notifier_list, which is too
> >     early to call SYSTEM_RESET2. PSCI would need to remember the reset
> >     type from the reboot-mode framework callback and use it
> >     psci_sys_reset.
> >  2. reboot mode assumes only one cookie/parameter is described in the
> >     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
> >     cookie.
> >  3. psci cpuidle driver already registers a driver against the
> >     arm,psci-1.0 compatible. Refactoring would be needed to have both a
> >     cpuidle and reboot-mode driver.
> > 
> > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > ---
> >  drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 104 insertions(+)
> > 
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
> >  static u32 psci_cpu_suspend_feature;
> >  static bool psci_system_reset2_supported;
> >  
> > +struct psci_reset_param {
> > +	const char *mode;
> > +	u32 reset_type;
> > +	u32 cookie;
> > +};
> > +static struct psci_reset_param *psci_reset_params __ro_after_init;
> > +static size_t num_psci_reset_params __ro_after_init;
> > +
> >  static inline bool psci_has_ext_power_state(void)
> >  {
> >  	return psci_cpu_suspend_feature &
> > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np)
> >  	return 0;
> >  }
> >  
> > +static void psci_vendor_system_reset2(const char *cmd)
> > +{
> > +	unsigned long ret;
> > +	size_t i;
> > +
> > +	for (i = 0; i < num_psci_reset_params; i++) {
> > +		if (!strcmp(psci_reset_params[i].mode, cmd)) {
> > +			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> > +					     psci_reset_params[i].reset_type,
> > +					     psci_reset_params[i].cookie, 0);
> > +			/*
> > +			 * if vendor reset fails, log it and fall back to
> > +			 * architecture reset types
> > +			 */
> > +			pr_err("failed to perform reset \"%s\": %ld\n", cmd,
> > +			       (long)ret);
> > +			return;
> > +		}
> > +	}
> > +}
> > +
> >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> >  			  void *data)
> >  {
> > +	/*
> > +	 * try to do the vendor system_reset2
> > +	 * If the reset fails or there wasn't a match on the command,
> > +	 * fall back to architectural resets
> > +	 */
> > +	if (data && num_psci_reset_params)
> > +		psci_vendor_system_reset2(data);
> > +
> >  	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
> >  	    psci_system_reset2_supported) {
> 
> This is a mess. To issue architectural warm reset we check reboot_mode,
> for vendor resets we ignore it - there is no rationale, that's the point
> I am making.

If I expand the comment to:


 * try todo the vendor system_reset2
 * If the reset fails or there wasn't a match on the command,
 * fall back to architectural resets.
 * Ignore reboot_mode enum to behave like setting a cookie, which don't
 * care about the reboot_mode.


Help to address this concern?

> 
> Also see my question on the other thread re: user space and reset
> "modes".
> 
> I appreciate we are not making progress but I don't want to pick up
> the pieces later after merging this code - it is unclear to me what's
> the best path forward - I would like to understand how other
> platforms/arches behave in this respect.
> 

I went through the couple hundred drivers which register reboot and
restart handlers. The majority don't care about reboot command nor
reboot_mode enum. The few that do:

Two drivers which I could find which care about the reboot command don't
look at the reboot_mode argument.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/efibc.c?h=v6.11#n35
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset/reboot-mode.c?h=v6.11#n42

One driver looks at the reboot command overrides the reboot_mode
argument:

[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/pnx4008_wdt.c?h=v6.11#n125

I wasn't able to find any platform/arches which check the reboot_mode
before reading the reboot command.

> >  		/*
> > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = {
> >  	{},
> >  };
> >  
> > +#define REBOOT_PREFIX "mode-"
> > +
> > +static int __init psci_init_system_reset2_modes(void)
> > +{
> > +	const size_t len = strlen(REBOOT_PREFIX);
> > +	struct psci_reset_param *param;
> > +	struct device_node *psci_np __free(device_node) = NULL;
> > +	struct device_node *np __free(device_node) = NULL;
> > +	struct property *prop;
> > +	size_t count = 0;
> > +	u32 magic[2];
> > +	int num;
> > +
> > +	if (!psci_system_reset2_supported)
> > +		return 0;
> > +
> > +	psci_np = of_find_matching_node(NULL, psci_of_match);
> > +	if (!psci_np)
> > +		return 0;
> > +
> > +	np = of_find_node_by_name(psci_np, "reset-types");
> > +	if (!np)
> > +		return 0;
> > +
> > +	for_each_property_of_node(np, prop) {
> > +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> > +			continue;
> > +		num = of_property_count_u32_elems(np, prop->name);
> > +		if (num != 1 && num != 2)
> > +			continue;
> > +
> > +		count++;
> > +	}
> > +
> > +	param = psci_reset_params =
> > +		kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL);
> > +	if (!psci_reset_params)
> > +		return -ENOMEM;
> > +
> > +	for_each_property_of_node(np, prop) {
> > +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> > +			continue;
> > +
> > +		param->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
> 
> FWIW - I think you need to keep the logic in the previous loop into account
> because that's what is used to allocate param, it is not a given that
> param is valid at this stage if I am not mistaken - the previous loop
> checked:
> 
> 	num = of_property_count_u32_elems(np, prop->name);
> 	if (num != 1 && num != 2)
> 		continue;

of_property_read_variable_u32_array() performs effectively the same
check.  It returns -EOVERFLOW if it couldn't find enough (== 0) or too
many values (>2). I currently have the added bonus of complaining in
dmesg about the bad reboot mode property, instead of silently ignoring.

- Elliot

> > +		if (!param->mode)
> > +			continue;
> > +
> > +		num = of_property_read_variable_u32_array(np, prop->name, magic,
> > +							  1, ARRAY_SIZE(magic));
> > +		if (num < 0) {
> > +			pr_warn("Failed to parse vendor reboot mode %s\n",
> > +				param->mode);
> > +			kfree_const(param->mode);
> > +			continue;
> > +		}
> > +
> > +		/* Force reset type to be in vendor space */
> > +		param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0];
> > +		param->cookie = num > 1 ? magic[1] : 0;
> > +		param++;
> > +		num_psci_reset_params++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +arch_initcall(psci_init_system_reset2_modes);
> > +
> >  int __init psci_dt_init(void)
> >  {
> >  	struct device_node *np;
> > 
> > -- 
> > 2.34.1
> > 

Re: [PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Lorenzo Pieralisi]

On Fri, Nov 15, 2024 at 11:08:13AM -0800, Elliot Berman wrote:
> On Fri, Nov 15, 2024 at 02:51:16PM +0100, Lorenzo Pieralisi wrote:
> > On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote:
> > > SoC vendors have different types of resets and are controlled through
> > > various registers. For instance, Qualcomm chipsets can reboot to a
> > > "download mode" that allows a RAM dump to be collected. Another example
> > > is they also support writing a cookie that can be read by bootloader
> > > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> > > vendor reset types to be implemented without requiring drivers for every
> > > register/cookie.
> > > 
> > > Add support in PSCI to statically map reboot mode commands from
> > > userspace to a vendor reset and cookie value using the device tree.
> > > 
> > > A separate initcall is needed to parse the devicetree, instead of using
> > > psci_dt_init because mm isn't sufficiently set up to allocate memory.
> > 
> > Nit: information below this point is more a cover letter than for the
> > commit log.
> > 
> > > Reboot mode framework is close but doesn't quite fit with the
> > > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> > > be solved but doesn't seem reasonable in sum:
> > >  1. reboot mode registers against the reboot_notifier_list, which is too
> > >     early to call SYSTEM_RESET2. PSCI would need to remember the reset
> > >     type from the reboot-mode framework callback and use it
> > >     psci_sys_reset.
> > >  2. reboot mode assumes only one cookie/parameter is described in the
> > >     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
> > >     cookie.
> > >  3. psci cpuidle driver already registers a driver against the
> > >     arm,psci-1.0 compatible. Refactoring would be needed to have both a
> > >     cpuidle and reboot-mode driver.
> > > 
> > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > > ---
> > >  drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 104 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644
> > > --- a/drivers/firmware/psci/psci.c
> > > +++ b/drivers/firmware/psci/psci.c
> > > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
> > >  static u32 psci_cpu_suspend_feature;
> > >  static bool psci_system_reset2_supported;
> > >  
> > > +struct psci_reset_param {
> > > +	const char *mode;
> > > +	u32 reset_type;
> > > +	u32 cookie;
> > > +};
> > > +static struct psci_reset_param *psci_reset_params __ro_after_init;
> > > +static size_t num_psci_reset_params __ro_after_init;
> > > +
> > >  static inline bool psci_has_ext_power_state(void)
> > >  {
> > >  	return psci_cpu_suspend_feature &
> > > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np)
> > >  	return 0;
> > >  }
> > >  
> > > +static void psci_vendor_system_reset2(const char *cmd)
> > > +{
> > > +	unsigned long ret;
> > > +	size_t i;
> > > +
> > > +	for (i = 0; i < num_psci_reset_params; i++) {
> > > +		if (!strcmp(psci_reset_params[i].mode, cmd)) {
> > > +			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> > > +					     psci_reset_params[i].reset_type,
> > > +					     psci_reset_params[i].cookie, 0);
> > > +			/*
> > > +			 * if vendor reset fails, log it and fall back to
> > > +			 * architecture reset types
> > > +			 */
> > > +			pr_err("failed to perform reset \"%s\": %ld\n", cmd,
> > > +			       (long)ret);
> > > +			return;
> > > +		}
> > > +	}
> > > +}
> > > +
> > >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> > >  			  void *data)
> > >  {
> > > +	/*
> > > +	 * try to do the vendor system_reset2
> > > +	 * If the reset fails or there wasn't a match on the command,
> > > +	 * fall back to architectural resets
> > > +	 */
> > > +	if (data && num_psci_reset_params)
> > > +		psci_vendor_system_reset2(data);
> > > +
> > >  	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
> > >  	    psci_system_reset2_supported) {
> > 
> > This is a mess. To issue architectural warm reset we check reboot_mode,
> > for vendor resets we ignore it - there is no rationale, that's the point
> > I am making.
> 
> If I expand the comment to:
> 
> 
>  * try todo the vendor system_reset2
>  * If the reset fails or there wasn't a match on the command,
>  * fall back to architectural resets.
>  * Ignore reboot_mode enum to behave like setting a cookie, which don't
>  * care about the reboot_mode.

/*
 * Check if the system supports vendor resets and issue
 * SYSTEM_RESET2 if the reboot command matches a vendor reset.
 * Ignore reboot_mode and execute SYSTEM_RESET2 with type and
 * cookie as defined by the firmware bindings.
 *
 * If the reset fails or there is not a match for the command
 * fall back to architectural resets; reset type detection in
 * this case will be done using reboot_mode.
 */

?

> Help to address this concern?

Not entirely, sorry, I will get back to this.

> > Also see my question on the other thread re: user space and reset
> > "modes".
> > 
> > I appreciate we are not making progress but I don't want to pick up
> > the pieces later after merging this code - it is unclear to me what's
> > the best path forward - I would like to understand how other
> > platforms/arches behave in this respect.
> > 
> 
> I went through the couple hundred drivers which register reboot and
> restart handlers. The majority don't care about reboot command nor
> reboot_mode enum. The few that do:
> 
> Two drivers which I could find which care about the reboot command don't
> look at the reboot_mode argument.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/efibc.c?h=v6.11#n35
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset/reboot-mode.c?h=v6.11#n42
> 
> One driver looks at the reboot command overrides the reboot_mode
> argument:
> 
> [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/pnx4008_wdt.c?h=v6.11#n125

Thanks for doing that, that helps.

> I wasn't able to find any platform/arches which check the reboot_mode
> before reading the reboot command.
> 
> > >  		/*
> > > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = {
> > >  	{},
> > >  };
> > >  
> > > +#define REBOOT_PREFIX "mode-"
> > > +
> > > +static int __init psci_init_system_reset2_modes(void)
> > > +{
> > > +	const size_t len = strlen(REBOOT_PREFIX);
> > > +	struct psci_reset_param *param;
> > > +	struct device_node *psci_np __free(device_node) = NULL;
> > > +	struct device_node *np __free(device_node) = NULL;
> > > +	struct property *prop;
> > > +	size_t count = 0;
> > > +	u32 magic[2];
> > > +	int num;
> > > +
> > > +	if (!psci_system_reset2_supported)
> > > +		return 0;
> > > +
> > > +	psci_np = of_find_matching_node(NULL, psci_of_match);
> > > +	if (!psci_np)
> > > +		return 0;
> > > +
> > > +	np = of_find_node_by_name(psci_np, "reset-types");
> > > +	if (!np)
> > > +		return 0;
> > > +
> > > +	for_each_property_of_node(np, prop) {
> > > +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> > > +			continue;
> > > +		num = of_property_count_u32_elems(np, prop->name);
> > > +		if (num != 1 && num != 2)
> > > +			continue;
> > > +
> > > +		count++;
> > > +	}
> > > +
> > > +	param = psci_reset_params =
> > > +		kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL);
> > > +	if (!psci_reset_params)
> > > +		return -ENOMEM;
> > > +
> > > +	for_each_property_of_node(np, prop) {
> > > +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> > > +			continue;
> > > +
> > > +		param->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
> > 
> > FWIW - I think you need to keep the logic in the previous loop into account
> > because that's what is used to allocate param, it is not a given that
> > param is valid at this stage if I am not mistaken - the previous loop
> > checked:
> > 
> > 	num = of_property_count_u32_elems(np, prop->name);
> > 	if (num != 1 && num != 2)
> > 		continue;
> 
> of_property_read_variable_u32_array() performs effectively the same
> check.  It returns -EOVERFLOW if it couldn't find enough (== 0) or too
> many values (>2). I currently have the added bonus of complaining in
> dmesg about the bad reboot mode property, instead of silently ignoring.

Right but we are dereferencing param (param->mode) before carrying out that
check.

Thanks,
Lorenzo

> 
> - Elliot
> 
> > > +		if (!param->mode)
> > > +			continue;
> > > +
> > > +		num = of_property_read_variable_u32_array(np, prop->name, magic,
> > > +							  1, ARRAY_SIZE(magic));
> > > +		if (num < 0) {
> > > +			pr_warn("Failed to parse vendor reboot mode %s\n",
> > > +				param->mode);
> > > +			kfree_const(param->mode);
> > > +			continue;
> > > +		}
> > > +
> > > +		/* Force reset type to be in vendor space */
> > > +		param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0];
> > > +		param->cookie = num > 1 ? magic[1] : 0;
> > > +		param++;
> > > +		num_psci_reset_params++;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +arch_initcall(psci_init_system_reset2_modes);
> > > +
> > >  int __init psci_dt_init(void)
> > >  {
> > >  	struct device_node *np;
> > > 
> > > -- 
> > > 2.34.1
> > > 

Re: [PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Elliot Berman]

On Mon, Nov 18, 2024 at 01:26:48PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Nov 15, 2024 at 11:08:13AM -0800, Elliot Berman wrote:
> > On Fri, Nov 15, 2024 at 02:51:16PM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote:
> > > > SoC vendors have different types of resets and are controlled through
> > > > various registers. For instance, Qualcomm chipsets can reboot to a
> > > > "download mode" that allows a RAM dump to be collected. Another example
> > > > is they also support writing a cookie that can be read by bootloader
> > > > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> > > > vendor reset types to be implemented without requiring drivers for every
> > > > register/cookie.
> > > > 
> > > > Add support in PSCI to statically map reboot mode commands from
> > > > userspace to a vendor reset and cookie value using the device tree.
> > > > 
> > > > A separate initcall is needed to parse the devicetree, instead of using
> > > > psci_dt_init because mm isn't sufficiently set up to allocate memory.
> > > 
> > > Nit: information below this point is more a cover letter than for the
> > > commit log.
> > > 
> > > > Reboot mode framework is close but doesn't quite fit with the
> > > > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> > > > be solved but doesn't seem reasonable in sum:
> > > >  1. reboot mode registers against the reboot_notifier_list, which is too
> > > >     early to call SYSTEM_RESET2. PSCI would need to remember the reset
> > > >     type from the reboot-mode framework callback and use it
> > > >     psci_sys_reset.
> > > >  2. reboot mode assumes only one cookie/parameter is described in the
> > > >     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
> > > >     cookie.
> > > >  3. psci cpuidle driver already registers a driver against the
> > > >     arm,psci-1.0 compatible. Refactoring would be needed to have both a
> > > >     cpuidle and reboot-mode driver.
> > > > 
> > > > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > > > ---
> > > >  drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 104 insertions(+)
> > > > 
> > > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > > > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644
> > > > --- a/drivers/firmware/psci/psci.c
> > > > +++ b/drivers/firmware/psci/psci.c
> > > > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
> > > >  static u32 psci_cpu_suspend_feature;
> > > >  static bool psci_system_reset2_supported;
> > > >  
> > > > +struct psci_reset_param {
> > > > +	const char *mode;
> > > > +	u32 reset_type;
> > > > +	u32 cookie;
> > > > +};
> > > > +static struct psci_reset_param *psci_reset_params __ro_after_init;
> > > > +static size_t num_psci_reset_params __ro_after_init;
> > > > +
> > > >  static inline bool psci_has_ext_power_state(void)
> > > >  {
> > > >  	return psci_cpu_suspend_feature &
> > > > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static void psci_vendor_system_reset2(const char *cmd)
> > > > +{
> > > > +	unsigned long ret;
> > > > +	size_t i;
> > > > +
> > > > +	for (i = 0; i < num_psci_reset_params; i++) {
> > > > +		if (!strcmp(psci_reset_params[i].mode, cmd)) {
> > > > +			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> > > > +					     psci_reset_params[i].reset_type,
> > > > +					     psci_reset_params[i].cookie, 0);
> > > > +			/*
> > > > +			 * if vendor reset fails, log it and fall back to
> > > > +			 * architecture reset types
> > > > +			 */
> > > > +			pr_err("failed to perform reset \"%s\": %ld\n", cmd,
> > > > +			       (long)ret);
> > > > +			return;
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> > > >  			  void *data)
> > > >  {
> > > > +	/*
> > > > +	 * try to do the vendor system_reset2
> > > > +	 * If the reset fails or there wasn't a match on the command,
> > > > +	 * fall back to architectural resets
> > > > +	 */
> > > > +	if (data && num_psci_reset_params)
> > > > +		psci_vendor_system_reset2(data);
> > > > +
> > > >  	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
> > > >  	    psci_system_reset2_supported) {
> > > 
> > > This is a mess. To issue architectural warm reset we check reboot_mode,
> > > for vendor resets we ignore it - there is no rationale, that's the point
> > > I am making.
> > 
> > If I expand the comment to:
> > 
> > 
> >  * try todo the vendor system_reset2
> >  * If the reset fails or there wasn't a match on the command,
> >  * fall back to architectural resets.
> >  * Ignore reboot_mode enum to behave like setting a cookie, which don't
> >  * care about the reboot_mode.
> 
> /*
>  * Check if the system supports vendor resets and issue
>  * SYSTEM_RESET2 if the reboot command matches a vendor reset.
>  * Ignore reboot_mode and execute SYSTEM_RESET2 with type and
>  * cookie as defined by the firmware bindings.
>  *
>  * If the reset fails or there is not a match for the command
>  * fall back to architectural resets; reset type detection in
>  * this case will be done using reboot_mode.
>  */
> 
> ?
> 
> > Help to address this concern?
> 
> Not entirely, sorry, I will get back to this.
> 
> > > Also see my question on the other thread re: user space and reset
> > > "modes".
> > > 
> > > I appreciate we are not making progress but I don't want to pick up
> > > the pieces later after merging this code - it is unclear to me what's
> > > the best path forward - I would like to understand how other
> > > platforms/arches behave in this respect.
> > > 
> > 
> > I went through the couple hundred drivers which register reboot and
> > restart handlers. The majority don't care about reboot command nor
> > reboot_mode enum. The few that do:
> > 
> > Two drivers which I could find which care about the reboot command don't
> > look at the reboot_mode argument.
> > 
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/efibc.c?h=v6.11#n35
> > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset/reboot-mode.c?h=v6.11#n42
> > 
> > One driver looks at the reboot command overrides the reboot_mode
> > argument:
> > 
> > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/pnx4008_wdt.c?h=v6.11#n125
> 
> Thanks for doing that, that helps.
> 
> > I wasn't able to find any platform/arches which check the reboot_mode
> > before reading the reboot command.
> > 
> > > >  		/*
> > > > @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = {
> > > >  	{},
> > > >  };
> > > >  
> > > > +#define REBOOT_PREFIX "mode-"
> > > > +
> > > > +static int __init psci_init_system_reset2_modes(void)
> > > > +{
> > > > +	const size_t len = strlen(REBOOT_PREFIX);
> > > > +	struct psci_reset_param *param;
> > > > +	struct device_node *psci_np __free(device_node) = NULL;
> > > > +	struct device_node *np __free(device_node) = NULL;
> > > > +	struct property *prop;
> > > > +	size_t count = 0;
> > > > +	u32 magic[2];
> > > > +	int num;
> > > > +
> > > > +	if (!psci_system_reset2_supported)
> > > > +		return 0;
> > > > +
> > > > +	psci_np = of_find_matching_node(NULL, psci_of_match);
> > > > +	if (!psci_np)
> > > > +		return 0;
> > > > +
> > > > +	np = of_find_node_by_name(psci_np, "reset-types");
> > > > +	if (!np)
> > > > +		return 0;
> > > > +
> > > > +	for_each_property_of_node(np, prop) {
> > > > +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> > > > +			continue;
> > > > +		num = of_property_count_u32_elems(np, prop->name);
> > > > +		if (num != 1 && num != 2)
> > > > +			continue;
> > > > +
> > > > +		count++;
> > > > +	}
> > > > +
> > > > +	param = psci_reset_params =
> > > > +		kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL);
> > > > +	if (!psci_reset_params)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	for_each_property_of_node(np, prop) {
> > > > +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> > > > +			continue;
> > > > +
> > > > +		param->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
> > > 
> > > FWIW - I think you need to keep the logic in the previous loop into account
> > > because that's what is used to allocate param, it is not a given that
> > > param is valid at this stage if I am not mistaken - the previous loop
> > > checked:
> > > 
> > > 	num = of_property_count_u32_elems(np, prop->name);
> > > 	if (num != 1 && num != 2)
> > > 		continue;
> > 
> > of_property_read_variable_u32_array() performs effectively the same
> > check.  It returns -EOVERFLOW if it couldn't find enough (== 0) or too
> > many values (>2). I currently have the added bonus of complaining in
> > dmesg about the bad reboot mode property, instead of silently ignoring.
> 
> Right but we are dereferencing param (param->mode) before carrying out that
> check.
> 

Ah, right, I see the problem. I'll send out another version after
settling on the other part of the discussion!

Thanks,
Elliot

Re: [PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Lorenzo Pieralisi]

On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote:
> SoC vendors have different types of resets and are controlled through
> various registers. For instance, Qualcomm chipsets can reboot to a
> "download mode" that allows a RAM dump to be collected. Another example
> is they also support writing a cookie that can be read by bootloader
> during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> vendor reset types to be implemented without requiring drivers for every
> register/cookie.
> 
> Add support in PSCI to statically map reboot mode commands from
> userspace to a vendor reset and cookie value using the device tree.
> 
> A separate initcall is needed to parse the devicetree, instead of using
> psci_dt_init because mm isn't sufficiently set up to allocate memory.
> 
> Reboot mode framework is close but doesn't quite fit with the
> design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> be solved but doesn't seem reasonable in sum:
>  1. reboot mode registers against the reboot_notifier_list, which is too
>     early to call SYSTEM_RESET2. PSCI would need to remember the reset
>     type from the reboot-mode framework callback and use it
>     psci_sys_reset.
>  2. reboot mode assumes only one cookie/parameter is described in the
>     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
>     cookie.
>  3. psci cpuidle driver already registers a driver against the
>     arm,psci-1.0 compatible. Refactoring would be needed to have both a
>     cpuidle and reboot-mode driver.
> 
> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
>  static u32 psci_cpu_suspend_feature;
>  static bool psci_system_reset2_supported;
>  
> +struct psci_reset_param {
> +	const char *mode;
> +	u32 reset_type;
> +	u32 cookie;
> +};
> +static struct psci_reset_param *psci_reset_params __ro_after_init;
> +static size_t num_psci_reset_params __ro_after_init;
> +
>  static inline bool psci_has_ext_power_state(void)
>  {
>  	return psci_cpu_suspend_feature &
> @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np)
>  	return 0;
>  }
>  
> +static void psci_vendor_system_reset2(const char *cmd)
> +{
> +	unsigned long ret;
> +	size_t i;
> +
> +	for (i = 0; i < num_psci_reset_params; i++) {
> +		if (!strcmp(psci_reset_params[i].mode, cmd)) {
> +			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> +					     psci_reset_params[i].reset_type,
> +					     psci_reset_params[i].cookie, 0);
> +			/*
> +			 * if vendor reset fails, log it and fall back to
> +			 * architecture reset types
> +			 */
> +			pr_err("failed to perform reset \"%s\": %ld\n", cmd,
> +			       (long)ret);
> +			return;
> +		}
> +	}
> +}
> +
>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>  			  void *data)
>  {
> +	/*
> +	 * try to do the vendor system_reset2
> +	 * If the reset fails or there wasn't a match on the command,
> +	 * fall back to architectural resets
> +	 */
> +	if (data && num_psci_reset_params)
> +		psci_vendor_system_reset2(data);

Mulling over this. If a command (data) was provided and a PSCI vendor
reset parsed at boot, if the vendor reset fails, isn't it correct to
just fail reboot instead of falling back to architectural resets ?

What's missing is defining the "contract" between the
LINUX_REBOOT_CMD_RESTART2 arg parameter and the kernel reboot
type that is executed.

I do wonder whether this is an opportunity to deprecate reboot_mode
altogether on arm64 (I think that the relationship between REBOOT_WARM
and REBOOT_SOFT with PSCI arch warm reset is already loose - let alone
falling back to cold reset if reboot_mode == REBOOT_GPIO - which does
not make any sense at all simply because REBOOT_GPIO is ill-defined to
say the least).

Thoughts ?

Thanks,
Lorenzo

> +
>  	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>  	    psci_system_reset2_supported) {
>  		/*
> @@ -750,6 +787,73 @@ static const struct of_device_id psci_of_match[] __initconst = {
>  	{},
>  };
>  
> +#define REBOOT_PREFIX "mode-"
> +
> +static int __init psci_init_system_reset2_modes(void)
> +{
> +	const size_t len = strlen(REBOOT_PREFIX);
> +	struct psci_reset_param *param;
> +	struct device_node *psci_np __free(device_node) = NULL;
> +	struct device_node *np __free(device_node) = NULL;
> +	struct property *prop;
> +	size_t count = 0;
> +	u32 magic[2];
> +	int num;
> +
> +	if (!psci_system_reset2_supported)
> +		return 0;
> +
> +	psci_np = of_find_matching_node(NULL, psci_of_match);
> +	if (!psci_np)
> +		return 0;
> +
> +	np = of_find_node_by_name(psci_np, "reset-types");
> +	if (!np)
> +		return 0;
> +
> +	for_each_property_of_node(np, prop) {
> +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> +			continue;
> +		num = of_property_count_u32_elems(np, prop->name);
> +		if (num != 1 && num != 2)
> +			continue;
> +
> +		count++;
> +	}
> +
> +	param = psci_reset_params =
> +		kcalloc(count, sizeof(*psci_reset_params), GFP_KERNEL);
> +	if (!psci_reset_params)
> +		return -ENOMEM;
> +
> +	for_each_property_of_node(np, prop) {
> +		if (strncmp(prop->name, REBOOT_PREFIX, len))
> +			continue;
> +
> +		param->mode = kstrdup_const(prop->name + len, GFP_KERNEL);
> +		if (!param->mode)
> +			continue;
> +
> +		num = of_property_read_variable_u32_array(np, prop->name, magic,
> +							  1, ARRAY_SIZE(magic));
> +		if (num < 0) {
> +			pr_warn("Failed to parse vendor reboot mode %s\n",
> +				param->mode);
> +			kfree_const(param->mode);
> +			continue;
> +		}
> +
> +		/* Force reset type to be in vendor space */
> +		param->reset_type = PSCI_1_1_RESET_TYPE_VENDOR_START | magic[0];
> +		param->cookie = num > 1 ? magic[1] : 0;
> +		param++;
> +		num_psci_reset_params++;
> +	}
> +
> +	return 0;
> +}
> +arch_initcall(psci_init_system_reset2_modes);
> +
>  int __init psci_dt_init(void)
>  {
>  	struct device_node *np;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v8 3/6] firmware: psci: Read and use vendor reset types

[Elliot Berman]

On Wed, Feb 26, 2025 at 03:28:47PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Nov 07, 2024 at 03:38:27PM -0800, Elliot Berman wrote:
> > SoC vendors have different types of resets and are controlled through
> > various registers. For instance, Qualcomm chipsets can reboot to a
> > "download mode" that allows a RAM dump to be collected. Another example
> > is they also support writing a cookie that can be read by bootloader
> > during next boot. PSCI offers a mechanism, SYSTEM_RESET2, for these
> > vendor reset types to be implemented without requiring drivers for every
> > register/cookie.
> > 
> > Add support in PSCI to statically map reboot mode commands from
> > userspace to a vendor reset and cookie value using the device tree.
> > 
> > A separate initcall is needed to parse the devicetree, instead of using
> > psci_dt_init because mm isn't sufficiently set up to allocate memory.
> > 
> > Reboot mode framework is close but doesn't quite fit with the
> > design and requirements for PSCI SYSTEM_RESET2. Some of these issues can
> > be solved but doesn't seem reasonable in sum:
> >  1. reboot mode registers against the reboot_notifier_list, which is too
> >     early to call SYSTEM_RESET2. PSCI would need to remember the reset
> >     type from the reboot-mode framework callback and use it
> >     psci_sys_reset.
> >  2. reboot mode assumes only one cookie/parameter is described in the
> >     device tree. SYSTEM_RESET2 uses 2: one for the type and one for
> >     cookie.
> >  3. psci cpuidle driver already registers a driver against the
> >     arm,psci-1.0 compatible. Refactoring would be needed to have both a
> >     cpuidle and reboot-mode driver.
> > 
> > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > ---
> >  drivers/firmware/psci/psci.c | 104 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 104 insertions(+)
> > 
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index 2328ca58bba61fdb677ac20a1a7447882cd0cf22..e60e3f8749c5a6732c51d23a2c1f453361132d9a 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -79,6 +79,14 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void)
> >  static u32 psci_cpu_suspend_feature;
> >  static bool psci_system_reset2_supported;
> >  
> > +struct psci_reset_param {
> > +	const char *mode;
> > +	u32 reset_type;
> > +	u32 cookie;
> > +};
> > +static struct psci_reset_param *psci_reset_params __ro_after_init;
> > +static size_t num_psci_reset_params __ro_after_init;
> > +
> >  static inline bool psci_has_ext_power_state(void)
> >  {
> >  	return psci_cpu_suspend_feature &
> > @@ -305,9 +313,38 @@ static int get_set_conduit_method(const struct device_node *np)
> >  	return 0;
> >  }
> >  
> > +static void psci_vendor_system_reset2(const char *cmd)
> > +{
> > +	unsigned long ret;
> > +	size_t i;
> > +
> > +	for (i = 0; i < num_psci_reset_params; i++) {
> > +		if (!strcmp(psci_reset_params[i].mode, cmd)) {
> > +			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
> > +					     psci_reset_params[i].reset_type,
> > +					     psci_reset_params[i].cookie, 0);
> > +			/*
> > +			 * if vendor reset fails, log it and fall back to
> > +			 * architecture reset types
> > +			 */
> > +			pr_err("failed to perform reset \"%s\": %ld\n", cmd,
> > +			       (long)ret);
> > +			return;
> > +		}
> > +	}
> > +}
> > +
> >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> >  			  void *data)
> >  {
> > +	/*
> > +	 * try to do the vendor system_reset2
> > +	 * If the reset fails or there wasn't a match on the command,
> > +	 * fall back to architectural resets
> > +	 */
> > +	if (data && num_psci_reset_params)
> > +		psci_vendor_system_reset2(data);
> 
> Mulling over this. If a command (data) was provided and a PSCI vendor
> reset parsed at boot, if the vendor reset fails, isn't it correct to
> just fail reboot instead of falling back to architectural resets ?
> 

Sure! I can change the behavior here.

> What's missing is defining the "contract" between the
> LINUX_REBOOT_CMD_RESTART2 arg parameter and the kernel reboot
> type that is executed.
> 
> I do wonder whether this is an opportunity to deprecate reboot_mode
> altogether on arm64 (I think that the relationship between REBOOT_WARM
> and REBOOT_SOFT with PSCI arch warm reset is already loose - let alone
> falling back to cold reset if reboot_mode == REBOOT_GPIO - which does
> not make any sense at all simply because REBOOT_GPIO is ill-defined to
> say the least).
> 
> Thoughts ?
> 

I'm not opposed to seeing how we can deprecate the reboot_mode enum for
arm64, but I think this should be an independent effort from the vendor
resets. I'm worried this takes us down the "don't let perfect be the
enemy of good" path to support vendor resets. I haven't seen much
interest in this issue outside the two of us, and thus changing
important infrastructure like reboot seems to me to be a long shot.

Thanks,
Elliot