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

[Huang, Ying]

Ben Cheatham <benjamin.cheatham@xxxxxxx> writes:

> 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)
>  */

This looks good to me!  Thanks!  Will do this in the next version.

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

--
Best Regards,
Huang, Ying