Re: [PATCH 2/3] cxl: Add CXL spec v3.0 interleave support

[Dave Jiang <dave.jiang@intel.com>]

On 8/9/2022 9:20 AM, Dan Williams wrote:
> Dave Jiang wrote:
>> CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register.
>> CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave
>> is capable. Bit 12 indicates that 16 way interleave is capable.
>>
>> Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in
>> cxl_interleave_verify() call to make sure those CAP bits matches the passed
>> in interleave value.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/hdm.c |    4 ++++
>>   drivers/cxl/cxl.h      |    2 ++
>>   drivers/cxl/cxlmem.h   |   29 +++++++++++++++++++++++++++++
>>   3 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 8143e2615957..50ff7387e425 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -80,6 +80,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>>   		cxlhdm->interleave_mask |= GENMASK(11, 8);
>>   	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
>>   		cxlhdm->interleave_mask |= GENMASK(14, 12);
>> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap))
>> +		cxlhdm->interleave_3_6_12 = true;
>> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap))
>> +		cxlhdm->interleave_16 = true;
>>   }
>>   
>>   static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 275979fbd15a..db9631d09dd0 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -42,6 +42,8 @@
>>   #define   CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
>>   #define   CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8)
>>   #define   CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9)
>> +#define   CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11)
>> +#define   CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12)
>>   #define CXL_HDM_DECODER_CTRL_OFFSET 0x4
>>   #define   CXL_HDM_DECODER_ENABLE BIT(1)
>>   #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10)
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index d5f872ca62f9..9b4b23b3b78a 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -398,9 +398,35 @@ struct cxl_hdm {
>>   	unsigned int decoder_count;
>>   	unsigned int target_count;
>>   	unsigned int interleave_mask;
>> +	bool interleave_3_6_12;
>> +	bool interleave_16;
>>   	struct cxl_port *port;
>>   };
>>   
>> +static inline bool valid_interleave(struct cxl_hdm *cxlhdm, u8 iw)
>> +{
>> +	switch (iw) {
>> +	case CXL_INTERLEAVE_1_WAY:
>> +	case CXL_INTERLEAVE_2_WAY:
>> +	case CXL_INTERLEAVE_4_WAY:
>> +	case CXL_INTERLEAVE_8_WAY:
>> +		return true;
>> +	case CXL_INTERLEAVE_16_WAY:
>> +		if (!cxlhdm->interleave_16)
>> +			return false;
>> +		return true;
>> +	case CXL_INTERLEAVE_3_WAY:
>> +	case CXL_INTERLEAVE_6_WAY:
>> +	case CXL_INTERLEAVE_12_WAY:
>> +		if (!cxlhdm->interleave_3_6_12)
>> +			return false;
>> +		return true;
>> +	default:
>> +	};
>> +
>> +	return false;
> I'd prefer something more compact like
>
>      test_bit(iw, &port->interleave_cap)
>
> ...where interleave_cap is just a mask with bits 1,2,3,4,6,8,12,16
> optionally set.

ok