[PATCH v2 0/3] Add support for WDIOF_CARDRESET on TI AM65x

[PATCH v2 0/3] Add support for WDIOF_CARDRESET on TI AM65x

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

The watchdog hardware of TI AM65X platform does not support
WDIOF_CARDRESET feature, add a reserved memory to save the watchdog
reset cause, to know if the board reboot is due to a watchdog reset.

Li Hua Qian (3):
  dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET
  arm64: dts: ti: Add reserved memory for watchdog
  watchdog:rit_wdt: Add support for WDIOF_CARDRESET

 .../bindings/watchdog/ti,rti-wdt.yaml         | 13 ++++-
 .../boot/dts/ti/k3-am65-iot2050-common.dtsi   | 11 +++++
 drivers/watchdog/rti_wdt.c                    | 48 +++++++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 0/3] Add support for WDIOF_CARDRESET on TI AM65x

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

The watchdog hardware of TI AM65X platform does not support
WDIOF_CARDRESET feature, add a reserved memory to save the watchdog
reset cause, to know if the board reboot is due to a watchdog reset.

Li Hua Qian (3):
  dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET
  arm64: dts: ti: Add reserved memory for watchdog
  watchdog:rit_wdt: Add support for WDIOF_CARDRESET

 .../bindings/watchdog/ti,rti-wdt.yaml         | 13 ++++-
 .../boot/dts/ti/k3-am65-iot2050-common.dtsi   | 11 +++++
 drivers/watchdog/rti_wdt.c                    | 48 +++++++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
watchdog cause. Add a reserved memory to know the last reboot was caused
by the watchdog card. In the reserved memory, some specific info will be
saved to indicate whether the watchdog reset was triggered in last
boot.

Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
---
 .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
index fc553211e42d..f227db08dc70 100644
--- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
@@ -26,7 +26,18 @@ properties:
       - ti,j7-rti-wdt
 
   reg:
-    maxItems: 1
+    maxItems: 2
+      description:
+	- Contains the address and the size of MCU RTI register.
+	- Contains the address and the size of reserved memory, which
+	  has the pre-stored watchdog reset cause as power-on reason. The
+	  second item is optional.
+	  In the reserved memory, the following values are needed at the
+	  first 12 bytes to tell that last boot was caused by watchdog
+	  reset.
+	  - PON_REASON_SOF_NUM:   0xBBBBCCCC
+	  - PON_REASON_MAGIC_NUM: 0xDDDDDDDD
+	  - PON_REASON_EOF_NUM:   0xCCCCBBBB
 
   clocks:
     maxItems: 1
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
watchdog cause. Add a reserved memory to know the last reboot was caused
by the watchdog card. In the reserved memory, some specific info will be
saved to indicate whether the watchdog reset was triggered in last
boot.

Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
---
 .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
index fc553211e42d..f227db08dc70 100644
--- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
@@ -26,7 +26,18 @@ properties:
       - ti,j7-rti-wdt
 
   reg:
-    maxItems: 1
+    maxItems: 2
+      description:
+	- Contains the address and the size of MCU RTI register.
+	- Contains the address and the size of reserved memory, which
+	  has the pre-stored watchdog reset cause as power-on reason. The
+	  second item is optional.
+	  In the reserved memory, the following values are needed at the
+	  first 12 bytes to tell that last boot was caused by watchdog
+	  reset.
+	  - PON_REASON_SOF_NUM:   0xBBBBCCCC
+	  - PON_REASON_MAGIC_NUM: 0xDDDDDDDD
+	  - PON_REASON_EOF_NUM:   0xCCCCBBBB
 
   clocks:
     maxItems: 1
-- 
2.34.1

[PATCH v2 2/3] arm64: dts: ti: Add reserved memory for watchdog

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

This patch adds a reserved memory for the TI AM65X platform watchdog to
reserve the specific info, triggering the watchdog reset in last boot,
to know if the board reboot is due to a watchdog reset.

Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
---
 arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
index e26bd988e522..77380e52a334 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
@@ -63,6 +63,12 @@ rtos_ipc_memory_region: ipc-memories@a2000000 {
 			alignment = <0x1000>;
 			no-map;
 		};
+
+		/* To reserve the power-on(PON) reason for watchdog reset */
+		wdt_reset_memory_region: wdt-memory@a2200000 {
+			reg = <0x00 0xa2200000 0x00 0x00001000>;
+			no-map;
+		};
 	};
 
 	leds {
@@ -718,3 +724,8 @@ &mcu_r5fss0_core1 {
 			<&mcu_r5fss0_core1_memory_region>;
 	mboxes = <&mailbox0_cluster1>, <&mbox_mcu_r5fss0_core1>;
 };
+
+&mcu_rti1 {
+	reg = <0x0 0x40610000 0x0 0x100>,
+	      <0x0 0xa2200000 0x0 0x1000>;
+};
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] arm64: dts: ti: Add reserved memory for watchdog

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

This patch adds a reserved memory for the TI AM65X platform watchdog to
reserve the specific info, triggering the watchdog reset in last boot,
to know if the board reboot is due to a watchdog reset.

Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
---
 arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
index e26bd988e522..77380e52a334 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
@@ -63,6 +63,12 @@ rtos_ipc_memory_region: ipc-memories@a2000000 {
 			alignment = <0x1000>;
 			no-map;
 		};
+
+		/* To reserve the power-on(PON) reason for watchdog reset */
+		wdt_reset_memory_region: wdt-memory@a2200000 {
+			reg = <0x00 0xa2200000 0x00 0x00001000>;
+			no-map;
+		};
 	};
 
 	leds {
@@ -718,3 +724,8 @@ &mcu_r5fss0_core1 {
 			<&mcu_r5fss0_core1_memory_region>;
 	mboxes = <&mailbox0_cluster1>, <&mbox_mcu_r5fss0_core1>;
 };
+
+&mcu_rti1 {
+	reg = <0x0 0x40610000 0x0 0x100>,
+	      <0x0 0xa2200000 0x0 0x1000>;
+};
-- 
2.34.1

[PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

This patch adds the WDIOF_CARDRESET support for the platform watchdog
whose hardware does not support this feature, to know if the board
reboot is due to a watchdog reset.

This is done via reserved memory(RAM), which indicates if specific
info saved, triggering the watchdog reset in last boot.

Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
---
 drivers/watchdog/rti_wdt.c | 48 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index ce8f18e93aa9..77fd6b54137c 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -18,6 +18,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
+#include <linux/of.h>
 
 #define DEFAULT_HEARTBEAT 60
 
@@ -52,6 +53,11 @@
 
 #define DWDST			BIT(1)
 
+#define PON_REASON_SOF_NUM	0xBBBBCCCC
+#define PON_REASON_MAGIC_NUM	0xDDDDDDDD
+#define PON_REASON_EOF_NUM	0xCCCCBBBB
+#define PON_REASON_ITEM_BITS	0xFFFFFFFF
+
 static int heartbeat = DEFAULT_HEARTBEAT;
 
 /*
@@ -198,6 +204,11 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	struct rti_wdt_device *wdt;
 	struct clk *clk;
 	u32 last_ping = 0;
+	u32 reserved_mem_size;
+	unsigned long *vaddr;
+	unsigned long paddr;
+	u32 data[3];
+	u32 reg[8];
 
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
@@ -284,6 +295,43 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "reg", reg,
+					 0, ARRAY_SIZE(reg));
+	if (ret < 0) {
+		dev_err(dev, "cannot read the reg info.\n");
+		goto err_iomap;
+	}
+
+	/*
+	 * If reserved memory is defined for watchdog reset cause.
+	 * Readout the Power-on(PON) reason and pass to bootstatus.
+	 */
+	if (ret == 8) {
+		paddr = reg[5];
+		reserved_mem_size = reg[7];
+		vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
+		if (vaddr == NULL) {
+			dev_err(dev, "Failed to map memory-region.\n");
+			goto err_iomap;
+		}
+
+		data[0] = *vaddr & PON_REASON_ITEM_BITS;
+		data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
+		data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
+
+		dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof = %lX\n",
+			data[0], data[1], data[2]);
+
+		if ((data[0] == PON_REASON_SOF_NUM)
+		    && (data[1] == PON_REASON_MAGIC_NUM)
+		    && (data[1] == PON_REASON_MAGIC_NUM)) {
+			dev_info(dev, "Watchdog reset cause detected.\n");
+			wdd->bootstatus |= WDIOF_CARDRESET;
+		}
+		memset(vaddr, 0, reserved_mem_size);
+		memunmap(vaddr);
+	}
+
 	watchdog_init_timeout(wdd, heartbeat, dev);
 
 	ret = watchdog_register_device(wdd);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[huaqian.li]

From: Li Hua Qian <huaqian.li@siemens.com>

This patch adds the WDIOF_CARDRESET support for the platform watchdog
whose hardware does not support this feature, to know if the board
reboot is due to a watchdog reset.

This is done via reserved memory(RAM), which indicates if specific
info saved, triggering the watchdog reset in last boot.

Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
---
 drivers/watchdog/rti_wdt.c | 48 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index ce8f18e93aa9..77fd6b54137c 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -18,6 +18,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
+#include <linux/of.h>
 
 #define DEFAULT_HEARTBEAT 60
 
@@ -52,6 +53,11 @@
 
 #define DWDST			BIT(1)
 
+#define PON_REASON_SOF_NUM	0xBBBBCCCC
+#define PON_REASON_MAGIC_NUM	0xDDDDDDDD
+#define PON_REASON_EOF_NUM	0xCCCCBBBB
+#define PON_REASON_ITEM_BITS	0xFFFFFFFF
+
 static int heartbeat = DEFAULT_HEARTBEAT;
 
 /*
@@ -198,6 +204,11 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	struct rti_wdt_device *wdt;
 	struct clk *clk;
 	u32 last_ping = 0;
+	u32 reserved_mem_size;
+	unsigned long *vaddr;
+	unsigned long paddr;
+	u32 data[3];
+	u32 reg[8];
 
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
@@ -284,6 +295,43 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "reg", reg,
+					 0, ARRAY_SIZE(reg));
+	if (ret < 0) {
+		dev_err(dev, "cannot read the reg info.\n");
+		goto err_iomap;
+	}
+
+	/*
+	 * If reserved memory is defined for watchdog reset cause.
+	 * Readout the Power-on(PON) reason and pass to bootstatus.
+	 */
+	if (ret == 8) {
+		paddr = reg[5];
+		reserved_mem_size = reg[7];
+		vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
+		if (vaddr == NULL) {
+			dev_err(dev, "Failed to map memory-region.\n");
+			goto err_iomap;
+		}
+
+		data[0] = *vaddr & PON_REASON_ITEM_BITS;
+		data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
+		data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
+
+		dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof = %lX\n",
+			data[0], data[1], data[2]);
+
+		if ((data[0] == PON_REASON_SOF_NUM)
+		    && (data[1] == PON_REASON_MAGIC_NUM)
+		    && (data[1] == PON_REASON_MAGIC_NUM)) {
+			dev_info(dev, "Watchdog reset cause detected.\n");
+			wdd->bootstatus |= WDIOF_CARDRESET;
+		}
+		memset(vaddr, 0, reserved_mem_size);
+		memunmap(vaddr);
+	}
+
 	watchdog_init_timeout(wdd, heartbeat, dev);
 
 	ret = watchdog_register_device(wdd);
-- 
2.34.1

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Krzysztof Kozlowski]

On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> watchdog cause. Add a reserved memory to know the last reboot was caused
> by the watchdog card. In the reserved memory, some specific info will be
> saved to indicate whether the watchdog reset was triggered in last
> boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---

Missing changelog.

>  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> index fc553211e42d..f227db08dc70 100644
> --- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> @@ -26,7 +26,18 @@ properties:
>        - ti,j7-rti-wdt
>  
>    reg:
> -    maxItems: 1
> +    maxItems: 2

The expected syntax is in such case:
  items:
    - description: ...
    - description: ...

You will find plenty of examples for this.

> +      description:
> +	- Contains the address and the size of MCU RTI register.
> +	- Contains the address and the size of reserved memory, which

I don't think Conor suggested using reg of the device, but reg of
reserved memory. This is not device address space, but just some random
memory.

memory-region seems proper to me. We were just discussing totally
useless new property of size.

What's more - you did not test it... so usual template:

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Krzysztof Kozlowski]

On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> watchdog cause. Add a reserved memory to know the last reboot was caused
> by the watchdog card. In the reserved memory, some specific info will be
> saved to indicate whether the watchdog reset was triggered in last
> boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---

Missing changelog.

>  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> index fc553211e42d..f227db08dc70 100644
> --- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> @@ -26,7 +26,18 @@ properties:
>        - ti,j7-rti-wdt
>  
>    reg:
> -    maxItems: 1
> +    maxItems: 2

The expected syntax is in such case:
  items:
    - description: ...
    - description: ...

You will find plenty of examples for this.

> +      description:
> +	- Contains the address and the size of MCU RTI register.
> +	- Contains the address and the size of reserved memory, which

I don't think Conor suggested using reg of the device, but reg of
reserved memory. This is not device address space, but just some random
memory.

memory-region seems proper to me. We were just discussing totally
useless new property of size.

What's more - you did not test it... so usual template:

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof

Re: [PATCH v2 2/3] arm64: dts: ti: Add reserved memory for watchdog

[Krzysztof Kozlowski]

On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> This patch adds a reserved memory for the TI AM65X platform watchdog to
> reserve the specific info, triggering the watchdog reset in last boot,
> to know if the board reboot is due to a watchdog reset.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---
>  arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> index e26bd988e522..77380e52a334 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> @@ -63,6 +63,12 @@ rtos_ipc_memory_region: ipc-memories@a2000000 {
>  			alignment = <0x1000>;
>  			no-map;
>  		};
> +
> +		/* To reserve the power-on(PON) reason for watchdog reset */
> +		wdt_reset_memory_region: wdt-memory@a2200000 {
> +			reg = <0x00 0xa2200000 0x00 0x00001000>;
> +			no-map;
> +		};
>  	};
>  
>  	leds {
> @@ -718,3 +724,8 @@ &mcu_r5fss0_core1 {
>  			<&mcu_r5fss0_core1_memory_region>;
>  	mboxes = <&mailbox0_cluster1>, <&mbox_mcu_r5fss0_core1>;
>  };
> +
> +&mcu_rti1 {
> +	reg = <0x0 0x40610000 0x0 0x100>,
> +	      <0x0 0xa2200000 0x0 0x1000>;

That's a total mess. reserved memory and IO address space. Nope.



Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] arm64: dts: ti: Add reserved memory for watchdog

[Krzysztof Kozlowski]

On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> This patch adds a reserved memory for the TI AM65X platform watchdog to
> reserve the specific info, triggering the watchdog reset in last boot,
> to know if the board reboot is due to a watchdog reset.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---
>  arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> index e26bd988e522..77380e52a334 100644
> --- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> @@ -63,6 +63,12 @@ rtos_ipc_memory_region: ipc-memories@a2000000 {
>  			alignment = <0x1000>;
>  			no-map;
>  		};
> +
> +		/* To reserve the power-on(PON) reason for watchdog reset */
> +		wdt_reset_memory_region: wdt-memory@a2200000 {
> +			reg = <0x00 0xa2200000 0x00 0x00001000>;
> +			no-map;
> +		};
>  	};
>  
>  	leds {
> @@ -718,3 +724,8 @@ &mcu_r5fss0_core1 {
>  			<&mcu_r5fss0_core1_memory_region>;
>  	mboxes = <&mailbox0_cluster1>, <&mbox_mcu_r5fss0_core1>;
>  };
> +
> +&mcu_rti1 {
> +	reg = <0x0 0x40610000 0x0 0x100>,
> +	      <0x0 0xa2200000 0x0 0x1000>;

That's a total mess. reserved memory and IO address space. Nope.



Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Rob Herring]

On Tue, 11 Jul 2023 17:17:11 +0800, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> watchdog cause. Add a reserved memory to know the last reboot was caused
> by the watchdog card. In the reserved memory, some specific info will be
> saved to indicate whether the watchdog reset was triggered in last
> boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---
>  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230711091713.1113010-2-huaqian.li@siemens.com

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

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

pip3 install dtschema --upgrade

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

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Rob Herring]

On Tue, 11 Jul 2023 17:17:11 +0800, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> watchdog cause. Add a reserved memory to know the last reboot was caused
> by the watchdog card. In the reserved memory, some specific info will be
> saved to indicate whether the watchdog reset was triggered in last
> boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---
>  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230711091713.1113010-2-huaqian.li@siemens.com

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

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

pip3 install dtschema --upgrade

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 11:23 +0200, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> > watchdog cause. Add a reserved memory to know the last reboot was
> > caused
> > by the watchdog card. In the reserved memory, some specific info
> > will be
> > saved to indicate whether the watchdog reset was triggered in last
> > boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> 
> Missing changelog.
Thanks for pointing out.
> 
> >  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13
> > ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-
> > wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-
> > wdt.yaml
> > index fc553211e42d..f227db08dc70 100644
> > --- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> > @@ -26,7 +26,18 @@ properties:
> >        - ti,j7-rti-wdt
> >  
> >    reg:
> > -    maxItems: 1
> > +    maxItems: 2
> 
> The expected syntax is in such case:
>   items:
>     - description: ...
>     - description: ...
> 
> You will find plenty of examples for this.
> 
> > +      description:
> > +       - Contains the address and the size of MCU RTI register.
> > +       - Contains the address and the size of reserved memory,
> > which
> 
> I don't think Conor suggested using reg of the device, but reg of
> reserved memory. This is not device address space, but just some
> random
> memory.
> 
> memory-region seems proper to me. We were just discussing totally
> useless new property of size.
I guess I misunderstood what Conor suggested, I will change back to
memory-region and get size from reserved-memory. Does this look good
to you?
> 
> What's more - you did not test it... so usual template:
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> 
> Best regards,
> Krzysztof
> 
Sorry for this, I will make sure this is no problem in the next
version.

