| Message ID | 20230804232726.1672782-1-alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/region: Refactor granularity select in cxl_port_setup_targets() | expand |
alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > > In cxl_port_setup_targets() the region driver validates the > configuration of auto-discovered region decoders, as well > as decoders the driver is preparing to program. > > The existing calculations use the encrypted interleave granularity s/encrypted/encoded/ > value, the eig, to create an interleave granularity that properly > fans out when routing an x1 interleave to a greater than x1 interleave. > > That all worked well, until this config came along: > Host Bridge: 2 way at 256 granularity > Switch Decoder_A: 1 way at 512 > Endpoint_X: 2 way at 512 > Switch Decoder_B: 1 way at 512 > Endpoint_Y: 2 way at 512 > > When the Host Bridge interleave is greater that 1, and the root s/that/than/ > decoder interleave is exactly 1, the region driver needs to > consider the number of targets in the region when calculating > the expected granularity. > > While examining the existing logic, and trying to cover the case > above, a couple of simplifications appeared, hence this proposed > refactoring. > > The first simplicfication is to apply the logic to the unencrypted s/unencrypted/nominal/ > values and use the existing helper function granularity_to_eig() to > translate the desired granularity to the encrypted form. This means > the comment and code regarding setting address bits is discarded. > Although that logic was not wrong, it adds a level of complexity that > is not required in the granularity selection. The eig and eiw are > indeed part of the routing instructions programmed into the decoders. > Up-level the discussion to plain ways and granularity for clearer > analysis. > > The second simplification reduces the logic to a single granularity > calculation that works for all cases. The new calculation doesn't > care if parent_iw => 1, because parent_iw is used as a multiplier. > > The refactor cleans up a useless assignment of eiw, made after the iw > is already calculated. > > Regression testing included an examination of all of the ways and > granularity selections made during a run of the cxl_test unit tests. > There were no differences in selections before and after this patch. Fixes: ("27b3f8d13830 cxl/region: Program target lists") > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/region.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) Fix *and* less code!? Yes, please! > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index e115ba382e04..5a1cc59cca99 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port, > } > > /* > - * If @parent_port is masking address bits, pick the next unused address > - * bit to route @port's targets. > + * Interleave granularity is a multiple of @parent_port granularity. > + * Multiplier is the parent port interleave ways. > */ > - if (parent_iw > 1 && cxl_rr->nr_targets > 1) { > - u32 address_bit = max(peig + peiw, eiw + peig); > - > - eig = address_bit - eiw + 1; > - } else { > - eiw = peiw; > - eig = peig; > + rc = granularity_to_eig(parent_ig * parent_iw, &eig); I was going to say "wait, does this work with x3 parent_iw", and I believe it does because this: /* * For purposes of address bit routing, use power-of-2 math for * switch ports. */ if (!is_power_of_2(parent_iw)) parent_iw /= 3; ...is handled in the is_cxl_root(parent_port) case. A thing of beauty this patch. Ship it! (modulo those minor nits above).
On Fri, 4 Aug 2023 16:27:26 -0700 alison.schofield@intel.com wrote: > From: Alison Schofield <alison.schofield@intel.com> > > In cxl_port_setup_targets() the region driver validates the > configuration of auto-discovered region decoders, as well > as decoders the driver is preparing to program. > > The existing calculations use the encrypted interleave granularity > value, the eig, to create an interleave granularity that properly > fans out when routing an x1 interleave to a greater than x1 interleave. > > That all worked well, until this config came along: > Host Bridge: 2 way at 256 granularity > Switch Decoder_A: 1 way at 512 > Endpoint_X: 2 way at 512 > Switch Decoder_B: 1 way at 512 > Endpoint_Y: 2 way at 512 Not sure this is a valid example Those endpoints need to have 2 way at 256 I think... Mapping out the result... 0000-0255 RP 0 -> Switch Decoder_A -> End_Point_X DPA 0-255 0256-0511 RP 1 -> Switch Decoder_B -> End_point_Y DPA 256-511 * 0512-0767 RP 0 -> Switch Decoder_A -> End_point_X DPA 0-255 * 0768-1023 RP 1 -> Switch Decoder_B -> End_point_Y DPA 256-511 1024-1279 RP 0 -> Switch Decoder_A -> End_point_X * address bit for 512 dropped not the one for 256 > > When the Host Bridge interleave is greater that 1, and the root > decoder interleave is exactly 1, the region driver needs to > consider the number of targets in the region when calculating > the expected granularity. > > While examining the existing logic, and trying to cover the case > above, a couple of simplifications appeared, hence this proposed > refactoring. > > The first simplicfication is to apply the logic to the unencrypted > values and use the existing helper function granularity_to_eig() to > translate the desired granularity to the encrypted form. This means > the comment and code regarding setting address bits is discarded. > Although that logic was not wrong, it adds a level of complexity that > is not required in the granularity selection. The eig and eiw are > indeed part of the routing instructions programmed into the decoders. > Up-level the discussion to plain ways and granularity for clearer > analysis. > > The second simplification reduces the logic to a single granularity > calculation that works for all cases. The new calculation doesn't > care if parent_iw => 1, because parent_iw is used as a multiplier. > > The refactor cleans up a useless assignment of eiw, made after the iw > is already calculated. > > Regression testing included an examination of all of the ways and > granularity selections made during a run of the cxl_test unit tests. > There were no differences in selections before and after this patch. > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/region.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index e115ba382e04..5a1cc59cca99 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port, > } > > /* > - * If @parent_port is masking address bits, pick the next unused address > - * bit to route @port's targets. > + * Interleave granularity is a multiple of @parent_port granularity. > + * Multiplier is the parent port interleave ways. > */ > - if (parent_iw > 1 && cxl_rr->nr_targets > 1) { > - u32 address_bit = max(peig + peiw, eiw + peig); > - > - eig = address_bit - eiw + 1; > - } else { > - eiw = peiw; > - eig = peig; > + rc = granularity_to_eig(parent_ig * parent_iw, &eig); > + if (rc) { > + dev_dbg(&cxlr->dev, > + "%s: invalid granularity calculation (%d * %d)\n", > + dev_name(&parent_port->dev), parent_ig, parent_iw); > + return rc; > } > > rc = eig_to_granularity(eig, &ig); > > base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9
On 8/4/23 16:27, alison.schofield@intel.com wrote: > From: Alison Schofield <alison.schofield@intel.com> > > In cxl_port_setup_targets() the region driver validates the > configuration of auto-discovered region decoders, as well > as decoders the driver is preparing to program. > > The existing calculations use the encrypted interleave granularity s/encrypted/encoded/ > value, the eig, to create an interleave granularity that properly > fans out when routing an x1 interleave to a greater than x1 interleave. > > That all worked well, until this config came along: > Host Bridge: 2 way at 256 granularity > Switch Decoder_A: 1 way at 512 > Endpoint_X: 2 way at 512 > Switch Decoder_B: 1 way at 512 > Endpoint_Y: 2 way at 512 > > When the Host Bridge interleave is greater that 1, and the root > decoder interleave is exactly 1, the region driver needs to > consider the number of targets in the region when calculating > the expected granularity. > > While examining the existing logic, and trying to cover the case > above, a couple of simplifications appeared, hence this proposed > refactoring. > > The first simplicfication is to apply the logic to the unencrypted s/simplicfication/simplification/ s/unencrypted/normal?/ > values and use the existing helper function granularity_to_eig() to > translate the desired granularity to the encrypted form. This means s/encrypted/encoded/ > the comment and code regarding setting address bits is discarded. > Although that logic was not wrong, it adds a level of complexity that > is not required in the granularity selection. The eig and eiw are > indeed part of the routing instructions programmed into the decoders. > Up-level the discussion to plain ways and granularity for clearer > analysis. > > The second simplification reduces the logic to a single granularity > calculation that works for all cases. The new calculation doesn't > care if parent_iw => 1, because parent_iw is used as a multiplier. > > The refactor cleans up a useless assignment of eiw, made after the iw > is already calculated. > > Regression testing included an examination of all of the ways and > granularity selections made during a run of the cxl_test unit tests. > There were no differences in selections before and after this patch. > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/region.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index e115ba382e04..5a1cc59cca99 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port, > } > > /* > - * If @parent_port is masking address bits, pick the next unused address > - * bit to route @port's targets. > + * Interleave granularity is a multiple of @parent_port granularity. > + * Multiplier is the parent port interleave ways. > */ > - if (parent_iw > 1 && cxl_rr->nr_targets > 1) { > - u32 address_bit = max(peig + peiw, eiw + peig); > - > - eig = address_bit - eiw + 1; > - } else { > - eiw = peiw; > - eig = peig; > + rc = granularity_to_eig(parent_ig * parent_iw, &eig); > + if (rc) { > + dev_dbg(&cxlr->dev, > + "%s: invalid granularity calculation (%d * %d)\n", > + dev_name(&parent_port->dev), parent_ig, parent_iw); > + return rc; > } > > rc = eig_to_granularity(eig, &ig); > > base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9
On Wed, Aug 09, 2023 at 03:28:36PM -0700, Dave Jiang wrote: > > > On 8/4/23 16:27, alison.schofield@intel.com wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > In cxl_port_setup_targets() the region driver validates the > > configuration of auto-discovered region decoders, as well > > as decoders the driver is preparing to program. > > > > The existing calculations use the encrypted interleave granularity > > s/encrypted/encoded/ Thanks. I'll clean up the language around this to use encoded & normal. > > > value, the eig, to create an interleave granularity that properly > > fans out when routing an x1 interleave to a greater than x1 interleave. > > > > That all worked well, until this config came along: > > Host Bridge: 2 way at 256 granularity > > Switch Decoder_A: 1 way at 512 > > Endpoint_X: 2 way at 512 > > Switch Decoder_B: 1 way at 512 > > Endpoint_Y: 2 way at 512 > > > > When the Host Bridge interleave is greater that 1, and the root > > decoder interleave is exactly 1, the region driver needs to > > consider the number of targets in the region when calculating > > the expected granularity. > > > > While examining the existing logic, and trying to cover the case > > above, a couple of simplifications appeared, hence this proposed > > refactoring. > > > > The first simplicfication is to apply the logic to the unencrypted > s/simplicfication/simplification/ > s/unencrypted/normal?/ Got it. > > > > values and use the existing helper function granularity_to_eig() to > > translate the desired granularity to the encrypted form. This means > > s/encrypted/encoded/ Got it. > > > the comment and code regarding setting address bits is discarded. > > Although that logic was not wrong, it adds a level of complexity that > > is not required in the granularity selection. The eig and eiw are > > indeed part of the routing instructions programmed into the decoders. > > Up-level the discussion to plain ways and granularity for clearer > > analysis. > > > > The second simplification reduces the logic to a single granularity > > calculation that works for all cases. The new calculation doesn't > > care if parent_iw => 1, because parent_iw is used as a multiplier. > > > > The refactor cleans up a useless assignment of eiw, made after the iw > > is already calculated. > > > > Regression testing included an examination of all of the ways and > > granularity selections made during a run of the cxl_test unit tests. > > There were no differences in selections before and after this patch. > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > --- > > drivers/cxl/core/region.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index e115ba382e04..5a1cc59cca99 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port, > > } > > /* > > - * If @parent_port is masking address bits, pick the next unused address > > - * bit to route @port's targets. > > + * Interleave granularity is a multiple of @parent_port granularity. > > + * Multiplier is the parent port interleave ways. > > */ > > - if (parent_iw > 1 && cxl_rr->nr_targets > 1) { > > - u32 address_bit = max(peig + peiw, eiw + peig); > > - > > - eig = address_bit - eiw + 1; > > - } else { > > - eiw = peiw; > > - eig = peig; > > + rc = granularity_to_eig(parent_ig * parent_iw, &eig); > > + if (rc) { > > + dev_dbg(&cxlr->dev, > > + "%s: invalid granularity calculation (%d * %d)\n", > > + dev_name(&parent_port->dev), parent_ig, parent_iw); > > + return rc; > > } > > rc = eig_to_granularity(eig, &ig); > > > > base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9
On Mon, Aug 07, 2023 at 02:26:44PM +0100, Jonathan Cameron wrote: > On Fri, 4 Aug 2023 16:27:26 -0700 > alison.schofield@intel.com wrote: > > > From: Alison Schofield <alison.schofield@intel.com> > > > > In cxl_port_setup_targets() the region driver validates the > > configuration of auto-discovered region decoders, as well > > as decoders the driver is preparing to program. > > > > The existing calculations use the encrypted interleave granularity > > value, the eig, to create an interleave granularity that properly > > fans out when routing an x1 interleave to a greater than x1 interleave. > > > > That all worked well, until this config came along: > > Host Bridge: 2 way at 256 granularity > > Switch Decoder_A: 1 way at 512 > > Endpoint_X: 2 way at 512 > > Switch Decoder_B: 1 way at 512 > > Endpoint_Y: 2 way at 512 > > Not sure this is a valid example > Those endpoints need to have 2 way at 256 I think... > > Mapping out the result... > > 0000-0255 RP 0 -> Switch Decoder_A -> End_Point_X DPA 0-255 > 0256-0511 RP 1 -> Switch Decoder_B -> End_point_Y DPA 256-511 * > 0512-0767 RP 0 -> Switch Decoder_A -> End_point_X DPA 0-255 * > 0768-1023 RP 1 -> Switch Decoder_B -> End_point_Y DPA 256-511 > 1024-1279 RP 0 -> Switch Decoder_A -> End_point_X > > * address bit for 512 dropped not the one for 256 Jonathan, Sorry for the typo. The endpoints were indeed iw: 2 ig: 256. I appreciate your mapping. Thanks, Alison > > > > When the Host Bridge interleave is greater that 1, and the root > > decoder interleave is exactly 1, the region driver needs to > > consider the number of targets in the region when calculating > > the expected granularity. > > > > While examining the existing logic, and trying to cover the case > > above, a couple of simplifications appeared, hence this proposed > > refactoring. > > > > The first simplicfication is to apply the logic to the unencrypted > > values and use the existing helper function granularity_to_eig() to > > translate the desired granularity to the encrypted form. This means > > the comment and code regarding setting address bits is discarded. > > Although that logic was not wrong, it adds a level of complexity that > > is not required in the granularity selection. The eig and eiw are > > indeed part of the routing instructions programmed into the decoders. > > Up-level the discussion to plain ways and granularity for clearer > > analysis. > > > > The second simplification reduces the logic to a single granularity > > calculation that works for all cases. The new calculation doesn't > > care if parent_iw => 1, because parent_iw is used as a multiplier. > > > > The refactor cleans up a useless assignment of eiw, made after the iw > > is already calculated. > > > > Regression testing included an examination of all of the ways and > > granularity selections made during a run of the cxl_test unit tests. > > There were no differences in selections before and after this patch. > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > --- > > drivers/cxl/core/region.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index e115ba382e04..5a1cc59cca99 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port, > > } > > > > /* > > - * If @parent_port is masking address bits, pick the next unused address > > - * bit to route @port's targets. > > + * Interleave granularity is a multiple of @parent_port granularity. > > + * Multiplier is the parent port interleave ways. > > */ > > - if (parent_iw > 1 && cxl_rr->nr_targets > 1) { > > - u32 address_bit = max(peig + peiw, eiw + peig); > > - > > - eig = address_bit - eiw + 1; > > - } else { > > - eiw = peiw; > > - eig = peig; > > + rc = granularity_to_eig(parent_ig * parent_iw, &eig); > > + if (rc) { > > + dev_dbg(&cxlr->dev, > > + "%s: invalid granularity calculation (%d * %d)\n", > > + dev_name(&parent_port->dev), parent_ig, parent_iw); > > + return rc; > > } > > > > rc = eig_to_granularity(eig, &ig); > > > > base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9 >
On Fri, Aug 04, 2023 at 07:49:14PM -0700, Dan Williams wrote: > alison.schofield@ wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > > > In cxl_port_setup_targets() the region driver validates the > > configuration of auto-discovered region decoders, as well > > as decoders the driver is preparing to program. > > > > The existing calculations use the encrypted interleave granularity > > s/encrypted/encoded/ Got it. > > > value, the eig, to create an interleave granularity that properly > > fans out when routing an x1 interleave to a greater than x1 interleave. > > > > That all worked well, until this config came along: > > Host Bridge: 2 way at 256 granularity > > Switch Decoder_A: 1 way at 512 > > Endpoint_X: 2 way at 512 > > Switch Decoder_B: 1 way at 512 > > Endpoint_Y: 2 way at 512 > > > > When the Host Bridge interleave is greater that 1, and the root > > s/that/than/ Got it. > > decoder interleave is exactly 1, the region driver needs to > > consider the number of targets in the region when calculating > > the expected granularity. > > > > While examining the existing logic, and trying to cover the case > > above, a couple of simplifications appeared, hence this proposed > > refactoring. > > > > The first simplicfication is to apply the logic to the unencrypted > > s/unencrypted/nominal/ Got it. > > > values and use the existing helper function granularity_to_eig() to > > translate the desired granularity to the encrypted form. This means > > the comment and code regarding setting address bits is discarded. > > Although that logic was not wrong, it adds a level of complexity that > > is not required in the granularity selection. The eig and eiw are > > indeed part of the routing instructions programmed into the decoders. > > Up-level the discussion to plain ways and granularity for clearer > > analysis. > > > > The second simplification reduces the logic to a single granularity > > calculation that works for all cases. The new calculation doesn't > > care if parent_iw => 1, because parent_iw is used as a multiplier. > > > > The refactor cleans up a useless assignment of eiw, made after the iw > > is already calculated. > > > > Regression testing included an examination of all of the ways and > > granularity selections made during a run of the cxl_test unit tests. > > There were no differences in selections before and after this patch. > > Fixes: ("27b3f8d13830 cxl/region: Program target lists") Got it. > > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > --- > > drivers/cxl/core/region.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > Fix *and* less code!? Yes, please! > :) > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index e115ba382e04..5a1cc59cca99 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port, > > } > > > > /* > > - * If @parent_port is masking address bits, pick the next unused address > > - * bit to route @port's targets. > > + * Interleave granularity is a multiple of @parent_port granularity. > > + * Multiplier is the parent port interleave ways. > > */ > > - if (parent_iw > 1 && cxl_rr->nr_targets > 1) { > > - u32 address_bit = max(peig + peiw, eiw + peig); > > - > > - eig = address_bit - eiw + 1; > > - } else { > > - eiw = peiw; > > - eig = peig; > > + rc = granularity_to_eig(parent_ig * parent_iw, &eig); > > I was going to say "wait, does this work with x3 parent_iw", and I > believe it does because this: > > /* > * For purposes of address bit routing, use power-of-2 math for > * switch ports. > */ > if (!is_power_of_2(parent_iw)) > parent_iw /= 3; > > ...is handled in the is_cxl_root(parent_port) case. I think I'll mention the above in the commit log. Thanks! > > A thing of beauty this patch. Ship it! (modulo those minor nits above).
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index e115ba382e04..5a1cc59cca99 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1154,16 +1154,15 @@ static int cxl_port_setup_targets(struct cxl_port *port, } /* - * If @parent_port is masking address bits, pick the next unused address - * bit to route @port's targets. + * Interleave granularity is a multiple of @parent_port granularity. + * Multiplier is the parent port interleave ways. */ - if (parent_iw > 1 && cxl_rr->nr_targets > 1) { - u32 address_bit = max(peig + peiw, eiw + peig); - - eig = address_bit - eiw + 1; - } else { - eiw = peiw; - eig = peig; + rc = granularity_to_eig(parent_ig * parent_iw, &eig); + if (rc) { + dev_dbg(&cxlr->dev, + "%s: invalid granularity calculation (%d * %d)\n", + dev_name(&parent_port->dev), parent_ig, parent_iw); + return rc; } rc = eig_to_granularity(eig, &ig);