Re: [RFC PATCH v1 06/10] xen/arm: split gic driver into generic and gicv2 driver

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

Hello Vijay,

Thanks for the patch.

On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Existing GIC driver has both generic code and hw specific
> code. Segregate GIC low level driver into gic-v2.c and
> keep generic code in existing gic.c file
> 
> GIC v2 driver registers required functions
> to generic GIC driver. This helps to plug in next version
> of GIC drivers like GIC v3.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---

[..]

> +
> +void __init gicv2_init(void)
> +{

Instead of calling gicv2_init and gicv3_init from generic, it would be
better to the device API (see xen/include/asm-arm/device.h). An example
of use is my iommu series (see https://patches.linaro.org/26032/
iommu_hardware_setup).

[..]

> -void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
> +static void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)

Why did you put static here?

>  {
> -    ASSERT(cpu < NR_GIC_CPU_IF);  /* Targets bitmap only supports 8
CPUs */
>      send_SGI_mask(cpumask_of(cpu), sgi);
>  }
>
> -void send_SGI_self(enum gic_sgi sgi)
> -{
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> -
> -    dsb();
> -
> -    GICD[GICD_SGIR] = GICD_SGI_TARGET_SELF
> -        | sgi;
> -}
> -

Why did you remove send_SGI_self?

>  void send_SGI_allbutself(enum gic_sgi sgi)
>  {
> -   ASSERT(sgi < 16); /* There are only 16 SGIs */
> +    cpumask_t all_others_mask;
> +    ASSERT(sgi < 16); /* There are only 16 SGIs */
>
> -   dsb();
> +    cpumask_andnot(&all_others_mask, &cpu_possible_map,
cpumask_of(smp_processor_id()));
>
> -   GICD[GICD_SGIR] = GICD_SGI_TARGET_OTHERS
> -       | sgi;
> +    dsb();
> +    send_SGI_mask(&all_others_mask, sgi);

Why did you remove the optmization for GICv2?

[..]

> +#define GIC_VERSION_V2 0x2
> +

I would prefer if you define an enum here with for now only one value
GIC_VERSION_V2.

>  /* SGI (AKA IPIs) */
>  enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
>      GIC_SGI_DUMP_STATE  = 1,
>      GIC_SGI_CALL_FUNCTION = 2,
>  };
> +
> +struct gic_hw_operations {

Can you explain a bit more your structure? Why do you need so all theses
callbacks...

> +    int (*gic_type)(void);

This functions always return the same value on GICv2 (e.g
GIC_VERSION_V2) and GICv3 (e.g GIC_VERSION_V3), right?

If so, using int type directly is better.

[..]
> +    void (*enable_irq)(int);
> +    void (*disable_irq)(int);
> +    void (*eoi_irq)(int);
> +    void (*deactivate_irq)(int);
> +    unsigned int (*ack_irq)(void);

I would prefer to introduce a new hw_controller here rather than adding
another abstraction.

[..]

> +    unsigned long (*read_cpu_rbase)(void);
> +    unsigned long (*read_cpu_sgi_rbase)(void);

The both function above are GICv3 specific and not defined in GICv2 ...
why do you need it? If you plan to implement later, please add them in
the corresponding patch.

Regards,

-- 
Julien Grall