Best regards,
Li Hua Qian

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 11:23 +0200, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> > watchdog cause. Add a reserved memory to know the last reboot was
> > caused
> > by the watchdog card. In the reserved memory, some specific info
> > will be
> > saved to indicate whether the watchdog reset was triggered in last
> > boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> 
> Missing changelog.
Thanks for pointing out.
> 
> >  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13
> > ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-
> > wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-
> > wdt.yaml
> > index fc553211e42d..f227db08dc70 100644
> > --- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> > @@ -26,7 +26,18 @@ properties:
> >        - ti,j7-rti-wdt
> >  
> >    reg:
> > -    maxItems: 1
> > +    maxItems: 2
> 
> The expected syntax is in such case:
>   items:
>     - description: ...
>     - description: ...
> 
> You will find plenty of examples for this.
> 
> > +      description:
> > +       - Contains the address and the size of MCU RTI register.
> > +       - Contains the address and the size of reserved memory,
> > which
> 
> I don't think Conor suggested using reg of the device, but reg of
> reserved memory. This is not device address space, but just some
> random
> memory.
> 
> memory-region seems proper to me. We were just discussing totally
> useless new property of size.
I guess I misunderstood what Conor suggested, I will change back to
memory-region and get size from reserved-memory. Does this look good
to you?
> 
> What's more - you did not test it... so usual template:
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> 
> Best regards,
> Krzysztof
> 
Sorry for this, I will make sure this is no problem in the next
version.

Best regards,
Li Hua Qian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 04:13 -0600, Rob Herring wrote:
> 
> On Tue, 11 Jul 2023 17:17:11 +0800, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> > watchdog cause. Add a reserved memory to know the last reboot was
> > caused
> > by the watchdog card. In the reserved memory, some specific info
> > will be
> > saved to indicate whether the watchdog reset was triggered in last
> > boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> >  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13
> > ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> [error] syntax error: mapping values are not allowed here (syntax)
> 
> dtschema/dtc warnings/errors:
> make[2]: *** Deleting file
> 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26:
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts]
> Error 1
> make[2]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:
> ignoring, error parsing file
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500:
> dt_binding_check] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230711091713.1113010-2-huaqian.li@siemens.com
> 
> The base for the series is generally the latest rc1. A different
> dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up
> to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself.
> Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up
> checking
> your schema. However, it must be unset to test all examples with your
> schema.
> 
Thanks for the test and your advice, I will make sure this is no
problem in the next version.

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 04:13 -0600, Rob Herring wrote:
> 
> On Tue, 11 Jul 2023 17:17:11 +0800, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> > watchdog cause. Add a reserved memory to know the last reboot was
> > caused
> > by the watchdog card. In the reserved memory, some specific info
> > will be
> > saved to indicate whether the watchdog reset was triggered in last
> > boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> >  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13
> > ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> [error] syntax error: mapping values are not allowed here (syntax)
> 
> dtschema/dtc warnings/errors:
> make[2]: *** Deleting file
> 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26:
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts]
> Error 1
> make[2]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:
> ignoring, error parsing file
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500:
> dt_binding_check] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230711091713.1113010-2-huaqian.li@siemens.com
> 
> The base for the series is generally the latest rc1. A different
> dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up
> to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself.
> Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up
> checking
> your schema. However, it must be unset to test all examples with your
> schema.
> 
Thanks for the test and your advice, I will make sure this is no
problem in the next version.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] arm64: dts: ti: Add reserved memory for watchdog

[Li, Hua Qian]

On Tue, 2023-07-11 at 11:24 +0200, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > This patch adds a reserved memory for the TI AM65X platform
> > watchdog to
> > reserve the specific info, triggering the watchdog reset in last
> > boot,
> > to know if the board reboot is due to a watchdog reset.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> >  arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi | 11
> > +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > index e26bd988e522..77380e52a334 100644
> > --- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > +++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > @@ -63,6 +63,12 @@ rtos_ipc_memory_region: ipc-memories@a2000000 {
> >                         alignment = <0x1000>;
> >                         no-map;
> >                 };
> > +
> > +               /* To reserve the power-on(PON) reason for watchdog
> > reset */
> > +               wdt_reset_memory_region: wdt-memory@a2200000 {
> > +                       reg = <0x00 0xa2200000 0x00 0x00001000>;
> > +                       no-map;
> > +               };
> >         };
> >  
> >         leds {
> > @@ -718,3 +724,8 @@ &mcu_r5fss0_core1 {
> >                         <&mcu_r5fss0_core1_memory_region>;
> >         mboxes = <&mailbox0_cluster1>, <&mbox_mcu_r5fss0_core1>;
> >  };
> > +
> > +&mcu_rti1 {
> > +       reg = <0x0 0x40610000 0x0 0x100>,
> > +             <0x0 0xa2200000 0x0 0x1000>;
> 
> That's a total mess. reserved memory and IO address space. Nope.
> 
> 
> 
> Best regards,
> Krzysztof
> 
Yes, I misunderstood the advice. I intend to roll back as I mentioned
in my last email. Any new suggestions please let me know, thank you!

Best regards,
Li Hua Qian

Re: [PATCH v2 2/3] arm64: dts: ti: Add reserved memory for watchdog

[Li, Hua Qian]

On Tue, 2023-07-11 at 11:24 +0200, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > This patch adds a reserved memory for the TI AM65X platform
> > watchdog to
> > reserve the specific info, triggering the watchdog reset in last
> > boot,
> > to know if the board reboot is due to a watchdog reset.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> >  arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi | 11
> > +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > index e26bd988e522..77380e52a334 100644
> > --- a/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > +++ b/arch/arm64/boot/dts/ti/k3-am65-iot2050-common.dtsi
> > @@ -63,6 +63,12 @@ rtos_ipc_memory_region: ipc-memories@a2000000 {
> >                         alignment = <0x1000>;
> >                         no-map;
> >                 };
> > +
> > +               /* To reserve the power-on(PON) reason for watchdog
> > reset */
> > +               wdt_reset_memory_region: wdt-memory@a2200000 {
> > +                       reg = <0x00 0xa2200000 0x00 0x00001000>;
> > +                       no-map;
> > +               };
> >         };
> >  
> >         leds {
> > @@ -718,3 +724,8 @@ &mcu_r5fss0_core1 {
> >                         <&mcu_r5fss0_core1_memory_region>;
> >         mboxes = <&mailbox0_cluster1>, <&mbox_mcu_r5fss0_core1>;
> >  };
> > +
> > +&mcu_rti1 {
> > +       reg = <0x0 0x40610000 0x0 0x100>,
> > +             <0x0 0xa2200000 0x0 0x1000>;
> 
> That's a total mess. reserved memory and IO address space. Nope.
> 
> 
> 
> Best regards,
> Krzysztof
> 
Yes, I misunderstood the advice. I intend to roll back as I mentioned
in my last email. Any new suggestions please let me know, thank you!

Best regards,
Li Hua Qian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Guenter Roeck]

On 7/11/23 02:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> This patch adds the WDIOF_CARDRESET support for the platform watchdog
> whose hardware does not support this feature, to know if the board
> reboot is due to a watchdog reset.
> 
> This is done via reserved memory(RAM), which indicates if specific
> info saved, triggering the watchdog reset in last boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---
>   drivers/watchdog/rti_wdt.c | 48 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index ce8f18e93aa9..77fd6b54137c 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -18,6 +18,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
> +#include <linux/of.h>
>   
>   #define DEFAULT_HEARTBEAT 60
>   
> @@ -52,6 +53,11 @@
>   
>   #define DWDST			BIT(1)
>   
> +#define PON_REASON_SOF_NUM	0xBBBBCCCC
> +#define PON_REASON_MAGIC_NUM	0xDDDDDDDD
> +#define PON_REASON_EOF_NUM	0xCCCCBBBB
> +#define PON_REASON_ITEM_BITS	0xFFFFFFFF
> +
>   static int heartbeat = DEFAULT_HEARTBEAT;
>   
>   /*
> @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   	struct rti_wdt_device *wdt;
>   	struct clk *clk;
>   	u32 last_ping = 0;
> +	u32 reserved_mem_size;
> +	unsigned long *vaddr;
> +	unsigned long paddr;
> +	u32 data[3];
> +	u32 reg[8];
>   
>   	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>   	if (!wdt)
> @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> +	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "reg", reg,
> +					 0, ARRAY_SIZE(reg));
> +	if (ret < 0) {
> +		dev_err(dev, "cannot read the reg info.\n");
> +		goto err_iomap;
> +	}

This aborts if the property does not exist, which is unacceptable.
Any such addition must be optional.

> +
> +	/*
> +	 * If reserved memory is defined for watchdog reset cause.
> +	 * Readout the Power-on(PON) reason and pass to bootstatus.
> +	 */
> +	if (ret == 8) {
> +		paddr = reg[5];
> +		reserved_mem_size = reg[7];

It seems odd that reserved_mem_size is not checked, and that it is even provided
given that it needs to be (at least) 24 bytes, and any other value does not really
make sense.

> +		vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
> +		if (vaddr == NULL) {
> +			dev_err(dev, "Failed to map memory-region.\n");
> +			goto err_iomap;

This returns 8, which would be an odd error return.

> +		}
> +
> +		data[0] = *vaddr & PON_REASON_ITEM_BITS;
> +		data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> +		data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> +

The & seems pointless / wasteful. Why ignore the upper 32 bits of each location ?
Either make it u32 or make it u64 and use the entire 64 bit. Besides,
vaddr[0..2] would make the code much easier to read.

> +		dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof = %lX\n",
> +			data[0], data[1], data[2]);
> +
> +		if ((data[0] == PON_REASON_SOF_NUM)
> +		    && (data[1] == PON_REASON_MAGIC_NUM)
> +		    && (data[1] == PON_REASON_MAGIC_NUM)) {

Unnecessary inner (), and I don't see the point of checking data[1] twice.

> +			dev_info(dev, "Watchdog reset cause detected.\n");

Unnecessary noise.

> +			wdd->bootstatus |= WDIOF_CARDRESET;
> +		}
> +		memset(vaddr, 0, reserved_mem_size);
> +		memunmap(vaddr);
> +	}

And some random data in the property is acceptable ? That is odd, especially
after mandating the property itself.

> +
>   	watchdog_init_timeout(wdd, heartbeat, dev);
>   
>   	ret = watchdog_register_device(wdd);

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Guenter Roeck]

