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

[PATCH v2 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, but not 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 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    | 38 ++++++++-
 .../bindings/power/reset/nvmem-reboot-mode.yaml    |  4 +
 .../devicetree/bindings/power/reset/qcom,pon.yaml  |  4 +
 .../bindings/power/reset/reboot-mode.yaml          | 12 ++-
 .../bindings/power/reset/syscon-reboot-mode.yaml   |  4 +
 arch/arm64/boot/dts/qcom/qcm6490-idp.dts           |  5 ++
 drivers/firmware/psci/psci.c                       | 90 ++++++++++++++++++++++
 7 files changed, 153 insertions(+), 4 deletions(-)
---
base-commit: b5d2afe8745bf3eef5a59a13313798d24f2af983
change-id: 20231016-arm-psci-system_reset2-vendor-reboots-cc3ad456c070

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

[PATCH v2 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  |  4 ++++
 .../devicetree/bindings/power/reset/reboot-mode.yaml         | 12 ++++++++++--
 .../devicetree/bindings/power/reset/syscon-reboot-mode.yaml  |  4 ++++
 4 files changed, 22 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..03b3b9be36de 100644
--- a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
+++ b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
@@ -30,6 +30,10 @@ properties:
 
 allOf:
   - $ref: reboot-mode.yaml#
+  - patternProperties:
+      "^mode-.*$":
+        items:
+          maxItems: 1
 
 required:
   - compatible
diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
index fc8105a7b9b2..95964e04d5d6 100644
--- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
+++ b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
@@ -68,6 +68,10 @@ allOf:
     then:
       allOf:
         - $ref: reboot-mode.yaml#
+        - patternProperties:
+            "^mode-.*$":
+              items:
+                maxItems: 1
 
       properties:
         reg:
diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml
index ad0a0b95cec1..b9617b8880c3 100644
--- a/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml
+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.yaml
@@ -28,13 +28,21 @@ description: |
 
 properties:
   mode-normal:
-    $ref: /schemas/types.yaml#/definitions/uint32
+    $ref: "#/patternProperties/^mode-.*$"
     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-matrix
+    # Note: to reference this schema, the allOf should include hint about
+    # maxItems for the inner cell
+    # allOf:
+    # - $ref: /schemas/power/reset/reboot-mode.yaml#
+    # - patternProperties:
+    #     "^mode-.*$":
+    #       items:
+    #         maxItems: 1
 
 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..bf6d68355e7f 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
@@ -31,6 +31,10 @@ properties:
 
 allOf:
   - $ref: reboot-mode.yaml#
+  - patternProperties:
+      "^mode-.*$":
+        items:
+          maxItems: 1
 
 unevaluatedProperties: false
 

-- 
2.34.1

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

[Krzysztof Kozlowski]

On 14/04/2024 21:30, 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  |  4 ++++
>  .../devicetree/bindings/power/reset/reboot-mode.yaml         | 12 ++++++++++--
>  .../devicetree/bindings/power/reset/syscon-reboot-mode.yaml  |  4 ++++
>  4 files changed, 22 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..03b3b9be36de 100644
> --- a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
> +++ b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
> @@ -30,6 +30,10 @@ properties:
>  
>  allOf:
>    - $ref: reboot-mode.yaml#
> +  - patternProperties:
> +      "^mode-.*$":
> +        items:
> +          maxItems: 1

You still need to limit total number of items. This only defines how
many items you have in each inner cell of the matrix. What about the
other cell?

I understood that you want something more or less equivalent, but the
code does not look like.



Best regards,
Krzysztof

[PATCH v2 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 | 38 +++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
index cbb012e217ab..ac778274b3ac 100644
--- a/Documentation/devicetree/bindings/arm/psci.yaml
+++ b/Documentation/devicetree/bindings/arm/psci.yaml
@@ -137,8 +137,31 @@ allOf:
       required:
         - cpu_off
         - cpu_on
-
-additionalProperties: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: arm,psci-1.0
+    then:
+      allOf:
+        - $ref: /schemas/power/reset/reboot-mode.yaml#
+        - properties:
+            # "mode-normal" is just SYSTEM_RESET
+            mode-normal: false
+        - patternProperties:
+            "^mode-.*$":
+              items:
+                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.
+
+unevaluatedProperties: false
 
 examples:
   - |+
@@ -261,4 +284,15 @@ examples:
         domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWRDN>;
       };
     };
+
+  - |+
+
+    // Case 5: SYSTEM_RESET2 vendor resets
+    psci {
+      compatible = "arm,psci-1.0";
+      method = "smc";
+
+      mode-edl = <0>;
+      mode-bootloader = <1 2>;
+    };
 ...

-- 
2.34.1

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

[Konrad Dybcio]

On 4/14/24 21:30, 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>
> ---

[...]


> +
> +    // Case 5: SYSTEM_RESET2 vendor resets
> +    psci {
> +      compatible = "arm,psci-1.0";
> +      method = "smc";
> +
> +      mode-edl = <0>;
> +      mode-bootloader = <1 2>;

These properties seem overly generic, and with PSCI only growing every
now and then, I think a reboot-mode (or similar) subnode would be in
order here

Konrad

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

[Sudeep Holla]

On Sun, Apr 14, 2024 at 12:30:25PM -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.
>

IIUC, PSCI SYSTEM_RESET will be always supported, so the choice of using
SYSTEM_RESET2 sounds like a system policy and must not have any information
in the device tree. All required support from PSCI is discoverable and
the policy choice must be userspace driven. That's my opinion.

-- 
Regards,
Sudeep

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

[Dmitry Baryshkov]

On Tue, 16 Apr 2024 at 12:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Sun, Apr 14, 2024 at 12:30:25PM -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.
> >
>
> IIUC, PSCI SYSTEM_RESET will be always supported, so the choice of using
> SYSTEM_RESET2 sounds like a system policy and must not have any information
> in the device tree. All required support from PSCI is discoverable and
> the policy choice must be userspace driven. That's my opinion.

Well, we are talking about the vendor-specific resets, which are not
discoverable. The spec defines them as "implementation defined".
For example, PCSI spec doesn't define a way to add "reset to
bootloader" or "reset to recovery" kinds of resets.

I think the bindings at least should make it clear that the vendor bit
it being set automatically. I won't comment whether this is a good
decision or not.

-- 
With best wishes
Dmitry

[PATCH v2 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.

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.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 drivers/firmware/psci/psci.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index d9629ff87861..e0a764743a20 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,66 @@ 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 *np;
+	struct property *prop;
+	size_t count = 0;
+	u32 magic[2];
+	int num;
+
+	if (!psci_system_reset2_supported)
+		return 0;
+
+	np = of_find_matching_node(NULL, psci_of_match);
+	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) {
+		of_node_put(np);
+		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++;
+	}
+
+	of_node_put(np);
+	return 0;
+}
+arch_initcall(psci_init_system_reset2_modes);
+
 int __init psci_dt_init(void)
 {
 	struct device_node *np;

-- 
2.34.1

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

[Konrad Dybcio]

On 4/14/24 21:30, 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.
> 
> 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.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---

[...]

> +arch_initcall(psci_init_system_reset2_modes);

Perhaps this could be called from \/

Konrad

> +
>   int __init psci_dt_init(void)
>   {
>   	struct device_node *np;
> 

[PATCH v2 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
index e4bfad50a669..a966f6c8dd7c 100644
--- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
+++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
@@ -126,6 +126,11 @@ debug_vm_mem: debug-vm@d0600000 {
 		};
 	};
 
+	psci {
+		mode-bootloader = <0x10001 0x2>;
+		mode-edl = <0 0x1>;
+	};
+
 	vph_pwr: vph-pwr-regulator {
 		compatible = "regulator-fixed";
 		regulator-name = "vph_pwr";

-- 
2.34.1

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

[Dmitry Baryshkov]

On Sun, 14 Apr 2024 at 22:32, Elliot Berman <quic_eberman@quicinc.com> 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>
> ---
>  arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> index e4bfad50a669..a966f6c8dd7c 100644
> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> @@ -126,6 +126,11 @@ debug_vm_mem: debug-vm@d0600000 {
>                 };
>         };
>
> +       psci {

Please use a label instead. Otherwise it looks as if you are adding
new device node.

> +               mode-bootloader = <0x10001 0x2>;
> +               mode-edl = <0 0x1>;
> +       };
> +
>         vph_pwr: vph-pwr-regulator {
>                 compatible = "regulator-fixed";
>                 regulator-name = "vph_pwr";
>
> --
> 2.34.1
>
>


-- 
With best wishes
Dmitry

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

[Elliot Berman]

On Mon, Apr 15, 2024 at 02:13:29AM +0300, Dmitry Baryshkov wrote:
> On Sun, 14 Apr 2024 at 22:32, Elliot Berman <quic_eberman@quicinc.com> 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>
> > ---
> >  arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > index e4bfad50a669..a966f6c8dd7c 100644
> > --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > @@ -126,6 +126,11 @@ debug_vm_mem: debug-vm@d0600000 {
> >                 };
> >         };
> >
> > +       psci {
> 
> Please use a label instead. Otherwise it looks as if you are adding
> new device node.
> 

Right. Fixed for the next revision.

Thanks,
Elliot

> > +               mode-bootloader = <0x10001 0x2>;
> > +               mode-edl = <0 0x1>;
> > +       };
> > +
> >         vph_pwr: vph-pwr-regulator {
> >                 compatible = "regulator-fixed";
> >                 regulator-name = "vph_pwr";
> >
> > --
> > 2.34.1
> >
> >
> 
> 
> -- 
> With best wishes
> Dmitry

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

[Konrad Dybcio]

On 4/15/24 02:32, Elliot Berman wrote:
> On Mon, Apr 15, 2024 at 02:13:29AM +0300, Dmitry Baryshkov wrote:
>> On Sun, 14 Apr 2024 at 22:32, Elliot Berman <quic_eberman@quicinc.com> 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>
>>> ---
>>>   arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>> index e4bfad50a669..a966f6c8dd7c 100644
>>> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>> @@ -126,6 +126,11 @@ debug_vm_mem: debug-vm@d0600000 {
>>>                  };
>>>          };
>>>
>>> +       psci {
>>
>> Please use a label instead. Otherwise it looks as if you are adding
>> new device node.
>>
> 
> Right. Fixed for the next revision.

Are you guys planning to make this sorta ABI-like?

If so (which would be greatly appreciated by the way..), perhaps you
could stick these magic values in dt-bindings and give them cool names

FWIW DEN0022 (my second-favorite book) suggests these values are almost
totally vendor-defined, so if I were Qualcomm, I'd take the creative
liberty to come up with a set of numbers and never ever ever change
them

Konrad

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

[Elliot Berman]

On Mon, Apr 15, 2024 at 09:42:40PM +0200, Konrad Dybcio wrote:
> 
> 
> On 4/15/24 02:32, Elliot Berman wrote:
> > On Mon, Apr 15, 2024 at 02:13:29AM +0300, Dmitry Baryshkov wrote:
> > > On Sun, 14 Apr 2024 at 22:32, Elliot Berman <quic_eberman@quicinc.com> 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>
> > > > ---
> > > >   arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 5 +++++
> > > >   1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > > > index e4bfad50a669..a966f6c8dd7c 100644
> > > > --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > > > +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > > > @@ -126,6 +126,11 @@ debug_vm_mem: debug-vm@d0600000 {
> > > >                  };
> > > >          };
> > > > 
> > > > +       psci {
> > > 
> > > Please use a label instead. Otherwise it looks as if you are adding
> > > new device node.
> > > 
> > 
> > Right. Fixed for the next revision.
> 
> Are you guys planning to make this sorta ABI-like?
> 
> If so (which would be greatly appreciated by the way..), perhaps you
> could stick these magic values in dt-bindings and give them cool names
> 
> FWIW DEN0022 (my second-favorite book) suggests these values are almost
> totally vendor-defined, so if I were Qualcomm, I'd take the creative
> liberty to come up with a set of numbers and never ever ever change
> them

This is my goal as well. I'd like to keep the magic values out of
dt-bindings until we get the vendor SYSTEM_RESET2 spread across more
devices, as things might need a bit of settling. Since having stable
vendor reset2 is (IMO) primarily a benefit to Qualcomm, I expect this
will happen naturally.

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Sudeep Holla]

On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.

That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
expected ? Does it mean it is not conformant to the specification ?

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

Why can't this be fully userspace driven ? What is the need to keep the
cookie in the DT ?


-- 
Regards,
Sudeep

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On 4/16/24 02:35, Sudeep Holla wrote:
> On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
> 
> That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> expected ? Does it mean it is not conformant to the specification ?
> 
>> 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.
>>
> 
> Why can't this be fully userspace driven ? What is the need to keep the
> cookie in the DT ?
> 
> 

Using the second example in the Device Tree:

mode-bootloader = <1 2>;

are you suggesting that within psci_vendor_sys_reset2() we would look at 
the data argument and assume that we have something like this in memory:

const char *cmd = data;

cmd[] = "bootloader 2"

where "bootloader" is the reboot command, and "2" is the cookie? From an 
util-linux, busybox, toybox, etc. we would have to concatenate those 
arguments with a space, but I suppose that would be doable.

For the use cases that I am after we did not have a need for the cookie, 
so I admit I did not think too much about it.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Sudeep Holla]

On Wed, Apr 17, 2024 at 10:50:07AM -0700, Florian Fainelli wrote:
> On 4/16/24 02:35, Sudeep Holla wrote:
> > On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
> >
> > That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> > expected ? Does it mean it is not conformant to the specification ?
> >
> > > 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.
> > >
> >
> > Why can't this be fully userspace driven ? What is the need to keep the
> > cookie in the DT ?
> >
> >
>
> Using the second example in the Device Tree:
>
> mode-bootloader = <1 2>;
>
> are you suggesting that within psci_vendor_sys_reset2() we would look at the
> data argument and assume that we have something like this in memory:
>
> const char *cmd = data;
>
> cmd[] = "bootloader 2"
>
> where "bootloader" is the reboot command, and "2" is the cookie? From an
> util-linux, busybox, toybox, etc. we would have to concatenate those
> arguments with a space, but I suppose that would be doable.
>

Yes that was my thought when I wrote the email. But since I have looked at
existing bindings and support in the kernel in little more detail I would say.
So I am not sure what would be the better choice for PSCI SYSTEM_RESET2
especially when there is some ground support to build.

So I am open for alternatives including this approach.

--
Regards,
Sudeep

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Elliot Berman]

On Fri, Apr 19, 2024 at 01:38:47PM +0100, Sudeep Holla wrote:
> On Wed, Apr 17, 2024 at 10:50:07AM -0700, Florian Fainelli wrote:
> > On 4/16/24 02:35, Sudeep Holla wrote:
> > > On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
> > >
> > > That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> > > expected ? Does it mean it is not conformant to the specification ?
> > >
> > > > 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.
> > > >
> > >
> > > Why can't this be fully userspace driven ? What is the need to keep the
> > > cookie in the DT ?
> > >
> > >
> >
> > Using the second example in the Device Tree:
> >
> > mode-bootloader = <1 2>;
> >
> > are you suggesting that within psci_vendor_sys_reset2() we would look at the
> > data argument and assume that we have something like this in memory:
> >
> > const char *cmd = data;
> >
> > cmd[] = "bootloader 2"
> >
> > where "bootloader" is the reboot command, and "2" is the cookie? From an
> > util-linux, busybox, toybox, etc. we would have to concatenate those
> > arguments with a space, but I suppose that would be doable.
> >
> 
> Yes that was my thought when I wrote the email. But since I have looked at
> existing bindings and support in the kernel in little more detail I would say.
> So I am not sure what would be the better choice for PSCI SYSTEM_RESET2
> especially when there is some ground support to build.
> 
> So I am open for alternatives including this approach.

If we can't go with the DT approach, my preference would be to go with a
bootconfig and sysfs for controlling the mappings, although I don't
think userspace need/should control the mappings of cmd -> cookies.

I wanted to check if you are okay with proceeding with the reboot-mode
DT bindings approach unless we have some other better standard? If yes,
do you have any preference based on Konrad's comment [1]? I can send out
v3 with the couple comments from Dmitry and Krzysztof's addressed.

Thanks,
Elliot

[1]: https://lore.kernel.org/all/20240419123847.ica22nft3sejqnm7@bogus/

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Elliot Berman]

