Patchwork irqchip: Add support for ARMv7-M's NVIC

login
register
mail settings
Submitter Uwe Kleine-König
Date March 12, 2013, 3:54 p.m.
Message ID <1363103673-24720-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/227067/
State New
Headers show

Comments

Uwe Kleine-König - March 12, 2013, 3:54 p.m.
From: Catalin Marinas <catalin.marinas@arm.com>

This interrupt controller is found on Cortex-M3 and Cortex-M4 machines.

[ukleinek: drop locking, switch to fasteoi handler, add irqdomain
and dt support, move to drivers/irq]
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/irqchip/Kconfig    |   4 ++
 drivers/irqchip/Makefile   |   1 +
 drivers/irqchip/irq-nvic.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+)
 create mode 100644 drivers/irqchip/irq-nvic.c
Russell King - ARM Linux - March 12, 2013, 4:01 p.m.
On Tue, Mar 12, 2013 at 04:54:33PM +0100, Uwe Kleine-König wrote:
> +#include <asm/irq.h>
> +#include <asm/io.h>

linux/io.h

> +	unsigned int irqs, i, irq_base;
> +
> +	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> +	if (IS_ERR_VALUE(irq_base)) {

Erm... irq_alloc_descs() returns a negative number on error.

	if ((int)irq_base < 0)

or make irq_base an int, and use:

	if (irq_base < 0)
Uwe Kleine-König - March 12, 2013, 7:27 p.m.
On Tue, Mar 12, 2013 at 04:01:01PM +0000, Russell King - ARM Linux wrote:
> On Tue, Mar 12, 2013 at 04:54:33PM +0100, Uwe Kleine-König wrote:
> > +#include <asm/irq.h>
> > +#include <asm/io.h>
> 
> linux/io.h
> 
> > +	unsigned int irqs, i, irq_base;
> > +
> > +	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> > +	if (IS_ERR_VALUE(irq_base)) {
> 
> Erm... irq_alloc_descs() returns a negative number on error.
> 
> 	if ((int)irq_base < 0)
> 
> or make irq_base an int, and use:
> 
> 	if (irq_base < 0)
Just for me: So the check using IS_ERR_VALUE is as wrong as the other
occurences in arch/arm that you just kicked out or is it just ugly?

I thought about it and went the "make irq_base int" route (and even
fixed the asm includes) even before I sent my patch. Just failed to pass
the right hash to format-patch after rebasing :-|

Best regards
Uwe
Thomas Gleixner - March 12, 2013, 7:57 p.m.
On Tue, 12 Mar 2013, Uwe Kleine-König wrote:
> +static struct nvic_chip_data nvic_data __read_mostly;

What the heck is this? You have a static struct which you set in
irqdata.chip_data?

> +static inline void __iomem *nvic_dist_base(struct irq_data *d)
> +{
> +	struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d);

And then you do the dance of reading the pointer to that static struct
out of irq_data and dereferencing it?

What's the point of this? 

> +	return nvic_data->dist_base;
> +}
> +
> +static void nvic_mask_irq(struct irq_data *d)
> +{
> +	u32 mask = 1 << (d->hwirq % 32);
> +
> +	writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4);

You're really missing the point of irq_data.chip_data 

If you set proper irq chip data per bank then this whole stuff is
reduced to:

   struct mydata *md = irq_data_get_irq_chip_data(d);
   u32 mask = 1 << (d->irq - md->irq_base);

   writel_relaxed(mask, md->iobase + NVIC_ICER);

Can you spot the difference and what that means in terms of
instruction cycles?

> +}
> +
> +static void nvic_unmask_irq(struct irq_data *d)
> +{
> +	u32 mask = 1 << (d->hwirq % 32);
> +
> +	writel_relaxed(mask, nvic_dist_base(d) + NVIC_ISER + d->hwirq / 32 * 4);
> +}
> +
> +void nvic_eoi(struct irq_data *d)

static ?

> +{
> +	/*
> +	 * 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 void __init nvic_init_bases(struct device_node *node,
> +		void __iomem *dist_base)

Please make this

static void __init nvic_init_bases(struct device_node *node,
                                   void __iomem *dist_base)

That's way easier to parse. Applies to all other multiline stuff as
well.

> +{
> +	unsigned int irqs, i, irq_base;
> +
> +	nvic_data.dist_base = dist_base;
> +
> +	irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32;
> +	if (irqs > 496)
> +		irqs = 496;

Magic constants. Please use proper defines. 

Aside of that without a proper comment the logic looks ass backwards.

> +	irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32;

Without looking at the datasheet I assume that the chip tells you the
number of interrupt banks where a result of 0 means 1 bank and so
forth.

How should a reviewer understand why the number of banks is limited to
31 without a comment? Do you really expect that a reviewer who
stumbles over that goes to search for the datasheet and verify what
you hacked up?

> +	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());

  irq_alloc_desc_from(16, irqs - 16, ...)

Also why are you allocating irqs-16 instead of the full number of
irqs?

> +	if (IS_ERR_VALUE(irq_base)) {

See Russells reply

> +		WARN(1, "Cannot allocate irq_descs\n");

What's the point of that warn? The call path is always the same. So
you are spamming dmesg with a pointless backtrace. And on top of that
you do:

> +		irq_base = 16;

So you cannot allocate irq_descs and then you set base to 16 and pray
that everything works?

> +	}
> +	nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0,
> +			&irq_domain_simple_ops, NULL);
> +	if (WARN_ON(!nvic_data.domain))
> +		return;

See above. Also you are leaking irqdescs though it's questionable
whether the machine can continue at all. And of course the init call
itself will return sucess.

> +	/*
> +	 * Set priority on all interrupts.
> +	 */
> +	for (i = 0; i < irqs; i += 4)
> +		writel_relaxed(0, dist_base + NVIC_IPRI + i);
> +
> +	/*
> +	 * Disable all interrupts
> +	 */
> +	for (i = 0; i < irqs; i += 32)
> +		writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32);
> +
> +	/*
> +	 * Setup the Linux IRQ subsystem.
> +	 */
> +	for (i = 0; i < irqs; i++) {
> +		irq_set_chip_and_handler(irq_base + i, &nvic_chip,
> +				handle_fasteoi_irq);

Above you do:
	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());

So you allocate irqs-16 interrupt descriptors and then you initialize
16 more than you allocated.

Brilliant stuff that.

> +		irq_set_chip_data(irq_base + i, &nvic_data);
> +		set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE);
> +	}
> +}
> +
> +static int __init nvic_of_init(struct device_node *node,
> +		struct device_node *parent)
> +{
> +	void __iomem *dist_base;
> +
> +	if (WARN_ON(!node))

Sigh.

Though the real question is: can this actually happen?

> +		return -ENODEV;
> +
> +	dist_base = of_iomap(node, 0);
> +	WARN(!dist_base, "unable to map nvic dist registers\n");

Brilliant. You can't map stuff and then you continue just for fun or
what?

> +	nvic_init_bases(node, dist_base);

Great. You have failure pathes in nvic_init_bases() and then you
return unconditionally success:

> +	return 0;
> +}

Thanks,

	tglx
Russell King - ARM Linux - March 12, 2013, 8:13 p.m.
On Tue, Mar 12, 2013 at 08:27:02PM +0100, Uwe Kleine-König wrote:
> On Tue, Mar 12, 2013 at 04:01:01PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Mar 12, 2013 at 04:54:33PM +0100, Uwe Kleine-König wrote:
> > > +#include <asm/irq.h>
> > > +#include <asm/io.h>
> > 
> > linux/io.h
> > 
> > > +	unsigned int irqs, i, irq_base;
> > > +
> > > +	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> > > +	if (IS_ERR_VALUE(irq_base)) {
> > 
> > Erm... irq_alloc_descs() returns a negative number on error.
> > 
> > 	if ((int)irq_base < 0)
> > 
> > or make irq_base an int, and use:
> > 
> > 	if (irq_base < 0)
> Just for me: So the check using IS_ERR_VALUE is as wrong as the other
> occurences in arch/arm that you just kicked out or is it just ugly?

See my recent patch removing all but one.

What we're suffering from here is a mentality problem - one which seems
to be basically this:

	If a macro exists which looks like it does the job I need,
	I must use it.  I won't look at the function and check its
	range of values that it returns, I'll just use it and hope
	it's the right thing.

The IS_ERR_VALUE() patch and my IS_ERR_OR_NULL() patches, I've spent on
each one less than a minute, greping, reading the function, checking its
range of return values, sometimes longer if I need to look at other
functions, and worked out what the valid range of return values are.

However, the general pattern in the kernel is this:

	For any function that returns an int, values of success will
	be positive.  Values indicating errors will be negative.

There are very few int-returning functions which violate that.  There
is one big, well known exception, and that's in the mmap() stuff,
where there's a need to return valid values in the range (0..TASK_SIZE)
but differentiate them from -ve errnos.  This is where IS_ERR_VALUE()
came from, and why it was created.  See 07ab67c8d0d7c (Fix
get_unmapped_area sanity tests).


Today, it seems that IS_ERR_VALUE() is now being used just as a subsitute
for testing for < 0... and it needs to stop.  See above - unless there's
a *good* reason, treat +ve values as success, -ve values as failure from
functions returning int.  Always design functions in the kernel like that.
Again - unless there's a *good* reason like needing to return 0..TASK_SIZE.
Uwe Kleine-König - March 12, 2013, 8:47 p.m.
Hello Thomas,

On Tue, Mar 12, 2013 at 08:57:34PM +0100, Thomas Gleixner wrote:
> On Tue, 12 Mar 2013, Uwe Kleine-König wrote:
> > +static struct nvic_chip_data nvic_data __read_mostly;
> 
> What the heck is this? You have a static struct which you set in
> irqdata.chip_data?
I copied that idea from the gic driver and didn't question it because I
thought it's too early to allocate memory when it's needed. Or do you
just wonder about the usage of this static variable?

> > +static inline void __iomem *nvic_dist_base(struct irq_data *d)
> > +{
> > +	struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d);
> 
> And then you do the dance of reading the pointer to that static struct
> out of irq_data and dereferencing it?
> 
> What's the point of this? 
The idea was to keep the functions generic anyhow.
 
> > +	return nvic_data->dist_base;
> > +}
> > +
> > +static void nvic_mask_irq(struct irq_data *d)
> > +{
> > +	u32 mask = 1 << (d->hwirq % 32);
> > +
> > +	writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4);
> 
> You're really missing the point of irq_data.chip_data 
> 
> If you set proper irq chip data per bank then this whole stuff is
> reduced to:
> 
>    struct mydata *md = irq_data_get_irq_chip_data(d);
>    u32 mask = 1 << (d->irq - md->irq_base);
> 
>    writel_relaxed(mask, md->iobase + NVIC_ICER);
> 
> Can you spot the difference and what that means in terms of
> instruction cycles?
Yeah I see. The cost is increased memory usage. You'd probably say that
the amount needed here is too small to care about. Still many decisions
like this sum up and make the 4 MiB of RAM I have a tight fit.
 
> > +}
> > +
> > +static void nvic_unmask_irq(struct irq_data *d)
> > +{
> > +	u32 mask = 1 << (d->hwirq % 32);
> > +
> > +	writel_relaxed(mask, nvic_dist_base(d) + NVIC_ISER + d->hwirq / 32 * 4);
> > +}
> > +
> > +void nvic_eoi(struct irq_data *d)
> 
> static ?
yes.

> > +{
> > +	/*
> > +	 * 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 void __init nvic_init_bases(struct device_node *node,
> > +		void __iomem *dist_base)
> 
> Please make this
> 
> static void __init nvic_init_bases(struct device_node *node,
>                                    void __iomem *dist_base)
> 
> That's way easier to parse. Applies to all other multiline stuff as
> well.
My version is like vim does the layout for me. It's the first time
someone opposes to it. The reason I prefer using a fixed indention is
that I don't need to touch the latter lines when for example the
function name or the function's type change.

Hmm, I can fix that if you insist.

> > +{
> > +	unsigned int irqs, i, irq_base;
> > +
> > +	nvic_data.dist_base = dist_base;
> > +
> > +	irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32;
> > +	if (irqs > 496)
> > +		irqs = 496;
> 
> Magic constants. Please use proper defines. 
> 
> Aside of that without a proper comment the logic looks ass backwards.
Ok.

> > +	irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32;
> 
> Without looking at the datasheet I assume that the chip tells you the
> number of interrupt banks where a result of 0 means 1 bank and so
> forth.
> 
> How should a reviewer understand why the number of banks is limited to
> 31 without a comment? Do you really expect that a reviewer who
> stumbles over that goes to search for the datasheet and verify what
> you hacked up?
Will add a comment.

> > +	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> 
>   irq_alloc_desc_from(16, irqs - 16, ...)
> 
> Also why are you allocating irqs-16 instead of the full number of
> irqs?
I already have a comment there in my tree.

> > +	if (IS_ERR_VALUE(irq_base)) {
> 
> See Russells reply
> 
> > +		WARN(1, "Cannot allocate irq_descs\n");
> 
> What's the point of that warn? The call path is always the same. So
> you are spamming dmesg with a pointless backtrace. And on top of that
> you do:
There is one warning per call to nvic_init_bases. So I don't expect more
than one message in dmesg.

> 
> > +		irq_base = 16;
> 
> So you cannot allocate irq_descs and then you set base to 16 and pray
> that everything works?
If something goes wrong here the machine is probably silent about it. So
continuing after a prayer might (or might not?) be an option.

> > +	}
> > +	nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0,
> > +			&irq_domain_simple_ops, NULL);
> > +	if (WARN_ON(!nvic_data.domain))
> > +		return;
> 
> See above. Also you are leaking irqdescs though it's questionable
> whether the machine can continue at all. And of course the init call
> itself will return sucess.
> 
> > +	/*
> > +	 * Set priority on all interrupts.
> > +	 */
> > +	for (i = 0; i < irqs; i += 4)
> > +		writel_relaxed(0, dist_base + NVIC_IPRI + i);
> > +
> > +	/*
> > +	 * Disable all interrupts
> > +	 */
> > +	for (i = 0; i < irqs; i += 32)
> > +		writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32);
> > +
> > +	/*
> > +	 * Setup the Linux IRQ subsystem.
> > +	 */
> > +	for (i = 0; i < irqs; i++) {
> > +		irq_set_chip_and_handler(irq_base + i, &nvic_chip,
> > +				handle_fasteoi_irq);
> 
> Above you do:
> 	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> 
> So you allocate irqs-16 interrupt descriptors and then you initialize
> 16 more than you allocated.
right.

> Brilliant stuff that.
> 
> > +		irq_set_chip_data(irq_base + i, &nvic_data);
> > +		set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE);
> > +	}
> > +}
> > +
> > +static int __init nvic_of_init(struct device_node *node,
> > +		struct device_node *parent)
> > +{
> > +	void __iomem *dist_base;
> > +
> > +	if (WARN_ON(!node))
> 
> Sigh.
> 
> Though the real question is: can this actually happen?
It didn't happen for me. What do you suggest? dropping WARN_ON?

> > +		return -ENODEV;
> > +
> > +	dist_base = of_iomap(node, 0);
> > +	WARN(!dist_base, "unable to map nvic dist registers\n");
> 
> Brilliant. You can't map stuff and then you continue just for fun or
> what?
What do you suggest? returning -ESOMETHING?
> 
> > +	nvic_init_bases(node, dist_base);
> 
> Great. You have failure pathes in nvic_init_bases() and then you
> return unconditionally success:
Most of your critics also apply to irq-gic.c. I will follow up with a
patch for that when you are happy with my work for the nvic.

Best regards and thanks for your feed-back,
Uwe
Thomas Gleixner - March 12, 2013, 9:40 p.m.
On Tue, 12 Mar 2013, Uwe Kleine-König wrote:

> Hello Thomas,
> 
> On Tue, Mar 12, 2013 at 08:57:34PM +0100, Thomas Gleixner wrote:
> > On Tue, 12 Mar 2013, Uwe Kleine-König wrote:
> > > +static struct nvic_chip_data nvic_data __read_mostly;
> > 
> > What the heck is this? You have a static struct which you set in
> > irqdata.chip_data?
> I copied that idea from the gic driver and didn't question it because I
> thought it's too early to allocate memory when it's needed. Or do you
> just wonder about the usage of this static variable?

Right, copying stuff from some other file is always a great excuse for
disabling the brain while coding.

Why the heck do you think it's safe to call irq_alloc_descs() from
that code then? Just because it did not explode in your face?

> > > +static inline void __iomem *nvic_dist_base(struct irq_data *d)
> > > +{
> > > +	struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d);
> > 
> > And then you do the dance of reading the pointer to that static struct
> > out of irq_data and dereferencing it?
> > 
> > What's the point of this? 
> The idea was to keep the functions generic anyhow.

Generic waste or what? If you dereference a static variable indirectly
via five indirections that makes the code obvious and the function
generic. I see ... NOT
  
> > > +	return nvic_data->dist_base;
> > > +}
> > > +
> > > +static void nvic_mask_irq(struct irq_data *d)
> > > +{
> > > +	u32 mask = 1 << (d->hwirq % 32);
> > > +
> > > +	writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4);
> > 
> > You're really missing the point of irq_data.chip_data 
> > 
> > If you set proper irq chip data per bank then this whole stuff is
> > reduced to:
> > 
> >    struct mydata *md = irq_data_get_irq_chip_data(d);
> >    u32 mask = 1 << (d->irq - md->irq_base);
> > 
> >    writel_relaxed(mask, md->iobase + NVIC_ICER);
> > 
> > Can you spot the difference and what that means in terms of
> > instruction cycles?
> Yeah I see. The cost is increased memory usage. You'd probably say that
> the amount needed here is too small to care about. Still many decisions
> like this sum up and make the 4 MiB of RAM I have a tight fit.

You did not answer my question completely.

You buy less memory usage for an increased instruction foot print
along with modulo/multiply/divide complexity in the interrupt hot
path.
  
> > > +static void __init nvic_init_bases(struct device_node *node,
> > > +		void __iomem *dist_base)
> > 
> > Please make this
> > 
> > static void __init nvic_init_bases(struct device_node *node,
> >                                    void __iomem *dist_base)
> > 
> > That's way easier to parse. Applies to all other multiline stuff as
> > well.
> My version is like vim does the layout for me. It's the first time
> someone opposes to it. The reason I prefer using a fixed indention is
> that I don't need to touch the latter lines when for example the
> function name or the function's type change.

I don't care about vim and your preferences. I care about the
readability and that's key for reviewing patch and reading code. Can
you spot the difference of:

static void __init nvic_init_bases(struct device_node *node,
	void __iomem *dist_base)
and

static void __init nvic_init_bases(struct device_node *node,
                                   void __iomem *dist_base)

Or

		irq_set_chip_and_handler(irq_base + i, &nvic_chip,
				handle_fasteoi_irq);
and

		irq_set_chip_and_handler(irq_base + i, &nvic_chip,
				         handle_fasteoi_irq);

The alignment of the arguments after the opening bracket makes it
entirely clear while your vim/lazyness style forces the reader to
decode it separately.
 
> Hmm, I can fix that if you insist.

You can Hmm as long as you want, except if you provide me an argument
why your magic vim setting is superiour.

> > > +	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> > 
> >   irq_alloc_desc_from(16, irqs - 16, ...)
> > 
> > Also why are you allocating irqs-16 instead of the full number of
> > irqs?
> I already have a comment there in my tree.

Brilliant answer. I ask you a question and you tell me that you have a
comment in your tree ????

Stop that nonsense, I don't care about your magic tree, but I care
about answers to a review question.
 
> > > +	if (IS_ERR_VALUE(irq_base)) {
> > 
> > See Russells reply
> > 
> > > +		WARN(1, "Cannot allocate irq_descs\n");
> > 
> > What's the point of that warn? The call path is always the same. So
> > you are spamming dmesg with a pointless backtrace. And on top of that
> > you do:
> There is one warning per call to nvic_init_bases. So I don't expect more
> than one message in dmesg.

You're missing the point again. It does not matter whether you expect
one or more. The point is, the call chain is known already. So why is
dumping a stack trace usefull? It's entirely sufficient to have a
pr_warn() or whatever, simply because this is the important
information which might scroll out of the observers window with a
stack trace which is completely useless. A stack trace is only
helpfull when the code in question can be called from a gazillion of
call sites. If the call chain is clear, it's pointless.
 
> > 
> > > +		irq_base = 16;
> > 
> > So you cannot allocate irq_descs and then you set base to 16 and pray
> > that everything works?
> If something goes wrong here the machine is probably silent about it. So
> continuing after a prayer might (or might not?) be an option.

What are you smoking? Either the machine can work with the
preallocated 16 irqs and allow you to retrieve additional debugging
info or it does not. It's that easy.
 
> > > +	}
> > > +	nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0,
> > > +			&irq_domain_simple_ops, NULL);
> > > +	if (WARN_ON(!nvic_data.domain))
> > > +		return;
> > 
> > See above. Also you are leaking irqdescs though it's questionable
> > whether the machine can continue at all. And of course the init call
> > itself will return sucess.

-ENOANSWER

