Patchwork [v4,3/7] arm/imx: add gic_handle_irq function

login
register
mail settings
Submitter Shawn Guo
Date Sept. 28, 2011, 9:06 a.m.
Message ID <1317200808-6275-4-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/116727/
State New
Headers show

Comments

Shawn Guo - Sept. 28, 2011, 9:06 a.m.
This is a plain translation of assembly gic irq handler to C function
for CONFIG_MULTI_IRQ_HANDLER support on imx family.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/plat-mxc/Makefile                   |    2 +-
 arch/arm/plat-mxc/gic.c                      |   47 ++++++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/common.h      |    2 +
 arch/arm/plat-mxc/include/mach/entry-macro.S |    6 +++
 4 files changed, 56 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/plat-mxc/gic.c
Russell King - ARM Linux - Sept. 29, 2011, 9:34 a.m.
On Wed, Sep 28, 2011 at 05:06:44PM +0800, Shawn Guo wrote:
> +#ifdef CONFIG_SMP
> +		else if (irqnr < 16) {
> +			writel_relaxed(irqstat, gic_cpu_base_addr +
> +						GIC_CPU_EOI);
> +			do_IPI(irqnr, regs);
> +		}
> +#endif
> +#ifdef CONFIG_LOCAL_TIMERS
> +		else if (irqnr == 29) {
> +			writel_relaxed(irqstat, gic_cpu_base_addr +
> +						GIC_CPU_EOI);
> +			do_local_timer(regs);
> +		}
> +#endif

As I've said for similar patches. neither of these two functions are
designed to be called from another C function (because they're marked
__exception or __exception_irq_entry).  Both of these markers tell the
unwinder that a struct pt_regs is located directly above the functions
stack frame.
Shawn Guo - Sept. 29, 2011, 2:08 p.m.
On Thu, Sep 29, 2011 at 10:34:09AM +0100, Russell King - ARM Linux wrote:
> On Wed, Sep 28, 2011 at 05:06:44PM +0800, Shawn Guo wrote:
> > +#ifdef CONFIG_SMP
> > +		else if (irqnr < 16) {
> > +			writel_relaxed(irqstat, gic_cpu_base_addr +
> > +						GIC_CPU_EOI);
> > +			do_IPI(irqnr, regs);
> > +		}
> > +#endif
> > +#ifdef CONFIG_LOCAL_TIMERS
> > +		else if (irqnr == 29) {
> > +			writel_relaxed(irqstat, gic_cpu_base_addr +
> > +						GIC_CPU_EOI);
> > +			do_local_timer(regs);
> > +		}
> > +#endif
> 
> As I've said for similar patches. neither of these two functions are
> designed to be called from another C function (because they're marked
> __exception or __exception_irq_entry).  Both of these markers tell the
> unwinder that a struct pt_regs is located directly above the functions
> stack frame.
> 
Can you please share your position on Marc's PPI and GIC
MULTI_IRQ_HANDLER series?  Are you possibly going to merge them in the
coming merge window?  If you are, I would directly move my imx6q onto
those series and save the local gic_handle_irq().  Otherwise, I may
have to add a wrapper for do_IPI() and do_local_timer() to work around
the pt_regs issue you pointed out here.
Shawn Guo - Sept. 29, 2011, 2:27 p.m.
On Thu, Sep 29, 2011 at 10:34:09AM +0100, Russell King - ARM Linux wrote:
> On Wed, Sep 28, 2011 at 05:06:44PM +0800, Shawn Guo wrote:
> > +#ifdef CONFIG_SMP
> > +		else if (irqnr < 16) {
> > +			writel_relaxed(irqstat, gic_cpu_base_addr +
> > +						GIC_CPU_EOI);
> > +			do_IPI(irqnr, regs);
> > +		}
> > +#endif
> > +#ifdef CONFIG_LOCAL_TIMERS
> > +		else if (irqnr == 29) {
> > +			writel_relaxed(irqstat, gic_cpu_base_addr +
> > +						GIC_CPU_EOI);
> > +			do_local_timer(regs);
> > +		}
> > +#endif
> 
> As I've said for similar patches. neither of these two functions are
> designed to be called from another C function (because they're marked
> __exception or __exception_irq_entry).  Both of these markers tell the
> unwinder that a struct pt_regs is located directly above the functions
> stack frame.
> 
I'm also wondering the consequence of not doing so, because we are
seeing icip_handle_irq() and ichp_handle_irq() (arch/arm/mach-pxa/irq.c)
are doing the same thing, and we are not running into any problem with
above code.
Shawn Guo - Oct. 1, 2011, 1:30 p.m.
On Thu, Sep 29, 2011 at 10:27:30PM +0800, Shawn Guo wrote:
> On Thu, Sep 29, 2011 at 10:34:09AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Sep 28, 2011 at 05:06:44PM +0800, Shawn Guo wrote:
> > > +#ifdef CONFIG_SMP
> > > +		else if (irqnr < 16) {
> > > +			writel_relaxed(irqstat, gic_cpu_base_addr +
> > > +						GIC_CPU_EOI);
> > > +			do_IPI(irqnr, regs);
> > > +		}
> > > +#endif
> > > +#ifdef CONFIG_LOCAL_TIMERS
> > > +		else if (irqnr == 29) {
> > > +			writel_relaxed(irqstat, gic_cpu_base_addr +
> > > +						GIC_CPU_EOI);
> > > +			do_local_timer(regs);
> > > +		}
> > > +#endif
> > 
> > As I've said for similar patches. neither of these two functions are
> > designed to be called from another C function (because they're marked
> > __exception or __exception_irq_entry).  Both of these markers tell the
> > unwinder that a struct pt_regs is located directly above the functions
> > stack frame.
> > 
> I'm also wondering the consequence of not doing so, because we are
> seeing icip_handle_irq() and ichp_handle_irq() (arch/arm/mach-pxa/irq.c)
> are doing the same thing, and we are not running into any problem with
> above code.
> 
Sorry.  Please ignore the above.
Russell King - ARM Linux - Oct. 1, 2011, 3:13 p.m.
On Thu, Sep 29, 2011 at 10:08:05PM +0800, Shawn Guo wrote:
> Can you please share your position on Marc's PPI and GIC
> MULTI_IRQ_HANDLER series?  Are you possibly going to merge them in the
> coming merge window?  If you are, I would directly move my imx6q onto
> those series and save the local gic_handle_irq().  Otherwise, I may
> have to add a wrapper for do_IPI() and do_local_timer() to work around
> the pt_regs issue you pointed out here.

