by Dave Jiang

[permalink] [raw]

Subject: Re: [PATCH v6 15/27] cxl/port: Store the port's Component Register mappings in struct cxl_port

On 6/21/23 20:51, Terry Bowman wrote:
> From: Robert Richter <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Terry Bowman <[email protected]>

Reviewed-by: Dave Jiang <[email protected]>
> ---
> drivers/cxl/core/port.c | 26 ++++++++++++++++++++++++++
> drivers/cxl/cxl.h | 2 ++
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 679226023f0c..2e239fd33df9 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -688,6 +688,28 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
> 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 0;
> +
> + 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_dev,
> resource_size_t component_reg_phys,
> @@ -711,6 +733,10 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
> if (rc)
> goto err;
>
> + rc = cxl_port_setup_regs(port, component_reg_phys);
> + if (rc)
> + goto err;
> +
> rc = device_add(dev);
> if (rc)
> goto err;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index fe95f08acb69..37fa5b565362 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -547,6 +547,7 @@ struct cxl_dax_region {
> * @regions: cxl_region_ref instances, regions mapped by this port
> * @parent_dport: dport that points to this port in the parent
> * @decoder_ida: allocator for decoder ids
> + * @comp_map: component register capability mappings
> * @nr_dports: number of entries in @dports
> * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> * @commit_end: cursor to track highest committed decoder for commit ordering
> @@ -566,6 +567,7 @@ struct cxl_port {
> struct xarray regions;
> struct cxl_dport *parent_dport;
> struct ida decoder_ida;
> + struct cxl_register_map comp_map;
> int nr_dports;
> int hdm_end;
> int commit_end;

2023-06-23 13:32:49

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v6 25/27] cxl/pci: Disable root port interrupts in RCH mode

On Thu, 22 Jun 2023 11:33:30 -0500
Terry Bowman <[email protected]> wrote:

> Hi Jonathan,
>
> On 6/22/23 08:12, Jonathan Cameron wrote:
> > On Wed, 21 Jun 2023 22:51:24 -0500
> > Terry Bowman <[email protected]> wrote:
> >
> >> The RCH root port contains root command AER registers that should not be
> >> enabled.[1] Disable these to prevent root port interrupts.
> >
> > I'm a little dubious about a 'because the spec says' so argument.
> > If we can describe the path by which spurious interrupts turn up then
> > great - if not then fair enough.
> >
>
> This was added to follow the spec. The RCH downstream port contains
> root port control pci express capability for enabling and disabling root
> port interrupts. The interrupts are (should be) disabled by default at
> powerup according to the PCI spec. We know SW does not enable because
> the RCH downstream port is not enumerated or managed by a port driver. I
> cant say this patch is absolutely necessary but was not comfortable with
> removing it either and want to avoid undefined behavior.
>
Maybe we should just abuse firmware authors and blame them for potentially
having changed the default :)

Fine as is.

Jonathan

> Regards,
> Terry