> > > +	/*
> > > +	 * Set priority on all interrupts.
> > > +	 */
> > > +	for (i = 0; i < irqs; i += 4)
> > > +		writel_relaxed(0, dist_base + NVIC_IPRI + i);
> > > +
> > > +	/*
> > > +	 * Disable all interrupts
> > > +	 */
> > > +	for (i = 0; i < irqs; i += 32)
> > > +		writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32);
> > > +
> > > +	/*
> > > +	 * Setup the Linux IRQ subsystem.
> > > +	 */
> > > +	for (i = 0; i < irqs; i++) {
> > > +		irq_set_chip_and_handler(irq_base + i, &nvic_chip,
> > > +				handle_fasteoi_irq);
> > 
> > Above you do:
> > 	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> > 
> > So you allocate irqs-16 interrupt descriptors and then you initialize
> > 16 more than you allocated.
> right.
> 
> > Brilliant stuff that.

-ENOANSWER
 
> > > +		irq_set_chip_data(irq_base + i, &nvic_data);
> > > +		set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE);
> > > +	}
> > > +}
> > > +
> > > +static int __init nvic_of_init(struct device_node *node,
> > > +		struct device_node *parent)
> > > +{
> > > +	void __iomem *dist_base;
> > > +
> > > +	if (WARN_ON(!node))
> > 
> > Sigh.
> > 
> > Though the real question is: can this actually happen?
> It didn't happen for me.

Great argument for writing silly code.

> What do you suggest? dropping WARN_ON?

Did you actually try to understand any of my review questions?

> > > +		return -ENODEV;
> > > +
> > > +	dist_base = of_iomap(node, 0);
> > > +	WARN(!dist_base, "unable to map nvic dist registers\n");
> > 
> > Brilliant. You can't map stuff and then you continue just for fun or
> > what?
> What do you suggest? returning -ESOMETHING?

-EMORON perhaps?

Come on, do I need to make any further suggestions? See above, either
the machine can survive the failure or it cannot.
 
> > > +	nvic_init_bases(node, dist_base);
> > 
> > Great. You have failure pathes in nvic_init_bases() and then you
> > return unconditionally success:

-ENOANSWER

> Most of your critics also apply to irq-gic.c.

That's completely irrelevant.

> I will follow up with a patch for that when you are happy with my
> work for the nvic.

Do you really think that copying crappy code, making that copied code
worse and on top of that nerve racking the reviewer makes you the
person of choice to fixup the initial crap ?

Thanks,

	tglx

Patch

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..ddfb3d8
--- /dev/null
+++ b/drivers/irqchip/irq-nvic.c
@@ -0,0 +1,136 @@ 
+/*
+ * 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)
+ */
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/compiler.h>
+#include <linux/smp.h>
+#include <linux/export.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/irqdomain.h>
+
+#include <asm/irq.h>
+#include <asm/io.h>
+#include <asm/mach/irq.h>
+
+#include "irqchip.h"
+
+#define NVIC_INTR_CTRL			(0x004)
+#define NVIC_ISER			(0x100)
+#define NVIC_ICER			(0x180)
+#define NVIC_IPRI			(0x400)
+
+struct nvic_chip_data {
+	void __iomem *dist_base;
+	struct irq_domain *domain;
+};
+
+static struct nvic_chip_data nvic_data __read_mostly;
+
+static inline void __iomem *nvic_dist_base(struct irq_data *d)
+{
+	struct nvic_chip_data *nvic_data = irq_data_get_irq_chip_data(d);
+	return nvic_data->dist_base;
+}
+
+static void nvic_mask_irq(struct irq_data *d)
+{
+	u32 mask = 1 << (d->hwirq % 32);
+
+	writel_relaxed(mask, nvic_dist_base(d) + NVIC_ICER + d->irq / 32 * 4);
+}
+
+static void nvic_unmask_irq(struct irq_data *d)
+{
+	u32 mask = 1 << (d->hwirq % 32);
+
+	writel_relaxed(mask, nvic_dist_base(d) + NVIC_ISER + d->hwirq / 32 * 4);
+}
+
+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 void __init nvic_init_bases(struct device_node *node,
+		void __iomem *dist_base)
+{
+	unsigned int irqs, i, irq_base;
+
+	nvic_data.dist_base = dist_base;
+
+	irqs = ((readl_relaxed(dist_base + NVIC_INTR_CTRL) & 0x0f) + 1) * 32;
+	if (irqs > 496)
+		irqs = 496;
+
+	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
+	if (IS_ERR_VALUE(irq_base)) {
+		WARN(1, "Cannot allocate irq_descs\n");
+		irq_base = 16;
+	}
+	nvic_data.domain = irq_domain_add_legacy(node, irqs - 16, irq_base, 0,
+			&irq_domain_simple_ops, NULL);
+	if (WARN_ON(!nvic_data.domain))
+		return;
+
+	/*
+	 * Set priority on all interrupts.
+	 */
+	for (i = 0; i < irqs; i += 4)
+		writel_relaxed(0, dist_base + NVIC_IPRI + i);
+
+	/*
+	 * Disable all interrupts
+	 */
+	for (i = 0; i < irqs; i += 32)
+		writel_relaxed(~0, dist_base + NVIC_ICER + i * 4 / 32);
+
+	/*
+	 * Setup the Linux IRQ subsystem.
+	 */
+	for (i = 0; i < irqs; i++) {
+		irq_set_chip_and_handler(irq_base + i, &nvic_chip,
+				handle_fasteoi_irq);
+		irq_set_chip_data(irq_base + i, &nvic_data);
+		set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE);
+	}
+}
+
+static int __init nvic_of_init(struct device_node *node,
+		struct device_node *parent)
+{
+	void __iomem *dist_base;
+
+	if (WARN_ON(!node))
+		return -ENODEV;
+
+	dist_base = of_iomap(node, 0);
+	WARN(!dist_base, "unable to map nvic dist registers\n");
+
+	nvic_init_bases(node, dist_base);
+
+	return 0;
+}
+IRQCHIP_DECLARE(cortex_m3_nvic, "arm,cortex-m3-nvic", nvic_of_init);