Re: [PATCH v8 5/5] PCI: qcom: Add support for ECAM feature

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 9/13/2025 2:37 AM, Bjorn Helgaas wrote:
> On Wed, Sep 03, 2025 at 02:57:21PM -0500, Bjorn Helgaas wrote:
>> On Thu, Aug 28, 2025 at 01:04:26PM +0530, Krishna Chaitanya Chundru wrote:
>>> The ELBI registers falls after the DBI space, PARF_SLV_DBI_ELBI register
>>> gives us the offset from which ELBI starts. So override ELBI with the
>>> offset from PARF_SLV_DBI_ELBI and cfg win to map these regions.
>>>
>>> On root bus, we have only the root port. Any access other than that
>>> should not go out of the link and should return all F's. Since the iATU
>>> is configured for the buses which starts after root bus, 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) from going
>>> outside the link through ECAM blocker through PARF registers.
> 
>>> +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;
>>> +
>>> +	/* Set the ECAM base */
>>> +	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 the Root Port. Any access to the PCIe
>>> +	 * region will go outside the PCIe link. As part of enumeration the PCI
>>> +	 * sw can try to read to vendor ID & device ID with different device
>>> +	 * number and function number under root bus. As any access other than
>>> +	 * root bus, device 0, function 0, should not go out of the link and
>>> +	 * should return all F's. Since the iATU is configured for the buses
>>> +	 * which starts after root bus, 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) from going outside the link.
>>> +	 */
>>> +	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;
>>
>> I guess this is an implicit restriction to a single Root Port on the
>> root bus at bb:00.0, right?  So when the qcom IP eventually supports
>> multiple Root Ports or even a single Root Port at a different
>> device/function number, this would have to be updated somehow?
> 
> 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).
> 
The firmware_manages doesn't use dwc way of reading/writing to the
config space, except for MSI's that doen't touch dwc core.

> 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?
> 
QCOM supports only bus 0.
>>> +	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);
>>> +}
>>> +
>>>   static int qcom_pcie_start_link(struct dw_pcie *pci)
>>>   {
>>>   	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>>> @@ -326,6 +383,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.
> 
There is no API in pci-qcom.c related to the host bridge configuration
currently, as we want to configure before enumeration starts we added
it here, we can move this to qcom_pcie_host_init() if you are ok with
it.

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