Re: [PATCH v9 3/4] PCI: qcom: Prepare for the DWC ECAM enablement

[Bjorn Helgaas <helgaas@kernel.org>]

Sorry, I missed your repost of this series, Mani.  I'll reiterate my
questions here.

I also deleted the pci/controller/dwc-ecam branch, where Krishna's v8
series was queued up, to avoid confusion (it looked like that branch
was ready to be included in linux-next, but it's not).

On Tue, Sep 09, 2025 at 12:37:52PM +0530, Manivannan Sadhasivam wrote:
> From: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> 
> To support the DWC ECAM mechanism, prepare the driver by performing below
> configurations:
> 
>   1. Since the ELBI region will be covered by the ECAM 'config' space,
>      override the 'elbi_base' with the address derived from 'dbi_base' and
>      the offset from PARF_SLV_DBI_ELBI register.
> 
>   2. Block the transactions from the host bridge to devices other than Root
>      Port on the root bus to return all F's. This is required when the 'CFG
>      Shift Feature' of iATU is enabled.

> +++ b/drivers/pci/controller/dwc/pcie-qcom.c

> +static void qcom_pci_config_ecam(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u64 addr, addr_end;
> +	u32 val;
> +
> +	writel_relaxed(lower_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE);
> +	writel_relaxed(upper_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE_HI);
> +
> +	/*
> +	 * The only device on root bus is a single Root Port. So if PCI core
> +	 * tries to access any devices other than Device/Function (0.0) in Bus
> +	 * 0, the TLP will go outside of the controller to the PCI bus. But with
> +	 * CFG Shift Feature (ECAM) enabled in iATU, there is no guarantee that
> +	 * the response is going to be all F's. Hence, to make sure that the
> +	 * requester gets all F's response for accesses other than the Root
> +	 * Port, configure iATU to block the transactions starting from function
> +	 * 1 of the root bus to the end of the root bus (i.e from dbi_base + 4kb
> +	 * to dbi_base + 1MB).
> +	 */
> +	addr = pci->dbi_phys_addr + SZ_4K;
> +	writel_relaxed(lower_32_bits(addr), pcie->parf + PARF_BLOCK_SLV_AXI_WR_BASE);
> +	writel_relaxed(upper_32_bits(addr), pcie->parf + PARF_BLOCK_SLV_AXI_WR_BASE_HI);
> +
> +	writel_relaxed(lower_32_bits(addr), pcie->parf + PARF_BLOCK_SLV_AXI_RD_BASE);
> +	writel_relaxed(upper_32_bits(addr), pcie->parf + PARF_BLOCK_SLV_AXI_RD_BASE_HI);
> +
> +	addr_end = pci->dbi_phys_addr + SZ_1M - 1;
> +
> +	writel_relaxed(lower_32_bits(addr_end), pcie->parf + PARF_BLOCK_SLV_AXI_WR_LIMIT);
> +	writel_relaxed(upper_32_bits(addr_end), pcie->parf + PARF_BLOCK_SLV_AXI_WR_LIMIT_HI);
> +
> +	writel_relaxed(lower_32_bits(addr_end), pcie->parf + PARF_BLOCK_SLV_AXI_RD_LIMIT);
> +	writel_relaxed(upper_32_bits(addr_end), pcie->parf + PARF_BLOCK_SLV_AXI_RD_LIMIT_HI);
> +
> +	val = readl_relaxed(pcie->parf + PARF_SYS_CTRL);
> +	val |= PCIE_ECAM_BLOCKER_EN;
> +	writel_relaxed(val, pcie->parf + PARF_SYS_CTRL);

The driver already supported ECAM in the existing "firmware_managed"
path (which looks untouched by this series and doesn't do any of this
iATU configuration).

And IIUC, this series adds support for ECAM whenever the DT 'config'
range is sufficiently aligned.  In this new ECAM support, it looks
like we look for and pay attention to 'bus-range' in this path:

  qcom_pcie_probe
    dw_pcie_host_init
      devm_pci_alloc_host_bridge
        devm_of_pci_bridge_init
          pci_parse_request_of_pci_ranges
            devm_of_pci_get_host_bridge_resources
              of_pci_parse_bus_range
                of_property_read_u32_array(node, "bus-range", ...)
      dw_pcie_host_get_resources
        res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config")
        pp->ecam_enabled = dw_pcie_ecam_enabled(pp, res)

Since qcom_pci_config_ecam() doesn't look at the root bus number at
all, is this also an implicit restriction that the root bus must be
bus 0?  Does qcom support root buses other than 0?

> +}
> +
>  static int qcom_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> @@ -326,6 +382,9 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>  		qcom_pcie_common_set_16gt_lane_margining(pci);
>  	}
>  
> +	if (pci->pp.ecam_enabled)
> +		qcom_pci_config_ecam(&pci->pp);

qcom_pcie_start_link() seems like a strange place to do this
ECAM-related iATU configuration.  ECAM is a function of the host
bridge, not of any particular Root Port or link.

>  	/* Enable Link Training state machine */
>  	if (pcie->cfg->ops->ltssm_enable)
>  		pcie->cfg->ops->ltssm_enable(pcie);