[PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple()

[PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple()

[Linus Walleij]

The MVEBU driver probably just wants a few IRQs. Using the simple
domain has the upside of allocating IRQ descriptors if need be,
especially in a SPARSE_IRQ environment.

Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Keep irq_create_mapping() and do not replace with
  irq_find_mapping() - if a linear domain is the outcome,
  we really need to allocate a descriptor on the first mapping
  call.
---
 drivers/gpio/gpio-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 902af43..e0bde06 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -645,8 +645,8 @@ static int __devinit mvebu_gpio_probe(struct platform_device *pdev)
 			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
 
 	/* Setup irq domain on top of the generic chip. */
-	mvchip->domain = irq_domain_add_legacy(np, mvchip->chip.ngpio,
-					       mvchip->irqbase, 0,
+	mvchip->domain = irq_domain_add_simple(np, mvchip->chip.ngpio,
+					       mvchip->irqbase,
 					       &irq_domain_simple_ops,
 					       mvchip);
 	if (!mvchip->domain) {
-- 
1.7.11.7

Re: [PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple()

[Andrew Lunn]

On Fri, Oct 19, 2012 at 12:54:02PM +0200, Linus Walleij wrote:
> The MVEBU driver probably just wants a few IRQs. Using the simple
> domain has the upside of allocating IRQ descriptors if need be,
> especially in a SPARSE_IRQ environment.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Keep irq_create_mapping() and do not replace with
>   irq_find_mapping() - if a linear domain is the outcome,
>   we really need to allocate a descriptor on the first mapping
>   call.
> ---
>  drivers/gpio/gpio-mvebu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index 902af43..e0bde06 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -645,8 +645,8 @@ static int __devinit mvebu_gpio_probe(struct platform_device *pdev)
>  			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
>  
>  	/* Setup irq domain on top of the generic chip. */
> -	mvchip->domain = irq_domain_add_legacy(np, mvchip->chip.ngpio,
> -					       mvchip->irqbase, 0,
> +	mvchip->domain = irq_domain_add_simple(np, mvchip->chip.ngpio,
> +					       mvchip->irqbase,
>  					       &irq_domain_simple_ops,
>  					       mvchip);
>  	if (!mvchip->domain) {
> -- 
> 1.7.11.7
> 

Hi Linus

I tested this on a kirkwood QNAP NAS box. gpio-keys still work and
generate interrupts etc.

Tested-by: Andrew Lunn <andrew@lunn.ch>

	   Andrew

Re: [PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple()

[Grant Likely]

On Fri, 19 Oct 2012 12:54:02 +0200, Linus Walleij <linus.walleij@linaro.org> wrote:
> The MVEBU driver probably just wants a few IRQs. Using the simple
> domain has the upside of allocating IRQ descriptors if need be,
> especially in a SPARSE_IRQ environment.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Applied, thanks.

g.

Re: [PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple()

[Thomas Petazzoni]

Dear Linus Walleij,

On Fri, 19 Oct 2012 12:54:02 +0200, Linus Walleij wrote:
> The MVEBU driver probably just wants a few IRQs. Using the simple
> domain has the upside of allocating IRQ descriptors if need be,
> especially in a SPARSE_IRQ environment.

Unfortunately, this creates the following warning at boot time for each
GPIO bank:

------------[ cut here ]------------
WARNING: at /home/thomas/projets/linux-2.6/kernel/irq/irqdomain.c:181 irq_domain_add_simple+0x8c/0x9c()
Cannot allocate irq_descs @ IRQ33, assuming pre-allocated
Modules linked in:
[<c0014360>] (unwind_backtrace+0x0/0xf8) from [<c001d2b0>] (warn_slowpath_common+0x4c/0x64)
[<c001d2b0>] (warn_slowpath_common+0x4c/0x64) from [<c001d35c>] (warn_slowpath_fmt+0x30/0x40)
[<c001d35c>] (warn_slowpath_fmt+0x30/0x40) from [<c0065be4>] (irq_domain_add_simple+0x8c/0x9c)
[<c0065be4>] (irq_domain_add_simple+0x8c/0x9c) from [<c02641f8>] (mvebu_gpio_probe+0x3cc/0x49c)
[<c02641f8>] (mvebu_gpio_probe+0x3cc/0x49c) from [<c0185000>] (platform_drv_probe+0x18/0x1c)
[<c0185000>] (platform_drv_probe+0x18/0x1c) from [<c0183db4>] (driver_probe_device+0x70/0x1f0)
[<c0183db4>] (driver_probe_device+0x70/0x1f0) from [<c01826ac>] (bus_for_each_drv+0x5c/0x88)
[<c01826ac>] (bus_for_each_drv+0x5c/0x88) from [<c0183d1c>] (device_attach+0x74/0x80)
[<c0183d1c>] (device_attach+0x74/0x80) from [<c01833b4>] (bus_probe_device+0x84/0xa8)
[<c01833b4>] (bus_probe_device+0x84/0xa8) from [<c0181ce0>] (device_add+0x4ac/0x57c)
[<c0181ce0>] (device_add+0x4ac/0x57c) from [<c01dfa60>] (of_platform_device_create_pdata+0x5c/0x80)
[<c01dfa60>] (of_platform_device_create_pdata+0x5c/0x80) from [<c01dfb50>] (of_platform_bus_create+0xcc/0x270)
[<c01dfb50>] (of_platform_bus_create+0xcc/0x270) from [<c01dfbb0>] (of_platform_bus_create+0x12c/0x270)
[<c01dfbb0>] (of_platform_bus_create+0x12c/0x270) from [<c01dfd54>] (of_platform_populate+0x60/0x98)
[<c01dfd54>] (of_platform_populate+0x60/0x98) from [<c033c938>] (armada_370_xp_dt_init+0x18/0x24)
[<c033c938>] (armada_370_xp_dt_init+0x18/0x24) from [<c03383a8>] (customize_machine+0x1c/0x28)
[<c03383a8>] (customize_machine+0x1c/0x28) from [<c0008704>] (do_one_initcall+0x34/0x174)
[<c0008704>] (do_one_initcall+0x34/0x174) from [<c0262060>] (kernel_init+0x100/0x2b4)
[<c0262060>] (kernel_init+0x100/0x2b4) from [<c000df98>] (ret_from_fork+0x14/0x3c)
---[ end trace 1b75b31a2719ed1c ]---

Of course, the fix should be to remove the irq_alloc_descs() from the
driver prior to calling irq_domain_add_simple(). But the thing is that
our gpio-mvebu driver uses the
irq_alloc_generic_chip()/irq_setup_generic_chip() infrastructure, which
it seems requires a legacy IRQ domain (it needs the base IRQ number).

Or maybe there's a proper fix I'm missing?

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple()

[Linus Walleij]

On Tue, Dec 11, 2012 at 4:20 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> Unfortunately, this creates the following warning at boot time for each
> GPIO bank:

Grant has a patch in his irqdomain tree that will turn this warning into
a simple pr_info() thing instead. It's not that bad... The immediate
problem will soon go away.

> Of course, the fix should be to remove the irq_alloc_descs() from the
> driver prior to calling irq_domain_add_simple(). But the thing is that
> our gpio-mvebu driver uses the
> irq_alloc_generic_chip()/irq_setup_generic_chip() infrastructure, which
> it seems requires a legacy IRQ domain (it needs the base IRQ number).

Actually it looks like a core infrastructure issue. Sorry for not
spotting this in the first place:

First you allocate some descriptors, just any descriptors, with
mvchip->irqbase = irq_alloc_descs(-1, 0, ngpios, -1);

Then you allocate a generic chip using the obtained
descriptor base:
gc = irq_alloc_generic_chip("mvebu_gpio_irq", 2, mvchip->irqbase,
				    mvchip->membase, handle_level_irq);

Then you set up the generic chip with a nailed down IRQ base number
from step 1:
irq_setup_generic_chip(gc, IRQ_MSK(ngpios), 0,
			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);

This, if I understand it correctly, is done because you have two different
chip types on the generic chip: one for high/low level IRQ and another
for rising/falling. (Which is a very nice way to use the generic chip!)

Finally set up the IRQ domain:
mvchip->domain = irq_domain_add_simple(np, mvchip->chip.ngpio,
				       mvchip->irqbase,
				       &irq_domain_simple_ops,
				       mvchip);

So the problem is that you cannot allocate a generic chip
without having a base IRQ at that point, if I understand
correctly. If this was not necessary you would not need to
allocate descriptors in advance.

Or rather: the *real* problem, which will face anyone trying
to implement a combined edge+level IRQ chip in a driver,
is that the generic irqchip does not play well with irqdomain.

Using legacy in this case is clearly wrong, the generic IRQ chip
is not one least bit legacy, it's top-of-the-line IRQ handling,
using generic code as we want.

However it seems generic chips cannot handle sparse IRQs
at all, it requires the descriptors to be allocated before
we create and instance of it.

So I see two solutions:

- Fix the generic chip to handle sparse IRQs by patching
  a lot in kernel/irq/generic-chip.c and allowing to pass
  something like < 0 for irq base.

- Add something like irq_domain_add_generic() for
  generic chips and handle the oddities there.

The latter would be a pretty straight-forward wrapper to legacy
domain as of now.

Any preference? Or should we just consider generic irqchips
a legacy case?

Yours,
Linus Walleij

Re: [PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple()

[Thomas Petazzoni]

Dear Linus Walleij,

On Wed, 12 Dec 2012 08:56:03 +0100, Linus Walleij wrote:

> > Unfortunately, this creates the following warning at boot time for each
> > GPIO bank:
> 
> Grant has a patch in his irqdomain tree that will turn this warning into
> a simple pr_info() thing instead. It's not that bad... The immediate
> problem will soon go away.

Ok.

> > Of course, the fix should be to remove the irq_alloc_descs() from the
> > driver prior to calling irq_domain_add_simple(). But the thing is that
> > our gpio-mvebu driver uses the
> > irq_alloc_generic_chip()/irq_setup_generic_chip() infrastructure, which
> > it seems requires a legacy IRQ domain (it needs the base IRQ number).
> 
> Actually it looks like a core infrastructure issue. Sorry for not
> spotting this in the first place:
> 
> First you allocate some descriptors, just any descriptors, with
> mvchip->irqbase = irq_alloc_descs(-1, 0, ngpios, -1);
> 
> Then you allocate a generic chip using the obtained
> descriptor base:
> gc = irq_alloc_generic_chip("mvebu_gpio_irq", 2, mvchip->irqbase,
> 				    mvchip->membase, handle_level_irq);
> 
> Then you set up the generic chip with a nailed down IRQ base number
> from step 1:
> irq_setup_generic_chip(gc, IRQ_MSK(ngpios), 0,
> 			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
> 
> This, if I understand it correctly, is done because you have two different
> chip types on the generic chip: one for high/low level IRQ and another
> for rising/falling. (Which is a very nice way to use the generic chip!)
> 
> Finally set up the IRQ domain:
> mvchip->domain = irq_domain_add_simple(np, mvchip->chip.ngpio,
> 				       mvchip->irqbase,
> 				       &irq_domain_simple_ops,
> 				       mvchip);
> 
> So the problem is that you cannot allocate a generic chip
> without having a base IRQ at that point, if I understand
> correctly. If this was not necessary you would not need to
> allocate descriptors in advance.

Yes that's my understand as well.

> Or rather: the *real* problem, which will face anyone trying
> to implement a combined edge+level IRQ chip in a driver,
> is that the generic irqchip does not play well with irqdomain.
> 
> Using legacy in this case is clearly wrong, the generic IRQ chip
> is not one least bit legacy, it's top-of-the-line IRQ handling,
> using generic code as we want.
> 
> However it seems generic chips cannot handle sparse IRQs
> at all, it requires the descriptors to be allocated before
> we create and instance of it.
> 
> So I see two solutions:
> 
> - Fix the generic chip to handle sparse IRQs by patching
>   a lot in kernel/irq/generic-chip.c and allowing to pass
>   something like < 0 for irq base.
> 
> - Add something like irq_domain_add_generic() for
>   generic chips and handle the oddities there.
> 
> The latter would be a pretty straight-forward wrapper to legacy
> domain as of now.
> 
> Any preference? Or should we just consider generic irqchips
> a legacy case?

There has been some work in the past to make generic-chip play well
with irqdomain (by Rob Herring), but apparently, none of that work has
been merged. See:

Subject: [PATCH v6] irq: add irq_domain support to generic-chip
Date: Wed,  8 Feb 2012 16:55:22 -0600

Also at:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/083897.html

Rob, do you intend to push something like this further? What were the
blocking points?

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com