[PATCH v3 0/4] Add support to read the restart reason from IMEM

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

With the CONFIG_WATCHDOG_SYSFS enabled, user can extract the information
as below:

cat /sys/devices/platform/soc@0/f410000.watchdog/watchdog/watchdog0/bootstatus
32

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v3:
- Picked up the relevant tags
- Dropped the fallback compatible handling
- Split the driver changes into 2. Introduce the device data in one and
  extend the same in another for the use case
- Linke to v2:
  https://lore.kernel.org/linux-arm-msm/20250416-wdt_reset_reason-v2-0-c65bba312914@oss.qualcomm.com/

Changes in v2:
- Dropped the RFC tag
- Reworked the driver changes to use the syscon API
- Link to v1:
  https://lore.kernel.org/linux-arm-msm/20250408-wdt_reset_reason-v1-0-e6ec30c2c926@oss.qualcomm.com/

---
Kathiravan Thirumoorthy (4):
      dt-bindings: sram: qcom,imem: Document IPQ5424 compatible
      arm64: dts: qcom: ipq5424: Add the IMEM node
      watchdog: qcom: introduce the device data for IPQ5424 watchdog device
      watchdog: qcom: add support to read the restart reason from IMEM

 .../devicetree/bindings/sram/qcom,imem.yaml        |  1 +
 arch/arm64/boot/dts/qcom/ipq5424.dtsi              |  9 +++++
 drivers/watchdog/qcom-wdt.c                        | 47 +++++++++++++++++++++-
 3 files changed, 55 insertions(+), 2 deletions(-)
---
base-commit: 3e039dcc9c1320c0d33ddd51c372dcc91d3ea3c7
change-id: 20250408-wdt_reset_reason-e12921963fa6

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

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

[Kathiravan Thirumoorthy]

Add compatible for Qualcomm's IPQ5424 IMEM.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v3:
	- Picked up the A-b tag
---
 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 v3 2/4] 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.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v3:
	- Picked up the R-b tag
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 v3 3/4] watchdog: qcom: introduce the device data for IPQ5424 watchdog device

[Kathiravan Thirumoorthy]

To retrieve the restart reason from IMEM, certain device specific data
like IMEM compatible to lookup, location of IMEM to read, etc should be
defined. To achieve that, introduce the separate device data for IPQ5424
and add the required details subsequently.

Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
---
Changes in v3:
	- New patch
---
 drivers/watchdog/qcom-wdt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..dfaac5995c84c1f377023e6e62770c5548528a4c 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -181,6 +181,12 @@ static const struct qcom_wdt_match_data match_data_apcs_tmr = {
 	.max_tick_count = 0x10000000U,
 };
 
+static const struct qcom_wdt_match_data match_data_ipq5424 = {
+	.offset = reg_offset_data_kpss,
+	.pretimeout = true,
+	.max_tick_count = 0xFFFFFU,
+};
+
 static const struct qcom_wdt_match_data match_data_kpss = {
 	.offset = reg_offset_data_kpss,
 	.pretimeout = true,
@@ -322,6 +328,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

[PATCH v3 4/4] 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, XBL update the information in the IMEM region.
Update the driver to read the restart reason from IMEM and populate the
bootstatus accordingly.

With the CONFIG_WATCHDOG_SYSFS enabled, user can extract the information
as below:

cat /sys/devices/platform/soc@0/f410000.watchdog/watchdog/watchdog0/bootstatus
32

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 v3:
	- Split the introduction of device data into separate patch
	- s/bootloaders/XBL - for clarity of which bootloader is
	  involved
	- Mention the sysfs path on to extract this information
	- s/compatible/imem_compatible in the device data structure to
	  avoid the confusion / better naming
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 | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index dfaac5995c84c1f377023e6e62770c5548528a4c..f2cb8bfdf53a5090bcfff6ea3a23005b629ef948 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 {
@@ -42,6 +44,9 @@ struct qcom_wdt_match_data {
 	const u32 *offset;
 	bool pretimeout;
 	u32 max_tick_count;
+	const char *imem_compatible;
+	unsigned int restart_reason_offset;
+	unsigned int non_secure_wdt_val;
 };
 
 struct qcom_wdt {
@@ -185,6 +190,9 @@ static const struct qcom_wdt_match_data match_data_ipq5424 = {
 	.offset = reg_offset_data_kpss,
 	.pretimeout = true,
 	.max_tick_count = 0xFFFFFU,
+	.imem_compatible = "qcom,ipq5424-imem",
+	.restart_reason_offset = 0x7b0,
+	.non_secure_wdt_val = 0x5,
 };
 
 static const struct qcom_wdt_match_data match_data_kpss = {
@@ -193,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->imem_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;
@@ -273,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

-- 
2.34.1

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

[Krzysztof Kozlowski]

On 02/05/2025 15:17, Kathiravan Thirumoorthy wrote:
>  
> +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->imem_compatible);
And how are you handling proper probe ordering? Use phandles and define
this as an ABI.

Best regards,
Krzysztof

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

[Guenter Roeck]

On 5/2/25 06:17, 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, XBL update the information in the IMEM region.
> Update the driver to read the restart reason from IMEM and populate the
> bootstatus accordingly.
> 
> With the CONFIG_WATCHDOG_SYSFS enabled, user can extract the information
> as below:
> 
> cat /sys/devices/platform/soc@0/f410000.watchdog/watchdog/watchdog0/bootstatus
> 32
> 
> 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 v3:
> 	- Split the introduction of device data into separate patch
> 	- s/bootloaders/XBL - for clarity of which bootloader is
> 	  involved
> 	- Mention the sysfs path on to extract this information
> 	- s/compatible/imem_compatible in the device data structure to
> 	  avoid the confusion / better naming
> 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 | 40 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index dfaac5995c84c1f377023e6e62770c5548528a4c..f2cb8bfdf53a5090bcfff6ea3a23005b629ef948 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 {
> @@ -42,6 +44,9 @@ struct qcom_wdt_match_data {
>   	const u32 *offset;
>   	bool pretimeout;
>   	u32 max_tick_count;
> +	const char *imem_compatible;
> +	unsigned int restart_reason_offset;
> +	unsigned int non_secure_wdt_val;
>   };
>   
>   struct qcom_wdt {
> @@ -185,6 +190,9 @@ static const struct qcom_wdt_match_data match_data_ipq5424 = {
>   	.offset = reg_offset_data_kpss,
>   	.pretimeout = true,
>   	.max_tick_count = 0xFFFFFU,
> +	.imem_compatible = "qcom,ipq5424-imem",
> +	.restart_reason_offset = 0x7b0,
> +	.non_secure_wdt_val = 0x5,
>   };
>   
>   static const struct qcom_wdt_match_data match_data_kpss = {
> @@ -193,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->imem_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;
> @@ -273,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;
> +	}

Seems odd to me that there is now a function qcom_wdt_get_restart_reason()
but it doesn't handle all means to get the restart reason. Maybe I missed it,
but what is the reason for that ? Why not move reading WDT_STS into
qcom_wdt_get_restart_reason() as well ?

Guenter


>   
>   	/*
>   	 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
> 

Re: [PATCH v3 3/4] watchdog: qcom: introduce the device data for IPQ5424 watchdog device

[Guenter Roeck]

On 5/2/25 06:17, Kathiravan Thirumoorthy wrote:
> To retrieve the restart reason from IMEM, certain device specific data
> like IMEM compatible to lookup, location of IMEM to read, etc should be
> defined. To achieve that, introduce the separate device data for IPQ5424
> and add the required details subsequently.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

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

[Konrad Dybcio]

On 5/2/25 3:17 PM, 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, XBL update the information in the IMEM region.
> Update the driver to read the restart reason from IMEM and populate the
> bootstatus accordingly.
> 
> With the CONFIG_WATCHDOG_SYSFS enabled, user can extract the information
> as below:
> 
> cat /sys/devices/platform/soc@0/f410000.watchdog/watchdog/watchdog0/bootstatus
> 32
> 
> 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>
> ---

[...]

> +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->imem_compatible);

Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi

That way all platform specifics will live in the DT, requiring no
hardcode-y driver changes on similar platforms

Konrad

Re: [PATCH v3 3/4] watchdog: qcom: introduce the device data for IPQ5424 watchdog device

[Dmitry Baryshkov]

On Fri, May 02, 2025 at 06:47:51PM +0530, Kathiravan Thirumoorthy wrote:
> To retrieve the restart reason from IMEM, certain device specific data
> like IMEM compatible to lookup, location of IMEM to read, etc should be
> defined. To achieve that, introduce the separate device data for IPQ5424
> and add the required details subsequently.
> 
> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
> ---
> Changes in v3:
> 	- New patch
> ---
>  drivers/watchdog/qcom-wdt.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..dfaac5995c84c1f377023e6e62770c5548528a4c 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -181,6 +181,12 @@ static const struct qcom_wdt_match_data match_data_apcs_tmr = {
>  	.max_tick_count = 0x10000000U,
>  };
>  
> +static const struct qcom_wdt_match_data match_data_ipq5424 = {
> +	.offset = reg_offset_data_kpss,
> +	.pretimeout = true,
> +	.max_tick_count = 0xFFFFFU,
> +};
> +
>  static const struct qcom_wdt_match_data match_data_kpss = {
>  	.offset = reg_offset_data_kpss,
>  	.pretimeout = true,
> @@ -322,6 +328,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 },

Shouldn't it be qcom,ipq5424-apss-wdt ?

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

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Fri, May 02, 2025 at 06:47:52PM +0530, 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, XBL update the information in the IMEM region.
> Update the driver to read the restart reason from IMEM and populate the
> bootstatus accordingly.
> 
> With the CONFIG_WATCHDOG_SYSFS enabled, user can extract the information
> as below:
> 
> cat /sys/devices/platform/soc@0/f410000.watchdog/watchdog/watchdog0/bootstatus
> 32
> 
> 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 v3:
> 	- Split the introduction of device data into separate patch
> 	- s/bootloaders/XBL - for clarity of which bootloader is
> 	  involved
> 	- Mention the sysfs path on to extract this information
> 	- s/compatible/imem_compatible in the device data structure to
> 	  avoid the confusion / better naming
> 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 | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index dfaac5995c84c1f377023e6e62770c5548528a4c..f2cb8bfdf53a5090bcfff6ea3a23005b629ef948 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 {
> @@ -42,6 +44,9 @@ struct qcom_wdt_match_data {
>  	const u32 *offset;
>  	bool pretimeout;
>  	u32 max_tick_count;
> +	const char *imem_compatible;
> +	unsigned int restart_reason_offset;
> +	unsigned int non_secure_wdt_val;
>  };
>  
>  struct qcom_wdt {
> @@ -185,6 +190,9 @@ static const struct qcom_wdt_match_data match_data_ipq5424 = {
>  	.offset = reg_offset_data_kpss,
>  	.pretimeout = true,
>  	.max_tick_count = 0xFFFFFU,
> +	.imem_compatible = "qcom,ipq5424-imem",
> +	.restart_reason_offset = 0x7b0,
> +	.non_secure_wdt_val = 0x5,
>  };
>  
>  static const struct qcom_wdt_match_data match_data_kpss = {
> @@ -193,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->imem_compatible);
> +	if (IS_ERR(imem))
> +		return PTR_ERR(imem);

Why? Just pass the syscon directly via DT.

> +
> +	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;
> @@ -273,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
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 3/4] watchdog: qcom: introduce the device data for IPQ5424 watchdog device

[Kathiravan Thirumoorthy]

On 5/2/2025 8:23 PM, Dmitry Baryshkov wrote:
> On Fri, May 02, 2025 at 06:47:51PM +0530, Kathiravan Thirumoorthy wrote:
>> To retrieve the restart reason from IMEM, certain device specific data
>> like IMEM compatible to lookup, location of IMEM to read, etc should be
>> defined. To achieve that, introduce the separate device data for IPQ5424
>> and add the required details subsequently.
>>
>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> ---
>> Changes in v3:
>> 	- New patch
>> ---
>>   drivers/watchdog/qcom-wdt.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..dfaac5995c84c1f377023e6e62770c5548528a4c 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -181,6 +181,12 @@ static const struct qcom_wdt_match_data match_data_apcs_tmr = {
>>   	.max_tick_count = 0x10000000U,
>>   };
>>   
>> +static const struct qcom_wdt_match_data match_data_ipq5424 = {
>> +	.offset = reg_offset_data_kpss,
>> +	.pretimeout = true,
>> +	.max_tick_count = 0xFFFFFU,
>> +};
>> +
>>   static const struct qcom_wdt_match_data match_data_kpss = {
>>   	.offset = reg_offset_data_kpss,
>>   	.pretimeout = true,
>> @@ -322,6 +328,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 },
> Shouldn't it be qcom,ipq5424-apss-wdt ?


Currently, the compatible string is "qcom,apss-wdt-ipq5424". So used as 
it is.


>
>>   	{ .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 v3 4/4] watchdog: qcom: add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

On 5/2/2025 7:03 PM, Krzysztof Kozlowski wrote:
> On 02/05/2025 15:17, Kathiravan Thirumoorthy wrote:
>>   
>> +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->imem_compatible);
> And how are you handling proper probe ordering? Use phandles and define
> this as an ABI.


Sure, I will follow the Konrad's suggestion.


> Best regards,
> Krzysztof

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

[Kathiravan Thirumoorthy]

On 5/2/2025 7:03 PM, Guenter Roeck wrote:
>>   static int qcom_wdt_probe(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>> @@ -273,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;
>> +    }
>
> Seems odd to me that there is now a function 
> qcom_wdt_get_restart_reason()
> but it doesn't handle all means to get the restart reason. Maybe I 
> missed it,
> but what is the reason for that ? Why not move reading WDT_STS into
> qcom_wdt_get_restart_reason() as well ?


No specific reason as such. I was little hesitant use "goto" statements 
and such as. So I thought this would be little cleaner approach. Please 
let me know if I have consolidate everything under 
qcom_wdt_get_restart_reason().


>
> Guenter
>
>
>>         /*
>>        * If 'timeout-sec' unspecified in devicetree, assume a 30 second

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

[Kathiravan Thirumoorthy]

On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>> +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->imem_compatible);
> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>
> That way all platform specifics will live in the DT, requiring no
> hardcode-y driver changes on similar platforms


Thanks. I thought about this API but it didn't strike that I can use the 
args to fetch and match the value.

I need a suggestion here. There is a plan to extend this feature to 
other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT 
cause as well. For IPQ5424, all 3 cause will support and for other IPQ 
platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any 
case, can I define the DT entry like below

         imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under 
value> <Overheat value>>;

and store these in values args[1], args[2] and args[3] respectively and 
use it for manipulation? If any of the platform doesn't support all 3, I 
can update the bindings and define the number of args as required.

Is this approach fine. Please let me know your comments.


>
> Konrad

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

[Kathiravan Thirumoorthy]

On 5/2/2025 8:24 PM, Dmitry Baryshkov wrote:
>> +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->imem_compatible);
>> +	if (IS_ERR(imem))
>> +		return PTR_ERR(imem);
> Why? Just pass the syscon directly via DT.


Ack. As replied to Konrad, will rework this.

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

[Konrad Dybcio]

On 5/2/25 6:08 PM, Kathiravan Thirumoorthy wrote:
> 
> On 5/2/2025 7:03 PM, Guenter Roeck wrote:
>>>   static int qcom_wdt_probe(struct platform_device *pdev)
>>>   {
>>>       struct device *dev = &pdev->dev;
>>> @@ -273,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;
>>> +    }
>>
>> Seems odd to me that there is now a function qcom_wdt_get_restart_reason()
>> but it doesn't handle all means to get the restart reason. Maybe I missed it,
>> but what is the reason for that ? Why not move reading WDT_STS into
>> qcom_wdt_get_restart_reason() as well ?
> 
> 
> No specific reason as such. I was little hesitant use "goto" statements and such as. So I thought this would be little cleaner approach. Please let me know if I have consolidate everything under qcom_wdt_get_restart_reason().

You can try grabbing the syscon handle, and if absent, fall back to the
common handling

Konrad

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

[Konrad Dybcio]

On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
> 
> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>> +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->imem_compatible);
>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>>
>> That way all platform specifics will live in the DT, requiring no
>> hardcode-y driver changes on similar platforms
> 
> 
> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value.
> 
> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below
> 
>         imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>;
> 
> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required.
Let's call the property qcom,restart-reason and only pass the register value

Because we may have any number of crazy combinations of various restart
reasons, we can go two paths:

1. promise really really really hard we won't be too crazy with the number
   of possible values and put them in the driver
2. go all out on DT properties (such as `bootstatus-overheat`,
`bootstatus-fanfault` etc.

I'd much prefer to go with 1 really.. If we used nvmem, we could have a map
of cell names to restart reasons, but we've already established IMEM is
volatile and we shouldn't mess up the convention just because that
subsystem has nicer APIs..

Unless we rename the subsystem to `fuses`, `magic-values` or something..
+Srini? :P

Konrad

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

[Kathiravan Thirumoorthy]

On 5/3/2025 3:53 AM, Konrad Dybcio wrote:
> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>>> +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->imem_compatible);
>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>>>
>>> That way all platform specifics will live in the DT, requiring no
>>> hardcode-y driver changes on similar platforms
>>
>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value.
>>
>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below
>>
>>          imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>;
>>
>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required.
> Let's call the property qcom,restart-reason and only pass the register value
>
> Because we may have any number of crazy combinations of various restart
> reasons, we can go two paths:
>
> 1. promise really really really hard we won't be too crazy with the number
>     of possible values and put them in the driver
> 2. go all out on DT properties (such as `bootstatus-overheat`,
> `bootstatus-fanfault` etc.


Thanks Konrad for the suggestions and the offline discussions.

@Guenter, I need a suggestion here. Currently as part of this series, we 
are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT 
reasons.

Once this is done, we do have the custom reason codes like Kernel Panic, 
Secure Watchdog Bite, Bus error timeout, Bus error access and few many. 
Is it okay to expose these values also via the bootstatus sysFS by 
extending the current list of reasons? Since these are outside the scope 
of watchdog, need your thoughts on this.


>
> I'd much prefer to go with 1 really.. If we used nvmem, we could have a map
> of cell names to restart reasons, but we've already established IMEM is
> volatile and we shouldn't mess up the convention just because that
> subsystem has nicer APIs..
>
> Unless we rename the subsystem to `fuses`, `magic-values` or something..
> +Srini? :P
>
> Konrad

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

[Kathiravan Thirumoorthy]

On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote:
>
> On 5/3/2025 3:53 AM, Konrad Dybcio wrote:
>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>>>> +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->imem_compatible);
>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see 
>>>> e.g.
>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in 
>>>> x1e80100.dtsi
>>>>
>>>> That way all platform specifics will live in the DT, requiring no
>>>> hardcode-y driver changes on similar platforms
>>>
>>> Thanks. I thought about this API but it didn't strike that I can use 
>>> the args to fetch and match the value.
>>>
>>> I need a suggestion here. There is a plan to extend this feature to 
>>> other IPQ targets and also support WDIOF_POWERUNDER and 
>>> WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support 
>>> and for other IPQ platforms, we are exploring how to integrate 
>>> WDIOF_OVERHEAT. In any case, can I define the DT entry like below
>>>
>>>          imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power 
>>> Under value> <Overheat value>>;
>>>
>>> and store these in values args[1], args[2] and args[3] respectively 
>>> and use it for manipulation? If any of the platform doesn't support 
>>> all 3, I can update the bindings and define the number of args as 
>>> required.
>> Let's call the property qcom,restart-reason and only pass the 
>> register value
>>
>> Because we may have any number of crazy combinations of various restart
>> reasons, we can go two paths:
>>
>> 1. promise really really really hard we won't be too crazy with the 
>> number
>>     of possible values and put them in the driver
>> 2. go all out on DT properties (such as `bootstatus-overheat`,
>> `bootstatus-fanfault` etc.
>
>
> Thanks Konrad for the suggestions and the offline discussions.
>
> @Guenter, I need a suggestion here. Currently as part of this series, 
> we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, 
> WDIOF_OVERHEAT reasons.
>
> Once this is done, we do have the custom reason codes like Kernel 
> Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and 
> few many. Is it okay to expose these values also via the bootstatus 
> sysFS by extending the current list of reasons? Since these are 
> outside the scope of watchdog, need your thoughts on this.


Konrad / Guenter,

We had a further discussion on this internally. Outcome is, it wouldn't 
be ideal to hook the custom restart reason codes in watchdog framework, 
since there is no involvement of watchdog in such cases. Also I don't 
find any references to hook the custom values in watchdog's bootstatus.

If this is fine, I'm planning to resend the series to handle only the 
non secure watchdog timeout case. In that case, as suggested by Konrad, 
everything will be handled in DT like below to avoid the device data.

imem,phandle = <&phandle <imem_offset> <value>>;

Kindly share your thoughts and inputs on this to proceed further.


>
>
>>
>> I'd much prefer to go with 1 really.. If we used nvmem, we could have 
>> a map
>> of cell names to restart reasons, but we've already established IMEM is
>> volatile and we shouldn't mess up the convention just because that
>> subsystem has nicer APIs..
>>
>> Unless we rename the subsystem to `fuses`, `magic-values` or something..
>> +Srini? :P
>>
>> Konrad

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

[Guenter Roeck]

On 5/14/25 06:15, Kathiravan Thirumoorthy wrote:
> 
> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote:
>>
>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote:
>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>>>>> +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->imem_compatible);
>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>>>>>
>>>>> That way all platform specifics will live in the DT, requiring no
>>>>> hardcode-y driver changes on similar platforms
>>>>
>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value.
>>>>
>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below
>>>>
>>>>          imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>;
>>>>
>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required.
>>> Let's call the property qcom,restart-reason and only pass the register value
>>>
>>> Because we may have any number of crazy combinations of various restart
>>> reasons, we can go two paths:
>>>
>>> 1. promise really really really hard we won't be too crazy with the number
>>>     of possible values and put them in the driver
>>> 2. go all out on DT properties (such as `bootstatus-overheat`,
>>> `bootstatus-fanfault` etc.
>>
>>
>> Thanks Konrad for the suggestions and the offline discussions.
>>
>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons.
>>
>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this.
> 
> 
> Konrad / Guenter,
> 
> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus.
> 
Correct. The watchdog subsystem can only handle watchdog triggered reboots/resets.


> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data.
> 
> imem,phandle = <&phandle <imem_offset> <value>>;
> 
> Kindly share your thoughts and inputs on this to proceed further.
> 
Sounds good to me.

Guenter

> 
>>
>>
>>>
>>> I'd much prefer to go with 1 really.. If we used nvmem, we could have a map
>>> of cell names to restart reasons, but we've already established IMEM is
>>> volatile and we shouldn't mess up the convention just because that
>>> subsystem has nicer APIs..
>>>
>>> Unless we rename the subsystem to `fuses`, `magic-values` or something..
>>> +Srini? :P
>>>
>>> Konrad

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

[Konrad Dybcio]

On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote:
> 
> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote:
>>
>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote:
>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>>>>> +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->imem_compatible);
>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>>>>>
>>>>> That way all platform specifics will live in the DT, requiring no
>>>>> hardcode-y driver changes on similar platforms
>>>>
>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value.
>>>>
>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below
>>>>
>>>>          imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>;
>>>>
>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required.
>>> Let's call the property qcom,restart-reason and only pass the register value
>>>
>>> Because we may have any number of crazy combinations of various restart
>>> reasons, we can go two paths:
>>>
>>> 1. promise really really really hard we won't be too crazy with the number
>>>     of possible values and put them in the driver
>>> 2. go all out on DT properties (such as `bootstatus-overheat`,
>>> `bootstatus-fanfault` etc.
>>
>>
>> Thanks Konrad for the suggestions and the offline discussions.
>>
>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons.
>>
>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this.
> 
> 
> Konrad / Guenter,
> 
> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus.
> 
> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data.
> 
> imem,phandle = <&phandle <imem_offset> <value>>;

the part before the comma is a vendor prefix, so that must be qcom,xyz

what are your plans for the other reboot reasons? are we scrapping them?

Konrad

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

[Kathiravan Thirumoorthy]

On 5/16/2025 4:48 PM, Konrad Dybcio wrote:
> On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote:
>> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote:
>>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote:
>>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
>>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>>>>>> +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->imem_compatible);
>>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>>>>>>
>>>>>> That way all platform specifics will live in the DT, requiring no
>>>>>> hardcode-y driver changes on similar platforms
>>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value.
>>>>>
>>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below
>>>>>
>>>>>           imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>;
>>>>>
>>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required.
>>>> Let's call the property qcom,restart-reason and only pass the register value
>>>>
>>>> Because we may have any number of crazy combinations of various restart
>>>> reasons, we can go two paths:
>>>>
>>>> 1. promise really really really hard we won't be too crazy with the number
>>>>      of possible values and put them in the driver
>>>> 2. go all out on DT properties (such as `bootstatus-overheat`,
>>>> `bootstatus-fanfault` etc.
>>>
>>> Thanks Konrad for the suggestions and the offline discussions.
>>>
>>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons.
>>>
>>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this.
>>
>> Konrad / Guenter,
>>
>> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus.
>>
>> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data.
>>
>> imem,phandle = <&phandle <imem_offset> <value>>;
> the part before the comma is a vendor prefix, so that must be qcom,xyz


Sure, will name it as qcom,imem-phandle. Hope this name is fine.


>
> what are your plans for the other reboot reasons? are we scrapping them?


No, we are not scrapping it. We are exploring further on where to put 
this. May be we can put those logic in some simple driver named as 
ipq-restart-reason.c under drivers/soc/qcom/?


>
> Konrad

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

[Konrad Dybcio]

On 5/16/25 2:52 PM, Kathiravan Thirumoorthy wrote:
> 
> On 5/16/2025 4:48 PM, Konrad Dybcio wrote:
>> On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote:
>>> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote:
>>>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote:
>>>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
>>>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>>>>>>> +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->imem_compatible);
>>>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
>>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>>>>>>>
>>>>>>> That way all platform specifics will live in the DT, requiring no
>>>>>>> hardcode-y driver changes on similar platforms
>>>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value.
>>>>>>
>>>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below
>>>>>>
>>>>>>           imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>;
>>>>>>
>>>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required.
>>>>> Let's call the property qcom,restart-reason and only pass the register value
>>>>>
>>>>> Because we may have any number of crazy combinations of various restart
>>>>> reasons, we can go two paths:
>>>>>
>>>>> 1. promise really really really hard we won't be too crazy with the number
>>>>>      of possible values and put them in the driver
>>>>> 2. go all out on DT properties (such as `bootstatus-overheat`,
>>>>> `bootstatus-fanfault` etc.
>>>>
>>>> Thanks Konrad for the suggestions and the offline discussions.
>>>>
>>>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons.
>>>>
>>>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this.
>>>
>>> Konrad / Guenter,
>>>
>>> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus.
>>>
>>> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data.
>>>
>>> imem,phandle = <&phandle <imem_offset> <value>>;
>> the part before the comma is a vendor prefix, so that must be qcom,xyz
> 
> 
> Sure, will name it as qcom,imem-phandle. Hope this name is fine.

just qcom,imem is fine, phandle is a datatype described in dt-bindings

>> what are your plans for the other reboot reasons? are we scrapping them?
> 
> 
> No, we are not scrapping it. We are exploring further on where to put this. May be we can put those logic in some simple driver named as ipq-restart-reason.c under drivers/soc/qcom/?

I see drivers/power/reset/at91-reset.c does something like this

Konrad

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

[Kathiravan Thirumoorthy]

On 5/16/2025 10:05 PM, Konrad Dybcio wrote:
> On 5/16/25 2:52 PM, Kathiravan Thirumoorthy wrote:
>> On 5/16/2025 4:48 PM, Konrad Dybcio wrote:
>>> On 5/14/25 3:15 PM, Kathiravan Thirumoorthy wrote:
>>>> On 5/6/2025 4:31 PM, Kathiravan Thirumoorthy wrote:
>>>>> On 5/3/2025 3:53 AM, Konrad Dybcio wrote:
>>>>>> On 5/2/25 6:28 PM, Kathiravan Thirumoorthy wrote:
>>>>>>> On 5/2/2025 7:33 PM, Konrad Dybcio wrote:
>>>>>>>>> +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->imem_compatible);
>>>>>>>> Try syscon_regmap_lookup_by_phandle_args() and pass a phandle, see e.g.
>>>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c & phy@1bfc000 in x1e80100.dtsi
>>>>>>>>
>>>>>>>> That way all platform specifics will live in the DT, requiring no
>>>>>>>> hardcode-y driver changes on similar platforms
>>>>>>> Thanks. I thought about this API but it didn't strike that I can use the args to fetch and match the value.
>>>>>>>
>>>>>>> I need a suggestion here. There is a plan to extend this feature to other IPQ targets and also support WDIOF_POWERUNDER and WDIOF_OVERHEAT cause as well. For IPQ5424, all 3 cause will support and for other IPQ platforms, we are exploring how to integrate WDIOF_OVERHEAT. In any case, can I define the DT entry like below
>>>>>>>
>>>>>>>            imem,phandle = <&imem 0x7b0 <Non secure WDT value> <Power Under value> <Overheat value>>;
>>>>>>>
>>>>>>> and store these in values args[1], args[2] and args[3] respectively and use it for manipulation? If any of the platform doesn't support all 3, I can update the bindings and define the number of args as required.
>>>>>> Let's call the property qcom,restart-reason and only pass the register value
>>>>>>
>>>>>> Because we may have any number of crazy combinations of various restart
>>>>>> reasons, we can go two paths:
>>>>>>
>>>>>> 1. promise really really really hard we won't be too crazy with the number
>>>>>>       of possible values and put them in the driver
>>>>>> 2. go all out on DT properties (such as `bootstatus-overheat`,
>>>>>> `bootstatus-fanfault` etc.
>>>>> Thanks Konrad for the suggestions and the offline discussions.
>>>>>
>>>>> @Guenter, I need a suggestion here. Currently as part of this series, we are planning to expose WDIOF_CARDRESET, WDIOF_POWERUNDER, WDIOF_OVERHEAT reasons.
>>>>>
>>>>> Once this is done, we do have the custom reason codes like Kernel Panic, Secure Watchdog Bite, Bus error timeout, Bus error access and few many. Is it okay to expose these values also via the bootstatus sysFS by extending the current list of reasons? Since these are outside the scope of watchdog, need your thoughts on this.
>>>> Konrad / Guenter,
>>>>
>>>> We had a further discussion on this internally. Outcome is, it wouldn't be ideal to hook the custom restart reason codes in watchdog framework, since there is no involvement of watchdog in such cases. Also I don't find any references to hook the custom values in watchdog's bootstatus.
>>>>
>>>> If this is fine, I'm planning to resend the series to handle only the non secure watchdog timeout case. In that case, as suggested by Konrad, everything will be handled in DT like below to avoid the device data.
>>>>
>>>> imem,phandle = <&phandle <imem_offset> <value>>;
>>> the part before the comma is a vendor prefix, so that must be qcom,xyz
>>
>> Sure, will name it as qcom,imem-phandle. Hope this name is fine.
> just qcom,imem is fine, phandle is a datatype described in dt-bindings


Sure thanks.


>
>>> what are your plans for the other reboot reasons? are we scrapping them?
>>
>> No, we are not scrapping it. We are exploring further on where to put this. May be we can put those logic in some simple driver named as ipq-restart-reason.c under drivers/soc/qcom/?
> I see drivers/power/reset/at91-reset.c does something like this


Thanks for the reference.  I will submit the patches in a couple of weeks.


>
> Konrad

Re: (subset) [PATCH v3 0/4] Add support to read the restart reason from IMEM

[Bjorn Andersson]

On Fri, 02 May 2025 18:47:48 +0530, 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).
> 
> [...]

Applied, thanks!

[1/4] dt-bindings: sram: qcom,imem: Document IPQ5424 compatible
      commit: 3fa1095979393d5b178264cc1bdfb473e80ab774

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: (subset) [PATCH v3 0/4] Add support to read the restart reason from IMEM

[Kathiravan Thirumoorthy]

On 8/12/2025 12:10 AM, Bjorn Andersson wrote:
> On Fri, 02 May 2025 18:47:48 +0530, 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).
>>
>> [...]
> Applied, thanks!
>
> [1/4] dt-bindings: sram: qcom,imem: Document IPQ5424 compatible
>        commit: 3fa1095979393d5b178264cc1bdfb473e80ab774


Bjorn, IIUC, based on the discussion [1], moving forward we should 
describe the IMEM as "mmio-sram" rather than the "syscon" or 
"simple-mfd". So we need to drop this change from tree.

[1] 
https://lore.kernel.org/linux-arm-msm/e4c5ecc3-fd97-4b13-a057-bb1a3b7f9207@kernel.org/


>
> Best regards,