2010-01-27 19:33:31

by Abhijeet Dharmapurikar

[permalink] [raw]
Subject: [PATCH 0/5] Arm Gic Changes

From: Abhijeet Dharmapurikar <[email protected]>

Few fixes and improvements to arch/arm/common/gic.c file.

Abhijeet Dharmapurikar (5):
gic: prevent gic from crossing NR_IRQ
gic: add callback for mask_ack
gic: Add set_type callback
gic: dont disable INT in ack callback
gic: initialize interrupts as per argument

arch/arm/common/gic.c | 114 ++++++++++++++++++++++++-----
arch/arm/include/asm/hardware/gic.h | 3 +-
arch/arm/mach-omap2/board-4430sdp.c | 3 +-
arch/arm/mach-realview/realview_eb.c | 10 ++-
arch/arm/mach-realview/realview_pb1176.c | 7 ++-
arch/arm/mach-realview/realview_pb11mp.c | 7 ++-
arch/arm/mach-realview/realview_pba8.c | 4 +-
arch/arm/mach-realview/realview_pbx.c | 5 +-
arch/arm/mach-ux500/cpu-u8500.c | 2 +-
9 files changed, 122 insertions(+), 33 deletions(-)


2010-01-27 19:32:44

by Abhijeet Dharmapurikar

[permalink] [raw]
Subject: [PATCH 1/5] gic: prevent gic from crossing NR_IRQ

From: Abhijeet Dharmapurikar <[email protected]>

The gic code tries to initialize interrupts beyond NR_IRQ. Prevent
code from doing that.

Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
---
arch/arm/common/gic.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 337741f..e763c4c 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -175,6 +175,11 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
set_irq_chained_handler(irq, gic_handle_cascade_irq);
}

