From patchwork Fri Jun 8 13:08:08 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Miquel Raynal X-Patchwork-Id: 10454419 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C894D60159 for ; Fri, 8 Jun 2018 13:08:46 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AA014292CC for ; Fri, 8 Jun 2018 13:08:46 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9D7F529379; Fri, 8 Jun 2018 13:08:46 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 1C097292CC for ; Fri, 8 Jun 2018 13:08:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=xTJQzQLtjPbkdRiNG8N2EhCc7tFeAMdmSqetbfTbHd8=; b=naVvFcaH7tyVzS TuBC7hAq76+FyFSF8+ExrhyNs0a+/EZlaz385N8KAR99GcyLGR+OvZ+TK7+3gWGSzBGkU4d9dYm0Y Mkd7993Sf+6qQm2sdaHzLvb1hvnS80T4HR971e4pgA/e4FOfxXgQDGJe/vK10JSfJqdmplVpAM8ZA eO1mOKV/aYeToTb+2pUxkWC6xj9LI+6rWctWpBZKRreprZW2hxItTPszY+KaMupyWt38Gi0Asjee9 Z79F2fs7GgdE3bdjjALbP3NVkBfYE94DnNR4q/4Hd8koOB0DAXdx3uLvp2jbA0D/boTcwaFQXsM/W HUpAeEa++cIOpmFjw2iw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1fRH88-0001vu-0M; Fri, 08 Jun 2018 13:08:36 +0000 Received: from mail.bootlin.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1fRH83-0001t0-TD for linux-arm-kernel@lists.infradead.org; Fri, 08 Jun 2018 13:08:34 +0000 Received: by mail.bootlin.com (Postfix, from userid 110) id 4CE272069C; Fri, 8 Jun 2018 15:08:19 +0200 (CEST) Received: from xps13 (AAubervilliers-681-1-128-7.w90-88.abo.wanadoo.fr [90.88.9.7]) by mail.bootlin.com (Postfix) with ESMTPSA id CE0B8203EB; Fri, 8 Jun 2018 15:08:08 +0200 (CEST) Date: Fri, 8 Jun 2018 15:08:08 +0200 From: Miquel Raynal To: Marc Zyngier Subject: Re: [PATCH v2 11/16] irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI) Message-ID: <20180608150808.7af98f18@xps13> In-Reply-To: References: <20180522094042.24770-1-miquel.raynal@bootlin.com> <20180522094042.24770-12-miquel.raynal@bootlin.com> Organization: Bootlin X-Mailer: Claws Mail 3.15.0-dirty (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20180608_060832_231291_A5A6BC80 X-CRM114-Status: GOOD ( 28.56 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Andrew Lunn , Jason Cooper , devicetree@vger.kernel.org, Antoine Tenart , Catalin Marinas , Gregory Clement , Haim Boot , Will Deacon , linux-kernel@vger.kernel.org, Maxime Chevallier , Nadav Haklai , Rob Herring , Thomas Petazzoni , Thomas Gleixner , Hanna Hawa , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Marc, Thank you for the review. On Wed, 23 May 2018 15:23:48 +0100, Marc Zyngier wrote: > On 22/05/18 10:40, Miquel Raynal wrote: > > An SEI driver provides an MSI domain through which it is possible to > > raise SEIs. > > > > Handle the NSR probe function in a more generic way to support other > > type of interrupts (ie. the SEIs). > > > > For clarity we do not use tree IRQ domains for now but linear ones > > instead, allocating the 207 ICU lines for each interrupt group. > > What's the rational for not using trees? Because that's effectively a > 100% overhead... There is none. I had a look at how to do it. In the ICU driver I would like to just drop the nvec parameter (number of interrupts in the domain) when calling platform_msi_create_device_domain(). The above function would call irq_domain_create_hierarchy() which would create a tree domain instead of a linear one because of nvec being 0. However, there is a check in platform_msi_alloc_priv_data() (also called by platform_msi_create_device_domain()) that will error out if nvec is null. I'm not 100% sure this is safe but I don't see the point of prohibiting nvec to be null here. So would you accept this change? if (dev->msi_domain->bus_token != DOMAIN_BUS_PLATFORM_MSI) { > > > Reallocating an ICU slot is prevented by the use of an ICU-wide bitmap. > > > > Signed-off-by: Miquel Raynal > > --- [...] > > @@ -131,7 +160,8 @@ static int > > mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > > unsigned long *hwirq, unsigned int *type) > > { > > - struct mvebu_icu *icu = platform_msi_get_host_data(d); > > + struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d); > > + struct mvebu_icu *icu = msi_data->icu; > > unsigned int param_count = icu->legacy_bindings ? 3 : 2; > > > > /* Check the count of the parameters in dt */ > > @@ -172,7 +202,9 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > > int err; > > unsigned long hwirq; > > struct irq_fwspec *fwspec = args; > > - struct mvebu_icu *icu = platform_msi_get_host_data(domain); > > + struct mvebu_icu_msi_data *msi_data = > > + platform_msi_get_host_data(domain); > > + struct mvebu_icu *icu = msi_data->icu; > > struct mvebu_icu_irq_data *icu_irqd; > > > > icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL); > > @@ -186,16 +218,22 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > > goto free_irqd; > > } > > > > + spin_lock(&icu->msi_lock); > > + err = bitmap_allocate_region(icu->msi_bitmap, hwirq, 0); > > + spin_unlock(&icu->msi_lock); > > This (and the freeing counterpart) could deserve a couple of helpers. Sure. > > > + if (err < 0) > > + goto free_irqd; > > + > > if (icu->legacy_bindings) > > icu_irqd->icu_group = fwspec->param[0]; > > else > > - icu_irqd->icu_group = ICU_GRP_NSR; > > + icu_irqd->icu_group = msi_data->subset_data->icu_group; > > icu_irqd->icu = icu; > > > > err = platform_msi_domain_alloc(domain, virq, nr_irqs); > > if (err) { > > dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n"); > > - goto free_irqd; > > + goto free_bitmap; > > } > > > > /* Make sure there is no interrupt left pending by the firmware */ [...] > > @@ -268,9 +332,30 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev) > > return 0; > > } > > > > +static const struct mvebu_icu_subset_data mvebu_icu_nsr_subset_data = { > > + .icu_group = ICU_GRP_NSR, > > + .offset_set_ah = ICU_SETSPI_NSR_AH, > > + .offset_set_al = ICU_SETSPI_NSR_AL, > > + .offset_clr_ah = ICU_CLRSPI_NSR_AH, > > + .offset_clr_al = ICU_CLRSPI_NSR_AL, > > +}; > > + > > +static const struct mvebu_icu_subset_data mvebu_icu_sei_subset_data = { > > + .icu_group = ICU_GRP_SEI, > > + .offset_set_ah = ICU_SET_SEI_AH, > > + .offset_set_al = ICU_SET_SEI_AL, > > + .offset_clr_ah = ICU_CLR_SEI_AH, > > + .offset_clr_al = ICU_CLR_SEI_AL, > > I thought SEI was edge only, given what you do in mvebu_icu_init. > Confused... AFAIK, the ICU can produce both level and edge MSI. Currently, when it comes to SEI, we don't use the .offset_clr_a[hl] entries because the SEI block expects edge-MSIs, but I thought useful to fill them anyway. I will remove them both to avoid the confusion. Thanks, Miquèl --- a/drivers/base/platform-msi.c +++ b/drivers/base/platform-msi.c @@ -203,7 +203,7 @@ platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec, * accordingly (which would impact the max number of MSI * capable devices). */ - if (!dev->msi_domain || !write_msi_msg || !nvec || nvec > MAX_DEV_MSIS) + if (!dev->msi_domain || !write_msi_msg || nvec > MAX_DEV_MSIS) return ERR_PTR(-EINVAL);