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@xxxxxxxx/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