linux-kernel - Re: [PATCH v4 8/9] PCI: aspeed: Add ASPEED PCIe RC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251028171330.GA1506282@bhelgaas>
Date: Tue, 28 Oct 2025 12:13:30 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org,
	robh@...nel.org, bhelgaas@...gle.com, krzk+dt@...nel.org,
	conor+dt@...nel.org, joel@....id.au, andrew@...econstruct.com.au,
	vkoul@...nel.org, kishon@...nel.org, linus.walleij@...aro.org,
	p.zabel@...gutronix.de, linux-aspeed@...ts.ozlabs.org,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-phy@...ts.infradead.org, openbmc@...ts.ozlabs.org,
	linux-gpio@...r.kernel.org
Subject: Re: [PATCH v4 8/9] PCI: aspeed: Add ASPEED PCIe RC driver

On Mon, Oct 27, 2025 at 05:58:24PM +0800, Jacky Chou wrote:
> Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> initialization, reset, clock, IRQ domain, and MSI domain setup.
> Implement platform-specific setup and register configuration for
> ASPEED. And provide PCI config space read/write and INTx/MSI
> interrupt handling.

> +config PCIE_ASPEED
> +	bool "ASPEED PCIe controller"
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	depends on OF
> +	depends on PCI_MSI
> +	select IRQ_MSI_LIB
> +	help
> +	  Enable this option to support the PCIe controller found on ASPEED
> +	  SoCs.
> +
> +	  This driver provides initialization and management for PCIe
> +	  Root Complex functionality, including interrupt and MSI support.

Maybe "INTx and MSI support", since MSI is an interrupt?

> +/* Complete status */

"Completion"

> +static int aspeed_ast2700_ahb_remap_to_bar(struct aspeed_pcie *pcie)
> +{
> +	struct resource_entry *win, *tmp;
> +	struct pci_host_bridge *bridge = pcie->host;
> +
> +	/* Configure AHB remapping to BAR on AST27x0.
> +	 * The BAR region is HW-fixed in AST27x0, these BARs will be filled
> +	 * in the ranges of pcie node in DT.
> +	 */

I don't understand what "HW-fixed" means here.  It looks like you're
writing host bridge window addresses (that came from DT) to the
hardware.  That sounds like they're not actually "fixed" but
programmable.

Host bridge windows are not BARs themselves.  Mem space for devices
below the host bridge is allocated from the windows, and the addresses
are programmed into BARs of those downstream devices.

Multi-line comment style:

  /*
   * Configure ...
   */

Wrap to fill 78 columns, or add blank lines between paragraphs.

> +	resource_list_for_each_entry_safe(win, tmp, &bridge->windows) {
> +		struct resource *res = win->res;
> +
> +		if (resource_type(res) == IORESOURCE_MEM &&
> +		    !(res->flags & IORESOURCE_MEM_64)) {
> +			writel(ASPEED_REMAP_BAR_BASE(res->start),
> +			       pcie->reg + ASPEED_H2X_REMAP_DIRECT_ADDR);
> +			return 0;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int aspeed_ast2700_setup(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +	struct device *dev = pcie->dev;
> +	int ret;
> +
> +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node,
> +						    "aspeed,pciecfg");
> +	if (IS_ERR(pcie->cfg))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg),
> +				     "failed to map pciecfg base\n");
> +
> +	regmap_update_bits(pcie->cfg, ASPEED_SCU_60,
> +			   ASPEED_RC_E2M_PATH_EN | ASPEED_RC_H2XS_PATH_EN |
> +			   ASPEED_RC_H2XD_PATH_EN | ASPEED_RC_H2XX_PATH_EN |
> +			   ASPEED_RC_UPSTREAM_MEM_EN,
> +			   ASPEED_RC_E2M_PATH_EN | ASPEED_RC_H2XS_PATH_EN |
> +			   ASPEED_RC_H2XD_PATH_EN | ASPEED_RC_H2XX_PATH_EN |
> +			   ASPEED_RC_UPSTREAM_MEM_EN);
> +	regmap_write(pcie->cfg, ASPEED_SCU_64,
> +		     ASPEED_RC0_DECODE_DMA_BASE(0) |
> +		     ASPEED_RC0_DECODE_DMA_LIMIT(0xff) |
> +		     ASPEED_RC1_DECODE_DMA_BASE(0) |
> +		     ASPEED_RC1_DECODE_DMA_LIMIT(0xff));
> +	regmap_write(pcie->cfg, ASPEED_SCU_70, ASPEED_DISABLE_EP_FUNC);
> +
> +	aspeed_host_reset(pcie);
> +
> +	writel(0, pcie->reg + ASPEED_H2X_CTRL);
> +	writel(ASPEED_H2X_BRIDGE_EN | ASPEED_H2X_BRIDGE_DIRECT_EN,
> +	       pcie->reg + ASPEED_H2X_CTRL);
> +
> +	ret = aspeed_ast2700_ahb_remap_to_bar(pcie);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to assign BAR\n");

This is not assigning *BARs*.  A host bridge doesn't have BARs in the
PCI spec sense.  It might have programmable address ranges, but the
host bridge is not itself a PCI device, so its programmability is
device specific.