| Message ID | 164740405408.3912056.16337643017370667205.stgit@dwillia2-desk3.amr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] cxl/pci: Cleanup repeated code in cxl_probe_regs() helpers | expand |
On Tue, 15 Mar 2022 21:14:14 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > The RAS Capabilitiy Structure is a CXL Component register capability > block. Unlike the HDM Decoder Capability, it will be referenced by the > cxl_pci driver in response to PCIe AER events. Due to this it is no > longer the case that cxl_map_component_regs() can assume that it should > map all component registers. Plumb a bitmask of capability ids to map > through cxl_map_component_regs(). > > For symmetry cxl_probe_device_regs() is updated to populate @id in > 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a > need to map a subset of the device registers per caller. I guess it doesn't hurt to add that, though without the mask being passed into appropriate functions it's a bit pointless. What we have is now 'half symmetric' perhaps. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> A few trivial comments inline, but basically looks good to me. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/hdm.c | 3 ++- > drivers/cxl/core/regs.c | 36 ++++++++++++++++++++++++++---------- > drivers/cxl/cxl.h | 7 ++++--- > 3 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 09221afca309..b348217ab704 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb, > return -ENXIO; > } > > - return cxl_map_component_regs(&port->dev, regs, &map); > + return cxl_map_component_regs(&port->dev, regs, &map, > + BIT(CXL_CM_CAP_CAP_ID_HDM)); > } > > /** > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index 219c7d0e43e2..c022c8937dfc 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, > if (!rmap) > continue; > rmap->valid = true; > + rmap->id = cap_id; > rmap->offset = CXL_CM_OFFSET + offset; > rmap->size = length; > } > @@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, > if (!rmap) > continue; > rmap->valid = true; > + rmap->id = cap_id; > rmap->offset = offset; > rmap->size = length; > } > @@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr, > } > > int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, > - struct cxl_register_map *map) > + struct cxl_register_map *map, unsigned long map_mask) Maybe pass an unsigned long *map_mask from the start for the inevitable capability IDs passing the minimum length of a long? Disadvantage being you'll need to roll a local BITMAP to pass in at all callsites. > { > - resource_size_t phys_addr; > - resource_size_t length; > - > - phys_addr = map->resource; > - phys_addr += map->component_map.hdm_decoder.offset; > - length = map->component_map.hdm_decoder.size; > - regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length); > - if (!regs->hdm_decoder) > - return -ENOMEM; > + struct mapinfo { > + struct cxl_reg_map *rmap; > + void __iomem **addr; > + } mapinfo[] = { > + { .rmap = &map->component_map.hdm_decoder, ®s->hdm_decoder }, As in previous instance, mixing two styles of assignment seems odd. > + }; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(mapinfo); i++) { > + struct mapinfo *mi = &mapinfo[i]; > + resource_size_t phys_addr; > + resource_size_t length; > + > + if (!mi->rmap->valid) > + continue; > + if (!test_bit(mi->rmap->id, &map_mask)) > + continue; > + phys_addr = map->resource + mi->rmap->offset; > + length = mi->rmap->size; > + *(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length); > + if (!*(mi->addr)) > + return -ENOMEM; > + } > > return 0; > } > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 2080a75c61fe..52bd77d8e22a 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -115,6 +115,7 @@ struct cxl_regs { > > struct cxl_reg_map { > bool valid; > + int id; > unsigned long offset; > unsigned long size; > }; > @@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, > struct cxl_component_reg_map *map); > void cxl_probe_device_regs(struct device *dev, void __iomem *base, > struct cxl_device_reg_map *map); > -int cxl_map_component_regs(struct device *dev, > - struct cxl_component_regs *regs, > - struct cxl_register_map *map); > +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, Worth the rewrapping and extra diff as a result? (I think it's precisely 80 chars) > + struct cxl_register_map *map, > + unsigned long map_mask); > int cxl_map_device_regs(struct device *dev, > struct cxl_device_regs *regs, > struct cxl_register_map *map); >
On 22-03-15 21:14:14, Dan Williams wrote: > The RAS Capabilitiy Structure is a CXL Component register capability > block. Unlike the HDM Decoder Capability, it will be referenced by the > cxl_pci driver in response to PCIe AER events. Due to this it is no > longer the case that cxl_map_component_regs() can assume that it should > map all component registers. Plumb a bitmask of capability ids to map > through cxl_map_component_regs(). > > For symmetry cxl_probe_device_regs() is updated to populate @id in > 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a > need to map a subset of the device registers per caller. This seems weird to me. You spent the first 4 or so patches consolidating the mapping into a nice loop only to break out an ID to do individual mappings again. Are you sure this is such a win over having discrete mapping functions? > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/core/hdm.c | 3 ++- > drivers/cxl/core/regs.c | 36 ++++++++++++++++++++++++++---------- > drivers/cxl/cxl.h | 7 ++++--- > 3 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 09221afca309..b348217ab704 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb, > return -ENXIO; > } > > - return cxl_map_component_regs(&port->dev, regs, &map); > + return cxl_map_component_regs(&port->dev, regs, &map, > + BIT(CXL_CM_CAP_CAP_ID_HDM)); > } > > /** > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index 219c7d0e43e2..c022c8937dfc 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, > if (!rmap) > continue; > rmap->valid = true; > + rmap->id = cap_id; > rmap->offset = CXL_CM_OFFSET + offset; > rmap->size = length; > } > @@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, > if (!rmap) > continue; > rmap->valid = true; > + rmap->id = cap_id; > rmap->offset = offset; > rmap->size = length; > } > @@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr, > } > > int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, > - struct cxl_register_map *map) > + struct cxl_register_map *map, unsigned long map_mask) > { > - resource_size_t phys_addr; > - resource_size_t length; > - > - phys_addr = map->resource; > - phys_addr += map->component_map.hdm_decoder.offset; > - length = map->component_map.hdm_decoder.size; > - regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length); > - if (!regs->hdm_decoder) > - return -ENOMEM; > + struct mapinfo { > + struct cxl_reg_map *rmap; > + void __iomem **addr; > + } mapinfo[] = { > + { .rmap = &map->component_map.hdm_decoder, ®s->hdm_decoder }, > + }; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(mapinfo); i++) { > + struct mapinfo *mi = &mapinfo[i]; > + resource_size_t phys_addr; > + resource_size_t length; > + > + if (!mi->rmap->valid) > + continue; > + if (!test_bit(mi->rmap->id, &map_mask)) > + continue; > + phys_addr = map->resource + mi->rmap->offset; > + length = mi->rmap->size; > + *(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length); > + if (!*(mi->addr)) > + return -ENOMEM; > + } > > return 0; > } > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 2080a75c61fe..52bd77d8e22a 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -115,6 +115,7 @@ struct cxl_regs { > > struct cxl_reg_map { > bool valid; > + int id; > unsigned long offset; > unsigned long size; > }; > @@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, > struct cxl_component_reg_map *map); > void cxl_probe_device_regs(struct device *dev, void __iomem *base, > struct cxl_device_reg_map *map); > -int cxl_map_component_regs(struct device *dev, > - struct cxl_component_regs *regs, > - struct cxl_register_map *map); > +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, > + struct cxl_register_map *map, > + unsigned long map_mask); > int cxl_map_device_regs(struct device *dev, > struct cxl_device_regs *regs, > struct cxl_register_map *map); >
On Thu, Mar 17, 2022 at 10:32 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 22-03-15 21:14:14, Dan Williams wrote: > > The RAS Capabilitiy Structure is a CXL Component register capability > > block. Unlike the HDM Decoder Capability, it will be referenced by the > > cxl_pci driver in response to PCIe AER events. Due to this it is no > > longer the case that cxl_map_component_regs() can assume that it should > > map all component registers. Plumb a bitmask of capability ids to map > > through cxl_map_component_regs(). > > > > For symmetry cxl_probe_device_regs() is updated to populate @id in > > 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a > > need to map a subset of the device registers per caller. > > This seems weird to me. You spent the first 4 or so patches consolidating the > mapping into a nice loop only to break out an ID to do individual mappings > again. Are you sure this is such a win over having discrete mapping functions? The loop is still there. This allows cxl_port and cxl_pci to share all the same logic save for a bitmap to select the block. You're angling for a: cxl_map_hdm_regs(&port->dev, regs, &map); ...? Internally that cxl_map_hdm_regs() should be sharing code with cxl_map_ras_regs(), so as far as I can see "discrete mapping functions" is just asking for the below, and I'd rather skip the extra wrapper: int cxl_map_hdm_regs(struct pci_dev *pdev, struct cxl_component_regs *regs, struct cxl_register_map *map) { return cxl_map_component_regs(&port->dev, regs, &map, BIT(CXL_CM_CAP_CAP_ID_HDM)); }
On Thu, Mar 17, 2022 at 3:57 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Tue, 15 Mar 2022 21:14:14 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > The RAS Capabilitiy Structure is a CXL Component register capability > > block. Unlike the HDM Decoder Capability, it will be referenced by the > > cxl_pci driver in response to PCIe AER events. Due to this it is no > > longer the case that cxl_map_component_regs() can assume that it should > > map all component registers. Plumb a bitmask of capability ids to map > > through cxl_map_component_regs(). > > > > For symmetry cxl_probe_device_regs() is updated to populate @id in > > 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a > > need to map a subset of the device registers per caller. > > I guess it doesn't hurt to add that, though without the mask being > passed into appropriate functions it's a bit pointless. What we have > is now 'half symmetric' perhaps. True, I was mainly worried about someone dumping the rmap->id in a debug session and wondering why all the ids are zero for device-register instances, but no need to add the per-id allocation since there's only one cxl_map_device_regs() caller today. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > A few trivial comments inline, but basically looks good to me. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > drivers/cxl/core/hdm.c | 3 ++- > > drivers/cxl/core/regs.c | 36 ++++++++++++++++++++++++++---------- > > drivers/cxl/cxl.h | 7 ++++--- > > 3 files changed, 32 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > > index 09221afca309..b348217ab704 100644 > > --- a/drivers/cxl/core/hdm.c > > +++ b/drivers/cxl/core/hdm.c > > @@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb, > > return -ENXIO; > > } > > > > - return cxl_map_component_regs(&port->dev, regs, &map); > > + return cxl_map_component_regs(&port->dev, regs, &map, > > + BIT(CXL_CM_CAP_CAP_ID_HDM)); > > } > > > > /** > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > > index 219c7d0e43e2..c022c8937dfc 100644 > > --- a/drivers/cxl/core/regs.c > > +++ b/drivers/cxl/core/regs.c > > @@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, > > if (!rmap) > > continue; > > rmap->valid = true; > > + rmap->id = cap_id; > > rmap->offset = CXL_CM_OFFSET + offset; > > rmap->size = length; > > } > > @@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, > > if (!rmap) > > continue; > > rmap->valid = true; > > + rmap->id = cap_id; > > rmap->offset = offset; > > rmap->size = length; > > } > > @@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr, > > } > > > > int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, > > - struct cxl_register_map *map) > > + struct cxl_register_map *map, unsigned long map_mask) > > Maybe pass an unsigned long *map_mask from the start for the inevitable > capability IDs passing the minimum length of a long? > Disadvantage being you'll need to roll a local BITMAP to pass in at all callsites. I'm ok to leave that to future folks since we're only up to 8 ids in today's spec, and the compiler will flag "error: left shift count >= width of type" if someone tries to add support for that high numbered capability id. > > > { > > - resource_size_t phys_addr; > > - resource_size_t length; > > - > > - phys_addr = map->resource; > > - phys_addr += map->component_map.hdm_decoder.offset; > > - length = map->component_map.hdm_decoder.size; > > - regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length); > > - if (!regs->hdm_decoder) > > - return -ENOMEM; > > + struct mapinfo { > > + struct cxl_reg_map *rmap; > > + void __iomem **addr; > > + } mapinfo[] = { > > + { .rmap = &map->component_map.hdm_decoder, ®s->hdm_decoder }, > > As in previous instance, mixing two styles of assignment seems odd. Yup, will fix. > > > + }; > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(mapinfo); i++) { > > + struct mapinfo *mi = &mapinfo[i]; > > + resource_size_t phys_addr; > > + resource_size_t length; > > + > > + if (!mi->rmap->valid) > > + continue; > > + if (!test_bit(mi->rmap->id, &map_mask)) > > + continue; > > + phys_addr = map->resource + mi->rmap->offset; > > + length = mi->rmap->size; > > + *(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length); > > + if (!*(mi->addr)) > > + return -ENOMEM; > > + } > > > > return 0; > > } > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 2080a75c61fe..52bd77d8e22a 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -115,6 +115,7 @@ struct cxl_regs { > > > > struct cxl_reg_map { > > bool valid; > > + int id; > > unsigned long offset; > > unsigned long size; > > }; > > @@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, > > struct cxl_component_reg_map *map); > > void cxl_probe_device_regs(struct device *dev, void __iomem *base, > > struct cxl_device_reg_map *map); > > -int cxl_map_component_regs(struct device *dev, > > - struct cxl_component_regs *regs, > > - struct cxl_register_map *map); > > +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, > > Worth the rewrapping and extra diff as a result? (I think it's precisely 80 chars) clang-format says yes. If I ask it to flow just the new @map_mask argument it reflows the whole declaration. However, this formatting change should be pushed back to "[PATCH 4/8] cxl/core/regs: Make cxl_map_{component, device}_regs() device generic", because that's the patch that reduced the first argument from "struct pci_dev *pdev" to the shorter "struct device *dev" allowing the second arg to move onto the same line. > > > + struct cxl_register_map *map, > > + unsigned long map_mask); > > int cxl_map_device_regs(struct device *dev, > > struct cxl_device_regs *regs, > > struct cxl_register_map *map); > > >
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 09221afca309..b348217ab704 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -92,7 +92,8 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb, return -ENXIO; } - return cxl_map_component_regs(&port->dev, regs, &map); + return cxl_map_component_regs(&port->dev, regs, &map, + BIT(CXL_CM_CAP_CAP_ID_HDM)); } /** diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index 219c7d0e43e2..c022c8937dfc 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -92,6 +92,7 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, if (!rmap) continue; rmap->valid = true; + rmap->id = cap_id; rmap->offset = CXL_CM_OFFSET + offset; rmap->size = length; } @@ -159,6 +160,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base, if (!rmap) continue; rmap->valid = true; + rmap->id = cap_id; rmap->offset = offset; rmap->size = length; } @@ -187,17 +189,31 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr, } int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, - struct cxl_register_map *map) + struct cxl_register_map *map, unsigned long map_mask) { - resource_size_t phys_addr; - resource_size_t length; - - phys_addr = map->resource; - phys_addr += map->component_map.hdm_decoder.offset; - length = map->component_map.hdm_decoder.size; - regs->hdm_decoder = devm_cxl_iomap_block(dev, phys_addr, length); - if (!regs->hdm_decoder) - return -ENOMEM; + struct mapinfo { + struct cxl_reg_map *rmap; + void __iomem **addr; + } mapinfo[] = { + { .rmap = &map->component_map.hdm_decoder, ®s->hdm_decoder }, + }; + int i; + + for (i = 0; i < ARRAY_SIZE(mapinfo); i++) { + struct mapinfo *mi = &mapinfo[i]; + resource_size_t phys_addr; + resource_size_t length; + + if (!mi->rmap->valid) + continue; + if (!test_bit(mi->rmap->id, &map_mask)) + continue; + phys_addr = map->resource + mi->rmap->offset; + length = mi->rmap->size; + *(mi->addr) = devm_cxl_iomap_block(dev, phys_addr, length); + if (!*(mi->addr)) + return -ENOMEM; + } return 0; } diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 2080a75c61fe..52bd77d8e22a 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -115,6 +115,7 @@ struct cxl_regs { struct cxl_reg_map { bool valid; + int id; unsigned long offset; unsigned long size; }; @@ -153,9 +154,9 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base, struct cxl_component_reg_map *map); void cxl_probe_device_regs(struct device *dev, void __iomem *base, struct cxl_device_reg_map *map); -int cxl_map_component_regs(struct device *dev, - struct cxl_component_regs *regs, - struct cxl_register_map *map); +int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs, + struct cxl_register_map *map, + unsigned long map_mask); int cxl_map_device_regs(struct device *dev, struct cxl_device_regs *regs, struct cxl_register_map *map);
The RAS Capabilitiy Structure is a CXL Component register capability block. Unlike the HDM Decoder Capability, it will be referenced by the cxl_pci driver in response to PCIe AER events. Due to this it is no longer the case that cxl_map_component_regs() can assume that it should map all component registers. Plumb a bitmask of capability ids to map through cxl_map_component_regs(). For symmetry cxl_probe_device_regs() is updated to populate @id in 'struct cxl_reg_map' even though cxl_map_device_regs() does not have a need to map a subset of the device registers per caller. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/hdm.c | 3 ++- drivers/cxl/core/regs.c | 36 ++++++++++++++++++++++++++---------- drivers/cxl/cxl.h | 7 ++++--- 3 files changed, 32 insertions(+), 14 deletions(-)