Re: [PATCH v7 2/2] PCI: keystone: Fix pci_ops for AM654x SoC

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Mar 28, 2024 at 02:20:41PM +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 is applicable to the controller version
> 4.90a which is present in AM654x SoCs.
> 
> Thus, as a fix, remove "ks_pcie_v3_65_add_bus()" and move its contents
> to the .msi_init callback "ks_pcie_msi_host_init()" which is specific to
> the 3.65a controller.
> 
> 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>
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Thanks for splitting this into two patches.  Krzysztof has applied
both to pci/controller/keystone and we hope to merge them for v6.10.

I *would* like the commit log to be at a little higher level if
possible.  Right now it's a detailed description at the level of the
code edits, but it doesn't say *why* we want this change.

I think the first cut at this was
https://lore.kernel.org/linux-pci/20231011123451.34827-1-s-vadapalli@ti.com/t/#u,
which mentioned Completion Timeouts during MSI-X configuration and 45
second delays during boot.

IIUC, prior to 6ab15b5e7057, ks_pcie_v3_65_scan_bus() initialized BAR
0 and was only used for v3.65a devices.  6ab15b5e7057 renamed it to
ks_pcie_v3_65_add_bus() and called it for both v3.65a and v4.90a.

I think the problem is that in the current code, the
ks_pcie_ops.add_bus() method (ks_pcie_v3_65_add_bus()) is used for all
devices (both v3.65a and v4.90a).  So I guess doing the BAR 0 setup on
v4.90a broke something there?

I'm not quite clear on the mechanism, but it would be helpful to at
least know what's wrong and on what platform.  E.g., currently v4.90
suffers Completion Timeouts and 45 second boot delays?  And this patch
fixes that?

> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 52 ++++++++---------------
>  1 file changed, 18 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 5c073e520628..6cb3a4713009 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -289,6 +289,24 @@ static void ks_pcie_clear_dbi_mode(struct keystone_pcie *ks_pcie)
>  
>  static int ks_pcie_msi_host_init(struct dw_pcie_rp *pp)
>  {
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
> +
> +	/* 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);
> +
>  	pp->msi_irq_chip = &ks_pcie_msi_irq_chip;
>  	return dw_pcie_allocate_domains(pp);
>  }
> @@ -445,44 +463,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,
>  };
>  
>  /**
> -- 
> 2.40.1
>