[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

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

[Joel Stanley]

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 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 at 1e785000 {
+		compatible = "aspeed,wdt";
+		reg = <0x1e785000 0x1c>;
+		interrupts = <27>;
+	};
-- 
2.8.1

[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@jms.id.au>
> ---
>  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 at 1e785000 {

IIRC, "watchdog at ..." 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Joel Stanley]

On Tue, May 10, 2016 at 6:08 AM, Rob Herring <robh@kernel.org> wrote:
> On Mon, May 09, 2016 at 10:04:36PM +0930, Joel Stanley wrote:
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>  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 at 1e785000 {
>
> IIRC, "watchdog at ..." is the correct generic name.

Cheers,

Joel

[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@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
+
+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

[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");
>

[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@roeck-us.net> 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");
>>
>

[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

[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

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

[Joel Stanley]

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
 1 file changed, 16 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..78ad314eaab9
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
@@ -0,0 +1,16 @@
+Aspeed Watchdog Timer
+
+Required properties:
+ - compatible: must be one of:
+	- "aspeed,ast2400-wdt"
+	- "aspeed,ast2500-wdt"
+
+ - reg: physical base address of the controller and length of memory mapped
+   region
+
+Example:
+
+	watchdog1: watchdog at 1e785000 {
+		compatible = "aspeed,ast2400-wdt";
+		reg = <0x1e785000 0x1c>;
+	};
-- 
2.8.1

[v2,1/2] doc/devicetree: Add Aspeed watchdog timer bindings

[Guenter Roeck]

On Thu, May 12, 2016 at 11:47:10PM +0930, Joel Stanley wrote:
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
>  1 file changed, 16 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..78ad314eaab9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> @@ -0,0 +1,16 @@
> +Aspeed Watchdog Timer
> +
> +Required properties:
> + - compatible: must be one of:
> +	- "aspeed,ast2400-wdt"
> +	- "aspeed,ast2500-wdt"
> +
> + - reg: physical base address of the controller and length of memory mapped
> +   region
> +
> +Example:
> +
> +	watchdog1: watchdog at 1e785000 {
> +		compatible = "aspeed,ast2400-wdt";
> +		reg = <0x1e785000 0x1c>;
> +	};

[PATCH v2 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@jms.id.au>
---
 drivers/watchdog/Kconfig      |  13 +++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/aspeed_wdt.c | 209 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 223 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 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..f7ae41ef4121
--- /dev/null
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -0,0 +1,209 @@
+/*
+ * 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,ast2400-wdt" },
+	{ .compatible = "aspeed,ast2500-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
+
+#define WDT_MIN_TIMEOUT		1
+/* 32 bits@1MHz, in milliseconds */
+#define WDT_MAX_TIMEOUT_MS	4294967
+#define WDT_DEFAULT_TIMEOUT	30
+#define WDT_1MHZ_CLOCK		1000000
+
+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);
+
+	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);
+
+	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
+
+	return 0;
+}
+
+static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int 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 = WDT_1MHZ_CLOCK;
+	wdt->wdd.info = &aspeed_wdt_info;
+	wdt->wdd.ops = &aspeed_wdt_ops;
+	wdt->wdd.min_timeout = WDT_MIN_TIMEOUT;
+	wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS;
+	wdt->wdd.parent = &pdev->dev;
+
+	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
+	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
+
+	/*
+	 * Control reset on a per-device basis to ensure the
+	 * host is not affected by a BMC reboot, so only reset
+	 * the SOC and not the full chip
+	 */
+	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
+		WDT_CTRL_1MHZ_CLK |
+		WDT_CTRL_RESET_SYSTEM;
+
+	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
+		aspeed_wdt_start(&wdt->wdd);
+		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+	}
+
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register\n");
+		return ret;
+	}
+
+	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);
+
+	return 0;
+}
+
+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

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

[Guenter Roeck]

On 05/12/2016 07:17 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 | 209 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 223 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 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..f7ae41ef4121
> --- /dev/null
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -0,0 +1,209 @@
> +/*
> + * 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;

I still don't see why you need this variable. Why not just use
WDT_1MHZ_CLOCK directly ?

> +	void __iomem		*base;
> +	u32			ctrl;
> +};
> +
> +static const struct of_device_id aspeed_wdt_of_table[] = {
> +	{ .compatible = "aspeed,ast2400-wdt" },
> +	{ .compatible = "aspeed,ast2500-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
> +
> +#define WDT_MIN_TIMEOUT		1
> +/* 32 bits at 1MHz, in milliseconds */
> +#define WDT_MAX_TIMEOUT_MS	4294967
> +#define WDT_DEFAULT_TIMEOUT	30
> +#define WDT_1MHZ_CLOCK		1000000
> +
> +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);
> +
> +	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);
> +
> +	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> +
> +	return 0;
> +}
> +
> +static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int timeout)
> +{
> +	wdd->timeout = timeout;
> +
> +	return aspeed_wdt_start(wdd);

The watchdog can be stopped here (with WDIOC_SETOPTIONS/WDIOS_DISABLECARD).
An implicit enable is not really a good idea; neither the infrastructure
nor user space would in this case be aware that the watchdog is running.

> +}
> +
> +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);
> +

