[PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Julien Grall]

The GIC hip04 driver was differring from GICv2. I suspect that some of
the changes in the common GIC code make boot fail on hip04. Although, I
don't have a platform to check so it has been only build tested.

List of GICv2 commit ported to the HIP04:
    commit ce12e6dba4b2d120e35dffd95a745452224e7144
    Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
    Date:   Fri Apr 10 16:21:10 2015 +1000

        xen/arm: Don't write to GICH_MISR

        GICH_MISR is read-only in GICv2.

        Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
        Reviewed-by: Julien Grall <julien.grall@citrix.com>
        Acked-by: Ian Campbell <ian.campbell@citrix.com>

    commit 2eb4f996547dc632aa94b2b7b4f783bec8ffe457
    Author: Julien Grall <julien.grall@linaro.org>
    Date:   Wed Apr 1 17:21:47 2015 +0100

        xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts

        GICD_TYPER.ITLinesNumber can encode up to 1024 interrupts. Although,
        IRQ 1020-1023 are reserved for special purpose.

        The result is used by the callers of gic_number_lines in order to check
        the validity of an IRQ.

        Signed-off-by: Julien Grall <julien.grall@linaro.org>
        Acked-by: Ian Campbell <ian.campbell@citrix.com>
        Cc: Frediano Ziglio <frediano.ziglio@huawei.com>
        Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

    commit e2d486b385ce58b6db7561417de28ba837dcd4ac
    Author: Julien Grall <julien.grall@linaro.org>
    Date:   Wed Apr 1 17:21:34 2015 +0100

        xen/arm: Divide GIC initialization in 2 parts

        Currently the function to translate IRQ from the device tree is set
        unconditionally  to be able to be able to retrieve serial/timer IRQ
        before the GIC has been initialized.

        It assumes that the xlate function won't ever changed. We may also need
        to have the primary interrupt controller very early.

        Rework the gic initialization in 2 parts:
            - gic_preinit: Get the interrupt controller device tree node and
            set up GIC and xlate callbacks
            - gic_init: Initialize the interrupt controller and the boot CPU
            interrupts.

        The former function will be called just after the IRQ subsystem as been
        initialized.

        Signed-off-by: Julien Grall <julien.grall@linaro.org>
        Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
        Acked-by: Ian Campbell <ian.campbell@citrix.com>
        Cc: Frediano Ziglio <frediano.ziglio@huawei.com>
        Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
---

So far I've got no news about previous changes in the GIC [1] applied in
April... Since then the driver is likely broken and would prevent Xen to
boot on a supported platform. This patch is trying to fix it even though
I don't have a board to test.

It's a prerequisite for an upcoming series which update/rework the
vGICv2 drivers and will impact the GICv2 drivers (~10 patches).

I'm concerned to see a newly driver (pushed last march) already orphan.
Does Huawei still plan to maintain this driver?

[1] http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02465.html
---
 xen/arch/arm/gic-hip04.c | 89 ++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 073ad33..efdccdb 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -267,6 +267,7 @@ static void __init hip04gic_dist_init(void)
     uint32_t type;
     uint32_t cpumask;
     uint32_t gic_cpus;
+    unsigned int nr_lines;
     int i;
 
     cpumask = readl_gicd(GICD_ITARGETSR) & 0xffff;
@@ -276,31 +277,34 @@ static void __init hip04gic_dist_init(void)
     writel_gicd(0, GICD_CTLR);
 
     type = readl_gicd(GICD_TYPER);
-    gicv2_info.nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
+    nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
     gic_cpus = 16;
     printk("GIC-HIP04: %d lines, %d cpu%s%s (IID %8.8x).\n",
-           gicv2_info.nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
+           nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
            (type & GICD_TYPE_SEC) ? ", secure" : "",
            readl_gicd(GICD_IIDR));
 
     /* Default all global IRQs to level, active low */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 16 )
+    for ( i = 32; i < nr_lines; i += 16 )
         writel_gicd(0x0, GICD_ICFGR + (i / 16) * 4);
 
     /* Route all global IRQs to this CPU */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 2 )
+    for ( i = 32; i < nr_lines; i += 2 )
         writel_gicd(cpumask, GICD_ITARGETSR + (i / 2) * 4);
 
     /* Default priority for global interrupts */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
+    for ( i = 32; i < nr_lines; i += 4 )
         writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
                     GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
                     GICD_IPRIORITYR + (i / 4) * 4);
 
     /* Disable all global interrupts */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 32 )
+    for ( i = 32; i < nr_lines; i += 32 )
         writel_gicd(~0x0, GICD_ICENABLER + (i / 32) * 4);
 
+    /* Only 1020 interrupts are supported */
+    gicv2_info.nr_lines = min(1020U, nr_lines);
+
     /* Turn on the distributor */
     writel_gicd(GICD_CTL_ENABLE, GICD_CTLR);
 }
@@ -351,8 +355,6 @@ static void __cpuinit hip04gic_hyp_init(void)
     vtr = readl_gich(GICH_VTR);
     nr_lrs  = (vtr & GICH_V2_VTR_NRLRGS) + 1;
     gicv2_info.nr_lrs = nr_lrs;
-
-    writel_gich(GICH_MISR_EOI, GICH_MISR);
 }
 
 static void __cpuinit hip04gic_hyp_disable(void)
@@ -688,37 +690,10 @@ static hw_irq_controller hip04gic_guest_irq_type = {
     .set_affinity = hip04gic_irq_set_affinity,
 };
 
-const static struct gic_hw_operations hip04gic_ops = {
-    .info                = &gicv2_info,
-    .secondary_init      = hip04gic_secondary_cpu_init,
-    .save_state          = hip04gic_save_state,
-    .restore_state       = hip04gic_restore_state,
-    .dump_state          = hip04gic_dump_state,
-    .gicv_setup          = hip04gicv_setup,
-    .gic_host_irq_type   = &hip04gic_host_irq_type,
-    .gic_guest_irq_type  = &hip04gic_guest_irq_type,
-    .eoi_irq             = hip04gic_eoi_irq,
-    .deactivate_irq      = hip04gic_dir_irq,
-    .read_irq            = hip04gic_read_irq,
-    .set_irq_properties  = hip04gic_set_irq_properties,
-    .send_SGI            = hip04gic_send_SGI,
-    .disable_interface   = hip04gic_disable_interface,
-    .update_lr           = hip04gic_update_lr,
-    .update_hcr_status   = hip04gic_hcr_status,
-    .clear_lr            = hip04gic_clear_lr,
-    .read_lr             = hip04gic_read_lr,
-    .write_lr            = hip04gic_write_lr,
-    .read_vmcr_priority  = hip04gic_read_vmcr_priority,
-    .read_apr            = hip04gic_read_apr,
-    .make_dt_node        = hip04gic_make_dt_node,
-};
-
-/* Set up the GIC */
-static int __init hip04gic_init(struct dt_device_node *node, const void *data)
+static int __init hip04gic_init(void)
 {
     int res;
-
-    dt_device_set_used_by(node, DOMID_XEN);
+    const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
     if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
@@ -741,9 +716,6 @@ static int __init hip04gic_init(struct dt_device_node *node, const void *data)
         panic("GIC-HIP04: Cannot find the maintenance IRQ");
     gicv2_info.maintenance_irq = res;
 
-    /* Set the GIC as the primary interrupt controller */
-    dt_interrupt_controller = node;
-
     /* TODO: Add check on distributor, cpu size */
 
     printk("GIC-HIP04 initialization:\n"
@@ -788,8 +760,43 @@ static int __init hip04gic_init(struct dt_device_node *node, const void *data)
 
     spin_unlock(&gicv2.lock);
 
+    return 0;
+}
+
+const static struct gic_hw_operations hip04gic_ops = {
+    .info                = &gicv2_info,
+    .init                = hip04gic_init,
+    .secondary_init      = hip04gic_secondary_cpu_init,
+    .save_state          = hip04gic_save_state,
+    .restore_state       = hip04gic_restore_state,
+    .dump_state          = hip04gic_dump_state,
+    .gicv_setup          = hip04gicv_setup,
+    .gic_host_irq_type   = &hip04gic_host_irq_type,
+    .gic_guest_irq_type  = &hip04gic_guest_irq_type,
+    .eoi_irq             = hip04gic_eoi_irq,
+    .deactivate_irq      = hip04gic_dir_irq,
+    .read_irq            = hip04gic_read_irq,
+    .set_irq_properties  = hip04gic_set_irq_properties,
+    .send_SGI            = hip04gic_send_SGI,
+    .disable_interface   = hip04gic_disable_interface,
+    .update_lr           = hip04gic_update_lr,
+    .update_hcr_status   = hip04gic_hcr_status,
+    .clear_lr            = hip04gic_clear_lr,
+    .read_lr             = hip04gic_read_lr,
+    .write_lr            = hip04gic_write_lr,
+    .read_vmcr_priority  = hip04gic_read_vmcr_priority,
+    .read_apr            = hip04gic_read_apr,
+    .make_dt_node        = hip04gic_make_dt_node,
+};
+
+/* Set up the GIC */
+static int __init hip04gic_preinit(struct dt_device_node *node,
+                                   const void *data)
+{
     gicv2_info.hw_version = GIC_V2;
+    gicv2_info.node = node;
     register_gic_ops(&hip04gic_ops);
+    dt_irq_xlate = gic_irq_xlate;
 
     return 0;
 }
@@ -802,7 +809,7 @@ static const struct dt_device_match hip04gic_dt_match[] __initconst =
 
 DT_DEVICE_START(hip04gic, "GIC-HIP04", DEVICE_GIC)
         .dt_match = hip04gic_dt_match,
-        .init = hip04gic_init,
+        .init = hip04gic_preinit,
 DT_DEVICE_END
 
 /*
-- 
2.1.4

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Zoltan Kiss]

Looks good at first glance, let me try it on a board.

On 06/05/15 19:52, Julien Grall wrote:
> The GIC hip04 driver was differring from GICv2. I suspect that some of
> the changes in the common GIC code make boot fail on hip04. Although, I
> don't have a platform to check so it has been only build tested.
>
> List of GICv2 commit ported to the HIP04:
>      commit ce12e6dba4b2d120e35dffd95a745452224e7144
>      Author: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>      Date:   Fri Apr 10 16:21:10 2015 +1000
>
>          xen/arm: Don't write to GICH_MISR
>
>          GICH_MISR is read-only in GICv2.
>
>          Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>          Reviewed-by: Julien Grall <julien.grall@citrix.com>
>          Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
>      commit 2eb4f996547dc632aa94b2b7b4f783bec8ffe457
>      Author: Julien Grall <julien.grall@linaro.org>
>      Date:   Wed Apr 1 17:21:47 2015 +0100
>
>          xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts
>
>          GICD_TYPER.ITLinesNumber can encode up to 1024 interrupts. Although,
>          IRQ 1020-1023 are reserved for special purpose.
>
>          The result is used by the callers of gic_number_lines in order to check
>          the validity of an IRQ.
>
>          Signed-off-by: Julien Grall <julien.grall@linaro.org>
>          Acked-by: Ian Campbell <ian.campbell@citrix.com>
>          Cc: Frediano Ziglio <frediano.ziglio@huawei.com>
>          Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
>
>      commit e2d486b385ce58b6db7561417de28ba837dcd4ac
>      Author: Julien Grall <julien.grall@linaro.org>
>      Date:   Wed Apr 1 17:21:34 2015 +0100
>
>          xen/arm: Divide GIC initialization in 2 parts
>
>          Currently the function to translate IRQ from the device tree is set
>          unconditionally  to be able to be able to retrieve serial/timer IRQ
>          before the GIC has been initialized.
>
>          It assumes that the xlate function won't ever changed. We may also need
>          to have the primary interrupt controller very early.
>
>          Rework the gic initialization in 2 parts:
>              - gic_preinit: Get the interrupt controller device tree node and
>              set up GIC and xlate callbacks
>              - gic_init: Initialize the interrupt controller and the boot CPU
>              interrupts.
>
>          The former function will be called just after the IRQ subsystem as been
>          initialized.
>
>          Signed-off-by: Julien Grall <julien.grall@linaro.org>
>          Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>          Acked-by: Ian Campbell <ian.campbell@citrix.com>
>          Cc: Frediano Ziglio <frediano.ziglio@huawei.com>
>          Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Cc: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---
>
> So far I've got no news about previous changes in the GIC [1] applied in
> April... Since then the driver is likely broken and would prevent Xen to
> boot on a supported platform. This patch is trying to fix it even though
> I don't have a board to test.
>
> It's a prerequisite for an upcoming series which update/rework the
> vGICv2 drivers and will impact the GICv2 drivers (~10 patches).
>
> I'm concerned to see a newly driver (pushed last march) already orphan.
> Does Huawei still plan to maintain this driver?
>
> [1] http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02465.html
> ---
>   xen/arch/arm/gic-hip04.c | 89 ++++++++++++++++++++++++++----------------------
>   1 file changed, 48 insertions(+), 41 deletions(-)
>
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> index 073ad33..efdccdb 100644
> --- a/xen/arch/arm/gic-hip04.c
> +++ b/xen/arch/arm/gic-hip04.c
> @@ -267,6 +267,7 @@ static void __init hip04gic_dist_init(void)
>       uint32_t type;
>       uint32_t cpumask;
>       uint32_t gic_cpus;
> +    unsigned int nr_lines;
>       int i;
>
>       cpumask = readl_gicd(GICD_ITARGETSR) & 0xffff;
> @@ -276,31 +277,34 @@ static void __init hip04gic_dist_init(void)
>       writel_gicd(0, GICD_CTLR);
>
>       type = readl_gicd(GICD_TYPER);
> -    gicv2_info.nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
> +    nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
>       gic_cpus = 16;
>       printk("GIC-HIP04: %d lines, %d cpu%s%s (IID %8.8x).\n",
> -           gicv2_info.nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
> +           nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
>              (type & GICD_TYPE_SEC) ? ", secure" : "",
>              readl_gicd(GICD_IIDR));
>
>       /* Default all global IRQs to level, active low */
> -    for ( i = 32; i < gicv2_info.nr_lines; i += 16 )
> +    for ( i = 32; i < nr_lines; i += 16 )
>           writel_gicd(0x0, GICD_ICFGR + (i / 16) * 4);
>
>       /* Route all global IRQs to this CPU */
> -    for ( i = 32; i < gicv2_info.nr_lines; i += 2 )
> +    for ( i = 32; i < nr_lines; i += 2 )
>           writel_gicd(cpumask, GICD_ITARGETSR + (i / 2) * 4);
>
>       /* Default priority for global interrupts */
> -    for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
> +    for ( i = 32; i < nr_lines; i += 4 )
>           writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
>                       GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
>                       GICD_IPRIORITYR + (i / 4) * 4);
>
>       /* Disable all global interrupts */
> -    for ( i = 32; i < gicv2_info.nr_lines; i += 32 )
> +    for ( i = 32; i < nr_lines; i += 32 )
>           writel_gicd(~0x0, GICD_ICENABLER + (i / 32) * 4);
>
> +    /* Only 1020 interrupts are supported */
> +    gicv2_info.nr_lines = min(1020U, nr_lines);
> +
>       /* Turn on the distributor */
>       writel_gicd(GICD_CTL_ENABLE, GICD_CTLR);
>   }
> @@ -351,8 +355,6 @@ static void __cpuinit hip04gic_hyp_init(void)
>       vtr = readl_gich(GICH_VTR);
>       nr_lrs  = (vtr & GICH_V2_VTR_NRLRGS) + 1;
>       gicv2_info.nr_lrs = nr_lrs;
> -
> -    writel_gich(GICH_MISR_EOI, GICH_MISR);
>   }
>
>   static void __cpuinit hip04gic_hyp_disable(void)
> @@ -688,37 +690,10 @@ static hw_irq_controller hip04gic_guest_irq_type = {
>       .set_affinity = hip04gic_irq_set_affinity,
>   };
>
> -const static struct gic_hw_operations hip04gic_ops = {
> -    .info                = &gicv2_info,
> -    .secondary_init      = hip04gic_secondary_cpu_init,
> -    .save_state          = hip04gic_save_state,
> -    .restore_state       = hip04gic_restore_state,
> -    .dump_state          = hip04gic_dump_state,
> -    .gicv_setup          = hip04gicv_setup,
> -    .gic_host_irq_type   = &hip04gic_host_irq_type,
> -    .gic_guest_irq_type  = &hip04gic_guest_irq_type,
> -    .eoi_irq             = hip04gic_eoi_irq,
> -    .deactivate_irq      = hip04gic_dir_irq,
> -    .read_irq            = hip04gic_read_irq,
> -    .set_irq_properties  = hip04gic_set_irq_properties,
> -    .send_SGI            = hip04gic_send_SGI,
> -    .disable_interface   = hip04gic_disable_interface,
> -    .update_lr           = hip04gic_update_lr,
> -    .update_hcr_status   = hip04gic_hcr_status,
> -    .clear_lr            = hip04gic_clear_lr,
> -    .read_lr             = hip04gic_read_lr,
> -    .write_lr            = hip04gic_write_lr,
> -    .read_vmcr_priority  = hip04gic_read_vmcr_priority,
> -    .read_apr            = hip04gic_read_apr,
> -    .make_dt_node        = hip04gic_make_dt_node,
> -};
> -
> -/* Set up the GIC */
> -static int __init hip04gic_init(struct dt_device_node *node, const void *data)
> +static int __init hip04gic_init(void)
>   {
>       int res;
> -
> -    dt_device_set_used_by(node, DOMID_XEN);
> +    const struct dt_device_node *node = gicv2_info.node;
>
>       res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
>       if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
> @@ -741,9 +716,6 @@ static int __init hip04gic_init(struct dt_device_node *node, const void *data)
>           panic("GIC-HIP04: Cannot find the maintenance IRQ");
>       gicv2_info.maintenance_irq = res;
>
> -    /* Set the GIC as the primary interrupt controller */
> -    dt_interrupt_controller = node;
> -
>       /* TODO: Add check on distributor, cpu size */
>
>       printk("GIC-HIP04 initialization:\n"
> @@ -788,8 +760,43 @@ static int __init hip04gic_init(struct dt_device_node *node, const void *data)
>
>       spin_unlock(&gicv2.lock);
>
> +    return 0;
> +}
> +
> +const static struct gic_hw_operations hip04gic_ops = {
> +    .info                = &gicv2_info,
> +    .init                = hip04gic_init,
> +    .secondary_init      = hip04gic_secondary_cpu_init,
> +    .save_state          = hip04gic_save_state,
> +    .restore_state       = hip04gic_restore_state,
> +    .dump_state          = hip04gic_dump_state,
> +    .gicv_setup          = hip04gicv_setup,
> +    .gic_host_irq_type   = &hip04gic_host_irq_type,
> +    .gic_guest_irq_type  = &hip04gic_guest_irq_type,
> +    .eoi_irq             = hip04gic_eoi_irq,
> +    .deactivate_irq      = hip04gic_dir_irq,
> +    .read_irq            = hip04gic_read_irq,
> +    .set_irq_properties  = hip04gic_set_irq_properties,
> +    .send_SGI            = hip04gic_send_SGI,
> +    .disable_interface   = hip04gic_disable_interface,
> +    .update_lr           = hip04gic_update_lr,
> +    .update_hcr_status   = hip04gic_hcr_status,
> +    .clear_lr            = hip04gic_clear_lr,
> +    .read_lr             = hip04gic_read_lr,
> +    .write_lr            = hip04gic_write_lr,
> +    .read_vmcr_priority  = hip04gic_read_vmcr_priority,
> +    .read_apr            = hip04gic_read_apr,
> +    .make_dt_node        = hip04gic_make_dt_node,
> +};
> +
> +/* Set up the GIC */
> +static int __init hip04gic_preinit(struct dt_device_node *node,
> +                                   const void *data)
> +{
>       gicv2_info.hw_version = GIC_V2;
> +    gicv2_info.node = node;
>       register_gic_ops(&hip04gic_ops);
> +    dt_irq_xlate = gic_irq_xlate;
>
>       return 0;
>   }
> @@ -802,7 +809,7 @@ static const struct dt_device_match hip04gic_dt_match[] __initconst =
>
>   DT_DEVICE_START(hip04gic, "GIC-HIP04", DEVICE_GIC)
>           .dt_match = hip04gic_dt_match,
> -        .init = hip04gic_init,
> +        .init = hip04gic_preinit,
>   DT_DEVICE_END
>
>   /*
>

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Ian Campbell]

On Thu, 2015-05-07 at 09:52 +0100, Zoltan Kiss wrote:
> Looks good at first glance, let me try it on a board.
> 
> On 06/05/15 19:52, Julien Grall wrote:
[...]
> > I'm concerned to see a newly driver (pushed last march) already orphan.
> > Does Huawei still plan to maintain this driver?

I share Julien's concerns here.

It would be good if those listed in the MAINTAINERS file for this device
would respond reasonably promptly to mails such as [1] and try to keep
on top of things or to find a {replacement /co-}maintainer who can do
so.

> > [1] http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02465.html

Ian.

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Zoltan Kiss]

Hi,

On 07/05/15 10:32, Ian Campbell wrote:
> On Thu, 2015-05-07 at 09:52 +0100, Zoltan Kiss wrote:
>> Looks good at first glance, let me try it on a board.
>>
>> On 06/05/15 19:52, Julien Grall wrote:
> [...]
>>> I'm concerned to see a newly driver (pushed last march) already orphan.
>>> Does Huawei still plan to maintain this driver?
>
> I share Julien's concerns here.
>
> It would be good if those listed in the MAINTAINERS file for this device
> would respond reasonably promptly to mails such as [1] and try to keep
> on top of things or to find a {replacement /co-}maintainer who can do
> so.

As I said, I've missed that thread entirely (not that hard given the 
traffic of the list), but now I've improved my mail filters to make sure 
I don't miss a mail where my Huawei address is on the Cc.
We are also looking to add new co-maintainers, because nowadays I'm 
working on other projects.
I need a few days to test the patch as my Xen test environment is not 
available right now.
>
>>> [1] http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02465.html
>
> Ian.
>

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Julien Grall]

Hi Zoltan,

On 07/05/2015 13:37, Zoltan Kiss wrote:

> On 07/05/15 10:32, Ian Campbell wrote:
>> On Thu, 2015-05-07 at 09:52 +0100, Zoltan Kiss wrote:
>>> Looks good at first glance, let me try it on a board.
>>>
>>> On 06/05/15 19:52, Julien Grall wrote:
>> [...]
>>>> I'm concerned to see a newly driver (pushed last march) already orphan.
>>>> Does Huawei still plan to maintain this driver?
>>
>> I share Julien's concerns here.
>>
>> It would be good if those listed in the MAINTAINERS file for this device
>> would respond reasonably promptly to mails such as [1] and try to keep
>> on top of things or to find a {replacement /co-}maintainer who can do
>> so.
>
> As I said, I've missed that thread entirely (not that hard given the
> traffic of the list), but now I've improved my mail filters to make sure
> I don't miss a mail where my Huawei address is on the Cc.
> We are also looking to add new co-maintainers, because nowadays I'm
> working on other projects.
> I need a few days to test the patch as my Xen test environment is not
> available right now.

Ping? Were you able to test it?

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Zoltan Kiss]

On 15/05/15 22:08, Julien Grall wrote:
> Hi Zoltan,
>
> On 07/05/2015 13:37, Zoltan Kiss wrote:
>
>> On 07/05/15 10:32, Ian Campbell wrote:
>>> On Thu, 2015-05-07 at 09:52 +0100, Zoltan Kiss wrote:
>>>> Looks good at first glance, let me try it on a board.
>>>>
>>>> On 06/05/15 19:52, Julien Grall wrote:
>>> [...]
>>>>> I'm concerned to see a newly driver (pushed last march) already
>>>>> orphan.
>>>>> Does Huawei still plan to maintain this driver?
>>>
>>> I share Julien's concerns here.
>>>
>>> It would be good if those listed in the MAINTAINERS file for this device
>>> would respond reasonably promptly to mails such as [1] and try to keep
>>> on top of things or to find a {replacement /co-}maintainer who can do
>>> so.
>>
>> As I said, I've missed that thread entirely (not that hard given the
>> traffic of the list), but now I've improved my mail filters to make sure
>> I don't miss a mail where my Huawei address is on the Cc.
>> We are also looking to add new co-maintainers, because nowadays I'm
>> working on other projects.
>> I need a few days to test the patch as my Xen test environment is not
>> available right now.
>
> Ping? Were you able to test it?

One of my colleague is doing that, to spread experience. It's going 
slower for him obviously to set up an enviroment, but we are working on 
that.

Zoli

>
> Regards,
>

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Julien Grall]

Hi,

On 18/05/15 14:36, Zoltan Kiss wrote:
> 
> 
> On 15/05/15 22:08, Julien Grall wrote:
>> Hi Zoltan,
>>
>> On 07/05/2015 13:37, Zoltan Kiss wrote:
>>
>>> On 07/05/15 10:32, Ian Campbell wrote:
>>>> On Thu, 2015-05-07 at 09:52 +0100, Zoltan Kiss wrote:
>>>>> Looks good at first glance, let me try it on a board.
>>>>>
>>>>> On 06/05/15 19:52, Julien Grall wrote:
>>>> [...]
>>>>>> I'm concerned to see a newly driver (pushed last march) already
>>>>>> orphan.
>>>>>> Does Huawei still plan to maintain this driver?
>>>>
>>>> I share Julien's concerns here.
>>>>
>>>> It would be good if those listed in the MAINTAINERS file for this
>>>> device
>>>> would respond reasonably promptly to mails such as [1] and try to keep
>>>> on top of things or to find a {replacement /co-}maintainer who can do
>>>> so.
>>>
>>> As I said, I've missed that thread entirely (not that hard given the
>>> traffic of the list), but now I've improved my mail filters to make sure
>>> I don't miss a mail where my Huawei address is on the Cc.
>>> We are also looking to add new co-maintainers, because nowadays I'm
>>> working on other projects.
>>> I need a few days to test the patch as my Xen test environment is not
>>> available right now.
>>
>> Ping? Were you able to test it?
> 
> One of my colleague is doing that, to spread experience. It's going
> slower for him obviously to set up an enviroment, but we are working on
> that.

Ping? Was your colleague able to setup Xen? Aside gic-hip04 is not
booting on Xen, this is also a blocker from few patch series based on
this rebase (GICv2 on GICv3, 128 VCPUs support...).

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Ian Campbell]

On Mon, 2015-06-01 at 12:13 +0100, Julien Grall wrote:
> Hi,
> 
> On 18/05/15 14:36, Zoltan Kiss wrote:
> > 
> > 
> > On 15/05/15 22:08, Julien Grall wrote:
> >> Hi Zoltan,
> >>
> >> On 07/05/2015 13:37, Zoltan Kiss wrote:
> >>
> >>> On 07/05/15 10:32, Ian Campbell wrote:
> >>>> On Thu, 2015-05-07 at 09:52 +0100, Zoltan Kiss wrote:
> >>>>> Looks good at first glance, let me try it on a board.
> >>>>>
> >>>>> On 06/05/15 19:52, Julien Grall wrote:
> >>>> [...]
> >>>>>> I'm concerned to see a newly driver (pushed last march) already
> >>>>>> orphan.
> >>>>>> Does Huawei still plan to maintain this driver?
> >>>>
> >>>> I share Julien's concerns here.
> >>>>
> >>>> It would be good if those listed in the MAINTAINERS file for this
> >>>> device
> >>>> would respond reasonably promptly to mails such as [1] and try to keep
> >>>> on top of things or to find a {replacement /co-}maintainer who can do
> >>>> so.
> >>>
> >>> As I said, I've missed that thread entirely (not that hard given the
> >>> traffic of the list), but now I've improved my mail filters to make sure
> >>> I don't miss a mail where my Huawei address is on the Cc.
> >>> We are also looking to add new co-maintainers, because nowadays I'm
> >>> working on other projects.
> >>> I need a few days to test the patch as my Xen test environment is not
> >>> available right now.
> >>
> >> Ping? Were you able to test it?
> > 
> > One of my colleague is doing that, to spread experience. It's going
> > slower for him obviously to set up an enviroment, but we are working on
> > that.
> 
> Ping? Was your colleague able to setup Xen? Aside gic-hip04 is not
> booting on Xen, this is also a blocker from few patch series based on
> this rebase (GICv2 on GICv3, 128 VCPUs support...).

FYI I won't be blocking anything due to this patch, if I trip over some
other patch not applying/building because of it then I'll either apply
it regardless of whether it has been tested/acked or patch the Makefile
to disable it in the build, or perhaps both.

Ian.

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Zoltan Kiss]

Hi,

Yes, we managed to test it, and it works. Then only thing I've found is 
this bit:

+    /* Only 1020 interrupts are supported */
+    gicv2_info.nr_lines = min(1020U, nr_lines);

This interrupt controller only supports 511, so 1020 should be replaced. 
We had such checking in the code in the early versions, and I looked 
everywhere in the archives to figure out why it was dropped before 
upstreaming, but I couldn't find it.
Other than this bit:

Reviewed-by: Zoltan Kiss <zoltan.kiss@huawei.com>
Tested-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

And sorry for the loong delay!

Regards,

Zoli


On 01/06/15 12:13, Julien Grall wrote:
> Hi,
>
> On 18/05/15 14:36, Zoltan Kiss wrote:
>>
>>
>> On 15/05/15 22:08, Julien Grall wrote:
>>> Hi Zoltan,
>>>
>>> On 07/05/2015 13:37, Zoltan Kiss wrote:
>>>
>>>> On 07/05/15 10:32, Ian Campbell wrote:
>>>>> On Thu, 2015-05-07 at 09:52 +0100, Zoltan Kiss wrote:
>>>>>> Looks good at first glance, let me try it on a board.
>>>>>>
>>>>>> On 06/05/15 19:52, Julien Grall wrote:
>>>>> [...]
>>>>>>> I'm concerned to see a newly driver (pushed last march) already
>>>>>>> orphan.
>>>>>>> Does Huawei still plan to maintain this driver?
>>>>>
>>>>> I share Julien's concerns here.
>>>>>
>>>>> It would be good if those listed in the MAINTAINERS file for this
>>>>> device
>>>>> would respond reasonably promptly to mails such as [1] and try to keep
>>>>> on top of things or to find a {replacement /co-}maintainer who can do
>>>>> so.
>>>>
>>>> As I said, I've missed that thread entirely (not that hard given the
>>>> traffic of the list), but now I've improved my mail filters to make sure
>>>> I don't miss a mail where my Huawei address is on the Cc.
>>>> We are also looking to add new co-maintainers, because nowadays I'm
>>>> working on other projects.
>>>> I need a few days to test the patch as my Xen test environment is not
>>>> available right now.
>>>
>>> Ping? Were you able to test it?
>>
>> One of my colleague is doing that, to spread experience. It's going
>> slower for him obviously to set up an enviroment, but we are working on
>> that.
>
> Ping? Was your colleague able to setup Xen? Aside gic-hip04 is not
> booting on Xen, this is also a blocker from few patch series based on
> this rebase (GICv2 on GICv3, 128 VCPUs support...).
>
> Regards,
>

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Julien Grall]

On 01/06/15 12:25, Zoltan Kiss wrote:
> Hi,
> 
> Yes, we managed to test it, and it works. Then only thing I've found is
> this bit:
> 
> +    /* Only 1020 interrupts are supported */
> +    gicv2_info.nr_lines = min(1020U, nr_lines);
> 
> This interrupt controller only supports 511, so 1020 should be replaced.
> We had such checking in the code in the early versions, and I looked
> everywhere in the archives to figure out why it was dropped before
> upstreaming, but I couldn't find it.

I'm aware of the 512 limit (see comment on the patch [1]). This change
was introduced on GICv2/GICv3 because the nr_lines will always be
aligned to 32, although IRQ 1020-1023 are reserved.

512 is a multiple of 32 and unless you have reserved IRQ below 512
and/or your GIC doesn't expose the right number of IRQ, this doesn't
harm and keep the change limited.

> Other than this bit:
> 
> Reviewed-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> Tested-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

Thanks. Can I get some Acked/Review on the other pending patch series?

http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg00944.html :
6 patches to ack.

Regards,

[1] https://patches.linaro.org/46100/


-- 
Julien Grall

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Zoltan Kiss]

On 01/06/15 13:11, Julien Grall wrote:
> On 01/06/15 12:25, Zoltan Kiss wrote:
>> Hi,
>>
>> Yes, we managed to test it, and it works. Then only thing I've found is
>> this bit:
>>
>> +    /* Only 1020 interrupts are supported */
>> +    gicv2_info.nr_lines = min(1020U, nr_lines);
>>
>> This interrupt controller only supports 511, so 1020 should be replaced.
>> We had such checking in the code in the early versions, and I looked
>> everywhere in the archives to figure out why it was dropped before
>> upstreaming, but I couldn't find it.
>
> I'm aware of the 512 limit (see comment on the patch [1]). This change
> was introduced on GICv2/GICv3 because the nr_lines will always be
> aligned to 32, although IRQ 1020-1023 are reserved.
>
> 512 is a multiple of 32 and unless you have reserved IRQ below 512
> and/or your GIC doesn't expose the right number of IRQ, this doesn't
> harm and keep the change limited.
>
>> Other than this bit:
>>
>> Reviewed-by: Zoltan Kiss <zoltan.kiss@huawei.com>
>> Tested-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>
> Thanks. Can I get some Acked/Review on the other pending patch series?
>
> http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg00944.html :
> 6 patches to ack.

