[PATCH 0/2] watchdog: Add Aspeed watchdog driver

[PATCH 0/2] watchdog: Add Aspeed watchdog driver

[Joel Stanley]

These patches add support for the watchdog timer found in Aspeed ast2400 and
ast2500 BMC SoCs.

The u-boot that ships on the edk and with many systems leaves the watchdog
running when jumping to the kernel, making this driver required for operation.

It has been tested on the ast2400 in an OpenPower Palmetto, and the ast2500-edk
dev board.

Cheers,

Joel

Joel Stanley (2):
  doc/devicetree: Add Aspeed watchdog timer bindings
  watchdog: Add Aspeed watchdog driver

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

-- 
2.8.1

--
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 1/2] doc/devicetree: Add Aspeed watchdog timer bindings

[Joel Stanley]

Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
new file mode 100644
index 000000000000..58184b161e0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
@@ -0,0 +1,14 @@
+Aspeed Watchdog Timer
+
+Required properties:
+ - compatible: must be "aspeed,wdt"
+ - reg: physical base address of the controller and length of memory mapped
+   region
+
+Example:
+
+	wdt1: wdt@1e785000 {
+		compatible = "aspeed,wdt";
+		reg = <0x1e785000 0x1c>;
+		interrupts = <27>;
+	};
-- 
2.8.1

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

Re: [PATCH 1/2] doc/devicetree: Add Aspeed watchdog timer bindings

[Rob Herring]

On Mon, May 09, 2016 at 10:04:36PM +0930, Joel Stanley wrote:
> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> new file mode 100644
> index 000000000000..58184b161e0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> @@ -0,0 +1,14 @@
> +Aspeed Watchdog Timer
> +
> +Required properties:
> + - compatible: must be "aspeed,wdt"

This should have the specific SOCs listed.

Rob

> + - reg: physical base address of the controller and length of memory mapped
> +   region
> +
> +Example:
> +
> +	wdt1: wdt@1e785000 {

IIRC, "watchdog@..." is the correct generic name.

> +		compatible = "aspeed,wdt";
> +		reg = <0x1e785000 0x1c>;
> +		interrupts = <27>;
> +	};
> -- 
> 2.8.1
> 
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] doc/devicetree: Add Aspeed watchdog timer bindings

[Joel Stanley]

On Tue, May 10, 2016 at 6:08 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, May 09, 2016 at 10:04:36PM +0930, Joel Stanley wrote:
>> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> new file mode 100644
>> index 000000000000..58184b161e0d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> @@ -0,0 +1,14 @@
>> +Aspeed Watchdog Timer
>> +
>> +Required properties:
>> + - compatible: must be "aspeed,wdt"
>
> This should have the specific SOCs listed.

Thanks, I'll fix it.

In the cases where the drivers had no change in the IP between
versions of the soc I wondered if it was neater to have a generic
string. You're saying that's wrong and we prefer using multiple
strings for all the SoCs instead?

>> +     wdt1: wdt@1e785000 {
>
> IIRC, "watchdog@..." is the correct generic name.

Cheers,

Joel
--
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

[PATCH 2/2] watchdog: Add Aspeed watchdog driver

[Joel Stanley]

Provides generic watchdog features as well as reboot support for the
Apeed SoC.

Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
---
 drivers/watchdog/Kconfig      |  13 +++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/aspeed_wdt.c | 199 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)
 create mode 100644 drivers/watchdog/aspeed_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index fb947655badd..4f0e2ded4785 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -661,6 +661,19 @@ config ATLAS7_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called atlas7_wdt.
 
+config ASPEED_WATCHDOG
+	tristate "Aspeed 2400 watchdog support"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include support for the watchdog timer
+	  in Apseed BMC SoCs.
+
+	  This driver is required to reboot the SoC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called aspeed_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index feb6270fdbde..36855375e0ce 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
 obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
 obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
 obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
+obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
new file mode 100644
index 000000000000..46cc71963c62
--- /dev/null
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
+ *
+ * Based on the qcom-watchdog driver
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+struct aspeed_wdt {
+	struct watchdog_device	wdd;
+	unsigned long		rate;
+	void __iomem		*base;
+	u32			ctrl;
+};
+
+static const struct of_device_id aspeed_wdt_of_table[] = {
+	{ .compatible = "aspeed,wdt" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
+
+#define WDT_STATUS		0x00
+#define WDT_RELOAD_VALUE	0x04
+#define WDT_RESTART		0x08
+#define WDT_CTRL		0x0C
+#define   WDT_CTRL_RESET_MODE_SOC	(0x00 << 5)
+#define   WDT_CTRL_RESET_MODE_FULL_CHIP	(0x01 << 5)
+#define	  WDT_CTRL_1MHZ_CLK		BIT(4)
+#define	  WDT_CTRL_WDT_EXT		BIT(3)
+#define	  WDT_CTRL_WDT_INTR		BIT(2)
+#define   WDT_CTRL_RESET_SYSTEM		BIT(1)
+#define   WDT_CTRL_ENABLE		BIT(0)
+
+#define	WDT_RESTART_MAGIC	0x4755
+
+static struct aspeed_wdt *to_aspeed_wdt(struct watchdog_device *wdd)
+{
+	return container_of(wdd, struct aspeed_wdt, wdd);
+}
+
+
+static void aspeed_wdt_enable(struct aspeed_wdt *wdt, int count)
+{
+	wdt->ctrl |= WDT_CTRL_ENABLE;
+
+	writel(0, wdt->base + WDT_CTRL);
+	writel(count, wdt->base + WDT_RELOAD_VALUE);
+	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
+	writel(wdt->ctrl, wdt->base + WDT_CTRL);
+}
+
+static int aspeed_wdt_start(struct watchdog_device *wdd)
+{
+	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
+
+	dev_dbg(wdd->parent, "starting with timeout of %d (rate %lu)\n",
+			wdd->timeout, wdt->rate);
+	aspeed_wdt_enable(wdt, wdd->timeout * wdt->rate);
+
+	return 0;
+}
+
+static int aspeed_wdt_stop(struct watchdog_device *wdd)
+{
+	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
+
+	wdt->ctrl &= ~WDT_CTRL_ENABLE;
+	writel(wdt->ctrl, wdt->base + WDT_CTRL);
+
+	return 0;
+}
+
+static int aspeed_wdt_ping(struct watchdog_device *wdd)
+{
+	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
+
+	dev_dbg(wdd->parent, "ping\n");
+	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
+	return 0;
+}
+
+static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int timeout)
+{
+	dev_dbg(wdd->parent, "timeout set to %u\n", timeout);
+	wdd->timeout = timeout;
+
+	return aspeed_wdt_start(wdd);
+}
+
+static int aspeed_wdt_restart(struct watchdog_device *wdd,
+			      unsigned long action, void *data)
+{
+	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
+
+	aspeed_wdt_enable(wdt, 128 * wdt->rate / 1000);
+
+	return 0;
+}
+
+static const struct watchdog_ops aspeed_wdt_ops = {
+	.start		= aspeed_wdt_start,
+	.stop		= aspeed_wdt_stop,
+	.ping		= aspeed_wdt_ping,
+	.set_timeout	= aspeed_wdt_set_timeout,
+	.restart	= aspeed_wdt_restart,
+	.owner		= THIS_MODULE,
+};
+
+static const struct watchdog_info aspeed_wdt_info = {
+	.options	= WDIOF_KEEPALIVEPING
+			| WDIOF_MAGICCLOSE
+			| WDIOF_SETTIMEOUT,
+	.identity	= KBUILD_MODNAME,
+};
+
+static int aspeed_wdt_remove(struct platform_device *pdev)
+{
+	struct aspeed_wdt *wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&wdt->wdd);
+
+	return 0;
+}
+
+static int aspeed_wdt_probe(struct platform_device *pdev)
+{
+	struct aspeed_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))
+		return PTR_ERR(wdt->base);
+
+	/*
+	 * The ast2400 wdt can run at PCLK, or 1MHz. The ast2500 only
+	 * runs at 1MHz. We chose to always run at 1MHz, as there's no
+	 * good reason to have a faster watchdog counter.
+	 */
+	wdt->rate = 1000000;
+	wdt->wdd.info = &aspeed_wdt_info;
+	wdt->wdd.ops = &aspeed_wdt_ops;
+	wdt->wdd.min_timeout = 1;
+	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
+	wdt->wdd.parent = &pdev->dev;
+
+	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
+	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
+
+	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
+		WDT_CTRL_1MHZ_CLK |
+		WDT_CTRL_RESET_SYSTEM;
+
+	/* Ensure hardware is in consistent state */
+	aspeed_wdt_stop(&wdt->wdd);
+
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register\n");
+		goto err;
+	}
+
+	dev_info(&pdev->dev, "rate %lu, max timeout %u, timeout %d\n",
+		 wdt->rate, wdt->wdd.max_timeout, wdt->wdd.timeout);
+
+	platform_set_drvdata(pdev, wdt);
+err:
+	return ret;
+}
+
+static struct platform_driver aspeed_watchdog_driver = {
+	.probe = aspeed_wdt_probe,
+	.remove = aspeed_wdt_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = of_match_ptr(aspeed_wdt_of_table),
+	},
+};
+module_platform_driver(aspeed_watchdog_driver);
+
+MODULE_DESCRIPTION("Aspeed Watchdog Driver");
+MODULE_LICENSE("GPL");
-- 
2.8.1

--
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 2/2] watchdog: Add Aspeed watchdog driver

[Guenter Roeck]

Hi Joel,

On 05/09/2016 05:34 AM, Joel Stanley wrote:
> Provides generic watchdog features as well as reboot support for the
> Apeed SoC.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>   drivers/watchdog/Kconfig      |  13 +++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/aspeed_wdt.c | 199 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 213 insertions(+)
>   create mode 100644 drivers/watchdog/aspeed_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index fb947655badd..4f0e2ded4785 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -661,6 +661,19 @@ config ATLAS7_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called atlas7_wdt.
>
> +config ASPEED_WATCHDOG
> +	tristate "Aspeed 2400 watchdog support"
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here to include support for the watchdog timer
> +	  in Apseed BMC SoCs.
> +
> +	  This driver is required to reboot the SoC.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called aspeed_wdt.
> +
>   # AVR32 Architecture
>
>   config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index feb6270fdbde..36855375e0ce 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -73,6 +73,7 @@ obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
>   obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>   obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
>   obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
> +obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>
>   # AVR32 Architecture
>   obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> new file mode 100644
> index 000000000000..46cc71963c62
> --- /dev/null
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright 2016 IBM Corporation
> + *
> + * Joel Stanley <joel@jms.id.au>
> + *
> + * Based on the qcom-watchdog driver
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +struct aspeed_wdt {
> +	struct watchdog_device	wdd;
> +	unsigned long		rate;
> +	void __iomem		*base;
> +	u32			ctrl;
> +};
> +
> +static const struct of_device_id aspeed_wdt_of_table[] = {
> +	{ .compatible = "aspeed,wdt" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
> +
> +#define WDT_STATUS		0x00
> +#define WDT_RELOAD_VALUE	0x04
> +#define WDT_RESTART		0x08
> +#define WDT_CTRL		0x0C
> +#define   WDT_CTRL_RESET_MODE_SOC	(0x00 << 5)
> +#define   WDT_CTRL_RESET_MODE_FULL_CHIP	(0x01 << 5)
> +#define	  WDT_CTRL_1MHZ_CLK		BIT(4)
> +#define	  WDT_CTRL_WDT_EXT		BIT(3)
> +#define	  WDT_CTRL_WDT_INTR		BIT(2)
> +#define   WDT_CTRL_RESET_SYSTEM		BIT(1)
> +#define   WDT_CTRL_ENABLE		BIT(0)
> +
> +#define	WDT_RESTART_MAGIC	0x4755

Please don't use tabs after #define.

> +
> +static struct aspeed_wdt *to_aspeed_wdt(struct watchdog_device *wdd)
> +{
> +	return container_of(wdd, struct aspeed_wdt, wdd);
> +}
> +
> +
Please no double empty lines.

> +static void aspeed_wdt_enable(struct aspeed_wdt *wdt, int count)
> +{
> +	wdt->ctrl |= WDT_CTRL_ENABLE;
> +
> +	writel(0, wdt->base + WDT_CTRL);
> +	writel(count, wdt->base + WDT_RELOAD_VALUE);
> +	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> +	writel(wdt->ctrl, wdt->base + WDT_CTRL);

Is it really necessary to write 0 into the ctrl register when enabling
the watchdog ? Problem with it is two-fold: It changes the clock rate
for a brief period of time, and it disables the watchdog while updating
the timeout. Both is undesirable unless really needed.

> +}
> +
> +static int aspeed_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> +
> +	dev_dbg(wdd->parent, "starting with timeout of %d (rate %lu)\n",
> +			wdd->timeout, wdt->rate);

Are those dev_dbg() really useful ?

> +	aspeed_wdt_enable(wdt, wdd->timeout * wdt->rate);
> +
> +	return 0;
> +}
> +
> +static int aspeed_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> +
> +	wdt->ctrl &= ~WDT_CTRL_ENABLE;
> +	writel(wdt->ctrl, wdt->base + WDT_CTRL);
> +
> +	return 0;
> +}
> +
> +static int aspeed_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> +
> +	dev_dbg(wdd->parent, "ping\n");
> +	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> +	return 0;
> +}
> +
> +static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int timeout)
> +{
> +	dev_dbg(wdd->parent, "timeout set to %u\n", timeout);
> +	wdd->timeout = timeout;
> +
> +	return aspeed_wdt_start(wdd);
> +}
> +
> +static int aspeed_wdt_restart(struct watchdog_device *wdd,
> +			      unsigned long action, void *data)
> +{
> +	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> +
> +	aspeed_wdt_enable(wdt, 128 * wdt->rate / 1000);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_ops aspeed_wdt_ops = {
> +	.start		= aspeed_wdt_start,
> +	.stop		= aspeed_wdt_stop,
> +	.ping		= aspeed_wdt_ping,
> +	.set_timeout	= aspeed_wdt_set_timeout,
> +	.restart	= aspeed_wdt_restart,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct watchdog_info aspeed_wdt_info = {
> +	.options	= WDIOF_KEEPALIVEPING
> +			| WDIOF_MAGICCLOSE
> +			| WDIOF_SETTIMEOUT,
> +	.identity	= KBUILD_MODNAME,
> +};
> +
> +static int aspeed_wdt_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static int aspeed_wdt_probe(struct platform_device *pdev)
> +{
> +	struct aspeed_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))
> +		return PTR_ERR(wdt->base);
> +
> +	/*
> +	 * The ast2400 wdt can run at PCLK, or 1MHz. The ast2500 only
> +	 * runs at 1MHz. We chose to always run at 1MHz, as there's no
> +	 * good reason to have a faster watchdog counter.
> +	 */
> +	wdt->rate = 1000000;

Why not just use a define ?

> +	wdt->wdd.info = &aspeed_wdt_info;
> +	wdt->wdd.ops = &aspeed_wdt_ops;
> +	wdt->wdd.min_timeout = 1;
> +	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
> +	wdt->wdd.parent = &pdev->dev;
> +
> +	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);

