Re: [PATCH 6/8] cxl/pci: Prepare for mapping RAS Capability Structure

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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, &regs->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);
>