Message-ID: <hxaj7c6siernov3qb36rmxpcpdbq6gilaejc27eij4iyqdzuzc@jt76e6vdkbdo>
Date: Mon, 15 Sep 2025 20:02:41 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Jingoo Han <jingoohan1@...il.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Krzysztof Kozlowski <krzk@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>, 
	Jonathan Chocron <jonnyc@...zon.com>, linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Subject: Re: [PATCH v9 3/4] PCI: qcom: Prepare for the DWC ECAM enablement

On Fri, Sep 12, 2025 at 04:44:32PM GMT, Bjorn Helgaas wrote:
> 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@....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).
> 

'firmware_managed' refers to the Qcom platforms supporting BIOS/bootloader that
has initialized the PCIe controller and configured ECAM mode. The driver doesn't
need to do any resource handling on its own including iATU as everything would
be handled by the BIOS/bootloader. This mode is used by the Qcom Automotive
SoCs.

And This mode is different from the DWC ECAM mode implemented in this series as
this one requires doing all iATU configuration in the driver itself. This mode
will be used by the Mobile/IoT/Compute SoCs.

Qcom SoCs have different requirement for different market segments. This is the
reason why we have many different implementations.

> 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?
> 

No, not at the moment.

> > +}
> > +
> >  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.
> 

I thought qcom_pci_config_ecam() needs to be called after
dw_pcie_config_ecam_iatu() and before pci_host_probe() in dw_pcie_host_init().
Since there were no other callbacks in-between other than this start_link()
callback, Krishna has called it from here.

But looks like qcom_pci_config_ecam() could be called before
dw_pcie_config_ecam_iatu() as replied by Krishna [1]. So I will move it to
qcom_pcie_host_init().

- Mani

[1] https://lore.kernel.org/linux-pci/d12e002b-e99e-4963-a732-4873e13c5419@oss.qualcomm.com

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists