[PATCH v2 0/5] Add support to read the restart reason from IMEM

[PATCH v2 0/5] 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.

Konrad, I sticked with syscon API to access the IMEM instead of exposing
it as mmio-sram to align with what is available in the mainline. Do let
me know if the current approach is still not correct / feasible.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v2:
- Dropped the RFC tag
- Reworked the driver changes to use the syscon API
- Link to v1: 20250408-wdt_reset_reason-v1-0-e6ec30c2c926@oss.qualcomm.com

---
Kathiravan Thirumoorthy (5):
      dt-bindings: sram: qcom,imem: Document IPQ5424 compatible
      arm64: dts: qcom: ipq5424: Add the IMEM node
      dt-bindings: watchdog: separate out the IPQ5424 compatilble
      arm64: dts: qcom: ipq5424: drop the fallback WDT compatible
      watchdog: qcom: add support to read the restart reason from IMEM

 .../devicetree/bindings/sram/qcom,imem.yaml        |  1 +
 .../devicetree/bindings/watchdog/qcom-wdt.yaml     |  7 +++-
 arch/arm64/boot/dts/qcom/ipq5424.dtsi              | 11 ++++-
 drivers/watchdog/qcom-wdt.c                        | 47 +++++++++++++++++++++-
 4 files changed, 61 insertions(+), 5 deletions(-)
---
base-commit: 7702d0130dc002bab2c3571ddb6ff68f82d99aea
change-id: 20250408-wdt_reset_reason-e12921963fa6

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

[PATCH v2 1/5] 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>
---
Changes in v2:
	- No changes
---
 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 v2 2/5] 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.

As described, overall IMEM region is 112KB but only initial 4KB is
accessible by all masters in the SoC.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v2:
	- Describe the entire IMEM region in the node
	- Explicitly call out that initial 4K only accessible by all
	  masters in the commit message
---
 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..4f18ea79502738c2b9cb4b13e8eb4ac4ddd89adf 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 0x1c000>;
