Received-SPF: pass (google.com: domain of linux-kernel+bounces-116991-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 25 Mar 2024 17:52:28 +0530
From: Siddharth Vadapalli <s-vadapalli@ti.com>
To: Niklas Cassel <cassel@kernel.org>
CC: Siddharth Vadapalli <s-vadapalli@ti.com>, <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: <ea0294d4-85d1-4784-acd7-dd247165f69b@ti.com>
References: <20240325053722.1955433-1-s-vadapalli@ti.com>
 <ZgFemQ8gHpB8yMef@ryzen>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZgFemQ8gHpB8yMef@ryzen>

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:
> > 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 <fancer.lancer@gmail.com>
> > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> > 
> > 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.

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.

Regards,
Siddharth.