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.
>
> 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.
> {
> - 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);
>
