Re: [PATCH v11 07/20] cxl/hdm: Use stored Component Register mappings to map HDM decoder capability

[Terry Bowman <Terry.Bowman@amd.com>]

Hi Jonathan,

I added responses below.

On 10/2/23 09:43, Jonathan Cameron wrote:
> On Wed, 27 Sep 2023 17:43:26 +0200
> Robert Richter <rrichter@amd.com> wrote:
> 
>> Now, that the Component Register mappings are stored, use them to
>> enable and map the HDM decoder capabilities. The Component Registers
>> do not need to be probed again for this, remove probing code.
>>
>> The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
>> Endpoint's component register mappings are located in the cxlds and
>> else in the port's structure. Duplicate the cxlds->reg_map in
>> port->reg_map for endpoint ports.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> [rework to drop cxl_port_get_comp_map()]
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> A few comments inline.
> 
> Also, Dan's SoB doesn't make sense if you are the Author and he's not
> the one sending the email.  The fun of patches bounced back and forwards
> is sometimes you have to tweak this stuff on each posting... :(
> 
>> ---
>>  drivers/cxl/core/hdm.c  | 48 ++++++++++++++++-------------------------
>>  drivers/cxl/core/port.c | 29 +++++++++++++++++++------
>>  drivers/cxl/mem.c       |  5 ++---
>>  3 files changed, 43 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 11d9971f3e8c..14a0d0017df3 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -81,26 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>>  		cxlhdm->interleave_mask |= GENMASK(14, 12);
>>  }
>>  
>> -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
>> -				struct cxl_component_regs *regs)
>> -{
>> -	struct cxl_register_map map = {
>> -		.host = &port->dev,
>> -		.resource = port->component_reg_phys,
>> -		.base = crb,
>> -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
>> -	};
>> -
>> -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
>> -	if (!map.component_map.hdm_decoder.valid) {
>> -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
>> -		/* unique error code to indicate no HDM decoder capability */
>> -		return -ENODEV;
>> -	}
>> -
>> -	return cxl_map_component_regs(&map, regs, BIT(CXL_CM_CAP_CAP_ID_HDM));
>> -}
>> -
>>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>>  {
>>  	struct cxl_hdm *cxlhdm;
>> @@ -155,7 +135,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>>  {
>>  	struct device *dev = &port->dev;
>>  	struct cxl_hdm *cxlhdm;
>> -	void __iomem *crb;
>> +	struct cxl_register_map *reg_map;
>>  	int rc;
>>  
>>  	cxlhdm = devm_kzalloc(dev, sizeof(*cxlhdm), GFP_KERNEL);
>> @@ -164,19 +144,29 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
>>  	cxlhdm->port = port;
>>  	dev_set_drvdata(dev, cxlhdm);
>>  
>> -	crb = ioremap(port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
>> -	if (!crb && info && info->mem_enabled) {
>> -		cxlhdm->decoder_count = info->ranges;
>> -		return cxlhdm;
>> -	} else if (!crb) {
>> +	reg_map = &port->reg_map;
> 
> Could you set this where it's defined above?
> 

Yes.

>> +	if (reg_map->resource == CXL_RESOURCE_NONE) {
> 
> A reminder comment on why/when this happens might be a good addition.
>

Yes, we will add.
 
>> +		if (info && info->mem_enabled) {
>> +			cxlhdm->decoder_count = info->ranges;
>> +			return cxlhdm;
>> +		}
> 
> Trivial (and true before this patch) but I'd rather see the error path out of line
> 

Ok, we will change the conditional check to be for the error case and the default path 
for success.

> 
> 		if (!info || !info->mem_enabled) {
> 			WARN_ON(1);
> 			dev_err(dev, "No ...
> 			...
> 		}
> 
> 		cxlhdm->decoder_count = info->ranges;
> 		return cxlhdm;
> 	}
> 
> 
>> +		WARN_ON(1);
>>  		dev_err(dev, "No component registers mapped\n");
>>  		return ERR_PTR(-ENXIO);
>>  	}
>>  
>> -	rc = map_hdm_decoder_regs(port, crb, &cxlhdm->regs);
>> -	iounmap(crb);
>> -	if (rc)
>> +	if (!reg_map->component_map.hdm_decoder.valid) {
>> +		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
>> +		/* unique error code to indicate no HDM decoder capability */
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	rc = cxl_map_component_regs(reg_map, &cxlhdm->regs,
>> +				    BIT(CXL_CM_CAP_CAP_ID_HDM));
>> +	if (rc) {
>> +		dev_dbg(dev, "Failed to map HDM capability.\n");
> dev_err() seems appropriate here.
> 

Yes, we will change dev_dbg() here to use dev_err().

Regards,
Terry

>>  		return ERR_PTR(rc);
>> +	}
>>  
>>  	parse_hdm_decoder_caps(cxlhdm);
>>  	if (cxlhdm->decoder_count == 0) {
>