+/*
+ * In case of multiple cascaded GICs, order calls to gic_dist_init with
+ * ascending irq_start
+ */
+
void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
unsigned int irq_start)
{
@@ -233,7 +238,7 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
/*
* Setup the Linux IRQ subsystem.
*/
- for (i = irq_start; i < gic_data[gic_nr].irq_offset + max_irq; i++) {
+ for (i = irq_start; i < NR_IRQS && i < gic_data[gic_nr].irq_offset + max_irq; i++) {
set_irq_chip(i, &gic_chip);
set_irq_chip_data(i, &gic_data[gic_nr]);
set_irq_handler(i, handle_level_irq);
--
1.5.6.3

2010-01-27 19:33:01

by Abhijeet Dharmapurikar

[permalink] [raw]
Subject: [PATCH 3/5] gic: Add set_type callback

From: Abhijeet Dharmapurikar <[email protected]>

Add gic_set_type callback to set an irq as level or edge triggered type

Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
---
arch/arm/common/gic.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 00172c4..709cf53 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -165,6 +165,48 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
chip->unmask(irq);
}

+static int gic_set_type(unsigned int irq, unsigned int flow_type)
+{
+ unsigned int register_index;
+ unsigned int bit_index;
+ unsigned int reg_value;
+
+ if (irq > 1020)
+ return -1;
+
+ /*
+ * Two bits each, calc the register and bit, 16 per 32 bit register
+ * accessible long word only
+ * But the field is NxN 1xN and rising/falling
+ */
+ register_index = (irq/16)<<2;
+ bit_index = (irq & 0xF)<<1;
+
+ spin_lock(&irq_controller_lock);
+ reg_value = readl(gic_dist_base(irq) + GIC_DIST_CONFIG +
+ register_index);
+ /*
+ * keep the nxn and 1xn , mask the edge level
+ * Edge is 1, level 0
+ */
+ reg_value = (reg_value & ~(2<<bit_index));
+ if (flow_type & (IRQ_TYPE_EDGE_RISING|IRQ_TYPE_EDGE_FALLING)) {
+ reg_value |= (2<<bit_index);
+ writel(reg_value, gic_dist_base(irq) + GIC_DIST_CONFIG
+ + register_index);
+ __set_irq_handler_unlocked(irq, handle_edge_irq);
+ }
+
+ if (flow_type & (IRQ_TYPE_LEVEL_HIGH|IRQ_TYPE_LEVEL_LOW)) {
+ writel(reg_value, gic_dist_base(irq) + GIC_DIST_CONFIG
+ + register_index);
+ __set_irq_handler_unlocked(irq, handle_level_irq);
+ }
+
+ spin_unlock(&irq_controller_lock);
+ return 0;
+}
+
static struct irq_chip gic_chip = {
.name = "GIC",
.ack = gic_ack_irq,
@@ -174,6 +216,7 @@ static struct irq_chip gic_chip = {
#ifdef CONFIG_SMP
.set_affinity = gic_set_cpu,
#endif
+ .set_type = gic_set_type,
};

void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
--
1.5.6.3

2010-01-27 19:32:40

by Abhijeet Dharmapurikar

[permalink] [raw]
Subject: [RFC PATCH 5/5] gic: initialize interrupts as per argument

From: Abhijeet Dharmapurikar <[email protected]>

Initialize interrupts as per an argument passed instead of initializing
them as active level low triggered interrupts.

Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
---
Configuring level triggered interrupts as default may not be correct on
some platforms, this changes the default behaviour of the gic_dist_init
function and also adds a flag to indicate whether the default should be level
or edge.

an argument.
arch/arm/common/gic.c | 25 ++++++++++++++++++-------
arch/arm/include/asm/hardware/gic.h | 3 ++-
arch/arm/mach-omap2/board-4430sdp.c | 3 ++-
arch/arm/mach-realview/realview_eb.c | 10 +++++++---
arch/arm/mach-realview/realview_pb1176.c | 7 +++++--
arch/arm/mach-realview/realview_pb11mp.c | 7 +++++--
arch/arm/mach-realview/realview_pba8.c | 4 +++-
arch/arm/mach-realview/realview_pbx.c | 5 +++--
arch/arm/mach-ux500/cpu-u8500.c | 2 +-
9 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index d47a1d7..1437f8e 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -236,10 +236,11 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
/*
* In case of multiple cascaded GICs, order calls to gic_dist_init with
* ascending irq_start
+ * flags are checked to initialize the config registers to level or edge
*/

void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
- unsigned int irq_start)
+ unsigned int irq_start, unsigned int flags)
{
unsigned int max_irq, i;
u32 cpumask = 1 << smp_processor_id();
@@ -269,11 +270,17 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
if (max_irq > max(1020, NR_IRQS))
max_irq = max(1020, NR_IRQS);

- /*
- * Set all global interrupts to be level triggered, active low.
- */
- for (i = 32; i < max_irq; i += 16)
- writel(0, base + GIC_DIST_CONFIG + i * 4 / 16);
+ /* default to level trigger if nothing specified */
+ if ((flags & IRQ_TYPE_SENSE_MASK) == 0)
+ flags |= IRQ_TYPE_LEVEL_LOW;
+
+ if (flags & (IRQ_TYPE_LEVEL_HIGH|IRQ_TYPE_LEVEL_LOW)) {
+ for (i = 32; i < max_irq; i += 16)
+ writel(0, base + GIC_DIST_CONFIG + i * 4 / 16);
+ } else {
+ for (i = 16; i < max_irq; i += 16)
+ writel(0xFFFFFFFF, base + GIC_DIST_CONFIG + i * 4 / 16);
+ }

/*
* Set all global interrupts to this CPU only.
@@ -293,13 +300,17 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
for (i = 0; i < max_irq; i += 32)
writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);

+
/*
* Setup the Linux IRQ subsystem.
*/
for (i = irq_start; i < NR_IRQS && i < gic_data[gic_nr].irq_offset + max_irq; i++) {
set_irq_chip(i, &gic_chip);
set_irq_chip_data(i, &gic_data[gic_nr]);
- set_irq_handler(i, handle_level_irq);
+ if (flags & (IRQ_TYPE_LEVEL_HIGH|IRQ_TYPE_LEVEL_LOW))
+ set_irq_handler(i, handle_level_irq);
+ else
+ set_irq_handler(i, handle_edge_irq);
set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
}

diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 7f34333..7ebccdc 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -33,7 +33,8 @@
#define GIC_DIST_SOFTINT 0xf00

#ifndef __ASSEMBLY__
-void gic_dist_init(unsigned int gic_nr, void __iomem *base, unsigned int irq_start);
+void gic_dist_init(unsigned int gic_nr, void __iomem *base,
+ unsigned int irq_start, unsigned int flags);
void gic_cpu_init(unsigned int gic_nr, void __iomem *base);
void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
index 0c6be6b..1f895e9 100644
--- a/arch/arm/mach-omap2/board-4430sdp.c
+++ b/arch/arm/mach-omap2/board-4430sdp.c
@@ -17,6 +17,7 @@
#include <linux/platform_device.h>
#include <linux/io.h>
#include <linux/gpio.h>
+#include <linux/irq.h>

#include <mach/hardware.h>
#include <asm/mach-types.h>
@@ -57,7 +58,7 @@ static void __init gic_init_irq(void)
/* Static mapping, never released */
base = ioremap(OMAP44XX_GIC_DIST_BASE, SZ_4K);
BUG_ON(!base);
- gic_dist_init(0, base, 29);
+ gic_dist_init(0, base, 29, IRQ_TYPE_LEVEL_LOW);

/* Static mapping, never released */
gic_cpu_base_addr = ioremap(OMAP44XX_GIC_CPU_BASE, SZ_512);
diff --git a/arch/arm/mach-realview/realview_eb.c b/arch/arm/mach-realview/realview_eb.c
index 7d857d3..cb119f9 100644
--- a/arch/arm/mach-realview/realview_eb.c
+++ b/arch/arm/mach-realview/realview_eb.c
@@ -26,6 +26,7 @@
#include <linux/amba/pl061.h>
#include <linux/amba/mmci.h>
#include <linux/io.h>
+#include <linux/irq.h>

#include <mach/hardware.h>
#include <asm/irq.h>
@@ -308,19 +309,22 @@ static void __init gic_init_irq(void)

/* core tile GIC, primary */
gic_cpu_base_addr = __io_address(REALVIEW_EB11MP_GIC_CPU_BASE);
- gic_dist_init(0, __io_address(REALVIEW_EB11MP_GIC_DIST_BASE), 29);
+ gic_dist_init(0, __io_address(REALVIEW_EB11MP_GIC_DIST_BASE),
+ 29, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, gic_cpu_base_addr);

#ifndef CONFIG_REALVIEW_EB_ARM11MP_REVB
/* board GIC, secondary */
- gic_dist_init(1, __io_address(REALVIEW_EB_GIC_DIST_BASE), 64);
+ gic_dist_init(1, __io_address(REALVIEW_EB_GIC_DIST_BASE),
+ 64, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(1, __io_address(REALVIEW_EB_GIC_CPU_BASE));
gic_cascade_irq(1, IRQ_EB11MP_EB_IRQ1);
#endif
} else {
/* board GIC, primary */
gic_cpu_base_addr = __io_address(REALVIEW_EB_GIC_CPU_BASE);
- gic_dist_init(0, __io_address(REALVIEW_EB_GIC_DIST_BASE), 29);
+ gic_dist_init(0, __io_address(REALVIEW_EB_GIC_DIST_BASE),
+ 29, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, gic_cpu_base_addr);
}
}
diff --git a/arch/arm/mach-realview/realview_pb1176.c b/arch/arm/mach-realview/realview_pb1176.c
index 44392e5..1e218f2 100644
--- a/arch/arm/mach-realview/realview_pb1176.c
+++ b/arch/arm/mach-realview/realview_pb1176.c
@@ -26,6 +26,7 @@
#include <linux/amba/pl061.h>
#include <linux/amba/mmci.h>
#include <linux/io.h>
+#include <linux/irq.h>

#include <mach/hardware.h>
#include <asm/irq.h>
@@ -267,11 +268,13 @@ static void __init gic_init_irq(void)
{
/* ARM1176 DevChip GIC, primary */
gic_cpu_base_addr = __io_address(REALVIEW_DC1176_GIC_CPU_BASE);
- gic_dist_init(0, __io_address(REALVIEW_DC1176_GIC_DIST_BASE), IRQ_DC1176_GIC_START);
+ gic_dist_init(0, __io_address(REALVIEW_DC1176_GIC_DIST_BASE),
+ IRQ_DC1176_GIC_START, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, gic_cpu_base_addr);

/* board GIC, secondary */
- gic_dist_init(1, __io_address(REALVIEW_PB1176_GIC_DIST_BASE), IRQ_PB1176_GIC_START);
+ gic_dist_init(1, __io_address(REALVIEW_PB1176_GIC_DIST_BASE),
+ IRQ_PB1176_GIC_START, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(1, __io_address(REALVIEW_PB1176_GIC_CPU_BASE));
gic_cascade_irq(1, IRQ_DC1176_PB_IRQ1);
}
diff --git a/arch/arm/mach-realview/realview_pb11mp.c b/arch/arm/mach-realview/realview_pb11mp.c
index 3e02731..2d45f9f 100644
--- a/arch/arm/mach-realview/realview_pb11mp.c
+++ b/arch/arm/mach-realview/realview_pb11mp.c
@@ -26,6 +26,7 @@
#include <linux/amba/pl061.h>
#include <linux/amba/mmci.h>
#include <linux/io.h>
+#include <linux/irq.h>

#include <mach/hardware.h>
#include <asm/irq.h>
@@ -273,11 +274,13 @@ static void __init gic_init_irq(void)

/* ARM11MPCore test chip GIC, primary */
gic_cpu_base_addr = __io_address(REALVIEW_TC11MP_GIC_CPU_BASE);
- gic_dist_init(0, __io_address(REALVIEW_TC11MP_GIC_DIST_BASE), 29);
+ gic_dist_init(0, __io_address(REALVIEW_TC11MP_GIC_DIST_BASE),
+ 29, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, gic_cpu_base_addr);

/* board GIC, secondary */
- gic_dist_init(1, __io_address(REALVIEW_PB11MP_GIC_DIST_BASE), IRQ_PB11MP_GIC_START);
+ gic_dist_init(1, __io_address(REALVIEW_PB11MP_GIC_DIST_BASE),
+ IRQ_PB11MP_GIC_START, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(1, __io_address(REALVIEW_PB11MP_GIC_CPU_BASE));
gic_cascade_irq(1, IRQ_TC11MP_PB_IRQ1);
}
diff --git a/arch/arm/mach-realview/realview_pba8.c b/arch/arm/mach-realview/realview_pba8.c
index fe4e25c..efdebcf 100644
--- a/arch/arm/mach-realview/realview_pba8.c
+++ b/arch/arm/mach-realview/realview_pba8.c
@@ -26,6 +26,7 @@
#include <linux/amba/pl061.h>
#include <linux/amba/mmci.h>
#include <linux/io.h>
+#include <linux/irq.h>

#include <asm/irq.h>
#include <asm/leds.h>
@@ -254,7 +255,8 @@ static void __init gic_init_irq(void)
{
/* ARM PB-A8 on-board GIC */
gic_cpu_base_addr = __io_address(REALVIEW_PBA8_GIC_CPU_BASE);
- gic_dist_init(0, __io_address(REALVIEW_PBA8_GIC_DIST_BASE), IRQ_PBA8_GIC_START);
+ gic_dist_init(0, __io_address(REALVIEW_PBA8_GIC_DIST_BASE),
+ IRQ_PBA8_GIC_START, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, __io_address(REALVIEW_PBA8_GIC_CPU_BASE));
}

diff --git a/arch/arm/mach-realview/realview_pbx.c b/arch/arm/mach-realview/realview_pbx.c
index a21a4b3..f1aa686 100644
--- a/arch/arm/mach-realview/realview_pbx.c
+++ b/arch/arm/mach-realview/realview_pbx.c
@@ -25,6 +25,7 @@
#include <linux/amba/pl061.h>
#include <linux/amba/mmci.h>
#include <linux/io.h>
+#include <linux/irq.h>

#include <asm/irq.h>
#include <asm/leds.h>
@@ -276,12 +277,12 @@ static void __init gic_init_irq(void)
if (core_tile_pbx11mp() || core_tile_pbxa9mp()) {
gic_cpu_base_addr = __io_address(REALVIEW_PBX_TILE_GIC_CPU_BASE);
gic_dist_init(0, __io_address(REALVIEW_PBX_TILE_GIC_DIST_BASE),
- 29);
+ 29, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, __io_address(REALVIEW_PBX_TILE_GIC_CPU_BASE));
} else {
gic_cpu_base_addr = __io_address(REALVIEW_PBX_GIC_CPU_BASE);
gic_dist_init(0, __io_address(REALVIEW_PBX_GIC_DIST_BASE),
- IRQ_PBX_GIC_START);
+ IRQ_PBX_GIC_START, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, __io_address(REALVIEW_PBX_GIC_CPU_BASE));
}
}
diff --git a/arch/arm/mach-ux500/cpu-u8500.c b/arch/arm/mach-ux500/cpu-u8500.c
index 5f05e58..68028fe 100644
--- a/arch/arm/mach-ux500/cpu-u8500.c
+++ b/arch/arm/mach-ux500/cpu-u8500.c
@@ -48,7 +48,7 @@ void __init u8500_map_io(void)

void __init u8500_init_irq(void)
{
- gic_dist_init(0, __io_address(U8500_GIC_DIST_BASE), 29);
+ gic_dist_init(0, __io_address(U8500_GIC_DIST_BASE), 29, IRQ_TYPE_LEVEL_LOW);
gic_cpu_init(0, __io_address(U8500_GIC_CPU_BASE));
}

--
1.5.6.3

2010-01-27 19:32:57

by Abhijeet Dharmapurikar

[permalink] [raw]
Subject: [PATCH 4/5] gic: dont disable INT in ack callback

From: Abhijeet Dharmapurikar <[email protected]>

Unless gic_ack_irq is called from __do_IRQ, interrupt should not
be disabled in the ack function. Disabling the interrupt causes
handle_edge_irq to never enable it again.

Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
---
arch/arm/common/gic.c | 29 +++++++++++++++++------------
1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 709cf53..d47a1d7 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -67,25 +67,30 @@ static inline unsigned int gic_irq(unsigned int irq)

/*
* Routines to acknowledge, disable and enable interrupts
- *
- * Linux assumes that when we're done with an interrupt we need to
- * unmask it, in the same way we need to unmask an interrupt when
- * we first enable it.
- *
- * The GIC has a separate notion of "end of interrupt" to re-enable
- * an interrupt after handling, in order to support hardware
- * prioritisation.
- *
- * We can make the GIC behave in the way that Linux expects by making
- * our "acknowledge" routine disable the interrupt, then mark it as
- * complete.
*/
static void gic_ack_irq(unsigned int irq)
{
u32 mask = 1 << (irq % 32);

spin_lock(&irq_controller_lock);
+
+#ifndef CONFIG_GENERIC_HARDIRQS_NO__DO_IRQ
+
+ /*
+ * Linux assumes that when we're done with an interrupt we need to
+ * unmask it, in the same way we need to unmask an interrupt when
+ * we first enable it.
+ *
+ * The GIC has a separate notion of "end of interrupt" to re-enable
+ * an interrupt after handling, in order to support hardware
+ * prioritisation.
+ *
+ * We can make the GIC behave in the way that Linux expects by making
+ * our "acknowledge" routine disable the interrupt, then mark it as
+ * complete.
+ */
writel(mask, gic_dist_base(irq) + GIC_DIST_ENABLE_CLEAR + (gic_irq(irq) / 32) * 4);
+#endif
writel(gic_irq(irq), gic_cpu_base(irq) + GIC_CPU_EOI);
spin_unlock(&irq_controller_lock);
}
--
1.5.6.3

2010-01-27 19:33:48

by Abhijeet Dharmapurikar

[permalink] [raw]
Subject: [PATCH 2/5] gic: add callback for mask_ack

From: Abhijeet Dharmapurikar <[email protected]>

add gic_mask_ack for faster processing of interrupts.

Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
---
arch/arm/common/gic.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index e763c4c..00172c4 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -108,6 +108,15 @@ static void gic_unmask_irq(unsigned int irq)
spin_unlock(&irq_controller_lock);
}

+static void gic_mask_ack_irq(unsigned int irq)
+{
+ u32 mask = 1 << (irq % 32);
+ spin_lock(&irq_controller_lock);
+ writel(mask, gic_dist_base(irq) + GIC_DIST_ENABLE_CLEAR + (gic_irq(irq) / 32) * 4);
+ writel(gic_irq(irq), gic_cpu_base(irq) + GIC_CPU_EOI);
+ spin_unlock(&irq_controller_lock);
+}
+
#ifdef CONFIG_SMP
static int gic_set_cpu(unsigned int irq, const struct cpumask *mask_val)
{
@@ -160,6 +169,7 @@ static struct irq_chip gic_chip = {
.name = "GIC",
.ack = gic_ack_irq,
.mask = gic_mask_irq,
+ .mask_ack = gic_mask_ack_irq,
.unmask = gic_unmask_irq,
#ifdef CONFIG_SMP
.set_affinity = gic_set_cpu,
--
1.5.6.3

2010-01-27 22:50:30

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 1/5] gic: prevent gic from crossing NR_IRQ

On Wed, Jan 27, 2010 at 11:32:25AM -0800, [email protected] wrote:
> From: Abhijeet Dharmapurikar <[email protected]>
>
> The gic code tries to initialize interrupts beyond NR_IRQ. Prevent
> code from doing that.

NAK. This is completely the wrong approach.

/*
* Find out how many interrupts are supported.
*/
max_irq = readl(base + GIC_DIST_CTR) & 0x1f;
max_irq = (max_irq + 1) * 32;

/*
* The GIC only supports up to 1020 interrupt sources.
* Limit this to either the architected maximum, or the
* platform maximum.
*/
if (max_irq > max(1020, NR_IRQS))
max_irq = max(1020, NR_IRQS);
...
for (i = irq_start; i < gic_data[gic_nr].irq_offset + max_irq; i++) {

This function is broken if irq_start != 0, and needs fixing - max_irq
needs to be limited to the _minimum_ of 1020 or NR_IRQS - irq_start.

2010-01-27 23:17:00

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 3/5] gic: Add set_type callback

On Wed, Jan 27, 2010 at 11:32:27AM -0800, [email protected] wrote:
> + if (irq > 1020)
> + return -1;

*unprintable*. Always use proper error codes.

> +
> + /*
> + * Two bits each, calc the register and bit, 16 per 32 bit register
> + * accessible long word only
> + * But the field is NxN 1xN and rising/falling
> + */
> + register_index = (irq/16)<<2;
> + bit_index = (irq & 0xF)<<1;
> +
> + spin_lock(&irq_controller_lock);
> + reg_value = readl(gic_dist_base(irq) + GIC_DIST_CONFIG +
> + register_index);
> + /*
> + * keep the nxn and 1xn , mask the edge level
> + * Edge is 1, level 0
> + */
> + reg_value = (reg_value & ~(2<<bit_index));
> + if (flow_type & (IRQ_TYPE_EDGE_RISING|IRQ_TYPE_EDGE_FALLING)) {
> + reg_value |= (2<<bit_index);
> + writel(reg_value, gic_dist_base(irq) + GIC_DIST_CONFIG
> + + register_index);
> + __set_irq_handler_unlocked(irq, handle_edge_irq);
> + }
> +
> + if (flow_type & (IRQ_TYPE_LEVEL_HIGH|IRQ_TYPE_LEVEL_LOW)) {
> + writel(reg_value, gic_dist_base(irq) + GIC_DIST_CONFIG
> + + register_index);
> + __set_irq_handler_unlocked(irq, handle_level_irq);
> + }

This needs to exclude the first 32 interrupts, where the interrupt
configuration is either read-only or banked between multiprocessor
CPUs.

2010-01-27 23:20:56

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 3/5] gic: Add set_type callback

