Re: [PATCH] PCI: brcmstb: Assign pcie->gen from pcie_get_link_speed()

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, May 05, 2026 at 10:58:49AM -0700, Florian Fainelli wrote:
> On 5/5/26 09:01, Bjorn Helgaas wrote:
> > On Mon, May 04, 2026 at 04:46:03PM -0700, Florian Fainelli wrote:
> > > On 5/4/26 10:26, Hans Zhang wrote:
> > > > On 5/5/26 00:58, Florian Fainelli wrote:
> > > > > On 5/2/26 04:40, Bjorn Helgaas wrote:
> > > > > > On Fri, May 01, 2026 at 01:24:38PM -0700, Florian Fainelli wrote:
> > > > > > > After commit 03f920936977 ("PCI: controller: Validate max-link-speed"),
> > > > > > > pcie->gen stopped being assigned and as a result the established PCIe
> > > > > > > link would stop supporting Gen3 speeds on 2712 since pcie->gen is used
> > > > > > > to populate LnkCntl2 and LnkCap in brcm_pcie_set_gen().
> > > > > > > 
> > > > > > > Link: https://github.com/raspberrypi/linux/issues/7343
> > > > > > > Reported-by: Dom Cobley <popcornmix@gmail.com>
> > > > > > > Reported-by: Phil Elwell <phil@raspberrypi.com>
> > > > > > > Fixes: 03f920936977 ("PCI: controller: Validate max-link-speed")
> > > > > > > Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > > > > > > ---
> > > > > > >    drivers/pci/controller/pcie-brcmstb.c | 3 +--
> > > > > > >    1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c
> > > > > > > b/drivers/pci/ controller/pcie-brcmstb.c
> > > > > > > index 714bcab97b60..6138fc4bc064 100644
> > > > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > > > @@ -2072,8 +2072,7 @@ static int brcm_pcie_probe(struct
> > > > > > > platform_device *pdev)
> > > > > > >            return PTR_ERR(pcie->clk);
> > > > > > >        ret = of_pci_get_max_link_speed(np);
> > > > > > > -    if (pcie_get_link_speed(ret) == PCI_SPEED_UNKNOWN)
> > > > > > > -        pcie->gen = 0;
> > > > > > > +    pcie->gen = pcie_get_link_speed(ret);
> > > > > > 
> > > > > > Take a look at https://sashiko.dev/#/
> > > > > > patchset/20260501202438.376033-1-
> > > > > > florian.fainelli%40broadcom.com
> > > > > > 
> > > > > > The notes at https://github.com/raspberrypi/linux/issues/7343 assumed
> > > > > > PCI_SPEED_UNKNOWN was 0, but in fact it is 0xff, which means you might
> > > > > > want the more defensive patch instead.
> > > > > > 
> > > > > > I'll be happy to replace what's on pci/for-linus if so.
> > > > > 
> > > > > I am starting to think a revert is the simplest path forward, it's
> > > > > not clear what pcie_get_link_speed() brings to the table honestly.
> > > > 
> > > > The pcie_get_link_speed function is designed to prevent other Root Port
> > > > drivers from accessing the array pcie_link_speed out of bounds.
> > > 
> > > Yes, so that's useful in the first hunk of your commit where we were
> > > printing the link speed without checking that the 'max-link-speed' would be
> > > bounds check, however it is not really useful for assigning to pcie->gen in
> > > our case, so I would prefer the second option:
> > > 
> > >            ret = of_pci_get_max_link_speed(np);
> > >   -       if (pcie_get_link_speed(ret) == PCI_SPEED_UNKNOWN)
> > >   -               pcie->gen = 0;
> > >   +       pcie->gen = (ret < 0) ? 0 : ret;
> > 
> > The point of this validation is to make sure we don't program the
> > brcmstb hardware with something it doesn't support.
> > 
> > of_pci_get_max_link_speed() only returns an error (ret < 0) if DT
> > didn't contain a 'max-link-speed' property.  That doesn't tell us
> > anything about what brcmstb devices support.
> > 
> > I think this test should be something like what advk_pcie_probe() or
> > rockchip_pcie_parse_dt() do.  If of_pci_get_max_link_speed() fails or
> > returns something not supported by the hardware, they default to the
> > fastest speed known to be supported.
> 
> Doing that could be challenging because it varies on a per-controller
> instance on some chips like 2712, this would also be a behavioral difference
> compared to today where the driver resorts to HW-defaults if the property is
> not specified.

I think it's OK to default to what the hardware advertises if that
makes sense for this hardware and driver.

The fact that this is all device & driver dependent was part of what
motivated the series in the first place -- the generic OF code can't
do that kind of per-controller validation.