Re: [PATCH v3 2/2] PCI: mediatek: Add support for EcoNet EN7528 SoC

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Mar 20, 2026 at 09:42:12AM +0000, Caleb James DeLisle wrote:
> Add support for the PCIe present on the EcoNet EN7528 (and EN751221) SoCs.
> 
> These SoCs have a mix of Gen1 and Gen2 capable ports, but the Gen2 ports
> require re-training after startup.
> ...

> +static int mtk_pcie_startup_port_en7528(struct mtk_pcie_port *port)
> +{
> +	struct mtk_pcie *pcie = port->pcie;
> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
> +	struct resource *mem = NULL;
> +	struct resource_entry *entry;
> +	u32 val, link_mask;
> +	int err;
> +
> +	entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
> +	if (entry)
> +		mem = entry->res;
> +	if (!mem)
> +		return -EINVAL;
> +
> +	if (!pcie->cfg) {
> +		dev_err(pcie->dev, "EN7528: pciecfg syscon not available\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Assert all reset signals */
> +	writel(0, port->base + PCIE_RST_CTRL);
> +
> +	/*
> +	 * Enable PCIe link down reset, if link status changed from link up to
> +	 * link down, this will reset MAC control registers and configuration
> +	 * space.
> +	 */
> +	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
> +
> +	/*
> +	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal) and
> +	 * 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST#

Include spec rev; the section numbers are typically specific to a spec
revision.  E.g., CEM r1.0, sec 2.2, 2.2.1.

> +	 * should be delayed 100ms (TPVPERL) for the power and clock to become
> +	 * stable.
> +	 */
> +	msleep(100);

Isn't there a #define for this in drivers/pci/pci.h?

> +	/* De-assert PHY, PE, PIPE, MAC and configuration reset */
> +	val = readl(port->base + PCIE_RST_CTRL);
> +	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
> +	       PCIE_MAC_SRSTB | PCIE_CRSTB;
> +	writel(val, port->base + PCIE_RST_CTRL);
> +
> +	writel(PCIE_CLASS_CODE | PCIE_REVISION_ID,
> +	       port->base + PCIE_CONF_REV_CLASS);
> +	writel(EN7528_HOST_MODE, port->base);
> +
> +	link_mask = (port->slot == 0) ? EN7528_RC0_LINKUP : EN7528_RC1_LINKUP;
> +
> +	/* 100ms timeout value should be enough for Gen1/2 training */
> +	err = regmap_read_poll_timeout(pcie->cfg, EN7528_LINKUP_REG, val,
> +				       !!(val & link_mask), 20,
> +				       100 * USEC_PER_MSEC);

Ditto.  Also take a look and see if this is relevant:
https://lore.kernel.org/all/20260320224821.2571373-1-thierry.reding@kernel.org

> +	if (err) {
> +		dev_err(pcie->dev, "EN7528: port%d link timeout\n", port->slot);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Set INTx mask */

Looks like this *clears* an INTx mask.  But the comment is probably
superfluous anyway.

> +	val = readl(port->base + PCIE_INT_MASK);
> +	val &= ~INTX_MASK;
> +	writel(val, port->base + PCIE_INT_MASK);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		mtk_pcie_enable_msi(port);
> +
> +	/* Set AHB to PCIe translation windows */
> +	val = lower_32_bits(mem->start) |
> +	      AHB2PCIE_SIZE(fls(resource_size(mem)));
> +	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
> +
> +	val = upper_32_bits(mem->start);
> +	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
> +
> +	writel(WIN_ENABLE, port->base + PCIE_AXI_WINDOW0);
> +
> +	return 0;
> +}
> +
>  static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
>  				      unsigned int devfn, int where)
>  {
> @@ -1149,6 +1236,30 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	if (err)
>  		goto put_resources;
>  
> +	/* Retrain Gen1 links to reach Gen2 where supported */
> +	if (pcie->soc->startup == mtk_pcie_startup_port_en7528) {

This looks like an ugly hack when placed here in mtk_pcie_probe(),
especially since it's after pci_host_probe() has already enumerated
the hierarchy.  Why can't this be inside
mtk_pcie_startup_port_en7528() itself?

> +		struct pci_bus *bus = host->bus;
> +		struct pci_dev *rc = NULL;
> +
> +		while ((rc = pci_get_class(PCI_CLASS_BRIDGE_PCI << 8, rc))) {
> +			int ret = -EOPNOTSUPP;

I don't get this.  pci_get_class() looks through the entire hierarchy,
but this driver should only care about the links directly attached to
Root Ports.

> +			if (rc->bus != bus)
> +				continue;
> +
> +			#if IS_BUILTIN(CONFIG_PCIE_MEDIATEK)
> +			ret = pcie_retrain_link(rc, true);
> +			#endif

Why is this specific to being builtin?  No other PCI controller
drivers do this.  Needs a comment about why this is special.
Typically such an #ifdef would be at the left margin.

> +			if (!ret)

Prefer the positive test ("if (ret)").

> +				dev_info(dev, "port%d link retrained\n",
> +					 PCI_SLOT(rc->devfn));
> +			else
> +				dev_info(dev, "port%d failed to retrain %pe\n",
> +					 PCI_SLOT(rc->devfn), ERR_PTR(ret));
> +		}
> +	}
> +
>  	return 0;
>  
>  put_resources:
> @@ -1264,8 +1375,15 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = {
>  	.quirks = MTK_PCIE_FIX_CLASS_ID | MTK_PCIE_FIX_DEVICE_ID,
>  };
>  
> +static const struct mtk_pcie_soc mtk_pcie_soc_en7528 = {
> +	.ops = &mtk_pcie_ops_v2,
> +	.startup = mtk_pcie_startup_port_en7528,
> +	.setup_irq = mtk_pcie_setup_irq,
> +};
> +
>  static const struct of_device_id mtk_pcie_ids[] = {
>  	{ .compatible = "airoha,an7583-pcie", .data = &mtk_pcie_soc_an7583 },
> +	{ .compatible = "econet,en7528-pcie", .data = &mtk_pcie_soc_en7528 },
>  	{ .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 },
>  	{ .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 },
>  	{ .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 },
> -- 
> 2.39.5
>