Re: [PATCH v8 3/4] xen/arm: Make gic-v2 code handle hip04-d01 platform

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

Hello Frediano,

On 03/03/15 11:19, Frediano Ziglio wrote:
> The GIC in this platform is mainly compatible with the standard
> GICv2 beside:
> - ITARGET is extended to 16 bit to support 16 CPUs;
> - SGI mask is extended to support 16 CPUs;
> - maximum supported interrupt is 510;

510 is not a multiple of 32. Is it normal?

This will result to having nr_lines = 512. What happen is we are trying
to access IRQ 510 and 511?

Also, is it possible to have GICH.VirtualID >= 510?

> - GICH APR and LR register offsets.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  MAINTAINERS              |   5 +
>  xen/arch/arm/Makefile    |   1 +
>  xen/arch/arm/gic-hip04.c | 396 ++++++++++++++++++++++++-----------------------
>  3 files changed, 209 insertions(+), 193 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0f04742..b17aab1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -138,6 +138,11 @@ F:	xen/drivers/char/omap-uart.c
>  F:	xen/drivers/char/pl011.c
>  F:	xen/drivers/passthrough/arm/
>  
> +HISILICON HIP04 SUPPORT
> +M:	Frediano Ziglio <frediano.ziglio@huawei.com>

It might be good to have 2 maintainers form Huawei on this file. Ian any
though?

> +S:	Supported
> +F:	xen/arch/arm/git-hip04.c

gic-hip04.c

> +
>  CPU POOLS
>  M:	Juergen Gross <jgross@suse.com>
>  S:	Supported
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 41aba2e..935999e 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -12,6 +12,7 @@ obj-y += domctl.o
>  obj-y += sysctl.o
>  obj-y += domain_build.o
>  obj-y += gic.o gic-v2.o
> +obj-$(CONFIG_ARM_32) += gic-hip04.o
>  obj-$(CONFIG_ARM_64) += gic-v3.o
>  obj-y += io.o
>  obj-y += irq.o
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index 20cdbc9..94abdc4 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -1,7 +1,8 @@
>  /*
> - * xen/arch/arm/gic-v2.c
> + * xen/arch/arm/gic-hip04.c
>   *
> - * ARM Generic Interrupt Controller support v2
> + * Generic Interrupt Controller for HiSilicon Hip04 platform
> + * Based heavily from gic-v2.c

Please add a commit ID. It would help you to keep track of the GIC.

>   *
>   * Tim Deegan <tim@xen.org>
>   * Copyright (c) 2011 Citrix Systems.
> @@ -71,59 +72,69 @@ static struct {
>      void __iomem * map_hbase; /* IO Address of virtual interface registers */
>      paddr_t vbase;            /* Address of virtual cpu interface registers */
>      spinlock_t lock;
> -} gicv2;
> +} hip04gic;
>  
> -static struct gic_info gicv2_info;
> +static struct gic_info hip04gic_info;

I think the renaming of gicv2 and gicv2_info is pointless here. Instead
of function name, it doesn't help for debugging.

It would also reduce the diff of this patch.

[..]

> -DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC)
> -        .dt_match = gicv2_dt_match,
> -        .init = gicv2_init,
> +DT_DEVICE_START(hip04gic, "GIC-HIP04", DEVICE_GIC)
> +    .dt_match = hip04gic_dt_match,
> +    .init = hip04gic_init,
>  DT_DEVICE_END

Please keep the same indentation as before.

Regards,

-- 
Julien Grall