Re: [PATCH v3 15/16] xen/arm: Update Dom0 GIC dt node with GICv3 information

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

Hello Vijaya,

On 15/04/14 12:17, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Update GIC device tree node for DOM0 with GICv3
> information. GIC hw specfic device tree information
> is moved to respective GIC driver.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>   xen/arch/arm/domain_build.c |    9 +++------
>   xen/arch/arm/gic-v2.c       |    9 +++++++++
>   xen/arch/arm/gic-v3.c       |   33 +++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic.c          |    5 +++++
>   xen/include/asm-arm/gic.h   |    4 ++++
>   5 files changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 5ca2f15..5ef08da 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -576,13 +576,10 @@ static int make_gic_node(const struct domain *d, void *fdt,
>           return -FDT_ERR_XEN(ENOMEM);
>   Hello
>       tmp = new_cells;
> -    DPRINT("  Set Distributor Base 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
> -           d->arch.vgic.dbase, d->arch.vgic.dbase + PAGE_SIZE - 1);
> -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
>
> -    DPRINT("  Set Cpu Base 0x%"PRIpaddr"-0x%"PRIpaddr"\n",
> -           d->arch.vgic.cbase, d->arch.vgic.cbase + (PAGE_SIZE * 2) - 1);
> -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> +    res = gic_make_node(d, node, fdt, tmp);


This solution is buggy because tmp is allocated for only 2 ranges. GICv3 
can have more than 2 ranges.

Why do you give tmp rather than moving the creation of the property 
"reg" in each GIC driver?

I would also move all this function in gic_make_node for more clarity.

>   static const char * const gicv2_dt_compat[] __initconst =
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 8625e0c..e27b094 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -30,6 +30,7 @@
>   #include <xen/softirq.h>
>   #include <xen/list.h>
>   #include <xen/device_tree.h>
> +#include <xen/libfdt/libfdt.h>
>   #include <xen/delay.h>
>   #include <asm/p2m.h>
>   #include <asm/domain.h>
> @@ -825,6 +826,37 @@ static unsigned int gicv3_read_vmcr_priority(void)
>               GICH_VMCR_PRIORITY_MASK);
>   }
>
> +int static gicv3_make_dt_node(const struct domain *d,

static should be before int.

> +              const struct dt_device_node *node, void *fdt, __be32 *cells)

With my previous remark, this callback should be:

gicv3_make_dt_node(const struct domain *d,
                    const struct dt_device_node *node, void *fdt)

And you allocate the number of cells in this function.

Regards,

-- 
Julien Grall