Re: [PATCH 2/6] cxl: Add BI register probing and port initialization

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Sun, 15 Mar 2026 13:27:37 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Add register probing for BI Route Table and BI Decoder capability
> structures in cxl_probe_component_regs(), and initialize BI registers
> during port probe for both switch ports and endpoint ports.
> 
> For switch ports, map BI Decoder registers on downstream ports and
> BI Route Table registers on upstream ports. For endpoint ports, map
> the BI Decoder registers directly into the port's register block.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Various minor things in here. Biggest one is whether we can use port->regs
for the switch upstream port registers.

> ---
>  drivers/cxl/core/regs.c | 13 ++++++
>  drivers/cxl/port.c      | 88 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 93710cf4f0a6..82e6018fd4cf 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -92,6 +92,18 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  			length = CXL_RAS_CAPABILITY_LENGTH;
>  			rmap = &map->ras;
>  			break;
> +		case CXL_CM_CAP_CAP_ID_BI_RT:
> +			dev_dbg(dev, "found BI RT capability (0x%x)\n",
> +				offset);
> +			length = CXL_BI_RT_CAPABILITY_LENGTH;
> +			rmap = &map->bi;
> +			break;
> +		case CXL_CM_CAP_CAP_ID_BI_DECODER:
> +			dev_dbg(dev, "found BI Decoder capability (0x%x)\n",
> +				offset);
> +			length = CXL_BI_DECODER_CAPABILITY_LENGTH;
> +			rmap = &map->bi;
This was what triggered my reply on whether we should separate the two bi
capabilities.  
> +			break;
>  		default:
>  			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
>  				offset);
> @@ -211,6 +223,7 @@ int cxl_map_component_regs(const struct cxl_register_map *map,
>  	} mapinfo[] = {
>  		{ &map->component_map.hdm_decoder, &regs->hdm_decoder },
>  		{ &map->component_map.ras, &regs->ras },
> +		{ &map->component_map.bi, &regs->bi },
>  	};
>  	int i;
>  
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index ada51948d52f..0540f0681ffb 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -58,6 +58,90 @@ static int discover_region(struct device *dev, void *unused)
>  	return 0;
>  }
>  
> +static int cxl_dport_init_bi(struct cxl_dport *dport)
> +{
> +	struct cxl_register_map *map = &dport->reg_map;
> +	struct device *dev = dport->dport_dev;
> +
> +	if (dport->regs.bi)
As below. Maybe a comment on why we might hit this twice.
> +		return 0;
> +
> +	if (!cxl_pci_flit_256(to_pci_dev(dev)))
> +		return 0;
> +
> +	if (!map->component_map.bi.valid) {
> +		dev_dbg(dev, "BI decoder registers not found\n");
> +		return 0;
> +	}
> +
> +	if (cxl_map_component_regs(map, &dport->regs.component,
> +				   BIT(CXL_CM_CAP_CAP_ID_BI_DECODER))) {
> +		dev_dbg(dev, "Failed to map BI decoder capability.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cxl_uport_init_bi(struct cxl_port *port, struct device *host)
> +{
> +	struct cxl_register_map *map = &port->reg_map;
> +
> +	if (port->uport_regs.bi)

Maybe a comment on why we'd hit this twice and locking requirements
that we don't end up racing between this check and initializing it.

In general uport_regs feels too general a name to me given we have
uport related registers mapped in various different places already.
I.e. in cxl_hdm and devm_cxl_port_ras_setup() which puts it in
port->regs.  Can we not put this one in there as well alongside the RAS
register map?  Assuming I read this right I'd be keen on a comment
update to make it clear that one is all about upstream port component
regs.


> +		return;
> +
> +	if (!map->component_map.bi.valid) {
> +		dev_dbg(host, "BI RT registers not found\n");
> +		return;
> +	}
> +
> +	map->host = host;
> +	if (cxl_map_component_regs(map, &port->uport_regs,
> +				   BIT(CXL_CM_CAP_CAP_ID_BI_RT)))
> +		dev_dbg(&port->dev, "Failed to map BI RT capability\n");
> +}
> +
> +static void cxl_endpoint_init_bi(struct cxl_port *port)
> +{
> +	struct cxl_register_map *map = &port->reg_map;
> +
> +	cxl_dport_init_bi(port->parent_dport);
> +
> +	if (!map->component_map.bi.valid)
> +		return;
> +
> +	if (cxl_map_component_regs(map, &port->regs,
> +				   BIT(CXL_CM_CAP_CAP_ID_BI_DECODER)))
> +		dev_dbg(&port->dev, "Failed to map BI decoder capability\n");
> +}
> +
> +static void cxl_switch_port_init_bi(struct cxl_port *port)
> +{
> +	struct cxl_dport *parent_dport = port->parent_dport;
> +
> +	if (is_cxl_root(to_cxl_port(port->dev.parent)))

Rings a bell as something I moaned about before in a different series
as a pattern that is repeated too much. Though I'm still not
hugely keen on this not being named for what it is really checking
about this device.  I believe this is excluding the

devices/platform/ACPI0017:00/root0/portX which are the host bridges.

	if (parent_port_is_cxl_root(port))
		return;

> +		return;
> +
> +	if (dev_is_pci(port->uport_dev) &&

I kind of wish we also had this named to indicate (I think) that it's
excluding CXL test devices.

> +	    !cxl_pci_flit_256(to_pci_dev(port->uport_dev)))
> +		return;
> +
> +	if (parent_dport && dev_is_pci(parent_dport->dport_dev)) {
> +		struct pci_dev *pdev = to_pci_dev(parent_dport->dport_dev);
> +
> +		switch (pci_pcie_type(pdev)) {
> +		case PCI_EXP_TYPE_ROOT_PORT:
> +		case PCI_EXP_TYPE_DOWNSTREAM:
> +			cxl_dport_init_bi(parent_dport);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	cxl_uport_init_bi(port, &port->dev);
> +}
> +
>  static int cxl_switch_port_probe(struct cxl_port *port)
>  {
>  	/* Reset nr_dports for rebind of driver */
> @@ -66,6 +150,8 @@ static int cxl_switch_port_probe(struct cxl_port *port)
>  	/* Cache the data early to ensure is_visible() works */
>  	read_cdat_data(port);
>  
> +	cxl_switch_port_init_bi(port);
> +
>  	return 0;
>  }
>  
> @@ -128,6 +214,8 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	read_cdat_data(port);
>  	cxl_endpoint_parse_cdat(port);
>  
> +	cxl_endpoint_init_bi(port);
> +
>  	get_device(&cxlmd->dev);
>  	rc = devm_add_action_or_reset(&port->dev, schedule_detach, cxlmd);
>  	if (rc)