From: Bjorn Helgaas <helgaas@kernel.org>
To: Jim Quinlan <james.quinlan@broadcom.com>
Cc: linux-pci@vger.kernel.org,
"Nicolas Saenz Julienne" <nsaenz@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
bcm-kernel-feedback-list@broadcom.com, jim2101024@gmail.com,
"Florian Fainelli" <florian.fainelli@broadcom.com>,
"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
"Krzysztof Wilczyński" <kwilczynski@kernel.org>,
"Manivannan Sadhasivam" <mani@kernel.org>,
"Rob Herring" <robh@kernel.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-rpi-kernel@lists.infradead.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-arm-kernel@lists.infradead.org>,
"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant
Date: Wed, 22 Oct 2025 17:45:53 -0500 [thread overview]
Message-ID: <20251022224553.GA1271981@bhelgaas> (raw)
In-Reply-To: <20251003170436.1446030-1-james.quinlan@broadcom.com>
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 */
next prev parent reply other threads:[~2025-10-22 22:45 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-03 17:04 [PATCH] PCI: brcmstb: Fix use of incorrect constant Jim Quinlan
2025-10-06 19:35 ` Florian Fainelli
2025-10-20 6:48 ` Manivannan Sadhasivam
2025-10-22 22:45 ` Bjorn Helgaas [this message]
2025-10-28 20:01 ` James Quinlan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251022224553.GA1271981@bhelgaas \
--to=helgaas@kernel.org \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=bhelgaas@google.com \
--cc=florian.fainelli@broadcom.com \
--cc=james.quinlan@broadcom.com \
--cc=jim2101024@gmail.com \
--cc=kwilczynski@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=lpieralisi@kernel.org \
--cc=mani@kernel.org \
--cc=nsaenz@kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).