Re: [Xen-devel] [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-04-28 at 20:11 +0530, Vijay Kilari wrote:
> Hi Ian,
> 
> On Wed, Apr 23, 2014 at 8:22 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Tue, 2014-04-15 at 16:47 +0530, vijay.kilari@xxxxxxxxx wrote:
> >> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> >>
> >> GIC driver contains both generic and hardware specific low
> >> level functionality in gic.c file.
> >>
> >> With this patch, low level functionality is moved to separate
> >> file gic-v2.c and generic code is kept in gic.c file
> >>
> >> Callbacks are registered by low level driver with generic driver
> >> and are called whereever required.
> >>
> >> The locking mechanism is now kept in generic code as is and
> >> hence it is upto generic driver to take proper locks before
> >> calling low level driver callbacks.
> >
> > Please can you document this no a per hook basis in the definition of
> > struct gic_hw_operations.
> >
> >> This helps to separate generic and hardware functionality
> >
> >> +static struct gic_hw_operations gic_ops;
> >
> > Please order the file:
> >         callbacks
> >         gic_hw_ops struct
> >         initialisation functions
> >
>    With comment from Julien, I have not moved gic lock to generic
> code. I checked and found that gic lock is used to lock generic code
> at only one place in gic_set_irq_properties which is not need as desc lock
> is taken and is enough.

My comment wasn't due to the gic_lock stuff, but rather the forward
declaration of gic_ops which can be avoided and hopefully allow the
declaration to become const.

> >> +static struct dt_irq * gicv2_maintenance_irq(void)
> >> +{
> >> +    return &gic.maintenance;
> >
> > How much does the generic maintenance interrupt handling do? Seems like
> > just routing it and requesting it, in which case I wonder if we
> > shouldn't just push all the MI stuff down into the specific drivers.
> > What do you think?
> 
>    With Stefano's patch set, MI handler is dummy. Though we move
> this stuff to specific drivers, still we need to have a callback to
> generic driver to handler it. Ex; gic_route_ppis() &
> init_maintenance_interrupt()
> of generic code needs a callback
> 
> >
> >> +static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> >> +{
> >> +    uint32_t lrv;
> >> +
> >> +    lrv = GICH[GICH_LR + lr];
> >> +    lr_reg->pirq = (lrv >> GICH_LR_PHYSICAL_SHIFT) & 
> >> GICH_LR_PHYSICAL_MASK;
> >> +    lr_reg->virq = (lrv >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> >> +    lr_reg->priority = (lrv >> GICH_LR_PRIORITY_SHIFT) & 
> >> GICH_LR_PRIORITY_MASK;
> >> +    lr_reg->state    = (lrv >> GICH_LR_STATE_SHIFT) & GICH_LR_STATE_MASK;
> >> +    lr_reg->hw_status = (lrv >> GICH_LR_HW_SHIFT) & GICH_LR_HW_MASK;
> >> +    lr_reg->grp = (lrv >> GICH_LR_GRP_SHIFT) & GICH_LR_GRP_MASK;
> >
> > Please either line up the = or don't, not a mixture.
> >
> >> +static struct gic_hw_operations gic_ops = {
> >
> > Can be const I think?
>    Could not make it const because, some fields like nr_lrs,
> gic_lines, hw_version
> needs to be populated at runtime/init code

I think this is due to mixing static ops and runtime data. Can we not
separate the static ops into a proper "ops" thing? As it stands gic_ops
is rather misnamed since it contains "instance variables" as well.

> >> +    .secondary_init      = gicv2_secondary_cpu_init,
> >> +    .get_maintenance_irq = gicv2_maintenance_irq,
> >> +    .save_state          = gicv2_save_state,
> >> +    .restore_state       = gicv2_restore_state,
> >> +    .dump_state          = gicv2_dump_state,
> >> +    .gicv_setup          = gicv_v2_init,
> >> +    .gic_irq_ops         = &irq_ops,
> >> +    .deactivate_irq      = gicv2_dir_irq,
> >> +    .ack_irq             = gicv2_ack_irq,
> >> +    .set_irq_property    = gicv2_set_irq_properties,
> >> +    .send_sgi            = gicv2_send_sgi,
> >> +    .disable_interface   = gicv2_disable_interface,
> >> +    .update_lr           = gicv2_update_lr,
> >> +    .update_hcr_status   = gicv2_hcr_status,
> >> +    .clear_lr            = gicv2_clear_lr,
> >> +    .read_lr             = gicv2_read_lr,
> >> +    .write_lr            = gicv2_write_lr,
> >> +    .read_vmcr_priority  = gicv2_read_vmcr_priority,
> >> +};
> >> +
> >> +/*
> >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> >> index eba41ee..2387e38 100644
> >> --- a/xen/include/asm-arm/gic.h
> >> +++ b/xen/include/asm-arm/gic.h
> >> @@ -43,12 +43,41 @@
> >>  #define SGI_TARGET_OTHERS  1
> >>  #define SGI_TARGET_SELF    2
> >>
> >> +#define GICH_LR_PENDING    1
> >> +#define GICH_LR_ACTIVE     2
> >> +
> >> +#define GICH_HCR_EN       (1 << 0)
> >> +#define GICH_HCR_UIE      (1 << 1)
> >> +#define GICH_HCR_LRENPIE  (1 << 2)
> >> +#define GICH_HCR_NPIE     (1 << 3)
> >> +#define GICH_HCR_VGRP0EIE (1 << 4)
> >> +#define GICH_HCR_VGRP0DIE (1 << 5)
> >> +#define GICH_HCR_VGRP1EIE (1 << 6)
> >> +#define GICH_HCR_VGRP1DIE (1 << 7)
> >
> > Didn't you just move all these out of this file in a previous patch?
> > Please don't do that.
>    In the next version, I plan not to have separate gic header file
> for V2 & V3. Instead I move(few) required gic version specific definitions
> in c file.
> 
> I have made a patch to make gic definitions common across v2 & v3
> by removing /4 in gic.h register address definitions. With this,
> patched gic driver to use ioremap instead fixmap and updated vgic driver
> accordingly

Sounds good. Thanks.

Ian
> 
> - Vijay
> 
> >
> > Ian.
> >



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