On Wed, Jan 27, 2010 at 11:32:27AM -0800, [email protected] wrote:
> + if (flow_type & (IRQ_TYPE_EDGE_RISING|IRQ_TYPE_EDGE_FALLING)) {
> + reg_value |= (2<<bit_index);
> + writel(reg_value, gic_dist_base(irq) + GIC_DIST_CONFIG
> + + register_index);
> + __set_irq_handler_unlocked(irq, handle_edge_irq);
> + }
> +
> + if (flow_type & (IRQ_TYPE_LEVEL_HIGH|IRQ_TYPE_LEVEL_LOW)) {

This is actually where things start to get rather sticky - because
there may well be on-chip inverters between the GIC and external
peripherals.

Since the GIC can only sense one edge or one level depending on the
hardware setup, it seems wrong to allow the configuration of both
high and low levels, and both edges.

2010-01-27 23:31:57

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [RFC PATCH 5/5] gic: initialize interrupts as per argument

On Wed, Jan 27, 2010 at 11:32:29AM -0800, [email protected] wrote:
> From: Abhijeet Dharmapurikar <[email protected]>
>
> Initialize interrupts as per an argument passed instead of initializing
> them as active level low triggered interrupts.
>
> Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
> ---
> Configuring level triggered interrupts as default may not be correct on
> some platforms, this changes the default behaviour of the gic_dist_init
> function and also adds a flag to indicate whether the default should be level
> or edge.

Changing the interrupt type for all interrupts is just wrong. For on-chip
peripherals, you generally want to use level based interrupts as they're
the most efficient and simplest way to signal interrupts.

It should be a per-interrupt thing, and we already have that through
request_irq() etc. Other platforms manage this just fine without having
to have special configuration like this.

2010-01-28 00:41:04

by Ben Dooks

[permalink] [raw]
Subject: Re: [PATCH 0/5] Arm Gic Changes

On Wed, Jan 27, 2010 at 11:32:24AM -0800, [email protected] wrote:
> From: Abhijeet Dharmapurikar <[email protected]>
>
> Few fixes and improvements to arch/arm/common/gic.c file.

both ARM and GIC in the title should be all in capitals, they are
abbreviations of something longer.

I think it would be nicer to make GIC in all the patch titles all capital
instead of all lower case.

> Abhijeet Dharmapurikar (5):
> gic: prevent gic from crossing NR_IRQ
> gic: add callback for mask_ack
> gic: Add set_type callback
> gic: dont disable INT in ack callback
> gic: initialize interrupts as per argument
>
> arch/arm/common/gic.c | 114 ++++++++++++++++++++++++-----
> arch/arm/include/asm/hardware/gic.h | 3 +-
> arch/arm/mach-omap2/board-4430sdp.c | 3 +-
> arch/arm/mach-realview/realview_eb.c | 10 ++-
> arch/arm/mach-realview/realview_pb1176.c | 7 ++-
> arch/arm/mach-realview/realview_pb11mp.c | 7 ++-
> arch/arm/mach-realview/realview_pba8.c | 4 +-
> arch/arm/mach-realview/realview_pbx.c | 5 +-
> arch/arm/mach-ux500/cpu-u8500.c | 2 +-
> 9 files changed, 122 insertions(+), 33 deletions(-)
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
--
Ben

Q: What's a light-year?
A: One-third less calories than a regular year.

2010-01-28 01:44:09

by Abhijeet Dharmapurikar

[permalink] [raw]
Subject: Re: [PATCH 3/5] gic: Add set_type callback

Russell King - ARM Linux wrote:
> On Wed, Jan 27, 2010 at 11:32:27AM -0800, [email protected] wrote:
>> + if (flow_type & (IRQ_TYPE_EDGE_RISING|IRQ_TYPE_EDGE_FALLING)) {
>> + reg_value |= (2<<bit_index);
>> + writel(reg_value, gic_dist_base(irq) + GIC_DIST_CONFIG
>> + + register_index);
>> + __set_irq_handler_unlocked(irq, handle_edge_irq);
>> + }
>> +
>> + if (flow_type & (IRQ_TYPE_LEVEL_HIGH|IRQ_TYPE_LEVEL_LOW)) {
>
> This is actually where things start to get rather sticky - because
> there may well be on-chip inverters between the GIC and external
> peripherals.
Actually the comment in gic_dist_init mentions about configuring SPI's
as level low when the GIC only supports level high. That suggests
presence of an inverter.
>
> Since the GIC can only sense one edge or one level depending on the
> hardware setup, it seems wrong to allow the configuration of both
> high and low levels, and both edges.
Agree, but this change at least lets us configure them as edge/level
triggered.

One solution (a rather easy one) is we simply reject LEVEL_LOW and
EDGE_FALLING and think as if the inverters belong to the peripherals
rather than the GIC.

Please suggest alternative implementation ideas.




2010-01-28 13:19:13

by Catalin Marinas

[permalink] [raw]
Subject: Re: [PATCH 1/5] gic: prevent gic from crossing NR_IRQ

On Wed, 2010-01-27 at 22:43 +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 27, 2010 at 11:32:25AM -0800, [email protected] wrote:
> > From: Abhijeet Dharmapurikar <[email protected]>
> >
> > The gic code tries to initialize interrupts beyond NR_IRQ. Prevent
> > code from doing that.
>
> NAK. This is completely the wrong approach.
>
> /*
> * Find out how many interrupts are supported.
> */
> max_irq = readl(base + GIC_DIST_CTR) & 0x1f;
> max_irq = (max_irq + 1) * 32;
>
> /*
> * The GIC only supports up to 1020 interrupt sources.
> * Limit this to either the architected maximum, or the
> * platform maximum.
> */
> if (max_irq > max(1020, NR_IRQS))
> max_irq = max(1020, NR_IRQS);
> ...
> for (i = irq_start; i < gic_data[gic_nr].irq_offset + max_irq; i++) {
>
> This function is broken if irq_start != 0, and needs fixing - max_irq
> needs to be limited to the _minimum_ of 1020 or NR_IRQS - irq_start.

Actually I think in this case max_irq should be left to whatever the
hardware reads (but not more than 1020) since further down in this
function we configure the IRQs for the GIC and we also want to disable
those beyond NR_IRQS.

The "for" loop for set_irq_chip() should go to the minimum of max_irqs
and NR_IRQS.

--
Catalin