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

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

[Manivannan Sadhasivam]

On Mon, Apr 13, 2026 at 02:03:39PM +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.
> 
> Co-developed-by: Ahmed Naseef <naseefkm@gmail.com>
> Signed-off-by: Ahmed Naseef <naseefkm@gmail.com>
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> ---
>  drivers/pci/controller/Kconfig         |   2 +-
>  drivers/pci/controller/pcie-mediatek.c | 133 +++++++++++++++++++++++++
>  2 files changed, 134 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 686349e09cd3..5808d5e407fd 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 75722524fe74..915a35825ce1 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -7,6 +7,7 @@
>   *	   Honghui Zhang <honghui.zhang@mediatek.com>
>   */
>  
> +#include <asm-generic/errno-base.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/iopoll.h>
> @@ -14,6 +15,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 +79,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 +92,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))
> @@ -148,12 +156,15 @@ struct mtk_pcie_port;
>   * @MTK_PCIE_FIX_DEVICE_ID: host's device ID needed to be fixed
>   * @MTK_PCIE_NO_MSI: Bridge has no MSI support, and relies on an external block
>   * @MTK_PCIE_SKIP_RSTB: Skip calling RSTB bits on PCIe probe
> + * @MTK_PCIE_RETRAIN: Re-train link to bridge after startup because some
> + *                    Gen2-capable devices start as Gen1.
>   */
>  enum mtk_pcie_quirks {
>  	MTK_PCIE_FIX_CLASS_ID = BIT(0),
>  	MTK_PCIE_FIX_DEVICE_ID = BIT(1),
>  	MTK_PCIE_NO_MSI = BIT(2),
>  	MTK_PCIE_SKIP_RSTB = BIT(3),
> +	MTK_PCIE_RETRAIN = BIT(4),
>  };
>  
>  /**
> @@ -753,6 +764,80 @@ 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);
> +
> +	msleep(PCIE_T_PVPERL_MS);
> +
> +	/* 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,
> +				       PCI_PM_D3COLD_WAIT * USEC_PER_MSEC);
> +	if (err) {
> +		dev_err(pcie->dev, "EN7528: port%d link timeout\n", port->slot);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Activate INTx interrupts */
> +	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 +1234,46 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	if (err)
>  		goto put_resources;
>  
> +	/* EN7528 PCIe initially comes up as Gen1 even if Gen2 is supported.
> +	 * The cannonical way to achieve Gen2 is to re-train the link
> +	 * immediately after setup. However, to save a lot of duplicated code
> +	 * we use pcie_retrain_link() which is usable once we have the pci_dev
> +	 * struct for the bridge, i.e. after pci_host_probe(). */

Use below style:

	/*
	 * ...
	 */

> +	if (pcie->soc->quirks & MTK_PCIE_RETRAIN) {
> +		int slot = of_get_pci_domain_nr(dev->of_node);

The returned value is not the slot number, but domain number. Both are different
numbering schemes.

> +		struct pci_dev *rc = NULL;
> +		int ret = -ENOENT;
> +
> +		if (slot >= 0)
> +			rc = pci_get_slot(host->bus, PCI_DEVFN(slot, 0));

This looks wrong. If your intention is to find the Root Port of the hierarchy,
then you should do:

		pci_get_slot(host->bus, PCI_DEVFN(0, 0));

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Caleb James DeLisle]

