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

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Sep 05, 2025 at 10:08:54PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Sep 05, 2025 at 10:47:42AM GMT, Krishna Chaitanya Chundru wrote:
> > On 9/4/2025 1:42 AM, 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.
> > > > 
> > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 70 ++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 70 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 5092752de23866ef95036bb3f8fae9bb06e8ea1e..8f3c86c77e2604fd7826083f63b66b4cb62a341d 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -55,6 +55,7 @@
> > > >   #define PARF_AXI_MSTR_WR_ADDR_HALT_V2		0x1a8
> > > >   #define PARF_Q2A_FLUSH				0x1ac
> > > >   #define PARF_LTSSM				0x1b0
> > > > +#define PARF_SLV_DBI_ELBI			0x1b4
> > > >   #define PARF_INT_ALL_STATUS			0x224
> > > >   #define PARF_INT_ALL_CLEAR			0x228
> > > >   #define PARF_INT_ALL_MASK			0x22c
> > > > @@ -64,6 +65,16 @@
> > > >   #define PARF_DBI_BASE_ADDR_V2_HI		0x354
> > > >   #define PARF_SLV_ADDR_SPACE_SIZE_V2		0x358
> > > >   #define PARF_SLV_ADDR_SPACE_SIZE_V2_HI		0x35c
> > > > +#define PARF_BLOCK_SLV_AXI_WR_BASE		0x360
> > > > +#define PARF_BLOCK_SLV_AXI_WR_BASE_HI		0x364
> > > > +#define PARF_BLOCK_SLV_AXI_WR_LIMIT		0x368
> > > > +#define PARF_BLOCK_SLV_AXI_WR_LIMIT_HI		0x36c
> > > > +#define PARF_BLOCK_SLV_AXI_RD_BASE		0x370
> > > > +#define PARF_BLOCK_SLV_AXI_RD_BASE_HI		0x374
> > > > +#define PARF_BLOCK_SLV_AXI_RD_LIMIT		0x378
> > > > +#define PARF_BLOCK_SLV_AXI_RD_LIMIT_HI		0x37c
> > > > +#define PARF_ECAM_BASE				0x380
> > > > +#define PARF_ECAM_BASE_HI			0x384
> > > >   #define PARF_NO_SNOOP_OVERRIDE			0x3d4
> > > >   #define PARF_ATU_BASE_ADDR			0x634
> > > >   #define PARF_ATU_BASE_ADDR_HI			0x638
> > > > @@ -87,6 +98,7 @@
> > > >   /* PARF_SYS_CTRL register fields */
> > > >   #define MAC_PHY_POWERDOWN_IN_P2_D_MUX_EN	BIT(29)
> > > > +#define PCIE_ECAM_BLOCKER_EN			BIT(26)
> > > >   #define MST_WAKEUP_EN				BIT(13)
> > > >   #define SLV_WAKEUP_EN				BIT(12)
> > > >   #define MSTR_ACLK_CGC_DIS			BIT(10)
> > > > @@ -134,6 +146,9 @@
> > > >   /* PARF_LTSSM register fields */
> > > >   #define LTSSM_EN				BIT(8)
> > > > +/* PARF_SLV_DBI_ELBI */
> > > > +#define SLV_DBI_ELBI_ADDR_BASE			GENMASK(11, 0)
> > > > +
> > > >   /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
> > > >   #define PARF_INT_ALL_LINK_UP			BIT(13)
> > > >   #define PARF_INT_MSI_DEV_0_7			GENMASK(30, 23)
> > > > @@ -317,6 +332,48 @@ static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> > > >   	qcom_perst_assert(pcie, false);
> > > >   }
> > > > +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;
> > > > +
> > > > +	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);
> > > > +
> > > >   	/* Enable Link Training state machine */
> > > >   	if (pcie->cfg->ops->ltssm_enable)
> > > >   		pcie->cfg->ops->ltssm_enable(pcie);
> > > > @@ -1314,6 +1374,7 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > > >   {
> > > >   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > >   	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> > > > +	u16 offset;
> > > >   	int ret;
> > > >   	qcom_ep_reset_assert(pcie);
> > > > @@ -1322,6 +1383,15 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > > >   	if (ret)
> > > >   		return ret;
> > > > +	if (pp->ecam_enabled) {
> > > > +		/*
> > > > +		 * Override ELBI when ECAM is enabled, as when ECAM
> > > > +		 * is enabled ELBI moves along with the dbi config space.
> > > > +		 */
> > > > +		offset = FIELD_GET(SLV_DBI_ELBI_ADDR_BASE, readl(pcie->parf + PARF_SLV_DBI_ELBI));
> > > > +		pci->elbi_base = pci->dbi_base + offset;
> > > 
> > > This looks like there might be a bisection hole between this
> > > patch and the previous patch that enables ECAM in the DWC core?
> > > Obviously I would want to avoid a bisection hole.
> 
> Theoretically there is a hole, but practically there isn't. ELBI
> register is only used by legacy Qcom SoCs and they do not support
> ECAM. So they will continue to use the valid ELBI register region
> from DT.

That's a real pain to figure out from code analysis, so I would prefer
to squash them to avoid even the appearance of a bisection hole.

> > > What happens to qcom ELBI accesses between these two patches?
> > > It looks like they would go to the wrong address until this
> > > elbi_base update.
> 
> Though there is no practical issue, it still makes sense to set
> 'elbi_base' to a valid region before enabling ECAM.