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

Krishna Chaitanya Chundru
2024-12-09T04:40:11Z

On 12/5/2024 4:10 AM, Bjorn Helgaas wrote:
> 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:
It makes sense to use this way in the next patch in the qcom driver will
call devm_pci_alloc_host_bridge() before calling dw_pcie_host_init() and
in dw_pcie_host_init() if the bridge is allocated dwc driver will skip
allocating the bridge so that other drivers will not be affected.

- Krishna Chaitanya.

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