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

Huang, Ying
2024-10-03T01:17:13Z

Ben Cheatham <benjamin.cheatham@amd.com> 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@intel.com>
>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Alison Schofield <alison.schofield@intel.com>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Alejandro Lucero <alucerop@amd.com>
>> ---
>> 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