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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

2015-01-13 11:54 GMT+00:00 Ian Campbell <Ian.Campbell@xxxxxxxxxx>:
> On Mon, 2014-11-03 at 10:11 +0000, 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.
>
> I'm not super keen on sprinkling all of these
> platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) and the related
> de-constification throughout this driver. In reality your interrupt
> controller might be "gic-like" but it is not actually a gic v2, it's
> something custom.
>

The quirk part was removed, Julien imported v3 in
http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/hisilicon-d01-v3

> I see that the Linux side has gone the route of making this an entirely
> separate driver. Since we have the gic_hw_operations abstraction these
> days I am inclined towards suggesting we go a similar route in Xen (so
> far the abstraction has supported gic v2 and v3, so you might find that
> you want to refactor slight, e.g. by hoisting or pushing down some
> functionality). The same goes for the GICH changes later on.
>

Do you think would be better to have another GIC driver instead. It
would be mostly a copy&paste job. Yes, I didn't understand the reason
behind changing GICH registers either but now the changes are in the
silicon.

> I'm even less keen on the related changes made to the vgic. My
> overwhelming preference is for guests to only ever see the
> architecturally standardised gic v2, v3 (and in the future v4 etc). This
> would mean that on your platform a single guest could only make use of 8
> CPUs, but as a whole the host would be able to use all 16. I think
> that's an acceptable tradeoff.
>
> (Hrm, looking a second time I don't really understand the patches you
> are doing to the vgic and to what extent they contradict or support what
> I say above, just moving the registers with no other changes seems odd
> to me. In any case please just use the standard vgic)
>
> Ian.
>

Yes, there is no change in vgic. The GICV is 100% compatible to
standard GICv2 and a normal GICv2 is presented to the guest.

Frediano

>> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/gic-v2.c     | 70 
>> ++++++++++++++++++++++++++++++++++++++---------
>>  xen/arch/arm/gic.c        |  5 +++-
>>  xen/include/asm-arm/gic.h |  4 ++-
>>  3 files changed, 64 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index faad1ff..eef55ed 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -79,16 +79,22 @@ 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 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 +138,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 +209,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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
>> +        writew_gicd(mask, GICD_ITARGETSR + irq * 2);
>> +    else
>> +        writeb_gicd(mask, GICD_ITARGETSR + irq);
>> +}
>> +
>>  /*
>>   * 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 );
>> +    }
>> +    else
>> +    {
>> +        gic_cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
>> +    }
>>      printk("GICv2: %d lines, %d cpu%s%s (IID %8.8x).\n",
>>             gicv2_info.nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
>>             (type & GICD_TYPE_SEC) ? ", secure" : "",
>> @@ -264,8 +285,19 @@ static void __init gicv2_dist_init(void)
>>          writel_gicd(0x0, GICD_ICFGR + (i / 16) * 4);
>>
>>      /* Route all global IRQs to this CPU */
>> -    for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
>> -        writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
>> +    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
>> +    {
>> +        cpumask |= cpumask << 16;
>> +        for ( i = 32; i < gicv2_info.nr_lines; i += 2 )
>> +            writel_gicd(cpumask, GICD_ITARGETSR + (i / 2) * 4);
>> +    }
>> +    else
>> +    {
>> +        cpumask |= cpumask << 8;
>> +        cpumask |= cpumask << 16;
>> +        for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
>> +            writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
>> +    }
>>
>>      /* Default priority for global interrupts */
>>      for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
>> @@ -285,7 +317,7 @@ static void __cpuinit gicv2_cpu_init(void)
>>  {
>>      int i;
>>
>> -    this_cpu(gic_cpu_id) = readl_gicd(GICD_ITARGETSR) & 0xff;
>> +    this_cpu(gic_cpu_id) = readl_gicd(GICD_ITARGETSR) & gic_cpu_mask;
>>
>>      /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so
>>       * even though they are controlled with GICD registers, they must
>> @@ -348,6 +380,11 @@ static int gicv2_secondary_cpu_init(void)
>>      return 0;
>>  }
>>
>> +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,
>>                      GICD_SGIR);
>>          break;
>>      default:
>> @@ -581,7 +618,7 @@ static void gicv2_irq_set_affinity(struct irq_desc 
>> *desc, const cpumask_t *cpu_m
>>      mask = gicv2_cpu_mask(cpu_mask);
>>
>>      /* Set target CPU mask (RAZ/WI on uniprocessor) */
>> -    writeb_gicd(mask, GICD_ITARGETSR + desc->irq);
>> +    gicv2_set_irq_mask(desc->irq, mask);
>>
>>      spin_unlock(&gicv2.lock);
>>  }
>> @@ -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 :
>> +                           1021;
>>
>>      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) )
>>          {
>>              local_irq_enable();
>>              do_IRQ(regs, irq, is_fiq);
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 187dc46..5adb628 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -155,10 +155,12 @@
>>  #define DT_COMPAT_GIC_400            "arm,gic-400"
>>  #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
>>  #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
>> +#define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
>>
>>  #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
>>                          DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
>> -                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400)
>> +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
>> +                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
>>
>>  #define DT_COMPAT_GIC_V3             "arm,gic-v3"
>>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel