Re: [RFC PATCH] PCI: keystone: Refactor ks_pcie_v3_65_add_bus() functionality

[Serge Semin]

On Fri, Oct 27, 2023 at 02:11:59PM +0530, Siddharth Vadapalli wrote:
> The .add_bus() callback "ks_pcie_v3_65_add_bus()" exists to setup BAR0 for
> MSI configuration. This method is expected to be invoked after the
> enumeration of endpoints for the v3.65a DWC PCIe IP Controller.
> 
> Based on the discussion at [0], the contents of "ks_pcie_v3_65_add_bus()"
> can be moved to the .host_init callback "ks_pcie_host_init()" and the
> .add_bus callback within "struct pci_ops ks_pcie_ops" is no longer
> required.
> 
> Hence, for the v3.65a DWC PCIe IP Controllers (!ks_pcie->is_am6), perform
> the MSI specific configuration of BAR0 within "ks_pcie_host_init()"
> itself.
> 
> [0] https://lore.kernel.org/r/20231019220847.GA1413474@bhelgaas/
> 
> Suggested-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> Suggested-by: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> ---
> Hello,
> 
> This patch is based on linux-next tagged next-20231027.
> This patch depends on the patch at:
> https://lore.kernel.org/r/20231019081330.2975470-1-s-vadapalli@xxxxxx/
> 
> Regards,
> Siddharth.
> 
>  drivers/pci/controller/dwc/pci-keystone.c | 51 ++++++++---------------
>  1 file changed, 17 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index e9245b7632c5..369f5e556df3 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,
>  };
>  
>  static struct pci_ops ks_pcie_am6_ops = {
> @@ -834,6 +800,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret < 0)
>  		return ret;
>  

> +	if (!ks_pcie->is_am6) {
> +		/* 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);
> +	}
> +

Seeing this is required for MSI IRQs what about moving it to the
ks_pcie_config_msi_irq() together with the BARs zeroing out performed
in the ks_pcie_setup_rc_app_regs() function especially seeing the
later function is dedicated for the 'app' regs initialization only
based on the function name. Bjorn, what do you think?

-Serge(y)

>  #ifdef CONFIG_ARM
>  	/*
>  	 * PCIe access errors that result into OCP errors are caught by ARM as
> -- 
> 2.34.1
>