Re: [PATCH 1/2] xen/arm: Introduce accessors for GICv2 MMIO reads/writes

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

Hi Ian,

On 07/09/2014 12:54 PM, Ian Campbell wrote:
> A future patch needs to make the accesses to GICC more complex. For consistency
> introduce read/write wrappers for all three regions (GICD, GICC, GICH).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/gic-v2.c |  157 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 94 insertions(+), 63 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index cc60af8..3820be5 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -60,16 +60,12 @@
>  #define GICH_V2_VMCR_PRIORITY_MASK   0x1f
>  #define GICH_V2_VMCR_PRIORITY_SHIFT  27
>  
> -#define GICD (gicv2.map_dbase)
> -#define GICC (gicv2.map_cbase)
> -#define GICH (gicv2.map_hbase)
> -
>  /* Global state */
>  static struct {
>      paddr_t dbase;            /* Address of distributor registers */
>      void __iomem * map_dbase; /* IO mapped Address of distributor registers */
>      paddr_t cbase;            /* Address of CPU interface registers */
> -    void __iomem * map_cbase; /* IO mapped Address of CPU interface registers*/
> +    void __iomem * map_cbase; /* IO mapped Address of CPU interface registers */
>      paddr_t hbase;            /* Address of virtual interface registers */
>      void __iomem * map_hbase; /* IO Address of virtual interface registers */
>      paddr_t vbase;            /* Address of virtual cpu interface registers */
> @@ -87,6 +83,41 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>  /* Maximum cpu interface per GIC */
>  #define NR_GIC_CPU_IF 8
>  
> +static inline void writeb_gicd(uint32_t val, int offset)

NIT: the value is 8 byte, so I would use uint8_t.

Also the offset should never be negative. I would use unsigned int (same
remark for the other helpers).

> +{
> +    writeb_relaxed(val, gicv2.map_dbase + offset);
> +}

The other part of the patch looks good to me.

Regards,



-- 
Julien Grall