Messages in this thread | | | Date | Thu, 27 May 2021 10:53:05 -0700 | From | Ira Weiny <> | Subject | Re: [PATCH v2 3/5] cxl/mem: Map registers based on capabilities |
| |
On Tue, May 25, 2021 at 10:52:14AM +0100, Jonathan Cameron wrote: > On Fri, 21 May 2021 17:11:52 -0700 > <ira.weiny@intel.com> wrote: > > > From: Ira Weiny <ira.weiny@intel.com> > > > > The information required to map registers based on capabilities is > > contained within the bars themselves. This means the bar must be mapped > > to read the information needed and then unmapped to map the individual > > parts of the BAR based on capabilities. > > > > Change cxl_setup_device_regs() to return a new cxl_register_map, change > > the name to cxl_probe_device_regs(). Allocate and place > > cxl_register_maps on a list while processing all of the specified > > register blocks. > > > > After probing all the register blocks go back and map smaller registers > > blocks based on their capabilities and dispose of the cxl_register_maps. > > > > NOTE: pci_iomap() is not managed automatically via pcim_enable_device() > > so be careful to call pci_iounmap() correctly. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > A couple of really minor queries inline, but otherwise looks good to me. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Thanks!
> > > > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c > > index 38979c97158d..add66a6ec875 100644 > > --- a/drivers/cxl/core.c > > +++ b/drivers/cxl/core.c > > @@ -3,6 +3,7 @@ > > #include <linux/io-64-nonatomic-lo-hi.h> > > #include <linux/device.h> > > #include <linux/module.h> > > +#include <linux/pci.h> > > #include "cxl.h" > > > > /** > > @@ -12,19 +13,13 @@ > > * point for cross-device interleave coordination through cxl ports. > > */ > > > > -/** > > - * cxl_setup_device_regs() - Detect CXL Device register blocks > > - * @dev: Host device of the @base mapping > > - * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface > > - * @regs: Base pointers for device register blocks (see CXL_DEVICE_REGS()) > > - */ > > Nice to keep docs given this is an exported function.
I can write something better but the above does not add much IMO. The parameter explanations are unnecessary IMO.
> > > > @@ -1030,30 +1091,38 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > > dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n", > > bar, offset, reg_type); > > > > - if (reg_type == CXL_REGLOC_RBI_MEMDEV) { > > - base = cxl_mem_map_regblock(cxlm, bar, offset); > > - if (!base) > > - return -ENOMEM; > > - break; > > + base = cxl_mem_map_regblock(cxlm, bar, offset); > > + if (!base) { > > + ret = -ENOMEM; > > + goto free_maps; > > } > > - } > > > > - if (i == regblocks) { > > - dev_err(dev, "Missing register locator for device registers\n"); > > - return -ENXIO; > > Do we have or need an equivalent of this check somewhere else?
Yes agreed! I moved the check to cxl_probe_regs which returns -ENXIO if the register sets expected are not found. A check is also added to RBI_COMPONENT register type in the following patch. This was moved mainly because each register type is going to know better what it needs for proper operation. cxl_probe_device_regs() really can't know that after this series.
Ira
| |