Pretty much the same here. Since max_timeout is a constant,
it is well known that it is larger than 30 seconds.
Might as well use defines for max_timeout and timeout.

> +	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> +	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |

Only reset tha SoC ? Are you sure this is what you want ?

> +		WDT_CTRL_1MHZ_CLK |
> +		WDT_CTRL_RESET_SYSTEM;
> +
> +	/* Ensure hardware is in consistent state */
> +	aspeed_wdt_stop(&wdt->wdd);
> +
An alternative might be to check if the watchdog is running, and inform
the watchdog core about it so it can generate heartbeats as required
until user space opens the watchdog device. This would ensure that the
system is protected during boot.

ctrl is really static except for the enable flag. Not really sure
if having a variable for it has any real benefits - you might as well
just read the register and update the enable flag instead as needed when
starting or stopping the watchdog. Is there a reason for not doing that ?

> +	ret = watchdog_register_device(&wdt->wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register\n");
> +		goto err;

Unnecessary goto. Just return.

> +	}
> +
> +	dev_info(&pdev->dev, "rate %lu, max timeout %u, timeout %d\n",
> +		 wdt->rate, wdt->wdd.max_timeout, wdt->wdd.timeout);
> +
> +	platform_set_drvdata(pdev, wdt);
> +err:
> +	return ret;
> +}
> +
> +static struct platform_driver aspeed_watchdog_driver = {
> +	.probe = aspeed_wdt_probe,
> +	.remove = aspeed_wdt_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = of_match_ptr(aspeed_wdt_of_table),
> +	},
> +};
> +module_platform_driver(aspeed_watchdog_driver);
> +
> +MODULE_DESCRIPTION("Aspeed Watchdog Driver");
> +MODULE_LICENSE("GPL");
>

