Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Wed, 25 Oct 2023 13:28:07 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Siddharth Vadapalli <s-vadapalli@ti.com>,
        Bjorn Helgaas <helgaas@kernel.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        bhelgaas@google.com, lpieralisi@kernel.org, robh@kernel.org,
        kw@linux.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        r-gunasekaran@ti.com, srk@ti.com
Subject: Re: [PATCH v3] PCI: keystone: Fix pci_ops for AM654x SoC
Message-ID: <3szgydgz7ege5h75mwdket5lniwy4oe52dq2uqehf74il5hc5j@oaqcbmfuf6de>
References: <20231023212628.GA1627567@bhelgaas>
 <c7851172-f474-42f3-9730-1f323f9e6c73@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c7851172-f474-42f3-9730-1f323f9e6c73@ti.com>
Precedence: bulk

Hi Siddharth, Bjorn

On Wed, Oct 25, 2023 at 10:32:50AM +0530, Siddharth Vadapalli wrote:
> Hello Bjorn,
> 
> On 24/10/23 02:56, Bjorn Helgaas wrote:
> > On Mon, Oct 23, 2023 at 05:05:30PM +0530, Siddharth Vadapalli wrote:
> >> On 23/10/23 16:12, Serge Semin wrote:
> >>
> >> ...
> >>
> >>> Siddharth, if it won't be that much bother and you have an access to
> >>> the v3.65-based Keystone PCIe device, could you please have a look
> >>> whether it's possible to implement what Bjorn suggested?
> >>
> >> Unfortunately I don't have any SoC/Device with me that has the v3.65 PCIe
> >> controller, so I will not be able to test Bjorn's suggestion.
> > 
> > Huh.  57e1d8206e48 ("MAINTAINERS: move Murali Karicheri to credits")
> > removed the maintainer for pci-keystone.c, so the driver hasn't had a
> > maintainer for over two years.
> > 
> > Given the fact that there's no maintainer, I'm more than happy to take
> > a patch to move this code to somewhere in the host_init() callback,
> > even if you don't have the hardware to test it.
> 
> Sure, I can work on a patch for it. The execution flow with the existing code is
> as follows:
> 
> ks_pcie_probe()
>     dw_pcie_host_init()
>         pci_host_probe()
>             ks_pcie_v3_65_add_bus()
> 
> So I assume that as long as ks_pcie_v3_65_add_bus() is invoked after
> pci_host_probe(), it should be acceptable. With this understanding, I plan to
> move it immediately after the invocation to dw_pcie_host_init() within
> ks_pcie_probe() conditionally (based on the is_am6 flag). The new call trace
> with this change will look like:
> 

> ks_pcie_probe()
>     dw_pcie_host_init()
>         pci_host_probe()
>     ks_pcie_v3_65_add_bus()

I guess what Bjorn implied was to add the ks_pcie_v3_65_add_bus()
invocation to the host_init() callback, i.e. in ks_pcie_host_init().
Moreover initializing the BARs/MSI after all the PCIe hierarchy has
been probed will eventually cause problems since MSI's won't work
until the ks_pcie_v3_65_add_bus() is called.

> 
> Since the .add_bus() method will be removed following this change, I suppose
> that the patch I post for it can be applied instead of this v3 patch which fixes
> pci_ops.
> 

> The patch I plan to post as a replacement for the current patch which shall also
> remove the ks_pcie_v3_65_add_bus() and the .add_bus() method is:

Just a note. Seeing the Bjorn's suggestion may cause a regression on
the Keystone PCIe Host v3.65 I would suggest to merge in the original
fix as is and post an additional cleanup patch, like below with proper
modifications, on top of it. Thus we'll minimize a risk to break
things at least on the stable kernels.

-Serge(y)

> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 0def919f89fa..3933e857ecaa 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -447,44 +447,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,
>  };
> 
>  /**
> @@ -1270,6 +1236,29 @@ static int ks_pcie_probe(struct platform_device *pdev)
>  		ret = dw_pcie_host_init(&pci->pp);
>  		if (ret < 0)
>  			goto err_get_sync;
> +
> +		if (!ks_pcie->is_am6) {
> +			/*
> +			 * For v3.65 DWC PCIe Controllers, setup BAR0 to enable
> +			 * inbound access for MSI_IRQ register.
> +			 */
> +
> +			/* 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);
> +		}
> +
>  		break;
>  	case DW_PCIE_EP_TYPE:
>  		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_EP)) {
> 
> Please review and let me know if this looks fine. If so, I will post the patch for it.
> 
> -- 
> Regards,
> Siddharth.