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 9003EC25B78 for ; Mon, 13 May 2024 21:54:09 +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: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:References: List-Owner; bh=TP0T8IwIBXLmoQUoxG1lUf2zeyy69dbxKOFbSqWRWI4=; b=vZfQq0h/C1O6eU Or3r1UV53PQhgYBxj6QONXRjPPN6CPSHy2CJYaSJKU2TJQJI8u+yZaNjErp16cVhUXJ/aG68EWph5 cE6n6qtFw6EpGzimIyP79Zsg+Z6xNU7WVZaKwGsJE7qmHws7FuKbww+YQup2KvCRGbYai6iYu8wMO Pq6vsZ435/WHAZ3fL29TrAsbuLDYaph2pP+1UNpNXvf7f/IGZLWLh4kIH4BiNt/NBXrYhkyVqv/u7 rB5I5W3NzOGF3xkpBrxwLJ3m0LODmyun1V5IuTOtYihXiePi3nNBo/12z5IhEhhyzYzA4OMKGAnb6 I1eVeQEAaH387ytv/+Tw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s6dcK-0000000EJBi-2YAc; Mon, 13 May 2024 21:53:56 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s6dcI-0000000EJB5-030f for linux-arm-kernel@lists.infradead.org; Mon, 13 May 2024 21:53:55 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 9CAF360FA2; Mon, 13 May 2024 21:53:52 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0FE55C113CC; Mon, 13 May 2024 21:53:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1715637232; bh=xYDOX8OokrT4sH7DNkwITxYGFuKzHAph5Phz8h32PK4=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=UPyD8bb3PuObTLRJxghWQFxqvzE6uKt4fTrymkorfIBPn/bQuaSDPUjjsXjc5UFdn FzB5fsSbpxbDDLvJ4f4PbdYNy9lAQsbBBTqa9EXQ30dWOzEKIvjjoX9BHKNi0J6EKa gx3bDjUE6z5pSldC28Ts5s6Xs2U2xLxpdsDKDOlLGJgzRrP6boUIHD1XoQB+LnTEBT AgGo9+LfW5hSWL8oHigg70jh052umXVrPHA39jZv+6GXvl6khaCFoXS3yzJ0nvDvp5 e4xFaLPgmf8zIwFSzs+FNpFZVq2xwlzmy7RBFS4Uk/gteQbO/IbSzZRvdpYHfR6ye8 5+VLGNIGII25g== Date: Mon, 13 May 2024 16:53:50 -0500 From: Bjorn Helgaas To: Siddharth Vadapalli Cc: lpieralisi@kernel.org, kw@linux.com, robh@kernel.org, bhelgaas@google.com, manivannan.sadhasivam@linaro.org, fancer.lancer@gmail.com, u.kleine-koenig@pengutronix.de, cassel@kernel.org, dlemoal@kernel.org, yoshihiro.shimoda.uh@renesas.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, srk@ti.com Subject: Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC Message-ID: <20240513215350.GA1996021@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240328085041.2916899-3-s-vadapalli@ti.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240513_145354_169702_EECE7FDB X-CRM114-Status: GOOD ( 29.05 ) 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="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Mar 28, 2024 at 02:20:41PM +0530, Siddharth Vadapalli wrote: > In the process of converting .scan_bus() callbacks to .add_bus(), the > ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus(). > The .scan_bus() method belonged to ks_pcie_host_ops which was specific > to controller version 3.65a, while the .add_bus() method had been added > to ks_pcie_ops which is shared between the controller versions 3.65a and > 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer > ks_pcie_v3_65_add_bus() method is applicable to the controller version > 4.90a which is present in AM654x SoCs. > > Thus, as a fix, remove "ks_pcie_v3_65_add_bus()" and move its contents > to the .msi_init callback "ks_pcie_msi_host_init()" which is specific to > the 3.65a controller. > > Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus") > Suggested-by: Serge Semin > Suggested-by: Bjorn Helgaas > Suggested-by: Niklas Cassel > Reviewed-by: Niklas Cassel > Signed-off-by: Siddharth Vadapalli Thanks for splitting this into two patches. Krzysztof has applied both to pci/controller/keystone and we hope to merge them for v6.10. I *would* like the commit log to be at a little higher level if possible. Right now it's a detailed description at the level of the code edits, but it doesn't say *why* we want this change. I think the first cut at this was https://lore.kernel.org/linux-pci/20231011123451.34827-1-s-vadapalli@ti.com/t/#u, which mentioned Completion Timeouts during MSI-X configuration and 45 second delays during boot. IIUC, prior to 6ab15b5e7057, ks_pcie_v3_65_scan_bus() initialized BAR 0 and was only used for v3.65a devices. 6ab15b5e7057 renamed it to ks_pcie_v3_65_add_bus() and called it for both v3.65a and v4.90a. I think the problem is that in the current code, the ks_pcie_ops.add_bus() method (ks_pcie_v3_65_add_bus()) is used for all devices (both v3.65a and v4.90a). So I guess doing the BAR 0 setup on v4.90a broke something there? I'm not quite clear on the mechanism, but it would be helpful to at least know what's wrong and on what platform. E.g., currently v4.90 suffers Completion Timeouts and 45 second boot delays? And this patch fixes that? > --- > drivers/pci/controller/dwc/pci-keystone.c | 52 ++++++++--------------- > 1 file changed, 18 insertions(+), 34 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 5c073e520628..6cb3a4713009 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -289,6 +289,24 @@ static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie) > > static int ks_pcie_msi_host_init(struct dw_pcie_rp *pp) > { > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); > + > + /* Configure and set up BAR0 */ > + ks_pcie_set_dbi_mode(ks_pcie); > + > + /* Enable BAR0 */ > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); > + > + ks_pcie_clear_dbi_mode(ks_pcie); > + > + /* > + * For BAR0, just setting bus address for inbound writes (MSI) should > + * be sufficient. Use physical address to avoid any conflicts. > + */ > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); > + > pp->msi_irq_chip = &ks_pcie_msi_irq_chip; > return dw_pcie_allocate_domains(pp); > } > @@ -445,44 +463,10 @@ static struct pci_ops ks_child_pcie_ops = { > .write = pci_generic_config_write, > }; > > -/** > - * ks_pcie_v3_65_add_bus() - keystone add_bus post initialization > - * @bus: A pointer to the PCI bus structure. > - * > - * This sets BAR0 to enable inbound access for MSI_IRQ register > - */ > -static int ks_pcie_v3_65_add_bus(struct pci_bus *bus) > -{ > - struct dw_pcie_rp *pp = bus->sysdata; > - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); > - > - if (!pci_is_root_bus(bus)) > - return 0; > - > - /* Configure and set up BAR0 */ > - ks_pcie_set_dbi_mode(ks_pcie); > - > - /* Enable BAR0 */ > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); > - > - ks_pcie_clear_dbi_mode(ks_pcie); > - > - /* > - * For BAR0, just setting bus address for inbound writes (MSI) should > - * be sufficient. Use physical address to avoid any conflicts. > - */ > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); > - > - return 0; > -} > - > static struct pci_ops ks_pcie_ops = { > .map_bus = dw_pcie_own_conf_map_bus, > .read = pci_generic_config_read, > .write = pci_generic_config_write, > - .add_bus = ks_pcie_v3_65_add_bus, > }; > > /** > -- > 2.40.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel