Re: [PATCH v2 3/7] xen/arm: Make gic-v2 code handle hip04-d01 platform

[Julien Grall <julien.grall@linaro.org>]

On 11/04/2014 01:31 PM, Julien Grall wrote:
> On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
>> The GIC in this platform is mainly compatible with the standard
>> GICv2 beside:
>> - ITARGET is extended to 16 bit to support 16 CPUs;
>> - SGI mask is extended to support 16 CPUs;
>> - maximum supported interrupt is 510.
>>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
>> ---
>>  xen/arch/arm/gic-v2.c     | 89 +++++++++++++++++++++++++++++++++++++++--------
>>  xen/arch/arm/gic.c        |  3 +-
>>  xen/include/asm-arm/gic.h |  4 ++-
>>  3 files changed, 80 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index faad1ff..9461fe3 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -79,16 +79,23 @@ static struct gic_info gicv2_info;
>>   * logical CPU numbering. Let's use mapping as returned by the GIC
>>   * itself
>>   */
>> -static DEFINE_PER_CPU(u8, gic_cpu_id);
>> +static DEFINE_PER_CPU(u16, gic_cpu_id);
>>  
>>  /* Maximum cpu interface per GIC */
>> -#define NR_GIC_CPU_IF 8
>> +static unsigned int nr_gic_cpu_if = 8;
>> +static unsigned int gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT;
>> +static unsigned int gic_cpu_mask = 0xff;
>>  
>>  static inline void writeb_gicd(uint8_t val, unsigned int offset)
>>  {
>>      writeb_relaxed(val, gicv2.map_dbase + offset);
>>  }
>>  
>> +static inline void writew_gicd(uint16_t val, unsigned int offset)
>> +{
>> +    writew_relaxed(val, gicv2.map_dbase + offset);
>> +}
>> +
>>  static inline void writel_gicd(uint32_t val, unsigned int offset)
>>  {
>>      writel_relaxed(val, gicv2.map_dbase + offset);
>> @@ -132,7 +139,7 @@ static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
>>      cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
>>      for_each_cpu( cpu, &possible_mask )
>>      {
>> -        ASSERT(cpu < NR_GIC_CPU_IF);
>> +        ASSERT(cpu < nr_gic_cpu_if);
>>          mask |= per_cpu(gic_cpu_id, cpu);
>>      }
>>  
>> @@ -203,6 +210,15 @@ static unsigned int gicv2_read_irq(void)
>>      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>>  }
>>  
>> +/* Set target CPU mask (RAZ/WI on uniprocessor) */
>> +static void gicv2_set_irq_mask(int irq, unsigned int mask)
>> +{
>> +    if ( nr_gic_cpu_if == 16 )
> 
> This check is very confusing, and even more in patch #5.

Sorry, I meant #7.

> Code executed under this check describes your platform and not a generic
> 16-CPU support (actually there is no spec for it).
> 
> I would introduce a new boolean or hide this check in a macro.
> 
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 70d10d6..8050a65 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -563,12 +563,13 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
>>  void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>>  {
>>      unsigned int irq;
>> +    unsigned int max_irq = gic_hw_ops->info->nr_lines;
>>  
>>      do  {
>>          /* Reading IRQ will ACK it */
>>          irq = gic_hw_ops->read_irq();
>>  
>> -        if ( likely(irq >= 16 && irq < 1021) )
>> +        if ( likely(irq >= 16 && irq < max_irq) )
> 
> On the previous version I've asked that need to explain in the commit
> message why this change is valid.
> 
> Regards,
> 


-- 
Julien Grall