On 22-01-12 14:53:28, Jonathan Cameron wrote:
> On Thu, 6 Jan 2022 16:37:56 -0800
> Ben Widawsky <ben.widawsky@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> 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);
