ARM: EXYNOS4: Adds External GIC

This patch adds external GIC io memory mapping
to support external GIC on EXYNOS4.

Signed-off-by Changhwan Youn <[email protected]>

---

?arch/arm/mach-exynos4/cpu.c ? ? ? ? ? ? ?| ? 10 ++++++++++
?arch/arm/mach-exynos4/include/mach/map.h | ? ?5 +++--
?arch/arm/plat-s5p/include/plat/map-s5p.h | ? ?5 +++--

?3 files changed, 16 insertions(+), 4 deletions(-)

This patch adds external GIC io memory mapping
to support external GIC on EXYNOS4.

Signed-off-by Changhwan Youn <[email protected]>

---

?arch/arm/mach-exynos4/cpu.c ? ? ? ? ? ? ?| ? 10 ++++++++++
?arch/arm/mach-exynos4/include/mach/map.h | ? ?5 +++--
?arch/arm/plat-s5p/include/plat/map-s5p.h | ? ?5 +++--

?3 files changed, 16 insertions(+), 4 deletions(-)

arch/arm/mach-exynos4/cpu.c +10 -0

index 1196f39..6a1ed74 100644
@@ -107,6 +107,16 @@ static struct map_desc exynos4_iodesc[] __initdata = {
 ? ? ? ? ? ? ? ?.pfn ? ? ? ? ? ?= __phys_to_pfn(EXYNOS4_PA_AUDSS),
 ? ? ? ? ? ? ? ?.length ? ? ? ? = SZ_1K,
 ? ? ? ? ? ? ? ?.type ? ? ? ? ? = MT_DEVICE,
+ ? ? ? }, {
+ ? ? ? ? ? ? ? .virtual ? ? ? ?= (unsigned long)S5P_VA_GIC_CPU,
+ ? ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(EXYNOS4_PA_GIC_CPU),
+ ? ? ? ? ? ? ? .length ? ? ? ? = SZ_64K,
+ ? ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
+ ? ? ? }, {
+ ? ? ? ? ? ? ? .virtual ? ? ? ?= (unsigned long)S5P_VA_GIC_DIST,
+ ? ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(EXYNOS4_PA_GIC_DIST),
+ ? ? ? ? ? ? ? .length ? ? ? ? = SZ_64K,
+ ? ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE,
 ? ? ? ?},
 ?};
 

arch/arm/mach-exynos4/include/mach/map.h +0 -0

index 57d8074..9d5d797 100644
@@ -61,10 +61,11 @@
 

?#define EXYNOS4_PA_COMBINER ? ? ? ? ? ?0x10448000

Quoted 2 lines

?#define EXYNOS4_PA_COMBINER ? ? ? ? ? ?0x10448000

Isn't this 0x10440000 if we are going to use External GIC?

Since Samsung EXYNOS4210 cannot support register banking in GIC, so needs to update CPU interface base address.
The 'gic_chip_data' is used for it, this patch moves gic_chip_data structure declaraton to arch/arm/include/asm/hardware/gic.h to use it.

Cc Russell King <[email protected]>

Signed-off-by Changhwan Youn <[email protected]>

---
 arch/arm/common/gic.c               |    6 ------
 arch/arm/include/asm/hardware/gic.h |    6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

arch/arm/common/gic.c +0 -6

index 4ddd0a6..23564ed 100644
@@ -38,12 +38,6 @@ static DEFINE_SPINLOCK(irq_controller_lock);
 /* Address of GIC 0 CPU interface */
 void __iomem *gic_cpu_base_addr __read_mostly;
 
-struct gic_chip_data {
-	unsigned int irq_offset;
-	void __iomem *dist_base;
-	void __iomem *cpu_base;
-};
-
 /*
  * Supported arch specific GIC irq extension.
  * Default make them NULL.

arch/arm/include/asm/hardware/gic.h +6 -1

index 0691f9d..435d3f8 100644
@@ -41,6 +41,12 @@ void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
 void gic_enable_ppi(unsigned int);
+
+struct gic_chip_data {
+	unsigned int irq_offset;
+	void __iomem *dist_base;
+	void __iomem *cpu_base;
+};
 #endif
 
 #endif
--

1.7.1

For full support of power modes, this patch adds implementation external GIC on EXYNOS4.

External GIC of Exynos4 cannot support register banking so several interrupt related code for CPU1 should be different from that of CPU0.

Signed-off-by Changhwan Youn <[email protected]>

---
 arch/arm/mach-exynos4/cpu.c                      |   10 ++++++++
 arch/arm/mach-exynos4/include/mach/entry-macro.S |    5 ++++
 arch/arm/mach-exynos4/include/mach/map.h         |    1 +
 arch/arm/mach-exynos4/platsmp.c                  |   27 +++++++++++++++++++++-
 4 files changed, 42 insertions(+), 1 deletions(-)

arch/arm/mach-exynos4/cpu.c +1 -0

index fa33294..40a866c 100644
@@ -16,6 +16,7 @@
 
 #include <asm/proc-fns.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <asm/hardware/gic.h>
 
 #include <plat/cpu.h>
 #include <plat/clock.h>

Hi Changhwan,

Quoted 6 lines

For full support of power modes, this patch adds implementation external GIC on EXYNOS4.

External GIC of Exynos4 cannot support register banking so several interrupt related code for CPU1 should be different from that of CPU0.

I just realized that patch has made it to mainline... Unfortunately, it seems quite broken to me:

Quoted 6 lines

Signed-off-by Changhwan Youn <[email protected]>

---
 arch/arm/mach-exynos4/cpu.c                      |   10 ++++++++
 arch/arm/mach-exynos4/include/mach/entry-macro.S |    5 ++++
 arch/arm/mach-exynos4/include/mach/map.h         |    1 +
 arch/arm/mach-exynos4/platsmp.c                  |   27

+++++++++++++++++++++-

Quoted 15 lines

 4 files changed, 42 insertions(+), 1 deletions(-)

arch/arm/mach-exynos4/cpu.c +1 -0

index fa33294..40a866c 100644
@@ -16,6 +16,7 @@
 
 #include <asm/proc-fns.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <asm/hardware/gic.h>
 
 #include <plat/cpu.h>
 #include <plat/clock.h>

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal) · line 168

Quoted 9 lines

arch/arm/mach-exynos4/cpu.c +6 -0

@@ -159,11 +160,20 @@ void __init exynos4_init_clocks(int xtal)
 	exynos4_setup_clocks();
 }
 
+static void exynos4_gic_irq_eoi(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+
+	gic_data->cpu_base = S5P_VA_GIC_CPU +
+			    (EXYNOS4_GIC_BANK_OFFSET *

smp_processor_id());

Here, you're overwriting a field that is shared among *all* the interrupts in the system. What if an interrupt comes up on another CPU? If you look at the implementation of gic_eoi_irq(), you'll definitely see the race.

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal) · line 176

Quoted 8 lines

arch/arm/mach-exynos4/cpu.c +3 -0

+}
+
 void __init exynos4_init_irq(void)
 {
 	int irq;
 
 	gic_init(0, IRQ_LOCALTIMER, S5P_VA_GIC_DIST, S5P_VA_GIC_CPU);
+	gic_arch_extn.irq_eoi = exynos4_gic_irq_eoi;

And here you're abusing the GIC extension feature.

I've also had a look at -next, and this has been extended further to support 4412. The problem with that is without banking, you're painfully working around the GIC driver. At that stage, I wonder if you wouldn't be better off with a separate driver instead of abusing the existing

Quoted 3 lines · Marc Zyngier

Hi Changhwan,

Hi Marc,

(Cc'ed Will Deacon and Russell King)

Quoted 44 lines · Changhwan Youn

Quoted 6 lines

For full support of power modes, this patch adds implementation external GIC on EXYNOS4.

External GIC of Exynos4 cannot support register banking so several interrupt related code for CPU1 should be different from that of CPU0.

I just realized that patch has made it to mainline... Unfortunately, it seems quite broken to me:

Quoted 6 lines

Signed-off-by Changhwan Youn <[email protected]>

---
 arch/arm/mach-exynos4/cpu.c                      |   10 ++++++++
 arch/arm/mach-exynos4/include/mach/entry-macro.S |    5 ++++
 arch/arm/mach-exynos4/include/mach/map.h         |    1 +
 arch/arm/mach-exynos4/platsmp.c                  |   27

+++++++++++++++++++++-

Quoted 15 lines

 4 files changed, 42 insertions(+), 1 deletions(-)

arch/arm/mach-exynos4/cpu.c +1 -0

index fa33294..40a866c 100644
@@ -16,6 +16,7 @@
 
 #include <asm/proc-fns.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <asm/hardware/gic.h>
 
 #include <plat/cpu.h>
 #include <plat/clock.h>

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal) · line 168

Quoted 9 lines

arch/arm/mach-exynos4/cpu.c +6 -0

@@ -159,11 +160,20 @@ void __init exynos4_init_clocks(int xtal)
 	exynos4_setup_clocks();
 }
 
+static void exynos4_gic_irq_eoi(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+
+	gic_data->cpu_base = S5P_VA_GIC_CPU +
+			    (EXYNOS4_GIC_BANK_OFFSET *

smp_processor_id());

Quoted 5 lines

Here, you're overwriting a field that is shared among *all* the interrupts in the system. What if an interrupt comes up on another CPU? If you look at the implementation of gic_eoi_irq(), you'll definitely see the race.

Hmm...as you can see in git log, the EXYNOS4210 cannot support register banking in GIC so this is needed.

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos4/cpu.c +2 -0

+}
+
 void __init exynos4_init_irq(void)
 {
 	int irq;
 

Quoted 5 lines

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal) · line 176

Quoted 2 lines

arch/arm/mach-exynos4/cpu.c +1 -0

 	gic_init(0, IRQ_LOCALTIMER, S5P_VA_GIC_DIST, S5P_VA_GIC_CPU);
+	gic_arch_extn.irq_eoi = exynos4_gic_irq_eoi;

And here you're abusing the GIC extension feature.

I think gic_arch_extn.irq_eoi can be overwritten in each architecture to support own specific extensions like in the EXYNOS4 case.

Quoted 4 lines

I've also had a look at -next, and this has been extended further to support 4412. The problem with that is without banking, you're painfully working around the GIC driver. At that stage, I wonder if you wouldn't be better off with a separate driver instead of abusing the existing

one...

Quoted 1 line

Well, in this case, you mean separate driver is better to us even though there is a gic driver in arch/arm/common? I don't think so because separate driver will probably have many duplicated codes and if common gic driver can support every silicons which have different version's gic it's better to us and should do.

Quoted 52 lines · Kukjin Kim

Quoted 3 lines · Marc Zyngier

Hi Changhwan,

Hi Marc,

(Cc'ed Will Deacon and Russell King)

Quoted 44 lines · Changhwan Youn

Quoted 6 lines

For full support of power modes, this patch adds implementation external GIC on EXYNOS4.

External GIC of Exynos4 cannot support register banking so several interrupt related code for CPU1 should be different from that of CPU0.

I just realized that patch has made it to mainline... Unfortunately, it seems quite broken to me:

Quoted 6 lines

Signed-off-by Changhwan Youn <[email protected]>

---
 arch/arm/mach-exynos4/cpu.c                      |   10 ++++++++
 arch/arm/mach-exynos4/include/mach/entry-macro.S |    5 ++++
 arch/arm/mach-exynos4/include/mach/map.h         |    1 +
 arch/arm/mach-exynos4/platsmp.c                  |   27

+++++++++++++++++++++-

Quoted 15 lines

 4 files changed, 42 insertions(+), 1 deletions(-)

arch/arm/mach-exynos4/cpu.c +1 -0

index fa33294..40a866c 100644
@@ -16,6 +16,7 @@
 
 #include <asm/proc-fns.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <asm/hardware/gic.h>
 
 #include <plat/cpu.h>
 #include <plat/clock.h>

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal) · line 168

Quoted 9 lines

arch/arm/mach-exynos4/cpu.c +6 -0

@@ -159,11 +160,20 @@ void __init exynos4_init_clocks(int xtal)
 	exynos4_setup_clocks();
 }
 
+static void exynos4_gic_irq_eoi(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+
+	gic_data->cpu_base = S5P_VA_GIC_CPU +
+			    (EXYNOS4_GIC_BANK_OFFSET *

smp_processor_id());

Quoted 7 lines

Quoted 5 lines

Here, you're overwriting a field that is shared among *all* the interrupts in the system. What if an interrupt comes up on another CPU? If you look at the implementation of gic_eoi_irq(), you'll definitely see the race.

Hmm...as you can see in git log, the EXYNOS4210 cannot support register banking in GIC so this is needed.

I don't dispute the need. I claim that the implementation is wrong, and will fail given the right timings.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-exynos4/cpu.c +2 -0

+}
+
 void __init exynos4_init_irq(void)
 {
 	int irq;
 

Quoted 7 lines

Quoted 5 lines

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal) · line 176

Quoted 2 lines

arch/arm/mach-exynos4/cpu.c +1 -0

 	gic_init(0, IRQ_LOCALTIMER, S5P_VA_GIC_DIST, S5P_VA_GIC_CPU);
+	gic_arch_extn.irq_eoi = exynos4_gic_irq_eoi;

And here you're abusing the GIC extension feature.

I think gic_arch_extn.irq_eoi can be overwritten in each architecture to support own specific extensions like in the EXYNOS4 case.

Sure. My point is you are diverting the GIC extension from its purpose, which is mostly to be able to control wake-up sources (as for example in the Tegra case). Here, you use this hooks to work around the fact that the GIC driver is written with banking in mind, which is quite a different thing.

Quoted 11 lines

Quoted 4 lines

I've also had a look at -next, and this has been extended further to support 4412. The problem with that is without banking, you're painfully working around the GIC driver. At that stage, I wonder if you wouldn't be better off with a separate driver instead of abusing the existing

one...

Quoted 1 line

Well, in this case, you mean separate driver is better to us even though there is a gic driver in arch/arm/common? I don't think so because separate driver will probably have many duplicated codes and if common gic driver can support every silicons which have different version's gic it's better to us and should do.

If you really insist on using the GIC common code, then I'd suggest to adapt it to your needs instead of working around the problem. What about making cpu_base a percpu field inside struct gic_chip_data? No hook abuse, and no race conditions. You could also do that for dist_base, as it looks to be required for the 4412.

From da57c3979543fcef47f1ae22b5224a1d7a96aa2d Mon Sep 17 00:00:00 2001

Quoted 10 lines · Marc Zyngier

Quoted 9 lines · Kukjin Kim

Quoted 3 lines · Marc Zyngier

Hi Changhwan,

Hi Marc,

(Cc'ed Will Deacon and Russell King)

Quoted 1 line · Changhwan Youn

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

For full support of power modes, this patch adds implementation external GIC on EXYNOS4.

External GIC of Exynos4 cannot support register banking so several interrupt related code for CPU1 should be different from that of CPU0.

I just realized that patch has made it to mainline... Unfortunately,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

seems quite broken to me:

Quoted 6 lines

Signed-off-by Changhwan Youn <[email protected]>

---
 arch/arm/mach-exynos4/cpu.c                      |   10 ++++++++
 arch/arm/mach-exynos4/include/mach/entry-macro.S |    5 ++++
 arch/arm/mach-exynos4/include/mach/map.h         |    1 +
 arch/arm/mach-exynos4/platsmp.c                  |   27

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

+++++++++++++++++++++-

Review

Quoted 3 lines

 4 files changed, 42 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

index fa33294..40a866c 100644

arch/arm/mach-exynos4/cpu.c +1 -0

@@ -16,6 +16,7 @@
 
 #include <asm/proc-fns.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <asm/hardware/gic.h>
 
 #include <plat/cpu.h>
 #include <plat/clock.h>

@@ -159,11 +160,20 @@ void __init exynos4_init_clocks(int xtal)
 	exynos4_setup_clocks();
 }
 

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal)

Quoted 3 lines

arch/arm/mach-exynos4/cpu.c +3 -0

+static void exynos4_gic_irq_eoi(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data =

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal)

Quoted 3 lines

arch/arm/mach-exynos4/cpu.c +3 -0

+
+	gic_data->cpu_base = S5P_VA_GIC_CPU +
+			    (EXYNOS4_GIC_BANK_OFFSET *

smp_processor_id());

Here, you're overwriting a field that is shared among *all* the interrupts in the system. What if an interrupt comes up on another

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

If you look at the implementation of gic_eoi_irq(), you'll definitely see the race.

Hmm...as you can see in git log, the EXYNOS4210 cannot support register banking in GIC so this is needed.

I don't dispute the need. I claim that the implementation is wrong, and will fail given the right timings.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal)

Quoted 7 lines

arch/arm/mach-exynos4/cpu.c +2 -0

+}
+
 void __init exynos4_init_irq(void)
 {
 	int irq;
 
 	gic_init(0, IRQ_LOCALTIMER, S5P_VA_GIC_DIST,

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

arch/arm/mach-exynos4/cpu.c void __init exynos4_init_clocks(int xtal) · line 176

Quoted 1 line

arch/arm/mach-exynos4/cpu.c +1 -0

+	gic_arch_extn.irq_eoi = exynos4_gic_irq_eoi;

And here you're abusing the GIC extension feature.

I think gic_arch_extn.irq_eoi can be overwritten in each architecture

Quoted 10 lines

Quoted 1 line

support own specific extensions like in the EXYNOS4 case.

Sure. My point is you are diverting the GIC extension from its purpose, which is mostly to be able to control wake-up sources (as for example in the Tegra case). Here, you use this hooks to work around the fact that the GIC driver is written with banking in mind, which is quite a different thing.

Quoted 2 lines

Quoted 2 lines

I've also had a look at -next, and this has been extended further to support 4412. The problem with that is without banking, you're

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

working around the GIC driver. At that stage, I wonder if you wouldn't be better off with a separate driver instead of abusing the existing

one...

Quoted 1 line

Well, in this case, you mean separate driver is better to us even

Quoted 1 line

Quoted 1 line

there is a gic driver in arch/arm/common? I don't think so because

Quoted 1 line

Quoted 1 line

driver will probably have many duplicated codes and if common gic

Quoted 1 line

Quoted 1 line

support every silicons which have different version's gic it's better

Quoted 7 lines

Quoted 1 line

and should do.

If you really insist on using the GIC common code, then I'd suggest to adapt it to your needs instead of working around the problem. What about making cpu_base a percpu field inside struct gic_chip_data? No hook abuse, and no race conditions. You could also do that for dist_base, as it looks to be required for the 4412.

So to make my suggestion completely clear, here's a patch I'm now carrying in my tree. It's only been test compiled on EXYNOS4, but works nicely on my 11MP. It turns both dist_base and cpu_base into per-cpu variables, removes these callbacks, removes your private copy of gic_cpu_init, and makes struct gic_chip_data private again.

What do you think?

So to make my suggestion completely clear, here's a patch I'm now carrying in my tree. It's only been test compiled on EXYNOS4, but works nicely on my 11MP. It turns both dist_base and cpu_base into per-cpu variables, removes these callbacks, removes your private copy of gic_cpu_init, and makes struct gic_chip_data private again.

What do you think?

Subject: [PATCH] ARM: gic: allow GIC to support non-banked setups

The GIC support code is heavily using the fact that hardware implementations are exposing banked registers. Unfortunately, it looks like at least one GIC implementation (EXYNOS4) offers both the distributor and the CPU interfaces at different addresses, depending on the CPU.

This problem is solved by turning the distributor and CPU interface addresses into per-cpu variables. The EXYNOS4 code is updated not to mess with the GIC internals while handling interrupts, and struct gic_chip_data is back to being private.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/common/gic.c               |   76 +++++++++++++++++++++++++++--------
 arch/arm/include/asm/hardware/gic.h |   17 +------
 arch/arm/mach-exynos4/cpu.c         |   14 ------
 arch/arm/mach-exynos4/platsmp.c     |   28 ++-----------
 4 files changed, 66 insertions(+), 69 deletions(-)

arch/arm/common/gic.c +3 -0

index b574931..c7521dd 100644
@@ -37,6 +37,20 @@
 #include <asm/mach/irq.h>
 #include <asm/hardware/gic.h>
 
+struct gic_chip_data {
+	unsigned int irq_offset;
+	void __percpu __iomem **dist_base;

Hi Marc,

Quoted 6 lines · Marc Zyngier

So to make my suggestion completely clear, here's a patch I'm now carrying in my tree. It's only been test compiled on EXYNOS4, but works nicely on my 11MP. It turns both dist_base and cpu_base into per-cpu variables, removes these callbacks, removes your private copy of gic_cpu_init, and makes struct gic_chip_data private again.

Quoted 1 line

What do you think?

This looks like the right sort of idea, although I'm deeply suspicious about per-cpu base addresses for the GIC distributor. It would be nice if the Samsung guys can elaborate on what's going on here. Few comments inline.

Quoted 29 lines

Subject: [PATCH] ARM: gic: allow GIC to support non-banked setups

The GIC support code is heavily using the fact that hardware implementations are exposing banked registers. Unfortunately, it looks like at least one GIC implementation (EXYNOS4) offers both the distributor and the CPU interfaces at different addresses, depending on the CPU.

This problem is solved by turning the distributor and CPU interface addresses into per-cpu variables. The EXYNOS4 code is updated not to mess with the GIC internals while handling interrupts, and struct gic_chip_data is back to being private.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/common/gic.c               |   76 +++++++++++++++++++++++++++--------
 arch/arm/include/asm/hardware/gic.h |   17 +------
 arch/arm/mach-exynos4/cpu.c         |   14 ------
 arch/arm/mach-exynos4/platsmp.c     |   28 ++-----------
 4 files changed, 66 insertions(+), 69 deletions(-)

arch/arm/common/gic.c +0 -0

index b574931..c7521dd 100644
@@ -37,6 +37,20 @@
 #include <asm/mach/irq.h>
 #include <asm/hardware/gic.h>
 

arch/arm/common/gic.c line 52

Quoted 13 lines

arch/arm/common/gic.c +13 -0

+struct gic_chip_data {
+	unsigned int irq_offset;
+	void __percpu __iomem **dist_base;
+	void __percpu __iomem **cpu_base;
+#ifdef CONFIG_CPU_PM
+	u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
+	u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
+	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
+	u32 __percpu *saved_ppi_enable;
+	u32 __percpu *saved_ppi_conf;
+#endif
+	unsigned int gic_irqs;
+};

I think you can use DECLARE_PER_CPU(void __iomem *dist_base) etc. instead.

arch/arm/common/gic.c line 56

Quoted 5 lines

arch/arm/common/gic.c +2 -1

+static void __cpuinit exynos4_secondary_init(unsigned int cpu)
 {
 	void __iomem *dist_base = S5P_VA_GIC_DIST +
-				(gic_bank_offset * smp_processor_id());
+				(gic_bank_offset * cpu_logical_map(cpu));

Again, I'm deeply suspicious of this code :) Is there not a common memory alias for the distributor across all of the CPUs?

Quoted 9 lines · Will Deacon

Hi Marc,

Quoted 6 lines · Marc Zyngier

So to make my suggestion completely clear, here's a patch I'm now carrying in my tree. It's only been test compiled on EXYNOS4, but works nicely on my 11MP. It turns both dist_base and cpu_base into per-cpu variables, removes these callbacks, removes your private copy of gic_cpu_init, and makes struct gic_chip_data private again.

Quoted 5 lines

Quoted 1 line

What do you think?

This looks like the right sort of idea, although I'm deeply suspicious about per-cpu base addresses for the GIC distributor. It would be nice if the Samsung guys can elaborate on what's going on here. Few comments inline.

Yeah, this seem odd, specially as what's currently in mainline doesn't try to play tricks with the distributor. It looks like having a per-core distributor address is required for PPI usage on these SoCs, but the commit message is uninformative at best:

https://github.com/sfrothwell/linux-
next/commit/637c2afa57ec9cd0ddc8879ea0cda4d8835ba71d#arch/arm/mach-exynos4/cpu.c

Again, comments from Samsung people would be much appreciated.

Quoted 29 lines

Quoted 29 lines

Subject: [PATCH] ARM: gic: allow GIC to support non-banked setups

The GIC support code is heavily using the fact that hardware implementations are exposing banked registers. Unfortunately, it looks like at least one GIC implementation (EXYNOS4) offers both the distributor and the CPU interfaces at different addresses, depending on the CPU.

This problem is solved by turning the distributor and CPU interface addresses into per-cpu variables. The EXYNOS4 code is updated not to mess with the GIC internals while handling interrupts, and struct gic_chip_data is back to being private.

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm/common/gic.c               |   76 +++++++++++++++++++++++++++--------
 arch/arm/include/asm/hardware/gic.h |   17 +------
 arch/arm/mach-exynos4/cpu.c         |   14 ------
 arch/arm/mach-exynos4/platsmp.c     |   28 ++-----------
 4 files changed, 66 insertions(+), 69 deletions(-)

arch/arm/common/gic.c +0 -0

index b574931..c7521dd 100644
@@ -37,6 +37,20 @@
 #include <asm/mach/irq.h>
 #include <asm/hardware/gic.h>
 

Quoted 15 lines

arch/arm/common/gic.c line 52

Quoted 13 lines

arch/arm/common/gic.c +13 -0

+struct gic_chip_data {
+	unsigned int irq_offset;
+	void __percpu __iomem **dist_base;
+	void __percpu __iomem **cpu_base;
+#ifdef CONFIG_CPU_PM
+	u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
+	u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
+	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
+	u32 __percpu *saved_ppi_enable;
+	u32 __percpu *saved_ppi_conf;
+#endif
+	unsigned int gic_irqs;
+};

I think you can use DECLARE_PER_CPU(void __iomem *dist_base) etc. instead.

Unfortunately, you can't. That would be nice though... ;-)

Quoted 8 lines

arch/arm/common/gic.c line 56

Quoted 5 lines

arch/arm/common/gic.c +2 -1

+static void __cpuinit exynos4_secondary_init(unsigned int cpu)
 {
 	void __iomem *dist_base = S5P_VA_GIC_DIST +
-				(gic_bank_offset * smp_processor_id());
+				(gic_bank_offset * cpu_logical_map(cpu));

Again, I'm deeply suspicious of this code :) Is there not a common memory alias for the distributor across all of the CPUs?

Kukjin, could you please comment on the presence of a common memory region for the distributor? This seem quite odd...

Thanks,

	M.

--
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 2 lines

Kukjin, could you please comment on the presence of a common memory region for the distributor? This seem quite odd...

Some registers in Distributor are banked for PPI and SGI support (banked interrupts). The register for pending and enable status of these interrupts are banked.

Marc, I think the approach in your patch is much better than mine if it doesn't hurt the performance of other platforms which use the common gic code.

I'll re-work the exynos4 interrupt code based on your patch though I'm not sure that it's possible to be merged in merge window.

Hi Changwan,

Quoted 3 lines · Changhwan Youn

Quoted 2 lines

Kukjin, could you please comment on the presence of a common memory region for the distributor? This seem quite odd...

Quoted 1 line

Some registers in Distributor are banked for PPI and SGI support (banked

interrupts).

Quoted 2 lines

The register for pending and enable status of these interrupts are banked.

Right, that explains it then.

Quoted 1 line

Marc, I think the approach in your patch is much better than mine if it

Quoted 1 line

the performance of other platforms which use the common gic code.

It probably doesn't hurt the general case too much (I expect a bit more pressure on the d-cache because of the per-cpu stuff, but nothing to be too worried about).

Quoted 2 lines

I'll re-work the exynos4 interrupt code based on your patch though I'm not sure that it's possible to be merged in merge window.

My main concern at the moment is that mainline is broken as far as EXYNOS4 is concerned (there's a race with the EOI hook), so that should get fixed first.

Quoted 7 lines · Marc Zyngier

Hi Changwan,

Quoted 3 lines · Changhwan Youn

Quoted 2 lines

Kukjin, could you please comment on the presence of a common memory region for the distributor? This seem quite odd...

Quoted 8 lines

Quoted 1 line

Some registers in Distributor are banked for PPI and SGI support (banked

interrupts).

Quoted 2 lines

The register for pending and enable status of these interrupts are banked.

Right, that explains it then.

Quoted 1 line

Marc, I think the approach in your patch is much better than mine if it

doesn't hurt

Quoted 6 lines

Quoted 1 line

the performance of other platforms which use the common gic code.

It probably doesn't hurt the general case too much (I expect a bit more pressure on the d-cache because of the per-cpu stuff, but nothing to be too worried about).

Quoted 7 lines

Quoted 2 lines

I'll re-work the exynos4 interrupt code based on your patch though I'm not sure that it's possible to be merged in merge window.

My main concern at the moment is that mainline is broken as far as EXYNOS4 is concerned (there's a race with the EOI hook), so that should get fixed first.

Hi Marc,

OK. I agree with Will and your opinions and I think Changhwan can fix it as per your suggestion, but he needs fixed/updated regarding gic codes to avoid re-work and conflicts with others. So it would be better to us if he could fix it after merging your patches even probably at the end of upcoming merge window. I hope he can do it before v3.2-rc1.

Quoted 9 lines · Kukjin Kim

Quoted 8 lines · Marc Zyngier

Hi Changwan,

Quoted 4 lines · Changhwan Youn

Quoted 2 lines

Kukjin, could you please comment on the presence of a common memory region for the distributor? This seem quite odd...

Some registers in Distributor are banked for PPI and SGI support

Quoted 7 lines

Quoted 7 lines

interrupts).

Quoted 2 lines

The register for pending and enable status of these interrupts are banked.

Right, that explains it then.

Quoted 1 line

Marc, I think the approach in your patch is much better than mine if

Quoted 10 lines

doesn't hurt

Quoted 9 lines

Quoted 1 line

the performance of other platforms which use the common gic code.

It probably doesn't hurt the general case too much (I expect a bit more pressure on the d-cache because of the per-cpu stuff, but nothing to be too worried about).

Quoted 2 lines

I'll re-work the exynos4 interrupt code based on your patch though I'm not sure that it's possible to be merged in merge window.

Quoted 7 lines

Quoted 4 lines

My main concern at the moment is that mainline is broken as far as EXYNOS4 is concerned (there's a race with the EOI hook), so that should get fixed first.

Hi Marc,

OK. I agree with Will and your opinions and I think Changhwan can fix it

Quoted 1 line

per your suggestion, but he needs fixed/updated regarding gic codes to

Quoted 1 line

re-work and conflicts with others. So it would be better to us if he

Quoted 1 line

fix it after merging your patches even probably at the end of upcoming

Quoted 1 line

window. I hope he can do it before v3.2-rc1.

Right. So this damned thing has made it to mainline in its full glory. Furthermore, the MCT code is also broken, as it uses the old PPI API (doesn't even compile).

Can we please fix this as soon as possible? I posted patches for both a while ago, with almost no reaction...

Quoted 9 lines · Will Deacon

Review

Quoted 5 lines · Marc Zyngier

(quoted diff) +2 -1

+static void __cpuinit exynos4_secondary_init(unsigned int cpu)
 {
 	void __iomem *dist_base = S5P_VA_GIC_DIST +
-				(gic_bank_offset * smp_processor_id());
+				(gic_bank_offset * cpu_logical_map(cpu));

Again, I'm deeply suspicious of this code :) Is there not a common memory alias for the distributor across all of the CPUs?

Kukjin, could you please comment on the presence of a common memory region for the distributor? This seem quite odd...

Quoted 11 lines · Marc Zyngier

Quoted 9 lines · Will Deacon

Review

Quoted 5 lines · Marc Zyngier

(quoted diff) +2 -1

+static void __cpuinit exynos4_secondary_init(unsigned int cpu)
 {
 	void __iomem *dist_base = S5P_VA_GIC_DIST +
-				(gic_bank_offset * smp_processor_id());
+				(gic_bank_offset * cpu_logical_map(cpu));

Again, I'm deeply suspicious of this code :) Is there not a common memory alias for the distributor across all of the CPUs?

Quoted 2 lines

Kukjin, could you please comment on the presence of a common memory region for the distributor? This seem quite odd...

It's not odd when you consider that there's per-CPU registers within the GIC distributor as well (for the first 32 GIC IRQs) as the per-CPU GIC CPU interfaces.

This patch adds implementation External GIC on EXYNOS4 SoC.

Note: need to update timer codes for supporting old type of EXYNOS4 SoCs.

[PATCH 1/7] ARM: EXYNOS4: Add external GIC io memory mapping
[PATCH 2/7] ARM: EXYNOS4: modify interrupt mappings for external GIC [PATCH 3/7] ARM: EXYNOS4: set the affinity of mct1 interrupt using IRQ_MCT_L1 [PATCH 4/7] ARM: GIC: move gic_chip_data structure declaration to header [PATCH 5/7] ARM: EXYNOS4: Add support external GIC
[PATCH 6/7] ARM: EXYNOS4: Remove clock event timers using ARM private timers [PATCH 7/7] ARM: EXYNOS4: Add chained enrty/exit function to uart interrupt handler --
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html

This patch adds implementation External GIC on EXYNOS4 SoC.

Note: need to update timer codes for supporting old type of EXYNOS4 SoCs.

[PATCH 1/7] ARM: EXYNOS4: Add external GIC io memory mapping [PATCH 2/7] ARM: EXYNOS4: modify interrupt mappings for external GIC [PATCH 3/7] ARM: EXYNOS4: set the affinity of mct1 interrupt using

[PATCH 4/7] ARM: GIC: move gic_chip_data structure declaration to header [PATCH 5/7] ARM: EXYNOS4: Add support external GIC
[PATCH 6/7] ARM: EXYNOS4: Remove clock event timers using ARM private

[PATCH 7/7] ARM: EXYNOS4: Add chained enrty/exit function to uart

handler