[PATCH v5 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[PATCH v5 0/4] 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.

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 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 (4):
      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: Add PSCI SYSTEM_RESET2 types for qcm6490-idp

 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/sc7280.dtsi               |  2 +-
 drivers/firmware/psci/psci.c                       | 92 ++++++++++++++++++++++
 8 files changed, 160 insertions(+), 3 deletions(-)
---
base-commit: e92bee9f861b466c676f0200be3e46af7bc4ac6b
change-id: 20231016-arm-psci-system_reset2-vendor-reboots-cc3ad456c070

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

[PATCH v5 1/4] 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.

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 627f8a6078c2..7f5f94673e9c 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 fc8105a7b9b2..3da3d02a6690 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 ad0a0b95cec1..3ddac06cec72 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 b6acff199cde..79ffc78b23ea 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 v5 2/4] 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.

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 cbb012e217ab..5e07c62fe5d7 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 v5 3/4] 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 | 92 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index d9629ff87861..e672b33b71d1 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -29,6 +29,8 @@
 #include <asm/smp_plat.h>
 #include <asm/suspend.h>
 
+#define REBOOT_PREFIX "mode-"
+
 /*
  * While a 64-bit OS can make calls with SMC32 calling conventions, for some
  * calls it is necessary to use SMC64 to pass or return 64-bit values.
@@ -79,6 +81,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;
+static size_t num_psci_reset_params;
+
 static inline bool psci_has_ext_power_state(void)
 {
 	return psci_cpu_suspend_feature &
@@ -305,9 +315,29 @@ static int get_set_conduit_method(const struct device_node *np)
 	return 0;
 }
 
+static void psci_vendor_sys_reset2(unsigned long action, void *data)
+{
+	const char *cmd = data;
+	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);
+			pr_err("failed to perform reset \"%s\": %ld\n",
+				cmd, (long)ret);
+		}
+	}
+}
+
 static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
 			  void *data)
 {
+	if (data && num_psci_reset_params)
+		psci_vendor_sys_reset2(action, data);
+
 	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
 	    psci_system_reset2_supported) {
 		/*
@@ -748,6 +778,68 @@ static const struct of_device_id psci_of_match[] __initconst = {
 	{},
 };
 
+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_elems_of_size(np, prop->name, sizeof(magic[0]));
+		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, 2);
+		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 == 2 ? 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 v5 4/4] arm64: dts: qcom: Add PSCI SYSTEM_RESET2 types for qcm6490-idp

[Elliot Berman]

Add nodes for the vendor-defined system 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.

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/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 e4bfad50a669..fd0a7dd14483 100644
--- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
+++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
@@ -448,6 +448,13 @@ led@3 {
 	};
 };
 
+&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 7e7f0f0fb41b..da25a3089419 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -848,7 +848,7 @@ pmu {
 		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
 	};
 
-	psci {
+	psci: psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
 

-- 
2.34.1

Re: [PATCH v5 4/4] arm64: dts: qcom: Add PSCI SYSTEM_RESET2 types for qcm6490-idp

[Konrad Dybcio]

On 6/17/24 19:18, Elliot Berman wrote:
> Add nodes for the vendor-defined system 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.
> 
> 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@linaro.org>

Konrad

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Sudeep Holla]

On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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.
>

I need to think through it but when you first introduced the generic
Documentation/devicetree/bindings/power/reset/reboot-mode.yaml bindings
I also looked at drivers/power/reset/reboot-mode.c

I assumed this extension to that binding would reuse the same and
PSCI would just do reboot_mode_register(). I didn't expect to see these
changes. I might have missing something but since the bindings is still
quite generic with additional cells that act as additional cookie for
reboot call, I still think that should be possible.

What am I missing here then ?

-- 
Regards,
Sudeep

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Elliot Berman]

On Wed, Jun 19, 2024 at 02:51:43PM +0100, Sudeep Holla wrote:
> On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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.
> >
> 
> I need to think through it but when you first introduced the generic
> Documentation/devicetree/bindings/power/reset/reboot-mode.yaml bindings
> I also looked at drivers/power/reset/reboot-mode.c
> 
> I assumed this extension to that binding would reuse the same and
> PSCI would just do reboot_mode_register(). I didn't expect to see these
> changes. I might have missing something but since the bindings is still
> quite generic with additional cells that act as additional cookie for
> reboot call, I still think that should be possible.
> 
> What am I missing here then ?
> 

Right, if that was only thing to "solve" to make it easy to use
reboot-mode framework, I agree we should update reboot mode framework to
work with the additional cells. There are a few other issues I mention
above which, when combined, make me feel that PSCI is different enough
from how reboot mode framework works that we shouldn't try to make PSCI
work with the framework. Issues #1 and #2 are pretty easy to solve
(whether they should be solved is different); I'm not sure a good
approach to issue #3.

Thanks,
Elliot

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Elliot Berman]

Hi Sudeep and Sebastian,

On Wed, Jun 19, 2024 at 08:28:06AM -0700, Elliot Berman wrote:
> On Wed, Jun 19, 2024 at 02:51:43PM +0100, Sudeep Holla wrote:
> > On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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.
> > >
> > 
> > I need to think through it but when you first introduced the generic
> > Documentation/devicetree/bindings/power/reset/reboot-mode.yaml bindings
> > I also looked at drivers/power/reset/reboot-mode.c
> > 
> > I assumed this extension to that binding would reuse the same and
> > PSCI would just do reboot_mode_register(). I didn't expect to see these
> > changes. I might have missing something but since the bindings is still
> > quite generic with additional cells that act as additional cookie for
> > reboot call, I still think that should be possible.
> > 
> > What am I missing here then ?
> > 
> 
> Right, if that was only thing to "solve" to make it easy to use
> reboot-mode framework, I agree we should update reboot mode framework to
> work with the additional cells. There are a few other issues I mention
> above which, when combined, make me feel that PSCI is different enough
> from how reboot mode framework works that we shouldn't try to make PSCI
> work with the framework. Issues #1 and #2 are pretty easy to solve
> (whether they should be solved is different); I'm not sure a good
> approach to issue #3.
> 

Does the reasoning I mention in the commit text make sense why PSCI should
avoid using the reboot-mode.c framework?

Thanks,
Elliot

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Sudeep Holla]

On Thu, Jun 20, 2024 at 04:37:09PM -0700, Elliot Berman wrote:
> Hi Sudeep and Sebastian,
> 
> On Wed, Jun 19, 2024 at 08:28:06AM -0700, Elliot Berman wrote:
> > On Wed, Jun 19, 2024 at 02:51:43PM +0100, Sudeep Holla wrote:
> > > On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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.
> > > >
> > > 
> > > I need to think through it but when you first introduced the generic
> > > Documentation/devicetree/bindings/power/reset/reboot-mode.yaml bindings
> > > I also looked at drivers/power/reset/reboot-mode.c
> > > 
> > > I assumed this extension to that binding would reuse the same and
> > > PSCI would just do reboot_mode_register(). I didn't expect to see these
> > > changes. I might have missing something but since the bindings is still
> > > quite generic with additional cells that act as additional cookie for
> > > reboot call, I still think that should be possible.
> > > 
> > > What am I missing here then ?
> > > 
> > 
> > Right, if that was only thing to "solve" to make it easy to use
> > reboot-mode framework, I agree we should update reboot mode framework to
> > work with the additional cells. There are a few other issues I mention
> > above which, when combined, make me feel that PSCI is different enough
> > from how reboot mode framework works that we shouldn't try to make PSCI
> > work with the framework. Issues #1 and #2 are pretty easy to solve
> > (whether they should be solved is different); I'm not sure a good
> > approach to issue #3.
> > 
> 
> Does the reasoning I mention in the commit text make sense why PSCI should
> avoid using the reboot-mode.c framework?

Sorry, I completely missed to see that you had already answered those
in your commit message. As mentioned earlier I haven't looked at the
reboot mode framework completely yet, so I can't comment on it yet.

I don't want to be blocker though if others are happy with this.

--
Regards,
Sudeep

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Sudeep Holla]

On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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.

Fair enough, good reason but as I mentioned I haven't looked at it in
details to provide any suggestion or to assert it can be solved.

>  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.

I assumed it shouldn't be too difficult to extend it to support 2 parameters.

>  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.

This one seems not a strong reason in my opinion. It was not designed for
cpuidle and it can't bind to that solely. If this becomes only reason, then
we need to fix that.

-- 
Regards,
Sudeep

Re: [PATCH v5 1/4] dt-bindings: power: reset: Convert mode-.* properties to array

[Rob Herring (Arm)]

On Mon, 17 Jun 2024 10:18:07 -0700, Elliot Berman wrote:
> 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.
> 
> 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(-)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v5 2/4] dt-bindings: arm: Document reboot mode magic

[Rob Herring (Arm)]

On Mon, 17 Jun 2024 10:18:08 -0700, Elliot Berman wrote:
> Add bindings to describe vendor-specific reboot modes. Values here
> correspond to valid parameters to vendor-specific reset types in PSCI
> SYSTEM_RESET2 call.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  Documentation/devicetree/bindings/arm/psci.yaml | 43 +++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Elliot Berman]

Hi Sudeep,

On Mon, Jun 24, 2024 at 04:41:42PM +0100, Sudeep Holla wrote:
> Sorry, I completely missed to see that you had already answered those
> in your commit message. As mentioned earlier I haven't looked at the
> reboot mode framework completely yet, so I can't comment on it yet.
> 
> I don't want to be blocker though if others are happy with this.

I think folks are satisfied with the other parts of the series and now
looking for your conclusion on the PSCI driver part.

Thanks,
Elliot

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Trilok Soni]

On 7/2/2024 4:06 PM, Elliot Berman wrote:
> Hi Sudeep,
> 
> On Mon, Jun 24, 2024 at 04:41:42PM +0100, Sudeep Holla wrote:
>> Sorry, I completely missed to see that you had already answered those
>> in your commit message. As mentioned earlier I haven't looked at the
>> reboot mode framework completely yet, so I can't comment on it yet.
>>
>> I don't want to be blocker though if others are happy with this.
> 
> I think folks are satisfied with the other parts of the series and now
> looking for your conclusion on the PSCI driver part.

I will be nice to get these patches picked up before 4th July holiday in US :).

-- 
---Trilok Soni

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Trilok Soni]

On 7/2/2024 4:42 PM, Trilok Soni wrote:
> On 7/2/2024 4:06 PM, Elliot Berman wrote:
>> Hi Sudeep,
>>
>> On Mon, Jun 24, 2024 at 04:41:42PM +0100, Sudeep Holla wrote:
>>> Sorry, I completely missed to see that you had already answered those
>>> in your commit message. As mentioned earlier I haven't looked at the
>>> reboot mode framework completely yet, so I can't comment on it yet.
>>>
>>> I don't want to be blocker though if others are happy with this.
>>
>> I think folks are satisfied with the other parts of the series and now
>> looking for your conclusion on the PSCI driver part.
> 
> I will be nice to get these patches picked up before 4th July holiday in US :).

Sorry to bug you again Sudeep - but I need confirmation that these patches looks good to you
and you will pick them up. Thanks. 
 

-- 
---Trilok Soni

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Sudeep Holla]

On Mon, Jul 08, 2024 at 08:50:58PM -0700, Trilok Soni wrote:
> On 7/2/2024 4:42 PM, Trilok Soni wrote:
> > On 7/2/2024 4:06 PM, Elliot Berman wrote:
> >> Hi Sudeep,
> >>
> >> On Mon, Jun 24, 2024 at 04:41:42PM +0100, Sudeep Holla wrote:
> >>> Sorry, I completely missed to see that you had already answered those
> >>> in your commit message. As mentioned earlier I haven't looked at the
> >>> reboot mode framework completely yet, so I can't comment on it yet.
> >>>
> >>> I don't want to be blocker though if others are happy with this.
> >>
> >> I think folks are satisfied with the other parts of the series and now
> >> looking for your conclusion on the PSCI driver part.
> >
> > I will be nice to get these patches picked up before 4th July holiday in US :).
>

July 3rd was already late to target v6.11 😉, the merge window may open next
Sunday. Ideally we prefer to have code reviewed and merged before previous
-rc6 so that it spends couple of weeks in -next before the merge. If I were
to merge, I freeze my branch by -rc5 and send PR to Arnd after that so that
Arnd gets some time with the integration of all other PRs.

> Sorry to bug you again Sudeep - but I need confirmation that these patches looks good to you
> and you will pick them up. Thanks.

FYI I am not the maintainer of PSCI. I have given my feedback but I haven't
been able to explore reset/reboot core support in much detail to provide
any further useful suggestions to move the code out of PSCI like I would
ideally like to. But that said I don't want to block this series just for
that reason.

--
Regards,
Sudeep

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Shivendra Pratap]

Tested-by: Shivendra Pratap <quic_spratap@quicinc.com> # on QCS6490-RB3GEN2, QCM6490-IDP

Can we take it up now?

On 7/9/2024 4:49 PM, Sudeep Holla wrote:
> On Mon, Jul 08, 2024 at 08:50:58PM -0700, Trilok Soni wrote:
>> On 7/2/2024 4:42 PM, Trilok Soni wrote:
>>> On 7/2/2024 4:06 PM, Elliot Berman wrote:
>>>> Hi Sudeep,
>>>>
>>>> On Mon, Jun 24, 2024 at 04:41:42PM +0100, Sudeep Holla wrote:
>>>>> Sorry, I completely missed to see that you had already answered those
>>>>> in your commit message. As mentioned earlier I haven't looked at the
>>>>> reboot mode framework completely yet, so I can't comment on it yet.
>>>>>
>>>>> I don't want to be blocker though if others are happy with this.
>>>>
>>>> I think folks are satisfied with the other parts of the series and now
>>>> looking for your conclusion on the PSCI driver part.
>>>
>>> I will be nice to get these patches picked up before 4th July holiday in US :).
>>
> 
> July 3rd was already late to target v6.11 😉, the merge window may open next
> Sunday. Ideally we prefer to have code reviewed and merged before previous
> -rc6 so that it spends couple of weeks in -next before the merge. If I were
> to merge, I freeze my branch by -rc5 and send PR to Arnd after that so that
> Arnd gets some time with the integration of all other PRs.
> 
>> Sorry to bug you again Sudeep - but I need confirmation that these patches looks good to you
>> and you will pick them up. Thanks.
> 
> FYI I am not the maintainer of PSCI. I have given my feedback but I haven't
> been able to explore reset/reboot core support in much detail to provide
> any further useful suggestions to move the code out of PSCI like I would
> ideally like to. But that said I don't want to block this series just for
> that reason.
> 
> --
> Regards,
> Sudeep

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Lorenzo Pieralisi]

On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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.

Please define "too early" (apologies if it has been explained before).

>     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.

That's surmountable I suppose.

>  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.

We could put together a PSCI "parent" driver that creates child platform
devices for idle and reboot drivers to match (which actually is not
really pretty but it would make more sense than matching the idle
driver only to the psci compatible string, which is what current code
does).

> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  drivers/firmware/psci/psci.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index d9629ff87861..e672b33b71d1 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -29,6 +29,8 @@
>  #include <asm/smp_plat.h>
>  #include <asm/suspend.h>
>  
> +#define REBOOT_PREFIX "mode-"
> +
>  /*
>   * While a 64-bit OS can make calls with SMC32 calling conventions, for some
>   * calls it is necessary to use SMC64 to pass or return 64-bit values.
> @@ -79,6 +81,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;
> +static size_t num_psci_reset_params;
> +
>  static inline bool psci_has_ext_power_state(void)
>  {
>  	return psci_cpu_suspend_feature &
> @@ -305,9 +315,29 @@ static int get_set_conduit_method(const struct device_node *np)
>  	return 0;
>  }
>  
> +static void psci_vendor_sys_reset2(unsigned long action, void *data)

'action' is unused and therefore it is not really needed.

> +{
> +	const char *cmd = data;
> +	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);
> +			pr_err("failed to perform reset \"%s\": %ld\n",
> +				cmd, (long)ret);
> +		}
> +	}
> +}
> +
>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>  			  void *data)
>  {
> +	if (data && num_psci_reset_params)

So, reboot_mode here is basically ignored; if there is a vendor defined
reset, we fire it off.

I think Mark mentioned his concerns earlier related to REBOOT_* mode and
reset type (granted, the context was different):

https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/

I would like to understand if this is the right thing to do before
accepting this patchset.

Thanks,
Lorenzo

> +		psci_vendor_sys_reset2(action, data);
> +
>  	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>  	    psci_system_reset2_supported) {
>  		/*
> @@ -748,6 +778,68 @@ static const struct of_device_id psci_of_match[] __initconst = {
>  	{},
>  };
>  
> +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_elems_of_size(np, prop->name, sizeof(magic[0]));
> +		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, 2);
> +		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 == 2 ? 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 v5 3/4] firmware: psci: Read and use vendor reset types

[Elliot Berman]

On Wed, Aug 07, 2024 at 05:02:38PM +0200, Lorenzo Pieralisi wrote:
> On Mon, Jun 17, 2024 at 10:18:09AM -0700, 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.
> 
> Please define "too early" (apologies if it has been explained before).
> 

My understanding is that reboot_notifier_list handlers shouldn't
actually be rebooting the system. I see it's generally used for one last
operation to put system into safer state. A few examples that are quick
to write based on what I see today: watchdogs disable themselves, PMUs
get torn down, xenbus tries to abort any requests. There are a couple
examples of drivers that *do* immediately reboot, but those seem to be
outlier. None of the reboot mode framework clients currently trigger
restart itself: they all set some cookies to give hint to firmware or
hardware what to do.

The restart_handler_list is documented to be a list of handlers that
should try to really restart the system. PSCI driver currently registers
against this.

> >     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.
> 
> That's surmountable I suppose.
> 
> >  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.
> 
> We could put together a PSCI "parent" driver that creates child platform
> devices for idle and reboot drivers to match (which actually is not
> really pretty but it would make more sense than matching the idle
> driver only to the psci compatible string, which is what current code
> does).
> 
> > Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > ---
> >  drivers/firmware/psci/psci.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 92 insertions(+)
> > 
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index d9629ff87861..e672b33b71d1 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -29,6 +29,8 @@
> >  #include <asm/smp_plat.h>
> >  #include <asm/suspend.h>
> >  
> > +#define REBOOT_PREFIX "mode-"
> > +
> >  /*
> >   * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> >   * calls it is necessary to use SMC64 to pass or return 64-bit values.
> > @@ -79,6 +81,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;
> > +static size_t num_psci_reset_params;
> > +
> >  static inline bool psci_has_ext_power_state(void)
> >  {
> >  	return psci_cpu_suspend_feature &
> > @@ -305,9 +315,29 @@ static int get_set_conduit_method(const struct device_node *np)
> >  	return 0;
> >  }
> >  
> > +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> 
> 'action' is unused and therefore it is not really needed.
> 
> > +{
> > +	const char *cmd = data;
> > +	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);
> > +			pr_err("failed to perform reset \"%s\": %ld\n",
> > +				cmd, (long)ret);
> > +		}
> > +	}
> > +}
> > +
> >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> >  			  void *data)
> >  {
> > +	if (data && num_psci_reset_params)
> 
> So, reboot_mode here is basically ignored; if there is a vendor defined
> reset, we fire it off.
> 
> I think Mark mentioned his concerns earlier related to REBOOT_* mode and
> reset type (granted, the context was different):
> 
> https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
> 
> I would like to understand if this is the right thing to do before
> accepting this patchset.
> 

I don't have any concerns to move this part below checking reboot_mode.
Or, I could add reboot_mode == REBOOT_COLD check.

I'm not sure how best to define the behavior if user sets
reboot_mode = REBOOT_WARM and then user does "reboot bootloader". IMO,
"REBOOT_WARM" is at odds with the "bootloader" command. We can have one
take precedence over the other. I chose for the vendor resets to take
priority, since if userspace is providing specific command at reboot
time, that's probably what they want.

Let me know your thoughts and I'm happy to change the behavior around.


Thanks,
Elliot

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Lorenzo Pieralisi]

On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:

[...]

> > > +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> > 
> > 'action' is unused and therefore it is not really needed.
> > 
> > > +{
> > > +	const char *cmd = data;
> > > +	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);
> > > +			pr_err("failed to perform reset \"%s\": %ld\n",
> > > +				cmd, (long)ret);
> > > +		}
> > > +	}
> > > +}
> > > +
> > >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> > >  			  void *data)
> > >  {
> > > +	if (data && num_psci_reset_params)
> > 
> > So, reboot_mode here is basically ignored; if there is a vendor defined
> > reset, we fire it off.
> > 
> > I think Mark mentioned his concerns earlier related to REBOOT_* mode and
> > reset type (granted, the context was different):
> > 
> > https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
> > 
> > I would like to understand if this is the right thing to do before
> > accepting this patchset.
> > 
> 
> I don't have any concerns to move this part below checking reboot_mode.
> Or, I could add reboot_mode == REBOOT_COLD check.

The question is how can we map vendor specific reboot magic to Linux
reboot modes sensibly in generic PSCI code - that's by definition
vendor specific.

Thanks,
Lorenzo

> I'm not sure how best to define the behavior if user sets
> reboot_mode = REBOOT_WARM and then user does "reboot bootloader". IMO,
> "REBOOT_WARM" is at odds with the "bootloader" command. We can have one
> take precedence over the other. I chose for the vendor resets to take
> priority, since if userspace is providing specific command at reboot
> time, that's probably what they want.
> 
> Let me know your thoughts and I'm happy to change the behavior around.
> 
> 
> Thanks,
> Elliot

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Elliot Berman]

On Fri, Aug 09, 2024 at 03:30:38PM +0200, Lorenzo Pieralisi wrote:
> On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:
> 
> [...]
> 
> > > > +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> > > 
> > > 'action' is unused and therefore it is not really needed.
> > > 
> > > > +{
> > > > +	const char *cmd = data;
> > > > +	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);
> > > > +			pr_err("failed to perform reset \"%s\": %ld\n",
> > > > +				cmd, (long)ret);
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > >  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> > > >  			  void *data)
> > > >  {
> > > > +	if (data && num_psci_reset_params)
> > > 
> > > So, reboot_mode here is basically ignored; if there is a vendor defined
> > > reset, we fire it off.
> > > 
> > > I think Mark mentioned his concerns earlier related to REBOOT_* mode and
> > > reset type (granted, the context was different):
> > > 
> > > https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
> > > 
> > > I would like to understand if this is the right thing to do before
> > > accepting this patchset.
> > > 
> > 
> > I don't have any concerns to move this part below checking reboot_mode.
> > Or, I could add reboot_mode == REBOOT_COLD check.
> 
> The question is how can we map vendor specific reboot magic to Linux
> reboot modes sensibly in generic PSCI code - that's by definition
> vendor specific.
> 

I don't think it's a reasonable thing to do. "reboot bootloader" or
"reboot edl" don't make sense to the Linux reboot modes.

I believe the Linux reboot modes enum is oriented to perspective of
Linux itself and the vendor resets are oriented towards behavior of the
SoC.

Thanks,
Elliot

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Shivendra Pratap]

On 8/9/2024 10:28 PM, Elliot Berman wrote:
> On Fri, Aug 09, 2024 at 03:30:38PM +0200, Lorenzo Pieralisi wrote:
>> On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:
>>
>> [...]
>>
>>>>> +static void psci_vendor_sys_reset2(unsigned long action, void *data)
>>>>
>>>> 'action' is unused and therefore it is not really needed.
>>>>
>>>>> +{
>>>>> +	const char *cmd = data;
>>>>> +	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);
>>>>> +			pr_err("failed to perform reset \"%s\": %ld\n",
>>>>> +				cmd, (long)ret);
>>>>> +		}
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>>>>>  			  void *data)
>>>>>  {
>>>>> +	if (data && num_psci_reset_params)
>>>>
>>>> So, reboot_mode here is basically ignored; if there is a vendor defined
>>>> reset, we fire it off.
>>>>
>>>> I think Mark mentioned his concerns earlier related to REBOOT_* mode and
>>>> reset type (granted, the context was different):
>>>>
>>>> https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
>>>>
>>>> I would like to understand if this is the right thing to do before
>>>> accepting this patchset.
>>>>
>>>
>>> I don't have any concerns to move this part below checking reboot_mode.
>>> Or, I could add reboot_mode == REBOOT_COLD check.
>>
>> The question is how can we map vendor specific reboot magic to Linux
>> reboot modes sensibly in generic PSCI code - that's by definition
>> vendor specific.
>>
> 
> I don't think it's a reasonable thing to do. "reboot bootloader" or
> "reboot edl" don't make sense to the Linux reboot modes.
> 
> I believe the Linux reboot modes enum is oriented to perspective of
> Linux itself and the vendor resets are oriented towards behavior of the
> SoC.
> 
> Thanks,
> Elliot
> 

Agree.

from perspective of linux reboot modes, kernel's current implementation in reset path is like:
__
#1 If reboot_mode is WARM/SOFT and PSCI_SYSRESET2 is supported 
    Call PSCI - SYSTEM_RESET2 - ARCH RESET
#2 ELSE
    Call PSCI - SYSTEM_RESET COLD RESET
___

ARM SPECS for PSCI SYSTEM_RESET2
This function extends SYSTEM_RESET. It provides:
• ARCH RESET: set Bit[31] to 0               = > This is already in place in condition #1.
• vendor-specific resets: set Bit[31] to 1.  = > current patchset adds this part before kernel's reboot_mode reset at #0.


In current patchset, we see a condition added at #0-psci_vendor_reset2 being called before kernel’s current reboot_mode condition and it can take any action only if all below conditions are satisfied.
- PSCI SYSTEM_RESET2 is supported.
- psci dt node defines an entry "bootloader" as a reboot-modes.
- User issues reboot with a command say - (reboot bootloader).
- If vendor reset fails, default reboot mode will execute as is.

Don't see if we will skip or break the kernel reboot_mode flow with this patch. 
Also if user issues reboot <cmd> and <cmd> is supported on SOC vendor reset psci node, should cmd take precedence over kernel reboot mode enum? may be yes? 

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Lorenzo Pieralisi]

On Mon, Aug 12, 2024 at 11:46:08PM +0530, Shivendra Pratap wrote:
> 
> 
> On 8/9/2024 10:28 PM, Elliot Berman wrote:
> > On Fri, Aug 09, 2024 at 03:30:38PM +0200, Lorenzo Pieralisi wrote:
> >> On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:
> >>
> >> [...]
> >>
> >>>>> +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> >>>>
> >>>> 'action' is unused and therefore it is not really needed.
> >>>>
> >>>>> +{
> >>>>> +	const char *cmd = data;
> >>>>> +	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);
> >>>>> +			pr_err("failed to perform reset \"%s\": %ld\n",
> >>>>> +				cmd, (long)ret);
> >>>>> +		}
> >>>>> +	}
> >>>>> +}
> >>>>> +
> >>>>>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> >>>>>  			  void *data)
> >>>>>  {
> >>>>> +	if (data && num_psci_reset_params)
> >>>>
> >>>> So, reboot_mode here is basically ignored; if there is a vendor defined
> >>>> reset, we fire it off.
> >>>>
> >>>> I think Mark mentioned his concerns earlier related to REBOOT_* mode and
> >>>> reset type (granted, the context was different):
> >>>>
> >>>> https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
> >>>>
> >>>> I would like to understand if this is the right thing to do before
> >>>> accepting this patchset.
> >>>>
> >>>
> >>> I don't have any concerns to move this part below checking reboot_mode.
> >>> Or, I could add reboot_mode == REBOOT_COLD check.
> >>
> >> The question is how can we map vendor specific reboot magic to Linux
> >> reboot modes sensibly in generic PSCI code - that's by definition
> >> vendor specific.
> >>
> > 
> > I don't think it's a reasonable thing to do. "reboot bootloader" or
> > "reboot edl" don't make sense to the Linux reboot modes.
> > 
> > I believe the Linux reboot modes enum is oriented to perspective of
> > Linux itself and the vendor resets are oriented towards behavior of the
> > SoC.
> > 
> > Thanks,
> > Elliot
> > 
> 
> Agree.
> 
> from perspective of linux reboot modes, kernel's current implementation in reset path is like:
> __
> #1 If reboot_mode is WARM/SOFT and PSCI_SYSRESET2 is supported 
>     Call PSCI - SYSTEM_RESET2 - ARCH RESET
> #2 ELSE
>     Call PSCI - SYSTEM_RESET COLD RESET
> ___
> 
> ARM SPECS for PSCI SYSTEM_RESET2
> This function extends SYSTEM_RESET. It provides:
> • ARCH RESET: set Bit[31] to 0               = > This is already in place in condition #1.
> • vendor-specific resets: set Bit[31] to 1.  = > current patchset adds this part before kernel's reboot_mode reset at #0.
> 
> 
> In current patchset, we see a condition added at #0-psci_vendor_reset2 being called before kernel’s current reboot_mode condition and it can take any action only if all below conditions are satisfied.
> - PSCI SYSTEM_RESET2 is supported.
> - psci dt node defines an entry "bootloader" as a reboot-modes.
> - User issues reboot with a command say - (reboot bootloader).
> - If vendor reset fails, default reboot mode will execute as is.
> 
> Don't see if we will skip or break the kernel reboot_mode flow with this patch. 
> Also if user issues reboot <cmd> and <cmd> is supported on SOC vendor reset psci node, should cmd take precedence over kernel reboot mode enum? may be yes? 
> 

Please wrap lines when replying.

I don't think it is a matter of precedence. reboot_mode and the reboot
command passed to the reboot() syscall are there for different (?)
reasons.

What I am asking is whether it is always safe to execute a PSCI vendor
reset irrispective of the reboot_mode value.

Lorenzo

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Elliot Berman]

On Thu, Aug 15, 2024 at 04:40:55PM +0200, Lorenzo Pieralisi wrote:
> On Mon, Aug 12, 2024 at 11:46:08PM +0530, Shivendra Pratap wrote:
> > 
> > 
> > On 8/9/2024 10:28 PM, Elliot Berman wrote:
> > > On Fri, Aug 09, 2024 at 03:30:38PM +0200, Lorenzo Pieralisi wrote:
> > >> On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:
> > >>
> > >> [...]
> > >>
> > >>>>> +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> > >>>>
> > >>>> 'action' is unused and therefore it is not really needed.
> > >>>>
> > >>>>> +{
> > >>>>> +	const char *cmd = data;
> > >>>>> +	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);
> > >>>>> +			pr_err("failed to perform reset \"%s\": %ld\n",
> > >>>>> +				cmd, (long)ret);
> > >>>>> +		}
> > >>>>> +	}
> > >>>>> +}
> > >>>>> +
> > >>>>>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> > >>>>>  			  void *data)
> > >>>>>  {
> > >>>>> +	if (data && num_psci_reset_params)
> > >>>>
> > >>>> So, reboot_mode here is basically ignored; if there is a vendor defined
> > >>>> reset, we fire it off.
> > >>>>
> > >>>> I think Mark mentioned his concerns earlier related to REBOOT_* mode and
> > >>>> reset type (granted, the context was different):
> > >>>>
> > >>>> https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
> > >>>>
> > >>>> I would like to understand if this is the right thing to do before
> > >>>> accepting this patchset.
> > >>>>
> > >>>
> > >>> I don't have any concerns to move this part below checking reboot_mode.
> > >>> Or, I could add reboot_mode == REBOOT_COLD check.
> > >>
> > >> The question is how can we map vendor specific reboot magic to Linux
> > >> reboot modes sensibly in generic PSCI code - that's by definition
> > >> vendor specific.
> > >>
> > > 
> > > I don't think it's a reasonable thing to do. "reboot bootloader" or
> > > "reboot edl" don't make sense to the Linux reboot modes.
> > > 
> > > I believe the Linux reboot modes enum is oriented to perspective of
> > > Linux itself and the vendor resets are oriented towards behavior of the
> > > SoC.
> > > 
> > > Thanks,
> > > Elliot
> > > 
> > 
> > Agree.
> > 
> > from perspective of linux reboot modes, kernel's current
> > implementation in reset path is like:
> >
> > __
> > #1 If reboot_mode is WARM/SOFT and PSCI_SYSRESET2 is supported 
> >     Call PSCI - SYSTEM_RESET2 - ARCH RESET
> > #2 ELSE
> >     Call PSCI - SYSTEM_RESET COLD RESET
> > ___
> > 
> > ARM SPECS for PSCI SYSTEM_RESET2
> > This function extends SYSTEM_RESET. It provides:
> > • ARCH RESET: set Bit[31] to 0               = > This is already in place in condition #1.
> > • vendor-specific resets: set Bit[31] to 1.  = > current patchset adds this part before kernel's reboot_mode reset at #0.
> > 
> > 
> > In current patchset, we see a condition added at
> > #0-psci_vendor_reset2 being called before kernel’s current
> > reboot_mode condition and it can take any action only if all below
> > conditions are satisfied.
> > - PSCI SYSTEM_RESET2 is supported.
> > - psci dt node defines an entry "bootloader" as a reboot-modes.
> > - User issues reboot with a command say - (reboot bootloader).
> > - If vendor reset fails, default reboot mode will execute as is.
> > 
> > Don't see if we will skip or break the kernel reboot_mode flow with
> > this patch.  Also if user issues reboot <cmd> and <cmd> is supported
> > on SOC vendor reset psci node, should cmd take precedence over
> > kernel reboot mode enum? may be yes? 
> > 
> 
> Please wrap lines when replying.
> 
> I don't think it is a matter of precedence. reboot_mode and the reboot
> command passed to the reboot() syscall are there for different (?)
> reasons.
> 
> What I am asking is whether it is always safe to execute a PSCI vendor
> reset irrispective of the reboot_mode value.

The only way I see it to be unsafe is we need some other driver using
the reboot_mode to configure something and then the PSCI vendor reset
being incompatible with whatever that other driver did. I don't see that
happens today, so it is up to us to decide what the policy ought to be.
The PSCI spec doesn't help us here because the reboot_mode enum is
totally a Linux construct. In my opinion, firmware should be able to
deal with whatever the driver did or (less ideal) the driver need to be
aware of the PSCI vendor resets. Thus, it would be always safe to
execute a PSCI vendor reset regardless of the reboot_mode value.

Thanks,
Elliot

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Shivendra Pratap]

On 8/15/2024 11:35 PM, Elliot Berman wrote:
> On Thu, Aug 15, 2024 at 04:40:55PM +0200, Lorenzo Pieralisi wrote:
>> On Mon, Aug 12, 2024 at 11:46:08PM +0530, Shivendra Pratap wrote:
>>>
>>>
>>> On 8/9/2024 10:28 PM, Elliot Berman wrote:
>>>> On Fri, Aug 09, 2024 at 03:30:38PM +0200, Lorenzo Pieralisi wrote:
>>>>> On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> +static void psci_vendor_sys_reset2(unsigned long action, void *data)
>>>>>>>
>>>>>>> 'action' is unused and therefore it is not really needed.
>>>>>>>
>>>>>>>> +{
>>>>>>>> +	const char *cmd = data;
>>>>>>>> +	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);
>>>>>>>> +			pr_err("failed to perform reset \"%s\": %ld\n",
>>>>>>>> +				cmd, (long)ret);
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>>>>>>>>  			  void *data)
>>>>>>>>  {
>>>>>>>> +	if (data && num_psci_reset_params)
>>>>>>>
>>>>>>> So, reboot_mode here is basically ignored; if there is a vendor defined
>>>>>>> reset, we fire it off.
>>>>>>>
>>>>>>> I think Mark mentioned his concerns earlier related to REBOOT_* mode and
>>>>>>> reset type (granted, the context was different):
>>>>>>>
>>>>>>> https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
>>>>>>>
>>>>>>> I would like to understand if this is the right thing to do before
>>>>>>> accepting this patchset.
>>>>>>>
>>>>>>
>>>>>> I don't have any concerns to move this part below checking reboot_mode.
>>>>>> Or, I could add reboot_mode == REBOOT_COLD check.
>>>>>
>>>>> The question is how can we map vendor specific reboot magic to Linux
>>>>> reboot modes sensibly in generic PSCI code - that's by definition
>>>>> vendor specific.
>>>>>
>>>>
>>>> I don't think it's a reasonable thing to do. "reboot bootloader" or
>>>> "reboot edl" don't make sense to the Linux reboot modes.
>>>>
>>>> I believe the Linux reboot modes enum is oriented to perspective of
>>>> Linux itself and the vendor resets are oriented towards behavior of the
>>>> SoC.
>>>>
>>>> Thanks,
>>>> Elliot
>>>>
>>>
>>> Agree.
>>>
>>> from perspective of linux reboot modes, kernel's current
>>> implementation in reset path is like:
>>>
>>> __
>>> #1 If reboot_mode is WARM/SOFT and PSCI_SYSRESET2 is supported 
>>>     Call PSCI - SYSTEM_RESET2 - ARCH RESET
>>> #2 ELSE
>>>     Call PSCI - SYSTEM_RESET COLD RESET
>>> ___
>>>
>>> ARM SPECS for PSCI SYSTEM_RESET2
>>> This function extends SYSTEM_RESET. It provides:
>>> • ARCH RESET: set Bit[31] to 0               = > This is already in place in condition #1.
>>> • vendor-specific resets: set Bit[31] to 1.  = > current patchset adds this part before kernel's reboot_mode reset at #0.
>>>
>>>
>>> In current patchset, we see a condition added at
>>> #0-psci_vendor_reset2 being called before kernel’s current
>>> reboot_mode condition and it can take any action only if all below
>>> conditions are satisfied.
>>> - PSCI SYSTEM_RESET2 is supported.
>>> - psci dt node defines an entry "bootloader" as a reboot-modes.
>>> - User issues reboot with a command say - (reboot bootloader).
>>> - If vendor reset fails, default reboot mode will execute as is.
>>>
>>> Don't see if we will skip or break the kernel reboot_mode flow with
>>> this patch.  Also if user issues reboot <cmd> and <cmd> is supported
>>> on SOC vendor reset psci node, should cmd take precedence over
>>> kernel reboot mode enum? may be yes? 
>>>
>>
>> Please wrap lines when replying.
sure. will try to take care.
>>
>> I don't think it is a matter of precedence. reboot_mode and the reboot
>> command passed to the reboot() syscall are there for different (?)
>> reasons.
>>
>> What I am asking is whether it is always safe to execute a PSCI vendor
>> reset irrispective of the reboot_mode value.
Valid point, but it depends on how we configure reboot mode and vendor reset.
If the configuration is conflicting in DT, then reboot_mode and vendor reset
may conflict and show non-predictable results.
For instance, on qcs6490, we have have nvmem-reboot-mode driver
which supports "reboot mode bootloader" function via its current DT as the PMIC
registers are accessible for write on this soc. If we enable nvmem-reboot-mode
and then configure vendor_reset2(mode-bootloader) to perform a different
function on reboot, they will conflict and may result in a non-predictable
behavior. The developer or soc vendor has to take care of this in any
case so this may be a invalid scenario?

May be vendor_reset2 gives more flexibility here on how a soc vendor may 
implement reboot modes and other vendor reset types. In case soc vendor
wants to keep some reboot mode register as open access, they can still
use reboot_mode driver and then others reboot/reset modes can be configured
via vendor_reset2.

For instance, on qcs6490, we can use nvmem-reboot-mode driver for 
"reboot mode bootloader" and use the current patch-vendor_reset2 for
"reboot mode edl". This can be configured via DT. Now even if we
enable both current-patch-vendor_reset2(reboot mode bootloader) 
and nvmem-reboot-mode (mode-bootloader) at same time on this soc,
they are harmless to each other and work as desired as both(DT entries)
align with each other and the PMIC registers are accessible to kernel. The
same thing can conflict, if we enable both drivers at same time and configure
them with conflicting parameters in DT for (reboot mode bootloader).

> 
> The only way I see it to be unsafe is we need some other driver using
> the reboot_mode to configure something and then the PSCI vendor reset
> being incompatible with whatever that other driver did. I don't see that
> happens today, so it is up to us to decide what the policy ought to be.
> The PSCI spec doesn't help us here because the reboot_mode enum is
> totally a Linux construct. In my opinion, firmware should be able to
> deal with whatever the driver did or (less ideal) the driver need to be
> aware of the PSCI vendor resets. Thus, it would be always safe to
> execute a PSCI vendor reset regardless of the reboot_mode value.
> 
> Thanks,
> Elliot
> 

Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

[Lorenzo Pieralisi]

On Thu, Aug 15, 2024 at 11:05:09AM -0700, Elliot Berman wrote:
> On Thu, Aug 15, 2024 at 04:40:55PM +0200, Lorenzo Pieralisi wrote:
> > On Mon, Aug 12, 2024 at 11:46:08PM +0530, Shivendra Pratap wrote:
> > > 
> > > 
> > > On 8/9/2024 10:28 PM, Elliot Berman wrote:
> > > > On Fri, Aug 09, 2024 at 03:30:38PM +0200, Lorenzo Pieralisi wrote:
> > > >> On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:
> > > >>
> > > >> [...]
> > > >>
> > > >>>>> +static void psci_vendor_sys_reset2(unsigned long action, void *data)
> > > >>>>
> > > >>>> 'action' is unused and therefore it is not really needed.
> > > >>>>
> > > >>>>> +{
> > > >>>>> +	const char *cmd = data;
> > > >>>>> +	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);
> > > >>>>> +			pr_err("failed to perform reset \"%s\": %ld\n",
> > > >>>>> +				cmd, (long)ret);
> > > >>>>> +		}
> > > >>>>> +	}
> > > >>>>> +}
> > > >>>>> +
> > > >>>>>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
> > > >>>>>  			  void *data)
> > > >>>>>  {
> > > >>>>> +	if (data && num_psci_reset_params)
> > > >>>>
> > > >>>> So, reboot_mode here is basically ignored; if there is a vendor defined
> > > >>>> reset, we fire it off.
> > > >>>>
> > > >>>> I think Mark mentioned his concerns earlier related to REBOOT_* mode and
> > > >>>> reset type (granted, the context was different):
> > > >>>>
> > > >>>> https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/
> > > >>>>
> > > >>>> I would like to understand if this is the right thing to do before
> > > >>>> accepting this patchset.
> > > >>>>
> > > >>>
> > > >>> I don't have any concerns to move this part below checking reboot_mode.
> > > >>> Or, I could add reboot_mode == REBOOT_COLD check.
> > > >>
> > > >> The question is how can we map vendor specific reboot magic to Linux
> > > >> reboot modes sensibly in generic PSCI code - that's by definition
> > > >> vendor specific.
> > > >>
> > > > 
> > > > I don't think it's a reasonable thing to do. "reboot bootloader" or
> > > > "reboot edl" don't make sense to the Linux reboot modes.
> > > > 
> > > > I believe the Linux reboot modes enum is oriented to perspective of
> > > > Linux itself and the vendor resets are oriented towards behavior of the
> > > > SoC.
> > > > 
> > > > Thanks,
> > > > Elliot
> > > > 
> > > 
> > > Agree.
> > > 
> > > from perspective of linux reboot modes, kernel's current
> > > implementation in reset path is like:
> > >
> > > __
> > > #1 If reboot_mode is WARM/SOFT and PSCI_SYSRESET2 is supported 
> > >     Call PSCI - SYSTEM_RESET2 - ARCH RESET
> > > #2 ELSE
> > >     Call PSCI - SYSTEM_RESET COLD RESET
> > > ___
> > > 
> > > ARM SPECS for PSCI SYSTEM_RESET2
> > > This function extends SYSTEM_RESET. It provides:
> > > • ARCH RESET: set Bit[31] to 0               = > This is already in place in condition #1.
> > > • vendor-specific resets: set Bit[31] to 1.  = > current patchset adds this part before kernel's reboot_mode reset at #0.
> > > 
> > > 
> > > In current patchset, we see a condition added at
> > > #0-psci_vendor_reset2 being called before kernel’s current
> > > reboot_mode condition and it can take any action only if all below
> > > conditions are satisfied.
> > > - PSCI SYSTEM_RESET2 is supported.
> > > - psci dt node defines an entry "bootloader" as a reboot-modes.
> > > - User issues reboot with a command say - (reboot bootloader).
> > > - If vendor reset fails, default reboot mode will execute as is.
> > > 
> > > Don't see if we will skip or break the kernel reboot_mode flow with
> > > this patch.  Also if user issues reboot <cmd> and <cmd> is supported
> > > on SOC vendor reset psci node, should cmd take precedence over
> > > kernel reboot mode enum? may be yes? 
> > > 
> > 
> > Please wrap lines when replying.
> > 
> > I don't think it is a matter of precedence. reboot_mode and the reboot
> > command passed to the reboot() syscall are there for different (?)
> > reasons.
> > 
> > What I am asking is whether it is always safe to execute a PSCI vendor
> > reset irrispective of the reboot_mode value.
> 
> The only way I see it to be unsafe is we need some other driver using
> the reboot_mode to configure something and then the PSCI vendor reset
> being incompatible with whatever that other driver did. I don't see that
> happens today, so it is up to us to decide what the policy ought to be.
> The PSCI spec doesn't help us here because the reboot_mode enum is
> totally a Linux construct. In my opinion, firmware should be able to
> deal with whatever the driver did or (less ideal) the driver need to be
> aware of the PSCI vendor resets. Thus, it would be always safe to
> execute a PSCI vendor reset regardless of the reboot_mode value.

It is hard to understand history behind reboot_mode and
the LINUX_REBOOT_CMD_RESTART2 cmd, at least *I* don't
understand it fully.

What I do understand is:

- reboot_mode can be set from userspace and kernel params
- It affects some drivers restart handler behaviours
- Incidentally, I noticed that reboot_mode affects the EFI reset
  being issued (and EFI ignores the cmd and platform specific
  resets AFAICS). This is not related to this thread but may provide
  some guidance
- if reboot_mode is set to REBOOT_GPIO - it is impossible to understand
  what PSCI code should do other than ignoring it ? It is not that
  REBOOT_WARM/COLD/HARD/SOFT are easier to fathom either to be honest,
  would be happy if anyone could chime in and shed some light.

My biggest fear here is that after merging this code, various quirks
based on what SYSTEM_RESET2 platform specific parameters are set-up
will appear, whereby a driver needs to do this or that in its restart
handler depending on the specific reset being issued in PSCI
(an example was provided in this same thread).

Thoughts ? I'd like to see some progress on this but it is proving
to be ways more complex than I thought initially.

Thanks,
Lorenzo