Re: [PATCH v4 2/4] PCI: rockchip: drive at 2.5 GT/s only and error out other speeds

[Geraldo Nascimento <geraldogabriel@gmail.com>]

On Fri, Feb 27, 2026 at 06:33:34PM +0100, Dragan Simic wrote:
> Hello Geraldo,
>

Hi Dragan,

> On Friday, February 27, 2026 06:36 CET, Geraldo Nascimento <geraldogabriel@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@vger.kernel.org
> > Reported-by: Dragan Simic <dsimic@manjaro.org>
> > Reported-by: Shawn Lin <shawn.lin@rock-chips.com>
> > Signed-off-by: Geraldo Nascimento <geraldogabriel@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.

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

Thank you,
Geraldo Nascimento