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

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

On Mon, 09 Jan 2023 14:43:54 -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 Registers provide the memory range
> for 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>

Second half of this patch seems to be an unrelated refactor...
I missed that entirely when reviewing v1. 


> 
> ---
> v2:
> - Refactor to put error case out of line. (Jonathan)
> - kdoc update. (Jonathan)
> - Remove init_emulated_hdm_decoder(), duplicate of
>   cxl_setup_hdm_decoder_from_dvsec().
> ---
>  drivers/cxl/core/hdm.c |   37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 165c0f382ce1..ed5e9ef3aa9b 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -747,6 +747,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 -EINVAL;
> +
> +		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, which, info);
> +	}
> +
>  	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));
> @@ -840,19 +847,15 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	return 0;
>  }

I have no problem with what follows, but I can't see a clear connection to rest of
the patch.

>  
> -/**
> - * 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,
> -				struct cxl_endpoint_dvsec_info *info)

Note the docs were wrong before this point...  Probably fix them earlier.

> +static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
>  {
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> -	struct cxl_port *port = cxlhdm->port;
> -	int i, committed;
> -	u64 dpa_base = 0;
> +	int committed, i;
>  	u32 ctrl;
>  
> +	if (!hdm)
> +		return;
> +
>  	/*
>  	 * Since the register resource was recently claimed via request_region()
>  	 * be careful about trusting the "not-committed" status until the commit
> @@ -869,6 +872,22 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  	/* ensure that future checks of committed can be trusted */
>  	if (committed != cxlhdm->decoder_count)
>  		msleep(20);
> +}
> +
> +/**
> + * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
> + * @cxlhdm: Structure to populate with HDM capabilities
> + * @info: cached DVSEC range register info
> + */
> +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 };
> 
> 
>