Re: [PATCH v1 1/6] PCI: brcmstb: Refactor max speed limit functionality

[Jim Quinlan]

On Thu, Feb 6, 2025 at 12:04 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Wed, Feb 05, 2025 at 02:12:01PM -0500, Jim Quinlan wrote:
> > Make changes to the code that limits the PCIe max speed.
> >
> > (1) Do the changes before link-up, not after.  We do not want
> >     to temporarily rise to a higher speed than desired.
>
> This is a functional change that should be split into its own patch.
> That will also make it obvious that this is not simple refactoring as
> the subject line advertises.
ack
>
> > (2) Use constants from pci_reg.h when possible
> > (3) Use uXX_replace_bits(...) for setting a register field.
>
> > (4) Use the internal link capabilities register for writing
> >     the max speed, not the official config space register
> >     where the speed field is RO.  Updating this field is
> >     not necessary to limit the speed so this mistake was
> >     harmless.
>
> Also a bug fix (though harmless in this case) that deserves to be
> split out so the distinction between the internal and the architected
> paths to the register is highlighted and may help prevent the same
> mistake in the future.
ack
>
> > Signed-off-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 546056f7f0d3..f8fc3d620ee2 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -47,6 +47,7 @@
> >
> >  #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY                    0x04dc
> >  #define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00
> > +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK       0xf
>
> If the format of this internal register is different, of course we
> need a new #define for this.  But if this is just a different path to
> LNKCAP, and both paths read the same bits in the same format, I don't
> see the point of a new #define.
ack
>
> >  #define PCIE_RC_CFG_PRIV1_ROOT_CAP                   0x4f8
> >  #define  PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK   0xf8
> > @@ -413,12 +414,12 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
> >  static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
> >  {
> >       u16 lnkctl2 = readw(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
> > -     u32 lnkcap = readl(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> > +     u32 lnkcap = readl(pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> >
> > -     lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
> > -     writel(lnkcap, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> > +     u32p_replace_bits(&lnkcap, gen, PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK);
> > +     writel(lnkcap, pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> >
> > -     lnkctl2 = (lnkctl2 & ~0xf) | gen;
> > +     u16p_replace_bits(&lnkctl2, gen, PCI_EXP_LNKCTL2_TLS);
>
>
> OK.  I am not really a fan of the uXX_replace_bits() thing because
> it's not widely used (I found 341 instances tree-wide, compared to
> 14000+ for FIELD_PREP()), grep can't find the definition easily, and
> stylistically it doesn't fit with GENMASK(), FIELD_PREP(), etc.
>
> But it's already used throughout brcmstb.c, so we should be
> consistent.

And here I thought that uXX_replace_bits()  was the up-and-coming
solution to be used :-)

Regards,
Jim Quinlan
Broadcom STB/CM

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature