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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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)

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