RE: [PATCH v5 12/26] cxl/port: Store the port's Component Register mappings in struct cxl_port

Dan Williams
2023-06-10T02:18:32Z

Terry Bowman wrote:
> From: Robert Richter <rrichter@amd.com>
>
> CXL capabilities are stored in the Component Registers. To use them,
> the specific I/O ranges of the capabilities must be determined by
> probing the registers. For this, the whole Component Register range
> needs to be mapped temporarily to detect the offset and length of a
> capability range.
>
> In order to use more than one capability of a component (e.g. RAS and
> HDM) the Component Register are probed and its mappings created
> multiple times. This also causes overlapping I/O ranges as the whole
> Component Register range must be mapped again while a capability's I/O
> range is already mapped.
>
> Different capabilities cannot be setup at the same time. E.g. the RAS
> capability must be made available as soon as the PCI driver is bound,
> the HDM decoder is setup later during port enumeration. Moreover,
> during early setup it is still unknown if a certain capability is
> needed. A central capability setup is therefore not possible,
> capabilities must be individually enabled once needed during
> initialization.
>
> To avoid a duplicate register probe and overlapping I/O mappings, only
> probe the Component Registers one time and store the Component
> Register mapping in struct port. The stored mappings can be used later
> to iomap the capability register range when enabling the capability,
> which will be implemented in a follow-on patch.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
> drivers/cxl/core/port.c | 33 +++++++++++++++++++++++++++++++++
> drivers/cxl/cxl.h | 2 ++
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 8b688ac506ca..305125b193ce 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -686,6 +686,28 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
> return ERR_PTR(rc);
> }
>
> +static int cxl_setup_comp_regs(struct device *dev, struct cxl_register_map *map,
> + resource_size_t component_reg_phys)
> +{
> + if (component_reg_phys == CXL_RESOURCE_NONE)
> + return -ENODEV;
> +
> + memset(map, 0, sizeof(*map));
> + map->dev = dev;
> + map->reg_type = CXL_REGLOC_RBI_COMPONENT;
> + map->resource = component_reg_phys;
> + map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
> +
> + return cxl_setup_regs(map);
> +}
> +
> +static inline int cxl_port_setup_regs(struct cxl_port *port,
> + resource_size_t component_reg_phys)
> +{
> + return cxl_setup_comp_regs(&port->dev, &port->comp_map,
> + component_reg_phys);
> +}
> +
> static struct cxl_port *__devm_cxl_add_port(struct device *host,
> struct device *uport,
> resource_size_t component_reg_phys,
> @@ -709,6 +731,17 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
> if (rc)
> goto err;
>
> + /*
> + * Some components may not use capablities or their
> + * implementation is optional. A component register block may
> + * not be present then and component_reg_phys is therefore
> + * unset. Instead run the check later when setting up the
> + * capabilities.
> + */
> + rc = cxl_port_setup_regs(port, component_reg_phys);
> + if (rc && rc != -ENODEV)
> + goto err;

I would prefer to unwrap some of the levels and just do:

port->comp_map = (struct cxl_register_map) {
.dev = &port->dev;
.reg_type = CXL_REGLOC_RBI_COMPONENT;
.resource = component_reg_phys;
.max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
};

if (component_reg_phys != CXL_RESOURCE_NONE) {
rc = cxl_setup_regs(&port->comp_map);
if (rc)
goto err;
}

...earlier inside of cxl_port_alloc() since that part is common to all
ports. Then the confusing comment becomes unnecessary because it is
clear that CXL_RESOURCE_NONE is an acceptable case.

Other than that, the approach and rationale looks good.