[PATCH 1/2] dt/bindings: Add bindings for PIC32 deadman timer peripheral

[PATCH 1/2] dt/bindings: Add bindings for PIC32 deadman timer peripheral

[Joshua Henderson]

From: Purna Chandra Mandal <purna.mandal@microchip.com>

Document the devicetree bindings for the deadman timer peripheral found on
Microchip PIC32 SoC class devices.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: <linux-mips@linux-mips.org>
---
Note: Please merge this patch series through the MIPS tree.
---
 .../bindings/watchdog/microchip,pic32-dmt.txt      |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
new file mode 100644
index 0000000..f7374ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
@@ -0,0 +1,19 @@
+* Microchip PIC32 Deadman Timer
+
+The deadman timer is used to reset the processor in the event of a software
+malfunction. It is a free-running instruction fetch timer, which is clocked
+whenever an instruction fetch occurs until a count match occurs.
+
+Required properties:
+- compatible: must be "microchip,pic32mzda-dmt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- clocks: phandle of parent clock (should be &PBCLK7).
+
+Example:
+
+	watchdog2: dmt@1f800a00 {
+		compatible = "microchip,pic32mzda-dmt";
+		reg = <0x1f800a00 0x80>;
+		clocks = <&PBCLK7>;
+	};
--
1.7.9.5

[PATCH 1/2] dt/bindings: Add bindings for PIC32 deadman timer peripheral

[Joshua Henderson]

From: Purna Chandra Mandal <purna.mandal-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>

Document the devicetree bindings for the deadman timer peripheral found on
Microchip PIC32 SoC class devices.

Signed-off-by: Purna Chandra Mandal <purna.mandal-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
Signed-off-by: Joshua Henderson <joshua.henderson-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
Cc: <linux-mips-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
---
Note: Please merge this patch series through the MIPS tree.
---
 .../bindings/watchdog/microchip,pic32-dmt.txt      |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
new file mode 100644
index 0000000..f7374ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
@@ -0,0 +1,19 @@
+* Microchip PIC32 Deadman Timer
+
+The deadman timer is used to reset the processor in the event of a software
+malfunction. It is a free-running instruction fetch timer, which is clocked
+whenever an instruction fetch occurs until a count match occurs.
+
+Required properties:
+- compatible: must be "microchip,pic32mzda-dmt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- clocks: phandle of parent clock (should be &PBCLK7).
+
+Example:
+
+	watchdog2: dmt@1f800a00 {
+		compatible = "microchip,pic32mzda-dmt";
+		reg = <0x1f800a00 0x80>;
+		clocks = <&PBCLK7>;
+	};
--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] dt/bindings: Add bindings for PIC32 deadman timer peripheral

[Joshua Henderson]

From: Purna Chandra Mandal <purna.mandal@microchip.com>

Document the devicetree bindings for the deadman timer peripheral found on
Microchip PIC32 SoC class devices.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: <linux-mips@linux-mips.org>
---
Note: Please merge this patch series through the MIPS tree.
---
 .../bindings/watchdog/microchip,pic32-dmt.txt      |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
new file mode 100644
index 0000000..f7374ee
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
@@ -0,0 +1,19 @@
+* Microchip PIC32 Deadman Timer
+
+The deadman timer is used to reset the processor in the event of a software
+malfunction. It is a free-running instruction fetch timer, which is clocked
+whenever an instruction fetch occurs until a count match occurs.
+
+Required properties:
+- compatible: must be "microchip,pic32mzda-dmt".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- clocks: phandle of parent clock (should be &PBCLK7).
+
+Example:
+
+	watchdog2: dmt@1f800a00 {
+		compatible = "microchip,pic32mzda-dmt";
+		reg = <0x1f800a00 0x80>;
+		clocks = <&PBCLK7>;
+	};
--
1.7.9.5

[PATCH 2/2] watchdog: pic32-dmt: Add PIC32 deadman timer driver support

[Joshua Henderson]

From: Purna Chandra Mandal <purna.mandal@microchip.com>

The primary function of the deadman timer (DMT) is to reset the processor
in the event of a software malfunction. The DMT is a free-running
instruction fetch timer, which is clocked whenever an instruction fetch
occurs until a count match occurs. Instructions are not fetched when
the processor is in sleep mode.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: <linux-mips@linux-mips.org>
---
Note: Please merge this patch series through the MIPS tree.
---
 drivers/watchdog/Kconfig     |   14 ++
 drivers/watchdog/Makefile    |    1 +
 drivers/watchdog/pic32-dmt.c |  296 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 drivers/watchdog/pic32-dmt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 131abc2..87985c6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1424,6 +1424,20 @@ config PIC32_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called pic32-wdt.

+config PIC32_DMT
+	tristate "Microchip PIC32 Deadman Timer"
+	select WATCHDOG_CORE
+	depends on MACH_PIC32
+	default y
+	help
+	  Watchdog driver for PIC32 instruction fetch counting timer. This specific
+	  timer is typically be used in misson critical and safety critical
+	  applications, where any single failure of the software functionality
+	  and sequencing must be detected.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called pic32-dmt.
+
 # PARISC Architecture

 # POWERPC Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 244ed80..d051c9c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -154,6 +154,7 @@ obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
 obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
 obj-$(CONFIG_MT7621_WDT) += mt7621_wdt.o
 obj-$(CONFIG_PIC32_WDT) += pic32-wdt.o
+obj-$(CONFIG_PIC32_DMT) += pic32-dmt.o

 # PARISC Architecture

diff --git a/drivers/watchdog/pic32-dmt.c b/drivers/watchdog/pic32-dmt.c
new file mode 100644
index 0000000..13dd1e3
--- /dev/null
+++ b/drivers/watchdog/pic32-dmt.c
@@ -0,0 +1,296 @@
+/*
+ * PIC32 deadman timer driver
+ *
+ * Purna Chandra Mandal <purna.mandal@microchip.com>
+ * Copyright (c) 2016, Microchip Technology Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pm.h>
+#include <linux/watchdog.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#include <asm/mach-pic32/pic32.h>
+
+/* Deadman Timer Regs */
+#define DMTCON_REG	0x00
+#define DMTPRECLR_REG	0x10
+#define DMTCLR_REG	0x20
+#define DMTSTAT_REG	0x30
+#define DMTCNT_REG	0x40
+#define DMTPSCNT_REG	0x60
+#define DMTPSINTV_REG	0x70
+
+/* Deadman Timer Regs fields */
+#define DMT_ON			0x8000
+#define DMT_STEP1_KEY		0x40
+#define DMT_STEP1_KEY_BYTE	1
+#define DMT_STEP2_KEY		0x08
+#define DMTSTAT_WINOPN		0x01
+#define DMTSTAT_EVENT		0x20
+#define DMTSTAT_BAD2		0x40
+#define DMTSTAT_BAD1		0x80
+
+/* Reset Control Register fields for watchdog */
+#define RESETCON_DMT_TIMEOUT    0x0020
+
+struct pic32_dmt {
+	spinlock_t	lock;
+	void __iomem	*regs;
+	struct clk	*clk;
+};
+
+static inline int dmt_is_enabled(struct pic32_dmt *dmt)
+{
+	return readl(dmt->regs + DMTCON_REG) & DMT_ON;
+}
+
+static inline void dmt_enable(struct pic32_dmt *dmt)
+{
+	writel(DMT_ON, PIC32_SET(dmt->regs + DMTCON_REG));
+}
+
+static inline void dmt_disable(struct pic32_dmt *dmt)
+{
+	writel(DMT_ON, PIC32_CLR(dmt->regs + DMTCON_REG));
+	cpu_relax();
+}
+
+static inline int dmt_bad_status(struct pic32_dmt *dmt)
+{
+	u32 val;
+
+	val = readl(dmt->regs + DMTSTAT_REG);
+	val &= (DMTSTAT_BAD1 | DMTSTAT_BAD2 | DMTSTAT_EVENT);
+	if (val)
+		pr_err("dmt: bad event generated: sts %08x\n", val);
+
+	return val;
+}
+
+static inline int dmt_keepalive(struct pic32_dmt *dmt)
+{
+	u32 v;
+
+	/* set pre-clear key */
+	writel(DMT_STEP1_KEY << 8, dmt->regs + DMTPRECLR_REG);
+
+	/* wait for DMT window to open */
+	for (;;) {
+		v = readl(dmt->regs + DMTSTAT_REG) & DMTSTAT_WINOPN;
+		if (v == DMTSTAT_WINOPN)
+			break;
+	}
+
+	/* apply key2 */
+	writel(DMT_STEP2_KEY, dmt->regs + DMTCLR_REG);
+
+	/* check whether keys are latched correctly */
+	return dmt_bad_status(dmt);
+}
+
+static inline u32 dmt_timeleft(struct pic32_dmt *dmt)
+{
+	u32 top = readl(dmt->regs + DMTPSCNT_REG);
+
+	return top - readl(dmt->regs + DMTCNT_REG);
+}
+
+static inline u32 dmt_interval_time_to_clear(struct pic32_dmt *dmt)
+{
+	return readl(dmt->regs + DMTPSINTV_REG);
+}
+
+static inline u32 pic32_dmt_get_timeout_secs(struct pic32_dmt *dmt)
+{
+	return readl(dmt->regs + DMTPSCNT_REG) / clk_get_rate(dmt->clk);
+}
+
+static inline u32 pic32_dmt_bootstatus(struct pic32_dmt *dmt)
+{
+	u32 v;
+	void __iomem *rst_base;
+
+	rst_base = ioremap(PIC32_BASE_RESET, 0x10);
+	if (!rst_base)
+		return 0;
+
+	v = readl(rst_base);
+
+	writel(RESETCON_DMT_TIMEOUT, PIC32_CLR(rst_base));
+
+	iounmap(rst_base);
+	return v & RESETCON_DMT_TIMEOUT;
+}
+
+static int pic32_dmt_start(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	spin_lock(&dmt->lock);
+	if (dmt_is_enabled(dmt))
+		goto done;
+
+	dmt_enable(dmt);
+done:
+	dmt_keepalive(dmt);
+	spin_unlock(&dmt->lock);
+
+	return 0;
+}
+
+static int pic32_dmt_stop(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	spin_lock(&dmt->lock);
+	dmt_disable(dmt);
+	spin_unlock(&dmt->lock);
+
+	return 0;
+}
+
+static int pic32_dmt_ping(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+	int ret;
+
+	spin_lock(&dmt->lock);
+	ret = dmt_keepalive(dmt);
+	spin_unlock(&dmt->lock);
+
+	return ret ? -EAGAIN : 0;
+}
+
+static unsigned int pic32_dmt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	return dmt_timeleft(dmt);
+}
+
+static const struct watchdog_ops pic32_dmt_fops = {
+	.owner		= THIS_MODULE,
+	.start		= pic32_dmt_start,
+	.stop		= pic32_dmt_stop,
+	.get_timeleft	= pic32_dmt_get_timeleft,
+	.ping		= pic32_dmt_ping,
+};
+
+static const struct watchdog_info pic32_dmt_ident = {
+	.options	= WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE,
+	.identity	= "PIC32 Deadman Timer",
+};
+
+static struct watchdog_device pic32_dmt_wdd = {
+	.info		= &pic32_dmt_ident,
+	.ops		= &pic32_dmt_fops,
+	.min_timeout	= 1,
+	.max_timeout	= UINT_MAX,
+};
+
+static int pic32_dmt_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct pic32_dmt *dmt;
+	struct resource *mem;
+	struct watchdog_device *wdd = &pic32_dmt_wdd;
+
+	dmt = devm_kzalloc(&pdev->dev, sizeof(*dmt), GFP_KERNEL);
+	if (IS_ERR(dmt))
+		return PTR_ERR(dmt);
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dmt->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(dmt->regs))
+		return PTR_ERR(dmt->regs);
+
+	dmt->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dmt->clk)) {
+		dev_err(&pdev->dev, "clk not found\n");
+		return PTR_ERR(dmt->clk);
+	}
+
+	ret = clk_prepare_enable(dmt->clk);
+	if (ret)
+		return ret;
+
+	wdd->max_timeout /= clk_get_rate(dmt->clk);
+	wdd->timeout = pic32_dmt_get_timeout_secs(dmt);
+	wdd->bootstatus = pic32_dmt_bootstatus(dmt) ? WDIOF_CARDRESET : 0;
+	if (!wdd->timeout) {
+		dev_err(&pdev->dev,
+			"timeout %dsec too small for DMT\n", wdd->timeout);
+		ret = -EINVAL;
+		goto out_disable_clk;
+	}
+
+	spin_lock_init(&dmt->lock);
+
+	dev_info(&pdev->dev, "max_timeout %d, min_timeout %d, cur_timeout %d\n",
+		 wdd->max_timeout, wdd->min_timeout, wdd->timeout);
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "watchdog register failed, err %d\n", ret);
+		goto out_disable_clk;
+	}
+
+	watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
+	watchdog_set_drvdata(wdd, dmt);
+
+	platform_set_drvdata(pdev, wdd);
+	return 0;
+
+out_disable_clk:
+	clk_disable_unprepare(dmt->clk);
+	return ret;
+}
+
+static int pic32_dmt_remove(struct platform_device *pdev)
+{
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	clk_disable_unprepare(dmt->clk);
+	watchdog_unregister_device(wdd);
+
+	return 0;
+}
+
+static const struct of_device_id pic32_dmt_of_ids[] = {
+	{ .compatible = "microchip,pic32mzda-dmt",},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pic32_dmt_of_ids);
+
+static struct platform_driver pic32_dmt_driver = {
+	.probe		= pic32_dmt_probe,
+	.remove		= pic32_dmt_remove,
+	.driver		= {
+		.name		= "pi32-dmt",
+		.owner		= THIS_MODULE,
+		.of_match_table = of_match_ptr(pic32_dmt_of_ids),
+	}
+};
+
+module_platform_driver(pic32_dmt_driver);
+
+MODULE_AUTHOR("Purna Chandra Mandal <purna.mandal@microchip.com>");
+MODULE_DESCRIPTION("Microchip PIC32 DMT Driver");
+MODULE_LICENSE("GPL");
--
1.7.9.5

[PATCH 2/2] watchdog: pic32-dmt: Add PIC32 deadman timer driver support

[Joshua Henderson]

From: Purna Chandra Mandal <purna.mandal@microchip.com>

The primary function of the deadman timer (DMT) is to reset the processor
in the event of a software malfunction. The DMT is a free-running
instruction fetch timer, which is clocked whenever an instruction fetch
occurs until a count match occurs. Instructions are not fetched when
the processor is in sleep mode.

Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: <linux-mips@linux-mips.org>
---
Note: Please merge this patch series through the MIPS tree.
---
 drivers/watchdog/Kconfig     |   14 ++
 drivers/watchdog/Makefile    |    1 +
 drivers/watchdog/pic32-dmt.c |  296 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 drivers/watchdog/pic32-dmt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 131abc2..87985c6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1424,6 +1424,20 @@ config PIC32_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called pic32-wdt.

+config PIC32_DMT
+	tristate "Microchip PIC32 Deadman Timer"
+	select WATCHDOG_CORE
+	depends on MACH_PIC32
+	default y
+	help
+	  Watchdog driver for PIC32 instruction fetch counting timer. This specific
+	  timer is typically be used in misson critical and safety critical
+	  applications, where any single failure of the software functionality
+	  and sequencing must be detected.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called pic32-dmt.
+
 # PARISC Architecture

 # POWERPC Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 244ed80..d051c9c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -154,6 +154,7 @@ obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
 obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
 obj-$(CONFIG_MT7621_WDT) += mt7621_wdt.o
 obj-$(CONFIG_PIC32_WDT) += pic32-wdt.o
+obj-$(CONFIG_PIC32_DMT) += pic32-dmt.o

 # PARISC Architecture

diff --git a/drivers/watchdog/pic32-dmt.c b/drivers/watchdog/pic32-dmt.c
new file mode 100644
index 0000000..13dd1e3
--- /dev/null
+++ b/drivers/watchdog/pic32-dmt.c
@@ -0,0 +1,296 @@
+/*
+ * PIC32 deadman timer driver
+ *
+ * Purna Chandra Mandal <purna.mandal@microchip.com>
+ * Copyright (c) 2016, Microchip Technology Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pm.h>
+#include <linux/watchdog.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#include <asm/mach-pic32/pic32.h>
+
+/* Deadman Timer Regs */
+#define DMTCON_REG	0x00
+#define DMTPRECLR_REG	0x10
+#define DMTCLR_REG	0x20
+#define DMTSTAT_REG	0x30
+#define DMTCNT_REG	0x40
+#define DMTPSCNT_REG	0x60
+#define DMTPSINTV_REG	0x70
+
+/* Deadman Timer Regs fields */
+#define DMT_ON			0x8000
+#define DMT_STEP1_KEY		0x40
+#define DMT_STEP1_KEY_BYTE	1
+#define DMT_STEP2_KEY		0x08
+#define DMTSTAT_WINOPN		0x01
+#define DMTSTAT_EVENT		0x20
+#define DMTSTAT_BAD2		0x40
+#define DMTSTAT_BAD1		0x80
+
+/* Reset Control Register fields for watchdog */
+#define RESETCON_DMT_TIMEOUT    0x0020
+
+struct pic32_dmt {
+	spinlock_t	lock;
+	void __iomem	*regs;
+	struct clk	*clk;
+};
+
+static inline int dmt_is_enabled(struct pic32_dmt *dmt)
+{
+	return readl(dmt->regs + DMTCON_REG) & DMT_ON;
+}
+
+static inline void dmt_enable(struct pic32_dmt *dmt)
+{
+	writel(DMT_ON, PIC32_SET(dmt->regs + DMTCON_REG));
+}
+
+static inline void dmt_disable(struct pic32_dmt *dmt)
+{
+	writel(DMT_ON, PIC32_CLR(dmt->regs + DMTCON_REG));
+	cpu_relax();
+}
+
+static inline int dmt_bad_status(struct pic32_dmt *dmt)
+{
+	u32 val;
+
+	val = readl(dmt->regs + DMTSTAT_REG);
+	val &= (DMTSTAT_BAD1 | DMTSTAT_BAD2 | DMTSTAT_EVENT);
+	if (val)
+		pr_err("dmt: bad event generated: sts %08x\n", val);
+
+	return val;
+}
+
+static inline int dmt_keepalive(struct pic32_dmt *dmt)
+{
+	u32 v;
+
+	/* set pre-clear key */
+	writel(DMT_STEP1_KEY << 8, dmt->regs + DMTPRECLR_REG);
+
+	/* wait for DMT window to open */
+	for (;;) {
+		v = readl(dmt->regs + DMTSTAT_REG) & DMTSTAT_WINOPN;
+		if (v == DMTSTAT_WINOPN)
+			break;
+	}
+
+	/* apply key2 */
+	writel(DMT_STEP2_KEY, dmt->regs + DMTCLR_REG);
+
+	/* check whether keys are latched correctly */
+	return dmt_bad_status(dmt);
+}
+
+static inline u32 dmt_timeleft(struct pic32_dmt *dmt)
+{
+	u32 top = readl(dmt->regs + DMTPSCNT_REG);
+
+	return top - readl(dmt->regs + DMTCNT_REG);
+}
+
+static inline u32 dmt_interval_time_to_clear(struct pic32_dmt *dmt)
+{
+	return readl(dmt->regs + DMTPSINTV_REG);
+}
+
+static inline u32 pic32_dmt_get_timeout_secs(struct pic32_dmt *dmt)
+{
+	return readl(dmt->regs + DMTPSCNT_REG) / clk_get_rate(dmt->clk);
+}
+
+static inline u32 pic32_dmt_bootstatus(struct pic32_dmt *dmt)
+{
+	u32 v;
+	void __iomem *rst_base;
+
+	rst_base = ioremap(PIC32_BASE_RESET, 0x10);
+	if (!rst_base)
+		return 0;
+
+	v = readl(rst_base);
+
+	writel(RESETCON_DMT_TIMEOUT, PIC32_CLR(rst_base));
+
+	iounmap(rst_base);
+	return v & RESETCON_DMT_TIMEOUT;
+}
+
+static int pic32_dmt_start(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	spin_lock(&dmt->lock);
+	if (dmt_is_enabled(dmt))
+		goto done;
+
+	dmt_enable(dmt);
+done:
+	dmt_keepalive(dmt);
+	spin_unlock(&dmt->lock);
+
+	return 0;
+}
+
+static int pic32_dmt_stop(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	spin_lock(&dmt->lock);
+	dmt_disable(dmt);
+	spin_unlock(&dmt->lock);
+
+	return 0;
+}
+
+static int pic32_dmt_ping(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+	int ret;
+
+	spin_lock(&dmt->lock);
+	ret = dmt_keepalive(dmt);
+	spin_unlock(&dmt->lock);
+
+	return ret ? -EAGAIN : 0;
+}
+
+static unsigned int pic32_dmt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	return dmt_timeleft(dmt);
+}
+
+static const struct watchdog_ops pic32_dmt_fops = {
+	.owner		= THIS_MODULE,
+	.start		= pic32_dmt_start,
+	.stop		= pic32_dmt_stop,
+	.get_timeleft	= pic32_dmt_get_timeleft,
+	.ping		= pic32_dmt_ping,
+};
+
+static const struct watchdog_info pic32_dmt_ident = {
+	.options	= WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE,
+	.identity	= "PIC32 Deadman Timer",
+};
+
+static struct watchdog_device pic32_dmt_wdd = {
+	.info		= &pic32_dmt_ident,
+	.ops		= &pic32_dmt_fops,
+	.min_timeout	= 1,
+	.max_timeout	= UINT_MAX,
+};
+
+static int pic32_dmt_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct pic32_dmt *dmt;
+	struct resource *mem;
+	struct watchdog_device *wdd = &pic32_dmt_wdd;
+
+	dmt = devm_kzalloc(&pdev->dev, sizeof(*dmt), GFP_KERNEL);
+	if (IS_ERR(dmt))
+		return PTR_ERR(dmt);
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dmt->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(dmt->regs))
+		return PTR_ERR(dmt->regs);
+
+	dmt->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dmt->clk)) {
+		dev_err(&pdev->dev, "clk not found\n");
+		return PTR_ERR(dmt->clk);
+	}
+
+	ret = clk_prepare_enable(dmt->clk);
+	if (ret)
+		return ret;
+
+	wdd->max_timeout /= clk_get_rate(dmt->clk);
+	wdd->timeout = pic32_dmt_get_timeout_secs(dmt);
+	wdd->bootstatus = pic32_dmt_bootstatus(dmt) ? WDIOF_CARDRESET : 0;
+	if (!wdd->timeout) {
+		dev_err(&pdev->dev,
+			"timeout %dsec too small for DMT\n", wdd->timeout);
+		ret = -EINVAL;
+		goto out_disable_clk;
+	}
+
+	spin_lock_init(&dmt->lock);
+
+	dev_info(&pdev->dev, "max_timeout %d, min_timeout %d, cur_timeout %d\n",
+		 wdd->max_timeout, wdd->min_timeout, wdd->timeout);
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "watchdog register failed, err %d\n", ret);
+		goto out_disable_clk;
+	}
+
+	watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
+	watchdog_set_drvdata(wdd, dmt);
+
+	platform_set_drvdata(pdev, wdd);
+	return 0;
+
+out_disable_clk:
+	clk_disable_unprepare(dmt->clk);
+	return ret;
+}
+
+static int pic32_dmt_remove(struct platform_device *pdev)
+{
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
+	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
+
+	clk_disable_unprepare(dmt->clk);
+	watchdog_unregister_device(wdd);
+
+	return 0;
+}
+
+static const struct of_device_id pic32_dmt_of_ids[] = {
+	{ .compatible = "microchip,pic32mzda-dmt",},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pic32_dmt_of_ids);
+
+static struct platform_driver pic32_dmt_driver = {
+	.probe		= pic32_dmt_probe,
+	.remove		= pic32_dmt_remove,
+	.driver		= {
+		.name		= "pi32-dmt",
+		.owner		= THIS_MODULE,
+		.of_match_table = of_match_ptr(pic32_dmt_of_ids),
+	}
+};
+
+module_platform_driver(pic32_dmt_driver);
+
+MODULE_AUTHOR("Purna Chandra Mandal <purna.mandal@microchip.com>");
+MODULE_DESCRIPTION("Microchip PIC32 DMT Driver");
+MODULE_LICENSE("GPL");
--
1.7.9.5

Re: [PATCH 2/2] watchdog: pic32-dmt: Add PIC32 deadman timer driver support

[Guenter Roeck]

On 02/01/2016 04:02 PM, Joshua Henderson wrote:
> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>
> The primary function of the deadman timer (DMT) is to reset the processor
> in the event of a software malfunction. The DMT is a free-running
> instruction fetch timer, which is clocked whenever an instruction fetch
> occurs until a count match occurs. Instructions are not fetched when
> the processor is in sleep mode.
>
> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: <linux-mips@linux-mips.org>

Please drop this Cc: from the commit log.

> ---
> Note: Please merge this patch series through the MIPS tree.
> ---
>   drivers/watchdog/Kconfig     |   14 ++
>   drivers/watchdog/Makefile    |    1 +
>   drivers/watchdog/pic32-dmt.c |  296 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 311 insertions(+)
>   create mode 100644 drivers/watchdog/pic32-dmt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 131abc2..87985c6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1424,6 +1424,20 @@ config PIC32_WDT
>   	  To compile this driver as a loadable module, choose M here.
>   	  The module will be called pic32-wdt.
>
> +config PIC32_DMT
> +	tristate "Microchip PIC32 Deadman Timer"
> +	select WATCHDOG_CORE
> +	depends on MACH_PIC32
> +	default y

Are you sure ?

> +	help
> +	  Watchdog driver for PIC32 instruction fetch counting timer. This specific
> +	  timer is typically be used in misson critical and safety critical
> +	  applications, where any single failure of the software functionality
> +	  and sequencing must be detected.
> +
> +	  To compile this driver as a loadable module, choose M here.
> +	  The module will be called pic32-dmt.
> +
>   # PARISC Architecture
>
>   # POWERPC Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 244ed80..d051c9c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -154,6 +154,7 @@ obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
>   obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
>   obj-$(CONFIG_MT7621_WDT) += mt7621_wdt.o
>   obj-$(CONFIG_PIC32_WDT) += pic32-wdt.o
> +obj-$(CONFIG_PIC32_DMT) += pic32-dmt.o
>
>   # PARISC Architecture
>
> diff --git a/drivers/watchdog/pic32-dmt.c b/drivers/watchdog/pic32-dmt.c
> new file mode 100644
> index 0000000..13dd1e3
> --- /dev/null
> +++ b/drivers/watchdog/pic32-dmt.c
> @@ -0,0 +1,296 @@
> +/*
> + * PIC32 deadman timer driver
> + *
> + * Purna Chandra Mandal <purna.mandal@microchip.com>
> + * Copyright (c) 2016, Microchip Technology Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pm.h>
> +#include <linux/watchdog.h>
> +#include <linux/spinlock.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>

Please list include files in alphabetic order.

> +
> +#include <asm/mach-pic32/pic32.h>
> +
> +/* Deadman Timer Regs */
> +#define DMTCON_REG	0x00
> +#define DMTPRECLR_REG	0x10
> +#define DMTCLR_REG	0x20
> +#define DMTSTAT_REG	0x30
> +#define DMTCNT_REG	0x40
> +#define DMTPSCNT_REG	0x60
> +#define DMTPSINTV_REG	0x70
> +
> +/* Deadman Timer Regs fields */
> +#define DMT_ON			0x8000
> +#define DMT_STEP1_KEY		0x40
> +#define DMT_STEP1_KEY_BYTE	1
> +#define DMT_STEP2_KEY		0x08
> +#define DMTSTAT_WINOPN		0x01
> +#define DMTSTAT_EVENT		0x20
> +#define DMTSTAT_BAD2		0x40
> +#define DMTSTAT_BAD1		0x80
> +

BIT() ?

> +/* Reset Control Register fields for watchdog */
> +#define RESETCON_DMT_TIMEOUT    0x0020
> +
> +struct pic32_dmt {
> +	spinlock_t	lock;
> +	void __iomem	*regs;
> +	struct clk	*clk;
> +};
> +
> +static inline int dmt_is_enabled(struct pic32_dmt *dmt)

Should return bool.

> +{
> +	return readl(dmt->regs + DMTCON_REG) & DMT_ON;
> +}
> +
> +static inline void dmt_enable(struct pic32_dmt *dmt)
> +{
> +	writel(DMT_ON, PIC32_SET(dmt->regs + DMTCON_REG));
> +}
> +
> +static inline void dmt_disable(struct pic32_dmt *dmt)
> +{
> +	writel(DMT_ON, PIC32_CLR(dmt->regs + DMTCON_REG));
> +	cpu_relax();

Is this needed ?

> +}
> +
> +static inline int dmt_bad_status(struct pic32_dmt *dmt)
> +{
> +	u32 val;
> +
> +	val = readl(dmt->regs + DMTSTAT_REG);
> +	val &= (DMTSTAT_BAD1 | DMTSTAT_BAD2 | DMTSTAT_EVENT);
> +	if (val)
> +		pr_err("dmt: bad event generated: sts %08x\n", val);
> +
Is this error message really necessary (in addition to the error being reported
to userspace) ?

Also, where used, the error is reported as EAGAIN, which suggests a normal condition.
If so, please drop the message.

> +	return val;

I would suggest to return a valid error code directly instead of converting
the zero/nonzero result to an error in the calling code.

> +}
> +
> +static inline int dmt_keepalive(struct pic32_dmt *dmt)
> +{
> +	u32 v;
> +
> +	/* set pre-clear key */
> +	writel(DMT_STEP1_KEY << 8, dmt->regs + DMTPRECLR_REG);
> +
> +	/* wait for DMT window to open */
> +	for (;;) {
> +		v = readl(dmt->regs + DMTSTAT_REG) & DMTSTAT_WINOPN;
> +		if (v == DMTSTAT_WINOPN)
> +			break;
> +	}

Really wait forever in a hot loop with no timeout and no means to escape ?

> +
> +	/* apply key2 */
> +	writel(DMT_STEP2_KEY, dmt->regs + DMTCLR_REG);
> +
> +	/* check whether keys are latched correctly */
> +	return dmt_bad_status(dmt);
> +}
> +
> +static inline u32 dmt_timeleft(struct pic32_dmt *dmt)
> +{
> +	u32 top = readl(dmt->regs + DMTPSCNT_REG);
> +
> +	return top - readl(dmt->regs + DMTCNT_REG);
> +}
> +
> +static inline u32 dmt_interval_time_to_clear(struct pic32_dmt *dmt)
> +{
> +	return readl(dmt->regs + DMTPSINTV_REG);
> +}
> +

This function is not used anywhere.

> +static inline u32 pic32_dmt_get_timeout_secs(struct pic32_dmt *dmt)
> +{
> +	return readl(dmt->regs + DMTPSCNT_REG) / clk_get_rate(dmt->clk);
> +}
> +
> +static inline u32 pic32_dmt_bootstatus(struct pic32_dmt *dmt)
> +{
> +	u32 v;
> +	void __iomem *rst_base;
> +
> +	rst_base = ioremap(PIC32_BASE_RESET, 0x10);
> +	if (!rst_base)
> +		return 0;
> +
> +	v = readl(rst_base);
> +
> +	writel(RESETCON_DMT_TIMEOUT, PIC32_CLR(rst_base));
> +
> +	iounmap(rst_base);
> +	return v & RESETCON_DMT_TIMEOUT;
> +}
> +
> +static int pic32_dmt_start(struct watchdog_device *wdd)
> +{
> +	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
> +
> +	spin_lock(&dmt->lock);

Is this lock (on top of the watchdg core lock) needed ?

> +	if (dmt_is_enabled(dmt))
> +		goto done;
> +
Can this happen ?

> +	dmt_enable(dmt);

If so, does it hurt to just enable it again ?

> +done:
> +	dmt_keepalive(dmt);

This returns an error. Is it not reported on purpose ?

> +	spin_unlock(&dmt->lock);
> +
> +	return 0;
> +}
> +
> +static int pic32_dmt_stop(struct watchdog_device *wdd)
> +{
> +	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
> +
> +	spin_lock(&dmt->lock);
> +	dmt_disable(dmt);
> +	spin_unlock(&dmt->lock);
> +
> +	return 0;
> +}
> +
> +static int pic32_dmt_ping(struct watchdog_device *wdd)
> +{
> +	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
> +	int ret;
> +
> +	spin_lock(&dmt->lock);
> +	ret = dmt_keepalive(dmt);
> +	spin_unlock(&dmt->lock);
> +
> +	return ret ? -EAGAIN : 0;
> +}
> +
> +static unsigned int pic32_dmt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
> +
> +	return dmt_timeleft(dmt);

Does that function return the remaining time in seconds ?

> +}
> +
> +static const struct watchdog_ops pic32_dmt_fops = {
> +	.owner		= THIS_MODULE,
> +	.start		= pic32_dmt_start,
> +	.stop		= pic32_dmt_stop,
> +	.get_timeleft	= pic32_dmt_get_timeleft,
> +	.ping		= pic32_dmt_ping,
> +};
> +
> +static const struct watchdog_info pic32_dmt_ident = {
> +	.options	= WDIOF_KEEPALIVEPING |
> +			  WDIOF_MAGICCLOSE,
> +	.identity	= "PIC32 Deadman Timer",
> +};
> +
> +static struct watchdog_device pic32_dmt_wdd = {
> +	.info		= &pic32_dmt_ident,
> +	.ops		= &pic32_dmt_fops,
> +	.min_timeout	= 1,
> +	.max_timeout	= UINT_MAX,
> +};
> +
> +static int pic32_dmt_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct pic32_dmt *dmt;
> +	struct resource *mem;
> +	struct watchdog_device *wdd = &pic32_dmt_wdd;
> +
> +	dmt = devm_kzalloc(&pdev->dev, sizeof(*dmt), GFP_KERNEL);
> +	if (IS_ERR(dmt))
> +		return PTR_ERR(dmt);
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dmt->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(dmt->regs))
> +		return PTR_ERR(dmt->regs);
> +
> +	dmt->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(dmt->clk)) {
> +		dev_err(&pdev->dev, "clk not found\n");
> +		return PTR_ERR(dmt->clk);
> +	}
> +
> +	ret = clk_prepare_enable(dmt->clk);
> +	if (ret)
> +		return ret;
> +
> +	wdd->max_timeout /= clk_get_rate(dmt->clk);

