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

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

Hi Vijay,

On 06/26/2014 06:33 AM, 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 absolute offset address instead of dividing the
> register offset by 4.

I remembered I requested to made some changes in this patch. I saw you
did it but you didn't specify in the changelog. People may not be agree
with your new changes and you should notifying about any change in the code!

[..]

> @@ -229,8 +232,7 @@ static void gic_set_irq_properties(struct irq_desc *desc,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority)
>  {
> -    volatile unsigned char *bytereg;
> -    uint32_t cfg, edgebit;
> +    uint32_t cfg, edgebit, itarget, ipriority;
>      unsigned int mask;
>      unsigned int irq = desc->irq;
>      unsigned int type = desc->arch.type;
> @@ -243,21 +245,28 @@ static void gic_set_irq_properties(struct irq_desc *desc,
>      mask = gic_cpu_mask(cpu_mask);
>  
>      /* Set edge / level */
> -    cfg = GICD[GICD_ICFGR + irq / 16];
> +    cfg = readl_relaxed(GICD + GICD_ICFGR + (irq / 16) * 4);
>      edgebit = 2u << (2 * (irq % 16));
>      if ( type & DT_IRQ_TYPE_LEVEL_MASK )
>          cfg &= ~edgebit;
>      else if ( type & DT_IRQ_TYPE_EDGE_BOTH )
>          cfg |= edgebit;
> -    GICD[GICD_ICFGR + irq / 16] = cfg;
> +    writel_relaxed(cfg, GICD + GICD_ICFGR + (irq / 16) * 4);
>  
>      /* Set target CPU mask (RAZ/WI on uniprocessor) */
> -    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> -    bytereg[irq] = mask;
> -
> +    itarget = readl_relaxed(GICD + GICD_ITARGETSR + (irq / 4) * 4);
> +    /* Clear mask */
> +    itarget &= ~(0xffu << (8 * (irq % 4)));
> +    /* Set mask */
> +    itarget |=  mask << (8 * (irq % 4));
> +    writel_relaxed(itarget, GICD + GICD_ITARGETSR + (irq / 4) * 4);

Why didn't you use writeb_relaxed? This change is complex for nothing.

writeb_relaxed(mask, GICD + GICD_ITARGETSR + irq);

> +
> +    ipriority = readl_relaxed(GICD + GICD_IPRIORITYR + (irq / 4) * 4);
> +    /* Clear priority */
> +    ipriority &= ~(0xffu << (8 * (irq % 4)));
>      /* Set priority */
> -    bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR);
> -    bytereg[irq] = priority;
> +    ipriority |=  priority << (8 * (irq % 4));
> +    writel_relaxed(ipriority, GICD + GICD_IPRIORITYR + (irq / 4) * 4);

writeb_relaxed(priority, GICD + GICD_IPRIORITYR + irq);

Regards,

-- 
Julien Grall