| Message ID | 20240925024647.46735-5-ying.huang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Preparation of type2 accelerators support | expand |
On Wed, Sep 25, 2024 at 10:46:46AM +0800, Huang Ying wrote: > The type of region is hard-coded as type 3 expander now, because this > is the only supported device type. As a preparation to support type 2 > accelerators, we set the type of region to that of the first endpoint. > Then, we will check whether the type of region is same as the type of > other endpoints of the region. Because what we really need is to make > sure the type of all endpoints of a region is same. > > The target type of endpoint devices comes from expander/accelerator > device drivers via struct cxl_dev_state. > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/acpi.c | 1 - > drivers/cxl/core/hdm.c | 28 +++++++++++++--------------- > drivers/cxl/core/port.c | 2 ++ > drivers/cxl/core/region.c | 13 +++++++------ > drivers/cxl/cxl.h | 1 + > 5 files changed, 23 insertions(+), 22 deletions(-) > Reviewed-by: Gregory Price <gourry@gourry.net> > static ssize_t create_pmem_region_store(struct device *dev, > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 99398c868d82..2a2d2c483654 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -324,6 +324,7 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev, > #define CXL_DECODER_F_MASK GENMASK(5, 0) > > enum cxl_decoder_type { > + CXL_DECODER_INVALID, nit - should this be an explicit value? > CXL_DECODER_ACCEL = 2, > CXL_DECODER_EXPANDER = 3, > }; > -- > 2.39.2 >
Gregory Price <gourry@gourry.net> writes: > On Wed, Sep 25, 2024 at 10:46:46AM +0800, Huang Ying wrote: >> The type of region is hard-coded as type 3 expander now, because this >> is the only supported device type. As a preparation to support type 2 >> accelerators, we set the type of region to that of the first endpoint. >> Then, we will check whether the type of region is same as the type of >> other endpoints of the region. Because what we really need is to make >> sure the type of all endpoints of a region is same. >> >> The target type of endpoint devices comes from expander/accelerator >> device drivers via struct cxl_dev_state. >> >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> >> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Davidlohr Bueso <dave@stgolabs.net> >> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> >> Cc: Dave Jiang <dave.jiang@intel.com> >> Cc: Alison Schofield <alison.schofield@intel.com> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> Cc: Ira Weiny <ira.weiny@intel.com> >> Cc: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/acpi.c | 1 - >> drivers/cxl/core/hdm.c | 28 +++++++++++++--------------- >> drivers/cxl/core/port.c | 2 ++ >> drivers/cxl/core/region.c | 13 +++++++------ >> drivers/cxl/cxl.h | 1 + >> 5 files changed, 23 insertions(+), 22 deletions(-) >> > > Reviewed-by: Gregory Price <gourry@gourry.net> Thanks! >> static ssize_t create_pmem_region_store(struct device *dev, >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index 99398c868d82..2a2d2c483654 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -324,6 +324,7 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev, >> #define CXL_DECODER_F_MASK GENMASK(5, 0) >> >> enum cxl_decoder_type { >> + CXL_DECODER_INVALID, > > nit - should this be an explicit value? Sure. Will fix this in the future version! >> CXL_DECODER_ACCEL = 2, >> CXL_DECODER_EXPANDER = 3, >> }; >> -- >> 2.39.2 >> -- Best Regards, Huang, Ying
On 9/24/24 9:46 PM, Huang Ying wrote: > The type of region is hard-coded as type 3 expander now, because this > is the only supported device type. As a preparation to support type 2 > accelerators, we set the type of region to that of the first endpoint. > Then, we will check whether the type of region is same as the type of > other endpoints of the region. Because what we really need is to make > sure the type of all endpoints of a region is same. > > The target type of endpoint devices comes from expander/accelerator > device drivers via struct cxl_dev_state. > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/acpi.c | 1 - > drivers/cxl/core/hdm.c | 28 +++++++++++++--------------- > drivers/cxl/core/port.c | 2 ++ > drivers/cxl/core/region.c | 13 +++++++------ > drivers/cxl/cxl.h | 1 + > 5 files changed, 23 insertions(+), 22 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 21486e471305..29c2a44b122c 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws, > > cxld = &cxlrd->cxlsd.cxld; > cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions); > - cxld->target_type = CXL_DECODER_EXPANDER; > cxld->hpa_range = (struct range) { > .start = cfmws->base_hpa, > .end = cfmws->base_hpa + cfmws->window_size - 1, > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 478fb6691759..c9accf8be71f 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > .end = base + size - 1, > }; > > + 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_EXPANDER; > + else > + cxld->target_type = CXL_DECODER_ACCEL; > + } > + > /* decoders are enabled if committed */ > if (committed) { > 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_HOSTONLY, ctrl)) { > - cxld->target_type = CXL_DECODER_EXPANDER; > + if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) > cxld->coherence = CXL_DECODER_HOSTONLYCOH; > - } else { > - cxld->target_type = CXL_DECODER_ACCEL; > + else > cxld->coherence = CXL_DECODER_DEVCOH; > - } > > guard(rwsem_write)(&cxl_region_rwsem); > if (cxld->id != cxl_num_decoders_committed(port)) { > @@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, > 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_EXPANDER; > - else > - cxld->target_type = CXL_DECODER_ACCEL; > if (cxlds->coherence == CXL_DEVCOH_HOSTONLY) > cxld->coherence = CXL_DECODER_HOSTONLYCOH; > else > cxld->coherence = CXL_DECODER_DEVCOH; > } else { > - /* To be overridden by region type/coherence at commit time */ > - cxld->target_type = CXL_DECODER_EXPANDER; > + /* To be overridden by region coherence at commit time */ > cxld->coherence = CXL_DECODER_HOSTONLYCOH; > } > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 2dee78e9b90c..5633b7316cb3 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev, > return sysfs_emit(buf, "accelerator\n"); > case CXL_DECODER_EXPANDER: > return sysfs_emit(buf, "expander\n"); > + default: > + break; You can drop this imo. It doesn't change anything functionally considering the break is immediately followed by a return. Thanks, Ben > } > return -ENXIO; > } > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index cec7d08b6f44..9c68ec445128 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr, > return -ENXIO; > } > > - if (cxled->cxld.target_type != cxlr->type) { > + /* Set the type of region to that of the first endpoint */ > + if (cxlr->type == CXL_DECODER_INVALID) { > + cxlr->type = cxled->cxld.target_type; > + } else if (cxled->cxld.target_type != cxlr->type) { > dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n", > dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), > cxled->cxld.target_type, cxlr->type); > @@ -2482,7 +2485,6 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb, > * @cxlrd: root decoder > * @id: memregion id to create, or memregion_free() on failure > * @mode: mode for the endpoint decoders of this region > - * @type: select whether this is an expander or accelerator (type-2 or type-3) > * > * This is the second step of region initialization. Regions exist within an > * address space which is mapped by a @cxlrd. > @@ -2492,8 +2494,7 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb, > */ > static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > int id, > - enum cxl_decoder_mode mode, > - enum cxl_decoder_type type) > + enum cxl_decoder_mode mode) > { > struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent); > struct cxl_region *cxlr; > @@ -2504,7 +2505,7 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > if (IS_ERR(cxlr)) > return cxlr; > cxlr->mode = mode; > - cxlr->type = type; > + cxlr->type = CXL_DECODER_INVALID; > > dev = &cxlr->dev; > rc = dev_set_name(dev, "region%d", id); > @@ -2576,7 +2577,7 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > return ERR_PTR(-EBUSY); > } > > - return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER); > + return devm_cxl_add_region(cxlrd, id, mode); > } > > static ssize_t create_pmem_region_store(struct device *dev, > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 99398c868d82..2a2d2c483654 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -324,6 +324,7 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev, > #define CXL_DECODER_F_MASK GENMASK(5, 0) > > enum cxl_decoder_type { > + CXL_DECODER_INVALID, > CXL_DECODER_ACCEL = 2, > CXL_DECODER_EXPANDER = 3, > };
Hi, Ben, Ben Cheatham <benjamin.cheatham@amd.com> writes: > On 9/24/24 9:46 PM, Huang Ying wrote: [snip] >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c >> index 2dee78e9b90c..5633b7316cb3 100644 >> --- a/drivers/cxl/core/port.c >> +++ b/drivers/cxl/core/port.c >> @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev, >> return sysfs_emit(buf, "accelerator\n"); >> case CXL_DECODER_EXPANDER: >> return sysfs_emit(buf, "expander\n"); >> + default: >> + break; > > You can drop this imo. It doesn't change anything functionally considering > the break is immediately followed by a return. Sorry, I cannot do that. Otherwise, there will be build error. > Thanks, > Ben > >> } >> return -ENXIO; >> } [snip] -- Best Regards, Huang, Ying
On 10/2/24 8:12 PM, Huang, Ying wrote: > Hi, Ben, > > Ben Cheatham <benjamin.cheatham@amd.com> writes: > >> On 9/24/24 9:46 PM, Huang Ying wrote: > > [snip] > >>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c >>> index 2dee78e9b90c..5633b7316cb3 100644 >>> --- a/drivers/cxl/core/port.c >>> +++ b/drivers/cxl/core/port.c >>> @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev, >>> return sysfs_emit(buf, "accelerator\n"); >>> case CXL_DECODER_EXPANDER: >>> return sysfs_emit(buf, "expander\n"); >>> + default: >>> + break; >> >> You can drop this imo. It doesn't change anything functionally considering >> the break is immediately followed by a return. > > Sorry, I cannot do that. Otherwise, there will be build error. > I thought that might be the case, but figured I'd suggest it anyway. Oh well! Thanks, Ben >> Thanks, >> Ben >> >>> } >>> return -ENXIO; >>> } > > [snip] > > -- > Best Regards, > Huang, Ying
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 21486e471305..29c2a44b122c 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -382,7 +382,6 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws, cxld = &cxlrd->cxlsd.cxld; cxld->flags = cfmws_to_decoder_flags(cfmws->restrictions); - cxld->target_type = CXL_DECODER_EXPANDER; cxld->hpa_range = (struct range) { .start = cfmws->base_hpa, .end = cfmws->base_hpa + cfmws->window_size - 1, diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 478fb6691759..c9accf8be71f 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -841,18 +841,25 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, .end = base + size - 1, }; + 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_EXPANDER; + else + cxld->target_type = CXL_DECODER_ACCEL; + } + /* decoders are enabled if committed */ if (committed) { 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_HOSTONLY, ctrl)) { - cxld->target_type = CXL_DECODER_EXPANDER; + if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl)) cxld->coherence = CXL_DECODER_HOSTONLYCOH; - } else { - cxld->target_type = CXL_DECODER_ACCEL; + else cxld->coherence = CXL_DECODER_DEVCOH; - } guard(rwsem_write)(&cxl_region_rwsem); if (cxld->id != cxl_num_decoders_committed(port)) { @@ -874,21 +881,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, 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_EXPANDER; - else - cxld->target_type = CXL_DECODER_ACCEL; if (cxlds->coherence == CXL_DEVCOH_HOSTONLY) cxld->coherence = CXL_DECODER_HOSTONLYCOH; else cxld->coherence = CXL_DECODER_DEVCOH; } else { - /* To be overridden by region type/coherence at commit time */ - cxld->target_type = CXL_DECODER_EXPANDER; + /* To be overridden by region coherence at commit time */ cxld->coherence = CXL_DECODER_HOSTONLYCOH; } diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 2dee78e9b90c..5633b7316cb3 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -139,6 +139,8 @@ static ssize_t target_type_show(struct device *dev, return sysfs_emit(buf, "accelerator\n"); case CXL_DECODER_EXPANDER: return sysfs_emit(buf, "expander\n"); + default: + break; } return -ENXIO; } diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index cec7d08b6f44..9c68ec445128 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1926,7 +1926,10 @@ static int cxl_region_attach(struct cxl_region *cxlr, return -ENXIO; } - if (cxled->cxld.target_type != cxlr->type) { + /* Set the type of region to that of the first endpoint */ + if (cxlr->type == CXL_DECODER_INVALID) { + cxlr->type = cxled->cxld.target_type; + } else if (cxled->cxld.target_type != cxlr->type) { dev_dbg(&cxlr->dev, "%s:%s type mismatch: %d vs %d\n", dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), cxled->cxld.target_type, cxlr->type); @@ -2482,7 +2485,6 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb, * @cxlrd: root decoder * @id: memregion id to create, or memregion_free() on failure * @mode: mode for the endpoint decoders of this region - * @type: select whether this is an expander or accelerator (type-2 or type-3) * * This is the second step of region initialization. Regions exist within an * address space which is mapped by a @cxlrd. @@ -2492,8 +2494,7 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb, */ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, int id, - enum cxl_decoder_mode mode, - enum cxl_decoder_type type) + enum cxl_decoder_mode mode) { struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent); struct cxl_region *cxlr; @@ -2504,7 +2505,7 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, if (IS_ERR(cxlr)) return cxlr; cxlr->mode = mode; - cxlr->type = type; + cxlr->type = CXL_DECODER_INVALID; dev = &cxlr->dev; rc = dev_set_name(dev, "region%d", id); @@ -2576,7 +2577,7 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, return ERR_PTR(-EBUSY); } - return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER); + return devm_cxl_add_region(cxlrd, id, mode); } static ssize_t create_pmem_region_store(struct device *dev, diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 99398c868d82..2a2d2c483654 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -324,6 +324,7 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev, #define CXL_DECODER_F_MASK GENMASK(5, 0) enum cxl_decoder_type { + CXL_DECODER_INVALID, CXL_DECODER_ACCEL = 2, CXL_DECODER_EXPANDER = 3, };
The type of region is hard-coded as type 3 expander now, because this is the only supported device type. As a preparation to support type 2 accelerators, we set the type of region to that of the first endpoint. Then, we will check whether the type of region is same as the type of other endpoints of the region. Because what we really need is to make sure the type of all endpoints of a region is same. The target type of endpoint devices comes from expander/accelerator device drivers via struct cxl_dev_state. Signed-off-by: "Huang, Ying" <ying.huang@intel.com> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Davidlohr Bueso <dave@stgolabs.net> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> Cc: Dave Jiang <dave.jiang@intel.com> Cc: Alison Schofield <alison.schofield@intel.com> Cc: Vishal Verma <vishal.l.verma@intel.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Alejandro Lucero <alucerop@amd.com> --- drivers/cxl/acpi.c | 1 - drivers/cxl/core/hdm.c | 28 +++++++++++++--------------- drivers/cxl/core/port.c | 2 ++ drivers/cxl/core/region.c | 13 +++++++------ drivers/cxl/cxl.h | 1 + 5 files changed, 23 insertions(+), 22 deletions(-)