Re: [PATCH 2/2] watchdog: Add Aspeed watchdog driver

[Joel Stanley]

Hey Guenter,

Thanks for the review.

On Tue, May 10, 2016 at 12:18 AM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> +#define WDT_STATUS             0x00
>> +#define WDT_RELOAD_VALUE       0x04
>> +#define WDT_RESTART            0x08
>> +#define WDT_CTRL               0x0C
>> +#define   WDT_CTRL_RESET_MODE_SOC      (0x00 << 5)
>> +#define   WDT_CTRL_RESET_MODE_FULL_CHIP        (0x01 << 5)
>> +#define          WDT_CTRL_1MHZ_CLK             BIT(4)
>> +#define          WDT_CTRL_WDT_EXT              BIT(3)
>> +#define          WDT_CTRL_WDT_INTR             BIT(2)
>> +#define   WDT_CTRL_RESET_SYSTEM                BIT(1)
>> +#define   WDT_CTRL_ENABLE              BIT(0)
>> +
>> +#define        WDT_RESTART_MAGIC       0x4755
>
>
> Please don't use tabs after #define.

Oops, typo. Thanks.

>
>> +
>> +static struct aspeed_wdt *to_aspeed_wdt(struct watchdog_device *wdd)
>> +{
>> +       return container_of(wdd, struct aspeed_wdt, wdd);
>> +}
>> +
>> +
>
> Please no double empty lines.
>
>> +static void aspeed_wdt_enable(struct aspeed_wdt *wdt, int count)
>> +{
>> +       wdt->ctrl |= WDT_CTRL_ENABLE;
>> +
>> +       writel(0, wdt->base + WDT_CTRL);
>> +       writel(count, wdt->base + WDT_RELOAD_VALUE);
>> +       writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
>> +       writel(wdt->ctrl, wdt->base + WDT_CTRL);
>
>
> Is it really necessary to write 0 into the ctrl register when enabling
> the watchdog ? Problem with it is two-fold: It changes the clock rate
> for a brief period of time, and it disables the watchdog while updating
> the timeout. Both is undesirable unless really needed.

In general I agree. However, the datasheet explicitly spells out these
four steps in this order. Writing zero is a shortcut; I could write
ctrl with the enable bit set to zero.

We've been using the driver in this fashion for the past year.

>> +}
>> +
>> +static int aspeed_wdt_start(struct watchdog_device *wdd)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       dev_dbg(wdd->parent, "starting with timeout of %d (rate %lu)\n",
>> +                       wdd->timeout, wdt->rate);
>
>
> Are those dev_dbg() really useful ?

I did use them in writing the driver. They can go.

>
>> +       aspeed_wdt_enable(wdt, wdd->timeout * wdt->rate);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       wdt->ctrl &= ~WDT_CTRL_ENABLE;
>> +       writel(wdt->ctrl, wdt->base + WDT_CTRL);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_ping(struct watchdog_device *wdd)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       dev_dbg(wdd->parent, "ping\n");
>> +       writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>> +                                 unsigned int timeout)
>> +{
>> +       dev_dbg(wdd->parent, "timeout set to %u\n", timeout);
>> +       wdd->timeout = timeout;
>> +
>> +       return aspeed_wdt_start(wdd);
>> +}
>> +
>> +static int aspeed_wdt_restart(struct watchdog_device *wdd,
>> +                             unsigned long action, void *data)
>> +{
>> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> +       aspeed_wdt_enable(wdt, 128 * wdt->rate / 1000);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct watchdog_ops aspeed_wdt_ops = {
>> +       .start          = aspeed_wdt_start,
>> +       .stop           = aspeed_wdt_stop,
>> +       .ping           = aspeed_wdt_ping,
>> +       .set_timeout    = aspeed_wdt_set_timeout,
>> +       .restart        = aspeed_wdt_restart,
>> +       .owner          = THIS_MODULE,
>> +};
>> +
>> +static const struct watchdog_info aspeed_wdt_info = {
>> +       .options        = WDIOF_KEEPALIVEPING
>> +                       | WDIOF_MAGICCLOSE
>> +                       | WDIOF_SETTIMEOUT,
>> +       .identity       = KBUILD_MODNAME,
>> +};
>> +
>> +static int aspeed_wdt_remove(struct platform_device *pdev)
>> +{
>> +       struct aspeed_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> +       watchdog_unregister_device(&wdt->wdd);
>> +
>> +       return 0;
>> +}
>> +
>> +static int aspeed_wdt_probe(struct platform_device *pdev)
>> +{
>> +       struct aspeed_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))
>> +               return PTR_ERR(wdt->base);
>> +
>> +       /*
>> +        * The ast2400 wdt can run at PCLK, or 1MHz. The ast2500 only
>> +        * runs at 1MHz. We chose to always run at 1MHz, as there's no
>> +        * good reason to have a faster watchdog counter.
>> +        */
>> +       wdt->rate = 1000000;
>
>
> Why not just use a define ?

I will add one.

The comment is informative though for people who read the ast2400
datasheet and wonder why we don't provide the option clocking with
pclk, I will leave it in.

>> +       wdt->wdd.info = &aspeed_wdt_info;
>> +       wdt->wdd.ops = &aspeed_wdt_ops;
>> +       wdt->wdd.min_timeout = 1;
>> +       wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
>> +       wdt->wdd.parent = &pdev->dev;
>> +
>> +       wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>
>
> Pretty much the same here. Since max_timeout is a constant,
> it is well known that it is larger than 30 seconds.
> Might as well use defines for max_timeout and timeout.

Ok.

>
>> +       watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
>> +
>> +       wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
>
>
> Only reset tha SoC ? Are you sure this is what you want ?

I think this is left over from experiments when tracking down a reset
bug in u-boot. I will confirm and change it back to FULL_CHIP.

>
>> +               WDT_CTRL_1MHZ_CLK |
>> +               WDT_CTRL_RESET_SYSTEM;
>> +
>> +       /* Ensure hardware is in consistent state */
>> +       aspeed_wdt_stop(&wdt->wdd);
>> +
>
> An alternative might be to check if the watchdog is running, and inform
> the watchdog core about it so it can generate heartbeats as required
> until user space opens the watchdog device. This would ensure that the
> system is protected during boot.

I will implement this.

