[PATCH 0/4] arm64: qcom: fix the reboot reason handling on sa8775p

[PATCH 0/4] arm64: qcom: fix the reboot reason handling on sa8775p

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

SA8775P uses nvmem to pass the reboot reason magic value to the bootloader.
Remove the reboot modes from the PON node and introduce an SDAM node passed
to the nvmem-reboot-mode driver. While at it: convert the bindings for
nvmem-reboot-mode to YAML and enable it for arm64 in defconfig.

Bartosz Golaszewski (2):
  arm64: defconfig: enable building the nvmem-reboot-mode module
  dt-bindings: power: reset: convert nvmem-reboot-mode bindings to YAML

Parikshit Pareek (2):
  arm64: dts: qcom: sa8775p: pmic: remove the PON modes
  arm64: dts: qcom: sa8775p: pmic: add the sdam_0 node

 .../power/reset/nvmem-reboot-mode.txt         | 26 ----------
 .../power/reset/nvmem-reboot-mode.yaml        | 52 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi   | 23 +++++++-
 arch/arm64/configs/defconfig                  |  1 +
 4 files changed, 74 insertions(+), 28 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt
 create mode 100644 Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml

-- 
2.37.2

[PATCH 1/4] arm64: dts: qcom: sa8775p: pmic: remove the PON modes

[Bartosz Golaszewski]

From: Parikshit Pareek <quic_ppareek@quicinc.com>

Remove the power on reasons with reboot from the pmm8654au_0_pon.
Instead, the PoN reaons should be part of different sdam_0 mode, to
be interoduced.

Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
index 7602cca47bae..5abdc239d3a6 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
@@ -108,8 +108,6 @@ pmm8654au_0_pon: pon@1200 {
 			compatible = "qcom,pmk8350-pon";
 			reg = <0x1200>, <0x800>;
 			reg-names = "hlos", "pbs";
-			mode-recovery = <0x1>;
-			mode-bootloader = <0x2>;
 
 			pmm8654au_0_pon_pwrkey: pwrkey {
 				compatible = "qcom,pmk8350-pwrkey";
-- 
2.37.2

Re: [PATCH 1/4] arm64: dts: qcom: sa8775p: pmic: remove the PON modes

[Krzysztof Kozlowski]

On 13/04/2023 15:17, Bartosz Golaszewski wrote:
> From: Parikshit Pareek <quic_ppareek@quicinc.com>
> 
> Remove the power on reasons with reboot from the pmm8654au_0_pon.
> Instead, the PoN reaons should be part of different sdam_0 mode, to

typo: reasons

> be interoduced.

introduced

Anyway it does not say why. Are these power reasons not correct?

> 
> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Best regards,
Krzysztof

Re: [PATCH 1/4] arm64: dts: qcom: sa8775p: pmic: remove the PON modes

[Shazad Hussain]

On 4/13/2023 9:42 PM, Krzysztof Kozlowski wrote:
> On 13/04/2023 15:17, Bartosz Golaszewski wrote:
>> From: Parikshit Pareek <quic_ppareek@quicinc.com>
>>
>> Remove the power on reasons with reboot from the pmm8654au_0_pon.
>> Instead, the PoN reaons should be part of different sdam_0 mode, to
> 
> typo: reasons
> 
>> be interoduced.
> 
> introduced
> 
> Anyway it does not say why. Are these power reasons not correct?
> 

Hi Krzysztof,
Since sm8350 the PMIC PON peripheral was split into PON_HLOS and PON_PBS
to avoid security concerns with HLOS APPS being able to trigger a PMIC
WARM_RESET unilaterally. When the split occurred, the spare registers
ended up in PON_PBS, not PON_HLOS. Thus at that time, we moved to using
an SDAM register for Linux “reboot reason” configuration. And bootloader
also SDAM register to get these reboot region data to get into
bootloader/edl, so to have this working we need to use SDAM.

>>
>> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> ---
> 
> Best regards,
> Krzysztof
> 

-Shazad

Re: [PATCH 1/4] arm64: dts: qcom: sa8775p: pmic: remove the PON modes

[Krzysztof Kozlowski]

On 18/04/2023 06:39, Shazad Hussain wrote:
> 
> 
> On 4/13/2023 9:42 PM, Krzysztof Kozlowski wrote:
>> On 13/04/2023 15:17, Bartosz Golaszewski wrote:
>>> From: Parikshit Pareek <quic_ppareek@quicinc.com>
>>>
>>> Remove the power on reasons with reboot from the pmm8654au_0_pon.
>>> Instead, the PoN reaons should be part of different sdam_0 mode, to
>>
>> typo: reasons
>>
>>> be interoduced.
>>
>> introduced
>>
>> Anyway it does not say why. Are these power reasons not correct?
>>
> 
> Hi Krzysztof,
> Since sm8350 the PMIC PON peripheral was split into PON_HLOS and PON_PBS
> to avoid security concerns with HLOS APPS being able to trigger a PMIC
> WARM_RESET unilaterally. When the split occurred, the spare registers
> ended up in PON_PBS, not PON_HLOS. Thus at that time, we moved to using
> an SDAM register for Linux “reboot reason” configuration. And bootloader
> also SDAM register to get these reboot region data to get into
> bootloader/edl, so to have this working we need to use SDAM.

The explanation of their correctness should be in commit msg.

Best regards,
Krzysztof

Re: [PATCH 1/4] arm64: dts: qcom: sa8775p: pmic: remove the PON modes

[Konrad Dybcio]

On 18.04.2023 06:39, Shazad Hussain wrote:
> 
> 
> On 4/13/2023 9:42 PM, Krzysztof Kozlowski wrote:
>> On 13/04/2023 15:17, Bartosz Golaszewski wrote:
>>> From: Parikshit Pareek <quic_ppareek@quicinc.com>
>>>
>>> Remove the power on reasons with reboot from the pmm8654au_0_pon.
>>> Instead, the PoN reaons should be part of different sdam_0 mode, to
>>
>> typo: reasons
>>
>>> be interoduced.
>>
>> introduced
>>
>> Anyway it does not say why. Are these power reasons not correct?
>>
> 
> Hi Krzysztof,
> Since sm8350 the PMIC PON peripheral was split into PON_HLOS and PON_PBS
> to avoid security concerns with HLOS APPS being able to trigger a PMIC
> WARM_RESET unilaterally. When the split occurred, the spare registers
> ended up in PON_PBS, not PON_HLOS. Thus at that time, we moved to using
> an SDAM register for Linux “reboot reason” configuration. And bootloader
> also SDAM register to get these reboot region data to get into
> bootloader/edl, so to have this working we need to use SDAM.
> 
Does that imply all PMICs following the PMK8350 scheme (separate HLOS and
PBS) should direct reboot mode writes to SDAM?

Konrad
>>>
>>> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> ---
>>
>> Best regards,
>> Krzysztof
>>
> 
> -Shazad

Re: [PATCH 1/4] arm64: dts: qcom: sa8775p: pmic: remove the PON modes

[Shazad Hussain]

On 4/18/2023 3:13 PM, Konrad Dybcio wrote:
> 
> 
> On 18.04.2023 06:39, Shazad Hussain wrote:
>>
>>
>> On 4/13/2023 9:42 PM, Krzysztof Kozlowski wrote:
>>> On 13/04/2023 15:17, Bartosz Golaszewski wrote:
>>>> From: Parikshit Pareek <quic_ppareek@quicinc.com>
>>>>
>>>> Remove the power on reasons with reboot from the pmm8654au_0_pon.
>>>> Instead, the PoN reaons should be part of different sdam_0 mode, to
>>>
>>> typo: reasons
>>>
>>>> be interoduced.
>>>
>>> introduced
>>>
>>> Anyway it does not say why. Are these power reasons not correct?
>>>
>>
>> Hi Krzysztof,
>> Since sm8350 the PMIC PON peripheral was split into PON_HLOS and PON_PBS
>> to avoid security concerns with HLOS APPS being able to trigger a PMIC
>> WARM_RESET unilaterally. When the split occurred, the spare registers
>> ended up in PON_PBS, not PON_HLOS. Thus at that time, we moved to using
>> an SDAM register for Linux “reboot reason” configuration. And bootloader
>> also SDAM register to get these reboot region data to get into
>> bootloader/edl, so to have this working we need to use SDAM.
>>
> Does that imply all PMICs following the PMK8350 scheme (separate HLOS and
> PBS) should direct reboot mode writes to SDAM?
> 
> Konrad

Yes, that's what the expectation is with bootloader using SDAM as well.

>>>>
>>>> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>> ---
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> -Shazad

-Shazad

[PATCH 2/4] arm64: defconfig: enable building the nvmem-reboot-mode module

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This module is used by the Qualcomm sa8775p platform for passing the
reboot reason to the bootloader. Enable building it in the arm64
defconfig as a module.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index e1063ab32658..5bdc9cede807 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -614,6 +614,7 @@ CONFIG_POWER_RESET_QCOM_PON=m
 CONFIG_POWER_RESET_XGENE=y
 CONFIG_POWER_RESET_SYSCON=y
 CONFIG_SYSCON_REBOOT_MODE=y
+CONFIG_NVMEM_REBOOT_MODE=m
 CONFIG_BATTERY_SBS=m
 CONFIG_BATTERY_BQ27XXX=y
 CONFIG_BATTERY_MAX17042=m
-- 
2.37.2

Re: [PATCH 2/4] arm64: defconfig: enable building the nvmem-reboot-mode module

[Krzysztof Kozlowski]

On 13/04/2023 15:17, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> This module is used by the Qualcomm sa8775p platform for passing the
> reboot reason to the bootloader. Enable building it in the arm64
> defconfig as a module.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/4] arm64: defconfig: enable building the nvmem-reboot-mode module

[Arnd Bergmann]

On Sun, Apr 16, 2023, at 17:16, Krzysztof Kozlowski wrote:
> On 13/04/2023 15:17, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> 
>> This module is used by the Qualcomm sa8775p platform for passing the
>> reboot reason to the bootloader. Enable building it in the arm64
>> defconfig as a module.
>> 
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Applied, thanks!

     Arnd

[PATCH 3/4] dt-bindings: power: reset: convert nvmem-reboot-mode bindings to YAML

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Convert the DT binding document for nvmem-reboot-mode from .txt to YAML.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../power/reset/nvmem-reboot-mode.txt         | 26 ----------
 .../power/reset/nvmem-reboot-mode.yaml        | 52 +++++++++++++++++++
 2 files changed, 52 insertions(+), 26 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt
 create mode 100644 Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml

diff --git a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt
deleted file mode 100644
index 752d6126d5da..000000000000
--- a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.txt
+++ /dev/null
@@ -1,26 +0,0 @@
-NVMEM reboot mode driver
-
-This driver gets reboot mode magic value from reboot-mode driver
-and stores it in a NVMEM cell named "reboot-mode". Then the bootloader
-can read it and take different action according to the magic
-value stored.
-
-Required properties:
-- compatible: should be "nvmem-reboot-mode".
-- nvmem-cells: A phandle to the reboot mode provided by a nvmem device.
-- nvmem-cell-names: Should be "reboot-mode".
-
-The rest of the properties should follow the generic reboot-mode description
-found in reboot-mode.txt
-
-Example:
-	reboot-mode {
-		compatible = "nvmem-reboot-mode";
-		nvmem-cells = <&reboot_mode>;
-		nvmem-cell-names = "reboot-mode";
-
-		mode-normal     = <0xAAAA5501>;
-		mode-bootloader = <0xBBBB5500>;
-		mode-recovery   = <0xCCCC5502>;
-		mode-test       = <0xDDDD5503>;
-	};
diff --git a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
new file mode 100644
index 000000000000..64a7d224c7dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/reset/nvmem-reboot-mode.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic NVMEM reboot mode driver
+
+maintainers:
+  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
+
+description: |
+  This driver gets the reboot mode magic value from the reboot-mode driver
+  and stores it in the NVMEM cell named "reboot-mode". The bootloader can
+  then read it and take different action according to the value.
+
+properties:
+  compatible:
+    const: nvmem-reboot-mode
+
+  nvmem-cells:
+    description: |
+      A phandle pointing to the nvmem-cells node where the vendor-specific
+      magic value representing the reboot mode is stored.
+    maxItems: 1
+
+  nvmem-cell-names:
+    items:
+      - const: reboot-mode
+
+patternProperties:
+  "^mode-.+":
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Vendor-specific mode value written to the mode register
+
+additionalProperties: false
+
+required:
+  - compatible
+  - nvmem-cells
+  - nvmem-cell-names
+
+examples:
+  - |
+    reboot-mode {
+        compatible = "nvmem-reboot-mode";
+        nvmem-cells = <&reboot_reason>;
+        nvmem-cell-names = "reboot-mode";
+        mode-recovery = <0x01>;
+        mode-bootloader = <0x02>;
+    };
+...
-- 
2.37.2

Re: [PATCH 3/4] dt-bindings: power: reset: convert nvmem-reboot-mode bindings to YAML

[Krzysztof Kozlowski]

On 13/04/2023 15:17, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Convert the DT binding document for nvmem-reboot-mode from .txt to YAML.

Thank you for your patch. There is something to discuss/improve.

> diff --git a/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
> new file mode 100644
> index 000000000000..64a7d224c7dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/nvmem-reboot-mode.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/nvmem-reboot-mode.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic NVMEM reboot mode driver

Drop "driver".
> +
> +maintainers:
> +  - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  This driver gets the reboot mode magic value from the reboot-mode driver
> +  and stores it in the NVMEM cell named "reboot-mode". The bootloader can
> +  then read it and take different action according to the value.
> +
> +properties:
> +  compatible:
> +    const: nvmem-reboot-mode
> +
> +  nvmem-cells:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      A phandle pointing to the nvmem-cells node where the vendor-specific
> +      magic value representing the reboot mode is stored.
> +    maxItems: 1
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: reboot-mode
> +
> +patternProperties:
> +  "^mode-.+":
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Vendor-specific mode value written to the mode register
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - nvmem-cells
> +  - nvmem-cell-names

put required: before additionalProperties

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 4/4] arm64: dts: qcom: sa8775p: pmic: add the sdam_0 node

