[PATCH RESEND v6 0/2] watchdog: Add support for ImgTec PowerDown Controller Watchdog Timer

[PATCH RESEND v6 0/2] watchdog: Add support for ImgTec PowerDown Controller Watchdog Timer

[Naidu Tellapati]

This patchset provides support for the PowerDown Controller (PDC) Watchdog Timer
found on Pistachio SOC from Imagination Technologies (ImgTec).

The series is based on 3.18-rc5.

I am re-sending the series with correct Signed-off-by headers.

Changes from v5:

 * Removed unused struct pdc_wdt_dev variables.

 * Removed out_wdt label in probe function.

Changes from v4:

 * Added warning message if initial timeout value exceeds maximum timeout.

Changes from v3:

 * Removed PDC_WD_MAX_DELAY.

 * Removed spinlock.

 * Removed timeout check inside pdc_wdt_set_timeout function.

 * Changed identity to IMG PDC Watchdog.

 * Removed pdc_wdt_set_timeout call from the probe function.

 * Removed pdc_wdt_shutdown function call from pdc_wdt_remove function
   and added pdc_wdt_stop function call instead.

Changes from v2:

 * Removed interrupt.h header file.

 * Removed clock rate-change notifier callback.

Changes from v1:

 * Arranged all include files in alphabetical order.

 * Arranged register field masks/shifts defines next to their
   corresponding register

 * Registered clock rate-change notifier callback.

 * Removed all references to 32 kHz fixed clock rate.
   Now the clock rate is read dynamically using clk_get_rate().

 * Defined a macro PDC_WD_CONFIG_ENABLE.

 * Added code for getting system clock.

 * Moved module_param stuffs next to the corresponding variable defines.

 * Added code in the function pdc_wdt_remove for stopping watchdog.

 * Removed MODULE_ALIAS.

 * Removed few unwanted spaces and unwanted blank lines.

Naidu Tellapati (2):
  watchdog: ImgTec PDC Watchdog Timer Driver
  DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

 .../devicetree/bindings/watchdog/imgpdc-wdt.txt    |   18 ++
 drivers/watchdog/Kconfig                           |   11 +
 drivers/watchdog/Makefile                          |    1 +
 drivers/watchdog/imgpdc_wdt.c                      |  298 ++++++++++++++++++++
 4 files changed, 328 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
 create mode 100644 drivers/watchdog/imgpdc_wdt.c

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

[PATCH RESEND v6 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[Naidu Tellapati]

This commit adds support for ImgTec PowerDown Controller Watchdog Timer.

Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jude.Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/watchdog/Kconfig      |   11 ++
 drivers/watchdog/Makefile     |    1 +
 drivers/watchdog/imgpdc_wdt.c |  298 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 310 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/imgpdc_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index d0107d4..a184f23 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1235,6 +1235,17 @@ config BCM_KONA_WDT_DEBUG
 
 	  If in doubt, say 'N'.
 
+config IMGPDC_WDT
+	tristate "Imagination Technologies PDC Watchdog Timer"
+	depends on HAS_IOMEM
+	depends on METAG || MIPS || COMPILE_TEST
+	help
+	  Driver for Imagination Technologies PowerDown Controller
+	  Watchdog Timer.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called imgpdc_wdt.
+
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index c569ec8..d4dfbb4 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -142,6 +142,7 @@ obj-$(CONFIG_OCTEON_WDT) += octeon-wdt.o
 octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o
 obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o
 obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
+obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
 
 # PARISC Architecture
 
diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
new file mode 100644
index 0000000..efb00b5
--- /dev/null
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -0,0 +1,298 @@
+/*
+ * Imagination Technologies PowerDown Controller Watchdog Timer.
+ *
+ * Copyright (c) 2014 Imagination Technologies Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * Based on drivers/watchdog/sunxi_wdt.c Copyright (c) 2013 Carlo Caione
+ *                                                     2012 Henrik Nordstrom
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/watchdog.h>
+
+/* registers */
+#define PDC_WD_SW_RESET			0x000
+
+#define PDC_WD_CONFIG			0x004
+#define PDC_WD_CONFIG_ENABLE		BIT(31)
+#define PDC_WD_CONFIG_DELAY_MASK	0x0000001f
+#define PDC_WD_CONFIG_DELAY_SHIFT	0
+
+#define PDC_WD_TICKLE1			0x008
+#define PDC_WD_TICKLE1_MAGIC		0xabcd1234
+
+#define PDC_WD_TICKLE2			0x00c
+#define PDC_WD_TICKLE2_MAGIC		0x4321dcba
+
+#define PDC_WD_TICKLE_STATUS_MASK	0x00000007
+#define PDC_WD_TICKLE_STATUS_SHIFT	0
+#define PDC_WD_TICKLE_STATUS_HRESET     0x0  /* Hard reset */
+#define PDC_WD_TICKLE_STATUS_TIMEOUT    0x1  /* Timeout */
+#define PDC_WD_TICKLE_STATUS_TICKLE     0x2  /* Tickled incorrectly */
+#define PDC_WD_TICKLE_STATUS_SRESET     0x3  /* Soft reset */
+#define PDC_WD_TICKLE_STATUS_USER       0x4  /* User reset */
+
+/* timeout in seconds */
+#define PDC_WD_MIN_TIMEOUT		1
+#define PDC_WD_DEFAULT_TIMEOUT		64
+
+static int timeout = PDC_WD_DEFAULT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "PDC watchdog delay in seconds (default 64s)");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
+
+struct pdc_wdt_dev {
+	struct watchdog_device wdt_dev;
+	struct clk *wdt_clk;
+	struct clk *sys_clk;
+	int min_delay;
+	void __iomem *base;
+};
+
+static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev)
+{
+	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	writel(PDC_WD_TICKLE1_MAGIC, wdt->base + PDC_WD_TICKLE1);
+	writel(PDC_WD_TICKLE2_MAGIC, wdt->base + PDC_WD_TICKLE2);
+
+	return 0;
+}
+
+static int pdc_wdt_stop(struct watchdog_device *wdt_dev)
+{
+	unsigned int val;
+	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	val = readl(wdt->base + PDC_WD_CONFIG);
+	val &= ~PDC_WD_CONFIG_ENABLE;
+	writel(val, wdt->base + PDC_WD_CONFIG);
+
+	/* Must tickle to finish the stop */
+	pdc_wdt_keepalive(wdt_dev);
+
+	return 0;
+}
+
+static int pdc_wdt_set_timeout(struct watchdog_device *wdt_dev,
+			       unsigned int new_timeout)
+{
+	unsigned int val;
+	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	wdt->wdt_dev.timeout = new_timeout;
+	/* round up to the next power of 2 */
+	new_timeout = order_base_2(new_timeout);
+	val = readl(wdt->base + PDC_WD_CONFIG);
+	val &= ~PDC_WD_CONFIG_DELAY_MASK;
+	val |= (new_timeout + wdt->min_delay) << PDC_WD_CONFIG_DELAY_SHIFT;
+	writel(val, wdt->base + PDC_WD_CONFIG);
+
+	return 0;
+}
+
+/* Start the watchdog timer (delay should already be set) */
+static int pdc_wdt_start(struct watchdog_device *wdt_dev)
+{
+	unsigned int val;
+	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
+
+	val = readl(wdt->base + PDC_WD_CONFIG);
+	val |= PDC_WD_CONFIG_ENABLE;
+	writel(val, wdt->base + PDC_WD_CONFIG);
+
+	return 0;
+}
+
+static struct watchdog_info pdc_wdt_info = {
+	.identity	= "IMG PDC Watchdog",
+	.options	= WDIOF_SETTIMEOUT |
+			  WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE,
+};
+
+/* Kernel interface */
+static const struct watchdog_ops pdc_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= pdc_wdt_start,
+	.stop		= pdc_wdt_stop,
+	.ping		= pdc_wdt_keepalive,
+	.set_timeout	= pdc_wdt_set_timeout,
+};
+
+static int pdc_wdt_probe(struct platform_device *pdev)
+{
+	int ret, val;
+	unsigned long clk_rate;
+	struct resource *res;
+	struct pdc_wdt_dev *pdc_wdt;
+
+	pdc_wdt = devm_kzalloc(&pdev->dev, sizeof(*pdc_wdt), GFP_KERNEL);
+	if (!pdc_wdt)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pdc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pdc_wdt->base))
+		return PTR_ERR(pdc_wdt->base);
+
+	pdc_wdt->sys_clk = devm_clk_get(&pdev->dev, "sys");
+	if (IS_ERR(pdc_wdt->sys_clk)) {
+		dev_err(&pdev->dev, "failed to get the sys clock.\n");
+		ret = PTR_ERR(pdc_wdt->wdt_clk);
+		return ret;
+	}
+
+	pdc_wdt->wdt_clk = devm_clk_get(&pdev->dev, "wdt");
+	if (IS_ERR(pdc_wdt->wdt_clk)) {
+		dev_err(&pdev->dev, "failed to get the wdt clock.\n");
+		ret = PTR_ERR(pdc_wdt->wdt_clk);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(pdc_wdt->sys_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not prepare or enable sys clock.\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(pdc_wdt->wdt_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not prepare or enable wdt clock.\n");
+		goto disable_sys_clk;
+	}
+
+	/* We use the clock rate to calculate the max timeout */
+	clk_rate = clk_get_rate(pdc_wdt->wdt_clk);
+	if (clk_rate == 0) {
+		dev_err(&pdev->dev, "failed to get clock rate\n");
+		ret = -EINVAL;
+		goto disable_wdt_clk;
+	}
+
+	if (order_base_2(clk_rate) > PDC_WD_CONFIG_DELAY_MASK + 1) {
+		dev_err(&pdev->dev, "invalid clock rate\n");
+		ret = -EINVAL;
+		goto disable_wdt_clk;
+	}
+
+	if (order_base_2(clk_rate) == 0)
+		pdc_wdt->wdt_dev.min_timeout = PDC_WD_MIN_TIMEOUT + 1;
+	else
+		pdc_wdt->wdt_dev.min_timeout = PDC_WD_MIN_TIMEOUT;
+
+	pdc_wdt->min_delay = order_base_2(clk_rate) - 1;
+
+	pdc_wdt->wdt_dev.info = &pdc_wdt_info;
+	pdc_wdt->wdt_dev.ops = &pdc_wdt_ops;
+	pdc_wdt->wdt_dev.max_timeout =
+			(1 << (PDC_WD_CONFIG_DELAY_MASK - pdc_wdt->min_delay));
+	pdc_wdt->wdt_dev.parent = &pdev->dev;
+
+	ret = watchdog_init_timeout(&pdc_wdt->wdt_dev, timeout, &pdev->dev);
+	if (ret < 0) {
+		pdc_wdt->wdt_dev.timeout = pdc_wdt->wdt_dev.max_timeout;
+		dev_warn(&pdev->dev,
+			 "Initial timeout out of range! setting max timeout\n");
+	}
+
+	pdc_wdt_stop(&pdc_wdt->wdt_dev);
+
+	/* Find what caused the last reset */
+	val = readl(pdc_wdt->base + PDC_WD_TICKLE1);
+	val = (val & PDC_WD_TICKLE_STATUS_MASK) >> PDC_WD_TICKLE_STATUS_SHIFT;
+	switch (val) {
+	case PDC_WD_TICKLE_STATUS_TICKLE:
+	case PDC_WD_TICKLE_STATUS_TIMEOUT:
+		pdc_wdt->wdt_dev.bootstatus |= WDIOF_CARDRESET;
+		dev_info(&pdev->dev,
+			 "watchdog module last reset due to timeout\n");
+		break;
+	case PDC_WD_TICKLE_STATUS_HRESET:
+		dev_info(&pdev->dev,
+			 "watchdog module last reset due to hard reset\n");
+		break;
+	case PDC_WD_TICKLE_STATUS_SRESET:
+		dev_info(&pdev->dev,
+			 "watchdog module last reset due to soft reset\n");
+		break;
+	case PDC_WD_TICKLE_STATUS_USER:
+		dev_info(&pdev->dev,
+			 "watchdog module last reset due to user reset\n");
+		break;
+	default:
+		dev_info(&pdev->dev,
+			 "contains an illegal status code (%08x)\n", val);
+		break;
+	}
+
+	watchdog_set_nowayout(&pdc_wdt->wdt_dev, nowayout);
+
+	platform_set_drvdata(pdev, pdc_wdt);
+	watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
+
+	ret = watchdog_register_device(&pdc_wdt->wdt_dev);
+	if (ret)
+		goto disable_wdt_clk;
+
+	return 0;
+
+disable_wdt_clk:
+	clk_disable_unprepare(pdc_wdt->wdt_clk);
+disable_sys_clk:
+	clk_disable_unprepare(pdc_wdt->sys_clk);
+	return ret;
+}
+
+static void pdc_wdt_shutdown(struct platform_device *pdev)
+{
+	struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
+
+	pdc_wdt_stop(&pdc_wdt->wdt_dev);
+}
+
+static int pdc_wdt_remove(struct platform_device *pdev)
+{
+	struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
+
+	pdc_wdt_stop(&pdc_wdt->wdt_dev);
+	watchdog_unregister_device(&pdc_wdt->wdt_dev);
+	clk_disable_unprepare(pdc_wdt->wdt_clk);
+	clk_disable_unprepare(pdc_wdt->sys_clk);
+
+	return 0;
+}
+
+static const struct of_device_id pdc_wdt_match[] = {
+	{ .compatible = "img,pdc-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pdc_wdt_match);
+
+static struct platform_driver pdc_wdt_driver = {
+	.driver = {
+		.name = "imgpdc-wdt",
+		.of_match_table	= pdc_wdt_match,
+	},
+	.probe = pdc_wdt_probe,
+	.remove = pdc_wdt_remove,
+	.shutdown = pdc_wdt_shutdown,
+};
+module_platform_driver(pdc_wdt_driver);
+
+MODULE_AUTHOR("Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
+MODULE_AUTHOR("Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("Imagination Technologies PDC Watchdog Timer Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.0.4

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

[PATCH RESEND v6 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

[Naidu Tellapati]

Add the devicetree binding document for ImgTec PDC Watchdog Timer.

Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jude Abraham <Jude.Abraham-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
No changes from v5.
---
 .../devicetree/bindings/watchdog/imgpdc-wdt.txt    |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
new file mode 100644
index 0000000..2f9ba77
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
@@ -0,0 +1,18 @@
+*ImgTec PowerDown Controller (PDC) Watchdog Timer (WDT)
+
+Required properties:
+- compatible : Should be "img,pdc-wdt"
+- reg : Should contain WDT registers location and length
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Should contain "wdt" and "sys"
+- interrupts : Should contain WDT interrupt
+
+Examples:
+
+wdt@18102100 {
+	compatible = "img,pdc-wdt";
+	reg = <0x18102100 0x100>;
+	clocks = <&pdc_wdt_clk>, <&sys_clk>;
+	clock-names = "wdt", "sys";
+	interrupts = <0 52 IRQ_TYPE_LEVEL_HIGH>;
+};
-- 
1.7.0.4

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

Re: [PATCH RESEND v6 0/2] watchdog: Add support for ImgTec PowerDown Controller Watchdog Timer

[Ezequiel Garcia]

Wim,

On 11/27/2014 01:54 AM, Naidu Tellapati wrote:
> This patchset provides support for the PowerDown Controller (PDC) Watchdog Timer
> found on Pistachio SOC from Imagination Technologies (ImgTec).
> 
> The series is based on 3.18-rc5.
> 
> I am re-sending the series with correct Signed-off-by headers.
> 
> Changes from v5:
> 
>  * Removed unused struct pdc_wdt_dev variables.
> 
>  * Removed out_wdt label in probe function.
> 
> Changes from v4:
> 
>  * Added warning message if initial timeout value exceeds maximum timeout.
> 
> Changes from v3:
> 
>  * Removed PDC_WD_MAX_DELAY.
> 
>  * Removed spinlock.
> 
>  * Removed timeout check inside pdc_wdt_set_timeout function.
> 
>  * Changed identity to IMG PDC Watchdog.
> 
>  * Removed pdc_wdt_set_timeout call from the probe function.
> 
>  * Removed pdc_wdt_shutdown function call from pdc_wdt_remove function
>    and added pdc_wdt_stop function call instead.
> 
> Changes from v2:
> 
>  * Removed interrupt.h header file.
> 
>  * Removed clock rate-change notifier callback.
> 
> Changes from v1:
> 
>  * Arranged all include files in alphabetical order.
> 
>  * Arranged register field masks/shifts defines next to their
>    corresponding register
> 
>  * Registered clock rate-change notifier callback.
> 
>  * Removed all references to 32 kHz fixed clock rate.
>    Now the clock rate is read dynamically using clk_get_rate().
> 
>  * Defined a macro PDC_WD_CONFIG_ENABLE.
> 
>  * Added code for getting system clock.
> 
>  * Moved module_param stuffs next to the corresponding variable defines.
> 
>  * Added code in the function pdc_wdt_remove for stopping watchdog.
> 
>  * Removed MODULE_ALIAS.
> 
>  * Removed few unwanted spaces and unwanted blank lines.
> 
> Naidu Tellapati (2):
>   watchdog: ImgTec PDC Watchdog Timer Driver
>   DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation
> 

Do you think it's possible to merge this series in time for v3.19?

Thanks!
-- 
Ezequiel
--
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 RESEND v6 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[Guenter Roeck]

On Thu, Nov 27, 2014 at 10:24:04AM +0530, Naidu Tellapati wrote:
> This commit adds support for ImgTec PowerDown Controller Watchdog Timer.
> 
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jude.Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Reviewed-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/watchdog/Kconfig      |   11 ++
>  drivers/watchdog/Makefile     |    1 +
>  drivers/watchdog/imgpdc_wdt.c |  298 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 310 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/watchdog/imgpdc_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index d0107d4..a184f23 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1235,6 +1235,17 @@ config BCM_KONA_WDT_DEBUG
>  
>  	  If in doubt, say 'N'.
>  
> +config IMGPDC_WDT
> +	tristate "Imagination Technologies PDC Watchdog Timer"
> +	depends on HAS_IOMEM
> +	depends on METAG || MIPS || COMPILE_TEST
> +	help
> +	  Driver for Imagination Technologies PowerDown Controller
> +	  Watchdog Timer.
> +
> +	  To compile this driver as a loadable module, choose M here.
> +	  The module will be called imgpdc_wdt.
> +
>  config LANTIQ_WDT
>  	tristate "Lantiq SoC watchdog"
>  	depends on LANTIQ
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index c569ec8..d4dfbb4 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_OCTEON_WDT) += octeon-wdt.o
>  octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o
>  obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o
>  obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
> +obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
>  
>  # PARISC Architecture
>  
> diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
> new file mode 100644
> index 0000000..efb00b5
> --- /dev/null
> +++ b/drivers/watchdog/imgpdc_wdt.c
> @@ -0,0 +1,298 @@
> +/*
> + * Imagination Technologies PowerDown Controller Watchdog Timer.
> + *
> + * Copyright (c) 2014 Imagination Technologies Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * Based on drivers/watchdog/sunxi_wdt.c Copyright (c) 2013 Carlo Caione
> + *                                                     2012 Henrik Nordstrom
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/watchdog.h>
> +
> +/* registers */
> +#define PDC_WD_SW_RESET			0x000
> +
> +#define PDC_WD_CONFIG			0x004
> +#define PDC_WD_CONFIG_ENABLE		BIT(31)
> +#define PDC_WD_CONFIG_DELAY_MASK	0x0000001f
> +#define PDC_WD_CONFIG_DELAY_SHIFT	0
> +
> +#define PDC_WD_TICKLE1			0x008
> +#define PDC_WD_TICKLE1_MAGIC		0xabcd1234
> +
> +#define PDC_WD_TICKLE2			0x00c
> +#define PDC_WD_TICKLE2_MAGIC		0x4321dcba
> +
> +#define PDC_WD_TICKLE_STATUS_MASK	0x00000007
> +#define PDC_WD_TICKLE_STATUS_SHIFT	0
> +#define PDC_WD_TICKLE_STATUS_HRESET     0x0  /* Hard reset */
> +#define PDC_WD_TICKLE_STATUS_TIMEOUT    0x1  /* Timeout */
> +#define PDC_WD_TICKLE_STATUS_TICKLE     0x2  /* Tickled incorrectly */
> +#define PDC_WD_TICKLE_STATUS_SRESET     0x3  /* Soft reset */
> +#define PDC_WD_TICKLE_STATUS_USER       0x4  /* User reset */
> +
> +/* timeout in seconds */
> +#define PDC_WD_MIN_TIMEOUT		1
> +#define PDC_WD_DEFAULT_TIMEOUT		64
> +
> +static int timeout = PDC_WD_DEFAULT_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "PDC watchdog delay in seconds (default 64s)");

It might be better to use  __MODULE_STRING(PDC_WD_DEFAULT_TIMEOUT)
to encode the default timeout.

> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started");
> +
> +struct pdc_wdt_dev {
> +	struct watchdog_device wdt_dev;
> +	struct clk *wdt_clk;
> +	struct clk *sys_clk;
> +	int min_delay;
> +	void __iomem *base;
> +};
> +
> +static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev)
> +{
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	writel(PDC_WD_TICKLE1_MAGIC, wdt->base + PDC_WD_TICKLE1);
> +	writel(PDC_WD_TICKLE2_MAGIC, wdt->base + PDC_WD_TICKLE2);
> +
> +	return 0;
> +}
> +
> +static int pdc_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +	unsigned int val;
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	val = readl(wdt->base + PDC_WD_CONFIG);
> +	val &= ~PDC_WD_CONFIG_ENABLE;
> +	writel(val, wdt->base + PDC_WD_CONFIG);
> +
> +	/* Must tickle to finish the stop */
> +	pdc_wdt_keepalive(wdt_dev);
> +
> +	return 0;
> +}
> +
> +static int pdc_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +			       unsigned int new_timeout)
> +{
> +	unsigned int val;
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	wdt->wdt_dev.timeout = new_timeout;
> +	/* round up to the next power of 2 */
> +	new_timeout = order_base_2(new_timeout);
> +	val = readl(wdt->base + PDC_WD_CONFIG);
> +	val &= ~PDC_WD_CONFIG_DELAY_MASK;
> +	val |= (new_timeout + wdt->min_delay) << PDC_WD_CONFIG_DELAY_SHIFT;
> +	writel(val, wdt->base + PDC_WD_CONFIG);
> +
> +	return 0;
> +}
> +
> +/* Start the watchdog timer (delay should already be set) */
> +static int pdc_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	unsigned int val;
> +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> +
> +	val = readl(wdt->base + PDC_WD_CONFIG);
> +	val |= PDC_WD_CONFIG_ENABLE;
> +	writel(val, wdt->base + PDC_WD_CONFIG);
> +
> +	return 0;
> +}
> +
> +static struct watchdog_info pdc_wdt_info = {
> +	.identity	= "IMG PDC Watchdog",
> +	.options	= WDIOF_SETTIMEOUT |
> +			  WDIOF_KEEPALIVEPING |
> +			  WDIOF_MAGICCLOSE,
> +};
> +
> +/* Kernel interface */
> +static const struct watchdog_ops pdc_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= pdc_wdt_start,
> +	.stop		= pdc_wdt_stop,
> +	.ping		= pdc_wdt_keepalive,
> +	.set_timeout	= pdc_wdt_set_timeout,
> +};
> +
> +static int pdc_wdt_probe(struct platform_device *pdev)
> +{
> +	int ret, val;
> +	unsigned long clk_rate;
> +	struct resource *res;
> +	struct pdc_wdt_dev *pdc_wdt;
> +
> +	pdc_wdt = devm_kzalloc(&pdev->dev, sizeof(*pdc_wdt), GFP_KERNEL);
> +	if (!pdc_wdt)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pdc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pdc_wdt->base))
> +		return PTR_ERR(pdc_wdt->base);
> +
> +	pdc_wdt->sys_clk = devm_clk_get(&pdev->dev, "sys");
> +	if (IS_ERR(pdc_wdt->sys_clk)) {
> +		dev_err(&pdev->dev, "failed to get the sys clock.\n");
> +		ret = PTR_ERR(pdc_wdt->wdt_clk);

					sys_clk
> +		return ret;

		return PTR_ERR(pdc_wdt->sys_clk);

On a higher level, what is sys_clk needed and used for in the first place ?
The need for wdt_clk is obvious, but there is no explanation for the need
of sys_clk, and the bindings file doesn't give a hint either.

> +	}
> +
> +	pdc_wdt->wdt_clk = devm_clk_get(&pdev->dev, "wdt");
> +	if (IS_ERR(pdc_wdt->wdt_clk)) {
> +		dev_err(&pdev->dev, "failed to get the wdt clock.\n");
> +		ret = PTR_ERR(pdc_wdt->wdt_clk);
> +		return ret;

		return PTR_ERR(pdc_wdt->wdt_clk);

> +	}
> +
> +	ret = clk_prepare_enable(pdc_wdt->sys_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not prepare or enable sys clock.\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(pdc_wdt->wdt_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not prepare or enable wdt clock.\n");
> +		goto disable_sys_clk;

I would suggest to drop the '.' in those messages.

> +	}
> +
> +	/* We use the clock rate to calculate the max timeout */
> +	clk_rate = clk_get_rate(pdc_wdt->wdt_clk);
> +	if (clk_rate == 0) {
> +		dev_err(&pdev->dev, "failed to get clock rate\n");
> +		ret = -EINVAL;
> +		goto disable_wdt_clk;
> +	}
> +
> +	if (order_base_2(clk_rate) > PDC_WD_CONFIG_DELAY_MASK + 1) {
> +		dev_err(&pdev->dev, "invalid clock rate\n");
> +		ret = -EINVAL;
> +		goto disable_wdt_clk;
> +	}
> +
> +	if (order_base_2(clk_rate) == 0)
> +		pdc_wdt->wdt_dev.min_timeout = PDC_WD_MIN_TIMEOUT + 1;
> +	else
> +		pdc_wdt->wdt_dev.min_timeout = PDC_WD_MIN_TIMEOUT;
> +
> +	pdc_wdt->min_delay = order_base_2(clk_rate) - 1;

Ultimately this means that min_delay can be smaller than 0,
which makes it quite difficult to validate the rest of the code
to ensure that it handles this case correctly.

Give that a clock rate of 1 is somewhere between highly unlikely and
unreasonable, wouldn't it be much easier to consider it to be invalid
and not try to handle it ?

> +
> +	pdc_wdt->wdt_dev.info = &pdc_wdt_info;
> +	pdc_wdt->wdt_dev.ops = &pdc_wdt_ops;
> +	pdc_wdt->wdt_dev.max_timeout =
> +			(1 << (PDC_WD_CONFIG_DELAY_MASK - pdc_wdt->min_delay));
> +	pdc_wdt->wdt_dev.parent = &pdev->dev;
> +
> +	ret = watchdog_init_timeout(&pdc_wdt->wdt_dev, timeout, &pdev->dev);
> +	if (ret < 0) {
> +		pdc_wdt->wdt_dev.timeout = pdc_wdt->wdt_dev.max_timeout;
> +		dev_warn(&pdev->dev,
> +			 "Initial timeout out of range! setting max timeout\n");
> +	}
> +
> +	pdc_wdt_stop(&pdc_wdt->wdt_dev);
> +
> +	/* Find what caused the last reset */
> +	val = readl(pdc_wdt->base + PDC_WD_TICKLE1);
> +	val = (val & PDC_WD_TICKLE_STATUS_MASK) >> PDC_WD_TICKLE_STATUS_SHIFT;
> +	switch (val) {
> +	case PDC_WD_TICKLE_STATUS_TICKLE:
> +	case PDC_WD_TICKLE_STATUS_TIMEOUT:
> +		pdc_wdt->wdt_dev.bootstatus |= WDIOF_CARDRESET;
> +		dev_info(&pdev->dev,
> +			 "watchdog module last reset due to timeout\n");
> +		break;
> +	case PDC_WD_TICKLE_STATUS_HRESET:
> +		dev_info(&pdev->dev,
> +			 "watchdog module last reset due to hard reset\n");
> +		break;
> +	case PDC_WD_TICKLE_STATUS_SRESET:
> +		dev_info(&pdev->dev,
> +			 "watchdog module last reset due to soft reset\n");
> +		break;
> +	case PDC_WD_TICKLE_STATUS_USER:
> +		dev_info(&pdev->dev,
> +			 "watchdog module last reset due to user reset\n");
> +		break;
> +	default:
> +		dev_info(&pdev->dev,
> +			 "contains an illegal status code (%08x)\n", val);
> +		break;
> +	}
> +
> +	watchdog_set_nowayout(&pdc_wdt->wdt_dev, nowayout);
> +
> +	platform_set_drvdata(pdev, pdc_wdt);
> +	watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
> +
> +	ret = watchdog_register_device(&pdc_wdt->wdt_dev);
> +	if (ret)
> +		goto disable_wdt_clk;
> +
> +	return 0;
> +
> +disable_wdt_clk:
> +	clk_disable_unprepare(pdc_wdt->wdt_clk);
> +disable_sys_clk:
> +	clk_disable_unprepare(pdc_wdt->sys_clk);
> +	return ret;
> +}
> +
> +static void pdc_wdt_shutdown(struct platform_device *pdev)
> +{
> +	struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> +
> +	pdc_wdt_stop(&pdc_wdt->wdt_dev);
> +}
> +
> +static int pdc_wdt_remove(struct platform_device *pdev)
> +{
> +	struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> +
> +	pdc_wdt_stop(&pdc_wdt->wdt_dev);
> +	watchdog_unregister_device(&pdc_wdt->wdt_dev);
> +	clk_disable_unprepare(pdc_wdt->wdt_clk);
> +	clk_disable_unprepare(pdc_wdt->sys_clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pdc_wdt_match[] = {
> +	{ .compatible = "img,pdc-wdt" },

Will "img" be added to the list of vendor prefixes with some other patch ?

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pdc_wdt_match);
> +
> +static struct platform_driver pdc_wdt_driver = {
> +	.driver = {
> +		.name = "imgpdc-wdt",
> +		.of_match_table	= pdc_wdt_match,
> +	},
> +	.probe = pdc_wdt_probe,
> +	.remove = pdc_wdt_remove,
> +	.shutdown = pdc_wdt_shutdown,
> +};
> +module_platform_driver(pdc_wdt_driver);
> +
> +MODULE_AUTHOR("Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> +MODULE_AUTHOR("Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Imagination Technologies PDC Watchdog Timer Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.0.4
> 
--
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 RESEND v6 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

[James Hartley]

Hi Guenter, 

Thanks for your comments, 

> -----Original Message-----
> From: Guenter Roeck [mailto:linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org]
> Sent: 15 December 2014 17:18
> To: Naidu Tellapati
> Cc: wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org; abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org; Ezequiel Garcia; James Hartley;
> James Hogan; Naidu Tellapati; Arul Ramasamy; linux-
> watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jude Abraham
> Subject: Re: [PATCH RESEND v6 1/2] watchdog: ImgTec PDC Watchdog Timer
> Driver
> 
> On Thu, Nov 27, 2014 at 10:24:04AM +0530, Naidu Tellapati wrote:
> > This commit adds support for ImgTec PowerDown Controller Watchdog
> Timer.
> >
> > Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Jude.Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Reviewed-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/watchdog/Kconfig      |   11 ++
> >  drivers/watchdog/Makefile     |    1 +
> >  drivers/watchdog/imgpdc_wdt.c |  298
> > +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 310 insertions(+), 0 deletions(-)  create mode
> > 100644 drivers/watchdog/imgpdc_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > d0107d4..a184f23 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1235,6 +1235,17 @@ config BCM_KONA_WDT_DEBUG
> >
> >  	  If in doubt, say 'N'.
> >
> > +config IMGPDC_WDT
> > +	tristate "Imagination Technologies PDC Watchdog Timer"
> > +	depends on HAS_IOMEM
> > +	depends on METAG || MIPS || COMPILE_TEST
> > +	help
> > +	  Driver for Imagination Technologies PowerDown Controller
> > +	  Watchdog Timer.
> > +
> > +	  To compile this driver as a loadable module, choose M here.
> > +	  The module will be called imgpdc_wdt.
> > +
> >  config LANTIQ_WDT
> >  	tristate "Lantiq SoC watchdog"
> >  	depends on LANTIQ
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index c569ec8..d4dfbb4 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -142,6 +142,7 @@ obj-$(CONFIG_OCTEON_WDT) += octeon-wdt.o
> > octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o
> >  obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o
> >  obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o
> > +obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
> >
> >  # PARISC Architecture
> >
> > diff --git a/drivers/watchdog/imgpdc_wdt.c
> > b/drivers/watchdog/imgpdc_wdt.c new file mode 100644 index
> > 0000000..efb00b5
> > --- /dev/null
> > +++ b/drivers/watchdog/imgpdc_wdt.c
> > @@ -0,0 +1,298 @@
> > +/*
> > + * Imagination Technologies PowerDown Controller Watchdog Timer.
> > + *
> > + * Copyright (c) 2014 Imagination Technologies Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms of the GNU General Public License version 2 as
> > +published by
> > + * the Free Software Foundation.
> > + *
> > + * Based on drivers/watchdog/sunxi_wdt.c Copyright (c) 2013 Carlo
> Caione
> > + *                                                     2012 Henrik Nordstrom
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/log2.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/watchdog.h>
> > +
> > +/* registers */
> > +#define PDC_WD_SW_RESET			0x000
> > +
> > +#define PDC_WD_CONFIG			0x004
> > +#define PDC_WD_CONFIG_ENABLE		BIT(31)
> > +#define PDC_WD_CONFIG_DELAY_MASK	0x0000001f
> > +#define PDC_WD_CONFIG_DELAY_SHIFT	0
> > +
> > +#define PDC_WD_TICKLE1			0x008
> > +#define PDC_WD_TICKLE1_MAGIC		0xabcd1234
> > +
> > +#define PDC_WD_TICKLE2			0x00c
> > +#define PDC_WD_TICKLE2_MAGIC		0x4321dcba
> > +
> > +#define PDC_WD_TICKLE_STATUS_MASK	0x00000007
> > +#define PDC_WD_TICKLE_STATUS_SHIFT	0
> > +#define PDC_WD_TICKLE_STATUS_HRESET     0x0  /* Hard reset */
> > +#define PDC_WD_TICKLE_STATUS_TIMEOUT    0x1  /* Timeout */
> > +#define PDC_WD_TICKLE_STATUS_TICKLE     0x2  /* Tickled incorrectly */
> > +#define PDC_WD_TICKLE_STATUS_SRESET     0x3  /* Soft reset */
> > +#define PDC_WD_TICKLE_STATUS_USER       0x4  /* User reset */
> > +
> > +/* timeout in seconds */
> > +#define PDC_WD_MIN_TIMEOUT		1
> > +#define PDC_WD_DEFAULT_TIMEOUT		64
> > +
> > +static int timeout = PDC_WD_DEFAULT_TIMEOUT;
> module_param(timeout,
> > +int, 0); MODULE_PARM_DESC(timeout, "PDC watchdog delay in seconds
> > +(default 64s)");
> 
> It might be better to use
> __MODULE_STRING(PDC_WD_DEFAULT_TIMEOUT)
> to encode the default timeout.
> 
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout,
> > +bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be
> stopped once
> > +started");
> > +
> > +struct pdc_wdt_dev {
> > +	struct watchdog_device wdt_dev;
> > +	struct clk *wdt_clk;
> > +	struct clk *sys_clk;
> > +	int min_delay;
> > +	void __iomem *base;
> > +};
> > +
> > +static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev) {
> > +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +	writel(PDC_WD_TICKLE1_MAGIC, wdt->base + PDC_WD_TICKLE1);
> > +	writel(PDC_WD_TICKLE2_MAGIC, wdt->base + PDC_WD_TICKLE2);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pdc_wdt_stop(struct watchdog_device *wdt_dev) {
> > +	unsigned int val;
> > +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +	val = readl(wdt->base + PDC_WD_CONFIG);
> > +	val &= ~PDC_WD_CONFIG_ENABLE;
> > +	writel(val, wdt->base + PDC_WD_CONFIG);
> > +
> > +	/* Must tickle to finish the stop */
> > +	pdc_wdt_keepalive(wdt_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pdc_wdt_set_timeout(struct watchdog_device *wdt_dev,
> > +			       unsigned int new_timeout)
> > +{
> > +	unsigned int val;
> > +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +	wdt->wdt_dev.timeout = new_timeout;
> > +	/* round up to the next power of 2 */
> > +	new_timeout = order_base_2(new_timeout);
> > +	val = readl(wdt->base + PDC_WD_CONFIG);
> > +	val &= ~PDC_WD_CONFIG_DELAY_MASK;
> > +	val |= (new_timeout + wdt->min_delay) <<
> PDC_WD_CONFIG_DELAY_SHIFT;
> > +	writel(val, wdt->base + PDC_WD_CONFIG);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Start the watchdog timer (delay should already be set) */ static
> > +int pdc_wdt_start(struct watchdog_device *wdt_dev) {
> > +	unsigned int val;
> > +	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
> > +
> > +	val = readl(wdt->base + PDC_WD_CONFIG);
> > +	val |= PDC_WD_CONFIG_ENABLE;
> > +	writel(val, wdt->base + PDC_WD_CONFIG);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct watchdog_info pdc_wdt_info = {
> > +	.identity	= "IMG PDC Watchdog",
> > +	.options	= WDIOF_SETTIMEOUT |
> > +			  WDIOF_KEEPALIVEPING |
> > +			  WDIOF_MAGICCLOSE,
> > +};
> > +
> > +/* Kernel interface */
> > +static const struct watchdog_ops pdc_wdt_ops = {
> > +	.owner		= THIS_MODULE,
> > +	.start		= pdc_wdt_start,
> > +	.stop		= pdc_wdt_stop,
> > +	.ping		= pdc_wdt_keepalive,
> > +	.set_timeout	= pdc_wdt_set_timeout,
> > +};
> > +
> > +static int pdc_wdt_probe(struct platform_device *pdev) {
> > +	int ret, val;
> > +	unsigned long clk_rate;
> > +	struct resource *res;
> > +	struct pdc_wdt_dev *pdc_wdt;
> > +
> > +	pdc_wdt = devm_kzalloc(&pdev->dev, sizeof(*pdc_wdt),
> GFP_KERNEL);
> > +	if (!pdc_wdt)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	pdc_wdt->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(pdc_wdt->base))
> > +		return PTR_ERR(pdc_wdt->base);
> > +
> > +	pdc_wdt->sys_clk = devm_clk_get(&pdev->dev, "sys");
> > +	if (IS_ERR(pdc_wdt->sys_clk)) {
> > +		dev_err(&pdev->dev, "failed to get the sys clock.\n");
> > +		ret = PTR_ERR(pdc_wdt->wdt_clk);
> 
> 					sys_clk
> > +		return ret;
> 
> 		return PTR_ERR(pdc_wdt->sys_clk);
> 
> On a higher level, what is sys_clk needed and used for in the first place ?
> The need for wdt_clk is obvious, but there is no explanation for the need of
> sys_clk, and the bindings file doesn't give a hint either.

I believe that the watchdog registers are clocked from sys_clk, so without that they are not accessible.  

> 
> > +	}
> > +
> > +	pdc_wdt->wdt_clk = devm_clk_get(&pdev->dev, "wdt");
> > +	if (IS_ERR(pdc_wdt->wdt_clk)) {
> > +		dev_err(&pdev->dev, "failed to get the wdt clock.\n");
> > +		ret = PTR_ERR(pdc_wdt->wdt_clk);
> > +		return ret;
> 
> 		return PTR_ERR(pdc_wdt->wdt_clk);
> 
> > +	}
> > +
> > +	ret = clk_prepare_enable(pdc_wdt->sys_clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "could not prepare or enable sys
> clock.\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(pdc_wdt->wdt_clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "could not prepare or enable wdt
> clock.\n");
> > +		goto disable_sys_clk;
> 
> I would suggest to drop the '.' in those messages.
> 
> > +	}
> > +
> > +	/* We use the clock rate to calculate the max timeout */
> > +	clk_rate = clk_get_rate(pdc_wdt->wdt_clk);
> > +	if (clk_rate == 0) {
> > +		dev_err(&pdev->dev, "failed to get clock rate\n");
> > +		ret = -EINVAL;
> > +		goto disable_wdt_clk;
> > +	}
> > +
> > +	if (order_base_2(clk_rate) > PDC_WD_CONFIG_DELAY_MASK + 1) {
> > +		dev_err(&pdev->dev, "invalid clock rate\n");
> > +		ret = -EINVAL;
> > +		goto disable_wdt_clk;
> > +	}
> > +
> > +	if (order_base_2(clk_rate) == 0)
> > +		pdc_wdt->wdt_dev.min_timeout =
> PDC_WD_MIN_TIMEOUT + 1;
> > +	else
> > +		pdc_wdt->wdt_dev.min_timeout =
> PDC_WD_MIN_TIMEOUT;
> > +
> > +	pdc_wdt->min_delay = order_base_2(clk_rate) - 1;
> 
> Ultimately this means that min_delay can be smaller than 0, which makes it
> quite difficult to validate the rest of the code to ensure that it handles this
> case correctly.
> 
> Give that a clock rate of 1 is somewhere between highly unlikely and
> unreasonable, wouldn't it be much easier to consider it to be invalid and not
> try to handle it ?

Sounds reasonable to me.

> 
> > +
> > +	pdc_wdt->wdt_dev.info = &pdc_wdt_info;
> > +	pdc_wdt->wdt_dev.ops = &pdc_wdt_ops;
> > +	pdc_wdt->wdt_dev.max_timeout =
> > +			(1 << (PDC_WD_CONFIG_DELAY_MASK - pdc_wdt-
> >min_delay));
> > +	pdc_wdt->wdt_dev.parent = &pdev->dev;
> > +
> > +	ret = watchdog_init_timeout(&pdc_wdt->wdt_dev, timeout, &pdev-
> >dev);
> > +	if (ret < 0) {
> > +		pdc_wdt->wdt_dev.timeout = pdc_wdt-
> >wdt_dev.max_timeout;
> > +		dev_warn(&pdev->dev,
> > +			 "Initial timeout out of range! setting max
> timeout\n");
> > +	}
> > +
> > +	pdc_wdt_stop(&pdc_wdt->wdt_dev);
> > +
> > +	/* Find what caused the last reset */
> > +	val = readl(pdc_wdt->base + PDC_WD_TICKLE1);
> > +	val = (val & PDC_WD_TICKLE_STATUS_MASK) >>
> PDC_WD_TICKLE_STATUS_SHIFT;
> > +	switch (val) {
> > +	case PDC_WD_TICKLE_STATUS_TICKLE:
> > +	case PDC_WD_TICKLE_STATUS_TIMEOUT:
> > +		pdc_wdt->wdt_dev.bootstatus |= WDIOF_CARDRESET;
> > +		dev_info(&pdev->dev,
> > +			 "watchdog module last reset due to timeout\n");
> > +		break;
> > +	case PDC_WD_TICKLE_STATUS_HRESET:
> > +		dev_info(&pdev->dev,
> > +			 "watchdog module last reset due to hard reset\n");
> > +		break;
> > +	case PDC_WD_TICKLE_STATUS_SRESET:
> > +		dev_info(&pdev->dev,
> > +			 "watchdog module last reset due to soft reset\n");
> > +		break;
> > +	case PDC_WD_TICKLE_STATUS_USER:
> > +		dev_info(&pdev->dev,
> > +			 "watchdog module last reset due to user reset\n");
> > +		break;
> > +	default:
> > +		dev_info(&pdev->dev,
> > +			 "contains an illegal status code (%08x)\n", val);
> > +		break;
> > +	}
> > +
> > +	watchdog_set_nowayout(&pdc_wdt->wdt_dev, nowayout);
> > +
> > +	platform_set_drvdata(pdev, pdc_wdt);
> > +	watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
> > +
> > +	ret = watchdog_register_device(&pdc_wdt->wdt_dev);
> > +	if (ret)
> > +		goto disable_wdt_clk;
> > +
> > +	return 0;
> > +
> > +disable_wdt_clk:
> > +	clk_disable_unprepare(pdc_wdt->wdt_clk);
> > +disable_sys_clk:
> > +	clk_disable_unprepare(pdc_wdt->sys_clk);
> > +	return ret;
> > +}
> > +
> > +static void pdc_wdt_shutdown(struct platform_device *pdev) {
> > +	struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> > +
> > +	pdc_wdt_stop(&pdc_wdt->wdt_dev);
> > +}
> > +
> > +static int pdc_wdt_remove(struct platform_device *pdev) {
> > +	struct pdc_wdt_dev *pdc_wdt = platform_get_drvdata(pdev);
> > +
> > +	pdc_wdt_stop(&pdc_wdt->wdt_dev);
> > +	watchdog_unregister_device(&pdc_wdt->wdt_dev);
> > +	clk_disable_unprepare(pdc_wdt->wdt_clk);
> > +	clk_disable_unprepare(pdc_wdt->sys_clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id pdc_wdt_match[] = {
> > +	{ .compatible = "img,pdc-wdt" },
> 
> Will "img" be added to the list of vendor prefixes with some other patch ?

I think Img is already in the list of vendor prefixes.

> 
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, pdc_wdt_match);
> > +
> > +static struct platform_driver pdc_wdt_driver = {
> > +	.driver = {
> > +		.name = "imgpdc-wdt",
> > +		.of_match_table	= pdc_wdt_match,
> > +	},
> > +	.probe = pdc_wdt_probe,
> > +	.remove = pdc_wdt_remove,
> > +	.shutdown = pdc_wdt_shutdown,
> > +};
> > +module_platform_driver(pdc_wdt_driver);
> > +
> > +MODULE_AUTHOR("Jude Abraham <Jude.Abraham-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> > +MODULE_AUTHOR("Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> > +MODULE_DESCRIPTION("Imagination Technologies PDC Watchdog Timer
> > +Driver"); MODULE_LICENSE("GPL v2");
> > --
> > 1.7.0.4
> >

James.
--
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