On 11/05/2015 01:06 AM, Oleksij Rempel wrote:
Add WD support for Alphascale asm9260 SoC. This driver
provide support for different function modes:
- HW mode to trigger SoC reset on timeout
- SW mode do soft reset if needed
- DEBUG mode
Optional support for stopping watchdog. If reset binding are not provided
this driver will work in nowayout mode.
In general, this is considered to be orthogonal: If the user doesn't want
nowayout, and if the watchdog can not be stopped, other drivers issue a warning
that the watchdog was not stopped, and that the system will likely restart.
Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
---
.../bindings/watchdog/alphascale-asm9260.txt | 39 ++
drivers/watchdog/Kconfig | 9 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/asm9260_wdt.c | 415 +++++++++++++++++++++
4 files changed, 464 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt
create mode 100644 drivers/watchdog/asm9260_wdt.c
diff --git a/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt b/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt
new file mode 100644
index 0000000..6e54d1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/alphascale-asm9260.txt
@@ -0,0 +1,39 @@
+Alphascale asm9260 Watchdog timer
+
+Required properties:
+
+- compatible : should be "alphascale,asm9260-wdt".
+- reg : Specifies base physical address and size of the registers.
+- clocks : the clocks feeding the watchdog timer. See clock-bindings.txt
+- clock-names : should be set to
+ "mod" - source for tick counter.
+ "ahb" - ahb gate.
+
+Optional properties:
+- resets : phandle pointing to the system reset controller with correct
+ reset line index for watchdog controller reset. This propertie is
+ required if you need to disable "nowayout" and it neened only with
+ CONFIG_WATCHDOG_NOWAYOUT=n.
+ Without reseting this WD controller, it is not possible to stop
+ counter.
+- reset-names : should be set to "wdt_rst" if "resets" is used.
+- timeout-sec : shall contain the default watchdog timeout in seconds,
+ if unset, the default timeout is 30 seconds.
+- alphascale,mode : tree modes are supported
+ "hw" - hw reset (defaul).
+ "sw" - sw reset.
+ "debug" - no action is taken.
+
+Example:
+
+watchdog0: watchdog@80048000 {
+ compatible = "alphascale,asm9260-wdt";
+ reg = <0x80048000 0x10>;
+ clocks = <&acc CLKID_SYS_WDT>, <&acc CLKID_AHB_WDT>;
+ clock-names = "mod", "ahb";
+ interrupts = <55>;
+ resets = <&rst WDT_RESET>;
+ reset-names = "wdt_rst";
+ timeout-sec = <30>;
+ alphascale,mode = "hw";
+};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c68edc1..cc5f675 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -173,6 +173,15 @@ config ARM_SP805_WATCHDOG
ARM Primecell SP805 Watchdog timer. This will reboot your system when
the timeout is reached.
+config ASM9260_WATCHDOG
+ tristate "Alphascale ASM9260 watchdog"
+ depends on MACH_ASM9260
+ depends on OF
+ select WATCHDOG_CORE
+ help
+ Watchdog timer embedded into Alphascale asm9260 chips. This will reboot your
+ system when the timeout is reached.
+
config AT91RM9200_WATCHDOG
tristate "AT91RM9200 watchdog"
depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c616e3..bd7b0cd 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
# ARM Architecture
obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
+obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o
obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o
diff --git a/drivers/watchdog/asm9260_wdt.c b/drivers/watchdog/asm9260_wdt.c
new file mode 100644
index 0000000..9f2c321
--- /dev/null
+++ b/drivers/watchdog/asm9260_wdt.c
@@ -0,0 +1,415 @@
+/*
+ * Watchdog driver for Alphascale ASM9260.
+ *
+ * Copyright (c) 2014 Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
I don't see any module parameters. Is this include needed ?
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/reset.h>
+#include <linux/uaccess.h>
Is this include needed ?
+#include <linux/watchdog.h>
+
+#define CLOCK_FREQ 1000000
+
+/* Watchdog Mode register */
+#define HW_WDMOD 0x00
+/* Wake interrupt. Set by HW, can't be cleared. */
+#define BM_MOD_WDINT BIT(3)
+/* This bit set if timeout reached. Cleared by SW. */
+#define BM_MOD_WDTOF BIT(2)
+/* HW Reset on timeout */
+#define BM_MOD_WDRESET BIT(1)
+/* WD enable */
+#define BM_MOD_WDEN BIT(0)
+
+/*
+ * Watchdog Timer Constant register
+ * Minimal value is 0xff, the meaning of this value
+ * depends on used clock: T = WDCLK * (0xff + 1) * 4
+ */
+#define HW_WDTC 0x04
+#define BM_WDTC_MAX(freq) (0x7fffffff / (freq))
+
+/* Watchdog Feed register */
+#define HW_WDFEED 0x08
+
+/* Watchdog Timer Value register */
+#define HW_WDTV 0x0c
+
+#define ASM9260_WDT_DEFAULT_TIMEOUT 30
+
+enum asm9260_wdt_mode {
+ HW_RESET,
+ SW_RESET,
+ DEBUG,
+};
+
+struct asm9260_wdt_priv {
+ struct device *dev;
+ struct watchdog_device wdd;
+ struct clk *clk;
+ struct clk *clk_ahb;
+ struct reset_control *rst;
+ struct notifier_block restart_handler;
+
+ void __iomem *iobase;
+ int irq;
+ unsigned long wdt_freq;
+ enum asm9260_wdt_mode mode;
+};
+
+static int asm9260_wdt_feed(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+ iowrite32(0xaa, priv->iobase + HW_WDFEED);
+ iowrite32(0x55, priv->iobase + HW_WDFEED);
+
+ return 0;
+}
+
+static unsigned int asm9260_wdt_gettimeleft(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 counter;
+
+ counter = ioread32(priv->iobase + HW_WDTV);
+
+ return DIV_ROUND_CLOSEST(counter, priv->wdt_freq);
+}
+
+static int asm9260_wdt_updatetimeout(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 counter;
+
+ counter = wdd->timeout * priv->wdt_freq;
+
+ iowrite32(counter, priv->iobase + HW_WDTC);
+
+ return 0;
+}
+
+static int asm9260_wdt_enable(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+ u32 mode = 0;
+
+ if (priv->mode == HW_RESET)
+ mode = BM_MOD_WDRESET;
+
+ iowrite32(BM_MOD_WDEN | mode, priv->iobase + HW_WDMOD);
+
+ asm9260_wdt_updatetimeout(wdd);
+
+ asm9260_wdt_feed(wdd);
+
+ return 0;
+}
+
+static int asm9260_wdt_disable(struct watchdog_device *wdd)
+{
+ struct asm9260_wdt_priv *priv = watchdog_get_drvdata(wdd);
+
+ /* The only way to disable WD is to reset it. */
+ reset_control_assert(priv->rst);
+ reset_control_deassert(priv->rst);
+
I am a bit wary of depending on the infrastructure to never call this code
if priv->rst is NULL, due to having nowayout set.
You should at least add a comment here indicating that the code
depends on this behavior.
My preferred solution, though, would be to stop playing with nowayout and
dump a warning to the console if the watchdog could not be stopped.
+ return 0;
+}
+
+static int asm9260_wdt_settimeout(struct watchdog_device *wdd, unsigned int to)
+{
+ wdd->timeout = to;
+ asm9260_wdt_updatetimeout(wdd);
+
+ return 0;
+}
+
+static void asm9260_wdt_sys_reset(struct asm9260_wdt_priv *priv)
+{
+ /* init WD if it was not started */
+ priv->wdd.timeout = 1;
+
Wouldn't it make more sense to write a smaller value into HW_WDTC directly ?
Unless I am missing something, this creates a one-second delay which seems
unnecessary.
+ iowrite32(BM_MOD_WDEN | BM_MOD_WDRESET, priv->iobase + HW_WDMOD);
+
+ asm9260_wdt_updatetimeout(&priv->wdd);
+ /* first pass correct sequence */
+ asm9260_wdt_feed(&priv->wdd);
+ /*
+ * Then write wrong pattern to the feed to trigger reset
+ * ASAP.
+ */
+ iowrite32(0xff, priv->iobase + HW_WDFEED);
+
+ mdelay(1000);
+}
+
+static irqreturn_t asm9260_wdt_irq(int irq, void *devid)
+{
+ struct asm9260_wdt_priv *priv = devid;
+ u32 stat;
+
+ stat = ioread32(priv->iobase + HW_WDMOD);
+ if (!(stat & BM_MOD_WDINT))
+ return IRQ_NONE;
+
+ if (priv->mode == DEBUG)
+ dev_info(priv->dev, "Watchdog Timeout. Do nothing.\n");
+ else {
+ dev_info(priv->dev, "Watchdog Timeout. Doing SW Reset.\n");
+ asm9260_wdt_sys_reset(priv);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int asm9260_restart_handler(struct notifier_block *this,
+ unsigned long mode, void *cmd)
+{
+ struct asm9260_wdt_priv *priv =
+ container_of(this, struct asm9260_wdt_priv, restart_handler);
+
+ asm9260_wdt_sys_reset(priv);
+
+ return NOTIFY_DONE;
+}
+
+static const struct watchdog_info asm9260_wdt_ident = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING
+ | WDIOF_MAGICCLOSE,
+ .identity = "Alphascale asm9260 Watchdog",
+};
+
+static struct watchdog_ops asm9260_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = asm9260_wdt_enable,
+ .stop = asm9260_wdt_disable,
+ .get_timeleft = asm9260_wdt_gettimeleft,
+ .ping = asm9260_wdt_feed,
+ .set_timeout = asm9260_wdt_settimeout,
+};
+
+static int __init asm9260_wdt_get_dt_clks(struct asm9260_wdt_priv *priv)
+{
+ int clk_idx = 0, err;
+ unsigned long clk;
+
+ priv->clk = devm_clk_get(priv->dev, "mod");
+ if (IS_ERR(priv->clk))
+ goto clk_err;
+
+ /* configure AHB clock */
+ clk_idx = 1;
+ priv->clk_ahb = devm_clk_get(priv->dev, "ahb");
+ if (IS_ERR(priv->clk_ahb))
+ goto clk_err;
+
Seems you are using clk_err only to display an error message.
This is not the intended use of error jump labels.
+ err = clk_prepare_enable(priv->clk_ahb);
+ if (err) {
+ dev_err(priv->dev, "Failed to enable ahb_clk!\n");
+ goto out_err;
+ }
+
+ err = clk_set_rate(priv->clk, CLOCK_FREQ);
+ if (err) {
+ clk_disable_unprepare(priv->clk_ahb);
+ dev_err(priv->dev, "Failed to set rate!\n");
+ goto out_err;
+ }
+
+ err = clk_prepare_enable(priv->clk);
+ if (err) {
+ clk_disable_unprepare(priv->clk_ahb);
+ dev_err(priv->dev, "Failed to enable clk!\n");
+ goto out_err;
+ }
+
+ /* wdt has internal divider */
+ clk = clk_get_rate(priv->clk);
+ if (!clk) {
+ clk_disable_unprepare(priv->clk);
+ clk_disable_unprepare(priv->clk_ahb);
The basic idea with error exit jumps is to keep the error handling
there. This code defeats the purpose.
+ dev_err(priv->dev, "Failed, clk is 0!\n");
+ goto out_err;
+ }
+
+ priv->wdt_freq = clk / 2;
+
+ return 0;
+
+clk_err:
+ dev_err(priv->dev, "Failed to get clk (%s)\n",
+ clk_idx ? "ahb" : "mod");
This is an odd use of an error return function. I would suggest to
create the error messages where they occur, and only handle cleanup here,
as suggested in the coding style document.
+out_err:
This label is not needed. Any code jumping to it can return immediately.
+ return -EFAULT;
EFAULT: /* Bad address */
Commonly used to identify a bad address passed from user space.
I don't think it is appropriate here. Besides, why override the already
known error in the first place ?
+}
+
+static int __init asm9260_wdt_get_dt_mode(struct asm9260_wdt_priv *priv)
+{
+ const char *tmp;
+ int ret;
+
+ /* default mode */
+ priv->mode = HW_RESET;
+
+ ret = of_property_read_string(priv->dev->of_node,
+ "alphascale,mode", &tmp);
Please align continuation lines with '('.
+ if (ret < 0)
+ return ret;
+
+ if (!strcmp(tmp, "hw"))
+ priv->mode = HW_RESET;
+ else if (!strcmp(tmp, "sw"))
+ priv->mode = SW_RESET;
+ else if (!strcmp(tmp, "debug"))
+ priv->mode = DEBUG;
+ else {
+ dev_warn(priv->dev, "unknown reset-type: %s. Using defaul \"hw\" mode.",
+ tmp);
s/defaul/default/
+ return -EINVAL;
Why return an error (instead of void) if it isn't used ?
+ }
+
+ return 0;
+}
+
+static int __init asm9260_wdt_probe(struct platform_device *pdev)
+{
+ struct asm9260_wdt_priv *priv;
+ struct watchdog_device *wdd;
+ struct resource *res;
+ bool nowayout = WATCHDOG_NOWAYOUT;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(struct asm9260_wdt_priv),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->iobase = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(priv->iobase))
+ return PTR_ERR(priv->iobase);
+
+ ret = asm9260_wdt_get_dt_clks(priv);
+ if (ret)
+ return ret;
+
+ priv->rst = devm_reset_control_get(&pdev->dev, "wdt_rst");
+ if (IS_ERR(priv->rst)) {
+ nowayout = 1;
+ priv->rst = NULL;
+ }
+
+ wdd = &priv->wdd;
+ wdd->info = &asm9260_wdt_ident;
+ wdd->ops = &asm9260_wdt_ops;
+ wdd->min_timeout = 1;
+ wdd->max_timeout = BM_WDTC_MAX(priv->wdt_freq);
+ wdd->parent = &pdev->dev;
+
+ watchdog_set_drvdata(wdd, priv);
+
+ /*
+ * If 'timeout-sec' unspecified in devicetree, assume a 30 second
+ * default, unless the max timeout is less than 30 seconds, then use
+ * the max instead.
+ */
+ wdd->timeout = ASM9260_WDT_DEFAULT_TIMEOUT;
+ watchdog_init_timeout(wdd, 0, &pdev->dev);
+
+ watchdog_set_nowayout(wdd, nowayout);
+
+ asm9260_wdt_get_dt_mode(priv);
+
+ if (priv->mode != HW_RESET)
+ priv->irq = platform_get_irq(pdev, 0);
+
+ if (priv->irq > 0) {
+ /*
+ * Not all supported platforms specify an interrupt for the
+ * watchdog, so let's make it optional.
+ */
+ ret = devm_request_irq(&pdev->dev, priv->irq,
+ asm9260_wdt_irq, IRQF_SHARED,
+ pdev->name, priv);
+ if (ret < 0)
+ dev_err(&pdev->dev, "failed to request IRQ\n");
dev_warn since you don't bail out.
+ }
+
+ ret = watchdog_register_device(wdd);
+ if (ret)
+ goto clk_off;
+
+ platform_set_drvdata(pdev, priv);
+
+ priv->restart_handler.notifier_call = asm9260_restart_handler;
+ priv->restart_handler.priority = 128;
+ ret = register_restart_handler(&priv->restart_handler);
+ if (ret)
+ dev_err(&pdev->dev, "cannot register restart handler\n");
dev_warn since you don't bail out.
+
+ dev_info(&pdev->dev, "Watchdog enabled (to=%d, nw=%d, mode=%d)\n",
+ wdd->timeout, nowayout, priv->mode);
+ return 0;
+
+clk_off:
+ clk_disable_unprepare(priv->clk);
+ clk_disable_unprepare(priv->clk_ahb);
+ return ret;
+}
+
+static void asm9260_wdt_shutdown(struct platform_device *pdev)
+{
+ struct asm9260_wdt_priv *priv = platform_get_drvdata(pdev);
+
+ asm9260_wdt_disable(&priv->wdd);
+}
+
+static int __exit asm9260_wdt_remove(struct platform_device *pdev)
+{
+ struct asm9260_wdt_priv *priv = platform_get_drvdata(pdev);
+
+ asm9260_wdt_shutdown(pdev);
You could just call asm9260_wdt_disable() directly.
+
+ unregister_restart_handler(&priv->restart_handler);
+ clk_disable_unprepare(priv->clk);
+ clk_disable_unprepare(priv->clk_ahb);
+
+ return 0;
+}
+
+static const struct of_device_id asm9260_wdt_of_match[] = {
+ { .compatible = "alphascale,asm9260-wdt"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, asm9260_wdt_of_match);
+
+static struct platform_driver asm9260_wdt_driver = {
+ .driver = {
+ .name = "asm9260-wdt",
+ .owner = THIS_MODULE,
+ .of_match_table = asm9260_wdt_of_match,
+ },
+ .probe = asm9260_wdt_probe,
+ .remove = asm9260_wdt_remove,
+ .shutdown = asm9260_wdt_shutdown,
+};
+module_platform_driver(asm9260_wdt_driver);
+
+MODULE_DESCRIPTION("asm9260 WatchDog Timer Driver");
+MODULE_AUTHOR("Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>");
+MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
