Re: [PATCH v9 06/27] cxl: add function for type2 cxl regs setup

[Alejandro Lucero Palau <alucerop@amd.com>]

On 1/18/25 01:51, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Create a new function for a type2 device initialising
>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> Reviewed-by: Fan Ni <fan.ni@samsung.com>
>> ---
>>   drivers/cxl/core/pci.c | 51 ++++++++++++++++++++++++++++++++++++++++++
>>   include/cxl/cxl.h      |  2 ++
>>   2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 5821d582c520..493ab33fe771 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -1107,6 +1107,57 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
>>   
>> +static int cxl_pci_setup_memdev_regs(struct pci_dev *pdev,
>> +				     struct cxl_dev_state *cxlds)
>> +{
>> +	struct cxl_register_map map;
>> +	int rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
>> +				cxlds->capabilities);
>> +	/*
>> +	 * This call can return -ENODEV if regs not found. This is not an error
>> +	 * for Type2 since these regs are not mandatory. If they do exist then
>> +	 * mapping them should not fail. If they should exist, it is with driver
>> +	 * calling cxl_pci_check_caps where the problem should be found.
>> +	 */
> There is no common definition of type-2 so the core should not try to
> assume it knows, or be told what is mandatory. Just export the raw
> helpers and leave it to the caller to make these decisions.


The code does not know, but it knows it does not know, therefore handles 
this new situation not needed before Type2 support in the generic code 
for the pci driver and Type3.

This is added to the API for accel drivers following the design 
restrictions I have commented earlier in another patch. Your suggestion 
seems to go against that decision what was implicitly taken after the 
first versions and which had no complains until now.

More about this same issue below.


>> +	if (rc == -ENODEV)
>> +		return 0;
>> +
>> +	if (rc)
>> +		return rc;
>> +
>> +	return cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>> +}
>> +
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
>> +{
>> +	int rc;
>> +
>> +	rc = cxl_pci_setup_memdev_regs(pdev, cxlds);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>> +				&cxlds->reg_map, cxlds->capabilities);
>> +	if (rc) {
>> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	if (!test_bit(CXL_CM_CAP_CAP_ID_RAS, cxlds->capabilities))
>> +		return rc;
>> +
>> +	rc = cxl_map_component_regs(&cxlds->reg_map,
>> +				    &cxlds->regs.component,
>> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
>> +	if (rc)
>> +		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, "CXL");
> Only after we have multiple instances of CXL accelerator drivers that
> start copying the same init code should a helper be created that wraps
> that duplication. Otherwise move this probing and error determination
> out to SFC for now.

I do not think moving this to the accel driver makes sense at this 
point, but I think it is worth to try to share this as much as possible 
with the current pci driver for Type3.