[PATCH v3 0/2] PCI: mediatek: Add support for EcoNet SoCs

[PATCH v3 0/2] PCI: mediatek: Add support for EcoNet SoCs

[Caleb James DeLisle]

Add EcoNet EN7528 (and EN751221) PCIe support.

Changes from v2:
* mediatek-pcie.yaml -> s/power-domain/power-domains/ and drop example
* Patch 3 dropped as it has been applied (Thanks!)
* v2: https://lore.kernel.org/linux-mips/20260316155157.679533-1-cjd@cjdns.fr/

Changes from v1:
* mediatek-pcie.yaml slot0 needs device-type = "pci", fix dt_binding_check
Link: https://lore.kernel.org/linux-mips/177334026016.3889069.9474337544951486443.robh@kernel.org
* v1: https://lore.kernel.org/linux-mips/20260312165332.569772-1-cjd@cjdns.fr/

This was split from a larger PCIe patchset which crossed multiple
subsystems. I'm not labeling this a v3 because it's a new patchset, but
I'm keeping the historical record anyway.

Changes from econet-pcie v2:
* mediatek-pcie.yaml add missing constraints to PCI node properties
* econet-pcie v2: https://lore.kernel.org/linux-mips/20260309131818.74467-1-cjd@cjdns.fr

Changes from econet-pcie v1:
* pcie-mediatek.c Exclude pcie_retrain_link() when building as a module
* econet-pcie v1: https://lore.kernel.org/linux-mips/20260303190948.694783-1-cjd@cjdns.fr/

Caleb James DeLisle (2):
  dt-bindings: PCI: mediatek: Add support for EcoNet EN7528
  PCI: mediatek: Add support for EcoNet EN7528 SoC

 .../bindings/pci/mediatek-pcie.yaml           |  26 ++++
 drivers/pci/controller/Kconfig                |   2 +-
 drivers/pci/controller/pcie-mediatek.c        | 118 ++++++++++++++++++
 3 files changed, 145 insertions(+), 1 deletion(-)


base-commit: 3fa5e5702a82d259897bd7e209469bc06368bf31
-- 
2.39.5

[PATCH v3 1/2] dt-bindings: PCI: mediatek: Add support for EcoNet EN7528

[Caleb James DeLisle]

Introduce EcoNet EN7528 SoC compatible in MediaTek PCIe controller
binding.

EcoNet PCIe controller has the same configuration model as
Mediatek v2 but is initiallized more similarly to an MT7621
PCIe.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 .../bindings/pci/mediatek-pcie.yaml           | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml
index 0b8c78ec4f91..7e1b0876c291 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml
@@ -14,6 +14,7 @@ properties:
     oneOf:
       - enum:
           - airoha,an7583-pcie
+          - econet,en7528-pcie
           - mediatek,mt2712-pcie
           - mediatek,mt7622-pcie
           - mediatek,mt7629-pcie
@@ -226,6 +227,31 @@ allOf:
 
         mediatek,pbus-csr: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: econet,en7528-pcie
+    then:
+      properties:
+        clocks:
+          maxItems: 1
+
+        clock-names:
+          maxItems: 1
+
+        reset: false
+
+        reset-names: false
+
+        power-domains: false
+
+        mediatek,pbus-csr: false
+
+      required:
+        - phys
+        - phy-names
+
 unevaluatedProperties: false
 
 examples:
-- 
2.39.5

Re: [PATCH v3 1/2] dt-bindings: PCI: mediatek: Add support for EcoNet EN7528

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

On Fri, Mar 20, 2026 at 09:42:11AM +0000, Caleb James DeLisle wrote:
> Introduce EcoNet EN7528 SoC compatible in MediaTek PCIe controller
> binding.
> 
> EcoNet PCIe controller has the same configuration model as
> Mediatek v2 but is initiallized more similarly to an MT7621
> PCIe.
> 
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>

Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: PCI: mediatek: Add support for EcoNet EN7528

[Bjorn Helgaas]

On Fri, Mar 20, 2026 at 09:42:11AM +0000, Caleb James DeLisle wrote:
> Introduce EcoNet EN7528 SoC compatible in MediaTek PCIe controller
> binding.
> 
> EcoNet PCIe controller has the same configuration model as
> Mediatek v2 but is initiallized more similarly to an MT7621
> PCIe.

s/initiallized/initialized/

Re: [PATCH v3 1/2] dt-bindings: PCI: mediatek: Add support for EcoNet EN7528

[Caleb James DeLisle]

On 23/03/2026 22:38, Bjorn Helgaas wrote:
> On Fri, Mar 20, 2026 at 09:42:11AM +0000, Caleb James DeLisle wrote:
>> Introduce EcoNet EN7528 SoC compatible in MediaTek PCIe controller
>> binding.
>>
>> EcoNet PCIe controller has the same configuration model as
>> Mediatek v2 but is initiallized more similarly to an MT7621
>> PCIe.
> s/initiallized/initialized/

Oops, thank you.

Caleb

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

[Caleb James DeLisle]

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.

Co-developed-by: Ahmed Naseef <naseefkm@gmail.com>
Signed-off-by: Ahmed Naseef <naseefkm@gmail.com>
Co-developed-by: Caleb James DeLisle <cjd@cjdns.fr>
Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 118 +++++++++++++++++++++++++
 2 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 5aaed8ac6e44..f6a5fcacb38d 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -209,7 +209,7 @@ config PCI_MVEBU
 
 config PCIE_MEDIATEK
 	tristate "MediaTek PCIe controller"
-	depends on ARCH_AIROHA || ARCH_MEDIATEK || COMPILE_TEST
+	depends on ARCH_AIROHA || ARCH_MEDIATEK || ECONET || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI
 	select IRQ_MSI_LIB
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 5defa5cc4c2b..84064061652a 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -14,6 +14,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqchip/irq-msi-lib.h>
 #include <linux/irqdomain.h>
+#include <linux/kconfig.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/msi.h>
@@ -77,6 +78,7 @@
 
 #define PCIE_CONF_VEND_ID	0x100
 #define PCIE_CONF_DEVICE_ID	0x102
+#define PCIE_CONF_REV_CLASS	0x104
 #define PCIE_CONF_CLASS_ID	0x106
 
 #define PCIE_INT_MASK		0x420
@@ -89,6 +91,11 @@
 #define MSI_MASK		BIT(23)
 #define MTK_MSI_IRQS_NUM	32
 
+#define EN7528_HOST_MODE	0x00804201
+#define EN7528_LINKUP_REG	0x50
+#define EN7528_RC0_LINKUP	BIT(1)
+#define EN7528_RC1_LINKUP	BIT(2)
+
 #define PCIE_AHB_TRANS_BASE0_L	0x438
 #define PCIE_AHB_TRANS_BASE0_H	0x43c
 #define AHB2PCIE_SIZE(x)	((x) & GENMASK(4, 0))
@@ -753,6 +760,86 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 	return 0;
 }
 
+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#
+	 * should be delayed 100ms (TPVPERL) for the power and clock to become
+	 * stable.
+	 */
+	msleep(100);
+
+	/* 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);
+	if (err) {
+		dev_err(pcie->dev, "EN7528: port%d link timeout\n", port->slot);
+		return -ETIMEDOUT;
+	}
+
+	/* Set INTx mask */
+	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) {
+		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;
+
+			if (rc->bus != bus)
+				continue;
+
+			#if IS_BUILTIN(CONFIG_PCIE_MEDIATEK)
+			ret = pcie_retrain_link(rc, true);
+			#endif
+
+			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

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

[Bjorn Helgaas]

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
> 

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

[Caleb James DeLisle]

On 23/03/2026 22:36, Bjorn Helgaas wrote:
> 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?

Indeed, and it has the proper reference to the spec revision. This hard 
coded 100 and the spec comment trickled down from vendor code. Will 
replace with the symbol.

>> +	/* 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
Okay, the hardcoded 100ms follows what is done in pcie-mediatek.c but 
from pcie-mediatek-gen3.c it appears the appropriate symbol is 
PCI_PM_D3COLD_WAIT so I'll substitute this.
>
>> +	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.
Indeed. "clear" -> "unmask" -> "activate" -> "set", I'll use the word 
"activate" since that's what we're doing.
>> +	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?

This is indeed frustrating. This little ritual has trickled down through 
2 or 3 generations of rewritten vendor code. What we know is that the 
Gen2 links come up as Gen1 on startup, but if you retrain them they 
upgrade to Gen2. Technically this could be done in 
mtk_pcie_startup_port_en7528(), but it won't be using 
pcie_retrain_link() because we don't yet have the pci_dev struct. Doing 
it manually is kind of ugly because we have to get the config offset of 
the bridge device which we're retraining - vendor code for this is a bit 
of a mess.

I think probably the least bad solution here is to stick with this, 
perhaps with a more descriptive / apologetic comment. WDYT?

>> +		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.
Good point, I'll switch this out for something more specific.
>> +			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.

Okay yes, good point. The reason for this is because pcie_retrain_link() 
is not exported and trying to re-implement the logic is the mess I 
alluded to earlier. Since this driver supports multiple devices and 
allows being compiled as a module, I opted to soft-fail in this case and 
it will log that it didn't retrain with reason -EOPNOTSUPP. In that case 
it would continue to run as a Gen1, and since this is an embedded 
application, I think we can rely on the integrator to make it a builtin 
if they're targeting the EN7528. But I can re-send with nicer 
documentation around this.

>
>> +			if (!ret)
> Prefer the positive test ("if (ret)").

Okay.


Thank you very much for the review.

Caleb


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