[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality
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 Then you can avoid the forward reference and hopefully make things 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? > +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? > + .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. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |