| Message ID | 20250218132356.1809075-4-rrichter@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Address translation support, part 2: Generic support and AMD Zen5 platform enablement | expand |
On Tue, Feb 18, 2025 at 02:23:44PM +0100, Robert Richter wrote: > Function cxl_calc_interleave_pos() contains code to calculate the > interleaving parameters of a port. Factor out that code for later > reuse. Add function cxl_port_calc_interleave() for this and introduce > struct cxl_interleave_context to collect all interleaving data. > > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/cxl/core/region.c | 63 ++++++++++++++++++++++++++------------- > 1 file changed, 43 insertions(+), 20 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c118bda93e86..ad4a6ce37216 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1800,27 +1800,34 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range, > return rc; > } > > +struct cxl_interleave_context { > + struct range *hpa_range; > + int pos; > +}; > + I get that this will be used later to pass information back, but this patch by itself is a little confusing because ctx seems pointless since the function still returns the position and accesses the hpa_range directly Looked at in isolation, having the context structure change in this patch than just adding the hpa_range as an argument and adding the context later when it's actually relevant. static int cxl_port_calc_interleave(struct cxl_port *port, struct range *hpa_range); > +static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled) > +{ > + struct cxl_port *iter, *port = cxled_to_port(cxled); > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct cxl_interleave_context ctx; > + int pos = 0; > + > + ctx = (struct cxl_interleave_context) { > + .hpa_range = &cxled->cxld.hpa_range, > + }; > + > + for (iter = cxled_to_port(cxled); pos >= 0 && iter; > + iter = parent_port_of(iter)) > + pos = cxl_port_calc_interleave(iter, &ctx); > > dev_dbg(&cxlmd->dev, > "decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n", > dev_name(&cxled->cxld.dev), dev_name(cxlmd->dev.parent), > - dev_name(&port->dev), range->start, range->end, pos); > + dev_name(&port->dev), ctx.hpa_range->start, ctx.hpa_range->end, > + ctx.pos); > context just gets discarded here and hpa_range and pos are otherwise still accessible if you just pass hpa_range as an argument. So I would push off adding the context argument to the patch that actually needs it. > return pos; > } ~Gregory
On Thu, Feb 20, 2025 at 11:28:40AM -0500, Gregory Price wrote: > On Tue, Feb 18, 2025 at 02:23:44PM +0100, Robert Richter wrote: > I get that this will be used later to pass information back, but this > patch by itself is a little confusing because ctx seems pointless since > the function still returns the position and accesses the hpa_range directly > > Looked at in isolation, having the context structure change in this > patch than just adding the hpa_range as an argument and adding the > context later when it's actually relevant. > > static int cxl_port_calc_interleave(struct cxl_port *port, > struct range *hpa_range); > Disregard my note here, I missed that pos has to be carried through. Didn't notice until I looked at patch 3 and 4 together. > + ctx->pos = ctx->pos * parent_ways + parent_pos; > + > + return ctx->pos; This looks fine Reviewed-by: Gregory Price <gourry@gourry.net>
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index c118bda93e86..ad4a6ce37216 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1800,27 +1800,34 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range, return rc; } +struct cxl_interleave_context { + struct range *hpa_range; + int pos; +}; + /** - * cxl_calc_interleave_pos() - calculate an endpoint position in a region - * @cxled: endpoint decoder member of given region + * cxl_port_calc_interleave() - calculate interleave config of an endpoint for @port + * @port: Port the new position is calculated for. + * @ctx: Interleave context * - * The endpoint position is calculated by traversing the topology from - * the endpoint to the root decoder and iteratively applying this - * calculation: + * The endpoint position for the next port is calculated by applying + * this calculation: * * position = position * parent_ways + parent_pos; * * ...where @position is inferred from switch and root decoder target lists. * + * The endpoint's position in a region can be calculated by traversing + * the topology from the endpoint to the root decoder and iteratively + * applying the function for each port. + * * Return: position >= 0 on success * -ENXIO on failure */ -static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled) +static int cxl_port_calc_interleave(struct cxl_port *port, + struct cxl_interleave_context *ctx) { - struct cxl_port *iter, *port = cxled_to_port(cxled); - struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); - struct range *range = &cxled->cxld.hpa_range; - int parent_ways = 0, parent_pos = 0, pos = 0; + int parent_ways = 0, parent_pos = 0; int rc; /* @@ -1852,22 +1859,38 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled) * complex topologies, including those with switches. */ - /* Iterate from endpoint to root_port refining the position */ - for (iter = port; iter; iter = parent_port_of(iter)) { - if (is_cxl_root(iter)) - break; + if (is_cxl_root(port)) + return 0; - rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways); - if (rc) - return rc; + rc = find_pos_and_ways(port, ctx->hpa_range, &parent_pos, &parent_ways); + if (rc) + return rc; - pos = pos * parent_ways + parent_pos; - } + ctx->pos = ctx->pos * parent_ways + parent_pos; + + return ctx->pos; +} + +static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled) +{ + struct cxl_port *iter, *port = cxled_to_port(cxled); + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); + struct cxl_interleave_context ctx; + int pos = 0; + + ctx = (struct cxl_interleave_context) { + .hpa_range = &cxled->cxld.hpa_range, + }; + + for (iter = cxled_to_port(cxled); pos >= 0 && iter; + iter = parent_port_of(iter)) + pos = cxl_port_calc_interleave(iter, &ctx); dev_dbg(&cxlmd->dev, "decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n", dev_name(&cxled->cxld.dev), dev_name(cxlmd->dev.parent), - dev_name(&port->dev), range->start, range->end, pos); + dev_name(&port->dev), ctx.hpa_range->start, ctx.hpa_range->end, + ctx.pos); return pos; }
Function cxl_calc_interleave_pos() contains code to calculate the interleaving parameters of a port. Factor out that code for later reuse. Add function cxl_port_calc_interleave() for this and introduce struct cxl_interleave_context to collect all interleaving data. Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/core/region.c | 63 ++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 20 deletions(-)