This interrupt controller is found on Cortex-M3 and Cortex-M4 machines.
Support for this controller appeared in Catalin's Cortex tree based on
2.6.33 but was nearly completely rewritten.
Signed-off-by: Catalin Marinas <[email protected]>
Signed-off-by: Uwe Kleine-König <[email protected]>
---
Notes:
Changes since v2, sent with
Message-id:[email protected]:
- drop superflous check for node != NULL in init function
- rework for linear irq domain
- drop seperate function for non-dt init
This patch triggers two checkpatch warnings:
WARNING: Avoid CamelCase: <nvic_do_IRQ>
WARNING: Avoid CamelCase: <handle_IRQ>
but I think they are OK for consistency?!
Moreover sparse tells me:
drivers/irqchip/irq-nvic.c:58:1: warning: symbol 'nvic_do_IRQ' was not declared. Should it be static?
nvic_do_IRQ is called from assembler only, so a declaration couldn't be
shared and I couldn't find a nice place for a declaration. Suggestions
welcome.
Thanks
Uwe
drivers/irqchip/Kconfig | 4 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-nvic.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 159 insertions(+)
create mode 100644 drivers/irqchip/irq-nvic.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index a350969..18657fd 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -10,6 +10,10 @@ config ARM_GIC
config GIC_NON_BANKED
bool
+config ARM_NVIC
+ bool
+ select IRQ_DOMAIN
+
config ARM_VIC
bool
select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 98e3b87..7227c5f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -7,5 +7,6 @@ obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi.o
obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o
obj-$(CONFIG_ARM_GIC) += irq-gic.o
+obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
obj-$(CONFIG_ARM_VIC) += irq-vic.o
obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o
diff --git a/drivers/irqchip/irq-nvic.c b/drivers/irqchip/irq-nvic.c
new file mode 100644
index 0000000..6e9ca8a
--- /dev/null
+++ b/drivers/irqchip/irq-nvic.c
@@ -0,0 +1,154 @@
+/*
+ * drivers/irq/irq-nvic.c
+ *
+ * Copyright (C) 2008 ARM Limited, All Rights Reserved.
+ * Copyright (C) 2013 Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Support for the Nested Vectored Interrupt Controller found on the
+ * ARMv7-M CPUs (Cortex-M3/M4)
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+
+#include <asm/v7m.h>
+#include <asm/exception.h>
+
+#include "irqchip.h"
+
+#define NVIC_ISER 0x000
+#define NVIC_ICER 0x080
+#define NVIC_IPR 0x300
+
+#define NVIC_MAX_BANKS 16
+/*
+ * Each bank handles 32 irqs. Only the 16th (= last) bank handles only
+ * 16 irqs.
+ */
+#define NVIC_MAX_IRQ ((NVIC_MAX_BANKS - 1) * 32 + 16)
+
+struct nvic_bank_data {
+ /*
+ * For irq i base holds nvic_base + 4 * i / 32. So you can access the
+ * right ISER register (i.e ISER[i / 32]) by just taking base + ISER.
+ * Ditto for ICER.
+ */
+ void __iomem *base;
+};
+
+static struct nvic_chip_data {
+ struct irq_domain *domain;
+ struct nvic_bank_data bdata[NVIC_MAX_BANKS];
+} nvic_chip_data;
+
+asmlinkage void __exception_irq_entry
+nvic_do_IRQ(irq_hw_number_t hwirq, struct pt_regs *regs)
+{
+ unsigned int irq = irq_linear_revmap(nvic_chip_data.domain, hwirq);
+
+ handle_IRQ(irq, regs);
+}
+
+static inline void __iomem *nvic_bank_base(struct irq_data *d)
+{
+ struct nvic_bank_data *bank_data = irq_data_get_irq_chip_data(d);
+ return bank_data->base;
+}
+
+static void nvic_mask_irq(struct irq_data *d)
+{
+ u32 mask = 1 << (d->hwirq % 32);
+
+ writel_relaxed(mask, nvic_bank_base(d) + NVIC_ICER);
+}
+
+static void nvic_unmask_irq(struct irq_data *d)
+{
+ u32 mask = 1 << (d->hwirq % 32);
+
+ writel_relaxed(mask, nvic_bank_base(d) + NVIC_ISER);
+}
+
+static void nvic_eoi(struct irq_data *d)
+{
+ /*
+ * This is a no-op as end of interrupt is signaled by the exception
+ * return sequence.
+ */
+}
+
+static struct irq_chip nvic_chip = {
+ .name = "NVIC",
+ .irq_mask = nvic_mask_irq,
+ .irq_unmask = nvic_unmask_irq,
+ .irq_eoi = nvic_eoi,
+};
+
+static int nvic_irq_domain_map(struct irq_domain *domain, unsigned int virq,
+ irq_hw_number_t hw)
+{
+ irq_set_chip_and_handler(virq, &nvic_chip,
+ handle_fasteoi_irq);
+ irq_set_chip_data(virq, nvic_chip_data.bdata + hw / 32);
+ set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
+
+ return 0;
+}
+
+static struct irq_domain_ops nvic_irq_domain_ops = {
+ .xlate = irq_domain_xlate_onetwocell,
+ .map = nvic_irq_domain_map,
+};
+
+static int __init nvic_of_init(struct device_node *node,
+ struct device_node *parent)
+{
+ void __iomem *nvic_base;
+ unsigned int irqs, i;
+ unsigned numbanks = (readl_relaxed(V7M_SCS_ICTR) &
+ V7M_SCS_ICTR_INTLINESNUM_MASK) + 1;
+
+ nvic_base = of_iomap(node, 0);
+ if (!nvic_base) {
+ pr_warn("unable to map nvic registers\n");
+ return -ENOMEM;
+ }
+
+ irqs = numbanks * 32;
+ if (irqs > NVIC_MAX_IRQ)
+ irqs = NVIC_MAX_IRQ;
+
+ for (i = 0; i < numbanks; ++i)
+ nvic_chip_data.bdata[i].base = nvic_base + 4 * i;
+
+ nvic_chip_data.domain =
+ irq_domain_add_linear(node, irqs, &nvic_irq_domain_ops, NULL);
+ if (!nvic_chip_data.domain) {
+ pr_warn("Failed to allocate irq domain\n");
+ return -ENOMEM;
+ }
+
+ /* Disable all interrupts */
+ for (i = 0; i < irqs; i += 32)
+ writel_relaxed(~0, nvic_base + NVIC_ICER + i * 4 / 32);
+
+ /* Set priority on all interrupts */
+ for (i = 0; i < irqs; i += 4)
+ writel_relaxed(0, nvic_base + NVIC_IPR + i);
+
+ return 0;
+}
+IRQCHIP_DECLARE(armv7m_nvic, "arm,armv7m-nvic", nvic_of_init);
--
1.8.2.rc3
On Wednesday 17 April 2013, Uwe Kleine-König wrote:
> This patch triggers two checkpatch warnings:
>
> WARNING: Avoid CamelCase: <nvic_do_IRQ>
> WARNING: Avoid CamelCase: <handle_IRQ>
>
> but I think they are OK for consistency?!
You obviously have no choice for handle_IRQ, but I think the common way to
name the first-level interrupt handler would be "nvic_handle_irq" here.
> Moreover sparse tells me:
>
> drivers/irqchip/irq-nvic.c:58:1: warning: symbol 'nvic_do_IRQ' was not declared. Should it be static?
>
> nvic_do_IRQ is called from assembler only, so a declaration couldn't be
> shared and I couldn't find a nice place for a declaration. Suggestions
> welcome.
Can't you make it static and call set_handle_irq() on it from the
probe function?
> + * Each bank handles 32 irqs. Only the 16th (= last) bank handles only
> + * 16 irqs.
> + */
> +#define NVIC_MAX_IRQ ((NVIC_MAX_BANKS - 1) * 32 + 16)
Is this actually inherent to the hardware design, or is the number of irqs
actually customizable? Also, why do you care about the maximum? You only
use it to check against the device tree provided value, but I suppose
you could just as well trust that property to be correct.
Arnd
On Wed, Apr 17, 2013 at 10:23:43PM +0200, Arnd Bergmann wrote:
> On Wednesday 17 April 2013, Uwe Kleine-K?nig wrote:
>
> > This patch triggers two checkpatch warnings:
> >
> > WARNING: Avoid CamelCase: <nvic_do_IRQ>
> > WARNING: Avoid CamelCase: <handle_IRQ>
> >
> > but I think they are OK for consistency?!
>
> You obviously have no choice for handle_IRQ, but I think the common way to
> name the first-level interrupt handler would be "nvic_handle_irq" here.
The function I called before is asm_do_IRQ which is another instance of
this naming scheme. But I agree that nvic_handle_irq is nicer and will
change to nvic_handle_irq in the next iteration.
> > Moreover sparse tells me:
> >
> > drivers/irqchip/irq-nvic.c:58:1: warning: symbol 'nvic_do_IRQ' was not declared. Should it be static?
> >
> > nvic_do_IRQ is called from assembler only, so a declaration couldn't be
> > shared and I couldn't find a nice place for a declaration. Suggestions
> > welcome.
>
> Can't you make it static and call set_handle_irq() on it from the
> probe function?
Yeah that works. Then nvic_handle_irq needs to determine the irq itself
which is currently done in the entry code.
> > + * Each bank handles 32 irqs. Only the 16th (= last) bank handles only
> > + * 16 irqs.
> > + */
> > +#define NVIC_MAX_IRQ ((NVIC_MAX_BANKS - 1) * 32 + 16)
>
> Is this actually inherent to the hardware design, or is the number of irqs
> actually customizable? Also, why do you care about the maximum? You only
> use it to check against the device tree provided value, but I suppose
> you could just as well trust that property to be correct.
I don't provide a value for the number of irqs in the device tree. There
is only the value INTLINESNUM in the V7M_SCS_ICTR register that is used
to determine the number of interrupt banks.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thursday 18 April 2013, Uwe Kleine-K?nig wrote:
> > > + * Each bank handles 32 irqs. Only the 16th (= last) bank handles only
> > > + * 16 irqs.
> > > + */
> > > +#define NVIC_MAX_IRQ ((NVIC_MAX_BANKS - 1) * 32 + 16)
> >
> > Is this actually inherent to the hardware design, or is the number of irqs
> > actually customizable? Also, why do you care about the maximum? You only
> > use it to check against the device tree provided value, but I suppose
> > you could just as well trust that property to be correct.
> I don't provide a value for the number of irqs in the device tree. There
> is only the value INTLINESNUM in the V7M_SCS_ICTR register that is used
> to determine the number of interrupt banks.
Ah, right. But do you have any reason to believe it could be wrong?
Arnd
On Thu, Apr 18, 2013 at 11:01:13AM +0200, Arnd Bergmann wrote:
> On Thursday 18 April 2013, Uwe Kleine-K?nig wrote:
> > > > + * Each bank handles 32 irqs. Only the 16th (= last) bank handles only
> > > > + * 16 irqs.
> > > > + */
> > > > +#define NVIC_MAX_IRQ ((NVIC_MAX_BANKS - 1) * 32 + 16)
> > >
> > > Is this actually inherent to the hardware design, or is the number of irqs
> > > actually customizable? Also, why do you care about the maximum? You only
> > > use it to check against the device tree provided value, but I suppose
> > > you could just as well trust that property to be correct.
> > I don't provide a value for the number of irqs in the device tree. There
> > is only the value INTLINESNUM in the V7M_SCS_ICTR register that is used
> > to determine the number of interrupt banks.
>
> Ah, right. But do you have any reason to believe it could be wrong?
No it's just that the mapping isn't linear in the end.
INTLINESNUM | number of irqs
0 | 32
1 | 64
2 | 96
3 | 128
4 | 160
5 | 192
6 | 224
7 | 256
8 | 288
9 | 320
10 | 352
11 | 384
12 | 416
13 | 448
14 | 480
15 | 496
That is, there are (INTLINESNUM + 1) * 32 irqs for INTLINESNUM < 15. For
INTLINESNUM == 15 there are only 496 and not 16 * 32 == 512. That's the
same on the gic (just with bigger numbers).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, 17 Apr 2013, Uwe Kleine-K?nig wrote:
> +struct nvic_bank_data {
> + /*
> + * For irq i base holds nvic_base + 4 * i / 32. So you can access the
> + * right ISER register (i.e ISER[i / 32]) by just taking base + ISER.
> + * Ditto for ICER.
> + */
> + void __iomem *base;
> +};
What's the point of a struct with a single member? Why not having an
array of base pointers ?
> +static struct nvic_chip_data {
> + struct irq_domain *domain;
> + struct nvic_bank_data bdata[NVIC_MAX_BANKS];
> +} nvic_chip_data;
> +
> +asmlinkage void __exception_irq_entry
> +nvic_do_IRQ(irq_hw_number_t hwirq, struct pt_regs *regs)
> +{
> + unsigned int irq = irq_linear_revmap(nvic_chip_data.domain, hwirq);
> +
> + handle_IRQ(irq, regs);
> +}
> +
> +static inline void __iomem *nvic_bank_base(struct irq_data *d)
> +{
> + struct nvic_bank_data *bank_data = irq_data_get_irq_chip_data(d);
> + return bank_data->base;
> +}
> +
> +static void nvic_mask_irq(struct irq_data *d)
> +{
> + u32 mask = 1 << (d->hwirq % 32);
> +
> + writel_relaxed(mask, nvic_bank_base(d) + NVIC_ICER);
> +}
> +
> +static void nvic_unmask_irq(struct irq_data *d)
> +{
> + u32 mask = 1 << (d->hwirq % 32);
> +
> + writel_relaxed(mask, nvic_bank_base(d) + NVIC_ISER);
> +}
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.
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.
Thanks,
tglx
On Thursday 18 April 2013, Uwe Kleine-K?nig wrote:
> That is, there are (INTLINESNUM + 1) * 32 irqs for INTLINESNUM < 15. For
> INTLINESNUM == 15 there are only 496 and not 16 * 32 == 512. That's the
> same on the gic (just with bigger numbers).
Ok, but since you are now using a linear domain, it doesn't actually hurt
to register 512 in that special case, right?
Arnd
On Thu, Apr 18, 2013 at 11:38:13AM +0200, Arnd Bergmann wrote:
> On Thursday 18 April 2013, Uwe Kleine-K?nig wrote:
> > That is, there are (INTLINESNUM + 1) * 32 irqs for INTLINESNUM < 15. For
> > INTLINESNUM == 15 there are only 496 and not 16 * 32 == 512. That's the
> > same on the gic (just with bigger numbers).
>
> Ok, but since you are now using a linear domain, it doesn't actually hurt
> to register 512 in that special case, right?
Well, it depends if allocating space for 16 unused unsigned ints hurts
(maybe not). And it makes mapping some irqs successfull while the irq
doesn't really exist. But probably this doesn't hurt either because the
problem already exists.
I don't care much. Is there another advantage beside saving a few source
lines/instructions?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Friday 19 April 2013, Uwe Kleine-K?nig wrote:
> On Thu, Apr 18, 2013 at 11:38:13AM +0200, Arnd Bergmann wrote:
> > On Thursday 18 April 2013, Uwe Kleine-K?nig wrote:
> > > That is, there are (INTLINESNUM + 1) * 32 irqs for INTLINESNUM < 15. For
> > > INTLINESNUM == 15 there are only 496 and not 16 * 32 == 512. That's the
> > > same on the gic (just with bigger numbers).
> >
> > Ok, but since you are now using a linear domain, it doesn't actually hurt
> > to register 512 in that special case, right?
> Well, it depends if allocating space for 16 unused unsigned ints hurts
> (maybe not).
As long as SPARSE_IRQ is enabled, the space won't actually be allocated.
> And it makes mapping some irqs successfull while the irq
> doesn't really exist. But probably this doesn't hurt either because the
> problem already exists.
>
> I don't care much. Is there another advantage beside saving a few source
> lines/instructions?
It just feels strange to read the value from hardware and then override
it anyway. But I agree it's not important either way.
Arnd
On Thu, Apr 18, 2013 at 11:35:22AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2013, Uwe Kleine-K?nig wrote:
> > +struct nvic_bank_data {
> > + /*
> > + * For irq i base holds nvic_base + 4 * i / 32. So you can access the
> > + * right ISER register (i.e ISER[i / 32]) by just taking base + ISER.
> > + * Ditto for ICER.
> > + */
> > + void __iomem *base;
> > +};
>
> What's the point of a struct with a single member? Why not having an
> array of base pointers ?
It gives a name to that single member and maybe makes future changes
easier. Obviously you could argue ...
When switching to generic irq chip this struct probably goes away, so I
suggest to postpone this discussion.
> > +static struct nvic_chip_data {
> > + struct irq_domain *domain;
> > + struct nvic_bank_data bdata[NVIC_MAX_BANKS];
> > +} nvic_chip_data;
> > +
> > +asmlinkage void __exception_irq_entry
> > +nvic_do_IRQ(irq_hw_number_t hwirq, struct pt_regs *regs)
> > +{
> > + unsigned int irq = irq_linear_revmap(nvic_chip_data.domain, hwirq);
> > +
> > + handle_IRQ(irq, regs);
> > +}
> > +
> > +static inline void __iomem *nvic_bank_base(struct irq_data *d)
> > +{
> > + struct nvic_bank_data *bank_data = irq_data_get_irq_chip_data(d);
> > + return bank_data->base;
> > +}
> > +
> > +static void nvic_mask_irq(struct irq_data *d)
> > +{
> > + u32 mask = 1 << (d->hwirq % 32);
> > +
> > + writel_relaxed(mask, nvic_bank_base(d) + NVIC_ICER);
> > +}
> > +
> > +static void nvic_unmask_irq(struct irq_data *d)
> > +{
> > + u32 mask = 1 << (d->hwirq % 32);
> > +
> > + writel_relaxed(mask, nvic_bank_base(d) + NVIC_ISER);
> > +}
>
> 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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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/ |
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=101 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
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
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);