linux-kernel - 回覆: [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] [day] [month] [year] [list]
Message-ID:
 <SEYPR06MB51340DD75653EAC61D724F589DFBA@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Thu, 30 Oct 2025 05:53:55 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "lpieralisi@...nel.org" <lpieralisi@...nel.org>, "kwilczynski@...nel.org"
	<kwilczynski@...nel.org>, "mani@...nel.org" <mani@...nel.org>,
	"robh@...nel.org" <robh@...nel.org>, "bhelgaas@...gle.com"
	<bhelgaas@...gle.com>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "joel@....id.au"
	<joel@....id.au>, "andrew@...econstruct.com.au"
	<andrew@...econstruct.com.au>, "vkoul@...nel.org" <vkoul@...nel.org>,
	"kishon@...nel.org" <kishon@...nel.org>, "linus.walleij@...aro.org"
	<linus.walleij@...aro.org>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "linux-gpio@...r.kernel.org"
	<linux-gpio@...r.kernel.org>
Subject:
 回覆: [PATCH v4 8/9] PCI: aspeed: Add ASPEED PCIe RC driver

Hi Bjorn,

Thank you for your reply.

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

Agreed.

> > +/* Complete status */
> 
> "Completion"
> 

Agreed.

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

I checked our design, and there were errors in my description and usage.
I want to configure the "PCI address" from the range property of dts.
Therefore, I also get the wrong address, here will get the soc view address.
I will modify our code and my description in next version.

> Multi-line comment style:
> 
>   /*
>    * Configure ...
>    */
> 
> Wrap to fill 78 columns, or add blank lines between paragraphs.
> 

Agreed.

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

As you said, host bridge doesn't have BARs.
Here I want to configure the "PCI address" to our design.
Will modify this in next version.

Thanks,
Jacky