[PATCH] PCI: brcmstb: Fix use of incorrect constant

[PATCH] PCI: brcmstb: Fix use of incorrect constant

[Jim Quinlan]

The driver was using the PCIE_LINK_STATE_L1 constant as a field mask for
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);

In addition, the driver was assuming that the HW is ASPM L1 capable when it
should not be telling the HW what it is capable of.

Fixes: caab002d5069 ("PCI: brcmstb: Disable L0s component of ASPM if requested")
Reported-by: Bjorn Helgaas <bhelgaas@google.com>

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 9afbd02ded35..7e9b2f6a604a 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -48,7 +48,6 @@
 
 #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
 
 #define PCIE_RC_CFG_PRIV1_ROOT_CAP			0x4f8
 #define  PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK	0xf8
@@ -1075,7 +1074,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	void __iomem *base = pcie->base;
 	struct pci_host_bridge *bridge;
 	struct resource_entry *entry;
-	u32 tmp, burst, aspm_support, num_lanes, num_lanes_cap;
+	u32 tmp, burst, num_lanes, num_lanes_cap;
 	u8 num_out_wins = 0;
 	int num_inbound_wins = 0;
 	int memc, ret;
@@ -1175,12 +1174,9 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 
 
 	/* 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 */

base-commit: 4ff71af020ae59ae2d83b174646fc2ad9fcd4dc4
-- 
2.34.1

Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant

[Florian Fainelli]

On 10/3/2025 10:04 AM, Jim Quinlan wrote:
> The driver was using the PCIE_LINK_STATE_L1 constant as a field mask for
> 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);
> 
> In addition, the driver was assuming that the HW is ASPM L1 capable when it
> should not be telling the HW what it is capable of.
> 
> Fixes: caab002d5069 ("PCI: brcmstb: Disable L0s component of ASPM if requested")
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant

[Manivannan Sadhasivam]

On Fri, 03 Oct 2025 13:04:36 -0400, Jim Quinlan wrote:
> The driver was using the PCIE_LINK_STATE_L1 constant as a field mask for
> 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);
> 
> In addition, the driver was assuming that the HW is ASPM L1 capable when it
> should not be telling the HW what it is capable of.
> 
> [...]

Applied, thanks!

[1/1] PCI: brcmstb: Fix use of incorrect constant
      commit: ad6014f77f6b66e862a912b5aa4571d00ab30405

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>

Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant

[Bjorn Helgaas]

On Fri, Oct 03, 2025 at 01:04:36PM -0400, Jim Quinlan wrote:
> The driver was using the PCIE_LINK_STATE_L1 constant as a field mask for
> 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

This all tangential questions for possible future changes, not
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.

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

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 */

Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant

[James Quinlan]

On 10/22/25 18:45, Bjorn Helgaas wrote:
> On Fri, Oct 03, 2025 at 01:04:36PM -0400, Jim Quinlan wrote:
>> The driver was using the PCIE_LINK_STATE_L1 constant as a field mask for
>> 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
> This all tangential questions for possible future changes, not
> 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?
yes and no, see below.
>
> 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 */