[PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[Lubomir Rintel]

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-watchdog@vger.kernel.org
---
 arch/arm/configs/bcm2835_defconfig |    4 +
 drivers/watchdog/Kconfig           |   11 +++
 drivers/watchdog/Makefile          |    1 +
 drivers/watchdog/bcm2835_wdt.c     |  158 ++++++++++++++++++++++++++++++++++++
 4 files changed, 174 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/bcm2835_wdt.c

diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
index 611bda2..8f1547a 100644
--- a/arch/arm/configs/bcm2835_defconfig
+++ b/arch/arm/configs/bcm2835_defconfig
@@ -67,6 +67,10 @@ CONFIG_I2C_BCM2835=y
 CONFIG_GPIO_SYSFS=y
 # CONFIG_HWMON is not set
 # CONFIG_USB_SUPPORT is not set
+CONFIG_WATCHDOG=y
+CONFIG_WATCHDOG_CORE=y
+CONFIG_BCM2835_WDT=y
+
 CONFIG_MMC=y
 CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_PLTFM=y
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9fcc70c..f4e4a8e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1098,6 +1098,17 @@ config BCM63XX_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called bcm63xx_wdt.
 
+config BCM2835_WDT
+	tristate "Broadcom BCM2835 hardware watchdog"
+	depends on ARCH_BCM2835
+	select WATCHDOG_CORE
+	help
+	  Watchdog driver for the built in watchdog hardware in Broadcom
+	  BCM2835 SoC.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called bcm2835_wdt.
+
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..1bf5675 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 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
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 0000000..834f201
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,158 @@
+/*
+ *  Watchdog driver for Broadcom BCM2835
+ *
+ *  Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/miscdevice.h>
+
+#define PM_RSTC				0x1c
+#define PM_RSTS				0x20
+#define PM_WDOG				0x24
+
+#define PM_PASSWORD			0x5a000000
+#define PM_RSTC_WRCFG_MASK		0x00000030
+#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
+#define PM_RSTS_HADWRH_SET		0x00000040
+
+#define PM_WDOG_TIME_SET		0x000fffff
+#define PM_RSTC_WRCFG_CLR		0xffffffcf
+#define PM_RSTC_WRCFG_SET		0x00000030
+#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
+#define PM_RSTC_RESET			0x00000102
+
+#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
+#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
+
+static int heartbeat = -1;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+static void __iomem *wdt_regs;
+static DEFINE_SPINLOCK(wdog_lock);
+
+static int bcm2835_wdt_start(struct watchdog_device *wdog)
+{
+	uint32_t cur;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdog_lock, flags);
+
+	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
+				PM_WDOG_TIME_SET), wdt_regs + PM_WDOG);
+	cur = readl_relaxed(wdt_regs + PM_RSTC);
+	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+		  PM_RSTC_WRCFG_FULL_RESET, wdt_regs + PM_RSTC);
+
+	spin_unlock_irqrestore(&wdog_lock, flags);
+
+	return 0;
+}
+
+static int bcm2835_wdt_stop(struct watchdog_device *wdog)
+{
+	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt_regs + PM_RSTC);
+	dev_info(wdog->dev, "Watchdog timer stopped");
+	return 0;
+}
+
+static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
+{
+	wdog->timeout = t;
+	return 0;
+}
+
+static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
+{
+	uint32_t ret = readl_relaxed(wdt_regs + PM_WDOG);
+	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
+}
+
+static struct watchdog_ops bcm2835_wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	bcm2835_wdt_start,
+	.stop =		bcm2835_wdt_stop,
+	.set_timeout =	bcm2835_wdt_set_timeout,
+	.get_timeleft =	bcm2835_wdt_get_timeleft,
+};
+
+static struct watchdog_info bcm2835_wdt_info = {
+	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+	.identity =	"Broacom BCM2835 Watchdog timer",
+};
+
+static struct watchdog_device bcm2835_wdt_wdd = {
+	.info =		&bcm2835_wdt_info,
+	.ops =		&bcm2835_wdt_ops,
+	.min_timeout =	1,
+	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+};
+
+static int bcm2835_wdt_probe(struct platform_device *op)
+{
+	struct device *dev = &op->dev;
+	struct device_node *np = dev->of_node;
+
+	wdt_regs = of_iomap(np, 0);
+	if (WARN(!wdt_regs, "failed to remap watchdog regs"))
+		return -ENODEV;
+	dev_info(dev, "Broadcom BCM2835 watchdog timer");
+
+	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
+	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
+	return watchdog_register_device(&bcm2835_wdt_wdd);
+}
+
+static int bcm2835_wdt_remove(struct platform_device *op)
+{
+	watchdog_unregister_device(&bcm2835_wdt_wdd);
+	iounmap(wdt_regs);
+
+	return 0;
+}
+
+static void bcm2835_wdt_shutdown(struct platform_device *op)
+{
+	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
+}
+
+static const struct of_device_id bcm2835_wdt_of_match[] = {
+	{ .compatible = "brcm,bcm2835-pm-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
+
+static struct platform_driver bcm2835_wdt_driver = {
+	.probe		= bcm2835_wdt_probe,
+	.remove		= bcm2835_wdt_remove,
+	.shutdown	= bcm2835_wdt_shutdown,
+	.driver = {
+		.name =		"bcm2835-wdt",
+		.owner =	THIS_MODULE,
+		.of_match_table = bcm2835_wdt_of_match,
+	},
+};
+
+module_platform_driver(bcm2835_wdt_driver);
+
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
1.7.1

Re: watchdog: Add Broadcom BCM2708 watchdog timer driver

[Guenter Roeck]

On Fri, Mar 22, 2013 at 12:55:07PM -0000, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-watchdog@vger.kernel.org
> 
> ---
> arch/arm/configs/bcm2835_defconfig |    4 +
>  drivers/watchdog/Kconfig           |   11 +++
>  drivers/watchdog/Makefile          |    1 +
>  drivers/watchdog/bcm2835_wdt.c     |  158 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 174 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/watchdog/bcm2835_wdt.c
> 
> diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
> index 611bda2..8f1547a 100644
> --- a/arch/arm/configs/bcm2835_defconfig
> +++ b/arch/arm/configs/bcm2835_defconfig
> @@ -67,6 +67,10 @@ CONFIG_I2C_BCM2835=y
>  CONFIG_GPIO_SYSFS=y
>  # CONFIG_HWMON is not set
>  # CONFIG_USB_SUPPORT is not set
> +CONFIG_WATCHDOG=y
> +CONFIG_WATCHDOG_CORE=y
> +CONFIG_BCM2835_WDT=y
> +
>  CONFIG_MMC=y
>  CONFIG_MMC_SDHCI=y
>  CONFIG_MMC_SDHCI_PLTFM=y
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 9fcc70c..f4e4a8e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1098,6 +1098,17 @@ config BCM63XX_WDT
>  	  To compile this driver as a loadable module, choose M here.
>  	  The module will be called bcm63xx_wdt.
>  
> +config BCM2835_WDT
> +	tristate "Broadcom BCM2835 hardware watchdog"
> +	depends on ARCH_BCM2835
> +	select WATCHDOG_CORE
> +	help
> +	  Watchdog driver for the built in watchdog hardware in Broadcom
> +	  BCM2835 SoC.
> +
> +	  To compile this driver as a loadable module, choose M here.
> +	  The module will be called bcm2835_wdt.
> +
>  config LANTIQ_WDT
>  	tristate "Lantiq SoC watchdog"
>  	depends on LANTIQ
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a300b94..1bf5675 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>  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
>  
>  # AVR32 Architecture
>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> new file mode 100644
> index 0000000..834f201
> --- /dev/null
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -0,0 +1,158 @@
> +/*
> + *  Watchdog driver for Broadcom BCM2835
> + *
> + *  Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
> + *
> + *  This program is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU General Public License
> + *  as published by the Free Software Foundation; either version
> + *  2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/miscdevice.h>
> +
> +#define PM_RSTC				0x1c
> +#define PM_RSTS				0x20
> +#define PM_WDOG				0x24
> +
> +#define PM_PASSWORD			0x5a000000
> +#define PM_RSTC_WRCFG_MASK		0x00000030
> +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> +#define PM_RSTS_HADWRH_SET		0x00000040
> +
> +#define PM_WDOG_TIME_SET		0x000fffff
> +#define PM_RSTC_WRCFG_CLR		0xffffffcf
> +#define PM_RSTC_WRCFG_SET		0x00000030
> +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> +#define PM_RSTC_RESET			0x00000102
> +
> +#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
> +#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
> +
> +static int heartbeat = -1;
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static void __iomem *wdt_regs;
> +static DEFINE_SPINLOCK(wdog_lock);
> +
> +static int bcm2835_wdt_start(struct watchdog_device *wdog)
> +{
> +	uint32_t cur;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdog_lock, flags);
> +
> +	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
> +				PM_WDOG_TIME_SET), wdt_regs + PM_WDOG);
> +	cur = readl_relaxed(wdt_regs + PM_RSTC);
> +	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
> +		  PM_RSTC_WRCFG_FULL_RESET, wdt_regs + PM_RSTC);
> +
Nitpick - I prefer people to use the recommended continuation line style,
but that is really up to the maintainer to decide.

> +	spin_unlock_irqrestore(&wdog_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_stop(struct watchdog_device *wdog)
> +{
> +	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt_regs + PM_RSTC);
> +	dev_info(wdog->dev, "Watchdog timer stopped");
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> +{
> +	wdog->timeout = t;

No need to update the actual chip timeout ?

> +	return 0;
> +}
> +
> +static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
> +{
> +	uint32_t ret = readl_relaxed(wdt_regs + PM_WDOG);
> +	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
> +}
> +
> +static struct watchdog_ops bcm2835_wdt_ops = {
> +	.owner =	THIS_MODULE,
> +	.start =	bcm2835_wdt_start,
> +	.stop =		bcm2835_wdt_stop,
> +	.set_timeout =	bcm2835_wdt_set_timeout,
> +	.get_timeleft =	bcm2835_wdt_get_timeleft,

No separate ping function ?

> +};
> +
> +static struct watchdog_info bcm2835_wdt_info = {
> +	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +			WDIOF_KEEPALIVEPING,
> +	.identity =	"Broacom BCM2835 Watchdog timer",
> +};
> +
> +static struct watchdog_device bcm2835_wdt_wdd = {
> +	.info =		&bcm2835_wdt_info,
> +	.ops =		&bcm2835_wdt_ops,
> +	.min_timeout =	1,
> +	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
> +};
> +
> +static int bcm2835_wdt_probe(struct platform_device *op)
> +{
> +	struct device *dev = &op->dev;
> +	struct device_node *np = dev->of_node;
> +
> +	wdt_regs = of_iomap(np, 0);
> +	if (WARN(!wdt_regs, "failed to remap watchdog regs"))
> +		return -ENODEV;

WARN seems to be a bit extreme. Is this necessary ?

> +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> +
> +	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);

Since heartbeat is by default set to -1, which is interpreted as unsigned
int, I would expect this call to return -EINVAL, leaving the default timeout
undefined. Is this really what you want ?

> +	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
> +	return watchdog_register_device(&bcm2835_wdt_wdd);

Leaking iomap if this fails.

Would be nice to have something like devm_of_iomap ...

> +}
> +
> +static int bcm2835_wdt_remove(struct platform_device *op)
> +{
> +	watchdog_unregister_device(&bcm2835_wdt_wdd);
> +	iounmap(wdt_regs);
> +
> +	return 0;
> +}
> +
> +static void bcm2835_wdt_shutdown(struct platform_device *op)
> +{
> +	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
> +}
> +
> +static const struct of_device_id bcm2835_wdt_of_match[] = {
> +	{ .compatible = "brcm,bcm2835-pm-wdt", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
> +
> +static struct platform_driver bcm2835_wdt_driver = {
> +	.probe		= bcm2835_wdt_probe,
> +	.remove		= bcm2835_wdt_remove,
> +	.shutdown	= bcm2835_wdt_shutdown,
> +	.driver = {
> +		.name =		"bcm2835-wdt",
> +		.owner =	THIS_MODULE,
> +		.of_match_table = bcm2835_wdt_of_match,
> +	},
> +};
> +
> +module_platform_driver(bcm2835_wdt_driver);
> +
> +module_param(heartbeat, int, 0);
> +MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
> +
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
> +MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[Arend van Spriel]

On 03/22/2013 01:55 PM, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-watchdog@vger.kernel.org
> ---
>  arch/arm/configs/bcm2835_defconfig |    4 +
>  drivers/watchdog/Kconfig           |   11 +++
>  drivers/watchdog/Makefile          |    1 +
>  drivers/watchdog/bcm2835_wdt.c     |  158 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 174 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/watchdog/bcm2835_wdt.c

Seems like the subject line of the patch is wrong. Otherwise the commit
message should be a little, tiny bit longer explainer how bcm2708 is
related to bcm2835.

Regards,
Arend

Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[Stephen Warren]

On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

A commit description would be useful.

>  arch/arm/configs/bcm2835_defconfig |    4 +
>  drivers/watchdog/Kconfig           |   11 +++
>  drivers/watchdog/Makefile          |    1 +
>  drivers/watchdog/bcm2835_wdt.c     |  158 ++++++++++++++++++++++++++++++++++++

The changes to bcm2835_defconfig should be a separate patch, since they
would be applied in the BCM2835 ARM sub-arch tree, whereas the driver
patch would be applied to the watchdog driver tree.

> diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig

> +CONFIG_BCM2835_WDT=y
> +
>  CONFIG_MMC=y

That blank line is a little odd; was this defconfig change created using
"make savedefconfig"?

> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c

> +static int heartbeat = -1;
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static void __iomem *wdt_regs;
> +static DEFINE_SPINLOCK(wdog_lock);

Can these be stored in a dynamically-allocated structure, stored in the
device's drvdata?

> +static struct platform_driver bcm2835_wdt_driver = {
...
> +};
> +
> +module_platform_driver(bcm2835_wdt_driver);

I believe it's typical not to leave a blank line before
module_platform_driver();

A couple of general comments:

1)

This driver touches the same registers that
arch/arm/mach-bcm2835/bcm2835.c uses to implement reboot and "power
off". Some co-ordination might be necessary.

The implementation of bcm2835_power_off() could easily be moved into
this driver, to avoid some of the need for co-ordination.

Moving bcm2835_restart() would be more tricky, since the ARM machine
descriptor needs a pointer to that function. I guess the kernel probably
ensures that none of the code in this watchdog driver is running by the
time bcm2835_restart() is called, although perhaps it'd be better to
have mach-bcm2835/bcm2835.c and this driver share a lock?

2)

I'm curious where you got the documentation to write this driver; this
HW module isn't described in BCM2835-ARM-Peripherals.pdf. I assume this
is based on the downstream kernel driver? If so, at least some credit in
the commit description might be appropriate. At least the relevant
commit downstream already has an appropriate Signed-off-by line:-)

Re: watchdog: Add Broadcom BCM2708 watchdog timer driver

[Lubomir Rintel]

On Fri, 2013-03-22 at 06:56 -0700, Guenter Roeck wrote:

Thank you for your response!

On Fri Mar 22 09:56:01 EDT 2013, Guenter Roeck wrote:
> On Fri, Mar 22, 2013 at 12:55:07PM -0000, Lubomir Rintel wrote:
...
> > +	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
> > +		  PM_RSTC_WRCFG_FULL_RESET, wdt_regs + PM_RSTC);
> > +
> Nitpick - I prefer people to use the recommended continuation line style,
> but that is really up to the maintainer to decide.

Well, I intended to comply with Documentation/CodingStyle, are you referring to 
it? I fail to understand what to do to be more compliant and could not really 
identify a style that would be consistently used across the kernel source. 
Should I cut then second line into two smaller parts that would be aligned with 
right line end?

...
> > +static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> > +{
> > +	wdog->timeout = t;
> 
> No need to update the actual chip timeout ?

No need to, watchdog core applies the new timeout by pinging the device (see 
below for what happens when this driver is pinged).

See: WDIOC_SETTIMEOUT in drivers/watchdog/watchdog_dev.c

...
> > +static struct watchdog_ops bcm2835_wdt_ops = {
> > +	.owner =	THIS_MODULE,
> > +	.start =	bcm2835_wdt_start,
> > +	.stop =		bcm2835_wdt_stop,
> > +	.set_timeout =	bcm2835_wdt_set_timeout,
> > +	.get_timeleft =	bcm2835_wdt_get_timeleft,
> 
> No separate ping function ?

The watchdog documentation core states:

  "Most hardware that does not support this as a separate function uses the
  start function to restart the watchdog timer hardware. And that's also what
  the watchdog timer driver core does."

This indeed applies to this driver.

...
> > +	if (WARN(!wdt_regs, "failed to remap watchdog regs"))
> > +		return -ENODEV;
> 
> WARN seems to be a bit extreme. Is this necessary ?

Probably not. I'll replace it with dev_err() instead.

> > +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> > +
> > +	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
> 
> Since heartbeat is by default set to -1, which is interpreted as unsigned
> int, I would expect this call to return -EINVAL, leaving the default timeout
> undefined. Is this really what you want ?

Well, I looked into orion-wdt for an example how to initialize the default 
timeout, but failed to understand it correctly. I thought that watchdog core 
picks a sensible value upon getting -1, which is incorrect. They in fact use 
initialize timeout with maximal value, and use a fall-through vi EINVAL to leave 
it untouched if it was not overridden. I'll do the same thing now.

> > +	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
> > +	return watchdog_register_device(&bcm2835_wdt_wdd);
> 
> Leaking iomap if this fails.

Oops. Fixing.

> Would be nice to have something like devm_of_iomap ...

That sounds sound to me. Sent out a separate patch implementing it, and I'll 
modify this if it gets merged.

-- 
Lubomir Rintel <lkundrak@v3.sk>

Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[Lubomir Rintel]

On Fri, 2013-03-22 at 19:46 +0100, Arend van Spriel wrote:

Thanks for your response!

> On 03/22/2013 01:55 PM, Lubomir Rintel wrote:
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Cc: linux-rpi-kernel@lists.infradead.org
> > Cc: linux-watchdog@vger.kernel.org
> > ---
> >  arch/arm/configs/bcm2835_defconfig |    4 +
> >  drivers/watchdog/Kconfig           |   11 +++
> >  drivers/watchdog/Makefile          |    1 +
> >  drivers/watchdog/bcm2835_wdt.c     |  158 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 174 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/watchdog/bcm2835_wdt.c
> 
> Seems like the subject line of the patch is wrong. Otherwise the commit
> message should be a little, tiny bit longer explainer how bcm2708 is
> related to bcm2835.

Good catch -- it indeed is wrong. It's probably still a good idea to
have a more descriptive commit message. I'll fix that up in next
revision.

-- 
Lubomir Rintel <lkundrak@v3.sk>

Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[Lubomir Rintel]

On Fri, 2013-03-22 at 20:24 -0600, Stephen Warren wrote:

Thank you for your response!

> On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
> > Signed-off-by: Lubomir Rintel <lkundrak at v3.sk>
> 
> A commit description would be useful.

I'll add a more descriptive one in next patch revision.

> >  arch/arm/configs/bcm2835_defconfig |    4 +
> >  drivers/watchdog/Kconfig           |   11 +++
> >  drivers/watchdog/Makefile          |    1 +
> >  drivers/watchdog/bcm2835_wdt.c     |  158 ++++++++++++++++++++++++++++++++++++
> 
> The changes to bcm2835_defconfig should be a separate patch, since they
> would be applied in the BCM2835 ARM sub-arch tree, whereas the driver
> patch would be applied to the watchdog driver tree.

Okay, makes sense to me.

> > diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
> 
> > +CONFIG_BCM2835_WDT=y
> > +
> >  CONFIG_MMC=y
> 
> That blank line is a little odd; was this defconfig change created using
> "make savedefconfig"?

No, I did not notice that the savedefconfig exists and modified the defconfig 
by hand. I'll use it for next patch revision.

> > diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> 
> > +static int heartbeat = -1;
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> > +static void __iomem *wdt_regs;
> > +static DEFINE_SPINLOCK(wdog_lock);
> 
> Can these be stored in a dynamically-allocated structure, stored in the
> device's drvdata?

Well, not hearbeat and nowayout, those are module parameters.

The other ones make perfect sense, I've attempted to use a per-device 
dynamically-allocated structure for those.

> > +static struct platform_driver bcm2835_wdt_driver = {
> ...
> > +};
> > +
> > +module_platform_driver(bcm2835_wdt_driver);
> 
> I believe it's typical not to leave a blank line before
> module_platform_driver();

Okay.

> A couple of general comments:
> 
> 1)
> 
> This driver touches the same registers that
> arch/arm/mach-bcm2835/bcm2835.c uses to implement reboot and "power
> off". Some co-ordination might be necessary.
> 
> The implementation of bcm2835_power_off() could easily be moved into
> this driver, to avoid some of the need for co-ordination.
> 
> Moving bcm2835_restart() would be more tricky, since the ARM machine
> descriptor needs a pointer to that function. I guess the kernel probably
> ensures that none of the code in this watchdog driver is running by the
> time bcm2835_restart() is called, although perhaps it'd be better to
> have mach-bcm2835/bcm2835.c and this driver share a lock?

I need help here, I'm not sure what's the proper way to address this
(whether to include the actual reboot code in the wdt driver or the
platform driver).

Is it okay to have the platform driver depend on watchdog timer?
Is it okay for the platform driver not to reboot properly if the kernel
is running without the wdt driver loaded?

(For now, I'll send a revised patch addressing the other issues so that
it can be reviewed without addressing this yet.)

> 2)
> 
> I'm curious where you got the documentation to write this driver; this
> HW module isn't described in BCM2835-ARM-Peripherals.pdf. I assume this
> is based on the downstream kernel driver? If so, at least some credit in
> the commit description might be appropriate. At least the relevant
> commit downstream already has an appropriate Signed-off-by line:-)

Your guess is right, used bcm2708_wdog driver from rpi-3.6.y as a reference. 
I'll add that information to the commit message.

The Signed-off-by line is indeed present, but unfortunately does not seem to be 
particularly appropriate:

Signed-off-by: popcornmix <popcornmix@gmail.com>

I'm wondering if it's actually relevant, since the useful bits of the 
downstream driver boil down to macro defines.

Have a nice day!

-- 
Lubomir Rintel <lkundrak@v3.sk>

[PATCH v2] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
used in Raspberry Pi and Roku 2 devices.

Interface to the Broadcom BCM2835 watchdog timer hardware is based on
"bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
"rpi-3.6.y" of git://github.com/raspberrypi/linux.git

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-watchdog@vger.kernel.org
---
Changes for v2:
    - Use per-device structure instead of global variables for lock and memory base
    - Fix default timeout setting
    - Do not leak memory if probe fails
    - Style fixes
    - Split out defconfig into a separate commit
    - Meaningful commit message with credit

 .../bindings/watchdog/brcm,bcm2835-pm-wdog.txt     |    5 +
 drivers/watchdog/Kconfig                           |   11 ++
 drivers/watchdog/Makefile                          |    1 +
 drivers/watchdog/bcm2835_wdt.c                     |  189 ++++++++++++++++++++
 4 files changed, 206 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/bcm2835_wdt.c

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
index d209366..f801d71 100644
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
@@ -5,9 +5,14 @@ Required properties:
 - compatible : should be "brcm,bcm2835-pm-wdt"
 - reg : Specifies base physical address and size of the registers.
 
+Optional properties:
+
+- timeout-sec   : Contains the watchdog timeout in seconds
+
 Example:
 
 watchdog {
 	compatible = "brcm,bcm2835-pm-wdt";
 	reg = <0x7e100000 0x28>;
+	timeout-sec = <10>;
 };
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9fcc70c..f4e4a8e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1098,6 +1098,17 @@ config BCM63XX_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called bcm63xx_wdt.
 
+config BCM2835_WDT
+	tristate "Broadcom BCM2835 hardware watchdog"
+	depends on ARCH_BCM2835
+	select WATCHDOG_CORE
+	help
+	  Watchdog driver for the built in watchdog hardware in Broadcom
+	  BCM2835 SoC.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called bcm2835_wdt.
+
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..1bf5675 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 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
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 0000000..3899638
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,189 @@
+/*
+ *  Watchdog driver for Broadcom BCM2835
+ *
+ *  Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/miscdevice.h>
+
+#define PM_RSTC				0x1c
+#define PM_RSTS				0x20
+#define PM_WDOG				0x24
+
+#define PM_PASSWORD			0x5a000000
+#define PM_RSTC_WRCFG_MASK		0x00000030
+#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
+#define PM_RSTS_HADWRH_SET		0x00000040
+
+#define PM_WDOG_TIME_SET		0x000fffff
+#define PM_RSTC_WRCFG_CLR		0xffffffcf
+#define PM_RSTC_WRCFG_SET		0x00000030
+#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
+#define PM_RSTC_RESET			0x00000102
+
+#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
+#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
+
+struct bcm2835_wdt {
+	void __iomem		*base;
+	spinlock_t		lock;
+};
+
+static int heartbeat = -1;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+static int bcm2835_wdt_start(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+	uint32_t cur;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
+				PM_WDOG_TIME_SET), wdt->base + PM_WDOG);
+	cur = readl_relaxed(wdt->base + PM_RSTC);
+	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+		  PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC);
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return 0;
+}
+
+static int bcm2835_wdt_stop(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC);
+	dev_info(wdog->dev, "Watchdog timer stopped");
+	return 0;
+}
+
+static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
+{
+	wdog->timeout = t;
+	return 0;
+}
+
+static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	uint32_t ret = readl_relaxed(wdt->base + PM_WDOG);
+	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
+}
+
+static struct watchdog_ops bcm2835_wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	bcm2835_wdt_start,
+	.stop =		bcm2835_wdt_stop,
+	.set_timeout =	bcm2835_wdt_set_timeout,
+	.get_timeleft =	bcm2835_wdt_get_timeleft,
+};
+
+static struct watchdog_info bcm2835_wdt_info = {
+	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+	.identity =	"Broadcom BCM2835 Watchdog timer",
+};
+
+static struct watchdog_device bcm2835_wdt_wdd = {
+	.info =		&bcm2835_wdt_info,
+	.ops =		&bcm2835_wdt_ops,
+	.min_timeout =	1,
+	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+	.timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+};
+
+static int bcm2835_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct bcm2835_wdt *wdt;
+	int err;
+
+	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(dev, "Failed to allocate memory for watchdog device");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&wdt->lock);
+
+	wdt->base = of_iomap(np, 0);
+	if (!wdt->base) {
+		dev_err(dev, "Failed to remap watchdog regs");
+		return -ENODEV;
+	}
+	dev_info(dev, "Broadcom BCM2835 watchdog timer");
+
+	platform_set_drvdata(pdev, wdt);
+
+	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
+	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
+	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
+	err = watchdog_register_device(&bcm2835_wdt_wdd);
+	if (err) {
+		dev_err(dev, "Failed to register watchdog device");
+		iounmap(wdt->base);
+	}
+	return err;
+}
+
+static int bcm2835_wdt_remove(struct platform_device *pdev)
+{
+	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
+
+	platform_set_drvdata(pdev, NULL);
+	watchdog_unregister_device(&bcm2835_wdt_wdd);
+	iounmap(wdt->base);
+
+	return 0;
+}
+
+static void bcm2835_wdt_shutdown(struct platform_device *pdev)
+{
+	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
+}
+
+static const struct of_device_id bcm2835_wdt_of_match[] = {
+	{ .compatible = "brcm,bcm2835-pm-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
+
+static struct platform_driver bcm2835_wdt_driver = {
+	.probe		= bcm2835_wdt_probe,
+	.remove		= bcm2835_wdt_remove,
+	.shutdown	= bcm2835_wdt_shutdown,
+	.driver = {
+		.name =		"bcm2835-wdt",
+		.owner =	THIS_MODULE,
+		.of_match_table = bcm2835_wdt_of_match,
+	},
+};
+module_platform_driver(bcm2835_wdt_driver);
+
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
1.7.1

[PATCH v2] arm: Add Broadcom BCM2835 watchdog timer driver to bcm2835_defconfig

[Lubomir Rintel]

This enables a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
used in Raspberry Pi and Roku 2 devices.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: linux-rpi-kernel@lists.infradead.org
---
Changes for v2:
    - Split out from the driver changeset
    - Regenerated with savedefconfig

 arch/arm/configs/bcm2835_defconfig |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
index 611bda2..729adf2 100644
--- a/arch/arm/configs/bcm2835_defconfig
+++ b/arch/arm/configs/bcm2835_defconfig
@@ -66,6 +66,8 @@ CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_BCM2835=y
 CONFIG_GPIO_SYSFS=y
 # CONFIG_HWMON is not set
+CONFIG_WATCHDOG=y
+CONFIG_BCM2835_WDT=y
 # CONFIG_USB_SUPPORT is not set
 CONFIG_MMC=y
 CONFIG_MMC_SDHCI=y
-- 
1.7.1

Re: watchdog: Add Broadcom BCM2708 watchdog timer driver

[Guenter Roeck]

On Sun, Mar 24, 2013 at 03:06:59PM +0100, Lubomir Rintel wrote:
> On Fri, 2013-03-22 at 06:56 -0700, Guenter Roeck wrote:
> 
> Thank you for your response!
> 
> On Fri Mar 22 09:56:01 EDT 2013, Guenter Roeck wrote:
> > On Fri, Mar 22, 2013 at 12:55:07PM -0000, Lubomir Rintel wrote:
> ...
> > > +	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
> > > +		  PM_RSTC_WRCFG_FULL_RESET, wdt_regs + PM_RSTC);
> > > +
> > Nitpick - I prefer people to use the recommended continuation line style,
> > but that is really up to the maintainer to decide.
> 
> Well, I intended to comply with Documentation/CodingStyle, are you referring to 
> it? I fail to understand what to do to be more compliant and could not really 
> identify a style that would be consistently used across the kernel source. 
> Should I cut then second line into two smaller parts that would be aligned with 
> right line end?
> 
I was referring to line continuation aligned with '(', such as

	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
		       PM_RSTC_WRCFG_FULL_RESET, wdt_regs + PM_RSTC);

I don't recall how deep the indentation was - does that not fit ?

> ...
> > > +static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> > > +{
> > > +	wdog->timeout = t;
> > 
> > No need to update the actual chip timeout ?
> 
> No need to, watchdog core applies the new timeout by pinging the device (see 
> below for what happens when this driver is pinged).
> 
> See: WDIOC_SETTIMEOUT in drivers/watchdog/watchdog_dev.c
> 
Ok, makes sense.

> ...
> > > +static struct watchdog_ops bcm2835_wdt_ops = {
> > > +	.owner =	THIS_MODULE,
> > > +	.start =	bcm2835_wdt_start,
> > > +	.stop =		bcm2835_wdt_stop,
> > > +	.set_timeout =	bcm2835_wdt_set_timeout,
> > > +	.get_timeleft =	bcm2835_wdt_get_timeleft,
> > 
> > No separate ping function ?
> 
> The watchdog documentation core states:
> 
>   "Most hardware that does not support this as a separate function uses the
>   start function to restart the watchdog timer hardware. And that's also what
>   the watchdog timer driver core does."
> 
> This indeed applies to this driver.
> 
Ok.

> ...
> > > +	if (WARN(!wdt_regs, "failed to remap watchdog regs"))
> > > +		return -ENODEV;
> > 
> > WARN seems to be a bit extreme. Is this necessary ?
> 
> Probably not. I'll replace it with dev_err() instead.
> 
> > > +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> > > +
> > > +	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
> > 
> > Since heartbeat is by default set to -1, which is interpreted as unsigned
> > int, I would expect this call to return -EINVAL, leaving the default timeout
> > undefined. Is this really what you want ?
> 
> Well, I looked into orion-wdt for an example how to initialize the default 
> timeout, but failed to understand it correctly. I thought that watchdog core 
> picks a sensible value upon getting -1, which is incorrect. They in fact use 
> initialize timeout with maximal value, and use a fall-through vi EINVAL to leave 
> it untouched if it was not overridden. I'll do the same thing now.
> 
Some user level documentation states that the default timeout for all drivers
would be 60 seconds. Unfortunately, that is not correct, as all drivers do
what they want. I myself use it as a guideline, ie use a 60 seconds default
unless there is a good reason to use another default value (eg if the
chip only supports a lower maximum).

> > > +	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
> > > +	return watchdog_register_device(&bcm2835_wdt_wdd);
> > 
> > Leaking iomap if this fails.
> 
> Oops. Fixing.
> 
> > Would be nice to have something like devm_of_iomap ...
> 
> That sounds sound to me. Sent out a separate patch implementing it, and I'll 
> modify this if it gets merged.
> 
Excellent!

Thanks,
Guenter

Re: [PATCH v2] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Guenter Roeck]

On Sun, Mar 24, 2013 at 03:22:53PM +0100, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.
> 
> Interface to the Broadcom BCM2835 watchdog timer hardware is based on
> "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
> "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
> 
I would suggest to put that into the coments at the top of the driver.
Also, if the original code has a copyright statement, you might want
to copy that.

> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-watchdog@vger.kernel.org
> ---
> Changes for v2:
>     - Use per-device structure instead of global variables for lock and memory base
>     - Fix default timeout setting
>     - Do not leak memory if probe fails
>     - Style fixes
>     - Split out defconfig into a separate commit
>     - Meaningful commit message with credit
> 
>  .../bindings/watchdog/brcm,bcm2835-pm-wdog.txt     |    5 +
>  drivers/watchdog/Kconfig                           |   11 ++
>  drivers/watchdog/Makefile                          |    1 +
>  drivers/watchdog/bcm2835_wdt.c                     |  189 ++++++++++++++++++++
>  4 files changed, 206 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/watchdog/bcm2835_wdt.c
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> index d209366..f801d71 100644
> --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> @@ -5,9 +5,14 @@ Required properties:
>  - compatible : should be "brcm,bcm2835-pm-wdt"
>  - reg : Specifies base physical address and size of the registers.
>  
> +Optional properties:
> +
> +- timeout-sec   : Contains the watchdog timeout in seconds
> +
>  Example:
>  
>  watchdog {
>  	compatible = "brcm,bcm2835-pm-wdt";
>  	reg = <0x7e100000 0x28>;
> +	timeout-sec = <10>;
>  };
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 9fcc70c..f4e4a8e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1098,6 +1098,17 @@ config BCM63XX_WDT
>  	  To compile this driver as a loadable module, choose M here.
>  	  The module will be called bcm63xx_wdt.
>  
> +config BCM2835_WDT
> +	tristate "Broadcom BCM2835 hardware watchdog"
> +	depends on ARCH_BCM2835
> +	select WATCHDOG_CORE
> +	help
> +	  Watchdog driver for the built in watchdog hardware in Broadcom
> +	  BCM2835 SoC.
> +
> +	  To compile this driver as a loadable module, choose M here.
> +	  The module will be called bcm2835_wdt.
> +
>  config LANTIQ_WDT
>  	tristate "Lantiq SoC watchdog"
>  	depends on LANTIQ
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a300b94..1bf5675 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>  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
>  
>  # AVR32 Architecture
>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> new file mode 100644
> index 0000000..3899638
> --- /dev/null
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -0,0 +1,189 @@
> +/*
> + *  Watchdog driver for Broadcom BCM2835
> + *
> + *  Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
> + *
> + *  This program is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU General Public License
> + *  as published by the Free Software Foundation; either version
> + *  2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/miscdevice.h>

Just noticed - you should not need that include.

> +
> +#define PM_RSTC				0x1c
> +#define PM_RSTS				0x20

Not used

> +#define PM_WDOG				0x24
> +
> +#define PM_PASSWORD			0x5a000000
> +#define PM_RSTC_WRCFG_MASK		0x00000030

Not used

> +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> +#define PM_RSTS_HADWRH_SET		0x00000040

Not used

> +
> +#define PM_WDOG_TIME_SET		0x000fffff
> +#define PM_RSTC_WRCFG_CLR		0xffffffcf
> +#define PM_RSTC_WRCFG_SET		0x00000030

Not used 

> +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020

Defined twice

> +#define PM_RSTC_RESET			0x00000102
> +
> +#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
> +#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
> +
> +struct bcm2835_wdt {
> +	void __iomem		*base;
> +	spinlock_t		lock;
> +};
> +
> +static int heartbeat = -1;

The parameter passed to watchdog_init_timeout is an unsigned int, so you should
really use an unsigned int here. Depending on -1 being converted to maxuint is a
bit of side effect programming. Easier to just set it to 0, which the watchdog
core interprets as invalid as well.

> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +
> +static int bcm2835_wdt_start(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +	uint32_t cur;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
> +				PM_WDOG_TIME_SET), wdt->base + PM_WDOG);
> +	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
> +		  PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC);
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_stop(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC);
> +	dev_info(wdog->dev, "Watchdog timer stopped");
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> +{
> +	wdog->timeout = t;
> +	return 0;
> +}
> +
> +static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	uint32_t ret = readl_relaxed(wdt->base + PM_WDOG);
> +	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
> +}
> +
> +static struct watchdog_ops bcm2835_wdt_ops = {
> +	.owner =	THIS_MODULE,
> +	.start =	bcm2835_wdt_start,
> +	.stop =		bcm2835_wdt_stop,
> +	.set_timeout =	bcm2835_wdt_set_timeout,
> +	.get_timeleft =	bcm2835_wdt_get_timeleft,
> +};
> +
> +static struct watchdog_info bcm2835_wdt_info = {
> +	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +			WDIOF_KEEPALIVEPING,
> +	.identity =	"Broadcom BCM2835 Watchdog timer",
> +};
> +
> +static struct watchdog_device bcm2835_wdt_wdd = {
> +	.info =		&bcm2835_wdt_info,
> +	.ops =		&bcm2835_wdt_ops,
> +	.min_timeout =	1,
> +	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
> +	.timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
> +};
> +
> +static int bcm2835_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct bcm2835_wdt *wdt;
> +	int err;
> +
> +	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(dev, "Failed to allocate memory for watchdog device");
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	wdt->base = of_iomap(np, 0);
> +	if (!wdt->base) {
> +		dev_err(dev, "Failed to remap watchdog regs");
> +		return -ENODEV;
> +	}
> +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> +
I think this should come later, after you are sure that there will be
no failure.

> +	platform_set_drvdata(pdev, wdt);
> +
> +	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
> +	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
> +	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
> +	err = watchdog_register_device(&bcm2835_wdt_wdd);
> +	if (err) {
> +		dev_err(dev, "Failed to register watchdog device");
> +		iounmap(wdt->base);
> +	}
> +	return err;
> +}
> +
> +static int bcm2835_wdt_remove(struct platform_device *pdev)
> +{
> +	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	platform_set_drvdata(pdev, NULL);

It is unnecessary to set this to NULL (the infrastructure does it for you).

> +	watchdog_unregister_device(&bcm2835_wdt_wdd);
> +	iounmap(wdt->base);
> +
> +	return 0;
> +}
> +
> +static void bcm2835_wdt_shutdown(struct platform_device *pdev)
> +{
> +	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
> +}
> +
> +static const struct of_device_id bcm2835_wdt_of_match[] = {
> +	{ .compatible = "brcm,bcm2835-pm-wdt", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
> +
> +static struct platform_driver bcm2835_wdt_driver = {
> +	.probe		= bcm2835_wdt_probe,
> +	.remove		= bcm2835_wdt_remove,
> +	.shutdown	= bcm2835_wdt_shutdown,
> +	.driver = {
> +		.name =		"bcm2835-wdt",
> +		.owner =	THIS_MODULE,
> +		.of_match_table = bcm2835_wdt_of_match,
> +	},
> +};
> +module_platform_driver(bcm2835_wdt_driver);
> +
> +module_param(heartbeat, int, 0);
> +MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
> +
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
> +MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> -- 
> 1.7.1
> 
> --
> 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 v2] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

On Sun, 2013-03-24 at 09:12 -0700, Guenter Roeck wrote:
> On Sun, Mar 24, 2013 at 03:22:53PM +0100, Lubomir Rintel wrote:
> > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > used in Raspberry Pi and Roku 2 devices.
> > 
> > Interface to the Broadcom BCM2835 watchdog timer hardware is based on
> > "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
> > "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
> > 
> I would suggest to put that into the coments at the top of the driver.
> Also, if the original code has a copyright statement, you might want
> to copy that.

Okay.

> > +#include <linux/miscdevice.h>
> 
> Just noticed - you should not need that include.

Well, I think the bottommost line of the driver uses that and it's a
good practice to provide a device number alias. Is that one completely
useless?

  MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

> > +#define PM_RSTS				0x20
> 
> Not used

Dropped.

> > +#define PM_RSTC_WRCFG_MASK		0x00000030
> 
> Not used

Dropped.

> > +#define PM_RSTS_HADWRH_SET		0x00000040
> 
> Not used

Dropped.

> > +#define PM_RSTC_WRCFG_SET		0x00000030
> 
> Not used 

Dropped.

> > +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> 
> Defined twice

Extra occurrence dropped.

> 
> > +#define PM_RSTC_RESET			0x00000102
> > +
> > +#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
> > +#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
> > +
> > +struct bcm2835_wdt {
> > +	void __iomem		*base;
> > +	spinlock_t		lock;
> > +};
> > +
> > +static int heartbeat = -1;
> 
> The parameter passed to watchdog_init_timeout is an unsigned int, so you should
> really use an unsigned int here. Depending on -1 being converted to maxuint is a
> bit of side effect programming. Easier to just set it to 0, which the watchdog
> core interprets as invalid as well.

Done.

> > +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> > +
> I think this should come later, after you are sure that there will be
> no failure.

Moved.

> > +	platform_set_drvdata(pdev, NULL);
> 
> It is unnecessary to set this to NULL (the infrastructure does it for you).

Okay.

I've seen this being done on way too many places. I've really should
have checked that though.

Thank you for your response, I'll submit an updated revision shortly.
-- 
Lubomir Rintel <lkundrak@v3.sk>

[PATCH v2] bcm2835: Add Broadcom BCM2835 RNG to the device tree

[Lubomir Rintel]

This adds a device tree binding for random number generator present on Broadcom
BCM2835 SoC, used in Raspberry Pi and Roku 2 devices.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: linux-rpi-kernel@lists.infradead.org
---
Changes for v2:
    - Split out from the driver changeset
    - Added documentation

 .../devicetree/bindings/rng/brcm,bcm2835.txt       |   13 +++++++++++++
 arch/arm/boot/dts/bcm2835.dtsi                     |    5 +++++
 2 files changed, 18 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rng/brcm,bcm2835.txt

diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.txt b/Documentation/devicetree/bindings/rng/brcm,bcm2835.txt
new file mode 100644
index 0000000..07ccdaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.txt
@@ -0,0 +1,13 @@
+BCM2835 Random number generator
+
+Required properties:
+
+- compatible : should be "brcm,bcm2835-rng"
+- reg : Specifies base physical address and size of the registers.
+
+Example:
+
+rng {
+        compatible = "brcm,bcm2835-rng";
+        reg = <0x7e104000 0x10>;
+};
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 7e0481e..dc22336 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -34,6 +34,11 @@
 			reg = <0x7e100000 0x28>;
 		};
 
+		rng {
+			compatible = "brcm,bcm2835-rng";
+			reg = <0x7e104000 0x10>;
+		};
+
 		uart@20201000 {
 			compatible = "brcm,bcm2835-pl011", "arm,pl011", "arm,primecell";
 			reg = <0x7e201000 0x1000>;
-- 
1.7.1

[PATCH v3] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
used in Raspberry Pi and Roku 2 devices.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-watchdog@vger.kernel.org
---
Changes for v2:
    - Use per-device structure instead of global variables for lock and memory base
    - Fix default timeout setting
    - Do not leak memory if probe fails
    - Style fixes
    - Split out defconfig into a separate commit
    - Meaningful commit message with credit

Changes for v3:
    - Add proper copyright notice to header
    - Drop status constants, we don't use them
    - Fix heartbeat parameter's type
    - Log driver initialization message once the device is probed

 .../bindings/watchdog/brcm,bcm2835-pm-wdog.txt     |    5 +
 drivers/watchdog/Kconfig                           |   11 ++
 drivers/watchdog/Makefile                          |    1 +
 drivers/watchdog/bcm2835_wdt.c                     |  190 ++++++++++++++++++++
 4 files changed, 207 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/bcm2835_wdt.c

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
index d209366..f801d71 100644
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
@@ -5,9 +5,14 @@ Required properties:
 - compatible : should be "brcm,bcm2835-pm-wdt"
 - reg : Specifies base physical address and size of the registers.
 
+Optional properties:
+
+- timeout-sec   : Contains the watchdog timeout in seconds
+
 Example:
 
 watchdog {
 	compatible = "brcm,bcm2835-pm-wdt";
 	reg = <0x7e100000 0x28>;
+	timeout-sec = <10>;
 };
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9fcc70c..f4e4a8e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1098,6 +1098,17 @@ config BCM63XX_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called bcm63xx_wdt.
 
+config BCM2835_WDT
+	tristate "Broadcom BCM2835 hardware watchdog"
+	depends on ARCH_BCM2835
+	select WATCHDOG_CORE
+	help
+	  Watchdog driver for the built in watchdog hardware in Broadcom
+	  BCM2835 SoC.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called bcm2835_wdt.
+
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..1bf5675 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 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
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 0000000..13a8c00
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,190 @@
+/*
+ * Watchdog driver for Broadcom BCM2835
+ *
+ * Interface to the Broadcom BCM2835 watchdog timer hardware is based on
+ * "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
+ * "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
+ *
+ * (c) Copyright 2010 Broadcom Europe Ltd
+ * Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/miscdevice.h>
+
+#define PM_RSTC				0x1c
+#define PM_WDOG				0x24
+
+#define PM_PASSWORD			0x5a000000
+
+#define PM_WDOG_TIME_SET		0x000fffff
+#define PM_RSTC_WRCFG_CLR		0xffffffcf
+#define PM_RSTC_WRCFG_SET		0x00000030
+#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
+#define PM_RSTC_RESET			0x00000102
+
+#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
+#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
+
+struct bcm2835_wdt {
+	void __iomem		*base;
+	spinlock_t		lock;
+};
+
+static unsigned int heartbeat = 0;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+static int bcm2835_wdt_start(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+	uint32_t cur;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
+				PM_WDOG_TIME_SET), wdt->base + PM_WDOG);
+	cur = readl_relaxed(wdt->base + PM_RSTC);
+	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+		  PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC);
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return 0;
+}
+
+static int bcm2835_wdt_stop(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC);
+	dev_info(wdog->dev, "Watchdog timer stopped");
+	return 0;
+}
+
+static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
+{
+	wdog->timeout = t;
+	return 0;
+}
+
+static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	uint32_t ret = readl_relaxed(wdt->base + PM_WDOG);
+	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
+}
+
+static struct watchdog_ops bcm2835_wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	bcm2835_wdt_start,
+	.stop =		bcm2835_wdt_stop,
+	.set_timeout =	bcm2835_wdt_set_timeout,
+	.get_timeleft =	bcm2835_wdt_get_timeleft,
+};
+
+static struct watchdog_info bcm2835_wdt_info = {
+	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+	.identity =	"Broadcom BCM2835 Watchdog timer",
+};
+
+static struct watchdog_device bcm2835_wdt_wdd = {
+	.info =		&bcm2835_wdt_info,
+	.ops =		&bcm2835_wdt_ops,
+	.min_timeout =	1,
+	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+	.timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+};
+
+static int bcm2835_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct bcm2835_wdt *wdt;
+	int err;
+
+	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(dev, "Failed to allocate memory for watchdog device");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&wdt->lock);
+
+	wdt->base = of_iomap(np, 0);
+	if (!wdt->base) {
+		dev_err(dev, "Failed to remap watchdog regs");
+		return -ENODEV;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
+	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
+	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
+	err = watchdog_register_device(&bcm2835_wdt_wdd);
+	if (err) {
+		dev_err(dev, "Failed to register watchdog device");
+		iounmap(wdt->base);
+	}
+	dev_info(dev, "Broadcom BCM2835 watchdog timer");
+
+	return err;
+}
+
+static int bcm2835_wdt_remove(struct platform_device *pdev)
+{
+	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
+
+	platform_set_drvdata(pdev, NULL);
+	watchdog_unregister_device(&bcm2835_wdt_wdd);
+	iounmap(wdt->base);
+
+	return 0;
+}
+
+static void bcm2835_wdt_shutdown(struct platform_device *pdev)
+{
+	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
+}
+
+static const struct of_device_id bcm2835_wdt_of_match[] = {
+	{ .compatible = "brcm,bcm2835-pm-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
+
+static struct platform_driver bcm2835_wdt_driver = {
+	.probe		= bcm2835_wdt_probe,
+	.remove		= bcm2835_wdt_remove,
+	.shutdown	= bcm2835_wdt_shutdown,
+	.driver = {
+		.name =		"bcm2835-wdt",
+		.owner =	THIS_MODULE,
+		.of_match_table = bcm2835_wdt_of_match,
+	},
+};
+module_platform_driver(bcm2835_wdt_driver);
+
+module_param(heartbeat, uint, 0);
+MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
1.7.1

Re: [PATCH v2] bcm2835: Add Broadcom BCM2835 RNG to the device tree

[Lubomir Rintel]

On Tue, 2013-03-26 at 18:48 +0100, Lubomir Rintel wrote:
> This adds a device tree binding for random number generator present on Broadcom
> BCM2835 SoC, used in Raspberry Pi and Roku 2 devices.

Please ignore this one; it has been re-sent here by accident.

Re: [PATCH v2] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Guenter Roeck]

On Tue, Mar 26, 2013 at 06:47:55PM +0100, Lubomir Rintel wrote:
> On Sun, 2013-03-24 at 09:12 -0700, Guenter Roeck wrote:
> > On Sun, Mar 24, 2013 at 03:22:53PM +0100, Lubomir Rintel wrote:
> > > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > > used in Raspberry Pi and Roku 2 devices.
> > > 
> > > Interface to the Broadcom BCM2835 watchdog timer hardware is based on
> > > "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
> > > "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
> > > 
> > I would suggest to put that into the coments at the top of the driver.
> > Also, if the original code has a copyright statement, you might want
> > to copy that.
> 
> Okay.
> 
> > > +#include <linux/miscdevice.h>
> > 
> > Just noticed - you should not need that include.
> 
> Well, I think the bottommost line of the driver uses that and it's a
> good practice to provide a device number alias. Is that one completely
> useless?
> 
>   MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> 
Ah, I didn't realize this is still used. The documentation suggests that it can be
kept when converting drivers to the watchdog core, so I guess there must be a reason.

Thanks,
Guenter

Re: [PATCH v3] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Guenter Roeck]

On Tue, Mar 26, 2013 at 06:50:00PM +0100, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-watchdog@vger.kernel.org
> ---
> Changes for v2:
>     - Use per-device structure instead of global variables for lock and memory base
>     - Fix default timeout setting
>     - Do not leak memory if probe fails
>     - Style fixes
>     - Split out defconfig into a separate commit
>     - Meaningful commit message with credit
> 
> Changes for v3:
>     - Add proper copyright notice to header
>     - Drop status constants, we don't use them
>     - Fix heartbeat parameter's type
>     - Log driver initialization message once the device is probed
> 
>  .../bindings/watchdog/brcm,bcm2835-pm-wdog.txt     |    5 +
>  drivers/watchdog/Kconfig                           |   11 ++
>  drivers/watchdog/Makefile                          |    1 +
>  drivers/watchdog/bcm2835_wdt.c                     |  190 ++++++++++++++++++++
>  4 files changed, 207 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/watchdog/bcm2835_wdt.c
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> index d209366..f801d71 100644
> --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> @@ -5,9 +5,14 @@ Required properties:
>  - compatible : should be "brcm,bcm2835-pm-wdt"
>  - reg : Specifies base physical address and size of the registers.
>  
> +Optional properties:
> +
> +- timeout-sec   : Contains the watchdog timeout in seconds
> +
>  Example:
>  
>  watchdog {
>  	compatible = "brcm,bcm2835-pm-wdt";
>  	reg = <0x7e100000 0x28>;
> +	timeout-sec = <10>;
>  };
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 9fcc70c..f4e4a8e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1098,6 +1098,17 @@ config BCM63XX_WDT
>  	  To compile this driver as a loadable module, choose M here.
>  	  The module will be called bcm63xx_wdt.
>  
> +config BCM2835_WDT
> +	tristate "Broadcom BCM2835 hardware watchdog"
> +	depends on ARCH_BCM2835
> +	select WATCHDOG_CORE
> +	help
> +	  Watchdog driver for the built in watchdog hardware in Broadcom
> +	  BCM2835 SoC.
> +
> +	  To compile this driver as a loadable module, choose M here.
> +	  The module will be called bcm2835_wdt.
> +
>  config LANTIQ_WDT
>  	tristate "Lantiq SoC watchdog"
>  	depends on LANTIQ
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a300b94..1bf5675 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>  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
>  
>  # AVR32 Architecture
>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> new file mode 100644
> index 0000000..13a8c00
> --- /dev/null
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -0,0 +1,190 @@
> +/*
> + * Watchdog driver for Broadcom BCM2835
> + *
> + * Interface to the Broadcom BCM2835 watchdog timer hardware is based on
> + * "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
> + * "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
> + *
> + * (c) Copyright 2010 Broadcom Europe Ltd
> + * Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/miscdevice.h>
> +
> +#define PM_RSTC				0x1c
> +#define PM_WDOG				0x24
> +
> +#define PM_PASSWORD			0x5a000000
> +
> +#define PM_WDOG_TIME_SET		0x000fffff
> +#define PM_RSTC_WRCFG_CLR		0xffffffcf
> +#define PM_RSTC_WRCFG_SET		0x00000030
> +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> +#define PM_RSTC_RESET			0x00000102
> +
> +#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
> +#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
> +
> +struct bcm2835_wdt {
> +	void __iomem		*base;
> +	spinlock_t		lock;
> +};
> +
> +static unsigned int heartbeat = 0;

checkpatch.pl says:

ERROR: do not initialise statics to 0 or NULL
#178: FILE: drivers/watchdog/bcm2835_wdt.c:44:
+static unsigned int heartbeat = 0;

> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +
> +static int bcm2835_wdt_start(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +	uint32_t cur;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
> +				PM_WDOG_TIME_SET), wdt->base + PM_WDOG);
> +	cur = readl_relaxed(wdt->base + PM_RSTC);
> +	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
> +		  PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC);
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_stop(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC);
> +	dev_info(wdog->dev, "Watchdog timer stopped");
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> +{
> +	wdog->timeout = t;
> +	return 0;
> +}
> +
> +static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	uint32_t ret = readl_relaxed(wdt->base + PM_WDOG);
> +	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
> +}
> +
> +static struct watchdog_ops bcm2835_wdt_ops = {
> +	.owner =	THIS_MODULE,
> +	.start =	bcm2835_wdt_start,
> +	.stop =		bcm2835_wdt_stop,
> +	.set_timeout =	bcm2835_wdt_set_timeout,
> +	.get_timeleft =	bcm2835_wdt_get_timeleft,
> +};
> +
> +static struct watchdog_info bcm2835_wdt_info = {
> +	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +			WDIOF_KEEPALIVEPING,
> +	.identity =	"Broadcom BCM2835 Watchdog timer",
> +};
> +
> +static struct watchdog_device bcm2835_wdt_wdd = {
> +	.info =		&bcm2835_wdt_info,
> +	.ops =		&bcm2835_wdt_ops,
> +	.min_timeout =	1,
> +	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
> +	.timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
> +};
> +
> +static int bcm2835_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct bcm2835_wdt *wdt;
> +	int err;
> +
> +	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(dev, "Failed to allocate memory for watchdog device");
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	wdt->base = of_iomap(np, 0);
> +	if (!wdt->base) {
> +		dev_err(dev, "Failed to remap watchdog regs");
> +		return -ENODEV;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
> +	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
> +	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
> +	err = watchdog_register_device(&bcm2835_wdt_wdd);
> +	if (err) {
> +		dev_err(dev, "Failed to register watchdog device");
> +		iounmap(wdt->base);
> +	}
> +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> +
Hmm .. that means you'll get the info message even in the error case.
Is that intentional ? It is inconsistent with the remap error message
above, which does not result in the info message.

> +	return err;
> +}
> +
> +static int bcm2835_wdt_remove(struct platform_device *pdev)
> +{
> +	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	platform_set_drvdata(pdev, NULL);

Still unnecessary.

Thanks,
Guenter

Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[Stephen Warren]

On 03/24/2013 08:12 AM, Lubomir Rintel wrote:
> On Fri, 2013-03-22 at 20:24 -0600, Stephen Warren wrote:
> 
> Thank you for your response!
> 
>> On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
>>> Signed-off-by: Lubomir Rintel <lkundrak at v3.sk>

>> I'm curious where you got the documentation to write this driver; this
>> HW module isn't described in BCM2835-ARM-Peripherals.pdf. I assume this
>> is based on the downstream kernel driver? If so, at least some credit in
>> the commit description might be appropriate. At least the relevant
>> commit downstream already has an appropriate Signed-off-by line:-)
> 
> Your guess is right, used bcm2708_wdog driver from rpi-3.6.y as a reference. 
> I'll add that information to the commit message.
> 
> The Signed-off-by line is indeed present, but unfortunately does not seem to be 
> particularly appropriate:
> 
> Signed-off-by: popcornmix <popcornmix@gmail.com>

That s-o-b line maps to Dom Cobley. In a previous message on the
linux-rpi-kernel mailing list, he gave his permission to re-write the
name part of that to "Dom Cobley". That would make the s-o-b useful.

http://lists.infradead.org/pipermail/linux-rpi-kernel/2012-September/000154.html

Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

[Stephen Warren]

On 03/24/2013 08:12 AM, Lubomir Rintel wrote:
> On Fri, 2013-03-22 at 20:24 -0600, Stephen Warren wrote:
> 
> Thank you for your response!
> 
>> On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
>>> Signed-off-by: Lubomir Rintel <lkundrak at v3.sk>

>> A couple of general comments:
>>
>> 1)
>>
>> This driver touches the same registers that
>> arch/arm/mach-bcm2835/bcm2835.c uses to implement reboot and "power
>> off". Some co-ordination might be necessary.
>>
>> The implementation of bcm2835_power_off() could easily be moved into
>> this driver, to avoid some of the need for co-ordination.
>>
>> Moving bcm2835_restart() would be more tricky, since the ARM machine
>> descriptor needs a pointer to that function. I guess the kernel probably
>> ensures that none of the code in this watchdog driver is running by the
>> time bcm2835_restart() is called, although perhaps it'd be better to
>> have mach-bcm2835/bcm2835.c and this driver share a lock?
> 
> I need help here, I'm not sure what's the proper way to address this
> (whether to include the actual reboot code in the wdt driver or the
> platform driver).

I assume by "platform driver" you mean the code in
arch/arm/mach-bcm2835? The phrase "platform driver" usually refers to a
struct platform_driver, so that usage is a little unusual. I think you
would usually say "arch code" to refer to mach-bcm2835/, or something
like that!

> Is it okay to have the platform driver depend on watchdog timer?
> Is it okay for the platform driver not to reboot properly if the kernel
> is running without the wdt driver loaded?
> 
> (For now, I'll send a revised patch addressing the other issues so that
> it can be reviewed without addressing this yet.)

I guess what we should do here is merge the driver as you've posted it,
then later we can migrate any code from arch/arm/mach-bcm2835 into the
WDT driver.

IIRC, there certainly are some existing WDT drivers that implement the
reboot hook for their platforms, so it's probably OK to migrate that way
sometime, although indeed the issues you raise do deserve some thought.

Re: [PATCH v3] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Stephen Warren]

On 03/26/2013 11:50 AM, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.

Since this patch defines a new DT binding, you should send it to
devicetree-discuss@lists.ozlabs.org too.

> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c

> +/*
> + * Watchdog driver for Broadcom BCM2835
> + *
> + * Interface to the Broadcom BCM2835 watchdog timer hardware is based on
> + * "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
> + * "rpi-3.6.y" of git://github.com/raspberrypi/linux.git

I see that the patch isn't S-o-b Luke in the downstream kernel. However,
it is S-o-b Dom Cobley (popcornmix), and they both work for Broadcom, so
I think that's OK.

> +static int bcm2835_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct bcm2835_wdt *wdt;
> +	int err;
> +
> +	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(dev, "Failed to allocate memory for watchdog device");
> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	wdt->base = of_iomap(np, 0);
> +	if (!wdt->base) {
> +		dev_err(dev, "Failed to remap watchdog regs");
> +		return -ENODEV;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);

Do you really need both of those? I would have thought just one would
have been enough.

I'd be tempted to put the platform_set_drvdata() call right after the
devm_kzalloc() of wdt, but it's not a big deal either way.

Re: [PATCH v3] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Stephen Warren]

On 03/26/2013 11:50 AM, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.

Tested-by: Stephen Warren <swarren@wwwdotorg.org>

Re: [PATCH v3] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

On Tue, 2013-03-26 at 21:40 -0600, Stephen Warren wrote:
> On 03/26/2013 11:50 AM, Lubomir Rintel wrote:
> > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > used in Raspberry Pi and Roku 2 devices.
> 
> Since this patch defines a new DT binding, you should send it to
> devicetree-discuss@lists.ozlabs.org too.

Okay.

...
> > + * Interface to the Broadcom BCM2835 watchdog timer hardware is based on
> > + * "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
> > + * "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
> 
> I see that the patch isn't S-o-b Luke in the downstream kernel. However,
> it is S-o-b Dom Cobley (popcornmix), and they both work for Broadcom, so
> I think that's OK.

Okay, I'll add it.

...
> > +	platform_set_drvdata(pdev, wdt);
> > +	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
> 
> Do you really need both of those? I would have thought just one would
> have been enough.

I think so; I would like to get rid of the platform_*() one, but we need
that in remove hook. All of the existing drivers in drivers/watchdog/
tree either use global data to represent a single device or use two of
the *_set_drvdata() functions -- the watchdog_*() one and dev_*(),
platform_*(), amba_*() or the like.

> I'd be tempted to put the platform_set_drvdata() call right after the
> devm_kzalloc() of wdt, but it's not a big deal either way.

Done.

I'm actually rather thankful for style fixes such as this, since I'm
only getting familiar with what's customary to do there.

Updated patch will follow shortly.

-- 
Lubomir Rintel <lkundrak@v3.sk>

Re: [PATCH v3] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

On Tue, 2013-03-26 at 14:03 -0700, Guenter Roeck wrote:
> On Tue, Mar 26, 2013 at 06:50:00PM +0100, Lubomir Rintel wrote:
> > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > used in Raspberry Pi and Roku 2 devices.

...
> > +static unsigned int heartbeat = 0;
> 
> checkpatch.pl says:
> 
> ERROR: do not initialise statics to 0 or NULL
> #178: FILE: drivers/watchdog/bcm2835_wdt.c:44:
> +static unsigned int heartbeat = 0;

Whoops. Will remove.

> > +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> > +
> Hmm .. that means you'll get the info message even in the error case.
> Is that intentional ? It is inconsistent with the remap error message
> above, which does not result in the info message.

That was unintentional -- will change that.

> > +	platform_set_drvdata(pdev, NULL);
> 
> Still unnecessary.

Oops -- I'll actually remove it this time.

-- 
Lubomir Rintel <lkundrak@v3.sk>

[PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
used in Raspberry Pi and Roku 2 devices.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Tested-by: Stephen Warren <swarren@wwwdotorg.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-watchdog@vger.kernel.org
---
Changes for v2:
    - Use per-device structure instead of global variables for lock and memory base
    - Fix default timeout setting
    - Do not leak memory if probe fails
    - Style fixes
    - Split out defconfig into a separate commit
    - Meaningful commit message with credit

Changes for v3:
    - Add proper copyright notice to header
    - Drop status constants, we don't use them
    - Fix heartbeat parameter's type
    - Log driver initialization message once the device is probed

Changes for v4:
    - Drop an useless initializer
    - Add a signoff from downstream
    - Do not announce the driver to the log if we failed to probe
    - Set platform data earlier and do not explicitly unset it

 .../bindings/watchdog/brcm,bcm2835-pm-wdog.txt     |    5 +
 drivers/watchdog/Kconfig                           |   11 ++
 drivers/watchdog/Makefile                          |    1 +
 drivers/watchdog/bcm2835_wdt.c                     |  190 ++++++++++++++++++++
 4 files changed, 207 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/bcm2835_wdt.c

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
index d209366..f801d71 100644
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
@@ -5,9 +5,14 @@ Required properties:
 - compatible : should be "brcm,bcm2835-pm-wdt"
 - reg : Specifies base physical address and size of the registers.
 
+Optional properties:
+
+- timeout-sec   : Contains the watchdog timeout in seconds
+
 Example:
 
 watchdog {
 	compatible = "brcm,bcm2835-pm-wdt";
 	reg = <0x7e100000 0x28>;
+	timeout-sec = <10>;
 };
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9fcc70c..f4e4a8e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1098,6 +1098,17 @@ config BCM63XX_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called bcm63xx_wdt.
 
+config BCM2835_WDT
+	tristate "Broadcom BCM2835 hardware watchdog"
+	depends on ARCH_BCM2835
+	select WATCHDOG_CORE
+	help
+	  Watchdog driver for the built in watchdog hardware in Broadcom
+	  BCM2835 SoC.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called bcm2835_wdt.
+
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..1bf5675 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 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
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 0000000..d47c842
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,190 @@
+/*
+ * Watchdog driver for Broadcom BCM2835
+ *
+ * Interface to the Broadcom BCM2835 watchdog timer hardware is based on
+ * "bcm2708_wdog" driver written by Luke Diamand that was obtained from branch
+ * "rpi-3.6.y" of git://github.com/raspberrypi/linux.git
+ *
+ * (c) Copyright 2010 Broadcom Europe Ltd
+ * Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/miscdevice.h>
+
+#define PM_RSTC				0x1c
+#define PM_WDOG				0x24
+
+#define PM_PASSWORD			0x5a000000
+
+#define PM_WDOG_TIME_SET		0x000fffff
+#define PM_RSTC_WRCFG_CLR		0xffffffcf
+#define PM_RSTC_WRCFG_SET		0x00000030
+#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
+#define PM_RSTC_RESET			0x00000102
+
+#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
+#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
+
+struct bcm2835_wdt {
+	void __iomem		*base;
+	spinlock_t		lock;
+};
+
+static unsigned int heartbeat;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+static int bcm2835_wdt_start(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+	uint32_t cur;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
+				PM_WDOG_TIME_SET), wdt->base + PM_WDOG);
+	cur = readl_relaxed(wdt->base + PM_RSTC);
+	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+		  PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC);
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return 0;
+}
+
+static int bcm2835_wdt_stop(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC);
+	dev_info(wdog->dev, "Watchdog timer stopped");
+	return 0;
+}
+
+static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
+{
+	wdog->timeout = t;
+	return 0;
+}
+
+static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	uint32_t ret = readl_relaxed(wdt->base + PM_WDOG);
+	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
+}
+
+static struct watchdog_ops bcm2835_wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	bcm2835_wdt_start,
+	.stop =		bcm2835_wdt_stop,
+	.set_timeout =	bcm2835_wdt_set_timeout,
+	.get_timeleft =	bcm2835_wdt_get_timeleft,
+};
+
+static struct watchdog_info bcm2835_wdt_info = {
+	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+	.identity =	"Broadcom BCM2835 Watchdog timer",
+};
+
+static struct watchdog_device bcm2835_wdt_wdd = {
+	.info =		&bcm2835_wdt_info,
+	.ops =		&bcm2835_wdt_ops,
+	.min_timeout =	1,
+	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+	.timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+};
+
+static int bcm2835_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct bcm2835_wdt *wdt;
+	int err;
+
+	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(dev, "Failed to allocate memory for watchdog device");
+		return -ENOMEM;
+	}
+	platform_set_drvdata(pdev, wdt);
+
+	spin_lock_init(&wdt->lock);
+
+	wdt->base = of_iomap(np, 0);
+	if (!wdt->base) {
+		dev_err(dev, "Failed to remap watchdog regs");
+		return -ENODEV;
+	}
+
+	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
+	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
+	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
+	err = watchdog_register_device(&bcm2835_wdt_wdd);
+	if (err) {
+		dev_err(dev, "Failed to register watchdog device");
+		iounmap(wdt->base);
+		return err;
+	}
+
+	dev_info(dev, "Broadcom BCM2835 watchdog timer");
+	return 0;
+}
+
+static int bcm2835_wdt_remove(struct platform_device *pdev)
+{
+	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&bcm2835_wdt_wdd);
+	iounmap(wdt->base);
+
+	return 0;
+}
+
+static void bcm2835_wdt_shutdown(struct platform_device *pdev)
+{
+	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
+}
+
+static const struct of_device_id bcm2835_wdt_of_match[] = {
+	{ .compatible = "brcm,bcm2835-pm-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
+
+static struct platform_driver bcm2835_wdt_driver = {
+	.probe		= bcm2835_wdt_probe,
+	.remove		= bcm2835_wdt_remove,
+	.shutdown	= bcm2835_wdt_shutdown,
+	.driver = {
+		.name =		"bcm2835-wdt",
+		.owner =	THIS_MODULE,
+		.of_match_table = bcm2835_wdt_of_match,
+	},
+};
+module_platform_driver(bcm2835_wdt_driver);
+
+module_param(heartbeat, uint, 0);
+MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
1.7.1

Re: [PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Guenter Roeck]

On Wed, Mar 27, 2013 at 05:40:24PM +0100, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Stephen Warren]

On 03/27/2013 10:40 AM, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>

Those two s-o-b lines should be swapped, since if Dom did sign off on
any part of this patch, he did it before you did.

That said...

I wonder if it's actually appropriate to include Dom's s-o-b here, since
I don't think he really wrote this patch itself. I think you mentioned
that you hadn't use much of the downstream driver except for some defines?

To be clear, I mentioned the existence of the S-o-b line downstream
simply to demonstrate that the commits you were getting information from
had correctly followed the process described in
Documentation/SubmittingPatches, and so it was OK to use that
information while creating a GPL'd driver.

So there are a couple of ways that this patch could have been created:

1) You took the downstream commit itself, cherry-picked it into the
upstream kernel, modified it to suit upstream, and then submitted that.
The modifications might be extensive, such as renaming the file,
removing parts of the code that the upstream watchdog core now handles,
adding some new features, fixing bugs, cleanup, etc.; whatever is needed
to upstream the patch.

In this case, I believe it would be appropriate to maintain any S-o-b
lines from the original downstream commit, and add yours. But, I believe
you should also (a) maintain the git author field from the original
downstream commit (b) include a list of the changes you made to the
patch in the commit description, so you can be blamed for them rather
than the original author:-)

2) You read the downstream commit for information, but created a
completely new driver for the upstream kernel, using the downstream
driver just as a reference. In this case, I believe it's fine for the
git author field to be you, and for the only s-o-b line present to be
yours, since you really did write the patch from scratch. However, you
should credit the downstream work in the (c) header and/or commit
description.

This current patch sees to be a slight hybrid of both approaches (you're
listed as the git author, but have included Dom's s-o-b line on a patch
I don't think he created, and wasn't directly derived from one he created).

I'm not sure if I'm being too picky. I guess I'll leave it up to Wim Van
Sebroeck, since he's the watchdog maintainer and would be the person who
applies this patch.

Re: [PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Guenter Roeck]

On Wed, Mar 27, 2013 at 09:00:29PM -0600, Stephen Warren wrote:
> On 03/27/2013 10:40 AM, Lubomir Rintel wrote:
> > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > used in Raspberry Pi and Roku 2 devices.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> 
> Those two s-o-b lines should be swapped, since if Dom did sign off on
> any part of this patch, he did it before you did.
> 
> That said...
> 
> I wonder if it's actually appropriate to include Dom's s-o-b here, since
> I don't think he really wrote this patch itself. I think you mentioned
> that you hadn't use much of the downstream driver except for some defines?
> 
> To be clear, I mentioned the existence of the S-o-b line downstream
> simply to demonstrate that the commits you were getting information from
> had correctly followed the process described in
> Documentation/SubmittingPatches, and so it was OK to use that
> information while creating a GPL'd driver.
> 
> So there are a couple of ways that this patch could have been created:
> 
> 1) You took the downstream commit itself, cherry-picked it into the
> upstream kernel, modified it to suit upstream, and then submitted that.
> The modifications might be extensive, such as renaming the file,
> removing parts of the code that the upstream watchdog core now handles,
> adding some new features, fixing bugs, cleanup, etc.; whatever is needed
> to upstream the patch.
> 
> In this case, I believe it would be appropriate to maintain any S-o-b
> lines from the original downstream commit, and add yours. But, I believe
> you should also (a) maintain the git author field from the original
> downstream commit (b) include a list of the changes you made to the
> patch in the commit description, so you can be blamed for them rather
> than the original author:-)
> 
> 2) You read the downstream commit for information, but created a
> completely new driver for the upstream kernel, using the downstream
> driver just as a reference. In this case, I believe it's fine for the
> git author field to be you, and for the only s-o-b line present to be
> yours, since you really did write the patch from scratch. However, you
> should credit the downstream work in the (c) header and/or commit
> description.
> 
> This current patch sees to be a slight hybrid of both approaches (you're
> listed as the git author, but have included Dom's s-o-b line on a patch
> I don't think he created, and wasn't directly derived from one he created).
> 
> I'm not sure if I'm being too picky. I guess I'll leave it up to Wim Van
> Sebroeck, since he's the watchdog maintainer and would be the person who
> applies this patch.
> 
I wondered about the same.

I think 2) would be more appropriate. My approach would have been to reference
previous work in the file header, something along the line of "derived from
xxx", and add a copyright statement from the original work if there was one -
pretty much what you propose above.

Guenter

Re: [PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Stephen Warren]

On 03/27/2013 10:40 AM, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.

Lubomir, I don't believe this patch was applied for 3.10. Are you
planning to fix up any remaining issues and repost for 3.11?

Re: [PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Wim Van Sebroeck]

Hi Lubomir,

> On 03/27/2013 10:40 AM, Lubomir Rintel wrote:
> > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > used in Raspberry Pi and Roku 2 devices.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> 
> Those two s-o-b lines should be swapped, since if Dom did sign off on
> any part of this patch, he did it before you did.
> 
> That said...
> 
> I wonder if it's actually appropriate to include Dom's s-o-b here, since
> I don't think he really wrote this patch itself. I think you mentioned
> that you hadn't use much of the downstream driver except for some defines?
> 
> To be clear, I mentioned the existence of the S-o-b line downstream
> simply to demonstrate that the commits you were getting information from
> had correctly followed the process described in
> Documentation/SubmittingPatches, and so it was OK to use that
> information while creating a GPL'd driver.
> 
> So there are a couple of ways that this patch could have been created:
> 
> 1) You took the downstream commit itself, cherry-picked it into the
> upstream kernel, modified it to suit upstream, and then submitted that.
> The modifications might be extensive, such as renaming the file,
> removing parts of the code that the upstream watchdog core now handles,
> adding some new features, fixing bugs, cleanup, etc.; whatever is needed
> to upstream the patch.
> 
> In this case, I believe it would be appropriate to maintain any S-o-b
> lines from the original downstream commit, and add yours. But, I believe
> you should also (a) maintain the git author field from the original
> downstream commit (b) include a list of the changes you made to the
> patch in the commit description, so you can be blamed for them rather
> than the original author:-)
> 
> 2) You read the downstream commit for information, but created a
> completely new driver for the upstream kernel, using the downstream
> driver just as a reference. In this case, I believe it's fine for the
> git author field to be you, and for the only s-o-b line present to be
> yours, since you really did write the patch from scratch. However, you
> should credit the downstream work in the (c) header and/or commit
> description.
> 
> This current patch sees to be a slight hybrid of both approaches (you're
> listed as the git author, but have included Dom's s-o-b line on a patch
> I don't think he created, and wasn't directly derived from one he created).
> 
> I'm not sure if I'm being too picky. I guess I'll leave it up to Wim Van
> Sebroeck, since he's the watchdog maintainer and would be the person who
> applies this patch.

Can you create a patch v5 with yourselve as author and add the necessary (c)
and references in it so that I can do the final review? (That is if situation
2 is indeed the case. If however it is situation 1 then we should get the
original code and have that as a patch and then add a second patch with your
modifications.).

Kind regards,
Wim.

Re: [PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

Hi Wim,

On Sun, 2013-05-26 at 16:22 +0200, Wim Van Sebroeck wrote:
> Hi Lubomir,
> 
> > On 03/27/2013 10:40 AM, Lubomir Rintel wrote:
> > > This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> > > used in Raspberry Pi and Roku 2 devices.
> > > 
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> > 
> > Those two s-o-b lines should be swapped, since if Dom did sign off on
> > any part of this patch, he did it before you did.
> > 
> > That said...
> > 
> > I wonder if it's actually appropriate to include Dom's s-o-b here, since
> > I don't think he really wrote this patch itself. I think you mentioned
> > that you hadn't use much of the downstream driver except for some defines?
> > 
> > To be clear, I mentioned the existence of the S-o-b line downstream
> > simply to demonstrate that the commits you were getting information from
> > had correctly followed the process described in
> > Documentation/SubmittingPatches, and so it was OK to use that
> > information while creating a GPL'd driver.
> > 
> > So there are a couple of ways that this patch could have been created:
> > 
> > 1) You took the downstream commit itself, cherry-picked it into the
> > upstream kernel, modified it to suit upstream, and then submitted that.
> > The modifications might be extensive, such as renaming the file,
> > removing parts of the code that the upstream watchdog core now handles,
> > adding some new features, fixing bugs, cleanup, etc.; whatever is needed
> > to upstream the patch.
> > 
> > In this case, I believe it would be appropriate to maintain any S-o-b
> > lines from the original downstream commit, and add yours. But, I believe
> > you should also (a) maintain the git author field from the original
> > downstream commit (b) include a list of the changes you made to the
> > patch in the commit description, so you can be blamed for them rather
> > than the original author:-)
> > 
> > 2) You read the downstream commit for information, but created a
> > completely new driver for the upstream kernel, using the downstream
> > driver just as a reference. In this case, I believe it's fine for the
> > git author field to be you, and for the only s-o-b line present to be
> > yours, since you really did write the patch from scratch. However, you
> > should credit the downstream work in the (c) header and/or commit
> > description.
> > 
> > This current patch sees to be a slight hybrid of both approaches (you're
> > listed as the git author, but have included Dom's s-o-b line on a patch
> > I don't think he created, and wasn't directly derived from one he created).
> > 
> > I'm not sure if I'm being too picky. I guess I'll leave it up to Wim Van
> > Sebroeck, since he's the watchdog maintainer and would be the person who
> > applies this patch.
> 
> Can you create a patch v5 with yourselve as author and add the necessary (c)
> and references in it so that I can do the final review? (That is if situation
> 2 is indeed the case. If however it is situation 1 then we should get the
> original code and have that as a patch and then add a second patch with your
> modifications.).

I'm still not sure what to do. For most part, the driver is written from
scratch, since the original one was not utilizing recent enough
frameworks (such as watchdog-core or device tree bindings). Thus the
patch against the original driver would not make any sense at all.

On the other hand, I've referred to the original driver for hardware
information and copied a couple of defines as they are (such as
SECS_TO_WDOG_TICKS), thus I figured out it's a required to include the
original driver's sign-off and copyright information.

Thus neither 1) nor 2) is the case, and I can't figure out myself what
needs to be changed at this point. I'm wondering if you could help me
decide?

Thank you,
-- 
Lubomir Rintel <lkundrak@v3.sk>

Re: [PATCH v4] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Stephen Warren]

On 06/18/2013 10:50 AM, Lubomir Rintel wrote:
> Hi Wim,
> 
> On Sun, 2013-05-26 at 16:22 +0200, Wim Van Sebroeck wrote:
>> Hi Lubomir,
>>
>>> On 03/27/2013 10:40 AM, Lubomir Rintel wrote:
>>>> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
>>>> used in Raspberry Pi and Roku 2 devices.
>>>>
>>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>>> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
>>>
>>> Those two s-o-b lines should be swapped, since if Dom did sign off on
>>> any part of this patch, he did it before you did.
>>>
>>> That said...
>>>
>>> I wonder if it's actually appropriate to include Dom's s-o-b here, since
>>> I don't think he really wrote this patch itself. I think you mentioned
>>> that you hadn't use much of the downstream driver except for some defines?
>>>
>>> To be clear, I mentioned the existence of the S-o-b line downstream
>>> simply to demonstrate that the commits you were getting information from
>>> had correctly followed the process described in
>>> Documentation/SubmittingPatches, and so it was OK to use that
>>> information while creating a GPL'd driver.
>>>
>>> So there are a couple of ways that this patch could have been created:
>>>
>>> 1) You took the downstream commit itself, cherry-picked it into the
>>> upstream kernel, modified it to suit upstream, and then submitted that.
>>> The modifications might be extensive, such as renaming the file,
>>> removing parts of the code that the upstream watchdog core now handles,
>>> adding some new features, fixing bugs, cleanup, etc.; whatever is needed
>>> to upstream the patch.
>>>
>>> In this case, I believe it would be appropriate to maintain any S-o-b
>>> lines from the original downstream commit, and add yours. But, I believe
>>> you should also (a) maintain the git author field from the original
>>> downstream commit (b) include a list of the changes you made to the
>>> patch in the commit description, so you can be blamed for them rather
>>> than the original author:-)
>>>
>>> 2) You read the downstream commit for information, but created a
>>> completely new driver for the upstream kernel, using the downstream
>>> driver just as a reference. In this case, I believe it's fine for the
>>> git author field to be you, and for the only s-o-b line present to be
>>> yours, since you really did write the patch from scratch. However, you
>>> should credit the downstream work in the (c) header and/or commit
>>> description.
>>>
>>> This current patch sees to be a slight hybrid of both approaches (you're
>>> listed as the git author, but have included Dom's s-o-b line on a patch
>>> I don't think he created, and wasn't directly derived from one he created).
>>>
>>> I'm not sure if I'm being too picky. I guess I'll leave it up to Wim Van
>>> Sebroeck, since he's the watchdog maintainer and would be the person who
>>> applies this patch.
>>
>> Can you create a patch v5 with yourselve as author and add the necessary (c)
>> and references in it so that I can do the final review? (That is if situation
>> 2 is indeed the case. If however it is situation 1 then we should get the
>> original code and have that as a patch and then add a second patch with your
>> modifications.).
> 
> I'm still not sure what to do. For most part, the driver is written from
> scratch, since the original one was not utilizing recent enough
> frameworks (such as watchdog-core or device tree bindings). Thus the
> patch against the original driver would not make any sense at all.
> 
> On the other hand, I've referred to the original driver for hardware
> information and copied a couple of defines as they are (such as
> SECS_TO_WDOG_TICKS), thus I figured out it's a required to include the
> original driver's sign-off and copyright information.
> 
> Thus neither 1) nor 2) is the case, and I can't figure out myself what
> needs to be changed at this point. I'm wondering if you could help me
> decide?

This sounds exactly like case (2) to me; copying a few defines for HW
constants doesn't really sound like having derived the driver from the
original commit.

[PATCH v5] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Lubomir Rintel]

This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
used in Raspberry Pi and Roku 2 devices.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Tested-by: Stephen Warren <swarren@wwwdotorg.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-rpi-kernel@lists.infradead.org
Cc: linux-watchdog@vger.kernel.org
Cc: devicetree-discuss@lists.ozlabs.org
---
Changes for v2:
    - Use per-device structure instead of global variables for lock and memory base
    - Fix default timeout setting
    - Do not leak memory if probe fails
    - Style fixes
    - Split out defconfig into a separate commit
    - Meaningful commit message with credit

Changes for v3:
    - Add proper copyright notice to header
    - Drop status constants, we don't use them
    - Fix heartbeat parameter's type
    - Log driver initialization message once the device is probed

Changes for v4:
    - Drop an useless initializer
    - Add a signoff from downstream
    - Do not announce the driver to the log if we failed to probe
    - Set platform data earlier and do not explicitly unset it

Changes for v5:
    - Copyright clarification

 .../bindings/watchdog/brcm,bcm2835-pm-wdog.txt     |    5 +
 drivers/watchdog/Kconfig                           |   11 ++
 drivers/watchdog/Makefile                          |    1 +
 drivers/watchdog/bcm2835_wdt.c                     |  189 ++++++++++++++++++++
 4 files changed, 206 insertions(+), 0 deletions(-)
 create mode 100644 drivers/watchdog/bcm2835_wdt.c

diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
index d209366..f801d71 100644
--- a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
+++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
@@ -5,9 +5,14 @@ Required properties:
 - compatible : should be "brcm,bcm2835-pm-wdt"
 - reg : Specifies base physical address and size of the registers.
 
+Optional properties:
+
+- timeout-sec   : Contains the watchdog timeout in seconds
+
 Example:
 
 watchdog {
 	compatible = "brcm,bcm2835-pm-wdt";
 	reg = <0x7e100000 0x28>;
+	timeout-sec = <10>;
 };
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index e89fc31..c3d3b16 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1098,6 +1098,17 @@ config BCM63XX_WDT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called bcm63xx_wdt.
 
+config BCM2835_WDT
+	tristate "Broadcom BCM2835 hardware watchdog"
+	depends on ARCH_BCM2835
+	select WATCHDOG_CORE
+	help
+	  Watchdog driver for the built in watchdog hardware in Broadcom
+	  BCM2835 SoC.
+
+	  To compile this driver as a loadable module, choose M here.
+	  The module will be called bcm2835_wdt.
+
 config LANTIQ_WDT
 	tristate "Lantiq SoC watchdog"
 	depends on LANTIQ
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a300b94..1bf5675 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 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
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 0000000..61566fc
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,189 @@
+/*
+ * Watchdog driver for Broadcom BCM2835
+ *
+ * "bcm2708_wdog" driver written by Luke Diamand that was obtained from
+ * branch "rpi-3.6.y" of git://github.com/raspberrypi/linux.git was used
+ * as a hardware reference for the Broadcom BCM2835 watchdog timer.
+ *
+ * Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/miscdevice.h>
+
+#define PM_RSTC				0x1c
+#define PM_WDOG				0x24
+
+#define PM_PASSWORD			0x5a000000
+
+#define PM_WDOG_TIME_SET		0x000fffff
+#define PM_RSTC_WRCFG_CLR		0xffffffcf
+#define PM_RSTC_WRCFG_SET		0x00000030
+#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
+#define PM_RSTC_RESET			0x00000102
+
+#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
+#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
+
+struct bcm2835_wdt {
+	void __iomem		*base;
+	spinlock_t		lock;
+};
+
+static unsigned int heartbeat;
+static bool nowayout = WATCHDOG_NOWAYOUT;
+
+static int bcm2835_wdt_start(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+	uint32_t cur;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
+				PM_WDOG_TIME_SET), wdt->base + PM_WDOG);
+	cur = readl_relaxed(wdt->base + PM_RSTC);
+	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+		  PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC);
+
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return 0;
+}
+
+static int bcm2835_wdt_stop(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC);
+	dev_info(wdog->dev, "Watchdog timer stopped");
+	return 0;
+}
+
+static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
+{
+	wdog->timeout = t;
+	return 0;
+}
+
+static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
+{
+	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
+
+	uint32_t ret = readl_relaxed(wdt->base + PM_WDOG);
+	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
+}
+
+static struct watchdog_ops bcm2835_wdt_ops = {
+	.owner =	THIS_MODULE,
+	.start =	bcm2835_wdt_start,
+	.stop =		bcm2835_wdt_stop,
+	.set_timeout =	bcm2835_wdt_set_timeout,
+	.get_timeleft =	bcm2835_wdt_get_timeleft,
+};
+
+static struct watchdog_info bcm2835_wdt_info = {
+	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
+			WDIOF_KEEPALIVEPING,
+	.identity =	"Broadcom BCM2835 Watchdog timer",
+};
+
+static struct watchdog_device bcm2835_wdt_wdd = {
+	.info =		&bcm2835_wdt_info,
+	.ops =		&bcm2835_wdt_ops,
+	.min_timeout =	1,
+	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+	.timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
+};
+
+static int bcm2835_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct bcm2835_wdt *wdt;
+	int err;
+
+	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(dev, "Failed to allocate memory for watchdog device");
+		return -ENOMEM;
+	}
+	platform_set_drvdata(pdev, wdt);
+
+	spin_lock_init(&wdt->lock);
+
+	wdt->base = of_iomap(np, 0);
+	if (!wdt->base) {
+		dev_err(dev, "Failed to remap watchdog regs");
+		return -ENODEV;
+	}
+
+	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
+	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
+	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
+	err = watchdog_register_device(&bcm2835_wdt_wdd);
+	if (err) {
+		dev_err(dev, "Failed to register watchdog device");
+		iounmap(wdt->base);
+		return err;
+	}
+
+	dev_info(dev, "Broadcom BCM2835 watchdog timer");
+	return 0;
+}
+
+static int bcm2835_wdt_remove(struct platform_device *pdev)
+{
+	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&bcm2835_wdt_wdd);
+	iounmap(wdt->base);
+
+	return 0;
+}
+
+static void bcm2835_wdt_shutdown(struct platform_device *pdev)
+{
+	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
+}
+
+static const struct of_device_id bcm2835_wdt_of_match[] = {
+	{ .compatible = "brcm,bcm2835-pm-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
+
+static struct platform_driver bcm2835_wdt_driver = {
+	.probe		= bcm2835_wdt_probe,
+	.remove		= bcm2835_wdt_remove,
+	.shutdown	= bcm2835_wdt_shutdown,
+	.driver = {
+		.name =		"bcm2835-wdt",
+		.owner =	THIS_MODULE,
+		.of_match_table = bcm2835_wdt_of_match,
+	},
+};
+module_platform_driver(bcm2835_wdt_driver);
+
+module_param(heartbeat, uint, 0);
+MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
-- 
1.7.1

Re: [PATCH v5] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Guenter Roeck]

On Tue, Jun 18, 2013 at 07:44:48PM +0200, Lubomir Rintel wrote:
> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-watchdog@vger.kernel.org
> Cc: devicetree-discuss@lists.ozlabs.org

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Changes for v2:
>     - Use per-device structure instead of global variables for lock and memory base
>     - Fix default timeout setting
>     - Do not leak memory if probe fails
>     - Style fixes
>     - Split out defconfig into a separate commit
>     - Meaningful commit message with credit
> 
> Changes for v3:
>     - Add proper copyright notice to header
>     - Drop status constants, we don't use them
>     - Fix heartbeat parameter's type
>     - Log driver initialization message once the device is probed
> 
> Changes for v4:
>     - Drop an useless initializer
>     - Add a signoff from downstream
>     - Do not announce the driver to the log if we failed to probe
>     - Set platform data earlier and do not explicitly unset it
> 
> Changes for v5:
>     - Copyright clarification
> 
>  .../bindings/watchdog/brcm,bcm2835-pm-wdog.txt     |    5 +
>  drivers/watchdog/Kconfig                           |   11 ++
>  drivers/watchdog/Makefile                          |    1 +
>  drivers/watchdog/bcm2835_wdt.c                     |  189 ++++++++++++++++++++
>  4 files changed, 206 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/watchdog/bcm2835_wdt.c
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> index d209366..f801d71 100644
> --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm2835-pm-wdog.txt
> @@ -5,9 +5,14 @@ Required properties:
>  - compatible : should be "brcm,bcm2835-pm-wdt"
>  - reg : Specifies base physical address and size of the registers.
>  
> +Optional properties:
> +
> +- timeout-sec   : Contains the watchdog timeout in seconds
> +
>  Example:
>  
>  watchdog {
>  	compatible = "brcm,bcm2835-pm-wdt";
>  	reg = <0x7e100000 0x28>;
> +	timeout-sec = <10>;
>  };
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index e89fc31..c3d3b16 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1098,6 +1098,17 @@ config BCM63XX_WDT
>  	  To compile this driver as a loadable module, choose M here.
>  	  The module will be called bcm63xx_wdt.
>  
> +config BCM2835_WDT
> +	tristate "Broadcom BCM2835 hardware watchdog"
> +	depends on ARCH_BCM2835
> +	select WATCHDOG_CORE
> +	help
> +	  Watchdog driver for the built in watchdog hardware in Broadcom
> +	  BCM2835 SoC.
> +
> +	  To compile this driver as a loadable module, choose M here.
> +	  The module will be called bcm2835_wdt.
> +
>  config LANTIQ_WDT
>  	tristate "Lantiq SoC watchdog"
>  	depends on LANTIQ
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a300b94..1bf5675 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>  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
>  
>  # AVR32 Architecture
>  obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
> new file mode 100644
> index 0000000..61566fc
> --- /dev/null
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -0,0 +1,189 @@
> +/*
> + * Watchdog driver for Broadcom BCM2835
> + *
> + * "bcm2708_wdog" driver written by Luke Diamand that was obtained from
> + * branch "rpi-3.6.y" of git://github.com/raspberrypi/linux.git was used
> + * as a hardware reference for the Broadcom BCM2835 watchdog timer.
> + *
> + * Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/miscdevice.h>
> +
> +#define PM_RSTC				0x1c
> +#define PM_WDOG				0x24
> +
> +#define PM_PASSWORD			0x5a000000
> +
> +#define PM_WDOG_TIME_SET		0x000fffff
> +#define PM_RSTC_WRCFG_CLR		0xffffffcf
> +#define PM_RSTC_WRCFG_SET		0x00000030
> +#define PM_RSTC_WRCFG_FULL_RESET	0x00000020
> +#define PM_RSTC_RESET			0x00000102
> +
> +#define SECS_TO_WDOG_TICKS(x) ((x) << 16)
> +#define WDOG_TICKS_TO_SECS(x) ((x) >> 16)
> +
> +struct bcm2835_wdt {
> +	void __iomem		*base;
> +	spinlock_t		lock;
> +};
> +
> +static unsigned int heartbeat;
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +
> +static int bcm2835_wdt_start(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +	uint32_t cur;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);
> +
> +	writel_relaxed(PM_PASSWORD | (SECS_TO_WDOG_TICKS(wdog->timeout) &
> +				PM_WDOG_TIME_SET), wdt->base + PM_WDOG);
> +	cur = readl_relaxed(wdt->base + PM_RSTC);
> +	writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
> +		  PM_RSTC_WRCFG_FULL_RESET, wdt->base + PM_RSTC);
> +
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_stop(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	writel_relaxed(PM_PASSWORD | PM_RSTC_RESET, wdt->base + PM_RSTC);
> +	dev_info(wdog->dev, "Watchdog timer stopped");
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t)
> +{
> +	wdog->timeout = t;
> +	return 0;
> +}
> +
> +static unsigned int bcm2835_wdt_get_timeleft(struct watchdog_device *wdog)
> +{
> +	struct bcm2835_wdt *wdt = watchdog_get_drvdata(wdog);
> +
> +	uint32_t ret = readl_relaxed(wdt->base + PM_WDOG);
> +	return WDOG_TICKS_TO_SECS(ret & PM_WDOG_TIME_SET);
> +}
> +
> +static struct watchdog_ops bcm2835_wdt_ops = {
> +	.owner =	THIS_MODULE,
> +	.start =	bcm2835_wdt_start,
> +	.stop =		bcm2835_wdt_stop,
> +	.set_timeout =	bcm2835_wdt_set_timeout,
> +	.get_timeleft =	bcm2835_wdt_get_timeleft,
> +};
> +
> +static struct watchdog_info bcm2835_wdt_info = {
> +	.options =	WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> +			WDIOF_KEEPALIVEPING,
> +	.identity =	"Broadcom BCM2835 Watchdog timer",
> +};
> +
> +static struct watchdog_device bcm2835_wdt_wdd = {
> +	.info =		&bcm2835_wdt_info,
> +	.ops =		&bcm2835_wdt_ops,
> +	.min_timeout =	1,
> +	.max_timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
> +	.timeout =	WDOG_TICKS_TO_SECS(PM_WDOG_TIME_SET),
> +};
> +
> +static int bcm2835_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct bcm2835_wdt *wdt;
> +	int err;
> +
> +	wdt = devm_kzalloc(dev, sizeof(struct bcm2835_wdt), GFP_KERNEL);
> +	if (!wdt) {
> +		dev_err(dev, "Failed to allocate memory for watchdog device");
> +		return -ENOMEM;
> +	}
> +	platform_set_drvdata(pdev, wdt);
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	wdt->base = of_iomap(np, 0);
> +	if (!wdt->base) {
> +		dev_err(dev, "Failed to remap watchdog regs");
> +		return -ENODEV;
> +	}
> +
> +	watchdog_set_drvdata(&bcm2835_wdt_wdd, wdt);
> +	watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev);
> +	watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout);
> +	err = watchdog_register_device(&bcm2835_wdt_wdd);
> +	if (err) {
> +		dev_err(dev, "Failed to register watchdog device");
> +		iounmap(wdt->base);
> +		return err;
> +	}
> +
> +	dev_info(dev, "Broadcom BCM2835 watchdog timer");
> +	return 0;
> +}
> +
> +static int bcm2835_wdt_remove(struct platform_device *pdev)
> +{
> +	struct bcm2835_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&bcm2835_wdt_wdd);
> +	iounmap(wdt->base);
> +
> +	return 0;
> +}
> +
> +static void bcm2835_wdt_shutdown(struct platform_device *pdev)
> +{
> +	bcm2835_wdt_stop(&bcm2835_wdt_wdd);
> +}
> +
> +static const struct of_device_id bcm2835_wdt_of_match[] = {
> +	{ .compatible = "brcm,bcm2835-pm-wdt", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, bcm2835_wdt_of_match);
> +
> +static struct platform_driver bcm2835_wdt_driver = {
> +	.probe		= bcm2835_wdt_probe,
> +	.remove		= bcm2835_wdt_remove,
> +	.shutdown	= bcm2835_wdt_shutdown,
> +	.driver = {
> +		.name =		"bcm2835-wdt",
> +		.owner =	THIS_MODULE,
> +		.of_match_table = bcm2835_wdt_of_match,
> +	},
> +};
> +module_platform_driver(bcm2835_wdt_driver);
> +
> +module_param(heartbeat, uint, 0);
> +MODULE_PARM_DESC(heartbeat, "Initial watchdog heartbeat in seconds");
> +
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
> +MODULE_DESCRIPTION("Driver for Broadcom BCM2835 watchdog timer");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> -- 
> 1.7.1
> 
> 

Re: [PATCH v5] watchdog: Add Broadcom BCM2835 watchdog timer driver

[Wim Van Sebroeck]

Hi Lubomir,

> This adds a driver for watchdog timer hardware present on Broadcom BCM2835 SoC,
> used in Raspberry Pi and Roku 2 devices.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-rpi-kernel@lists.infradead.org
> Cc: linux-watchdog@vger.kernel.org
> Cc: devicetree-discuss@lists.ozlabs.org

Added to linux-watchdog-next.

Kind regards,
Wim.