Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant
From: James Quinlan
Date: Tue Oct 28 2025 - 16:02:00 EST
On 10/22/25 18:45, Bjorn Helgaas wrote:
On Fri, Oct 03, 2025 at 01:04:36PM -0400, Jim Quinlan wrote:yes and no, see below.
The driver was using the PCIE_LINK_STATE_L1 constant as a field mask forThis all tangential questions for possible future changes, not
setting the private PCI_EXP_LNKCAP register, but this constant is
Linux-created and has nothing to do with the PCIe spec. Serendipitously,
the value of this constant was correct for its usage until after 6.1, when
its value changed from BIT(1) to BIT(2);
...
#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY 0x04dc
#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_WIDTH_MASK 0x1f0
-#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00
anything for *this* patch.
We have these in include/uapi/linux/pci_regs.h:
#define PCI_EXP_LNKCAP 0x0c /* Link Capabilities */
#define PCI_EXP_LNKCAP_MLW 0x000003f0 /* Maximum Link Width */
#define PCI_EXP_LNKCAP_ASPMS 0x00000c00 /* ASPM Support */
#define PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */
Since you're using PCI_EXP_LNKCAP_ASPM_L0S below for writes to
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY, I assume
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY is another name for PCI_EXP_LNKCAP?
If so, it looks like we should also use PCI_EXP_LNKCAP_MLW instead of
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_WIDTH_MASK (although the
value is different for some reason; maybe 0x1f0 just reflects the
limits of brcmstb).
It would be nice to have a #define for the base of the PCIe
Capability so we could use that plus PCI_EXP_LNKCAP instead of
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY.
Hi Bjorn,
I believe this has come up before. Our "priv" registers should be exact mirrors of standard PCI config space registers to allow us to set RO fields. Unfortunately, sometimes the "priv" fields do not exactly match the standard version.
As you have noticed, our field for MAX_LINK_WIDTH_MASK is 0x1f0 instead of 0x3f0. The reason for this is not necessarily a BRCM limitation; our priv register has a CPM field mask of 0x200 (!) instead of where you would expect: 0x40000. Further, everything in this priv register is different from its counterpart from bit 18 and higher.
But you did something like that already for PCI_EXP_LNKCTL2 using
BRCM_PCIE_CAP_REGS (0x00ac), which means LNKCTL2 and LNKCAP must be
at:
LNKCTL2: 0x00dc (0x00ac + 0x30)
LNKCAP: 0x04dc (0x04d0 + 0x0c)
which doesn't look at all like they would both be in the actual PCIe
Capability format.
#define PCIE_RC_CFG_PRIV1_ROOT_CAP 0x4f8
#define PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK 0xf8
Just like the fields of our "priv" registers may be non-standard, so may be the addresses of our "priv" registers. The first few registers are in the same order as EXP_CAP, but then the rest are not, so using standard offsets does not work.
I don't know why HW did these things. I can add a comment to the code like "priv register addresses and fields may not correspond to standard PCI registers".
Regards,
Jim Quinlan
Broadcom STB/CM
From its usage to "un-advertise L1 substates",
PCIE_RC_CFG_PRIV1_ROOT_CAP looks like it might be related to
PCI_L1SS_CAP, but 0xf8 doesn't really match up to
PCI_L1SS_CAP_L1_PM_SS (0x10)
If this is really related to PCI_L1SS_CAP, can we use the names from
pci_regs.h somehow?
/* Don't advertise L0s capability if 'aspm-no-l0s' */
- aspm_support = PCIE_LINK_STATE_L1;
- if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
- aspm_support |= PCIE_LINK_STATE_L0S;
tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
- u32p_replace_bits(&tmp, aspm_support,
- PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+ if (of_property_read_bool(pcie->np, "aspm-no-l0s"))
+ tmp &= ~PCI_EXP_LNKCAP_ASPM_L0S;
writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
/* 'tmp' still holds the contents of PRIV1_LINK_CAPABILITY */