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

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Sep 15, 2025 at 12:48:06PM +0530, Krishna Chaitanya Chundru wrote:
> 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:

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

Since of_pci_parse_bus_range() reads the bus range from DT, is there a
place that validates that the root bus is 0?

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

Sounds like a better place to me.

Bjorn