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).

[PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

Add this new platform to Xen.
This platform require 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  | 258 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 259 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..bf38c23
--- /dev/null
+++ b/xen/arch/arm/platforms/hip04.c
@@ -0,0 +1,258 @@
+/*
+ * 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>
+
+#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;
+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)
+{
+    /* TODO */
+}
+
+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) ) {
+        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 int __init hip04_smp_init(void)
+{
+    struct dt_device_node *np, *np_fab;
+    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;
+
+    msg = "failed to get bootwrapper-phys\n";
+    if ( !dt_property_read_u32(np, "bootwrapper-phys",
+                               &hip04_boot.bootwrapper_phys) )
+        goto err;
+
+    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 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:
+    printk("%s", msg);
+    return -ENXIO;
+}
+
+static int hip04_cpu_up(int cpu)
+{
+    unsigned int cluster = cpu / 4;
+    unsigned long data;
+    cpu %= 4;
+
+    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 01/10] xen/arm: Implement hip04-d01 platform

[Julien Grall]

Hi Frediano,

On 11/03/2014 10:11 AM, Frediano Ziglio wrote:
> Add this new platform to Xen.
> This platform require specific code to initialize CPUs.

s/require/requires/

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>
> ---

[..]

> diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
> new file mode 100644
> index 0000000..bf38c23
> --- /dev/null
> +++ b/xen/arch/arm/platforms/hip04.c
> @@ -0,0 +1,258 @@

[..]

> +
> +struct hip04_secondary_cpu_data {

coding style:

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;
> +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)
> +{
> +    /* TODO */

Why did you implement the reset in a separate patch rather than here?

> +}
> +
> +static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
> +{
> +    unsigned long data;
> +
> +    if (!fabric)

Coding style:

if ( !fabric )

> +        return;
> +    data = readl_relaxed(fabric + FAB_SF_MODE);
> +    if (on)

if ( on )

> +        data |= 1 << cluster;
> +    else
> +        data &= ~(1 << cluster);
> +    writel_relaxed(data, fabric + FAB_SF_MODE);
> +    while (1) {

while ( 1 )
{

> +        if (data == readl_relaxed(fabric + FAB_SF_MODE))

if ( ... )

> +            break;
> +    }

The loop is turning in an infinite loop if the reading value is never
correct.

> +}
> +
> +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) {

if ( ...
     ... )
{

> +        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;

Missing blank line

> +    return true;
> +}
> +
> +static bool hip04_cluster_down(unsigned int cluster)
> +{
> +    int i;
> +
> +    for (i = 0; i < HIP04_MAX_CPUS_PER_CLUSTER; i++)

for ( ... )

> +        if (hip04_cpu_table[cluster][i])

if ( ... )

> +            return false;
> +    return true;
> +}
> +
> +static void hip04_cluster_up(unsigned int cluster)
> +{
> +    unsigned long data, mask;
> +
> +    if ( hip04_cluster_down(cluster) ) {

Wouldn't it be easier to return if the cluster is up? It will one layer
of indentation.

> +        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 int __init hip04_smp_init(void)
> +{
> +    struct dt_device_node *np, *np_fab;

The device node are not modified so:

const struct

[..]

> +    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:

For consistence, you should unmap everything you mapped with ioremap_*

> +    printk("%s", msg);
> +    return -ENXIO;
> +}
> +
> +static int hip04_cpu_up(int cpu)
> +{
> +    unsigned int cluster = cpu / 4;

The number 4 is confusing here, why not using the define you've created
above? Such as  HIP04_MAX_CPUS_PER_CLUSTER

> +    unsigned long data;

The coding style requires a blank line after the declarations block.

> +    cpu %= 4;

Ditto for the number 4.

Regards,

-- 
Julien Grall

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Ian Campbell]

On Mon, 2014-11-03 at 10:11 +0000, Frediano Ziglio wrote:
> Add this new platform to Xen.
> This platform require specific code to initialize CPUs.

What is the "bootwrapper"? Are you running this on real silicon or on an
emulator? Can the platform be made to do PSCI instead?

> +    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");

Please add a reference to the DT bindings document for these values.

linux/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
seems related but doesn't talk about most of these fields.

> +    msg = "hisilicon,hip04-fabric missing in DT\n";
> +    if ( !np_fab )
> +        goto err;
> +
> +    msg = "failed to get bootwrapper-phys\n";
> +    if ( !dt_property_read_u32(np, "bootwrapper-phys",
> +                               &hip04_boot.bootwrapper_phys) )
> +        goto err;
> +
> +    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;

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

2015-01-13 11:58 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
> On Mon, 2014-11-03 at 10:11 +0000, Frediano Ziglio wrote:
>> Add this new platform to Xen.
>> This platform require specific code to initialize CPUs.
>
> What is the "bootwrapper"? Are you running this on real silicon or on an
> emulator? Can the platform be made to do PSCI instead?
>

Very real. It's actually on my desk and I'm not in Matrix :-)
Has no PSCI support. Would be honestly very great. As we (as company)
write the firmware could be technically doable. There is no plan. This
piece of software is meant to bring the CPU from Secure mode to
Unsecure Hypervisor mode before calling kernel/hypervisor code and
provide supervisor calls.

>> +    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
>
> Please add a reference to the DT bindings document for these values.
>
> linux/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> seems related but doesn't talk about most of these fields.
>

There are documentation in the Linaro kernel, see
https://git.linaro.org/kernel/linux-linaro-tracking.git/blob/HEAD:/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt.
I hope it will be merged soon.

Frediano

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Ian Campbell]

On Tue, 2015-01-13 at 14:09 +0000, Frediano Ziglio wrote:
> 2015-01-13 11:58 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
> > On Mon, 2014-11-03 at 10:11 +0000, Frediano Ziglio wrote:
> >> Add this new platform to Xen.
> >> This platform require specific code to initialize CPUs.
> >
> > What is the "bootwrapper"? Are you running this on real silicon or on an
> > emulator? Can the platform be made to do PSCI instead?
> >
> 
> Very real. It's actually on my desk and I'm not in Matrix :-)

OK. The choice of bootwrapper as a name is a bit unfortunate, since it
is already used for something else, but oh well.

> Has no PSCI support. Would be honestly very great. As we (as company)
> write the firmware could be technically doable. There is no plan. This
> piece of software is meant to bring the CPU from Secure mode to
> Unsecure Hypervisor mode before calling kernel/hypervisor code and
> provide supervisor calls.

Sounds a lot like PSCI to me, except non-standard ;-)

> >> +    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
> >
> > Please add a reference to the DT bindings document for these values.
> >
> > linux/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> > seems related but doesn't talk about most of these fields.
> >
> 
> There are documentation in the Linaro kernel, see
> https://git.linaro.org/kernel/linux-linaro-tracking.git/blob/HEAD:/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt.
> I hope it will be merged soon.

Thanks, but this doesn't seem to cover many of the properties used by
the code you are adding, e.g. bootwrapper-{size,magic},
relocation-{entry,size} (in fact it suggests they are part of a
boot-method array).

I get the feeling these might be legacy/deprecated. Perhaps we could get
away without supporting such things?

Ian.

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

2015-01-13 14:42 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
> On Tue, 2015-01-13 at 14:09 +0000, Frediano Ziglio wrote:
>> 2015-01-13 11:58 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
>> > On Mon, 2014-11-03 at 10:11 +0000, Frediano Ziglio wrote:
>> >> Add this new platform to Xen.
>> >> This platform require specific code to initialize CPUs.
>> >
>> > What is the "bootwrapper"? Are you running this on real silicon or on an
>> > emulator? Can the platform be made to do PSCI instead?
>> >
>>
>> Very real. It's actually on my desk and I'm not in Matrix :-)
>
> OK. The choice of bootwrapper as a name is a bit unfortunate, since it
> is already used for something else, but oh well.
>
>> Has no PSCI support. Would be honestly very great. As we (as company)
>> write the firmware could be technically doable. There is no plan. This
>> piece of software is meant to bring the CPU from Secure mode to
>> Unsecure Hypervisor mode before calling kernel/hypervisor code and
>> provide supervisor calls.
>
> Sounds a lot like PSCI to me, except non-standard ;-)
>
>> >> +    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
>> >
>> > Please add a reference to the DT bindings document for these values.
>> >
>> > linux/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
>> > seems related but doesn't talk about most of these fields.
>> >
>>
>> There are documentation in the Linaro kernel, see
>> https://git.linaro.org/kernel/linux-linaro-tracking.git/blob/HEAD:/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt.
>> I hope it will be merged soon.
>
> Thanks, but this doesn't seem to cover many of the properties used by
> the code you are adding, e.g. bootwrapper-{size,magic},
> relocation-{entry,size} (in fact it suggests they are part of a
> boot-method array).
>
> I get the feeling these might be legacy/deprecated. Perhaps we could get
> away without supporting such things?
>
> Ian.
>

No, this is the newest code. Yes, PSCI would be definitively much
better and way more standard.

Frediano

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Ian Campbell]

On Tue, 2015-01-13 at 15:11 +0000, Frediano Ziglio wrote:
> 2015-01-13 14:42 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
> > On Tue, 2015-01-13 at 14:09 +0000, Frediano Ziglio wrote:
> >> 2015-01-13 11:58 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
> >> > On Mon, 2014-11-03 at 10:11 +0000, Frediano Ziglio wrote:
> >> >> Add this new platform to Xen.
> >> >> This platform require specific code to initialize CPUs.
> >> >
> >> > What is the "bootwrapper"? Are you running this on real silicon or on an
> >> > emulator? Can the platform be made to do PSCI instead?
> >> >
> >>
> >> Very real. It's actually on my desk and I'm not in Matrix :-)
> >
> > OK. The choice of bootwrapper as a name is a bit unfortunate, since it
> > is already used for something else, but oh well.
> >
> >> Has no PSCI support. Would be honestly very great. As we (as company)
> >> write the firmware could be technically doable. There is no plan. This
> >> piece of software is meant to bring the CPU from Secure mode to
> >> Unsecure Hypervisor mode before calling kernel/hypervisor code and
> >> provide supervisor calls.
> >
> > Sounds a lot like PSCI to me, except non-standard ;-)
> >
> >> >> +    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
> >> >
> >> > Please add a reference to the DT bindings document for these values.
> >> >
> >> > linux/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> >> > seems related but doesn't talk about most of these fields.
> >> >
> >>
> >> There are documentation in the Linaro kernel, see
> >> https://git.linaro.org/kernel/linux-linaro-tracking.git/blob/HEAD:/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt.
> >> I hope it will be merged soon.
> >
> > Thanks, but this doesn't seem to cover many of the properties used by
> > the code you are adding, e.g. bootwrapper-{size,magic},
> > relocation-{entry,size} (in fact it suggests they are part of a
> > boot-method array).
> >
> > I get the feeling these might be legacy/deprecated. Perhaps we could get
> > away without supporting such things?
> >
> > Ian.
> >
> 
> No, this is the newest code.

So in this:
    if ( !dt_property_read_u32(np, "bootwrapper-phys",
                               &hip04_boot.bootwrapper_phys) ) {
[...read boot-method array...]
    }
    else
    {
[... read a bunch of separate properties...]
    }

Both sides of the if are used by non-legacy code? That's,...
unfortunate.

I note that only the if case, not the else case, is covered by the
hisilicon.txt bindings...

Ian.

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

