| Message ID | 1390431915-5115-8-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 01/22/2014 03:05 PM, Ezequiel Garcia wrote: > DT-enabled where an irqchip driver for the brigde interrupt controller is > available can handle the watchdog IRQ properly. Therefore, we request > the interruption and add a dummy handler that merely calls panic(). > > This is done in order to have an initial 'ack' of the interruption, > which clears the watchdog state. > > Furthermore, since some platforms don't have such IRQ, this commit > makes the interruption specification optional. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > .../devicetree/bindings/watchdog/marvel.txt | 2 ++ > drivers/watchdog/orion_wdt.c | 24 +++++++++++++++++++++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt > index 5dc8d30..0731fbd 100644 > --- a/Documentation/devicetree/bindings/watchdog/marvel.txt > +++ b/Documentation/devicetree/bindings/watchdog/marvel.txt > @@ -7,6 +7,7 @@ Required Properties: > > Optional properties: > > +- interrupts : Contains the IRQ for watchdog expiration > - timeout-sec : Contains the watchdog timeout in seconds > > Example: > @@ -14,6 +15,7 @@ Example: > wdt@20300 { > compatible = "marvell,orion-wdt"; > reg = <0x20300 0x28>; > + interrupts = <3>; > timeout-sec = <10>; > status = "okay"; > }; > diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c > index 2dbeee9..f5e7b17 100644 > --- a/drivers/watchdog/orion_wdt.c > +++ b/drivers/watchdog/orion_wdt.c > @@ -19,6 +19,7 @@ > #include <linux/platform_device.h> > #include <linux/watchdog.h> > #include <linux/init.h> > +#include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/clk.h> > #include <linux/err.h> > @@ -109,10 +110,16 @@ static struct watchdog_device orion_wdt = { > .min_timeout = 1, > }; > > +static irqreturn_t orion_wdt_irq(int irq, void *devid) > +{ > + panic("Watchdog Timeout"); > + return IRQ_HANDLED; > +} > + > static int orion_wdt_probe(struct platform_device *pdev) > { > struct resource *res; > - int ret; > + int ret, irq; > > clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(clk)) { > @@ -145,6 +152,21 @@ static int orion_wdt_probe(struct platform_device *pdev) > /* Let's make sure the watchdog is fully stopped */ > orion_wdt_stop(&orion_wdt); > > + /* It's important to request the IRQ once the watchdog is disabled */ s/once/only after/ > + irq = platform_get_irq(pdev, 0); > + if (irq >= 0) { > + /* > + * Not all supported platforms specify an interruption for the s/interruption/interrupt pin/ (or interrupt) Nitpicks, so Reviewed-by: Guenter Roeck <linux@roeck-us.net> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 26, 2014 at 09:04:12AM +0100, Thomas Petazzoni wrote: > Dear Ezequiel Garcia, > > On Wed, 22 Jan 2014 20:05:04 -0300, Ezequiel Garcia wrote: > > DT-enabled where an irqchip driver for the brigde interrupt controller is > > available can handle the watchdog IRQ properly. Therefore, we request > > the interruption and add a dummy handler that merely calls panic(). > > I don't quiite understand the first sentence of this commit log, and > the commit title looks wrong. Maybe a bad copy/paste or something? > Hm... yes it doesn't look right. It should read: "DT-enabled platforms, where the irqchip driver for the brigde interrupt controller is available, can handle the watchdog IRQ properly. Therefore, we request the interrupt and add a dummy handler that merely calls panic()". I guess we can re-phrase it be a bit more readable. Why does th commit title looks wrong? By requesting the IRQ we are "handling it", no? > > > > This is done in order to have an initial 'ack' of the interruption, > > interruption -> interrupt > Right.
Dear Ezequiel Garcia, On Sun, 26 Jan 2014 10:14:46 -0300, Ezequiel Garcia wrote: > > I don't quiite understand the first sentence of this commit log, and > > the commit title looks wrong. Maybe a bad copy/paste or something? > > > > Hm... yes it doesn't look right. It should read: > > "DT-enabled platforms, where the irqchip driver for the brigde interrupt > controller is available, can handle the watchdog IRQ properly. Therefore, > we request the interrupt and add a dummy handler that merely calls panic()". Ok. > I guess we can re-phrase it be a bit more readable. > > Why does th commit title looks wrong? By requesting the IRQ we are > "handling it", no? Right, but it looks "truncated". Maybe something like: watchdog: orion: handle irq to avoid having to clear BRIDGE_CLAUSE or something like that (adjust to the actual reality, I haven't followed all the implications). Thomas
diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt index 5dc8d30..0731fbd 100644 --- a/Documentation/devicetree/bindings/watchdog/marvel.txt +++ b/Documentation/devicetree/bindings/watchdog/marvel.txt @@ -7,6 +7,7 @@ Required Properties: Optional properties: +- interrupts : Contains the IRQ for watchdog expiration - timeout-sec : Contains the watchdog timeout in seconds Example: @@ -14,6 +15,7 @@ Example: wdt@20300 { compatible = "marvell,orion-wdt"; reg = <0x20300 0x28>; + interrupts = <3>; timeout-sec = <10>; status = "okay"; }; diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c index 2dbeee9..f5e7b17 100644 --- a/drivers/watchdog/orion_wdt.c +++ b/drivers/watchdog/orion_wdt.c @@ -19,6 +19,7 @@ #include <linux/platform_device.h> #include <linux/watchdog.h> #include <linux/init.h> +#include <linux/interrupt.h> #include <linux/io.h> #include <linux/clk.h> #include <linux/err.h> @@ -109,10 +110,16 @@ static struct watchdog_device orion_wdt = { .min_timeout = 1, }; +static irqreturn_t orion_wdt_irq(int irq, void *devid) +{ + panic("Watchdog Timeout"); + return IRQ_HANDLED; +} + static int orion_wdt_probe(struct platform_device *pdev) { struct resource *res; - int ret; + int ret, irq; clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(clk)) { @@ -145,6 +152,21 @@ static int orion_wdt_probe(struct platform_device *pdev) /* Let's make sure the watchdog is fully stopped */ orion_wdt_stop(&orion_wdt); + /* It's important to request the IRQ once the watchdog is disabled */ + irq = platform_get_irq(pdev, 0); + if (irq >= 0) { + /* + * Not all supported platforms specify an interruption for the + * watchdog, so let's make it optional. + */ + ret = devm_request_irq(&pdev->dev, irq, orion_wdt_irq, 0, + pdev->name, &orion_wdt); + if (ret < 0) { + dev_err(&pdev->dev, "failed to request IRQ\n"); + goto disable_clk; + } + } + watchdog_set_nowayout(&orion_wdt, nowayout); ret = watchdog_register_device(&orion_wdt); if (ret)
DT-enabled where an irqchip driver for the brigde interrupt controller is available can handle the watchdog IRQ properly. Therefore, we request the interruption and add a dummy handler that merely calls panic(). This is done in order to have an initial 'ack' of the interruption, which clears the watchdog state. Furthermore, since some platforms don't have such IRQ, this commit makes the interruption specification optional. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- .../devicetree/bindings/watchdog/marvel.txt | 2 ++ drivers/watchdog/orion_wdt.c | 24 +++++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-)