On 7/11/23 02:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> This patch adds the WDIOF_CARDRESET support for the platform watchdog
> whose hardware does not support this feature, to know if the board
> reboot is due to a watchdog reset.
> 
> This is done via reserved memory(RAM), which indicates if specific
> info saved, triggering the watchdog reset in last boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---
>   drivers/watchdog/rti_wdt.c | 48 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index ce8f18e93aa9..77fd6b54137c 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -18,6 +18,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
> +#include <linux/of.h>
>   
>   #define DEFAULT_HEARTBEAT 60
>   
> @@ -52,6 +53,11 @@
>   
>   #define DWDST			BIT(1)
>   
> +#define PON_REASON_SOF_NUM	0xBBBBCCCC
> +#define PON_REASON_MAGIC_NUM	0xDDDDDDDD
> +#define PON_REASON_EOF_NUM	0xCCCCBBBB
> +#define PON_REASON_ITEM_BITS	0xFFFFFFFF
> +
>   static int heartbeat = DEFAULT_HEARTBEAT;
>   
>   /*
> @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   	struct rti_wdt_device *wdt;
>   	struct clk *clk;
>   	u32 last_ping = 0;
> +	u32 reserved_mem_size;
> +	unsigned long *vaddr;
> +	unsigned long paddr;
> +	u32 data[3];
> +	u32 reg[8];
>   
>   	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>   	if (!wdt)
> @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> +	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "reg", reg,
> +					 0, ARRAY_SIZE(reg));
> +	if (ret < 0) {
> +		dev_err(dev, "cannot read the reg info.\n");
> +		goto err_iomap;
> +	}

This aborts if the property does not exist, which is unacceptable.
Any such addition must be optional.

> +
> +	/*
> +	 * If reserved memory is defined for watchdog reset cause.
> +	 * Readout the Power-on(PON) reason and pass to bootstatus.
> +	 */
> +	if (ret == 8) {
> +		paddr = reg[5];
> +		reserved_mem_size = reg[7];

It seems odd that reserved_mem_size is not checked, and that it is even provided
given that it needs to be (at least) 24 bytes, and any other value does not really
make sense.

> +		vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB);
> +		if (vaddr == NULL) {
> +			dev_err(dev, "Failed to map memory-region.\n");
> +			goto err_iomap;

This returns 8, which would be an odd error return.

> +		}
> +
> +		data[0] = *vaddr & PON_REASON_ITEM_BITS;
> +		data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> +		data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> +

The & seems pointless / wasteful. Why ignore the upper 32 bits of each location ?
Either make it u32 or make it u64 and use the entire 64 bit. Besides,
vaddr[0..2] would make the code much easier to read.

> +		dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof = %lX\n",
> +			data[0], data[1], data[2]);
> +
> +		if ((data[0] == PON_REASON_SOF_NUM)
> +		    && (data[1] == PON_REASON_MAGIC_NUM)
> +		    && (data[1] == PON_REASON_MAGIC_NUM)) {

Unnecessary inner (), and I don't see the point of checking data[1] twice.

> +			dev_info(dev, "Watchdog reset cause detected.\n");

Unnecessary noise.

> +			wdd->bootstatus |= WDIOF_CARDRESET;
> +		}
> +		memset(vaddr, 0, reserved_mem_size);
> +		memunmap(vaddr);
> +	}

And some random data in the property is acceptable ? That is odd, especially
after mandating the property itself.

