[PATCH v2 1/2] watchdog: Add Cadence WDT driver

[PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Harini Katakam]

Add Cadence WDT driver. This is used by Xilinx Zynq.

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---

Sorry for the delay in sending v2.
v2 changes:
- Update IO helpers.
- Use dev_dbg instead of dev_info where possible.
- Use watchdog_init_timeout
- Spinlock for restart register where missing.
- Change order of probe to move reboot notifier register to the end
- Remove unecessary comments
- Do clk_prepare_enabel and clk_disable_unprepare in resume/suspend
  respectively.
- There is an input from dts to decide whether internal reset should be
  enabled or not. When this is enabled, reset happens wutomatically when
  counter reaches zero. In case this is not enabled, a message is disaplayed
  to indicate that the watchdog has timed out. Elaborated this message
  to describe the above.

---
 drivers/watchdog/Kconfig       |    7 +
 drivers/watchdog/Makefile      |    1 +
 drivers/watchdog/cadence_wdt.c |  517 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 525 insertions(+)
 create mode 100644 drivers/watchdog/cadence_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 74ec8fc..7df70c1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -147,6 +147,13 @@ config AT91SAM9X_WATCHDOG
 	  Watchdog timer embedded into AT91SAM9X and AT91CAP9 chips. This will
 	  reboot your system when the timeout is reached.
 
+config CADENCE_WATCHDOG
+	tristate "Cadence Watchdog Timer"
+	select WATCHDOG_CORE
+	help
+	  Say Y here if you want to include support for the watchdog
+	  timer in the Xilinx Zynq.
+
 config 21285_WATCHDOG
 	tristate "DC21285 watchdog"
 	depends on FOOTBRIDGE
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 1b5f3d5..a0d9912 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
 obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
 obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
 obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
+obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
 obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o
 obj-$(CONFIG_TWL4030_WATCHDOG) += twl4030_wdt.o
 obj-$(CONFIG_21285_WATCHDOG) += wdt285.o
diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
new file mode 100644
index 0000000..0b26369
--- /dev/null
+++ b/drivers/watchdog/cadence_wdt.c
@@ -0,0 +1,517 @@
+/*
+ * Cadence WDT driver - Used by Xilinx Zynq
+ *
+ * Copyright (C) 2010 - 2014 Xilinx, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define CDNS_WDT_DEFAULT_TIMEOUT	10
+/* Supports 1 - 516 sec */
+#define CDNS_WDT_MIN_TIMEOUT	1
+#define CDNS_WDT_MAX_TIMEOUT	516
+
+/* Restart key */
+#define CDNS_WDT_RESTART_KEY 0x00001999
+
+/* Counter register access key */
+#define CDNS_WDT_REGISTER_ACCESS_KEY 0x00920000
+
+/* Counter value divisor */
+#define CDNS_WDT_COUNTER_VALUE_DIVISOR 0x1000
+
+/* Clock prescaler value and selection */
+#define CDNS_WDT_PRESCALE_64	64
+#define CDNS_WDT_PRESCALE_512	512
+#define CDNS_WDT_PRESCALE_4096	4096
+#define CDNS_WDT_PRESCALE_SELECT_64	1
+#define CDNS_WDT_PRESCALE_SELECT_512	2
+#define CDNS_WDT_PRESCALE_SELECT_4096	3
+
+/* Input clock frequency */
+#define CDNS_WDT_CLK_10MHZ	10000000
+#define CDNS_WDT_CLK_75MHZ	75000000
+
+/* Counter maximum value */
+#define CDNS_WDT_COUNTER_MAX 0xFFF
+
+static int wdt_timeout = CDNS_WDT_DEFAULT_TIMEOUT;
+static int nowayout = WATCHDOG_NOWAYOUT;
+
+module_param(wdt_timeout, int, 0);
+MODULE_PARM_DESC(wdt_timeout,
+		 "Watchdog time in seconds. (default="
+		 __MODULE_STRING(CDNS_WDT_DEFAULT_TIMEOUT) ")");
+
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout,
+		 "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+/**
+ * struct cdns_wdt - Watchdog device structure
+ * @regs: baseaddress of device
+ * @rst: reset flag
+ * @clk: struct clk * of a clock source
+ * @prescaler: for saving prescaler value
+ * @ctrl_clksel: counter clock prescaler selection
+ * @io_lock: spinlock for IO register access
+ * @cdns_wdt_device: watchdog device structure
+ * @cdns_wdt_notifier: notifier structure
+ *
+ * Structure containing parameters specific to cadence watchdog.
+ */
+struct cdns_wdt {
+	void __iomem		*regs;
+	u32			rst;
+	struct clk		*clk;
+	u32			prescaler;
+	u32			ctrl_clksel;
+	spinlock_t		io_lock;
+	struct watchdog_device	cdns_wdt_device;
+	struct notifier_block	cdns_wdt_notifier;
+};
+
+/* Write access to Registers */
+static inline void cdns_wdt_writereg(struct cdns_wdt *wdt, u32 offset, u32 val)
+{
+	writel_relaxed(val, wdt->regs + offset);
+}
+
+/*************************Register Map**************************************/
+
+/* Register Offsets for the WDT */
+#define CDNS_WDT_ZMR_OFFSET	0x0	/* Zero Mode Register */
+#define CDNS_WDT_CCR_OFFSET	0x4	/* Counter Control Register */
+#define CDNS_WDT_RESTART_OFFSET	0x8	/* Restart Register */
+#define CDNS_WDT_SR_OFFSET	0xC	/* Status Register */
+
+/*
+ * Zero Mode Register - This register controls how the time out is indicated
+ * and also contains the access code to allow writes to the register (0xABC).
+ */
+#define CDNS_WDT_ZMR_WDEN_MASK	0x00000001 /* Enable the WDT */
+#define CDNS_WDT_ZMR_RSTEN_MASK	0x00000002 /* Enable the reset output */
+#define CDNS_WDT_ZMR_IRQEN_MASK	0x00000004 /* Enable IRQ output */
+#define CDNS_WDT_ZMR_RSTLEN_16	0x00000030 /* Reset pulse of 16 pclk cycles */
+#define CDNS_WDT_ZMR_ZKEY_VAL	0x00ABC000 /* Access key, 0xABC << 12 */
+/*
+ * Counter Control register - This register controls how fast the timer runs
+ * and the reset value and also contains the access code to allow writes to
+ * the register.
+ */
+#define CDNS_WDT_CCR_CRV_MASK	0x00003FFC /* Counter reset value */
+
+/**
+ * cdns_wdt_stop - Stop the watchdog.
+ *
+ * @wdd: watchdog device
+ *
+ * Read the contents of the ZMR register, clear the WDEN bit
+ * in the register and set the access key for successful write.
+ *
+ * Return: always 0
+ */
+static int cdns_wdt_stop(struct watchdog_device *wdd)
+{
+	struct cdns_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	spin_lock(&wdt->io_lock);
+	cdns_wdt_writereg(wdt, CDNS_WDT_ZMR_OFFSET,
+			  CDNS_WDT_ZMR_ZKEY_VAL & (~CDNS_WDT_ZMR_WDEN_MASK));
+	spin_unlock(&wdt->io_lock);
+
+	return 0;
+}
+
+/**
+ * cdns_wdt_reload - Reload the watchdog timer (i.e. pat the watchdog).
+ *
+ * @wdd: watchdog device
+ *
+ * Write the restart key value (0x00001999) to the restart register.
+ *
+ * Return: always 0
+ */
+static int cdns_wdt_reload(struct watchdog_device *wdd)
+{
+	struct cdns_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	spin_lock(&wdt->io_lock);
+	cdns_wdt_writereg(wdt, CDNS_WDT_RESTART_OFFSET,
+			  CDNS_WDT_RESTART_KEY);
+	spin_unlock(&wdt->io_lock);
+
+	return 0;
+}
+
+/**
+ * cdns_wdt_start - Enable and start the watchdog.
+ *
+ * @wdd: watchdog device
+ *
+ * The counter value is calculated according to the formula:
+ *		calculated count = (timeout * clock) / prescaler + 1.
+ * The calculated count is divided by 0x1000 to obtain the field value
+ * to write to counter control register.
+ * Clears the contents of prescaler and counter reset value. Sets the
+ * prescaler to 4096 and the calculated count and access key
+ * to write to CCR Register.
+ * Sets the WDT (WDEN bit) and either the Reset signal(RSTEN bit)
+ * or Interrupt signal(IRQEN) with a specified cycles and the access
+ * key to write to ZMR Register.
+ *
+ * Return: always 0
+ */
+static int cdns_wdt_start(struct watchdog_device *wdd)
+{
+	struct cdns_wdt *wdt = watchdog_get_drvdata(wdd);
+	unsigned int data = 0;
+	unsigned short count;
+	unsigned long clock_f = clk_get_rate(wdt->clk);
+
+	/*
+	 * Counter value divisor to obtain the value of
+	 * counter reset to be written to control register.
+	 */
+	count = (wdd->timeout * (clock_f / wdt->prescaler)) /
+		 CDNS_WDT_COUNTER_VALUE_DIVISOR + 1;
+
+	if (count > CDNS_WDT_COUNTER_MAX)
+		count = CDNS_WDT_COUNTER_MAX;
+
+	spin_lock(&wdt->io_lock);
+	cdns_wdt_writereg(wdt, CDNS_WDT_ZMR_OFFSET,
+			  CDNS_WDT_ZMR_ZKEY_VAL);
+
+	count = (count << 2) & CDNS_WDT_CCR_CRV_MASK;
+
+	/* Write counter access key first to be able write to register */
+	data = count | CDNS_WDT_REGISTER_ACCESS_KEY | wdt->ctrl_clksel;
+	cdns_wdt_writereg(wdt, CDNS_WDT_CCR_OFFSET, data);
+	data = CDNS_WDT_ZMR_WDEN_MASK | CDNS_WDT_ZMR_RSTLEN_16 |
+	       CDNS_WDT_ZMR_ZKEY_VAL;
+
+	/* Reset on timeout if specified in device tree. */
+	if (wdt->rst) {
+		data |= CDNS_WDT_ZMR_RSTEN_MASK;
+		data &= ~CDNS_WDT_ZMR_IRQEN_MASK;
+	} else {
+		data &= ~CDNS_WDT_ZMR_RSTEN_MASK;
+		data |= CDNS_WDT_ZMR_IRQEN_MASK;
+	}
+	cdns_wdt_writereg(wdt, CDNS_WDT_ZMR_OFFSET, data);
+	cdns_wdt_writereg(wdt, CDNS_WDT_RESTART_OFFSET,
+			  CDNS_WDT_RESTART_KEY);
+	spin_unlock(&wdt->io_lock);
+
+	return 0;
+}
+
+/**
+ * cdns_wdt_settimeout - Set a new timeout value for the watchdog device.
+ *
+ * @wdd: watchdog device
+ * @new_time: new timeout value that needs to be set
+ * Return: 0 on success
+ *
+ * Update the watchdog_device timeout with new value which is used when
+ * cdns_wdt_start is called.
+ */
+static int cdns_wdt_settimeout(struct watchdog_device *wdd,
+			       unsigned int new_time)
+{
+	wdd->timeout = new_time;
+
+	return cdns_wdt_start(wdd);
+}
+
+/**
+ * cdns_wdt_irq_handler - Notifies of watchdog timeout.
+ *
+ * @irq: interrupt number
+ * @dev_id: pointer to a platform device structure
+ * Return: IRQ_HANDLED
+ *
+ * The handler is invoked when the watchdog times out and a
+ * reset on timeout has not been enabled.
+ */
+static irqreturn_t cdns_wdt_irq_handler(int irq, void *dev_id)
+{
+	struct platform_device *pdev = dev_id;
+
+	dev_info(&pdev->dev,
+		 "Watchdog timed out. Internal reset not enabled\n");
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Info structure used to indicate the features supported by the device
+ * to the upper layers. This is defined in watchdog.h header file.
+ */
+static struct watchdog_info cdns_wdt_info = {
+	.identity	= "cdns_wdt watchdog",
+	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE,
+};
+
+/* Watchdog Core Ops */
+static struct watchdog_ops cdns_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = cdns_wdt_start,
+	.stop = cdns_wdt_stop,
+	.ping = cdns_wdt_reload,
+	.set_timeout = cdns_wdt_settimeout,
+};
+
+/**
+ * cdns_wdt_notify_sys - Notifier for reboot or shutdown.
+ *
+ * @this: handle to notifier block
+ * @code: turn off indicator
+ * @unused: unused
+ * Return: NOTIFY_DONE
+ *
+ * This notifier is invoked whenever the system reboot or shutdown occur
+ * because we need to disable the WDT before system goes down as WDT might
+ * reset on the next boot.
+ */
+static int cdns_wdt_notify_sys(struct notifier_block *this, unsigned long code,
+			       void *unused)
+{
+	struct cdns_wdt *wdt = container_of(this, struct cdns_wdt,
+					    cdns_wdt_notifier);
+	if (code == SYS_DOWN || code == SYS_HALT)
+		cdns_wdt_stop(&wdt->cdns_wdt_device);
+
+	return NOTIFY_DONE;
+}
+
+/************************Platform Operations*****************************/
+/**
+ * cdns_wdt_probe - Probe call for the device.
+ *
+ * @pdev: handle to the platform device structure.
+ * Return: 0 on success, negative error otherwise.
+ *
+ * It does all the memory allocation and registration for the device.
+ */
+static int cdns_wdt_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	int ret, irq;
+	unsigned long clock_f;
+	struct cdns_wdt *wdt;
+	struct watchdog_device *cdns_wdt_device;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	cdns_wdt_device = &wdt->cdns_wdt_device;
+	cdns_wdt_device->info = &cdns_wdt_info;
+	cdns_wdt_device->ops = &cdns_wdt_ops;
+	cdns_wdt_device->timeout = CDNS_WDT_DEFAULT_TIMEOUT;
+	cdns_wdt_device->min_timeout = CDNS_WDT_MIN_TIMEOUT;
+	cdns_wdt_device->max_timeout = CDNS_WDT_MAX_TIMEOUT;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(wdt->regs))
+		return PTR_ERR(wdt->regs);
+
+	/* Register the interrupt */
+	of_property_read_u32(pdev->dev.of_node, "reset", &wdt->rst);
+	irq = platform_get_irq(pdev, 0);
+	if (!wdt->rst && irq >= 0) {
+		ret = devm_request_irq(&pdev->dev, irq, cdns_wdt_irq_handler, 0,
+				       pdev->name, pdev);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"cannot register interrupt handler err=%d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	/* Initialize the members of cdns_wdt structure */
+	cdns_wdt_device->parent = &pdev->dev;
+
+	ret = watchdog_init_timeout(cdns_wdt_device, wdt_timeout, &pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to set timeout value\n");
+		return ret;
+	}
+
+	watchdog_set_nowayout(cdns_wdt_device, nowayout);
+	watchdog_set_drvdata(cdns_wdt_device, wdt);
+
+	wdt->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(wdt->clk)) {
+		dev_err(&pdev->dev, "input clock not found\n");
+		ret = PTR_ERR(wdt->clk);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(wdt->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to enable clock\n");
+		return ret;
+	}
+
+	clock_f = clk_get_rate(wdt->clk);
+	if (clock_f <= CDNS_WDT_CLK_75MHZ) {
+		wdt->prescaler = CDNS_WDT_PRESCALE_512;
+		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_512;
+	} else {
+		wdt->prescaler = CDNS_WDT_PRESCALE_4096;
+		wdt->ctrl_clksel = CDNS_WDT_PRESCALE_SELECT_4096;
+	}
+
+	spin_lock_init(&wdt->io_lock);
+
+	wdt->cdns_wdt_notifier.notifier_call = &cdns_wdt_notify_sys;
+	ret = register_reboot_notifier(&wdt->cdns_wdt_notifier);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "cannot register reboot notifier err=%d)\n",
+			ret);
+		goto err_clk_disable;
+	}
+
+	ret = watchdog_register_device(cdns_wdt_device);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register wdt device\n");
+		goto err_clk_disable;
+	}
+	platform_set_drvdata(pdev, wdt);
+
+	dev_dbg(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds%s\n",
+		 wdt->regs, cdns_wdt_device->timeout,
+		 nowayout ? ", nowayout" : "");
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(wdt->clk);
+
+	return ret;
+}
+
+/**
+ * cdns_wdt_remove - Probe call for the device.
+ *
+ * @pdev: handle to the platform device structure.
+ * Return: 0 on success, otherwise negative error.
+ *
+ * Unregister the device after releasing the resources.
+ */
+static int cdns_wdt_remove(struct platform_device *pdev)
+{
+	struct cdns_wdt *wdt = platform_get_drvdata(pdev);
+
+	cdns_wdt_stop(&wdt->cdns_wdt_device);
+	watchdog_unregister_device(&wdt->cdns_wdt_device);
+	unregister_reboot_notifier(&wdt->cdns_wdt_notifier);
+	clk_disable_unprepare(wdt->clk);
+
+	return 0;
+}
+
+/**
+ * cdns_wdt_shutdown - Stop the device.
+ *
+ * @pdev: handle to the platform structure.
+ *
+ */
+static void cdns_wdt_shutdown(struct platform_device *pdev)
+{
+	struct cdns_wdt *wdt = platform_get_drvdata(pdev);
+
+	cdns_wdt_stop(&wdt->cdns_wdt_device);
+	clk_disable_unprepare(wdt->clk);
+}
+
+/**
+ * cdns_wdt_suspend - Stop the device.
+ *
+ * @dev: handle to the device structure.
+ * Return: 0 always.
+ */
+static int __maybe_unused cdns_wdt_suspend(struct device *dev)
+{
+	struct platform_device *pdev = container_of(dev,
+			struct platform_device, dev);
+	struct cdns_wdt *wdt = platform_get_drvdata(pdev);
+
+	cdns_wdt_stop(&wdt->cdns_wdt_device);
+	clk_disable_unprepare(wdt->clk);
+
+	return 0;
+}
+
+/**
+ * cdns_wdt_resume - Resume the device.
+ *
+ * @dev: handle to the device structure.
+ * Return: 0 on success, errno otherwise.
+ */
+static int __maybe_unused cdns_wdt_resume(struct device *dev)
+{
+	int ret;
+	struct platform_device *pdev = container_of(dev,
+			struct platform_device, dev);
+	struct cdns_wdt *wdt = platform_get_drvdata(pdev);
+
+	ret = clk_prepare_enable(wdt->clk);
+	if (ret) {
+		dev_err(dev, "unable to enable clock\n");
+		return ret;
+	}
+	cdns_wdt_start(&wdt->cdns_wdt_device);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cdns_wdt_pm_ops, cdns_wdt_suspend, cdns_wdt_resume);
+
+static struct of_device_id cdns_wdt_of_match[] = {
+	{ .compatible = "xlnx,zynq-wdt-r1p2", },
+	{ .compatible = "cdns,wdt-r1p2", },
+	{ /* end of table */ }
+};
+MODULE_DEVICE_TABLE(of, cdns_wdt_of_match);
+
+/* Driver Structure */
+static struct platform_driver cdns_wdt_driver = {
+	.probe		= cdns_wdt_probe,
+	.remove		= cdns_wdt_remove,
+	.shutdown	= cdns_wdt_shutdown,
+	.driver		= {
+		.name	= "cdns-wdt",
+		.owner	= THIS_MODULE,
+		.of_match_table = cdns_wdt_of_match,
+		.pm	= &cdns_wdt_pm_ops,
+	},
+};
+
+module_platform_driver(cdns_wdt_driver);
+
+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION("Watchdog driver for Cadence WDT");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH v2 2/2] devicetree: Add Cadence WDT devicetree bindings documentation

