Re: [RFC 3/5] cxl: Separate coherence from target type

[Ben Cheatham]

Hi Huang, quick comments in this patch and the next.

On 9/24/24 9:46 PM, Huang Ying wrote:
> Previously, target type (expander or accelerator) and
> coherence (HOSTONLY (HDM-H) or DEV (HDM-D/DB)) are synonym.  So target
> type is used to designate coherence too.  However, it's possible for
> expanders to use HDM-DB now.  So, we need to separate coherence from
> target type.
> 
> Accordingly, the HOSTONLY field of decoder ctrl
> register (CXL_HDM_DECODER0_CTRL_HOSTONLY) need to be set according to
> coherence.
> 
> The coherence of decoders can not be determined via target type too.
> So, accelerator/expander device drivers need to specify coherence
> explicitly via newly added coherence field in struct cxl_dev_state.
> 
> The coherence of each end points in a region need to be same.  So, the
> coherence of the first end point is recorded in struct region.  Which
> will be checked against the coherence of all other end points.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
> Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> Cc: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> Cc: Dave Jiang <dave.jiang@xxxxxxxxx>
> Cc: Alison Schofield <alison.schofield@xxxxxxxxx>
> Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> Cc: Alejandro Lucero <alucerop@xxxxxxx>
> ---
>  drivers/cxl/core/hdm.c    | 22 +++++++++++++++-------
>  drivers/cxl/core/mbox.c   |  1 +
>  drivers/cxl/core/port.c   |  1 +
>  drivers/cxl/core/region.c | 37 ++++++++++++++++++++++++++++++++++---
>  drivers/cxl/cxl.h         |  9 +++++++++
>  drivers/cxl/cxlmem.h      | 11 +++++++++++
>  6 files changed, 71 insertions(+), 10 deletions(-)
> 

[snip]

>  
> +/*
> + * enum cxl_devcoherence - the coherence of the cxl device
> + * @CXL_DEVCOH_DEV      - HDM-D or HDM-DB
> + * @CXL_DEVCOH_HOSTONLY - HDM-H
> + */

Could I suggest mapping the coherence type to the expected device type(s) in this
comment? My thinking here is that the coherence types aren't exactly straightforward
and having the device types they correspond to would help ease any confusion, especially
since it looks like we are expecting type 2 driver writers to fill this in manually. I'm
thinking something along the lines of:
/*
 * enum cxl_devcoherence - the coherence of the cxl device
 * @CXL_DEVCOH_DEV      - HDM-D (type 2) or HDM-DB (type 2/3)
 * @CXL_DEVCOH_HOSTONLY - HDM-H (type 3)
 */

> +enum cxl_devcoherence {
> +	CXL_DEVCOH_DEV,
> +	CXL_DEVCOH_HOSTONLY,
> +};
> +
>  /**
>   * struct cxl_dpa_perf - DPA performance property entry
>   * @dpa_range: range for DPA address
> @@ -438,6 +448,7 @@ struct cxl_dev_state {
>  	struct resource ram_res;
>  	u64 serial;
>  	enum cxl_devtype type;
> +	enum cxl_devcoherence coherence;
>  };
>  
>  /**