On 12/05/2026 13:55, Manivannan Sadhasivam wrote:
> On Mon, Apr 13, 2026 at 02:03:39PM +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.
>>
>> Co-developed-by: Ahmed Naseef <naseefkm@gmail.com>
>> Signed-off-by: Ahmed Naseef <naseefkm@gmail.com>
>> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
>> ---
>>   drivers/pci/controller/Kconfig         |   2 +-
>>   drivers/pci/controller/pcie-mediatek.c | 133 +++++++++++++++++++++++++
>>   2 files changed, 134 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
>> index 686349e09cd3..5808d5e407fd 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 75722524fe74..915a35825ce1 100644
>> --- a/drivers/pci/controller/pcie-mediatek.c
>> +++ b/drivers/pci/controller/pcie-mediatek.c
>> @@ -7,6 +7,7 @@
>>    *	   Honghui Zhang <honghui.zhang@mediatek.com>
>>    */
>>   
>> +#include <asm-generic/errno-base.h>
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <linux/iopoll.h>
>> @@ -14,6 +15,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 +79,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 +92,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))
>> @@ -148,12 +156,15 @@ struct mtk_pcie_port;
>>    * @MTK_PCIE_FIX_DEVICE_ID: host's device ID needed to be fixed
>>    * @MTK_PCIE_NO_MSI: Bridge has no MSI support, and relies on an external block
>>    * @MTK_PCIE_SKIP_RSTB: Skip calling RSTB bits on PCIe probe
>> + * @MTK_PCIE_RETRAIN: Re-train link to bridge after startup because some
>> + *                    Gen2-capable devices start as Gen1.
>>    */
>>   enum mtk_pcie_quirks {
>>   	MTK_PCIE_FIX_CLASS_ID = BIT(0),
>>   	MTK_PCIE_FIX_DEVICE_ID = BIT(1),
>>   	MTK_PCIE_NO_MSI = BIT(2),
>>   	MTK_PCIE_SKIP_RSTB = BIT(3),
>> +	MTK_PCIE_RETRAIN = BIT(4),
>>   };
>>   
>>   /**
>> @@ -753,6 +764,80 @@ 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);
>> +
>> +	msleep(PCIE_T_PVPERL_MS);
>> +
>> +	/* 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,
>> +				       PCI_PM_D3COLD_WAIT * USEC_PER_MSEC);
>> +	if (err) {
>> +		dev_err(pcie->dev, "EN7528: port%d link timeout\n", port->slot);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	/* Activate INTx interrupts */
>> +	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 +1234,46 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>>   	if (err)
>>   		goto put_resources;
>>   
>> +	/* EN7528 PCIe initially comes up as Gen1 even if Gen2 is supported.
>> +	 * The cannonical way to achieve Gen2 is to re-train the link
>> +	 * immediately after setup. However, to save a lot of duplicated code
>> +	 * we use pcie_retrain_link() which is usable once we have the pci_dev
>> +	 * struct for the bridge, i.e. after pci_host_probe(). */
> Use below style:
>
> 	/*
> 	 * ...
> 	 */


Right, sorry, thanks.


>> +	if (pcie->soc->quirks & MTK_PCIE_RETRAIN) {
>> +		int slot = of_get_pci_domain_nr(dev->of_node);
> The returned value is not the slot number, but domain number. Both are different
> numbering schemes.
>
>> +		struct pci_dev *rc = NULL;
>> +		int ret = -ENOENT;
>> +
>> +		if (slot >= 0)
>> +			rc = pci_get_slot(host->bus, PCI_DEVFN(slot, 0));
> This looks wrong. If your intention is to find the Root Port of the hierarchy,
> then you should do:
>
> 		pci_get_slot(host->bus, PCI_DEVFN(0, 0));


Thank you for your review. What's happening here is the hardware exposes 
two sets of registers for the two devices, but it expects that they 
might all be controlled by one driver instance - so they hard-wired the 
second root hub to slot 1. The Mediatek driver here wants to be 
instantiated twice, so we end up with something like this:


0000:00:00.0 PCI bridge: MEDIATEK Corp. Device 0810 (rev 03)
0000:01:00.0 Network controller: MEDIATEK Corp. MT7662E 802.11ac PCI 
Express Wireless Network Adapter
0001:00:01.0 PCI bridge: MEDIATEK Corp. Device 0811 (rev 02)
0001:01:00.0 Network controller: MEDIATEK Corp. MT7603E 802.11bgn PCI 
Express Wireless Network Adapter

So when it's domain 1, it's also slot 1 (unless the DT is written 
backwards).


The original code from Ahmed Naseef used a loop with pci_get_class() to 
get all of the bridges that were on the right bus, but it was pointed 
out in an earlier review that this would also re-train any hypothetical 
bridge downstream of the root. So the current code specifically 
re-trains the right bridge.

I can re-send with this explanation wrapped up in the comment, or I 
guess I could write a loop that tries slot 0 and 1 to find the bridge, 
but I feel like the loop is a bit hair-splitting so I think my 
preference would be to just explain it better.

WDYT?

Caleb


>
> - Mani
>

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

[Manivannan Sadhasivam]

On Wed, May 13, 2026 at 04:31:32PM +0200, Caleb James DeLisle wrote:
> 
> On 12/05/2026 13:55, Manivannan Sadhasivam wrote:
> > On Mon, Apr 13, 2026 at 02:03:39PM +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.
> > > 
> > > Co-developed-by: Ahmed Naseef <naseefkm@gmail.com>
> > > Signed-off-by: Ahmed Naseef <naseefkm@gmail.com>
> > > Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> > > ---
> > >   drivers/pci/controller/Kconfig         |   2 +-
> > >   drivers/pci/controller/pcie-mediatek.c | 133 +++++++++++++++++++++++++
> > >   2 files changed, 134 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > index 686349e09cd3..5808d5e407fd 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 75722524fe74..915a35825ce1 100644
> > > --- a/drivers/pci/controller/pcie-mediatek.c
> > > +++ b/drivers/pci/controller/pcie-mediatek.c
> > > @@ -7,6 +7,7 @@
> > >    *	   Honghui Zhang <honghui.zhang@mediatek.com>
> > >    */
> > > +#include <asm-generic/errno-base.h>
> > >   #include <linux/clk.h>
> > >   #include <linux/delay.h>
> > >   #include <linux/iopoll.h>
> > > @@ -14,6 +15,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 +79,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 +92,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))
> > > @@ -148,12 +156,15 @@ struct mtk_pcie_port;
> > >    * @MTK_PCIE_FIX_DEVICE_ID: host's device ID needed to be fixed
> > >    * @MTK_PCIE_NO_MSI: Bridge has no MSI support, and relies on an external block
> > >    * @MTK_PCIE_SKIP_RSTB: Skip calling RSTB bits on PCIe probe
> > > + * @MTK_PCIE_RETRAIN: Re-train link to bridge after startup because some
> > > + *                    Gen2-capable devices start as Gen1.
> > >    */
> > >   enum mtk_pcie_quirks {
> > >   	MTK_PCIE_FIX_CLASS_ID = BIT(0),
> > >   	MTK_PCIE_FIX_DEVICE_ID = BIT(1),
> > >   	MTK_PCIE_NO_MSI = BIT(2),
> > >   	MTK_PCIE_SKIP_RSTB = BIT(3),
> > > +	MTK_PCIE_RETRAIN = BIT(4),
> > >   };
> > >   /**
> > > @@ -753,6 +764,80 @@ 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);
> > > +
> > > +	msleep(PCIE_T_PVPERL_MS);
> > > +
> > > +	/* 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,
> > > +				       PCI_PM_D3COLD_WAIT * USEC_PER_MSEC);
> > > +	if (err) {
> > > +		dev_err(pcie->dev, "EN7528: port%d link timeout\n", port->slot);
> > > +		return -ETIMEDOUT;
> > > +	}
> > > +
> > > +	/* Activate INTx interrupts */
> > > +	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 +1234,46 @@ static int mtk_pcie_probe(struct platform_device *pdev)
> > >   	if (err)
> > >   		goto put_resources;
> > > +	/* EN7528 PCIe initially comes up as Gen1 even if Gen2 is supported.
> > > +	 * The cannonical way to achieve Gen2 is to re-train the link
> > > +	 * immediately after setup. However, to save a lot of duplicated code
> > > +	 * we use pcie_retrain_link() which is usable once we have the pci_dev
> > > +	 * struct for the bridge, i.e. after pci_host_probe(). */
> > Use below style:
> > 
> > 	/*
> > 	 * ...
> > 	 */
> 
> 
> Right, sorry, thanks.
> 
> 
> > > +	if (pcie->soc->quirks & MTK_PCIE_RETRAIN) {
> > > +		int slot = of_get_pci_domain_nr(dev->of_node);
> > The returned value is not the slot number, but domain number. Both are different
> > numbering schemes.
> > 
> > > +		struct pci_dev *rc = NULL;
> > > +		int ret = -ENOENT;
> > > +
> > > +		if (slot >= 0)
> > > +			rc = pci_get_slot(host->bus, PCI_DEVFN(slot, 0));
> > This looks wrong. If your intention is to find the Root Port of the hierarchy,
> > then you should do:
> > 
> > 		pci_get_slot(host->bus, PCI_DEVFN(0, 0));
> 
> 
> Thank you for your review. What's happening here is the hardware exposes two
> sets of registers for the two devices, but it expects that they might all be
> controlled by one driver instance - so they hard-wired the second root hub
> to slot 1. The Mediatek driver here wants to be instantiated twice, so we
> end up with something like this:
> 
> 
> 0000:00:00.0 PCI bridge: MEDIATEK Corp. Device 0810 (rev 03)
> 0000:01:00.0 Network controller: MEDIATEK Corp. MT7662E 802.11ac PCI Express
> Wireless Network Adapter
> 0001:00:01.0 PCI bridge: MEDIATEK Corp. Device 0811 (rev 02)
> 0001:01:00.0 Network controller: MEDIATEK Corp. MT7603E 802.11bgn PCI
> Express Wireless Network Adapter
> 
> So when it's domain 1, it's also slot 1 (unless the DT is written
> backwards).
> 

Oops! Thanks for the explanation.

> 
> The original code from Ahmed Naseef used a loop with pci_get_class() to get
> all of the bridges that were on the right bus, but it was pointed out in an
> earlier review that this would also re-train any hypothetical bridge
> downstream of the root. So the current code specifically re-trains the right
> bridge.
> 
> I can re-send with this explanation wrapped up in the comment, or I guess I
> could write a loop that tries slot 0 and 1 to find the bridge, but I feel
> like the loop is a bit hair-splitting so I think my preference would be to
> just explain it better.
> 
> WDYT?
> 

Adding comments with the above info would suffice.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Bjorn Helgaas]

On Mon, Apr 13, 2026 at 02:03:39PM +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.

> +#include <asm-generic/errno-base.h>

Looks odd; why is this here?  There are basically no other drivers
that do this.

> @@ -1149,6 +1234,46 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	if (err)
>  		goto put_resources;
>  
> +	/* EN7528 PCIe initially comes up as Gen1 even if Gen2 is supported.
> +	 * The cannonical way to achieve Gen2 is to re-train the link
> +	 * immediately after setup. However, to save a lot of duplicated code
> +	 * we use pcie_retrain_link() which is usable once we have the pci_dev
> +	 * struct for the bridge, i.e. after pci_host_probe(). */

s/cannonical/canonical/

> +	if (pcie->soc->quirks & MTK_PCIE_RETRAIN) {
> +		int slot = of_get_pci_domain_nr(dev->of_node);

I suppose of_get_pci_domain_nr() is sort of an implicit way to
identify the Gen2 ports?  Worth at least a comment about this DT
connection.  Maybe it could be replaced by using
pcie_get_supported_speeds() or similar?

> +		struct pci_dev *rc = NULL;

s/rc/rp/ to avoid confusing "root port" for "return code" or "root
complex".

> +		int ret = -ENOENT;
> +
> +		if (slot >= 0)
> +			rc = pci_get_slot(host->bus, PCI_DEVFN(slot, 0));

Instead of fiddling with pci_get_slot(), which adds refcount issues
and artificial device/function number dependencies, I think it would
be better to iterate over the devices on host->bus, e.g., with
"for_each_pci_bridge(dev, host->bus)" as in iproc_pcie_setup().

> +		if (rc) {
> +			ret = -EOPNOTSUPP;
> +
> +			/* pcie_retrain_link() is not an exported symbol but
> +			 * this driver supports being built as a loadable
> +			 * module. Someone using this on an EN7528 should make
> +			 * it builtin, or accept Gen1 PCI. */
> +#if IS_BUILTIN(CONFIG_PCIE_MEDIATEK)
> +			ret = pcie_retrain_link(rc, true);
> +#endif

This looks like a confusing user experience if built as a module, with
no hint to the user about why the link is slower than it should be.
I guess "failed to retrain" is a bit of a hint, but it's not really a
clue about how to fix it.

> +		}
> +
> +		if (ret) {
> +			dev_info(dev, "port%d failed to retrain %pe\n", slot,
> +				 ERR_PTR(ret));

This is basically an error path and there's nothing else to do, so if
you return directly here (especially if you factor this to a separate
function), the "normal" path below can be unindented.

> +		} else {
> +			u16 lnksta;
> +			u32 speed;
> +
> +			pcie_capability_read_word(rc, PCI_EXP_LNKSTA, &lnksta);
> +			speed = lnksta & PCI_EXP_LNKSTA_CLS;
> +
> +			dev_info(dev, "port%d link retrained, speed %s\n", slot,
> +				 pci_speed_string(pcie_link_speed[speed]));
> +		}
> +	}

Maybe factor the retrain block into a helper function.

I'm sort of squinting at this whole link retrain thing to begin with.
After the controller is configured correctly, the hardware is supposed
to train the link automatically by itself.

Did something change between mtk_pcie_startup_port_en7528() and now
that means the link will train at Gen2?  Whatever that change is,
could it be done in mtk_pcie_startup_port_en7528()?

What happens when the downstream device is put in D3cold and the link
retrains after power is restored?  Does it train at Gen2 then, without
assistance like this?

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

[Caleb James DeLisle]

On 12/05/2026 18:55, Bjorn Helgaas wrote:
> On Mon, Apr 13, 2026 at 02:03:39PM +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.
>> +#include <asm-generic/errno-base.h>
> Looks odd; why is this here?  There are basically no other drivers
> that do this.


Whoops :facepalm:, must have been coding in my sleep and I wanted ENOENT.


>
>> @@ -1149,6 +1234,46 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>>   	if (err)
>>   		goto put_resources;
>>   
>> +	/* EN7528 PCIe initially comes up as Gen1 even if Gen2 is supported.
>> +	 * The cannonical way to achieve Gen2 is to re-train the link
>> +	 * immediately after setup. However, to save a lot of duplicated code
>> +	 * we use pcie_retrain_link() which is usable once we have the pci_dev
>> +	 * struct for the bridge, i.e. after pci_host_probe(). */
> s/cannonical/canonical/
OK
>
>> +	if (pcie->soc->quirks & MTK_PCIE_RETRAIN) {
>> +		int slot = of_get_pci_domain_nr(dev->of_node);
> I suppose of_get_pci_domain_nr() is sort of an implicit way to
> identify the Gen2 ports?  Worth at least a comment about this DT
> connection.  Maybe it could be replaced by using
> pcie_get_supported_speeds() or similar?


The explanation for this is here: 
https://lore.kernel.org/linux-mips/20260413140339.16238-1-cjd@cjdns.fr/T/#m6d893b861425378c0d094a142e6191d59dcc5192 
- and please do weigh in if you think I ought to change the logic there.


However you raise a good point about re-training Gen1 links, currently 
we're attempting to re-train everything. All of these hubs self-identify 
as Gen2 so we can't short-circuit with pcie_get_supported_speeds(). 
Re-training will remain at Gen1 if either the PHY is a Gen1-only PHY, or 
if the actual card (e.g. wifi chip) only supports Gen1. My feeling is 
that matching on the PHY DT node and short-circuiting is not a good 
idea, but I can improve the comment a bit.


>
>> +		struct pci_dev *rc = NULL;
> s/rc/rp/ to avoid confusing "root port" for "return code" or "root
> complex".
OK
>
>> +		int ret = -ENOENT;
>> +
>> +		if (slot >= 0)
>> +			rc = pci_get_slot(host->bus, PCI_DEVFN(slot, 0));
> Instead of fiddling with pci_get_slot(), which adds refcount issues
> and artificial device/function number dependencies, I think it would
> be better to iterate over the devices on host->bus, e.g., with
> "for_each_pci_bridge(dev, host->bus)" as in iproc_pcie_setup().
Oh great, thank you !
>
>> +		if (rc) {
>> +			ret = -EOPNOTSUPP;
>> +
>> +			/* pcie_retrain_link() is not an exported symbol but
>> +			 * this driver supports being built as a loadable
>> +			 * module. Someone using this on an EN7528 should make
>> +			 * it builtin, or accept Gen1 PCI. */
>> +#if IS_BUILTIN(CONFIG_PCIE_MEDIATEK)
>> +			ret = pcie_retrain_link(rc, true);
>> +#endif
> This looks like a confusing user experience if built as a module, with
> no hint to the user about why the link is slower than it should be.
> I guess "failed to retrain" is a bit of a hint, but it's not really a
> clue about how to fix it.
My logic was that the person whose going to configure a kernel for one 
of these things is pretty advanced - probably an OpenWRT developer - so 
they don't need that much hand-holding. But I guess it doesn't cost that 
much to add an `if (!IS_BUILTIN(CONFIG_PCIE_MEDIATEK))` with a warning log.
>
>> +		}
>> +
>> +		if (ret) {
>> +			dev_info(dev, "port%d failed to retrain %pe\n", slot,
>> +				 ERR_PTR(ret));
> This is basically an error path and there's nothing else to do, so if
> you return directly here (especially if you factor this to a separate
> function), the "normal" path below can be unindented.
Indeed, and if "not a builtin" is handled separately then this is truly 
an unexpected error so it's quite reasonable to goto put_resources.
>
>> +		} else {
>> +			u16 lnksta;
>> +			u32 speed;
>> +
>> +			pcie_capability_read_word(rc, PCI_EXP_LNKSTA, &lnksta);
>> +			speed = lnksta & PCI_EXP_LNKSTA_CLS;
>> +
>> +			dev_info(dev, "port%d link retrained, speed %s\n", slot,
>> +				 pci_speed_string(pcie_link_speed[speed]));
>> +		}
>> +	}
> Maybe factor the retrain block into a helper function.
Makes sense.
> I'm sort of squinting at this whole link retrain thing to begin with.
> After the controller is configured correctly, the hardware is supposed
> to train the link automatically by itself.
>
> Did something change between mtk_pcie_startup_port_en7528() and now
> that means the link will train at Gen2?  Whatever that change is,
> could it be done in mtk_pcie_startup_port_en7528()?
Per the comment with the misspelled "canonical", the reference code 
finds and pokes the re-training registers immediately during setup, but 
doing that manually is a fair bit of code and it's nicer to wait until 
the registers are mapped and use pcie_retrain_link().
> What happens when the downstream device is put in D3cold and the link
> retrains after power is restored?  Does it train at Gen2 then, without
> assistance like this?

I just tried pulling the driver from the wifi and then unbinding the 
bridge, then re-scanning. All throughout the process current_link_speed 
remains at 5.0. You're much more knowledgeable in this than me, but if I 
had a guess, I'd say this was a hardware bug that was fixed in 
subsequent versions (MT7621) and the workaround was to do a retrain once 
immediately after setup. But I don't want to warrant that as true 
because it's just me guessing...


Thanks,

Caleb