Re: [PATCH 13/13] cxl: Program decoders for regions

[Ben Widawsky <ben.widawsky@intel.com>]

On 22-01-12 14:53:28, Jonathan Cameron wrote:
> On Thu,  6 Jan 2022 16:37:56 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > Do the HDM decoder programming for all endpoints and host bridges in a
> > region. Switches are currently unimplemented.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> Hi Ben,
> 
> Minor bug in the maths below that I'd missed eyeballing the registers
> because it happened to give nearly the write value for my normal test config
> by complete coincidence...
> 
> You may well have already caught this one - I've not checked your latest
> tree.
> 
> > +/**
> > + * cxl_commit_decoder() - Program a configured cxl_decoder
> > + * @cxld: The preconfigured cxl decoder.
> > + *
> > + * A cxl decoder that is to be committed should have been earmarked as enabled.
> > + * This mechanism acts as a soft reservation on the decoder.
> > + *
> > + * Returns 0 if commit was successful, negative error code otherwise.
> > + */
> > +int cxl_commit_decoder(struct cxl_decoder *cxld)
> > +{
> > +	u32 ctrl, tl_lo, tl_hi, base_lo, base_hi, size_lo, size_hi;
> > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > +	struct cxl_port_state *cxlps;
> > +	void __iomem *hdm_decoder;
> > +	int rc;
> > +
> > +	/*
> > +	 * Decoder flags are entirely software controlled and therefore this
> > +	 * case is purely a driver bug.
> > +	 */
> > +	if (dev_WARN_ONCE(&port->dev, (cxld->flags & CXL_DECODER_F_ENABLE) != 0,
> > +			  "Invalid %s enable state\n", dev_name(&cxld->dev)))
> > +		return -ENXIO;
> > +
> > +	cxlps = dev_get_drvdata(&port->dev);
> > +	hdm_decoder = cxlps->regs.hdm_decoder;
> > +	ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> > +
> > +	/*
> > +	 * A decoder that's currently active cannot be changed without the
> > +	 * system being quiesced. While the driver should prevent against this,
> > +	 * for a variety of reasons the hardware might not be in sync with the
> > +	 * hardware and so, do not splat on error.
> > +	 */
> > +	size_hi = readl(hdm_decoder +
> > +			CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(cxld->id));
> > +	size_lo =
> > +		readl(hdm_decoder + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(cxld->id));
> > +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl) &&
> > +	    (size_lo + size_hi)) {
> > +		dev_err(&port->dev, "Tried to change an active decoder (%s)\n",
> > +			dev_name(&cxld->dev));
> > +		return -EBUSY;
> > +	}
> > +
> > +	u32p_replace_bits(&ctrl, 8 - ilog2(cxld->interleave_granularity),
> 
> This maths is wrong.   interleave_granularity is stored here as log2() anyway
> and should be cxld->interleave_granularity - 8;

Thanks. interleave_granularity was supposed to move to the absolute value and so
the math here should be correct (although in my current rev I have extracted the
math to a separate function).

I'll fix the meaning of granularity...

> 
> 
> 
> > +			  CXL_HDM_DECODER0_CTRL_IG_MASK);
> > +	u32p_replace_bits(&ctrl, ilog2(cxld->interleave_ways),
> > +			  CXL_HDM_DECODER0_CTRL_IW_MASK);