+			ranges = <0 0 0x08600000 0x1c000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
+
 		usb3: usb3@8a00000 {
 			compatible = "qcom,ipq5424-dwc3", "qcom,dwc3";
 			reg = <0 0x08af8800 0 0x400>;

-- 
2.34.1

[PATCH v2 3/5] dt-bindings: watchdog: separate out the IPQ5424 compatilble

[Kathiravan Thirumoorthy]

To retrieve the system restart reason code from IMEM, need to define the
certain device specific data. To achieve that, decouple the IPQ5424
compatible from the existing list and define along with 'qcom,kpss-wdt'.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v2:
	- New patch
---
 Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
index 49e2b807db0bc9d3edfc93ec41ad0df0b74ed032..e800f53381ef5626787eff1029bc94177e2635a4 100644
--- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
@@ -20,7 +20,6 @@ properties:
               - qcom,kpss-wdt-ipq4019
               - qcom,apss-wdt-ipq5018
               - qcom,apss-wdt-ipq5332
-              - qcom,apss-wdt-ipq5424
               - qcom,apss-wdt-ipq9574
               - qcom,apss-wdt-msm8226
               - qcom,apss-wdt-msm8974
@@ -56,6 +55,8 @@ properties:
               - qcom,kpss-wdt-msm8960
           - const: qcom,kpss-timer
           - const: qcom,msm-timer
+      - items:
+          - const: qcom,apss-wdt-ipq5424
 
   reg:
     maxItems: 1
@@ -93,7 +94,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: qcom,kpss-wdt
+            enum:
+              - qcom,apss-wdt-ipq5424
+              - qcom,kpss-wdt
     then:
       properties:
         clock-frequency: false

-- 
2.34.1

[PATCH v2 4/5] arm64: dts: qcom: ipq5424: drop the fallback WDT compatible

[Kathiravan Thirumoorthy]

To retrieve the restart reason from IMEM, certain device specific data
to be used. To achieve that, drop the fallback compatible.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v2:
	- New Patch
---
 arch/arm64/boot/dts/qcom/ipq5424.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
index 4f18ea79502738c2b9cb4b13e8eb4ac4ddd89adf..21252352b7328e4a1b7ba6ca7080f73722f097ad 100644
--- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
@@ -376,7 +376,7 @@ intc: interrupt-controller@f200000 {
 		};
 
 		watchdog@f410000 {
-			compatible = "qcom,apss-wdt-ipq5424", "qcom,kpss-wdt";
+			compatible = "qcom,apss-wdt-ipq5424";
 			reg = <0 0x0f410000 0 0x1000>;
 			interrupts = <GIC_SPI 0 IRQ_TYPE_EDGE_RISING>;
 			clocks = <&sleep_clk>;

-- 
2.34.1

[PATCH v2 5/5] watchdog: qcom: 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>
---
Changes in v2:
	- Use the syscon API to access the IMEM region
	- Handle the error cases returned by qcom_wdt_get_restart_reason
	- Define device specific data to retrieve the IMEM compatible,
	  offset and the value for non secure WDT, which allows to
	  extend the support for other SoCs
---
 drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -7,9 +7,11 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/watchdog.h>
 
 enum wdt_reg {
@@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = {
 };
 
 struct qcom_wdt_match_data {
+	const char *compatible;
+	unsigned int restart_reason_offset;
+	unsigned int non_secure_wdt_val;
 	const u32 *offset;
 	bool pretimeout;
 	u32 max_tick_count;
@@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = {
 	.identity	= KBUILD_MODNAME,
 };
 
+static const struct qcom_wdt_match_data match_data_ipq5424 = {
+	.compatible = "qcom,ipq5424-imem",
+	.restart_reason_offset = 0x7b0,
+	.non_secure_wdt_val = 0x5,
+	.offset = reg_offset_data_kpss,
+	.pretimeout = true,
+	.max_tick_count = 0xFFFFFU,
+};
+
 static const struct qcom_wdt_match_data match_data_apcs_tmr = {
 	.offset = reg_offset_data_apcs_tmr,
 	.pretimeout = false,
@@ -187,6 +201,29 @@ 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,
+					const struct qcom_wdt_match_data *data)
+{
+	struct regmap *imem;
+	unsigned int val;
+	int ret;
+
+	imem = syscon_regmap_lookup_by_compatible(data->compatible);
+	if (IS_ERR(imem))
+		return PTR_ERR(imem);
+
+	ret = regmap_read(imem, data->restart_reason_offset, &val);
+	if (ret) {
+		dev_err(wdt->wdd.parent, "failed to read the restart reason info\n");
+		return ret;
+	}
+
+	if (val == data->non_secure_wdt_val)
+		wdt->wdd.bootstatus = WDIOF_CARDRESET;
+
+	return 0;
+}
+
 static int qcom_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -267,8 +304,13 @@ 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)
-		wdt->wdd.bootstatus = WDIOF_CARDRESET;
+	ret = qcom_wdt_get_restart_reason(wdt, data);
+	if (ret == -ENODEV) {
+		if (readl(wdt_addr(wdt, WDT_STS)) & 1)
+			wdt->wdd.bootstatus = WDIOF_CARDRESET;
+	} else if (ret) {
+		return ret;
+	}
 
 	/*
 	 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
@@ -322,6 +364,7 @@ static const struct dev_pm_ops qcom_wdt_pm_ops = {
 };
 
 static const struct of_device_id qcom_wdt_of_table[] = {
+	{ .compatible = "qcom,apss-wdt-ipq5424", .data = &match_data_ipq5424 },
 	{ .compatible = "qcom,kpss-timer", .data = &match_data_apcs_tmr },
 	{ .compatible = "qcom,scss-timer", .data = &match_data_apcs_tmr },
 	{ .compatible = "qcom,kpss-wdt", .data = &match_data_kpss },

-- 
2.34.1

Re: [PATCH v2 5/5] watchdog: qcom: add support to read the restart reason from IMEM

[Konrad Dybcio]

On 4/16/25 10:29 AM, 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>
> ---
> Changes in v2:
> 	- Use the syscon API to access the IMEM region
> 	- Handle the error cases returned by qcom_wdt_get_restart_reason
> 	- Define device specific data to retrieve the IMEM compatible,
> 	  offset and the value for non secure WDT, which allows to
> 	  extend the support for other SoCs
> ---
>  drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -7,9 +7,11 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  #include <linux/watchdog.h>
>  
>  enum wdt_reg {
> @@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = {
>  };
>  
>  struct qcom_wdt_match_data {
> +	const char *compatible;
> +	unsigned int restart_reason_offset;
> +	unsigned int non_secure_wdt_val;
>  	const u32 *offset;
>  	bool pretimeout;
>  	u32 max_tick_count;
> @@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = {
>  	.identity	= KBUILD_MODNAME,
>  };
>  
> +static const struct qcom_wdt_match_data match_data_ipq5424 = {
> +	.compatible = "qcom,ipq5424-imem",
> +	.restart_reason_offset = 0x7b0,
> +	.non_secure_wdt_val = 0x5,
> +	.offset = reg_offset_data_kpss,
> +	.pretimeout = true,
> +	.max_tick_count = 0xFFFFFU,
> +};
> +
>  static const struct qcom_wdt_match_data match_data_apcs_tmr = {
>  	.offset = reg_offset_data_apcs_tmr,
>  	.pretimeout = false,
> @@ -187,6 +201,29 @@ 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,

double space> +					const struct qcom_wdt_match_data *data)

Please align this

> +{
> +	struct regmap *imem;
> +	unsigned int val;
> +	int ret;
> +
> +	imem = syscon_regmap_lookup_by_compatible(data->compatible);

I still think nvmem could be better here, as it allows to plug in
more magic values

Konrad

Re: [PATCH v2 2/5] arm64: dts: qcom: ipq5424: Add the IMEM node

[Konrad Dybcio]

On 4/16/25 10:29 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.
> 
> As described, overall IMEM region is 112KB but only initial 4KB is
> accessible by all masters in the SoC.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---

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

Konrad

Re: [PATCH v2 5/5] watchdog: qcom: add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

On 4/16/2025 8:21 PM, Konrad Dybcio wrote:
>>   	.max_tick_count = 0xFFFFFU,
>>   };
>>   
>> +static int  qcom_wdt_get_restart_reason(struct qcom_wdt *wdt,
> double space> +					const struct qcom_wdt_match_data *data)
>
> Please align this


Ack.


>> +{
>> +	struct regmap *imem;
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	imem = syscon_regmap_lookup_by_compatible(data->compatible);
> I still think nvmem could be better here, as it allows to plug in
> more magic values


Sure, I will be on vacation next week. I shall check on how to use nvmem 
here once I'm back.


>
> Konrad

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

[Rob Herring (Arm)]

On Wed, 16 Apr 2025 13:59:18 +0530, Kathiravan Thirumoorthy wrote:
> Add compatible for Qualcomm's IPQ5424 IMEM.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
> Changes in v2:
> 	- No changes
> ---
>  Documentation/devicetree/bindings/sram/qcom,imem.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

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

Re: [PATCH v2 3/5] dt-bindings: watchdog: separate out the IPQ5424 compatilble

[Rob Herring]

On Wed, Apr 16, 2025 at 01:59:20PM +0530, Kathiravan Thirumoorthy wrote:
> To retrieve the system restart reason code from IMEM, need to define the
> certain device specific data. To achieve that, decouple the IPQ5424
> compatible from the existing list and define along with 'qcom,kpss-wdt'.

You have missed the whole point of why there's both a specific 
compatible and a fallback. The specific one existed for a case like this 
where you need to start distinguishing the specific device. In short, 
this binding and dts changes are not needed at all, only the driver 
change is needed. Then you maintain forwards and backwards 
compatibility. 

> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
> Changes in v2:
> 	- New patch
> ---
>  Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
> index 49e2b807db0bc9d3edfc93ec41ad0df0b74ed032..e800f53381ef5626787eff1029bc94177e2635a4 100644
> --- a/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml
> @@ -20,7 +20,6 @@ properties:
>                - qcom,kpss-wdt-ipq4019
>                - qcom,apss-wdt-ipq5018
>                - qcom,apss-wdt-ipq5332
> -              - qcom,apss-wdt-ipq5424
>                - qcom,apss-wdt-ipq9574
>                - qcom,apss-wdt-msm8226
>                - qcom,apss-wdt-msm8974
> @@ -56,6 +55,8 @@ properties:
>                - qcom,kpss-wdt-msm8960
>            - const: qcom,kpss-timer
>            - const: qcom,msm-timer
> +      - items:
> +          - const: qcom,apss-wdt-ipq5424
>  
>    reg:
>      maxItems: 1
> @@ -93,7 +94,9 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: qcom,kpss-wdt
> +            enum:
> +              - qcom,apss-wdt-ipq5424
> +              - qcom,kpss-wdt
>      then:
>        properties:
>          clock-frequency: false
> 
> -- 
> 2.34.1
> 

Re: [PATCH v2 4/5] arm64: dts: qcom: ipq5424: drop the fallback WDT compatible

[Rob Herring]

On Wed, Apr 16, 2025 at 01:59:21PM +0530, Kathiravan Thirumoorthy wrote:
> To retrieve the restart reason from IMEM, certain device specific data
> to be used. To achieve that, drop the fallback compatible.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
> Changes in v2:
> 	- New Patch
> ---
>  arch/arm64/boot/dts/qcom/ipq5424.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Just so it is abundantly clear. NAK.

See my reply on the binding patch.

> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5424.dtsi b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
> index 4f18ea79502738c2b9cb4b13e8eb4ac4ddd89adf..21252352b7328e4a1b7ba6ca7080f73722f097ad 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5424.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5424.dtsi
> @@ -376,7 +376,7 @@ intc: interrupt-controller@f200000 {
>  		};
>  
>  		watchdog@f410000 {
> -			compatible = "qcom,apss-wdt-ipq5424", "qcom,kpss-wdt";
> +			compatible = "qcom,apss-wdt-ipq5424";
>  			reg = <0 0x0f410000 0 0x1000>;
>  			interrupts = <GIC_SPI 0 IRQ_TYPE_EDGE_RISING>;
>  			clocks = <&sleep_clk>;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v2 3/5] dt-bindings: watchdog: separate out the IPQ5424 compatilble

[Kathiravan Thirumoorthy]

On 4/22/2025 1:13 AM, Rob Herring wrote:
> On Wed, Apr 16, 2025 at 01:59:20PM +0530, Kathiravan Thirumoorthy wrote:
>> To retrieve the system restart reason code from IMEM, need to define the
>> certain device specific data. To achieve that, decouple the IPQ5424
>> compatible from the existing list and define along with 'qcom,kpss-wdt'.
> You have missed the whole point of why there's both a specific
> compatible and a fallback. The specific one existed for a case like this
> where you need to start distinguishing the specific device. In short,
> this binding and dts changes are not needed at all, only the driver
> change is needed. Then you maintain forwards and backwards
> compatibility.

Yeah, I completely missed that. Thanks for pointing out. Will drop this 
and the DTS patch in the next spin.

Thanks,

Kathiravan T.

Re: [PATCH v2 5/5] watchdog: qcom: add support to read the restart reason from IMEM

[Konrad Dybcio]

On 4/17/25 8:19 AM, Kathiravan Thirumoorthy wrote:
> 
> On 4/16/2025 8:21 PM, Konrad Dybcio wrote:
>>>       .max_tick_count = 0xFFFFFU,
>>>   };
>>>   +static int  qcom_wdt_get_restart_reason(struct qcom_wdt *wdt,
>> double space> +                    const struct qcom_wdt_match_data *data)
>>
>> Please align this
> 
> 
> Ack.
> 
> 
>>> +{
>>> +    struct regmap *imem;
>>> +    unsigned int val;
>>> +    int ret;
>>> +
>>> +    imem = syscon_regmap_lookup_by_compatible(data->compatible);
>> I still think nvmem could be better here, as it allows to plug in
>> more magic values
> 
> 
> Sure, I will be on vacation next week. I shall check on how to use nvmem here once I'm back.

We talked offline and I learned that IMEM is not in fact non-volatile, so
while good looking, the nvram APIs are probably not really fit for it.

Let's continue with the syscon approach.

Konrad

Re: [PATCH v2 5/5] watchdog: qcom: add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

On 4/30/2025 7:58 PM, Konrad Dybcio wrote:
> On 4/17/25 8:19 AM, Kathiravan Thirumoorthy wrote:
>> On 4/16/2025 8:21 PM, Konrad Dybcio wrote:
>>>>        .max_tick_count = 0xFFFFFU,
>>>>    };
>>>>    +static int  qcom_wdt_get_restart_reason(struct qcom_wdt *wdt,
>>> double space> +                    const struct qcom_wdt_match_data *data)
>>>
>>> Please align this
>>
>> Ack.
>>
>>
>>>> +{
>>>> +    struct regmap *imem;
>>>> +    unsigned int val;
>>>> +    int ret;
>>>> +
>>>> +    imem = syscon_regmap_lookup_by_compatible(data->compatible);
>>> I still think nvmem could be better here, as it allows to plug in
>>> more magic values
>>
>> Sure, I will be on vacation next week. I shall check on how to use nvmem here once I'm back.
> We talked offline and I learned that IMEM is not in fact non-volatile, so
> while good looking, the nvram APIs are probably not really fit for it.
>
> Let's continue with the syscon approach.


Thanks Konrad for the discussion.


>
> Konrad

Re: [PATCH v2 5/5] watchdog: qcom: add support to read the restart reason from IMEM

[Bryan O'Donoghue]

On 16/04/2025 09:29, 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.

Which bootloaders ?

Do you mean bootrom or one of the subsequent phase bootloaders ?

Please be specific about which bootloader populates this data i.e. if I 
switch my bootloader to u-boot do I loose the added flag ?

> 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>
What I'd really love to see here is an example of reading out the data 
from sysfs.

How do I as a user/consumer of this new functionality parse the new data 
it provides ?

Ideally do this in the commit log and recommend doing it in the cover 
letter to, as people don't always read both when commenting on patches.

> ---
> Changes in v2:
> 	- Use the syscon API to access the IMEM region
> 	- Handle the error cases returned by qcom_wdt_get_restart_reason
> 	- Define device specific data to retrieve the IMEM compatible,
> 	  offset and the value for non secure WDT, which allows to
> 	  extend the support for other SoCs
> ---
>   drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -7,9 +7,11 @@
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>   #include <linux/watchdog.h>
> 
>   enum wdt_reg {
> @@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = {
>   };
> 
>   struct qcom_wdt_match_data {
> +	const char *compatible;
> +	unsigned int restart_reason_offset;
> +	unsigned int non_secure_wdt_val;
>   	const u32 *offset;
>   	bool pretimeout;
>   	u32 max_tick_count;
> @@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = {
>   	.identity	= KBUILD_MODNAME,
>   };
> 
> +static const struct qcom_wdt_match_data match_data_ipq5424 = {
> +	.compatible = "qcom,ipq5424-imem",
> +	.restart_reason_offset = 0x7b0,
> +	.non_secure_wdt_val = 0x5,
> +	.offset = reg_offset_data_kpss,
> +	.pretimeout = true,
> +	.max_tick_count = 0xFFFFFU,
> +};
> +
You should separate the addition of your compatibles and their 
descriptor tables from generic functional extensions.

i.e. add the compat string and the above table in a subsequent patch.

>   static const struct qcom_wdt_match_data match_data_apcs_tmr = {
>   	.offset = reg_offset_data_apcs_tmr,
>   	.pretimeout = false,
> @@ -187,6 +201,29 @@ 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,
> +					const struct qcom_wdt_match_data *data)
> +{
> +	struct regmap *imem;
> +	unsigned int val;
> +	int ret;
> +
> +	imem = syscon_regmap_lookup_by_compatible(data->compatible);
> +	if (IS_ERR(imem))
> +		return PTR_ERR(imem);
> +
> +	ret = regmap_read(imem, data->restart_reason_offset, &val);
> +	if (ret) {
> +		dev_err(wdt->wdd.parent, "failed to read the restart reason info\n");
> +		return ret;
> +	}
> +
> +	if (val == data->non_secure_wdt_val)
> +		wdt->wdd.bootstatus = WDIOF_CARDRESET;
> +
> +	return 0;
> +}
> +
>   static int qcom_wdt_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -267,8 +304,13 @@ 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)
> -		wdt->wdd.bootstatus = WDIOF_CARDRESET;
> +	ret = qcom_wdt_get_restart_reason(wdt, data);
> +	if (ret == -ENODEV) {
> +		if (readl(wdt_addr(wdt, WDT_STS)) & 1)
> +			wdt->wdd.bootstatus = WDIOF_CARDRESET;
> +	} else if (ret) {
> +		return ret;
> +	}
> 
>   	/*
>   	 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
> @@ -322,6 +364,7 @@ static const struct dev_pm_ops qcom_wdt_pm_ops = {
>   };
> 
>   static const struct of_device_id qcom_wdt_of_table[] = {
> +	{ .compatible = "qcom,apss-wdt-ipq5424", .data = &match_data_ipq5424 },
>   	{ .compatible = "qcom,kpss-timer", .data = &match_data_apcs_tmr },
>   	{ .compatible = "qcom,scss-timer", .data = &match_data_apcs_tmr },
>   	{ .compatible = "qcom,kpss-wdt", .data = &match_data_kpss },
> 
> --
> 2.34.1
> 
> 

Re: [PATCH v2 5/5] watchdog: qcom: add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

On 5/1/2025 5:31 AM, Bryan O'Donoghue wrote:
> On 16/04/2025 09:29, 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.
> Which bootloaders ?
>
> Do you mean bootrom or one of the subsequent phase bootloaders ?


It is updated by the XBL. I shall mention it explicitly.


>
> Please be specific about which bootloader populates this data i.e. if I
> switch my bootloader to u-boot do I loose the added flag ?
>
>> 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>
> What I'd really love to see here is an example of reading out the data
> from sysfs.
>
> How do I as a user/consumer of this new functionality parse the new data
> it provides ?
>
> Ideally do this in the commit log and recommend doing it in the cover
> letter to, as people don't always read both when commenting on patches.


Sure, will mention the sysfs path and its output in the commit log and 
cover letter.


>
>> ---
>> Changes in v2:
>> 	- Use the syscon API to access the IMEM region
>> 	- Handle the error cases returned by qcom_wdt_get_restart_reason
>> 	- Define device specific data to retrieve the IMEM compatible,
>> 	  offset and the value for non secure WDT, which allows to
>> 	  extend the support for other SoCs
>> ---
>>    drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
>>    1 file changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -7,9 +7,11 @@
>>    #include <linux/interrupt.h>
>>    #include <linux/io.h>
>>    #include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>>    #include <linux/module.h>
>>    #include <linux/of.h>
>>    #include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>>    #include <linux/watchdog.h>
>>
>>    enum wdt_reg {
>> @@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = {
>>    };
>>
>>    struct qcom_wdt_match_data {
>> +	const char *compatible;
>> +	unsigned int restart_reason_offset;
>> +	unsigned int non_secure_wdt_val;
>>    	const u32 *offset;
>>    	bool pretimeout;
>>    	u32 max_tick_count;
>> @@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = {
>>    	.identity	= KBUILD_MODNAME,
>>    };
>>
>> +static const struct qcom_wdt_match_data match_data_ipq5424 = {
>> +	.compatible = "qcom,ipq5424-imem",
>> +	.restart_reason_offset = 0x7b0,
>> +	.non_secure_wdt_val = 0x5,
>> +	.offset = reg_offset_data_kpss,
>> +	.pretimeout = true,
>> +	.max_tick_count = 0xFFFFFU,
>> +};
>> +
> You should separate the addition of your compatibles and their
> descriptor tables from generic functional extensions.
>
> i.e. add the compat string and the above table in a subsequent patch.


Got it. Will split the patch into 2.


>
>>    static const struct qcom_wdt_match_data match_data_apcs_tmr = {
>>    	.offset = reg_offset_data_apcs_tmr,
>>    	.pretimeout = false,
>> @@ -187,6 +201,29 @@ 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,
>> +					const struct qcom_wdt_match_data *data)
>> +{
>> +	struct regmap *imem;
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	imem = syscon_regmap_lookup_by_compatible(data->compatible);
>> +	if (IS_ERR(imem))
>> +		return PTR_ERR(imem);
>> +
>> +	ret = regmap_read(imem, data->restart_reason_offset, &val);
>> +	if (ret) {
>> +		dev_err(wdt->wdd.parent, "failed to read the restart reason info\n");
>> +		return ret;
>> +	}
>> +
>> +	if (val == data->non_secure_wdt_val)
>> +		wdt->wdd.bootstatus = WDIOF_CARDRESET;
>> +
>> +	return 0;
>> +}
>> +
>>    static int qcom_wdt_probe(struct platform_device *pdev)
>>    {
>>    	struct device *dev = &pdev->dev;
>> @@ -267,8 +304,13 @@ 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)
>> -		wdt->wdd.bootstatus = WDIOF_CARDRESET;
>> +	ret = qcom_wdt_get_restart_reason(wdt, data);
>> +	if (ret == -ENODEV) {
>> +		if (readl(wdt_addr(wdt, WDT_STS)) & 1)
>> +			wdt->wdd.bootstatus = WDIOF_CARDRESET;
>> +	} else if (ret) {
>> +		return ret;
>> +	}
>>
>>    	/*
>>    	 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
>> @@ -322,6 +364,7 @@ static const struct dev_pm_ops qcom_wdt_pm_ops = {
>>    };
>>
>>    static const struct of_device_id qcom_wdt_of_table[] = {
>> +	{ .compatible = "qcom,apss-wdt-ipq5424", .data = &match_data_ipq5424 },
>>    	{ .compatible = "qcom,kpss-timer", .data = &match_data_apcs_tmr },
>>    	{ .compatible = "qcom,scss-timer", .data = &match_data_apcs_tmr },
>>    	{ .compatible = "qcom,kpss-wdt", .data = &match_data_kpss },
>>
>> --
>> 2.34.1
>>
>>