Re: [PATCH v4 2/4] PCI:?==?utf-8?q? rockchip: drive at 2.5 GT/s only and error out other speeds

[Dragan Simic]

On Friday, February 27, 2026 23:47 CET, Geraldo Nascimento <geraldogabriel at gmail.com> wrote:
> On Fri, Feb 27, 2026 at 06:33:34PM +0100, Dragan Simic wrote:
> > On Friday, February 27, 2026 06:36 CET, Geraldo Nascimento <geraldogabriel at gmail.com> wrote:
> > > Configure the core to be driven at 2.5 GT/s Link Speed and error
> > > out on any other speed. The reason is that Shawn Lin from Rockchip
> > > has reiterated that there may be danger of "catastrophic failure"
> > > in using their PCIe with 5.0 GT/s speeds.
> > > 
> > > While Rockchip has done so informally without issuing a proper
> > > errata, and the particulars are thus unknown, this may cause
> > > data loss or worse.
> > > 
> > > This change is corroborated by RK3399 official datasheet [1], which
> > > states maximum link speed for this platform is 2.5 GT/s.
> > > 
> > > [1] https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf
> > > 
> > > Fixes: 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")
> > > Link: https://lore.kernel.org/all/ffd05070-9879-4468-94e3-b88968b4c21b@rock-chips.com/
> > > Cc: stable at vger.kernel.org
> > > Reported-by: Dragan Simic <dsimic at manjaro.org>
> > > Reported-by: Shawn Lin <shawn.lin at rock-chips.com>
> > > Signed-off-by: Geraldo Nascimento <geraldogabriel at gmail.com>
> > > ---
> > >  drivers/pci/controller/pcie-rockchip.c | 13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> > > index 0f88da378805..26fc350a66c1 100644
> > > --- a/drivers/pci/controller/pcie-rockchip.c
> > > +++ b/drivers/pci/controller/pcie-rockchip.c
> > > @@ -66,8 +66,9 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> > >  	}
> > >  
> > >  	rockchip->link_gen = of_pci_get_max_link_speed(node);
> > > -	if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
> > > -		rockchip->link_gen = 2;
> > > +	if (rockchip->link_gen < 0 || rockchip->link_gen >= 2)
> > > +		return dev_err_probe(dev, rockchip->link_gen,
> > > +				     "Invalid Link Speed\n");
> > 
> > I'm quite surprised to see what happened here in the v4?  The changes
> > introduced in this diff block in the v3 were perfectly fine, i.e. we need
> > to correct any runtime occurrences of Gen2 speed setting in the rockchip_
> > pcie_parse_dt() function, together with emitting a warning that urges
> > the users and the board dtb maintainer to fix the board dtb.  I'll get
> > back to this below.
> 
> I see what you mean now. Sure, this could be incorporated for v5 without
> a problem and is the more proper way to solve it.

Actually, we need to do it the way I suggested above, i.e. the v3 way,
because doing it like in the v4 diff chunk above could break PCIe on some
boards that currently have Gen2 configured in their dtb files, which may
get their kernels updated without updating their dtb files as well, for
whatever reason.  Basically, we need to avoid such breakage scenarios.

> > >  	for (i = 0; i < ROCKCHIP_NUM_PM_RSTS; i++)
> > >  		rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];
> > > @@ -147,12 +148,8 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> > >  		goto err_exit_phy;
> > >  	}
> > >  
> > > -	if (rockchip->link_gen == 2)
> > > -		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
> > > -				    PCIE_CLIENT_CONFIG);
> > > -	else
> > > -		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
> > > -				    PCIE_CLIENT_CONFIG);
> > > +	rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
> > > +			    PCIE_CLIENT_CONFIG);
> > >  
> > >  	regs = PCIE_CLIENT_ARI_ENABLE |
> > >  	       PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes);
> > 
> > At this point we need to check and bail out if no longer supported Gen2
> > speed setting is received, which also applies to the appropriate spot in
> > the PCIe endpoint driver.
> 
> Right, it is a bit of a double-check I think but it can't hurt.
> 
> > To clarify, it's the responsibility of the rockchip_pcie_parse_dt()
> > function to validate and possibly correct the speed setting, because it
> > is what receives that setting as a value from the board dtb, while the
> > consumers of that validated speed setting should check it, to make sure
> > the speed is right because they no longer can handle requests for Gen2
> > speed, and simply bail out if the received speed isn't right, i.e. if
> > it differs from Gen1.
> 
> Like I said, it ends being a double-check, because we can't let probe
> progress if link_gen ends up being different than 1. Either we force
> 2.5 GT/s with a fair warning or we error out on the probe already.

Ah, sorry, I somehow missed that both rockchip_pcie_host_init_port() and
rockchip_pcie_ep_link_training() are defined as static functions, so you're
right that checking for "link_gen == 1" in them again would be redundant.