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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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.

> 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.


>  /**
>   * 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;
>  };