| Message ID | 20210902195017.2516472-4-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enumerate midlevel and endpoint decoders | expand |
On Thu, 2 Sep 2021 12:50:07 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > Decoders will be added to the bus either already active (committed in > spec parlance), or inactive. From the driver perspective, the set of > devices comprising the former are those which are brought up by system > firmware; decoders that implement: volatile regions, persistent regions, > or platform specific (ie. CFMWS) constraints. Such devices have a given > interleave programming already in place. Inactive decoders on the other > hand, do not have any interleave programming in place. The set of > devices comprising that are hostbridges, switches, and endpoint devices. > > Allow adding inactive decoders by removing this check. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> Makes sense. I assume there is no easy way to modify this check to still be applied if the encoder is active? Otherwise Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/bus.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > index 9d98dd50d424..8d5061b0794d 100644 > --- a/drivers/cxl/core/bus.c > +++ b/drivers/cxl/core/bus.c > @@ -532,9 +532,6 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, > if (IS_ERR(cxld)) > return PTR_ERR(cxld); > > - if (cxld->interleave_ways < 1) > - return -EINVAL; > - > port = to_cxl_port(cxld->dev.parent); > device_lock(&port->dev); > if (list_empty(&port->dports)) {
On Fri, Sep 3, 2021 at 7:25 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 2 Sep 2021 12:50:07 -0700 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > Decoders will be added to the bus either already active (committed in > > spec parlance), or inactive. From the driver perspective, the set of > > devices comprising the former are those which are brought up by system > > firmware; decoders that implement: volatile regions, persistent regions, > > or platform specific (ie. CFMWS) constraints. Such devices have a given > > interleave programming already in place. Inactive decoders on the other > > hand, do not have any interleave programming in place. The set of > > devices comprising that are hostbridges, switches, and endpoint devices. > > > > Allow adding inactive decoders by removing this check. I thought I agreed with this initially, but the spec initializes the default value of IW to 0 (== x1 interleave). It is impossible for a decoder to ever have less than one interleave-way defined. Instead "Decoder Size == 0" is a disabled decoder.
On 21-09-10 12:00:29, Dan Williams wrote: > On Fri, Sep 3, 2021 at 7:25 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Thu, 2 Sep 2021 12:50:07 -0700 > > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > Decoders will be added to the bus either already active (committed in > > > spec parlance), or inactive. From the driver perspective, the set of > > > devices comprising the former are those which are brought up by system > > > firmware; decoders that implement: volatile regions, persistent regions, > > > or platform specific (ie. CFMWS) constraints. Such devices have a given > > > interleave programming already in place. Inactive decoders on the other > > > hand, do not have any interleave programming in place. The set of > > > devices comprising that are hostbridges, switches, and endpoint devices. > > > > > > Allow adding inactive decoders by removing this check. > > I thought I agreed with this initially, but the spec initializes the > default value of IW to 0 (== x1 interleave). It is impossible for a > decoder to ever have less than one interleave-way defined. Instead > "Decoder Size == 0" is a disabled decoder. Well I later add state as to whether or not the decoder is active too. I don't disagree with your logic, though I find this more awkward than what my patch does. For the sake of moving things forward smoothly, I will drop this patch and simply set iw = 1 for new, inactive decoders being enumerated.
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c index 9d98dd50d424..8d5061b0794d 100644 --- a/drivers/cxl/core/bus.c +++ b/drivers/cxl/core/bus.c @@ -532,9 +532,6 @@ int cxl_decoder_add(struct device *host, struct cxl_decoder *cxld, if (IS_ERR(cxld)) return PTR_ERR(cxld); - if (cxld->interleave_ways < 1) - return -EINVAL; - port = to_cxl_port(cxld->dev.parent); device_lock(&port->dev); if (list_empty(&port->dports)) {
Decoders will be added to the bus either already active (committed in spec parlance), or inactive. From the driver perspective, the set of devices comprising the former are those which are brought up by system firmware; decoders that implement: volatile regions, persistent regions, or platform specific (ie. CFMWS) constraints. Such devices have a given interleave programming already in place. Inactive decoders on the other hand, do not have any interleave programming in place. The set of devices comprising that are hostbridges, switches, and endpoint devices. Allow adding inactive decoders by removing this check. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/core/bus.c | 3 --- 1 file changed, 3 deletions(-)