| Message ID | 168592153054.1948938.12344684637653088842.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl: Device memory setup | expand |
On Sun, 04 Jun 2023 16:32:10 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > In preparation for device-memory region creation, arrange for decoders > of CXL_DEVTYPE_DEVMEM memdevs to default to CXL_DECODER_DEVMEM for their > target type. Why? CXL_DEVTYPE_DEVMEM might just be a non CLASS code compliant HDM-H only device. I'd want those drivers to always set this explicitly. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/hdm.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index de8a3fb28331..ca3b99c6eacf 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -856,12 +856,22 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > } > port->commit_end = cxld->id; > } else { > - /* unless / until type-2 drivers arrive, assume type-3 */ > if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) { > ctrl |= CXL_HDM_DECODER0_CTRL_TYPE; > writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); This is setting it to be HOSTMEM if it was previously DEVMEM and that makes it inconsistent with the state cached below. Not sure why it was conditional in the first place - writing to existing value should have been safe and would be less code... > } > - cxld->target_type = CXL_DECODER_HOSTMEM; > + if (cxled) { > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > + > + if (cxlds->type == CXL_DEVTYPE_CLASSMEM) > + cxld->target_type = CXL_DECODER_HOSTMEM; > + else > + cxld->target_type = CXL_DECODER_DEVMEM; > + } else { > + /* To be overridden by region type at commit time */ > + cxld->target_type = CXL_DECODER_HOSTMEM; > + } > } > rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl), > &cxld->interleave_ways); >
Jonathan Cameron wrote: > On Sun, 04 Jun 2023 16:32:10 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > In preparation for device-memory region creation, arrange for decoders > > of CXL_DEVTYPE_DEVMEM memdevs to default to CXL_DECODER_DEVMEM for their > > target type. > > Why? CXL_DEVTYPE_DEVMEM might just be a non CLASS code compliant HDM-H > only device. I'd want those drivers to always set this explicitly. As it stands /sys/bus/cxl/devices/decoderX.Y/target_type is read-only. For non-class-code compliant HDM-H device, or even a device that supports mixed HDM-H + HDM-DB operation depending on the decoder, there would need to be some mechanism to communicate that at decoder instantiation time. So the default derived from CXL_DEVTYPE_* is indeed an arbitrary placeholder until a use case for more precision comes along. In that case I think target_type becomes writable, and "none" becomes a valid return value. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/hdm.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > index de8a3fb28331..ca3b99c6eacf 100644 > > --- a/drivers/cxl/core/hdm.c > > +++ b/drivers/cxl/core/hdm.c > > @@ -856,12 +856,22 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > > } > > port->commit_end = cxld->id; > > } else { > > - /* unless / until type-2 drivers arrive, assume type-3 */ > > if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) { > > ctrl |= CXL_HDM_DECODER0_CTRL_TYPE; > > writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); > > This is setting it to be HOSTMEM if it was previously DEVMEM and that > makes it inconsistent with the state cached below. Oh, definite oversight. > Not sure why it was conditional in the first place - writing to existing value > should have been safe and would be less code... Agree, that's busted, will fix. > > > } > > - cxld->target_type = CXL_DECODER_HOSTMEM; > > + if (cxled) { > > + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > > + struct cxl_dev_state *cxlds = cxlmd->cxlds; > > + > > + if (cxlds->type == CXL_DEVTYPE_CLASSMEM) > > + cxld->target_type = CXL_DECODER_HOSTMEM; > > + else > > + cxld->target_type = CXL_DECODER_DEVMEM; > > + } else { > > + /* To be overridden by region type at commit time */ > > + cxld->target_type = CXL_DECODER_HOSTMEM; > > + } > > } > > rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl), > > &cxld->interleave_ways); > > >
Jonathan Cameron wrote: > On Sun, 04 Jun 2023 16:32:10 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > In preparation for device-memory region creation, arrange for decoders > > of CXL_DEVTYPE_DEVMEM memdevs to default to CXL_DECODER_DEVMEM for their > > target type. > > Why? CXL_DEVTYPE_DEVMEM might just be a non CLASS code compliant HDM-H > only device. I'd want those drivers to always set this explicitly. > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/core/hdm.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > index de8a3fb28331..ca3b99c6eacf 100644 > > --- a/drivers/cxl/core/hdm.c > > +++ b/drivers/cxl/core/hdm.c > > @@ -856,12 +856,22 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > > } > > port->commit_end = cxld->id; > > } else { > > - /* unless / until type-2 drivers arrive, assume type-3 */ > > if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) { > > ctrl |= CXL_HDM_DECODER0_CTRL_TYPE; > > writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); > > This is setting it to be HOSTMEM if it was previously DEVMEM and that > makes it inconsistent with the state cached below. > > Not sure why it was conditional in the first place - writing to existing value > should have been safe and would be less code... folded in the following... -- >8 -- diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 8deb362a7e44..715c1f103739 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -572,7 +572,7 @@ static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl) { u32p_replace_bits(ctrl, !!(cxld->target_type == CXL_DECODER_HOSTONLYMEM), - CXL_HDM_DECODER0_CTRL_TYPE); + CXL_HDM_DECODER0_CTRL_HOSTONLY); } static int cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt) @@ -840,7 +840,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, cxld->flags |= CXL_DECODER_F_ENABLE; if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK) cxld->flags |= CXL_DECODER_F_LOCK; - if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl)) + if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) cxld->target_type = CXL_DECODER_HOSTONLYMEM; else cxld->target_type = CXL_DECODER_DEVMEM; @@ -859,14 +859,14 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, } port->commit_end = cxld->id; } else { - if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) { - ctrl |= CXL_HDM_DECODER0_CTRL_TYPE; - writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); - } if (cxled) { struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); struct cxl_dev_state *cxlds = cxlmd->cxlds; + /* + * Default by devtype until a device arrives that needs + * more precision. + */ if (cxlds->type == CXL_DEVTYPE_CLASSMEM) cxld->target_type = CXL_DECODER_HOSTONLYMEM; else @@ -875,6 +875,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, /* To be overridden by region type at commit time */ cxld->target_type = CXL_DECODER_HOSTONLYMEM; } + + if (!FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl) && + cxld->target_type == CXL_DECODER_HOSTONLYMEM) { + ctrl |= CXL_HDM_DECODER0_CTRL_HOSTONLY; + writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); + } } rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl), &cxld->interleave_ways); diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index ae0965ac8c5a..f309b1387858 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -56,7 +56,7 @@ #define CXL_HDM_DECODER0_CTRL_COMMIT BIT(9) #define CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10) #define CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11) -#define CXL_HDM_DECODER0_CTRL_TYPE BIT(12) +#define CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12) #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24) #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28) #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
On Tue, 13 Jun 2023 15:32:54 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > > On Sun, 04 Jun 2023 16:32:10 -0700 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > In preparation for device-memory region creation, arrange for decoders > > > of CXL_DEVTYPE_DEVMEM memdevs to default to CXL_DECODER_DEVMEM for their > > > target type. > > > > Why? CXL_DEVTYPE_DEVMEM might just be a non CLASS code compliant HDM-H > > only device. I'd want those drivers to always set this explicitly. > > > > > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > --- > > > drivers/cxl/core/hdm.c | 14 ++++++++++++-- > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > > index de8a3fb28331..ca3b99c6eacf 100644 > > > --- a/drivers/cxl/core/hdm.c > > > +++ b/drivers/cxl/core/hdm.c > > > @@ -856,12 +856,22 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > > > } > > > port->commit_end = cxld->id; > > > } else { > > > - /* unless / until type-2 drivers arrive, assume type-3 */ > > > if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) { > > > ctrl |= CXL_HDM_DECODER0_CTRL_TYPE; > > > writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); > > > > This is setting it to be HOSTMEM if it was previously DEVMEM and that > > makes it inconsistent with the state cached below. > > > > Not sure why it was conditional in the first place - writing to existing value > > should have been safe and would be less code... > > folded in the following... LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > -- >8 -- > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 8deb362a7e44..715c1f103739 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -572,7 +572,7 @@ static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl) > { > u32p_replace_bits(ctrl, > !!(cxld->target_type == CXL_DECODER_HOSTONLYMEM), > - CXL_HDM_DECODER0_CTRL_TYPE); > + CXL_HDM_DECODER0_CTRL_HOSTONLY); > } > > static int cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt) > @@ -840,7 +840,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > cxld->flags |= CXL_DECODER_F_ENABLE; > if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK) > cxld->flags |= CXL_DECODER_F_LOCK; > - if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl)) > + if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) > cxld->target_type = CXL_DECODER_HOSTONLYMEM; > else > cxld->target_type = CXL_DECODER_DEVMEM; > @@ -859,14 +859,14 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > } > port->commit_end = cxld->id; > } else { > - if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) { > - ctrl |= CXL_HDM_DECODER0_CTRL_TYPE; > - writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); > - } > if (cxled) { > struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > struct cxl_dev_state *cxlds = cxlmd->cxlds; > > + /* > + * Default by devtype until a device arrives that needs > + * more precision. > + */ > if (cxlds->type == CXL_DEVTYPE_CLASSMEM) > cxld->target_type = CXL_DECODER_HOSTONLYMEM; > else > @@ -875,6 +875,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > /* To be overridden by region type at commit time */ > cxld->target_type = CXL_DECODER_HOSTONLYMEM; > } > + > + if (!FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl) && > + cxld->target_type == CXL_DECODER_HOSTONLYMEM) { > + ctrl |= CXL_HDM_DECODER0_CTRL_HOSTONLY; > + writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); > + } > } > rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl), > &cxld->interleave_ways); > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index ae0965ac8c5a..f309b1387858 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -56,7 +56,7 @@ > #define CXL_HDM_DECODER0_CTRL_COMMIT BIT(9) > #define CXL_HDM_DECODER0_CTRL_COMMITTED BIT(10) > #define CXL_HDM_DECODER0_CTRL_COMMIT_ERROR BIT(11) > -#define CXL_HDM_DECODER0_CTRL_TYPE BIT(12) > +#define CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12) > #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24) > #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28) > #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index de8a3fb28331..ca3b99c6eacf 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -856,12 +856,22 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, } port->commit_end = cxld->id; } else { - /* unless / until type-2 drivers arrive, assume type-3 */ if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) { ctrl |= CXL_HDM_DECODER0_CTRL_TYPE; writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); } - cxld->target_type = CXL_DECODER_HOSTMEM; + if (cxled) { + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); + struct cxl_dev_state *cxlds = cxlmd->cxlds; + + if (cxlds->type == CXL_DEVTYPE_CLASSMEM) + cxld->target_type = CXL_DECODER_HOSTMEM; + else + cxld->target_type = CXL_DECODER_DEVMEM; + } else { + /* To be overridden by region type at commit time */ + cxld->target_type = CXL_DECODER_HOSTMEM; + } } rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl), &cxld->interleave_ways);
In preparation for device-memory region creation, arrange for decoders of CXL_DEVTYPE_DEVMEM memdevs to default to CXL_DECODER_DEVMEM for their target type. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/hdm.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)