Re: [PATCH v3] irqchip: Add support for ARMv7-M's NVIC

Uwe Kleine-König
2013-04-22T10:02:50Z

Hello,

(for the new readers of this thread: This is about using

u32 mask = 1 << (d->hwirq % 32);

instead of

u32 mask = 1 << (d->irq - gc->irq_base);

in the callbacks for the irq generic chip.)

On Fri, Apr 19, 2013 at 05:09:27PM +0200, Uwe Kleine-König wrote:
> On Thu, Apr 18, 2013 at 11:35:22AM +0200, Thomas Gleixner wrote:
> > On Wed, 17 Apr 2013, Uwe Kleine-König wrote:
> > How is that different from what the generic irq chip implementation
> > does? The only difference is that mask is generated by d->hwirq and
> > not by d->irq. And due to the fact, that you use a full linear mapping
> > between hwirq and virq the generic code simply works.
> I'm not sure what you mean when you say "full linear mapping". AFAICT
> using irq_domain_add_linear doesn't imply that two consecutive hardware
> irq numbers get consecutive Linux irq numbers, so using d->irq won't work.
>
> > Even if it would not work, it would be trivial to extend the generic
> > chip with that functionality instead of hacking another slightly
> > different copy of the same thing.
> I will try that and report back.
I wonder if using hwirq % 32 should work everywhere where now d->irq -
gc->irqbase is used. Depending on d->hwirq and not d->irq has the upside
of working with non-legacy irq domains, too.

Looking at next-20130419 the affected functions
(irq_gc_mask_disable_reg, irq_gc_mask_set_bit, irq_gc_mask_clr_bit,
irq_gc_unmask_enable_reg, irq_gc_ack_set_bit, irq_gc_ack_clr_bit,
irq_gc_mask_disable_reg_and_ack, irq_gc_eoi, irq_gc_set_wake) are used
in:

arch/arm/mach-davinci/irq.c
arch/arm/mach-imx/avic.c
arch/arm/mach-imx/tzic.c
arch/arm/mach-omap2/irq.c
arch/arm/mach-omap2/prm_common.c

arch/arm/mach-s5p64x0/common.c
-> uses irq_base=101 for irq_alloc_generic_chip
arch/arm/plat-orion/gpio.c
-> depends on how orion_gpio_of_init is called. No callers
found.

arch/arm/plat-orion/irq.c
arch/arm/plat-samsung/irq-vic-timer.c
-> used for a single irq that isn't a multiple of 32

arch/arm/plat-samsung/s5p-irq-gpioint.c
-> would need % 8?

arch/mips/jz4740/gpio.c
-> JZ4740_IRQ_GPIO(0) != JZ4740_IRQ_GPIO0 ?
-> uses 56 + i * 32 as irqbase

arch/mips/jz4740/irq.c
-> uses 8 as irqbase

arch/sh/boards/mach-se/7343/irq.c
-> uses irq_base = irq_linear_revmap(se7343_irq_domain, 0) where
se7343_irq_domain is a linear domain.
AFAICT this is a bug. (After adding the domain they map all irqs
in increasing order which currently seems to guarantee that it
works. But IMHO it should use a legacy domain.)

arch/sh/boards/mach-se/7722/irq.c
as above.

drivers/gpio/gpio-mxc.c
drivers/gpio/gpio-mxs.c
drivers/gpio/gpio-omap.c
drivers/gpio/gpio-sodaville.c
drivers/irqchip/irq-sirfsoc.c
drivers/mfd/jz4740-adc.c
-> uses platform_get_irq(pdev, 1) as irq_base for 5 irqs.

For the uncommented files using %32 instead of -gc->irq_base should
work.

So it seems I cannot just substitute how the mask is called.

The options I see are:

- introduce a new set of functions
Do you have a nice naming scheme?
irq_gc_unmask_enable_reg_hwirqmod32? Or should I rename the existing
ones to irq_gc_unmask_enable_reg_irqbaseoffset?
- use
u32 mask = 1 << (d->hwirq - gc->irq_base) % 32;
This is ugly but might work assuming irq_base == 0 for chips with irq
domain support and hwirq == irq for the others.

I'm not lucky with the options, so I'm looking forward to suggestions.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |