Re: [PATCH v2 4/5] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Bjorn Helgaas]

On Tue, Sep 16, 2025 at 06:04:16PM +0800, Randolph Lin wrote:
> Add driver support for DesignWare based PCIe controller in Andes
> QiLai SoC. The driver only supports the Root Complex mode.

> +config PCIE_ANDES_QILAI
> +	bool "ANDES QiLai PCIe controller"
> +	depends on OF && (RISCV || COMPILE_TEST)
> +	depends on PCI_MSI
> +	depends on ARCH_ANDES

This prevents a lot of compile testing.  AFAICT, no other controller
depends directly on the arch.  Most do something like these:

  depends on MACH_ARTPEC6 || COMPILE_TEST
  depends on ARCH_MXC || COMPILE_TEST
  depends on OF && (ARM || ARCH_LAYERSCAPE || COMPILE_TEST)

> +	select PCIE_DW_HOST
> +	help
> +          Say Y here to enable PCIe controller support on Andes QiLai SoCs,
> +	  which operate in Root Complex mode. The Andes QiLai SoCs PCIe
> +	  controller is based on DesignWare IP (5.97a version) and therefore
> +	  the driver re-uses the DesignWare core functions to implement the
> +	  driver. The Andes QiLai SoC has three Root Complexes (RCs): one
> +	  operates on PCIe 4.0 with 4 lanes at 0x80000000, while the other
> +	  two operate on PCIe 4.0 with 2 lanes at 0xA0000000 and 0xC0000000,
> +	  respectively.

I assume these addresses come from devicetree, so I don't think
there's any need to include them here.

Fix space/tab indentation issue on first line of help text.  Do the
indentation the same way as the rest of the file.

> + * Refer to Table A4-5 (Memory type encoding) in the
> + * AMBA AXI and ACE Protocol Specification.
> + * The selected value corresponds to the Memory type field:
> + * "Write-back, Read and Write-allocate".

Add blank line between paragraphs or rewrap into a single paragraph.

> +static
> +bool qilai_pcie_outbound_atu_addr_valid(struct dw_pcie *pci,
> +					const struct dw_pcie_ob_atu_cfg *atu,
> +					u64 *limit_addr)
> +{
> +	u64 parent_bus_addr = atu->parent_bus_addr;
> +
> +	*limit_addr = parent_bus_addr + atu->size - 1;
> +
> +	/*
> +	 * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
> +	 * only need to ensure addresses below 4 GB match pci->region_limit.
> +	 */
> +	if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
> +	    lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
> +	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> +	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
> +		return false;

Seems a little bit strange.  Is this something that could be expressed
via devicetree?  Or something peculiar about QiLai that's different
from all the other DWC-based controllers?

> + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> + * Write-Back, Read and Write Allocate mode.
> + * The IOCP HW target is SoC last-level cache (L2 Cache), which serves as the
> + * system cache.
> + * The IOCP HW helps maintain cache monitoring, ensuring that the device can
> + * snoop data from/to the cache.

Add blank lines between paragraphs (or rewrap into a single paragraph
if that's what you intend).

> +static struct platform_driver qilai_pcie_driver = {
> +	.probe = qilai_pcie_probe,
> +	.driver = {
> +		.name	= "qilai-pcie",
> +		.of_match_table = qilai_pcie_of_match,
> +		/* only test passed at PROBE_DEFAULT_STRATEGY */
> +		.probe_type = PROBE_DEFAULT_STRATEGY,

This is the only use of PROBE_DEFAULT_STRATEGY in the entire tree, so
I doubt you need it.  If you do, please explain why in more detail.

Bjorn