Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
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 >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
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 >> 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: watchdog: Add Broadcom BCM2708 watchdog timer driver
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 -, 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
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
>
> 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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
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 > > Cc: Stephen Warren > > Cc: Wim Van Sebroeck > > Cc: linux-rpi-ker...@lists.infradead.org > > Cc: linux-watch...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: watchdog: Add Broadcom BCM2708 watchdog timer driver
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 -, 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel
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:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
On 03/22/2013 01:55 PM, Lubomir Rintel wrote: > Signed-off-by: Lubomir Rintel > Cc: Stephen Warren > Cc: Wim Van Sebroeck > Cc: linux-rpi-ker...@lists.infradead.org > Cc: linux-watch...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: watchdog: Add Broadcom BCM2708 watchdog timer driver
On Fri, Mar 22, 2013 at 12:55:07PM -, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel
> Cc: Stephen Warren
> Cc: Wim Van Sebroeck
> Cc: linux-rpi-ker...@lists.infradead.org
> Cc: linux-watch...@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 000..834f201
> --- /dev/null
> +++ b/drivers/watchdog/bcm2835_wdt.c
> @@ -0,0 +1,158 @@
> +/*
> + * Watchdog driver for Broadcom BCM2835
> + *
> + * Copyright (C) 2013 Lubomir Rintel
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define PM_RSTC 0x1c
> +#define PM_RSTS 0x20
> +#define PM_WDOG 0x24
> +
> +#define PM_PASSWORD 0x5a00
> +#define PM_RSTC_WRCFG_MASK 0x0030
> +#define PM_RSTC_WRCFG_FULL_RESET 0x0020
> +#define PM_RSTS_HADWRH_SET 0x0040
> +
> +#define PM_WDOG_TIME_SET 0x000f
> +#define PM_RSTC_WRCFG_CLR0xffcf
> +#define PM_RSTC_WRCFG_SET0x0030
> +#define PM_RSTC_WRCFG_FULL_RESET 0x0020
> +#define PM_RSTC_RESET0x0102
> +
> +#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 ?
> + re
[PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
Signed-off-by: Lubomir Rintel
Cc: Stephen Warren
Cc: Wim Van Sebroeck
Cc: linux-rpi-ker...@lists.infradead.org
Cc: linux-watch...@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 000..834f201
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,158 @@
+/*
+ * Watchdog driver for Broadcom BCM2835
+ *
+ * Copyright (C) 2013 Lubomir Rintel
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define PM_RSTC0x1c
+#define PM_RSTS0x20
+#define PM_WDOG0x24
+
+#define PM_PASSWORD0x5a00
+#define PM_RSTC_WRCFG_MASK 0x0030
+#define PM_RSTC_WRCFG_FULL_RESET 0x0020
+#define PM_RSTS_HADWRH_SET 0x0040
+
+#define PM_WDOG_TIME_SET 0x000f
+#define PM_RSTC_WRCFG_CLR 0xffcf
+#define PM_RSTC_WRCFG_SET 0x0030
+#define PM_RSTC_WRCFG_FULL_RESET 0x0020
+#define PM_RSTC_RESET 0x0102
+
+#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_timelef
