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

On Thu, Feb 6, 2025 at 12:04 PM Bjorn Helgaas [off-list ref] wrote:

On Wed, Feb 05, 2025 at 02:12:01PM -0500, Jim Quinlan wrote:

quoted

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

quoted

(2) Use constants from pci_reg.h when possible
(3) Use uXX_replace_bits(...) for setting a register field.

quoted

(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

quoted

Signed-off-by: Jim Quinlan <redacted>
---
 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

quoted

 #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