| Message ID | 20130914152124.317379835@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Saturday 14 September 2013, Domenico Andreoli wrote: > + > +static int __init bcm4760_wdt_init(void) > +{ > + struct device_node *node; > + > + node = of_find_matching_node(NULL, bcm4760_pm_wdt_match); > + if (!node) { > + pr_info("No bcm4760 watchdog node\n"); > + return -1; > + } > + > + wdt_regs = of_iomap(node, 0); > + of_node_put(node); Since this is now in the drivers directory and initialized at regular module_init level, I'd ask you to register it as a proper platform_driver and move the initialization into the probe() callback. You also need to put Wim as the watchdog subsystem maintainer on Cc (I did in this reply) and ask him to merge the driver. I assume that Wim will ask you to add a watchdog_register_device() call and a set of watchdog_ops to make this a functional driver. From the platform perspective, I don't care, but it is certainly a logical step. Arnd
On Sun, Sep 15, 2013 at 08:09:26PM +0200, Arnd Bergmann wrote: > On Saturday 14 September 2013, Domenico Andreoli wrote: > > + > > +static int __init bcm4760_wdt_init(void) > > +{ > > + struct device_node *node; > > + > > + node = of_find_matching_node(NULL, bcm4760_pm_wdt_match); > > + if (!node) { > > + pr_info("No bcm4760 watchdog node\n"); > > + return -1; > > + } > > + > > + wdt_regs = of_iomap(node, 0); > > + of_node_put(node); > > Since this is now in the drivers directory and initialized at regular > module_init level, I'd ask you to register it as a proper platform_driver > and move the initialization into the probe() callback. You also need to > put Wim as the watchdog subsystem maintainer on Cc (I did in this reply) > and ask him to merge the driver. > > I assume that Wim will ask you to add a watchdog_register_device() call > and a set of watchdog_ops to make this a functional driver. From the > platform perspective, I don't care, but it is certainly a logical step. issue here is that there is already a proper watchdog driver, the sp805. so I guess now the task shifts to adding restart hook support to it, right? Kind regards, Domenico
On Sunday 15 September 2013, Domenico Andreoli wrote: > issue here is that there is already a proper watchdog driver, the sp805. > > so I guess now the task shifts to adding restart hook support to it, right? Yes, correct. There is an interesting question however: we have to deal with the same driver being used in some machines that need to use it as the only way to reset the system, as well as the case where you actually want to use some other method. An easy way to handle this would be a boolean device tree property that tells the driver whether or not to register, but we might want to come up with a more sophisticated way to have multiple reset handlers registered an prioritized so we try the "best" one first. Maybe someone else has an opinion on this. If not, just do the property. Arnd
On Sun, Sep 15, 2013 at 10:10:36PM +0200, Arnd Bergmann wrote: > On Sunday 15 September 2013, Domenico Andreoli wrote: > > issue here is that there is already a proper watchdog driver, the sp805. > > > > so I guess now the task shifts to adding restart hook support to it, right? > > Yes, correct. > > There is an interesting question however: we have to deal with the same driver > being used in some machines that need to use it as the only way to reset the > system, as well as the case where you actually want to use some other method. in a certain sense, there is space for a generic watchdog based restart hook mechanism but the current watchdog ops do not provide the necessary guarantees in atomic context. > An easy way to handle this would be a boolean device tree property that > tells the driver whether or not to register, but we might want to > come up with a more sophisticated way to have multiple reset handlers > registered an prioritized so we try the "best" one first. Maybe someone > else has an opinion on this. If not, just do the property. the simplest solution I see is adding a DT option to the sp805 driver so that it registers the restart hook when asked to do so. need to investigate whether the sp805 DT support is provided by some generic AMBA mechanism or is completely missing. > > Arnd thanks, Domenico
On Monday 16 September 2013, Domenico Andreoli wrote: > On Sun, Sep 15, 2013 at 10:10:36PM +0200, Arnd Bergmann wrote: > > On Sunday 15 September 2013, Domenico Andreoli wrote: > > > issue here is that there is already a proper watchdog driver, the sp805. > > > > > > so I guess now the task shifts to adding restart hook support to it, right? > > > > Yes, correct. > > > > There is an interesting question however: we have to deal with the same driver > > being used in some machines that need to use it as the only way to reset the > > system, as well as the case where you actually want to use some other method. > > in a certain sense, there is space for a generic watchdog based restart > hook mechanism but the current watchdog ops do not provide the necessary > guarantees in atomic context. It sounds like a good idea. I see another problem there, which is that registering a restart hook is architecture specific at the moment, so it would also require generalizing that, or alternatively keeping watchdog based restart specific to ARM and any architecture that adds support in a similar way. > > An easy way to handle this would be a boolean device tree property that > > tells the driver whether or not to register, but we might want to > > come up with a more sophisticated way to have multiple reset handlers > > registered an prioritized so we try the "best" one first. Maybe someone > > else has an opinion on this. If not, just do the property. > > the simplest solution I see is adding a DT option to the sp805 driver so > that it registers the restart hook when asked to do so. > > need to investigate whether the sp805 DT support is provided by some generic > AMBA mechanism or is completely missing. I think it should just work if you put the right properties into DT: it only uses one memory resource (from "reg" property) and one clk (from "clocks" property), and those are automatically there for AMBA devices. If the hardware does not fill the correct primecell ID, you may have to add a "arm,primecell-periphid=<0x00141805>" property. Arnd
On Monday 16 September 2013 14:00:01 Wim Van Sebroeck wrote: > > I remember that the Cobalt Devices have no ability to reboot themselves. > That's why the driver uses the reboot_notifier for rebooting the device. > See drivers/watchdog/alim7101_wdt.c . Hmm, a notifier seems the wrong approach here, because we really want to handle all reboot_notifiers before actually rebooting, and we'd enter the wdt driver at a random point in the notifier chain. It probably works by chance if the watchdog reboots a second after its notifier is called, and all other calls are done as well, but it doesn't seem like a clean solution. > I'll have a look to have an extra ops that adds this functionality in the framework. Ok, cool. Thanks! Arnd
Hi Wim, On Mon, Sep 16, 2013 at 10:01:08PM +0200, Arnd Bergmann wrote: > On Monday 16 September 2013 14:00:01 Wim Van Sebroeck wrote: > > > > I remember that the Cobalt Devices have no ability to reboot themselves. > > That's why the driver uses the reboot_notifier for rebooting the device. > > See drivers/watchdog/alim7101_wdt.c . > > Hmm, a notifier seems the wrong approach here, because we really want > to handle all reboot_notifiers before actually rebooting, and we'd enter > the wdt driver at a random point in the notifier chain. It probably works > by chance if the watchdog reboots a second after its notifier is called, > and all other calls are done as well, but it doesn't seem like a clean > solution. > > > I'll have a look to have an extra ops that adds this functionality in the framework. I'd like to do something here. I'm thinking at a simple "atomic_reset()" wdt op which purpose it to use the specific watchdog to trigger a HW reset. As the name implies, the call needs to be callable in atomic context. Any other constrains I'm missing? At this point a DT attribute in the wdt stanzas could be used to register the reboot hook at runtime. thanks, Domenico
Index: b/Documentation/devicetree/bindings/watchdog/brcm,bcm4760-pm-wdt.txt =================================================================== --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm4760-pm-wdt.txt @@ -0,0 +1,14 @@ +Broadcom BCM4760 watchdog timer device tree bindings +---------------------------------------------------- + +Required properties: + +- compatible : should be "brcm,bcm4760-pm-wdt" +- reg : Specifies base physical address and size of the registers. + +Example: + +watchdog { + compatible = "brcm,bcm4760-pm-wdt"; + reg = <0xbd000 0x1000>; +}; Index: b/arch/arm/boot/dts/bcm4760.dtsi =================================================================== --- a/arch/arm/boot/dts/bcm4760.dtsi +++ b/arch/arm/boot/dts/bcm4760.dtsi @@ -39,6 +39,11 @@ reg = <0xbc000 0x1000>; }; + watchdog { + compatible = "brcm,bcm4760-pm-wdt"; + reg = <0xbd000 0x1000>; + }; + vic0: interrupt-controller@80000 { compatible = "brcm,bcm4760-pl192", "arm,pl192-vic", "arm,primecell"; reg = <0x80000 0x1000>; Index: b/drivers/watchdog/Makefile =================================================================== --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -32,6 +32,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb. obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o +obj-$(CONFIG_BCM4760_WDT) += bcm4760_wdt.o obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o obj-$(CONFIG_TWL4030_WATCHDOG) += twl4030_wdt.o obj-$(CONFIG_21285_WATCHDOG) += wdt285.o Index: b/drivers/watchdog/bcm4760_wdt.c =================================================================== --- /dev/null +++ b/drivers/watchdog/bcm4760_wdt.c @@ -0,0 +1,109 @@ +/* + * Broadcom BCM4760 based ARM11 SoCs watchdog support + * + * Copyright (C) 2013 Domenico Andreoli <domenico.andreoli@linux.com> + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/of_address.h> + +#include <asm/mach/map.h> +#include <asm/system_misc.h> + +#define BCM4760_WDT_LOAD 0x000 +#define BCM4760_WDT_CTRL 0x008 +#define BCM4760_WDT_INTCLR 0x00c +#define BCM4760_WDT_LOCK 0xc00 + +#define BCM4760_WDT_PASSWORD 0x1acce551 +#define BCM4760_WDT_INTEN BIT(0) +#define BCM4760_WDT_RESEN BIT(1) + +/* + * The machine restart method can be called from an atomic context so we won't + * be able to ioremap the regs then. + */ +static void __iomem *wdt_regs; + +static void *arm_pm_restart_saved; + +static void bcm4760_restart(enum reboot_mode mode, const char *cmd) +{ + /* unlock watchdog registers */ + writel(BCM4760_WDT_PASSWORD, wdt_regs + BCM4760_WDT_LOCK); + + /* disable watchdog */ + writel(0, wdt_regs + BCM4760_WDT_CTRL); + udelay(20); + + /* clear the irq status */ + writel(1, wdt_regs + BCM4760_WDT_INTCLR); + udelay(20); + + /* expire after 5 cycles (~156us) */ + writel(5, wdt_regs + BCM4760_WDT_LOAD); + /* enable watchdog */ + writel(BCM4760_WDT_INTEN | BCM4760_WDT_RESEN, + wdt_regs + BCM4760_WDT_CTRL); + + /* lock watchdog registers */ + writel(1, wdt_regs + BCM4760_WDT_LOCK); + /* wait the bite */ + udelay(400); +} + +static const struct of_device_id bcm4760_pm_wdt_match[] __initconst = { + { .compatible = "brcm,bcm4760-pm-wdt" }, + {}, +}; + +static int __init bcm4760_wdt_init(void) +{ + struct device_node *node; + + node = of_find_matching_node(NULL, bcm4760_pm_wdt_match); + if (!node) { + pr_info("No bcm4760 watchdog node\n"); + return -1; + } + + wdt_regs = of_iomap(node, 0); + of_node_put(node); + + if (!wdt_regs) { + pr_err("Can't remap watchdog registers\n"); + return -1; + } + + /* unlock watchdog registers */ + writel(BCM4760_WDT_PASSWORD, wdt_regs + BCM4760_WDT_LOCK); + /* disable watchdog */ + writel(0, wdt_regs + BCM4760_WDT_CTRL); + /* lock watchdog registers */ + writel(1, wdt_regs + BCM4760_WDT_LOCK); + + arm_pm_restart_saved = arm_pm_restart; + arm_pm_restart = bcm4760_restart; + return 0; +} + +static int __exit bcm4760_wdt_exit(void) +{ + arm_pm_restart = arm_pm_restart_saved; + iounmap(wdt_regs); + return 0; +} + +module_init(bcm4760_wdt_init); +module_exit(bcm4760_wdt_exit); Index: b/arch/arm/mach-bcm/Kconfig =================================================================== --- a/arch/arm/mach-bcm/Kconfig +++ b/arch/arm/mach-bcm/Kconfig @@ -23,6 +23,8 @@ config ARCH_BCM4760 bool "Broadcom BCM4760 based SoCs (ARM11)" if ARCH_MULTI_V6 select ARM_AMBA select ARM_VIC + select BCM4760_WDT select CLKSRC_OF select GENERIC_CLOCKEVENTS select SOC_BUS + select WATCHDOG Index: b/drivers/watchdog/Kconfig =================================================================== --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -130,6 +130,17 @@ config AT91SAM9X_WATCHDOG Watchdog timer embedded into AT91SAM9X and AT91CAP9 chips. This will reboot your system when the timeout is reached. +config BCM4760_WDT + tristate "Broadcom BCM4760 hardware watchdog" + depends on ARCH_BCM4760 + select WATCHDOG_CORE + help + Watchdog driver for the built in watchdog hardware in Broadcom + BCM4760 SoC, currently used only as restart-hook implementation. + + To compile this driver as a loadable module, choose M here. + The module will be called bcm4760_wdt. + config 21285_WATCHDOG tristate "DC21285 watchdog" depends on FOOTBRIDGE