[PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Feng Kan]

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(-)

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 */
-- 
1.7.6.1

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Marc Zyngier]

[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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Feng Kan]

>>       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...
>

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Anup Patel]

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/

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Anup Patel]

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/

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Anup Patel]

(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

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Rob Herring]

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

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Anup Patel]

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

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Feng Kan]

>
>> 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...
>

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Rob Herring]

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

Re: [PATCH 1/1] irq-gic: add capability to set bypass flag in GIC

[Anup Patel]

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/