[PATCH RFC 0/6] Add support to read the restart reason from IMEM

[PATCH RFC 0/6] Add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

In Qualcomm IPQ SoC, if the system is rebooted due to the watchdog
timeout, there is no way to identify it. Current approach of checking
the EXPIRED_STATUS in WDT_STS is not working.

To achieve this, if the system is rebooted due to watchdog timeout, the
information is captured in the IMEM by the bootloader (along with other
reason codes as well).

This series attempts to address this by adding the support to read the
IMEM and populate the information via bootstatus sysfs file. As of now,
we are handling only the non secure watchdog timeout reason.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Kathiravan Thirumoorthy (6):
      dt-bindings: sram: qcom,imem: Document IPQ5424 compatible
      arm64: dts: qcom: ipq5424: Add the IMEM node
      dt-bindings: watchdog: Add Qualcomm restart reason binding
      dt-bindings: sram: qcom,imem: add the support for restart reason
      watchdog: qcom-wdt: add support to read the restart reason from IMEM
      arm64: dts: qcom: ipq5424: add node for the restart reason information

 .../devicetree/bindings/sram/qcom,imem.yaml        | 25 ++++++++++++
 .../bindings/watchdog/qcom,restart-reason.yaml     | 46 ++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/ipq5424.dtsi              | 14 +++++++
 drivers/watchdog/qcom-wdt.c                        | 40 ++++++++++++++++++-
 4 files changed, 124 insertions(+), 1 deletion(-)
---
base-commit: 7702d0130dc002bab2c3571ddb6ff68f82d99aea
change-id: 20250408-wdt_reset_reason-e12921963fa6

Best regards,
-- 
Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>

[PATCH RFC 1/6] dt-bindings: sram: qcom,imem: Document IPQ5424 compatible

[Kathiravan Thirumoorthy]

Add compatible for Qualcomm's IPQ5424 IMEM.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/sram/qcom,imem.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 2711f90d9664b70fcd1e2f7e2dfd3386ed5c1952..dec1b1ee924cf1386f559eb262ea864f2788c165 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -18,6 +18,7 @@ properties:
     items:
       - enum:
           - qcom,apq8064-imem
+          - qcom,ipq5424-imem
           - qcom,msm8226-imem
           - qcom,msm8974-imem
           - qcom,msm8976-imem

-- 
2.34.1

[PATCH RFC 2/6] arm64: dts: qcom: ipq5424: Add the IMEM node

[Kathiravan Thirumoorthy]

Add the IMEM node to the device tree to extract debugging information
like system restart reason, which is populated via IMEM. Define the
IMEM region to enable this functionality. Corresponding DTS and driver
changes will be added incrementally.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/ipq5424.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
index 5d6ed2172b1bb0a57c593f121f387ec917f42419..a772736f314f46d11c473160c522af4edeb900b7 100644
--- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
@@ -486,6 +486,15 @@ ssphy_0: phy@7d000 {
 			status = "disabled";
 		};
 
+		sram@8600000 {
+			compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
+			reg = <0 0x08600000 0 0x1000>;
+			ranges = <0 0 0x08600000 0x1000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
+
 		usb3: usb3@8a00000 {
 			compatible = "qcom,ipq5424-dwc3", "qcom,dwc3";
 			reg = <0 0x08af8800 0 0x400>;

-- 
2.34.1

[PATCH RFC 3/6] dt-bindings: watchdog: Add Qualcomm restart reason binding

[Kathiravan Thirumoorthy]

Add a devicetree binding for the Qualcomm IPQ SOCs restart reason
information region found in the IMEM, allowing the system to identify
the cause of a restart.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 .../bindings/watchdog/qcom,restart-reason.yaml     | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml b/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..babbaa70b114f9691018ed6cb10bfa78e18fad64
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/qcom,restart-reason.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm IPQ SoC restart reason location
+
+maintainers:
+  - Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
+
+description:
+  The Qualcomm IPQ SoC restart reason memory region, in IMEM, is used to
+  identify the cause of the system restart. This will be helpful to identify
+  the cause when the RAM dump collection is disabled.
+
+properties:
+  compatible:
+    const: qcom,restart-reason-info
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    imem@8600000 {
+      compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
+      reg = <0x08600000 0x1000>;
+
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      ranges = <0 0x08600000 0x1000>;
+
+      restart-reason@7b0 {
+        compatible = "qcom,restart-reason-info";
+        reg = <0x7b0 0x4>;
+      };
+    };
+...

-- 
2.34.1

[PATCH RFC 4/6] dt-bindings: sram: qcom,imem: add the support for restart reason

[Kathiravan Thirumoorthy]

In the Qualcomm IPQ SoCs, system restart reason is captured in the IMEM
location by bootloaders and Linux populates this information to the
userspace. Add a child node for the restart reason in the IMEM region.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 .../devicetree/bindings/sram/qcom,imem.yaml        | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index dec1b1ee924cf1386f559eb262ea864f2788c165..c3dab5fbc88c1515bfb3585f18aed9e01ae36fe4 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -56,6 +56,10 @@ patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+  "^restart-reason@[0-9a-f]+$":
+    $ref: /schemas/watchdog/qcom,restart-reason.yaml#
+    description: IPQ SoC restart reason region
+
 required:
   - compatible
   - reg
@@ -82,3 +86,23 @@ examples:
             };
         };
     };
+
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        sram@8600000 {
+            compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
+            reg = <0 0x08600000 0 0x1000>;
+            ranges = <0 0 0x08600000 0x1000>;
+
+            #address-cells = <1>;
+            #size-cells = <1>;
+
+            restart-reason@7b0 {
+                compatible = "qcom,restart-reason-info";
+                reg = <0x7b0 0x4>;
+            };
+        };
+    };

-- 
2.34.1

[PATCH RFC 5/6] watchdog: qcom-wdt: add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

When the system boots up after a watchdog reset, the EXPIRED_STATUS bit
in the WDT_STS register is cleared. To identify if the system was restarted
due to WDT expiry, bootloaders update the information in the IMEM region.
Update the driver to read the restart reason from IMEM and populate the
bootstatus accordingly.

For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new
function qcom_wdt_get_restart_reason() to read the restart reason from
IMEM.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 drivers/watchdog/qcom-wdt.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..54d6eaa132ab9f63e1312a69ad51b7a14f78fe2d 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -9,6 +9,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 
@@ -22,6 +23,8 @@ enum wdt_reg {
 
 #define QCOM_WDT_ENABLE		BIT(0)
 
+#define NON_SECURE_WDT_RESET	0x5
+
 static const u32 reg_offset_data_apcs_tmr[] = {
 	[WDT_RST] = 0x38,
 	[WDT_EN] = 0x40,
@@ -187,6 +190,39 @@ static const struct qcom_wdt_match_data match_data_kpss = {
 	.max_tick_count = 0xFFFFFU,
 };
 
+static int  qcom_wdt_get_restart_reason(struct qcom_wdt *wdt)
+{
+	struct device_node *np;
+	struct resource imem;
+	void __iomem *base;
+	int ret;
+
+	np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info");
+	if (!np)
+		return -ENOENT;
+
+	ret = of_address_to_resource(np, 0, &imem);
+	of_node_put(np);
+	if (ret < 0) {
+		dev_err(wdt->wdd.parent, "can't translate OF node address\n");
+		return ret;
+	}
+
+	base = ioremap(imem.start, resource_size(&imem));
+	if (!base) {
+		dev_err(wdt->wdd.parent, "failed to map restart reason info region\n");
+		return -ENOMEM;
+	}
+
+	memcpy_fromio(&ret, base, sizeof(ret));
+	iounmap(base);
+
+	if (ret == NON_SECURE_WDT_RESET)
+		wdt->wdd.bootstatus = WDIOF_CARDRESET;
+
+	return 0;
+}
+
 static int qcom_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -267,7 +303,9 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	wdt->wdd.parent = dev;
 	wdt->layout = data->offset;
 
-	if (readl(wdt_addr(wdt, WDT_STS)) & 1)
+	ret = qcom_wdt_get_restart_reason(wdt);
+	if (ret == -ENOENT &&
+	    readl(wdt_addr(wdt, WDT_STS)) & 1)
 		wdt->wdd.bootstatus = WDIOF_CARDRESET;
 
 	/*

-- 
2.34.1

[PATCH RFC 6/6] arm64: dts: qcom: ipq5424: add node for the restart reason information

[Kathiravan Thirumoorthy]

Add a child node to the IMEM region to capture the system restart reason
in Qualcomm IPQ SoCs. This information is populated by the WDT driver via
bootstatus sysfs.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/ipq5424.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
index a772736f314f46d11c473160c522af4edeb900b7..d399ae506748b22c1dc653d357c6fd071dd67f04 100644
--- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
@@ -493,6 +493,11 @@ sram@8600000 {
 
 			#address-cells = <1>;
 			#size-cells = <1>;
+
+			restart-reason@7b0 {
+				compatible = "qcom,restart-reason-info";
+				reg = <0x7b0 0x4>;
+			};
 		};
 
 		usb3: usb3@8a00000 {

-- 
2.34.1

Re: [PATCH RFC 3/6] dt-bindings: watchdog: Add Qualcomm restart reason binding

[Rob Herring (Arm)]

On Tue, 08 Apr 2025 14:19:53 +0530, Kathiravan Thirumoorthy wrote:
> Add a devicetree binding for the Qualcomm IPQ SOCs restart reason
> information region found in the IMEM, allowing the system to identify
> the cause of a restart.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  .../bindings/watchdog/qcom,restart-reason.yaml     | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.example.dtb: imem@8600000 (qcom,ipq5424-imem): 'restart-reason@7b0' does not match any of the regexes: '^pil-reloc@[0-9a-f]+$', 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/sram/qcom,imem.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250408-wdt_reset_reason-v1-3-e6ec30c2c926@oss.qualcomm.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH RFC 5/6] watchdog: qcom-wdt: add support to read the restart reason from IMEM

[Guenter Roeck]

On 4/8/25 01:49, Kathiravan Thirumoorthy wrote:
> When the system boots up after a watchdog reset, the EXPIRED_STATUS bit
> in the WDT_STS register is cleared. To identify if the system was restarted
> due to WDT expiry, bootloaders update the information in the IMEM region.
> Update the driver to read the restart reason from IMEM and populate the
> bootstatus accordingly.
> 
> For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new
> function qcom_wdt_get_restart_reason() to read the restart reason from
> IMEM.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>   drivers/watchdog/qcom-wdt.c | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..54d6eaa132ab9f63e1312a69ad51b7a14f78fe2d 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -9,6 +9,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> +#include <linux/of_address.h>
>   #include <linux/platform_device.h>
>   #include <linux/watchdog.h>
>   
> @@ -22,6 +23,8 @@ enum wdt_reg {
>   
>   #define QCOM_WDT_ENABLE		BIT(0)
>   
> +#define NON_SECURE_WDT_RESET	0x5
> +
>   static const u32 reg_offset_data_apcs_tmr[] = {
>   	[WDT_RST] = 0x38,
>   	[WDT_EN] = 0x40,
> @@ -187,6 +190,39 @@ static const struct qcom_wdt_match_data match_data_kpss = {
>   	.max_tick_count = 0xFFFFFU,
>   };
>   
> +static int  qcom_wdt_get_restart_reason(struct qcom_wdt *wdt)
> +{
> +	struct device_node *np;
> +	struct resource imem;
> +	void __iomem *base;
> +	int ret;
> +
> +	np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info");
> +	if (!np)
> +		return -ENOENT;
> +
> +	ret = of_address_to_resource(np, 0, &imem);
> +	of_node_put(np);
> +	if (ret < 0) {
> +		dev_err(wdt->wdd.parent, "can't translate OF node address\n");
> +		return ret;
> +	}
> +
> +	base = ioremap(imem.start, resource_size(&imem));
> +	if (!base) {
> +		dev_err(wdt->wdd.parent, "failed to map restart reason info region\n");
> +		return -ENOMEM;
> +	}
> +
> +	memcpy_fromio(&ret, base, sizeof(ret));
> +	iounmap(base);
> +
> +	if (ret == NON_SECURE_WDT_RESET)
> +		wdt->wdd.bootstatus = WDIOF_CARDRESET;
> +
> +	return 0;
> +}
> +
>   static int qcom_wdt_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -267,7 +303,9 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>   	wdt->wdd.parent = dev;
>   	wdt->layout = data->offset;
>   
> -	if (readl(wdt_addr(wdt, WDT_STS)) & 1)
> +	ret = qcom_wdt_get_restart_reason(wdt);
> +	if (ret == -ENOENT &&
> +	    readl(wdt_addr(wdt, WDT_STS)) & 1)
>   		wdt->wdd.bootstatus = WDIOF_CARDRESET;

This ignores all other error returns from qcom_wdt_get_restart_reason(),
but in that function it generates several dev_err(). Either make those
messages less than an error, or treat them as error and drop out here.

Thanks,
Guenter

Re: [PATCH RFC 1/6] dt-bindings: sram: qcom,imem: Document IPQ5424 compatible

[Krzysztof Kozlowski]

On 08/04/2025 10:49, Kathiravan Thirumoorthy wrote:
> Add compatible for Qualcomm's IPQ5424 IMEM.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  Documentation/devicetree/bindings/sram/qcom,imem.yaml | 1 +
>  1 file changed, 1 insertion(+)

Why is this RFC? What is not finished here? I could not find explanation
in cover letter.

Best regards,
Krzysztof

Re: [PATCH RFC 3/6] dt-bindings: watchdog: Add Qualcomm restart reason binding

[Krzysztof Kozlowski]

On 08/04/2025 10:49, Kathiravan Thirumoorthy wrote:
> Add a devicetree binding for the Qualcomm IPQ SOCs restart reason
> information region found in the IMEM, allowing the system to identify
> the cause of a restart.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  .../bindings/watchdog/qcom,restart-reason.yaml     | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml b/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..babbaa70b114f9691018ed6cb10bfa78e18fad64
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/qcom,restart-reason.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm IPQ SoC restart reason location
> +
> +maintainers:
> +  - Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> +
> +description:
> +  The Qualcomm IPQ SoC restart reason memory region, in IMEM, is used to
> +  identify the cause of the system restart. This will be helpful to identify
> +  the cause when the RAM dump collection is disabled.
> +
> +properties:
> +  compatible:
> +    const: qcom,restart-reason-info

No generic compatibles.

OTOH, I don't see much of a value of this being a separate node.

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    imem@8600000 {
> +      compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
> +      reg = <0x08600000 0x1000>;
> +
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      ranges = <0 0x08600000 0x1000>;

Drop all above.

> +
> +      restart-reason@7b0 {
> +        compatible = "qcom,restart-reason-info";
> +        reg = <0x7b0 0x4>;
> +      };
> +    };
> +...
> 


Best regards,
Krzysztof

Re: [PATCH RFC 4/6] dt-bindings: sram: qcom,imem: add the support for restart reason

[Krzysztof Kozlowski]

On 08/04/2025 10:49, Kathiravan Thirumoorthy wrote:
> In the Qualcomm IPQ SoCs, system restart reason is captured in the IMEM
> location by bootloaders and Linux populates this information to the
> userspace. Add a child node for the restart reason in the IMEM region.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  .../devicetree/bindings/sram/qcom,imem.yaml        | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> index dec1b1ee924cf1386f559eb262ea864f2788c165..c3dab5fbc88c1515bfb3585f18aed9e01ae36fe4 100644
> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> @@ -56,6 +56,10 @@ patternProperties:
>      $ref: /schemas/remoteproc/qcom,pil-info.yaml#
>      description: Peripheral image loader relocation region
>  
> +  "^restart-reason@[0-9a-f]+$":
> +    $ref: /schemas/watchdog/qcom,restart-reason.yaml#

Just fold it here... but really, there is little point in describing
memory layout of syscon block register by register. What will be next?
Another entry for one more register? And then another? And another? And
every time you will claim "just this one", because you decide not to
provide complete picture when sending bindings for the first time?


Best regards,
Krzysztof

Re: [PATCH RFC 5/6] watchdog: qcom-wdt: add support to read the restart reason from IMEM

[Krzysztof Kozlowski]

On 08/04/2025 10:49, Kathiravan Thirumoorthy wrote:
> When the system boots up after a watchdog reset, the EXPIRED_STATUS bit
> in the WDT_STS register is cleared. To identify if the system was restarted
> due to WDT expiry, bootloaders update the information in the IMEM region.
> Update the driver to read the restart reason from IMEM and populate the
> bootstatus accordingly.
> 
> For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new
> function qcom_wdt_get_restart_reason() to read the restart reason from
> IMEM.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  drivers/watchdog/qcom-wdt.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..54d6eaa132ab9f63e1312a69ad51b7a14f78fe2d 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -9,6 +9,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/watchdog.h>
>  
> @@ -22,6 +23,8 @@ enum wdt_reg {
>  
>  #define QCOM_WDT_ENABLE		BIT(0)
>  
> +#define NON_SECURE_WDT_RESET	0x5
> +
>  static const u32 reg_offset_data_apcs_tmr[] = {
>  	[WDT_RST] = 0x38,
>  	[WDT_EN] = 0x40,
> @@ -187,6 +190,39 @@ static const struct qcom_wdt_match_data match_data_kpss = {
>  	.max_tick_count = 0xFFFFFU,
>  };
>  
> +static int  qcom_wdt_get_restart_reason(struct qcom_wdt *wdt)
> +{
> +	struct device_node *np;
> +	struct resource imem;
> +	void __iomem *base;
> +	int ret;
> +
> +	np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info");
> +	if (!np)

That's not how you express dependencies between devices.

> +		return -ENOENT;
> +
> +	ret = of_address_to_resource(np, 0, &imem);
> +	of_node_put(np);
> +	if (ret < 0) {
> +		dev_err(wdt->wdd.parent, "can't translate OF node address\n");
> +		return ret;
> +	}
> +
> +	base = ioremap(imem.start, resource_size(&imem));
> +	if (!base) {
> +		dev_err(wdt->wdd.parent, "failed to map restart reason info region\n");
> +		return -ENOMEM;
> +	}
> +
> +	memcpy_fromio(&ret, base, sizeof(ret));
> +	iounmap(base);

All this is wrong usage of syscon API, missing devlinks, messing up with
other device's address space.


Best regards,
Krzysztof

Re: [PATCH RFC 2/6] arm64: dts: qcom: ipq5424: Add the IMEM node

[Konrad Dybcio]

On 4/8/25 10:49 AM, Kathiravan Thirumoorthy wrote:
> Add the IMEM node to the device tree to extract debugging information
> like system restart reason, which is populated via IMEM. Define the
> IMEM region to enable this functionality. Corresponding DTS and driver
> changes will be added incrementally.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5424.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
> index 5d6ed2172b1bb0a57c593f121f387ec917f42419..a772736f314f46d11c473160c522af4edeb900b7 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
> @@ -486,6 +486,15 @@ ssphy_0: phy@7d000 {
>  			status = "disabled";
>  		};
>  
> +		sram@8600000 {
> +			compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
> +			reg = <0 0x08600000 0 0x1000>;
> +			ranges = <0 0 0x08600000 0x1000>;

It looks like this should be a little longer

Konrad

Re: [PATCH RFC 0/6] Add support to read the restart reason from IMEM

[Konrad Dybcio]

On 4/8/25 10:49 AM, Kathiravan Thirumoorthy wrote:
> In Qualcomm IPQ SoC, if the system is rebooted due to the watchdog
> timeout, there is no way to identify it. Current approach of checking
> the EXPIRED_STATUS in WDT_STS is not working.
> 
> To achieve this, if the system is rebooted due to watchdog timeout, the
> information is captured in the IMEM by the bootloader (along with other
> reason codes as well).
> 
> This series attempts to address this by adding the support to read the
> IMEM and populate the information via bootstatus sysfs file. As of now,
> we are handling only the non secure watchdog timeout reason.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---

So I think it would be easier to model this as nvram (something like
"nvmem-rmem" with imem perhaps modeled as "mmio-sram") and then consume
that value through in-kernel APIs (or write to it, as necessary)

Taking a quick look at mobile, it seems like "reboot bootloader" and
friends use a similar mechanism

Konrad

Re: [PATCH RFC 1/6] dt-bindings: sram: qcom,imem: Document IPQ5424 compatible

[Kathiravan Thirumoorthy]

On 4/9/2025 12:28 PM, Krzysztof Kozlowski wrote:
> On 08/04/2025 10:49, Kathiravan Thirumoorthy wrote:
>> Add compatible for Qualcomm's IPQ5424 IMEM.
>>
>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> ---
>>   Documentation/devicetree/bindings/sram/qcom,imem.yaml | 1 +
>>   1 file changed, 1 insertion(+)
> Why is this RFC? What is not finished here? I could not find explanation
> in cover letter.


I added the IMEM node to retrieve the restart reason which is used in 
this series. Since I wasn't sure about the idea which is followed here, 
I made the whole series as RFC. Going forward, I shall explain why the 
series is made as RFC in cover letter.

With respect to this patch, nothing is pending. I can separate out the 
dt-binding and DTS for IMEM from this series and post it. Please let me 
know.


>
> Best regards,
> Krzysztof

Re: [PATCH RFC 2/6] arm64: dts: qcom: ipq5424: Add the IMEM node

[Kathiravan Thirumoorthy]

On 4/10/2025 12:11 AM, Konrad Dybcio wrote:
> On 4/8/25 10:49 AM, Kathiravan Thirumoorthy wrote:
>> Add the IMEM node to the device tree to extract debugging information
>> like system restart reason, which is populated via IMEM. Define the
>> IMEM region to enable this functionality. Corresponding DTS and driver
>> changes will be added incrementally.
>>
>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5424.dtsi | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>> index 5d6ed2172b1bb0a57c593f121f387ec917f42419..a772736f314f46d11c473160c522af4edeb900b7 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>> @@ -486,6 +486,15 @@ ssphy_0: phy@7d000 {
>>   			status = "disabled";
>>   		};
>>   
>> +		sram@8600000 {
>> +			compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
>> +			reg = <0 0x08600000 0 0x1000>;
>> +			ranges = <0 0 0x08600000 0x1000>;
> It looks like this should be a little longer


Yes. It is 112KB. But only first 4KB is accessible by all masters in the 
system, remaining regions are access protected by TZ. I shall mention 
this in the commit message in the next version.


>
> Konrad

Re: [PATCH RFC 3/6] dt-bindings: watchdog: Add Qualcomm restart reason binding

[Kathiravan Thirumoorthy]

On 4/9/2025 12:30 PM, Krzysztof Kozlowski wrote:
> On 08/04/2025 10:49, Kathiravan Thirumoorthy wrote:
>> Add a devicetree binding for the Qualcomm IPQ SOCs restart reason
>> information region found in the IMEM, allowing the system to identify
>> the cause of a restart.
>>
>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> ---
>>   .../bindings/watchdog/qcom,restart-reason.yaml     | 46 ++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml b/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..babbaa70b114f9691018ed6cb10bfa78e18fad64
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/watchdog/qcom,restart-reason.yaml
>> @@ -0,0 +1,46 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/watchdog/qcom,restart-reason.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm IPQ SoC restart reason location
>> +
>> +maintainers:
>> +  - Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> +
>> +description:
>> +  The Qualcomm IPQ SoC restart reason memory region, in IMEM, is used to
>> +  identify the cause of the system restart. This will be helpful to identify
>> +  the cause when the RAM dump collection is disabled.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,restart-reason-info
> No generic compatibles.
>
> OTOH, I don't see much of a value of this being a separate node.

I leveraged this based on the qcom,pil-info.yaml [1]. I guess, I see the 
point. I will drop all these. Just define IMEM node and in the watchdog 
driver, I will get the regmap of the syscon / IMEM node and do 
regmap_read at the desired offset. Please let me know if this approach 
is fine.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml 

Re: [PATCH RFC 5/6] watchdog: qcom-wdt: add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

On 4/8/2025 6:03 PM, Guenter Roeck wrote:
>>   static int qcom_wdt_probe(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>> @@ -267,7 +303,9 @@ static int qcom_wdt_probe(struct platform_device 
>> *pdev)
>>       wdt->wdd.parent = dev;
>>       wdt->layout = data->offset;
>>   -    if (readl(wdt_addr(wdt, WDT_STS)) & 1)
>> +    ret = qcom_wdt_get_restart_reason(wdt);
>> +    if (ret == -ENOENT &&
>> +        readl(wdt_addr(wdt, WDT_STS)) & 1)
>>           wdt->wdd.bootstatus = WDIOF_CARDRESET;
>
> This ignores all other error returns from qcom_wdt_get_restart_reason(),
> but in that function it generates several dev_err(). Either make those
> messages less than an error, or treat them as error and drop out here. 


Thanks for pointing this out. I will handle these errors in the next 
version.

Re: [PATCH RFC 5/6] watchdog: qcom-wdt: add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

On 4/9/2025 12:33 PM, Krzysztof Kozlowski wrote:
>> +static int  qcom_wdt_get_restart_reason(struct qcom_wdt *wdt)
>> +{
>> +	struct device_node *np;
>> +	struct resource imem;
>> +	void __iomem *base;
>> +	int ret;
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info");
>> +	if (!np)
> That's not how you express dependencies between devices.


As I mentioned in the bindings patch, I leveraged this from the 
qcom_pil_info.c[1]. I shall use the syscon_regmap_lookup_by_compatible() 
function.


>
>> +		return -ENOENT;
>> +
>> +	ret = of_address_to_resource(np, 0, &imem);
>> +	of_node_put(np);
>> +	if (ret < 0) {
>> +		dev_err(wdt->wdd.parent, "can't translate OF node address\n");
>> +		return ret;
>> +	}
>> +
>> +	base = ioremap(imem.start, resource_size(&imem));
>> +	if (!base) {
>> +		dev_err(wdt->wdd.parent, "failed to map restart reason info region\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	memcpy_fromio(&ret, base, sizeof(ret));
>> +	iounmap(base);
> All this is wrong usage of syscon API, missing devlinks, messing up with
> other device's address space.

I shall use regmap_read() instead of memcpy_fromio().

Re: [PATCH RFC 2/6] arm64: dts: qcom: ipq5424: Add the IMEM node

[Konrad Dybcio]

On 4/11/25 7:01 AM, Kathiravan Thirumoorthy wrote:
> 
> On 4/10/2025 12:11 AM, Konrad Dybcio wrote:
>> On 4/8/25 10:49 AM, Kathiravan Thirumoorthy wrote:
>>> Add the IMEM node to the device tree to extract debugging information
>>> like system restart reason, which is populated via IMEM. Define the
>>> IMEM region to enable this functionality. Corresponding DTS and driver
>>> changes will be added incrementally.
>>>
>>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/ipq5424.dtsi | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>>> index 5d6ed2172b1bb0a57c593f121f387ec917f42419..a772736f314f46d11c473160c522af4edeb900b7 100644
>>> --- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>>> @@ -486,6 +486,15 @@ ssphy_0: phy@7d000 {
>>>               status = "disabled";
>>>           };
>>>   +        sram@8600000 {
>>> +            compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
>>> +            reg = <0 0x08600000 0 0x1000>;
>>> +            ranges = <0 0 0x08600000 0x1000>;
>> It looks like this should be a little longer
> 
> 
> Yes. It is 112KB. But only first 4KB is accessible by all masters in the system, remaining regions are access protected by TZ. I shall mention this in the commit message in the next version.

I think we should describe the full length of it - it's only if we
add a subnode that we actually access it

Konrad

Re: [PATCH RFC 2/6] arm64: dts: qcom: ipq5424: Add the IMEM node

[Kathiravan Thirumoorthy]

On 4/11/2025 2:33 PM, Konrad Dybcio wrote:
> On 4/11/25 7:01 AM, Kathiravan Thirumoorthy wrote:
>> On 4/10/2025 12:11 AM, Konrad Dybcio wrote:
>>> On 4/8/25 10:49 AM, Kathiravan Thirumoorthy wrote:
>>>> Add the IMEM node to the device tree to extract debugging information
>>>> like system restart reason, which is populated via IMEM. Define the
>>>> IMEM region to enable this functionality. Corresponding DTS and driver
>>>> changes will be added incrementally.
>>>>
>>>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/ipq5424.dtsi | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>>>> index 5d6ed2172b1bb0a57c593f121f387ec917f42419..a772736f314f46d11c473160c522af4edeb900b7 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
>>>> @@ -486,6 +486,15 @@ ssphy_0: phy@7d000 {
>>>>                status = "disabled";
>>>>            };
>>>>    +        sram@8600000 {
>>>> +            compatible = "qcom,ipq5424-imem", "syscon", "simple-mfd";
>>>> +            reg = <0 0x08600000 0 0x1000>;
>>>> +            ranges = <0 0 0x08600000 0x1000>;
>>> It looks like this should be a little longer
>>
>> Yes. It is 112KB. But only first 4KB is accessible by all masters in the system, remaining regions are access protected by TZ. I shall mention this in the commit message in the next version.
> I think we should describe the full length of it - it's only if we
> add a subnode that we actually access it


Sure got it.  I will describe the full length in next revision. In that 
case, it would be the driver's responsibility not to go beyond the 
initial 4K.


>
> Konrad