Please just assign UINT_MAX / clk_get_rate(dmt->clk).

> +	wdd->timeout = pic32_dmt_get_timeout_secs(dmt);
> +	wdd->bootstatus = pic32_dmt_bootstatus(dmt) ? WDIOF_CARDRESET : 0;
> +	if (!wdd->timeout) {
> +		dev_err(&pdev->dev,
> +			"timeout %dsec too small for DMT\n", wdd->timeout);

wdd->timeout is 0 here, so displaying the variable does not provide much value.
Besides "0sec" looks a bit odd.

> +		ret = -EINVAL;
> +		goto out_disable_clk;
> +	}
> +
> +	spin_lock_init(&dmt->lock);
> +
> +	dev_info(&pdev->dev, "max_timeout %d, min_timeout %d, cur_timeout %d\n",
> +		 wdd->max_timeout, wdd->min_timeout, wdd->timeout);

min_timeout is a constant and thus not worth reporting.
Please report the current timeout as "timeout".

> +	ret = watchdog_register_device(wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "watchdog register failed, err %d\n", ret);
> +		goto out_disable_clk;
> +	}
> +
> +	watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
> +	watchdog_set_drvdata(wdd, dmt);

Race condition: drvdata may be needed after the call to watchdog_register_device().

> +
> +	platform_set_drvdata(pdev, wdd);
> +	return 0;
> +
> +out_disable_clk:
> +	clk_disable_unprepare(dmt->clk);
> +	return ret;
> +}
> +
> +static int pic32_dmt_remove(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wdd = platform_get_drvdata(pdev);
> +	struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
> +
> +	clk_disable_unprepare(dmt->clk);
> +	watchdog_unregister_device(wdd);

Unregister first ?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pic32_dmt_of_ids[] = {
> +	{ .compatible = "microchip,pic32mzda-dmt",},

Is the documentation for this compatible string added with a separate patch ?

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, pic32_dmt_of_ids);
> +
> +static struct platform_driver pic32_dmt_driver = {
> +	.probe		= pic32_dmt_probe,
> +	.remove		= pic32_dmt_remove,
> +	.driver		= {
> +		.name		= "pi32-dmt",
> +		.owner		= THIS_MODULE,
> +		.of_match_table = of_match_ptr(pic32_dmt_of_ids),
> +	}
> +};
> +
> +module_platform_driver(pic32_dmt_driver);
> +
> +MODULE_AUTHOR("Purna Chandra Mandal <purna.mandal@microchip.com>");
> +MODULE_DESCRIPTION("Microchip PIC32 DMT Driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
>

Re: [PATCH 1/2] dt/bindings: Add bindings for PIC32 deadman timer peripheral

[Sergei Shtylyov]

On 2/2/2016 3:02 AM, Joshua Henderson wrote:

> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>
> Document the devicetree bindings for the deadman timer peripheral found on
> Microchip PIC32 SoC class devices.
>
> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: <linux-mips@linux-mips.org>
> ---
> Note: Please merge this patch series through the MIPS tree.
> ---
>   .../bindings/watchdog/microchip,pic32-dmt.txt      |   19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
>
> diff --git a/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
> new file mode 100644
> index 0000000..f7374ee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt
> @@ -0,0 +1,19 @@
> +* Microchip PIC32 Deadman Timer
> +
> +The deadman timer is used to reset the processor in the event of a software
> +malfunction. It is a free-running instruction fetch timer, which is clocked
> +whenever an instruction fetch occurs until a count match occurs.
> +
> +Required properties:
> +- compatible: must be "microchip,pic32mzda-dmt".
> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +- clocks: phandle of parent clock (should be &PBCLK7).
> +
> +Example:
> +
> +	watchdog2: dmt@1f800a00 {

    The node names should be generic, i.e. "watchdog" in this case.

[...]

MBR, Sergei

Re: [PATCH 2/2] watchdog: pic32-dmt: Add PIC32 deadman timer driver support

[Joshua Henderson]

Guenter,

On 02/01/2016 08:04 PM, Guenter Roeck wrote:
> On 02/01/2016 04:02 PM, Joshua Henderson wrote:
>> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>>
>> The primary function of the deadman timer (DMT) is to reset the processor
>> in the event of a software malfunction. The DMT is a free-running
>> instruction fetch timer, which is clocked whenever an instruction fetch
>> occurs until a count match occurs. Instructions are not fetched when
>> the processor is in sleep mode.
>>
>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
>> Cc: Ralf Baechle <ralf@linux-mips.org>
>> Cc: <linux-mips@linux-mips.org>
> 
> Please drop this Cc: from the commit log.
> 

Ack.

>> ---
>> Note: Please merge this patch series through the MIPS tree.
>> ---
>>   drivers/watchdog/Kconfig     |   14 ++
>>   drivers/watchdog/Makefile    |    1 +
>>   drivers/watchdog/pic32-dmt.c |  296 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 311 insertions(+)
>>   create mode 100644 drivers/watchdog/pic32-dmt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 131abc2..87985c6 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1424,6 +1424,20 @@ config PIC32_WDT
>>         To compile this driver as a loadable module, choose M here.
>>         The module will be called pic32-wdt.
>>
>> +config PIC32_DMT
>> +    tristate "Microchip PIC32 Deadman Timer"
>> +    select WATCHDOG_CORE
>> +    depends on MACH_PIC32
>> +    default y
> 
> Are you sure ?

No. Will remove.

>> +    help
>> +      Watchdog driver for PIC32 instruction fetch counting timer. This specific
>> +      timer is typically be used in misson critical and safety critical
>> +      applications, where any single failure of the software functionality
>> +      and sequencing must be detected.
>> +
>> +      To compile this driver as a loadable module, choose M here.
>> +      The module will be called pic32-dmt.
>> +
>>   # PARISC Architecture
>>
>>   # POWERPC Architecture
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 244ed80..d051c9c 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -154,6 +154,7 @@ obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
>>   obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
>>   obj-$(CONFIG_MT7621_WDT) += mt7621_wdt.o
>>   obj-$(CONFIG_PIC32_WDT) += pic32-wdt.o
>> +obj-$(CONFIG_PIC32_DMT) += pic32-dmt.o
>>
>>   # PARISC Architecture
>>
>> diff --git a/drivers/watchdog/pic32-dmt.c b/drivers/watchdog/pic32-dmt.c
>> new file mode 100644
>> index 0000000..13dd1e3
>> --- /dev/null
>> +++ b/drivers/watchdog/pic32-dmt.c
>> @@ -0,0 +1,296 @@
>> +/*
>> + * PIC32 deadman timer driver
>> + *
>> + * Purna Chandra Mandal <purna.mandal@microchip.com>
>> + * Copyright (c) 2016, Microchip Technology Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/watchdog.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
> 
> Please list include files in alphabetic order.
> 

Will do.

>> +
>> +#include <asm/mach-pic32/pic32.h>
>> +
>> +/* Deadman Timer Regs */
>> +#define DMTCON_REG    0x00
>> +#define DMTPRECLR_REG    0x10
>> +#define DMTCLR_REG    0x20
>> +#define DMTSTAT_REG    0x30
>> +#define DMTCNT_REG    0x40
>> +#define DMTPSCNT_REG    0x60
>> +#define DMTPSINTV_REG    0x70
>> +
>> +/* Deadman Timer Regs fields */
>> +#define DMT_ON            0x8000
>> +#define DMT_STEP1_KEY        0x40
>> +#define DMT_STEP1_KEY_BYTE    1
>> +#define DMT_STEP2_KEY        0x08
>> +#define DMTSTAT_WINOPN        0x01
>> +#define DMTSTAT_EVENT        0x20
>> +#define DMTSTAT_BAD2        0x40
>> +#define DMTSTAT_BAD1        0x80
>> +
> 
> BIT() ?

Ack.

>> +/* Reset Control Register fields for watchdog */
>> +#define RESETCON_DMT_TIMEOUT    0x0020
>> +
>> +struct pic32_dmt {
>> +    spinlock_t    lock;
>> +    void __iomem    *regs;
>> +    struct clk    *clk;
>> +};
>> +
>> +static inline int dmt_is_enabled(struct pic32_dmt *dmt)
> 
> Should return bool.

This function will be dropped.

>> +{
>> +    return readl(dmt->regs + DMTCON_REG) & DMT_ON;
>> +}
>> +
>> +static inline void dmt_enable(struct pic32_dmt *dmt)
>> +{
>> +    writel(DMT_ON, PIC32_SET(dmt->regs + DMTCON_REG));
>> +}
>> +
>> +static inline void dmt_disable(struct pic32_dmt *dmt)
>> +{
>> +    writel(DMT_ON, PIC32_CLR(dmt->regs + DMTCON_REG));
>> +    cpu_relax();
> 
> Is this needed ?
> 

Something is, but probably not this. No watchdog registers may not be read or written in the CPU clock cycle immediately following the instruction that clears the module’s ON bit.  I'll comment and investigate a better solution.

>> +}
>> +
>> +static inline int dmt_bad_status(struct pic32_dmt *dmt)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(dmt->regs + DMTSTAT_REG);
>> +    val &= (DMTSTAT_BAD1 | DMTSTAT_BAD2 | DMTSTAT_EVENT);
>> +    if (val)
>> +        pr_err("dmt: bad event generated: sts %08x\n", val);
>> +
> Is this error message really necessary (in addition to the error being reported
> to userspace) ?
> 
> Also, where used, the error is reported as EAGAIN, which suggests a normal condition.
> If so, please drop the message.

It's not really necessary. Probably should have been dbg, but will remove.

>> +    return val;
> 
> I would suggest to return a valid error code directly instead of converting
> the zero/nonzero result to an error in the calling code.

Ack.

>> +}
>> +
>> +static inline int dmt_keepalive(struct pic32_dmt *dmt)
>> +{
>> +    u32 v;
>> +
>> +    /* set pre-clear key */
>> +    writel(DMT_STEP1_KEY << 8, dmt->regs + DMTPRECLR_REG);
>> +
>> +    /* wait for DMT window to open */
>> +    for (;;) {
>> +        v = readl(dmt->regs + DMTSTAT_REG) & DMTSTAT_WINOPN;
>> +        if (v == DMTSTAT_WINOPN)
>> +            break;
>> +    }
> 
> Really wait forever in a hot loop with no timeout and no means to escape ?

I'll find a max to break the loop.  Whats the worst that could happen?  The CPU will reset. :)

> 
>> +
>> +    /* apply key2 */
>> +    writel(DMT_STEP2_KEY, dmt->regs + DMTCLR_REG);
>> +
>> +    /* check whether keys are latched correctly */
>> +    return dmt_bad_status(dmt);
>> +}
>> +
>> +static inline u32 dmt_timeleft(struct pic32_dmt *dmt)
>> +{
>> +    u32 top = readl(dmt->regs + DMTPSCNT_REG);
>> +
>> +    return top - readl(dmt->regs + DMTCNT_REG);
>> +}
>> +
>> +static inline u32 dmt_interval_time_to_clear(struct pic32_dmt *dmt)
>> +{
>> +    return readl(dmt->regs + DMTPSINTV_REG);
>> +}
>> +
> 
> This function is not used anywhere.

Removed.

>> +static inline u32 pic32_dmt_get_timeout_secs(struct pic32_dmt *dmt)
>> +{
>> +    return readl(dmt->regs + DMTPSCNT_REG) / clk_get_rate(dmt->clk);
>> +}
>> +
>> +static inline u32 pic32_dmt_bootstatus(struct pic32_dmt *dmt)
>> +{
>> +    u32 v;
>> +    void __iomem *rst_base;
>> +
>> +    rst_base = ioremap(PIC32_BASE_RESET, 0x10);
>> +    if (!rst_base)
>> +        return 0;
>> +
>> +    v = readl(rst_base);
>> +
>> +    writel(RESETCON_DMT_TIMEOUT, PIC32_CLR(rst_base));
>> +
>> +    iounmap(rst_base);
>> +    return v & RESETCON_DMT_TIMEOUT;
>> +}
>> +
>> +static int pic32_dmt_start(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    spin_lock(&dmt->lock);
> 
> Is this lock (on top of the watchdg core lock) needed ?

It's not needed the watchdog_core mutex in the path.

> 
>> +    if (dmt_is_enabled(dmt))
>> +        goto done;
>> +
> Can this happen ?

It can, but this logic isn't really necessary.

> 
>> +    dmt_enable(dmt);
> 
> If so, does it hurt to just enable it again ?

I will confirm, but I don't think it hurts to enable again in this case.

> 
>> +done:
>> +    dmt_keepalive(dmt);
> 
> This returns an error. Is it not reported on purpose ?

It does make sense to go ahead and report the error, as long as -EAGAIN makes sense from pic32_dmt_start().  Or some other error.  Any thoughts?

> 
>> +    spin_unlock(&dmt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int pic32_dmt_stop(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    spin_lock(&dmt->lock);
>> +    dmt_disable(dmt);
>> +    spin_unlock(&dmt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int pic32_dmt_ping(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +    int ret;
>> +
>> +    spin_lock(&dmt->lock);
>> +    ret = dmt_keepalive(dmt);
>> +    spin_unlock(&dmt->lock);
>> +
>> +    return ret ? -EAGAIN : 0;
>> +}
>> +
>> +static unsigned int pic32_dmt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    return dmt_timeleft(dmt);
> 
> Does that function return the remaining time in seconds ?

It does not.  Will remove.

> 
>> +}
>> +
>> +static const struct watchdog_ops pic32_dmt_fops = {
>> +    .owner        = THIS_MODULE,
>> +    .start        = pic32_dmt_start,
>> +    .stop        = pic32_dmt_stop,
>> +    .get_timeleft    = pic32_dmt_get_timeleft,

Will also drop this function.

>> +    .ping        = pic32_dmt_ping,
>> +};
>> +
>> +static const struct watchdog_info pic32_dmt_ident = {
>> +    .options    = WDIOF_KEEPALIVEPING |
>> +              WDIOF_MAGICCLOSE,
>> +    .identity    = "PIC32 Deadman Timer",
>> +};
>> +
>> +static struct watchdog_device pic32_dmt_wdd = {
>> +    .info        = &pic32_dmt_ident,
>> +    .ops        = &pic32_dmt_fops,
>> +    .min_timeout    = 1,
>> +    .max_timeout    = UINT_MAX,
>> +};
>> +
>> +static int pic32_dmt_probe(struct platform_device *pdev)
>> +{
>> +    int ret;
>> +    struct pic32_dmt *dmt;
>> +    struct resource *mem;
>> +    struct watchdog_device *wdd = &pic32_dmt_wdd;
>> +
>> +    dmt = devm_kzalloc(&pdev->dev, sizeof(*dmt), GFP_KERNEL);
>> +    if (IS_ERR(dmt))
>> +        return PTR_ERR(dmt);
>> +
>> +    mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    dmt->regs = devm_ioremap_resource(&pdev->dev, mem);
>> +    if (IS_ERR(dmt->regs))
>> +        return PTR_ERR(dmt->regs);
>> +
>> +    dmt->clk = devm_clk_get(&pdev->dev, NULL);
>> +    if (IS_ERR(dmt->clk)) {
>> +        dev_err(&pdev->dev, "clk not found\n");
>> +        return PTR_ERR(dmt->clk);
>> +    }
>> +
>> +    ret = clk_prepare_enable(dmt->clk);
>> +    if (ret)
>> +        return ret;
>> +
>> +    wdd->max_timeout /= clk_get_rate(dmt->clk);
> 
> Please just assign UINT_MAX / clk_get_rate(dmt->clk).
> 

As with the watchdog driver, I will just drop this calculation.

>> +    wdd->timeout = pic32_dmt_get_timeout_secs(dmt);
>> +    wdd->bootstatus = pic32_dmt_bootstatus(dmt) ? WDIOF_CARDRESET : 0;
>> +    if (!wdd->timeout) {
>> +        dev_err(&pdev->dev,
>> +            "timeout %dsec too small for DMT\n", wdd->timeout);
> 
> wdd->timeout is 0 here, so displaying the variable does not provide much value.
> Besides "0sec" looks a bit odd.

Agreed.

> 
>> +        ret = -EINVAL;
>> +        goto out_disable_clk;
>> +    }
>> +
>> +    spin_lock_init(&dmt->lock);
>> +
>> +    dev_info(&pdev->dev, "max_timeout %d, min_timeout %d, cur_timeout %d\n",
>> +         wdd->max_timeout, wdd->min_timeout, wdd->timeout);
> 
> min_timeout is a constant and thus not worth reporting.
> Please report the current timeout as "timeout".

Ack.

> 
>> +    ret = watchdog_register_device(wdd);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "watchdog register failed, err %d\n", ret);
>> +        goto out_disable_clk;
>> +    }
>> +
>> +    watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
>> +    watchdog_set_drvdata(wdd, dmt);
> 
> Race condition: drvdata may be needed after the call to watchdog_register_device().

Ack.

> 
>> +
>> +    platform_set_drvdata(pdev, wdd);
>> +    return 0;
>> +
>> +out_disable_clk:
>> +    clk_disable_unprepare(dmt->clk);
>> +    return ret;
>> +}
>> +
>> +static int pic32_dmt_remove(struct platform_device *pdev)
>> +{
>> +    struct watchdog_device *wdd = platform_get_drvdata(pdev);
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    clk_disable_unprepare(dmt->clk);
>> +    watchdog_unregister_device(wdd);
> 
> Unregister first ?

Ack.

> 
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id pic32_dmt_of_ids[] = {
>> +    { .compatible = "microchip,pic32mzda-dmt",},
> 
> Is the documentation for this compatible string added with a separate patch ?
> 

Yes. I goofed up and did not Cc: you directly. I'll make sure and Cc: you in V2.  See [PATCH 1/2] of this series: https://lkml.org/lkml/2016/2/1/865

>> +    { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, pic32_dmt_of_ids);
>> +
>> +static struct platform_driver pic32_dmt_driver = {
>> +    .probe        = pic32_dmt_probe,
>> +    .remove        = pic32_dmt_remove,
>> +    .driver        = {
>> +        .name        = "pi32-dmt",
>> +        .owner        = THIS_MODULE,
>> +        .of_match_table = of_match_ptr(pic32_dmt_of_ids),
>> +    }
>> +};
>> +
>> +module_platform_driver(pic32_dmt_driver);
>> +
>> +MODULE_AUTHOR("Purna Chandra Mandal <purna.mandal@microchip.com>");
>> +MODULE_DESCRIPTION("Microchip PIC32 DMT Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 1.7.9.5
>>
>>
> 

Thanks,
Josh

Re: [PATCH 2/2] watchdog: pic32-dmt: Add PIC32 deadman timer driver support

[Joshua Henderson]

Guenter,

On 02/01/2016 08:04 PM, Guenter Roeck wrote:
> On 02/01/2016 04:02 PM, Joshua Henderson wrote:
>> From: Purna Chandra Mandal <purna.mandal@microchip.com>
>>
>> The primary function of the deadman timer (DMT) is to reset the processor
>> in the event of a software malfunction. The DMT is a free-running
>> instruction fetch timer, which is clocked whenever an instruction fetch
>> occurs until a count match occurs. Instructions are not fetched when
>> the processor is in sleep mode.
>>
>> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
>> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
>> Cc: Ralf Baechle <ralf@linux-mips.org>
>> Cc: <linux-mips@linux-mips.org>
> 
> Please drop this Cc: from the commit log.
> 

Ack.

>> ---
>> Note: Please merge this patch series through the MIPS tree.
>> ---
>>   drivers/watchdog/Kconfig     |   14 ++
>>   drivers/watchdog/Makefile    |    1 +
>>   drivers/watchdog/pic32-dmt.c |  296 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 311 insertions(+)
>>   create mode 100644 drivers/watchdog/pic32-dmt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 131abc2..87985c6 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1424,6 +1424,20 @@ config PIC32_WDT
>>         To compile this driver as a loadable module, choose M here.
>>         The module will be called pic32-wdt.
>>
>> +config PIC32_DMT
>> +    tristate "Microchip PIC32 Deadman Timer"
>> +    select WATCHDOG_CORE
>> +    depends on MACH_PIC32
>> +    default y
> 
> Are you sure ?

No. Will remove.

>> +    help
>> +      Watchdog driver for PIC32 instruction fetch counting timer. This specific
>> +      timer is typically be used in misson critical and safety critical
>> +      applications, where any single failure of the software functionality
>> +      and sequencing must be detected.
>> +
>> +      To compile this driver as a loadable module, choose M here.
>> +      The module will be called pic32-dmt.
>> +
>>   # PARISC Architecture
>>
>>   # POWERPC Architecture
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 244ed80..d051c9c 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -154,6 +154,7 @@ obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
>>   obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
>>   obj-$(CONFIG_MT7621_WDT) += mt7621_wdt.o
>>   obj-$(CONFIG_PIC32_WDT) += pic32-wdt.o
>> +obj-$(CONFIG_PIC32_DMT) += pic32-dmt.o
>>
>>   # PARISC Architecture
>>
>> diff --git a/drivers/watchdog/pic32-dmt.c b/drivers/watchdog/pic32-dmt.c
>> new file mode 100644
>> index 0000000..13dd1e3
>> --- /dev/null
>> +++ b/drivers/watchdog/pic32-dmt.c
>> @@ -0,0 +1,296 @@
>> +/*
>> + * PIC32 deadman timer driver
>> + *
>> + * Purna Chandra Mandal <purna.mandal@microchip.com>
>> + * Copyright (c) 2016, Microchip Technology Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/watchdog.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
> 
> Please list include files in alphabetic order.
> 

Will do.

>> +
>> +#include <asm/mach-pic32/pic32.h>
>> +
>> +/* Deadman Timer Regs */
>> +#define DMTCON_REG    0x00
>> +#define DMTPRECLR_REG    0x10
>> +#define DMTCLR_REG    0x20
>> +#define DMTSTAT_REG    0x30
>> +#define DMTCNT_REG    0x40
>> +#define DMTPSCNT_REG    0x60
>> +#define DMTPSINTV_REG    0x70
>> +
>> +/* Deadman Timer Regs fields */
>> +#define DMT_ON            0x8000
>> +#define DMT_STEP1_KEY        0x40
>> +#define DMT_STEP1_KEY_BYTE    1
>> +#define DMT_STEP2_KEY        0x08
>> +#define DMTSTAT_WINOPN        0x01
>> +#define DMTSTAT_EVENT        0x20
>> +#define DMTSTAT_BAD2        0x40
>> +#define DMTSTAT_BAD1        0x80
>> +
> 
> BIT() ?

Ack.

>> +/* Reset Control Register fields for watchdog */
>> +#define RESETCON_DMT_TIMEOUT    0x0020
>> +
>> +struct pic32_dmt {
>> +    spinlock_t    lock;
>> +    void __iomem    *regs;
>> +    struct clk    *clk;
>> +};
>> +
>> +static inline int dmt_is_enabled(struct pic32_dmt *dmt)
> 
> Should return bool.

This function will be dropped.

>> +{
>> +    return readl(dmt->regs + DMTCON_REG) & DMT_ON;
>> +}
>> +
>> +static inline void dmt_enable(struct pic32_dmt *dmt)
>> +{
>> +    writel(DMT_ON, PIC32_SET(dmt->regs + DMTCON_REG));
>> +}
>> +
>> +static inline void dmt_disable(struct pic32_dmt *dmt)
>> +{
>> +    writel(DMT_ON, PIC32_CLR(dmt->regs + DMTCON_REG));
>> +    cpu_relax();
> 
> Is this needed ?
> 

Something is, but probably not this. No watchdog registers may not be read or written in the CPU clock cycle immediately following the instruction that clears the module’s ON bit.  I'll comment and investigate a better solution.

>> +}
>> +
>> +static inline int dmt_bad_status(struct pic32_dmt *dmt)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(dmt->regs + DMTSTAT_REG);
>> +    val &= (DMTSTAT_BAD1 | DMTSTAT_BAD2 | DMTSTAT_EVENT);
>> +    if (val)
>> +        pr_err("dmt: bad event generated: sts %08x\n", val);
>> +
> Is this error message really necessary (in addition to the error being reported
> to userspace) ?
> 
> Also, where used, the error is reported as EAGAIN, which suggests a normal condition.
> If so, please drop the message.

It's not really necessary. Probably should have been dbg, but will remove.

>> +    return val;
> 
> I would suggest to return a valid error code directly instead of converting
> the zero/nonzero result to an error in the calling code.

Ack.

>> +}
>> +
>> +static inline int dmt_keepalive(struct pic32_dmt *dmt)
>> +{
>> +    u32 v;
>> +
>> +    /* set pre-clear key */
>> +    writel(DMT_STEP1_KEY << 8, dmt->regs + DMTPRECLR_REG);
>> +
>> +    /* wait for DMT window to open */
>> +    for (;;) {
>> +        v = readl(dmt->regs + DMTSTAT_REG) & DMTSTAT_WINOPN;
>> +        if (v == DMTSTAT_WINOPN)
>> +            break;
>> +    }
> 
> Really wait forever in a hot loop with no timeout and no means to escape ?

I'll find a max to break the loop.  Whats the worst that could happen?  The CPU will reset. :)

> 
>> +
>> +    /* apply key2 */
>> +    writel(DMT_STEP2_KEY, dmt->regs + DMTCLR_REG);
>> +
>> +    /* check whether keys are latched correctly */
>> +    return dmt_bad_status(dmt);
>> +}
>> +
>> +static inline u32 dmt_timeleft(struct pic32_dmt *dmt)
>> +{
>> +    u32 top = readl(dmt->regs + DMTPSCNT_REG);
>> +
>> +    return top - readl(dmt->regs + DMTCNT_REG);
>> +}
>> +
>> +static inline u32 dmt_interval_time_to_clear(struct pic32_dmt *dmt)
>> +{
>> +    return readl(dmt->regs + DMTPSINTV_REG);
>> +}
>> +
> 
> This function is not used anywhere.

Removed.

>> +static inline u32 pic32_dmt_get_timeout_secs(struct pic32_dmt *dmt)
>> +{
>> +    return readl(dmt->regs + DMTPSCNT_REG) / clk_get_rate(dmt->clk);
>> +}
>> +
>> +static inline u32 pic32_dmt_bootstatus(struct pic32_dmt *dmt)
>> +{
>> +    u32 v;
>> +    void __iomem *rst_base;
>> +
>> +    rst_base = ioremap(PIC32_BASE_RESET, 0x10);
>> +    if (!rst_base)
>> +        return 0;
>> +
>> +    v = readl(rst_base);
>> +
>> +    writel(RESETCON_DMT_TIMEOUT, PIC32_CLR(rst_base));
>> +
>> +    iounmap(rst_base);
>> +    return v & RESETCON_DMT_TIMEOUT;
>> +}
>> +
>> +static int pic32_dmt_start(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    spin_lock(&dmt->lock);
> 
> Is this lock (on top of the watchdg core lock) needed ?

It's not needed the watchdog_core mutex in the path.

> 
>> +    if (dmt_is_enabled(dmt))
>> +        goto done;
>> +
> Can this happen ?

It can, but this logic isn't really necessary.

> 
>> +    dmt_enable(dmt);
> 
> If so, does it hurt to just enable it again ?

I will confirm, but I don't think it hurts to enable again in this case.

> 
>> +done:
>> +    dmt_keepalive(dmt);
> 
> This returns an error. Is it not reported on purpose ?

It does make sense to go ahead and report the error, as long as -EAGAIN makes sense from pic32_dmt_start().  Or some other error.  Any thoughts?

> 
>> +    spin_unlock(&dmt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int pic32_dmt_stop(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    spin_lock(&dmt->lock);
>> +    dmt_disable(dmt);
>> +    spin_unlock(&dmt->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int pic32_dmt_ping(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +    int ret;
>> +
>> +    spin_lock(&dmt->lock);
>> +    ret = dmt_keepalive(dmt);
>> +    spin_unlock(&dmt->lock);
>> +
>> +    return ret ? -EAGAIN : 0;
>> +}
>> +
>> +static unsigned int pic32_dmt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    return dmt_timeleft(dmt);
> 
> Does that function return the remaining time in seconds ?

It does not.  Will remove.

> 
>> +}
>> +
>> +static const struct watchdog_ops pic32_dmt_fops = {
>> +    .owner        = THIS_MODULE,
>> +    .start        = pic32_dmt_start,
>> +    .stop        = pic32_dmt_stop,
>> +    .get_timeleft    = pic32_dmt_get_timeleft,

Will also drop this function.

>> +    .ping        = pic32_dmt_ping,
>> +};
>> +
>> +static const struct watchdog_info pic32_dmt_ident = {
>> +    .options    = WDIOF_KEEPALIVEPING |
>> +              WDIOF_MAGICCLOSE,
>> +    .identity    = "PIC32 Deadman Timer",
>> +};
>> +
>> +static struct watchdog_device pic32_dmt_wdd = {
>> +    .info        = &pic32_dmt_ident,
>> +    .ops        = &pic32_dmt_fops,
>> +    .min_timeout    = 1,
>> +    .max_timeout    = UINT_MAX,
>> +};
>> +
>> +static int pic32_dmt_probe(struct platform_device *pdev)
>> +{
>> +    int ret;
>> +    struct pic32_dmt *dmt;
>> +    struct resource *mem;
>> +    struct watchdog_device *wdd = &pic32_dmt_wdd;
>> +
>> +    dmt = devm_kzalloc(&pdev->dev, sizeof(*dmt), GFP_KERNEL);
>> +    if (IS_ERR(dmt))
>> +        return PTR_ERR(dmt);
>> +
>> +    mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    dmt->regs = devm_ioremap_resource(&pdev->dev, mem);
>> +    if (IS_ERR(dmt->regs))
>> +        return PTR_ERR(dmt->regs);
>> +
>> +    dmt->clk = devm_clk_get(&pdev->dev, NULL);
>> +    if (IS_ERR(dmt->clk)) {
>> +        dev_err(&pdev->dev, "clk not found\n");
>> +        return PTR_ERR(dmt->clk);
>> +    }
>> +
>> +    ret = clk_prepare_enable(dmt->clk);
>> +    if (ret)
>> +        return ret;
>> +
>> +    wdd->max_timeout /= clk_get_rate(dmt->clk);
> 
> Please just assign UINT_MAX / clk_get_rate(dmt->clk).
> 

As with the watchdog driver, I will just drop this calculation.

>> +    wdd->timeout = pic32_dmt_get_timeout_secs(dmt);
>> +    wdd->bootstatus = pic32_dmt_bootstatus(dmt) ? WDIOF_CARDRESET : 0;
>> +    if (!wdd->timeout) {
>> +        dev_err(&pdev->dev,
>> +            "timeout %dsec too small for DMT\n", wdd->timeout);
> 
> wdd->timeout is 0 here, so displaying the variable does not provide much value.
> Besides "0sec" looks a bit odd.

Agreed.

> 
>> +        ret = -EINVAL;
>> +        goto out_disable_clk;
>> +    }
>> +
>> +    spin_lock_init(&dmt->lock);
>> +
>> +    dev_info(&pdev->dev, "max_timeout %d, min_timeout %d, cur_timeout %d\n",
>> +         wdd->max_timeout, wdd->min_timeout, wdd->timeout);
> 
> min_timeout is a constant and thus not worth reporting.
> Please report the current timeout as "timeout".

Ack.

> 
>> +    ret = watchdog_register_device(wdd);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "watchdog register failed, err %d\n", ret);
>> +        goto out_disable_clk;
>> +    }
>> +
>> +    watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
>> +    watchdog_set_drvdata(wdd, dmt);
> 
> Race condition: drvdata may be needed after the call to watchdog_register_device().

Ack.

> 
>> +
>> +    platform_set_drvdata(pdev, wdd);
>> +    return 0;
>> +
>> +out_disable_clk:
>> +    clk_disable_unprepare(dmt->clk);
>> +    return ret;
>> +}
>> +
>> +static int pic32_dmt_remove(struct platform_device *pdev)
>> +{
>> +    struct watchdog_device *wdd = platform_get_drvdata(pdev);
>> +    struct pic32_dmt *dmt = watchdog_get_drvdata(wdd);
>> +
>> +    clk_disable_unprepare(dmt->clk);
>> +    watchdog_unregister_device(wdd);
> 
> Unregister first ?

Ack.

> 
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id pic32_dmt_of_ids[] = {
>> +    { .compatible = "microchip,pic32mzda-dmt",},
> 
> Is the documentation for this compatible string added with a separate patch ?
> 

Yes. I goofed up and did not Cc: you directly. I'll make sure and Cc: you in V2.  See [PATCH 1/2] of this series: https://lkml.org/lkml/2016/2/1/865

>> +    { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, pic32_dmt_of_ids);
>> +
>> +static struct platform_driver pic32_dmt_driver = {
>> +    .probe        = pic32_dmt_probe,
>> +    .remove        = pic32_dmt_remove,
>> +    .driver        = {
>> +        .name        = "pi32-dmt",
>> +        .owner        = THIS_MODULE,
>> +        .of_match_table = of_match_ptr(pic32_dmt_of_ids),
>> +    }
>> +};
>> +
>> +module_platform_driver(pic32_dmt_driver);
>> +
>> +MODULE_AUTHOR("Purna Chandra Mandal <purna.mandal@microchip.com>");
>> +MODULE_DESCRIPTION("Microchip PIC32 DMT Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 1.7.9.5
>>
>>
> 

Thanks,
Josh

Re: [PATCH 1/2] dt/bindings: Add bindings for PIC32 deadman timer peripheral

[Rob Herring]

On Mon, Feb 01, 2016 at 05:02:23PM -0700, Joshua Henderson wrote:
> From: Purna Chandra Mandal <purna.mandal@microchip.com>
> 
> Document the devicetree bindings for the deadman timer peripheral found on
> Microchip PIC32 SoC class devices.
> 
> Signed-off-by: Purna Chandra Mandal <purna.mandal@microchip.com>
> Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: <linux-mips@linux-mips.org>
> ---
> Note: Please merge this patch series through the MIPS tree.
> ---
>  .../bindings/watchdog/microchip,pic32-dmt.txt      |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/microchip,pic32-dmt.txt

Other than the node name comment:

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