Re: [PATCH 2/5] cxl: Move HDM decoder and register map definitions to include/cxl/pci.h
From: Srirangan Madhavan
Date: Fri Mar 06 2026 - 19:41:10 EST
> So moving them to the public header is just unnecessary code churn.
Apologies for that oversight. I can remove the unnecessary moves from this patch.
> Also, specifically regarding cxl_find_regblock(), where 5/ implements a
> new cxl_find_component_regblock(). The latter looks significantly
> similar to the original. Shouldn't we refactor the new PCI implemented
> version to be more generic rather than scope reduced, and export it for
> use by CXL code to avoid the duplication?
Regarding the duplication between cxl_find_component_regblock()
and the CXL subsystem's cxl_find_regblock():
I understand the duplication isn't ideal. I looked into generalizing the PCI version
(cxl_find_component_regblock) and exporting it for CXL to use, but exporting just
the find step from PCI while the rest of the register discovery pipeline stays in CXL
felt like an inconsistent split across the two subsystems. It seems like addressing
that would mean refactoring a significant portion of regs.c into the PCI subsystem,
which also doesn't appear correct.
Given that, would it be acceptable to keep the current scope-reduced version in the
PCI?
Please let me know if it would be acceptable to just refactor the cxl_find_component_regblock
and export it from pci.
Thank you.
Regards,
Srirangan.
________________________________________
From: Alex Williamson <alwilliamson@xxxxxxxxxx>
Sent: Friday, March 6, 2026 9:45 AM
To: Srirangan Madhavan
Cc: bhelgaas@xxxxxxxxxx; dan.j.williams@xxxxxxxxx; dave.jiang@xxxxxxxxx; jonathan.cameron@xxxxxxxxxx; ira.weiny@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; alison.schofield@xxxxxxxxx; dave@xxxxxxxxxxxx; Jeshua Smith; Vikram Sethi; Sai Yashwanth Reddy Kancherla; Vishal Aslot; Shanker Donthineni; Manish Honap; Vidya Sagar; Jiandi An; Matt Ochs; Derek Schumacher; linux-cxl@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 2/5] cxl: Move HDM decoder and register map definitions to include/cxl/pci.h
On Fri, 6 Mar 2026 08:00:16 +0000
<smadhavan@xxxxxxxxxx> wrote:
> From: Srirangan Madhavan <smadhavan@xxxxxxxxxx>
>
> Move CXL HDM decoder register defines, register map structs
> (cxl_reg_map, cxl_component_reg_map, cxl_device_reg_map,
> cxl_pmu_reg_map, cxl_register_map), cxl_hdm_decoder_count(),
> enum cxl_regloc_type, and cxl_find_regblock()/cxl_setup_regs()
> declarations from internal CXL headers to include/cxl/pci.h.
Regarding cxl_find_regblock()/cxl_setup_regs()..
> This makes them accessible to code outside the CXL subsystem, in
> particular the PCI core CXL state save/restore support added in a
> subsequent patch.
Yet in 5/:
The Register Locator DVSEC is parsed directly via PCI config space
reads rather than calling cxl_find_regblock()/cxl_setup_regs(),
since this code lives in the PCI core and must not depend on CXL
module symbols.
So moving them to the public header is just unnecessary code churn.
Also, specifically regarding cxl_find_regblock(), where 5/ implements a
new cxl_find_component_regblock(). The latter looks significantly
similar to the original. Shouldn't we refactor the new PCI implemented
version to be more generic rather than scope reduced, and export it for
use by CXL code to avoid the duplication? Thanks,
Alex
> No functional change.
>
> Signed-off-by: Srirangan Madhavan <smadhavan@xxxxxxxxxx>
> ---
> drivers/cxl/cxl.h | 107 +----------------------------------
> drivers/cxl/cxlpci.h | 10 ----
> include/cxl/pci.h | 129 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 130 insertions(+), 116 deletions(-)
> create mode 100644 include/cxl/pci.h
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 04c673e7cdb0..0c84695449d8 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -12,6 +12,7 @@
> #include <linux/node.h>
> #include <linux/io.h>
> #include <linux/range.h>
> +#include <cxl/pci.h>
>
> extern const struct nvdimm_security_ops *cxl_security_ops;
>
> @@ -23,63 +24,6 @@ extern const struct nvdimm_security_ops *cxl_security_ops;
> * (port-driver, region-driver, nvdimm object-drivers... etc).
> */
>
> -/* CXL 2.0 8.2.4 CXL Component Register Layout and Definition */
> -#define CXL_COMPONENT_REG_BLOCK_SIZE SZ_64K
> -
> -/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/
> -#define CXL_CM_OFFSET 0x1000
> -#define CXL_CM_CAP_HDR_OFFSET 0x0
> -#define CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0)
> -#define CM_CAP_HDR_CAP_ID 1
> -#define CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16)
> -#define CM_CAP_HDR_CAP_VERSION 1
> -#define CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20)
> -#define CM_CAP_HDR_CACHE_MEM_VERSION 1
> -#define CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24)
> -#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20)
> -
> -#define CXL_CM_CAP_CAP_ID_RAS 0x2
> -#define CXL_CM_CAP_CAP_ID_HDM 0x5
> -#define CXL_CM_CAP_CAP_HDM_VERSION 1
> -
> -/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */
> -#define CXL_HDM_DECODER_CAP_OFFSET 0x0
> -#define CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
> -#define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
> -#define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8)
> -#define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9)
> -#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11)
> -#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12)
> -#define CXL_HDM_DECODER_CTRL_OFFSET 0x4
> -#define CXL_HDM_DECODER_ENABLE BIT(1)
> -#define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10)
> -#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET(i) (0x20 * (i) + 0x14)
> -#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET(i) (0x20 * (i) + 0x18)
> -#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(i) (0x20 * (i) + 0x1c)
> -#define CXL_HDM_DECODER0_CTRL_OFFSET(i) (0x20 * (i) + 0x20)
> -#define CXL_HDM_DECODER0_CTRL_IG_MASK GENMASK(3, 0)
> -#define CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
> -#define CXL_HDM_DECODER0_CTRL_LOCK BIT(8)
> -#define CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
> -#define CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
> -#define CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11)
> -#define CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12)
> -#define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
> -#define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> -#define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
> -#define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
> -
> -/* HDM decoder control register constants CXL 3.0 8.2.5.19.7 */
> -#define CXL_DECODER_MIN_GRANULARITY 256
> -#define CXL_DECODER_MAX_ENCODED_IG 6
> -
> -static inline int cxl_hdm_decoder_count(u32 cap_hdr)
> -{
> - int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
> -
> - return val ? val * 2 : 1;
> -}
> -
> /* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
> static inline int eig_to_granularity(u16 eig, unsigned int *granularity)
> {
> @@ -246,51 +190,6 @@ struct cxl_regs {
> );
> };
>
> -struct cxl_reg_map {
> - bool valid;
> - int id;
> - unsigned long offset;
> - unsigned long size;
> -};
> -
> -struct cxl_component_reg_map {
> - struct cxl_reg_map hdm_decoder;
> - struct cxl_reg_map ras;
> -};
> -
> -struct cxl_device_reg_map {
> - struct cxl_reg_map status;
> - struct cxl_reg_map mbox;
> - struct cxl_reg_map memdev;
> -};
> -
> -struct cxl_pmu_reg_map {
> - struct cxl_reg_map pmu;
> -};
> -
> -/**
> - * struct cxl_register_map - DVSEC harvested register block mapping parameters
> - * @host: device for devm operations and logging
> - * @base: virtual base of the register-block-BAR + @block_offset
> - * @resource: physical resource base of the register block
> - * @max_size: maximum mapping size to perform register search
> - * @reg_type: see enum cxl_regloc_type
> - * @component_map: cxl_reg_map for component registers
> - * @device_map: cxl_reg_maps for device registers
> - * @pmu_map: cxl_reg_maps for CXL Performance Monitoring Units
> - */
> -struct cxl_register_map {
> - struct device *host;
> - void __iomem *base;
> - resource_size_t resource;
> - resource_size_t max_size;
> - u8 reg_type;
> - union {
> - struct cxl_component_reg_map component_map;
> - struct cxl_device_reg_map device_map;
> - struct cxl_pmu_reg_map pmu_map;
> - };
> -};
>
> void cxl_probe_component_regs(struct device *dev, void __iomem *base,
> struct cxl_component_reg_map *map);
> @@ -304,13 +203,9 @@ int cxl_map_device_regs(const struct cxl_register_map *map,
> int cxl_map_pmu_regs(struct cxl_register_map *map, struct cxl_pmu_regs *regs);
>
> #define CXL_INSTANCES_COUNT -1
> -enum cxl_regloc_type;
> int cxl_count_regblock(struct pci_dev *pdev, enum cxl_regloc_type type);
> int cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_type type,
> struct cxl_register_map *map, unsigned int index);
> -int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> - struct cxl_register_map *map);
> -int cxl_setup_regs(struct cxl_register_map *map);
> struct cxl_dport;
> resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
> struct cxl_dport *dport);
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 0cf64218aa16..9e825c039dd9 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -13,16 +13,6 @@
> */
> #define CXL_PCI_DEFAULT_MAX_VECTORS 16
>
> -/* Register Block Identifier (RBI) */
> -enum cxl_regloc_type {
> - CXL_REGLOC_RBI_EMPTY = 0,
> - CXL_REGLOC_RBI_COMPONENT,
> - CXL_REGLOC_RBI_VIRT,
> - CXL_REGLOC_RBI_MEMDEV,
> - CXL_REGLOC_RBI_PMU,
> - CXL_REGLOC_RBI_TYPES
> -};
> -
> /*
> * Table Access DOE, CDAT Read Entry Response
> *
> diff --git a/include/cxl/pci.h b/include/cxl/pci.h
> new file mode 100644
> index 000000000000..763a048262c7
> --- /dev/null
> +++ b/include/cxl/pci.h
> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __CXL_CXL_PCI_H__
> +#define __CXL_CXL_PCI_H__
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +#include <linux/types.h>
> +
> +/* Register Block Identifier (RBI) */
> +enum cxl_regloc_type {
> + CXL_REGLOC_RBI_EMPTY = 0,
> + CXL_REGLOC_RBI_COMPONENT,
> + CXL_REGLOC_RBI_VIRT,
> + CXL_REGLOC_RBI_MEMDEV,
> + CXL_REGLOC_RBI_PMU,
> + CXL_REGLOC_RBI_TYPES
> +};
> +
> +/* CXL 2.0 8.2.4 CXL Component Register Layout and Definition */
> +#define CXL_COMPONENT_REG_BLOCK_SIZE SZ_64K
> +
> +/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers */
> +#define CXL_CM_OFFSET 0x1000
> +#define CXL_CM_CAP_HDR_OFFSET 0x0
> +#define CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0)
> +#define CM_CAP_HDR_CAP_ID 1
> +#define CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16)
> +#define CM_CAP_HDR_CAP_VERSION 1
> +#define CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20)
> +#define CM_CAP_HDR_CACHE_MEM_VERSION 1
> +#define CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24)
> +#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20)
> +
> +#define CXL_CM_CAP_CAP_ID_RAS 0x2
> +#define CXL_CM_CAP_CAP_ID_HDM 0x5
> +#define CXL_CM_CAP_CAP_HDM_VERSION 1
> +
> +/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */
> +#define CXL_HDM_DECODER_CAP_OFFSET 0x0
> +#define CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
> +#define CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
> +#define CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8)
> +#define CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9)
> +#define CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11)
> +#define CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12)
> +#define CXL_HDM_DECODER_CTRL_OFFSET 0x4
> +#define CXL_HDM_DECODER_ENABLE BIT(1)
> +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10)
> +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET(i) (0x20 * (i) + 0x14)
> +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET(i) (0x20 * (i) + 0x18)
> +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(i) (0x20 * (i) + 0x1c)
> +#define CXL_HDM_DECODER0_CTRL_OFFSET(i) (0x20 * (i) + 0x20)
> +#define CXL_HDM_DECODER0_CTRL_IG_MASK GENMASK(3, 0)
> +#define CXL_HDM_DECODER0_CTRL_IW_MASK GENMASK(7, 4)
> +#define CXL_HDM_DECODER0_CTRL_LOCK BIT(8)
> +#define CXL_HDM_DECODER0_CTRL_COMMIT BIT(9)
> +#define CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10)
> +#define CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11)
> +#define CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12)
> +#define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
> +#define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> +#define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
> +#define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
> +
> +/* HDM decoder control register constants CXL 3.0 8.2.5.19.7 */
> +#define CXL_DECODER_MIN_GRANULARITY 256
> +#define CXL_DECODER_MAX_ENCODED_IG 6
> +
> +static inline int cxl_hdm_decoder_count(u32 cap_hdr)
> +{
> + int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
> +
> + return val ? val * 2 : 1;
> +}
> +
> +struct cxl_reg_map {
> + bool valid;
> + int id;
> + unsigned long offset;
> + unsigned long size;
> +};
> +
> +struct cxl_component_reg_map {
> + struct cxl_reg_map hdm_decoder;
> + struct cxl_reg_map ras;
> +};
> +
> +struct cxl_device_reg_map {
> + struct cxl_reg_map status;
> + struct cxl_reg_map mbox;
> + struct cxl_reg_map memdev;
> +};
> +
> +struct cxl_pmu_reg_map {
> + struct cxl_reg_map pmu;
> +};
> +
> +/**
> + * struct cxl_register_map - DVSEC harvested register block mapping parameters
> + * @host: device for devm operations and logging
> + * @base: virtual base of the register-block-BAR + @block_offset
> + * @resource: physical resource base of the register block
> + * @max_size: maximum mapping size to perform register search
> + * @reg_type: see enum cxl_regloc_type
> + * @component_map: cxl_reg_map for component registers
> + * @device_map: cxl_reg_maps for device registers
> + * @pmu_map: cxl_reg_maps for CXL Performance Monitoring Units
> + */
> +struct cxl_register_map {
> + struct device *host;
> + void __iomem *base;
> + resource_size_t resource;
> + resource_size_t max_size;
> + u8 reg_type;
> + union {
> + struct cxl_component_reg_map component_map;
> + struct cxl_device_reg_map device_map;
> + struct cxl_pmu_reg_map pmu_map;
> + };
> +};
> +
> +struct pci_dev;
> +
> +int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> + struct cxl_register_map *map);
> +int cxl_setup_regs(struct cxl_register_map *map);
> +
> +#endif /* __CXL_CXL_PCI_H__ */
> --
> 2.43.0
>