[PATCH v3 0/3] watchdog: add support for QCOM WDT

[PATCH v3 0/3] watchdog: add support for QCOM WDT

[Josh Cartwright]

This patchset provides support for the Watchdog Timer (WDT) found in the Krait
Processor Sub-system (KPSS) of the MSM8960, APQ8064, and IPQ8064 chips.

This driver is implemented ontop of WATCHDOG_CORE, and therefore its primary
interface is through userspace.  The implemantion is currently very basic (i.e.
it doesn't support PRETIMEOUT, even though it could be implemented through the
WDT's BARK functionality).  It should also be fairly easy to extend this driver
in the future to support newer chipsets as well.

Patch 3 also extends the driver to also register a restart_notifier, making it
possible for the WDT to act as a restart mechanism if more favorable mechanisms
don't work.  This is important for some boards which don't support PS_HOLD,
like the IPQ8064-based AP148 board.

Changes since v2:
  - "input clock phandle" -> "input clock" in device tree documentation
  - Fixup error handling paths during probe()
  - Use a more sane timeout (128ms), and add msleep()
  - Add some additional sanitation for clock rates

Changes since v1:
  - Make use of clock API instead of using a 'clock-frequency' property
  - Setup default timeout of 30 seconds when one is not specified
  - Add remove() function to allow for module unloading
  - Don't acquire/release watchdog lock on restart
  - Don't bail completely if restart_handler registration fails

Josh Cartwright (3):
  watchdog: qcom: add support for KPSS WDT
  watchdog: qcom: document device tree bindings
  watchdog: qcom: register a restart notifier

 .../devicetree/bindings/watchdog/qcom-wdt.txt      |  22 ++
 drivers/watchdog/Kconfig                           |  13 ++
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/qcom-wdt.c                        | 227 +++++++++++++++++++++
 4 files changed, 263 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
 create mode 100644 drivers/watchdog/qcom-wdt.c

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 1/3] watchdog: qcom: add support for KPSS WDT

[Josh Cartwright]

Add a driver for the watchdog timer block found in the Krait Processor
Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 drivers/watchdog/Kconfig    |  13 +++
 drivers/watchdog/Makefile   |   1 +
 drivers/watchdog/qcom-wdt.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 drivers/watchdog/qcom-wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 79d2589..c389ed7 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -421,6 +421,19 @@ config SIRFSOC_WATCHDOG
 	  Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When
 	  the watchdog triggers the system will be reset.
 
+config QCOM_WDT
+	tristate "QCOM watchdog"
+	depends on HAS_IOMEM
+	depends on ARCH_QCOM
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include Watchdog timer support for the watchdog found
+	  on QCOM chipsets.  Currently supported targets are the MSM8960,
+	  APQ8064, and IPQ8064.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called qcom_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 985a66c..cede21e 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
 obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
 obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o
 obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o
+obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
 obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
 
 # AVR32 Architecture
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
new file mode 100644
index 0000000..0f56ca3
--- /dev/null
+++ b/drivers/watchdog/qcom-wdt.c
@@ -0,0 +1,189 @@
+/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define WDT_RST		0x0
+#define WDT_EN		0x8
+#define WDT_BITE_TIME	0x24
+
+struct qcom_wdt {
+	struct watchdog_device	wdd;
+	struct clk		*clk;
+	unsigned long		rate;
+	void __iomem		*base;
+};
+
+static inline
+struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
+{
+	return container_of(wdd, struct qcom_wdt, wdd);
+}
+
+static int qcom_wdt_start(struct watchdog_device *wdd)
+{
+	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+
+	writel(0, wdt->base + WDT_EN);
+	writel(1, wdt->base + WDT_RST);
+	writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME);
+	writel(1, wdt->base + WDT_EN);
+	return 0;
+}
+
+static int qcom_wdt_stop(struct watchdog_device *wdd)
+{
+	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+
+	writel(0, wdt->base + WDT_EN);
+	return 0;
+}
+
+static int qcom_wdt_ping(struct watchdog_device *wdd)
+{
+	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+
+	writel(1, wdt->base + WDT_RST);
+	return 0;
+}
+
+static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
+				unsigned int timeout)
+{
+	wdd->timeout = timeout;
+	return qcom_wdt_start(wdd);
+}
+
+static const struct watchdog_ops qcom_wdt_ops = {
+	.start		= qcom_wdt_start,
+	.stop		= qcom_wdt_stop,
+	.ping		= qcom_wdt_ping,
+	.set_timeout	= qcom_wdt_set_timeout,
+	.owner		= THIS_MODULE,
+};
+
+static const struct watchdog_info qcom_wdt_info = {
+	.options	= WDIOF_KEEPALIVEPING
+			| WDIOF_MAGICCLOSE
+			| WDIOF_SETTIMEOUT,
+	.identity	= KBUILD_MODNAME,
+};
+
+static int qcom_wdt_probe(struct platform_device *pdev)
+{
+	struct qcom_wdt *wdt;
+	struct resource *res;
+	int ret;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(wdt->base)) {
+		ret = PTR_ERR(wdt->base);
+		goto err_out;
+	}
+
+	wdt->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(wdt->clk)) {
+		ret = PTR_ERR(wdt->clk);
+		goto err_out;
+	}
+
+	ret = clk_prepare_enable(wdt->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to setup clock\n");
+		goto err_out;
+	}
+
+	/*
+	 * We use the clock rate to calculate the max timeout, so ensure it's
+	 * not zero to avoid a divide-by-zero exception.
+	 *
+	 * WATCHDOG_CORE assumes units of seconds, if the WDT is clocked such
+	 * that it would bite before a second elapses it's usefulness is
+	 * limited.  Bail if this is the case.
+	 */
+	wdt->rate = clk_get_rate(wdt->clk);
+	if (wdt->rate == 0 ||
+	    wdt->rate > 0x10000000U) {
+		dev_err(&pdev->dev, "invalid clock rate\n");
+		ret = -EINVAL;
+		goto err_clk_unprepare;
+	}
+
+	wdt->wdd.dev = &pdev->dev;
+	wdt->wdd.info = &qcom_wdt_info;
+	wdt->wdd.ops = &qcom_wdt_ops;
+	wdt->wdd.min_timeout = 1;
+	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
+
+	/*
+	 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
+	 * default, unless the max timeout is less than 30 seconds, then use
+	 * the max instead.
+	 */
+	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
+	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
+
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register watchdog\n");
+		goto err_clk_unprepare;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+	return 0;
+
+err_clk_unprepare:
+	clk_disable_unprepare(wdt->clk);
+err_out:
+	return ret;
+}
+
+static int qcom_wdt_remove(struct platform_device *pdev)
+{
+	struct qcom_wdt *wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&wdt->wdd);
+	clk_disable_unprepare(wdt->clk);
+	return 0;
+}
+
+static const struct of_device_id qcom_wdt_of_table[] = {
+	{ .compatible = "qcom,kpss-wdt-msm8960", },
+	{ .compatible = "qcom,kpss-wdt-apq8064", },
+	{ .compatible = "qcom,kpss-wdt-ipq8064", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
+
+static struct platform_driver qcom_watchdog_driver = {
+	.probe	= qcom_wdt_probe,
+	.remove	= qcom_wdt_remove,
+	.driver	= {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= qcom_wdt_of_table,
+	},
+};
+module_platform_driver(qcom_watchdog_driver);
+
+MODULE_DESCRIPTION("QCOM KPSS Watchdog Driver");
+MODULE_LICENSE("GPL v2");
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 2/3] watchdog: qcom: document device tree bindings

[Josh Cartwright]

The Qualcomm Krait Processor Sub-system (KPSS) contains one or more
instances of the WDT.  Provide documentation on how to describe these in
the device tree.

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 .../devicetree/bindings/watchdog/qcom-wdt.txt      | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
new file mode 100644
index 0000000..c75566e
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
@@ -0,0 +1,22 @@
+Qualcomm Krait Processor Sub-system (KPSS) Watchdog
+---------------------------------------------------
+
+Required properties :
+- compatible : shall contain only one of the following:
+
+			"qcom,kpss-wdt-msm8960"
+			"qcom,kpss-wdt-apq8064"
+			"qcom,kpss-wdt-ipq8064"
+
+- reg : shall contain base register location and length
+- clocks : shall contain the input clock
+- timeout-sec : shall contain the default watchdog timeout in seconds,
+                if unset, the default timeout is 30 seconds
+
+Example:
+	watchdog at 208a038 {
+		compatible = "qcom,kpss-wdt-ipq8064";
+		reg = <0x0208a038 0x40>;
+		clocks = <&sleep_clk>;
+		timeout-sec = <10>;
+	};
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 3/3] watchdog: qcom: register a restart notifier

[Josh Cartwright]

The WDT's BITE_TIME warm-reset behavior can be leveraged as a last
resort mechanism for triggering chip reset.  Usually, other restart
methods (such as PS_HOLD) are preferrable for issuing a more complete
reset of the chip.  As such, keep the priority of the watchdog notifier
low.

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 drivers/watchdog/qcom-wdt.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 0f56ca3..8ce339f 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -10,12 +10,14 @@
  * GNU General Public License for more details.
  *
  */
+#include <linux/delay.h>
 #include <linux/clk.h>
 #include <linux/io.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 WDT_RST		0x0
@@ -26,6 +28,7 @@ struct qcom_wdt {
 	struct watchdog_device	wdd;
 	struct clk		*clk;
 	unsigned long		rate;
+	struct notifier_block	restart_nb;
 	void __iomem		*base;
 };
 
@@ -84,6 +87,32 @@ static const struct watchdog_info qcom_wdt_info = {
 	.identity	= KBUILD_MODNAME,
 };
 
+static int qcom_wdt_restart(struct notifier_block *nb, unsigned long action,
+			    void *data)
+{
+	struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb);
+	u32 timeout;
+
+	/*
+	 * Trigger watchdog bite:
+	 *    Setup BITE_TIME to be 128ms, and enable WDT.
+	 */
+	timeout = 128 * wdt->rate / 1000;
+
+	writel(0, wdt->base + WDT_EN);
+	writel(1, wdt->base + WDT_RST);
+	writel(timeout, wdt->base + WDT_BITE_TIME);
+	writel(1, wdt->base + WDT_EN);
+
+	/*
+	 * Actually make sure the above sequence hits hardware before sleeping.
+	 */
+	wmb();
+
+	msleep(150);
+	return NOTIFY_DONE;
+}
+
 static int qcom_wdt_probe(struct platform_device *pdev)
 {
 	struct qcom_wdt *wdt;
@@ -149,6 +178,14 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 		goto err_clk_unprepare;
 	}
 
+	/*
+	 * WDT restart notifier has priority 0 (use as a last resort)
+	 */
+	wdt->restart_nb.notifier_call = qcom_wdt_restart;
+	ret = register_restart_handler(&wdt->restart_nb);
+	if (ret)
+		dev_err(&pdev->dev, "failed to setup restart handler\n");
+
 	platform_set_drvdata(pdev, wdt);
 	return 0;
 
@@ -162,6 +199,7 @@ static int qcom_wdt_remove(struct platform_device *pdev)
 {
 	struct qcom_wdt *wdt = platform_get_drvdata(pdev);
 
+	unregister_restart_handler(&wdt->restart_nb);
 	watchdog_unregister_device(&wdt->wdd);
 	clk_disable_unprepare(wdt->clk);
 	return 0;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 1/3] watchdog: qcom: add support for KPSS WDT

[Guenter Roeck]

On Thu, Sep 25, 2014 at 12:48:51PM -0500, Josh Cartwright wrote:
> Add a driver for the watchdog timer block found in the Krait Processor
> Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>

Hi Josh,

just a couple of minor comments this time (yes, I know,
I am being difficult ;-).

Thanks,
Guenter

