| Message ID | 1390310774-20781-5-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/21/2014 05:26 AM, 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 | 22 +++++++++++++++++++++- > 2 files changed, 23 insertions(+), 1 deletion(-) > [ ... ] > 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)) { > @@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev) > if (!wdt_reg) > return -ENOMEM; > > + irq = platform_get_irq(pdev, 0); > + if (irq > 0) { 0 is a valid interrupt number, and platform_get_irq returns an error code on errors. Should be >= 0. Guenter
On Tue, Jan 21, 2014 at 06:31:02AM -0800, Guenter Roeck wrote: > On 01/21/2014 05:26 AM, 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 | 22 +++++++++++++++++++++- > > 2 files changed, 23 insertions(+), 1 deletion(-) > > > > [ ... ] > > > 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)) { > > @@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev) > > if (!wdt_reg) > > return -ENOMEM; > > > > + irq = platform_get_irq(pdev, 0); > > + if (irq > 0) { > > 0 is a valid interrupt number, and platform_get_irq returns an error code on errors. > Should be >= 0. > Yes, indeed. I'll wait to see if there's any other feedback and then send a v4. Thanks a lot!
Hi Guenter, On Tue, Jan 21, 2014 at 06:31:02AM -0800, Guenter Roeck wrote: [..] > > @@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev) > > if (!wdt_reg) > > return -ENOMEM; > > > > + irq = platform_get_irq(pdev, 0); > > + if (irq > 0) { > > 0 is a valid interrupt number, and platform_get_irq returns an error code on errors. > Should be >= 0. > I'm revisiting this one. I believe this is not the hardware interrupt number, but the one mapped into virq space. So, 0 is not a valid interrupt number. Right?
On 01/26/2014 10:30 PM, Ezequiel Garcia wrote: > Hi Guenter, > > On Tue, Jan 21, 2014 at 06:31:02AM -0800, Guenter Roeck wrote: > [..] >>> @@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev) >>> if (!wdt_reg) >>> return -ENOMEM; >>> >>> + irq = platform_get_irq(pdev, 0); >>> + if (irq > 0) { >> >> 0 is a valid interrupt number, and platform_get_irq returns an error code on errors. >> Should be >= 0. >> > > I'm revisiting this one. I believe this is not the hardware interrupt > number, but the one mapped into virq space. So, 0 is not a valid > interrupt number. > > Right? > Hi, If so, the entire interrupt numbering scheme appears broken. Conceptually it should not make a difference where the interrupt is coming from. If the virq system returns 0 for invalid (non-configured) interrupts, and non-configured 'real' interrupts are reported as -ENXIO, all bets are off. How would a driver know what to expect ? And how would one be expected to review such non-deterministic code ? FWIW, platform_get_irq() does return -ENXIO for invalid interrupts. If there is an independent notion of "0 is an invalid interrupt", it is well hidden. Anyway, if you think the driver should treat 0 as invalid interrupt, go ahead. Who am I to know. Just please don't use my Reviewed-by in this case. Thanks, Guenter
On Sun, Jan 26, 2014 at 10:42:19PM -0800, Guenter Roeck wrote: > On 01/26/2014 10:30 PM, Ezequiel Garcia wrote: > > Hi Guenter, > > > > On Tue, Jan 21, 2014 at 06:31:02AM -0800, Guenter Roeck wrote: > > [..] > >>> @@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev) > >>> if (!wdt_reg) > >>> return -ENOMEM; > >>> > >>> + irq = platform_get_irq(pdev, 0); > >>> + if (irq > 0) { > >> > >> 0 is a valid interrupt number, and platform_get_irq returns an error code on errors. > >> Should be >= 0. > >> > > > > I'm revisiting this one. I believe this is not the hardware interrupt > > number, but the one mapped into virq space. So, 0 is not a valid > > interrupt number. > > > > Right? > > > > If so, the entire interrupt numbering scheme appears broken. Conceptually it should > not make a difference where the interrupt is coming from. If the virq system > returns 0 for invalid (non-configured) interrupts, and non-configured 'real' > interrupts are reported as -ENXIO, all bets are off. How would a driver know > what to expect ? And how would one be expected to review such non-deterministic > code ? > I wouldn't know that much. I'm just pointing out that '0' doesn't seem to be a valid IRQ number in this context (i.e. virq space). > FWIW, platform_get_irq() does return -ENXIO for invalid interrupts. If there > is an independent notion of "0 is an invalid interrupt", it is well hidden. > Yes, AFAICS platform_get_irq() returns a negative error or a positive irq number. I fail to see how it's able return '0' (of course, I can be wrong). > Anyway, if you think the driver should treat 0 as invalid interrupt, go ahead. > Who am I to know. Just please don't use my Reviewed-by in this case. > Quite frankly not sure how to handle this best. A quick grep through the code doesn't help either: lots of drivers treat 0 as a valid interrupt and lots treat it as invalid. So I guess it doesn't really matter... Can someone shed a light on this?
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 0433ea8..c8ab377 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> @@ -111,10 +112,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)) { @@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev) if (!wdt_reg) return -ENOMEM; + 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 = request_irq(irq, orion_wdt_irq, 0, pdev->name, NULL); + if (ret < 0) { + dev_err(&pdev->dev, "failed to request IRQ\n"); + return ret; + } + } + wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk; orion_wdt.timeout = wdt_max_duration;
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 | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-)