xen/arm: Add support for Huawei hip04-d01 platform

xen/arm: Add support for Huawei hip04-d01 platform

[Frediano Ziglio]

This set of patches add Xen support for hip04-d01 platform (see https://wiki.linaro.org/Boards/D01 for details).

The patch "xen/arm: Move vGIC registers on Hip04 platform" is actually a workaround and should have in the future a proper implementation in Xen (unfortunately not straightforward to do it).

Changes from v1:
- style (Julien Grall);
- make gicv2_send_SGI faster (Julien Grall);
- cleanup correctly if hip04_smp_init fails (Julien Grall);
- remove quirks using compatibility (Ian Campbell);
- other minor suggestions by Julien Grall.

[PATCH v2 1/7] xen/device_tree: Add new helper to read arrays from a DTB

[Frediano Ziglio]

From: Zoltan Kiss <zoltan.kiss@linaro.org>

Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/common/device_tree.c      | 13 ++++++++++---
 xen/include/xen/device_tree.h | 11 +++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index f72b2e9..1a886c0 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -160,14 +160,21 @@ const void *dt_get_property(const struct dt_device_node *np,
 bool_t dt_property_read_u32(const struct dt_device_node *np,
                          const char *name, u32 *out_value)
 {
-    u32 len;
+    return dt_property_read_u32_array(np, name, out_value, 1);
+}
+
+bool_t dt_property_read_u32_array(const struct dt_device_node *np,
+                                  const char *name, u32 *out_value, u16 out_len)
+{
+    u32 len, i;
     const __be32 *val;
 
     val = dt_get_property(np, name, &len);
-    if ( !val || len < sizeof(*out_value) )
+    if ( !val || len < sizeof(*out_value) * out_len )
         return 0;
 
-    *out_value = be32_to_cpup(val);
+    for ( i = 0; i < out_len; i++, val++ )
+        out_value[i] = be32_to_cpup(val);
 
     return 1;
 }
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 08db8bc..629bfb2 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -346,6 +346,17 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np,
 bool_t dt_property_read_u32(const struct dt_device_node *np,
                             const char *name, u32 *out_value);
 /**
+ * dt_property_read_u32_array - Helper to read a u32 array property.
+ * @np: node to get the value
+ * @name: name of the property
+ * @out_value: pointer to return value
+ * @out_len: length of the array
+ *
+ * Return true if get the desired value.
+ */
+bool_t dt_property_read_u32_array(const struct dt_device_node *np,
+                                  const char *name, u32 *out_value, u16 out_len);
+/**
  * dt_property_read_u64 - Helper to read a u64 property.
  * @np: node to get the value
  * @name: name of the property
-- 
1.9.1

Re: [PATCH v2 1/7] xen/device_tree: Add new helper to read arrays from a DTB

[Julien Grall]

Hi Frediano,

On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> From: Zoltan Kiss <zoltan.kiss@linaro.org>
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>

You forgot to keep my reviewed-by from the last version.

Regards,

> ---
>  xen/common/device_tree.c      | 13 ++++++++++---
>  xen/include/xen/device_tree.h | 11 +++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f72b2e9..1a886c0 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -160,14 +160,21 @@ const void *dt_get_property(const struct dt_device_node *np,
>  bool_t dt_property_read_u32(const struct dt_device_node *np,
>                           const char *name, u32 *out_value)
>  {
> -    u32 len;
> +    return dt_property_read_u32_array(np, name, out_value, 1);
> +}
> +
> +bool_t dt_property_read_u32_array(const struct dt_device_node *np,
> +                                  const char *name, u32 *out_value, u16 out_len)
> +{
> +    u32 len, i;
>      const __be32 *val;
>  
>      val = dt_get_property(np, name, &len);
> -    if ( !val || len < sizeof(*out_value) )
> +    if ( !val || len < sizeof(*out_value) * out_len )
>          return 0;
>  
> -    *out_value = be32_to_cpup(val);
> +    for ( i = 0; i < out_len; i++, val++ )
> +        out_value[i] = be32_to_cpup(val);
>  
>      return 1;
>  }
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 08db8bc..629bfb2 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -346,6 +346,17 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np,
>  bool_t dt_property_read_u32(const struct dt_device_node *np,
>                              const char *name, u32 *out_value);
>  /**
> + * dt_property_read_u32_array - Helper to read a u32 array property.
> + * @np: node to get the value
> + * @name: name of the property
> + * @out_value: pointer to return value
> + * @out_len: length of the array
> + *
> + * Return true if get the desired value.
> + */
> +bool_t dt_property_read_u32_array(const struct dt_device_node *np,
> +                                  const char *name, u32 *out_value, u16 out_len);
> +/**
>   * dt_property_read_u64 - Helper to read a u64 property.
>   * @np: node to get the value
>   * @name: name of the property
> 


-- 
Julien Grall

Re: [PATCH v2 1/7] xen/device_tree: Add new helper to read arrays from a DTB

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> > From: Zoltan Kiss <zoltan.kiss@linaro.org>
> >
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> 
> You forgot to keep my reviewed-by from the last version.
> 
> Regards,
> 

You sent two mails, on first you add your reviewed-by but on the next one you put some comments so I though your reviewed-by was not valid anymore

Frediano

> > ---
> >  xen/common/device_tree.c      | 13 ++++++++++---
> >  xen/include/xen/device_tree.h | 11 +++++++++++
> >  2 files changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index
> > f72b2e9..1a886c0 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -160,14 +160,21 @@ const void *dt_get_property(const struct
> > dt_device_node *np,  bool_t dt_property_read_u32(const struct
> dt_device_node *np,
> >                           const char *name, u32 *out_value)  {
> > -    u32 len;
> > +    return dt_property_read_u32_array(np, name, out_value, 1); }
> > +
> > +bool_t dt_property_read_u32_array(const struct dt_device_node *np,
> > +                                  const char *name, u32 *out_value,
> > +u16 out_len) {
> > +    u32 len, i;
> >      const __be32 *val;
> >
> >      val = dt_get_property(np, name, &len);
> > -    if ( !val || len < sizeof(*out_value) )
> > +    if ( !val || len < sizeof(*out_value) * out_len )
> >          return 0;
> >
> > -    *out_value = be32_to_cpup(val);
> > +    for ( i = 0; i < out_len; i++, val++ )
> > +        out_value[i] = be32_to_cpup(val);
> >
> >      return 1;
> >  }
> > diff --git a/xen/include/xen/device_tree.h
> > b/xen/include/xen/device_tree.h index 08db8bc..629bfb2 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -346,6 +346,17 @@ const struct dt_property *dt_find_property(const
> > struct dt_device_node *np,  bool_t dt_property_read_u32(const struct
> dt_device_node *np,
> >                              const char *name, u32 *out_value);
> >  /**
> > + * dt_property_read_u32_array - Helper to read a u32 array property.
> > + * @np: node to get the value
> > + * @name: name of the property
> > + * @out_value: pointer to return value
> > + * @out_len: length of the array
> > + *
> > + * Return true if get the desired value.
> > + */
> > +bool_t dt_property_read_u32_array(const struct dt_device_node *np,
> > +                                  const char *name, u32 *out_value,
> > +u16 out_len);
> > +/**
> >   * dt_property_read_u64 - Helper to read a u64 property.
> >   * @np: node to get the value
> >   * @name: name of the property
> >
> 
> 
> --
> Julien Grall

Re: [PATCH v2 1/7] xen/device_tree: Add new helper to read arrays from a DTB

[Julien Grall]

On 11/04/2014 01:55 PM, Frediano Ziglio wrote:
>>
>> Hi Frediano,
>>
>> On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
>>> From: Zoltan Kiss <zoltan.kiss@linaro.org>
>>>
>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
>>
>> You forgot to keep my reviewed-by from the last version.
>>
>> Regards,
>>
> 
> You sent two mails, on first you add your reviewed-by but on the next one you put some comments so I though your reviewed-by was not valid anymore

Fair enough. I tend to keep Reviewed-by/Acked-by for minors changes such
as coding style.

Anyway:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,



-- 
Julien Grall

[PATCH v2 2/7] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

Add this new platform to Xen.
This platform requires specific code to initialize CPUs.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/platforms/Makefile |   1 +
 xen/arch/arm/platforms/hip04.c  | 308 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 309 insertions(+)
 create mode 100644 xen/arch/arm/platforms/hip04.c

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 8f47c16..d0b2d99 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_ARM_32) += midway.o
 obj-$(CONFIG_ARM_32) += omap5.o
 obj-$(CONFIG_ARM_32) += sunxi.o
 obj-$(CONFIG_ARM_64) += seattle.o
+obj-$(CONFIG_ARM_32) += hip04.o
 obj-$(CONFIG_ARM_64) += xgene-storm.o
diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
new file mode 100644
index 0000000..799b5b3
--- /dev/null
+++ b/xen/arch/arm/platforms/hip04.c
@@ -0,0 +1,308 @@
+/*
+ * xen/arch/arm/platforms/hip04.c
+ *
+ * HiSilicon HIP-04 D01 board
+ *
+ * Copyright (c) 2012-2013 Hisilicon Ltd.
+ * Copyright (c) 2012-2013 Linaro Ltd.
+ * Copyright (c) 2014 Huawei Tech. Co., Ltd.
+ *
+ * Author: Frediano Ziglio <frediano.ziglio@huawei.com>
+ *
+ * Original code from Linux kernel arch/arm/mach-hisi/hisilicon.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/platform.h>
+#include <xen/mm.h>
+#include <xen/vmap.h>
+#include <asm/io.h>
+#include <asm/gic.h>
+#include <xen/delay.h>
+
+#define CORE_RESET_BIT(x)            (1 << x)
+#define NEON_RESET_BIT(x)            (1 << (x + 4))
+#define CORE_DEBUG_RESET_BIT(x)      (1 << (x + 9))
+#define CLUSTER_L2_RESET_BIT         (1 << 8)
+#define CLUSTER_DEBUG_RESET_BIT      (1 << 13)
+
+#define CLUSTER_L2_RESET_STATUS      (1 << 8)
+#define CLUSTER_DEBUG_RESET_STATUS   (1 << 13)
+
+#define SC_CPU_RESET_DREQ(x)         (0x524 + (x << 3))    /* unreset */
+#define SC_CPU_RESET_STATUS(x)       (0x1520 + (x << 3))
+
+#define FAB_SF_MODE                  0x0c
+
+#define HIP04_MAX_CLUSTERS           4
+#define HIP04_MAX_CPUS_PER_CLUSTER   4
+
+struct hip04_secondary_cpu_data
+{
+    u32 bootwrapper_phys;
+    u32 bootwrapper_size;
+    u32 bootwrapper_magic;
+    u32 relocation_entry;
+    u32 relocation_size;
+};
+
+static void __iomem *relocation, *sysctrl, *fabric, *gb2;
+static int hip04_cpu_table[HIP04_MAX_CLUSTERS][HIP04_MAX_CPUS_PER_CLUSTER];
+static struct hip04_secondary_cpu_data hip04_boot;
+
+static void hip04_reset(void)
+{
+    unsigned long data;
+
+    if ( !gb2 )
+        return;
+
+    data = readl_relaxed(gb2);
+    writel_relaxed(data & ~0x4000000u, gb2);
+
+    mdelay(10);
+}
+
+static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
+{
+    unsigned long data;
+
+    if ( !fabric )
+        return;
+    data = readl_relaxed(fabric + FAB_SF_MODE);
+    if ( on )
+        data |= 1 << cluster;
+    else
+        data &= ~(1 << cluster);
+    writel_relaxed(data, fabric + FAB_SF_MODE);
+    while ( 1 )
+    {
+        if ( data == readl_relaxed(fabric + FAB_SF_MODE) )
+            break;
+    }
+}
+
+static bool __init hip04_cpu_table_init(void)
+{
+    unsigned int mpidr, cpu, cluster;
+
+    mpidr = cpu_logical_map(smp_processor_id());
+    cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+    cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+    if ( cluster >= HIP04_MAX_CLUSTERS ||
+        cpu >= HIP04_MAX_CPUS_PER_CLUSTER )
+    {
+        printk(XENLOG_ERR "%s: boot CPU is out of bound!\n", __func__);
+        return false;
+    }
+
+    hip04_set_snoop_filter(cluster, 1);
+    hip04_cpu_table[cluster][cpu] = 1;
+    return true;
+}
+
+static bool hip04_cluster_down(unsigned int cluster)
+{
+    int i;
+
+    for ( i = 0; i < HIP04_MAX_CPUS_PER_CLUSTER; i++ )
+        if ( hip04_cpu_table[cluster][i] )
+            return false;
+    return true;
+}
+
+static void hip04_cluster_up(unsigned int cluster)
+{
+    unsigned long data, mask;
+
+    if ( !hip04_cluster_down(cluster) )
+        return;
+
+    data = CLUSTER_L2_RESET_BIT | CLUSTER_DEBUG_RESET_BIT;
+    writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+    do {
+        mask = CLUSTER_L2_RESET_STATUS | \
+               CLUSTER_DEBUG_RESET_STATUS;
+        data = readl_relaxed(sysctrl + \
+                     SC_CPU_RESET_STATUS(cluster));
+    } while ( data & mask );
+    hip04_set_snoop_filter(cluster, 1);
+}
+
+static void __init hip04_iounmap(void __iomem **p)
+{
+    if ( *p )
+    {
+        iounmap(*p);
+        *p = NULL;
+    }
+}
+
+static int __init hip04_smp_init(void)
+{
+    const struct dt_device_node *np, *np_fab, *bw;
+    const char *msg;
+    u64 addr, size;
+
+    np = dt_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
+    msg = "hisilicon,sysctrl missing in DT\n";
+    if ( !np )
+        goto err;
+
+    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
+    msg = "hisilicon,hip04-fabric missing in DT\n";
+    if ( !np_fab )
+        goto err;
+
+    if ( !dt_property_read_u32(np, "bootwrapper-phys",
+                               &hip04_boot.bootwrapper_phys) ) {
+        u32 boot_method[4];
+        bw = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-bootwrapper");
+        msg = "hisilicon,hip04-bootwrapper missing in DT\n";
+        if ( !bw )
+            goto err;
+
+        msg = "failed to get boot-method\n";
+        if ( !dt_property_read_u32_array(bw, "boot-method", boot_method, 4) )
+            goto err;
+        hip04_boot.bootwrapper_phys = boot_method[0];
+        hip04_boot.bootwrapper_size = boot_method[1];
+        hip04_boot.bootwrapper_magic = 0xa5a5a5a5;
+        hip04_boot.relocation_entry = boot_method[2];
+        hip04_boot.relocation_size = boot_method[3];
+    }
+    else
+    {
+        msg = "failed to get bootwrapper-size\n";
+        if ( !dt_property_read_u32(np, "bootwrapper-size",
+                                   &hip04_boot.bootwrapper_size) )
+            goto err;
+
+        msg = "failed to get bootwrapper-magic\n";
+        if ( !dt_property_read_u32(np, "bootwrapper-magic",
+                                   &hip04_boot.bootwrapper_magic) )
+            goto err;
+
+        msg = "failed to get relocation-entry\n";
+        if ( !dt_property_read_u32(np, "relocation-entry",
+                                   &hip04_boot.relocation_entry) )
+            goto err;
+
+        msg = "failed to get relocation-size\n";
+        if ( !dt_property_read_u32(np, "relocation-size",
+                                   &hip04_boot.relocation_size) )
+            goto err;
+    }
+
+    relocation = ioremap_nocache(hip04_boot.relocation_entry,
+                                 hip04_boot.relocation_size);
+    msg = "failed to map relocation space\n";
+    if ( !relocation )
+        goto err;
+
+    msg = "Error in \"hisilicon,sysctrl\"\n";
+    if ( dt_device_get_address(np, 0, &addr, &size) )
+        goto err;
+    sysctrl = ioremap_nocache(addr, size);
+    if ( !sysctrl )
+        goto err;
+
+    msg = "Error in \"hisilicon,hip04-fabric\"\n";
+    if ( dt_device_get_address(np_fab, 0, &addr, &size) )
+        goto err;
+    fabric = ioremap_nocache(addr, size);
+    if ( !fabric )
+        goto err;
+
+    msg = "Error mapping GB2\n";
+    gb2 = ioremap_nocache(0xe4002000, 0x1000);
+    if ( !gb2 )
+        goto err;
+
+    msg = "Error initializing SMP table\n";
+    if ( !hip04_cpu_table_init() )
+        goto err;
+
+    writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+    writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+    writel_relaxed(__pa(init_secondary), relocation + 8);
+    writel_relaxed(0, relocation + 12);
+
+    return 0;
+
+err:
+    hip04_iounmap(&relocation);
+    hip04_iounmap(&sysctrl);
+    hip04_iounmap(&fabric);
+    hip04_iounmap(&gb2);
+
+    printk("%s", msg);
+    return -ENXIO;
+}
+
+static int hip04_cpu_up(int cpu)
+{
+    unsigned int cluster;
+    unsigned long data;
+
+    cluster = cpu / HIP04_MAX_CPUS_PER_CLUSTER;
+    cpu %= HIP04_MAX_CPUS_PER_CLUSTER;
+
+    writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+    writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+    writel_relaxed(__pa(init_secondary), relocation + 8);
+    writel_relaxed(0, relocation + 12);
+
+    hip04_cluster_up(cluster);
+
+    hip04_cpu_table[cluster][cpu]++;
+
+    data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+           CORE_DEBUG_RESET_BIT(cpu);
+    writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+
+    return cpu_up_send_sgi(cpu);
+}
+
+
+static const char * const hip04_dt_compat[] __initconst =
+{
+    "hisilicon,hip04-d01",
+    NULL
+};
+
+static const struct dt_device_match hip04_blacklist_dev[] __initconst =
+{
+    /* Hardware power management */
+    DT_MATCH_COMPATIBLE("hisilicon,sysctrl"),
+    DT_MATCH_COMPATIBLE("hisilicon,hip04-fabric"),
+    { /* sentinel */ },
+};
+
+
+PLATFORM_START(hip04, "HISILICON HIP04")
+    .compatible = hip04_dt_compat,
+    .smp_init = hip04_smp_init,
+    .cpu_up = hip04_cpu_up,
+    .reset = hip04_reset,
+    .blacklist_dev = hip04_blacklist_dev,
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1

Re: [PATCH v2 2/7] xen/arm: Implement hip04-d01 platform

[Julien Grall]

Hi Frediano,

On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> Add this new platform to Xen.
> This platform requires specific code to initialize CPUs.

I guess your platform doesn't support PSCI?

> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
> +static void hip04_reset(void)
> +{
> +    unsigned long data;
> +
> +    if ( !gb2 )
> +        return;

gb2 will never be NULL. Xen will panic if the initialization callback
failed.

> +
> +    data = readl_relaxed(gb2);
> +    writel_relaxed(data & ~0x4000000u, gb2);
> +
> +    mdelay(10);
> +}
> +
> +static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
> +{
> +    unsigned long data;
> +
> +    if ( !fabric )
> +        return;

Ditto.


[..]

> +static void __init hip04_iounmap(void __iomem **p)
> +{
> +    if ( *p )
> +    {
> +        iounmap(*p);
> +        *p = NULL;
> +    }
> +}

What is used for? Should not iounmap enough?

Regards,

-- 
Julien Grall

Re: [PATCH v2 2/7] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> > Add this new platform to Xen.
> > This platform requires specific code to initialize CPUs.
> 
> I guess your platform doesn't support PSCI?
> 

No, there is no PSCI support.

> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> > ---
> > +static void hip04_reset(void)
> > +{
> > +    unsigned long data;
> > +
> > +    if ( !gb2 )
> > +        return;
> 
> gb2 will never be NULL. Xen will panic if the initialization callback
> failed.
> 
> > +
> > +    data = readl_relaxed(gb2);
> > +    writel_relaxed(data & ~0x4000000u, gb2);
> > +
> > +    mdelay(10);
> > +}
> > +
> > +static void hip04_set_snoop_filter(unsigned int cluster, unsigned
> int
> > +on) {
> > +    unsigned long data;
> > +
> > +    if ( !fabric )
> > +        return;
> 
> Ditto.
> 

Done

> 
> [..]
> 
> > +static void __init hip04_iounmap(void __iomem **p) {
> > +    if ( *p )
> > +    {
> > +        iounmap(*p);
> > +        *p = NULL;
> > +    }
> > +}
> 
> What is used for? Should not iounmap enough?
> 

I just like to clear pointers after freeing them.

Frediano

Re: [PATCH v2 2/7] xen/arm: Implement hip04-d01 platform

[Julien Grall]

On 11/04/2014 01:52 PM, Frediano Ziglio wrote:
>>
>> [..]
>>
>>> +static void __init hip04_iounmap(void __iomem **p) {
>>> +    if ( *p )
>>> +    {
>>> +        iounmap(*p);
>>> +        *p = NULL;
>>> +    }
>>> +}
>>
>> What is used for? Should not iounmap enough?
>>
> 
> I just like to clear pointers after freeing them.

It's not really useful :).

If you really want to keep the *p = NULL. This could be simplify into:

hip04_iounmap(....)
{
   iounmap(*p);
   *p = NULL;
}

This is because iounmap takes care of NULL pointer.

Regards,

-- 
Julien Grall

Re: [PATCH v2 2/7] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

> On 11/04/2014 01:52 PM, Frediano Ziglio wrote:
> >>
> >> [..]
> >>
> >>> +static void __init hip04_iounmap(void __iomem **p) {
> >>> +    if ( *p )
> >>> +    {
> >>> +        iounmap(*p);
> >>> +        *p = NULL;
> >>> +    }
> >>> +}
> >>
> >> What is used for? Should not iounmap enough?
> >>
> >
> > I just like to clear pointers after freeing them.
> 
> It's not really useful :).
> 

Just paranoia :)
Never liked dandling pointers.

> If you really want to keep the *p = NULL. This could be simplify into:
> 
> hip04_iounmap(....)
> {
>    iounmap(*p);
>    *p = NULL;
> }
> 
> This is because iounmap takes care of NULL pointer.
> 

Are you sure? I looked at code and is not a simple vm_free call, it does some mapping even if address is NULL.

Regards,
  Frediano

Re: [PATCH v2 2/7] xen/arm: Implement hip04-d01 platform

[Julien Grall]

On 11/04/2014 02:17 PM, Frediano Ziglio wrote:
>> If you really want to keep the *p = NULL. This could be simplify into:
>>
>> hip04_iounmap(....)
>> {
>>    iounmap(*p);
>>    *p = NULL;
>> }
>>
>> This is because iounmap takes care of NULL pointer.
>>
> 
> Are you sure? I looked at code and is not a simple vm_free call, it does some mapping even if address is NULL.

Yes, vm_size(va) will return 0 and destroy_xen_mappings won't unmap
anything because start == end.

I agree that an explicit check in vunmap would be nice.

Regards,

-- 
Julien Grall

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

[Frediano Ziglio]

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.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-v2.c     | 89 +++++++++++++++++++++++++++++++++++++++--------
 xen/arch/arm/gic.c        |  3 +-
 xen/include/asm-arm/gic.h |  4 ++-
 3 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index faad1ff..9461fe3 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -79,16 +79,23 @@ static struct gic_info gicv2_info;
  * logical CPU numbering. Let's use mapping as returned by the GIC
  * itself
  */
-static DEFINE_PER_CPU(u8, gic_cpu_id);
+static DEFINE_PER_CPU(u16, gic_cpu_id);
 
 /* Maximum cpu interface per GIC */
-#define NR_GIC_CPU_IF 8
+static unsigned int nr_gic_cpu_if = 8;
+static unsigned int gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT;
+static unsigned int gic_cpu_mask = 0xff;
 
 static inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
     writeb_relaxed(val, gicv2.map_dbase + offset);
 }
 
+static inline void writew_gicd(uint16_t val, unsigned int offset)
+{
+    writew_relaxed(val, gicv2.map_dbase + offset);
+}
+
 static inline void writel_gicd(uint32_t val, unsigned int offset)
 {
     writel_relaxed(val, gicv2.map_dbase + offset);
@@ -132,7 +139,7 @@ static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
     cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
     for_each_cpu( cpu, &possible_mask )
     {
-        ASSERT(cpu < NR_GIC_CPU_IF);
+        ASSERT(cpu < nr_gic_cpu_if);
         mask |= per_cpu(gic_cpu_id, cpu);
     }
 
@@ -203,6 +210,15 @@ static unsigned int gicv2_read_irq(void)
     return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
 }
 
+/* Set target CPU mask (RAZ/WI on uniprocessor) */
+static void gicv2_set_irq_mask(int irq, unsigned int mask)
+{
+    if ( nr_gic_cpu_if == 16 )
+        writew_gicd(mask, GICD_ITARGETSR + irq * 2);
+    else
+        writeb_gicd(mask, GICD_ITARGETSR + irq);
+}
+
 /*
  * needs to be called with a valid cpu_mask, ie each cpu in the mask has
  * already called gic_cpu_init
@@ -230,7 +246,7 @@ static void gicv2_set_irq_properties(struct irq_desc *desc,
     writel_gicd(cfg, GICD_ICFGR + (irq / 16) * 4);
 
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
-    writeb_gicd(mask, GICD_ITARGETSR + irq);
+    gicv2_set_irq_mask(irq, mask);
     /* Set priority */
     writeb_gicd(priority, GICD_IPRIORITYR + irq);
 
@@ -244,16 +260,21 @@ static void __init gicv2_dist_init(void)
     uint32_t gic_cpus;
     int i;
 
-    cpumask = readl_gicd(GICD_ITARGETSR) & 0xff;
-    cpumask |= cpumask << 8;
-    cpumask |= cpumask << 16;
+    cpumask = readl_gicd(GICD_ITARGETSR) & gic_cpu_mask;
 
     /* Disable the distributor */
     writel_gicd(0, GICD_CTLR);
 
     type = readl_gicd(GICD_TYPER);
     gicv2_info.nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
-    gic_cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
+    if ( nr_gic_cpu_if == 16 )
+    {
+        gic_cpus = 16;
+    }
+    else
+    {
+        gic_cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
+    }
     printk("GICv2: %d lines, %d cpu%s%s (IID %8.8x).\n",
            gicv2_info.nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
            (type & GICD_TYPE_SEC) ? ", secure" : "",
@@ -264,8 +285,19 @@ static void __init gicv2_dist_init(void)
         writel_gicd(0x0, GICD_ICFGR + (i / 16) * 4);
 
     /* Route all global IRQs to this CPU */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
-        writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
+    if ( nr_gic_cpu_if == 16 )
+    {
+        cpumask |= cpumask << 16;
+        for ( i = 32; i < gicv2_info.nr_lines; i += 2 )
+            writel_gicd(cpumask, GICD_ITARGETSR + (i / 2) * 4);
+    }
+    else
+    {
+        cpumask |= cpumask << 8;
+        cpumask |= cpumask << 16;
+        for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
+            writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
+    }
 
     /* Default priority for global interrupts */
     for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
@@ -285,7 +317,7 @@ static void __cpuinit gicv2_cpu_init(void)
 {
     int i;
 
-    this_cpu(gic_cpu_id) = readl_gicd(GICD_ITARGETSR) & 0xff;
+    this_cpu(gic_cpu_id) = readl_gicd(GICD_ITARGETSR) & gic_cpu_mask;
 
     /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so
      * even though they are controlled with GICD registers, they must
@@ -366,7 +398,7 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
         cpumask_and(&online_mask, cpu_mask, &cpu_online_map);
         mask = gicv2_cpu_mask(&online_mask);
         writel_gicd(GICD_SGI_TARGET_LIST |
-                    (mask << GICD_SGI_TARGET_SHIFT) | sgi,
+                    (mask << gicd_sgi_target_shift) | sgi,
                     GICD_SGIR);
         break;
     default:
@@ -581,7 +613,7 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_m
     mask = gicv2_cpu_mask(cpu_mask);
 
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
-    writeb_gicd(mask, GICD_ITARGETSR + desc->irq);
+    gicv2_set_irq_mask(desc->irq, mask);
 
     spin_unlock(&gicv2.lock);
 }
@@ -684,12 +716,19 @@ const static struct gic_hw_operations gicv2_ops = {
 };
 
 /* Set up the GIC */
-static int __init gicv2_init(struct dt_device_node *node, const void *data)
+static int __init gicv2_init_common(struct dt_device_node *node, const void *data, bool hip04)
 {
     int res;
 
     dt_device_set_used_by(node, DOMID_XEN);
 
+    if ( hip04 )
+    {
+        nr_gic_cpu_if = 16;
+        gicd_sgi_target_shift = 8;
+        gic_cpu_mask = 0xffff;
+    }
+
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
     if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
         panic("GICv2: Cannot find a valid address for the distributor");
@@ -764,6 +803,16 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
     return 0;
 }
 
+static int __init gicv2_init(struct dt_device_node *node, const void *data)
+{
+    return gicv2_init_common(node, data, false);
+}
+
+static int __init hip04_gicv2_init(struct dt_device_node *node, const void *data)
+{
+    return gicv2_init_common(node, data, true);
+}
+
 static const char * const gicv2_dt_compat[] __initconst =
 {
     DT_COMPAT_GIC_CORTEX_A15,
@@ -777,6 +826,18 @@ DT_DEVICE_START(gicv2, "GICv2:", DEVICE_GIC)
         .init = gicv2_init,
 DT_DEVICE_END
 
+static const char * const hip04_gicv2_dt_compat[] __initconst =
+{
+    DT_COMPAT_GIC_HIP04,
+    NULL
+};
+
+DT_DEVICE_START(hip04_gicv2, "GICv2:", DEVICE_GIC)
+        .compatible = hip04_gicv2_dt_compat,
+        .init = hip04_gicv2_init,
+DT_DEVICE_END
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 70d10d6..8050a65 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -563,12 +563,13 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
 void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
 {
     unsigned int irq;
+    unsigned int max_irq = gic_hw_ops->info->nr_lines;
 
     do  {
         /* Reading IRQ will ACK it */
         irq = gic_hw_ops->read_irq();
 
-        if ( likely(irq >= 16 && irq < 1021) )
+        if ( likely(irq >= 16 && irq < max_irq) )
         {
             local_irq_enable();
             do_IRQ(regs, irq, is_fiq);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 187dc46..5adb628 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -155,10 +155,12 @@
 #define DT_COMPAT_GIC_400            "arm,gic-400"
 #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
 #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
+#define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
 
 #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
                         DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
-                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400)
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
 
 #define DT_COMPAT_GIC_V3             "arm,gic-v3"
 
-- 
1.9.1

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

[Julien Grall]

On 11/03/2014 04:46 PM, 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.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/gic-v2.c     | 89 +++++++++++++++++++++++++++++++++++++++--------
>  xen/arch/arm/gic.c        |  3 +-
>  xen/include/asm-arm/gic.h |  4 ++-
>  3 files changed, 80 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index faad1ff..9461fe3 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -79,16 +79,23 @@ static struct gic_info gicv2_info;
>   * logical CPU numbering. Let's use mapping as returned by the GIC
>   * itself
>   */
> -static DEFINE_PER_CPU(u8, gic_cpu_id);
> +static DEFINE_PER_CPU(u16, gic_cpu_id);
>  
>  /* Maximum cpu interface per GIC */
> -#define NR_GIC_CPU_IF 8
> +static unsigned int nr_gic_cpu_if = 8;
> +static unsigned int gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT;
> +static unsigned int gic_cpu_mask = 0xff;
>  
>  static inline void writeb_gicd(uint8_t val, unsigned int offset)
>  {
>      writeb_relaxed(val, gicv2.map_dbase + offset);
>  }
>  
> +static inline void writew_gicd(uint16_t val, unsigned int offset)
> +{
> +    writew_relaxed(val, gicv2.map_dbase + offset);
> +}
> +
>  static inline void writel_gicd(uint32_t val, unsigned int offset)
>  {
>      writel_relaxed(val, gicv2.map_dbase + offset);
> @@ -132,7 +139,7 @@ static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
>      cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
>      for_each_cpu( cpu, &possible_mask )
>      {
> -        ASSERT(cpu < NR_GIC_CPU_IF);
> +        ASSERT(cpu < nr_gic_cpu_if);
>          mask |= per_cpu(gic_cpu_id, cpu);
>      }
>  
> @@ -203,6 +210,15 @@ static unsigned int gicv2_read_irq(void)
>      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>  }
>  
> +/* Set target CPU mask (RAZ/WI on uniprocessor) */
> +static void gicv2_set_irq_mask(int irq, unsigned int mask)
> +{
> +    if ( nr_gic_cpu_if == 16 )

This check is very confusing, and even more in patch #5.

Code executed under this check describes your platform and not a generic
16-CPU support (actually there is no spec for it).

I would introduce a new boolean or hide this check in a macro.

> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 70d10d6..8050a65 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -563,12 +563,13 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
>  void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>  {
>      unsigned int irq;
> +    unsigned int max_irq = gic_hw_ops->info->nr_lines;
>  
>      do  {
>          /* Reading IRQ will ACK it */
>          irq = gic_hw_ops->read_irq();
>  
> -        if ( likely(irq >= 16 && irq < 1021) )
> +        if ( likely(irq >= 16 && irq < max_irq) )

On the previous version I've asked that need to explain in the commit
message why this change is valid.

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 11/04/2014 01:31 PM, Julien Grall wrote:
> On 11/03/2014 04:46 PM, 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.
>>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
>> ---
>>  xen/arch/arm/gic-v2.c     | 89 +++++++++++++++++++++++++++++++++++++++--------
>>  xen/arch/arm/gic.c        |  3 +-
>>  xen/include/asm-arm/gic.h |  4 ++-
>>  3 files changed, 80 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index faad1ff..9461fe3 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -79,16 +79,23 @@ static struct gic_info gicv2_info;
>>   * logical CPU numbering. Let's use mapping as returned by the GIC
>>   * itself
>>   */
>> -static DEFINE_PER_CPU(u8, gic_cpu_id);
>> +static DEFINE_PER_CPU(u16, gic_cpu_id);
>>  
>>  /* Maximum cpu interface per GIC */
>> -#define NR_GIC_CPU_IF 8
>> +static unsigned int nr_gic_cpu_if = 8;
>> +static unsigned int gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT;
>> +static unsigned int gic_cpu_mask = 0xff;
>>  
>>  static inline void writeb_gicd(uint8_t val, unsigned int offset)
>>  {
>>      writeb_relaxed(val, gicv2.map_dbase + offset);
>>  }
>>  
>> +static inline void writew_gicd(uint16_t val, unsigned int offset)
>> +{
>> +    writew_relaxed(val, gicv2.map_dbase + offset);
>> +}
>> +
>>  static inline void writel_gicd(uint32_t val, unsigned int offset)
>>  {
>>      writel_relaxed(val, gicv2.map_dbase + offset);
>> @@ -132,7 +139,7 @@ static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
>>      cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
>>      for_each_cpu( cpu, &possible_mask )
>>      {
>> -        ASSERT(cpu < NR_GIC_CPU_IF);
>> +        ASSERT(cpu < nr_gic_cpu_if);
>>          mask |= per_cpu(gic_cpu_id, cpu);
>>      }
>>  
>> @@ -203,6 +210,15 @@ static unsigned int gicv2_read_irq(void)
>>      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>>  }
>>  
>> +/* Set target CPU mask (RAZ/WI on uniprocessor) */
>> +static void gicv2_set_irq_mask(int irq, unsigned int mask)
>> +{
>> +    if ( nr_gic_cpu_if == 16 )
> 
> This check is very confusing, and even more in patch #5.

Sorry, I meant #7.

> Code executed under this check describes your platform and not a generic
> 16-CPU support (actually there is no spec for it).
> 
> I would introduce a new boolean or hide this check in a macro.
> 
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 70d10d6..8050a65 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -563,12 +563,13 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi)
>>  void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>>  {
>>      unsigned int irq;
>> +    unsigned int max_irq = gic_hw_ops->info->nr_lines;
>>  
>>      do  {
>>          /* Reading IRQ will ACK it */
>>          irq = gic_hw_ops->read_irq();
>>  
>> -        if ( likely(irq >= 16 && irq < 1021) )
>> +        if ( likely(irq >= 16 && irq < max_irq) )
> 
> On the previous version I've asked that need to explain in the commit
> message why this change is valid.
> 
> Regards,
> 


-- 
Julien Grall

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

[Frediano Ziglio]

> On 11/03/2014 04:46 PM, 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.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> > ---
> >  xen/arch/arm/gic-v2.c     | 89
> +++++++++++++++++++++++++++++++++++++++--------
> >  xen/arch/arm/gic.c        |  3 +-
> >  xen/include/asm-arm/gic.h |  4 ++-
> >  3 files changed, 80 insertions(+), 16 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > faad1ff..9461fe3 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -79,16 +79,23 @@ static struct gic_info gicv2_info;
> >   * logical CPU numbering. Let's use mapping as returned by the GIC
> >   * itself
> >   */
> > -static DEFINE_PER_CPU(u8, gic_cpu_id);
> > +static DEFINE_PER_CPU(u16, gic_cpu_id);
> >
> >  /* Maximum cpu interface per GIC */
> > -#define NR_GIC_CPU_IF 8
> > +static unsigned int nr_gic_cpu_if = 8; static unsigned int
> > +gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT; static unsigned int
> > +gic_cpu_mask = 0xff;
> >
> >  static inline void writeb_gicd(uint8_t val, unsigned int offset)  {
> >      writeb_relaxed(val, gicv2.map_dbase + offset);  }
> >
> > +static inline void writew_gicd(uint16_t val, unsigned int offset) {
> > +    writew_relaxed(val, gicv2.map_dbase + offset); }
> > +
> >  static inline void writel_gicd(uint32_t val, unsigned int offset)  {
> >      writel_relaxed(val, gicv2.map_dbase + offset); @@ -132,7 +139,7
> > @@ static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
> >      cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> >      for_each_cpu( cpu, &possible_mask )
> >      {
> > -        ASSERT(cpu < NR_GIC_CPU_IF);
> > +        ASSERT(cpu < nr_gic_cpu_if);
> >          mask |= per_cpu(gic_cpu_id, cpu);
> >      }
> >
> > @@ -203,6 +210,15 @@ static unsigned int gicv2_read_irq(void)
> >      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);  }
> >
> > +/* Set target CPU mask (RAZ/WI on uniprocessor) */ static void
> > +gicv2_set_irq_mask(int irq, unsigned int mask) {
> > +    if ( nr_gic_cpu_if == 16 )
> 
> This check is very confusing, and even more in patch #5.
> 
> Code executed under this check describes your platform and not a
> generic 16-CPU support (actually there is no spec for it).
> 
> I would introduce a new boolean or hide this check in a macro.
> 

In some cases is not so terrible, as it's the only 16-bit implementation and as it's assuming ITARGETSR is 16 bit instead of 8. In other cases (like the compatible cases) I fully agree.

I agree a macro should be enough. Something like is_hip04() sounds ok?

> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index
> > 70d10d6..8050a65 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -563,12 +563,13 @@ static void do_sgi(struct cpu_user_regs *regs,
> > enum gic_sgi sgi)  void gic_interrupt(struct cpu_user_regs *regs, int
> > is_fiq)  {
> >      unsigned int irq;
> > +    unsigned int max_irq = gic_hw_ops->info->nr_lines;
> >
> >      do  {
> >          /* Reading IRQ will ACK it */
> >          irq = gic_hw_ops->read_irq();
> >
> > -        if ( likely(irq >= 16 && irq < 1021) )
> > +        if ( likely(irq >= 16 && irq < max_irq) )
> 
> On the previous version I've asked that need to explain in the commit
> message why this change is valid.
> 

Regards,
  Frediano

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

[Julien Grall]

On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
>  /* Set up the GIC */
> -static int __init gicv2_init(struct dt_device_node *node, const void *data)
> +static int __init gicv2_init_common(struct dt_device_node *node, const void *data, bool hip04)
>  {
>      int res;
>  
>      dt_device_set_used_by(node, DOMID_XEN);
>  
> +    if ( hip04 )
> +    {
> +        nr_gic_cpu_if = 16;
> +        gicd_sgi_target_shift = 8;
> +        gic_cpu_mask = 0xffff;
> +    }
> +

And this could be moved in hip04_gicv2_init. So there is no need of
adding a boolean to the arguments.

-- 
Julien Grall

[PATCH v2 4/7] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Frediano Ziglio]

This name can appear in some Linux kernel repos. Not very fortunate,
but to avoid others spending an hour to spot that few characters
difference it worth to work around it.

Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-v2.c     | 1 +
 xen/include/asm-arm/gic.h | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 9461fe3..04e1850 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -829,6 +829,7 @@ DT_DEVICE_END
 static const char * const hip04_gicv2_dt_compat[] __initconst =
 {
     DT_COMPAT_GIC_HIP04,
+    DT_COMPAT_GIC_HIP04_2,
     NULL
 };
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 5adb628..3d2b3db 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -156,11 +156,13 @@
 #define DT_COMPAT_GIC_CORTEX_A15     "arm,cortex-a15-gic"
 #define DT_COMPAT_GIC_CORTEX_A7      "arm,cortex-a7-gic"
 #define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-gic"
+#define DT_COMPAT_GIC_HIP04_2        "hisilicon,hip04-intc"
 
 #define DT_MATCH_GIC_V2 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A15), \
                         DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7), \
                         DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400), \
-                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04), \
+                        DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04_2)
 
 #define DT_COMPAT_GIC_V3             "arm,gic-v3"
 
-- 
1.9.1

[PATCH v2 5/7] xen/arm: handle GICH register changes for hip04-d01 platform

[Frediano Ziglio]

The GICH in this platform is mainly compatible with the standard
GICv2 beside APR and LR register offsets.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/gic-v2.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 04e1850..411b104 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -61,6 +61,9 @@
 #define GICH_V2_VMCR_PRIORITY_MASK   0x1f
 #define GICH_V2_VMCR_PRIORITY_SHIFT  27
 
+#define HIP04_GICH_APR  0x70
+#define HIP04_GICH_LR   0x80
+
 /* Global state */
 static struct {
     paddr_t dbase;            /* Address of distributor registers */
@@ -85,6 +88,8 @@ static DEFINE_PER_CPU(u16, gic_cpu_id);
 static unsigned int nr_gic_cpu_if = 8;
 static unsigned int gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT;
 static unsigned int gic_cpu_mask = 0xff;
+static unsigned int gich_apr = GICH_APR;
+static unsigned int gich_lr = GICH_LR;
 
 static inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
@@ -155,9 +160,9 @@ static void gicv2_save_state(struct vcpu *v)
      * accessed simultaneously by another pCPU.
      */
     for ( i = 0; i < gicv2_info.nr_lrs; i++ )
-        v->arch.gic.v2.lr[i] = readl_gich(GICH_LR + i * 4);
+        v->arch.gic.v2.lr[i] = readl_gich(gich_lr + i * 4);
 
-    v->arch.gic.v2.apr = readl_gich(GICH_APR);
+    v->arch.gic.v2.apr = readl_gich(gich_apr);
     v->arch.gic.v2.vmcr = readl_gich(GICH_VMCR);
     /* Disable until next VCPU scheduled */
     writel_gich(0, GICH_HCR);
@@ -168,9 +173,9 @@ static void gicv2_restore_state(const struct vcpu *v)
     int i;
 
     for ( i = 0; i < gicv2_info.nr_lrs; i++ )
-        writel_gich(v->arch.gic.v2.lr[i], GICH_LR + i * 4);
+        writel_gich(v->arch.gic.v2.lr[i], gich_lr + i * 4);
 
-    writel_gich(v->arch.gic.v2.apr, GICH_APR);
+    writel_gich(v->arch.gic.v2.apr, gich_apr);
     writel_gich(v->arch.gic.v2.vmcr, GICH_VMCR);
     writel_gich(GICH_HCR_EN, GICH_HCR);
 }
@@ -183,7 +188,7 @@ static void gicv2_dump_state(const struct vcpu *v)
     {
         for ( i = 0; i < gicv2_info.nr_lrs; i++ )
             printk("   HW_LR[%d]=%x\n", i,
-                   readl_gich(GICH_LR + i * 4));
+                   readl_gich(gich_lr + i * 4));
     }
     else
     {
@@ -437,12 +442,12 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p,
                             << GICH_V2_LR_PHYSICAL_SHIFT);
     }
 
-    writel_gich(lr_reg, GICH_LR + lr * 4);
+    writel_gich(lr_reg, gich_lr + lr * 4);
 }
 
 static void gicv2_clear_lr(int lr)
 {
-    writel_gich(0, GICH_LR + lr * 4);
+    writel_gich(0, gich_lr + lr * 4);
 }
 
 static int gicv2v_setup(struct domain *d)
@@ -492,7 +497,7 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
 {
     uint32_t lrv;
 
-    lrv          = readl_gich(GICH_LR + lr * 4);
+    lrv          = readl_gich(gich_lr + lr * 4);
     lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
     lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
     lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
@@ -515,7 +520,7 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
                                        << GICH_V2_LR_HW_SHIFT)  |
           ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) );
 
-    writel_gich(lrv, GICH_LR + lr * 4);
+    writel_gich(lrv, gich_lr + lr * 4);
 }
 
 static void gicv2_hcr_status(uint32_t flag, bool_t status)
@@ -538,7 +543,7 @@ static unsigned int gicv2_read_vmcr_priority(void)
 
 static unsigned int gicv2_read_apr(int apr_reg)
 {
-   return readl_gich(GICH_APR);
+   return readl_gich(gich_apr);
 }
 
 static void gicv2_irq_enable(struct irq_desc *desc)
@@ -727,6 +732,8 @@ static int __init gicv2_init_common(struct dt_device_node *node, const void *dat
         nr_gic_cpu_if = 16;
         gicd_sgi_target_shift = 8;
         gic_cpu_mask = 0xffff;
+        gich_apr = HIP04_GICH_APR;
+        gich_lr = HIP04_GICH_LR;
     }
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
-- 
1.9.1

[PATCH v2 6/7] xen/arm: Force domains to use normal GICv2 driver on Hip04 platform

[Frediano Ziglio]

Until vGIC support is not implemented and tested, this will prevent
guest kernels to use their Hip04 driver, or crash when they don't
have any.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/gic-v2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 411b104..cea9edc 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -639,6 +639,12 @@ static int gicv2_make_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOENT);
     }
 
+    if ( nr_gic_cpu_if == 16 )
+    {
+        compatible = DT_COMPAT_GIC_CORTEX_A15;
+        len = strlen((char*) compatible) + 1;
+    }
+
     res = fdt_begin_node(fdt, "interrupt-controller");
     if ( res )
         return res;
-- 
1.9.1

Re: [PATCH v2 6/7] xen/arm: Force domains to use normal GICv2 driver on Hip04 platform

[Julien Grall]

Hi Frediano,

FYI, this is only force DOM0 to use the normal GICv2 drivers. I.e the
title of this patch is wrong.

Do you have any plan to support hi-silicon vGIC in Xen? This would allow
guest running with more than 8 cores.

Regards,

On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> Until vGIC support is not implemented and tested, this will prevent
> guest kernels to use their Hip04 driver, or crash when they don't
> have any.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> ---
>  xen/arch/arm/gic-v2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 411b104..cea9edc 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -639,6 +639,12 @@ static int gicv2_make_dt_node(const struct domain *d,
>          return -FDT_ERR_XEN(ENOENT);
>      }
>  
> +    if ( nr_gic_cpu_if == 16 )
> +    {
> +        compatible = DT_COMPAT_GIC_CORTEX_A15;
> +        len = strlen((char*) compatible) + 1;
> +    }
> +
>      res = fdt_begin_node(fdt, "interrupt-controller");
>      if ( res )
>          return res;
> 


-- 
Julien Grall

Re: [PATCH v2 6/7] xen/arm: Force domains to use normal GICv2 driver on Hip04 platform

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> FYI, this is only force DOM0 to use the normal GICv2 drivers. I.e the
> title of this patch is wrong.
> 
> Do you have any plan to support hi-silicon vGIC in Xen? This would
> allow guest running with more than 8 cores.
> 
> Regards,
> 

Actually there's no plan to do it.

Frediano

> On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> > Until vGIC support is not implemented and tested, this will prevent
> > guest kernels to use their Hip04 driver, or crash when they don't
> have
> > any.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > ---
> >  xen/arch/arm/gic-v2.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > 411b104..cea9edc 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -639,6 +639,12 @@ static int gicv2_make_dt_node(const struct
> domain *d,
> >          return -FDT_ERR_XEN(ENOENT);
> >      }
> >
> > +    if ( nr_gic_cpu_if == 16 )
> > +    {
> > +        compatible = DT_COMPAT_GIC_CORTEX_A15;
> > +        len = strlen((char*) compatible) + 1;
> > +    }
> > +
> >      res = fdt_begin_node(fdt, "interrupt-controller");
> >      if ( res )
> >          return res;
> >
> 
> 
> --
> Julien Grall

[PATCH v2 7/7] xen/arm: Move vGIC registers on Hip04 platform

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/gic-v2.c     | 15 +++++++++++++--
 xen/include/asm-arm/gic.h |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index cea9edc..eb8cc19 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOMEM);
 
     tmp = new_cells;
-    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
-    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
+
+    if ( nr_gic_cpu_if == 16 )
+    {
+        dt_set_range(&tmp, node, d->arch.vgic.dbase - HIP04_VGIC_REG_OFFSET,
+                     PAGE_SIZE);
+        dt_set_range(&tmp, node, d->arch.vgic.cbase - HIP04_VGIC_REG_OFFSET,
+                     PAGE_SIZE * 2);
+    }
+    else
+    {
+        dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
+        dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
+    }
 
     res = fdt_property(fdt, "reg", new_cells, len);
     xfree(new_cells);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 3d2b3db..5af8201 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -147,6 +147,8 @@
 #define GICH_LR_PENDING         1
 #define GICH_LR_ACTIVE          2
 
+#define HIP04_VGIC_REG_OFFSET   0xe0000000
+
 #ifndef __ASSEMBLY__
 #include <xen/device_tree.h>
 #include <xen/irq.h>
-- 
1.9.1

Re: [PATCH v2 7/7] xen/arm: Move vGIC registers on Hip04 platform

[Julien Grall]

Hi Frediano,

On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/gic-v2.c     | 15 +++++++++++++--
>  xen/include/asm-arm/gic.h |  2 ++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index cea9edc..eb8cc19 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct domain *d,
>          return -FDT_ERR_XEN(ENOMEM);
>  
>      tmp = new_cells;
> -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> +
> +    if ( nr_gic_cpu_if == 16 )
> +    {
> +        dt_set_range(&tmp, node, d->arch.vgic.dbase - HIP04_VGIC_REG_OFFSET,
> +                     PAGE_SIZE);
> +        dt_set_range(&tmp, node, d->arch.vgic.cbase - HIP04_VGIC_REG_OFFSET,
> +                     PAGE_SIZE * 2);
> +    }
> +    else
> +    {
> +        dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> +        dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> +    }
>  
>      res = fdt_property(fdt, "reg", new_cells, len);
>      xfree(new_cells);
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 3d2b3db..5af8201 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -147,6 +147,8 @@
>  #define GICH_LR_PENDING         1
>  #define GICH_LR_ACTIVE          2
>  
> +#define HIP04_VGIC_REG_OFFSET   0xe0000000
> +

Please move this define in gic-v2.c. The header gic.h should only
contains value that needs to be shared with the vgic and/or the other
GIC drivers.

Also, where does come from the offset? Any pointer to the documentation?

Regards,

-- 
Julien Grall

Re: [PATCH v2 7/7] xen/arm: Move vGIC registers on Hip04 platform

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> > ---
> >  xen/arch/arm/gic-v2.c     | 15 +++++++++++++--
> >  xen/include/asm-arm/gic.h |  2 ++
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > cea9edc..eb8cc19 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct
> domain *d,
> >          return -FDT_ERR_XEN(ENOMEM);
> >
> >      tmp = new_cells;
> > -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> > +
> > +    if ( nr_gic_cpu_if == 16 )
> > +    {
> > +        dt_set_range(&tmp, node, d->arch.vgic.dbase -
> HIP04_VGIC_REG_OFFSET,
> > +                     PAGE_SIZE);
> > +        dt_set_range(&tmp, node, d->arch.vgic.cbase -
> HIP04_VGIC_REG_OFFSET,
> > +                     PAGE_SIZE * 2);
> > +    }
> > +    else
> > +    {
> > +        dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > +        dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> > +    }
> >
> >      res = fdt_property(fdt, "reg", new_cells, len);
> >      xfree(new_cells);
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index 3d2b3db..5af8201 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -147,6 +147,8 @@
> >  #define GICH_LR_PENDING         1
> >  #define GICH_LR_ACTIVE          2
> >
> > +#define HIP04_VGIC_REG_OFFSET   0xe0000000
> > +
> 
> Please move this define in gic-v2.c. The header gic.h should only
> contains value that needs to be shared with the vgic and/or the other
> GIC drivers.
> 
> Also, where does come from the offset? Any pointer to the documentation?
> 

