Messages in this thread | | | Date | Wed, 12 Dec 2012 10:16:47 +0100 | From | Thomas Petazzoni <> | Subject | Re: [PATCH 3/4 v2] gpio/mvebu: convert to use irq_domain_add_simple() |
| |
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
| |