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

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

[Uwe Kleine-König]

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

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);
-- 
1.8.2.rc2

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

[Uwe Kleine-König]

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

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);
-- 
1.8.2.rc2

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

[Russell King - ARM Linux]

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)

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

[Russell King - ARM Linux]

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)

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

[Uwe Kleine-König]

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

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

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

[Uwe Kleine-König]

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

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

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

[Thomas Gleixner]

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

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

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

[Thomas Gleixner]

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

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

[Russell King - ARM Linux]

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.

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

[Russell King - ARM Linux]

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.

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

[Uwe Kleine-König]

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

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

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

[Uwe Kleine-König]

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

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

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

[Thomas Gleixner]

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

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] irqchip: Add support for ARMv7-M's NVIC

[Thomas Gleixner]

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 v2] irqchip: Add support for ARMv7-M's NVIC

[Uwe Kleine-König]

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 <catalin.marinas@arm.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

changes since v1:

 - fixed base address to match documentation
   So reading ICTR (which isn't in the nvic's address range) uses a new #define
   in asm/v7m.h which isn't in mainline yet. That's ugly but I don't have a
   better idea how to solve it without platform code.
 - s/NVIC_IPRI/NVIC_IPR/ to match documentation
 - use pr_warn instead of WARN/WARN_ON
 - do proper error handling, don't use IS_ERR_VALUE
 - drop the wrong skipping of the 16 system exceptions. They are not counted in
   ICTR_INTLINESNUM.
 - dynamically allocate chip data
 - drop the irq_domain* member from chip data as it's only used in the probe
   callback
 - change compatible string to arm,armv7m-nvic
 - drop a few unused #includes, use some linux/ #includes instead of asm/ ones
 - change indention to please tglx' eyes

A failure to probe the nvic makes the machine unresponsive. Does this 
have any implications on how the driver should behave when something
goes wrong? Another issue is that up to now the exception handling
simply calls asm_do_IRQ(16, ..) for the first nvic interrupt. So there
is a mismatch if irq_alloc_descs_from(16, ...) doesn't return 16. I
added error handling for that assuming that's fine for now, but in the long run
a better fix would be nice. What is the preferred approach to fix that? Use a
global variable that holds the irq_base? Or should I use a mapping function
instead?

 drivers/irqchip/Kconfig    |   4 ++
 drivers/irqchip/Makefile   |   1 +
 drivers/irqchip/irq-nvic.c | 176 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 181 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..721c328
--- /dev/null
+++ b/drivers/irqchip/irq-nvic.c
@@ -0,0 +1,176 @@
+/*
+ * 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 "irqchip.h"
+
+#define NVIC_ISER			0x000
+#define NVIC_ICER			0x080
+#define NVIC_IPR			0x300
+
+/*
+ * Each bank handles 32 irqs. Only the 16th (= last) bank handles only
+ * 16 irqs yielding a maximum of 15 * 32 + 16 = 496 interrupts.
+ */
+#define NVIC_MAX_IRQ		496
+
+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 inline void __iomem *nvic_bank_base(struct irq_data *d)
+{
+	struct nvic_bank_data *data = irq_data_get_irq_chip_data(d);
+	return 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 __init nvic_init_bases(struct device_node *node,
+				  void __iomem *nvic_base)
+{
+	unsigned int irqs, i;
+	int irq_base, ret = -ENOMEM;
+	struct irq_domain *irq_domain;
+	struct nvic_bank_data *bank_data;
+	unsigned numbanks = (readl_relaxed(V7M_SCS_ICTR) &
+			     V7M_SCS_ICTR_INTLINESNUM_MASK) + 1;
+
+	irqs = numbanks * 32;
+	if (irqs > NVIC_MAX_IRQ)
+		irqs = NVIC_MAX_IRQ;
+
+	bank_data = kzalloc(sizeof(*bank_data) * numbanks, GFP_KERNEL);
+	if (!bank_data) {
+		pr_warn("Failed to allocate chip data");
+		goto err_alloc_bank_data;
+	}
+	for (i = 0; i < numbanks; ++i)
+		bank_data[i].base = nvic_base + 4 * i;
+
+	irq_base = irq_alloc_descs_from(16, irqs, numa_node_id());
+	if (irq_base < 0) {
+		pr_warn("Cannot allocate irq_descs\n");
+		ret = irq_base;
+		goto err_irq_alloc_descs;
+	}
+	if (irq_base != 16) {
+		/*
+		 * The entry code just passes the exception number (i.e. irq
+		 * number + 16) to asm_do_IRQ, so the offset needs to be fixed
+		 * here.
+		 */
+		pr_warn("Failed to allocate irq_descs at offset 16\n");
+		goto err_wrong_irq_base;
+	}
+
+	irq_domain = irq_domain_add_legacy(node, irqs, irq_base, 0,
+					   &irq_domain_simple_ops, NULL);
+	if (!irq_domain) {
+		pr_warn("Failed to allocate irq domain\n");
+		goto err_domain_add;
+	}
+
+	/* 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);
+
+	/* 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, bank_data + i / 32);
+		set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE);
+	}
+
+	return 0;
+
+err_domain_add:
+err_wrong_irq_base:
+
+	irq_free_descs(irq_base, irqs - 16);
+err_irq_alloc_descs:
+
+	kfree(bank_data);
+err_alloc_bank_data:
+
+	return ret;
+}
+
+static int __init nvic_of_init(struct device_node *node,
+			       struct device_node *parent)
+{
+	void __iomem *nvic_base;
+
+	if (!node)
+		return -ENODEV;
+
+	nvic_base = of_iomap(node, 0);
+	if (!nvic_base) {
+		pr_warn("unable to map nvic registers\n");
+		return -ENOMEM;
+	}
+
+	return nvic_init_bases(node, nvic_base);
+}
+IRQCHIP_DECLARE(armv7m_nvic, "arm,armv7m-nvic", nvic_of_init);
-- 
1.8.2.rc2

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

[Uwe Kleine-König]

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 <catalin.marinas@arm.com>
Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

changes since v1:

 - fixed base address to match documentation
   So reading ICTR (which isn't in the nvic's address range) uses a new #define
   in asm/v7m.h which isn't in mainline yet. That's ugly but I don't have a
   better idea how to solve it without platform code.
 - s/NVIC_IPRI/NVIC_IPR/ to match documentation
 - use pr_warn instead of WARN/WARN_ON
 - do proper error handling, don't use IS_ERR_VALUE
 - drop the wrong skipping of the 16 system exceptions. They are not counted in
   ICTR_INTLINESNUM.
 - dynamically allocate chip data
 - drop the irq_domain* member from chip data as it's only used in the probe
   callback
 - change compatible string to arm,armv7m-nvic
 - drop a few unused #includes, use some linux/ #includes instead of asm/ ones
 - change indention to please tglx' eyes

A failure to probe the nvic makes the machine unresponsive. Does this 
have any implications on how the driver should behave when something
goes wrong? Another issue is that up to now the exception handling
simply calls asm_do_IRQ(16, ..) for the first nvic interrupt. So there
is a mismatch if irq_alloc_descs_from(16, ...) doesn't return 16. I
added error handling for that assuming that's fine for now, but in the long run
a better fix would be nice. What is the preferred approach to fix that? Use a
global variable that holds the irq_base? Or should I use a mapping function
instead?

 drivers/irqchip/Kconfig    |   4 ++
 drivers/irqchip/Makefile   |   1 +
 drivers/irqchip/irq-nvic.c | 176 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 181 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..721c328
--- /dev/null
+++ b/drivers/irqchip/irq-nvic.c
@@ -0,0 +1,176 @@
+/*
+ * 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 "irqchip.h"
+
+#define NVIC_ISER			0x000
+#define NVIC_ICER			0x080
+#define NVIC_IPR			0x300
+
+/*
+ * Each bank handles 32 irqs. Only the 16th (= last) bank handles only
+ * 16 irqs yielding a maximum of 15 * 32 + 16 = 496 interrupts.
+ */
+#define NVIC_MAX_IRQ		496
+
+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 inline void __iomem *nvic_bank_base(struct irq_data *d)
+{
+	struct nvic_bank_data *data = irq_data_get_irq_chip_data(d);
+	return 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 __init nvic_init_bases(struct device_node *node,
+				  void __iomem *nvic_base)
+{
+	unsigned int irqs, i;
+	int irq_base, ret = -ENOMEM;
+	struct irq_domain *irq_domain;
+	struct nvic_bank_data *bank_data;
+	unsigned numbanks = (readl_relaxed(V7M_SCS_ICTR) &
+			     V7M_SCS_ICTR_INTLINESNUM_MASK) + 1;
+
+	irqs = numbanks * 32;
+	if (irqs > NVIC_MAX_IRQ)
+		irqs = NVIC_MAX_IRQ;
+
+	bank_data = kzalloc(sizeof(*bank_data) * numbanks, GFP_KERNEL);
+	if (!bank_data) {
+		pr_warn("Failed to allocate chip data");
+		goto err_alloc_bank_data;
+	}
+	for (i = 0; i < numbanks; ++i)
+		bank_data[i].base = nvic_base + 4 * i;
+
+	irq_base = irq_alloc_descs_from(16, irqs, numa_node_id());
+	if (irq_base < 0) {
+		pr_warn("Cannot allocate irq_descs\n");
+		ret = irq_base;
+		goto err_irq_alloc_descs;
+	}
+	if (irq_base != 16) {
+		/*
+		 * The entry code just passes the exception number (i.e. irq
+		 * number + 16) to asm_do_IRQ, so the offset needs to be fixed
+		 * here.
+		 */
+		pr_warn("Failed to allocate irq_descs@offset 16\n");
+		goto err_wrong_irq_base;
+	}
+
+	irq_domain = irq_domain_add_legacy(node, irqs, irq_base, 0,
+					   &irq_domain_simple_ops, NULL);
+	if (!irq_domain) {
+		pr_warn("Failed to allocate irq domain\n");
+		goto err_domain_add;
+	}
+
+	/* 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);
+
+	/* 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, bank_data + i / 32);
+		set_irq_flags(irq_base + i, IRQF_VALID | IRQF_PROBE);
+	}
+
+	return 0;
+
+err_domain_add:
+err_wrong_irq_base:
+
+	irq_free_descs(irq_base, irqs - 16);
+err_irq_alloc_descs:
+
+	kfree(bank_data);
+err_alloc_bank_data:
+
+	return ret;
+}
+
+static int __init nvic_of_init(struct device_node *node,
+			       struct device_node *parent)
+{
+	void __iomem *nvic_base;
+
+	if (!node)
+		return -ENODEV;
+
+	nvic_base = of_iomap(node, 0);
+	if (!nvic_base) {
+		pr_warn("unable to map nvic registers\n");
+		return -ENOMEM;
+	}
+
+	return nvic_init_bases(node, nvic_base);
+}
+IRQCHIP_DECLARE(armv7m_nvic, "arm,armv7m-nvic", nvic_of_init);
-- 
1.8.2.rc2

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

[Uwe Kleine-König]

Hello Thomas,

On Mon, Mar 18, 2013 at 02:20:26PM +0100, Uwe Kleine-König wrote:
> 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 <catalin.marinas@arm.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> changes since v1:
> 
>  - fixed base address to match documentation
>    So reading ICTR (which isn't in the nvic's address range) uses a new #define
>    in asm/v7m.h which isn't in mainline yet. That's ugly but I don't have a
>    better idea how to solve it without platform code.
>  - s/NVIC_IPRI/NVIC_IPR/ to match documentation
>  - use pr_warn instead of WARN/WARN_ON
>  - do proper error handling, don't use IS_ERR_VALUE
>  - drop the wrong skipping of the 16 system exceptions. They are not counted in
>    ICTR_INTLINESNUM.
>  - dynamically allocate chip data
>  - drop the irq_domain* member from chip data as it's only used in the probe
>    callback
>  - change compatible string to arm,armv7m-nvic
>  - drop a few unused #includes, use some linux/ #includes instead of asm/ ones
>  - change indention to please tglx' eyes
> 
> A failure to probe the nvic makes the machine unresponsive. Does this 
> have any implications on how the driver should behave when something
> goes wrong? Another issue is that up to now the exception handling
> simply calls asm_do_IRQ(16, ..) for the first nvic interrupt. So there
> is a mismatch if irq_alloc_descs_from(16, ...) doesn't return 16. I
> added error handling for that assuming that's fine for now, but in the long run
> a better fix would be nice. What is the preferred approach to fix that? Use a
> global variable that holds the irq_base? Or should I use a mapping function
> instead?
I didn't hear back from you since I sent out v2. Do you still have some
concerns?

I intend to put the Cortex-M3 stuff into next. Assuming it's just lack
of time on your side that stops you from commenting, would you mind if I
put this patch into next, too?

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello Thomas,

On Mon, Mar 18, 2013 at 02:20:26PM +0100, Uwe Kleine-K?nig wrote:
> 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 <catalin.marinas@arm.com>
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> changes since v1:
> 
>  - fixed base address to match documentation
>    So reading ICTR (which isn't in the nvic's address range) uses a new #define
>    in asm/v7m.h which isn't in mainline yet. That's ugly but I don't have a
>    better idea how to solve it without platform code.
>  - s/NVIC_IPRI/NVIC_IPR/ to match documentation
>  - use pr_warn instead of WARN/WARN_ON
>  - do proper error handling, don't use IS_ERR_VALUE
>  - drop the wrong skipping of the 16 system exceptions. They are not counted in
>    ICTR_INTLINESNUM.
>  - dynamically allocate chip data
>  - drop the irq_domain* member from chip data as it's only used in the probe
>    callback
>  - change compatible string to arm,armv7m-nvic
>  - drop a few unused #includes, use some linux/ #includes instead of asm/ ones
>  - change indention to please tglx' eyes
> 
> A failure to probe the nvic makes the machine unresponsive. Does this 
> have any implications on how the driver should behave when something
> goes wrong? Another issue is that up to now the exception handling
> simply calls asm_do_IRQ(16, ..) for the first nvic interrupt. So there
> is a mismatch if irq_alloc_descs_from(16, ...) doesn't return 16. I
> added error handling for that assuming that's fine for now, but in the long run
> a better fix would be nice. What is the preferred approach to fix that? Use a
> global variable that holds the irq_base? Or should I use a mapping function
> instead?
I didn't hear back from you since I sent out v2. Do you still have some
concerns?

I intend to put the Cortex-M3 stuff into next. Assuming it's just lack
of time on your side that stops you from commenting, would you mind if I
put this patch into next, too?

Best regards
Uwe

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

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

[Arnd Bergmann]

On Monday 18 March 2013, Uwe Kleine-König wrote:

> +static int __init nvic_init_bases(struct device_node *node,
> +				  void __iomem *nvic_base)
> +{

There is probably no point to keep this function separate from
nvic_of_init any more, unless you plan to mke it globally
accessible and called from non-DT platforms. In that case
you would need an irq_base argument though.

> +	irq_base = irq_alloc_descs_from(16, irqs, numa_node_id());
> +	if (irq_base < 0) {
> +		pr_warn("Cannot allocate irq_descs\n");
> +		ret = irq_base;
> +		goto err_irq_alloc_descs;
> +	}
> +	if (irq_base != 16) {
> +		/*
> +		 * The entry code just passes the exception number (i.e. irq
> +		 * number + 16) to asm_do_IRQ, so the offset needs to be fixed
> +		 * here.
> +		 */
> +		pr_warn("Failed to allocate irq_descs at offset 16\n");
> +		goto err_wrong_irq_base;
> +	}
> +
> +	irq_domain = irq_domain_add_legacy(node, irqs, irq_base, 0,
> +					   &irq_domain_simple_ops, NULL);

Why do you use a legacy domain here, and hardcode the irq_base?
For a fully DT-enabled platform, the base should not matter and you
can use irq_domain_add_linear, while a legacy platform would likely
need a different base.

> +static int __init nvic_of_init(struct device_node *node,
> +			       struct device_node *parent)
> +{
> +	void __iomem *nvic_base;
> +
> +	if (!node)
> +		return -ENODEV;

As Thomas commented, the check for !node is pointless here.

	Arnd

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

[Arnd Bergmann]

On Monday 18 March 2013, Uwe Kleine-K?nig wrote:

> +static int __init nvic_init_bases(struct device_node *node,
> +				  void __iomem *nvic_base)
> +{

There is probably no point to keep this function separate from
nvic_of_init any more, unless you plan to mke it globally
accessible and called from non-DT platforms. In that case
you would need an irq_base argument though.

> +	irq_base = irq_alloc_descs_from(16, irqs, numa_node_id());
> +	if (irq_base < 0) {
> +		pr_warn("Cannot allocate irq_descs\n");
> +		ret = irq_base;
> +		goto err_irq_alloc_descs;
> +	}
> +	if (irq_base != 16) {
> +		/*
> +		 * The entry code just passes the exception number (i.e. irq
> +		 * number + 16) to asm_do_IRQ, so the offset needs to be fixed
> +		 * here.
> +		 */
> +		pr_warn("Failed to allocate irq_descs at offset 16\n");
> +		goto err_wrong_irq_base;
> +	}
> +
> +	irq_domain = irq_domain_add_legacy(node, irqs, irq_base, 0,
> +					   &irq_domain_simple_ops, NULL);

Why do you use a legacy domain here, and hardcode the irq_base?
For a fully DT-enabled platform, the base should not matter and you
can use irq_domain_add_linear, while a legacy platform would likely
need a different base.

> +static int __init nvic_of_init(struct device_node *node,
> +			       struct device_node *parent)
> +{
> +	void __iomem *nvic_base;
> +
> +	if (!node)
> +		return -ENODEV;

As Thomas commented, the check for !node is pointless here.

	Arnd