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

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

[Uwe Kleine-König]

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\x101 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/  |

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

[Arnd Bergmann]

On Monday 22 April 2013, Uwe Kleine-König wrote:
> 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);

While not technically any different, I would suggest writing the above
as (d->hwirq & 0x1f), which makes it clear to the reader that this is
intended to be a cheap bit mask operation rather than an expensive
division.
> 
> arch/arm/mach-s5p64x0/common.c
> 	-> uses irq_base\x101 for irq_alloc_generic_chip

I think there are plans to replace this code with
drivers/pinctrl/pinctrl-samsung.c, but I don't know if patches
exist already.

> arch/arm/plat-orion/gpio.c
> 	-> depends on how orion_gpio_of_init is called. No callers
> 	found.

As of f9e7592230b, this code has been replaced with a pinctrl driver
and can be killed.

> 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

Tomasz Figa has a patch set to remove this file, will likely get merged
in 3.11.

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

AFAICT this code is the same driver as arch/arm/mach-s5p64x0/common.c
and will meet the same fate.

> 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.

Right. I think irq_domain_add_simple() is the right interface to use
here.

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

I'm not sure I understand why this doesn't work for the ones that use
a base that isn't a multiple of 32. Since you are masking the hwirq
rather than the irq number, it will still be zero-based, won't it?

	Arnd

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

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 501 bytes --]

On Mon, 22 Apr 2013, Arnd Bergmann wrote:
> On Monday 22 April 2013, Uwe Kleine-König wrote:
> > For the uncommented files using %32 instead of -gc->irq_base should
> > work.
> 
> I'm not sure I understand why this doesn't work for the ones that use
> a base that isn't a multiple of 32. Since you are masking the hwirq
> rather than the irq number, it will still be zero-based, won't it?

The issue is that for anything which uses this w/o irq domains
d->hwirq is 0.

Thanks,

	tglx

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

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5821 bytes --]

On Mon, 22 Apr 2013, Uwe Kleine-König wrote:
> 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?

Shudder. We do not need new functions at all, we just need to think a
bit.

>  - 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.

Which is not correct, as hwirq is always 0 for !irqdomain users. And
no, we don't set it to irq.

Calculating the mask over and over is stupid to begin with. We can
precompute it and store it in irq_data->mask. For the existing generic
chip users we simply precompute it with 1 << (d->hwirq - gc->irq_base)
and let the irqdomain code calculate it for everything else.

Hmm?

Thanks,

	tglx
---

diff --git a/include/linux/irq.h b/include/linux/irq.h
index bc4e066..03fd64a 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -119,6 +119,7 @@ struct irq_domain;
 
 /**
  * struct irq_data - per irq and irq chip data passed down to chip functions
+ * @mask:		precomputed bitmask for accessing the chip registers
  * @irq:		interrupt number
  * @hwirq:		hardware interrupt number, local to the interrupt domain
  * @node:		node index useful for balancing
@@ -138,6 +139,7 @@ struct irq_domain;
  * irq_data.
  */
 struct irq_data {
+	u32			mask;
 	unsigned int		irq;
 	unsigned long		hwirq;
 	unsigned int		node;
@@ -700,10 +702,14 @@ struct irq_chip_generic {
  * @IRQ_GC_INIT_NESTED_LOCK:	Set the lock class of the irqs to nested for
  *				irq chips which need to call irq_set_wake() on
  *				the parent irq. Usually GPIO implementations
+ * @IRQ_GC_NO_MASK:		Do not calculate irq_data->mask
+ * @IRQ_GC_MASK_FROM_HWIRQ:	Calculate irq_data->mask from the hwirq number
  */
 enum irq_gc_flags {
 	IRQ_GC_INIT_MASK_CACHE		= 1 << 0,
 	IRQ_GC_INIT_NESTED_LOCK		= 1 << 1,
+	IRQ_GC_NO_MASK			= 1 << 2,
+	IRQ_GC_MASK_FROM_HWIRQ		= 1 << 4,
 };
 
 /* Generic chip callback functions */
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index c89295a..a013a35 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -39,7 +39,7 @@ void irq_gc_noop(struct irq_data *d)
 void irq_gc_mask_disable_reg(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->disable);
@@ -57,7 +57,7 @@ void irq_gc_mask_disable_reg(struct irq_data *d)
 void irq_gc_mask_set_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
 	gc->mask_cache |= mask;
@@ -75,7 +75,7 @@ void irq_gc_mask_set_bit(struct irq_data *d)
 void irq_gc_mask_clr_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
 	gc->mask_cache &= ~mask;
@@ -93,7 +93,7 @@ void irq_gc_mask_clr_bit(struct irq_data *d)
 void irq_gc_unmask_enable_reg(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->enable);
@@ -108,7 +108,7 @@ void irq_gc_unmask_enable_reg(struct irq_data *d)
 void irq_gc_ack_set_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
@@ -122,7 +122,7 @@ void irq_gc_ack_set_bit(struct irq_data *d)
 void irq_gc_ack_clr_bit(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = ~(1 << (d->irq - gc->irq_base));
+	u32 mask = ~d->mask;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack);
@@ -136,7 +136,7 @@ void irq_gc_ack_clr_bit(struct irq_data *d)
 void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->mask);
@@ -151,7 +151,7 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 void irq_gc_eoi(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
 	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->eoi);
@@ -169,7 +169,7 @@ void irq_gc_eoi(struct irq_data *d)
 int irq_gc_set_wake(struct irq_data *d, unsigned int on)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	u32 mask = 1 << (d->irq - gc->irq_base);
+	u32 mask = d->mask;
 
 	if (!(mask & gc->wake_enabled))
 		return -EINVAL;
@@ -254,6 +254,15 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
 		if (flags & IRQ_GC_INIT_NESTED_LOCK)
 			irq_set_lockdep_class(i, &irq_nested_lock_class);
 
+		if (!(flags & IRQ_GC_NO_MASK)) {
+			struct irq_data *d = irq_get_irq_data(i);
+			u32 mask;
+
+			if (flags & IRQ_GC_MASK_FROM_HWIRQ)
+				d->mask = 1 << (d->hwirq % 32);
+			else
+				d->mask = 1 << (i - gc->irq_base);
+		}
 		irq_set_chip_and_handler(i, &ct->chip, ct->handler);
 		irq_set_chip_data(i, gc);
 		irq_modify_status(i, clr, set);