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

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

On 1/18/23 3:12 AM, Jonathan Cameron wrote:
> On Tue, 17 Jan 2023 16:19:39 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> 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
> 
> That's strange - number of bits shouldn't matter.
> hough I got parameters backwards above which doesn't help.
> FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl)

ah yeah that was it. oops.

> 
>> 		if (ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED)
>> 			return false;
>>>
>>>    
>>>> +		if (committed)
>>>> +			return false;
>>>> +	}
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>    
>>>>   
>>>    
>