Re: [PATCH v3 02/20] cxl: add capabilities field to cxl_dev_state and cxl_port

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

On 9/13/24 18:25, Jonathan Cameron wrote:
> On Sat, 7 Sep 2024 09:18:18 +0100
> alejandro.lucero-palau@amd.com 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 for keeping device capabilities as discovered during
>> initialization.
>>
>> Add same field to cxl_port which for an endpoint will use those
>> capabilities discovered previously, and which will be initialized when
>> calling cxl_port_setup_regs for no endpoints.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Hi,
>
> My only real suggestion on this one is to use a bitmap to make
> it easy to extend the capabilities as needed in future.
> That means passing an unsigned long pointer around.
>

It makes sense.

I'll do.

>> @@ -600,6 +600,7 @@ struct cxl_dax_region {
>>    * @cdat: Cached CDAT data
>>    * @cdat_available: Should a CDAT attribute be available in sysfs
>>    * @pci_latency: Upstream latency in picoseconds
>> + * @capabilities: those capabilities as defined in device mapped registers
>>    */
>>   struct cxl_port {
>>   	struct device dev;
>> @@ -623,6 +624,7 @@ struct cxl_port {
>>   	} cdat;
>>   	bool cdat_available;
>>   	long pci_latency;
>> +	u32 capabilities;
> Use DECLARE_BITMAP() for this to make life easy should we ever
> have more than 32.
>
>>   };
>>   
>>   /**
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index afb53d058d62..37c043100300 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -424,6 +424,7 @@ struct cxl_dpa_perf {
>>    * @ram_res: Active Volatile memory capacity configuration
>>    * @serial: PCIe Device Serial Number
>>    * @type: Generic Memory Class device or Vendor Specific Memory device
>> + * @capabilities: those capabilities as defined in device mapped registers
>>    */
>>   struct cxl_dev_state {
>>   	struct device *dev;
>> @@ -438,6 +439,7 @@ struct cxl_dev_state {
>>   	struct resource ram_res;
>>   	u64 serial;
>>   	enum cxl_devtype type;
>> +	u32 capabilities;
> As above, use a bitmap and access it with the various bitmap operators
> so that we aren't constrained to 32 bits given half are
> used already.


I though maybe I should use u64 ...


>
>>   };
>>   
>>   /**
>> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
>> index e78eefa82123..930b1b9c1d6a 100644
>> --- a/include/linux/cxl/cxl.h
>> +++ b/include/linux/cxl/cxl.h
>> @@ -12,6 +12,36 @@ enum cxl_resource {
>>   	CXL_ACCEL_RES_PMEM,
>>   };
>>   
>> +/* Capabilities as defined for:
> Trivial but cxl tends to do
> /*
>   * Capabilities ..
>
> style multiline comments.


Right. I'll fit it.


>> + *
>> + *	Component Registers (Table 8-22 CXL 3.0 specification)
>> + *	Device Registers (8.2.8.2.1 CXL 3.0 specification)
>> + */
>> +
>> +enum cxl_dev_cap {
>> +	/* capabilities from Component Registers */
>> +	CXL_DEV_CAP_RAS,
>> +	CXL_DEV_CAP_SEC,
>> +	CXL_DEV_CAP_LINK,
>> +	CXL_DEV_CAP_HDM,
>> +	CXL_DEV_CAP_SEC_EXT,
>> +	CXL_DEV_CAP_IDE,
>> +	CXL_DEV_CAP_SNOOP_FILTER,
>> +	CXL_DEV_CAP_TIMEOUT_AND_ISOLATION,
>> +	CXL_DEV_CAP_CACHEMEM_EXT,
>> +	CXL_DEV_CAP_BI_ROUTE_TABLE,
>> +	CXL_DEV_CAP_BI_DECODER,
>> +	CXL_DEV_CAP_CACHEID_ROUTE_TABLE,
>> +	CXL_DEV_CAP_CACHEID_DECODER,
>> +	CXL_DEV_CAP_HDM_EXT,
>> +	CXL_DEV_CAP_METADATA_EXT,
>> +	/* capabilities from Device Registers */
>> +	CXL_DEV_CAP_DEV_STATUS,
>> +	CXL_DEV_CAP_MAILBOX_PRIMARY,
>> +	CXL_DEV_CAP_MAILBOX_SECONDARY,
> Dan raised this one already - definitely not something any
> driver in Linux should be aware of.  Hence just drop this entry.


You mean never ever or just by now?

Maybe I'm missing something specific to the secondary mailbox, but if 
the rule is if none used yet, do not add it, then there are other caps I 
should remove as well.


>> +	CXL_DEV_CAP_MEMDEV,
>> +};
>> +
>>   struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
>>   
>>   void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec);