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

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

Hi Frediano,

On 11/03/2014 10:11 AM, Frediano Ziglio wrote:
>  /*
>   * needs to be called with a valid cpu_mask, ie each cpu in the mask has
>   * already called gic_cpu_init
> @@ -230,7 +245,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
>      writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
>  
>      /* Set target CPU mask (RAZ/WI on uniprocessor) */
> -    writeb_gicd(mask, GICD_ITARGETSR + irq);
> +    gicv2_set_irq_mask(irq, mask);
>      /* Set priority */
>      writeb_gicd(priority, GICD_IPRIORITYR + irq);
>  
> @@ -244,16 +259,22 @@ static void __init gicv2_dist_init(void)
>      uint32_t gic_cpus;
>      int i;
>  
> -    cpumask = readl_gicd(GICD_ITARGETSR) & 0xff;
> -    cpumask |= cpumask << 8;
> -    cpumask |= cpumask << 16;
> +    cpumask = readl_gicd(GICD_ITARGETSR) & gic_cpu_mask;
>  
>      /* Disable the distributor */
>      writel_gicd(0, GICD_CTLR);
>  
>      type = readl_gicd(GICD_TYPER);
>      gicv2_info.nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
> -    gic_cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
> +    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        gic_cpus = 16;
> +        BUG_ON( gicv2_info.nr_lines > 510 );

Could you add a define for this value and add a comment. It would avoid
to scratch his head trying to understand why 510.

[..]

> +static inline unsigned gicd_sgi_target_shift(void)
> +{
> +    return 8 + 16 - nr_gic_cpu_if;
> +}
> +
>  static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>                             const cpumask_t *cpu_mask)
>  {
> @@ -366,7 +403,7 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>          cpumask_and(&online_mask, cpu_mask, &cpu_online_map);
>          mask = gicv2_cpu_mask(&online_mask);
>          writel_gicd(GICD_SGI_TARGET_LIST |
> -                    (mask << GICD_SGI_TARGET_SHIFT) | sgi,
> +                    (mask << gicd_sgi_target_shift()) | sgi,

gicv2_send_SGI is heavily used, it's not acceptable to compute the value
of the shift every time. Hence this always give you the same value. You
should define a static and initialize it during the initialization.

> @@ -690,6 +727,12 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>  
>      dt_device_set_used_by(node, DOMID_XEN);
>  
> +    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        nr_gic_cpu_if = 16;
> +        gic_cpu_mask = 0xffff;
> +    }
> +
>      res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
>      if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
>          panic("GICv2: Cannot find a valid address for the distributor");
> @@ -769,6 +812,7 @@ static const char * const gicv2_dt_compat[] __initconst =
>      DT_COMPAT_GIC_CORTEX_A15,
>      DT_COMPAT_GIC_CORTEX_A7,
>      DT_COMPAT_GIC_400,
> +    DT_COMPAT_GIC_HIP04,
>      NULL
>  };
>  
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 70d10d6..cd934cf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -563,12 +563,15 @@ 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 = platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) ?
> +                           510 :

Ditto for the 510.

> +                           1021;

gic_interrupt is called thousand time per second, anything that never
changes should define only once during initialization.

Rather than using hardcoding the max number, I would use
gicv2_info.nr_lines which contains the maximum number of IRQ used by
this GIC. Any value above this value is already an error.

That would require either a separate patch or adding a comment in the
commit message.

Regards,

-- 
Julien Grall