Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

147 views
Skip to first unread message

Lubomir Rintel

unread,
Mar 22, 2013, 9:00:02 AM3/22/13
to
Signed-off-by: Lubomir Rintel <lkun...@v3.sk>
Cc: Stephen Warren <swa...@wwwdotorg.org>
Cc: Wim Van Sebroeck <w...@iguana.be>
Cc: linux-rp...@lists.infradead.org
Cc: linux-w...@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 <lkun...@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 <lkun...@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-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Guenter Roeck

unread,
Mar 22, 2013, 11:40:01 AM3/22/13
to
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 ...

Arend van Spriel

unread,
Mar 22, 2013, 2:50:02 PM3/22/13
to
On 03/22/2013 01:55 PM, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel <lkun...@v3.sk>
> Cc: Stephen Warren <swa...@wwwdotorg.org>
> Cc: Wim Van Sebroeck <w...@iguana.be>
> Cc: linux-rp...@lists.infradead.org
> Cc: linux-w...@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

Stephen Warren

unread,
Mar 22, 2013, 10:30:02 PM3/22/13
to
On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel <lkun...@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:-)

Lubomir Rintel

unread,
Mar 24, 2013, 10:10:02 AM3/24/13
to
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 <lkun...@v3.sk>

Lubomir Rintel

unread,
Mar 24, 2013, 10:10:02 AM3/24/13
to
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 <lkun...@v3.sk>
> > Cc: Stephen Warren <swa...@wwwdotorg.org>
> > Cc: Wim Van Sebroeck <w...@iguana.be>
> > Cc: linux-rp...@lists.infradead.org
> > Cc: linux-w...@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 <lkun...@v3.sk>

Lubomir Rintel

unread,
Mar 24, 2013, 10:20:01 AM3/24/13
to
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 <popco...@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 <lkun...@v3.sk>

Lubomir Rintel

unread,
Mar 24, 2013, 10:30:02 AM3/24/13
to
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 <lkun...@v3.sk>
Cc: Stephen Warren <swa...@wwwdotorg.org>
Cc: Wim Van Sebroeck <w...@iguana.be>
Cc: linux-rp...@lists.infradead.org
Cc: linux-w...@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>;
};
index 0000000..3899638
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,189 @@
+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,
+ },
+};

Guenter Roeck

unread,
Mar 24, 2013, 11:40:01 AM3/24/13
to
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

Guenter Roeck

unread,
Mar 24, 2013, 12:20:01 PM3/24/13
to
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.
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);
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).
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in

Lubomir Rintel

unread,
Mar 26, 2013, 1:50:02 PM3/26/13
to
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 <lkun...@v3.sk>

Guenter Roeck

unread,
Mar 26, 2013, 3:50:02 PM3/26/13
to
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

Stephen Warren

unread,
Mar 26, 2013, 11:30:02 PM3/26/13
to
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 <popco...@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

Stephen Warren

unread,
Mar 26, 2013, 11:40:02 PM3/26/13
to
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.
0 new messages