Re: [PATCH v3] PCI: keystone: Fix pci_ops for AM654x SoC

[Siddharth Vadapalli]

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()

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:

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.