On Thu, Mar 17, 2022 at 3:57 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Tue, 15 Mar 2022 21:14:14 -0700
> Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxx>
> A few trivial comments inline, but basically looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> > ---
> > 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);
> >
>
