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

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

[Markus Mayer]

This series introduces the watchdog driver for the BCM281xx family of
mobile SoCs.

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

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

-- 
1.7.9.5

[PATCH 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        |   21 +++
 drivers/watchdog/Makefile       |    1 +
 drivers/watchdog/bcm_kona_wdt.c |  399 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 421 insertions(+)
 create mode 100644 drivers/watchdog/bcm_kona_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index d1d53f3..59013f6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1121,6 +1121,27 @@ 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.
+
+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..c47ac77
--- /dev/null
+++ b/drivers/watchdog/bcm_kona_wdt.c
@@ -0,0 +1,399 @@
+/*
+ * 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/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_MASK	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			10000
+
+#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;
+	int resolution;
+	spinlock_t lock;
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+	struct dentry *debugfs;
+#endif
+};
+
+static uint32_t secure_register_read(void __iomem *addr, int *timeout)
+{
+	uint32_t val;
+	unsigned count = 0;
+
+	do {
+		val = readl_relaxed(addr);
+		count++;
+	} while ((val & SECWDOG_WD_LOAD_FLAG_MASK) != 0 &&
+		count < SECWDOG_MAX_TRY);
+	if (timeout)
+		*timeout = ((val & SECWDOG_WD_LOAD_FLAG_MASK) != 0);
+
+	/* We always mask out reserved bits before returning the value. */
+	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)
+{
+	uint32_t ctl_val, cur_val;
+	int ret, ctl_timeout, cur_timeout;
+	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,
+				&ctl_timeout);
+	cur_val = secure_register_read(wdt->base + SECWDOG_COUNT_REG,
+				&cur_timeout);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	if (ctl_timeout || cur_timeout) {
+		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", res,
+				wdt->resolution, ctl_sec, ctl, ctl, cur_sec,
+				cur, cur);
+	}
+
+	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 (!dir)
+		return NULL;
+
+	if (!debugfs_create_file("info", S_IFREG | S_IRUGO, dir, wdt,
+				&bcm_kona_dbg_operations))
+		goto out_err;
+
+	return dir;
+
+out_err:
+	debugfs_remove_recursive(dir);
+	return NULL;
+}
+
+static void bcm_kona_debugfs_exit(struct dentry *debugfs)
+{
+	debugfs_remove_recursive(debugfs);
+}
+
+#endif /* CONFIG_BCM_KONA_WDT_DEBUG */
+
+
+static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
+{
+	uint32_t val;
+	int timeout;
+	unsigned long flags;
+	int ret = 0;
+
+	if (wdt->resolution > SECWDOG_MAX_RES)
+		return -EINVAL;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
+	if (!timeout) {
+		val &= ~SECWDOG_RES_MASK;
+		val |= wdt->resolution << SECWDOG_CLKS_SHIFT;
+		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
+	} else {
+		ret = -EAGAIN;
+	}
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return ret;
+}
+
+static int bcm_kona_wdt_set_timeout_reg(struct watchdog_device *wdog)
+{
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
+	uint32_t val;
+	int timeout;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
+	if (!timeout) {
+		val &= ~SECWDOG_COUNT_MASK;
+		val |= SECS_TO_TICKS(wdog->timeout, wdt);
+		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
+	} else {
+		ret = -EAGAIN;
+	}
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return ret;
+}
+
+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);
+	uint32_t val;
+	int timeout;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+	val = secure_register_read(wdt->base + SECWDOG_COUNT_REG, &timeout);
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	if (timeout)
+		return -EAGAIN;
+
+	return TICKS_TO_SECS(val & SECWDOG_COUNT_MASK, wdt);
+}
+
+static int bcm_kona_wdt_start(struct watchdog_device *wdog)
+{
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
+	uint32_t val;
+	int timeout;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
+	if (!timeout) {
+		val &= ~SECWDOG_COUNT_MASK;
+		val |= SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK |
+			SECS_TO_TICKS(wdog->timeout, wdt);
+		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
+	} else {
+		ret = -EAGAIN;
+	}
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	if (!timeout)
+		dev_info(wdog->dev, "Watchdog timer started");
+
+	return ret;
+}
+
+static int bcm_kona_wdt_stop(struct watchdog_device *wdog)
+{
+	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
+	uint32_t val;
+	int timeout, timeleft;
+	unsigned long flags;
+	int ret = 0;
+
+	timeleft = bcm_kona_wdt_get_timeleft(wdog);
+	if (timeleft < 0)
+		return ret;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
+	if (!timeout) {
+		val &= ~(SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK |
+			SECWDOG_COUNT_MASK);
+		val |= SECS_TO_TICKS(timeleft, wdt);
+		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
+	} else {
+		ret = -EAGAIN;
+	}
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	if (!timeout)
+		dev_info(wdog->dev, "Watchdog timer stopped");
+
+	return ret;
+}
+
+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);
+	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_info(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_info(&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");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
1.7.9.5

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

[Guenter Roeck]

On Fri, Nov 08, 2013 at 12:44:47PM -0800, 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>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>

Overall I am a bit concerned about the heavy use of -EAGAIN, which is a first
for watchdog drivers. The condition seems to be around secure_register_read.
If that is stuck for some reason, it may cause endless retries. Not sure if
that is a good idea. -ETIMEDOUT might be a betetr idea.

> ---
>  drivers/watchdog/Kconfig        |   21 +++
>  drivers/watchdog/Makefile       |    1 +
>  drivers/watchdog/bcm_kona_wdt.c |  399 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 421 insertions(+)
>  create mode 100644 drivers/watchdog/bcm_kona_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index d1d53f3..59013f6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1121,6 +1121,27 @@ 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.
> +
It is customary to state the module name if drivers are built as module.

> +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..c47ac77
> --- /dev/null
> +++ b/drivers/watchdog/bcm_kona_wdt.c
> @@ -0,0 +1,399 @@
> +/*
> + * 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/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_MASK	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			10000
> +
> +#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;
> +	int resolution;
> +	spinlock_t lock;
> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +	struct dentry *debugfs;
> +#endif
> +};
> +
> +static uint32_t secure_register_read(void __iomem *addr, int *timeout)
> +{
>From the context, it appears that timeout is really a boolean and does not
reflect a count (as one might think). I would suggest to make it a boolean.

> +	uint32_t val;
> +	unsigned count = 0;
> +
> +	do {
> +		val = readl_relaxed(addr);
> +		count++;
> +	} while ((val & SECWDOG_WD_LOAD_FLAG_MASK) != 0 &&

The "!= 0" is really unnecessary here.

Also, wonder if there should be a specific delay in the loop. Looping without
delay means this will time out faster on faster machines, which may cause
unnecessary failures at some point.

Overall I am a bit concerned that this is always called with interrupts
disabled. Looping up to 10,000 times with interrupts disabled is a lot.
Is this really necessary, or would a mutex be good enough ?

> +		count < SECWDOG_MAX_TRY);
> +	if (timeout)
> +		*timeout = ((val & SECWDOG_WD_LOAD_FLAG_MASK) != 0);
> +
> +	/* We always mask out reserved bits before returning the value. */
> +	val &= SECWDOG_RESERVED_MASK;
> +

You might consider defining the function as int32_t or as int, and return a
negative error value (-EBUSY or -ETIMEDOUT). That would simplyfy the code a bit.

> +	return val;
> +}
> +
> +
Single empty line is sufficient.

> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
> +
> +static int bcm_kona_wdt_dbg_show(struct seq_file *s, void *data)
> +{
> +	uint32_t ctl_val, cur_val;
> +	int ret, ctl_timeout, cur_timeout;
> +	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,
> +				&ctl_timeout);
> +	cur_val = secure_register_read(wdt->base + SECWDOG_COUNT_REG,
> +				&cur_timeout);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	if (ctl_timeout || cur_timeout) {
> +		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", res,
> +				wdt->resolution, ctl_sec, ctl, ctl, cur_sec,
> +				cur, cur);
> +	}
> +
> +	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 (!dir)
> +		return NULL;

debugfs_create_dir() returns an ERR_PTR in error cases.
That probably won't matter if DEBUG_FS is not defined, but I don't think
debugfs_create_file will like it if there is a genuine error.

> +
> +	if (!debugfs_create_file("info", S_IFREG | S_IRUGO, dir, wdt,
> +				&bcm_kona_dbg_operations))
> +		goto out_err;
> +
> +	return dir;
> +
> +out_err:
> +	debugfs_remove_recursive(dir);
> +	return NULL;
> +}
> +
> +static void bcm_kona_debugfs_exit(struct dentry *debugfs)
> +{
> +	debugfs_remove_recursive(debugfs);
> +}
> +
> +#endif /* CONFIG_BCM_KONA_WDT_DEBUG */
> +
> +

Single empty line should be sufficient.

> +static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
> +{
> +	uint32_t val;
> +	int timeout;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (wdt->resolution > SECWDOG_MAX_RES)
> +		return -EINVAL;
> +
This can never happen; see below.

> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~SECWDOG_RES_MASK;
> +		val |= wdt->resolution << SECWDOG_CLKS_SHIFT;
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;
> +	}
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int bcm_kona_wdt_set_timeout_reg(struct watchdog_device *wdog)
> +{
> +	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
> +	uint32_t val;
> +	int timeout;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~SECWDOG_COUNT_MASK;
> +		val |= SECS_TO_TICKS(wdog->timeout, wdt);
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;
> +	}
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return ret;
> +}
> +
> +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);
> +	uint32_t val;
> +	int timeout;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +	val = secure_register_read(wdt->base + SECWDOG_COUNT_REG, &timeout);
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	if (timeout)
> +		return -EAGAIN;
> +
> +	return TICKS_TO_SECS(val & SECWDOG_COUNT_MASK, wdt);
> +}
> +
> +static int bcm_kona_wdt_start(struct watchdog_device *wdog)
> +{
> +	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
> +	uint32_t val;
> +	int timeout;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~SECWDOG_COUNT_MASK;
> +		val |= SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK |
> +			SECS_TO_TICKS(wdog->timeout, wdt);
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;
> +	}
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	if (!timeout)
> +		dev_info(wdog->dev, "Watchdog timer started");
> +
Since you don't have a ping function, and unless I am missing something, this
message would be displayed for each watchdog ping. That is really not a good
idea.

> +	return ret;
> +}
> +
> +static int bcm_kona_wdt_stop(struct watchdog_device *wdog)
> +{
> +	struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
> +	uint32_t val;
> +	int timeout, timeleft;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	timeleft = bcm_kona_wdt_get_timeleft(wdog);
> +	if (timeleft < 0)
> +		return ret;
> +
Does this really make sense ? There is only an error if reading the
watchdog counter register was unsuccessful. In this case, you just return
w/o actually stopping the watchdog, but you claim success. That seems odd.

> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~(SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK |
> +			SECWDOG_COUNT_MASK);
> +		val |= SECS_TO_TICKS(timeleft, wdt);
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;
> +	}
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	if (!timeout)
> +		dev_info(wdog->dev, "Watchdog timer stopped");
> +
All that noise.

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

ret can be -EAGAIN or -EINVAL. -EINVAL suggests a bad internale error (hopefully
SECWDOG_DEFAULT_RESOLUTION is defined to be smaller than SECWDOG_MAX_RES),
and if it is -EAGAIN there should be no error message.

Actually, bcm_kona_wdt_set_resolution_reg is only called from here, meaning the
error check in the function is really unnecessary.

> +		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);
> +	if (ret) {
> +		dev_err(dev, "Failed set watchdog timeout");

The only error case is -EAGAIN. I don't think there should be an error mesasge
in this case (though I am not sure what the reaction should be).

> +		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_info(dev, "Broadcom Kona Watchdog Timer");
> +
Such messages are in general considered nuisance nowadays. I would suggest to
remove it (or ask Greg KH for advice).

> +	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_info(&pdev->dev, "Watchdog driver disabled");
> +
Even more nuisance.

> +	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");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

This alias is being removed.

> -- 
> 1.7.9.5
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Markus Mayer]

On 11 November 2013 09:34, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Nov 08, 2013 at 12:44:47PM -0800, 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>
>> Reviewed-by: Matt Porter <matt.porter@linaro.org>
>
> Overall I am a bit concerned about the heavy use of -EAGAIN, which is a first
> for watchdog drivers. The condition seems to be around secure_register_read.
> If that is stuck for some reason, it may cause endless retries. Not sure if
> that is a good idea. -ETIMEDOUT might be a betetr idea.
>
>> ---
>>  drivers/watchdog/Kconfig        |   21 +++
>>  drivers/watchdog/Makefile       |    1 +
>>  drivers/watchdog/bcm_kona_wdt.c |  399 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 421 insertions(+)
>>  create mode 100644 drivers/watchdog/bcm_kona_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index d1d53f3..59013f6 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1121,6 +1121,27 @@ 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.
>> +
> It is customary to state the module name if drivers are built as module.
>
>> +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..c47ac77
>> --- /dev/null
>> +++ b/drivers/watchdog/bcm_kona_wdt.c
>> @@ -0,0 +1,399 @@
>> +/*
>> + * 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/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_MASK    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                      10000
>> +
>> +#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;
>> +     int resolution;
>> +     spinlock_t lock;
>> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
>> +     struct dentry *debugfs;
>> +#endif
>> +};
>> +
>> +static uint32_t secure_register_read(void __iomem *addr, int *timeout)
>> +{
> From the context, it appears that timeout is really a boolean and does not
> reflect a count (as one might think). I would suggest to make it a boolean.
>
>> +     uint32_t val;
>> +     unsigned count = 0;
>> +
>> +     do {
>> +             val = readl_relaxed(addr);
>> +             count++;
>> +     } while ((val & SECWDOG_WD_LOAD_FLAG_MASK) != 0 &&
>
> The "!= 0" is really unnecessary here.
>
> Also, wonder if there should be a specific delay in the loop. Looping without
> delay means this will time out faster on faster machines, which may cause
> unnecessary failures at some point.
>
> Overall I am a bit concerned that this is always called with interrupts
> disabled. Looping up to 10,000 times with interrupts disabled is a lot.
> Is this really necessary, or would a mutex be good enough ?
>
>> +             count < SECWDOG_MAX_TRY);
>> +     if (timeout)
>> +             *timeout = ((val & SECWDOG_WD_LOAD_FLAG_MASK) != 0);
>> +
>> +     /* We always mask out reserved bits before returning the value. */
>> +     val &= SECWDOG_RESERVED_MASK;
>> +
>
> You might consider defining the function as int32_t or as int, and return a
> negative error value (-EBUSY or -ETIMEDOUT). That would simplyfy the code a bit.
>
>> +     return val;
>> +}
>> +
>> +
> Single empty line is sufficient.
>
>> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
>> +
>> +static int bcm_kona_wdt_dbg_show(struct seq_file *s, void *data)
>> +{
>> +     uint32_t ctl_val, cur_val;
>> +     int ret, ctl_timeout, cur_timeout;
>> +     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,
>> +                             &ctl_timeout);
>> +     cur_val = secure_register_read(wdt->base + SECWDOG_COUNT_REG,
>> +                             &cur_timeout);
>> +     spin_unlock_irqrestore(&wdt->lock, flags);
>> +
>> +     if (ctl_timeout || cur_timeout) {
>> +             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", res,
>> +                             wdt->resolution, ctl_sec, ctl, ctl, cur_sec,
>> +                             cur, cur);
>> +     }
>> +
>> +     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 (!dir)
>> +             return NULL;
>
> debugfs_create_dir() returns an ERR_PTR in error cases.
> That probably won't matter if DEBUG_FS is not defined, but I don't think
> debugfs_create_file will like it if there is a genuine error.
>
>> +
>> +     if (!debugfs_create_file("info", S_IFREG | S_IRUGO, dir, wdt,
>> +                             &bcm_kona_dbg_operations))
>> +             goto out_err;
>> +
>> +     return dir;
>> +
>> +out_err:
>> +     debugfs_remove_recursive(dir);
>> +     return NULL;
>> +}
>> +
>> +static void bcm_kona_debugfs_exit(struct dentry *debugfs)
>> +{
>> +     debugfs_remove_recursive(debugfs);
>> +}
>> +
>> +#endif /* CONFIG_BCM_KONA_WDT_DEBUG */
>> +
>> +
>
> Single empty line should be sufficient.
>
>> +static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
>> +{
>> +     uint32_t val;
>> +     int timeout;
>> +     unsigned long flags;
>> +     int ret = 0;
>> +
>> +     if (wdt->resolution > SECWDOG_MAX_RES)
>> +             return -EINVAL;
>> +
> This can never happen; see below.

Right now it can't, so I can remove it for now. However, I am
considering a proposal for making the resolution available to be set
by userland. So this error case could happen then. That will be a
separate RFC / patch series, though.

>> +     spin_lock_irqsave(&wdt->lock, flags);
>> +
>> +     val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
>> +     if (!timeout) {
>> +             val &= ~SECWDOG_RES_MASK;
>> +             val |= wdt->resolution << SECWDOG_CLKS_SHIFT;
>> +             writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
>> +     } else {
>> +             ret = -EAGAIN;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&wdt->lock, flags);
>> +
>> +     return ret;
>> +}
>> +
>> +static int bcm_kona_wdt_set_timeout_reg(struct watchdog_device *wdog)
>> +{
>> +     struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
>> +     uint32_t val;
>> +     int timeout;
>> +     unsigned long flags;
>> +     int ret = 0;
>> +
>> +     spin_lock_irqsave(&wdt->lock, flags);
>> +
>> +     val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
>> +     if (!timeout) {
>> +             val &= ~SECWDOG_COUNT_MASK;
>> +             val |= SECS_TO_TICKS(wdog->timeout, wdt);
>> +             writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
>> +     } else {
>> +             ret = -EAGAIN;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&wdt->lock, flags);
>> +
>> +     return ret;
>> +}
>> +
>> +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);
>> +     uint32_t val;
>> +     int timeout;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&wdt->lock, flags);
>> +     val = secure_register_read(wdt->base + SECWDOG_COUNT_REG, &timeout);
>> +     spin_unlock_irqrestore(&wdt->lock, flags);
>> +
>> +     if (timeout)
>> +             return -EAGAIN;
>> +
>> +     return TICKS_TO_SECS(val & SECWDOG_COUNT_MASK, wdt);
>> +}
>> +
>> +static int bcm_kona_wdt_start(struct watchdog_device *wdog)
>> +{
>> +     struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
>> +     uint32_t val;
>> +     int timeout;
>> +     unsigned long flags;
>> +     int ret = 0;
>> +
>> +     spin_lock_irqsave(&wdt->lock, flags);
>> +
>> +     val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
>> +     if (!timeout) {
>> +             val &= ~SECWDOG_COUNT_MASK;
>> +             val |= SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK |
>> +                     SECS_TO_TICKS(wdog->timeout, wdt);
>> +             writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
>> +     } else {
>> +             ret = -EAGAIN;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&wdt->lock, flags);
>> +
>> +     if (!timeout)
>> +             dev_info(wdog->dev, "Watchdog timer started");
>> +
> Since you don't have a ping function, and unless I am missing something, this
> message would be displayed for each watchdog ping. That is really not a good
> idea.

You are right. I added this when I did experiment with a separate ping
function. There wasn't much benefit to having it, so I took it back
out.

>> +     return ret;
>> +}
>> +
>> +static int bcm_kona_wdt_stop(struct watchdog_device *wdog)
>> +{
>> +     struct bcm_kona_wdt *wdt = watchdog_get_drvdata(wdog);
>> +     uint32_t val;
>> +     int timeout, timeleft;
>> +     unsigned long flags;
>> +     int ret = 0;
>> +
>> +     timeleft = bcm_kona_wdt_get_timeleft(wdog);
>> +     if (timeleft < 0)
>> +             return ret;
>> +
> Does this really make sense ? There is only an error if reading the
> watchdog counter register was unsuccessful. In this case, you just return
> w/o actually stopping the watchdog, but you claim success. That seems odd.

No. It doesn't make sense. It's also left-over from my experiments
with a separate ping function.

>> +     spin_lock_irqsave(&wdt->lock, flags);
>> +
>> +     val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
>> +     if (!timeout) {
>> +             val &= ~(SECWDOG_EN_MASK | SECWDOG_SRSTEN_MASK |
>> +                     SECWDOG_COUNT_MASK);
>> +             val |= SECS_TO_TICKS(timeleft, wdt);
>> +             writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
>> +     } else {
>> +             ret = -EAGAIN;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&wdt->lock, flags);
>> +
>> +     if (!timeout)
>> +             dev_info(wdog->dev, "Watchdog timer stopped");
>> +
> All that noise.

Would it be acceptable to turn these calls into dev_dbg() calls, here
and elsewhere?

>> +     return ret;
>> +}
>> +
>> +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);
>
> ret can be -EAGAIN or -EINVAL. -EINVAL suggests a bad internale error (hopefully
> SECWDOG_DEFAULT_RESOLUTION is defined to be smaller than SECWDOG_MAX_RES),
> and if it is -EAGAIN there should be no error message.
>
> Actually, bcm_kona_wdt_set_resolution_reg is only called from here, meaning the
> error check in the function is really unnecessary.

This again goes back to making resolution available to userland. Then
bcm_kona_wdt_set_resolution_reg() would be called from elsewhere. Why
is it bad to print an error message on timeout? Would this still apply
if I switch the code to -ETIMEDOUT?

>> +             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);
>> +     if (ret) {
>> +             dev_err(dev, "Failed set watchdog timeout");
>
> The only error case is -EAGAIN. I don't think there should be an error mesasge
> in this case (though I am not sure what the reaction should be).

I am thinking that probe() needs to return an error if setting the
timeout fails, as it can't really rely on the watchdog timer or let
the system use it. Shouldn't that be accompanied by an error message
letting the user know what happened?

>> +             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_info(dev, "Broadcom Kona Watchdog Timer");
>> +
> Such messages are in general considered nuisance nowadays. I would suggest to
> remove it (or ask Greg KH for advice).
>
>> +     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_info(&pdev->dev, "Watchdog driver disabled");
>> +
> Even more nuisance.
>
>> +     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");
>> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
>
> This alias is being removed.
>
>> --
>> 1.7.9.5

Thanks,
-Markus

-- 
Markus Mayer
Broadcom Landing Team

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

[Guenter Roeck]

On 11/12/2013 02:17 PM, Markus Mayer wrote:

>>> +
>>> +     if (!timeout)
>>> +             dev_info(wdog->dev, "Watchdog timer stopped");
>>> +
>> All that noise.
>
> Would it be acceptable to turn these calls into dev_dbg() calls, here
> and elsewhere?
>

Ok with me.


>>> +
>>> +     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);
>>
>> ret can be -EAGAIN or -EINVAL. -EINVAL suggests a bad internale error (hopefully
>> SECWDOG_DEFAULT_RESOLUTION is defined to be smaller than SECWDOG_MAX_RES),
>> and if it is -EAGAIN there should be no error message.
>>
>> Actually, bcm_kona_wdt_set_resolution_reg is only called from here, meaning the
>> error check in the function is really unnecessary.
>
> This again goes back to making resolution available to userland. Then
> bcm_kona_wdt_set_resolution_reg() would be called from elsewhere. Why
> is it bad to print an error message on timeout? Would this still apply

