RE: [PATCH v3 4/8] cxl: emulate HDM decoder from DVSEC range registers

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang wrote:
> In the case where HDM decoder register block exists but is not programmed
> and at the same time the DVSEC range register range is active, populate the
> CXL decoder object 'cxl_decoder' with info from DVSEC range registers.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v2:
> - Set target_type to CXL_DECODER_EXPANDER (type 3). (Jonathan)
> - Skip HDM enabling if DVSEC range is active. (Jonathan)
> ---
>  drivers/cxl/core/hdm.c |   36 +++++++++++++++++++++++++++++++++---
>  drivers/cxl/core/pci.c |    2 +-
>  drivers/cxl/cxl.h      |    3 ++-
>  drivers/cxl/port.c     |    2 +-
>  4 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index dcc16d7cb8f3..af1f5f906f52 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -679,9 +679,34 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
>  	return 0;
>  }
>  
> +static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
> +					    struct cxl_decoder *cxld, int which,
> +					    struct cxl_endpoint_dvsec_info *info)
> +{
> +	if (!is_cxl_endpoint(port))
> +		return -EOPNOTSUPP;
> +
> +	if (info->dvsec_range[which].start == CXL_RESOURCE_NONE)
> +		return -ENXIO;
> +
> +	cxld->target_type = CXL_DECODER_EXPANDER;
> +	cxld->commit = NULL;
> +	cxld->reset = NULL;
> +
> +	cxld->hpa_range = (struct range) {
> +		.start = info->dvsec_range[which].start,
> +		.end = info->dvsec_range[which].end,
> +	};

They're both 'struct range' values so this can just be an implict
memcpy:

	cxld->hpa_range = info->dvsec_range[which];

> +	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> +	port->commit_end = cxld->id;

I think F_LOCK should ultimately come from whether the associated CFMWS is locked,
but for now perhaps a comment like:

/*
 * Set the emulated decoder as locked pending additional support to
 * change the range registers at run time.
 */

...basically just to indicate that there is no requirement that they be
locked, but that the unlock case needs quite a bit more work and
testing.

> +
> +	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)
> +			    u64 *dpa_base, struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_endpoint_decoder *cxled = NULL;
>  	u64 size, base, skip, dpa_size;
> @@ -717,6 +742,10 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		.end = base + size - 1,
>  	};
>  
> +	if (cxled && !committed &&
> +	    info->dvsec_range[which].start != CXL_RESOURCE_NONE)

Ah, here is where that CXL_RESOURCE_NONE is used, can this condition
just be:

    range_len(&info->dvsec_range[which])

...i.e. non-zero size?

> +		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> +
>  	/* decoders are enabled if committed */
>  	if (committed) {
>  		cxld->flags |= CXL_DECODER_F_ENABLE;
> @@ -790,7 +819,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
>   * @cxlhdm: Structure to populate with HDM capabilities
>   */
> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
> +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;
> @@ -842,7 +872,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  			cxld = &cxlsd->cxld;
>  		}
>  
> -		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base);
> +		rc = init_hdm_decoder(port, cxld, target_map, hdm, i, &dpa_base, info);

Looks like a long line for clang-format to trim.

>  		if (rc) {
>  			put_device(&cxld->dev);
>  			return rc;
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 97690c429e05..81882ea94adf 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -427,7 +427,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	 * Decoder Capability Enable.
>  	 */
>  	if (info->mem_enabled)
> -		return -EBUSY;
> +		return 0;
>  
>  	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>  	if (rc)
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1057affb2db0..ea9548cbc7eb 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -644,7 +644,8 @@ struct cxl_endpoint_dvsec_info {
>  
>  struct cxl_hdm;
>  struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
> -int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm);
> +int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> +				struct cxl_endpoint_dvsec_info *info);
>  int devm_cxl_add_passthrough_decoder(struct cxl_port *port);
>  int cxl_dvsec_rr_decode(struct pci_dev *pdev, int dvsec,
>  			struct cxl_endpoint_dvsec_info *info);
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index 404639a1c3d0..7f1b71c5cf15 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -79,7 +79,7 @@ static int cxl_port_probe(struct device *dev)
>  		}
>  	}
>  
> -	rc = devm_cxl_enumerate_decoders(cxlhdm);
> +	rc = devm_cxl_enumerate_decoders(cxlhdm, &info);
>  	if (rc) {
>  		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
>  		return rc;
> 
>