diff mbox

[v2,02/10] gpio: pxa: avoid to use global irq base

Message ID 511CADE4.1000007@compulab.co.il (mailing list archive)
State New, archived
Headers show

Commit Message

Igor Grinberg Feb. 14, 2013, 9:27 a.m. UTC 
On 02/13/13 16:55, Haojian Zhuang wrote:
> On 13 February 2013 22:18, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> On 02/03/13 12:15, Haojian Zhuang wrote:
>>> Avoid to use global irq_base in gpio-pxa driver. Define irq_base in each
>>> pxa_gpio_chip instead. Then we can avoid to use macro PXA_GPIO_TO_IRQ() &
>>> MMP_GPIO_TO_IRQ().
>>>
>>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>>
>> Ok. This patch breaks the NFS root on my PXA based systems.
>> I still haven't found the cause of the breakage.
>>
> 
> It's so strange. I tested it OK on pxa910 without DT. Could you help
> to check whether your GPIO interrupt still works?

It looks like I've figured this out...
For em-x270 as an example, if I move the IRQ resource assignment to runtime:


The Ethernet is alive and NFS root works fine.

So my conclusion, is that we still need to have some work done
before we can switch to using IRQ_DOMAIN.
As you can see from above patch, we at least must deal with the
PXA_GPIO_TO_IRQ macros and alike that have compile time assumptions
which obviously get broken once you switch to the IRQ_DOMAIN.

What do you think?

Comments

Linus Walleij Feb. 14, 2013, 12:19 p.m. UTC | #1 
On Thu, Feb 14, 2013 at 10:27 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:

> It looks like I've figured this out...
> For em-x270 as an example, if I move the IRQ resource assignment to runtime:
>
> diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
> index 1b64114..178cc0b 100644
> --- a/arch/arm/mach-pxa/em-x270.c
> +++ b/arch/arm/mach-pxa/em-x270.c
> @@ -210,8 +210,6 @@ static struct resource em_x270_dm9000_resource[] = {
>                 .flags = IORESOURCE_MEM,
>         },
>         [2] = {
> -               .start = EM_X270_ETHIRQ,
> -               .end   = EM_X270_ETHIRQ,
>                 .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
>         }
>  };
> @@ -232,6 +230,9 @@ static struct platform_device em_x270_dm9000 = {
>
>  static void __init em_x270_init_dm9000(void)
>  {
> +       em_x270_dm9000_resource[2].start = gpio_to_irq(GPIO41_ETHIRQ);
> +       em_x270_dm9000_resource[2].end = gpio_to_irq(GPIO41_ETHIRQ);
> +
>         em_x270_dm9000_platdata.flags |= dm9000_flags;
>         platform_device_register(&em_x270_dm9000);
>  }
>
> The Ethernet is alive and NFS root works fine.
>
> So my conclusion, is that we still need to have some work done
> before we can switch to using IRQ_DOMAIN.
> As you can see from above patch, we at least must deal with the
> PXA_GPIO_TO_IRQ macros and alike that have compile time assumptions
> which obviously get broken once you switch to the IRQ_DOMAIN.
>
> What do you think?

I think it seems like you should review all the IRQ assignments in that
former GPIO range. Statically encoding IRQ numbers for GPIO things
is usually not a good idea, it's better to always use gpio_to_irq()
on these and only keep hard-coded GPIO numbers around (atleast
just *one* problem to worry about).

Your,
Linus Walleij
Igor Grinberg Feb. 14, 2013, 12:45 p.m. UTC | #2 
On 02/14/13 14:19, Linus Walleij wrote:
> On Thu, Feb 14, 2013 at 10:27 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> 
>> It looks like I've figured this out...
>> For em-x270 as an example, if I move the IRQ resource assignment to runtime:
>>
>> diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
>> index 1b64114..178cc0b 100644
>> --- a/arch/arm/mach-pxa/em-x270.c
>> +++ b/arch/arm/mach-pxa/em-x270.c
>> @@ -210,8 +210,6 @@ static struct resource em_x270_dm9000_resource[] = {
>>                 .flags = IORESOURCE_MEM,
>>         },
>>         [2] = {
>> -               .start = EM_X270_ETHIRQ,
>> -               .end   = EM_X270_ETHIRQ,
>>                 .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
>>         }
>>  };
>> @@ -232,6 +230,9 @@ static struct platform_device em_x270_dm9000 = {
>>
>>  static void __init em_x270_init_dm9000(void)
>>  {
>> +       em_x270_dm9000_resource[2].start = gpio_to_irq(GPIO41_ETHIRQ);
>> +       em_x270_dm9000_resource[2].end = gpio_to_irq(GPIO41_ETHIRQ);
>> +
>>         em_x270_dm9000_platdata.flags |= dm9000_flags;
>>         platform_device_register(&em_x270_dm9000);
>>  }
>>
>> The Ethernet is alive and NFS root works fine.
>>
>> So my conclusion, is that we still need to have some work done
>> before we can switch to using IRQ_DOMAIN.
>> As you can see from above patch, we at least must deal with the
>> PXA_GPIO_TO_IRQ macros and alike that have compile time assumptions
>> which obviously get broken once you switch to the IRQ_DOMAIN.
>>
>> What do you think?
> 
> I think it seems like you should review all the IRQ assignments in that
> former GPIO range. Statically encoding IRQ numbers for GPIO things
> is usually not a good idea, it's better to always use gpio_to_irq()
> on these and only keep hard-coded GPIO numbers around (atleast
> just *one* problem to worry about).

Well, it always worked like this...
Now it is clear that we cannot continue doing it
(at least not with current IRQ_DOMAIN).

In order to merge this patch, we need to either remove
the static PXA_GPIO_TO_IRQ macros and alike, or teach the IRQ_DOMAIN
somehow to know about these.
I don't know what from the above is easier to do, as I did not looked
deeper into this.

I might have some time to look into this in two weeks from now,
but I guess it will be too late as Haojian wants this patch set
to go into 3.9.
Linus Walleij Feb. 14, 2013, 3:34 p.m. UTC | #3 
On Thu, Feb 14, 2013 at 1:45 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:

> In order to merge this patch, we need to either remove
> the static PXA_GPIO_TO_IRQ macros and alike, or teach the IRQ_DOMAIN
> somehow to know about these.
> I don't know what from the above is easier to do, as I did not looked
> deeper into this.

I think it's quite easy to get the irqdomain in place and working
correctly. It's intended to solve exactly this kind of thing.

> I might have some time to look into this in two weeks from now,
> but I guess it will be too late as Haojian wants this patch set
> to go into 3.9.

This patch as outlined by me in the previous comments, with a
complete working irqdomain is welcome if you can get it tested.

Yours,
Linus Walleij
Haojian Zhuang Feb. 17, 2013, 2:54 p.m. UTC | #4 
On 14 February 2013 20:45, Igor Grinberg <grinberg@compulab.co.il> wrote:
> On 02/14/13 14:19, Linus Walleij wrote:
>> On Thu, Feb 14, 2013 at 10:27 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>
>>> It looks like I've figured this out...
>>> For em-x270 as an example, if I move the IRQ resource assignment to runtime:
>>>
>>> diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
>>> index 1b64114..178cc0b 100644
>>> --- a/arch/arm/mach-pxa/em-x270.c
>>> +++ b/arch/arm/mach-pxa/em-x270.c
>>> @@ -210,8 +210,6 @@ static struct resource em_x270_dm9000_resource[] = {
>>>                 .flags = IORESOURCE_MEM,
>>>         },
>>>         [2] = {
>>> -               .start = EM_X270_ETHIRQ,
>>> -               .end   = EM_X270_ETHIRQ,
>>>                 .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
>>>         }
>>>  };
>>> @@ -232,6 +230,9 @@ static struct platform_device em_x270_dm9000 = {
>>>
>>>  static void __init em_x270_init_dm9000(void)
>>>  {
>>> +       em_x270_dm9000_resource[2].start = gpio_to_irq(GPIO41_ETHIRQ);
>>> +       em_x270_dm9000_resource[2].end = gpio_to_irq(GPIO41_ETHIRQ);
>>> +
>>>         em_x270_dm9000_platdata.flags |= dm9000_flags;
>>>         platform_device_register(&em_x270_dm9000);
>>>  }
>>>
>>> The Ethernet is alive and NFS root works fine.
>>>
>>> So my conclusion, is that we still need to have some work done
>>> before we can switch to using IRQ_DOMAIN.
>>> As you can see from above patch, we at least must deal with the
>>> PXA_GPIO_TO_IRQ macros and alike that have compile time assumptions
>>> which obviously get broken once you switch to the IRQ_DOMAIN.
>>>
>>> What do you think?
>>
>> I think it seems like you should review all the IRQ assignments in that
>> former GPIO range. Statically encoding IRQ numbers for GPIO things
>> is usually not a good idea, it's better to always use gpio_to_irq()
>> on these and only keep hard-coded GPIO numbers around (atleast
>> just *one* problem to worry about).
>
> Well, it always worked like this...
> Now it is clear that we cannot continue doing it
> (at least not with current IRQ_DOMAIN).
>
> In order to merge this patch, we need to either remove
> the static PXA_GPIO_TO_IRQ macros and alike, or teach the IRQ_DOMAIN
> somehow to know about these.
> I don't know what from the above is easier to do, as I did not looked
> deeper into this.
>
> I might have some time to look into this in two weeks from now,
> but I guess it will be too late as Haojian wants this patch set
> to go into 3.9.
>
>
> --
> Regards,
> Igor.

I can append pdata->irq_base to resolve this issue. While I'm
finished, I'll send it
out.

I don't want to change too much files in pxa directory. If I have enough time, I
would put resource on supporting DT & mutliplatform on pxa. So I'll
fix this issue.

Thanks
Haojian
diff mbox

Patch

diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
index 1b64114..178cc0b 100644
--- a/arch/arm/mach-pxa/em-x270.c
+++ b/arch/arm/mach-pxa/em-x270.c
@@ -210,8 +210,6 @@  static struct resource em_x270_dm9000_resource[] = {
 		.flags = IORESOURCE_MEM,
 	},
 	[2] = {
-		.start = EM_X270_ETHIRQ,
-		.end   = EM_X270_ETHIRQ,
 		.flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
 	}
 };
@@ -232,6 +230,9 @@  static struct platform_device em_x270_dm9000 = {
 
 static void __init em_x270_init_dm9000(void)
 {
+	em_x270_dm9000_resource[2].start = gpio_to_irq(GPIO41_ETHIRQ);
+	em_x270_dm9000_resource[2].end = gpio_to_irq(GPIO41_ETHIRQ);
+
 	em_x270_dm9000_platdata.flags |= dm9000_flags;
 	platform_device_register(&em_x270_dm9000);
 }