Re: [PATCH v4 05/16] xen/arm: use ioremap to map gic-v2 registers

[Julien Grall <julien.grall@linaro.org>]

Hi Vijay,

Thank you for the patch. Did you verify that GICv2 is correctly working 
with theses changes?

On 26/05/14 11:26, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> gic-v2 driver uses fixmap to map the registers.
> Instead use ioremap to access mmio registers.
>
> With this patch, gic-v2 register definitions are updated
> to use obsolute offset address instead of dividing the
> register offset by 4.
>
> Update vgic driver logic to compute using obsolute register

s/obsolute/obsolete/

>       /* Local settings: interface controller */
> -    GICC[GICC_PMR] = 0xff;                /* Don't mask by priority */
> -    GICC[GICC_BPR] = 0;                   /* Finest granularity of priority */
> -    GICC[GICC_CTLR] = GICC_CTL_ENABLE|GICC_CTL_EOI;    /* Turn on delivery */
> +    writel_relaxed(0xff, GICC + GICC_PMR);                /* Don't mask by priority */
> +    writel_relaxed(0x0, GICC + GICC_BPR);                   /* Finest granularity of priority */
> +    writel_relaxed(GICC_CTL_ENABLE|GICC_CTL_EOI, GICC + GICC_CTLR);    /* Turn on delivery */

The coding style requests 80 characters per line.

> @@ -700,7 +713,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>                       irq, v->domain->domain_id, v->vcpu_id, i);
>   #endif
>       } else {
> -        GICH[GICH_LR + i] = 0;
> +        writel_relaxed(0, GICH + GICH_LR + i * 4);
>           clear_bit(i, &this_cpu(lr_mask));
>
>           if ( p->desc != NULL )
> @@ -808,7 +821,7 @@ int gic_events_need_delivery(void)
>       struct pending_irq *p;
>       unsigned long flags;
>
> -    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> +    mask_priority = (readl_relaxed(GICH + GICH_VMCR) >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;

Same remark here.

>   /* Number of ranks of interrupt registers for a domain */
>   #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
> @@ -55,7 +55,7 @@ static inline int REG_RANK_NR(int b, uint32_t n)
>    * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> with
>    * <b>-bits-per-interrupt.
>    */
> -#define REG_RANK_INDEX(b, n) ((n) & ((b)-1))
> +#define REG_RANK_INDEX(b, n) (((n) >> 2) & ((b)-1))
>
>   /*
>    * Returns rank corresponding to a GICD_<FOO><n> register for
> @@ -63,7 +63,9 @@ static inline int REG_RANK_NR(int b, uint32_t n)
>    */
>   static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)
>   {
> -    int rank = REG_RANK_NR(b, n);
> +    int rank;
> +    n = n >> 2;
> +    rank = REG_RANK_NR(b, n);

As n is only used for REG_RANK_NR, I would do:

int rank = REG_RANK_NR(b, n >> 2)

Otherwise than the few changes here, the patch looks good to me, 
assuming you did some tests on GICv2.

Regards,

-- 
Julien Grall