Skip to contentSkip to conversation

watchdog: Add Aspeed watchdog driver

PATCH Superseded Joel Stanley 4 participants 20 messages May 9, 2016 – Jul 17, 2016

Subsystems (3)
Device tree bindings Watchdog device drivers Arm/Aspeed machine support
lore
Share & tools
lore

Joel Stanley

author maintainer
#0 lore raw

cover watchdog: Add Aspeed watchdog driver

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
Signature · 1 line
2.8.1
author maintainer
#1 lore raw

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

Signed-off-by Joel Stanley <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +14 -0
new file mode 100644
index 000000000000..58184b161e0d
@@ -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>;
+	};
Signature · 1 line
2.8.1
maintainer
#2 lore raw

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

Quoted 12 lines · Joel Stanley
Signed-off-by Joel Stanley <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +0 -0
new file mode 100644
index 000000000000..58184b161e0d

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt line 4

Quoted 4 lines
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +4 -0
@@ -0,0 +1,14 @@
+Aspeed Watchdog Timer
+
+Required properties:
+ - compatible: must be "aspeed,wdt"

This should have the specific SOCs listed.

Rob

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt line 10

Quoted 6 lines
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +6 -0
+ - 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.

Quoted 11 lines
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +4 -0
+		compatible = "aspeed,wdt";
+		reg = <0x1e785000 0x1c>;
+		interrupts = <27>;
+	};
Signature · 6 lines
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
author maintainer
#3 lore raw

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

Quoted 7 lines · Rob Herring
Quoted 6 lines · Joel Stanley
Signed-off-by Joel Stanley <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
Quoted 12 lines

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt line 4

Quoted 10 lines
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +4 -0
new file mode 100644
index 000000000000..58184b161e0d
@@ -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?

Quoted 3 lines

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt line 10

Quoted 1 line
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +1 -0
+     wdt1: wdt at 1e785000 {

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

Cheers,

Joel

author maintainer
#4 lore raw

PATCH 2/2 watchdog: Add Aspeed watchdog driver

In reply to Joel Stanley

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

Signed-off-by Joel Stanley <[email protected]>
---
 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
drivers/watchdog/Kconfig +13 -0
index fb947655badd..4f0e2ded4785 100644
@@ -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
drivers/watchdog/Makefile +1 -0
index feb6270fdbde..36855375e0ce 100644
@@ -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
drivers/watchdog/aspeed_wdt.c new file +199 -0

199 lines not on this page · raw message

Signature · 1 line
2.8.1
maintainer
#5 lore raw

PATCH 2/2 watchdog: Add Aspeed watchdog driver

Hi Joel,

Quoted 88 lines · Joel Stanley

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

Signed-off-by Joel Stanley <[email protected]>
---
  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

drivers/watchdog/Kconfig +13 -0
index fb947655badd..4f0e2ded4785 100644
@@ -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
drivers/watchdog/Makefile +1 -0
index feb6270fdbde..36855375e0ce 100644
@@ -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
drivers/watchdog/aspeed_wdt.c new file +35 -0
new file mode 100644
index 000000000000..46cc71963c62
@@ -0,0 +1,199 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * Joel Stanley <[email protected]>
+ *
+ * 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

drivers/watchdog/aspeed_wdt.c line 45

Quoted 10 lines
drivers/watchdog/aspeed_wdt.c new file +10 -0
+#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.

drivers/watchdog/aspeed_wdt.c line 52

Quoted 7 lines
drivers/watchdog/aspeed_wdt.c new file +7 -0
+
+static struct aspeed_wdt *to_aspeed_wdt(struct watchdog_device *wdd)
+{
+	return container_of(wdd, struct aspeed_wdt, wdd);
+}
+
+

Please no double empty lines.

drivers/watchdog/aspeed_wdt.c line 60

Quoted 8 lines
drivers/watchdog/aspeed_wdt.c new file +8 -0
+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.

drivers/watchdog/aspeed_wdt.c line 68

Quoted 8 lines
drivers/watchdog/aspeed_wdt.c new file +8 -0
+}
+
+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 ?

Quoted 79 lines
drivers/watchdog/aspeed_wdt.c new file +79 -0
+	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);

drivers/watchdog/aspeed_wdt.c line 157

Quoted 10 lines
drivers/watchdog/aspeed_wdt.c new file +10 -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 ?

drivers/watchdog/aspeed_wdt.c line 164

Quoted 7 lines
drivers/watchdog/aspeed_wdt.c new file +7 -0
+	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.

drivers/watchdog/aspeed_wdt.c line 167

Quoted 3 lines
drivers/watchdog/aspeed_wdt.c new file +3 -0
+	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 ?

drivers/watchdog/aspeed_wdt.c line 173

Quoted 6 lines
drivers/watchdog/aspeed_wdt.c new file +6 -0
+		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 ?

drivers/watchdog/aspeed_wdt.c line 177

Quoted 4 lines
drivers/watchdog/aspeed_wdt.c new file +4 -0
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register\n");
+		goto err;

Unnecessary goto. Just return.

Quoted 23 lines
drivers/watchdog/aspeed_wdt.c new file +22 -0
+	}
+
+	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");
author maintainer
#6 lore raw

PATCH 2/2 watchdog: Add Aspeed watchdog driver

Hey Guenter,

Thanks for the review.

Quoted 8 lines · Guenter Roeck
Quoted 8 lines
drivers/watchdog/aspeed_wdt.c new file +8 -0
+#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)
Quoted 8 lines

drivers/watchdog/aspeed_wdt.c line 45

Quoted 5 lines
drivers/watchdog/aspeed_wdt.c new file +5 -0
+#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.

Quoted 11 lines

drivers/watchdog/aspeed_wdt.c line 52

Quoted 8 lines
drivers/watchdog/aspeed_wdt.c new file +7 -0
+
+static struct aspeed_wdt *to_aspeed_wdt(struct watchdog_device *wdd)
+{
+       return container_of(wdd, struct aspeed_wdt, wdd);
+}
+
+

Please no double empty lines.

Quoted 14 lines

drivers/watchdog/aspeed_wdt.c line 60

Quoted 8 lines
drivers/watchdog/aspeed_wdt.c new file +8 -0
+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.

Quoted 11 lines

drivers/watchdog/aspeed_wdt.c line 68

Quoted 8 lines
drivers/watchdog/aspeed_wdt.c new file +8 -0
+}
+
+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.

Quoted 85 lines
Quoted 85 lines
drivers/watchdog/aspeed_wdt.c new file +84 -0
+       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);
+
+       /*
Quoted 8 lines

drivers/watchdog/aspeed_wdt.c line 157

Quoted 5 lines
drivers/watchdog/aspeed_wdt.c new file +5 -0
+        * 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.

Quoted 9 lines

drivers/watchdog/aspeed_wdt.c line 164

Quoted 7 lines
drivers/watchdog/aspeed_wdt.c new file +7 -0
+       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);
Quoted 3 lines

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.

Quoted 7 lines

drivers/watchdog/aspeed_wdt.c line 167

Quoted 4 lines
drivers/watchdog/aspeed_wdt.c new file +3 -0
+       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.

Quoted 12 lines

drivers/watchdog/aspeed_wdt.c line 173

Quoted 7 lines
drivers/watchdog/aspeed_wdt.c new file +6 -0
+               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.

Quoted 4 lines

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.

Quoted 8 lines

drivers/watchdog/aspeed_wdt.c line 177

Quoted 5 lines
drivers/watchdog/aspeed_wdt.c new file +4 -0
+       ret = watchdog_register_device(&wdt->wdd);
+       if (ret) {
+               dev_err(&pdev->dev, "failed to register\n");
+               goto err;

Unnecessary goto. Just return.

Ok.

Quoted 26 lines
Quoted 25 lines
drivers/watchdog/aspeed_wdt.c new file +22 -0
+       }
+
+       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");
maintainer
#7 lore raw

PATCH 2/2 watchdog: Add Aspeed watchdog driver

Hi Joel,

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

[ ... ]

Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
drivers/watchdog/aspeed_wdt.c new file +7 -0
+
+       /*
+        * 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;
Quoted 8 lines
Quoted 1 line

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.

[ ... ]

Quoted 9 lines
Quoted 5 lines

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

author maintainer
#8 lore raw

v2 cover watchdog: Add Aspeed watchdog driver

In reply to 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
Signature · 1 line
2.8.1
author maintainer
#9 lore raw

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

Signed-off-by Joel Stanley <[email protected]>
---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +16 -0
new file mode 100644
index 000000000000..78ad314eaab9
@@ -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>;
+	};
Signature · 1 line
2.8.1
maintainer
#10 lore raw

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

Quoted 1 line · Joel Stanley
Signed-off-by Joel Stanley <[email protected]>
Reviewed-by Guenter Roeck <[email protected]>
Quoted 27 lines
---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +16 -0
new file mode 100644
index 000000000000..78ad314eaab9
@@ -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>;
+	};
author maintainer
#11 lore raw

PATCH v2 2/2 watchdog: Add Aspeed watchdog driver

In reply to Joel Stanley

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

Signed-off-by Joel Stanley <[email protected]>
---
 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
drivers/watchdog/Kconfig +13 -0
index fb947655badd..4f0e2ded4785 100644
@@ -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
drivers/watchdog/Makefile +1 -0
index feb6270fdbde..36855375e0ce 100644
@@ -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
drivers/watchdog/aspeed_wdt.c new file +209 -0

209 lines not on this page · raw message

Signature · 1 line
2.8.1
maintainer
#12 lore raw

PATCH v2 2/2 watchdog: Add Aspeed watchdog driver

Quoted 65 lines · Joel Stanley

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

Signed-off-by Joel Stanley <[email protected]>
---
  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

drivers/watchdog/Kconfig +13 -0
index fb947655badd..4f0e2ded4785 100644
@@ -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
drivers/watchdog/Makefile +1 -0
index feb6270fdbde..36855375e0ce 100644
@@ -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
drivers/watchdog/aspeed_wdt.c new file +12 -0
new file mode 100644
index 000000000000..f7ae41ef4121
@@ -0,0 +1,209 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * Joel Stanley <[email protected]>
+ *
+ * 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.
+ */

drivers/watchdog/aspeed_wdt.c line 22

Quoted 10 lines
drivers/watchdog/aspeed_wdt.c new file +10 -0
+#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 ?

Quoted 70 lines
drivers/watchdog/aspeed_wdt.c new file +70 -0
+	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);

drivers/watchdog/aspeed_wdt.c line 102

Quoted 10 lines
drivers/watchdog/aspeed_wdt.c new file +10 -0
+
+	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.

drivers/watchdog/aspeed_wdt.c line 111

Quoted 9 lines
drivers/watchdog/aspeed_wdt.c new file +9 -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 / 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.

Quoted 71 lines
drivers/watchdog/aspeed_wdt.c new file +71 -0
+	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);
+	}

drivers/watchdog/aspeed_wdt.c line 192

Quoted 10 lines
drivers/watchdog/aspeed_wdt.c new file +10 -0
+
+	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.

Quoted 18 lines
drivers/watchdog/aspeed_wdt.c new file +17 -0
+	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");
author maintainer
#13 lore raw

PATCH v2 2/2 watchdog: Add Aspeed watchdog driver

Quoted 7 lines · Guenter Roeck

drivers/watchdog/aspeed_wdt.c line 22

Quoted 4 lines
drivers/watchdog/aspeed_wdt.c new file +4 -0
+
+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.

Quoted 10 lines

drivers/watchdog/aspeed_wdt.c line 102

Quoted 6 lines
drivers/watchdog/aspeed_wdt.c new file +6 -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.

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

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

Quoted 5 lines
Quoted 5 lines
drivers/watchdog/aspeed_wdt.c new file +5 -0
+}
+
+static int aspeed_wdt_restart(struct watchdog_device *wdd,
+                             unsigned long action, void *data)
+{
Quoted 8 lines

drivers/watchdog/aspeed_wdt.c line 111

Quoted 4 lines
drivers/watchdog/aspeed_wdt.c new file +4 -0
+       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!).

Quoted 4 lines

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.

Quoted 7 lines
Quoted 7 lines
drivers/watchdog/aspeed_wdt.c new file +7 -0
+       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",
Quoted 8 lines

drivers/watchdog/aspeed_wdt.c line 192

Quoted 2 lines
drivers/watchdog/aspeed_wdt.c new file +2 -0
+                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

author maintainer
#14 lore raw

v3 cover watchdog: Add Aspeed watchdog driver

In reply to 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
Signature · 1 line
2.8.1
author maintainer
#15 lore raw

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

Signed-off-by Joel Stanley <[email protected]>
---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +16 -0
new file mode 100644
index 000000000000..c5e74d7b4406
@@ -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>;
+	};
Signature · 1 line
2.8.1
maintainer
#16 lore raw

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

Quoted 1 line · Joel Stanley
Signed-off-by Joel Stanley <[email protected]>
Reviewed-by Guenter Roeck <[email protected]>
Quoted 28 lines
---
  .../devicetree/bindings/watchdog/aspeed-wdt.txt          | 16 ++++++++++++++++
  1 file changed, 16 insertions(+)

create mode 100644 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt new file +16 -0
new file mode 100644
index 000000000000..c5e74d7b4406
@@ -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>;
+	};
author maintainer
#17 lore raw

PATCH v3 2/2 watchdog: Add Aspeed watchdog driver

In reply to Joel Stanley

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

Signed-off-by Joel Stanley <[email protected]>
---
 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
drivers/watchdog/Kconfig +13 -0
index fb947655badd..4f0e2ded4785 100644
@@ -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
drivers/watchdog/Makefile +1 -0
index feb6270fdbde..36855375e0ce 100644
@@ -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
drivers/watchdog/aspeed_wdt.c new file +212 -0

212 lines not on this page · raw message

Signature · 1 line
2.8.1
maintainer
#18 lore raw

PATCH v3 2/2 watchdog: Add Aspeed watchdog driver

Quoted 4 lines · Joel Stanley

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

Signed-off-by Joel Stanley <[email protected]>
Reviewed-by Guenter Roeck <[email protected]>
Quoted 262 lines
---
  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

drivers/watchdog/Kconfig +13 -0
index fb947655badd..4f0e2ded4785 100644
@@ -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
drivers/watchdog/Makefile +1 -0
index feb6270fdbde..36855375e0ce 100644
@@ -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
drivers/watchdog/aspeed_wdt.c new file +212 -0
new file mode 100644
index 000000000000..f5ad8023c2e6
@@ -0,0 +1,212 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * Joel Stanley <[email protected]>
+ *
+ * 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");
#19 lore raw
In reply to Joel Stanley

Hi Joel,

Quoted 36 lines

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

Quoted 11 lines
 .../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
Signature · 1 line
2.8.1

These patches have been added to linux-watchdog-next.

Kind regards,
Wim.