[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20260114185358.00004dce@huawei.com>
Date: Wed, 14 Jan 2026 18:53:58 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Terry Bowman <terry.bowman@....com>
CC: <dave@...olabs.net>, <dave.jiang@...el.com>, <alison.schofield@...el.com>,
<dan.j.williams@...el.com>, <bhelgaas@...gle.com>, <shiju.jose@...wei.com>,
<ming.li@...omail.com>, <Smita.KoralahalliChannabasappa@....com>,
<rrichter@....com>, <dan.carpenter@...aro.org>,
<PradeepVineshReddy.Kodamati@....com>, <lukas@...ner.de>,
<Benjamin.Cheatham@....com>, <sathyanarayanan.kuppuswamy@...ux.intel.com>,
<linux-cxl@...r.kernel.org>, <vishal.l.verma@...el.com>, <alucerop@....com>,
<ira.weiny@...el.com>, <linux-kernel@...r.kernel.org>,
<linux-pci@...r.kernel.org>
Subject: Re: [PATCH v14 02/34] PCI: Update CXL DVSEC definitions
On Wed, 14 Jan 2026 12:20:23 -0600
Terry Bowman <terry.bowman@....com> wrote:
> CXL DVSEC definitions were recently moved into uapi/pci_regs.h, but the
> newly added macros do not follow the file's existing naming conventions.
> The current format uses CXL_DVSEC_XYZ, while the new CXL entries must
> instead use the PCI_DVSEC_CXL_XYZ prefix to match the conventions already
> established in pci_regs.h.
>
> The new CXL DVSEC macros also introduce _MASK and _OFFSET suffixes, which
> are not used anywhere else in the file. These suffixes lengthen the
> identifiers and reduce readability. Remove _MASK and _OFFSET from the
> recently added definitions.
>
> Additionally, remove PCI_DVSEC_HEADER1_LENGTH, as it duplicates the existing
> PCI_DVSEC_HEADER1_LEN() macro.
>
> Update all existing references to use the new macro names.
>
> Finally, update the inline documentation to reference the latest revision
> of the CXL specification.
>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Dan Williams <dan.j.williams@...el.com>
>
> ---
>
> Changes in v13->v14:
> - New patch. Split from previous patch such that there is now a separate
> move patch and a format fix patch.
> - Formatting update requested (Bjorn)
> - Remove PCI_DVSEC_HEADER1_LENGTH_MASK because it duplicates
> PCI_DVSEC_HEADER1_LEN() (Bjorn)
> - Add Dan's review-by
A couple of name choices don't quite work to my reading.
I care more about the _DEVICE_ one than the other.
If consensus is that it is fine with out then I could probably be
persuaded.
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 6c4b6f19b18e..662582bdccf0 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> +/* CXL r4.0, 8.1.3: PCIe DVSEC for CXL Device */
> +#define PCI_DVSEC_CXL_DEVICE 0
> +#define PCI_DVSEC_CXL_CAP 0xA
Why drop the _DEVICE_ bit of these I'd kind of expect
#define PCI_DVSEC_CXL_DEVICE_CAP
to indicate which DVSEC it is in.
> +#define PCI_DVSEC_CXL_MEM_CAPABLE _BITUL(2)
> +#define PCI_DVSEC_CXL_HDM_COUNT __GENMASK(5, 4)
> +#define PCI_DVSEC_CXL_CTRL 0xC
> +#define PCI_DVSEC_CXL_MEM_ENABLE _BITUL(2)
> +#define PCI_DVSEC_CXL_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10))
> +#define PCI_DVSEC_CXL_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10))
> +#define PCI_DVSEC_CXL_MEM_INFO_VALID _BITUL(0)
> +#define PCI_DVSEC_CXL_MEM_ACTIVE _BITUL(1)
> +#define PCI_DVSEC_CXL_MEM_SIZE_LOW __GENMASK(31, 28)
> +#define PCI_DVSEC_CXL_RANGE_BASE_HIGH(i) (0x20 + (i * 0x10))
> +#define PCI_DVSEC_CXL_RANGE_BASE_LOW(i) (0x24 + (i * 0x10))
> +#define PCI_DVSEC_CXL_MEM_BASE_LOW __GENMASK(31, 28)
>
> +/* CXL r4.0, 8.1.4: Non-CXL Function Map DVSEC */
> +#define PCI_DVSEC_CXL_FUNCTION_MAP 2
> +
> +/* CXL r4.0, 8.1.5: Extensions DVSEC for Ports */
> +#define PCI_DVSEC_CXL_PORT 3
> +#define PCI_DVSEC_CXL_PORT_CTL 0x0c
> +#define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR 0x00000001
> +
> +/* CXL r4.0, 8.1.6: GPF DVSEC for CXL Port */
> +#define PCI_DVSEC_CXL_PORT_GPF 4
Nothing like ambiguous naming in the CXL spec as the
following fields sound like they are in the CXL_PORT dvsec
but they aren't. Well the spec avoids it with GPF_FOR_PORT
but we don't want to go there. I wonder...
PCI_DVSEC_CXL_PORTGPF maybe to avoid that?
Sigh. It's probably not worth it and does look horrible, so stick
with these.
> +#define PCI_DVSEC_CXL_PORT_GPF_PHASE_1_CONTROL 0x0C
> +#define PCI_DVSEC_CXL_PORT_GPF_PHASE_1_TMO_BASE __GENMASK(3, 0)
> +#define PCI_DVSEC_CXL_PORT_GPF_PHASE_1_TMO_SCALE __GENMASK(11, 8)
> +#define PCI_DVSEC_CXL_PORT_GPF_PHASE_2_CONTROL 0xE
> +#define PCI_DVSEC_CXL_PORT_GPF_PHASE_2_TMO_BASE __GENMASK(3, 0)
> +#define PCI_DVSEC_CXL_PORT_GPF_PHASE_2_TMO_SCALE __GENMASK(11, 8)
> +
> +/* CXL r4.0, 8.1.7: GPF DVSEC for CXL Device */
> +#define PCI_DVSEC_CXL_DEVICE_GPF 5
> +
> +/* CXL r4.0, 8.1.9: Register Locator DVSEC */
> +#define PCI_DVSEC_CXL_REG_LOCATOR 8
> +#define PCI_DVSEC_CXL_REG_LOCATOR_BLOCK1 0xC
> +#define PCI_DVSEC_CXL_REG_LOCATOR_BIR __GENMASK(2, 0)
> +#define PCI_DVSEC_CXL_REG_LOCATOR_BLOCK_ID __GENMASK(15, 8)
> +#define PCI_DVSEC_CXL_REG_LOCATOR_BLOCK_OFF_LOW __GENMASK(31, 16)
>
> #endif /* LINUX_PCI_REGS_H */
Powered by blists - more mailing lists