2015-01-13 15:23 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
> On Tue, 2015-01-13 at 15:11 +0000, Frediano Ziglio wrote:
>> 2015-01-13 14:42 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
>> > On Tue, 2015-01-13 at 14:09 +0000, Frediano Ziglio wrote:
>> >> 2015-01-13 11:58 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
>> >> > On Mon, 2014-11-03 at 10:11 +0000, Frediano Ziglio wrote:
>> >> >> Add this new platform to Xen.
>> >> >> This platform require specific code to initialize CPUs.
>> >> >
>> >> > What is the "bootwrapper"? Are you running this on real silicon or on an
>> >> > emulator? Can the platform be made to do PSCI instead?
>> >> >
>> >>
>> >> Very real. It's actually on my desk and I'm not in Matrix :-)
>> >
>> > OK. The choice of bootwrapper as a name is a bit unfortunate, since it
>> > is already used for something else, but oh well.
>> >
>> >> Has no PSCI support. Would be honestly very great. As we (as company)
>> >> write the firmware could be technically doable. There is no plan. This
>> >> piece of software is meant to bring the CPU from Secure mode to
>> >> Unsecure Hypervisor mode before calling kernel/hypervisor code and
>> >> provide supervisor calls.
>> >
>> > Sounds a lot like PSCI to me, except non-standard ;-)
>> >
>> >> >> +    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
>> >> >
>> >> > Please add a reference to the DT bindings document for these values.
>> >> >
>> >> > linux/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
>> >> > seems related but doesn't talk about most of these fields.
>> >> >
>> >>
>> >> There are documentation in the Linaro kernel, see
>> >> https://git.linaro.org/kernel/linux-linaro-tracking.git/blob/HEAD:/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt.
>> >> I hope it will be merged soon.
>> >
>> > Thanks, but this doesn't seem to cover many of the properties used by
>> > the code you are adding, e.g. bootwrapper-{size,magic},
>> > relocation-{entry,size} (in fact it suggests they are part of a
>> > boot-method array).
>> >
>> > I get the feeling these might be legacy/deprecated. Perhaps we could get
>> > away without supporting such things?
>> >
>> > Ian.
>> >
>>
>> No, this is the newest code.
>
> So in this:
>     if ( !dt_property_read_u32(np, "bootwrapper-phys",
>                                &hip04_boot.bootwrapper_phys) ) {
> [...read boot-method array...]
>     }
>     else
>     {
> [... read a bunch of separate properties...]
>     }
>
> Both sides of the if are used by non-legacy code? That's,...
> unfortunate.
>
> I note that only the if case, not the else case, is covered by the
> hisilicon.txt bindings...
>
> Ian.
>

Yes, you are right, there are actually two versions of these settings.
Actually there has been a lot of bouncing between the two versions. We
are also having some discussion on Linaro ML about implementing PSCI.

Frediano

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Frediano Ziglio]

2015-01-13 15:48 GMT+00:00 Frediano Ziglio <freddy77@gmail.com>:
> 2015-01-13 15:23 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
>> On Tue, 2015-01-13 at 15:11 +0000, Frediano Ziglio wrote:
>>> 2015-01-13 14:42 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
>>> > On Tue, 2015-01-13 at 14:09 +0000, Frediano Ziglio wrote:
>>> >> 2015-01-13 11:58 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
>>> >> > On Mon, 2014-11-03 at 10:11 +0000, Frediano Ziglio wrote:
>>> >> >> Add this new platform to Xen.
>>> >> >> This platform require specific code to initialize CPUs.
>>> >> >
>>> >> > What is the "bootwrapper"? Are you running this on real silicon or on an
>>> >> > emulator? Can the platform be made to do PSCI instead?
>>> >> >
>>> >>
>>> >> Very real. It's actually on my desk and I'm not in Matrix :-)
>>> >
>>> > OK. The choice of bootwrapper as a name is a bit unfortunate, since it
>>> > is already used for something else, but oh well.
>>> >
>>> >> Has no PSCI support. Would be honestly very great. As we (as company)
>>> >> write the firmware could be technically doable. There is no plan. This
>>> >> piece of software is meant to bring the CPU from Secure mode to
>>> >> Unsecure Hypervisor mode before calling kernel/hypervisor code and
>>> >> provide supervisor calls.
>>> >
>>> > Sounds a lot like PSCI to me, except non-standard ;-)
>>> >
>>> >> >> +    np_fab = dt_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
>>> >> >
>>> >> > Please add a reference to the DT bindings document for these values.
>>> >> >
>>> >> > linux/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
>>> >> > seems related but doesn't talk about most of these fields.
>>> >> >
>>> >>
>>> >> There are documentation in the Linaro kernel, see
>>> >> https://git.linaro.org/kernel/linux-linaro-tracking.git/blob/HEAD:/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt.
>>> >> I hope it will be merged soon.
>>> >
>>> > Thanks, but this doesn't seem to cover many of the properties used by
>>> > the code you are adding, e.g. bootwrapper-{size,magic},
>>> > relocation-{entry,size} (in fact it suggests they are part of a
>>> > boot-method array).
>>> >
>>> > I get the feeling these might be legacy/deprecated. Perhaps we could get
>>> > away without supporting such things?
>>> >
>>> > Ian.
>>> >
>>>
>>> No, this is the newest code.
>>
>> So in this:
>>     if ( !dt_property_read_u32(np, "bootwrapper-phys",
>>                                &hip04_boot.bootwrapper_phys) ) {
>> [...read boot-method array...]
>>     }
>>     else
>>     {
>> [... read a bunch of separate properties...]
>>     }
>>
>> Both sides of the if are used by non-legacy code? That's,...
>> unfortunate.
>>
>> I note that only the if case, not the else case, is covered by the
>> hisilicon.txt bindings...
>>
>> Ian.
>>
>
> Yes, you are right, there are actually two versions of these settings.
> Actually there has been a lot of bouncing between the two versions. We
> are also having some discussion on Linaro ML about implementing PSCI.
>

