On 06/10/11 09:18, Marc Zyngier wrote:
> On 06/10/11 07:30, Kukjin Kim wrote:
>> Marc Zyngier wrote:
>>>
>>> Hi Changhwan,
>>>
>> Hi Marc,
>>
>> (Cc'ed Will Deacon and Russell King)
>>
>>> On 20/06/11 08:34, Changhwan Youn wrote:
>>>> 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:
>>>
>>>> Signed-off-by: Changhwan Youn <chaos.youn@xxxxxxxxxxx>
>>>> ---
>>>> 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(-)
>>>>
>>>> diff --git a/arch/arm/mach-exynos4/cpu.c b/arch/arm/mach-exynos4/cpu.c
>>>> index fa33294..40a866c 100644
>>>> --- a/arch/arm/mach-exynos4/cpu.c
>>>> +++ b/arch/arm/mach-exynos4/cpu.c
>>>> @@ -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();
>>>> }
>>>>
>>>> +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.
>>>
>> 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.
>
>>>> +}
>>>> +
>>>> 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 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.
>
>>> 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...
>>>
>> 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.
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?
M.
--
Jazz is not dead. It just smells funny...>From da57c3979543fcef47f1ae22b5224a1d7a96aa2d Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@xxxxxxx>
Date: Fri, 7 Oct 2011 10:23:31 +0100
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 <marc.zyngier@xxxxxxx>
---
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(-)
diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index b574931..c7521dd 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -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;
+ 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;
+};
+
static DEFINE_RAW_SPINLOCK(irq_controller_lock);
/* Address of GIC 0 CPU interface */
@@ -61,16 +75,26 @@ struct irq_chip gic_arch_extn = {
static struct gic_chip_data gic_data[MAX_GIC_NR] __read_mostly;
+static inline void __iomem *gic_data_dist_base(struct gic_chip_data *data)
+{
+ return *__this_cpu_ptr(data->dist_base);
+}
+
static inline void __iomem *gic_dist_base(struct irq_data *d)
{
struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
- return gic_data->dist_base;
+ return gic_data_dist_base(gic_data);
+}
+
+static inline void __iomem *gic_data_cpu_base(struct gic_chip_data *data)
+{
+ return *__this_cpu_ptr(data->cpu_base);
}
static inline void __iomem *gic_cpu_base(struct irq_data *d)
{
struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
- return gic_data->cpu_base;
+ return gic_data_cpu_base(gic_data);
}
static inline unsigned int gic_irq(struct irq_data *d)
@@ -243,7 +267,7 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
chained_irq_enter(chip, desc);
raw_spin_lock(&irq_controller_lock);
- status = readl_relaxed(chip_data->cpu_base + GIC_CPU_INTACK);
+ status = readl_relaxed(gic_data_cpu_base(chip_data) + GIC_CPU_INTACK);
raw_spin_unlock(&irq_controller_lock);
gic_irq = (status & 0x3ff);
@@ -287,7 +311,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
{
unsigned int gic_irqs, irq_limit, i;
u32 cpumask;
- void __iomem *base = gic->dist_base;
+ void __iomem *base = gic_data_dist_base(gic);
u32 cpu = 0;
u32 nrppis = 0, ppi_base = 0;
@@ -380,8 +404,8 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
{
- void __iomem *dist_base = gic->dist_base;
- void __iomem *base = gic->cpu_base;
+ void __iomem *dist_base = gic_data_dist_base(gic);
+ void __iomem *base = gic_data_cpu_base(gic);
int i;
/*
@@ -418,7 +442,7 @@ static void gic_dist_save(unsigned int gic_nr)
BUG();
gic_irqs = gic_data[gic_nr].gic_irqs;
- dist_base = gic_data[gic_nr].dist_base;
+ dist_base = gic_data_dist_base(&gic_data[gic_nr]);
if (!dist_base)
return;
@@ -453,7 +477,7 @@ static void gic_dist_restore(unsigned int gic_nr)
BUG();
gic_irqs = gic_data[gic_nr].gic_irqs;
- dist_base = gic_data[gic_nr].dist_base;
+ dist_base = gic_data_dist_base(&gic_data[gic_nr]);
if (!dist_base)
return;
@@ -489,8 +513,8 @@ static void gic_cpu_save(unsigned int gic_nr)
if (gic_nr >= MAX_GIC_NR)
BUG();
- dist_base = gic_data[gic_nr].dist_base;
- cpu_base = gic_data[gic_nr].cpu_base;
+ dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+ cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
if (!dist_base || !cpu_base)
return;
@@ -515,8 +539,8 @@ static void gic_cpu_restore(unsigned int gic_nr)
if (gic_nr >= MAX_GIC_NR)
BUG();
- dist_base = gic_data[gic_nr].dist_base;
- cpu_base = gic_data[gic_nr].cpu_base;
+ dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+ cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
if (!dist_base || !cpu_base)
return;
@@ -588,12 +612,24 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
void __iomem *dist_base, void __iomem *cpu_base)
{
struct gic_chip_data *gic;
+ int cpu;
BUG_ON(gic_nr >= MAX_GIC_NR);
gic = &gic_data[gic_nr];
- gic->dist_base = dist_base;
- gic->cpu_base = cpu_base;
+ gic->dist_base = alloc_percpu(void __iomem *);
+ gic->cpu_base = alloc_percpu(void __iomem *);
+ if (WARN_ON(!gic->dist_base || !gic->cpu_base)) {
+ free_percpu(gic->dist_base);
+ free_percpu(gic->cpu_base);
+ return;
+ }
+
+ for_each_possible_cpu(cpu) {
+ *per_cpu_ptr(gic->dist_base, cpu) = dist_base;
+ *per_cpu_ptr(gic->cpu_base, cpu) = cpu_base;
+ }
+
gic->irq_offset = (irq_start - 1) & ~31;
if (gic_nr == 0)
@@ -605,11 +641,17 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
gic_pm_init(gic);
}
-void __cpuinit gic_secondary_init(unsigned int gic_nr)
+void __cpuinit gic_secondary_init_base(unsigned int gic_nr,
+ void __iomem *dist_base,
+ void __iomem *cpu_base)
{
BUG_ON(gic_nr >= MAX_GIC_NR);
- gic_cpu_init(&gic_data[gic_nr]);
+ if (dist_base)
+ *__this_cpu_ptr(gic_data[gic_nr].dist_base) = dist_base;
+ if (cpu_base)
+ *__this_cpu_ptr(gic_data[gic_nr].cpu_base) = cpu_base;
+ gic_cpu_init(&gic_data[gic_nr]);
}
#ifdef CONFIG_SMP
@@ -629,6 +671,6 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
dsb();
/* this always happens on GIC0 */
- writel_relaxed(map << 16 | irq, gic_data[0].dist_base + GIC_DIST_SOFTINT);
+ writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
}
#endif
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 2b7ec2b..d45d78b 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -36,24 +36,13 @@
extern struct irq_chip gic_arch_extn;
void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
-void gic_secondary_init(unsigned int);
+void gic_secondary_init_base(unsigned int, void __iomem *, void __iomem *);
void gic_handle_irq(struct pt_regs *regs);
void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
-struct gic_chip_data {
- unsigned int irq_offset;
- void __iomem *dist_base;
- void __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;
-};
+#define gic_secondary_init(n) gic_secondary_init_base((n), NULL, NULL)
+
#endif
#endif
diff --git a/arch/arm/mach-exynos4/cpu.c b/arch/arm/mach-exynos4/cpu.c
index c682887..09e2760 100644
--- a/arch/arm/mach-exynos4/cpu.c
+++ b/arch/arm/mach-exynos4/cpu.c
@@ -204,17 +204,6 @@ void __init exynos4_init_clocks(int xtal)
exynos4_setup_clocks();
}
-static void exynos4_gic_irq_fix_base(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 +
- (gic_bank_offset * smp_processor_id());
-
- gic_data->dist_base = S5P_VA_GIC_DIST +
- (gic_bank_offset * smp_processor_id());
-}
-
void __init exynos4_init_irq(void)
{
int irq;
@@ -222,9 +211,6 @@ void __init exynos4_init_irq(void)
gic_bank_offset = soc_is_exynos4412() ? 0x4000 : 0x8000;
gic_init(0, IRQ_PPI(0), S5P_VA_GIC_DIST, S5P_VA_GIC_CPU);
- gic_arch_extn.irq_eoi = exynos4_gic_irq_fix_base;
- gic_arch_extn.irq_unmask = exynos4_gic_irq_fix_base;
- gic_arch_extn.irq_mask = exynos4_gic_irq_fix_base;
for (irq = 0; irq < MAX_COMBINER_NR; irq++) {
diff --git a/arch/arm/mach-exynos4/platsmp.c b/arch/arm/mach-exynos4/platsmp.c
index 56377d2..76cb2ba 100644
--- a/arch/arm/mach-exynos4/platsmp.c
+++ b/arch/arm/mach-exynos4/platsmp.c
@@ -67,39 +67,19 @@ static void __iomem *scu_base_addr(void)
static DEFINE_SPINLOCK(boot_lock);
-static void __cpuinit exynos4_gic_secondary_init(void)
+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));
void __iomem *cpu_base = S5P_VA_GIC_CPU +
- (gic_bank_offset * smp_processor_id());
- int i;
-
- /*
- * Deal with the banked PPI and SGI interrupts - disable all
- * PPI interrupts, ensure all SGI interrupts are enabled.
- */
- __raw_writel(0xffff0000, dist_base + GIC_DIST_ENABLE_CLEAR);
- __raw_writel(0x0000ffff, dist_base + GIC_DIST_ENABLE_SET);
+ (gic_bank_offset * cpu_logical_map(cpu));
/*
- * Set priority on PPI and SGI interrupts
- */
- for (i = 0; i < 32; i += 4)
- __raw_writel(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4);
-
- __raw_writel(0xf0, cpu_base + GIC_CPU_PRIMASK);
- __raw_writel(1, cpu_base + GIC_CPU_CTRL);
-}
-
-static void __cpuinit exynos4_secondary_init(unsigned int cpu)
-{
- /*
* if any interrupts are already enabled for the primary
* core (e.g. timer irq), then they will not have been enabled
* for us: do so
*/
- exynos4_gic_secondary_init();
+ gic_secondary_init_base(0, dist_base, cpu_base);
/*
* let the primary processor know we're out of the
--
1.7.0.4
