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

[Julien Grall <julien.grall@citrix.com>]

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@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> 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