> ---
>  drivers/watchdog/Kconfig    |  13 +++
>  drivers/watchdog/Makefile   |   1 +
>  drivers/watchdog/qcom-wdt.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 203 insertions(+)
>  create mode 100644 drivers/watchdog/qcom-wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 79d2589..c389ed7 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -421,6 +421,19 @@ config SIRFSOC_WATCHDOG
>  	  Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When
>  	  the watchdog triggers the system will be reset.
>  
> +config QCOM_WDT
> +	tristate "QCOM watchdog"
> +	depends on HAS_IOMEM
> +	depends on ARCH_QCOM
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here to include Watchdog timer support for the watchdog found
> +	  on QCOM chipsets.  Currently supported targets are the MSM8960,
> +	  APQ8064, and IPQ8064.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called qcom_wdt.
> +
>  # AVR32 Architecture
>  
>  config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 985a66c..cede21e 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
>  obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
>  obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o
>  obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o
> +obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
>  obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>  
>  # AVR32 Architecture
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> new file mode 100644
> index 0000000..0f56ca3
> --- /dev/null
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -0,0 +1,189 @@
> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define WDT_RST		0x0
> +#define WDT_EN		0x8
> +#define WDT_BITE_TIME	0x24
> +
> +struct qcom_wdt {
> +	struct watchdog_device	wdd;
> +	struct clk		*clk;
> +	unsigned long		rate;
> +	void __iomem		*base;
> +};
> +
> +static inline
> +struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
> +{
> +	return container_of(wdd, struct qcom_wdt, wdd);
> +}
> +
> +static int qcom_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +
> +	writel(0, wdt->base + WDT_EN);
> +	writel(1, wdt->base + WDT_RST);
> +	writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME);
> +	writel(1, wdt->base + WDT_EN);
> +	return 0;
> +}
> +
> +static int qcom_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +
> +	writel(0, wdt->base + WDT_EN);
> +	return 0;
> +}
> +
> +static int qcom_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +
> +	writel(1, wdt->base + WDT_RST);
> +	return 0;
> +}
> +
> +static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
> +				unsigned int timeout)
> +{
> +	wdd->timeout = timeout;
> +	return qcom_wdt_start(wdd);
> +}
> +
> +static const struct watchdog_ops qcom_wdt_ops = {
> +	.start		= qcom_wdt_start,
> +	.stop		= qcom_wdt_stop,
> +	.ping		= qcom_wdt_ping,
> +	.set_timeout	= qcom_wdt_set_timeout,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct watchdog_info qcom_wdt_info = {
> +	.options	= WDIOF_KEEPALIVEPING
> +			| WDIOF_MAGICCLOSE
> +			| WDIOF_SETTIMEOUT,
> +	.identity	= KBUILD_MODNAME,
> +};
> +
> +static int qcom_wdt_probe(struct platform_device *pdev)
> +{
> +	struct qcom_wdt *wdt;
> +	struct resource *res;
> +	int ret;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	wdt->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(wdt->base)) {
> +		ret = PTR_ERR(wdt->base);
> +		goto err_out;

This is really unnecessary. Just return PTR_ERR((wdt->base) as you did before.
No need to make the code more complicated than necessary.

Basic rule is: If you can return immediately, do it. Otherwise use goto
and have a single error handler.

> +	}
> +
> +	wdt->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(wdt->clk)) {
> +		ret = PTR_ERR(wdt->clk);
> +		goto err_out;

Same here.

> +	}
> +
> +	ret = clk_prepare_enable(wdt->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to setup clock\n");
> +		goto err_out;

and here.

> +	}
> +
> +	/*
> +	 * We use the clock rate to calculate the max timeout, so ensure it's
> +	 * not zero to avoid a divide-by-zero exception.
> +	 *
> +	 * WATCHDOG_CORE assumes units of seconds, if the WDT is clocked such
> +	 * that it would bite before a second elapses it's usefulness is
> +	 * limited.  Bail if this is the case.
> +	 */
> +	wdt->rate = clk_get_rate(wdt->clk);
> +	if (wdt->rate == 0 ||
> +	    wdt->rate > 0x10000000U) {
> +		dev_err(&pdev->dev, "invalid clock rate\n");
> +		ret = -EINVAL;
> +		goto err_clk_unprepare;
> +	}
> +
> +	wdt->wdd.dev = &pdev->dev;
> +	wdt->wdd.info = &qcom_wdt_info;
> +	wdt->wdd.ops = &qcom_wdt_ops;
> +	wdt->wdd.min_timeout = 1;
> +	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
> +
> +	/*
> +	 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
> +	 * default, unless the max timeout is less than 30 seconds, then use
> +	 * the max instead.
> +	 */
> +	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);

Good idea.

