[PATCH 0/2] watchdog: at91sam9_wdt: add new feature support

[PATCH 0/2] watchdog: at91sam9_wdt: add new feature support

[Wenyou Yang]

Hi Wim,

Atmel Watchdog Timer has a new feature from SAMA5D4, the Watchdog Timer Mode
Register can be written more than once, so the driver can enable/disable
the watchdog timer hardware and set the watchdog timer hardware timeout.

The patch set is to add new feature.

Wenyou Yang (2):
  drivers: watchdog: at91sam9_wdt: add new feature support
  Documentation: dt: binding: atmel-wdt: add a new compitable

 .../devicetree/bindings/watchdog/atmel-wdt.txt     |    4 +-
 drivers/watchdog/at91sam9_wdt.c                    |  255 ++++++++++++++------
 drivers/watchdog/at91sam9_wdt.h                    |    4 +
 3 files changed, 193 insertions(+), 70 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support

[Wenyou Yang]

In the datasheet, the new feature is describled as
"WDT_MR can be written until a LOCKMR command is issued in WDT_CR".
That is to say, as long as the bootstrap and u-boot don't issue a LOCKMR
command, WDT_MR can be written in kernel.

So the driver can enable/disable the watchdog timer hardware,
set WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields
of WDT_MR register to set the watchdog timer timeout.

The timer is not necessary that regularly sends a keepalive ping to
the watchdog timer hardware.

It is introduced from sama5d4 SoCs.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 drivers/watchdog/at91sam9_wdt.c |  255 ++++++++++++++++++++++++++++-----------
 drivers/watchdog/at91sam9_wdt.h |    4 +
 2 files changed, 190 insertions(+), 69 deletions(-)

diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
index 1443b3c..6b61084 100644
--- a/drivers/watchdog/at91sam9_wdt.c
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -10,9 +10,12 @@
  */
 
 /*
+ * For AT91SAM9x SoCs, the Watchdog Timer has the following constraint.
  * The Watchdog Timer Mode Register can be only written to once. If the
  * timeout need to be set from Linux, be sure that the bootstrap or the
  * bootloader doesn't write to this register.
+ * From SAMA5D4, the Watchdog Timer Mode Register can be written
+ * until a LOCKMR command is issued in WDT_CR.
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -80,6 +83,11 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 #define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
+
+struct at91wdt_variant {
+	bool mr_writable;
+};
+
 struct at91wdt {
 	struct watchdog_device wdd;
 	void __iomem *base;
@@ -90,6 +98,9 @@ struct at91wdt {
 	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
 	bool nowayout;
 	unsigned int irq;
+	bool use_timer;
+	bool enabled;
+	struct at91wdt_variant *drv_data;
 };
 
 /* ......................................................................... */
@@ -133,21 +144,67 @@ static void at91_ping(unsigned long data)
 static int at91_wdt_start(struct watchdog_device *wdd)
 {
 	struct at91wdt *wdt = to_wdt(wdd);
-	/* calculate when the next userspace timeout will be */
-	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
+	u32 reg;
+
+	if (wdt->drv_data->mr_writable) {
+		reg = wdt_read(wdt, AT91_WDT_MR);
+		reg &= ~AT91_WDT_WDDIS;
+		wdt_write(wdt, AT91_WDT_MR, reg);
+	} else {
+		/* calculate when the next userspace timeout will be */
+		wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
+	}
+
 	return 0;
 }
 
 static int at91_wdt_stop(struct watchdog_device *wdd)
 {
-	/* The watchdog timer hardware can not be stopped... */
+	struct at91wdt *wdt = to_wdt(wdd);
+	u32 reg;
+
+	if (wdt->drv_data->mr_writable) {
+		reg = wdt_read(wdt, AT91_WDT_MR);
+		reg |= AT91_WDT_WDDIS;
+		wdt_write(wdt, AT91_WDT_MR, reg);
+	}
+
+	return 0;
+}
+
+static int at91_wdt_ping(struct watchdog_device *wdd)
+{
+	struct at91wdt *wdt = to_wdt(wdd);
+
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+
 	return 0;
 }
 
 static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
 {
-	wdd->timeout = new_timeout;
-	return at91_wdt_start(wdd);
+	struct at91wdt *wdt = to_wdt(wdd);
+	u32 reg, timeout;
+
+	if (wdt->drv_data->mr_writable) {
+		timeout = secs_to_ticks(new_timeout);
+		if (timeout > WDT_COUNTER_MAX_TICKS)
+			return -EINVAL;
+
+		reg = wdt_read(wdt, AT91_WDT_MR);
+		reg &= ~AT91_WDT_WDV;
+		reg |= AT91_WDT_WDV_(timeout);
+		reg &= ~AT91_WDT_WDD;
+		reg |= AT91_WDT_WDD_(timeout);
+		wdt_write(wdt, AT91_WDT_MR, reg);
+
+		wdd->timeout = new_timeout;
+
+		return 0;
+	} else {
+		wdd->timeout = new_timeout;
+		return at91_wdt_start(wdd);
+	}
 }
 
 static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
@@ -161,50 +218,65 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	unsigned long max_heartbeat;
 	struct device *dev = &pdev->dev;
 
-	tmp = wdt_read(wdt, AT91_WDT_MR);
-	if ((tmp & mask) != (wdt->mr & mask)) {
-		if (tmp == WDT_MR_RESET) {
-			wdt_write(wdt, AT91_WDT_MR, wdt->mr);
-			tmp = wdt_read(wdt, AT91_WDT_MR);
+	if (wdt->drv_data->mr_writable) {
+		wdt->use_timer = false;
+
+		wdt_write(wdt, AT91_WDT_MR, wdt->mr | AT91_WDT_WDDIS);
+	} else {
+		wdt->use_timer = true;
+
+		tmp = wdt_read(wdt, AT91_WDT_MR);
+		if ((tmp & mask) != (wdt->mr & mask)) {
+			if (tmp == WDT_MR_RESET) {
+				wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+				tmp = wdt_read(wdt, AT91_WDT_MR);
+			}
 		}
-	}
 
-	if (tmp & AT91_WDT_WDDIS) {
-		if (wdt->mr & AT91_WDT_WDDIS)
-			return 0;
-		dev_err(dev, "watchdog is disabled\n");
-		return -EINVAL;
+		if (tmp & AT91_WDT_WDDIS) {
+			if (wdt->mr & AT91_WDT_WDDIS)
+				return 0;
+
+			dev_err(dev, "watchdog is disabled\n");
+			return -EINVAL;
+		}
 	}
 
-	value = tmp & AT91_WDT_WDV;
-	delta = (tmp & AT91_WDT_WDD) >> 16;
+	tmp = wdt_read(wdt, AT91_WDT_MR);
+	wdt->enabled = (tmp & AT91_WDT_WDDIS) ? false : true;
 
-	if (delta < value)
-		min_heartbeat = ticks_to_hz_roundup(value - delta);
+	if (wdt->use_timer) {
+		value = tmp & AT91_WDT_WDV;
+		delta = (tmp & AT91_WDT_WDD) >> 16;
 
-	max_heartbeat = ticks_to_hz_rounddown(value);
-	if (!max_heartbeat) {
-		dev_err(dev,
-			"heartbeat is too small for the system to handle it correctly\n");
-		return -EINVAL;
-	}
+		if (delta < value)
+			min_heartbeat = ticks_to_hz_roundup(value - delta);
 
-	/*
-	 * Try to reset the watchdog counter 4 or 2 times more often than
-	 * actually requested, to avoid spurious watchdog reset.
-	 * If this is not possible because of the min_heartbeat value, reset
-	 * it at the min_heartbeat period.
-	 */
-	if ((max_heartbeat / 4) >= min_heartbeat)
-		wdt->heartbeat = max_heartbeat / 4;
-	else if ((max_heartbeat / 2) >= min_heartbeat)
-		wdt->heartbeat = max_heartbeat / 2;
-	else
-		wdt->heartbeat = min_heartbeat;
-
-	if (max_heartbeat < min_heartbeat + 4)
-		dev_warn(dev,
-			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+		max_heartbeat = ticks_to_hz_rounddown(value);
+		if (!max_heartbeat) {
+			dev_err(dev,
+				"heartbeat is too small for the system to handle it correctly\n");
+			return -EINVAL;
+		}
+
+		/*
+		 * Try to reset the watchdog counter 4 or 2 times more often than
+		 * actually requested, to avoid spurious watchdog reset.
+		 * If this is not possible because of the min_heartbeat value, reset
+		 * it at the min_heartbeat period.
+		 */
+		if ((max_heartbeat / 4) >= min_heartbeat)
+			wdt->heartbeat = max_heartbeat / 4;
+		else if ((max_heartbeat / 2) >= min_heartbeat)
+			wdt->heartbeat = max_heartbeat / 2;
+		else
+			wdt->heartbeat = min_heartbeat;
+
+		if (max_heartbeat < min_heartbeat + 4) {
+			dev_warn(dev,
+				 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
+		}
+	}
 
 	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
 		err = request_irq(wdt->irq, wdt_interrupt,
@@ -215,21 +287,24 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 			return err;
 	}
 
-	if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
-		dev_warn(dev,
-			 "watchdog already configured differently (mr = %x expecting %x)\n",
-			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
+	if (wdt->use_timer) {
+		if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask)) {
+			dev_warn(dev,
+				 "watchdog already configured differently (mr = %x expecting %x)\n",
+				 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
+		}
 
-	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
+		setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
 
-	/*
-	 * Use min_heartbeat the first time to avoid spurious watchdog reset:
-	 * we don't know for how long the watchdog counter is running, and
-	 *  - resetting it right now might trigger a watchdog fault reset
-	 *  - waiting for heartbeat time might lead to a watchdog timeout
-	 *    reset
-	 */
-	mod_timer(&wdt->timer, jiffies + min_heartbeat);
+		/*
+		 * Use min_heartbeat the first time to avoid spurious watchdog reset:
+		 * we don't know for how long the watchdog counter is running, and
+		 *  - resetting it right now might trigger a watchdog fault reset
+		 *  - waiting for heartbeat time might lead to a watchdog timeout
+		 *    reset
+		 */
+		mod_timer(&wdt->timer, jiffies + min_heartbeat);
+	}
 
 	/* Try to set timeout from device tree first */
 	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
@@ -239,12 +314,14 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
 	if (err)
 		goto out_stop_timer;
 
-	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
+	if (wdt->use_timer)
+		wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
 
 	return 0;
 
 out_stop_timer:
-	del_timer(&wdt->timer);
+	if (wdt->use_timer)
+		del_timer(&wdt->timer);
 	return err;
 }
 
@@ -256,13 +333,54 @@ static const struct watchdog_info at91_wdt_info = {
 						WDIOF_MAGICCLOSE,
 };
 
-static const struct watchdog_ops at91_wdt_ops = {
+static struct watchdog_ops at91_wdt_ops = {
 	.owner =	THIS_MODULE,
 	.start =	at91_wdt_start,
 	.stop =		at91_wdt_stop,
 	.set_timeout =	at91_wdt_set_timeout,
 };
 
+static const struct at91wdt_variant drv_data_at91sam9260 = {
+	.mr_writable = false,
+};
+
+#if defined(CONFIG_OF)
+static const struct at91wdt_variant drv_data_sama5d4 = {
+	.mr_writable = true,
+};
+
+static const struct of_device_id at91_wdt_dt_ids[] = {
+	{ .compatible = "atmel,at91sam9260-wdt",
+	  .data = &drv_data_at91sam9260 },
+	{ .compatible = "atmel,sama5d4-wdt",
+	  .data = &drv_data_sama5d4 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids);
+#endif
+
+static const struct platform_device_id at91wdt_ids[] = {
+	{
+		.name = "at91_wdt",
+		.driver_data = (unsigned long)&drv_data_at91sam9260,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(platform, at91wdt_ids);
+
+static struct at91wdt_variant *at91wdt_get_drv_data(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	if (pdev->dev.of_node) {
+		match = of_match_node(at91_wdt_dt_ids, pdev->dev.of_node);
+		return (struct at91wdt_variant *)match->data;
+	} else {
+		return (struct at91wdt_variant *)
+			platform_get_device_id(pdev)->driver_data;
+	}
+}
+
 #if defined(CONFIG_OF)
 static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
 {
@@ -336,6 +454,10 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 	if (!wdt)
 		return -ENOMEM;
 
+	wdt->drv_data = at91wdt_get_drv_data(pdev);
+	if (wdt->drv_data->mr_writable)
+		at91_wdt_ops.ping = at91_wdt_ping;
+
 	wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
 		  AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
 	wdt->mr_mask = 0x3FFFFFFF;
@@ -364,8 +486,12 @@ static int __init at91wdt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, wdt);
 
-	pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
-		wdt->wdd.timeout, wdt->nowayout);
+	if (wdt->enabled) {
+		dev_info(&pdev->dev, "enabled (heartbeat=%d sec, nowayout=%d)\n",
+			wdt->wdd.timeout, wdt->nowayout);
+	} else {
+		dev_info(&pdev->dev, "not enabled\n");
+	}
 
 	return 0;
 }
@@ -381,15 +507,6 @@ static int __exit at91wdt_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#if defined(CONFIG_OF)
-static const struct of_device_id at91_wdt_dt_ids[] = {
-	{ .compatible = "atmel,at91sam9260-wdt" },
-	{ /* sentinel */ }
-};
-
-MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids);
-#endif
-
 static struct platform_driver at91wdt_driver = {
 	.remove		= __exit_p(at91wdt_remove),
 	.driver		= {
diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
index c6fbb2e6..79add4f 100644
--- a/drivers/watchdog/at91sam9_wdt.h
+++ b/drivers/watchdog/at91sam9_wdt.h
@@ -22,11 +22,15 @@
 
 #define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
 #define		AT91_WDT_WDV		(0xfff << 0)		/* Counter Value */
+#define			AT91_WDT_WDV_MSK	(0xfff)
+#define			AT91_WDT_WDV_(x)	(((x) & AT91_WDT_WDV_MSK) << 0)
 #define		AT91_WDT_WDFIEN		(1     << 12)		/* Fault Interrupt Enable */
 #define		AT91_WDT_WDRSTEN	(1     << 13)		/* Reset Processor */
 #define		AT91_WDT_WDRPROC	(1     << 14)		/* Timer Restart */
 #define		AT91_WDT_WDDIS		(1     << 15)		/* Watchdog Disable */
 #define		AT91_WDT_WDD		(0xfff << 16)		/* Delta Value */
+#define			AT91_WDT_WDD_MSK	(0xfff)
+#define			AT91_WDT_WDD_(x)	(((x) & AT91_WDT_WDD_MSK) << 16)
 #define		AT91_WDT_WDDBGHLT	(1     << 28)		/* Debug Halt */
 #define		AT91_WDT_WDIDLEHLT	(1     << 29)		/* Idle Halt */
 
-- 
1.7.9.5

Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support

[Guenter Roeck]

On 07/28/2015 12:00 AM, Wenyou Yang wrote:
> In the datasheet, the new feature is describled as
> "WDT_MR can be written until a LOCKMR command is issued in WDT_CR".
> That is to say, as long as the bootstrap and u-boot don't issue a LOCKMR
> command, WDT_MR can be written in kernel.
>
> So the driver can enable/disable the watchdog timer hardware,
> set WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields
> of WDT_MR register to set the watchdog timer timeout.
>
> The timer is not necessary that regularly sends a keepalive ping to
> the watchdog timer hardware.
>
> It is introduced from sama5d4 SoCs.
>
Since there are so many changes, I wonder is a separate driver would make more sense.

Thoughts ?

Guenter

> Signed-off-by: Wenyou Yang <wenyou.yang-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/watchdog/at91sam9_wdt.c |  255 ++++++++++++++++++++++++++++-----------
>   drivers/watchdog/at91sam9_wdt.h |    4 +
>   2 files changed, 190 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> index 1443b3c..6b61084 100644
> --- a/drivers/watchdog/at91sam9_wdt.c
> +++ b/drivers/watchdog/at91sam9_wdt.c
> @@ -10,9 +10,12 @@
>    */
>
>   /*
> + * For AT91SAM9x SoCs, the Watchdog Timer has the following constraint.
>    * The Watchdog Timer Mode Register can be only written to once. If the
>    * timeout need to be set from Linux, be sure that the bootstrap or the
>    * bootloader doesn't write to this register.
> + * From SAMA5D4, the Watchdog Timer Mode Register can be written
> + * until a LOCKMR command is issued in WDT_CR.
>    */
>
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -80,6 +83,11 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>   	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>
>   #define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
> +
> +struct at91wdt_variant {
> +	bool mr_writable;
> +};
> +
>   struct at91wdt {
>   	struct watchdog_device wdd;
>   	void __iomem *base;
> @@ -90,6 +98,9 @@ struct at91wdt {
>   	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
>   	bool nowayout;
>   	unsigned int irq;
> +	bool use_timer;
> +	bool enabled;
> +	struct at91wdt_variant *drv_data;
>   };
>
>   /* ......................................................................... */
> @@ -133,21 +144,67 @@ static void at91_ping(unsigned long data)
>   static int at91_wdt_start(struct watchdog_device *wdd)
>   {
>   	struct at91wdt *wdt = to_wdt(wdd);
> -	/* calculate when the next userspace timeout will be */
> -	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> +	u32 reg;
> +
> +	if (wdt->drv_data->mr_writable) {
> +		reg = wdt_read(wdt, AT91_WDT_MR);
> +		reg &= ~AT91_WDT_WDDIS;
> +		wdt_write(wdt, AT91_WDT_MR, reg);
> +	} else {
> +		/* calculate when the next userspace timeout will be */
> +		wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> +	}
> +
>   	return 0;
>   }
>
>   static int at91_wdt_stop(struct watchdog_device *wdd)
>   {
> -	/* The watchdog timer hardware can not be stopped... */
> +	struct at91wdt *wdt = to_wdt(wdd);
> +	u32 reg;
> +
> +	if (wdt->drv_data->mr_writable) {
> +		reg = wdt_read(wdt, AT91_WDT_MR);
> +		reg |= AT91_WDT_WDDIS;
> +		wdt_write(wdt, AT91_WDT_MR, reg);
> +	}
> +
> +	return 0;
> +}
> +
> +static int at91_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct at91wdt *wdt = to_wdt(wdd);
> +
> +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> +
>   	return 0;
>   }
>
>   static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int new_timeout)
>   {
> -	wdd->timeout = new_timeout;
> -	return at91_wdt_start(wdd);
> +	struct at91wdt *wdt = to_wdt(wdd);
> +	u32 reg, timeout;
> +
> +	if (wdt->drv_data->mr_writable) {
> +		timeout = secs_to_ticks(new_timeout);
> +		if (timeout > WDT_COUNTER_MAX_TICKS)
> +			return -EINVAL;
> +
> +		reg = wdt_read(wdt, AT91_WDT_MR);
> +		reg &= ~AT91_WDT_WDV;
> +		reg |= AT91_WDT_WDV_(timeout);
> +		reg &= ~AT91_WDT_WDD;
> +		reg |= AT91_WDT_WDD_(timeout);
> +		wdt_write(wdt, AT91_WDT_MR, reg);
> +
> +		wdd->timeout = new_timeout;
> +
> +		return 0;
> +	} else {
> +		wdd->timeout = new_timeout;
> +		return at91_wdt_start(wdd);
> +	}
>   }
>
>   static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> @@ -161,50 +218,65 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>   	unsigned long max_heartbeat;
>   	struct device *dev = &pdev->dev;
>
> -	tmp = wdt_read(wdt, AT91_WDT_MR);
> -	if ((tmp & mask) != (wdt->mr & mask)) {
> -		if (tmp == WDT_MR_RESET) {
> -			wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> -			tmp = wdt_read(wdt, AT91_WDT_MR);
> +	if (wdt->drv_data->mr_writable) {
> +		wdt->use_timer = false;
> +
> +		wdt_write(wdt, AT91_WDT_MR, wdt->mr | AT91_WDT_WDDIS);
> +	} else {
> +		wdt->use_timer = true;
> +
> +		tmp = wdt_read(wdt, AT91_WDT_MR);
> +		if ((tmp & mask) != (wdt->mr & mask)) {
> +			if (tmp == WDT_MR_RESET) {
> +				wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +				tmp = wdt_read(wdt, AT91_WDT_MR);
> +			}
>   		}
> -	}
>
> -	if (tmp & AT91_WDT_WDDIS) {
> -		if (wdt->mr & AT91_WDT_WDDIS)
> -			return 0;
> -		dev_err(dev, "watchdog is disabled\n");
> -		return -EINVAL;
> +		if (tmp & AT91_WDT_WDDIS) {
> +			if (wdt->mr & AT91_WDT_WDDIS)
> +				return 0;
> +
> +			dev_err(dev, "watchdog is disabled\n");
> +			return -EINVAL;
> +		}
>   	}
>
> -	value = tmp & AT91_WDT_WDV;
> -	delta = (tmp & AT91_WDT_WDD) >> 16;
> +	tmp = wdt_read(wdt, AT91_WDT_MR);
> +	wdt->enabled = (tmp & AT91_WDT_WDDIS) ? false : true;
>
> -	if (delta < value)
> -		min_heartbeat = ticks_to_hz_roundup(value - delta);
> +	if (wdt->use_timer) {
> +		value = tmp & AT91_WDT_WDV;
> +		delta = (tmp & AT91_WDT_WDD) >> 16;
>
> -	max_heartbeat = ticks_to_hz_rounddown(value);
> -	if (!max_heartbeat) {
> -		dev_err(dev,
> -			"heartbeat is too small for the system to handle it correctly\n");
> -		return -EINVAL;
> -	}
> +		if (delta < value)
> +			min_heartbeat = ticks_to_hz_roundup(value - delta);
>
> -	/*
> -	 * Try to reset the watchdog counter 4 or 2 times more often than
> -	 * actually requested, to avoid spurious watchdog reset.
> -	 * If this is not possible because of the min_heartbeat value, reset
> -	 * it at the min_heartbeat period.
> -	 */
> -	if ((max_heartbeat / 4) >= min_heartbeat)
> -		wdt->heartbeat = max_heartbeat / 4;
> -	else if ((max_heartbeat / 2) >= min_heartbeat)
> -		wdt->heartbeat = max_heartbeat / 2;
> -	else
> -		wdt->heartbeat = min_heartbeat;
> -
> -	if (max_heartbeat < min_heartbeat + 4)
> -		dev_warn(dev,
> -			 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> +		max_heartbeat = ticks_to_hz_rounddown(value);
> +		if (!max_heartbeat) {
> +			dev_err(dev,
> +				"heartbeat is too small for the system to handle it correctly\n");
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * Try to reset the watchdog counter 4 or 2 times more often than
> +		 * actually requested, to avoid spurious watchdog reset.
> +		 * If this is not possible because of the min_heartbeat value, reset
> +		 * it at the min_heartbeat period.
> +		 */
> +		if ((max_heartbeat / 4) >= min_heartbeat)
> +			wdt->heartbeat = max_heartbeat / 4;
> +		else if ((max_heartbeat / 2) >= min_heartbeat)
> +			wdt->heartbeat = max_heartbeat / 2;
> +		else
> +			wdt->heartbeat = min_heartbeat;
> +
> +		if (max_heartbeat < min_heartbeat + 4) {
> +			dev_warn(dev,
> +				 "min heartbeat and max heartbeat might be too close for the system to handle it correctly\n");
> +		}
> +	}
>
>   	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
>   		err = request_irq(wdt->irq, wdt_interrupt,
> @@ -215,21 +287,24 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>   			return err;
>   	}
>
> -	if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
> -		dev_warn(dev,
> -			 "watchdog already configured differently (mr = %x expecting %x)\n",
> -			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> +	if (wdt->use_timer) {
> +		if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask)) {
> +			dev_warn(dev,
> +				 "watchdog already configured differently (mr = %x expecting %x)\n",
> +				 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> +		}
>
> -	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> +		setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
>
> -	/*
> -	 * Use min_heartbeat the first time to avoid spurious watchdog reset:
> -	 * we don't know for how long the watchdog counter is running, and
> -	 *  - resetting it right now might trigger a watchdog fault reset
> -	 *  - waiting for heartbeat time might lead to a watchdog timeout
> -	 *    reset
> -	 */
> -	mod_timer(&wdt->timer, jiffies + min_heartbeat);
> +		/*
> +		 * Use min_heartbeat the first time to avoid spurious watchdog reset:
> +		 * we don't know for how long the watchdog counter is running, and
> +		 *  - resetting it right now might trigger a watchdog fault reset
> +		 *  - waiting for heartbeat time might lead to a watchdog timeout
> +		 *    reset
> +		 */
> +		mod_timer(&wdt->timer, jiffies + min_heartbeat);
> +	}
>
>   	/* Try to set timeout from device tree first */
>   	if (watchdog_init_timeout(&wdt->wdd, 0, dev))
> @@ -239,12 +314,14 @@ static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
>   	if (err)
>   		goto out_stop_timer;
>
> -	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
> +	if (wdt->use_timer)
> +		wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
>
>   	return 0;
>
>   out_stop_timer:
> -	del_timer(&wdt->timer);
> +	if (wdt->use_timer)
> +		del_timer(&wdt->timer);
>   	return err;
>   }
>
> @@ -256,13 +333,54 @@ static const struct watchdog_info at91_wdt_info = {
>   						WDIOF_MAGICCLOSE,
>   };
>
> -static const struct watchdog_ops at91_wdt_ops = {
> +static struct watchdog_ops at91_wdt_ops = {
>   	.owner =	THIS_MODULE,
>   	.start =	at91_wdt_start,
>   	.stop =		at91_wdt_stop,
>   	.set_timeout =	at91_wdt_set_timeout,
>   };
>
> +static const struct at91wdt_variant drv_data_at91sam9260 = {
> +	.mr_writable = false,
> +};
> +
> +#if defined(CONFIG_OF)
> +static const struct at91wdt_variant drv_data_sama5d4 = {
> +	.mr_writable = true,
> +};
> +
> +static const struct of_device_id at91_wdt_dt_ids[] = {
> +	{ .compatible = "atmel,at91sam9260-wdt",
> +	  .data = &drv_data_at91sam9260 },
> +	{ .compatible = "atmel,sama5d4-wdt",
> +	  .data = &drv_data_sama5d4 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids);
> +#endif
> +
> +static const struct platform_device_id at91wdt_ids[] = {
> +	{
> +		.name = "at91_wdt",
> +		.driver_data = (unsigned long)&drv_data_at91sam9260,
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, at91wdt_ids);
> +
> +static struct at91wdt_variant *at91wdt_get_drv_data(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +
> +	if (pdev->dev.of_node) {
> +		match = of_match_node(at91_wdt_dt_ids, pdev->dev.of_node);
> +		return (struct at91wdt_variant *)match->data;
> +	} else {
> +		return (struct at91wdt_variant *)
> +			platform_get_device_id(pdev)->driver_data;
> +	}
> +}
> +
>   #if defined(CONFIG_OF)
>   static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
>   {
> @@ -336,6 +454,10 @@ static int __init at91wdt_probe(struct platform_device *pdev)
>   	if (!wdt)
>   		return -ENOMEM;
>
> +	wdt->drv_data = at91wdt_get_drv_data(pdev);
> +	if (wdt->drv_data->mr_writable)
> +		at91_wdt_ops.ping = at91_wdt_ping;
> +
>   	wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN | AT91_WDT_WDD |
>   		  AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
>   	wdt->mr_mask = 0x3FFFFFFF;
> @@ -364,8 +486,12 @@ static int __init at91wdt_probe(struct platform_device *pdev)
>
>   	platform_set_drvdata(pdev, wdt);
>
> -	pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
> -		wdt->wdd.timeout, wdt->nowayout);
> +	if (wdt->enabled) {
> +		dev_info(&pdev->dev, "enabled (heartbeat=%d sec, nowayout=%d)\n",
> +			wdt->wdd.timeout, wdt->nowayout);
> +	} else {
> +		dev_info(&pdev->dev, "not enabled\n");
> +	}
>
>   	return 0;
>   }
> @@ -381,15 +507,6 @@ static int __exit at91wdt_remove(struct platform_device *pdev)
>   	return 0;
>   }
>
> -#if defined(CONFIG_OF)
> -static const struct of_device_id at91_wdt_dt_ids[] = {
> -	{ .compatible = "atmel,at91sam9260-wdt" },
> -	{ /* sentinel */ }
> -};
> -
> -MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids);
> -#endif
> -
>   static struct platform_driver at91wdt_driver = {
>   	.remove		= __exit_p(at91wdt_remove),
>   	.driver		= {
> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
> index c6fbb2e6..79add4f 100644
> --- a/drivers/watchdog/at91sam9_wdt.h
> +++ b/drivers/watchdog/at91sam9_wdt.h
> @@ -22,11 +22,15 @@
>
>   #define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
>   #define		AT91_WDT_WDV		(0xfff << 0)		/* Counter Value */
> +#define			AT91_WDT_WDV_MSK	(0xfff)
> +#define			AT91_WDT_WDV_(x)	(((x) & AT91_WDT_WDV_MSK) << 0)
>   #define		AT91_WDT_WDFIEN		(1     << 12)		/* Fault Interrupt Enable */
>   #define		AT91_WDT_WDRSTEN	(1     << 13)		/* Reset Processor */
>   #define		AT91_WDT_WDRPROC	(1     << 14)		/* Timer Restart */
>   #define		AT91_WDT_WDDIS		(1     << 15)		/* Watchdog Disable */
>   #define		AT91_WDT_WDD		(0xfff << 16)		/* Delta Value */
> +#define			AT91_WDT_WDD_MSK	(0xfff)
> +#define			AT91_WDT_WDD_(x)	(((x) & AT91_WDT_WDD_MSK) << 16)
>   #define		AT91_WDT_WDDBGHLT	(1     << 28)		/* Debug Halt */
>   #define		AT91_WDT_WDIDLEHLT	(1     << 29)		/* Idle Halt */
>
>

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

RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support

[Yang, Wenyou]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 15985 bytes --]

Hi Guenter,

Thank you very much for your review.

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: 2015Äê7ÔÂ28ÈÕ 15:14
> To: Yang, Wenyou; wim@iguana.be; robh+dt@kernel.org; pawel.moll@arm.com;
> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; galak@codeaurora.org
> Cc: sylvain.rochet@finsecur.com; Ferre, Nicolas; boris.brezillon@free-
> electrons.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature
> support
> 
> On 07/28/2015 12:00 AM, Wenyou Yang wrote:
> > In the datasheet, the new feature is describled as "WDT_MR can be
> > written until a LOCKMR command is issued in WDT_CR".
> > That is to say, as long as the bootstrap and u-boot don't issue a
> > LOCKMR command, WDT_MR can be written in kernel.
> >
> > So the driver can enable/disable the watchdog timer hardware, set
> > WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields of
> > WDT_MR register to set the watchdog timer timeout.
> >
> > The timer is not necessary that regularly sends a keepalive ping to
> > the watchdog timer hardware.
> >
> > It is introduced from sama5d4 SoCs.
> >
> Since there are so many changes, I wonder is a separate driver would make more
> sense.
Yes, a bit many changes.
I thought reuse the driver code.
If a separate driver, I am afraid it includes much duplicated code. 
After all, it is for the same device with different feature. 

I don't think it is necessary to have multiple drivers for the same peripheral with different feature.
 
> 
> Thoughts ?
> 
> Guenter
> 
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >   drivers/watchdog/at91sam9_wdt.c |  255
> ++++++++++++++++++++++++++++-----------
> >   drivers/watchdog/at91sam9_wdt.h |    4 +
> >   2 files changed, 190 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/watchdog/at91sam9_wdt.c
> > b/drivers/watchdog/at91sam9_wdt.c index 1443b3c..6b61084 100644
> > --- a/drivers/watchdog/at91sam9_wdt.c
> > +++ b/drivers/watchdog/at91sam9_wdt.c
> > @@ -10,9 +10,12 @@
> >    */
> >
> >   /*
> > + * For AT91SAM9x SoCs, the Watchdog Timer has the following constraint.
> >    * The Watchdog Timer Mode Register can be only written to once. If the
> >    * timeout need to be set from Linux, be sure that the bootstrap or the
> >    * bootloader doesn't write to this register.
> > + * From SAMA5D4, the Watchdog Timer Mode Register can be written
> > + * until a LOCKMR command is issued in WDT_CR.
> >    */
> >
> >   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -80,6 +83,11 @@
> > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
> "
> >   	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >
> >   #define to_wdt(wdd) container_of(wdd, struct at91wdt, wdd)
> > +
> > +struct at91wdt_variant {
> > +	bool mr_writable;
> > +};
> > +
> >   struct at91wdt {
> >   	struct watchdog_device wdd;
> >   	void __iomem *base;
> > @@ -90,6 +98,9 @@ struct at91wdt {
> >   	unsigned long heartbeat;	/* WDT heartbeat in jiffies */
> >   	bool nowayout;
> >   	unsigned int irq;
> > +	bool use_timer;
> > +	bool enabled;
> > +	struct at91wdt_variant *drv_data;
> >   };
> >
> >   /*
> > ......................................................................... */ @@ -133,21 +144,67 @@
> static void at91_ping(unsigned long data)
> >   static int at91_wdt_start(struct watchdog_device *wdd)
> >   {
> >   	struct at91wdt *wdt = to_wdt(wdd);
> > -	/* calculate when the next userspace timeout will be */
> > -	wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> > +	u32 reg;
> > +
> > +	if (wdt->drv_data->mr_writable) {
> > +		reg = wdt_read(wdt, AT91_WDT_MR);
> > +		reg &= ~AT91_WDT_WDDIS;
> > +		wdt_write(wdt, AT91_WDT_MR, reg);
> > +	} else {
> > +		/* calculate when the next userspace timeout will be */
> > +		wdt->next_heartbeat = jiffies + wdd->timeout * HZ;
> > +	}
> > +
> >   	return 0;
> >   }
> >
> >   static int at91_wdt_stop(struct watchdog_device *wdd)
> >   {
> > -	/* The watchdog timer hardware can not be stopped... */
> > +	struct at91wdt *wdt = to_wdt(wdd);
> > +	u32 reg;
> > +
> > +	if (wdt->drv_data->mr_writable) {
> > +		reg = wdt_read(wdt, AT91_WDT_MR);
> > +		reg |= AT91_WDT_WDDIS;
> > +		wdt_write(wdt, AT91_WDT_MR, reg);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int at91_wdt_ping(struct watchdog_device *wdd) {
> > +	struct at91wdt *wdt = to_wdt(wdd);
> > +
> > +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY |
> AT91_WDT_WDRSTT);
> > +
> >   	return 0;
> >   }
> >
> >   static int at91_wdt_set_timeout(struct watchdog_device *wdd, unsigned int
> new_timeout)
> >   {
> > -	wdd->timeout = new_timeout;
> > -	return at91_wdt_start(wdd);
> > +	struct at91wdt *wdt = to_wdt(wdd);
> > +	u32 reg, timeout;
> > +
> > +	if (wdt->drv_data->mr_writable) {
> > +		timeout = secs_to_ticks(new_timeout);
> > +		if (timeout > WDT_COUNTER_MAX_TICKS)
> > +			return -EINVAL;
> > +
> > +		reg = wdt_read(wdt, AT91_WDT_MR);
> > +		reg &= ~AT91_WDT_WDV;
> > +		reg |= AT91_WDT_WDV_(timeout);
> > +		reg &= ~AT91_WDT_WDD;
> > +		reg |= AT91_WDT_WDD_(timeout);
> > +		wdt_write(wdt, AT91_WDT_MR, reg);
> > +
> > +		wdd->timeout = new_timeout;
> > +
> > +		return 0;
> > +	} else {
> > +		wdd->timeout = new_timeout;
> > +		return at91_wdt_start(wdd);
> > +	}
> >   }
> >
> >   static int at91_wdt_init(struct platform_device *pdev, struct
> > at91wdt *wdt) @@ -161,50 +218,65 @@ static int at91_wdt_init(struct
> platform_device *pdev, struct at91wdt *wdt)
> >   	unsigned long max_heartbeat;
> >   	struct device *dev = &pdev->dev;
> >
> > -	tmp = wdt_read(wdt, AT91_WDT_MR);
> > -	if ((tmp & mask) != (wdt->mr & mask)) {
> > -		if (tmp == WDT_MR_RESET) {
> > -			wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> > -			tmp = wdt_read(wdt, AT91_WDT_MR);
> > +	if (wdt->drv_data->mr_writable) {
> > +		wdt->use_timer = false;
> > +
> > +		wdt_write(wdt, AT91_WDT_MR, wdt->mr | AT91_WDT_WDDIS);
> > +	} else {
> > +		wdt->use_timer = true;
> > +
> > +		tmp = wdt_read(wdt, AT91_WDT_MR);
> > +		if ((tmp & mask) != (wdt->mr & mask)) {
> > +			if (tmp == WDT_MR_RESET) {
> > +				wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> > +				tmp = wdt_read(wdt, AT91_WDT_MR);
> > +			}
> >   		}
> > -	}
> >
> > -	if (tmp & AT91_WDT_WDDIS) {
> > -		if (wdt->mr & AT91_WDT_WDDIS)
> > -			return 0;
> > -		dev_err(dev, "watchdog is disabled\n");
> > -		return -EINVAL;
> > +		if (tmp & AT91_WDT_WDDIS) {
> > +			if (wdt->mr & AT91_WDT_WDDIS)
> > +				return 0;
> > +
> > +			dev_err(dev, "watchdog is disabled\n");
> > +			return -EINVAL;
> > +		}
> >   	}
> >
> > -	value = tmp & AT91_WDT_WDV;
> > -	delta = (tmp & AT91_WDT_WDD) >> 16;
> > +	tmp = wdt_read(wdt, AT91_WDT_MR);
> > +	wdt->enabled = (tmp & AT91_WDT_WDDIS) ? false : true;
> >
> > -	if (delta < value)
> > -		min_heartbeat = ticks_to_hz_roundup(value - delta);
> > +	if (wdt->use_timer) {
> > +		value = tmp & AT91_WDT_WDV;
> > +		delta = (tmp & AT91_WDT_WDD) >> 16;
> >
> > -	max_heartbeat = ticks_to_hz_rounddown(value);
> > -	if (!max_heartbeat) {
> > -		dev_err(dev,
> > -			"heartbeat is too small for the system to handle it
> correctly\n");
> > -		return -EINVAL;
> > -	}
> > +		if (delta < value)
> > +			min_heartbeat = ticks_to_hz_roundup(value - delta);
> >
> > -	/*
> > -	 * Try to reset the watchdog counter 4 or 2 times more often than
> > -	 * actually requested, to avoid spurious watchdog reset.
> > -	 * If this is not possible because of the min_heartbeat value, reset
> > -	 * it at the min_heartbeat period.
> > -	 */
> > -	if ((max_heartbeat / 4) >= min_heartbeat)
> > -		wdt->heartbeat = max_heartbeat / 4;
> > -	else if ((max_heartbeat / 2) >= min_heartbeat)
> > -		wdt->heartbeat = max_heartbeat / 2;
> > -	else
> > -		wdt->heartbeat = min_heartbeat;
> > -
> > -	if (max_heartbeat < min_heartbeat + 4)
> > -		dev_warn(dev,
> > -			 "min heartbeat and max heartbeat might be too close for
> the system to handle it correctly\n");
> > +		max_heartbeat = ticks_to_hz_rounddown(value);
> > +		if (!max_heartbeat) {
> > +			dev_err(dev,
> > +				"heartbeat is too small for the system to handle it
> correctly\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		/*
> > +		 * Try to reset the watchdog counter 4 or 2 times more often than
> > +		 * actually requested, to avoid spurious watchdog reset.
> > +		 * If this is not possible because of the min_heartbeat value, reset
> > +		 * it at the min_heartbeat period.
> > +		 */
> > +		if ((max_heartbeat / 4) >= min_heartbeat)
> > +			wdt->heartbeat = max_heartbeat / 4;
> > +		else if ((max_heartbeat / 2) >= min_heartbeat)
> > +			wdt->heartbeat = max_heartbeat / 2;
> > +		else
> > +			wdt->heartbeat = min_heartbeat;
> > +
> > +		if (max_heartbeat < min_heartbeat + 4) {
> > +			dev_warn(dev,
> > +				 "min heartbeat and max heartbeat might be too
> close for the system to handle it correctly\n");
> > +		}
> > +	}
> >
> >   	if ((tmp & AT91_WDT_WDFIEN) && wdt->irq) {
> >   		err = request_irq(wdt->irq, wdt_interrupt, @@ -215,21 +287,24
> @@
> > static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >   			return err;
> >   	}
> >
> > -	if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask))
> > -		dev_warn(dev,
> > -			 "watchdog already configured differently (mr = %x
> expecting %x)\n",
> > -			 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> > +	if (wdt->use_timer) {
> > +		if ((tmp & wdt->mr_mask) != (wdt->mr & wdt->mr_mask)) {
> > +			dev_warn(dev,
> > +				 "watchdog already configured differently (mr
> = %x expecting %x)\n",
> > +				 tmp & wdt->mr_mask, wdt->mr & wdt->mr_mask);
> > +		}
> >
> > -	setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> > +		setup_timer(&wdt->timer, at91_ping, (unsigned long)wdt);
> >
> > -	/*
> > -	 * Use min_heartbeat the first time to avoid spurious watchdog reset:
> > -	 * we don't know for how long the watchdog counter is running, and
> > -	 *  - resetting it right now might trigger a watchdog fault reset
> > -	 *  - waiting for heartbeat time might lead to a watchdog timeout
> > -	 *    reset
> > -	 */
> > -	mod_timer(&wdt->timer, jiffies + min_heartbeat);
> > +		/*
> > +		 * Use min_heartbeat the first time to avoid spurious watchdog
> reset:
> > +		 * we don't know for how long the watchdog counter is running,
> and
> > +		 *  - resetting it right now might trigger a watchdog fault reset
> > +		 *  - waiting for heartbeat time might lead to a watchdog timeout
> > +		 *    reset
> > +		 */
> > +		mod_timer(&wdt->timer, jiffies + min_heartbeat);
> > +	}
> >
> >   	/* Try to set timeout from device tree first */
> >   	if (watchdog_init_timeout(&wdt->wdd, 0, dev)) @@ -239,12 +314,14 @@
> > static int at91_wdt_init(struct platform_device *pdev, struct at91wdt *wdt)
> >   	if (err)
> >   		goto out_stop_timer;
> >
> > -	wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
> > +	if (wdt->use_timer)
> > +		wdt->next_heartbeat = jiffies + wdt->wdd.timeout * HZ;
> >
> >   	return 0;
> >
> >   out_stop_timer:
> > -	del_timer(&wdt->timer);
> > +	if (wdt->use_timer)
> > +		del_timer(&wdt->timer);
> >   	return err;
> >   }
> >
> > @@ -256,13 +333,54 @@ static const struct watchdog_info at91_wdt_info = {
> >   						WDIOF_MAGICCLOSE,
> >   };
> >
> > -static const struct watchdog_ops at91_wdt_ops = {
> > +static struct watchdog_ops at91_wdt_ops = {
> >   	.owner =	THIS_MODULE,
> >   	.start =	at91_wdt_start,
> >   	.stop =		at91_wdt_stop,
> >   	.set_timeout =	at91_wdt_set_timeout,
> >   };
> >
> > +static const struct at91wdt_variant drv_data_at91sam9260 = {
> > +	.mr_writable = false,
> > +};
> > +
> > +#if defined(CONFIG_OF)
> > +static const struct at91wdt_variant drv_data_sama5d4 = {
> > +	.mr_writable = true,
> > +};
> > +
> > +static const struct of_device_id at91_wdt_dt_ids[] = {
> > +	{ .compatible = "atmel,at91sam9260-wdt",
> > +	  .data = &drv_data_at91sam9260 },
> > +	{ .compatible = "atmel,sama5d4-wdt",
> > +	  .data = &drv_data_sama5d4 },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids); #endif
> > +
> > +static const struct platform_device_id at91wdt_ids[] = {
> > +	{
> > +		.name = "at91_wdt",
> > +		.driver_data = (unsigned long)&drv_data_at91sam9260,
> > +	},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(platform, at91wdt_ids);
> > +
> > +static struct at91wdt_variant *at91wdt_get_drv_data(struct
> > +platform_device *pdev) {
> > +	const struct of_device_id *match;
> > +
> > +	if (pdev->dev.of_node) {
> > +		match = of_match_node(at91_wdt_dt_ids, pdev->dev.of_node);
> > +		return (struct at91wdt_variant *)match->data;
> > +	} else {
> > +		return (struct at91wdt_variant *)
> > +			platform_get_device_id(pdev)->driver_data;
> > +	}
> > +}
> > +
> >   #if defined(CONFIG_OF)
> >   static int of_at91wdt_init(struct device_node *np, struct at91wdt *wdt)
> >   {
> > @@ -336,6 +454,10 @@ static int __init at91wdt_probe(struct platform_device
> *pdev)
> >   	if (!wdt)
> >   		return -ENOMEM;
> >
> > +	wdt->drv_data = at91wdt_get_drv_data(pdev);
> > +	if (wdt->drv_data->mr_writable)
> > +		at91_wdt_ops.ping = at91_wdt_ping;
> > +
> >   	wdt->mr = (WDT_HW_TIMEOUT * 256) | AT91_WDT_WDRSTEN |
> AT91_WDT_WDD |
> >   		  AT91_WDT_WDDBGHLT | AT91_WDT_WDIDLEHLT;
> >   	wdt->mr_mask = 0x3FFFFFFF;
> > @@ -364,8 +486,12 @@ static int __init at91wdt_probe(struct
> > platform_device *pdev)
> >
> >   	platform_set_drvdata(pdev, wdt);
> >
> > -	pr_info("enabled (heartbeat=%d sec, nowayout=%d)\n",
> > -		wdt->wdd.timeout, wdt->nowayout);
> > +	if (wdt->enabled) {
> > +		dev_info(&pdev->dev, "enabled (heartbeat=%d sec,
> nowayout=%d)\n",
> > +			wdt->wdd.timeout, wdt->nowayout);
> > +	} else {
> > +		dev_info(&pdev->dev, "not enabled\n");
> > +	}
> >
> >   	return 0;
> >   }
> > @@ -381,15 +507,6 @@ static int __exit at91wdt_remove(struct
> platform_device *pdev)
> >   	return 0;
> >   }
> >
> > -#if defined(CONFIG_OF)
> > -static const struct of_device_id at91_wdt_dt_ids[] = {
> > -	{ .compatible = "atmel,at91sam9260-wdt" },
> > -	{ /* sentinel */ }
> > -};
> > -
> > -MODULE_DEVICE_TABLE(of, at91_wdt_dt_ids); -#endif
> > -
> >   static struct platform_driver at91wdt_driver = {
> >   	.remove		= __exit_p(at91wdt_remove),
> >   	.driver		= {
> > diff --git a/drivers/watchdog/at91sam9_wdt.h
> > b/drivers/watchdog/at91sam9_wdt.h index c6fbb2e6..79add4f 100644
> > --- a/drivers/watchdog/at91sam9_wdt.h
> > +++ b/drivers/watchdog/at91sam9_wdt.h
> > @@ -22,11 +22,15 @@
> >
> >   #define AT91_WDT_MR		0x04			/* Watchdog
> Mode Register */
> >   #define		AT91_WDT_WDV		(0xfff << 0)		/*
> Counter Value */
> > +#define			AT91_WDT_WDV_MSK	(0xfff)
> > +#define			AT91_WDT_WDV_(x)	(((x) &
> AT91_WDT_WDV_MSK) << 0)
> >   #define		AT91_WDT_WDFIEN		(1     << 12)		/*
> Fault Interrupt Enable */
> >   #define		AT91_WDT_WDRSTEN	(1     << 13)		/*
> Reset Processor */
> >   #define		AT91_WDT_WDRPROC	(1     << 14)		/*
> Timer Restart */
> >   #define		AT91_WDT_WDDIS		(1     << 15)		/*
> Watchdog Disable */
> >   #define		AT91_WDT_WDD		(0xfff << 16)		/*
> Delta Value */
> > +#define			AT91_WDT_WDD_MSK	(0xfff)
> > +#define			AT91_WDT_WDD_(x)	(((x) &
> AT91_WDT_WDD_MSK) << 16)
> >   #define		AT91_WDT_WDDBGHLT	(1     << 28)		/*
> Debug Halt */
> >   #define		AT91_WDT_WDIDLEHLT	(1     << 29)		/*
> Idle Halt */
> >
> >


Best Regards,
Wenyou Yang

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±Á«\…Ú Š{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support

[Guenter Roeck]

On 07/28/2015 05:38 PM, Yang, Wenyou wrote:
> Hi Guenter,
>
> Thank you very much for your review.
>
>> -----Original Message-----
>> From: Guenter Roeck [mailto:linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org]
>> Sent: 2015年7月28日 15:14
>> To: Yang, Wenyou; wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; pawel.moll@arm.com;
>> mark.rutland-5wv7dgnIgG8@public.gmane.org; ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org; galak@codeaurora.org
>> Cc: sylvain.rochet-ETtyaVkrhkNWk0Htik3J/w@public.gmane.org; Ferre, Nicolas; boris.brezillon@free-
>> electrons.com; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TasMV2rI37PzA@public.gmane.orgorg; linux-
>> watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature
>> support
>>
>> On 07/28/2015 12:00 AM, Wenyou Yang wrote:
>>> In the datasheet, the new feature is describled as "WDT_MR can be
>>> written until a LOCKMR command is issued in WDT_CR".
>>> That is to say, as long as the bootstrap and u-boot don't issue a
>>> LOCKMR command, WDT_MR can be written in kernel.
>>>
>>> So the driver can enable/disable the watchdog timer hardware, set
>>> WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields of
>>> WDT_MR register to set the watchdog timer timeout.
>>>
>>> The timer is not necessary that regularly sends a keepalive ping to
>>> the watchdog timer hardware.
>>>
>>> It is introduced from sama5d4 SoCs.
>>>
>> Since there are so many changes, I wonder is a separate driver would make more
>> sense.
> Yes, a bit many changes.
> I thought reuse the driver code.
> If a separate driver, I am afraid it includes much duplicated code.
> After all, it is for the same device with different feature.
>
> I don't think it is necessary to have multiple drivers for the same peripheral with different feature.
>

The concept for the two mechanisms is all different: In one, the watchdog keepalive is triggered
from timer code. In the other, the watchdog timeout is triggered directly from the heartbeat
function. One assumes that the watchdog is always running, and that it must be pinged even
if closed. The other disables the watchdog on close.

What I _can_ see is that the driver is becoming an unmaintainable mess, with lots of if/else
in pretty much every function. I consider this much less desirable than a bit of code
duplication - if there is any. Seriously, most of the added code might as well be for
a completely different chip.

Guenter

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

RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature support

[Yang, Wenyou]

Hi Guenter,

Thank you for your prompt answer.

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: 2015年7月29日 9:23
> To: Yang, Wenyou; wim@iguana.be; robh+dt@kernel.org; pawel.moll@arm.com;
> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; galak@codeaurora.org
> Cc: sylvain.rochet@finsecur.com; Ferre, Nicolas; boris.brezillon@free-
> electrons.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature
> support
> 
> On 07/28/2015 05:38 PM, Yang, Wenyou wrote:
> > Hi Guenter,
> >
> > Thank you very much for your review.
> >
> >> -----Original Message-----
> >> From: Guenter Roeck [mailto:linux@roeck-us.net]
> >> Sent: 2015年7月28日 15:14
> >> To: Yang, Wenyou; wim@iguana.be; robh+dt@kernel.org;
> >> pawel.moll@arm.com; mark.rutland@arm.com;
> >> ijc+devicetree@hellion.org.uk; galak@codeaurora.org
> >> Cc: sylvain.rochet@finsecur.com; Ferre, Nicolas;
> >> boris.brezillon@free- electrons.com; devicetree@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; linux- watchdog@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new
> >> feature support
> >>
> >> On 07/28/2015 12:00 AM, Wenyou Yang wrote:
> >>> In the datasheet, the new feature is describled as "WDT_MR can be
> >>> written until a LOCKMR command is issued in WDT_CR".
> >>> That is to say, as long as the bootstrap and u-boot don't issue a
> >>> LOCKMR command, WDT_MR can be written in kernel.
> >>>
> >>> So the driver can enable/disable the watchdog timer hardware, set
> >>> WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields of
> >>> WDT_MR register to set the watchdog timer timeout.
> >>>
> >>> The timer is not necessary that regularly sends a keepalive ping to
> >>> the watchdog timer hardware.
> >>>
> >>> It is introduced from sama5d4 SoCs.
> >>>
> >> Since there are so many changes, I wonder is a separate driver would
> >> make more sense.
> > Yes, a bit many changes.
> > I thought reuse the driver code.
> > If a separate driver, I am afraid it includes much duplicated code.
> > After all, it is for the same device with different feature.
> >
> > I don't think it is necessary to have multiple drivers for the same peripheral with
> different feature.
> >
> 
> The concept for the two mechanisms is all different: In one, the watchdog
> keepalive is triggered from timer code. In the other, the watchdog timeout is
> triggered directly from the heartbeat function. One assumes that the watchdog is
> always running, and that it must be pinged even if closed. The other disables the
> watchdog on close.
> 
> What I _can_ see is that the driver is becoming an unmaintainable mess, with lots
> of if/else in pretty much every function. I consider this much less desirable than a
> bit of code duplication - if there is any. Seriously, most of the added code might as
> well be for a completely different chip.

You are right, I accepted your advice. I will rewrite it. Thanks.

> 
> Guenter

Best Regards,
Wenyou Yang

[PATCH 2/2] Documentation: dt: binding: atmel-wdt: add a new compitable

[Wenyou Yang]

Add a new compatible "atmel,sama5d4-wdt" for SAMA5D4,
which suports the new feature, the WDT_MR register can be written more once.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---
 .../devicetree/bindings/watchdog/atmel-wdt.txt     |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
index a4d8697..060c682 100644
--- a/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/atmel-wdt.txt
@@ -3,7 +3,9 @@
 ** at91sam9-wdt
 
 Required properties:
-- compatible: must be "atmel,at91sam9260-wdt".
+- compatible : should be one among the following
+	(a) "atmel,at91sam9260-wdt" for AT91SAM9x and SAMA5D3 SoCs
+	(b) "atmel,sama5d4-wdt" for SAMA5D4
 - reg: physical base address of the controller and length of memory mapped
   region.
 
-- 
1.7.9.5