diff mbox

[v3,04/15] watchdog: orion: Handle IRQ

Message ID 1390310774-20781-5-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ezequiel Garcia Jan. 21, 2014, 1:26 p.m. UTC
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(-)

Comments

Guenter Roeck Jan. 21, 2014, 2:31 p.m. UTC | #1
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
Ezequiel Garcia Jan. 21, 2014, 2:35 p.m. UTC | #2
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!
Ezequiel Garcia Jan. 27, 2014, 6:30 a.m. UTC | #3
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?
Guenter Roeck Jan. 27, 2014, 6:42 a.m. UTC | #4
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
Ezequiel Garcia Jan. 27, 2014, 7:18 a.m. UTC | #5
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 mbox

Patch

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;