My personal opinion of Marc's PPI patches hasn't changed: I think
integrating them into genirq just makes the whole thing a lot more
complicated (and error-prone) than it otherwise needs to be.

For example, it is completely invalid to call any of the existing
genirq APIs for a PPI interrupt, because there is no sane way to ensure
that the intended CPU is targetted - unless we're operating from a thread
which is bound specifically to a single CPU.

However, Marc has been working with Thomas to produce something more
generic (and not ARM specific) and I don't have an opinion on that.
This addresses the issues (like the one I mention above) but I've not
been following it any further than that.

Patch

diff --git a/arch/arm/plat-mxc/Makefile b/arch/arm/plat-mxc/Makefile
index d53c35f..b9f0f5f 100644
--- a/arch/arm/plat-mxc/Makefile
+++ b/arch/arm/plat-mxc/Makefile
@@ -5,7 +5,7 @@ 
 # Common support
 obj-y := clock.o time.o devices.o cpu.o system.o irq-common.o
 
-# MX51 uses the TZIC interrupt controller, older platforms use AVIC
+obj-$(CONFIG_ARM_GIC) += gic.o
 obj-$(CONFIG_MXC_TZIC) += tzic.o
 obj-$(CONFIG_MXC_AVIC) += avic.o
 
diff --git a/arch/arm/plat-mxc/gic.c b/arch/arm/plat-mxc/gic.c
new file mode 100644
index 0000000..487d12c
--- /dev/null
+++ b/arch/arm/plat-mxc/gic.c
@@ -0,0 +1,47 @@ 
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/io.h>
+#include <asm/localtimer.h>
+#include <asm/hardware/gic.h>
+#ifdef CONFIG_SMP
+#include <asm/smp.h>
+#endif
+
+asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
+{
+	u32 irqstat, irqnr;
+
+	do {
+		irqstat = readl_relaxed(gic_cpu_base_addr + GIC_CPU_INTACK);
+		irqnr = irqstat & 0x3ff;
+		if (irqnr == 1023)
+			break;
+
+		if (irqnr > 29 && irqnr < 1021)
+			handle_IRQ(irqnr, regs);
+#ifdef CONFIG_SMP
+		else if (irqnr < 16) {
+			writel_relaxed(irqstat, gic_cpu_base_addr +
+						GIC_CPU_EOI);
+			do_IPI(irqnr, regs);
+		}
+#endif
+#ifdef CONFIG_LOCAL_TIMERS
+		else if (irqnr == 29) {
+			writel_relaxed(irqstat, gic_cpu_base_addr +
+						GIC_CPU_EOI);
+			do_local_timer(regs);
+		}
+#endif
+	} while (1);
+}
diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
index 9f15a79..988fa9a 100644
--- a/arch/arm/plat-mxc/include/mach/common.h
+++ b/arch/arm/plat-mxc/include/mach/common.h
@@ -75,6 +75,7 @@  extern void imx_print_silicon_rev(const char *cpu, int srev);
 
 void avic_handle_irq(struct pt_regs *);
 void tzic_handle_irq(struct pt_regs *);
+void gic_handle_irq(struct pt_regs *);
 
 #define imx1_handle_irq avic_handle_irq
 #define imx21_handle_irq avic_handle_irq
@@ -85,5 +86,6 @@  void tzic_handle_irq(struct pt_regs *);
 #define imx50_handle_irq tzic_handle_irq
 #define imx51_handle_irq tzic_handle_irq
 #define imx53_handle_irq tzic_handle_irq
+#define imx6q_handle_irq gic_handle_irq
 
 #endif
diff --git a/arch/arm/plat-mxc/include/mach/entry-macro.S b/arch/arm/plat-mxc/include/mach/entry-macro.S
index 842fbcb..9fe0dfc 100644
--- a/arch/arm/plat-mxc/include/mach/entry-macro.S
+++ b/arch/arm/plat-mxc/include/mach/entry-macro.S
@@ -22,3 +22,9 @@ 
 
 	.macro	get_irqnr_and_base, irqnr, irqstat, base, tmp
 	.endm
+
+	.macro test_for_ipi, irqnr, irqstat, base, tmp
+	.endm
+
+	.macro test_for_ltirq, irqnr, irqstat, base, tmp
+	.endm