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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

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