> ctrl is really static except for the enable flag. Not really sure
> if having a variable for it has any real benefits - you might as well
> just read the register and update the enable flag instead as needed when
> starting or stopping the watchdog. Is there a reason for not doing that ?

Not really. I find it cleaner to keep a copy of the register instead
of read-modify-write, but if you feel strongly about it I can change.

>
>> +       ret = watchdog_register_device(&wdt->wdd);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed to register\n");
>> +               goto err;
>
>
> Unnecessary goto. Just return.

Ok.

>
>
>> +       }
>> +
>> +       dev_info(&pdev->dev, "rate %lu, max timeout %u, timeout %d\n",
>> +                wdt->rate, wdt->wdd.max_timeout, wdt->wdd.timeout);
>> +
>> +       platform_set_drvdata(pdev, wdt);
>> +err:
>> +       return ret;
>> +}
>> +
>> +static struct platform_driver aspeed_watchdog_driver = {
>> +       .probe = aspeed_wdt_probe,
>> +       .remove = aspeed_wdt_remove,
>> +       .driver = {
>> +               .name = KBUILD_MODNAME,
>> +               .of_match_table = of_match_ptr(aspeed_wdt_of_table),
>> +       },
>> +};
>> +module_platform_driver(aspeed_watchdog_driver);
>> +
>> +MODULE_DESCRIPTION("Aspeed Watchdog Driver");
>> +MODULE_LICENSE("GPL");
>>
>
--
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 2/2] watchdog: Add Aspeed watchdog driver

[Guenter Roeck]

Hi Joel,

On 05/10/2016 04:10 AM, Joel Stanley wrote:

[ ... ]

>>> +
>>> +       /*
>>> +        * The ast2400 wdt can run at PCLK, or 1MHz. The ast2500 only
>>> +        * runs at 1MHz. We chose to always run at 1MHz, as there's no
>>> +        * good reason to have a faster watchdog counter.
>>> +        */
>>> +       wdt->rate = 1000000;
>>
>>
>> Why not just use a define ?
>
> I will add one.
>
> The comment is informative though for people who read the ast2400
> datasheet and wonder why we don't provide the option clocking with
> pclk, I will leave it in.
>

The comment is ok. I just find it unnecessary to have a variable
instead of a constant.

[ ... ]
>
>> ctrl is really static except for the enable flag. Not really sure
>> if having a variable for it has any real benefits - you might as well
>> just read the register and update the enable flag instead as needed when
>> starting or stopping the watchdog. Is there a reason for not doing that ?
>
> Not really. I find it cleaner to keep a copy of the register instead
> of read-modify-write, but if you feel strongly about it I can change.
>

No worries. Not worth arguing about.

Guenter

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

[PATCH v2 0/2] watchdog: Add Aspeed watchdog driver

[Joel Stanley]

Hello,

These patches add support for the watchdog timer found in Aspeed ast2400 and
ast2500 BMC SoCs.

The u-boot that ships on the edk and with many systems leaves the watchdog
running when jumping to the kernel, making this driver required for operation.

It has been tested on the ast2400 in an OpenPower Palmetto, and the ast2500-edk
dev board.

Changes in v2:

Addressed review from Guenter:
 - dropped the drv_dbgs
 - added defines for all of the constants
 - clarified the type of reset we want
 - leaves the watchdog running

This also updates the device tree compatible string following review from Rob.
As Arnd already took my device tree tree, I will send him a patch to fix that.

Cheers,

Joel

Joel Stanley (2):
  doc/devicetree: Add Aspeed watchdog timer bindings
  watchdog: Add Aspeed watchdog driver

 .../devicetree/bindings/watchdog/aspeed-wdt.txt    |  16 ++
 drivers/watchdog/Kconfig                           |  13 ++
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/aspeed_wdt.c                      | 209 +++++++++++++++++++++
 4 files changed, 239 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
 create mode 100644 drivers/watchdog/aspeed_wdt.c

-- 
2.8.1

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