| Message ID | 20250205191213.29202-2-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcmstb: Misc small tweaks and fixes | expand |
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. > (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. > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > 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. > #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.
On Thu, Feb 6, 2025 at 12:04 PM Bjorn Helgaas <helgaas@kernel.org> 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@broadcom.com> > > --- > > 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
On Thu, Feb 06, 2025 at 01:27:44PM -0500, Jim Quinlan wrote: > On Thu, Feb 6, 2025 at 12:04 PM Bjorn Helgaas <helgaas@kernel.org> 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. > ... > And here I thought that uXX_replace_bits() was the up-and-coming > solution to be used :-) :) Yeah, I was kind of surprised that it isn't used more, given that it was merged in 2017. And it *does* reduce some repetition, which is definitely an advantage. But the fact that those functions are hard to find with grep is a big turnoff for me. I wasn't really a fan of GENMASK() and FIELD_PREP() at first either, but now I'm a big fan because you don't need _SHIFT #defines and it reduces shift/mask errors. So probably the same will happen with uXX_replace_bits().
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 #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); writew(lnkctl2, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2); } @@ -1324,6 +1325,10 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie) bool ssc_good = false; int ret, i; + /* Limit the generation if specified */ + if (pcie->gen) + brcm_pcie_set_gen(pcie, pcie->gen); + /* Unassert the fundamental reset */ ret = pcie->cfg->perst_set(pcie, 0); if (ret) @@ -1350,9 +1355,6 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie) brcm_config_clkreq(pcie); - if (pcie->gen) - brcm_pcie_set_gen(pcie, pcie->gen); - if (pcie->ssc) { ret = brcm_pcie_set_ssc(pcie); if (ret == 0)
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. (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. Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- drivers/pci/controller/pcie-brcmstb.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)