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 E114AC54E64 for ; Mon, 25 Mar 2024 11:23:27 +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=j/3CBtJUOltuRzi9NnDhGxT5hlFAI9owedGgGM4ELqE=; b=QsEIvBOG8z1VHf VD6FqOVb5dGg3mFAT7PYm0AUe5ASDPmSe5CNyGXGQmnW+zWbt+ZgrLZhe1H1SRmd30RhSJ0CPjw8k cjW01OLXA+hLvvXauG8iScJ/UtglCymDCPFFnMmPlk+zacIQ+bah6u6iniWPadDZUZ1Rfa6xE69PW Hmh4xAhjAH9W5t2hYMNdLMJ9bkf4ONEHA86eGzbR/j8s/ivnv8NCdAtEs4CkVTdtaZeGpBv3jDlRk Z8HkwSUm5v5fjdWTODa489c/JLD6KsvNToj/2p3AmAZ6gsBB4XoSPJTzvCIxAzeT0Anla/smsSPg/ KDQIeqam5LmBZOGb0JGA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1roiQB-0000000H1a5-0IiN; Mon, 25 Mar 2024 11:23:19 +0000 Received: from sin.source.kernel.org ([2604:1380:40e1:4800::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1roiQ7-0000000H1YT-19Tk for linux-arm-kernel@lists.infradead.org; Mon, 25 Mar 2024 11:23:17 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 001A3CE176E; Mon, 25 Mar 2024 11:23:13 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5FB56C433C7; Mon, 25 Mar 2024 11:23:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1711365791; bh=2JNQFjYNPVPxTSXEdzTDIvFBar8n4Vzc6fueech50RA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mDNf9urnayjKnHME8f7+8KbiM5Q2KPrJtZDZzFOPhQsHFconJCwKstiSJCvgwadwC MINur/NnfllB+oDF+sCjN/u3CNIjLjVDzl/aW+ueVg2W/WK8UL3wPl5AWdAl0tQomr J4imBrauiuRu5m8BOOhaU5iq2E3qJDoSDVkppHFGuR8AipmRNJk4GROu+uu787G9th dgb8o3wPGdeJYGlDj6e48rdkSpV+lTI8bYQEC6juJOpSN0JhS10nmH0V7jt3A76gAc MW4b+DFWBFD1fI9WWaUon4THyRXmzR4ygTJf6ZHqihRetNmcTP2CPLbveDG6+6iOmV i4mqfkJoJsbNQ== Date: Mon, 25 Mar 2024 12:23:05 +0100 From: Niklas Cassel 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, 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 v4] PCI: keystone: Fix pci_ops for AM654x SoC Message-ID: References: <20240325053722.1955433-1-s-vadapalli@ti.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240325053722.1955433-1-s-vadapalli@ti.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240325_042315_759738_D3255988 X-CRM114-Status: GOOD ( 38.46 ) 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 Mon, Mar 25, 2024 at 11:07:22AM +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 are applicable to the controller version > 4.90a which is present in AM654x SoCs. > > Thus, as a fix, move the contents of "ks_pcie_v3_65_add_bus()" to the > .host_init callback "ks_pcie_host_init()" and execute it only for non > AM654x SoC devices which have the v3.65a DWC PCIe IP Controllers. > > Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus") > Suggested-by: Serge Semin > Suggested-by: Bjorn Helgaas > Signed-off-by: Siddharth Vadapalli > --- > > Hello, > > This patch is based on linux-next tagged next-20240325. > This patch is technically the next version for the v3 patch at: > https://patchwork.kernel.org/project/linux-pci/patch/20231019081330.2975470-1-s-vadapalli@ti.com/ > but the implementation is based on the RFC patch at: > https://patchwork.kernel.org/project/linux-pci/patch/20231027084159.4166188-1-s-vadapalli@ti.com/ > Since the RFC patch mentioned above fixes the same issue being > fixed by the v3 patch, I have dropped the v3 patch and am using > the RFC patch since it is a cleaner implementation and was discussed at: > https://lore.kernel.org/r/20231019220847.GA1413474@bhelgaas/ > > Regards, > Siddharth. > > drivers/pci/controller/dwc/pci-keystone.c | 51 ++++++++--------------- > 1 file changed, 17 insertions(+), 34 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 844de4418724..f45bdeac520a 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -445,44 +445,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, > }; > > /** > @@ -822,6 +788,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp) > if (ret < 0) > return ret; > > + if (!ks_pcie->is_am6) { Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6). >From reading the old threads, it appears that v3.65a: -Has no support for iATUs. iATU-specific resource handling code is to be bypassed for v3.65 h/w. Thus v3.65a has it's own .child_ops implementation, so that pcie-designware-host.c does not configure the iATUs. -v3.65a has it's own .msi_init implementation, so that pcie-designware-host.c does not call dw_pcie_msi_host_init() to configure the MSI controller. While 4.90a: -Does have iATU support. -Does use the generic dw_pcie_msi_host_init(). Considering the major differences (with v3.65a being the outlier) here, I think it would have been a much wiser idea to have two different glue drivers for these two compatibles (ti,keystone-pcie and ti,am654-pcie-rc). Right now the driver is quite hard to read, most of the functions in this driver exist because v3.65a does not have an iATU and does not use the generic DWC way to handle MSIs. Additionally, you have "if (!ks_pcie->is_am6)" spread out all over the driver, to control quite major things, like if you should overload .child_ops, or if you should set up inbound translation without an iATU. This makes is even harder to see which code is actually used for am654... like the fact that it actually uses the generic way to handle MSIs... The driver for am654 would be much nicer since many of the functions in this driver would not be needed (and the fact that you have only implemented EP support for am654 and not for v3.65a). All EP related stuff would be in the am654 file/driver. You could keep the quirky stuff for v3.65a in the existing pci-keystone.c driver. (I guess if there is a function that is identical between the twos, you could have a pci-keystone-common.{c,h} that can be used by both drivers, but from the looks of it, they seem to share very little code. Kind regards, Niklas > + /* 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); > + } > + > #ifdef CONFIG_ARM > /* > * PCIe access errors that result into OCP errors are caught by ARM as > -- > 2.40.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel