[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
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. > 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? 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 > >> + .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 - Vijay > > Ian. > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |