Re: [PATCH v4 10/10] cxl: add HDM decoder and IDE save/restore

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Tue, 20 Jan 2026 22:26:10 +0000
smadhavan@nvidia.com wrote:

> From: Srirangan Madhavan <smadhavan@nvidia.com>
> 
> Extend state save/restore to HDM decoder and IDE registers for Type 2
> devices. The HDM/IDE blocks are located via the component register map,
> then preserved across reset to retain decoder configuration and IDE
> policy. This avoids losing HDM/IDE programming when cxl_reset is issued.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>

Split the two up. One patch for HDM, one for IDE.

Been a while since I read the IDE stuff.

Isn't reset going to trip up the encryption to the extent that we need
to redo the key exchange etc?

> ---
>  drivers/cxl/core/regs.c |   7 ++
>  drivers/cxl/cxl.h       |   4 ++
>  drivers/cxl/pci.c       | 153 ++++++++++++++++++++++++++++++++++++++--
>  include/cxl/pci.h       |  43 +++++++++++
>  4 files changed, 201 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index ecdb22ae6952..76d6869d82ea 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -93,6 +93,12 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  			length = CXL_RAS_CAPABILITY_LENGTH;
>  			rmap = &map->ras;
>  			break;
> +		case CXL_CM_CAP_CAP_ID_IDE:
> +			dev_dbg(dev, "found IDE capability (0x%x)\n",
> +				offset);

Trivial: Fits on one line under 80 chars.

> +			length = CXL_IDE_CAPABILITY_LENGTH;
> +			rmap = &map->ide;
> +			break;

> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 7d6a0ef70b2d..eb735d0ae175 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c

> +/*
> + * CXL HDM Decoder register save/restore
> + */
> +static int cxl_save_hdm_state(struct cxl_dev_state *cxlds,
> +			      struct cxl_type2_saved_state *state)
> +{
> +	void __iomem *hdm = cxlds->regs.hdm_decoder;
> +	u32 cap, ctrl;
> +	int i, count;
> +
> +	if (!hdm)
> +		return 0;
> +
> +	cap = readl(hdm + CXL_HDM_DECODER_CAP_OFFSET);
> +	count = cap & CXL_HDM_DECODER_COUNT_MASK;

Use FIELD_GET() for this so we don't need to go check the definition to be
sure there isn't a shift.

> +	count = min(count, CXL_MAX_DECODERS);
> +
> +	state->hdm_decoder_count = count;
> +	state->hdm_global_ctrl = readl(hdm + CXL_HDM_DECODER_GLOBAL_CTRL_OFFSET);
> +
> +	for (i = 0; i < count; i++) {
> +		struct cxl_hdm_decoder_state *d = &state->decoders[i];
> +		u32 base_low, base_high, size_low, size_high;
> +		u32 dpa_skip_low, dpa_skip_high;
> +
> +		base_low = readl(hdm + CXL_HDM_DECODER_BASE_LOW(i));
> +		base_high = readl(hdm + CXL_HDM_DECODER_BASE_HIGH(i));
> +		size_low = readl(hdm + CXL_HDM_DECODER_SIZE_LOW(i));
> +		size_high = readl(hdm + CXL_HDM_DECODER_SIZE_HIGH(i));
> +		ctrl = readl(hdm + CXL_HDM_DECODER_CTRL(i));
> +		dpa_skip_low = readl(hdm + CXL_HDM_DECODER_DPA_SKIP_LOW(i));
> +		dpa_skip_high = readl(hdm + CXL_HDM_DECODER_DPA_SKIP_HIGH(i));
> +
> +		d->base = ((u64)base_high << 32) | base_low;
> +		d->size = ((u64)size_high << 32) | size_low;
> +		d->ctrl = ctrl;
> +		d->dpa_skip = ((u64)dpa_skip_high << 32) | dpa_skip_low;
> +		d->enabled = !!(ctrl & CXL_HDM_DECODER_ENABLE);

I'm lazy. Why not just stash the register values as a raw block and
put them back? Do we want to put them back if they aren't locked?
We ripped down the region so I'd be kind of expecting the type 2
driver for the specific hardware to need to put this stuff back
as part of it's own reinit.


> +	}
> +
> +	return 0;
> +}