That was related to -EAGAIN. Which would be bad here anyway as it could result
in an endless loop if there is a problem with the chip.

> if I switch the code to -ETIMEDOUT?
>
That is one option, or -EIO if the condition indicates a chip error.

>>> +             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);
>>> +     if (ret) {
>>> +             dev_err(dev, "Failed set watchdog timeout");
>>
>> The only error case is -EAGAIN. I don't think there should be an error mesasge
>> in this case (though I am not sure what the reaction should be).
>
> I am thinking that probe() needs to return an error if setting the
> timeout fails, as it can't really rely on the watchdog timer or let
> the system use it. Shouldn't that be accompanied by an error message
> letting the user know what happened?
>
Oh, I agree it should return an error, and an error message is ok as well.
I am just sure it should not be -EAGAIN, but I don't know what it should be.
Maybe -ETIMEDOUT, or -EIO.

>>> +             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_info(dev, "Broadcom Kona Watchdog Timer");
>>> +
>> Such messages are in general considered nuisance nowadays. I would suggest to
>> remove it (or ask Greg KH for advice).
>>

Referring to your other mail.... seems those messages are falling out of favor.
I consider it a nuisance, though so far I let it go through. The messages do
increase boot time, especially on systems with slow serial console, and IMO
do not provide any real value. Users either don't care, or can check if the
driver is loaded by other means. I would suggest to at least make it dev_dbg.

Thanks,
Guenter

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

[One Thousand Gnomes]

> +static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
> +{
> +	uint32_t val;
> +	int timeout;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (wdt->resolution > SECWDOG_MAX_RES)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~SECWDOG_RES_MASK;
> +		val |= wdt->resolution << SECWDOG_CLKS_SHIFT;
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;

This is I think the wrong choice of return. If the register fails to read
then presumably the device is b*ggered ? In which case return something
like -EIO and log something nasty.

EAGAIN has fairly specific semantics around signals and/or specific
requests for an I/O operation not to wait.

> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
> +	if (!timeout) {
> +		val &= ~SECWDOG_COUNT_MASK;
> +		val |= SECS_TO_TICKS(wdog->timeout, wdt);
> +		writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
> +	} else {
> +		ret = -EAGAIN;
> +	}
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);

As an aside you could fold most of these functions into one single helper
method that read CTRL_REG, did the and and or and wrote the result back
rather than repeating yourself. But hey if you like typing...

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

What if there is no resource present ?

> +	dev_info(dev, "Broadcom Kona Watchdog Timer");

The module load succeeded - the user knows it loaded from that. Likewise
the unload spewage is unwanted and the kernel would just drown in such
messages if we didn't keep them in check. If you need the for debug then
mark them dev_dbg but otherwise bin them.

Alan

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

[Markus Mayer]

On 12 November 2013 15:39, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
>
>> +static int bcm_kona_wdt_set_resolution_reg(struct bcm_kona_wdt *wdt)
>> +{
>> +     uint32_t val;
>> +     int timeout;
>> +     unsigned long flags;
>> +     int ret = 0;
>> +
>> +     if (wdt->resolution > SECWDOG_MAX_RES)
>> +             return -EINVAL;
>> +
>> +     spin_lock_irqsave(&wdt->lock, flags);
>> +
>> +     val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
>> +     if (!timeout) {
>> +             val &= ~SECWDOG_RES_MASK;
>> +             val |= wdt->resolution << SECWDOG_CLKS_SHIFT;
>> +             writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
>> +     } else {
>> +             ret = -EAGAIN;
>
> This is I think the wrong choice of return. If the register fails to read
> then presumably the device is b*ggered ? In which case return something
> like -EIO and log something nasty.
>
> EAGAIN has fairly specific semantics around signals and/or specific
> requests for an I/O operation not to wait.

I will change that based on Guenter's and your comments.

>> +     spin_lock_irqsave(&wdt->lock, flags);
>> +
>> +     val = secure_register_read(wdt->base + SECWDOG_CTRL_REG, &timeout);
>> +     if (!timeout) {
>> +             val &= ~SECWDOG_COUNT_MASK;
>> +             val |= SECS_TO_TICKS(wdog->timeout, wdt);
>> +             writel_relaxed(val, wdt->base + SECWDOG_CTRL_REG);
>> +     } else {
>> +             ret = -EAGAIN;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&wdt->lock, flags);
>
> As an aside you could fold most of these functions into one single helper
> method that read CTRL_REG, did the and and or and wrote the result back
> rather than repeating yourself. But hey if you like typing...

I'll look into that.

>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> What if there is no resource present ?

Then devm_ioremap_resource() will print an error message and return
with ERR_PTR(-EINVAL). Subsequently probe() will fail. So it'll work
as intended.

>> +     dev_info(dev, "Broadcom Kona Watchdog Timer");
>
> The module load succeeded - the user knows it loaded from that. Likewise
> the unload spewage is unwanted and the kernel would just drown in such
> messages if we didn't keep them in check. If you need the for debug then
> mark them dev_dbg but otherwise bin them.

I already changed them all to "debug" in my local tree. For this one
message, I am feeling a bit ambivalent, though. The user only knows
loading of the module succeeded if the user manually loaded the module
-- which, I imagine, would almost never be the case. The driver is
either going to be built in or loaded automatically by some facility.
In which case it might be nice to have some confirmation of that fact
in the logs.

Regards,
-Markus

-- 
Markus Mayer
Broadcom Landing Team

[PATCH 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