| Message ID | 1463062631-16432-3-git-send-email-joel@jms.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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"); >
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
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 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); +} + +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");
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