[PATCH 1/3] dt-bindings: watchdog: sam9x60_wdt: add bindings

[PATCH 1/3] dt-bindings: watchdog: sam9x60_wdt: add bindings

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Add bindings for Microchip SAM9X60 Watchdog Timer

It has the same bindings as
Documentation/devicetree/bindings/watchdog/atmel-sama5d4-wdt.txt
except the compatible.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 .../devicetree/bindings/watchdog/sam9x60-wdt.txt   | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt b/Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt
new file mode 100644
index 00000000..74b4e2d
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt
@@ -0,0 +1,34 @@
+* Microchip SAM9X60 Watchdog Timer (WDT) Controller
+
+Required properties:
+- compatible: "microchip,sam9x60-wdt"
+- reg: base physical address and length of memory mapped region.
+
+Optional properties:
+- timeout-sec: watchdog timeout value (in seconds).
+- interrupts: interrupt number to the CPU.
+- atmel,watchdog-type: should be "hardware" or "software".
+	"hardware": enable watchdog fault reset. A watchdog fault triggers
+		    watchdog reset.
+	"software": enable watchdog fault interrupt. A watchdog fault asserts
+		    watchdog interrupt.
+- atmel,idle-halt: present if you want to stop the watchdog when the CPU is
+		   in idle state.
+	CAUTION: This property should be used with care, it actually makes the
+	watchdog not counting when the CPU is in idle state, therefore the
+	watchdog reset time depends on mean CPU usage and will not reset at all
+	if the CPU stop working while it is in idle state, which is probably
+	not what you want.
+- atmel,dbg-halt: present if you want to stop the watchdog when the CPU is
+		  in debug state.
+
+Example:
+	watchdog@ffffff80 {
+		compatible = "microchip,sam9x60-wdt";
+		reg = <0xffffff80 0x24>;
+		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 5>;
+		timeout-sec = <10>;
+		atmel,watchdog-type = "hardware";
+		atmel,dbg-halt;
+		atmel,idle-halt;
+	};
-- 
2.7.4

[PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

This is the driver for SAM9X60 watchdog timer.
The offered functionality is the same as sama5d4_wdt.
The difference comes in register map, way to configure the timeout and
interrupts.
Developed starting from sama5d4_wdt.c

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/watchdog/Kconfig       |   9 ++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/sam9x60_wdt.c | 335 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/watchdog/sam9x60_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 58e7c10..3562e26 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -416,6 +416,15 @@ config SAMA5D4_WATCHDOG
 	  Its Watchdog Timer Mode Register can be written more than once.
 	  This will reboot your system when the timeout is reached.
 
+config SAM9X60_WATCHDOG
+	tristate "Microchip SAM9X60 Watchdog Timer"
+	depends on ARCH_AT91 || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  Microchip SAM9X60 watchdog timer is embedded into SAM9X60 chips.
+	  Its Watchdog Timer Mode Register can be written more than once.
+	  This will reboot your system when the timeout is reached.
+
 config CADENCE_WATCHDOG
 	tristate "Cadence Watchdog Timer"
 	depends on HAS_IOMEM
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2ee352b..93ba599 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
 obj-$(CONFIG_S3C2410_WATCHDOG) += s3c2410_wdt.o
 obj-$(CONFIG_SA1100_WATCHDOG) += sa1100_wdt.o
 obj-$(CONFIG_SAMA5D4_WATCHDOG) += sama5d4_wdt.o
+obj-$(CONFIG_SAM9X60_WATCHDOG) += sam9x60_wdt.o
 obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
 obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
 obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
diff --git a/drivers/watchdog/sam9x60_wdt.c b/drivers/watchdog/sam9x60_wdt.c
new file mode 100644
index 00000000..f612230
--- /dev/null
+++ b/drivers/watchdog/sam9x60_wdt.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Microchip SAM9X60 Watchdog Timer
+ *
+ * Copyright (C) 2019 Microchip Technology, Inc.
+ * Author: Eugen Hristev <eugen.hristev@microchip.com>
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
+#define		AT91_WDT_WDRSTT		BIT(0)		/* Restart */
+#define		AT91_WDT_KEY		(0xa5 << 24)		/* KEY Password */
+
+#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
+#define		AT91_WDT_PERIODRST	BIT(4)		/* Period Reset */
+#define		AT91_WDT_RPTHRST	BIT(5)		/* Minimum Restart Period */
+#define		AT91_WDT_WDDIS		BIT(12)		/* Disable */
+#define		AT91_WDT_WDDBGHLT	BIT(28)		/* Debug Halt */
+#define		AT91_WDT_WDIDLEHLT	BIT(29)		/* Idle Halt */
+
+#define AT91_WDT_VR		0x08			/* Watchdog Timer Value Register */
+
+#define AT91_WDT_WLR		0x0c
+#define		AT91_WDT_COUNTER	(0xfff << 0)		/* Watchdog Period Value */
+#define		AT91_WDT_SET_COUNTER(x)	((x) & AT91_WDT_COUNTER)
+
+#define AT91_WDT_IER		0x14			/* Interrupt Enable Register */
+#define		AT91_WDT_PERINT		BIT(0)		/* Period Interrupt Enable */
+#define AT91_WDT_IDR		0x18			/* Interrupt Disable Register */
+#define AT91_WDT_ISR		0x1c			/* Interrupt Status Register */
+
+/* minimum and maximum watchdog timeout, in seconds */
+#define MIN_WDT_TIMEOUT		1
+#define MAX_WDT_TIMEOUT		16
+#define WDT_DEFAULT_TIMEOUT	MAX_WDT_TIMEOUT
+
+#define WDT_SEC2TICKS(s)	((s) ? (((s) << 8) - 1) : 0)
+
+struct sam9x60_wdt {
+	struct watchdog_device	wdd;
+	void __iomem		*reg_base;
+	u32			mr;
+	u32			ir;
+	unsigned long		last_ping;
+};
+
+static int wdt_timeout;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_timeout, int, 0);
+MODULE_PARM_DESC(wdt_timeout,
+		 "Watchdog timeout in seconds. (default = "
+		 __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+		 "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
+
+#define wdt_read(wdt, field) \
+	readl_relaxed((wdt)->reg_base + (field))
+
+/* 4 slow clock periods is 4/32768 = 122.07us*/
+#define WDT_DELAY	usecs_to_jiffies(123)
+
+static void wdt_write(struct sam9x60_wdt *wdt, u32 field, u32 val)
+{
+	/*
+	 * WDT_CR and WDT_MR must not be modified within three slow clock
+	 * periods following a restart of the watchdog performed by a write
+	 * access in WDT_CR.
+	 */
+	while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
+		usleep_range(30, 125);
+	writel_relaxed(val, wdt->reg_base + field);
+	wdt->last_ping = jiffies;
+}
+
+static void wdt_write_nosleep(struct sam9x60_wdt *wdt, u32 field, u32 val)
+{
+	if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
+		usleep_range(123, 250);
+	writel_relaxed(val, wdt->reg_base + field);
+	wdt->last_ping = jiffies;
+}
+
+static int sam9x60_wdt_start(struct watchdog_device *wdd)
+{
+	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	wdt->mr &= ~AT91_WDT_WDDIS;
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+	wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
+
+	return 0;
+}
+
+static int sam9x60_wdt_stop(struct watchdog_device *wdd)
+{
+	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	wdt->mr |= AT91_WDT_WDDIS;
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+	wdt_write_nosleep(wdt, AT91_WDT_IDR, wdt->ir);
+
+	return 0;
+}
+
+static int sam9x60_wdt_ping(struct watchdog_device *wdd)
+{
+	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+
+	return 0;
+}
+
+static int sam9x60_wdt_set_timeout(struct watchdog_device *wdd,
+				   unsigned int timeout)
+{
+	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	wdt_write(wdt, AT91_WDT_WLR,
+		  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(timeout)));
+
+	wdd->timeout = timeout;
+
+	return 0;
+}
+
+static const struct watchdog_info sam9x60_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
+	.identity = "Microchip SAM9X60 Watchdog",
+};
+
+static const struct watchdog_ops sam9x60_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = sam9x60_wdt_start,
+	.stop = sam9x60_wdt_stop,
+	.ping = sam9x60_wdt_ping,
+	.set_timeout = sam9x60_wdt_set_timeout,
+};
+
+static irqreturn_t sam9x60_wdt_irq_handler(int irq, void *dev_id)
+{
+	struct sam9x60_wdt *wdt = platform_get_drvdata(dev_id);
+
+	if (wdt_read(wdt, AT91_WDT_ISR)) {
+		pr_crit("Microchip Watchdog Software Reset\n");
+		emergency_restart();
+		pr_crit("Reboot didn't succeed\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int of_sam9x60_wdt_init(struct device_node *np, struct sam9x60_wdt *wdt)
+{
+	const char *tmp;
+
+	wdt->mr = AT91_WDT_WDDIS;
+
+	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
+	    !strcmp(tmp, "software"))
+		wdt->ir = AT91_WDT_PERINT;
+	else
+		wdt->mr |= AT91_WDT_PERIODRST;
+
+	if (of_property_read_bool(np, "atmel,idle-halt"))
+		wdt->mr |= AT91_WDT_WDIDLEHLT;
+
+	if (of_property_read_bool(np, "atmel,dbg-halt"))
+		wdt->mr |= AT91_WDT_WDDBGHLT;
+
+	return 0;
+}
+
+static int sam9x60_wdt_init(struct sam9x60_wdt *wdt)
+{
+	u32 reg;
+	/*
+	 * When booting and resuming, the bootloader may have changed the
+	 * watchdog configuration.
+	 * If the watchdog is already running, we can safely update it.
+	 * Else, we have to disable it properly.
+	 */
+	if (wdt_enabled) {
+		wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
+		wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
+		wdt_write(wdt, AT91_WDT_WLR,
+			  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT)));
+
+	} else {
+		reg = wdt_read(wdt, AT91_WDT_MR);
+		if (!(reg & AT91_WDT_WDDIS))
+			wdt_write_nosleep(wdt, AT91_WDT_MR,
+					  reg | AT91_WDT_WDDIS);
+	}
+	return 0;
+}
+
+static int sam9x60_wdt_probe(struct platform_device *pdev)
+{
+	struct watchdog_device *wdd;
+	struct sam9x60_wdt *wdt;
+	struct resource *res;
+	void __iomem *regs;
+	u32 irq = 0;
+	int ret;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdd = &wdt->wdd;
+	wdd->timeout = WDT_DEFAULT_TIMEOUT;
+	wdd->info = &sam9x60_wdt_info;
+	wdd->ops = &sam9x60_wdt_ops;
+	wdd->min_timeout = MIN_WDT_TIMEOUT;
+	wdd->max_timeout = MAX_WDT_TIMEOUT;
+	wdt->last_ping = jiffies;
+
+	watchdog_set_drvdata(wdd, wdt);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	wdt->reg_base = regs;
+
+	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (!irq)
+		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
+
+	ret = of_sam9x60_wdt_init(pdev->dev.of_node, wdt);
+	if (ret)
+		return ret;
+
+	if ((wdt->ir & AT91_WDT_PERINT) && irq) {
+		ret = devm_request_irq(&pdev->dev, irq, sam9x60_wdt_irq_handler,
+				       IRQF_SHARED | IRQF_IRQPOLL |
+				       IRQF_NO_SUSPEND, pdev->name, pdev);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"cannot register interrupt handler\n");
+			return ret;
+		}
+	}
+
+	watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev);
+
+	ret = sam9x60_wdt_init(wdt);
+	if (ret)
+		return ret;
+
+	watchdog_set_nowayout(wdd, nowayout);
+
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register watchdog device\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+
+	dev_info(&pdev->dev, "initialized (timeout = %d sec, nowayout = %d)\n",
+		 wdd->timeout, nowayout);
+
+	return 0;
+}
+
+static int sam9x60_wdt_remove(struct platform_device *pdev)
+{
+	struct sam9x60_wdt *wdt = platform_get_drvdata(pdev);
+
+	sam9x60_wdt_stop(&wdt->wdd);
+
+	watchdog_unregister_device(&wdt->wdd);
+
+	return 0;
+}
+
+static const struct of_device_id sam9x60_wdt_of_match[] = {
+	{ .compatible = "microchip,sam9x60-wdt", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sam9x60_wdt_of_match);
+
+#ifdef CONFIG_PM_SLEEP
+static int sam9x60_wdt_resume(struct device *dev)
+{
+	struct sam9x60_wdt *wdt = dev_get_drvdata(dev);
+
+	/*
+	 * FIXME: writing MR also pings the watchdog which may not be desired.
+	 * This should only be done when the registers are lost on suspend but
+	 * there is no way to get this information right now.
+	 */
+	sam9x60_wdt_init(wdt);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(sam9x60_wdt_pm_ops, NULL,
+			 sam9x60_wdt_resume);
+
+static struct platform_driver sam9x60_wdt_driver = {
+	.probe		= sam9x60_wdt_probe,
+	.remove		= sam9x60_wdt_remove,
+	.driver		= {
+		.name	= "sam9x60_wdt",
+		.pm	= &sam9x60_wdt_pm_ops,
+		.of_match_table = sam9x60_wdt_of_match,
+	}
+};
+module_platform_driver(sam9x60_wdt_driver);
+
+MODULE_AUTHOR("Eugen Hristev");
+MODULE_DESCRIPTION("Microchip SAM9X60 Watchdog Timer driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH 3/3] MAINTAINERS: add sam9x60_wdt

[Eugen.Hristev]

From: Eugen Hristev <eugen.hristev@microchip.com>

Add sam9x60_wdt watchdog driver to at91 soc support.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 296de2b..109b030 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1972,6 +1972,7 @@ F:	arch/arm/boot/dts/sama*.dtsi
 F:	arch/arm/include/debug/at91.S
 F:	drivers/memory/atmel*
 F:	drivers/watchdog/sama5d4_wdt.c
+F:	drivers/watchdog/sam9x60_wdt.c
 X:	drivers/input/touchscreen/atmel_mxt_ts.c
 X:	drivers/net/wireless/atmel/
 
-- 
2.7.4

Re: [PATCH 1/3] dt-bindings: watchdog: sam9x60_wdt: add bindings

[Alexandre Belloni]

On 02/10/2019 07:35:23+0000, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> Add bindings for Microchip SAM9X60 Watchdog Timer
> 
> It has the same bindings as
> Documentation/devicetree/bindings/watchdog/atmel-sama5d4-wdt.txt
> except the compatible.
> 

Maybe it can then use the same documentation file. However, I think you
should already use the yaml dt bindings schema instead of a simple text
file.

> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  .../devicetree/bindings/watchdog/sam9x60-wdt.txt   | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt b/Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt
> new file mode 100644
> index 00000000..74b4e2d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/sam9x60-wdt.txt
> @@ -0,0 +1,34 @@
> +* Microchip SAM9X60 Watchdog Timer (WDT) Controller
> +
> +Required properties:
> +- compatible: "microchip,sam9x60-wdt"
> +- reg: base physical address and length of memory mapped region.
> +
> +Optional properties:
> +- timeout-sec: watchdog timeout value (in seconds).
> +- interrupts: interrupt number to the CPU.
> +- atmel,watchdog-type: should be "hardware" or "software".
> +	"hardware": enable watchdog fault reset. A watchdog fault triggers
> +		    watchdog reset.
> +	"software": enable watchdog fault interrupt. A watchdog fault asserts
> +		    watchdog interrupt.
> +- atmel,idle-halt: present if you want to stop the watchdog when the CPU is
> +		   in idle state.
> +	CAUTION: This property should be used with care, it actually makes the
> +	watchdog not counting when the CPU is in idle state, therefore the
> +	watchdog reset time depends on mean CPU usage and will not reset at all
> +	if the CPU stop working while it is in idle state, which is probably
> +	not what you want.
> +- atmel,dbg-halt: present if you want to stop the watchdog when the CPU is
> +		  in debug state.
> +
> +Example:
> +	watchdog@ffffff80 {
> +		compatible = "microchip,sam9x60-wdt";
> +		reg = <0xffffff80 0x24>;
> +		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 5>;
> +		timeout-sec = <10>;
> +		atmel,watchdog-type = "hardware";
> +		atmel,dbg-halt;
> +		atmel,idle-halt;
> +	};
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Alexandre Belloni]

Hi,

On 02/10/2019 07:35:26+0000, Eugen.Hristev@microchip.com wrote:
> +static void wdt_write(struct sam9x60_wdt *wdt, u32 field, u32 val)
> +{
> +	/*
> +	 * WDT_CR and WDT_MR must not be modified within three slow clock
> +	 * periods following a restart of the watchdog performed by a write
> +	 * access in WDT_CR.
> +	 */
> +	while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> +		usleep_range(30, 125);
> +	writel_relaxed(val, wdt->reg_base + field);
> +	wdt->last_ping = jiffies;
> +}
> +
> +static void wdt_write_nosleep(struct sam9x60_wdt *wdt, u32 field, u32 val)
> +{
> +	if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> +		usleep_range(123, 250);

So you have a _nosleep function that does sleep?

> +	writel_relaxed(val, wdt->reg_base + field);
> +	wdt->last_ping = jiffies;
> +}
> +
> +static int sam9x60_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt->mr &= ~AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +	wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);

I don't think AT91_WDT_IER needs to be protected, you can probably write
it directly. Also, you certainly need to do that before starting the
watchdog to avoid race conditions.

> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt->mr |= AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +	wdt_write_nosleep(wdt, AT91_WDT_IDR, wdt->ir);
> +

I don't think AT91_WDT_IDR needs to be protected.

> +	return 0;
> +}
> +
> +static int sam9x60_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_set_timeout(struct watchdog_device *wdd,
> +				   unsigned int timeout)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt_write(wdt, AT91_WDT_WLR,
> +		  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(timeout)));
> +

I don't think AT91_WDT_WLR needs to be protected.

> +	wdd->timeout = timeout;
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info sam9x60_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
> +	.identity = "Microchip SAM9X60 Watchdog",
> +};
> +
> +static const struct watchdog_ops sam9x60_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = sam9x60_wdt_start,
> +	.stop = sam9x60_wdt_stop,
> +	.ping = sam9x60_wdt_ping,
> +	.set_timeout = sam9x60_wdt_set_timeout,
> +};
> +
> +static irqreturn_t sam9x60_wdt_irq_handler(int irq, void *dev_id)
> +{
> +	struct sam9x60_wdt *wdt = platform_get_drvdata(dev_id);
> +
> +	if (wdt_read(wdt, AT91_WDT_ISR)) {
> +		pr_crit("Microchip Watchdog Software Reset\n");
> +		emergency_restart();
> +		pr_crit("Reboot didn't succeed\n");
> +	}

I'm not really convinced by the software restart use case but I guess it
is to be able to shut down while still flushing data to the storage.
This would not protect against kernel issues then.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int of_sam9x60_wdt_init(struct device_node *np, struct sam9x60_wdt *wdt)
> +{
> +	const char *tmp;
> +
> +	wdt->mr = AT91_WDT_WDDIS;
> +
> +	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> +	    !strcmp(tmp, "software"))
> +		wdt->ir = AT91_WDT_PERINT;
> +	else
> +		wdt->mr |= AT91_WDT_PERIODRST;
> +
> +	if (of_property_read_bool(np, "atmel,idle-halt"))
> +		wdt->mr |= AT91_WDT_WDIDLEHLT;
> +
> +	if (of_property_read_bool(np, "atmel,dbg-halt"))
> +		wdt->mr |= AT91_WDT_WDDBGHLT;
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_init(struct sam9x60_wdt *wdt)
> +{
> +	u32 reg;
> +	/*
> +	 * When booting and resuming, the bootloader may have changed the
> +	 * watchdog configuration.
> +	 * If the watchdog is already running, we can safely update it.
> +	 * Else, we have to disable it properly.
> +	 */
> +	if (wdt_enabled) {
> +		wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> +		wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
> +		wdt_write(wdt, AT91_WDT_WLR,
> +			  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT)));
> +
> +	} else {
> +		reg = wdt_read(wdt, AT91_WDT_MR);
> +		if (!(reg & AT91_WDT_WDDIS))
> +			wdt_write_nosleep(wdt, AT91_WDT_MR,
> +					  reg | AT91_WDT_WDDIS);
> +	}
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_probe(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wdd;
> +	struct sam9x60_wdt *wdt;
> +	struct resource *res;
> +	void __iomem *regs;
> +	u32 irq = 0;
> +	int ret;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	wdd = &wdt->wdd;
> +	wdd->timeout = WDT_DEFAULT_TIMEOUT;
> +	wdd->info = &sam9x60_wdt_info;
> +	wdd->ops = &sam9x60_wdt_ops;
> +	wdd->min_timeout = MIN_WDT_TIMEOUT;
> +	wdd->max_timeout = MAX_WDT_TIMEOUT;
> +	wdt->last_ping = jiffies;
> +
> +	watchdog_set_drvdata(wdd, wdt);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	wdt->reg_base = regs;
> +
> +	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +	if (!irq)
> +		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> +
> +	ret = of_sam9x60_wdt_init(pdev->dev.of_node, wdt);
> +	if (ret)
> +		return ret;
> +
> +	if ((wdt->ir & AT91_WDT_PERINT) && irq) {
> +		ret = devm_request_irq(&pdev->dev, irq, sam9x60_wdt_irq_handler,
> +				       IRQF_SHARED | IRQF_IRQPOLL |
> +				       IRQF_NO_SUSPEND, pdev->name, pdev);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"cannot register interrupt handler\n");
> +			return ret;
> +		}
> +	}
> +
> +	watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev);
> +
> +	ret = sam9x60_wdt_init(wdt);
> +	if (ret)
> +		return ret;
> +
> +	watchdog_set_nowayout(wdd, nowayout);
> +
> +	ret = watchdog_register_device(wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register watchdog device\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	dev_info(&pdev->dev, "initialized (timeout = %d sec, nowayout = %d)\n",
> +		 wdd->timeout, nowayout);
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_remove(struct platform_device *pdev)
> +{
> +	struct sam9x60_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	sam9x60_wdt_stop(&wdt->wdd);
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sam9x60_wdt_of_match[] = {
> +	{ .compatible = "microchip,sam9x60-wdt", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sam9x60_wdt_of_match);
> +
> +#ifdef CONFIG_PM_SLEEP

Most of the logic has been copy/pasted from sama5d4_wdt.c and this
already miss some improvement that have been made between the time you
copied it and now.

Are you sure both drivers shouldn't be merged? I feel like this will be a
maintenance hell if we don't do that now.

> +static int sam9x60_wdt_resume(struct device *dev)
> +{
> +	struct sam9x60_wdt *wdt = dev_get_drvdata(dev);
> +
> +	/*
> +	 * FIXME: writing MR also pings the watchdog which may not be desired.
> +	 * This should only be done when the registers are lost on suspend but
> +	 * there is no way to get this information right now.
> +	 */
> +	sam9x60_wdt_init(wdt);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(sam9x60_wdt_pm_ops, NULL,
> +			 sam9x60_wdt_resume);
> +
> +static struct platform_driver sam9x60_wdt_driver = {
> +	.probe		= sam9x60_wdt_probe,
> +	.remove		= sam9x60_wdt_remove,
> +	.driver		= {
> +		.name	= "sam9x60_wdt",
> +		.pm	= &sam9x60_wdt_pm_ops,
> +		.of_match_table = sam9x60_wdt_of_match,
> +	}
> +};
> +module_platform_driver(sam9x60_wdt_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev");
> +MODULE_DESCRIPTION("Microchip SAM9X60 Watchdog Timer driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Eugen.Hristev]

On 02.10.2019 13:23, Alexandre Belloni wrote:

> Hi,
> 
> On 02/10/2019 07:35:26+0000, Eugen.Hristev@microchip.com wrote:
>> +static void wdt_write(struct sam9x60_wdt *wdt, u32 field, u32 val)
>> +{
>> +	/*
>> +	 * WDT_CR and WDT_MR must not be modified within three slow clock
>> +	 * periods following a restart of the watchdog performed by a write
>> +	 * access in WDT_CR.
>> +	 */
>> +	while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
>> +		usleep_range(30, 125);
>> +	writel_relaxed(val, wdt->reg_base + field);
>> +	wdt->last_ping = jiffies;
>> +}
>> +
>> +static void wdt_write_nosleep(struct sam9x60_wdt *wdt, u32 field, u32 val)
>> +{
>> +	if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
>> +		usleep_range(123, 250);
> 
> So you have a _nosleep function that does sleep?
> 
>> +	writel_relaxed(val, wdt->reg_base + field);
>> +	wdt->last_ping = jiffies;
>> +}
>> +
>> +static int sam9x60_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	wdt->mr &= ~AT91_WDT_WDDIS;
>> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>> +	wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
> 
> I don't think AT91_WDT_IER needs to be protected, you can probably write
> it directly. Also, you certainly need to do that before starting the
> watchdog to avoid race conditions.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int sam9x60_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	wdt->mr |= AT91_WDT_WDDIS;
>> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>> +	wdt_write_nosleep(wdt, AT91_WDT_IDR, wdt->ir);
>> +
> 
> I don't think AT91_WDT_IDR needs to be protected.
> 
>> +	return 0;
>> +}
>> +
>> +static int sam9x60_wdt_ping(struct watchdog_device *wdd)
>> +{
>> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sam9x60_wdt_set_timeout(struct watchdog_device *wdd,
>> +				   unsigned int timeout)
>> +{
>> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	wdt_write(wdt, AT91_WDT_WLR,
>> +		  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(timeout)));
>> +
> 
> I don't think AT91_WDT_WLR needs to be protected.
> 
>> +	wdd->timeout = timeout;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct watchdog_info sam9x60_wdt_info = {
>> +	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
>> +	.identity = "Microchip SAM9X60 Watchdog",
>> +};
>> +
>> +static const struct watchdog_ops sam9x60_wdt_ops = {
>> +	.owner = THIS_MODULE,
>> +	.start = sam9x60_wdt_start,
>> +	.stop = sam9x60_wdt_stop,
>> +	.ping = sam9x60_wdt_ping,
>> +	.set_timeout = sam9x60_wdt_set_timeout,
>> +};
>> +
>> +static irqreturn_t sam9x60_wdt_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct sam9x60_wdt *wdt = platform_get_drvdata(dev_id);
>> +
>> +	if (wdt_read(wdt, AT91_WDT_ISR)) {
>> +		pr_crit("Microchip Watchdog Software Reset\n");
>> +		emergency_restart();
>> +		pr_crit("Reboot didn't succeed\n");
>> +	}
> 
> I'm not really convinced by the software restart use case but I guess it
> is to be able to shut down while still flushing data to the storage.
> This would not protect against kernel issues then.

Hi Alexandre,

That's correct. It is to do a software shutdown instead of hard reboot 
by hardware. It has it;s use cases, so I preserved the same level of 
functionality as in sama5d4_wdt

> 
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int of_sam9x60_wdt_init(struct device_node *np, struct sam9x60_wdt *wdt)
>> +{
>> +	const char *tmp;
>> +
>> +	wdt->mr = AT91_WDT_WDDIS;
>> +
>> +	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>> +	    !strcmp(tmp, "software"))
>> +		wdt->ir = AT91_WDT_PERINT;
>> +	else
>> +		wdt->mr |= AT91_WDT_PERIODRST;
>> +
>> +	if (of_property_read_bool(np, "atmel,idle-halt"))
>> +		wdt->mr |= AT91_WDT_WDIDLEHLT;
>> +
>> +	if (of_property_read_bool(np, "atmel,dbg-halt"))
>> +		wdt->mr |= AT91_WDT_WDDBGHLT;
>> +
>> +	return 0;
>> +}
>> +
>> +static int sam9x60_wdt_init(struct sam9x60_wdt *wdt)
>> +{
>> +	u32 reg;
>> +	/*
>> +	 * When booting and resuming, the bootloader may have changed the
>> +	 * watchdog configuration.
>> +	 * If the watchdog is already running, we can safely update it.
>> +	 * Else, we have to disable it properly.
>> +	 */
>> +	if (wdt_enabled) {
>> +		wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
>> +		wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
>> +		wdt_write(wdt, AT91_WDT_WLR,
>> +			  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT)));
>> +
>> +	} else {
>> +		reg = wdt_read(wdt, AT91_WDT_MR);
>> +		if (!(reg & AT91_WDT_WDDIS))
>> +			wdt_write_nosleep(wdt, AT91_WDT_MR,
>> +					  reg | AT91_WDT_WDDIS);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int sam9x60_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct watchdog_device *wdd;
>> +	struct sam9x60_wdt *wdt;
>> +	struct resource *res;
>> +	void __iomem *regs;
>> +	u32 irq = 0;
>> +	int ret;
>> +
>> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>> +	if (!wdt)
>> +		return -ENOMEM;
>> +
>> +	wdd = &wdt->wdd;
>> +	wdd->timeout = WDT_DEFAULT_TIMEOUT;
>> +	wdd->info = &sam9x60_wdt_info;
>> +	wdd->ops = &sam9x60_wdt_ops;
>> +	wdd->min_timeout = MIN_WDT_TIMEOUT;
>> +	wdd->max_timeout = MAX_WDT_TIMEOUT;
>> +	wdt->last_ping = jiffies;
>> +
>> +	watchdog_set_drvdata(wdd, wdt);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +
>> +	wdt->reg_base = regs;
>> +
>> +	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>> +	if (!irq)
>> +		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
>> +
>> +	ret = of_sam9x60_wdt_init(pdev->dev.of_node, wdt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if ((wdt->ir & AT91_WDT_PERINT) && irq) {
>> +		ret = devm_request_irq(&pdev->dev, irq, sam9x60_wdt_irq_handler,
>> +				       IRQF_SHARED | IRQF_IRQPOLL |
>> +				       IRQF_NO_SUSPEND, pdev->name, pdev);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +				"cannot register interrupt handler\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev);
>> +
>> +	ret = sam9x60_wdt_init(wdt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	watchdog_set_nowayout(wdd, nowayout);
>> +
>> +	ret = watchdog_register_device(wdd);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to register watchdog device\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, wdt);
>> +
>> +	dev_info(&pdev->dev, "initialized (timeout = %d sec, nowayout = %d)\n",
>> +		 wdd->timeout, nowayout);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sam9x60_wdt_remove(struct platform_device *pdev)
>> +{
>> +	struct sam9x60_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> +	sam9x60_wdt_stop(&wdt->wdd);
>> +
>> +	watchdog_unregister_device(&wdt->wdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id sam9x60_wdt_of_match[] = {
>> +	{ .compatible = "microchip,sam9x60-wdt", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sam9x60_wdt_of_match);
>> +
>> +#ifdef CONFIG_PM_SLEEP
> 
> Most of the logic has been copy/pasted from sama5d4_wdt.c and this
> already miss some improvement that have been made between the time you
> copied it and now.

I will fix accordingly. As I said in the commit message, sama5d4_wdt is 
used as a starting point so yes, all the functionality is the same, 
except the actual hardware interaction.

> 
> Are you sure both drivers shouldn't be merged? I feel like this will be a
> maintenance hell if we don't do that now.

It could be merged, but we should do so ?
Could have two compatibles, with platform data, selectable, and with 
different functions, that can be selected.. either this or that.
You think that's a better way to handle this new IP block ?
I would like to avoid having a big driver covering multiple different 
hardware pieces, but that's just my preference. I can rework this into a 
single driver if it's better that way.

Eugen

> 
>> +static int sam9x60_wdt_resume(struct device *dev)
>> +{
>> +	struct sam9x60_wdt *wdt = dev_get_drvdata(dev);
>> +
>> +	/*
>> +	 * FIXME: writing MR also pings the watchdog which may not be desired.
>> +	 * This should only be done when the registers are lost on suspend but
>> +	 * there is no way to get this information right now.
>> +	 */
>> +	sam9x60_wdt_init(wdt);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(sam9x60_wdt_pm_ops, NULL,
>> +			 sam9x60_wdt_resume);
>> +
>> +static struct platform_driver sam9x60_wdt_driver = {
>> +	.probe		= sam9x60_wdt_probe,
>> +	.remove		= sam9x60_wdt_remove,
>> +	.driver		= {
>> +		.name	= "sam9x60_wdt",
>> +		.pm	= &sam9x60_wdt_pm_ops,
>> +		.of_match_table = sam9x60_wdt_of_match,
>> +	}
>> +};
>> +module_platform_driver(sam9x60_wdt_driver);
>> +
>> +MODULE_AUTHOR("Eugen Hristev");
>> +MODULE_DESCRIPTION("Microchip SAM9X60 Watchdog Timer driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.7.4
>>
> 

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Guenter Roeck]

On 10/2/19 12:35 AM, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> This is the driver for SAM9X60 watchdog timer.
> The offered functionality is the same as sama5d4_wdt.
> The difference comes in register map, way to configure the timeout and
> interrupts.
> Developed starting from sama5d4_wdt.c
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>   drivers/watchdog/Kconfig       |   9 ++
>   drivers/watchdog/Makefile      |   1 +
>   drivers/watchdog/sam9x60_wdt.c | 335 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 345 insertions(+)
>   create mode 100644 drivers/watchdog/sam9x60_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 58e7c10..3562e26 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -416,6 +416,15 @@ config SAMA5D4_WATCHDOG
>   	  Its Watchdog Timer Mode Register can be written more than once.
>   	  This will reboot your system when the timeout is reached.
>   
> +config SAM9X60_WATCHDOG
> +	tristate "Microchip SAM9X60 Watchdog Timer"
> +	depends on ARCH_AT91 || COMPILE_TEST

depends on HAS_IOMEM

> +	select WATCHDOG_CORE
> +	help
> +	  Microchip SAM9X60 watchdog timer is embedded into SAM9X60 chips.
> +	  Its Watchdog Timer Mode Register can be written more than once.
> +	  This will reboot your system when the timeout is reached.
> +
>   config CADENCE_WATCHDOG
>   	tristate "Cadence Watchdog Timer"
>   	depends on HAS_IOMEM
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 2ee352b..93ba599 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
>   obj-$(CONFIG_S3C2410_WATCHDOG) += s3c2410_wdt.o
>   obj-$(CONFIG_SA1100_WATCHDOG) += sa1100_wdt.o
>   obj-$(CONFIG_SAMA5D4_WATCHDOG) += sama5d4_wdt.o
> +obj-$(CONFIG_SAM9X60_WATCHDOG) += sam9x60_wdt.o
>   obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
>   obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
>   obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
> diff --git a/drivers/watchdog/sam9x60_wdt.c b/drivers/watchdog/sam9x60_wdt.c
> new file mode 100644
> index 00000000..f612230
> --- /dev/null
> +++ b/drivers/watchdog/sam9x60_wdt.c
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Microchip SAM9X60 Watchdog Timer
> + *
> + * Copyright (C) 2019 Microchip Technology, Inc.
> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +
> +#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
> +#define		AT91_WDT_WDRSTT		BIT(0)		/* Restart */
> +#define		AT91_WDT_KEY		(0xa5 << 24)		/* KEY Password */
> +
> +#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
> +#define		AT91_WDT_PERIODRST	BIT(4)		/* Period Reset */
> +#define		AT91_WDT_RPTHRST	BIT(5)		/* Minimum Restart Period */
> +#define		AT91_WDT_WDDIS		BIT(12)		/* Disable */
> +#define		AT91_WDT_WDDBGHLT	BIT(28)		/* Debug Halt */
> +#define		AT91_WDT_WDIDLEHLT	BIT(29)		/* Idle Halt */
> +
> +#define AT91_WDT_VR		0x08			/* Watchdog Timer Value Register */
> +
> +#define AT91_WDT_WLR		0x0c
> +#define		AT91_WDT_COUNTER	(0xfff << 0)		/* Watchdog Period Value */
> +#define		AT91_WDT_SET_COUNTER(x)	((x) & AT91_WDT_COUNTER)
> +
> +#define AT91_WDT_IER		0x14			/* Interrupt Enable Register */
> +#define		AT91_WDT_PERINT		BIT(0)		/* Period Interrupt Enable */
> +#define AT91_WDT_IDR		0x18			/* Interrupt Disable Register */
> +#define AT91_WDT_ISR		0x1c			/* Interrupt Status Register */
> +
> +/* minimum and maximum watchdog timeout, in seconds */
> +#define MIN_WDT_TIMEOUT		1
> +#define MAX_WDT_TIMEOUT		16
> +#define WDT_DEFAULT_TIMEOUT	MAX_WDT_TIMEOUT
> +
> +#define WDT_SEC2TICKS(s)	((s) ? (((s) << 8) - 1) : 0)
> +
> +struct sam9x60_wdt {
> +	struct watchdog_device	wdd;
> +	void __iomem		*reg_base;
> +	u32			mr;
> +	u32			ir;
> +	unsigned long		last_ping;
> +};
> +
> +static int wdt_timeout;
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +
> +module_param(wdt_timeout, int, 0);
> +MODULE_PARM_DESC(wdt_timeout,
> +		 "Watchdog timeout in seconds. (default = "
> +		 __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
> +
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +		 "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))

Please no use in variables in macros without referencing them in the argument.

> +
> +#define wdt_read(wdt, field) \
> +	readl_relaxed((wdt)->reg_base + (field))
> +
> +/* 4 slow clock periods is 4/32768 = 122.07us*/
> +#define WDT_DELAY	usecs_to_jiffies(123)
> +
> +static void wdt_write(struct sam9x60_wdt *wdt, u32 field, u32 val)
> +{
> +	/*
> +	 * WDT_CR and WDT_MR must not be modified within three slow clock
> +	 * periods following a restart of the watchdog performed by a write
> +	 * access in WDT_CR.
> +	 */
> +	while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> +		usleep_range(30, 125);
> +	writel_relaxed(val, wdt->reg_base + field);
> +	wdt->last_ping = jiffies;
> +}
> +
> +static void wdt_write_nosleep(struct sam9x60_wdt *wdt, u32 field, u32 val)
> +{
> +	if (time_before(jiffies, wdt->last_ping + WDT_DELAY))

WDT_DELAY is most likely 1 under all circumstances. If the last access was just before
a tick, this won't guarantee that really 123 uS expired.

In situations like this, I would suggest not to rely on jiffies related functions.
If the last access time is stored based on ktime_get(), the remaining time in
us can be calculated using ktime_us_delta(). Then just sleep (or delay in case
of nosleep) for that amount of microseconds.

Also, I don't see why this second function would be necessary.
A single function with an if() should be sufficient.

	ktime_t delta = WDT_DELAY_US - ktime_us_delta(ktime_get(), wdt->last_ping);

	if (delta > 0)
		usleep_range(delta, delta * 2);
	writel_relaxed(val, wdt->reg_base + field);
	wdt->last_ping = ktime_get();

> +		usleep_range(123, 250);
> +	writel_relaxed(val, wdt->reg_base + field);
> +	wdt->last_ping = jiffies;
> +}
> +
> +static int sam9x60_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt->mr &= ~AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +	wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt->mr |= AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +	wdt_write_nosleep(wdt, AT91_WDT_IDR, wdt->ir);
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_set_timeout(struct watchdog_device *wdd,
> +				   unsigned int timeout)
> +{
> +	struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	wdt_write(wdt, AT91_WDT_WLR,
> +		  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(timeout)));
> +
> +	wdd->timeout = timeout;
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info sam9x60_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
> +	.identity = "Microchip SAM9X60 Watchdog",
> +};
> +
> +static const struct watchdog_ops sam9x60_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = sam9x60_wdt_start,
> +	.stop = sam9x60_wdt_stop,
> +	.ping = sam9x60_wdt_ping,
> +	.set_timeout = sam9x60_wdt_set_timeout,
> +};
> +
> +static irqreturn_t sam9x60_wdt_irq_handler(int irq, void *dev_id)
> +{
> +	struct sam9x60_wdt *wdt = platform_get_drvdata(dev_id);
> +
> +	if (wdt_read(wdt, AT91_WDT_ISR)) {
> +		pr_crit("Microchip Watchdog Software Reset\n");
> +		emergency_restart();
> +		pr_crit("Reboot didn't succeed\n");
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int of_sam9x60_wdt_init(struct device_node *np, struct sam9x60_wdt *wdt)
> +{
> +	const char *tmp;
> +
> +	wdt->mr = AT91_WDT_WDDIS;
> +
> +	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> +	    !strcmp(tmp, "software"))
> +		wdt->ir = AT91_WDT_PERINT;
> +	else
> +		wdt->mr |= AT91_WDT_PERIODRST;
> +
> +	if (of_property_read_bool(np, "atmel,idle-halt"))
> +		wdt->mr |= AT91_WDT_WDIDLEHLT;
> +
> +	if (of_property_read_bool(np, "atmel,dbg-halt"))
> +		wdt->mr |= AT91_WDT_WDDBGHLT;
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_init(struct sam9x60_wdt *wdt)
> +{
> +	u32 reg;
> +	/*
> +	 * When booting and resuming, the bootloader may have changed the
> +	 * watchdog configuration.
> +	 * If the watchdog is already running, we can safely update it.
> +	 * Else, we have to disable it properly.
> +	 */
> +	if (wdt_enabled) {
> +		wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
> +		wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
> +		wdt_write(wdt, AT91_WDT_WLR,
> +			  AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT)));
> +
> +	} else {
> +		reg = wdt_read(wdt, AT91_WDT_MR);
> +		if (!(reg & AT91_WDT_WDDIS))
> +			wdt_write_nosleep(wdt, AT91_WDT_MR,
> +					  reg | AT91_WDT_WDDIS);
> +	}

If the watchdog may be running at boot time, the watchdog core
should be informed about it.

> +	return 0;
> +}
> +
> +static int sam9x60_wdt_probe(struct platform_device *pdev)
> +{
> +	struct watchdog_device *wdd;
> +	struct sam9x60_wdt *wdt;
> +	struct resource *res;
> +	void __iomem *regs;
> +	u32 irq = 0;

Unnecessary initialization.

> +	int ret;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	wdd = &wdt->wdd;
> +	wdd->timeout = WDT_DEFAULT_TIMEOUT;
> +	wdd->info = &sam9x60_wdt_info;
> +	wdd->ops = &sam9x60_wdt_ops;
> +	wdd->min_timeout = MIN_WDT_TIMEOUT;
> +	wdd->max_timeout = MAX_WDT_TIMEOUT;
> +	wdt->last_ping = jiffies;
> +
> +	watchdog_set_drvdata(wdd, wdt);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	regs = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	wdt->reg_base = regs;
> +
> +	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +	if (!irq)
> +		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> +

The interrupt property is optional. Not providing it does not warrant a warning.

> +	ret = of_sam9x60_wdt_init(pdev->dev.of_node, wdt);
> +	if (ret)
> +		return ret;
> +
> +	if ((wdt->ir & AT91_WDT_PERINT) && irq) {

... even more so if it isn't actually used in some cases. irq_of_parse_and_map()
should probably only be called in the first place if AT91_WDT_PERINT is set.

> +		ret = devm_request_irq(&pdev->dev, irq, sam9x60_wdt_irq_handler,
> +				       IRQF_SHARED | IRQF_IRQPOLL |
> +				       IRQF_NO_SUSPEND, pdev->name, pdev);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"cannot register interrupt handler\n");
> +			return ret;
> +		}
> +	}
> +
> +	watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev);
> +
> +	ret = sam9x60_wdt_init(wdt);
> +	if (ret)
> +		return ret;
> +
> +	watchdog_set_nowayout(wdd, nowayout);
> +
> +	ret = watchdog_register_device(wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register watchdog device\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	dev_info(&pdev->dev, "initialized (timeout = %d sec, nowayout = %d)\n",
> +		 wdd->timeout, nowayout);
> +
> +	return 0;
> +}
> +
> +static int sam9x60_wdt_remove(struct platform_device *pdev)
> +{
> +	struct sam9x60_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	sam9x60_wdt_stop(&wdt->wdd);
> +

Please use watchdog_stop_on_unregister() instead.

> +	watchdog_unregister_device(&wdt->wdd);
> +

Please use devm_watchdog_register_device() and drop the remove function.

> +	return 0;
> +}
> +
> +static const struct of_device_id sam9x60_wdt_of_match[] = {
> +	{ .compatible = "microchip,sam9x60-wdt", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sam9x60_wdt_of_match);
> +
> +#ifdef CONFIG_PM_SLEEP

Please use __maybe_unused

> +static int sam9x60_wdt_resume(struct device *dev)
> +{
> +	struct sam9x60_wdt *wdt = dev_get_drvdata(dev);
> +
> +	/*
> +	 * FIXME: writing MR also pings the watchdog which may not be desired.
> +	 * This should only be done when the registers are lost on suspend but
> +	 * there is no way to get this information right now.
> +	 */

This seems wrong. The bootloader may have stopped the watchdog in the
suspend/resume cycle. This does not properly re-initialize it.

Also, is there really no need to stop the watchdog on suspend ?

> +	sam9x60_wdt_init(wdt);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(sam9x60_wdt_pm_ops, NULL,
> +			 sam9x60_wdt_resume);
> +
> +static struct platform_driver sam9x60_wdt_driver = {
> +	.probe		= sam9x60_wdt_probe,
> +	.remove		= sam9x60_wdt_remove,
> +	.driver		= {
> +		.name	= "sam9x60_wdt",
> +		.pm	= &sam9x60_wdt_pm_ops,
> +		.of_match_table = sam9x60_wdt_of_match,
> +	}
> +};
> +module_platform_driver(sam9x60_wdt_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev");
> +MODULE_DESCRIPTION("Microchip SAM9X60 Watchdog Timer driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5670 bytes --]

Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Eugen-Hristev-microchip-com/dt-bindings-watchdog-sam9x60_wdt-add-bindings/20191002-200155
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers//watchdog/sam9x60_wdt.c: In function 'sam9x60_wdt_ping':
>> drivers//watchdog/sam9x60_wdt.c:23:24: warning: large integer implicitly truncated to unsigned type [-Woverflow]
    #define  AT91_WDT_KEY  (0xa5 << 24)  /* KEY Password */
                           ^
>> drivers//watchdog/sam9x60_wdt.c:126:30: note: in expansion of macro 'AT91_WDT_KEY'
     wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
                                 ^~~~~~~~~~~~

vim +/AT91_WDT_KEY +126 drivers//watchdog/sam9x60_wdt.c

    20	
    21	#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
    22	#define		AT91_WDT_WDRSTT		BIT(0)		/* Restart */
  > 23	#define		AT91_WDT_KEY		(0xa5 << 24)		/* KEY Password */
    24	
    25	#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
    26	#define		AT91_WDT_PERIODRST	BIT(4)		/* Period Reset */
    27	#define		AT91_WDT_RPTHRST	BIT(5)		/* Minimum Restart Period */
    28	#define		AT91_WDT_WDDIS		BIT(12)		/* Disable */
    29	#define		AT91_WDT_WDDBGHLT	BIT(28)		/* Debug Halt */
    30	#define		AT91_WDT_WDIDLEHLT	BIT(29)		/* Idle Halt */
    31	
    32	#define AT91_WDT_VR		0x08			/* Watchdog Timer Value Register */
    33	
    34	#define AT91_WDT_WLR		0x0c
    35	#define		AT91_WDT_COUNTER	(0xfff << 0)		/* Watchdog Period Value */
    36	#define		AT91_WDT_SET_COUNTER(x)	((x) & AT91_WDT_COUNTER)
    37	
    38	#define AT91_WDT_IER		0x14			/* Interrupt Enable Register */
    39	#define		AT91_WDT_PERINT		BIT(0)		/* Period Interrupt Enable */
    40	#define AT91_WDT_IDR		0x18			/* Interrupt Disable Register */
    41	#define AT91_WDT_ISR		0x1c			/* Interrupt Status Register */
    42	
    43	/* minimum and maximum watchdog timeout, in seconds */
    44	#define MIN_WDT_TIMEOUT		1
    45	#define MAX_WDT_TIMEOUT		16
    46	#define WDT_DEFAULT_TIMEOUT	MAX_WDT_TIMEOUT
    47	
    48	#define WDT_SEC2TICKS(s)	((s) ? (((s) << 8) - 1) : 0)
    49	
    50	struct sam9x60_wdt {
    51		struct watchdog_device	wdd;
    52		void __iomem		*reg_base;
    53		u32			mr;
    54		u32			ir;
    55		unsigned long		last_ping;
    56	};
    57	
    58	static int wdt_timeout;
    59	static bool nowayout = WATCHDOG_NOWAYOUT;
    60	
    61	module_param(wdt_timeout, int, 0);
    62	MODULE_PARM_DESC(wdt_timeout,
    63			 "Watchdog timeout in seconds. (default = "
    64			 __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
    65	
    66	module_param(nowayout, bool, 0);
    67	MODULE_PARM_DESC(nowayout,
    68			 "Watchdog cannot be stopped once started (default="
    69			 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
    70	
    71	#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
    72	
    73	#define wdt_read(wdt, field) \
    74		readl_relaxed((wdt)->reg_base + (field))
    75	
    76	/* 4 slow clock periods is 4/32768 = 122.07us*/
    77	#define WDT_DELAY	usecs_to_jiffies(123)
    78	
    79	static void wdt_write(struct sam9x60_wdt *wdt, u32 field, u32 val)
    80	{
    81		/*
    82		 * WDT_CR and WDT_MR must not be modified within three slow clock
    83		 * periods following a restart of the watchdog performed by a write
    84		 * access in WDT_CR.
    85		 */
    86		while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
    87			usleep_range(30, 125);
    88		writel_relaxed(val, wdt->reg_base + field);
    89		wdt->last_ping = jiffies;
    90	}
    91	
    92	static void wdt_write_nosleep(struct sam9x60_wdt *wdt, u32 field, u32 val)
    93	{
    94		if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
    95			usleep_range(123, 250);
    96		writel_relaxed(val, wdt->reg_base + field);
    97		wdt->last_ping = jiffies;
    98	}
    99	
   100	static int sam9x60_wdt_start(struct watchdog_device *wdd)
   101	{
   102		struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
   103	
   104		wdt->mr &= ~AT91_WDT_WDDIS;
   105		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
   106		wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
   107	
   108		return 0;
   109	}
   110	
   111	static int sam9x60_wdt_stop(struct watchdog_device *wdd)
   112	{
   113		struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
   114	
   115		wdt->mr |= AT91_WDT_WDDIS;
   116		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
   117		wdt_write_nosleep(wdt, AT91_WDT_IDR, wdt->ir);
   118	
   119		return 0;
   120	}
   121	
   122	static int sam9x60_wdt_ping(struct watchdog_device *wdd)
   123	{
   124		struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
   125	
 > 126		wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
   127	
   128		return 0;
   129	}
   130	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54932 bytes --]

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Eugen.Hristev]

On 02.10.2019 16:16, Guenter Roeck wrote:

> 
> On 10/2/19 12:35 AM, Eugen.Hristev@microchip.com wrote:
>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> This is the driver for SAM9X60 watchdog timer.
>> The offered functionality is the same as sama5d4_wdt.
>> The difference comes in register map, way to configure the timeout and
>> interrupts.
>> Developed starting from sama5d4_wdt.c
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   drivers/watchdog/Kconfig       |   9 ++
>>   drivers/watchdog/Makefile      |   1 +
>>   drivers/watchdog/sam9x60_wdt.c | 335 
>> +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 345 insertions(+)
>>   create mode 100644 drivers/watchdog/sam9x60_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 58e7c10..3562e26 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -416,6 +416,15 @@ config SAMA5D4_WATCHDOG
>>         Its Watchdog Timer Mode Register can be written more than once.
>>         This will reboot your system when the timeout is reached.
>> +config SAM9X60_WATCHDOG
>> +    tristate "Microchip SAM9X60 Watchdog Timer"
>> +    depends on ARCH_AT91 || COMPILE_TEST
> 
> depends on HAS_IOMEM
> 
>> +    select WATCHDOG_CORE
>> +    help
>> +      Microchip SAM9X60 watchdog timer is embedded into SAM9X60 chips.
>> +      Its Watchdog Timer Mode Register can be written more than once.
>> +      This will reboot your system when the timeout is reached.
>> +
>>   config CADENCE_WATCHDOG
>>       tristate "Cadence Watchdog Timer"
>>       depends on HAS_IOMEM
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 2ee352b..93ba599 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -52,6 +52,7 @@ obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
>>   obj-$(CONFIG_S3C2410_WATCHDOG) += s3c2410_wdt.o
>>   obj-$(CONFIG_SA1100_WATCHDOG) += sa1100_wdt.o
>>   obj-$(CONFIG_SAMA5D4_WATCHDOG) += sama5d4_wdt.o
>> +obj-$(CONFIG_SAM9X60_WATCHDOG) += sam9x60_wdt.o
>>   obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
>>   obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
>>   obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
>> diff --git a/drivers/watchdog/sam9x60_wdt.c 
>> b/drivers/watchdog/sam9x60_wdt.c
>> new file mode 100644
>> index 00000000..f612230
>> --- /dev/null
>> +++ b/drivers/watchdog/sam9x60_wdt.c
>> @@ -0,0 +1,335 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for Microchip SAM9X60 Watchdog Timer
>> + *
>> + * Copyright (C) 2019 Microchip Technology, Inc.
>> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
>> + *
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> +#include <linux/watchdog.h>
>> +
>> +#define AT91_WDT_CR        0x00            /* Watchdog Control 
>> Register */
>> +#define        AT91_WDT_WDRSTT        BIT(0)        /* Restart */
>> +#define        AT91_WDT_KEY        (0xa5 << 24)        /* KEY 
>> Password */
>> +
>> +#define AT91_WDT_MR        0x04            /* Watchdog Mode Register */
>> +#define        AT91_WDT_PERIODRST    BIT(4)        /* Period Reset */
>> +#define        AT91_WDT_RPTHRST    BIT(5)        /* Minimum Restart 
>> Period */
>> +#define        AT91_WDT_WDDIS        BIT(12)        /* Disable */
>> +#define        AT91_WDT_WDDBGHLT    BIT(28)        /* Debug Halt */
>> +#define        AT91_WDT_WDIDLEHLT    BIT(29)        /* Idle Halt */
>> +
>> +#define AT91_WDT_VR        0x08            /* Watchdog Timer Value 
>> Register */
>> +
>> +#define AT91_WDT_WLR        0x0c
>> +#define        AT91_WDT_COUNTER    (0xfff << 0)        /* Watchdog 
>> Period Value */
>> +#define        AT91_WDT_SET_COUNTER(x)    ((x) & AT91_WDT_COUNTER)
>> +
>> +#define AT91_WDT_IER        0x14            /* Interrupt Enable 
>> Register */
>> +#define        AT91_WDT_PERINT        BIT(0)        /* Period 
>> Interrupt Enable */
>> +#define AT91_WDT_IDR        0x18            /* Interrupt Disable 
>> Register */
>> +#define AT91_WDT_ISR        0x1c            /* Interrupt Status 
>> Register */
>> +
>> +/* minimum and maximum watchdog timeout, in seconds */
>> +#define MIN_WDT_TIMEOUT        1
>> +#define MAX_WDT_TIMEOUT        16
>> +#define WDT_DEFAULT_TIMEOUT    MAX_WDT_TIMEOUT
>> +
>> +#define WDT_SEC2TICKS(s)    ((s) ? (((s) << 8) - 1) : 0)
>> +
>> +struct sam9x60_wdt {
>> +    struct watchdog_device    wdd;
>> +    void __iomem        *reg_base;
>> +    u32            mr;
>> +    u32            ir;
>> +    unsigned long        last_ping;
>> +};
>> +
>> +static int wdt_timeout;
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +
>> +module_param(wdt_timeout, int, 0);
>> +MODULE_PARM_DESC(wdt_timeout,
>> +         "Watchdog timeout in seconds. (default = "
>> +         __MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
>> +
>> +module_param(nowayout, bool, 0);
>> +MODULE_PARM_DESC(nowayout,
>> +         "Watchdog cannot be stopped once started (default="
>> +         __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +
>> +#define wdt_enabled (!(wdt->mr & AT91_WDT_WDDIS))
> 
> Please no use in variables in macros without referencing them in the 
> argument.
> 
>> +
>> +#define wdt_read(wdt, field) \
>> +    readl_relaxed((wdt)->reg_base + (field))
>> +
>> +/* 4 slow clock periods is 4/32768 = 122.07us*/
>> +#define WDT_DELAY    usecs_to_jiffies(123)
>> +
>> +static void wdt_write(struct sam9x60_wdt *wdt, u32 field, u32 val)
>> +{
>> +    /*
>> +     * WDT_CR and WDT_MR must not be modified within three slow clock
>> +     * periods following a restart of the watchdog performed by a write
>> +     * access in WDT_CR.
>> +     */
>> +    while (time_before(jiffies, wdt->last_ping + WDT_DELAY))
>> +        usleep_range(30, 125);
>> +    writel_relaxed(val, wdt->reg_base + field);
>> +    wdt->last_ping = jiffies;
>> +}
>> +
>> +static void wdt_write_nosleep(struct sam9x60_wdt *wdt, u32 field, u32 
>> val)
>> +{
>> +    if (time_before(jiffies, wdt->last_ping + WDT_DELAY))
> 
> WDT_DELAY is most likely 1 under all circumstances. If the last access 
> was just before
> a tick, this won't guarantee that really 123 uS expired.
> 
> In situations like this, I would suggest not to rely on jiffies related 
> functions.
> If the last access time is stored based on ktime_get(), the remaining 
> time in
> us can be calculated using ktime_us_delta(). Then just sleep (or delay 
> in case
> of nosleep) for that amount of microseconds.
> 
> Also, I don't see why this second function would be necessary.
> A single function with an if() should be sufficient.
> 
>      ktime_t delta = WDT_DELAY_US - ktime_us_delta(ktime_get(), 
> wdt->last_ping);
> 
>      if (delta > 0)
>          usleep_range(delta, delta * 2);
>      writel_relaxed(val, wdt->reg_base + field);
>      wdt->last_ping = ktime_get();
> 
>> +        usleep_range(123, 250);
>> +    writel_relaxed(val, wdt->reg_base + field);
>> +    wdt->last_ping = jiffies;
>> +}
>> +
>> +static int sam9x60_wdt_start(struct watchdog_device *wdd)
>> +{
>> +    struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +    wdt->mr &= ~AT91_WDT_WDDIS;
>> +    wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>> +    wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sam9x60_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +    struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +    wdt->mr |= AT91_WDT_WDDIS;
>> +    wdt_write(wdt, AT91_WDT_MR, wdt->mr);
>> +    wdt_write_nosleep(wdt, AT91_WDT_IDR, wdt->ir);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sam9x60_wdt_ping(struct watchdog_device *wdd)
>> +{
>> +    struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +    wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sam9x60_wdt_set_timeout(struct watchdog_device *wdd,
>> +                   unsigned int timeout)
>> +{
>> +    struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> +    wdt_write(wdt, AT91_WDT_WLR,
>> +          AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(timeout)));
>> +
>> +    wdd->timeout = timeout;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct watchdog_info sam9x60_wdt_info = {
>> +    .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | 
>> WDIOF_KEEPALIVEPING,
>> +    .identity = "Microchip SAM9X60 Watchdog",
>> +};
>> +
>> +static const struct watchdog_ops sam9x60_wdt_ops = {
>> +    .owner = THIS_MODULE,
>> +    .start = sam9x60_wdt_start,
>> +    .stop = sam9x60_wdt_stop,
>> +    .ping = sam9x60_wdt_ping,
>> +    .set_timeout = sam9x60_wdt_set_timeout,
>> +};
>> +
>> +static irqreturn_t sam9x60_wdt_irq_handler(int irq, void *dev_id)
>> +{
>> +    struct sam9x60_wdt *wdt = platform_get_drvdata(dev_id);
>> +
>> +    if (wdt_read(wdt, AT91_WDT_ISR)) {
>> +        pr_crit("Microchip Watchdog Software Reset\n");
>> +        emergency_restart();
>> +        pr_crit("Reboot didn't succeed\n");
>> +    }
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int of_sam9x60_wdt_init(struct device_node *np, struct 
>> sam9x60_wdt *wdt)
>> +{
>> +    const char *tmp;
>> +
>> +    wdt->mr = AT91_WDT_WDDIS;
>> +
>> +    if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>> +        !strcmp(tmp, "software"))
>> +        wdt->ir = AT91_WDT_PERINT;
>> +    else
>> +        wdt->mr |= AT91_WDT_PERIODRST;
>> +
>> +    if (of_property_read_bool(np, "atmel,idle-halt"))
>> +        wdt->mr |= AT91_WDT_WDIDLEHLT;
>> +
>> +    if (of_property_read_bool(np, "atmel,dbg-halt"))
>> +        wdt->mr |= AT91_WDT_WDDBGHLT;
>> +
>> +    return 0;
>> +}
>> +
>> +static int sam9x60_wdt_init(struct sam9x60_wdt *wdt)
>> +{
>> +    u32 reg;
>> +    /*
>> +     * When booting and resuming, the bootloader may have changed the
>> +     * watchdog configuration.
>> +     * If the watchdog is already running, we can safely update it.
>> +     * Else, we have to disable it properly.
>> +     */
>> +    if (wdt_enabled) {
>> +        wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr);
>> +        wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir);
>> +        wdt_write(wdt, AT91_WDT_WLR,
>> +              AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT)));
>> +
>> +    } else {
>> +        reg = wdt_read(wdt, AT91_WDT_MR);
>> +        if (!(reg & AT91_WDT_WDDIS))
>> +            wdt_write_nosleep(wdt, AT91_WDT_MR,
>> +                      reg | AT91_WDT_WDDIS);
>> +    }
> 
> If the watchdog may be running at boot time, the watchdog core
> should be informed about it.
> 
>> +    return 0;
>> +}
>> +
>> +static int sam9x60_wdt_probe(struct platform_device *pdev)
>> +{
>> +    struct watchdog_device *wdd;
>> +    struct sam9x60_wdt *wdt;
>> +    struct resource *res;
>> +    void __iomem *regs;
>> +    u32 irq = 0;
> 
> Unnecessary initialization.
> 
>> +    int ret;
>> +
>> +    wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>> +    if (!wdt)
>> +        return -ENOMEM;
>> +
>> +    wdd = &wdt->wdd;
>> +    wdd->timeout = WDT_DEFAULT_TIMEOUT;
>> +    wdd->info = &sam9x60_wdt_info;
>> +    wdd->ops = &sam9x60_wdt_ops;
>> +    wdd->min_timeout = MIN_WDT_TIMEOUT;
>> +    wdd->max_timeout = MAX_WDT_TIMEOUT;
>> +    wdt->last_ping = jiffies;
>> +
>> +    watchdog_set_drvdata(wdd, wdt);
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    regs = devm_ioremap_resource(&pdev->dev, res);
> 
> devm_platform_ioremap_resource()
> 
>> +    if (IS_ERR(regs))
>> +        return PTR_ERR(regs);
>> +
>> +    wdt->reg_base = regs;
>> +
>> +    irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>> +    if (!irq)
>> +        dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
>> +
> 
> The interrupt property is optional. Not providing it does not warrant a 
> warning.
> 
>> +    ret = of_sam9x60_wdt_init(pdev->dev.of_node, wdt);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if ((wdt->ir & AT91_WDT_PERINT) && irq) {
> 
> ... even more so if it isn't actually used in some cases. 
> irq_of_parse_and_map()
> should probably only be called in the first place if AT91_WDT_PERINT is 
> set.
> 
>> +        ret = devm_request_irq(&pdev->dev, irq, sam9x60_wdt_irq_handler,
>> +                       IRQF_SHARED | IRQF_IRQPOLL |
>> +                       IRQF_NO_SUSPEND, pdev->name, pdev);
>> +        if (ret) {
>> +            dev_err(&pdev->dev,
>> +                "cannot register interrupt handler\n");
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev);
>> +
>> +    ret = sam9x60_wdt_init(wdt);
>> +    if (ret)
>> +        return ret;
>> +
>> +    watchdog_set_nowayout(wdd, nowayout);
>> +
>> +    ret = watchdog_register_device(wdd);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "failed to register watchdog device\n");
>> +        return ret;
>> +    }
>> +
>> +    platform_set_drvdata(pdev, wdt);
>> +
>> +    dev_info(&pdev->dev, "initialized (timeout = %d sec, nowayout = 
>> %d)\n",
>> +         wdd->timeout, nowayout);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sam9x60_wdt_remove(struct platform_device *pdev)
>> +{
>> +    struct sam9x60_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> +    sam9x60_wdt_stop(&wdt->wdd);
>> +
> 
> Please use watchdog_stop_on_unregister() instead.
> 
>> +    watchdog_unregister_device(&wdt->wdd);
>> +
> 
> Please use devm_watchdog_register_device() and drop the remove function.
> 
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id sam9x60_wdt_of_match[] = {
>> +    { .compatible = "microchip,sam9x60-wdt", },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(of, sam9x60_wdt_of_match);
>> +
>> +#ifdef CONFIG_PM_SLEEP
> 
> Please use __maybe_unused
> 
>> +static int sam9x60_wdt_resume(struct device *dev)
>> +{
>> +    struct sam9x60_wdt *wdt = dev_get_drvdata(dev);
>> +
>> +    /*
>> +     * FIXME: writing MR also pings the watchdog which may not be 
>> desired.
>> +     * This should only be done when the registers are lost on 
>> suspend but
>> +     * there is no way to get this information right now.
>> +     */
> 
> This seems wrong. The bootloader may have stopped the watchdog in the
> suspend/resume cycle. This does not properly re-initialize it.
> 
> Also, is there really no need to stop the watchdog on suspend ?
> 
>> +    sam9x60_wdt_init(wdt);
>> +
>> +    return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(sam9x60_wdt_pm_ops, NULL,
>> +             sam9x60_wdt_resume);
>> +
>> +static struct platform_driver sam9x60_wdt_driver = {
>> +    .probe        = sam9x60_wdt_probe,
>> +    .remove        = sam9x60_wdt_remove,
>> +    .driver        = {
>> +        .name    = "sam9x60_wdt",
>> +        .pm    = &sam9x60_wdt_pm_ops,
>> +        .of_match_table = sam9x60_wdt_of_match,
>> +    }
>> +};
>> +module_platform_driver(sam9x60_wdt_driver);
>> +
>> +MODULE_AUTHOR("Eugen Hristev");
>> +MODULE_DESCRIPTION("Microchip SAM9X60 Watchdog Timer driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 

Hello Guenter,

Thank you for the feedback.
After reviewing this, can you please guide me towards one of the 
possible two directions: merge this driver with sama5d4_wdt , and have a 
single driver with support for both hardware blocks; or, have this 
driver separately , as in this patch series?

According to this, I will work on the next version of the patches.

Thanks,

Eugen

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Guenter Roeck]

On 10/7/19 12:58 AM, Eugen.Hristev@microchip.com wrote:
[ ... ]
> Hello Guenter,
> 
> Thank you for the feedback.
> After reviewing this, can you please guide me towards one of the
> possible two directions: merge this driver with sama5d4_wdt , and have a
> single driver with support for both hardware blocks; or, have this
> driver separately , as in this patch series?
> 

I noticed the similarities. I don't know if it makes sense to reconcile
the two drivers; it seems to me the new chip uses the same basic core with
enhancements. In general, I prefer a single driver, but only if the result
doesn't end up being an if/else mess. Ultimately, it is really your call
to make.

Guenter

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Alexandre Belloni]

On 07/10/2019 05:36:38-0700, Guenter Roeck wrote:
> On 10/7/19 12:58 AM, Eugen.Hristev@microchip.com wrote:
> [ ... ]
> > Hello Guenter,
> > 
> > Thank you for the feedback.
> > After reviewing this, can you please guide me towards one of the
> > possible two directions: merge this driver with sama5d4_wdt , and have a
> > single driver with support for both hardware blocks; or, have this
> > driver separately , as in this patch series?
> > 
> 
> I noticed the similarities. I don't know if it makes sense to reconcile
> the two drivers; it seems to me the new chip uses the same basic core with
> enhancements. In general, I prefer a single driver, but only if the result
> doesn't end up being an if/else mess. Ultimately, it is really your call
> to make.
> 

Most if not all your comments were already addressed in the other
driver. The main difference in the register interface is the location of
the counter that only really affects sama5d4_wdt_set_timeout and that
could be abstracted away by using a different struct watchdog_ops.
Interrupt enabling is also done differently, I don't think it has a huge
impact.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver

[Eugen.Hristev]

On 07.10.2019 16:14, Alexandre Belloni wrote:

> 
> On 07/10/2019 05:36:38-0700, Guenter Roeck wrote:
>> On 10/7/19 12:58 AM, Eugen.Hristev@microchip.com wrote:
>> [ ... ]
>>> Hello Guenter,
>>>
>>> Thank you for the feedback.
>>> After reviewing this, can you please guide me towards one of the
>>> possible two directions: merge this driver with sama5d4_wdt , and have a
>>> single driver with support for both hardware blocks; or, have this
>>> driver separately , as in this patch series?
>>>
>>
>> I noticed the similarities. I don't know if it makes sense to reconcile
>> the two drivers; it seems to me the new chip uses the same basic core with
>> enhancements. In general, I prefer a single driver, but only if the result
>> doesn't end up being an if/else mess. Ultimately, it is really your call
>> to make.
>>
> 
> Most if not all your comments were already addressed in the other
> driver. The main difference in the register interface is the location of
> the counter that only really affects sama5d4_wdt_set_timeout and that
> could be abstracted away by using a different struct watchdog_ops.
> Interrupt enabling is also done differently, I don't think it has a huge
> impact.
> 

Thank you Guenter and Alexandre,

I will start working on a v2 with a merged driver.

Thanks again,
Eugen