Hi,
   I finally finish to do a PSCI implementation in the boot wrapper
for this platform. You can find it at
https://github.com/freddy77/bootwrapper/tree/psci. It works fine with
Xen which is able to setup CPUs and reboot correctly without using
platform code.

I actually wondering why the branch at
http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/hisilicon-d01-v3
was not merged into the master branch after the release. Actually
using the above boot wrapper I was able to remove two patches from the
patchset. See https://github.com/freddy77/xen/commits/frediano/use_psci.

Why this patch was not integrated?
http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commitdiff;h=612816078b81e3b244d61843f0c27830bbf3da69.
It was not related specifically to board and already discussed.

Frediano

Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

[Ian Campbell]

On Thu, 2015-02-19 at 14:23 +0000, Frediano Ziglio wrote:

> I actually wondering why the branch at
> http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/hisilicon-d01-v3
> was not merged into the master branch after the release.

Because AFAIK it hasn't been resubmitted as a patch series or pinged
since the release.

It seems like it needs reworking to drop the old SMP code in favour of
PSCI anyway.

Ian.

[PATCH 02/10] xen/arm: Implement hip04-d01 board reboot

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/platforms/hip04.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
index bf38c23..62d2034 100644
--- a/xen/arch/arm/platforms/hip04.c
+++ b/xen/arch/arm/platforms/hip04.c
@@ -27,6 +27,7 @@
 #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))
@@ -53,13 +54,21 @@ struct hip04_secondary_cpu_data {
     u32 relocation_size;
 };
 
-static void __iomem *relocation, *sysctrl, *fabric;
+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)
 {
-    /* TODO */
+    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)
@@ -186,6 +195,11 @@ static int __init hip04_smp_init(void)
     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;
-- 
1.9.1

Re: [PATCH 02/10] xen/arm: Implement hip04-d01 board reboot

[Julien Grall]

Hi Frediano,

I would fold this patch in #1.

Regards,

On 11/03/2014 10:11 AM, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> ---
>  xen/arch/arm/platforms/hip04.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
> index bf38c23..62d2034 100644
> --- a/xen/arch/arm/platforms/hip04.c
> +++ b/xen/arch/arm/platforms/hip04.c
> @@ -27,6 +27,7 @@
>  #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))
> @@ -53,13 +54,21 @@ struct hip04_secondary_cpu_data {
>      u32 relocation_size;
>  };
>  
> -static void __iomem *relocation, *sysctrl, *fabric;
> +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)
>  {
> -    /* TODO */
> +    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)
> @@ -186,6 +195,11 @@ static int __init hip04_smp_init(void)
>      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;
> 


-- 
Julien Grall

[PATCH 03/10] xen/arm: Define quirk for Hip04 GICv2 divergence

[Frediano Ziglio]

From: Zoltan Kiss <zoltan.kiss@huawei.com>

Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/platforms/hip04.c | 6 ++++++
 xen/include/asm-arm/platform.h | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
index 62d2034..024c8a0 100644
--- a/xen/arch/arm/platforms/hip04.c
+++ b/xen/arch/arm/platforms/hip04.c
@@ -253,12 +253,18 @@ static const struct dt_device_match hip04_blacklist_dev[] __initconst =
     { /* sentinel */ },
 };
 
+static uint32_t hip04_quirks(void)
+{
+    return PLATFORM_QUIRK_GICV2_16_CPU;
+}
+
 
 PLATFORM_START(hip04, "HISILICON HIP04")
     .compatible = hip04_dt_compat,
     .smp_init = hip04_smp_init,
     .cpu_up = hip04_cpu_up,
     .reset = hip04_reset,
