Re: [PATCH v2 15/15] xen/arm: update GIC dt node with GIC v3 information

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

Hello Vijaya,

Thank you for the patch.

The name of this patch is still wrong. Which GIC DT node are you updating?

On 04/04/2014 12:56 PM, vijay.kilari@gmail.com wrote:
> +    if ( hw_type == GIC_VERSION_V3 )
> +    {
> +        res = dt_property_read_u32(gic, "redistributor-stride", &rd_stride);
> +        if ( !res )
> +            rd_stride = 0;
> +    }


You have skipped some of my remarks on V1. I would definitely prefer to
have a callback in your {v,}GIC structure which add the specific GIC
properties following the version supported by DOM0.

[..]

>  static struct dt_irq * gic_maintenance_irq(void)
>  {
>      return &gic.maintenance;
> @@ -786,6 +792,7 @@ static unsigned int gic_read_vmcr_priority(void)
>  }
>  
>  static struct gic_hw_operations gic_ops = {
> +    .gic_type            = gic_hw_type,

This should go in patch #5. As I said on V1, you don't need a callback
for return a fixed number (that you've initialized in gicv3_init). You
can store a number in this structure.

At the same time what is the difference between this number and the one
in GIC state?

>      .nr_lines            = gic_nr_lines,
>      .nr_lrs              = gic_nr_lrs,
>      .get_maintenance_irq = gic_maintenance_irq,
> @@ -893,6 +900,7 @@ static int __init gicv3_init(struct dt_device_node *node, const void *data)
>      gic_cpu_init();
>      gic_hyp_init();
>  
> +    gic.hw_version = GIC_VERSION_V3;
>      /* Register hw ops*/
>      register_gic_ops(&gic_ops);
>      return 0;
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 87a7ad0..e09d5a5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -58,6 +58,11 @@ void update_cpu_lr_mask(void)
>  
>  static void gic_clear_one_lr(struct vcpu *v, int i);
>  
> +int gic_hw_version(void)
> +{
> +    return gic_hw_ops->gic_type();
> +}
> +
>  unsigned int gic_number_lines(void)
>  {
>      return gic_hw_ops->nr_lines();
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index eadbfcc..32212f9 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -51,6 +51,9 @@
>  #define GICH_HCR_VGRP1EIE (1 << 6)
>  #define GICH_HCR_VGRP1DIE (1 << 7)
>  
> +#define GIC_VERSION_V3 0x3
> +#define GIC_VERSION_V2 0x2
> +

Should not be defined earlier?

-- 
Julien Grall