| Message ID | 20210902195017.2516472-9-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enumerate midlevel and endpoint decoders | expand |
On Thu, 2 Sep 2021 12:50:12 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > CXL endpoints contain HDM decoders that are architecturally the same as > a CXL switch, or a CXL hostbridge. While some restrictions are in place > for endpoints, they will require the same enumeration logic to determine > the number and abilities of the HDM decoders. > > Utilizing the existing port APIs from cxl_core is the simplest way to > gain access to the same set of information that switches and hostbridges > have. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> In of itself seems sensible. (note I'm reviewing these one at a time but reserve the right to throw my hands up in horror at the end result ;) Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/bus.c | 5 ++++- > drivers/cxl/mem.c | 10 +++++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > index 56f57302d27b..f26095b40f5c 100644 > --- a/drivers/cxl/core/bus.c > +++ b/drivers/cxl/core/bus.c > @@ -377,7 +377,10 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > > dev = &port->dev; > if (parent_port) > - rc = dev_set_name(dev, "port%d", port->id); > + if (host->type == &cxl_memdev_type) > + rc = dev_set_name(dev, "devport%d", port->id); > + else > + rc = dev_set_name(dev, "port%d", port->id); > else > rc = dev_set_name(dev, "root%d", port->id); > if (rc) > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index b6dc34d18a86..9d5a3a29cda1 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -63,6 +63,7 @@ static int cxl_mem_probe(struct device *dev) > struct device *pdev_parent = cxlm->dev->parent; > struct pci_dev *pdev = to_pci_dev(cxlm->dev); > struct device *port_dev; > + int rc; > > if (!is_cxl_mem_enabled(pdev)) > return -ENODEV; > @@ -72,7 +73,14 @@ static int cxl_mem_probe(struct device *dev) > if (!port_dev) > return -ENODEV; > > - return 0; > + /* TODO: Obtain component registers */ > + rc = PTR_ERR_OR_ZERO(devm_cxl_add_port(&cxlmd->dev, &cxlmd->dev, oh. Nasty / efficient depending on how you look at it. > + CXL_RESOURCE_NONE, > + to_cxl_port(port_dev))); > + if (rc) > + dev_err(dev, "Unable to add devices upstream port"); > + > + return rc; > } > > static void cxl_mem_remove(struct device *dev)
On Thu, Sep 2, 2021 at 12:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > CXL endpoints contain HDM decoders that are architecturally the same as > a CXL switch, or a CXL hostbridge. While some restrictions are in place > for endpoints, they will require the same enumeration logic to determine > the number and abilities of the HDM decoders. > > Utilizing the existing port APIs from cxl_core is the simplest way to > gain access to the same set of information that switches and hostbridges > have. Per the comment a few patches back I think this patch deserves to be moved before and referenced by the endpoint-decoder patch. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/core/bus.c | 5 ++++- > drivers/cxl/mem.c | 10 +++++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c > index 56f57302d27b..f26095b40f5c 100644 > --- a/drivers/cxl/core/bus.c > +++ b/drivers/cxl/core/bus.c > @@ -377,7 +377,10 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, > > dev = &port->dev; > if (parent_port) > - rc = dev_set_name(dev, "port%d", port->id); > + if (host->type == &cxl_memdev_type) > + rc = dev_set_name(dev, "devport%d", port->id); While I am certain that a root port will always be at the root, I'm only 99% convinced that port in a device will never have child-ports, so I'm inclined that this still be named "portX" and userspace must consult portX/devtype to determine the port rather than infer it from the name. > + else > + rc = dev_set_name(dev, "port%d", port->id); > else > rc = dev_set_name(dev, "root%d", port->id); > if (rc) > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index b6dc34d18a86..9d5a3a29cda1 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -63,6 +63,7 @@ static int cxl_mem_probe(struct device *dev) > struct device *pdev_parent = cxlm->dev->parent; > struct pci_dev *pdev = to_pci_dev(cxlm->dev); > struct device *port_dev; > + int rc; > > if (!is_cxl_mem_enabled(pdev)) > return -ENODEV; > @@ -72,7 +73,14 @@ static int cxl_mem_probe(struct device *dev) > if (!port_dev) > return -ENODEV; > > - return 0; > + /* TODO: Obtain component registers */ The agent that registered the memdev should have already enumerated them for this device. Let's not duplicate that enumeration. I would hope that this driver could be PCI details free and only operate on memory-mapped resources. > + rc = PTR_ERR_OR_ZERO(devm_cxl_add_port(&cxlmd->dev, &cxlmd->dev, I'd prefer this be broken out on multiple lines. port = devm_cxl_add_port(...); rc = PTR_ERR_OR_ZERO(port); > + CXL_RESOURCE_NONE, > + to_cxl_port(port_dev))); > + if (rc) > + dev_err(dev, "Unable to add devices upstream port"); Perhaps: "Failed to register port" ...it will already be clear that it's a device port from the device-name and driver that will be prepended to this print. > + > + return rc; > } > > static void cxl_mem_remove(struct device *dev) > -- > 2.33.0 >
diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c index 56f57302d27b..f26095b40f5c 100644 --- a/drivers/cxl/core/bus.c +++ b/drivers/cxl/core/bus.c @@ -377,7 +377,10 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport, dev = &port->dev; if (parent_port) - rc = dev_set_name(dev, "port%d", port->id); + if (host->type == &cxl_memdev_type) + rc = dev_set_name(dev, "devport%d", port->id); + else + rc = dev_set_name(dev, "port%d", port->id); else rc = dev_set_name(dev, "root%d", port->id); if (rc) diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index b6dc34d18a86..9d5a3a29cda1 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -63,6 +63,7 @@ static int cxl_mem_probe(struct device *dev) struct device *pdev_parent = cxlm->dev->parent; struct pci_dev *pdev = to_pci_dev(cxlm->dev); struct device *port_dev; + int rc; if (!is_cxl_mem_enabled(pdev)) return -ENODEV; @@ -72,7 +73,14 @@ static int cxl_mem_probe(struct device *dev) if (!port_dev) return -ENODEV; - return 0; + /* TODO: Obtain component registers */ + rc = PTR_ERR_OR_ZERO(devm_cxl_add_port(&cxlmd->dev, &cxlmd->dev, + CXL_RESOURCE_NONE, + to_cxl_port(port_dev))); + if (rc) + dev_err(dev, "Unable to add devices upstream port"); + + return rc; } static void cxl_mem_remove(struct device *dev)
CXL endpoints contain HDM decoders that are architecturally the same as a CXL switch, or a CXL hostbridge. While some restrictions are in place for endpoints, they will require the same enumeration logic to determine the number and abilities of the HDM decoders. Utilizing the existing port APIs from cxl_core is the simplest way to gain access to the same set of information that switches and hostbridges have. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/core/bus.c | 5 ++++- drivers/cxl/mem.c | 10 +++++++++- 2 files changed, 13 insertions(+), 2 deletions(-)