Well,
  I think I already sent a mail for this problem but got no reply (or I missed it).

The problem came from how the DTB is in Linux and how Xen override devices in the DTB.

On Linux I have

/ {
...
       soc {
                /* It's a 32-bit SoC. */
                #address-cells = <1>;
                #size-cells = <1>;
                compatible = "simple-bus";
                interrupt-parent = <&gic>;
                ranges = <0 0 0xe0000000 0x10000000>;

                gic: interrupt-controller@c01000 {
                        compatible = "hisilicon,hip04-intc";
                        #interrupt-cells = <3>;
                        #address-cells = <0>;
                        interrupt-controller;
                        interrupts = <1 9 0xf04>;

                        reg = <0xc01000 0x1000>, <0xc02000 0x1000>,
                              <0xc04000 0x2000>, <0xc06000 0x2000>;
                };
...

So the address of controller is 0xec01000 (see ranges in /soc and reg in /soc/interrupt-controller@c01000).

Now Xen compute address as 0xec01000 (which is fine) but then when it has to provide a virtual gic it just replace the gic entry with one with reg with 0xec01000 not taking into account the range is putting the reg into. This lead kernel to think the address is 0xe0000000+0xec01000 instead of 0xe00000000+0xc01000. Now... the DTB from Linux is perfectly legal but Xen does not handle it properly. I mostly consider this as a temporary workaround (I wrote a small comment on first version).

About solution to this there are different options:
- put gic always in the root to to have full range without any offset;
- fix reg according to range. This however could lead to extend the range;
- remove all ranges (and fix all devices' reg) to have always no offsets.

Regards,
  Frediano

Re: [PATCH v2 7/7] xen/arm: Move vGIC registers on Hip04 platform

[Ian Campbell]

On Tue, 2014-11-04 at 14:42 +0000, Frediano Ziglio wrote:
> > 
> > Hi Frediano,
> > 
> > On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> > > ---
> > >  xen/arch/arm/gic-v2.c     | 15 +++++++++++++--
> > >  xen/include/asm-arm/gic.h |  2 ++
> > >  2 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > > cea9edc..eb8cc19 100644
> > > --- a/xen/arch/arm/gic-v2.c
> > > +++ b/xen/arch/arm/gic-v2.c
> > > @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct
> > domain *d,
> > >          return -FDT_ERR_XEN(ENOMEM);
> > >
> > >      tmp = new_cells;
> > > -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > > -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> > > +
> > > +    if ( nr_gic_cpu_if == 16 )
> > > +    {
> > > +        dt_set_range(&tmp, node, d->arch.vgic.dbase -
> > HIP04_VGIC_REG_OFFSET,
> > > +                     PAGE_SIZE);
> > > +        dt_set_range(&tmp, node, d->arch.vgic.cbase -
> > HIP04_VGIC_REG_OFFSET,
> > > +                     PAGE_SIZE * 2);
> > > +    }
> > > +    else
> > > +    {
> > > +        dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > > +        dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> > > +    }
> > >
> > >      res = fdt_property(fdt, "reg", new_cells, len);
> > >      xfree(new_cells);
> > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > > index 3d2b3db..5af8201 100644
> > > --- a/xen/include/asm-arm/gic.h
> > > +++ b/xen/include/asm-arm/gic.h
> > > @@ -147,6 +147,8 @@
> > >  #define GICH_LR_PENDING         1
> > >  #define GICH_LR_ACTIVE          2
> > >
> > > +#define HIP04_VGIC_REG_OFFSET   0xe0000000
> > > +
> > 
> > Please move this define in gic-v2.c. The header gic.h should only
> > contains value that needs to be shared with the vgic and/or the other
> > GIC drivers.
> > 
> > Also, where does come from the offset? Any pointer to the documentation?
> > 
> 
> Well,
>   I think I already sent a mail for this problem but got no reply (or I missed it).
> 
> The problem came from how the DTB is in Linux and how Xen override devices in the DTB.
> 
> On Linux I have
> 
> / {
> ...
>        soc {
>                 /* It's a 32-bit SoC. */
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 compatible = "simple-bus";
>                 interrupt-parent = <&gic>;
>                 ranges = <0 0 0xe0000000 0x10000000>;
> 
>                 gic: interrupt-controller@c01000 {
>                         compatible = "hisilicon,hip04-intc";
>                         #interrupt-cells = <3>;
>                         #address-cells = <0>;
>                         interrupt-controller;
>                         interrupts = <1 9 0xf04>;
> 
>                         reg = <0xc01000 0x1000>, <0xc02000 0x1000>,
>                               <0xc04000 0x2000>, <0xc06000 0x2000>;
>                 };
> ...
> 
> So the address of controller is 0xec01000 (see ranges in /soc and reg in /soc/interrupt-controller@c01000).
> 
> Now Xen compute address as 0xec01000 (which is fine) but then when it has to provide a virtual gic it just replace the gic entry with one with reg with 0xec01000 not taking into account the range is putting the reg into. This lead kernel to think the address is 0xe0000000+0xec01000 instead of 0xe00000000+0xc01000. Now... the DTB from Linux is perfectly legal but Xen does not handle it properly. I mostly consider this as a temporary workaround (I wrote a small comment on first version).
> 
> About solution to this there are different options:
> - put gic always in the root to to have full range without any offset;
> - fix reg according to range. This however could lead to extend the range;
> - remove all ranges (and fix all devices' reg) to have always no offsets.

  - Propagate the host GICC register value literally over, having 
    mapping the GICV there. i.e. don't translate the value which is 
    written to the DT.

None of the other options sound very good to me.

Ian.

Re: [PATCH v2 7/7] xen/arm: Move vGIC registers on Hip04 platform

[Frediano Ziglio]

> On Tue, 2014-11-04 at 14:42 +0000, Frediano Ziglio wrote:
> > >
> > > Hi Frediano,
> > >
> > > On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> > > > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > > > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> > > > ---
> > > >  xen/arch/arm/gic-v2.c     | 15 +++++++++++++--
> > > >  xen/include/asm-arm/gic.h |  2 ++
> > > >  2 files changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > > > cea9edc..eb8cc19 100644
> > > > --- a/xen/arch/arm/gic-v2.c
> > > > +++ b/xen/arch/arm/gic-v2.c
> > > > @@ -669,8 +669,19 @@ static int gicv2_make_dt_node(const struct
> > > domain *d,
> > > >          return -FDT_ERR_XEN(ENOMEM);
> > > >
> > > >      tmp = new_cells;
> > > > -    dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > > > -    dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE * 2);
> > > > +
> > > > +    if ( nr_gic_cpu_if == 16 )
> > > > +    {
> > > > +        dt_set_range(&tmp, node, d->arch.vgic.dbase -
> > > HIP04_VGIC_REG_OFFSET,
> > > > +                     PAGE_SIZE);
> > > > +        dt_set_range(&tmp, node, d->arch.vgic.cbase -
> > > HIP04_VGIC_REG_OFFSET,
> > > > +                     PAGE_SIZE * 2);
> > > > +    }
> > > > +    else
> > > > +    {
> > > > +        dt_set_range(&tmp, node, d->arch.vgic.dbase, PAGE_SIZE);
> > > > +        dt_set_range(&tmp, node, d->arch.vgic.cbase, PAGE_SIZE *
> 2);
> > > > +    }
> > > >
> > > >      res = fdt_property(fdt, "reg", new_cells, len);
> > > >      xfree(new_cells);
> > > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-
> arm/gic.h
> > > > index 3d2b3db..5af8201 100644
> > > > --- a/xen/include/asm-arm/gic.h
> > > > +++ b/xen/include/asm-arm/gic.h
> > > > @@ -147,6 +147,8 @@
> > > >  #define GICH_LR_PENDING         1
> > > >  #define GICH_LR_ACTIVE          2
> > > >
> > > > +#define HIP04_VGIC_REG_OFFSET   0xe0000000
> > > > +
> > >
> > > Please move this define in gic-v2.c. The header gic.h should only
> > > contains value that needs to be shared with the vgic and/or the
> > > other GIC drivers.
> > >
> > > Also, where does come from the offset? Any pointer to the
> documentation?
> > >
> >
> > Well,
> >   I think I already sent a mail for this problem but got no reply (or
> I missed it).
> >
> > The problem came from how the DTB is in Linux and how Xen override
> devices in the DTB.
> >
> > On Linux I have
> >
> > / {
> > ...
> >        soc {
> >                 /* It's a 32-bit SoC. */
> >                 #address-cells = <1>;
> >                 #size-cells = <1>;
> >                 compatible = "simple-bus";
> >                 interrupt-parent = <&gic>;
> >                 ranges = <0 0 0xe0000000 0x10000000>;
> >
> >                 gic: interrupt-controller@c01000 {
> >                         compatible = "hisilicon,hip04-intc";
> >                         #interrupt-cells = <3>;
> >                         #address-cells = <0>;
> >                         interrupt-controller;
> >                         interrupts = <1 9 0xf04>;
> >
> >                         reg = <0xc01000 0x1000>, <0xc02000 0x1000>,
> >                               <0xc04000 0x2000>, <0xc06000 0x2000>;
> >                 };
> > ...
> >
> > So the address of controller is 0xec01000 (see ranges in /soc and reg
> in /soc/interrupt-controller@c01000).
> >
> > Now Xen compute address as 0xec01000 (which is fine) but then when it
> has to provide a virtual gic it just replace the gic entry with one
> with reg with 0xec01000 not taking into account the range is putting
> the reg into. This lead kernel to think the address is
> 0xe0000000+0xec01000 instead of 0xe00000000+0xc01000. Now... the DTB
> from Linux is perfectly legal but Xen does not handle it properly. I
> mostly consider this as a temporary workaround (I wrote a small comment
> on first version).
> >
> > About solution to this there are different options:
> > - put gic always in the root to to have full range without any offset;
> > - fix reg according to range. This however could lead to extend the
> > range;
> > - remove all ranges (and fix all devices' reg) to have always no
> offsets.
> 
>   - Propagate the host GICC register value literally over, having
>     mapping the GICV there. i.e. don't translate the value which is
>     written to the DT.
> 
> None of the other options sound very good to me.
> 
> Ian.

Yes,
  You are right, KISS rule apply!

Implemented and working correctly.

Regards,
  Frediano