+    .quirks = hip04_quirks,
     .blacklist_dev = hip04_blacklist_dev,
 PLATFORM_END
 
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index eefaca6..537fba5 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -60,6 +60,11 @@ struct platform_desc {
  */
 #define PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI       (1 << 1)
 
+/*
+ * Quirk for platforms where GICv2 has to handle 16 CPUs
+ */
+#define PLATFORM_QUIRK_GICV2_16_CPU       (1 << 2)
+
 void __init platform_init(void);
 int __init platform_init_time(void);
 int __init platform_specific_mapping(struct domain *d);
-- 
1.9.1

Re: [PATCH 03/10] xen/arm: Define quirk for Hip04 GICv2 divergence

[Julien Grall]

Hi Frediano,

This could be fold in #4.

On 11/03/2014 10:11 AM, Frediano Ziglio wrote:
> From: Zoltan Kiss <zoltan.kiss@huawei.com>
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/platforms/hip04.c | 6 ++++++
>  xen/include/asm-arm/platform.h | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
> index 62d2034..024c8a0 100644
> --- a/xen/arch/arm/platforms/hip04.c
> +++ b/xen/arch/arm/platforms/hip04.c
> @@ -253,12 +253,18 @@ static const struct dt_device_match hip04_blacklist_dev[] __initconst =
>      { /* sentinel */ },
>  };
>  
> +static uint32_t hip04_quirks(void)
> +{
> +    return PLATFORM_QUIRK_GICV2_16_CPU;
> +}
> +
>  
>  PLATFORM_START(hip04, "HISILICON HIP04")
>      .compatible = hip04_dt_compat,
>      .smp_init = hip04_smp_init,
>      .cpu_up = hip04_cpu_up,
>      .reset = hip04_reset,
> +    .quirks = hip04_quirks,
>      .blacklist_dev = hip04_blacklist_dev,
>  PLATFORM_END
>  
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index eefaca6..537fba5 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -60,6 +60,11 @@ struct platform_desc {
>   */
>  #define PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI       (1 << 1)
>  
> +/*
> + * Quirk for platforms where GICv2 has to handle 16 CPUs
> + */
> +#define PLATFORM_QUIRK_GICV2_16_CPU       (1 << 2)
> +

Actually you use the quirk to do hisilicon specific (mostly in patch
#6). I would rename the quirk to show it's platform specific, something
like:

PLATFORM_QUIRK_HISILICON_GICV2 or PLATFROM_QUIRK_HISILICON_GICV2_16_CPU.

Regards,

-- 
Julien Grall

Re: [PATCH 03/10] xen/arm: Define quirk for Hip04 GICv2 divergence

[Ian Campbell]

On Mon, 2014-11-03 at 13:50 +0000, Julien Grall wrote:
> Actually you use the quirk to do hisilicon specific (mostly in patch
> #6). I would rename the quirk to show it's platform specific, something
> like:
> 
> PLATFORM_QUIRK_HISILICON_GICV2 or PLATFROM_QUIRK_HISILICON_GICV2_16_CPU.

I think this should be keyed of the DT compatible string, since there is
one, it's not really a quirk, it's essentially implementing a different
hardware spec.

Ian.

[PATCH 04/10] 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     | 70 ++++++++++++++++++++++++++++++++++++++---------
 xen/arch/arm/gic.c        |  5 +++-
 xen/include/asm-arm/gic.h |  4 ++-
 3 files changed, 64 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index faad1ff..eef55ed 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -79,16 +79,22 @@ 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 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 +138,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 +209,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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
+        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 +245,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 +259,22 @@ 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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
+    {
+        gic_cpus = 16;
+        BUG_ON( gicv2_info.nr_lines > 510 );
+    }
+    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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
+    {
+        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
@@ -348,6 +380,11 @@ static int gicv2_secondary_cpu_init(void)
     return 0;
 }
 
+static inline unsigned gicd_sgi_target_shift(void)
+{
+    return 8 + 16 - nr_gic_cpu_if;
+}
+
 static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
                            const cpumask_t *cpu_mask)
 {
@@ -366,7 +403,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 +618,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);
 }
@@ -690,6 +727,12 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
 
     dt_device_set_used_by(node, DOMID_XEN);
 
+    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
+    {
+        nr_gic_cpu_if = 16;
+        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");
@@ -769,6 +812,7 @@ static const char * const gicv2_dt_compat[] __initconst =
     DT_COMPAT_GIC_CORTEX_A15,
     DT_COMPAT_GIC_CORTEX_A7,
     DT_COMPAT_GIC_400,
+    DT_COMPAT_GIC_HIP04,
     NULL
 };
 
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 70d10d6..cd934cf 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -563,12 +563,15 @@ 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 = platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) ?
+                           510 :
+                           1021;
 
     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 04/10] xen/arm: Make gic-v2 code handle hip04-d01 platform

[Julien Grall]

Hi Frediano,

On 11/03/2014 10:11 AM, Frediano Ziglio wrote:
>  /*
>   * needs to be called with a valid cpu_mask, ie each cpu in the mask has
>   * already called gic_cpu_init
> @@ -230,7 +245,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 +259,22 @@ 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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        gic_cpus = 16;
> +        BUG_ON( gicv2_info.nr_lines > 510 );

Could you add a define for this value and add a comment. It would avoid
to scratch his head trying to understand why 510.

[..]

> +static inline unsigned gicd_sgi_target_shift(void)
> +{
> +    return 8 + 16 - nr_gic_cpu_if;
> +}
> +
>  static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>                             const cpumask_t *cpu_mask)
>  {
> @@ -366,7 +403,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,

gicv2_send_SGI is heavily used, it's not acceptable to compute the value
of the shift every time. Hence this always give you the same value. You
should define a static and initialize it during the initialization.

> @@ -690,6 +727,12 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>  
>      dt_device_set_used_by(node, DOMID_XEN);
>  
> +    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        nr_gic_cpu_if = 16;
> +        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");
> @@ -769,6 +812,7 @@ static const char * const gicv2_dt_compat[] __initconst =
>      DT_COMPAT_GIC_CORTEX_A15,
>      DT_COMPAT_GIC_CORTEX_A7,
>      DT_COMPAT_GIC_400,
> +    DT_COMPAT_GIC_HIP04,
>      NULL
>  };
>  
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 70d10d6..cd934cf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -563,12 +563,15 @@ 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 = platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) ?
> +                           510 :

Ditto for the 510.

> +                           1021;

gic_interrupt is called thousand time per second, anything that never
changes should define only once during initialization.

Rather than using hardcoding the max number, I would use
gicv2_info.nr_lines which contains the maximum number of IRQ used by
this GIC. Any value above this value is already an error.

That would require either a separate patch or adding a comment in the
commit message.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2014-11-03 at 10:11 +0000, 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.

I'm not super keen on sprinkling all of these
platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) and the related
de-constification throughout this driver. In reality your interrupt
controller might be "gic-like" but it is not actually a gic v2, it's
something custom.

I see that the Linux side has gone the route of making this an entirely
separate driver. Since we have the gic_hw_operations abstraction these
days I am inclined towards suggesting we go a similar route in Xen (so
far the abstraction has supported gic v2 and v3, so you might find that
you want to refactor slight, e.g. by hoisting or pushing down some
functionality). The same goes for the GICH changes later on.

I'm even less keen on the related changes made to the vgic. My
overwhelming preference is for guests to only ever see the
architecturally standardised gic v2, v3 (and in the future v4 etc). This
would mean that on your platform a single guest could only make use of 8
CPUs, but as a whole the host would be able to use all 16. I think
that's an acceptable tradeoff.

(Hrm, looking a second time I don't really understand the patches you
are doing to the vgic and to what extent they contradict or support what
I say above, just moving the registers with no other changes seems odd
to me. In any case please just use the standard vgic)

Ian.

> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/gic-v2.c     | 70 ++++++++++++++++++++++++++++++++++++++---------
>  xen/arch/arm/gic.c        |  5 +++-
>  xen/include/asm-arm/gic.h |  4 ++-
>  3 files changed, 64 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index faad1ff..eef55ed 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -79,16 +79,22 @@ 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 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 +138,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 +209,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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +        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 +245,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 +259,22 @@ 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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        gic_cpus = 16;
> +        BUG_ON( gicv2_info.nr_lines > 510 );
> +    }
> +    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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        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
> @@ -348,6 +380,11 @@ static int gicv2_secondary_cpu_init(void)
>      return 0;
>  }
>  
> +static inline unsigned gicd_sgi_target_shift(void)
> +{
> +    return 8 + 16 - nr_gic_cpu_if;
> +}
> +
>  static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>                             const cpumask_t *cpu_mask)
>  {
> @@ -366,7 +403,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 +618,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);
>  }
> @@ -690,6 +727,12 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>  
>      dt_device_set_used_by(node, DOMID_XEN);
>  
> +    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        nr_gic_cpu_if = 16;
> +        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");
> @@ -769,6 +812,7 @@ static const char * const gicv2_dt_compat[] __initconst =
>      DT_COMPAT_GIC_CORTEX_A15,
>      DT_COMPAT_GIC_CORTEX_A7,
>      DT_COMPAT_GIC_400,
> +    DT_COMPAT_GIC_HIP04,
>      NULL
>  };
>  
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 70d10d6..cd934cf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -563,12 +563,15 @@ 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 = platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) ?
> +                           510 :
> +                           1021;
>  
>      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"
>  

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

[Frediano Ziglio]

2015-01-13 11:54 GMT+00:00 Ian Campbell <Ian.Campbell@citrix.com>:
> On Mon, 2014-11-03 at 10:11 +0000, 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.
>
> I'm not super keen on sprinkling all of these
> platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) and the related
> de-constification throughout this driver. In reality your interrupt
> controller might be "gic-like" but it is not actually a gic v2, it's
> something custom.
>

The quirk part was removed, Julien imported v3 in
http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/hisilicon-d01-v3

> I see that the Linux side has gone the route of making this an entirely
> separate driver. Since we have the gic_hw_operations abstraction these
> days I am inclined towards suggesting we go a similar route in Xen (so
> far the abstraction has supported gic v2 and v3, so you might find that
> you want to refactor slight, e.g. by hoisting or pushing down some
> functionality). The same goes for the GICH changes later on.
>

Do you think would be better to have another GIC driver instead. It
would be mostly a copy&paste job. Yes, I didn't understand the reason
behind changing GICH registers either but now the changes are in the
silicon.

> I'm even less keen on the related changes made to the vgic. My
> overwhelming preference is for guests to only ever see the
> architecturally standardised gic v2, v3 (and in the future v4 etc). This
> would mean that on your platform a single guest could only make use of 8
> CPUs, but as a whole the host would be able to use all 16. I think
> that's an acceptable tradeoff.
>
> (Hrm, looking a second time I don't really understand the patches you
> are doing to the vgic and to what extent they contradict or support what
> I say above, just moving the registers with no other changes seems odd
> to me. In any case please just use the standard vgic)
>
> Ian.
>

Yes, there is no change in vgic. The GICV is 100% compatible to
standard GICv2 and a normal GICv2 is presented to the guest.

Frediano

>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
>> ---
>>  xen/arch/arm/gic-v2.c     | 70 ++++++++++++++++++++++++++++++++++++++---------
>>  xen/arch/arm/gic.c        |  5 +++-
>>  xen/include/asm-arm/gic.h |  4 ++-
>>  3 files changed, 64 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index faad1ff..eef55ed 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -79,16 +79,22 @@ 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 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 +138,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 +209,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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
>> +        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 +245,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 +259,22 @@ 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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
>> +    {
>> +        gic_cpus = 16;
>> +        BUG_ON( gicv2_info.nr_lines > 510 );
>> +    }
>> +    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 ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
>> +    {
>> +        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
>> @@ -348,6 +380,11 @@ static int gicv2_secondary_cpu_init(void)
>>      return 0;
>>  }
>>
>> +static inline unsigned gicd_sgi_target_shift(void)
>> +{
>> +    return 8 + 16 - nr_gic_cpu_if;
>> +}
>> +
>>  static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode,
>>                             const cpumask_t *cpu_mask)
>>  {
>> @@ -366,7 +403,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 +618,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);
>>  }
>> @@ -690,6 +727,12 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>>
>>      dt_device_set_used_by(node, DOMID_XEN);
>>
>> +    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
>> +    {
>> +        nr_gic_cpu_if = 16;
>> +        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");
>> @@ -769,6 +812,7 @@ static const char * const gicv2_dt_compat[] __initconst =
>>      DT_COMPAT_GIC_CORTEX_A15,
>>      DT_COMPAT_GIC_CORTEX_A7,
>>      DT_COMPAT_GIC_400,
>> +    DT_COMPAT_GIC_HIP04,
>>      NULL
>>  };
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 70d10d6..cd934cf 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -563,12 +563,15 @@ 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 = platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) ?
>> +                           510 :
>> +                           1021;
>>
>>      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"
>>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH 05/10] xen/arm: Add support for DTBs with strange names of Hip04 GICv2

[Frediano Ziglio]

From: Zoltan Kiss <zoltan.kiss@huawei.com>

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 eef55ed..85c7f11 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -813,6 +813,7 @@ static const char * const gicv2_dt_compat[] __initconst =
     DT_COMPAT_GIC_CORTEX_A7,
     DT_COMPAT_GIC_400,
     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 06/10] 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     | 24 ++++++++++++++----------
 xen/include/asm-arm/gic.h |  2 ++
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 85c7f11..e7bf331 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -84,6 +84,8 @@ static DEFINE_PER_CPU(u16, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 static unsigned int nr_gic_cpu_if = 8;
 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)
 {
@@ -154,9 +156,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);
@@ -167,9 +169,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);
 }
@@ -182,7 +184,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
     {
@@ -442,12 +444,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)
@@ -497,7 +499,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;
@@ -520,7 +522,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)
@@ -543,7 +545,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)
@@ -731,6 +733,8 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
     {
         nr_gic_cpu_if = 16;
         gic_cpu_mask = 0xffff;
+        gich_apr = HIP04_GICH_APR;
+        gich_lr = HIP04_GICH_LR;
     }
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 3d2b3db..804bf24 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -88,6 +88,8 @@
 #define GICH_ELSR1      (0x34)
 #define GICH_APR        (0xF0)
 #define GICH_LR         (0x100)
+#define HIP04_GICH_APR  (0x70)
+#define HIP04_GICH_LR   (0x80)
 
 /* Register bits */
 #define GICD_CTL_ENABLE 0x1
-- 
1.9.1

Re: [PATCH 06/10] xen/arm: handle GICH register changes for hip04-d01 platform

[Julien Grall]

Hi Frediano,

On 11/03/2014 10:11 AM, Frediano Ziglio wrote:
>  static void gicv2_irq_enable(struct irq_desc *desc)
> @@ -731,6 +733,8 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>      {
>          nr_gic_cpu_if = 16;
>          gic_cpu_mask = 0xffff;
> +        gich_apr = HIP04_GICH_APR;
> +        gich_lr = HIP04_GICH_LR;
>      }
>  
>      res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 3d2b3db..804bf24 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -88,6 +88,8 @@
>  #define GICH_ELSR1      (0x34)
>  #define GICH_APR        (0xF0)
>  #define GICH_LR         (0x100)
> +#define HIP04_GICH_APR  (0x70)
> +#define HIP04_GICH_LR   (0x80)

Please move thoses 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.

Regards,

-- 
Julien Grall

[PATCH 07/10] 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 e7bf331..d92e2c0 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -641,6 +641,12 @@ static int gicv2_make_dt_node(const struct domain *d,
         return -FDT_ERR_XEN(ENOENT);
     }
 
+    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
+    {
+        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 07/10] 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.

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 10:11 AM, 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 e7bf331..d92e2c0 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -641,6 +641,12 @@ static int gicv2_make_dt_node(const struct domain *d,
>          return -FDT_ERR_XEN(ENOENT);
>      }
>  
> +    if ( platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU) )
> +    {
> +        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 07/10] xen/arm: Force domains to use normal GICv2 driver on Hip04 platform

[Ian Campbell]

On Mon, 2014-11-03 at 14:16 +0000, Julien Grall wrote:
> Hi Frediano,
> 
> FYI, this is only force DOM0 to use the normal GICv2 drivers.
> 
> Do you have any plan to support hi-silicon vGIC in Xen? This would allow
> guest running with more than 8 cores.

I'm against exposing any non-standardised vGIC to guests. So long as the
host can use all processors I think it is OK to constrain the guests in
this way. I don't want to end up having loads of custom vGIC drivers for
every time a silicon vendor deviates from the architecturally specified
components.

[PATCH 08/10] 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 d92e2c0..330157b 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -671,8 +671,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 (platform_has_quirk(PLATFORM_QUIRK_GICV2_16_CPU))
+    {
+        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 804bf24..c748791 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -149,6 +149,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

[PATCH 09/10] 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      | 14 ++++++++++----
 xen/include/xen/device_tree.h | 11 +++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index f72b2e9..e97c28b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -160,19 +160,25 @@ 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;
 }
 
-
 bool_t dt_property_read_u64(const struct dt_device_node *np,
                          const char *name, u64 *out_value)
 {
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 08db8bc..5fcd9c4 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: lenght 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 09/10] xen/device_tree: Add new helper to read arrays from a DTB

[Julien Grall]

Hi Frediano,

On 11/03/2014 10:12 AM, Frediano Ziglio wrote:
> From: Zoltan Kiss <zoltan.kiss@linaro.org>
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

> ---
>  xen/common/device_tree.c      | 14 ++++++++++----
>  xen/include/xen/device_tree.h | 11 +++++++++++
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f72b2e9..e97c28b 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -160,19 +160,25 @@ 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;
>  }
>  
> -
>  bool_t dt_property_read_u64(const struct dt_device_node *np,
>                           const char *name, u64 *out_value)
>  {
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 08db8bc..5fcd9c4 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: lenght 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 09/10] xen/device_tree: Add new helper to read arrays from a DTB

[Julien Grall]

Sorry I forgot the 2 NITs (see below)

On 11/03/2014 10:12 AM, Frediano Ziglio wrote:
> From: Zoltan Kiss <zoltan.kiss@linaro.org>
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/common/device_tree.c      | 14 ++++++++++----
>  xen/include/xen/device_tree.h | 11 +++++++++++
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f72b2e9..e97c28b 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -160,19 +160,25 @@ 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;
>  }
>  
> -

Spurious change.

>  bool_t dt_property_read_u64(const struct dt_device_node *np,
>                           const char *name, u64 *out_value)
>  {
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 08db8bc..5fcd9c4 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: lenght of the array

s/lenght/length/

Regards,


-- 
Julien Grall

[PATCH 10/10] xen/arm: Handle different bootwrapper locations for Hip04 platform

[Frediano Ziglio]

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

Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
---
 xen/arch/arm/platforms/hip04.c | 63 ++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
index 024c8a0..dec4984 100644
--- a/xen/arch/arm/platforms/hip04.c
+++ b/xen/arch/arm/platforms/hip04.c
@@ -136,7 +136,7 @@ static void hip04_cluster_up(unsigned int cluster)
 
 static int __init hip04_smp_init(void)
 {
-    struct dt_device_node *np, *np_fab;
+    struct dt_device_node *np, *np_fab, *bw;
     const char *msg;
     u64 addr, size;
 
@@ -150,30 +150,45 @@ static int __init hip04_smp_init(void)
     if ( !np_fab )
         goto err;
 
-    msg = "failed to get bootwrapper-phys\n";
     if ( !dt_property_read_u32(np, "bootwrapper-phys",
-                               &hip04_boot.bootwrapper_phys) )
-        goto err;
-
-    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;
+                               &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);
-- 
1.9.1

Re: [PATCH 10/10] xen/arm: Handle different bootwrapper locations for Hip04 platform

[Julien Grall]

Hi Frediano,

This could be merged in #1.

Regards,

On 11/03/2014 10:12 AM, Frediano Ziglio wrote:
> From: Zoltan Kiss <zoltan.kiss@linaro.org>
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>  xen/arch/arm/platforms/hip04.c | 63 ++++++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
> index 024c8a0..dec4984 100644
> --- a/xen/arch/arm/platforms/hip04.c
> +++ b/xen/arch/arm/platforms/hip04.c
> @@ -136,7 +136,7 @@ static void hip04_cluster_up(unsigned int cluster)
>  
>  static int __init hip04_smp_init(void)
>  {
> -    struct dt_device_node *np, *np_fab;
> +    struct dt_device_node *np, *np_fab, *bw;
>      const char *msg;
>      u64 addr, size;
>  
> @@ -150,30 +150,45 @@ static int __init hip04_smp_init(void)
>      if ( !np_fab )
>          goto err;
>  
> -    msg = "failed to get bootwrapper-phys\n";
>      if ( !dt_property_read_u32(np, "bootwrapper-phys",
> -                               &hip04_boot.bootwrapper_phys) )
> -        goto err;
> -
> -    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;
> +                               &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);
> 


-- 
Julien Grall

Re: [PATCH 10/10] xen/arm: Handle different bootwrapper locations for Hip04 platform

[Frediano Ziglio]

> 
> Hi Frediano,
> 
> This could be merged in #1.
> 
> Regards,
> 

No, as it require the patch just before it so merging to #1 cause a commit that does not compile

Frediano

> On 11/03/2014 10:12 AM, Frediano Ziglio wrote:
> > From: Zoltan Kiss <zoltan.kiss@linaro.org>
> >
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> > ---
> >  xen/arch/arm/platforms/hip04.c | 63
> > ++++++++++++++++++++++++++----------------
> >  1 file changed, 39 insertions(+), 24 deletions(-)
> >
> > diff --git a/xen/arch/arm/platforms/hip04.c
> > b/xen/arch/arm/platforms/hip04.c index 024c8a0..dec4984 100644
> > --- a/xen/arch/arm/platforms/hip04.c
> > +++ b/xen/arch/arm/platforms/hip04.c
> > @@ -136,7 +136,7 @@ static void hip04_cluster_up(unsigned int cluster)
> >
> >  static int __init hip04_smp_init(void)  {
> > -    struct dt_device_node *np, *np_fab;
> > +    struct dt_device_node *np, *np_fab, *bw;
> >      const char *msg;
> >      u64 addr, size;
> >
> > @@ -150,30 +150,45 @@ static int __init hip04_smp_init(void)
> >      if ( !np_fab )
> >          goto err;
> >
> > -    msg = "failed to get bootwrapper-phys\n";
> >      if ( !dt_property_read_u32(np, "bootwrapper-phys",
> > -                               &hip04_boot.bootwrapper_phys) )
> > -        goto err;
> > -
> > -    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;
> > +                               &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);
> >
> 
> 
> --
> Julien Grall

Re: [PATCH 10/10] xen/arm: Handle different bootwrapper locations for Hip04 platform

[Julien Grall]

On 11/03/2014 03:28 PM, Frediano Ziglio wrote:
>>
>> Hi Frediano,
>>
>> This could be merged in #1.
>>
>> Regards,
>>
> 
> No, as it require the patch just before it so merging to #1 cause a commit that does not compile

Why don't you move the patch #9 earlier in the patch series too?

Regards,

-- 
Julien Grall

Re: [PATCH 10/10] xen/arm: Handle different bootwrapper locations for Hip04 platform

[Frediano Ziglio]

> On 11/03/2014 03:28 PM, Frediano Ziglio wrote:
> >>
> >> Hi Frediano,
> >>
> >> This could be merged in #1.
> >>
> >> Regards,
> >>
> >
> > No, as it require the patch just before it so merging to #1 cause a
> commit that does not compile
> 
> Why don't you move the patch #9 earlier in the patch series too?
> 
> Regards,
> 

So move patch #9 and #10 in another position? Looks ok, which position?

I did most of changes you requested. I was thinking to post another version with this last change.

The change suggested by Ian (do not use a quirk) is still in my TODO list as could be quite big.

I don't know which constant I should use for the 510 number.

Frediano

Re: [PATCH 10/10] xen/arm: Handle different bootwrapper locations for Hip04 platform

[Julien Grall]

On 11/03/2014 03:36 PM, Frediano Ziglio wrote:
>> On 11/03/2014 03:28 PM, Frediano Ziglio wrote:
>>>>
>>>> Hi Frediano,
>>>>
>>>> This could be merged in #1.
>>>>
>>>> Regards,
>>>>
>>>
>>> No, as it require the patch just before it so merging to #1 cause a
>> commit that does not compile
>>
>> Why don't you move the patch #9 earlier in the patch series too?
>>
>> Regards,
>>
> 
> So move patch #9 and #10 in another position? Looks ok, which position?

I would move #9 to the top of the list and merge #10 in the current #1.

> I did most of changes you requested. I was thinking to post another version with this last change.
> 
> The change suggested by Ian (do not use a quirk) is still in my TODO list as could be quite big.
> 
> I don't know which constant I should use for the 510 number.

The first one is not useful as it will catch buggy hardware, the second
could be replace by nr_lines.

So you could even drop the 2 510.

-- 
Julien Grall