> +	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> +	ret = watchdog_register_device(&wdt->wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register watchdog\n");
> +		goto err_clk_unprepare;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +	return 0;
> +
> +err_clk_unprepare:
> +	clk_disable_unprepare(wdt->clk);
> +err_out:
> +	return ret;
> +}
> +
> +static int qcom_wdt_remove(struct platform_device *pdev)
> +{
> +	struct qcom_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +	clk_disable_unprepare(wdt->clk);
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_wdt_of_table[] = {
> +	{ .compatible = "qcom,kpss-wdt-msm8960", },
> +	{ .compatible = "qcom,kpss-wdt-apq8064", },
> +	{ .compatible = "qcom,kpss-wdt-ipq8064", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
> +
> +static struct platform_driver qcom_watchdog_driver = {
> +	.probe	= qcom_wdt_probe,
> +	.remove	= qcom_wdt_remove,
> +	.driver	= {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= qcom_wdt_of_table,
> +	},
> +};
> +module_platform_driver(qcom_watchdog_driver);
> +
> +MODULE_DESCRIPTION("QCOM KPSS Watchdog Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

[PATCH v3 3/3] watchdog: qcom: register a restart notifier

[Guenter Roeck]

On Thu, Sep 25, 2014 at 12:48:53PM -0500, Josh Cartwright wrote:
> The WDT's BITE_TIME warm-reset behavior can be leveraged as a last
> resort mechanism for triggering chip reset.  Usually, other restart
> methods (such as PS_HOLD) are preferrable for issuing a more complete
> reset of the chip.  As such, keep the priority of the watchdog notifier
> low.
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
>  drivers/watchdog/qcom-wdt.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 0f56ca3..8ce339f 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -10,12 +10,14 @@
>   * GNU General Public License for more details.
>   *
>   */
> +#include <linux/delay.h>

Nitpick: Please keep alphabetical order of include files.
That makes it easier to identify include files later on.

>  #include <linux/clk.h>
>  #include <linux/io.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 WDT_RST		0x0
> @@ -26,6 +28,7 @@ struct qcom_wdt {
>  	struct watchdog_device	wdd;
>  	struct clk		*clk;
>  	unsigned long		rate;
> +	struct notifier_block	restart_nb;
>  	void __iomem		*base;
>  };
>  
> @@ -84,6 +87,32 @@ static const struct watchdog_info qcom_wdt_info = {
>  	.identity	= KBUILD_MODNAME,
>  };
>  
> +static int qcom_wdt_restart(struct notifier_block *nb, unsigned long action,
> +			    void *data)
> +{
> +	struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb);
> +	u32 timeout;
> +
> +	/*
> +	 * Trigger watchdog bite:
> +	 *    Setup BITE_TIME to be 128ms, and enable WDT.
> +	 */
> +	timeout = 128 * wdt->rate / 1000;
> +
> +	writel(0, wdt->base + WDT_EN);
> +	writel(1, wdt->base + WDT_RST);
> +	writel(timeout, wdt->base + WDT_BITE_TIME);
> +	writel(1, wdt->base + WDT_EN);
> +
> +	/*
> +	 * Actually make sure the above sequence hits hardware before sleeping.
> +	 */
> +	wmb();
> +
> +	msleep(150);
> +	return NOTIFY_DONE;
> +}
> +
>  static int qcom_wdt_probe(struct platform_device *pdev)
>  {
>  	struct qcom_wdt *wdt;
> @@ -149,6 +178,14 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  		goto err_clk_unprepare;
>  	}
>  
> +	/*
> +	 * WDT restart notifier has priority 0 (use as a last resort)
> +	 */
> +	wdt->restart_nb.notifier_call = qcom_wdt_restart;
> +	ret = register_restart_handler(&wdt->restart_nb);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to setup restart handler\n");
> +
>  	platform_set_drvdata(pdev, wdt);
>  	return 0;
>  
> @@ -162,6 +199,7 @@ static int qcom_wdt_remove(struct platform_device *pdev)
>  {
>  	struct qcom_wdt *wdt = platform_get_drvdata(pdev);
>  
> +	unregister_restart_handler(&wdt->restart_nb);
>  	watchdog_unregister_device(&wdt->wdd);
>  	clk_disable_unprepare(wdt->clk);
>  	return 0;
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

[PATCH v3 2/3] watchdog: qcom: document device tree bindings

[Guenter Roeck]

On Thu, Sep 25, 2014 at 12:48:52PM -0500, Josh Cartwright wrote:
> The Qualcomm Krait Processor Sub-system (KPSS) contains one or more
> instances of the WDT.  Provide documentation on how to describe these in
> the device tree.
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
>  .../devicetree/bindings/watchdog/qcom-wdt.txt      | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> new file mode 100644
> index 0000000..c75566e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
> @@ -0,0 +1,22 @@
> +Qualcomm Krait Processor Sub-system (KPSS) Watchdog
> +---------------------------------------------------
> +
> +Required properties :
> +- compatible : shall contain only one of the following:
> +
> +			"qcom,kpss-wdt-msm8960"
> +			"qcom,kpss-wdt-apq8064"
> +			"qcom,kpss-wdt-ipq8064"
> +
> +- reg : shall contain base register location and length
> +- clocks : shall contain the input clock
> +- timeout-sec : shall contain the default watchdog timeout in seconds,
> +                if unset, the default timeout is 30 seconds

Hi Josh,

timeout-sec is optional, not mandatory.

Thanks,
Guenter

> +
> +Example:
> +	watchdog at 208a038 {
> +		compatible = "qcom,kpss-wdt-ipq8064";
> +		reg = <0x0208a038 0x40>;
> +		clocks = <&sleep_clk>;
> +		timeout-sec = <10>;
> +	};
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

[PATCH v3 1/3] watchdog: qcom: add support for KPSS WDT

[Josh Cartwright]

On Thu, Sep 25, 2014 at 11:38:57AM -0700, Guenter Roeck wrote:
> On Thu, Sep 25, 2014 at 12:48:51PM -0500, Josh Cartwright wrote:
> > Add a driver for the watchdog timer block found in the Krait Processor
> > Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
> > 
> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> 
> Hi Josh,
> 
> just a couple of minor comments this time (yes, I know,
> I am being difficult ;-).

Difficult, maybe, but at least someone's taking a look!  Thanks, again.

[..]
> > +++ b/drivers/watchdog/qcom-wdt.c
[..]
> > +static int qcom_wdt_probe(struct platform_device *pdev)
> > +{
> > +	struct qcom_wdt *wdt;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > +	if (!wdt)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	wdt->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(wdt->base)) {
> > +		ret = PTR_ERR(wdt->base);
> > +		goto err_out;
> 
> This is really unnecessary. Just return PTR_ERR((wdt->base) as you did before.
> No need to make the code more complicated than necessary.
> 
> Basic rule is: If you can return immediately, do it. Otherwise use goto
> and have a single error handler.
> 
> > +	}
> > +
> > +	wdt->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(wdt->clk)) {
> > +		ret = PTR_ERR(wdt->clk);
> > +		goto err_out;
> 
> Same here.
> 
> > +	}
> > +
> > +	ret = clk_prepare_enable(wdt->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to setup clock\n");
> > +		goto err_out;
> 
> and here.

Okay, I can fix these up.

  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 2/3] watchdog: qcom: document device tree bindings

[Josh Cartwright]

On Thu, Sep 25, 2014 at 11:43:14AM -0700, Guenter Roeck wrote:
> On Thu, Sep 25, 2014 at 12:48:52PM -0500, Josh Cartwright wrote:
[..]
> > +- timeout-sec : shall contain the default watchdog timeout in seconds,
> > +                if unset, the default timeout is 30 seconds
> 
> Hi Josh,
> 
> timeout-sec is optional, not mandatory.

Indeed, I made this change in v2, but didn't reflect it in the document.
Good catch. Will fix.

  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3 3/3] watchdog: qcom: register a restart notifier

[Josh Cartwright]

On Thu, Sep 25, 2014 at 11:41:49AM -0700, Guenter Roeck wrote:
> On Thu, Sep 25, 2014 at 12:48:53PM -0500, Josh Cartwright wrote:
> > The WDT's BITE_TIME warm-reset behavior can be leveraged as a last
> > resort mechanism for triggering chip reset.  Usually, other restart
> > methods (such as PS_HOLD) are preferrable for issuing a more complete
> > reset of the chip.  As such, keep the priority of the watchdog notifier
> > low.
> > 
> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > ---
> >  drivers/watchdog/qcom-wdt.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > index 0f56ca3..8ce339f 100644
> > --- a/drivers/watchdog/qcom-wdt.c
> > +++ b/drivers/watchdog/qcom-wdt.c
> > @@ -10,12 +10,14 @@
> >   * GNU General Public License for more details.
> >   *
> >   */
> > +#include <linux/delay.h>
> 
> Nitpick: Please keep alphabetical order of include files.
> That makes it easier to identify include files later on.

That was my intent, but apparently I fail at the alphabet :).  Normally
instead of thinking I pipe the #include list through 'sort'.  Not sure
why I didn't do so this time around.

Thanks,
  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation