Re: [PATCH v2 7/8] cxl: Add emulation when HDM decoders are not committed

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

On 1/13/23 7:07 AM, Jonathan Cameron wrote:
> On Mon, 09 Jan 2023 14:44:03 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> For the case where DVSEC range register(s) are active and HDM decoders are
>> not committed, use RR to provide emulation. A first pass is done to note
>> whether any decoders are committed. If there are no committed endpoint
>> decoders, then DVSEC ranges will be used for emulation.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Hi Dave
> 
> One trivial suggestion inline. With that tidied up
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>   drivers/cxl/core/hdm.c |   39 ++++++++++++++++++++++++++++++++-------
>>   drivers/cxl/cxl.h      |    1 +
>>   2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index ed5e9ef3aa9b..40844ff2fe52 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -729,6 +729,33 @@ static int cxl_setup_hdm_decoder_from_dvsec(struct cxl_port *port,
>>   	return 0;
>>   }
>>   
>> +static bool should_emulate_decoders(struct cxl_hdm *cxlhdm)
>> +{
>> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>> +	bool committed;
>> +	u32 ctrl;
>> +	int i;
>> +
>> +	if (!is_cxl_endpoint(cxlhdm->port))
>> +		return false;
>> +
>> +	if (!hdm)
>> +		return true;
>> +
>> +	/*
>> +	 * If any decoders are committed already, there should not be any
>> +	 * emulated DVSEC decoders.
>> +	 */
>> +	for (i = 0; i < cxlhdm->decoder_count; i++) {
>> +		ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i));
>> +		committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
> 
> FIELD_GET() is nicer than !! I think.
> 
> Also, local variable isn't useful so I'd get rid of it.
> 
> 		if (FIELD_GET(ctrl, CXL_HDM_DECODER0_CTRL_COMMITTED))
> 			return false;

Compiler does not seem to like single bit mask. How about
		if (ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)
			return false;
> 
> 
>> +		if (committed)
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
> 
>>
>