Re: [PATCH v9 03/27] cxl: add capabilities field to cxl_dev_state and cxl_port

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

On 1/18/25 01:37, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Type2 devices have some Type3 functionalities as optional like an mbox
>> or an hdm decoder, and CXL core needs a way to know what an CXL accelerator
>> implements.
>>
>> Add a new field to cxl_dev_state for keeping device capabilities as
>> discovered during initialization. Add same field to cxl_port as registers
>> discovery is also used during port initialization.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
>> Reviewed-by: Fan Ni <fan.ni@samsung.com>
>> ---
>>   drivers/cxl/core/port.c | 11 +++++++----
>>   drivers/cxl/core/regs.c | 23 ++++++++++++++++-------
>>   drivers/cxl/cxl.h       |  9 ++++++---
>>   drivers/cxl/cxlmem.h    |  2 ++
>>   drivers/cxl/pci.c       | 10 ++++++----
>>   include/cxl/cxl.h       | 19 +++++++++++++++++++
>>   6 files changed, 56 insertions(+), 18 deletions(-)
>>
> [..]
>> @@ -113,11 +118,12 @@ EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, "CXL");
>>    * @dev: Host device of the @base mapping
>>    * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
>>    * @map: Map object describing the register block information found
>> + * @caps: capabilities to be set when discovered
>>    *
>>    * Probe for device register information and return it in map object.
>>    */
>>   void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>> -			   struct cxl_device_reg_map *map)
>> +			   struct cxl_device_reg_map *map, unsigned long *caps)
>>   {
>>   	int cap, cap_count;
>>   	u64 cap_array;
>> @@ -146,10 +152,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>>   		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
>>   			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
>>   			rmap = &map->status;
>> +			set_bit(CXL_DEV_CAP_DEV_STATUS, caps);
>>   			break;
>>   		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
>>   			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
>>   			rmap = &map->mbox;
>> +			set_bit(CXL_DEV_CAP_MAILBOX_PRIMARY, caps);
>>   			break;
>>   		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
>>   			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
>> @@ -157,6 +165,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>>   		case CXLDEV_CAP_CAP_ID_MEMDEV:
>>   			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
>>   			rmap = &map->memdev;
>> +			set_bit(CXL_DEV_CAP_MEMDEV, caps);
> I do not understand the rationale for a capability bitmap. There is
> already a 'valid' flag in 'struct cxl_reg_map' for all register blocks.
> Any optional core functionality should key off those existing flags.


The current code is based on Type3 and the registers and capabilities 
are defined as mandatory, I think except RAS.

With Type2 we have optional capabilities like mailbox and hdm, and the 
code probing the regs should not make any assumption about what should 
be there.

With this patchset the capabilities to expect are set by the accel 
driver and compared with those discovered when probing CXL regs. 
Although the capabilities check could use the cxl_reg_map, I consider it 
is convenient to have a capability bitmap for keeping those discovered 
and easily checking them against those expected by the accel driver, and 
reporting them (if necessary) as well without further processing.