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.