[PATCH v2 0/2] watchdog: bcm281xx: Watchdog Driver

[PATCH v2 0/2] watchdog: bcm281xx: Watchdog Driver

[Markus Mayer]

This is version 2 of the watchdog driver for the BCM281xx family of mobile SoCs.

Changes since version 1:

- Added module name to "help" section in Kconfig
- A few cosmetic code simplifications and fixes
- Removed most dev_info() calls and changed the remaining ones to dev_dbg()
- Renamed SECWDOG_WD_LOAD_FLAG_MASK to SECWDOG_WD_LOAD_FLAG
- Added some comments to secure_register_read() and struct bcm_kona_wdt
- Added delay to secure_register_read()
- Reduced maximum retry loop from 10000 to 1000
- Introduced "busy_count" variable to count how often secure_register_read()
  gets stalled; this is available through debugfs
- Simplified secure_register_read() to return -ETIMEDOUT rather than using
  a variable parameter to indicate a timeout error
- Got rid of all uses of -EAGAIN
- Fixed return value check for debugfs_create_dir()
- Simplified bcm_kona_wdt_debugfs_init() by getting rid of goto
- Created new generic function bcm_kona_wdt_ctrl_reg_modify()
- The following functions now use bcm_kona_wdt_ctrl_reg_modify():
    - bcm_kona_wdt_set_resolution_reg()
    - bcm_kona_wdt_set_timeout_reg()
    - bcm_kona_wdt_stop()
- Made bcm_kona_wdt_set_timeout_reg() more generic, so bcm_kona_wdt_start()
  can use it
- Removed MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR)


Markus Mayer (2):
  watchdog: bcm281xx: Watchdog Driver
  ARM: bcm281xx: watchdog configuration

 arch/arm/configs/bcm_defconfig  |    3 +
 drivers/watchdog/Kconfig        |   22 +++
 drivers/watchdog/Makefile       |    1 +
 drivers/watchdog/bcm_kona_wdt.c |  367 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 393 insertions(+)
 create mode 100644 drivers/watchdog/bcm_kona_wdt.c

-- 
1.7.9.5

[PATCH v2 1/2] watchdog: bcm281xx: Watchdog Driver

[Markus Mayer]

This commit adds support for the watchdog timer used on the BCM281xx
family of SoCs.

Signed-off-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 drivers/watchdog/Kconfig        |   22 +++
 drivers/watchdog/Makefile       |    1 +
 drivers/watchdog/bcm_kona_wdt.c |  367 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 390 insertions(+)
 create mode 100644 drivers/watchdog/bcm_kona_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index d1d53f3..fe8bd21 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1121,6 +1121,28 @@ config BCM2835_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called bcm2835_wdt.
 
+config BCM_KONA_WDT
+	tristate "BCM Kona Watchdog"
+	depends on ARCH_BCM
+	select WATCHDOG_CORE
+	help
+	  Support for the watchdog timer on the following Broadcom BCM281xx 
+	  family, which includes BCM11130, BCM11140, BCM11351, BCM28145 and
+	  BCM28155 variants.
+
+	  Say 'Y' or 'M' here to enable the driver. The module will be called
+	  bcm_kona_wdt.
+
+config BCM_KONA_WDT_DEBUG
+	bool "DEBUGFS support for BCM Kona Watchdog"
+	depends on BCM_KONA_WDT
+	help
+	  If enabled, adds /sys/kernel/debug/bcm-kona-wdt/info which provides
+	  access to the driver's internal data structures as well as watchdog
+	  timer hardware registres.
+
+	  If in doubt, say 'N'.
+
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 6c5bb27..7c860ca 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
 obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
 obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
 obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
+obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/bcm_kona_wdt.c b/drivers/watchdog/bcm_kona_wdt.c
new file mode 100644
index 0000000..5a03d5a
--- /dev/null
+++ b/drivers/watchdog/bcm_kona_wdt.c
@@ -0,0 +1,367 @@
+/*
+ * Copyright (C) 2013 Broadcom Corporation
+ *
+ * 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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define SECWDOG_CTRL_REG		0x00000000
+#define SECWDOG_COUNT_REG		0x00000004
+
+#define SECWDOG_RESERVED_MASK		0x1dffffff
+#define SECWDOG_WD_LOAD_FLAG		0x10000000
+#define SECWDOG_EN_MASK			0x08000000
+#define SECWDOG_SRSTEN_MASK		0x04000000
+#define SECWDOG_RES_MASK		0x00f00000
+#define SECWDOG_COUNT_MASK		0x000fffff
+
+#define SECWDOG_MAX_COUNT		SECWDOG_COUNT_MASK
+#define SECWDOG_CLKS_SHIFT		20
+#define SECWDOG_MAX_RES			15
+#define SECWDOG_DEFAULT_RESOLUTION	4
+#define SECWDOG_MAX_TRY			1000
+
+#define SECS_TO_TICKS(x, w)		((x) << (w)->resolution)
+#define TICKS_TO_SECS(x, w)		((x) >> (w)->resolution)
+
+#define BCM_KONA_WDT_NAME		"bcm-kona-wdt"
+
+struct bcm_kona_wdt {
+	void __iomem *base;
+	/*
+	 * One watchdog tick is 1/(2^resolution) seconds. Resolution can take
+	 * the values 0-15, meaning one tick can be 1s to 30.52us. Our default
+	 * resolution of 4 means one tick is 62.5ms.
+	 *
+	 * The watchdog counter is 20 bits. Depending on resolution, the maximum
+	 * counter value of 0xfffff expires after about 12 days (resolution 0)
+	 * down to only 32s (resolution 15). The default resolution of 4 gives
+	 * us a maximum of about 18 hours and 12 minutes before the watchdog
+	 * times out.
+	 */
+	int resolution;
+	spinlock_t lock;
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+	struct dentry *debugfs;
+#endif
+};
+
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+static unsigned long busy_count = 0;
+#endif
+
+static int secure_register_read(void __iomem *addr)
+{
+	uint32_t val;
+	unsigned count = 0;
+
+	/*
+	 * If the WD_LOAD_FLAG is set, the watchdog counter field is being
+	 * updated in hardware. Once the WD timer is updated in hardware, it
+	 * gets cleared.
+	 */
+	do {
+		if (unlikely(count > 1)) {
+			udelay(5);
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+			/* To keep track how often we get stalled here. */
+			busy_count++;
+#endif
+		}
+		val = readl_relaxed(addr);
+		count++;
+	} while ((val & SECWDOG_WD_LOAD_FLAG) && count < SECWDOG_MAX_TRY);
+
+	/* This is the only place we return a negative value. */
+	if (val & SECWDOG_WD_LOAD_FLAG) {
+		return -ETIMEDOUT;
+	}
+
+	/* We always mask out reserved bits. */
+	val &= SECWDOG_RESERVED_MASK;
+
+	return val;
+}
+
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+
+static int bcm_kona_wdt_dbg_show(struct seq_file *s, void *data)
+{
+	int ctl_val, cur_val, ret;
+	unsigned long flags;
+	struct bcm_kona_wdt *wdt = s->private;
+
+	if (!wdt)
+		return seq_printf(s, "No device pointer\n");
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	ctl_val = secure_register_read(wdt->base + SECWDOG_CTRL_REG);
+	cur_val = secure_register_read(wdt->base + SECWDOG_COUNT_REG);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	if (ctl_val < 0 || cur_val < 0) {
+		ret = seq_printf(s, "Error accessing hardware\n");
+	} else {
+		int ctl, cur, ctl_sec, cur_sec, res;
+
+		ctl = ctl_val & SECWDOG_COUNT_MASK;
+		res = (ctl_val & SECWDOG_RES_MASK) >> SECWDOG_CLKS_SHIFT;
+		cur = cur_val & SECWDOG_COUNT_MASK;
+		ctl_sec = TICKS_TO_SECS(ctl, wdt);
+		cur_sec = TICKS_TO_SECS(cur, wdt);
+		ret = seq_printf(s, "Resolution: %d / %d\n"
+				"Control: %d s / %d (%#x) ticks\n"
+				"Current: %d s / %d (%#x) ticks\n"
+				"Busy count: %lu\n", res,
+				wdt->resolution, ctl_sec, ctl, ctl, cur_sec,
+				cur, cur, busy_count);
+	}
+
+	return ret;
+}
+
+static int bcm_kona_dbg_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, bcm_kona_wdt_dbg_show, inode->i_private);
+}
+
+static const struct file_operations bcm_kona_dbg_operations = {
+	.open		= bcm_kona_dbg_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static struct dentry *bcm_kona_wdt_debugfs_init(struct bcm_kona_wdt *wdt,
+	struct watchdog_device *wdd)
+{
+	struct dentry *dir;
+
+	dir = debugfs_create_dir(BCM_KONA_WDT_NAME, NULL);
+	if (IS_ERR_OR_NULL(dir))
+		return NULL;
+
+	if (debugfs_create_file("info", S_IFREG | S_IRUGO, dir, wdt,
+				&bcm_kona_dbg_operations))
+		return dir;
+
+	/* Clean up */
+	debugfs_remove_recursive(dir);
+	return NULL;
+}
+
+static void bcm_kona_debugfs_exit(struct dentry *dir)
+{
+	debugfs_remove_recursive(dir);
+}
+
+#endif /* CONFIG_BCM_KONA_WDT_DEBUG */
+
+static int bcm_kona_wdt_ctrl_reg_modify(struct bcm_kona_wdt *wdt,
+					unsigned mask, unsigned newval)
+{
+	int val;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG);
+	if (val < 0) {
+		ret = val;
+	} else {
+		val &= ~mask;
+		val |= newval;
+		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
+	}
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return ret;
+}
+
+static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
+{
+	if (wdt->resolution > SECWDOG_MAX_RES)
+		return -EINVAL;
+
+	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_RES_MASK,
+					wdt->resolution << SECWDOG_CLKS_SHIFT);
+}
+
+static int bcm_kona_wdt_set_timeout_reg(struct watchdog_device *wdog,
+					unsigned watchdog_flags)
+{
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_COUNT_MASK,
+					SECS_TO_TICKS(wdog->timeout, wdt) |
+					watchdog_flags);
+}
+
+static int bcm_kona_wdt_set_timeout(struct watchdog_device *wdog,
+	unsigned int t)
+{
+	wdog->timeout = t;
+	return 0;
+}
+
+static unsigned int bcm_kona_wdt_get_timeleft(struct watchdog_device *wdog)
+{
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
+	int val;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	val = secure_register_read(wdt->base + SECWDOG_COUNT_REG);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	if (val < 0)
+		return val;
+
+	return TICKS_TO_SECS(val & SECWDOG_COUNT_MASK, wdt);
+}
+
+static int bcm_kona_wdt_start(struct watchdog_device *wdog)
+{
+	return bcm_kona_wdt_set_timeout_reg(wdog,
+					SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK);
+}
+
+static int bcm_kona_wdt_stop(struct watchdog_device *wdog)
+{
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_EN_MASK |
+						SECWDOG_SRSTEN_MASK, 0);
+}
+
+static struct watchdog_ops bcm_kona_wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	bcm_kona_wdt_start,
+	.stop =		bcm_kona_wdt_stop,
+	.set_timeout =	bcm_kona_wdt_set_timeout,
+	.get_timeleft =	bcm_kona_wdt_get_timeleft,
+};
+
+static struct watchdog_info bcm_kona_wdt_info = {
+	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+	.identity =	"Broadcom Kona Watchdog Timer",
+};
+
+static struct watchdog_device bcm_kona_wdt_wdd = {
+	.info =		&bcm_kona_wdt_info,
+	.ops =		&bcm_kona_wdt_ops,
+	.min_timeout =	1,
+	.max_timeout =	SECWDOG_MAX_COUNT >> SECWDOG_DEFAULT_RESOLUTION,
+	.timeout =	SECWDOG_MAX_COUNT >> SECWDOG_DEFAULT_RESOLUTION,
+};
+
+static void bcm_kona_wdt_shutdown(struct platform_device *pdev)
+{
+	bcm_kona_wdt_stop(&bcm_kona_wdt_wdd);
+}
+
+static int bcm_kona_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm_kona_wdt *wdt;
+	struct resource *res;
+	int ret;
+
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(dev, "Failed to allocate memory for watchdog device");
+		return -ENOMEM;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(wdt->base))
+		return -ENODEV;
+
+	wdt->resolution = SECWDOG_DEFAULT_RESOLUTION;
+	ret = bcm_kona_wdt_set_resolution_reg(wdt);
+	if (ret) {
+		dev_err(dev, "Failed to set resolution (error: %d)", ret);
+		return ret;
+	}
+
+	spin_lock_init(&wdt->lock);
+	platform_set_drvdata(pdev, wdt);
+	watchdog_set_drvdata(&bcm_kona_wdt_wdd, wdt);
+
+	ret = bcm_kona_wdt_set_timeout_reg(&bcm_kona_wdt_wdd, 0);
+	if (ret) {
+		dev_err(dev, "Failed set watchdog timeout");
+		return ret;
+	}
+
+	ret = watchdog_register_device(&bcm_kona_wdt_wdd);
+	if (ret) {
+		dev_err(dev, "Failed to register watchdog device");
+		return ret;
+	}
+
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+	wdt->debugfs = bcm_kona_wdt_debugfs_init(wdt, &bcm_kona_wdt_wdd);
+#endif
+	dev_dbg(dev, "Broadcom Kona Watchdog Timer");
+
+	return 0;
+}
+
+static int bcm_kona_wdt_remove(struct platform_device *pdev)
+{
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+	struct bcm_kona_wdt *wdt = platform_get_drvdata(pdev);
+
+	if (wdt->debugfs)
+		bcm_kona_debugfs_exit(wdt->debugfs);
+#endif /* CONFIG_BCM_KONA_WDT_DEBUG */
+	bcm_kona_wdt_shutdown(pdev);
+	watchdog_unregister_device(&bcm_kona_wdt_wdd);
+	dev_dbg(&pdev->dev, "Watchdog driver disabled");
+
+	return 0;
+}
+
+static const struct of_device_id bcm_kona_wdt_of_match[] = {
+	{ .compatible = "brcm,kona-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm_kona_wdt_of_match);
+
+static struct platform_driver bcm_kona_wdt_driver = {
+	.driver = {
+			.name = BCM_KONA_WDT_NAME,
+			.owner = THIS_MODULE,
+			.of_match_table = bcm_kona_wdt_of_match,
+		  },
+	.probe = bcm_kona_wdt_probe,
+	.remove = bcm_kona_wdt_remove,
+	.shutdown = bcm_kona_wdt_shutdown,
+};
+
+module_platform_driver(bcm_kona_wdt_driver);
+
+MODULE_AUTHOR("Markus Mayer <mmayer@broadcom.com>");
+MODULE_DESCRIPTION("Broadcom Kona Watchdog Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH v2 1/2] watchdog: bcm281xx: Watchdog Driver

[Markus Mayer]

On 15 November 2013 12:58, Markus Mayer <markus.mayer@linaro.org> wrote:
> This commit adds support for the watchdog timer used on the BCM281xx
> family of SoCs.
>
> Signed-off-by: Markus Mayer <markus.mayer@linaro.org>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---
>  drivers/watchdog/Kconfig        |   22 +++
>  drivers/watchdog/Makefile       |    1 +
>  drivers/watchdog/bcm_kona_wdt.c |  367 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 390 insertions(+)
>  create mode 100644 drivers/watchdog/bcm_kona_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index d1d53f3..fe8bd21 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1121,6 +1121,28 @@ config BCM2835_WDT
>           To compile this driver as a loadable module, choose M here.
>           The module will be called bcm2835_wdt.
>
> +config BCM_KONA_WDT
> +       tristate "BCM Kona Watchdog"
> +       depends on ARCH_BCM
> +       select WATCHDOG_CORE
> +       help
> +         Support for the watchdog timer on the following Broadcom BCM281xx
> +         family, which includes BCM11130, BCM11140, BCM11351, BCM28145 and
> +         BCM28155 variants.
> +
> +         Say 'Y' or 'M' here to enable the driver. The module will be called
> +         bcm_kona_wdt.
> +
> +config BCM_KONA_WDT_DEBUG
> +       bool "DEBUGFS support for BCM Kona Watchdog"
> +       depends on BCM_KONA_WDT
> +       help
> +         If enabled, adds /sys/kernel/debug/bcm-kona-wdt/info which provides
> +         access to the driver's internal data structures as well as watchdog
> +         timer hardware registres.
> +
> +         If in doubt, say 'N'.
> +
>  config LANTIQ_WDT
>         tristate "Lantiq SoC watchdog"
>         depends on LANTIQ
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 6c5bb27..7c860ca 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
>  obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
>  obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
>  obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
> +obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>
>  # AVR32 Architecture
>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/bcm_kona_wdt.c b/drivers/watchdog/bcm_kona_wdt.c
> new file mode 100644
> index 0000000..5a03d5a
> --- /dev/null
> +++ b/drivers/watchdog/bcm_kona_wdt.c
> @@ -0,0 +1,367 @@
> +/*
> + * Copyright (C) 2013 Broadcom Corporation
> + *
> + * 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 version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define SECWDOG_CTRL_REG               0x00000000
> +#define SECWDOG_COUNT_REG              0x00000004
> +
> +#define SECWDOG_RESERVED_MASK          0x1dffffff
> +#define SECWDOG_WD_LOAD_FLAG           0x10000000
> +#define SECWDOG_EN_MASK                        0x08000000
> +#define SECWDOG_SRSTEN_MASK            0x04000000
> +#define SECWDOG_RES_MASK               0x00f00000
> +#define SECWDOG_COUNT_MASK             0x000fffff
> +
> +#define SECWDOG_MAX_COUNT              SECWDOG_COUNT_MASK
> +#define SECWDOG_CLKS_SHIFT             20
> +#define SECWDOG_MAX_RES                        15
> +#define SECWDOG_DEFAULT_RESOLUTION     4
> +#define SECWDOG_MAX_TRY                        1000
> +
> +#define SECS_TO_TICKS(x, w)            ((x) << (w)->resolution)
> +#define TICKS_TO_SECS(x, w)            ((x) >> (w)->resolution)
> +
> +#define BCM_KONA_WDT_NAME              "bcm-kona-wdt"

I just noticed that this should be "bcm_kona_wdt" instead.

-Markus

[PATCH v2 1/2] watchdog: bcm281xx: Watchdog Driver

[One Thousand Gnomes]

> > +
> > +#define BCM_KONA_WDT_NAME              "bcm-kona-wdt"
> 
> I just noticed that this should be "bcm_kona_wdt" instead.


Otherwise looks good.

Alan

[PATCH v2 1/2] watchdog: bcm281xx: Watchdog Driver

[Guenter Roeck]

On 11/15/2013 12:58 PM, Markus Mayer wrote:
> This commit adds support for the watchdog timer used on the BCM281xx
> family of SoCs.
>
> Signed-off-by: Markus Mayer <markus.mayer@linaro.org>

You are using two different e-mail addresses, the one here and the one in MODULE_AUTHOR.
Any chance to use only one ? Of course, there may be a reason, so maybe it is ok.

> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---
>   drivers/watchdog/Kconfig        |   22 +++
>   drivers/watchdog/Makefile       |    1 +
>   drivers/watchdog/bcm_kona_wdt.c |  367 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 390 insertions(+)
>   create mode 100644 drivers/watchdog/bcm_kona_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index d1d53f3..fe8bd21 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1121,6 +1121,28 @@ config BCM2835_WDT
>   	  To compile this driver as a loadable module, choose M here.
>   	  The module will be called bcm2835_wdt.
>
> +config BCM_KONA_WDT
> +	tristate "BCM Kona Watchdog"
> +	depends on ARCH_BCM
> +	select WATCHDOG_CORE
> +	help
> +	  Support for the watchdog timer on the following Broadcom BCM281xx
> +	  family, which includes BCM11130, BCM11140, BCM11351, BCM28145 and
> +	  BCM28155 variants.
> +
> +	  Say 'Y' or 'M' here to enable the driver. The module will be called
> +	  bcm_kona_wdt.
> +
> +config BCM_KONA_WDT_DEBUG
> +	bool "DEBUGFS support for BCM Kona Watchdog"
> +	depends on BCM_KONA_WDT
> +	help
> +	  If enabled, adds /sys/kernel/debug/bcm-kona-wdt/info which provides
> +	  access to the driver's internal data structures as well as watchdog
> +	  timer hardware registres.
> +
> +	  If in doubt, say 'N'.
> +
>   config LANTIQ_WDT
>   	tristate "Lantiq SoC watchdog"
>   	depends on LANTIQ
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 6c5bb27..7c860ca 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
>   obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
>   obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
>   obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
> +obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>
>   # AVR32 Architecture
>   obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/bcm_kona_wdt.c b/drivers/watchdog/bcm_kona_wdt.c
> new file mode 100644
> index 0000000..5a03d5a
> --- /dev/null
> +++ b/drivers/watchdog/bcm_kona_wdt.c
> @@ -0,0 +1,367 @@
> +/*
> + * Copyright (C) 2013 Broadcom Corporation
> + *
> + * 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 version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define SECWDOG_CTRL_REG		0x00000000
> +#define SECWDOG_COUNT_REG		0x00000004
> +
> +#define SECWDOG_RESERVED_MASK		0x1dffffff
> +#define SECWDOG_WD_LOAD_FLAG		0x10000000
> +#define SECWDOG_EN_MASK			0x08000000
> +#define SECWDOG_SRSTEN_MASK		0x04000000
> +#define SECWDOG_RES_MASK		0x00f00000
> +#define SECWDOG_COUNT_MASK		0x000fffff
> +
> +#define SECWDOG_MAX_COUNT		SECWDOG_COUNT_MASK
> +#define SECWDOG_CLKS_SHIFT		20
> +#define SECWDOG_MAX_RES			15
> +#define SECWDOG_DEFAULT_RESOLUTION	4
> +#define SECWDOG_MAX_TRY			1000
> +
> +#define SECS_TO_TICKS(x, w)		((x) << (w)->resolution)
> +#define TICKS_TO_SECS(x, w)		((x) >> (w)->resolution)
> +
> +#define BCM_KONA_WDT_NAME		"bcm-kona-wdt"
> +

You mentioned this should be s/-/_/g in your other mail. Wonder why ?
This name doesn't have to match the file name.

> +struct bcm_kona_wdt {
> +	void __iomem *base;
> +	/*
> +	 * One watchdog tick is 1/(2^resolution) seconds. Resolution can take
> +	 * the values 0-15, meaning one tick can be 1s to 30.52us. Our default
> +	 * resolution of 4 means one tick is 62.5ms.
> +	 *
> +	 * The watchdog counter is 20 bits. Depending on resolution, the maximum
> +	 * counter value of 0xfffff expires after about 12 days (resolution 0)
> +	 * down to only 32s (resolution 15). The default resolution of 4 gives
> +	 * us a maximum of about 18 hours and 12 minutes before the watchdog
> +	 * times out.
> +	 */
> +	int resolution;
> +	spinlock_t lock;
> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +	struct dentry *debugfs;
> +#endif
> +};
> +
> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +static unsigned long busy_count = 0;

ERROR: do not initialise statics to 0 or NULL

> +#endif
> +
> +static int secure_register_read(void __iomem *addr)
> +{
> +	uint32_t val;
> +	unsigned count = 0;
> +
> +	/*
> +	 * If the WD_LOAD_FLAG is set, the watchdog counter field is being
> +	 * updated in hardware. Once the WD timer is updated in hardware, it
> +	 * gets cleared.
> +	 */
> +	do {
> +		if (unlikely(count > 1)) {
> +			udelay(5);
> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +			/* To keep track how often we get stalled here. */
> +			busy_count++;
> +#endif

Does the total number of stalls over the lifetime of the driver really provide value ?
It doesn't really tell you anything, or does it (the number will increase over time, so what ?).

It might make more sense to count the overall maximum, such as with

	if (count > busy_count)
		busy_count = count;

after the end of the loop. Just a thought.

> +		}
> +		val = readl_relaxed(addr);
> +		count++;
> +	} while ((val & SECWDOG_WD_LOAD_FLAG) && count < SECWDOG_MAX_TRY);
> +
> +	/* This is the only place we return a negative value. */
> +	if (val & SECWDOG_WD_LOAD_FLAG) {
> +		return -ETIMEDOUT;
> +	}
> +

WARNING: braces {} are not necessary for single statement blocks

> +	/* We always mask out reserved bits. */
> +	val &= SECWDOG_RESERVED_MASK;
> +
> +	return val;
> +}
> +
> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +
> +static int bcm_kona_wdt_dbg_show(struct seq_file *s, void *data)
> +{
> +	int ctl_val, cur_val, ret;
> +	unsigned long flags;
> +	struct bcm_kona_wdt *wdt = s->private;
> +
> +	if (!wdt)
> +		return seq_printf(s, "No device pointer\n");
> +
WARNING: Prefer seq_puts to seq_printf

> +	spin_lock_irqsave(&wdt->lock, flags);
> +	ctl_val = secure_register_read(wdt->base + SECWDOG_CTRL_REG);
> +	cur_val = secure_register_read(wdt->base + SECWDOG_COUNT_REG);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	if (ctl_val < 0 || cur_val < 0) {
> +		ret = seq_printf(s, "Error accessing hardware\n");

WARNING: Prefer seq_puts to seq_printf

> +	} else {
> +		int ctl, cur, ctl_sec, cur_sec, res;
> +
> +		ctl = ctl_val & SECWDOG_COUNT_MASK;
> +		res = (ctl_val & SECWDOG_RES_MASK) >> SECWDOG_CLKS_SHIFT;
> +		cur = cur_val & SECWDOG_COUNT_MASK;
> +		ctl_sec = TICKS_TO_SECS(ctl, wdt);
> +		cur_sec = TICKS_TO_SECS(cur, wdt);
> +		ret = seq_printf(s, "Resolution: %d / %d\n"
> +				"Control: %d s / %d (%#x) ticks\n"
> +				"Current: %d s / %d (%#x) ticks\n"
> +				"Busy count: %lu\n", res,
> +				wdt->resolution, ctl_sec, ctl, ctl, cur_sec,
> +				cur, cur, busy_count);
> +	}
> +
> +	return ret;
> +}
> +
> +static int bcm_kona_dbg_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, bcm_kona_wdt_dbg_show, inode->i_private);
> +}
> +
> +static const struct file_operations bcm_kona_dbg_operations = {
> +	.open		= bcm_kona_dbg_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};
> +
> +static struct dentry *bcm_kona_wdt_debugfs_init(struct bcm_kona_wdt *wdt,
> +	struct watchdog_device *wdd)
> +{
> +	struct dentry *dir;
> +
> +	dir = debugfs_create_dir(BCM_KONA_WDT_NAME, NULL);
> +	if (IS_ERR_OR_NULL(dir))
> +		return NULL;
> +
> +	if (debugfs_create_file("info", S_IFREG | S_IRUGO, dir, wdt,
> +				&bcm_kona_dbg_operations))
> +		return dir;
> +
> +	/* Clean up */
> +	debugfs_remove_recursive(dir);
> +	return NULL;
> +}
> +
> +static void bcm_kona_debugfs_exit(struct dentry *dir)
> +{
> +	debugfs_remove_recursive(dir);
> +}
> +
> +#endif /* CONFIG_BCM_KONA_WDT_DEBUG */
> +
> +static int bcm_kona_wdt_ctrl_reg_modify(struct bcm_kona_wdt *wdt,
> +					unsigned mask, unsigned newval)
> +{
> +	int val;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG);
> +	if (val < 0) {
> +		ret = val;
> +	} else {
> +		val &= ~mask;
> +		val |= newval;
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	}
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
> +{
> +	if (wdt->resolution > SECWDOG_MAX_RES)
> +		return -EINVAL;
> +
> +	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_RES_MASK,
> +					wdt->resolution << SECWDOG_CLKS_SHIFT);
> +}
> +
> +static int bcm_kona_wdt_set_timeout_reg(struct watchdog_device *wdog,
> +					unsigned watchdog_flags)
> +{
> +	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_COUNT_MASK,
> +					SECS_TO_TICKS(wdog->timeout, wdt) |
> +					watchdog_flags);
> +}
> +
> +static int bcm_kona_wdt_set_timeout(struct watchdog_device *wdog,
> +	unsigned int t)
> +{
> +	wdog->timeout = t;
> +	return 0;
> +}
> +
> +static unsigned int bcm_kona_wdt_get_timeleft(struct watchdog_device *wdog)
> +{
> +	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
> +	int val;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	val = secure_register_read(wdt->base + SECWDOG_COUNT_REG);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	if (val < 0)
> +		return val;
> +
> +	return TICKS_TO_SECS(val & SECWDOG_COUNT_MASK, wdt);
> +}
> +
> +static int bcm_kona_wdt_start(struct watchdog_device *wdog)
> +{
> +	return bcm_kona_wdt_set_timeout_reg(wdog,
> +					SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK);
> +}
> +
> +static int bcm_kona_wdt_stop(struct watchdog_device *wdog)
> +{
> +	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	return bcm_kona_wdt_ctrl_reg_modify(wdt, SECWDOG_EN_MASK |
> +						SECWDOG_SRSTEN_MASK, 0);

Your second-line indentation is really inconsistent. Preferred is to align with (.
If you don't like that, can you at least align to the same column ?

> +}
> +
> +static struct watchdog_ops bcm_kona_wdt_ops = {
> +	.owner =	THIS_MODULE,
> +	.start =	bcm_kona_wdt_start,
> +	.stop =		bcm_kona_wdt_stop,
> +	.set_timeout =	bcm_kona_wdt_set_timeout,
> +	.get_timeleft =	bcm_kona_wdt_get_timeleft,
> +};
> +
> +static struct watchdog_info bcm_kona_wdt_info = {
> +	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +			WDIOF_KEEPALIVEPING,
> +	.identity =	"Broadcom Kona Watchdog Timer",
> +};
> +
> +static struct watchdog_device bcm_kona_wdt_wdd = {
> +	.info =		&bcm_kona_wdt_info,
> +	.ops =		&bcm_kona_wdt_ops,
> +	.min_timeout =	1,
> +	.max_timeout =	SECWDOG_MAX_COUNT >> SECWDOG_DEFAULT_RESOLUTION,
> +	.timeout =	SECWDOG_MAX_COUNT >> SECWDOG_DEFAULT_RESOLUTION,
> +};
> +
> +static void bcm_kona_wdt_shutdown(struct platform_device *pdev)
> +{
> +	bcm_kona_wdt_stop(&bcm_kona_wdt_wdd);
> +}
> +
> +static int bcm_kona_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct bcm_kona_wdt *wdt;
> +	struct resource *res;
> +	int ret;
> +
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(dev, "Failed to allocate memory for watchdog device");

devm_kzalloc() already dumps a message, so this message is unnecessary.

> +		return -ENOMEM;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	wdt->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(wdt->base))
> +		return -ENODEV;
> +
> +	wdt->resolution = SECWDOG_DEFAULT_RESOLUTION;
> +	ret = bcm_kona_wdt_set_resolution_reg(wdt);
> +	if (ret) {
> +		dev_err(dev, "Failed to set resolution (error: %d)", ret);
> +		return ret;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +	platform_set_drvdata(pdev, wdt);
> +	watchdog_set_drvdata(&bcm_kona_wdt_wdd, wdt);
> +
> +	ret = bcm_kona_wdt_set_timeout_reg(&bcm_kona_wdt_wdd, 0);
> +	if (ret) {
> +		dev_err(dev, "Failed set watchdog timeout");
> +		return ret;
> +	}
> +
> +	ret = watchdog_register_device(&bcm_kona_wdt_wdd);
> +	if (ret) {
> +		dev_err(dev, "Failed to register watchdog device");
> +		return ret;
> +	}
> +
> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +	wdt->debugfs = bcm_kona_wdt_debugfs_init(wdt, &bcm_kona_wdt_wdd);
> +#endif
> +	dev_dbg(dev, "Broadcom Kona Watchdog Timer");
> +
> +	return 0;
> +}
> +
> +static int bcm_kona_wdt_remove(struct platform_device *pdev)
> +{
> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +	struct bcm_kona_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	if (wdt->debugfs)
> +		bcm_kona_debugfs_exit(wdt->debugfs);
> +#endif /* CONFIG_BCM_KONA_WDT_DEBUG */
> +	bcm_kona_wdt_shutdown(pdev);
> +	watchdog_unregister_device(&bcm_kona_wdt_wdd);
> +	dev_dbg(&pdev->dev, "Watchdog driver disabled");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id bcm_kona_wdt_of_match[] = {
> +	{ .compatible = "brcm,kona-wdt", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bcm_kona_wdt_of_match);
> +
> +static struct platform_driver bcm_kona_wdt_driver = {
> +	.driver = {
> +			.name = BCM_KONA_WDT_NAME,
> +			.owner = THIS_MODULE,
> +			.of_match_table =bcm_kona_wdt_of_match,
> +		  },
> +	.probe = bcm_kona_wdt_probe,
> +	.remove = bcm_kona_wdt_remove,
> +	.shutdown = bcm_kona_wdt_shutdown,
> +};
> +
> +module_platform_driver(bcm_kona_wdt_driver);
> +
> +MODULE_AUTHOR("Markus Mayer <mmayer@broadcom.com>");
> +MODULE_DESCRIPTION("Broadcom Kona Watchdog Driver");
> +MODULE_LICENSE("GPL v2");
>

You might also need

MODULE_ALIAS("platform:" BCM_KONA_WDT_NAME);

at least if the driver is ever loaded w/o fdt.

[PATCH v2 1/2] watchdog: bcm281xx: Watchdog Driver

[Markus Mayer]

On 15 November 2013 20:27, Guenter Roeck <linux@roeck-us.net> wrote:
> On 11/15/2013 12:58 PM, Markus Mayer wrote:
>>
>> This commit adds support for the watchdog timer used on the BCM281xx
>> family of SoCs.
>>
>> Signed-off-by: Markus Mayer <markus.mayer@linaro.org>
>
>
> You are using two different e-mail addresses, the one here and the one in
> MODULE_AUTHOR.
> Any chance to use only one ? Of course, there may be a reason, so maybe it
> is ok.

There is indeed reason behind the madness. I am wearing two hats of
sorts. I work for Broadcom, but I am assigned to Linaro, hence the
somewhat adventurous construction with two e-mail addresses.

I addressed your remaining comments and will be sending out v3 of the
patch shortly.

Thanks,
-Markus

-- 
Markus Mayer
Broadcom Landing Team

[PATCH v2 2/2] ARM: bcm281xx: watchdog configuration

[Markus Mayer]

This commit enables the watchdog driver for the BCM281xx family of SoCs.

Signed-off-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 arch/arm/configs/bcm_defconfig |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/bcm_defconfig b/arch/arm/configs/bcm_defconfig
index 6e49310..8b98e11 100644
--- a/arch/arm/configs/bcm_defconfig
+++ b/arch/arm/configs/bcm_defconfig
@@ -130,3 +130,6 @@ CONFIG_CRC_ITU_T=y
 CONFIG_CRC7=y
 CONFIG_XZ_DEC=y
 CONFIG_AVERAGE=y
+CONFIG_WATCHDOG=y
+CONFIG_BCM_KONA_WDT=y
+CONFIG_BCM_KONA_WDT_DEBUG=y
-- 
1.7.9.5