> +
>   	watchdog_init_timeout(wdd, heartbeat, dev);
>   
>   	ret = watchdog_register_device(wdd);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> On 7/11/23 02:17, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > This patch adds the WDIOF_CARDRESET support for the platform
> > watchdog
> > whose hardware does not support this feature, to know if the board
> > reboot is due to a watchdog reset.
> > 
> > This is done via reserved memory(RAM), which indicates if specific
> > info saved, triggering the watchdog reset in last boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> >   drivers/watchdog/rti_wdt.c | 48
> > ++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/watchdog/rti_wdt.c
> > b/drivers/watchdog/rti_wdt.c
> > index ce8f18e93aa9..77fd6b54137c 100644
> > --- a/drivers/watchdog/rti_wdt.c
> > +++ b/drivers/watchdog/rti_wdt.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/types.h>
> >   #include <linux/watchdog.h>
> > +#include <linux/of.h>
> >   
> >   #define DEFAULT_HEARTBEAT 60
> >   
> > @@ -52,6 +53,11 @@
> >   
> >   #define DWDST                 BIT(1)
> >   
> > +#define PON_REASON_SOF_NUM     0xBBBBCCCC
> > +#define PON_REASON_MAGIC_NUM   0xDDDDDDDD
> > +#define PON_REASON_EOF_NUM     0xCCCCBBBB
> > +#define PON_REASON_ITEM_BITS   0xFFFFFFFF
> > +
> >   static int heartbeat = DEFAULT_HEARTBEAT;
> >   
> >   /*
> > @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
> > platform_device *pdev)
> >         struct rti_wdt_device *wdt;
> >         struct clk *clk;
> >         u32 last_ping = 0;
> > +       u32 reserved_mem_size;
> > +       unsigned long *vaddr;
> > +       unsigned long paddr;
> > +       u32 data[3];
> > +       u32 reg[8];
> >   
> >         wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> >         if (!wdt)
> > @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
> > platform_device *pdev)
> >                 }
> >         }
> >   
> > +       ret = of_property_read_variable_u32_array(pdev-
> > >dev.of_node, "reg", reg,
> > +                                        0, ARRAY_SIZE(reg));
> > +       if (ret < 0) {
> > +               dev_err(dev, "cannot read the reg info.\n");
> > +               goto err_iomap;
> > +       }
> 
> This aborts if the property does not exist, which is unacceptable.
> Any such addition must be optional.
Agree, refactor it.
> 
> > +
> > +       /*
> > +        * If reserved memory is defined for watchdog reset cause.
> > +        * Readout the Power-on(PON) reason and pass to bootstatus.
> > +        */
> > +       if (ret == 8) {
> > +               paddr = reg[5];
> > +               reserved_mem_size = reg[7];
> 
> It seems odd that reserved_mem_size is not checked, 
ACK
> and that it is even provided
> given that it needs to be (at least) 24 bytes, and any other value
> does not really
> make sense.
> 
I was thinkg to add the reliability, but it seems to be unnecessary and
pointless. Were you suggesting that 8 bytes are enough?

> > +               vaddr = memremap(paddr, reserved_mem_size,
> > MEMREMAP_WB);
> > +               if (vaddr == NULL) {
> > +                       dev_err(dev, "Failed to map memory-
> > region.\n");
> > +                       goto err_iomap;
> 
> This returns 8, which would be an odd error return.
> 
ACK,refactor it.
> > +               }
> > +
> > +               data[0] = *vaddr & PON_REASON_ITEM_BITS;
> > +               data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> > +               data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> > +
> 
> The & seems pointless / wasteful. Why ignore the upper 32 bits of
> each location ?
> Either make it u32 or make it u64 and use the entire 64 bit. Besides,
> vaddr[0..2] would make the code much easier to read.
> 
ACK, refactor it.
> > +               dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof
> > = %lX\n",
> > +                       data[0], data[1], data[2]);
> > +
> > +               if ((data[0] == PON_REASON_SOF_NUM)
> > +                   && (data[1] == PON_REASON_MAGIC_NUM)
> > +                   && (data[1] == PON_REASON_MAGIC_NUM)) {
> 
> Unnecessary inner (), and I don't see the point of checking data[1]
> twice.
Yeah, a typo happened.
> 
> > +                       dev_info(dev, "Watchdog reset cause
> > detected.\n");
> 
> Unnecessary noise.
ACK,rename dev_info to dev_dbg.
> 
> > +                       wdd->bootstatus |= WDIOF_CARDRESET;
> > +               }
> > +               memset(vaddr, 0, reserved_mem_size);
> > +               memunmap(vaddr);
> > +       }
> 
> And some random data in the property is acceptable ? That is odd,
> especially
> after mandating the property itself.
> 
Yeah, do you have any suggestions about how to store the watchdog
reset cause?

Best regards,
Li Hua Qian
> > +
> >         watchdog_init_timeout(wdd, heartbeat, dev);
> >   
> >         ret = watchdog_register_device(wdd);
> 

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> On 7/11/23 02:17, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > This patch adds the WDIOF_CARDRESET support for the platform
> > watchdog
> > whose hardware does not support this feature, to know if the board
> > reboot is due to a watchdog reset.
> > 
> > This is done via reserved memory(RAM), which indicates if specific
> > info saved, triggering the watchdog reset in last boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> >   drivers/watchdog/rti_wdt.c | 48
> > ++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/watchdog/rti_wdt.c
> > b/drivers/watchdog/rti_wdt.c
> > index ce8f18e93aa9..77fd6b54137c 100644
> > --- a/drivers/watchdog/rti_wdt.c
> > +++ b/drivers/watchdog/rti_wdt.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/types.h>
> >   #include <linux/watchdog.h>
> > +#include <linux/of.h>
> >   
> >   #define DEFAULT_HEARTBEAT 60
> >   
> > @@ -52,6 +53,11 @@
> >   
> >   #define DWDST                 BIT(1)
> >   
> > +#define PON_REASON_SOF_NUM     0xBBBBCCCC
> > +#define PON_REASON_MAGIC_NUM   0xDDDDDDDD
> > +#define PON_REASON_EOF_NUM     0xCCCCBBBB
> > +#define PON_REASON_ITEM_BITS   0xFFFFFFFF
> > +
> >   static int heartbeat = DEFAULT_HEARTBEAT;
> >   
> >   /*
> > @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
> > platform_device *pdev)
> >         struct rti_wdt_device *wdt;
> >         struct clk *clk;
> >         u32 last_ping = 0;
> > +       u32 reserved_mem_size;
> > +       unsigned long *vaddr;
> > +       unsigned long paddr;
> > +       u32 data[3];
> > +       u32 reg[8];
> >   
> >         wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> >         if (!wdt)
> > @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
> > platform_device *pdev)
> >                 }
> >         }
> >   
> > +       ret = of_property_read_variable_u32_array(pdev-
> > >dev.of_node, "reg", reg,
> > +                                        0, ARRAY_SIZE(reg));
> > +       if (ret < 0) {
> > +               dev_err(dev, "cannot read the reg info.\n");
> > +               goto err_iomap;
> > +       }
> 
> This aborts if the property does not exist, which is unacceptable.
> Any such addition must be optional.
Agree, refactor it.
> 
> > +
> > +       /*
> > +        * If reserved memory is defined for watchdog reset cause.
> > +        * Readout the Power-on(PON) reason and pass to bootstatus.
> > +        */
> > +       if (ret == 8) {
> > +               paddr = reg[5];
> > +               reserved_mem_size = reg[7];
> 
> It seems odd that reserved_mem_size is not checked, 
ACK
> and that it is even provided
> given that it needs to be (at least) 24 bytes, and any other value
> does not really
> make sense.
> 
I was thinkg to add the reliability, but it seems to be unnecessary and
pointless. Were you suggesting that 8 bytes are enough?

> > +               vaddr = memremap(paddr, reserved_mem_size,
> > MEMREMAP_WB);
> > +               if (vaddr == NULL) {
> > +                       dev_err(dev, "Failed to map memory-
> > region.\n");
> > +                       goto err_iomap;
> 
> This returns 8, which would be an odd error return.
> 
ACK,refactor it.
> > +               }
> > +
> > +               data[0] = *vaddr & PON_REASON_ITEM_BITS;
> > +               data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> > +               data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> > +
> 
> The & seems pointless / wasteful. Why ignore the upper 32 bits of
> each location ?
> Either make it u32 or make it u64 and use the entire 64 bit. Besides,
> vaddr[0..2] would make the code much easier to read.
> 
ACK, refactor it.
> > +               dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof
> > = %lX\n",
> > +                       data[0], data[1], data[2]);
> > +
> > +               if ((data[0] == PON_REASON_SOF_NUM)
> > +                   && (data[1] == PON_REASON_MAGIC_NUM)
> > +                   && (data[1] == PON_REASON_MAGIC_NUM)) {
> 
> Unnecessary inner (), and I don't see the point of checking data[1]
> twice.
Yeah, a typo happened.
> 
> > +                       dev_info(dev, "Watchdog reset cause
> > detected.\n");
> 
> Unnecessary noise.
ACK,rename dev_info to dev_dbg.
> 
> > +                       wdd->bootstatus |= WDIOF_CARDRESET;
> > +               }
> > +               memset(vaddr, 0, reserved_mem_size);
> > +               memunmap(vaddr);
> > +       }
> 
> And some random data in the property is acceptable ? That is odd,
> especially
> after mandating the property itself.
> 
Yeah, do you have any suggestions about how to store the watchdog
reset cause?

Best regards,
Li Hua Qian
> > +
> >         watchdog_init_timeout(wdd, heartbeat, dev);
> >   
> >         ret = watchdog_register_device(wdd);
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Guenter Roeck]

On 7/11/23 21:03, Li, Hua Qian wrote:
> On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
>> On 7/11/23 02:17, huaqian.li@siemens.com wrote:
>>> From: Li Hua Qian <huaqian.li@siemens.com>
>>>
>>> This patch adds the WDIOF_CARDRESET support for the platform
>>> watchdog
>>> whose hardware does not support this feature, to know if the board
>>> reboot is due to a watchdog reset.
>>>
>>> This is done via reserved memory(RAM), which indicates if specific
>>> info saved, triggering the watchdog reset in last boot.
>>>
>>> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
>>> ---
>>>    drivers/watchdog/rti_wdt.c | 48
>>> ++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/rti_wdt.c
>>> b/drivers/watchdog/rti_wdt.c
>>> index ce8f18e93aa9..77fd6b54137c 100644
>>> --- a/drivers/watchdog/rti_wdt.c
>>> +++ b/drivers/watchdog/rti_wdt.c
>>> @@ -18,6 +18,7 @@
>>>    #include <linux/pm_runtime.h>
>>>    #include <linux/types.h>
>>>    #include <linux/watchdog.h>
>>> +#include <linux/of.h>
>>>    
>>>    #define DEFAULT_HEARTBEAT 60
>>>    
>>> @@ -52,6 +53,11 @@
>>>    
>>>    #define DWDST                 BIT(1)
>>>    
>>> +#define PON_REASON_SOF_NUM     0xBBBBCCCC
>>> +#define PON_REASON_MAGIC_NUM   0xDDDDDDDD
>>> +#define PON_REASON_EOF_NUM     0xCCCCBBBB
>>> +#define PON_REASON_ITEM_BITS   0xFFFFFFFF
>>> +
>>>    static int heartbeat = DEFAULT_HEARTBEAT;
>>>    
>>>    /*
>>> @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>>          struct rti_wdt_device *wdt;
>>>          struct clk *clk;
>>>          u32 last_ping = 0;
>>> +       u32 reserved_mem_size;
>>> +       unsigned long *vaddr;
>>> +       unsigned long paddr;
>>> +       u32 data[3];
>>> +       u32 reg[8];
>>>    
>>>          wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>>          if (!wdt)
>>> @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>>                  }
>>>          }
>>>    
>>> +       ret = of_property_read_variable_u32_array(pdev-
>>>> dev.of_node, "reg", reg,
>>> +                                        0, ARRAY_SIZE(reg));
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "cannot read the reg info.\n");
>>> +               goto err_iomap;
>>> +       }
>>
>> This aborts if the property does not exist, which is unacceptable.
>> Any such addition must be optional.
> Agree, refactor it.
>>
>>> +
>>> +       /*
>>> +        * If reserved memory is defined for watchdog reset cause.
>>> +        * Readout the Power-on(PON) reason and pass to bootstatus.
>>> +        */
>>> +       if (ret == 8) {
>>> +               paddr = reg[5];
>>> +               reserved_mem_size = reg[7];
>>
>> It seems odd that reserved_mem_size is not checked,
> ACK
>> and that it is even provided
>> given that it needs to be (at least) 24 bytes, and any other value
>> does not really
>> make sense.
>>
> I was thinkg to add the reliability, but it seems to be unnecessary and
> pointless. Were you suggesting that 8 bytes are enough?
> 

No.
>>> MEMREMAP_WB);
>>> +               if (vaddr == NULL) {
>>> +                       dev_err(dev, "Failed to map memory-
>>> region.\n");
>>> +                       goto err_iomap;
>>
>> This returns 8, which would be an odd error return.
>>
> ACK,refactor it.
>>> +               }
>>> +
>>> +               data[0] = *vaddr & PON_REASON_ITEM_BITS;
>>> +               data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
>>> +               data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
>>> +
>>
>> The & seems pointless / wasteful. Why ignore the upper 32 bits of
>> each location ?
>> Either make it u32 or make it u64 and use the entire 64 bit. Besides,
>> vaddr[0..2] would make the code much easier to read.
>>
> ACK, refactor it.
>>> +               dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof
>>> = %lX\n",
>>> +                       data[0], data[1], data[2]);
>>> +
>>> +               if ((data[0] == PON_REASON_SOF_NUM)
>>> +                   && (data[1] == PON_REASON_MAGIC_NUM)
>>> +                   && (data[1] == PON_REASON_MAGIC_NUM)) {
>>
>> Unnecessary inner (), and I don't see the point of checking data[1]
>> twice.
> Yeah, a typo happened.
>>
>>> +                       dev_info(dev, "Watchdog reset cause
>>> detected.\n");
>>
>> Unnecessary noise.
> ACK,rename dev_info to dev_dbg.
>>
>>> +                       wdd->bootstatus |= WDIOF_CARDRESET;
>>> +               }
>>> +               memset(vaddr, 0, reserved_mem_size);
>>> +               memunmap(vaddr);
>>> +       }
>>
>> And some random data in the property is acceptable ? That is odd,
>> especially
>> after mandating the property itself.
>>
> Yeah, do you have any suggestions about how to store the watchdog
> reset cause?
> 

No, and that is not the point I was trying to make. Your code actively
aborts probe if the "reg" property does not exist at all, but then it
silently ignores if it contains a random number of elements (other
than 8). For example, something like
		reg = <0x1234 0x5678>
is silently ignored. If anything, _that_ should return -EINVAL
because it is obviously wrong.

On a higher level, the entire code puzzles me. Obviously there
must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
into some memory area. That means the BIOS/ROMMON must be able
to detect that situation. Why not use the same code to detect this
in the driver without the complexity of passing it from BIOS/ROMMON
to driver in some random memory area ?

> Best regards,
> Li Hua Qian
>>> +
>>>          watchdog_init_timeout(wdd, heartbeat, dev);
>>>    
>>>          ret = watchdog_register_device(wdd);
>>
> 

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Guenter Roeck]

On 7/11/23 21:03, Li, Hua Qian wrote:
> On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
>> On 7/11/23 02:17, huaqian.li@siemens.com wrote:
>>> From: Li Hua Qian <huaqian.li@siemens.com>
>>>
>>> This patch adds the WDIOF_CARDRESET support for the platform
>>> watchdog
>>> whose hardware does not support this feature, to know if the board
>>> reboot is due to a watchdog reset.
>>>
>>> This is done via reserved memory(RAM), which indicates if specific
>>> info saved, triggering the watchdog reset in last boot.
>>>
>>> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
>>> ---
>>>    drivers/watchdog/rti_wdt.c | 48
>>> ++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/rti_wdt.c
>>> b/drivers/watchdog/rti_wdt.c
>>> index ce8f18e93aa9..77fd6b54137c 100644
>>> --- a/drivers/watchdog/rti_wdt.c
>>> +++ b/drivers/watchdog/rti_wdt.c
>>> @@ -18,6 +18,7 @@
>>>    #include <linux/pm_runtime.h>
>>>    #include <linux/types.h>
>>>    #include <linux/watchdog.h>
>>> +#include <linux/of.h>
>>>    
>>>    #define DEFAULT_HEARTBEAT 60
>>>    
>>> @@ -52,6 +53,11 @@
>>>    
>>>    #define DWDST                 BIT(1)
>>>    
>>> +#define PON_REASON_SOF_NUM     0xBBBBCCCC
>>> +#define PON_REASON_MAGIC_NUM   0xDDDDDDDD
>>> +#define PON_REASON_EOF_NUM     0xCCCCBBBB
>>> +#define PON_REASON_ITEM_BITS   0xFFFFFFFF
>>> +
>>>    static int heartbeat = DEFAULT_HEARTBEAT;
>>>    
>>>    /*
>>> @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>>          struct rti_wdt_device *wdt;
>>>          struct clk *clk;
>>>          u32 last_ping = 0;
>>> +       u32 reserved_mem_size;
>>> +       unsigned long *vaddr;
>>> +       unsigned long paddr;
>>> +       u32 data[3];
>>> +       u32 reg[8];
>>>    
>>>          wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>>          if (!wdt)
>>> @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
>>> platform_device *pdev)
>>>                  }
>>>          }
>>>    
>>> +       ret = of_property_read_variable_u32_array(pdev-
>>>> dev.of_node, "reg", reg,
>>> +                                        0, ARRAY_SIZE(reg));
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "cannot read the reg info.\n");
>>> +               goto err_iomap;
>>> +       }
>>
>> This aborts if the property does not exist, which is unacceptable.
>> Any such addition must be optional.
> Agree, refactor it.
>>
>>> +
>>> +       /*
>>> +        * If reserved memory is defined for watchdog reset cause.
>>> +        * Readout the Power-on(PON) reason and pass to bootstatus.
>>> +        */
>>> +       if (ret == 8) {
>>> +               paddr = reg[5];
>>> +               reserved_mem_size = reg[7];
>>
>> It seems odd that reserved_mem_size is not checked,
> ACK
>> and that it is even provided
>> given that it needs to be (at least) 24 bytes, and any other value
>> does not really
>> make sense.
>>
> I was thinkg to add the reliability, but it seems to be unnecessary and
> pointless. Were you suggesting that 8 bytes are enough?
> 

No.
>>> MEMREMAP_WB);
>>> +               if (vaddr == NULL) {
>>> +                       dev_err(dev, "Failed to map memory-
>>> region.\n");
>>> +                       goto err_iomap;
>>
>> This returns 8, which would be an odd error return.
>>
> ACK,refactor it.
>>> +               }
>>> +
>>> +               data[0] = *vaddr & PON_REASON_ITEM_BITS;
>>> +               data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
>>> +               data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
>>> +
>>
>> The & seems pointless / wasteful. Why ignore the upper 32 bits of
>> each location ?
>> Either make it u32 or make it u64 and use the entire 64 bit. Besides,
>> vaddr[0..2] would make the code much easier to read.
>>
> ACK, refactor it.
>>> +               dev_dbg(dev, "Watchdog: sof = %lX, data = %lX, eof
>>> = %lX\n",
>>> +                       data[0], data[1], data[2]);
>>> +
>>> +               if ((data[0] == PON_REASON_SOF_NUM)
>>> +                   && (data[1] == PON_REASON_MAGIC_NUM)
>>> +                   && (data[1] == PON_REASON_MAGIC_NUM)) {
>>
>> Unnecessary inner (), and I don't see the point of checking data[1]
>> twice.
> Yeah, a typo happened.
>>
>>> +                       dev_info(dev, "Watchdog reset cause
>>> detected.\n");
>>
>> Unnecessary noise.
> ACK,rename dev_info to dev_dbg.
>>
>>> +                       wdd->bootstatus |= WDIOF_CARDRESET;
>>> +               }
>>> +               memset(vaddr, 0, reserved_mem_size);
>>> +               memunmap(vaddr);
>>> +       }
>>
>> And some random data in the property is acceptable ? That is odd,
>> especially
>> after mandating the property itself.
>>
> Yeah, do you have any suggestions about how to store the watchdog
> reset cause?
> 

No, and that is not the point I was trying to make. Your code actively
aborts probe if the "reg" property does not exist at all, but then it
silently ignores if it contains a random number of elements (other
than 8). For example, something like
		reg = <0x1234 0x5678>
is silently ignored. If anything, _that_ should return -EINVAL
because it is obviously wrong.

On a higher level, the entire code puzzles me. Obviously there
must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
into some memory area. That means the BIOS/ROMMON must be able
to detect that situation. Why not use the same code to detect this
in the driver without the complexity of passing it from BIOS/ROMMON
to driver in some random memory area ?

> Best regards,
> Li Hua Qian
>>> +
>>>          watchdog_init_timeout(wdd, heartbeat, dev);
>>>    
>>>          ret = watchdog_register_device(wdd);
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 22:01 -0700, Guenter Roeck wrote:
> On 7/11/23 21:03, Li, Hua Qian wrote:
> > On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> > > On 7/11/23 02:17, huaqian.li@siemens.com wrote:
> > > > From: Li Hua Qian <huaqian.li@siemens.com>
> > > > 
> > > > This patch adds the WDIOF_CARDRESET support for the platform
> > > > watchdog
> > > > whose hardware does not support this feature, to know if the
> > > > board
> > > > reboot is due to a watchdog reset.
> > > > 
> > > > This is done via reserved memory(RAM), which indicates if
> > > > specific
> > > > info saved, triggering the watchdog reset in last boot.
> > > > 
> > > > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > > > ---
> > > >    drivers/watchdog/rti_wdt.c | 48
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 48 insertions(+)
> > > > 
> > > > diff --git a/drivers/watchdog/rti_wdt.c
> > > > b/drivers/watchdog/rti_wdt.c
> > > > index ce8f18e93aa9..77fd6b54137c 100644
> > > > --- a/drivers/watchdog/rti_wdt.c
> > > > +++ b/drivers/watchdog/rti_wdt.c
> > > > @@ -18,6 +18,7 @@
> > > >    #include <linux/pm_runtime.h>
> > > >    #include <linux/types.h>
> > > >    #include <linux/watchdog.h>
> > > > +#include <linux/of.h>
> > > >    
> > > >    #define DEFAULT_HEARTBEAT 60
> > > >    
> > > > @@ -52,6 +53,11 @@
> > > >    
> > > >    #define DWDST                 BIT(1)
> > > >    
> > > > +#define PON_REASON_SOF_NUM     0xBBBBCCCC
> > > > +#define PON_REASON_MAGIC_NUM   0xDDDDDDDD
> > > > +#define PON_REASON_EOF_NUM     0xCCCCBBBB
> > > > +#define PON_REASON_ITEM_BITS   0xFFFFFFFF
> > > > +
> > > >    static int heartbeat = DEFAULT_HEARTBEAT;
> > > >    
> > > >    /*
> > > > @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
> > > > platform_device *pdev)
> > > >          struct rti_wdt_device *wdt;
> > > >          struct clk *clk;
> > > >          u32 last_ping = 0;
> > > > +       u32 reserved_mem_size;
> > > > +       unsigned long *vaddr;
> > > > +       unsigned long paddr;
> > > > +       u32 data[3];
> > > > +       u32 reg[8];
> > > >    
> > > >          wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> > > >          if (!wdt)
> > > > @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
> > > > platform_device *pdev)
> > > >                  }
> > > >          }
> > > >    
> > > > +       ret = of_property_read_variable_u32_array(pdev-
> > > > > dev.of_node, "reg", reg,
> > > > +                                        0, ARRAY_SIZE(reg));
> > > > +       if (ret < 0) {
> > > > +               dev_err(dev, "cannot read the reg info.\n");
> > > > +               goto err_iomap;
> > > > +       }
> > > 
> > > This aborts if the property does not exist, which is
> > > unacceptable.
> > > Any such addition must be optional.
> > Agree, refactor it.
> > > 
> > > > +
> > > > +       /*
> > > > +        * If reserved memory is defined for watchdog reset
> > > > cause.
> > > > +        * Readout the Power-on(PON) reason and pass to
> > > > bootstatus.
> > > > +        */
> > > > +       if (ret == 8) {
> > > > +               paddr = reg[5];
> > > > +               reserved_mem_size = reg[7];
> > > 
> > > It seems odd that reserved_mem_size is not checked,
> > ACK
> > > and that it is even provided
> > > given that it needs to be (at least) 24 bytes, and any other
> > > value
> > > does not really
> > > make sense.
> > > 
> > I was thinkg to add the reliability, but it seems to be unnecessary
> > and
> > pointless. Were you suggesting that 8 bytes are enough?
> > 
> 
> No.
I guess I misunderstood you. Here you was suggesting that
reserved_mem_size should be checked and make sure the value to be at
least 24 bytes. Am I understanding you correctly?
> > > > MEMREMAP_WB);
> > > > +               if (vaddr == NULL) {
> > > > +                       dev_err(dev, "Failed to map memory-
> > > > region.\n");
> > > > +                       goto err_iomap;
> > > 
> > > This returns 8, which would be an odd error return.
> > > 
> > ACK,refactor it.
> > > > +               }
> > > > +
> > > > +               data[0] = *vaddr & PON_REASON_ITEM_BITS;
> > > > +               data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> > > > +               data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> > > > +
> > > 
> > > The & seems pointless / wasteful. Why ignore the upper 32 bits of
> > > each location ?
> > > Either make it u32 or make it u64 and use the entire 64 bit.
> > > Besides,
> > > vaddr[0..2] would make the code much easier to read.
> > > 
> > ACK, refactor it.
> > > > +               dev_dbg(dev, "Watchdog: sof = %lX, data = %lX,
> > > > eof
> > > > = %lX\n",
> > > > +                       data[0], data[1], data[2]);
> > > > +
> > > > +               if ((data[0] == PON_REASON_SOF_NUM)
> > > > +                   && (data[1] == PON_REASON_MAGIC_NUM)
> > > > +                   && (data[1] == PON_REASON_MAGIC_NUM)) {
> > > 
> > > Unnecessary inner (), and I don't see the point of checking
> > > data[1]
> > > twice.
> > Yeah, a typo happened.
> > > 
> > > > +                       dev_info(dev, "Watchdog reset cause
> > > > detected.\n");
> > > 
> > > Unnecessary noise.
> > ACK,rename dev_info to dev_dbg.
> > > 
> > > > +                       wdd->bootstatus |= WDIOF_CARDRESET;
> > > > +               }
> > > > +               memset(vaddr, 0, reserved_mem_size);
> > > > +               memunmap(vaddr);
> > > > +       }
> > > 
> > > And some random data in the property is acceptable ? That is odd,
> > > especially
> > > after mandating the property itself.
> > > 
> > Yeah, do you have any suggestions about how to store the watchdog
> > reset cause?
> > 
> 
> No, and that is not the point I was trying to make. Your code
> actively
> aborts probe if the "reg" property does not exist at all, but then it
> silently ignores if it contains a random number of elements (other
> than 8). For example, something like
>                 reg = <0x1234 0x5678>
> is silently ignored. If anything, _that_ should return -EINVAL
> because it is obviously wrong.
Now I get it, many thanks for pointing out.
> 
> On a higher level, the entire code puzzles me. Obviously there
> must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
> into some memory area. That means the BIOS/ROMMON must be able
> to detect that situation. Why not use the same code to detect this
> in the driver without the complexity of passing it from BIOS/ROMMON
> to driver in some random memory area ?
> 
In TI AM65x cases, the hardware is not capable of issuing a reset on
its own, and is not possible to record or detect the watchdog reset
situation. So `k3-rit-wdt` firmware which runs in a mcu core was used
to detect the watchdog interrupt and reset the Soc. Here I am trying to
write the reason in this firmware when watchdog interrupt happens, and
read it out in kernel.

> > Best regards,
> > Li Hua Qian
> > > > +
> > > >          watchdog_init_timeout(wdd, heartbeat, dev);
> > > >    
> > > >          ret = watchdog_register_device(wdd);
> > > 
> > 
> 

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Li, Hua Qian]

On Tue, 2023-07-11 at 22:01 -0700, Guenter Roeck wrote:
> On 7/11/23 21:03, Li, Hua Qian wrote:
> > On Tue, 2023-07-11 at 19:32 -0700, Guenter Roeck wrote:
> > > On 7/11/23 02:17, huaqian.li@siemens.com wrote:
> > > > From: Li Hua Qian <huaqian.li@siemens.com>
> > > > 
> > > > This patch adds the WDIOF_CARDRESET support for the platform
> > > > watchdog
> > > > whose hardware does not support this feature, to know if the
> > > > board
> > > > reboot is due to a watchdog reset.
> > > > 
> > > > This is done via reserved memory(RAM), which indicates if
> > > > specific
> > > > info saved, triggering the watchdog reset in last boot.
> > > > 
> > > > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > > > ---
> > > >    drivers/watchdog/rti_wdt.c | 48
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 48 insertions(+)
> > > > 
> > > > diff --git a/drivers/watchdog/rti_wdt.c
> > > > b/drivers/watchdog/rti_wdt.c
> > > > index ce8f18e93aa9..77fd6b54137c 100644
> > > > --- a/drivers/watchdog/rti_wdt.c
> > > > +++ b/drivers/watchdog/rti_wdt.c
> > > > @@ -18,6 +18,7 @@
> > > >    #include <linux/pm_runtime.h>
> > > >    #include <linux/types.h>
> > > >    #include <linux/watchdog.h>
> > > > +#include <linux/of.h>
> > > >    
> > > >    #define DEFAULT_HEARTBEAT 60
> > > >    
> > > > @@ -52,6 +53,11 @@
> > > >    
> > > >    #define DWDST                 BIT(1)
> > > >    
> > > > +#define PON_REASON_SOF_NUM     0xBBBBCCCC
> > > > +#define PON_REASON_MAGIC_NUM   0xDDDDDDDD
> > > > +#define PON_REASON_EOF_NUM     0xCCCCBBBB
> > > > +#define PON_REASON_ITEM_BITS   0xFFFFFFFF
> > > > +
> > > >    static int heartbeat = DEFAULT_HEARTBEAT;
> > > >    
> > > >    /*
> > > > @@ -198,6 +204,11 @@ static int rti_wdt_probe(struct
> > > > platform_device *pdev)
> > > >          struct rti_wdt_device *wdt;
> > > >          struct clk *clk;
> > > >          u32 last_ping = 0;
> > > > +       u32 reserved_mem_size;
> > > > +       unsigned long *vaddr;
> > > > +       unsigned long paddr;
> > > > +       u32 data[3];
> > > > +       u32 reg[8];
> > > >    
> > > >          wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> > > >          if (!wdt)
> > > > @@ -284,6 +295,43 @@ static int rti_wdt_probe(struct
> > > > platform_device *pdev)
> > > >                  }
> > > >          }
> > > >    
> > > > +       ret = of_property_read_variable_u32_array(pdev-
> > > > > dev.of_node, "reg", reg,
> > > > +                                        0, ARRAY_SIZE(reg));
> > > > +       if (ret < 0) {
> > > > +               dev_err(dev, "cannot read the reg info.\n");
> > > > +               goto err_iomap;
> > > > +       }
> > > 
> > > This aborts if the property does not exist, which is
> > > unacceptable.
> > > Any such addition must be optional.
> > Agree, refactor it.
> > > 
> > > > +
> > > > +       /*
> > > > +        * If reserved memory is defined for watchdog reset
> > > > cause.
> > > > +        * Readout the Power-on(PON) reason and pass to
> > > > bootstatus.
> > > > +        */
> > > > +       if (ret == 8) {
> > > > +               paddr = reg[5];
> > > > +               reserved_mem_size = reg[7];
> > > 
> > > It seems odd that reserved_mem_size is not checked,
> > ACK
> > > and that it is even provided
> > > given that it needs to be (at least) 24 bytes, and any other
> > > value
> > > does not really
> > > make sense.
> > > 
> > I was thinkg to add the reliability, but it seems to be unnecessary
> > and
> > pointless. Were you suggesting that 8 bytes are enough?
> > 
> 
> No.
I guess I misunderstood you. Here you was suggesting that
reserved_mem_size should be checked and make sure the value to be at
least 24 bytes. Am I understanding you correctly?
> > > > MEMREMAP_WB);
> > > > +               if (vaddr == NULL) {
> > > > +                       dev_err(dev, "Failed to map memory-
> > > > region.\n");
> > > > +                       goto err_iomap;
> > > 
> > > This returns 8, which would be an odd error return.
> > > 
> > ACK,refactor it.
> > > > +               }
> > > > +
> > > > +               data[0] = *vaddr & PON_REASON_ITEM_BITS;
> > > > +               data[1] = *(vaddr + 1) & PON_REASON_ITEM_BITS;
> > > > +               data[2] = *(vaddr + 2) & PON_REASON_ITEM_BITS;
> > > > +
> > > 
> > > The & seems pointless / wasteful. Why ignore the upper 32 bits of
> > > each location ?
> > > Either make it u32 or make it u64 and use the entire 64 bit.
> > > Besides,
> > > vaddr[0..2] would make the code much easier to read.
> > > 
> > ACK, refactor it.
> > > > +               dev_dbg(dev, "Watchdog: sof = %lX, data = %lX,
> > > > eof
> > > > = %lX\n",
> > > > +                       data[0], data[1], data[2]);
> > > > +
> > > > +               if ((data[0] == PON_REASON_SOF_NUM)
> > > > +                   && (data[1] == PON_REASON_MAGIC_NUM)
> > > > +                   && (data[1] == PON_REASON_MAGIC_NUM)) {
> > > 
> > > Unnecessary inner (), and I don't see the point of checking
> > > data[1]
> > > twice.
> > Yeah, a typo happened.
> > > 
> > > > +                       dev_info(dev, "Watchdog reset cause
> > > > detected.\n");
> > > 
> > > Unnecessary noise.
> > ACK,rename dev_info to dev_dbg.
> > > 
> > > > +                       wdd->bootstatus |= WDIOF_CARDRESET;
> > > > +               }
> > > > +               memset(vaddr, 0, reserved_mem_size);
> > > > +               memunmap(vaddr);
> > > > +       }
> > > 
> > > And some random data in the property is acceptable ? That is odd,
> > > especially
> > > after mandating the property itself.
> > > 
> > Yeah, do you have any suggestions about how to store the watchdog
> > reset cause?
> > 
> 
> No, and that is not the point I was trying to make. Your code
> actively
> aborts probe if the "reg" property does not exist at all, but then it
> silently ignores if it contains a random number of elements (other
> than 8). For example, something like
>                 reg = <0x1234 0x5678>
> is silently ignored. If anything, _that_ should return -EINVAL
> because it is obviously wrong.
Now I get it, many thanks for pointing out.
> 
> On a higher level, the entire code puzzles me. Obviously there
> must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
> into some memory area. That means the BIOS/ROMMON must be able
> to detect that situation. Why not use the same code to detect this
> in the driver without the complexity of passing it from BIOS/ROMMON
> to driver in some random memory area ?
> 
In TI AM65x cases, the hardware is not capable of issuing a reset on
its own, and is not possible to record or detect the watchdog reset
situation. So `k3-rit-wdt` firmware which runs in a mcu core was used
to detect the watchdog interrupt and reset the Soc. Here I am trying to
write the reason in this firmware when watchdog interrupt happens, and
read it out in kernel.

> > Best regards,
> > Li Hua Qian
> > > > +
> > > >          watchdog_init_timeout(wdd, heartbeat, dev);
> > > >    
> > > >          ret = watchdog_register_device(wdd);
> > > 
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Jan Kiszka]

On 12.07.23 08:05, Li, Hua Qian (DI FA CTR IPC CN PRC4) wrote:
> On Tue, 2023-07-11 at 22:01 -0700, Guenter Roeck wrote:
>> On a higher level, the entire code puzzles me. Obviously there
>> must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
>> into some memory area. That means the BIOS/ROMMON must be able
>> to detect that situation. Why not use the same code to detect this
>> in the driver without the complexity of passing it from BIOS/ROMMON
>> to driver in some random memory area ?
>>
> In TI AM65x cases, the hardware is not capable of issuing a reset on
> its own, and is not possible to record or detect the watchdog reset
> situation. So `k3-rit-wdt` firmware which runs in a mcu core was used
> to detect the watchdog interrupt and reset the Soc. Here I am trying to
> write the reason in this firmware when watchdog interrupt happens, and
> read it out in kernel.

See https://github.com/siemens/k3-rti-wdt/issues/1 for where this
started and where the firmware part comes into play.

I still wonder why we couldn't have had a nicer hardware watchdog
instead but this can't be changed anymore.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] watchdog:rit_wdt: Add support for WDIOF_CARDRESET

[Jan Kiszka]

On 12.07.23 08:05, Li, Hua Qian (DI FA CTR IPC CN PRC4) wrote:
> On Tue, 2023-07-11 at 22:01 -0700, Guenter Roeck wrote:
>> On a higher level, the entire code puzzles me. Obviously there
>> must be a means for the BIOS or ROMMON to write PON_REASON_SOF_NUM
>> into some memory area. That means the BIOS/ROMMON must be able
>> to detect that situation. Why not use the same code to detect this
>> in the driver without the complexity of passing it from BIOS/ROMMON
>> to driver in some random memory area ?
>>
> In TI AM65x cases, the hardware is not capable of issuing a reset on
> its own, and is not possible to record or detect the watchdog reset
> situation. So `k3-rit-wdt` firmware which runs in a mcu core was used
> to detect the watchdog interrupt and reset the Soc. Here I am trying to
> write the reason in this firmware when watchdog interrupt happens, and
> read it out in kernel.

See https://github.com/siemens/k3-rti-wdt/issues/1 for where this
started and where the firmware part comes into play.

I still wonder why we couldn't have had a nicer hardware watchdog
instead but this can't be changed anymore.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux