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

[Bjorn Helgaas]

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.

s/ecam/ECAM/
s/dbi/DBI/
s/IATU/iATU/ (Seems to be the convention?  Also below)
s/parf/PARF/ (I assume an initialism?)

Use conventional format for PCI bus addresses ... I assume "0:0:1"
means bus 0, device 0, function 1, which would normally be formatted
as "00:00.1" (also below).

> +static int qcom_pci_config_ecam_blocker(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(lower_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE);
> +	writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE_HI);
> +
> +	/*
> +	 * 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.

s/IATU/iATU/ to match other usage.

Use conventional formatting of PCI bus/device/function addresses.

Unless the root bus number is hard-wired to be zero, maybe this should
say "root bus" instead of "bus 0"?

There is no architected presence of a PCIe Root Complex as a PCI
device.  Maybe this should say "the only device on bus 0 is the *Root
Port*"?

Or maybe there's a PCI device with some sort of device-specific
interface to Root Complex registers?  But if that were the *only*
device on bus 0, there would be no Root Port to reach other devices,
so this doesn't seem likely.

> +static bool qcom_pcie_check_ecam_support(struct device *dev)

Rename to be an assertion that can be either true or false, e.g.,
"ecam_supported".  "Check" doesn't hint about what true/false mean.

> +{
> +	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]?

> +	config_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> +	if (!config_res)
> +		return false;

Move of_pci_parse_bus_range() (if it's needed) down here so it's
together with the use of the results.  No point in calling it before
looking for "config".

> +	bus_config_space_count = resource_size(config_res) >> PCIE_ECAM_BUS_SHIFT;
> +	if (resource_size(&bus_range) > bus_config_space_count)
> +		return false;
> +
> +	return true;
> +}