DM9000_IRQ_FLAGS

DM9000_IRQ_FLAGS

[Daniel Mack]

Hi,

on Toradex' Colibri, a PXA270 based board with a DM9000 ethernet
controller, this driver won't work due to unsuitable DM9000_IRQ_FLAGS.
If I understood the code behind request_irq() correctly, it's not
recommended to register an IRQ without any of the IRQT_* flags set.

Is there any concerns about applying the patch below?

Best regards,
Daniel

diff --git a/drivers/net/dm9000.c b/drivers/net/dm9000.c
index 3286d2a..6a20a54 100644
--- a/drivers/net/dm9000.c
+++ b/drivers/net/dm9000.c
@@ -66,6 +66,7 @@
 #include <linux/dm9000.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
+#include <linux/irq.h>
 
 #include <asm/delay.h>
 #include <asm/irq.h>
@@ -113,7 +114,7 @@
 #define writesl	outsl
 #define DM9000_IRQ_FLAGS	(IRQF_SHARED | IRQF_TRIGGER_HIGH)
 #else
-#define DM9000_IRQ_FLAGS	IRQF_SHARED
+#define DM9000_IRQ_FLAGS	(IRQF_SHARED | IRQT_RISING)
 #endif
 
 /*

Re: DM9000_IRQ_FLAGS

[Remy Bohmer]

Hello Daniel,

This controller is also used on many other boards, like the e.g. Atmel
AT91sam9261-ek board. On that board on both the rising _and_ falling
edge an interrupt is generated.

I can test tomorrow if this patch leaves this board in tact, but
should the board-specific code not add this flag if it is required ?
By modifying this driver you will interfere the behavior of other
boards, and I do not know if there any level triggered types used.


Kind Regards,

Remy Bohmer


2007/12/11, Daniel Mack <daniel@caiaq.de>:
> Hi,
>
> on Toradex' Colibri, a PXA270 based board with a DM9000 ethernet
> controller, this driver won't work due to unsuitable DM9000_IRQ_FLAGS.
> If I understood the code behind request_irq() correctly, it's not
> recommended to register an IRQ without any of the IRQT_* flags set.
>
> Is there any concerns about applying the patch below?
>
> Best regards,
> Daniel
>
> diff --git a/drivers/net/dm9000.c b/drivers/net/dm9000.c
> index 3286d2a..6a20a54 100644
> --- a/drivers/net/dm9000.c
> +++ b/drivers/net/dm9000.c
> @@ -66,6 +66,7 @@
>  #include <linux/dm9000.h>
>  #include <linux/delay.h>
>  #include <linux/platform_device.h>
> +#include <linux/irq.h>
>
>  #include <asm/delay.h>
>  #include <asm/irq.h>
> @@ -113,7 +114,7 @@
>  #define writesl        outsl
>  #define DM9000_IRQ_FLAGS       (IRQF_SHARED | IRQF_TRIGGER_HIGH)
>  #else
> -#define DM9000_IRQ_FLAGS       IRQF_SHARED
> +#define DM9000_IRQ_FLAGS       (IRQF_SHARED | IRQT_RISING)
>  #endif
>
>  /*
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: DM9000_IRQ_FLAGS

[Daniel Mack]

Hi Remy,

On Tue, Dec 11, 2007 at 09:31:03PM +0100, Remy Bohmer wrote:
> This controller is also used on many other boards, like the e.g. Atmel
> AT91sam9261-ek board. On that board on both the rising _and_ falling
> edge an interrupt is generated.

However, request_irq() is called with IRQF_SHARED only, so neither
IRQT_RISING nor IRQT_FALLING is set and the value defaults to
IRQT_NOEDGE. How can you get IRQs?

> I can test tomorrow if this patch leaves this board in tact, but
> should the board-specific code not add this flag if it is required ?
> By modifying this driver you will interfere the behavior of other
> boards, and I do not know if there any level triggered types used.

Actually, the best way to go is to let the platform resources flags 
decide about that with something like

	resource->flags = IORESOURCE_IRQ | IRQT_RISING;

but the dm9000 does not care about them at all. Changing that would also
imply modifications to all board support code.

Regards,
Daniel

Re: DM9000_IRQ_FLAGS

[Remy Bohmer]

Hello Daniel,

> However, request_irq() is called with IRQF_SHARED only, so neither
> IRQT_RISING nor IRQT_FALLING is set and the value defaults to
> IRQT_NOEDGE. How can you get IRQs?

The DM9000 adapter is connected to a GPIO line on the at91sam9261-ek
board, the board-specific code has a generic handler for every GPIO
interrupt. So, the Device driver __does not need to know__ the type of
interrupt, It just installs a handler by request_irq().

So, these flags are dependant on the board where the driver is
installed, and thus everything you want to define fixed in the driver
is not generic and thus wrong.
This is why a NACK your patch.

> Actually, the best way to go is to let the platform resources flags
> decide about that with something like
>
>         resource->flags = IORESOURCE_IRQ | IRQT_RISING;
>
These types of flags are never meant to be compatible, and thus they
shall not be stored in the same flags variable. The same is valid for
the flags IRQF_* and IRQT* flags, so request_irq() is also not the
place to do this.

You should look at the routine set_irq_type() in kernel/irq/chip.c.
This routine is supposed to be used for the IRQT_RISING type of flags
Just do a grep of IRQT_RISING in the arch/arm tree and you will find
enough examples.

Kind Regards,

Remy

Re: DM9000_IRQ_FLAGS

[Daniel Mack]

Hi,

On Dec 12, 2007, at 3:46 PM, Remy Bohmer wrote:

> The DM9000 adapter is connected to a GPIO line on the at91sam9261-ek
> board, the board-specific code has a generic handler for every GPIO
> interrupt. So, the Device driver __does not need to know__ the type of
> interrupt, It just installs a handler by request_irq().

Ok.

>>         resource->flags = IORESOURCE_IRQ | IRQT_RISING;
>>
> These types of flags are never meant to be compatible, and thus they
> shall not be stored in the same flags variable. The same is valid for
> the flags IRQF_* and IRQT* flags, so request_irq() is also not the
> place to do this.

Ah, so I was trapped by faulty code doing that. I thought as the
type of interrupt is also a detail information about the resource,
this is the place to put is. However, it also works with
set_irq_type().

Thanks for sorting that out :)

Best regards,
Daniel

Re: DM9000_IRQ_FLAGS

[Ben Dooks]

On Tue, Dec 11, 2007 at 08:18:23PM +0100, Daniel Mack wrote:
> Hi,
> 
> on Toradex' Colibri, a PXA270 based board with a DM9000 ethernet
> controller, this driver won't work due to unsuitable DM9000_IRQ_FLAGS.
> If I understood the code behind request_irq() correctly, it's not
> recommended to register an IRQ without any of the IRQT_* flags set.
> 
> Is there any concerns about applying the patch below?

Yes, that will possibly break all systems using level-triggered
interrupts.

Probably the best solution is to pass the data via the platform
information being fed to the device.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: DM9000_IRQ_FLAGS

[Ben Dooks]

On Wed, Dec 12, 2007 at 02:41:53PM +0100, Daniel Mack wrote:
> Hi Remy,
> 
> On Tue, Dec 11, 2007 at 09:31:03PM +0100, Remy Bohmer wrote:
> > This controller is also used on many other boards, like the e.g. Atmel
> > AT91sam9261-ek board. On that board on both the rising _and_ falling
> > edge an interrupt is generated.
> 
> However, request_irq() is called with IRQF_SHARED only, so neither
> IRQT_RISING nor IRQT_FALLING is set and the value defaults to
> IRQT_NOEDGE. How can you get IRQs?
> 
> > I can test tomorrow if this patch leaves this board in tact, but
> > should the board-specific code not add this flag if it is required ?
> > By modifying this driver you will interfere the behavior of other
> > boards, and I do not know if there any level triggered types used.
> 
> Actually, the best way to go is to let the platform resources flags 
> decide about that with something like
> 
> 	resource->flags = IORESOURCE_IRQ | IRQT_RISING;
> 
> but the dm9000 does not care about them at all. Changing that would also
> imply modifications to all board support code.

I did have a go at trying to get people to pass the information this
way, but it seem to be ignored last time I sent it. I can dig out the
code that converts resource->flags to IRQT_ flags.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: DM9000_IRQ_FLAGS

[Remy Bohmer]

Hello Ben,

> >
> > Actually, the best way to go is to let the platform resources flags
> > decide about that with something like
> >
> >       resource->flags = IORESOURCE_IRQ | IRQT_RISING;
> >
> > but the dm9000 does not care about them at all. Changing that would also
> > imply modifications to all board support code.
>
> I did have a go at trying to get people to pass the information this
> way, but it seem to be ignored last time I sent it. I can dig out the
> code that converts resource->flags to IRQT_ flags.

I thought this issue was already solved by using set_irq_type() in the
BSP, just like all the other boards do...

Kind Regards,

Remy

Re: DM9000_IRQ_FLAGS

[Daniel Mack]

On Thu, Dec 20, 2007 at 10:54:34AM +0100, Remy Bohmer wrote:
> > I did have a go at trying to get people to pass the information this
> > way, but it seem to be ignored last time I sent it. I can dig out the
> > code that converts resource->flags to IRQT_ flags.
> 
> I thought this issue was already solved by using set_irq_type() in the
> BSP, just like all the other boards do...

Yes, exactly. I was trapped by another board file which specifies
interrupt types using 'struct resources'->flags for that
(arch/arm/mach-pxa/trizeps4.c) which is, as you pointed out earlier, not
the way to go. 

Sorry for the noise.

Best regards,
Daniel