| Message ID | 1384897358-22940-1-git-send-email-fkan@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[dropping <patches@apm.com> from the CC list, as someone seems to have tripped on the config file, and I'm tired of getting bounces] Feng, On 19/11/13 21:42, Feng Kan wrote: > The GIC-400 implementation allows for FIQ and IRQ bypass. In the > X-Gene implementation, the FIQ bypass must be enabled at all time. > Otherwise, some PPI will appear as FIQ and cause kernel problem. How comes? Are only PPIs affected? When you say "some PPIs", can you be more specific? > Signed-off-by: Feng Kan <fkan@apm.com> > --- > drivers/irqchip/irq-gic.c | 15 +++++++++++---- > include/linux/irqchip/arm-gic.h | 4 ++-- > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index d0e9480..aa7342e 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -65,6 +65,7 @@ struct gic_chip_data { > #endif > struct irq_domain *domain; > unsigned int gic_irqs; > + unsigned int bypass_flag; > #ifdef CONFIG_GIC_NON_BANKED > void __iomem *(*get_base)(union gic_base *); > #endif > @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) > writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); > > writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); > - writel_relaxed(1, base + GIC_CPU_CTRL); > + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); > } > > void gic_cpu_if_down(void) > @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) > writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); > > writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); > - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); > + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); > } > > static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) > @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { > > void __init gic_init_bases(unsigned int gic_nr, int irq_start, > void __iomem *dist_base, void __iomem *cpu_base, > - u32 percpu_offset, struct device_node *node) > + u32 percpu_offset, u32 bypass_val, > + struct device_node *node) > { > irq_hw_number_t hwirq_base; > struct gic_chip_data *gic; > @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > > set_handle_irq(gic_handle_irq); > > + gic->bypass_flag = (bypass_val & 0xf) << 4; Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. > gic_chip.flags |= gic_arch_extn.flags; > gic_dist_init(gic); > gic_cpu_init(gic); > @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) > void __iomem *cpu_base; > void __iomem *dist_base; > u32 percpu_offset; > + u32 bypass_val; > int irq; > > if (WARN_ON(!node)) > @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) > if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) > percpu_offset = 0; > > - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); > + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) > + bypass_val = 0; [adding Mark on Cc, so he can comment on the DT parts] Where's the DT documentation update? Also, as this is an implementation-specific quirk, you should consider using a separate compatible string and move the handling of this issue into some X-Gene specific code. > + gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, bypass_val, node); > > if (parent) { > irq = irq_of_parse_and_map(node, 0); > diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h > index 0e5d9ec..999515b 100644 > --- a/include/linux/irqchip/arm-gic.h > +++ b/include/linux/irqchip/arm-gic.h > @@ -64,14 +64,14 @@ struct device_node; > extern struct irq_chip gic_arch_extn; > > void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, > - u32 offset, struct device_node *); > + u32 offset, u32 bypass_val, struct device_node *); > void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); > void gic_cpu_if_down(void); > > static inline void gic_init(unsigned int nr, int start, > void __iomem *dist , void __iomem *cpu) > { > - gic_init_bases(nr, start, dist, cpu, 0, NULL); > + gic_init_bases(nr, start, dist, cpu, 0, 0, NULL); > } > > #endif /* __ASSEMBLY */ > Cheers, M.
>> set_handle_irq(gic_handle_irq); >> >> + gic->bypass_flag = (bypass_val & 0xf) << 4; >Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. The only time those bits are touched are when the dts is modified with the bypass flag. Otherwise those bits remain untouched as before. >> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >> + bypass_val = 0; > > [adding Mark on Cc, so he can comment on the DT parts] > > Where's the DT documentation update? Also, as this is an > implementation-specific quirk, you should consider using a separate > compatible string and move the handling of this issue into some X-Gene > specific code. Yes, the only way to do that nicely would be to add another irq-xgene file. Since I see little of gic specific implementation in the irq-gic file. To add another irq-xgene file for one change seems excessive at the time. Given that in v2 these bypass bits are added, is it better to check for v2 dts property before modify the CPU_CTRL register? > >> + gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, bypass_val, node); >> >> if (parent) { >> irq = irq_of_parse_and_map(node, 0); >> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h >> index 0e5d9ec..999515b 100644 >> --- a/include/linux/irqchip/arm-gic.h >> +++ b/include/linux/irqchip/arm-gic.h >> @@ -64,14 +64,14 @@ struct device_node; >> extern struct irq_chip gic_arch_extn; >> >> void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, >> - u32 offset, struct device_node *); >> + u32 offset, u32 bypass_val, struct device_node *); >> void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); >> void gic_cpu_if_down(void); >> >> static inline void gic_init(unsigned int nr, int start, >> void __iomem *dist , void __iomem *cpu) >> { >> - gic_init_bases(nr, start, dist, cpu, 0, NULL); >> + gic_init_bases(nr, start, dist, cpu, 0, 0, NULL); >> } >> >> #endif /* __ASSEMBLY */ >> > > Cheers, > > M. > -- > Jazz is not dead. It just smells funny... >
On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > [dropping <patches@apm.com> from the CC list, as someone seems to have > tripped on the config file, and I'm tired of getting bounces] > > Feng, > > On 19/11/13 21:42, Feng Kan wrote: >> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >> X-Gene implementation, the FIQ bypass must be enabled at all time. >> Otherwise, some PPI will appear as FIQ and cause kernel problem. > > How comes? Are only PPIs affected? When you say "some PPIs", can you be > more specific? > >> Signed-off-by: Feng Kan <fkan@apm.com> >> --- >> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >> include/linux/irqchip/arm-gic.h | 4 ++-- >> 2 files changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index d0e9480..aa7342e 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -65,6 +65,7 @@ struct gic_chip_data { >> #endif >> struct irq_domain *domain; >> unsigned int gic_irqs; >> + unsigned int bypass_flag; >> #ifdef CONFIG_GIC_NON_BANKED >> void __iomem *(*get_base)(union gic_base *); >> #endif >> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >> >> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >> - writel_relaxed(1, base + GIC_CPU_CTRL); >> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >> } >> >> void gic_cpu_if_down(void) >> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >> >> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >> } >> >> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >> >> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> void __iomem *dist_base, void __iomem *cpu_base, >> - u32 percpu_offset, struct device_node *node) >> + u32 percpu_offset, u32 bypass_val, >> + struct device_node *node) >> { >> irq_hw_number_t hwirq_base; >> struct gic_chip_data *gic; >> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> >> set_handle_irq(gic_handle_irq); >> >> + gic->bypass_flag = (bypass_val & 0xf) << 4; > > Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. > >> gic_chip.flags |= gic_arch_extn.flags; >> gic_dist_init(gic); >> gic_cpu_init(gic); >> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >> void __iomem *cpu_base; >> void __iomem *dist_base; >> u32 percpu_offset; >> + u32 bypass_val; >> int irq; >> >> if (WARN_ON(!node)) >> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >> percpu_offset = 0; >> >> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >> + bypass_val = 0; > > [adding Mark on Cc, so he can comment on the DT parts] > > Where's the DT documentation update? Also, as this is an > implementation-specific quirk, you should consider using a separate > compatible string and move the handling of this issue into some X-Gene > specific code. Adding separate compatible string for X-Gene specific GIC will break VGIC code for X-Gene because VGIC code looks for DT node compatible to "arm,cortex-a15-gic". We don't want to break currently working VGIC code for X-Gene. The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and PPI28 (Legacy-FIQ). We should have more cleaner and optional device tree binding for GIC which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 and FIQBypDisGrp1 bits for X-Gene. Regards, Anup > >> + gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, bypass_val, node); >> >> if (parent) { >> irq = irq_of_parse_and_map(node, 0); >> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h >> index 0e5d9ec..999515b 100644 >> --- a/include/linux/irqchip/arm-gic.h >> +++ b/include/linux/irqchip/arm-gic.h >> @@ -64,14 +64,14 @@ struct device_node; >> extern struct irq_chip gic_arch_extn; >> >> void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, >> - u32 offset, struct device_node *); >> + u32 offset, u32 bypass_val, struct device_node *); >> void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); >> void gic_cpu_if_down(void); >> >> static inline void gic_init(unsigned int nr, int start, >> void __iomem *dist , void __iomem *cpu) >> { >> - gic_init_bases(nr, start, dist, cpu, 0, NULL); >> + gic_init_bases(nr, start, dist, cpu, 0, 0, NULL); >> } >> >> #endif /* __ASSEMBLY */ >> > > Cheers, > > M. > -- > Jazz is not dead. It just smells funny... > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Sat, Nov 23, 2013 at 2:11 PM, Anup Patel <anup@brainfault.org> wrote: > On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> [dropping <patches@apm.com> from the CC list, as someone seems to have >> tripped on the config file, and I'm tired of getting bounces] >> >> Feng, >> >> On 19/11/13 21:42, Feng Kan wrote: >>> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >>> X-Gene implementation, the FIQ bypass must be enabled at all time. >>> Otherwise, some PPI will appear as FIQ and cause kernel problem. >> >> How comes? Are only PPIs affected? When you say "some PPIs", can you be >> more specific? >> >>> Signed-off-by: Feng Kan <fkan@apm.com> >>> --- >>> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >>> include/linux/irqchip/arm-gic.h | 4 ++-- >>> 2 files changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index d0e9480..aa7342e 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -65,6 +65,7 @@ struct gic_chip_data { >>> #endif >>> struct irq_domain *domain; >>> unsigned int gic_irqs; >>> + unsigned int bypass_flag; >>> #ifdef CONFIG_GIC_NON_BANKED >>> void __iomem *(*get_base)(union gic_base *); >>> #endif >>> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >>> >>> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >>> - writel_relaxed(1, base + GIC_CPU_CTRL); >>> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >>> } >>> >>> void gic_cpu_if_down(void) >>> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >>> >>> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >>> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >>> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >>> } >>> >>> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >>> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >>> >>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> void __iomem *dist_base, void __iomem *cpu_base, >>> - u32 percpu_offset, struct device_node *node) >>> + u32 percpu_offset, u32 bypass_val, >>> + struct device_node *node) >>> { >>> irq_hw_number_t hwirq_base; >>> struct gic_chip_data *gic; >>> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> >>> set_handle_irq(gic_handle_irq); >>> >>> + gic->bypass_flag = (bypass_val & 0xf) << 4; >> >> Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. >> >>> gic_chip.flags |= gic_arch_extn.flags; >>> gic_dist_init(gic); >>> gic_cpu_init(gic); >>> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>> void __iomem *cpu_base; >>> void __iomem *dist_base; >>> u32 percpu_offset; >>> + u32 bypass_val; >>> int irq; >>> >>> if (WARN_ON(!node)) >>> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >>> percpu_offset = 0; >>> >>> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >>> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >>> + bypass_val = 0; >> >> [adding Mark on Cc, so he can comment on the DT parts] >> >> Where's the DT documentation update? Also, as this is an >> implementation-specific quirk, you should consider using a separate >> compatible string and move the handling of this issue into some X-Gene >> specific code. > > Adding separate compatible string for X-Gene specific GIC will break > VGIC code for X-Gene because VGIC code looks for DT node compatible > to "arm,cortex-a15-gic". We don't want to break currently working VGIC > code for X-Gene. > > The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a > feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene > is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf > event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 > and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set > these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and > PPI28 (Legacy-FIQ). > > We should have more cleaner and optional device tree binding for GIC > which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 > and FIQBypDisGrp1 bits for X-Gene. > > Regards, > Anup Adding Kumar Sankaran. > >> >>> + gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, bypass_val, node); >>> >>> if (parent) { >>> irq = irq_of_parse_and_map(node, 0); >>> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h >>> index 0e5d9ec..999515b 100644 >>> --- a/include/linux/irqchip/arm-gic.h >>> +++ b/include/linux/irqchip/arm-gic.h >>> @@ -64,14 +64,14 @@ struct device_node; >>> extern struct irq_chip gic_arch_extn; >>> >>> void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, >>> - u32 offset, struct device_node *); >>> + u32 offset, u32 bypass_val, struct device_node *); >>> void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); >>> void gic_cpu_if_down(void); >>> >>> static inline void gic_init(unsigned int nr, int start, >>> void __iomem *dist , void __iomem *cpu) >>> { >>> - gic_init_bases(nr, start, dist, cpu, 0, NULL); >>> + gic_init_bases(nr, start, dist, cpu, 0, 0, NULL); >>> } >>> >>> #endif /* __ASSEMBLY */ >>> >> >> Cheers, >> >> M. >> -- >> Jazz is not dead. It just smells funny... >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/
On 23/11/13 08:41, Anup Patel wrote: > On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> [dropping <patches@apm.com> from the CC list, as someone seems to have >> tripped on the config file, and I'm tired of getting bounces] >> >> Feng, >> >> On 19/11/13 21:42, Feng Kan wrote: >>> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >>> X-Gene implementation, the FIQ bypass must be enabled at all time. >>> Otherwise, some PPI will appear as FIQ and cause kernel problem. >> >> How comes? Are only PPIs affected? When you say "some PPIs", can you be >> more specific? >> >>> Signed-off-by: Feng Kan <fkan@apm.com> >>> --- >>> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >>> include/linux/irqchip/arm-gic.h | 4 ++-- >>> 2 files changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index d0e9480..aa7342e 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -65,6 +65,7 @@ struct gic_chip_data { >>> #endif >>> struct irq_domain *domain; >>> unsigned int gic_irqs; >>> + unsigned int bypass_flag; >>> #ifdef CONFIG_GIC_NON_BANKED >>> void __iomem *(*get_base)(union gic_base *); >>> #endif >>> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >>> >>> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >>> - writel_relaxed(1, base + GIC_CPU_CTRL); >>> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >>> } >>> >>> void gic_cpu_if_down(void) >>> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >>> >>> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >>> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >>> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >>> } >>> >>> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >>> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >>> >>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> void __iomem *dist_base, void __iomem *cpu_base, >>> - u32 percpu_offset, struct device_node *node) >>> + u32 percpu_offset, u32 bypass_val, >>> + struct device_node *node) >>> { >>> irq_hw_number_t hwirq_base; >>> struct gic_chip_data *gic; >>> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> >>> set_handle_irq(gic_handle_irq); >>> >>> + gic->bypass_flag = (bypass_val & 0xf) << 4; >> >> Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. >> >>> gic_chip.flags |= gic_arch_extn.flags; >>> gic_dist_init(gic); >>> gic_cpu_init(gic); >>> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>> void __iomem *cpu_base; >>> void __iomem *dist_base; >>> u32 percpu_offset; >>> + u32 bypass_val; >>> int irq; >>> >>> if (WARN_ON(!node)) >>> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >>> percpu_offset = 0; >>> >>> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >>> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >>> + bypass_val = 0; >> >> [adding Mark on Cc, so he can comment on the DT parts] >> >> Where's the DT documentation update? Also, as this is an >> implementation-specific quirk, you should consider using a separate >> compatible string and move the handling of this issue into some X-Gene >> specific code. > > Adding separate compatible string for X-Gene specific GIC will break > VGIC code for X-Gene because VGIC code looks for DT node compatible > to "arm,cortex-a15-gic". We don't want to break currently working VGIC > code for X-Gene. I'm sure you could manage adding another compat string to the vgic code... > The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a > feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene > is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf Amazing. Someone managed to push the weird-o-meter one level higher. Were you *that* short on PPIs that you had to use these two? > event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 > and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set > these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and > PPI28 (Legacy-FIQ). > > We should have more cleaner and optional device tree binding for GIC > which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 > and FIQBypDisGrp1 bits for X-Gene. Well, here's an alternative approach for you. As you said, this is in no way X-Gene specific, so maybe we should address it directly in the GIC code. Just hook something in the irq_mask/irq_unmask methods, so we can detect the use of these two PPIs, and toggle the bypass flags accordingly bits accordingly. M.
(Adding correct "Kumar Sankaran" to CC. My mistake.) (Adding back patches@apm.com. It is fixed now). On Mon, Nov 25, 2013 at 2:52 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 23/11/13 08:41, Anup Patel wrote: >> On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> [dropping <patches@apm.com> from the CC list, as someone seems to have >>> tripped on the config file, and I'm tired of getting bounces] >>> >>> Feng, >>> >>> On 19/11/13 21:42, Feng Kan wrote: >>>> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >>>> X-Gene implementation, the FIQ bypass must be enabled at all time. >>>> Otherwise, some PPI will appear as FIQ and cause kernel problem. >>> >>> How comes? Are only PPIs affected? When you say "some PPIs", can you be >>> more specific? >>> >>>> Signed-off-by: Feng Kan <fkan@apm.com> >>>> --- >>>> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >>>> include/linux/irqchip/arm-gic.h | 4 ++-- >>>> 2 files changed, 13 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index d0e9480..aa7342e 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -65,6 +65,7 @@ struct gic_chip_data { >>>> #endif >>>> struct irq_domain *domain; >>>> unsigned int gic_irqs; >>>> + unsigned int bypass_flag; >>>> #ifdef CONFIG_GIC_NON_BANKED >>>> void __iomem *(*get_base)(union gic_base *); >>>> #endif >>>> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >>>> >>>> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >>>> - writel_relaxed(1, base + GIC_CPU_CTRL); >>>> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >>>> } >>>> >>>> void gic_cpu_if_down(void) >>>> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >>>> >>>> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >>>> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >>>> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >>>> } >>>> >>>> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >>>> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >>>> >>>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> void __iomem *dist_base, void __iomem *cpu_base, >>>> - u32 percpu_offset, struct device_node *node) >>>> + u32 percpu_offset, u32 bypass_val, >>>> + struct device_node *node) >>>> { >>>> irq_hw_number_t hwirq_base; >>>> struct gic_chip_data *gic; >>>> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> >>>> set_handle_irq(gic_handle_irq); >>>> >>>> + gic->bypass_flag = (bypass_val & 0xf) << 4; >>> >>> Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. >>> >>>> gic_chip.flags |= gic_arch_extn.flags; >>>> gic_dist_init(gic); >>>> gic_cpu_init(gic); >>>> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>> void __iomem *cpu_base; >>>> void __iomem *dist_base; >>>> u32 percpu_offset; >>>> + u32 bypass_val; >>>> int irq; >>>> >>>> if (WARN_ON(!node)) >>>> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >>>> percpu_offset = 0; >>>> >>>> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >>>> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >>>> + bypass_val = 0; >>> >>> [adding Mark on Cc, so he can comment on the DT parts] >>> >>> Where's the DT documentation update? Also, as this is an >>> implementation-specific quirk, you should consider using a separate >>> compatible string and move the handling of this issue into some X-Gene >>> specific code. >> >> Adding separate compatible string for X-Gene specific GIC will break >> VGIC code for X-Gene because VGIC code looks for DT node compatible >> to "arm,cortex-a15-gic". We don't want to break currently working VGIC >> code for X-Gene. > > I'm sure you could manage adding another compat string to the vgic code... > >> The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a >> feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene >> is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf > > Amazing. Someone managed to push the weird-o-meter one level higher. > Were you *that* short on PPIs that you had to use these two? > >> event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >> and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set >> these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and >> PPI28 (Legacy-FIQ). >> >> We should have more cleaner and optional device tree binding for GIC >> which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >> and FIQBypDisGrp1 bits for X-Gene. > > Well, here's an alternative approach for you. As you said, this is in no > way X-Gene specific, so maybe we should address it directly in the GIC > code. Just hook something in the irq_mask/irq_unmask methods, so we can > detect the use of these two PPIs, and toggle the bypass flags > accordingly bits accordingly. Actually, this might not be a general solution because IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 and FIQBypDisGrp1 bits are not present in all GIC versions. For GICv1 (like you mentioned already), all these bits are reserved. For GICv2 without security extensions, the bits IRQBypDisGrp0 and FIQBypDisGrp0 are reserved. For GICv2 with security extensions, all bits are available. As you can see setting these bits may require more info about the type of GIC. I was thinking about having four optional boolean DT flags namely: irq-byp-disable-group0, fiq-byp-disable-group0, irq-byp-disable-group1, and fiq-byp-disable-group1. This way people can set all (or subset) of these bits based on GIC type. What do you think about this approach? > > M. > -- > Jazz is not dead. It just smells funny... > -- Anup
On Sat, Nov 23, 2013 at 2:41 AM, Anup Patel <anup@brainfault.org> wrote: > On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> [dropping <patches@apm.com> from the CC list, as someone seems to have >> tripped on the config file, and I'm tired of getting bounces] >> >> Feng, >> >> On 19/11/13 21:42, Feng Kan wrote: >>> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >>> X-Gene implementation, the FIQ bypass must be enabled at all time. >>> Otherwise, some PPI will appear as FIQ and cause kernel problem. >> >> How comes? Are only PPIs affected? When you say "some PPIs", can you be >> more specific? >> >>> Signed-off-by: Feng Kan <fkan@apm.com> >>> --- >>> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >>> include/linux/irqchip/arm-gic.h | 4 ++-- >>> 2 files changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index d0e9480..aa7342e 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -65,6 +65,7 @@ struct gic_chip_data { >>> #endif >>> struct irq_domain *domain; >>> unsigned int gic_irqs; >>> + unsigned int bypass_flag; >>> #ifdef CONFIG_GIC_NON_BANKED >>> void __iomem *(*get_base)(union gic_base *); >>> #endif >>> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >>> >>> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >>> - writel_relaxed(1, base + GIC_CPU_CTRL); >>> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >>> } >>> >>> void gic_cpu_if_down(void) >>> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >>> >>> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >>> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >>> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >>> } >>> >>> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >>> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >>> >>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> void __iomem *dist_base, void __iomem *cpu_base, >>> - u32 percpu_offset, struct device_node *node) >>> + u32 percpu_offset, u32 bypass_val, >>> + struct device_node *node) >>> { >>> irq_hw_number_t hwirq_base; >>> struct gic_chip_data *gic; >>> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> >>> set_handle_irq(gic_handle_irq); >>> >>> + gic->bypass_flag = (bypass_val & 0xf) << 4; >> >> Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. >> >>> gic_chip.flags |= gic_arch_extn.flags; >>> gic_dist_init(gic); >>> gic_cpu_init(gic); >>> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>> void __iomem *cpu_base; >>> void __iomem *dist_base; >>> u32 percpu_offset; >>> + u32 bypass_val; >>> int irq; >>> >>> if (WARN_ON(!node)) >>> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >>> percpu_offset = 0; >>> >>> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >>> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >>> + bypass_val = 0; >> >> [adding Mark on Cc, so he can comment on the DT parts] >> >> Where's the DT documentation update? Also, as this is an >> implementation-specific quirk, you should consider using a separate >> compatible string and move the handling of this issue into some X-Gene >> specific code. > > Adding separate compatible string for X-Gene specific GIC will break > VGIC code for X-Gene because VGIC code looks for DT node compatible > to "arm,cortex-a15-gic". We don't want to break currently working VGIC > code for X-Gene. > > The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a > feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene > is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf > event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 > and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set > these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and > PPI28 (Legacy-FIQ). > > We should have more cleaner and optional device tree binding for GIC > which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 > and FIQBypDisGrp1 bits for X-Gene. IIRC, all these bits are secure mode only (or only a subset are banked). Whether an SoC supports secure mode or not, the current policy for the kernel is to do any secure mode setup in firmware or bootloader. Rob
On Mon, Nov 25, 2013 at 9:13 PM, Rob Herring <robherring2@gmail.com> wrote: > On Sat, Nov 23, 2013 at 2:41 AM, Anup Patel <anup@brainfault.org> wrote: >> On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> [dropping <patches@apm.com> from the CC list, as someone seems to have >>> tripped on the config file, and I'm tired of getting bounces] >>> >>> Feng, >>> >>> On 19/11/13 21:42, Feng Kan wrote: >>>> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >>>> X-Gene implementation, the FIQ bypass must be enabled at all time. >>>> Otherwise, some PPI will appear as FIQ and cause kernel problem. >>> >>> How comes? Are only PPIs affected? When you say "some PPIs", can you be >>> more specific? >>> >>>> Signed-off-by: Feng Kan <fkan@apm.com> >>>> --- >>>> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >>>> include/linux/irqchip/arm-gic.h | 4 ++-- >>>> 2 files changed, 13 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index d0e9480..aa7342e 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -65,6 +65,7 @@ struct gic_chip_data { >>>> #endif >>>> struct irq_domain *domain; >>>> unsigned int gic_irqs; >>>> + unsigned int bypass_flag; >>>> #ifdef CONFIG_GIC_NON_BANKED >>>> void __iomem *(*get_base)(union gic_base *); >>>> #endif >>>> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >>>> >>>> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >>>> - writel_relaxed(1, base + GIC_CPU_CTRL); >>>> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >>>> } >>>> >>>> void gic_cpu_if_down(void) >>>> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >>>> >>>> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >>>> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >>>> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >>>> } >>>> >>>> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >>>> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >>>> >>>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> void __iomem *dist_base, void __iomem *cpu_base, >>>> - u32 percpu_offset, struct device_node *node) >>>> + u32 percpu_offset, u32 bypass_val, >>>> + struct device_node *node) >>>> { >>>> irq_hw_number_t hwirq_base; >>>> struct gic_chip_data *gic; >>>> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> >>>> set_handle_irq(gic_handle_irq); >>>> >>>> + gic->bypass_flag = (bypass_val & 0xf) << 4; >>> >>> Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. >>> >>>> gic_chip.flags |= gic_arch_extn.flags; >>>> gic_dist_init(gic); >>>> gic_cpu_init(gic); >>>> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>> void __iomem *cpu_base; >>>> void __iomem *dist_base; >>>> u32 percpu_offset; >>>> + u32 bypass_val; >>>> int irq; >>>> >>>> if (WARN_ON(!node)) >>>> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >>>> percpu_offset = 0; >>>> >>>> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >>>> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >>>> + bypass_val = 0; >>> >>> [adding Mark on Cc, so he can comment on the DT parts] >>> >>> Where's the DT documentation update? Also, as this is an >>> implementation-specific quirk, you should consider using a separate >>> compatible string and move the handling of this issue into some X-Gene >>> specific code. >> >> Adding separate compatible string for X-Gene specific GIC will break >> VGIC code for X-Gene because VGIC code looks for DT node compatible >> to "arm,cortex-a15-gic". We don't want to break currently working VGIC >> code for X-Gene. >> >> The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a >> feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene >> is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf >> event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >> and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set >> these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and >> PPI28 (Legacy-FIQ). >> >> We should have more cleaner and optional device tree binding for GIC >> which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >> and FIQBypDisGrp1 bits for X-Gene. > > IIRC, all these bits are secure mode only (or only a subset are > banked). Whether an SoC supports secure mode or not, the current > policy for the kernel is to do any secure mode setup in firmware or > bootloader. Group0 are secure interrupts and Group1 are non-secure interrupts. Does this mean we should only touch IRQBypDisGrp1 and FIQBypDisGrp1 bits from kernel ?? > > Rob -- Anup
> >> The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a >> feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene >> is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf > > Amazing. Someone managed to push the weird-o-meter one level higher. > Were you *that* short on PPIs that you had to use these two? We don't support bypass enable at all, nothing to do with the PPI. It must be set to bypass disable always for us. Those two are the unfortunate victims. > >> event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >> and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set >> these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and >> PPI28 (Legacy-FIQ). >> >> We should have more cleaner and optional device tree binding for GIC >> which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >> and FIQBypDisGrp1 bits for X-Gene. > > Well, here's an alternative approach for you. As you said, this is in no > way X-Gene specific, so maybe we should address it directly in the GIC > code. Just hook something in the irq_mask/irq_unmask methods, so we can > detect the use of these two PPIs, and toggle the bypass flags > accordingly bits accordingly. Yes, I hope Catalin could comment on this one. I don't think he would like this as well since it is our platform specific. > > M. > -- > Jazz is not dead. It just smells funny... >
On Mon, Nov 25, 2013 at 10:00 AM, Anup Patel <anup@brainfault.org> wrote: > On Mon, Nov 25, 2013 at 9:13 PM, Rob Herring <robherring2@gmail.com> wrote: >> On Sat, Nov 23, 2013 at 2:41 AM, Anup Patel <anup@brainfault.org> wrote: >>> On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>> [dropping <patches@apm.com> from the CC list, as someone seems to have >>>> tripped on the config file, and I'm tired of getting bounces] >>>> >>>> Feng, >>>> >>>> On 19/11/13 21:42, Feng Kan wrote: >>>>> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >>>>> X-Gene implementation, the FIQ bypass must be enabled at all time. >>>>> Otherwise, some PPI will appear as FIQ and cause kernel problem. >>>> >>>> How comes? Are only PPIs affected? When you say "some PPIs", can you be >>>> more specific? >>>> >>>>> Signed-off-by: Feng Kan <fkan@apm.com> >>>>> --- >>>>> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >>>>> include/linux/irqchip/arm-gic.h | 4 ++-- >>>>> 2 files changed, 13 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>>> index d0e9480..aa7342e 100644 >>>>> --- a/drivers/irqchip/irq-gic.c >>>>> +++ b/drivers/irqchip/irq-gic.c >>>>> @@ -65,6 +65,7 @@ struct gic_chip_data { >>>>> #endif >>>>> struct irq_domain *domain; >>>>> unsigned int gic_irqs; >>>>> + unsigned int bypass_flag; >>>>> #ifdef CONFIG_GIC_NON_BANKED >>>>> void __iomem *(*get_base)(union gic_base *); >>>>> #endif >>>>> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >>>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >>>>> >>>>> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >>>>> - writel_relaxed(1, base + GIC_CPU_CTRL); >>>>> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >>>>> } >>>>> >>>>> void gic_cpu_if_down(void) >>>>> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >>>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >>>>> >>>>> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >>>>> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >>>>> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >>>>> } >>>>> >>>>> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >>>>> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >>>>> >>>>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>> void __iomem *dist_base, void __iomem *cpu_base, >>>>> - u32 percpu_offset, struct device_node *node) >>>>> + u32 percpu_offset, u32 bypass_val, >>>>> + struct device_node *node) >>>>> { >>>>> irq_hw_number_t hwirq_base; >>>>> struct gic_chip_data *gic; >>>>> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>> >>>>> set_handle_irq(gic_handle_irq); >>>>> >>>>> + gic->bypass_flag = (bypass_val & 0xf) << 4; >>>> >>>> Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. >>>> >>>>> gic_chip.flags |= gic_arch_extn.flags; >>>>> gic_dist_init(gic); >>>>> gic_cpu_init(gic); >>>>> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>>> void __iomem *cpu_base; >>>>> void __iomem *dist_base; >>>>> u32 percpu_offset; >>>>> + u32 bypass_val; >>>>> int irq; >>>>> >>>>> if (WARN_ON(!node)) >>>>> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>>> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >>>>> percpu_offset = 0; >>>>> >>>>> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >>>>> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >>>>> + bypass_val = 0; >>>> >>>> [adding Mark on Cc, so he can comment on the DT parts] >>>> >>>> Where's the DT documentation update? Also, as this is an >>>> implementation-specific quirk, you should consider using a separate >>>> compatible string and move the handling of this issue into some X-Gene >>>> specific code. >>> >>> Adding separate compatible string for X-Gene specific GIC will break >>> VGIC code for X-Gene because VGIC code looks for DT node compatible >>> to "arm,cortex-a15-gic". We don't want to break currently working VGIC >>> code for X-Gene. >>> >>> The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a >>> feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene >>> is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf >>> event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >>> and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set >>> these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and >>> PPI28 (Legacy-FIQ). >>> >>> We should have more cleaner and optional device tree binding for GIC >>> which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >>> and FIQBypDisGrp1 bits for X-Gene. >> >> IIRC, all these bits are secure mode only (or only a subset are >> banked). Whether an SoC supports secure mode or not, the current >> policy for the kernel is to do any secure mode setup in firmware or >> bootloader. > > Group0 are secure interrupts and Group1 are non-secure interrupts. Right, but the non-secure view of the GIC has no concept of groups. They are only visible in secure mode. > Does this mean we should only touch IRQBypDisGrp1 and FIQBypDisGrp1 > bits from kernel ?? No, the kernel should only touch bits defined (by the GIC arch spec) to be available in non-secure mode. Specifically this is bit 0 only. I guess you will have to change accesses to cpu ctrl reg to a read-mod-write to preserve any bits you need set. Rob
On Tue, Nov 26, 2013 at 9:05 PM, Rob Herring <robherring2@gmail.com> wrote: > On Mon, Nov 25, 2013 at 10:00 AM, Anup Patel <anup@brainfault.org> wrote: >> On Mon, Nov 25, 2013 at 9:13 PM, Rob Herring <robherring2@gmail.com> wrote: >>> On Sat, Nov 23, 2013 at 2:41 AM, Anup Patel <anup@brainfault.org> wrote: >>>> On Wed, Nov 20, 2013 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>>> [dropping <patches@apm.com> from the CC list, as someone seems to have >>>>> tripped on the config file, and I'm tired of getting bounces] >>>>> >>>>> Feng, >>>>> >>>>> On 19/11/13 21:42, Feng Kan wrote: >>>>>> The GIC-400 implementation allows for FIQ and IRQ bypass. In the >>>>>> X-Gene implementation, the FIQ bypass must be enabled at all time. >>>>>> Otherwise, some PPI will appear as FIQ and cause kernel problem. >>>>> >>>>> How comes? Are only PPIs affected? When you say "some PPIs", can you be >>>>> more specific? >>>>> >>>>>> Signed-off-by: Feng Kan <fkan@apm.com> >>>>>> --- >>>>>> drivers/irqchip/irq-gic.c | 15 +++++++++++---- >>>>>> include/linux/irqchip/arm-gic.h | 4 ++-- >>>>>> 2 files changed, 13 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>>>> index d0e9480..aa7342e 100644 >>>>>> --- a/drivers/irqchip/irq-gic.c >>>>>> +++ b/drivers/irqchip/irq-gic.c >>>>>> @@ -65,6 +65,7 @@ struct gic_chip_data { >>>>>> #endif >>>>>> struct irq_domain *domain; >>>>>> unsigned int gic_irqs; >>>>>> + unsigned int bypass_flag; >>>>>> #ifdef CONFIG_GIC_NON_BANKED >>>>>> void __iomem *(*get_base)(union gic_base *); >>>>>> #endif >>>>>> @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >>>>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); >>>>>> >>>>>> writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); >>>>>> - writel_relaxed(1, base + GIC_CPU_CTRL); >>>>>> + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); >>>>>> } >>>>>> >>>>>> void gic_cpu_if_down(void) >>>>>> @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) >>>>>> writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); >>>>>> >>>>>> writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); >>>>>> - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); >>>>>> + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); >>>>>> } >>>>>> >>>>>> static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) >>>>>> @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { >>>>>> >>>>>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>>> void __iomem *dist_base, void __iomem *cpu_base, >>>>>> - u32 percpu_offset, struct device_node *node) >>>>>> + u32 percpu_offset, u32 bypass_val, >>>>>> + struct device_node *node) >>>>>> { >>>>>> irq_hw_number_t hwirq_base; >>>>>> struct gic_chip_data *gic; >>>>>> @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>>> >>>>>> set_handle_irq(gic_handle_irq); >>>>>> >>>>>> + gic->bypass_flag = (bypass_val & 0xf) << 4; >>>>> >>>>> Beware, the top 2 bits are reserved on GICv1, and shouldn't be messed with. >>>>> >>>>>> gic_chip.flags |= gic_arch_extn.flags; >>>>>> gic_dist_init(gic); >>>>>> gic_cpu_init(gic); >>>>>> @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>>>> void __iomem *cpu_base; >>>>>> void __iomem *dist_base; >>>>>> u32 percpu_offset; >>>>>> + u32 bypass_val; >>>>>> int irq; >>>>>> >>>>>> if (WARN_ON(!node)) >>>>>> @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) >>>>>> if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) >>>>>> percpu_offset = 0; >>>>>> >>>>>> - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); >>>>>> + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) >>>>>> + bypass_val = 0; >>>>> >>>>> [adding Mark on Cc, so he can comment on the DT parts] >>>>> >>>>> Where's the DT documentation update? Also, as this is an >>>>> implementation-specific quirk, you should consider using a separate >>>>> compatible string and move the handling of this issue into some X-Gene >>>>> specific code. >>>> >>>> Adding separate compatible string for X-Gene specific GIC will break >>>> VGIC code for X-Gene because VGIC code looks for DT node compatible >>>> to "arm,cortex-a15-gic". We don't want to break currently working VGIC >>>> code for X-Gene. >>>> >>>> The Legacy-IRQ bypass disable and Legacy-FIQ bypass disable is a >>>> feature of GIC-400 and its not X-Gene specific. The only difference in X-Gene >>>> is that we use PPI31 (Legacy-IRQ) for timer and PPI28 (Legacy-FIQ) for perf >>>> event. The issue is that IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >>>> and FIQBypDisGrp1 bits are 0 by default and for X-Gene we need to set >>>> these bits to 1 so that GIC-400 does not bypass PPI31 (Legacy-IRQ) and >>>> PPI28 (Legacy-FIQ). >>>> >>>> We should have more cleaner and optional device tree binding for GIC >>>> which can help us set IRQBypDisGrp0, FIQBypDisGrp0, IRQBypDisGrp1 >>>> and FIQBypDisGrp1 bits for X-Gene. >>> >>> IIRC, all these bits are secure mode only (or only a subset are >>> banked). Whether an SoC supports secure mode or not, the current >>> policy for the kernel is to do any secure mode setup in firmware or >>> bootloader. >> >> Group0 are secure interrupts and Group1 are non-secure interrupts. > > Right, but the non-secure view of the GIC has no concept of groups. > They are only visible in secure mode. > >> Does this mean we should only touch IRQBypDisGrp1 and FIQBypDisGrp1 >> bits from kernel ?? > > No, the kernel should only touch bits defined (by the GIC arch spec) > to be available in non-secure mode. Specifically this is bit 0 only. I > guess you will have to change accesses to cpu ctrl reg to a > read-mod-write to preserve any bits you need set. Thats wrong. If you refer page 124 & 125 of GICv2 architecture reference manual (more specifically IHI0048B_gic_architecture_specification.pdf) then you will find Figure 4-23 and Table 4-30 which clearly shows that IRQBypDisGrp1 and FIQBypDisGrp1 bits are part of the Non-secure copy of GICC_CTRL register hence we can access this bits from kernel. -- Anup > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index d0e9480..aa7342e 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -65,6 +65,7 @@ struct gic_chip_data { #endif struct irq_domain *domain; unsigned int gic_irqs; + unsigned int bypass_flag; #ifdef CONFIG_GIC_NON_BANKED void __iomem *(*get_base)(union gic_base *); #endif @@ -450,7 +451,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4); writel_relaxed(0xf0, base + GIC_CPU_PRIMASK); - writel_relaxed(1, base + GIC_CPU_CTRL); + writel_relaxed(gic->bypass_flag | 1, base + GIC_CPU_CTRL); } void gic_cpu_if_down(void) @@ -591,7 +592,7 @@ static void gic_cpu_restore(unsigned int gic_nr) writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4); writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK); - writel_relaxed(1, cpu_base + GIC_CPU_CTRL); + writel_relaxed(gic_data[gic_nr].bypass_flag | 1, cpu_base + GIC_CPU_CTRL); } static int gic_notifier(struct notifier_block *self, unsigned long cmd, void *v) @@ -733,7 +734,8 @@ const struct irq_domain_ops gic_irq_domain_ops = { void __init gic_init_bases(unsigned int gic_nr, int irq_start, void __iomem *dist_base, void __iomem *cpu_base, - u32 percpu_offset, struct device_node *node) + u32 percpu_offset, u32 bypass_val, + struct device_node *node) { irq_hw_number_t hwirq_base; struct gic_chip_data *gic; @@ -821,6 +823,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, set_handle_irq(gic_handle_irq); + gic->bypass_flag = (bypass_val & 0xf) << 4; gic_chip.flags |= gic_arch_extn.flags; gic_dist_init(gic); gic_cpu_init(gic); @@ -835,6 +838,7 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) void __iomem *cpu_base; void __iomem *dist_base; u32 percpu_offset; + u32 bypass_val; int irq; if (WARN_ON(!node)) @@ -849,7 +853,10 @@ int __init gic_of_init(struct device_node *node, struct device_node *parent) if (of_property_read_u32(node, "cpu-offset", &percpu_offset)) percpu_offset = 0; - gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node); + if (of_property_read_u32(node, "bypass-flags", &bypass_val)) + bypass_val = 0; + + gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, bypass_val, node); if (parent) { irq = irq_of_parse_and_map(node, 0); diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index 0e5d9ec..999515b 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -64,14 +64,14 @@ struct device_node; extern struct irq_chip gic_arch_extn; void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, - u32 offset, struct device_node *); + u32 offset, u32 bypass_val, struct device_node *); void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); void gic_cpu_if_down(void); static inline void gic_init(unsigned int nr, int start, void __iomem *dist , void __iomem *cpu) { - gic_init_bases(nr, start, dist, cpu, 0, NULL); + gic_init_bases(nr, start, dist, cpu, 0, 0, NULL); } #endif /* __ASSEMBLY */
The GIC-400 implementation allows for FIQ and IRQ bypass. In the X-Gene implementation, the FIQ bypass must be enabled at all time. Otherwise, some PPI will appear as FIQ and cause kernel problem. Signed-off-by: Feng Kan <fkan@apm.com> --- drivers/irqchip/irq-gic.c | 15 +++++++++++---- include/linux/irqchip/arm-gic.h | 4 ++-- 2 files changed, 13 insertions(+), 6 deletions(-)