From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v6 06/31] xen/arm: ITS: Port ITS driver to Xen Date: Mon, 31 Aug 2015 16:41:31 +0100 Message-ID: <55E475AB.8040309@citrix.com> References: <1441019208-2764-1-git-send-email-vijay.kilari@gmail.com> <1441019208-2764-7-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1441019208-2764-7-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, Vijaya Kumar K , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org 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 > > 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