| Message ID | 1351608860-24617-2-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Rob, On Tue, 30 Oct 2012 09:54:18 -0500, Rob Herring wrote: > From: Rob Herring <rob.herring@calxeda.com> > > Now that we have drivers/irqchip, move GIC irqchip to drivers/irqchip. This > is necessary to share the GIC with arm and arm64. > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > arch/arm/common/Kconfig | 8 -------- > arch/arm/common/Makefile | 1 - > drivers/irqchip/Kconfig | 8 ++++++++ > drivers/irqchip/Makefile | 1 + > arch/arm/common/gic.c => drivers/irqchip/irq-gic.c | 0 > drivers/irqchip/irqchip.c | 10 ++++++++++ > drivers/irqchip/irqchip.h | 1 + > 7 files changed, 20 insertions(+), 9 deletions(-) What about arch/arm/include/asm/hardware/gic.h ? Contrary to the current version of the bcm2835 IRQ controller driver and the armada-370-xp IRQ controller driver, the GIC and VIC drivers not only expose a <foo>_of_init() function, but also other functions that are directly used by several non-DT capable ARM sub-architectures. Of course, it works by leaving gic.h where it is now, but it sounds strange to have the driver in drivers/irqchip/ and the header file in arch/arm/include/asm/hardware/, especially if the goal is to be able to use those drivers in arm64. Best regards, Thomas
On 10/30/2012 10:01 AM, Thomas Petazzoni wrote: > Rob, > > On Tue, 30 Oct 2012 09:54:18 -0500, Rob Herring wrote: >> From: Rob Herring <rob.herring@calxeda.com> >> >> Now that we have drivers/irqchip, move GIC irqchip to drivers/irqchip. This >> is necessary to share the GIC with arm and arm64. >> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> --- >> arch/arm/common/Kconfig | 8 -------- >> arch/arm/common/Makefile | 1 - >> drivers/irqchip/Kconfig | 8 ++++++++ >> drivers/irqchip/Makefile | 1 + >> arch/arm/common/gic.c => drivers/irqchip/irq-gic.c | 0 >> drivers/irqchip/irqchip.c | 10 ++++++++++ >> drivers/irqchip/irqchip.h | 1 + >> 7 files changed, 20 insertions(+), 9 deletions(-) > > What about arch/arm/include/asm/hardware/gic.h ? > > Contrary to the current version of the bcm2835 IRQ controller driver > and the armada-370-xp IRQ controller driver, the GIC and VIC drivers > not only expose a <foo>_of_init() function, but also other functions > that are directly used by several non-DT capable ARM sub-architectures. > > Of course, it works by leaving gic.h where it is now, but it sounds > strange to have the driver in drivers/irqchip/ and the header file in > arch/arm/include/asm/hardware/, especially if the goal is to be able to > use those drivers in arm64. Right. I'll have to move it. I wasn't really thinking about arm64 until this morning. Rob
On 10/30/2012 11:05 AM, Rob Herring wrote: > On 10/30/2012 10:01 AM, Thomas Petazzoni wrote: >> Rob, >> >> On Tue, 30 Oct 2012 09:54:18 -0500, Rob Herring wrote: >>> From: Rob Herring <rob.herring@calxeda.com> >>> >>> Now that we have drivers/irqchip, move GIC irqchip to drivers/irqchip. This >>> is necessary to share the GIC with arm and arm64. >>> >>> Signed-off-by: Rob Herring <rob.herring@calxeda.com> >>> Cc: Russell King <linux@arm.linux.org.uk> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> --- >>> arch/arm/common/Kconfig | 8 -------- >>> arch/arm/common/Makefile | 1 - >>> drivers/irqchip/Kconfig | 8 ++++++++ >>> drivers/irqchip/Makefile | 1 + >>> arch/arm/common/gic.c => drivers/irqchip/irq-gic.c | 0 >>> drivers/irqchip/irqchip.c | 10 ++++++++++ >>> drivers/irqchip/irqchip.h | 1 + >>> 7 files changed, 20 insertions(+), 9 deletions(-) >> >> What about arch/arm/include/asm/hardware/gic.h ? >> >> Contrary to the current version of the bcm2835 IRQ controller driver >> and the armada-370-xp IRQ controller driver, the GIC and VIC drivers >> not only expose a <foo>_of_init() function, but also other functions >> that are directly used by several non-DT capable ARM sub-architectures. >> >> Of course, it works by leaving gic.h where it is now, but it sounds >> strange to have the driver in drivers/irqchip/ and the header file in >> arch/arm/include/asm/hardware/, especially if the goal is to be able to >> use those drivers in arm64. > > Right. I'll have to move it. I wasn't really thinking about arm64 until > this morning. Looking at this some more, arm64 doesn't need most of what's in gic.h. The register defines should be moved into the .c file. The remaining function declarations either are not needed (i.e. gic_init) or should should be done like the handle_irq function pointer init. We don't want to have platform code calling gic_cascade_irq or gic_raise_softirq directly. Perhaps we need to support this generically in irqchip code. So I'll leave them in the current header and arm64 can add the necessary support it needs. Rob
Rob, On Tue, 30 Oct 2012 12:21:20 -0500, Rob Herring wrote: > > Right. I'll have to move it. I wasn't really thinking about arm64 until > > this morning. > > Looking at this some more, arm64 doesn't need most of what's in gic.h. > The register defines should be moved into the .c file. The remaining > function declarations either are not needed (i.e. gic_init) or should > should be done like the handle_irq function pointer init. We don't want > to have platform code calling gic_cascade_irq or gic_raise_softirq > directly. Perhaps we need to support this generically in irqchip code. > So I'll leave them in the current header and arm64 can add the necessary > support it needs. The thing is that we have the same problem for the armada-370-xp IRQ controller driver. In its current form, it doesn't need to expose any symbol to the mach-mvebu code except the initialization function. However, with the SMP support, we need to expose a bunch of symbols, which kind of violates the whole idea of the drivers/irqchip infrastructure, which was aiming at limiting the number of header files added in <linux/irqchip/...> for each and every IRQ controller driver. As you say, we maybe need to support thing like yyy_raise_softirq() generically in the irqchip code. Thomas
On Tue, Oct 30, 2012 at 12:21:20PM -0500, Rob Herring wrote: > Looking at this some more, arm64 doesn't need most of what's in gic.h. > The register defines should be moved into the .c file. The remaining > function declarations either are not needed (i.e. gic_init) or should > should be done like the handle_irq function pointer init. We don't want > to have platform code calling gic_cascade_irq or gic_raise_softirq > directly. Softirqs are about the SPIs which are used for SMP IPIs and platform specific wakeup of CPUs. And platform code _needs_ to specify the way IPIs are delivered on the platform. irqchip can't do that because irqchip knows nothing about SPIs (neither does genirq.) The thing about gic_cascade_irq() is that it's to do with handling the (rare) case of having a system with two GICs cascaded together. There's only one set of platforms I know of which has that kind of madness and it's the ARM development platforms, where the baseboard has a GIC, and the SMP tile has its own GIC as part of the SMP implementation. Apart from that, gic_cascade_irq() should not be used - it should probably be ifdef'd out when not on one of the ARM dev platforms which suffer this weirdness.
On 10/30/2012 05:47 PM, Russell King - ARM Linux wrote: > On Tue, Oct 30, 2012 at 12:21:20PM -0500, Rob Herring wrote: >> Looking at this some more, arm64 doesn't need most of what's in gic.h. >> The register defines should be moved into the .c file. The remaining >> function declarations either are not needed (i.e. gic_init) or should >> should be done like the handle_irq function pointer init. We don't want >> to have platform code calling gic_cascade_irq or gic_raise_softirq >> directly. > > Softirqs are about the SPIs which are used for SMP IPIs and platform > specific wakeup of CPUs. And platform code _needs_ to specify the > way IPIs are delivered on the platform. irqchip can't do that because > irqchip knows nothing about SPIs (neither does genirq.) Right. v7 is unchanged, so the question is really only about how v8 will do this. Hopefully, ARM is standardizing this for v8. We probably want the gic (or other irqchip) to setup a raise_softirq function ptr on init rather than having a direct call to gic_raise_softirq. Rob > The thing about gic_cascade_irq() is that it's to do with handling the > (rare) case of having a system with two GICs cascaded together. There's > only one set of platforms I know of which has that kind of madness and > it's the ARM development platforms, where the baseboard has a GIC, and > the SMP tile has its own GIC as part of the SMP implementation. > > Apart from that, gic_cascade_irq() should not be used - it should > probably be ifdef'd out when not on one of the ARM dev platforms which > suffer this weirdness. >
On Tue, Oct 30, 2012 at 07:04:38PM -0500, Rob Herring wrote: > On 10/30/2012 05:47 PM, Russell King - ARM Linux wrote: > > On Tue, Oct 30, 2012 at 12:21:20PM -0500, Rob Herring wrote: > >> Looking at this some more, arm64 doesn't need most of what's in gic.h. > >> The register defines should be moved into the .c file. The remaining > >> function declarations either are not needed (i.e. gic_init) or should > >> should be done like the handle_irq function pointer init. We don't want > >> to have platform code calling gic_cascade_irq or gic_raise_softirq > >> directly. > > > > Softirqs are about the SPIs which are used for SMP IPIs and platform > > specific wakeup of CPUs. And platform code _needs_ to specify the > > way IPIs are delivered on the platform. irqchip can't do that because > > irqchip knows nothing about SPIs (neither does genirq.) > > Right. v7 is unchanged, so the question is really only about how v8 will > do this. Hopefully, ARM is standardizing this for v8. We probably want > the gic (or other irqchip) to setup a raise_softirq function ptr on init > rather than having a direct call to gic_raise_softirq. We already have that, except it's up to platforms to setup that pointer via a function call into the SMP code - it's called set_smp_cross_call(). We could move that into the GIC code, as we don't have anyone with a GIC which doesn't use gic_raise_softirq. The only thing to remember is that there's non-SMP platforms with GICs.
On Wed, Oct 31, 2012 at 12:04:38AM +0000, Rob Herring wrote: > On 10/30/2012 05:47 PM, Russell King - ARM Linux wrote: > > On Tue, Oct 30, 2012 at 12:21:20PM -0500, Rob Herring wrote: > >> Looking at this some more, arm64 doesn't need most of what's in gic.h. > >> The register defines should be moved into the .c file. The remaining > >> function declarations either are not needed (i.e. gic_init) or should > >> should be done like the handle_irq function pointer init. We don't want > >> to have platform code calling gic_cascade_irq or gic_raise_softirq > >> directly. > > > > Softirqs are about the SPIs which are used for SMP IPIs and platform > > specific wakeup of CPUs. And platform code _needs_ to specify the > > way IPIs are delivered on the platform. irqchip can't do that because > > irqchip knows nothing about SPIs (neither does genirq.) > > Right. v7 is unchanged, so the question is really only about how v8 will > do this. Hopefully, ARM is standardizing this for v8. We probably want > the gic (or other irqchip) to setup a raise_softirq function ptr on init > rather than having a direct call to gic_raise_softirq. In my sample ARMv8 model code I have an older version of gic.c moved to drivers/irqchip and modified just for the arm64 needs. The gic_of_init() function calls set_smp_cross_call(git_raise_softirq). http://git.kernel.org/?p=linux/kernel/git/cmarinas/linux-aarch64.git;a=commitdiff;h=5cd20480f4d7b56160b3312df14fba3b2bda6798 The gic_secondary_init() is done from a CPU notifier as I wanted to separate this from the SoC code (even on arch/arm, all the calls to gic_secondary_init() are the same, so it could be factored out to a notifier or some function pointer set by gic.c). And let's assume there is no need for gic_cascade_irq().
On 10/31/2012 06:56 AM, Catalin Marinas wrote: > On Wed, Oct 31, 2012 at 12:04:38AM +0000, Rob Herring wrote: >> On 10/30/2012 05:47 PM, Russell King - ARM Linux wrote: >>> On Tue, Oct 30, 2012 at 12:21:20PM -0500, Rob Herring wrote: >>>> Looking at this some more, arm64 doesn't need most of what's in gic.h. >>>> The register defines should be moved into the .c file. The remaining >>>> function declarations either are not needed (i.e. gic_init) or should >>>> should be done like the handle_irq function pointer init. We don't want >>>> to have platform code calling gic_cascade_irq or gic_raise_softirq >>>> directly. >>> >>> Softirqs are about the SPIs which are used for SMP IPIs and platform >>> specific wakeup of CPUs. And platform code _needs_ to specify the >>> way IPIs are delivered on the platform. irqchip can't do that because >>> irqchip knows nothing about SPIs (neither does genirq.) >> >> Right. v7 is unchanged, so the question is really only about how v8 will >> do this. Hopefully, ARM is standardizing this for v8. We probably want >> the gic (or other irqchip) to setup a raise_softirq function ptr on init >> rather than having a direct call to gic_raise_softirq. > > In my sample ARMv8 model code I have an older version of gic.c moved to > drivers/irqchip and modified just for the arm64 needs. The gic_of_init() > function calls set_smp_cross_call(git_raise_softirq). > > http://git.kernel.org/?p=linux/kernel/git/cmarinas/linux-aarch64.git;a=commitdiff;h=5cd20480f4d7b56160b3312df14fba3b2bda6798 > > The gic_secondary_init() is done from a CPU notifier as I wanted to > separate this from the SoC code (even on arch/arm, all the calls to > gic_secondary_init() are the same, so it could be factored out to a > notifier or some function pointer set by gic.c). > > And let's assume there is no need for gic_cascade_irq(). What about other asm header dependencies? cpu_logical_map is needed for example. I guess I'll leave all those for now. Rob
On Wed, Oct 31, 2012 at 09:29:19AM -0500, Rob Herring wrote: > What about other asm header dependencies? cpu_logical_map is needed for > example. I guess I'll leave all those for now. That's one of the down-sides to moving this code out of arch/arm - your existing set of five patches adds to the required asm headers (eg, it now needs set_smp_cross_call() from ARMs asm/smp.h, whereas it didn't need that before) and any SMP architecture re-using the GIC will need to provide this function whether or not they use the SPI facility. Further comments against your patch set...
diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig index 45ceeb0..7bf52b2 100644 --- a/arch/arm/common/Kconfig +++ b/arch/arm/common/Kconfig @@ -1,11 +1,3 @@ -config ARM_GIC - bool - select IRQ_DOMAIN - select MULTI_IRQ_HANDLER - -config GIC_NON_BANKED - bool - config ARM_VIC bool select IRQ_DOMAIN diff --git a/arch/arm/common/Makefile b/arch/arm/common/Makefile index e8a4e58..4104b82 100644 --- a/arch/arm/common/Makefile +++ b/arch/arm/common/Makefile @@ -2,7 +2,6 @@ # Makefile for the linux kernel. # -obj-$(CONFIG_ARM_GIC) += gic.o obj-$(CONFIG_ARM_VIC) += vic.o obj-$(CONFIG_ICST) += icst.o obj-$(CONFIG_SA1111) += sa1111.o diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 88b0929..2d7f350 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -1,3 +1,11 @@ config IRQCHIP def_bool y depends on OF_IRQ + +config ARM_GIC + bool + select IRQ_DOMAIN + select MULTI_IRQ_HANDLER + +config GIC_NON_BANKED + bool diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 5148ffd..94118db 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_IRQCHIP) += irqchip.o obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o +obj-$(CONFIG_ARM_GIC) += irq-gic.o diff --git a/arch/arm/common/gic.c b/drivers/irqchip/irq-gic.c similarity index 100% rename from arch/arm/common/gic.c rename to drivers/irqchip/irq-gic.c diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c index f36d423..3f37397 100644 --- a/drivers/irqchip/irqchip.c +++ b/drivers/irqchip/irqchip.c @@ -14,6 +14,16 @@ #include "irqchip.h" static const struct of_device_id irqchip_of_match[] __initconst = { +#ifdef CONFIG_ARM_GIC + { + .compatible = "arm,cortex-a15-gic", + .data = gic_of_init, + }, + { + .compatible = "arm,cortex-a9-gic", + .data = gic_of_init, + }, +#endif #ifdef CONFIG_ARCH_BCM2835 { .compatible = "brcm,bcm2835-armctrl-ic", diff --git a/drivers/irqchip/irqchip.h b/drivers/irqchip/irqchip.h index 0a0d7af..62773ab3 100644 --- a/drivers/irqchip/irqchip.h +++ b/drivers/irqchip/irqchip.h @@ -14,5 +14,6 @@ int bcm2835_irqchip_init(struct device_node *node, struct device_node *parent); int armada_370_xp_mpic_of_init(struct device_node *node, struct device_node *parent); +int gic_of_init(struct device_node *node, struct device_node *parent); #endif