[Bartosz Golaszewski]

From: Parikshit Pareek <quic_ppareek@quicinc.com>

Introduce sdam_0 node, which is to be used via nvmem for power on
reasons during reboot. Add supported PoN reaons supported via sdam_0
node.

Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
index 5abdc239d3a6..49bf7b08f5b6 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
@@ -88,6 +88,14 @@ trip1 {
 			};
 		};
 	};
+
+	reboot_reason {
+		compatible = "nvmem-reboot-mode";
+		nvmem-cells = <&reboot_reason>;
+		nvmem-cell-names = "reboot-mode";
+		mode-recovery = <0x01>;
+		mode-bootloader = <0x02>;
+	};
 };
 
 &spmi_bus {
@@ -133,6 +141,19 @@ pmm8654au_0_gpios: gpio@8800 {
 			interrupt-controller;
 			#interrupt-cells = <2>;
 		};
+
+		pmm8654au_0_sdam_0: nvram@7100 {
+			compatible = "qcom,spmi-sdam";
+			reg = <0x7100>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x7100 0x100>;
+
+			reboot_reason: reboot-reason@48 {
+				reg = <0x48 0x1>;
+				bits = <1 7>;
+			};
+		};
 	};
 
 	pmm8654au_1: pmic@2 {
-- 
2.37.2

Re: [PATCH 4/4] arm64: dts: qcom: sa8775p: pmic: add the sdam_0 node

[Krzysztof Kozlowski]

On 13/04/2023 15:17, Bartosz Golaszewski wrote:
> From: Parikshit Pareek <quic_ppareek@quicinc.com>
> 
> Introduce sdam_0 node, which is to be used via nvmem for power on
> reasons during reboot. Add supported PoN reaons supported via sdam_0
> node.
> 
> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
> index 5abdc239d3a6..49bf7b08f5b6 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-pmics.dtsi
> @@ -88,6 +88,14 @@ trip1 {
>  			};
>  		};
>  	};
> +
> +	reboot_reason {

No underscores in node names. This is almost always called reboot-mode.
Please, do not use downstream naming and conventions.

> +		compatible = "nvmem-reboot-mode";
> +		nvmem-cells = <&reboot_reason>;
> +		nvmem-cell-names = "reboot-mode";
> +		mode-recovery = <0x01>;
> +		mode-bootloader = <0x02>;
> +	};
>  };
Best regards,
Krzysztof

Re: [PATCH 0/4] arm64: qcom: fix the reboot reason handling on sa8775p

[Arnd Bergmann]

On Thu, Apr 13, 2023, at 15:17, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> SA8775P uses nvmem to pass the reboot reason magic value to the bootloader.
> Remove the reboot modes from the PON node and introduce an SDAM node passed
> to the nvmem-reboot-mode driver. While at it: convert the bindings for
> nvmem-reboot-mode to YAML and enable it for arm64 in defconfig.
>
> Bartosz Golaszewski (2):
>   arm64: defconfig: enable building the nvmem-reboot-mode module
>   dt-bindings: power: reset: convert nvmem-reboot-mode bindings to YAML
>
> Parikshit Pareek (2):
>   arm64: dts: qcom: sa8775p: pmic: remove the PON modes
>   arm64: dts: qcom: sa8775p: pmic: add the sdam_0 node

It looks like it's too late for the dts patches in v6.4, but I was
picking up defconfig patches from the list, and that one seems useful
regardless of the rest, so I added it to the soc/defconfig branch.

     Arnd