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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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