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

[Dave Jiang <dave.jiang@intel.com>]

On 2/7/23 3:39 PM, Dan Williams wrote:
> 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];
> 

ok

>> +	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.
>   */
> 
ok

> ...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?

In order to do this, I think we need to do
.start = 0,
.end = CXL_RESOURCE_NONE,

So range_len() == 0 in the end.

> 
>> +		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.

ok

> 
>>   		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;
>>
>>
> 
>