[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v6 06/31] xen/arm: ITS: Port ITS driver to Xen



Hi Vijay,

This patch now looks good. A few comments below.

First, I've noticed that you moved again its_send_inv into patch #13. On a previous version (v4) we asked you to keep all the code imported by Linux in a single patch. You moved it correctly in v5 but then moved again out in this version.

You could have avoid this move by splitting the patch #13 in 2 parts:
        - part 1: introduce the hw_irq_controller for LPIs
        - part 2: plumbing the hw_irq_controller

The part 1 would live before patch #11 which enable the ITS compilation.

On 31/08/2015 12:06, vijay.kilari@xxxxxxxxx wrote:
From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>

The linux driver is based on 4.1 with below commit id

3ad2a5f57656a14d964b673a5a0e4ab0e583c870

I'm sure the commit ID should be changed as you also include "591e5bec13f15feb13fc445b6c9c59954711c4ac".

Note that it's the only commit modifying drivers/irqchip/gic-v3-its.c since the commit ID you mentioned above.

[..]

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
new file mode 100644
index 0000000..4ad8d8e
--- /dev/null
+++ b/xen/arch/arm/gic-v3-its.c
@@ -0,0 +1,1011 @@

[...]

+static void its_lpi_free(struct its_device *dev)
+{
+    int lpi;
+
+    spin_lock(&lpi_lock);
+
+    for ( lpi = dev->event_map.lpi_base;
+        lpi < (dev->event_map.lpi_base + dev->event_map.nr_lpis);
+        lpi += IRQS_PER_CHUNK )

The indentation was valid on the previous version. Why did you change it?

+    {
+        int chunk = its_lpi_to_chunk(lpi);
+
+        if (chunk > lpi_chunks)
+            its_err("Bad LPI chunk %d\n", chunk);
+        if ( test_bit(chunk, lpi_bitmap) )
+            clear_bit(chunk, lpi_bitmap);
+    }
+
+    spin_unlock(&lpi_lock);
+
+    xfree(dev->event_map.lpi_map);
+}

[...]

+static bool gic_rdists_supports_plpis(void)
+{
+    return !!(readl_relaxed(gic_data_rdist().rbase + GICR_TYPER) & 
GICR_TYPER_PLPIS);

The line should be less 80 characters. Please split it.


+}
+
+int its_cpu_init(void)
+{
+    if ( !list_empty(&its_nodes) )
+    {
+        if ( !gic_rdists_supports_plpis() )
+        {
+            its_info("CPU%d: LPIs not supported\n", smp_processor_id());
+            return -ENXIO;
+        }
+        its_cpu_init_lpis();
+        its_cpu_init_collection();
+    }
+
+    return 0;
+}
+
+int __init its_init(struct rdist_prop *rdists)
+{
+    struct dt_device_node *np = NULL;
+
+    static const struct dt_device_match its_device_ids[] __initconst =
+    {
+        DT_MATCH_GIC_ITS,
+        { /* sentinel */ },
+    };
+
+    for (np = dt_find_matching_node(NULL, its_device_ids); np;
+        np = dt_find_matching_node(np, its_device_ids))

Coding style:

for ( ... )

And the indentation of the second line is still wrong it should be

for ( np ...
      np = dt_find... )

+        its_probe(np);
+
+    if ( list_empty(&its_nodes) )
+    {
+        its_warn("ITS: No ITS available, not enabling LPIs\n");
+        return -ENXIO;
+    }
+
+    gic_rdists = rdists;
+    its_alloc_lpi_tables();
+    its_lpi_init(rdists->id_bits);
+
+    return 0;
+}

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.