Is that the smallest accepted value, or is there some other reason
not to make it smaller ?

It is also quite common to wait here to let the reset 'catch'
before returning. This would ensure that the system doesn't trigger
a lower priority reset.

> +	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 = WDT_1MHZ_CLOCK;
> +	wdt->wdd.info = &aspeed_wdt_info;
> +	wdt->wdd.ops = &aspeed_wdt_ops;
> +	wdt->wdd.min_timeout = WDT_MIN_TIMEOUT;
> +	wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS;
> +	wdt->wdd.parent = &pdev->dev;
> +
> +	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
> +	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> +	/*
> +	 * Control reset on a per-device basis to ensure the
> +	 * host is not affected by a BMC reboot, so only reset
> +	 * the SOC and not the full chip
> +	 */
> +	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
> +		WDT_CTRL_1MHZ_CLK |
> +		WDT_CTRL_RESET_SYSTEM;
> +
> +	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
> +		aspeed_wdt_start(&wdt->wdd);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +	}
> +
> +	ret = watchdog_register_device(&wdt->wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register\n");
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "rate %lu, max timeout %u, timeout %d\n",
> +		 wdt->rate, wdt->wdd.max_timeout, wdt->wdd.timeout);
> +

'rate 1000000' doesn't seem very informative. All it does is to expose
an implementation detail which no one really cares about. Even if you
were to add 'Hz', I would argue that no one will even understand
what it means without looking into the driver, but then it will be
understood without the message.

> +	platform_set_drvdata(pdev, wdt);
> +
> +	return 0;
> +}
> +
> +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");
>

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

[Joel Stanley]

On Sun, May 15, 2016 at 7:04 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> +
>> +struct aspeed_wdt {
>> +       struct watchdog_device  wdd;
>> +       unsigned long           rate;
>
> I still don't see why you need this variable. Why not just use
> WDT_1MHZ_CLOCK directly ?

Yes, I can. It's left over from when the driver ran the device from pclk.

>> +static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>> +                                 unsigned int timeout)
>> +{
>> +       wdd->timeout = timeout;
>> +
>> +       return aspeed_wdt_start(wdd);
>
> The watchdog can be stopped here (with WDIOC_SETOPTIONS/WDIOS_DISABLECARD).
> An implicit enable is not really a good idea; neither the infrastructure
> nor user space would in this case be aware that the watchdog is running.

Okay, good to know. The documentation does not mention this.

I will change it to write the value but not change the running state.

>> +}
>> +
>> +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);
>> +
>
>
> Is that the smallest accepted value, or is there some other reason
> not to make it smaller ?

It's what the vendor BSP was using, and it's what we've been using
since I wrote this code (which happens to be one year ago today!).

>
> It is also quite common to wait here to let the reset 'catch'
> before returning. This would ensure that the system doesn't trigger
> a lower priority reset.

Okay. I will add a mdelay(1000) here.

>> +       ret = watchdog_register_device(&wdt->wdd);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed to register\n");
>> +               return ret;
>> +       }
>> +
>> +       dev_info(&pdev->dev, "rate %lu, max timeout %u, timeout %d\n",
>> +                wdt->rate, wdt->wdd.max_timeout, wdt->wdd.timeout);
>> +
>
> 'rate 1000000' doesn't seem very informative. All it does is to expose
> an implementation detail which no one really cares about. Even if you
> were to add 'Hz', I would argue that no one will even understand
> what it means without looking into the driver, but then it will be
> understood without the message.

Okay. I will remove the dev_info all together.

Cheers,

Joel

[PATCH v3 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 v3:
 - remove rate from struct aspeed_wdt in favour of #define
 - don't start the timer when setting the timeout
 - wait after initiaitng the reboot so it catches
 - remove dev_info from startup

 - update bindings following review from Rob (this didn't make it to the
 mailing list in v2)

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

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                      | 212 +++++++++++++++++++++
 4 files changed, 242 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
 create mode 100644 drivers/watchdog/aspeed_wdt.c

-- 
2.8.1

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

[Joel Stanley]

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
 1 file changed, 16 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..c5e74d7b4406
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
@@ -0,0 +1,16 @@
+Aspeed Watchdog Timer
+
+Required properties:
+ - compatible: must be one of:
+	- "aspeed,ast2400-wdt"
+	- "aspeed,ast2500-wdt"
+
+ - reg: physical base address of the controller and length of memory mapped
+   region
+
+Example:
+
+	wdt1: watchdog at 1e785000 {
+		compatible = "aspeed,ast2400-wdt";
+		reg = <0x1e785000 0x1c>;
+	};
-- 
2.8.1

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

[Guenter Roeck]

On 05/18/2016 01:20 AM, Joel Stanley wrote:
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
>   1 file changed, 16 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..c5e74d7b4406
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> @@ -0,0 +1,16 @@
> +Aspeed Watchdog Timer
> +
> +Required properties:
> + - compatible: must be one of:
> +	- "aspeed,ast2400-wdt"
> +	- "aspeed,ast2500-wdt"
> +
> + - reg: physical base address of the controller and length of memory mapped
> +   region
> +
> +Example:
> +
> +	wdt1: watchdog at 1e785000 {
> +		compatible = "aspeed,ast2400-wdt";
> +		reg = <0x1e785000 0x1c>;
> +	};
>

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

[Joel Stanley]

Provides generic watchdog features as well as reboot support for the
Aspeed SoCs.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/watchdog/Kconfig      |  13 +++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/aspeed_wdt.c | 212 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 226 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..f5ad8023c2e6
--- /dev/null
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -0,0 +1,212 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * 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/delay.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>
+
+struct aspeed_wdt {
+	struct watchdog_device	wdd;
+	void __iomem		*base;
+	u32			ctrl;
+};
+
+static const struct of_device_id aspeed_wdt_of_table[] = {
+	{ .compatible = "aspeed,ast2400-wdt" },
+	{ .compatible = "aspeed,ast2500-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
+
+/* 32 bits@1MHz, in milliseconds */
+#define WDT_MAX_TIMEOUT_MS	4294967
+#define WDT_DEFAULT_TIMEOUT	30
+#define WDT_RATE_1MHZ		1000000
+
+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);
+
+	aspeed_wdt_enable(wdt, wdd->timeout * WDT_RATE_1MHZ);
+
+	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);
+
+	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
+
+	return 0;
+}
+
+static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int timeout)
+{
+	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
+	u32 actual;
+
+	wdd->timeout = timeout;
+
+	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
+
+	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
+	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
+
+	return 0;
+}
+
+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_1MHZ / 1000);
+
+	mdelay(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->wdd.info = &aspeed_wdt_info;
+	wdt->wdd.ops = &aspeed_wdt_ops;
+	wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS;
+	wdt->wdd.parent = &pdev->dev;
+
+	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
+	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
+
+	/*
+	 * Control reset on a per-device basis to ensure the
+	 * host is not affected by a BMC reboot, so only reset
+	 * the SOC and not the full chip
+	 */
+	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
+		WDT_CTRL_1MHZ_CLK |
+		WDT_CTRL_RESET_SYSTEM;
+
+	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
+		aspeed_wdt_start(&wdt->wdd);
+		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+	}
+
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+
+	return 0;
+}
+
+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

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

[Guenter Roeck]

On 05/18/2016 01:21 AM, Joel Stanley wrote:
> Provides generic watchdog features as well as reboot support for the
> Aspeed SoCs.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/Kconfig      |  13 +++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/aspeed_wdt.c | 212 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 226 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..f5ad8023c2e6
> --- /dev/null
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -0,0 +1,212 @@
> +/*
> + * Copyright 2016 IBM Corporation
> + *
> + * Joel Stanley <joel@jms.id.au>
> + *
> + * 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/delay.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>
> +
> +struct aspeed_wdt {
> +	struct watchdog_device	wdd;
> +	void __iomem		*base;
> +	u32			ctrl;
> +};
> +
> +static const struct of_device_id aspeed_wdt_of_table[] = {
> +	{ .compatible = "aspeed,ast2400-wdt" },
> +	{ .compatible = "aspeed,ast2500-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
> +
> +/* 32 bits at 1MHz, in milliseconds */
> +#define WDT_MAX_TIMEOUT_MS	4294967
> +#define WDT_DEFAULT_TIMEOUT	30
> +#define WDT_RATE_1MHZ		1000000
> +
> +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);
> +
> +	aspeed_wdt_enable(wdt, wdd->timeout * WDT_RATE_1MHZ);
> +
> +	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);
> +
> +	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> +
> +	return 0;
> +}
> +
> +static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int timeout)
> +{
> +	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> +	u32 actual;
> +
> +	wdd->timeout = timeout;
> +
> +	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +
> +	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> +	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> +
> +	return 0;
> +}
> +
> +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_1MHZ / 1000);
> +
> +	mdelay(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->wdd.info = &aspeed_wdt_info;
> +	wdt->wdd.ops = &aspeed_wdt_ops;
> +	wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS;
> +	wdt->wdd.parent = &pdev->dev;
> +
> +	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
> +	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> +	/*
> +	 * Control reset on a per-device basis to ensure the
> +	 * host is not affected by a BMC reboot, so only reset
> +	 * the SOC and not the full chip
> +	 */
> +	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
> +		WDT_CTRL_1MHZ_CLK |
> +		WDT_CTRL_RESET_SYSTEM;
> +
> +	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
> +		aspeed_wdt_start(&wdt->wdd);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +	}
> +
> +	ret = watchdog_register_device(&wdt->wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	return 0;
> +}
> +
> +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");
>