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.
