Messages in this thread | | | Date | Fri, 9 Jun 2023 18:36:48 -0700 | From | Dan Williams <> | Subject | RE: [PATCH v5 11/26] cxl/pci: Early setup RCH dport component registers from RCRB |
| |
Terry Bowman wrote: > From: Robert Richter <rrichter@amd.com> > > CXL RAS capabilities must be enabled and accessible as soon as the CXL > endpoint is detected in the PCI hierarchy and bound to the cxl_pci > driver. This needs to be independent of other modules such as cxl_port > or cxl_mem. > > CXL RAS capabilities reside in the Component Registers. For an RCH > this is determined by probing RCRB which is implemented very late once > the CXL Memory Device is created. > > Change this by moving the RCRB probe to the cxl_pci driver. Do this by > using a new introduced function cxl_pci_find_port() similar to > cxl_mem_find_port() to determine the involved dport by the endpoint's > PCI handle. Plug this into the existing cxl_pci_setup_regs() function > to setup Component Registers. Probe the RCRB in case the Component > Registers cannot be located through the CXL Register Locator > capability. > > This unifies code and early sets up the Component Registers at the > same time for both, VH and RCH mode. Only the cxl_pci driver is > involved for this. This allows an early mapping of the CXL RAS > capability registers.
This is problematic because it creates an implicit dependency between cxl_acpi and cxl_pci. More comments below:
> > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/port.c | 7 +++++++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/mem.c | 10 ---------- > drivers/cxl/pci.c | 37 ++++++++++++++++++++++++++++++++++++- > 4 files changed, 45 insertions(+), 11 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 82de858506c7..8b688ac506ca 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1476,6 +1476,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > } > EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL); > > +struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, > + struct cxl_dport **dport) > +{ > + return find_cxl_port(pdev->dev.parent, dport); > +} > +EXPORT_SYMBOL_NS_GPL(cxl_pci_find_port, CXL); > + > struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, > struct cxl_dport **dport) > { > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 1c6fe53e9dc7..e5ae5f4e6669 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -670,6 +670,8 @@ struct cxl_port *find_cxl_root(struct cxl_port *port); > int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd); > void cxl_bus_rescan(void); > void cxl_bus_drain(void); > +struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, > + struct cxl_dport **dport); > struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, > struct cxl_dport **dport); > bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd); > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index a34d6560c25c..0643852444f3 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -65,16 +65,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > ep->next = down; > } > > - /* > - * The component registers for an RCD might come from the > - * host-bridge RCRB if they are not already mapped via the > - * typical register locator mechanism. > - */ > - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > - cxlds->component_reg_phys = > - cxl_probe_rcrb(&cxlmd->dev, parent_dport->rcrb.base, > - NULL, CXL_RCRB_UPSTREAM); > - > endpoint = devm_cxl_add_port(host, &cxlmd->dev, > cxlds->component_reg_phys, > parent_dport); > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 945ca0304d68..2975b232fcd1 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -274,13 +274,48 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds) > return 0; > } > > +/* Extract RCRB, use same function interface as cxl_find_regblock(). */ > +static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, > + enum cxl_regloc_type type, > + struct cxl_register_map *map) > +{ > + struct cxl_dport *dport; > + resource_size_t component_reg_phys; > + > + memset(map, 0, sizeof(*map)); > + map->dev = &pdev->dev; > + map->resource = CXL_RESOURCE_NONE; > + > + if (type != CXL_REGLOC_RBI_COMPONENT) > + return -ENODEV;
Why would a function called cxl_rcrb_get_comp_regs() need a type parameter that only takes one acceptable value. I would drop the parameter and move this distinction to the caller.
> + > + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch) > + return -ENXIO;
This should return -EPROBE_DEFER in the !cxl_pci_find_port() case to try to await cxl_acpi initialization.
> + > + component_reg_phys = cxl_probe_rcrb(&pdev->dev, dport->rcrb.base, > + NULL, CXL_RCRB_UPSTREAM); > + if (component_reg_phys == CXL_RESOURCE_NONE) > + return -ENXIO; > + > + map->resource = component_reg_phys; > + map->reg_type = type; > + map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE; > + > + return 0; > +} > + > static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, > struct cxl_register_map *map) > { > int rc; > > + /* > + * If the Register Locator DVSEC does not contain the > + * Component Registers, assume it is an RCH and try to extract > + * them from an RCRB. > + */ > rc = cxl_find_regblock(pdev, type, map); > - if (rc) > + if (rc && cxl_rcrb_get_comp_regs(pdev, type, map)) > return rc;
I am not a big fan of just assuming this is an RCD, especially when there is the is_cxl_restricted() helper just beneath this.
| |