[Harini Katakam]

Add cadence-wdt bindings documentation.

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---

v2:
No changes

---
 .../devicetree/bindings/watchdog/cadence-wdt.txt   |   26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/cadence-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
new file mode 100644
index 0000000..1f7a732
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
@@ -0,0 +1,26 @@
+Zynq Watchdog Device Tree Bindings
+-------------------------------------------
+
+Required properties:
+- compatible		: Should be "xlnx,zynq-wdt-r1p2" or "cdns,wdt-r1p2".
+- clocks		: Clock phandles (see clock bindings for details).
+- reg			: Physical base address and size of WDT registers map.
+- interrupts		: Property with a value describing the interrupt
+			  number.
+- interrupt-parent	: Must be core interrupt controller.
+
+Optional properties
+- reset			: Reset interrupt.
+- timeout-sec		: Watchdog timeout value (in seconds).
+
+Example:
+
+		wdt@f8005000 {
+			compatible = "xlnx,zynq-wdt-r1p2";
+			clocks = <&clkc 45>;
+			interrupt-parent = <&intc>;
+			interrupts = <0 9 1>;
+			reg = <0xf8005000 0x1000>;
+			reset = <0>;
+			timeout-sec = <10>;
+		} ;
-- 
1.7.9.5

Re: [PATCH v2 2/2] devicetree: Add Cadence WDT devicetree bindings documentation

[Mark Rutland]

On Tue, May 27, 2014 at 11:18:15AM +0100, Harini Katakam wrote:
> Add cadence-wdt bindings documentation.
> 
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> 
> v2:
> No changes
> 
> ---
>  .../devicetree/bindings/watchdog/cadence-wdt.txt   |   26 ++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> new file mode 100644
> index 0000000..1f7a732
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> @@ -0,0 +1,26 @@
> +Zynq Watchdog Device Tree Bindings
> +-------------------------------------------
> +
> +Required properties:
> +- compatible		: Should be "xlnx,zynq-wdt-r1p2" or "cdns,wdt-r1p2".

What's the difference? Can "cdns,wdt-r1p2" be a fallback for
"xlnx,zynq-wdt-r1p2"?

> +- clocks		: Clock phandles (see clock bindings for details).

Nit: Clocks aren't just phandles.

How many clocks? This implies several, the example shows one.

> +- reg			: Physical base address and size of WDT registers map.
> +- interrupts		: Property with a value describing the interrupt
> +			  number.

We know this is a property with a value. Describe what it logically is
(and optionally, the type is interrupt-sepcifier, though that gets
fuzzier with the interrupts-extended property).

> +- interrupt-parent	: Must be core interrupt controller.
> +
> +Optional properties
> +- reset			: Reset interrupt.

What does this mean? What type is this?

I have no idea if this is a boolean telling the OS to reset the
interrupt initially, or an index describing which interrupt is the reset
interrupt in either the interrupts property or some HW registers.

Cheers,
Mark.

> +- timeout-sec		: Watchdog timeout value (in seconds).
> +
> +Example:
> +
> +		wdt@f8005000 {
> +			compatible = "xlnx,zynq-wdt-r1p2";
> +			clocks = <&clkc 45>;
> +			interrupt-parent = <&intc>;
> +			interrupts = <0 9 1>;
> +			reg = <0xf8005000 0x1000>;
> +			reset = <0>;
> +			timeout-sec = <10>;
> +		} ;
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Mark Rutland]

On Tue, May 27, 2014 at 11:18:14AM +0100, Harini Katakam wrote:
> Add Cadence WDT driver. This is used by Xilinx Zynq.
> 
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> 
> Sorry for the delay in sending v2.
> v2 changes:
> - Update IO helpers.
> - Use dev_dbg instead of dev_info where possible.
> - Use watchdog_init_timeout
> - Spinlock for restart register where missing.
> - Change order of probe to move reboot notifier register to the end
> - Remove unecessary comments
> - Do clk_prepare_enabel and clk_disable_unprepare in resume/suspend
>   respectively.
> - There is an input from dts to decide whether internal reset should be
>   enabled or not. When this is enabled, reset happens wutomatically when
>   counter reaches zero. In case this is not enabled, a message is disaplayed
>   to indicate that the watchdog has timed out. Elaborated this message
>   to describe the above.

When is that useful?

[...]

> +       of_property_read_u32(pdev->dev.of_node, "reset", &wdt->rst);

For booleans use an empty property and of_property_read_bool.

[...]

> +static struct of_device_id cdns_wdt_of_match[] = {
> +       { .compatible = "xlnx,zynq-wdt-r1p2", },
> +       { .compatible = "cdns,wdt-r1p2", },

If these can currently be handled identically, why not just have
"cdns,wdt-r1p2" in the driver and in your dts have:

compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";

If we need to distinguish the two for some reason later we can always
add the "xlnx,zynq-wdt-r1p2" string to the driver.

Cheers,
Mark.

Re: [PATCH v2 2/2] devicetree: Add Cadence WDT devicetree bindings documentation

[Harini Katakam]

Hi Mark,

On Thu, May 29, 2014 at 2:42 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, May 27, 2014 at 11:18:15AM +0100, Harini Katakam wrote:
>> Add cadence-wdt bindings documentation.
>>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>> ---
>>
>> v2:
>> No changes

<snip>

>> +Optional properties
>> +- reset                      : Reset interrupt.
>
> What does this mean? What type is this?
>
> I have no idea if this is a boolean telling the OS to reset the
> interrupt initially, or an index describing which interrupt is the reset
> interrupt in either the interrupts property or some HW registers.
>

The watchdog has an option to automatically reset when the counter
reaches zero. This property is an input based on which this automatic
reset is enabled or disabled.
I will improve the description of this property.

Regards,
Harini

Re: [PATCH v2 2/2] devicetree: Add Cadence WDT devicetree bindings documentation

[Michal Simek]

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

On 05/27/2014 12:18 PM, Harini Katakam wrote:
> Add cadence-wdt bindings documentation.
> 
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> 
> v2:
> No changes
> 
> ---
>  .../devicetree/bindings/watchdog/cadence-wdt.txt   |   26 ++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> new file mode 100644
> index 0000000..1f7a732
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> @@ -0,0 +1,26 @@
> +Zynq Watchdog Device Tree Bindings
> +-------------------------------------------
> +
> +Required properties:
> +- compatible		: Should be "xlnx,zynq-wdt-r1p2" or "cdns,wdt-r1p2".
> +- clocks		: Clock phandles (see clock bindings for details).
> +- reg			: Physical base address and size of WDT registers map.
> +- interrupts		: Property with a value describing the interrupt
> +			  number.
> +- interrupt-parent	: Must be core interrupt controller.
> +
> +Optional properties
> +- reset			: Reset interrupt.
> +- timeout-sec		: Watchdog timeout value (in seconds).
> +
> +Example:
> +
> +		wdt@f8005000 {

please use watchdog@ here
which is listed in ePAPR

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Harini Katakam]

Hi Mark,

On Thu, May 29, 2014 at 2:51 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, May 27, 2014 at 11:18:14AM +0100, Harini Katakam wrote:
>> Add Cadence WDT driver. This is used by Xilinx Zynq.
>>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>> ---
>>
>> Sorry for the delay in sending v2.
>> v2 changes:
>> - Update IO helpers.
>> - Use dev_dbg instead of dev_info where possible.
>> - Use watchdog_init_timeout
>> - Spinlock for restart register where missing.
>> - Change order of probe to move reboot notifier register to the end
>> - Remove unecessary comments
>> - Do clk_prepare_enabel and clk_disable_unprepare in resume/suspend
>>   respectively.
>> - There is an input from dts to decide whether internal reset should be
>>   enabled or not. When this is enabled, reset happens wutomatically when
>>   counter reaches zero. In case this is not enabled, a message is disaplayed
>>   to indicate that the watchdog has timed out. Elaborated this message
>>   to describe the above.
>
> When is that useful?
>

Automatic reset enable is an option. When it's not selected, this is a generic
way of giving some indication. If some other action is preferred
instead of reset,
then that can be added here.

Regards,
Harini

Re: [PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Michal Simek]

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

Hi Mark,

>> +static struct of_device_id cdns_wdt_of_match[] = {
>> +       { .compatible = "xlnx,zynq-wdt-r1p2", },
>> +       { .compatible = "cdns,wdt-r1p2", },
> 
> If these can currently be handled identically, why not just have
> "cdns,wdt-r1p2" in the driver and in your dts have:
> 
> compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";
> 
> If we need to distinguish the two for some reason later we can always
> add the "xlnx,zynq-wdt-r1p2" string to the driver.

I would prefer to have 2 compatible strings just because
of that we don't know what is different compare to origin cadence
version. We have done the same for spi-cadence.c that's why
it shouldn't be any problem to keep it as is.
Having zynq compatible property here and using it give us option
that if another SoC vendor come with new configuration or clean
cadence one we can simple handle it without changing compatible
property for us.

thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Mark Rutland]

On Thu, May 29, 2014 at 10:43:05AM +0100, Michal Simek wrote:
> Hi Mark,
> 
> >> +static struct of_device_id cdns_wdt_of_match[] = {
> >> +       { .compatible = "xlnx,zynq-wdt-r1p2", },
> >> +       { .compatible = "cdns,wdt-r1p2", },
> > 
> > If these can currently be handled identically, why not just have
> > "cdns,wdt-r1p2" in the driver and in your dts have:
> > 
> > compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";
> > 
> > If we need to distinguish the two for some reason later we can always
> > add the "xlnx,zynq-wdt-r1p2" string to the driver.
> 
> I would prefer to have 2 compatible strings just because
> of that we don't know what is different compare to origin cadence
> version. We have done the same for spi-cadence.c that's why
> it shouldn't be any problem to keep it as is.
> Having zynq compatible property here and using it give us option
> that if another SoC vendor come with new configuration or clean
> cadence one we can simple handle it without changing compatible
> property for us.

Sure, we can have two documented strings. But as I mention for the
moment the driver only needs to support the one string so long as a
given dts has both. Then we can later distinguish the zynq variant (or
any other) as necessary.

Cheers,
Mark.

Re: [PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Michal Simek]

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

On 05/29/2014 03:19 PM, Mark Rutland wrote:
> On Thu, May 29, 2014 at 10:43:05AM +0100, Michal Simek wrote:
>> Hi Mark,
>>
>>>> +static struct of_device_id cdns_wdt_of_match[] = {
>>>> +       { .compatible = "xlnx,zynq-wdt-r1p2", },
>>>> +       { .compatible = "cdns,wdt-r1p2", },
>>>
>>> If these can currently be handled identically, why not just have
>>> "cdns,wdt-r1p2" in the driver and in your dts have:
>>>
>>> compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";
>>>
>>> If we need to distinguish the two for some reason later we can always
>>> add the "xlnx,zynq-wdt-r1p2" string to the driver.
>>
>> I would prefer to have 2 compatible strings just because
>> of that we don't know what is different compare to origin cadence
>> version. We have done the same for spi-cadence.c that's why
>> it shouldn't be any problem to keep it as is.
>> Having zynq compatible property here and using it give us option
>> that if another SoC vendor come with new configuration or clean
>> cadence one we can simple handle it without changing compatible
>> property for us.
> 
> Sure, we can have two documented strings. But as I mention for the
> moment the driver only needs to support the one string so long as a
> given dts has both. Then we can later distinguish the zynq variant (or
> any other) as necessary.

ok then. Let's use just "xlnx,zynq-wdt-r1p2" compatible string here
and remove "cdns,wdt-r1p2"

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Mark Rutland]

On Mon, Jun 02, 2014 at 07:25:16AM +0100, Michal Simek wrote:
> On 05/29/2014 03:19 PM, Mark Rutland wrote:
> > On Thu, May 29, 2014 at 10:43:05AM +0100, Michal Simek wrote:
> >> Hi Mark,
> >>
> >>>> +static struct of_device_id cdns_wdt_of_match[] = {
> >>>> +       { .compatible = "xlnx,zynq-wdt-r1p2", },
> >>>> +       { .compatible = "cdns,wdt-r1p2", },
> >>>
> >>> If these can currently be handled identically, why not just have
> >>> "cdns,wdt-r1p2" in the driver and in your dts have:
> >>>
> >>> compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";
> >>>
> >>> If we need to distinguish the two for some reason later we can always
> >>> add the "xlnx,zynq-wdt-r1p2" string to the driver.
> >>
> >> I would prefer to have 2 compatible strings just because
> >> of that we don't know what is different compare to origin cadence
> >> version. We have done the same for spi-cadence.c that's why
> >> it shouldn't be any problem to keep it as is.
> >> Having zynq compatible property here and using it give us option
> >> that if another SoC vendor come with new configuration or clean
> >> cadence one we can simple handle it without changing compatible
> >> property for us.
> > 
> > Sure, we can have two documented strings. But as I mention for the
> > moment the driver only needs to support the one string so long as a
> > given dts has both. Then we can later distinguish the zynq variant (or
> > any other) as necessary.
> 
> ok then. Let's use just "xlnx,zynq-wdt-r1p2" compatible string here
> and remove "cdns,wdt-r1p2"

That if anything seems backwards -- the driver should jsut take the most
general string for now: "cdns,wdt-r1p2", while the dtb will have both:

compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";

The driver can later disintguish "xlnx,zynq-wdt-r1p2" specially if
necessary.

Cheers,
Mark.

Re: [PATCH v2 1/2] watchdog: Add Cadence WDT driver

[Michal Simek]

On 06/02/2014 03:17 PM, Mark Rutland wrote:
> On Mon, Jun 02, 2014 at 07:25:16AM +0100, Michal Simek wrote:
>> On 05/29/2014 03:19 PM, Mark Rutland wrote:
>>> On Thu, May 29, 2014 at 10:43:05AM +0100, Michal Simek wrote:
>>>> Hi Mark,
>>>>
>>>>>> +static struct of_device_id cdns_wdt_of_match[] = {
>>>>>> +       { .compatible = "xlnx,zynq-wdt-r1p2", },
>>>>>> +       { .compatible = "cdns,wdt-r1p2", },
>>>>>
>>>>> If these can currently be handled identically, why not just have
>>>>> "cdns,wdt-r1p2" in the driver and in your dts have:
>>>>>
>>>>> compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";
>>>>>
>>>>> If we need to distinguish the two for some reason later we can always
>>>>> add the "xlnx,zynq-wdt-r1p2" string to the driver.
>>>>
>>>> I would prefer to have 2 compatible strings just because
>>>> of that we don't know what is different compare to origin cadence
>>>> version. We have done the same for spi-cadence.c that's why
>>>> it shouldn't be any problem to keep it as is.
>>>> Having zynq compatible property here and using it give us option
>>>> that if another SoC vendor come with new configuration or clean
>>>> cadence one we can simple handle it without changing compatible
>>>> property for us.
>>>
>>> Sure, we can have two documented strings. But as I mention for the
>>> moment the driver only needs to support the one string so long as a
>>> given dts has both. Then we can later distinguish the zynq variant (or
>>> any other) as necessary.
>>
>> ok then. Let's use just "xlnx,zynq-wdt-r1p2" compatible string here
>> and remove "cdns,wdt-r1p2"
> 
> That if anything seems backwards -- the driver should jsut take the most
> general string for now: "cdns,wdt-r1p2", while the dtb will have both:
> 
> compatible = "xlnx,zynq-wdt-r1p2", "cdns,wdt-r1p2";
> 
> The driver can later disintguish "xlnx,zynq-wdt-r1p2" specially if
> necessary.

ah. Ok I see your point now. No problem to do it in this way.

Thanks,
Michal