| Message ID | 1350393709-23546-5-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 16, 2012 at 02:21:48PM +0100, Lorenzo Pieralisi wrote: > From: Nicolas Pitre <nicolas.pitre@linaro.org> > > The GIC interface numbering does not necessarily follow the logical > CPU numbering, especially for complex topologies such as multi-cluster > systems. > > Fortunately we can easily probe the GIC to create a mapping as the > Interrupt Processor Targets Registers for the first 32 interrupts are > read-only, and each field returns a value that always corresponds to > the processor reading the register. > > Initially all mappings target all CPUs in case an IPI is required to > boot secondary CPUs. It is refined as those CPUs discover what their > actual mapping is. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> This is a really neat idea! > --- > arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c > index aa52699..1338a55 100644 > --- a/arch/arm/common/gic.c > +++ b/arch/arm/common/gic.c > @@ -70,6 +70,13 @@ struct gic_chip_data { > static DEFINE_RAW_SPINLOCK(irq_controller_lock); > > /* > + * The GIC mapping of CPU interfaces does not necessarily match > + * the logical CPU numbering. Let's use a mapping as returned > + * by the GIC itself. > + */ > +static u8 gic_cpu_map[8] __read_mostly; Can we have a #define for the number CPUs supported by the GIC? It gets used a fair amount in this patch for loop bounds etc. > +/* > * Supported arch specific GIC irq extension. > * Default make them NULL. > */ > @@ -242,7 +249,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > return -EINVAL; > > mask = 0xff << shift; > - bit = 1 << (cpu_logical_map(cpu) + shift); > + bit = gic_cpu_map[cpu] << shift; > > raw_spin_lock(&irq_controller_lock); > val = readl_relaxed(reg) & ~mask; > @@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic) > u32 cpumask; > unsigned int gic_irqs = gic->gic_irqs; > void __iomem *base = gic_data_dist_base(gic); > - u32 cpu = cpu_logical_map(smp_processor_id()); > - > - cpumask = 1 << cpu; > - cpumask |= cpumask << 8; > - cpumask |= cpumask << 16; > > writel_relaxed(0, base + GIC_DIST_CTRL); > > @@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic) > /* > * Set all global interrupts to this CPU only. > */ > + cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0); > for (i = 32; i < gic_irqs; i += 4) > writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4); > > @@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic) > { > void __iomem *dist_base = gic_data_dist_base(gic); > void __iomem *base = gic_data_cpu_base(gic); > + unsigned int cpu_mask, cpu = smp_processor_id(); > int i; > > /* > + * Get what the GIC says our CPU mask is. > + */ > + BUG_ON(cpu >= 8); > + cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0); Making the mask a u8 and using readb_relaxed here makes this bit of code clearer to me (and the GIC apparently allows such an access to this register). > + gic_cpu_map[cpu] = cpu_mask; > + > + /* > + * Clear our mask from the other map entries in case they're > + * still undefined. > + */ > + for (i = 0; i < 8; i++) > + if (i != cpu) > + gic_cpu_map[i] &= ~cpu_mask; > + > + /* > * Deal with the banked PPI and SGI interrupts - disable all > * PPI interrupts, ensure all SGI interrupts are enabled. > */ > @@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > { > irq_hw_number_t hwirq_base; > struct gic_chip_data *gic; > - int gic_irqs, irq_base; > + int gic_irqs, irq_base, i; > > BUG_ON(gic_nr >= MAX_GIC_NR); > > @@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > } > > /* > + * Initialize the CPU interface map to all CPUs. > + * It will be refined as each CPU probes its ID. > + */ > + for (i = 0; i < 8; i++) > + gic_cpu_map[i] = 0xff; > + > + /* > * For primary GICs, skip over SGIs. > * For secondary GICs, skip over PPIs, too. > */ > @@ -737,7 +763,7 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) > > /* Convert our logical CPU mask into a physical one. */ > for_each_cpu(cpu, mask) > - map |= 1 << cpu_logical_map(cpu); > + map |= gic_cpu_map[cpu]; > > /* > * Ensure that stores to Normal memory are visible to the With those changes: Acked-by: Will Deacon <will.deacon@arm.com> Will
On Tue, 6 Nov 2012, Will Deacon wrote: > On Tue, Oct 16, 2012 at 02:21:48PM +0100, Lorenzo Pieralisi wrote: > > From: Nicolas Pitre <nicolas.pitre@linaro.org> > > > > The GIC interface numbering does not necessarily follow the logical > > CPU numbering, especially for complex topologies such as multi-cluster > > systems. > > > > Fortunately we can easily probe the GIC to create a mapping as the > > Interrupt Processor Targets Registers for the first 32 interrupts are > > read-only, and each field returns a value that always corresponds to > > the processor reading the register. > > > > Initially all mappings target all CPUs in case an IPI is required to > > boot secondary CPUs. It is refined as those CPUs discover what their > > actual mapping is. > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > This is a really neat idea! > > > --- > > arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 34 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c > > index aa52699..1338a55 100644 > > --- a/arch/arm/common/gic.c > > +++ b/arch/arm/common/gic.c > > @@ -70,6 +70,13 @@ struct gic_chip_data { > > static DEFINE_RAW_SPINLOCK(irq_controller_lock); > > > > /* > > + * The GIC mapping of CPU interfaces does not necessarily match > > + * the logical CPU numbering. Let's use a mapping as returned > > + * by the GIC itself. > > + */ > > +static u8 gic_cpu_map[8] __read_mostly; > > Can we have a #define for the number CPUs supported by the GIC? It gets > used a fair amount in this patch for loop bounds etc. Sure. I'll respin the patch. > > > +/* > > * Supported arch specific GIC irq extension. > > * Default make them NULL. > > */ > > @@ -242,7 +249,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > > return -EINVAL; > > > > mask = 0xff << shift; > > - bit = 1 << (cpu_logical_map(cpu) + shift); > > + bit = gic_cpu_map[cpu] << shift; > > > > raw_spin_lock(&irq_controller_lock); > > val = readl_relaxed(reg) & ~mask; > > @@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic) > > u32 cpumask; > > unsigned int gic_irqs = gic->gic_irqs; > > void __iomem *base = gic_data_dist_base(gic); > > - u32 cpu = cpu_logical_map(smp_processor_id()); > > - > > - cpumask = 1 << cpu; > > - cpumask |= cpumask << 8; > > - cpumask |= cpumask << 16; > > > > writel_relaxed(0, base + GIC_DIST_CTRL); > > > > @@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic) > > /* > > * Set all global interrupts to this CPU only. > > */ > > + cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0); > > for (i = 32; i < gic_irqs; i += 4) > > writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4); > > > > @@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic) > > { > > void __iomem *dist_base = gic_data_dist_base(gic); > > void __iomem *base = gic_data_cpu_base(gic); > > + unsigned int cpu_mask, cpu = smp_processor_id(); > > int i; > > > > /* > > + * Get what the GIC says our CPU mask is. > > + */ > > + BUG_ON(cpu >= 8); > > + cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0); > > Making the mask a u8 and using readb_relaxed here makes this bit of code > clearer to me (and the GIC apparently allows such an access to this > register). Not always. At least RTSM throws an exception if you do so. Been there. > > + gic_cpu_map[cpu] = cpu_mask; > > + > > + /* > > + * Clear our mask from the other map entries in case they're > > + * still undefined. > > + */ > > + for (i = 0; i < 8; i++) > > + if (i != cpu) > > + gic_cpu_map[i] &= ~cpu_mask; > > + > > + /* > > * Deal with the banked PPI and SGI interrupts - disable all > > * PPI interrupts, ensure all SGI interrupts are enabled. > > */ > > @@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > > { > > irq_hw_number_t hwirq_base; > > struct gic_chip_data *gic; > > - int gic_irqs, irq_base; > > + int gic_irqs, irq_base, i; > > > > BUG_ON(gic_nr >= MAX_GIC_NR); > > > > @@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > > } > > > > /* > > + * Initialize the CPU interface map to all CPUs. > > + * It will be refined as each CPU probes its ID. > > + */ > > + for (i = 0; i < 8; i++) > > + gic_cpu_map[i] = 0xff; > > + > > + /* > > * For primary GICs, skip over SGIs. > > * For secondary GICs, skip over PPIs, too. > > */ > > @@ -737,7 +763,7 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) > > > > /* Convert our logical CPU mask into a physical one. */ > > for_each_cpu(cpu, mask) > > - map |= 1 << cpu_logical_map(cpu); > > + map |= gic_cpu_map[cpu]; > > > > /* > > * Ensure that stores to Normal memory are visible to the > > With those changes: > > Acked-by: Will Deacon <will.deacon@arm.com> > > Will >
On Tue, Nov 06, 2012 at 10:59:35PM +0000, Nicolas Pitre wrote: > On Tue, 6 Nov 2012, Will Deacon wrote: > > > arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++-------- > > > 1 file changed, 34 insertions(+), 8 deletions(-) > > > > > > diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c > > > index aa52699..1338a55 100644 > > > --- a/arch/arm/common/gic.c > > > +++ b/arch/arm/common/gic.c > > > @@ -70,6 +70,13 @@ struct gic_chip_data { > > > static DEFINE_RAW_SPINLOCK(irq_controller_lock); > > > > > > /* > > > + * The GIC mapping of CPU interfaces does not necessarily match > > > + * the logical CPU numbering. Let's use a mapping as returned > > > + * by the GIC itself. > > > + */ > > > +static u8 gic_cpu_map[8] __read_mostly; > > > > Can we have a #define for the number CPUs supported by the GIC? It gets > > used a fair amount in this patch for loop bounds etc. > > Sure. I'll respin the patch. Cheers Nicolas. > > > /* > > > + * Get what the GIC says our CPU mask is. > > > + */ > > > + BUG_ON(cpu >= 8); > > > + cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0); > > > > Making the mask a u8 and using readb_relaxed here makes this bit of code > > clearer to me (and the GIC apparently allows such an access to this > > register). > > Not always. At least RTSM throws an exception if you do so. > Been there. That would be a bug in the RTSM then. Have you reported it to support? (if not, I can chase this one up). I'd rather we just fix the model than work around it in Linux. Will
On Wed, 7 Nov 2012, Will Deacon wrote: > On Tue, Nov 06, 2012 at 10:59:35PM +0000, Nicolas Pitre wrote: > > On Tue, 6 Nov 2012, Will Deacon wrote: > > > > /* > > > > + * Get what the GIC says our CPU mask is. > > > > + */ > > > > + BUG_ON(cpu >= 8); > > > > + cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0); > > > > > > Making the mask a u8 and using readb_relaxed here makes this bit of code > > > clearer to me (and the GIC apparently allows such an access to this > > > register). > > > > Not always. At least RTSM throws an exception if you do so. > > Been there. > > That would be a bug in the RTSM then. Have you reported it to support? (if > not, I can chase this one up). I'd rather we just fix the model than work > around it in Linux. I have no problem with you chasing it down with the support people. I don't want to wait for fixed RTSM versions to be released and the whole world to migrate to them though. While the readl is maybe marginally unintuitive compared to a readb here, the code is always using readl everywhere else already, even using bit masking and shifting when a readb/writeb could have made the code much simpler (see gic_set_affinity() for example). I therefore much prefer to stick to a proven 32-bit access than risking regression on some possible implementation where the 8-bit access wasn't properly implemented as the doc says it should and never exercised before. In other words, I prefer erring on the safe side here. Nicolas
diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index aa52699..1338a55 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c @@ -70,6 +70,13 @@ struct gic_chip_data { static DEFINE_RAW_SPINLOCK(irq_controller_lock); /* + * The GIC mapping of CPU interfaces does not necessarily match + * the logical CPU numbering. Let's use a mapping as returned + * by the GIC itself. + */ +static u8 gic_cpu_map[8] __read_mostly; + +/* * Supported arch specific GIC irq extension. * Default make them NULL. */ @@ -242,7 +249,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, return -EINVAL; mask = 0xff << shift; - bit = 1 << (cpu_logical_map(cpu) + shift); + bit = gic_cpu_map[cpu] << shift; raw_spin_lock(&irq_controller_lock); val = readl_relaxed(reg) & ~mask; @@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic) u32 cpumask; unsigned int gic_irqs = gic->gic_irqs; void __iomem *base = gic_data_dist_base(gic); - u32 cpu = cpu_logical_map(smp_processor_id()); - - cpumask = 1 << cpu; - cpumask |= cpumask << 8; - cpumask |= cpumask << 16; writel_relaxed(0, base + GIC_DIST_CTRL); @@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic) /* * Set all global interrupts to this CPU only. */ + cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0); for (i = 32; i < gic_irqs; i += 4) writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4); @@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic) { void __iomem *dist_base = gic_data_dist_base(gic); void __iomem *base = gic_data_cpu_base(gic); + unsigned int cpu_mask, cpu = smp_processor_id(); int i; /* + * Get what the GIC says our CPU mask is. + */ + BUG_ON(cpu >= 8); + cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0); + gic_cpu_map[cpu] = cpu_mask; + + /* + * Clear our mask from the other map entries in case they're + * still undefined. + */ + for (i = 0; i < 8; i++) + if (i != cpu) + gic_cpu_map[i] &= ~cpu_mask; + + /* * Deal with the banked PPI and SGI interrupts - disable all * PPI interrupts, ensure all SGI interrupts are enabled. */ @@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, { irq_hw_number_t hwirq_base; struct gic_chip_data *gic; - int gic_irqs, irq_base; + int gic_irqs, irq_base, i; BUG_ON(gic_nr >= MAX_GIC_NR); @@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, } /* + * Initialize the CPU interface map to all CPUs. + * It will be refined as each CPU probes its ID. + */ + for (i = 0; i < 8; i++) + gic_cpu_map[i] = 0xff; + + /* * For primary GICs, skip over SGIs. * For secondary GICs, skip over PPIs, too. */ @@ -737,7 +763,7 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) /* Convert our logical CPU mask into a physical one. */ for_each_cpu(cpu, mask) - map |= 1 << cpu_logical_map(cpu); + map |= gic_cpu_map[cpu]; /* * Ensure that stores to Normal memory are visible to the