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

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

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.

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