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: [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 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 popcorn...@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 -- 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 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. -- 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: [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 lkund...@v3.sk Cc: Stephen Warren swar...@wwwdotorg.org Cc: Wim Van Sebroeck w...@iguana.be 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 lkund...@v3.sk -- 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 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 popcorn...@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 lkund...@v3.sk
--
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/
[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(_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(_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 =
[PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
Signed-off-by: Lubomir Rintel lkund...@v3.sk
Cc: Stephen Warren swar...@wwwdotorg.org
Cc: Wim Van Sebroeck w...@iguana.be
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 lkund...@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_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 =
Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
On 03/22/2013 01:55 PM, Lubomir Rintel wrote: Signed-off-by: Lubomir Rintel lkund...@v3.sk Cc: Stephen Warren swar...@wwwdotorg.org Cc: Wim Van Sebroeck w...@iguana.be 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: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver
On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
Signed-off-by: Lubomir Rintel lkund...@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:-)
--
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/
