Re: [PATCH v2 04/15] cxl: add capabilities field to cxl_dev_state

Alejandro Lucero Palau
2024-08-15T15:44:21Z

On 8/4/24 18:22, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 18:28:24 +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 a CXL accelerator
>> implements.
>>
>> Add a new field for keeping device capabilities to be initialised by
>> Type2 drivers. Advertise all those capabilities for Type3.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> In general seems a reasonable approach, so just minor comments.
>
>> ---
>> drivers/cxl/core/mbox.c | 1 +
>> drivers/cxl/core/memdev.c | 4 +++-
>> drivers/cxl/core/port.c | 2 +-
>> drivers/cxl/core/regs.c | 11 ++++++-----
>> drivers/cxl/cxl.h | 2 +-
>> drivers/cxl/cxlmem.h | 4 ++++
>> drivers/cxl/pci.c | 15 +++++++++------
>> drivers/net/ethernet/sfc/efx_cxl.c | 3 ++-
>> include/linux/cxl_accel_mem.h | 5 ++++-
>> 9 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index 2626f3fff201..2ba7d36e3f38 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1424,6 +1424,7 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
>> mds->cxlds.reg_map.host = dev;
>> mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
>> mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
>> + mds->cxlds.capabilities = CXL_DRIVER_CAP_HDM | CXL_DRIVER_CAP_MBOX;
> Add a reference for this perhaps. Make it clear that a type3 device must
> support mailbox and hdm by pointing at requirement for the various structures
> in a spec reference.
>

I think it would be worth to have documentation, distilling out
dis-ambiguities from the specs about mandatory/optional registers.

>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index af8169ccdbc0..8f2a820bd92d 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -405,6 +405,9 @@ struct cxl_dpa_perf {
>> int qos_class;
>> };
>>
>> +#define CXL_DRIVER_CAP_HDM 0x1
>> +#define CXL_DRIVER_CAP_MBOX 0x2
>> +
> Enum and BIT() for the defines. Avoids someone in future
> thinking they can define 0x3 to be something.
>
> Definitely only one definition as well. Seems reasonable for
> this to be CXL wide.
>

OK.

Thanks!

>> /**
>> * struct cxl_dev_state - The driver device state
>> *
>> @@ -438,6 +441,7 @@ struct cxl_dev_state {
>> struct resource ram_res;
>> u64 serial;
>> enum cxl_devtype type;
>> + uint8_t capabilities;
>> };