On Tue, Apr 16, 2024 at 10:35:22AM +0100, Sudeep Holla wrote:
> On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
> 
> That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> expected ? Does it mean it is not conformant to the specification ?
> 

I was motivating the reason for using SYSTEM_RESET2. How to set the PMIC
register and IMEM cookie can change between chipsets. Using
SYSTEM_RESET2 alows us to abstract how to perform the reset.

> > 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.
> >
> 
> Why can't this be fully userspace driven ? What is the need to keep the
> cookie in the DT ?

As Dmitry pointed out, this information isn't discoverable. I suppose
we could technically use bootconfig or kernel command-line to convey the
map although I think devicetree is the right spot for this mapping.

- Other vendor-specific bits for PSCI are described in the devicetree.
  One example is the suspend param (e.g. the StateID) for cpu idle
  states.
- Describing firmware bits in the DT isn't unprecedented, and putting
  this information outside the DT means that other OSes (besides Linux)
  need their own way to convey this information.
- PSCI would be the odd one out that reboot mode map is not described in
  DT. Other reboot-mode drivers specify the mapping in the DT. Userspace
  that runs with firmware that support vendor reset2 need to make sure
  they can configure the mapping early enough.

Thanks,
Elliot

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

On 4/17/24 14:54, Elliot Berman wrote:
> On Tue, Apr 16, 2024 at 10:35:22AM +0100, Sudeep Holla wrote:
>> On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
>>
>> That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
>> expected ? Does it mean it is not conformant to the specification ?
>>
> 
> I was motivating the reason for using SYSTEM_RESET2. How to set the PMIC
> register and IMEM cookie can change between chipsets. Using
> SYSTEM_RESET2 alows us to abstract how to perform the reset.
> 
>>> 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.
>>>
>>
>> Why can't this be fully userspace driven ? What is the need to keep the
>> cookie in the DT ?
> 
> As Dmitry pointed out, this information isn't discoverable. I suppose
> we could technically use bootconfig or kernel command-line to convey the
> map although I think devicetree is the right spot for this mapping.
> 
> - Other vendor-specific bits for PSCI are described in the devicetree.
>    One example is the suspend param (e.g. the StateID) for cpu idle
>    states.
> - Describing firmware bits in the DT isn't unprecedented, and putting
>    this information outside the DT means that other OSes (besides Linux)
>    need their own way to convey this information.
> - PSCI would be the odd one out that reboot mode map is not described in
>    DT. Other reboot-mode drivers specify the mapping in the DT. Userspace
>    that runs with firmware that support vendor reset2 need to make sure
>    they can configure the mapping early enough.

FWIW, I read Sudeep's response as being specifically inquiring about the 
'cookie' parameter, do you see a need for that to be in described in the 
DT or could that just be an user-space parameter that is conveyed 
through the reboot system call?
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Elliot Berman]

On Wed, Apr 17, 2024 at 03:01:00PM -0700, Florian Fainelli wrote:
> On 4/17/24 14:54, Elliot Berman wrote:
> > On Tue, Apr 16, 2024 at 10:35:22AM +0100, Sudeep Holla wrote:
> > > On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
> > > 
> > > That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> > > expected ? Does it mean it is not conformant to the specification ?
> > > 
> > 
> > I was motivating the reason for using SYSTEM_RESET2. How to set the PMIC
> > register and IMEM cookie can change between chipsets. Using
> > SYSTEM_RESET2 alows us to abstract how to perform the reset.
> > 
> > > > 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.
> > > > 
> > > 
> > > Why can't this be fully userspace driven ? What is the need to keep the
> > > cookie in the DT ?
> > 
> > As Dmitry pointed out, this information isn't discoverable. I suppose
> > we could technically use bootconfig or kernel command-line to convey the
> > map although I think devicetree is the right spot for this mapping.
> > 
> > - Other vendor-specific bits for PSCI are described in the devicetree.
> >    One example is the suspend param (e.g. the StateID) for cpu idle
> >    states.
> > - Describing firmware bits in the DT isn't unprecedented, and putting
> >    this information outside the DT means that other OSes (besides Linux)
> >    need their own way to convey this information.
> > - PSCI would be the odd one out that reboot mode map is not described in
> >    DT. Other reboot-mode drivers specify the mapping in the DT. Userspace
> >    that runs with firmware that support vendor reset2 need to make sure
> >    they can configure the mapping early enough.
> 
> FWIW, I read Sudeep's response as being specifically inquiring about the
> 'cookie' parameter, do you see a need for that to be in described in the DT
> or could that just be an user-space parameter that is conveyed through the
> reboot system call?

Ah, I had thought the ask was for the reboot type as well as the cookie.
We don't do this for hardware-based reboot mode cookies and I didn't see
why we should ask userspace to do something different for PSCI.
It seems to me that SYSTEM_RESET2 fits nicely with the existing design
for reboot-mode bindings.

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Sudeep Holla]

On Wed, Apr 17, 2024 at 02:54:41PM -0700, Elliot Berman wrote:
> On Tue, Apr 16, 2024 at 10:35:22AM +0100, Sudeep Holla wrote:
> > On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
> >
> > That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> > expected ? Does it mean it is not conformant to the specification ?
> >
>
> I was motivating the reason for using SYSTEM_RESET2. How to set the PMIC
> register and IMEM cookie can change between chipsets. Using
> SYSTEM_RESET2 alows us to abstract how to perform the reset.

Fair enough. But I assume you are not providing the details of PMIC register
or IMEM cookie via DT.

Anyways you did confirm if PSCI SYSTEM_RESET works as expected or not. That
is default and must work.

> > > 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.
> > >
> >
> > Why can't this be fully userspace driven ? What is the need to keep the
> > cookie in the DT ?
>
> As Dmitry pointed out, this information isn't discoverable. I suppose
> we could technically use bootconfig or kernel command-line to convey the
> map although I think devicetree is the right spot for this mapping.
>

Yes and as usual DT has become dumping ground for firmware that don't
make things discoverable. Make crap that Qcom puts in the DT are firmware
related and can be make discoverable. Anyways it is sad that no efforts
to make it so are done as DT is always there to provide shortcuts.

> - Other vendor-specific bits for PSCI are described in the devicetree.
>   One example is the suspend param (e.g. the StateID) for cpu idle
>   states.

You are right, but that is the only example I can see and it was done
in very early days of PSCI. It shouldn't be example if there are better
ways.

> - Describing firmware bits in the DT isn't unprecedented, and putting
>   this information outside the DT means that other OSes (besides Linux)
>   need their own way to convey this information.

Correct but it can be Qcom specific firmware interface. There are so many
already. This splitting information between firmware and DT works well
for vertically integrated things which probably is the case with most of
Qcom SoCs but it is prone to issues if DT and firmware mismatch. Firmware
discovery eliminates such issues.

> - PSCI would be the odd one out that reboot mode map is not described in
>   DT. Other reboot-mode drivers specify the mapping in the DT. Userspace
>   that runs with firmware that support vendor reset2 need to make sure
>   they can configure the mapping early enough.
>

Well I am not saying not to this yet, just exploring and getting more info
so that whatever is done here can be reused on all PSCI based systems.

--
Regards,
Sudeep

Re: [PATCH v2 0/4] Implement vendor resets for PSCI SYSTEM_RESET2

[Elliot Berman]

On Fri, Apr 19, 2024 at 09:53:45AM +0100, Sudeep Holla wrote:
> On Wed, Apr 17, 2024 at 02:54:41PM -0700, Elliot Berman wrote:
> > On Tue, Apr 16, 2024 at 10:35:22AM +0100, Sudeep Holla wrote:
> > > On Sun, Apr 14, 2024 at 12:30:23PM -0700, 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.
> > >
> > > That doesn't sound good. Do you mean PSCI SYSTEM_RESET doesn't work as
> > > expected ? Does it mean it is not conformant to the specification ?
> > >
> >
> > I was motivating the reason for using SYSTEM_RESET2. How to set the PMIC
> > register and IMEM cookie can change between chipsets. Using
> > SYSTEM_RESET2 alows us to abstract how to perform the reset.
> 
> Fair enough. But I assume you are not providing the details of PMIC register
> or IMEM cookie via DT.

Kernel doesn't need this info.

> 
> Anyways you did confirm if PSCI SYSTEM_RESET works as expected or not. That
> is default and must work.
> 

Yes, SYSTEM_RESET works on Quacomm firmware. The bindings disallow
trying to override the default reboot. (reboot command = NULL or "") The
PSCI parsing of the DT also doesn't have any of the special handling to
deal with "mode-normal".

> > > > 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.
> > > >
> > >
> > > Why can't this be fully userspace driven ? What is the need to keep the
> > > cookie in the DT ?
> >
> > As Dmitry pointed out, this information isn't discoverable. I suppose
> > we could technically use bootconfig or kernel command-line to convey the
> > map although I think devicetree is the right spot for this mapping.
> >
> 
> Yes and as usual DT has become dumping ground for firmware that don't
> make things discoverable. Make crap that Qcom puts in the DT are firmware
> related and can be make discoverable. Anyways it is sad that no efforts
> to make it so are done as DT is always there to provide shortcuts.
> 
> > - Other vendor-specific bits for PSCI are described in the devicetree.
> >   One example is the suspend param (e.g. the StateID) for cpu idle
> >   states.
> 
> You are right, but that is the only example I can see and it was done
> in very early days of PSCI. It shouldn't be example if there are better
> ways.
> 
> > - Describing firmware bits in the DT isn't unprecedented, and putting
> >   this information outside the DT means that other OSes (besides Linux)
> >   need their own way to convey this information.
> 
> Correct but it can be Qcom specific firmware interface. There are so many
> already. This splitting information between firmware and DT works well
> for vertically integrated things which probably is the case with most of
> Qcom SoCs but it is prone to issues if DT and firmware mismatch. Firmware
> discovery eliminates such issues.
> 

I worry about designing interfaces both in Qualcomm firmware and in
the PSCI driver which doesn't really suit handling the discovery. We can
implement the dynamic discovery mechanims once there is a board which
needs it.

Thanks,
Elliot