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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Mar 20, 2014 at 9:32 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Vijay,
>
> Thanks for the patch.
>
> On 03/19/2014 02:17 PM, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxx>
>> ---
>
> [..]
>
>> +
>> +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).
>
OK. I will check

> [..]
>
>> -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?
>
I think it is not being called from outside of this file.
Should I keep it non static for future use?

>>  {
>> -    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?
>
It is not used at all.

>>  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?
What was the optimization that was there earlier?
The gic-v2.c is handling the hw specific code

>
> [..]
>
>> +#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.
>
Can you please explain more on what is meant by new hw_controller?

> [..]
>
>> +    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.
>
OK. I will introduce with GICv3 patch
> Regards,
>
> --
> Julien Grall

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