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

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

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