brcmstb PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK issue

[Jim Quinlan]

On Thu, Sep 25, 2025 at 3:44 PM Bjorn Helgaas <helgaas at kernel.org> wrote:
>
> I think caab002d5069 ("PCI: brcmstb: Disable L0s component of ASPM if
> requested") introduced a problem:
>
>   #define PCIE_LINK_STATE_L0S             (BIT(0) | BIT(1)) /* Upstr/dwnstr L0s */
>   #define PCIE_LINK_STATE_L1              BIT(2)  /* L1 state */
>
>   +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK   0xc00
>
>   +       /* 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);
>
>
> PCIE_LINK_STATE_L0S is two bits, PCIE_LINK_STATE_L1 is one bit, and
> the u32p_replace_bits() tries to put all three bits into the two-bit
> PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK field.


Thanks for the catch.  It is a bug.  It did not have any effect until
some point after 5.15,
when PCIE_LINK_STATE_L1 went from BIT(1) to BIT(2).  Regardless, I
should have been ORing
PCI_EXP_LNKCAP_ASPM_{L1,,L0s} all along.

I'll submit a fix ASAP.

Thanks & regards,
Jim Quinlan
Broadcom STB/CM


>
> Interestingly, the compiler only warns about this when !CONFIG_OF
> because in that case the of_property_read_bool() stub always returns
> "false".  A Kconfig tweak is needed to build this with !CONFIG_OF.