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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hello Vijay,

On 22/03/14 09:40, Vijay Kilari wrote:
> 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?
I think this change should not be part of this patch. As you said this 
patch should be only code movement, and adding prototype is definitely 
not code movement.
If you want to keep it, I would prefer a separate patch.

> > >   {
> > > -    ASSERT(cpu < NR_GIC_CPU_IF);  /* Targets bitmap only supports 8
> > CPUs */
> > >       send_SGI_mask(cpumask_of(cpu), sgi);
> > >   }
It might interesting to keep it just in case.

If you plan to remove it from
> > > -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.
Same as send_SGI_one. It's not part of this patch.

> > >   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?
GICD_SGI_TARGET_OTHERS, will send an SGI to every CPU even if the CPU is 
not yet online (e.g not registered by Xen). It's used during secondary 
boot (see cpu_up_send_sgi).
Here you are breaking SMP boot on GICv2.

> > [..]
> > > +    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?
oops, I meant hw_irq_controller sorry. (see xen/include/xen/irq.h).

Regards,

--
Julien Grall

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