Re: [PATCH v6 6/7] PCI: aspeed: Add ASPEED PCIe RC driver

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Dec 01, 2025 at 02:29:16PM +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.
> ...

> +struct aspeed_pcie {
> +	struct pci_host_bridge *host;
> +	struct device *dev;
> +	void __iomem *reg;
> +	struct regmap *ahbc;
> +	struct regmap *cfg;
> +	const struct aspeed_pcie_rc_platform *platform;
> +	struct list_head ports;
> +
> +	u8 tx_tag;
> +	int host_bus_num;

Only needs a u8.

> +static int aspeed_pcie_port_init(struct aspeed_pcie_port *port)
> +{
> +	struct aspeed_pcie *pcie = port->pcie;
> +	struct device *dev = pcie->dev;
> +	int ret;
> +
> +	ret = clk_prepare_enable(port->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to set clock for slot (%d)\n",
> +				     port->slot);
> +
> +	ret = phy_init(port->phy);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to init phy pcie for slot (%d)\n",
> +				     port->slot);
> +
> +	ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to set phy mode for slot (%d)\n",
> +				     port->slot);
> +
> +	reset_control_deassert(port->perst);
> +	mdelay(PCIE_RESET_CONFIG_WAIT_MS);

I think this should use msleep().

> +static int aspeed_ast2600_setup(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +	struct device *dev = pcie->dev;
> +
> +	pcie->ahbc = syscon_regmap_lookup_by_phandle(dev->of_node,
> +						     "aspeed,ahbc");
> +	if (IS_ERR(pcie->ahbc))
> +		return dev_err_probe(dev, PTR_ERR(pcie->ahbc),
> +				     "failed to map ahbc base\n");
> +
> +	aspeed_host_reset(pcie);
> +
> +	regmap_write(pcie->ahbc, ASPEED_AHBC_KEY, ASPEED_AHBC_UNLOCK_KEY);
> +	regmap_update_bits(pcie->ahbc, ASPEED_AHBC_ADDR_MAPPING,
> +			   ASPEED_PCIE_RC_MEMORY_EN, ASPEED_PCIE_RC_MEMORY_EN);
> +	regmap_write(pcie->ahbc, ASPEED_AHBC_KEY, ASPEED_AHBC_UNLOCK);
> +
> +	/* Due to the BAR assignment is fixed mapping on 0x6000_0000.*/
> +	writel(ASPEED_AHB_REMAP_LO_ADDR(0x600) | ASPEED_AHB_MASK_LO_ADDR(0xe00),
> +	       pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG0);
> +	writel(ASPEED_AHB_REMAP_HI_ADDR(0),
> +	       pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG1);

I assume this ASPEED_H2X_AHB_ADDR_CONFIG is doing basically the same
thing as aspeed_ast2700_remap_pci_addr() below, so see the comments
there.

> +	writel(ASPEED_AHB_MASK_HI_ADDR(~0),
> +	       pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG2);
> +	writel(ASPEED_H2X_BRIDGE_EN, pcie->reg + ASPEED_H2X_CTRL);
> +
> +	writel(ASPEED_PCIE_RX_DMA_EN | ASPEED_PCIE_RX_LINEAR |
> +	       ASPEED_PCIE_RX_MSI_SEL | ASPEED_PCIE_RX_MSI_EN |
> +	       ASPEED_PCIE_WAIT_RX_TLP_CLR | ASPEED_PCIE_RC_RX_ENABLE |
> +	       ASPEED_PCIE_RC_ENABLE,
> +	       pcie->reg + ASPEED_H2X_DEV_CTRL);
> +
> +	writel(ASPEED_RC_TLP_TX_TAG_NUM, pcie->reg + ASPEED_H2X_DEV_TX_TAG);
> +
> +	pcie->host->ops = &aspeed_ast2600_pcie_ops;
> +	pcie->host->child_ops = &aspeed_ast2600_pcie_child_ops;
> +
> +	return 0;
> +}
> +
> +static int aspeed_ast2700_remap_pci_addr(struct aspeed_pcie *pcie)
> +{
> +	struct device_node *dev_node = pcie->dev->of_node;
> +	struct of_pci_range range;
> +	struct of_pci_range_parser parser;
> +	int ret;
> +
> +	ret = of_pci_range_parser_init(&parser, dev_node);
> +	if (ret)
> +		return ret;
> +
> +	for_each_of_pci_range(&parser, &range) {
> +		if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) {
> +			writel(ASPEED_REMAP_PCI_ADDR_31_12(range.pci_addr),
> +			       pcie->reg + ASPEED_H2X_REMAP_PCI_ADDR_LO);
> +			writel(ASPEED_REMAP_PCI_ADDR_63_32(range.pci_addr),
> +			       pcie->reg + ASPEED_H2X_REMAP_PCI_ADDR_HI);
> +			return 0;

It looks like this is essentially hardcoding the offset between the
parent-bus-address and the child-bus-address in the DT 'ranges'
property.  Since ASPEED_REMAP_PCI_ADDR_31_12() and
ASPEED_REMAP_PCI_ADDR_63_32() do nothing except mask out the low 12
bits, I assume that offset is zero.

But this should not be hard-coded at all; it should be extracted from
'ranges'.  I don't think we really have a consistent way of doing
this, but you can take a look at how these other drivers program
"outbound" mappings like this using bridge->windows:

  cdns_pcie_hpa_host_init_address_translation()
  dw_pcie_iatu_setup()
  mobiveil_host_init()
  xgene_pcie_map_ranges()
  iproc_pcie_map_ranges()
  rzg3s_pcie_parse_map_ranges()

> +static int aspeed_pcie_parse_dt(struct aspeed_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	int ret;
> +
> +	for_each_available_child_of_node_scoped(node, child) {
> +		int slot;
> +		const char *type;
> +
> +		ret = of_property_read_string(child, "device_type", &type);
> +		if (ret || strcmp(type, "pci"))
> +			continue;
> +
> +		ret = of_pci_get_devfn(child);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to parse devfn\n");
> +
> +		slot = PCI_SLOT(ret);
> +
> +		ret = aspeed_pcie_parse_port(pcie, child, slot);
> +		if (ret)
> +			return ret;

It looks unnecessarily complicated to put each port on a list in
aspeed_pcie_parse_port() and then iterate over that list in
aspeed_pcie_init_ports().

I think you could just do something like:

  aspeed_pcie_parse_port();
  aspeed_pcie_port_init();

and get rid of the list completely.

> +	}
> +
> +	if (list_empty(&pcie->ports))
> +		return dev_err_probe(dev, -ENODEV,
> +				     "No PCIe port found in DT\n");
> +
> +	return 0;
> +}
> +
> +static int aspeed_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *host;
> +	struct aspeed_pcie *pcie;
> +	struct resource_entry *entry;
> +	const struct aspeed_pcie_rc_platform *md;
> +	int irq, ret;
> +
> +	md = of_device_get_match_data(dev);
> +	if (!md)
> +		return -ENODEV;
> +
> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> +	if (!host)
> +		return -ENOMEM;
> +
> +	pcie = pci_host_bridge_priv(host);
> +	pcie->dev = dev;
> +	pcie->tx_tag = 0;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pcie->platform = md;
> +	pcie->host = host;
> +	INIT_LIST_HEAD(&pcie->ports);
> +
> +	/* Get root bus num for cfg command to decide tlp type 0 or type 1 */
> +	entry = resource_list_first_type(&host->windows, IORESOURCE_BUS);
> +	if (entry)
> +		pcie->host_bus_num = entry->res->start;

s/host_bus_num/root_bus_nr/   to match struct altera_pcie