From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 75DC8C433F5 for ; Tue, 22 Feb 2022 15:47:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=Ne1C7MCjOXtQGG2GWlFwj5wdUFUtx+cTRIeXD4cqb5A=; b=xY3XRIaKzapZZh FFyVK7aRkR7y9WKS3Q5rWWanTdQUO6LVSxTz6xnOL7MWxmGje6KgEf63oxkRyEnMRR1utx88Lraro p16k7AQhDX6jaVA9Mg2ZZP/pxNQ+sARmhD1jvKobexVZGNfsPO702J22jLzMnBfRKG2Ak3wwDdfdJ 7jPP0dEqD0x1TcmGYP6JyPsFfbeQj127MKCSDOT0b01YsMCM9VE4dq/ZrPVJqgUBQqS5NhXNXgivc X1qTYAUWedWdIRazKHVHB0dgH0EONI1AUhofPrtbC146m6gdgo0jTeRrhuVqlqOwUiW6I9bbF/yNg P8anlTybaw/RGwsemtfA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nMXN5-00ANV8-LJ; Tue, 22 Feb 2022 15:46:35 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nMXMY-00ANBb-Lj for linux-arm-kernel@lists.infradead.org; Tue, 22 Feb 2022 15:46:05 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 65B1B1042; Tue, 22 Feb 2022 07:46:01 -0800 (PST) Received: from lpieralisi (e121166-lin.cambridge.arm.com [10.1.196.255]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id ED12E3F5A1; Tue, 22 Feb 2022 07:45:59 -0800 (PST) Date: Tue, 22 Feb 2022 15:45:53 +0000 From: Lorenzo Pieralisi To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: robh+dt@kernel.org, Bjorn Helgaas , Thomas Petazzoni , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Marek =?iso-8859-1?Q?Beh=FAn?= , Russell King , Marc Zyngier , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 10/11] PCI: mvebu: Implement support for legacy INTx interrupts Message-ID: <20220222154553.GA19919@lpieralisi> References: <20220105150239.9628-1-pali@kernel.org> <20220112151814.24361-1-pali@kernel.org> <20220112151814.24361-11-pali@kernel.org> <20220211171917.GA740@lpieralisi> <20220211175202.gku5pkwn5wmjo5al@pali> <20220216234039.stxv5ndd6ai23sbb@pali> <20220222102057.GA17238@lpieralisi> <20220222105129.jg5kwmhvhggsv72n@pali> <20220222152409.GA18799@lpieralisi> <20220222154226.mwu7d3silgmwzeqc@pali> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220222154226.mwu7d3silgmwzeqc@pali> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220222_074602_892451_D81E0D94 X-CRM114-Status: GOOD ( 54.84 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Feb 22, 2022 at 04:42:26PM +0100, Pali Roh=E1r wrote: > On Tuesday 22 February 2022 15:24:09 Lorenzo Pieralisi wrote: > > On Tue, Feb 22, 2022 at 11:51:29AM +0100, Pali Roh=E1r wrote: > > > On Tuesday 22 February 2022 10:21:06 Lorenzo Pieralisi wrote: > > > > On Thu, Feb 17, 2022 at 12:40:39AM +0100, Pali Roh=E1r wrote: > > > > > On Friday 11 February 2022 18:52:02 Pali Roh=E1r wrote: > > > > > > On Friday 11 February 2022 17:19:17 Lorenzo Pieralisi wrote: > > > > > > > On Wed, Jan 12, 2022 at 04:18:13PM +0100, Pali Roh=E1r wrote: > > > > > > > > This adds support for legacy INTx interrupts received from = other PCIe > > > > > > > > devices and which are reported by a new INTx irq chip. > > > > > > > > = > > > > > > > > With this change, kernel can distinguish between INTA, INTB= , INTC and INTD > > > > > > > > interrupts. > > > > > > > > = > > > > > > > > Note that for this support, device tree files has to be pro= perly adjusted > > > > > > > > to provide "interrupts" or "interrupts-extended" property w= ith intx > > > > > > > > interrupt source, "interrupt-names" property with "intx" st= ring and also > > > > > > > > 'interrupt-controller' subnode must be defined. > > > > > > > > = > > > > > > > > If device tree files do not provide these nodes then driver= would work as > > > > > > > > before. > > > > > > > = > > > > > > > Nit: this information is not useful. DT rules are written in = DT > > > > > > > bindings, not in kernel commit logs. All I am saying is that = firmware > > > > > > > developers should not have to read this log to write firmware. > > > > > > = > > > > > > It was not intended for firmware developers, but for reviewers = of this > > > > > > patch to understand, what is happening in code and that with ol= d DT > > > > > > files this patch does not change driver behavior (=3D work as b= efore). > > > > > > = > > > > > > > > Signed-off-by: Pali Roh=E1r > > > > > > > > --- > > > > > > > > drivers/pci/controller/pci-mvebu.c | 185 +++++++++++++++++= ++++++++++-- > > > > > > > > 1 file changed, 177 insertions(+), 8 deletions(-) > > > > > > > > = > > > > > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/p= ci/controller/pci-mvebu.c > > > > > > > > index 1e90ab888075..dbb6ecb4cb70 100644 > > > > > > > > --- a/drivers/pci/controller/pci-mvebu.c > > > > > > > > +++ b/drivers/pci/controller/pci-mvebu.c > > > > > > > > @@ -54,9 +54,10 @@ > > > > > > > > PCIE_CONF_ADDR_EN) > > > > > > > > #define PCIE_CONF_DATA_OFF 0x18fc > > > > > > > > #define PCIE_INT_CAUSE_OFF 0x1900 > > > > > > > > +#define PCIE_INT_UNMASK_OFF 0x1910 > > > > > > > = > > > > > > > Nit: I understand it is tempting but here you are redefining = or better > > > > > > > giving a proper label to a register. Separate patch please. > > > > > > = > > > > > > Ok! > > > > > > = > > > > > > > > +#define PCIE_INT_INTX(i) BIT(24+i) > > > > > > > > #define PCIE_INT_PM_PME BIT(28) > > > > > > > > -#define PCIE_MASK_OFF 0x1910 > > > > > > > = > > > > > > > See above. > > > > > > > = > > > > > > > > -#define PCIE_MASK_ENABLE_INTS 0x0f000000 > > > > > > > > +#define PCIE_INT_ALL_MASK GENMASK(31, 0) > > > > > > > > #define PCIE_CTRL_OFF 0x1a00 > > > > > > > > #define PCIE_CTRL_X1_MODE 0x0001 > > > > > > > > #define PCIE_CTRL_RC_MODE BIT(1) > > > > > > > > @@ -110,6 +111,9 @@ struct mvebu_pcie_port { > > > > > > > > struct mvebu_pcie_window iowin; > > > > > > > > u32 saved_pcie_stat; > > > > > > > > struct resource regs; > > > > > > > > + struct irq_domain *intx_irq_domain; > > > > > > > > + raw_spinlock_t irq_lock; > > > > > > > > + int intx_irq; > > > > > > > > }; > > > > > > > > = > > > > > > > > static inline void mvebu_writel(struct mvebu_pcie_port *po= rt, u32 val, u32 reg) > > > > > > > > @@ -235,7 +239,7 @@ static void mvebu_pcie_setup_wins(struc= t mvebu_pcie_port *port) > > > > > > > > = > > > > > > > > static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *po= rt) > > > > > > > > { > > > > > > > > - u32 ctrl, lnkcap, cmd, dev_rev, mask; > > > > > > > > + u32 ctrl, lnkcap, cmd, dev_rev, unmask; > > > > > > > > = > > > > > > > > /* Setup PCIe controller to Root Complex mode. */ > > > > > > > > ctrl =3D mvebu_readl(port, PCIE_CTRL_OFF); > > > > > > > > @@ -288,10 +292,30 @@ static void mvebu_pcie_setup_hw(struc= t mvebu_pcie_port *port) > > > > > > > > /* Point PCIe unit MBUS decode windows to DRAM space. */ > > > > > > > > mvebu_pcie_setup_wins(port); > > > > > > > > = > > > > > > > > - /* Enable interrupt lines A-D. */ > > > > > > > > - mask =3D mvebu_readl(port, PCIE_MASK_OFF); > > > > > > > > - mask |=3D PCIE_MASK_ENABLE_INTS; > > > > > > > > - mvebu_writel(port, mask, PCIE_MASK_OFF); > > > > > > > > + /* Mask all interrupt sources. */ > > > > > > > > + mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_UNMASK_OF= F); > > > > > > > > + > > > > > > > > + /* Clear all interrupt causes. */ > > > > > > > > + mvebu_writel(port, ~PCIE_INT_ALL_MASK, PCIE_INT_CAUSE_OFF= ); > > > > > > > > + > > > > > > > > + if (port->intx_irq <=3D 0) { > > > > > > > > + /* > > > > > > > > + * When neither "summary" interrupt, nor "intx" interrup= t was > > > > > > > > + * specified in DT then unmask all legacy INTx interrupt= s as in > > > > > > > > + * this case driver does not provide a way for masking a= nd > > > > > > > > + * unmasking of individual legacy INTx interrupts. In th= is case > > > > > > > > + * all interrupts, including legacy INTx are reported vi= a one > > > > > > > > + * shared GIC source and therefore kernel cannot disting= uish > > > > > > > > + * which individual legacy INTx was triggered. These int= errupts > > > > > > > > + * are shared, so it should not cause any issue. Just > > > > > > > > + * performance penalty as every PCIe interrupt handler n= eeds to > > > > > > > > + * be called when some interrupt is triggered. > > > > > > > > + */ > > > > > > > = > > > > > > > This comment applies to current mainline right (ie it describ= es how > > > > > > > current mainline handles INTx) ? IMO you should split it out = in a > > > > > > > separate patch. > > > > > > = > > > > > > This above comment describe what happens in if-branch when intx= _irq is > > > > > > not set (as written in comment "when intx interrupt was not spe= cified in > > > > > > DT"). You are right that this is also the behavior in the curre= nt > > > > > > mainline. > > > > > > = > > > > > > I'm not sure if this comment can be split out as support for "i= ntx" > > > > > > interrupt is in this patch. > > > > > > = > > > > > > > I understand it is hard but a patch is a logical _change_, th= is > > > > > > > comment is a change per se, it is a clarification on current > > > > > > > behaviour. > > > > > > = > > > > > > Ok, I could try to split this comment into two patches, but par= t about > > > > > > if-branch comment needs to stay in "this" patch. > > > > > = > > > > > I have done it locally. > > > > > = > > > > > Let me know when I should resend this patch series and I will inc= lude > > > > > into it also these changes. > > > > = > > > > Hi, > > > > = > > > > yes please resend it and I will merge it. > > > = > > > Done! > > > https://lore.kernel.org/linux-pci/20220222104625.28461-1-pali@kernel.= org/T/#u > > = > > Can you rebase it please on top of my pci/mvebu branch ? > > = > > https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/ > > = > > Forgive me, I forgot to mention that, thanks. > > = > > Lorenzo > = > Ok! I rebased V3 on top of c3bd7dc553eea5a3595ca3aa0adee9bf83622a1f > (pci/mvebu branch in your repo), fixed conflicts and pushed to my git > repo https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/ > as commit 42402f0cfc362ffb0b7e464f420d6ead342dab2b (lpieralisi-pci-mvebu > branch). It is enough? Or do you want me to resend it via emails? You need to re-send it to linux-pci ML, I only apply patches posted there, sorry for the churn, thanks. Lorenzo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel