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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/11/23 02:17, huaqian.li@xxxxxxxxxxx wrote:
From: Li Hua Qian <huaqian.li@xxxxxxxxxxx>

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@xxxxxxxxxxx>
---
  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);




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux