Re: [PATCH v2 1/4] PCI:?==?utf-8?q? rockchip: limit RK3399 to 2.5 GT/s to prevent damage

[Dragan Simic]

Hello Diederik,

On Thursday, December 18, 2025 11:01 CET, "Diederik de Haas" <diederik at cknow-tech.com> wrote:
> On Thu Dec 18, 2025 at 10:47 AM CET, Dragan Simic wrote:
> > On Thursday, December 18, 2025 09:05 CET, Manivannan Sadhasivam <mani at kernel.org> wrote:
> >> On Mon, Nov 17, 2025 at 06:47:05PM -0300, Geraldo Nascimento wrote:
> >> > Shawn Lin from Rockchip has reiterated that there may be danger in using
> >> > their PCIe with 5.0 GT/s speeds. Warn the user if they make a DT change
> >> > from the default and drive at 2.5 GT/s only, even if the DT
> >> > max-link-speed property is invalid or inexistent.
> >> > 
> >> > This change is corroborated by RK3399 official datasheet [1], which
> >> > says 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>
> >> > Reviewed-by: Dragan Simic <dsimic at manjaro.org>
> >> > Signed-off-by: Geraldo Nascimento <geraldogabriel at gmail.com>
> >> > ---
> >> >  drivers/pci/controller/pcie-rockchip.c | 10 ++++++++--
> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >> > 
> >> > diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> >> > index 0f88da378805..992ccf4b139e 100644
> >> > --- a/drivers/pci/controller/pcie-rockchip.c
> >> > +++ b/drivers/pci/controller/pcie-rockchip.c
> >> > @@ -66,8 +66,14 @@ 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) {
> >> > +		rockchip->link_gen = 1;
> >> > +		dev_warn(dev, "invalid max-link-speed, set to 2.5 GT/s\n");
> >> > +	}
> >> > +	else if (rockchip->link_gen == 2) {
> >> > +		rockchip->link_gen = 1;
> >> > +		dev_warn(dev, "5.0 GT/s is dangerous, set to 2.5 GT/s\n");
> >> 
> >> What does 'danger' really mean here? Link instability or something else?
> >> Error messages should be precise and not fearmongering.
> >
> > I agree that the original wording is a bit suboptimal, and I'd suggest
> > to Geraldo that the produced warning message is changed to
> >
> >   "5.0 GT/s may cause data corruption, limited to to 2.5 GT/s\n"
> >
> > or something similar, to better reflect the actual underlying issue.
> 
> s/limited to to/therefore limit speed to/ ?

That would work well in a book or an article, while slightly terse
wording is usually preferred in the messages produced by the kernel,
or in log messages in general.  Such an approach compacts as much
information as possible in as few words as possible, while still
remaining (mostly) grammatically correct.