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

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

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;



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