Sure. Btw. do you have them in a public repo somewhere? It would make it 
a little bit easier to apply and test.
>
> Regards,
>
> [1] https://patches.linaro.org/46100/
>
>

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Julien Grall]

On 01/06/15 14:18, Zoltan Kiss wrote:
> Sure. Btw. do you have them in a public repo somewhere? It would make it
> a little bit easier to apply and test.

It's based on the latest staging:

http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-huawei/gicv2-on-gicv3

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Ian Campbell]

On Mon, 2015-06-01 at 12:25 +0100, Zoltan Kiss wrote:
> Reviewed-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> Tested-by: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>

Acked + applied, thanks.

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Shameerali Kolothum Thodi]

Hi Julien/Zoli,

>http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-huawei/gicv2-on-gicv3
I had a run of this staging on our D01 board.

Xen fails to boot with below error:
(XEN) domain 0: vGIC requested is not supported
(XEN) CPU0: Unexpected Trap: Data Abort

After going through the code, it looks to me that hip04 gicv2_info is missing the vgic_versions initialisation. Made the following changes and xen booted and started dom0.

--- ../xen-org/arch/arm/gic-hip04.c   2015-06-01 16:33:40.000000000 +0100
+++ ./xen/arch/arm/gic-hip04.c  2015-06-09 13:21:44.355012016 +0100
@@ -735,6 +735,7 @@ static int __init hip04gic_preinit(struc
                                    const void *data)
 {
     gicv2_info.hw_version = GIC_V2;
+    gicv2_info.vgic_versions = GIC_V2;
     gicv2_info.node = node;
     register_gic_ops(&hip04gic_ops);
     dt_irq_xlate = gic_irq_xlate;

(Sorry, this is a diff not a proper git patch)
Please correct me, if this is not the correct way to init the vgic_versions.

Regards,
Shameer

-----Original Message-----
From: Julien Grall [mailto:julien.grall@citrix.com] 
Sent: 01 June 2015 16:35
To: Zoltan Kiss; Julien Grall; Ian Campbell
Cc: xen-devel@lists.xenproject.org; stefano.stabellini@citrix.com; tim@xen.org; Shameerali Kolothum Thodi; Zoltan Kiss
Subject: Re: [Xen-devel] [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

On 01/06/15 14:18, Zoltan Kiss wrote:
> Sure. Btw. do you have them in a public repo somewhere? It would make 
> it a little bit easier to apply and test.

It's based on the latest staging:

http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-huawei/gicv2-on-gicv3

Regards,

--
Julien Grall

Re: [PATCH] xen/arm: gic-hip04: Resync the driver with the GICv2

[Julien Grall]

On 09/06/2015 09:03, Shameerali Kolothum Thodi wrote:
> Hi Julien/Zoli,

Hi Shameer,

>> http://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-huawei/gicv2-on-gicv3
> I had a run of this staging on our D01 board.

Thank you for testing.

> Xen fails to boot with below error:
> (XEN) domain 0: vGIC requested is not supported
> (XEN) CPU0: Unexpected Trap: Data Abort
>
> After going through the code, it looks to me that hip04 gicv2_info is missing the vgic_versions initialisation. Made the following changes and xen booted and started dom0.
>
> --- ../xen-org/arch/arm/gic-hip04.c   2015-06-01 16:33:40.000000000 +0100
> +++ ./xen/arch/arm/gic-hip04.c  2015-06-09 13:21:44.355012016 +0100
> @@ -735,6 +735,7 @@ static int __init hip04gic_preinit(struc
>                                      const void *data)
>   {
>       gicv2_info.hw_version = GIC_V2;
> +    gicv2_info.vgic_versions = GIC_V2;
>       gicv2_info.node = node;
>       register_gic_ops(&hip04gic_ops);
>       dt_irq_xlate = gic_irq_xlate;
>
> (Sorry, this is a diff not a proper git patch)
> Please correct me, if this is not the correct way to init the vgic_versions.

This is correct. Although, I'm planning to rework the way that the GIC 
tells the supported vGIC in the next version of this GICv2 on GICv3 series.

Regards,

-- 
Julien Grall