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 193E2C54E58 for ; Tue, 26 Mar 2024 04:59:47 +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=aWMfk9jOUKYBZVaaKyKcb8yQtB9FoZNr+WwT8u3f+Bw=; b=o+0BhC0B5SlNt2 CbbuVbYpHzIAy/dP+4fkg4kKK/6XunoiC85oGIjTBt1cia5EVcxXWa3LGf7i5KsXNCCOFTnxpjFCS W3tcfQt9XSydIaBuzwLQoJzrrJzUTDonbXTiVTQrKSkuUtM8icXCZ1IYirJ9lZA1k3L8mnF3or28m aI2Qnj1i0RUzp4ZEI0FnlZdEo7/wul+vtyj+xCjs8w2lmFZgw5ZFzKl6JlSBOpdI9h5hk/GbI04qT aSovGLRl3Mu/NHZqeGbMfjMMHWOsGkWx0q6hgJaOGs3OrV0Hi7QF6a94ioMcrDDykXpsmLTxtx8Ut vJhcvUQASn0BySELPhig==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1royuE-0000000355L-1kAP; Tue, 26 Mar 2024 04:59:26 +0000 Received: from lelv0143.ext.ti.com ([198.47.23.248]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1royuB-0000000354T-0Ycp for linux-arm-kernel@lists.infradead.org; Tue, 26 Mar 2024 04:59:24 +0000 Received: from fllv0034.itg.ti.com ([10.64.40.246]) by lelv0143.ext.ti.com (8.15.2/8.15.2) with ESMTP id 42Q4xCWg102011; Mon, 25 Mar 2024 23:59:12 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1711429152; bh=cUjb4FFIv54rr7MmbTYuEQBtNz+9FaFMStjEdNaoD9U=; h=Date:From:To:CC:Subject:References:In-Reply-To; b=aVi8fu7mnEddy0Tfz30Xi/MNV2qTnyIW/245IyarM3FUwzwYb6s9CGVR1lclYL6QD nj0DECU66JTcSvJsxhxSnhAXjQ4wfBv3wNkbFYSgKDJnNbJKJSdAh2KXq7QO68bG18 Vx2ZxgCQyzNrYYHyFPg0mSQv73PRXuZjlX3aoOtI= Received: from DFLE111.ent.ti.com (dfle111.ent.ti.com [10.64.6.32]) by fllv0034.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 42Q4xCTI037922 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 25 Mar 2024 23:59:12 -0500 Received: from DFLE103.ent.ti.com (10.64.6.24) by DFLE111.ent.ti.com (10.64.6.32) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Mon, 25 Mar 2024 23:59:12 -0500 Received: from lelvsmtp6.itg.ti.com (10.180.75.249) by DFLE103.ent.ti.com (10.64.6.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Mon, 25 Mar 2024 23:59:12 -0500 Received: from localhost (uda0492258.dhcp.ti.com [172.24.227.9]) by lelvsmtp6.itg.ti.com (8.15.2/8.15.2) with ESMTP id 42Q4xBbt061615; Mon, 25 Mar 2024 23:59:11 -0500 Date: Tue, 26 Mar 2024 10:29:10 +0530 From: Siddharth Vadapalli To: Niklas Cassel CC: Siddharth Vadapalli , , , , , , , , , , , , , Subject: Re: [PATCH v4] PCI: keystone: Fix pci_ops for AM654x SoC Message-ID: <750df13b-ee2f-4067-a9e0-7fd1ace384cf@ti.com> References: <20240325053722.1955433-1-s-vadapalli@ti.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240325_215923_328191_26D99BA1 X-CRM114-Status: GOOD ( 52.03 ) 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 02:45:09PM +0100, Niklas Cassel wrote: > Hello Siddharth, > > On Mon, Mar 25, 2024 at 05:52:28PM +0530, Siddharth Vadapalli wrote: > > On Mon, Mar 25, 2024 at 12:23:05PM +0100, Niklas Cassel wrote: > > > On Mon, Mar 25, 2024 at 11:07:22AM +0530, Siddharth Vadapalli wrote: > > > > @@ -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. > > > > Thank you for reviewing the patch. I agree that two drivers will be > > better considering the !ks_pcie->is_am6 present throughout the driver. > > However, I hope you notice the fact that commit: > > 6ab15b5e7057 PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus > > introduced a regression in a driver which was working prior to that > > commit for AM654. While there are flaws in the driver and it needs to be > > split to handle v3.65a and other versions in a cleaner manner, I am > > unable to understand why that is a precursor to fixing the regression. > > > > If splitting the driver is the only way to fix this regression, please > > let me know and I will work on that instead, though it will take up more > > time. > > I think you are misunderstanding me. > > I think this patch is fine, except for the comment that I gave: > "Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6)." > > Like: > > /* > * This is only needed for !am654 since it has its own msi_irq_chip > * implementation. (am654 uses the generic msi_irq_chip implementation.) > */ > if (!ks_pcie->is_am6) { > ... > } > > > In fact, if you move this code to ks_pcie_msi_host_init(), instead of > ks_pcie_host_init(), you would not need a comment (or a if (!ks_pcie->is_am6)), > since ks_pcie_msi_host_init() is only executed by !am654. This seems much better :) In the current code, the execution is as follows: ks_pcie_probe() dw_pcie_host_init() pci_host_probe() ks_pcie_v3_65_add_bus() Moving the contents of ks_pcie_v3_65_add_bus() to ks_pcie_msi_host_init() will result in: ks_pcie_probe() dw_pcie_host_init() if (pci_msi_enabled()) if (pp->ops->msi_init) { ret = pp->ops->msi_init(pp); ks_pcie_msi_host_init() pci_host_probe() I will update this patch based on your suggestion. If it's alright, may I also add your "Suggested-by" tag for the v5 patch? Please let me know. > > > > > My suggestion to split this driver to two different drivers is just because > I noticed how different they are (am654 has iATUs, uses generic msi_irq_chip > implementation and has EP-mode support. !am654 has no iATUs, its own MSI > implementation and no EP-mode support.) > > So the am654 driver would look like most other DWC glue drivers. > The non-am654 driver would look mostly like it looks today, except you would > remove the EP-mode support. > > However, this suggestion can of course be implemented sometime in the future > and should not be a blocker for the patch in $subject. Thank you for clarifying. Regards, Siddharth. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel