Re: [PATCH 3/3] PCI: qcom: Enable ECAM feature based on config size

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Dec 04, 2024 at 07:56:54AM +0530, Krishna Chaitanya Chundru wrote:
> On 12/4/2024 12:29 AM, Bjorn Helgaas wrote:
> > On Sun, Nov 17, 2024 at 03:30:20AM +0530, Krishna chaitanya chundru wrote:
> > > Enable the ECAM feature if the config space size is equal to size required
> > > to represent number of buses in the bus range property.
> > > 
> > > The ELBI registers falls after the DBI space, so use the cfg win returned
> > > from the ecam init to map these regions instead of doing the ioremap again.
> > > ELBI starts at offset 0xf20 from dbi.
> > > 
> > > On bus 0, we have only the root complex. Any access other than that should
> > > not go out of the link and should return all F's. Since the IATU is
> > > configured for bus 1 onwards, block the transactions for bus 0:0:1 to
> > > 0:31:7 (i.e., from dbi_base + 4KB to dbi_base + 1MB) from going outside the
> > > link through ecam blocker through parf registers.

> > > +static bool qcom_pcie_check_ecam_support(struct device *dev)
> > > +{
> > > +	struct platform_device *pdev = to_platform_device(dev);
> > > +	struct resource bus_range, *config_res;
> > > +	u64 bus_config_space_count;
> > > +	int ret;
> > > +
> > > +	/* If bus range is not present, keep the bus range as maximum value */
> > > +	ret = of_pci_parse_bus_range(dev->of_node, &bus_range);
> > > +	if (ret) {
> > > +		bus_range.start = 0x0;
> > > +		bus_range.end = 0xff;
> > > +	}
> > 
> > I would have thought the generic OF parsing would already default to
> > [bus 00-ff]?
> > 
> if there is no bus-range of_pci_parse_bus_range is not updating it[1],
> the bus ranges is being updated to default value in
> devm_of_pci_get_host_bridge_resources()[2]

Understood.  But qcom uses dw_pcie_host_init(), which calls
devm_pci_alloc_host_bridge(), which ultimately calls
of_pci_parse_bus_range() and defaults to [bus 00-ff] if there's no
bus-range in DT:

  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(0, 0xff)
              of_pci_parse_bus_range

So the question is why you need to do that again here.

I see that qcom_pcie_probe() calls qcom_pcie_check_ecam_support()
*before* it calls dw_pcie_host_init(), so I guess that's the immediate
answer.

But this is another reason why I think qcom_pcie_check_ecam_support()
is kind of a sub-optimal solution here.

I wonder if we should factor the devm_pci_alloc_host_bridge() call out
of dw_pcie_host_init() so drivers can take advantage of the DT parsing
it does.  It looks like mobiveil does it that way:

  ls_g4_pcie_probe
    devm_pci_alloc_host_bridge
    mobiveil_pcie_host_probe

  mobiveil_pcie_probe
    devm_pci_alloc_host_bridge
    mobiveil_pcie_host_probe

> [1]https://elixir.bootlin.com/linux/v6.12.1/source/drivers/pci/of.c#L193
> [2]https://elixir.bootlin.com/linux/v6.12.1/source/drivers/pci/of.c#L347