Re: [RFC PATCH 6/8] cxl: create emulated decoders for devices without HDM decoders

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 30 Nov 2022 16:12:49 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> CXL rev3.0 spec 8.1.3
> RCDs may not have HDM register blocks. Create fake decoders based on CXL
> PCIe DVSEC registers. The DVSEC Range Regiters provide the memory range for

Spell check (not that I can talk about spelling ;)

> these decoder structs. For the RCD, there can be up to 2 decoders depending
> on the DVSEC Capability register HDM_count.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

A few trivial things inline. LGTM otherwise.

> ---
>  drivers/cxl/core/hdm.c |   59 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 3a9e9b854587..60b6c141f514 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -721,6 +721,29 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>  	return 0;
>  }
>  
> +static int init_emulated_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> +				     struct cxl_endpoint_dvsec_info *info, int which)
> +{
> +	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
> +		return -ENODEV;
> +
> +	cxld->hpa_range = (struct range) {
> +		.start = info->dvsec_range[which].start,
> +		.end = info->dvsec_range[which].end,
> +	};
> +
> +	cxld->flags = CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> +	cxld->target_type = CXL_DECODER_ACCELERATOR;

Why accelerator? Comment needed.
blank line

> +	if (cxld->id != port->commit_end + 1) {
> +		dev_warn(&port->dev,
> +			 "decoder%d.%d: Committed out of order\n",
> +			 port->id, cxld->id);
> +		return -ENXIO;
> +	}
blank line

> +	port->commit_end = cxld->id;
blank line.
> +	return 0;
> +}
> +
>  static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  			    int *target_map, void __iomem *hdm, int which,
>  			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
> @@ -739,6 +762,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	if (is_endpoint_decoder(&cxld->dev))
>  		cxled = to_cxl_endpoint_decoder(&cxld->dev);
>  
> +	if (!hdm) {
> +		if (cxled)
> +			return init_emulated_hdm_decoder(port, cxld, info, which);
> +		else
> +			return -EINVAL;

		if (!cxled)
			return -EINVAL;

		return init_emulated_hdm_decoder(....);

I'd prefer puting the error case out of line and normal one in main flow
as much as possible.

> +	}
> +
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
>  	base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
>  	size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
> @@ -832,19 +862,15 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	return 0;
>  }
>  

> +
> +/**
> + * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
> + * @cxlhdm: Structure to populate with HDM capabilities

Docs update got missed somewhere.

> + */
> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				struct cxl_endpoint_dvsec_info *info)
> +{
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	struct cxl_port *port = cxlhdm->port;
> +	int i;
> +	u64 dpa_base = 0;
> +
> +	cxl_settle_decoders(cxlhdm);
>  
>  	for (i = 0; i < cxlhdm->decoder_count; i++) {
>  		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };
> 
>