| Message ID | 20211022183709.1199701-13-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL Region Creation / HDM decoder programming | expand |
On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Bake component registers into the memdev creation API in order to be > able to use them as part of driver probing. > > Component register base addresses are obtained through PCI mechanisms. > As such it makes most sense for the cxl_pci driver to obtain that > address. In order to reuse the port driver for enumerating decoder > resources for an endpoint, it is desirable to be able to add the > endpoint as a port. Unfortunately, by the time an endpoint driver would > run, it no longer has any concept of the underlying PCI device (this is > done intentionally to provide separation between drivers). Changelog looks good. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/core/memdev.c | 5 +++-- > drivers/cxl/cxlmem.h | 5 ++++- > drivers/cxl/pci.c | 17 ++++++++++++++++- > tools/testing/cxl/test/mem.c | 3 ++- > 4 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index bf1b04d00ff4..15762c16d83f 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -276,8 +276,8 @@ static const struct file_operations cxl_memdev_fops = { > .llseek = noop_llseek, > }; > > -struct cxl_memdev * > -devm_cxl_add_memdev(struct cxl_mem *cxlm) > +struct cxl_memdev *devm_cxl_add_memdev(struct cxl_mem *cxlm, > + resource_size_t component_reg_phys) > { > struct cxl_memdev *cxlmd; > struct device *dev; > @@ -298,6 +298,7 @@ devm_cxl_add_memdev(struct cxl_mem *cxlm) > * needed as this is ordered with cdev_add() publishing the device. > */ > cxlmd->cxlm = cxlm; > + cxlmd->creg_base = component_reg_phys; Let's pick one style for both, either rename component_reg_phys to creg_base, or creg_base to component_reg_phys. My preference is component_reg_phys, but I won't put up much of a fight against creg_base. > > cdev = &cxlmd->cdev; > rc = cdev_device_add(cdev, dev); > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index c4f450ad434d..62fe8e2c59e4 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -35,12 +35,14 @@ > * @cdev: char dev core object for ioctl operations > * @cxlm: pointer to the parent device driver data > * @id: id number of this memdev instance. > + * @creg_base: register base of component registers > */ > struct cxl_memdev { > struct device dev; > struct cdev cdev; > struct cxl_mem *cxlm; > int id; > + resource_size_t creg_base; > }; > > static inline struct cxl_memdev *to_cxl_memdev(struct device *dev) > @@ -48,7 +50,8 @@ static inline struct cxl_memdev *to_cxl_memdev(struct device *dev) > return container_of(dev, struct cxl_memdev, dev); > } > > -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_mem *cxlm); > +struct cxl_memdev *devm_cxl_add_memdev(struct cxl_mem *cxlm, > + resource_size_t component_reg_phys); > > /** > * struct cxl_mbox_cmd - A command to be submitted to hardware. > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index d1adc759d051..96a312ed8269 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -421,6 +421,7 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, > > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > + resource_size_t creg = CXL_RESOURCE_NONE; > struct cxl_register_map map; > struct cxl_memdev *cxlmd; > struct cxl_mem *cxlm; > @@ -465,7 +466,21 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > - cxlmd = devm_cxl_add_memdev(cxlm); > + /* > + * If the component registers can't be found, the cxl_pci driver may > + * still be useful for management functions so don't return an error. This comment makes sense... > + * > + * XXX: Creating the device is going to kick of the cxl_mem probing. > + * That probe requires the component registers. Therefore, the register > + * block must always be found first. > + */ ..., but I don't understand the point of this comment. Given that devm_cxl_add_memdev() takes the base address as an argument it's already clear that the component registers need to be found before devm_cxl_add_memdev(). > + rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map); > + if (rc) > + dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc); > + else > + creg = cxl_reg_block(pdev, &map); > + > + cxlmd = devm_cxl_add_memdev(cxlm, creg); > if (IS_ERR(cxlmd)) > return PTR_ERR(cxlmd); > > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c > index 12a8437a9ca0..471fc7fb5418 100644 > --- a/tools/testing/cxl/test/mem.c > +++ b/tools/testing/cxl/test/mem.c > @@ -227,7 +227,8 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > if (rc) > return rc; > > - cxlmd = devm_cxl_add_memdev(cxlm); > + /* TODO: mock component registers, or... */ How about: /* cxl_test does not emulate registers, any memdev operations that imply component register access will be mocked at the memdev operations interface */ > + cxlmd = devm_cxl_add_memdev(cxlm, CXL_RESOURCE_NONE); > if (IS_ERR(cxlmd)) > return PTR_ERR(cxlmd); > > -- > 2.33.1 >
On 21-10-31 13:13:32, Dan Williams wrote: > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > [snip] > > - cxlmd = devm_cxl_add_memdev(cxlm); > > + /* > > + * If the component registers can't be found, the cxl_pci driver may > > + * still be useful for management functions so don't return an error. > > This comment makes sense... > > > + * > > + * XXX: Creating the device is going to kick of the cxl_mem probing. > > + * That probe requires the component registers. Therefore, the register > > + * block must always be found first. > > + */ > > ..., but I don't understand the point of this comment. Given that > devm_cxl_add_memdev() takes the base address as an argument it's > already clear that the component registers need to be found before > devm_cxl_add_memdev(). > That comment was created before devm_cxl_add_memdev() took the base. Indeed the comment no longer makes sense. > > + rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map); > > + if (rc) > > + dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc); > > + else > > + creg = cxl_reg_block(pdev, &map); > > + > > + cxlmd = devm_cxl_add_memdev(cxlm, creg); > > if (IS_ERR(cxlmd)) > > return PTR_ERR(cxlmd); > > > > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c > > index 12a8437a9ca0..471fc7fb5418 100644 > > --- a/tools/testing/cxl/test/mem.c > > +++ b/tools/testing/cxl/test/mem.c > > @@ -227,7 +227,8 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > > if (rc) > > return rc; > > > > - cxlmd = devm_cxl_add_memdev(cxlm); > > + /* TODO: mock component registers, or... */ > > How about: > > /* cxl_test does not emulate registers, any memdev operations that > imply component register access will be mocked at the memdev > operations interface */ > Sounds good to me. > > + cxlmd = devm_cxl_add_memdev(cxlm, CXL_RESOURCE_NONE); > > if (IS_ERR(cxlmd)) > > return PTR_ERR(cxlmd); > > > > -- > > 2.33.1 > >
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index bf1b04d00ff4..15762c16d83f 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -276,8 +276,8 @@ static const struct file_operations cxl_memdev_fops = { .llseek = noop_llseek, }; -struct cxl_memdev * -devm_cxl_add_memdev(struct cxl_mem *cxlm) +struct cxl_memdev *devm_cxl_add_memdev(struct cxl_mem *cxlm, + resource_size_t component_reg_phys) { struct cxl_memdev *cxlmd; struct device *dev; @@ -298,6 +298,7 @@ devm_cxl_add_memdev(struct cxl_mem *cxlm) * needed as this is ordered with cdev_add() publishing the device. */ cxlmd->cxlm = cxlm; + cxlmd->creg_base = component_reg_phys; cdev = &cxlmd->cdev; rc = cdev_device_add(cdev, dev); diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index c4f450ad434d..62fe8e2c59e4 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -35,12 +35,14 @@ * @cdev: char dev core object for ioctl operations * @cxlm: pointer to the parent device driver data * @id: id number of this memdev instance. + * @creg_base: register base of component registers */ struct cxl_memdev { struct device dev; struct cdev cdev; struct cxl_mem *cxlm; int id; + resource_size_t creg_base; }; static inline struct cxl_memdev *to_cxl_memdev(struct device *dev) @@ -48,7 +50,8 @@ static inline struct cxl_memdev *to_cxl_memdev(struct device *dev) return container_of(dev, struct cxl_memdev, dev); } -struct cxl_memdev *devm_cxl_add_memdev(struct cxl_mem *cxlm); +struct cxl_memdev *devm_cxl_add_memdev(struct cxl_mem *cxlm, + resource_size_t component_reg_phys); /** * struct cxl_mbox_cmd - A command to be submitted to hardware. diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index d1adc759d051..96a312ed8269 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -421,6 +421,7 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { + resource_size_t creg = CXL_RESOURCE_NONE; struct cxl_register_map map; struct cxl_memdev *cxlmd; struct cxl_mem *cxlm; @@ -465,7 +466,21 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; - cxlmd = devm_cxl_add_memdev(cxlm); + /* + * If the component registers can't be found, the cxl_pci driver may + * still be useful for management functions so don't return an error. + * + * XXX: Creating the device is going to kick of the cxl_mem probing. + * That probe requires the component registers. Therefore, the register + * block must always be found first. + */ + rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map); + if (rc) + dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc); + else + creg = cxl_reg_block(pdev, &map); + + cxlmd = devm_cxl_add_memdev(cxlm, creg); if (IS_ERR(cxlmd)) return PTR_ERR(cxlmd); diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c index 12a8437a9ca0..471fc7fb5418 100644 --- a/tools/testing/cxl/test/mem.c +++ b/tools/testing/cxl/test/mem.c @@ -227,7 +227,8 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) if (rc) return rc; - cxlmd = devm_cxl_add_memdev(cxlm); + /* TODO: mock component registers, or... */ + cxlmd = devm_cxl_add_memdev(cxlm, CXL_RESOURCE_NONE); if (IS_ERR(cxlmd)) return PTR_ERR(cxlmd);
Bake component registers into the memdev creation API in order to be able to use them as part of driver probing. Component register base addresses are obtained through PCI mechanisms. As such it makes most sense for the cxl_pci driver to obtain that address. In order to reuse the port driver for enumerating decoder resources for an endpoint, it is desirable to be able to add the endpoint as a port. Unfortunately, by the time an endpoint driver would run, it no longer has any concept of the underlying PCI device (this is done intentionally to provide separation between drivers). Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/core/memdev.c | 5 +++-- drivers/cxl/cxlmem.h | 5 ++++- drivers/cxl/pci.c | 17 ++++++++++++++++- tools/testing/cxl/test/mem.c | 3 ++- 4 files changed, 25 insertions(+), 5 deletions(-)