diff mbox series

[7/8] cxl/port: Introduce cxl_port objects

Message ID 162042791852.1202325.8197739881935753009.stgit@dwillia2-desk3.amr.corp.intel.com
State New
Headers show
Series CXL Port Enumeration and Plans for v5.14 | expand

Commit Message

Dan Williams May 7, 2021, 10:51 p.m. UTC 
Once the cxl_root is established then other ports in the hierarchy can
be attached. The cxl_port object, unlike cxl_root that is associated
with host bridges, is associated with PCIe Root Ports or PCIe Switch
Ports. Add cxl_port instances for all PCIe Root Ports in an ACPI0016
host bridge. The cxl_port instances for PCIe Switch Ports are not
included here as those are to be modeled as another service device
registered on the pcie_port_bus_type.

A sample sysfs topology for a single-host-bridge with
single-PCIe/CXL-port follows:

/sys/bus/cxl/devices/root0
├── address_space0
│   ├── devtype
│   ├── end
│   ├── start
│   ├── supports_ram
│   ├── supports_type2
│   ├── supports_type3
│   └── uevent
├── address_space1
│   ├── devtype
│   ├── end
│   ├── start
│   ├── supports_pmem
│   ├── supports_type2
│   ├── supports_type3
│   └── uevent
├── devtype
├── port1
│   ├── devtype
│   ├── host -> ../../../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
│   ├── port2
│   │   ├── devtype
│   │   ├── host -> ../../../../../pci0000:34/0000:34:00.0
│   │   ├── subsystem -> ../../../../../../bus/cxl
│   │   ├── target_id
│   │   └── uevent
│   ├── subsystem -> ../../../../../bus/cxl
│   ├── target_id
│   └── uevent
├── subsystem -> ../../../../bus/cxl
├── target_id
└── uevent

In this listing the system-wide-singleton root0 has 2 address spaces, 1
PMEM and 1 RAM. Those address spaces are accessed through port1 which
represents the upstream port of an ACPI0016 host-bridge. A
multi-host-bridge system would have other ports as peers to port1 to
additionally decode root level address spaces. Port2 in this diagram
represents the single downstream port of the host-bridge. Were it to be
a multi-ported-host-bridge there would be peers / siblings of port2 with
port1 as their common ancestor.

The rationale for this port hierarchy is to be able to walk the HDM
decoder register sets that each port implements. Additionally it
provides a representation of host-bridge interleave which will be
necessary for follow-on work that adds CXL region devices.

The details in the /sys/bus/cxl hierarchy that are not suitable to be
represented in the /sys/bus/pci hierarchy are:
- memory address spaces that are interleaved across host bridges
- common sub-device functionality represented by CXL component + device
  registers (enumerated via DVSEC or platform firmware (ACPI CEDT)).

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/acpi.c |   99 +++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/core.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h  |    5 ++
 3 files changed, 224 insertions(+), 1 deletion(-)

Comments

kernel test robot May 8, 2021, 2:24 a.m. UTC | #1 
Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on a38fd8748464831584a19438cbb3082b5a2dab15]

url:    https://github.com/0day-ci/linux/commits/Dan-Williams/CXL-Port-Enumeration-and-Plans-for-v5-14/20210508-065317
base:   a38fd8748464831584a19438cbb3082b5a2dab15
config: i386-randconfig-a003-20210507 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/10f83390eae24effd86455f46429d03ae7c35f53
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dan-Williams/CXL-Port-Enumeration-and-Plans-for-v5-14/20210508-065317
        git checkout 10f83390eae24effd86455f46429d03ae7c35f53
        # save the attached .config to linux build tree
        make W=1 W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/cxl/acpi.c: In function 'cxl_acpi_register_ports':
>> drivers/cxl/acpi.c:76:55: warning: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Woverflow]
      76 |  port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
         |                                                       ^~~~~


vim +76 drivers/cxl/acpi.c

    61	
    62	/*
    63	 * A host bridge may contain one or more root ports.  Register each port
    64	 * as a child of the cxl_root.
    65	 */
    66	static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
    67					   struct cxl_port *port, int idx)
    68	{
    69		struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
    70		struct cxl_walk_context ctx;
    71	
    72		if (!pci_root)
    73			return -ENXIO;
    74	
    75		/* TODO: fold in CEDT.CHBS retrieval */
  > 76		port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
    77		if (IS_ERR(port))
    78			return PTR_ERR(port);
    79		dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
    80			dev_name(&port->dev));
    81	
    82		ctx = (struct cxl_walk_context) {
    83			.dev = dev,
    84			.root = pci_root->bus,
    85			.port = port,
    86		};
    87		pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
    88	
    89		if (ctx.count == 0)
    90			return -ENODEV;
    91		return ctx.error;
    92	}
    93	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Jonathan Cameron May 10, 2021, 3:21 p.m. UTC | #2 
On Fri, 7 May 2021 15:51:58 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Once the cxl_root is established then other ports in the hierarchy can
> be attached. The cxl_port object, unlike cxl_root that is associated
> with host bridges, is associated with PCIe Root Ports or PCIe Switch
> Ports. Add cxl_port instances for all PCIe Root Ports in an ACPI0016
> host bridge. The cxl_port instances for PCIe Switch Ports are not
> included here as those are to be modeled as another service device
> registered on the pcie_port_bus_type.
> 
> A sample sysfs topology for a single-host-bridge with
> single-PCIe/CXL-port follows:
> 
> /sys/bus/cxl/devices/root0
> ├── address_space0
> │   ├── devtype
> │   ├── end
> │   ├── start
> │   ├── supports_ram
> │   ├── supports_type2
> │   ├── supports_type3
> │   └── uevent
> ├── address_space1
> │   ├── devtype
> │   ├── end
> │   ├── start
> │   ├── supports_pmem
> │   ├── supports_type2
> │   ├── supports_type3
> │   └── uevent
> ├── devtype
> ├── port1
> │   ├── devtype
> │   ├── host -> ../../../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> │   ├── port2
> │   │   ├── devtype
> │   │   ├── host -> ../../../../../pci0000:34/0000:34:00.0
> │   │   ├── subsystem -> ../../../../../../bus/cxl
> │   │   ├── target_id
> │   │   └── uevent
> │   ├── subsystem -> ../../../../../bus/cxl
> │   ├── target_id
> │   └── uevent
> ├── subsystem -> ../../../../bus/cxl
> ├── target_id
> └── uevent
> 
> In this listing the system-wide-singleton root0 has 2 address spaces, 1
> PMEM and 1 RAM. Those address spaces are accessed through port1 which
> represents the upstream port of an ACPI0016 host-bridge. A
> multi-host-bridge system would have other ports as peers to port1 to
> additionally decode root level address spaces. Port2 in this diagram
> represents the single downstream port of the host-bridge. Were it to be
> a multi-ported-host-bridge there would be peers / siblings of port2 with
> port1 as their common ancestor.

I guess it would be a pain to emulate a system that actually had
multiple ports at the last level. Pity as would have made your
explanation here a little easier to follow.

> 
> The rationale for this port hierarchy is to be able to walk the HDM
> decoder register sets that each port implements. Additionally it
> provides a representation of host-bridge interleave which will be
> necessary for follow-on work that adds CXL region devices.
> 
> The details in the /sys/bus/cxl hierarchy that are not suitable to be
> represented in the /sys/bus/pci hierarchy are:
> - memory address spaces that are interleaved across host bridges
> - common sub-device functionality represented by CXL component + device
>   registers (enumerated via DVSEC or platform firmware (ACPI CEDT)).

I'm sold :)

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/acpi.c |   99 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/core.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h  |    5 ++
>  3 files changed, 224 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index d54c2d5de730..bc2a35ae880b 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -5,18 +5,117 @@
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/acpi.h>
> +#include <linux/pci.h>
>  #include "cxl.h"
>  
> +static int match_ACPI0016(struct device *dev, const void *host)
> +{
> +	struct acpi_device *adev = to_acpi_device(dev);
> +	const char *hid = acpi_device_hid(adev);
> +
> +	return strcmp(hid, "ACPI0016") == 0;
> +}
> +
> +struct cxl_walk_context {
> +	struct device *dev;
> +	struct pci_bus *root;
> +	struct cxl_port *port;
> +	int error;
> +	int count;
> +};
> +
> +static int match_add_root_ports(struct pci_dev *pdev, void *data)
> +{
> +	struct cxl_walk_context *ctx = data;
> +	struct pci_bus *root_bus = ctx->root;
> +	struct cxl_port *port = ctx->port;
> +	int type = pci_pcie_type(pdev);
> +	struct device *dev = ctx->dev;
> +	resource_size_t cxl_regs_phys;
> +	int target_id = ctx->count;
> +
> +	if (pdev->bus != root_bus)
> +		return 0;
> +	if (!pci_is_pcie(pdev))
> +		return 0;
> +	if (type != PCI_EXP_TYPE_ROOT_PORT)
> +		return 0;
> +
> +	ctx->count++;
> +
> +	/* TODO walk DVSEC to find component register base */
> +	cxl_regs_phys = -1;
> +
> +	port = devm_cxl_add_port(dev, port, &pdev->dev, target_id,
> +				 cxl_regs_phys);
> +	if (IS_ERR(port)) {
> +		ctx->error = PTR_ERR(port);
> +		return ctx->error;
> +	}
> +
> +	dev_dbg(dev, "%s: register: %s\n", dev_name(&pdev->dev),
> +		dev_name(&port->dev));
> +
> +	return 0;
> +}
> +
> +/*
> + * A host bridge may contain one or more root ports.  Register each port
> + * as a child of the cxl_root.
> + */
> +static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
> +				   struct cxl_port *port, int idx)
> +{
> +	struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
> +	struct cxl_walk_context ctx;
> +
> +	if (!pci_root)
> +		return -ENXIO;
> +
> +	/* TODO: fold in CEDT.CHBS retrieval */
> +	port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
> +	if (IS_ERR(port))
> +		return PTR_ERR(port);
> +	dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
> +		dev_name(&port->dev));
> +
> +	ctx = (struct cxl_walk_context) {
> +		.dev = dev,
> +		.root = pci_root->bus,
> +		.port = port,
> +	};
> +	pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
> +
> +	if (ctx.count == 0)
> +		return -ENODEV;
> +	return ctx.error;
> +}
> +
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct device *bridge = NULL;
>  	struct cxl_root *cxl_root;
> +	int rc, i = 0;
>  
>  	cxl_root = devm_cxl_add_root(dev, NULL, 0);
>  	if (IS_ERR(cxl_root))
>  		return PTR_ERR(cxl_root);
>  	dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
>  
> +	while (true) {
> +		bridge = bus_find_device(adev->dev.bus, bridge, dev,
> +					 match_ACPI0016);
> +		if (!bridge)
> +			break;
> +
> +		rc = cxl_acpi_register_ports(dev, to_acpi_device(bridge),
> +					     &cxl_root->port, i++);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 347824a62a66..cc9af9033292 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -148,6 +148,15 @@ static void cxl_root_release(struct device *dev)
>  	kfree(cxl_root);
>  }
>  
> +static void cxl_port_release(struct device *dev)
> +{
> +	struct cxl_port *port = to_cxl_port(dev);
> +
> +	ida_free(&cxl_port_ida, port->id);
> +	put_device(port->port_host);
> +	kfree(port);
> +}
> +
>  static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
>  			      char *buf)
>  {
> @@ -178,6 +187,12 @@ static const struct device_type cxl_root_type = {
>  	.groups = cxl_port_attribute_groups,
>  };
>  
> +static const struct device_type cxl_port_type = {
> +	.name = "cxl_port",
> +	.release = cxl_port_release,
> +	.groups = cxl_port_attribute_groups,
> +};
> +
>  struct cxl_root *to_cxl_root(struct device *dev)
>  {
>  	if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> @@ -188,7 +203,9 @@ struct cxl_root *to_cxl_root(struct device *dev)
>  
>  struct cxl_port *to_cxl_port(struct device *dev)
>  {
> -	if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> +	if (dev_WARN_ONCE(dev,
> +			  dev->type != &cxl_root_type &&
> +			  dev->type != &cxl_port_type,
>  			  "not a cxl_port device\n"))
>  		return NULL;
>  	return container_of(dev, struct cxl_port, dev);
> @@ -367,6 +384,108 @@ struct cxl_root *devm_cxl_add_root(struct device *host,
>  }
>  EXPORT_SYMBOL_GPL(devm_cxl_add_root);
>  
> +static void cxl_unlink_port(void *_port)
> +{
> +	struct cxl_port *port = _port;
> +
> +	sysfs_remove_link(&port->dev.kobj, "host");
> +}
> +
> +static int devm_cxl_link_port(struct device *dev, struct cxl_port *port)
> +{
> +	int rc;
> +
> +	rc = sysfs_create_link(&port->dev.kobj, &port->port_host->kobj, "host");
> +	if (rc)
> +		return rc;
> +	return devm_add_action_or_reset(dev, cxl_unlink_port, port);
> +}
> +
> +static struct cxl_port *cxl_port_alloc(struct cxl_port *parent_port,
> +				       struct device *port_dev, int target_id,
> +				       resource_size_t component_regs_phys)
> +{
> +	struct cxl_port *port;
> +	struct device *dev;
> +	int rc;
> +
> +	if (!port_dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> +	if (rc < 0)
> +		goto err;
> +
> +	port->id = rc;
> +	port->target_id = target_id;
> +	port->port_host = get_device(port_dev);
> +	port->component_regs_phys = component_regs_phys;
> +
> +	dev = &port->dev;
> +	device_initialize(dev);
> +	device_set_pm_not_required(dev);
> +	dev->parent = &parent_port->dev;
> +	dev->bus = &cxl_bus_type;
> +	dev->type = &cxl_port_type;
> +
> +	return port;
> +
> +err:
> +	kfree(port);
> +	return ERR_PTR(rc);
> +}
> +
> +/**
> + * devm_cxl_add_port() - add a cxl_port to the topology
> + * @host: devm context / discovery agent
> + * @parent_port: immediate ancestor towards cxl_root
> + * @port_host: PCI or platform-firmware device hosting this port
> + * @target_id: ordinal id relative to other siblings under @parent_port
> + * @component_regs_phys: CXL component register base address
> + */
> +struct cxl_port *devm_cxl_add_port(struct device *host,
> +				   struct cxl_port *parent_port,
> +				   struct device *port_host, int target_id,
> +				   resource_size_t component_regs_phys)
> +{
> +	struct cxl_port *port;
> +	struct device *dev;
> +	int rc;
> +
> +	port = cxl_port_alloc(parent_port, port_host, target_id,
> +			      component_regs_phys);
> +	if (IS_ERR(port))
> +		return port;
> +
> +	dev = &port->dev;
> +	rc = dev_set_name(dev, "port%d", port->id);
> +	if (rc)
> +		goto err;
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		goto err;
> +
> +	rc = devm_add_action_or_reset(host, unregister_dev, dev);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	rc = devm_cxl_link_port(host, port);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return port;
> +
> +err:
> +	put_device(dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> +
>  /**
>   * cxl_setup_device_regs() - Detect CXL Device register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 5cd1173151e5..71a991bdacb7 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -134,5 +134,10 @@ struct cxl_address_space_dev *to_cxl_address_space(struct device *dev);
>  struct cxl_root *devm_cxl_add_root(struct device *parent,
>  				   struct cxl_address_space *cxl_space,
>  				   int nr_spaces);
> +struct cxl_port *devm_cxl_add_port(struct device *host,
> +				   struct cxl_port *parent_port,
> +				   struct device *port_host, int target_id,
> +				   resource_size_t component_regs_phys);
> +
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
>
Dan Williams May 12, 2021, 6:36 a.m. UTC | #3 
On Mon, May 10, 2021 at 8:23 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 7 May 2021 15:51:58 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Once the cxl_root is established then other ports in the hierarchy can
> > be attached. The cxl_port object, unlike cxl_root that is associated
> > with host bridges, is associated with PCIe Root Ports or PCIe Switch
> > Ports. Add cxl_port instances for all PCIe Root Ports in an ACPI0016
> > host bridge. The cxl_port instances for PCIe Switch Ports are not
> > included here as those are to be modeled as another service device
> > registered on the pcie_port_bus_type.
> >
> > A sample sysfs topology for a single-host-bridge with
> > single-PCIe/CXL-port follows:
> >
> > /sys/bus/cxl/devices/root0
> > ├── address_space0
> > │   ├── devtype
> > │   ├── end
> > │   ├── start
> > │   ├── supports_ram
> > │   ├── supports_type2
> > │   ├── supports_type3
> > │   └── uevent
> > ├── address_space1
> > │   ├── devtype
> > │   ├── end
> > │   ├── start
> > │   ├── supports_pmem
> > │   ├── supports_type2
> > │   ├── supports_type3
> > │   └── uevent
> > ├── devtype
> > ├── port1
> > │   ├── devtype
> > │   ├── host -> ../../../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> > │   ├── port2
> > │   │   ├── devtype
> > │   │   ├── host -> ../../../../../pci0000:34/0000:34:00.0
> > │   │   ├── subsystem -> ../../../../../../bus/cxl
> > │   │   ├── target_id
> > │   │   └── uevent
> > │   ├── subsystem -> ../../../../../bus/cxl
> > │   ├── target_id
> > │   └── uevent
> > ├── subsystem -> ../../../../bus/cxl
> > ├── target_id
> > └── uevent
> >
> > In this listing the system-wide-singleton root0 has 2 address spaces, 1
> > PMEM and 1 RAM. Those address spaces are accessed through port1 which
> > represents the upstream port of an ACPI0016 host-bridge. A
> > multi-host-bridge system would have other ports as peers to port1 to
> > additionally decode root level address spaces. Port2 in this diagram
> > represents the single downstream port of the host-bridge. Were it to be
> > a multi-ported-host-bridge there would be peers / siblings of port2 with
> > port1 as their common ancestor.
>
> I guess it would be a pain to emulate a system that actually had
> multiple ports at the last level. Pity as would have made your
> explanation here a little easier to follow.
>

A pain in QEMU, but maybe not with a mocked implementation similar to
what gets injected for the nvdimm "nfit_test". I'll take a look.

> > The rationale for this port hierarchy is to be able to walk the HDM
> > decoder register sets that each port implements. Additionally it
> > provides a representation of host-bridge interleave which will be
> > necessary for follow-on work that adds CXL region devices.
> >
> > The details in the /sys/bus/cxl hierarchy that are not suitable to be
> > represented in the /sys/bus/pci hierarchy are:
> > - memory address spaces that are interleaved across host bridges
> > - common sub-device functionality represented by CXL component + device
> >   registers (enumerated via DVSEC or platform firmware (ACPI CEDT)).
>
> I'm sold :)

Thanks for the vote of confidence. It sounded like Bjorn was sold too
at the end of our last thread... I'll Cc him on this patch directly in
the resend.

>
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks for the review Jonathan, appreciate it.
diff mbox series

Patch

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index d54c2d5de730..bc2a35ae880b 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -5,18 +5,117 @@ 
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/acpi.h>
+#include <linux/pci.h>
 #include "cxl.h"
 
+static int match_ACPI0016(struct device *dev, const void *host)
+{
+	struct acpi_device *adev = to_acpi_device(dev);
+	const char *hid = acpi_device_hid(adev);
+
+	return strcmp(hid, "ACPI0016") == 0;
+}
+
+struct cxl_walk_context {
+	struct device *dev;
+	struct pci_bus *root;
+	struct cxl_port *port;
+	int error;
+	int count;
+};
+
+static int match_add_root_ports(struct pci_dev *pdev, void *data)
+{
+	struct cxl_walk_context *ctx = data;
+	struct pci_bus *root_bus = ctx->root;
+	struct cxl_port *port = ctx->port;
+	int type = pci_pcie_type(pdev);
+	struct device *dev = ctx->dev;
+	resource_size_t cxl_regs_phys;
+	int target_id = ctx->count;
+
+	if (pdev->bus != root_bus)
+		return 0;
+	if (!pci_is_pcie(pdev))
+		return 0;
+	if (type != PCI_EXP_TYPE_ROOT_PORT)
+		return 0;
+
+	ctx->count++;
+
+	/* TODO walk DVSEC to find component register base */
+	cxl_regs_phys = -1;
+
+	port = devm_cxl_add_port(dev, port, &pdev->dev, target_id,
+				 cxl_regs_phys);
+	if (IS_ERR(port)) {
+		ctx->error = PTR_ERR(port);
+		return ctx->error;
+	}
+
+	dev_dbg(dev, "%s: register: %s\n", dev_name(&pdev->dev),
+		dev_name(&port->dev));
+
+	return 0;
+}
+
+/*
+ * A host bridge may contain one or more root ports.  Register each port
+ * as a child of the cxl_root.
+ */
+static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
+				   struct cxl_port *port, int idx)
+{
+	struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
+	struct cxl_walk_context ctx;
+
+	if (!pci_root)
+		return -ENXIO;
+
+	/* TODO: fold in CEDT.CHBS retrieval */
+	port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
+	if (IS_ERR(port))
+		return PTR_ERR(port);
+	dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
+		dev_name(&port->dev));
+
+	ctx = (struct cxl_walk_context) {
+		.dev = dev,
+		.root = pci_root->bus,
+		.port = port,
+	};
+	pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
+
+	if (ctx.count == 0)
+		return -ENODEV;
+	return ctx.error;
+}
+
 static int cxl_acpi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct device *bridge = NULL;
 	struct cxl_root *cxl_root;
+	int rc, i = 0;
 
 	cxl_root = devm_cxl_add_root(dev, NULL, 0);
 	if (IS_ERR(cxl_root))
 		return PTR_ERR(cxl_root);
 	dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
 
+	while (true) {
+		bridge = bus_find_device(adev->dev.bus, bridge, dev,
+					 match_ACPI0016);
+		if (!bridge)
+			break;
+
+		rc = cxl_acpi_register_ports(dev, to_acpi_device(bridge),
+					     &cxl_root->port, i++);
+		if (rc)
+			return rc;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 347824a62a66..cc9af9033292 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -148,6 +148,15 @@  static void cxl_root_release(struct device *dev)
 	kfree(cxl_root);
 }
 
+static void cxl_port_release(struct device *dev)
+{
+	struct cxl_port *port = to_cxl_port(dev);
+
+	ida_free(&cxl_port_ida, port->id);
+	put_device(port->port_host);
+	kfree(port);
+}
+
 static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
@@ -178,6 +187,12 @@  static const struct device_type cxl_root_type = {
 	.groups = cxl_port_attribute_groups,
 };
 
+static const struct device_type cxl_port_type = {
+	.name = "cxl_port",
+	.release = cxl_port_release,
+	.groups = cxl_port_attribute_groups,
+};
+
 struct cxl_root *to_cxl_root(struct device *dev)
 {
 	if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
@@ -188,7 +203,9 @@  struct cxl_root *to_cxl_root(struct device *dev)
 
 struct cxl_port *to_cxl_port(struct device *dev)
 {
-	if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
+	if (dev_WARN_ONCE(dev,
+			  dev->type != &cxl_root_type &&
+			  dev->type != &cxl_port_type,
 			  "not a cxl_port device\n"))
 		return NULL;
 	return container_of(dev, struct cxl_port, dev);
@@ -367,6 +384,108 @@  struct cxl_root *devm_cxl_add_root(struct device *host,
 }
 EXPORT_SYMBOL_GPL(devm_cxl_add_root);
 
+static void cxl_unlink_port(void *_port)
+{
+	struct cxl_port *port = _port;
+
+	sysfs_remove_link(&port->dev.kobj, "host");
+}
+
+static int devm_cxl_link_port(struct device *dev, struct cxl_port *port)
+{
+	int rc;
+
+	rc = sysfs_create_link(&port->dev.kobj, &port->port_host->kobj, "host");
+	if (rc)
+		return rc;
+	return devm_add_action_or_reset(dev, cxl_unlink_port, port);
+}
+
+static struct cxl_port *cxl_port_alloc(struct cxl_port *parent_port,
+				       struct device *port_dev, int target_id,
+				       resource_size_t component_regs_phys)
+{
+	struct cxl_port *port;
+	struct device *dev;
+	int rc;
+
+	if (!port_dev)
+		return ERR_PTR(-EINVAL);
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return ERR_PTR(-ENOMEM);
+
+	rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
+	if (rc < 0)
+		goto err;
+
+	port->id = rc;
+	port->target_id = target_id;
+	port->port_host = get_device(port_dev);
+	port->component_regs_phys = component_regs_phys;
+
+	dev = &port->dev;
+	device_initialize(dev);
+	device_set_pm_not_required(dev);
+	dev->parent = &parent_port->dev;
+	dev->bus = &cxl_bus_type;
+	dev->type = &cxl_port_type;
+
+	return port;
+
+err:
+	kfree(port);
+	return ERR_PTR(rc);
+}
+
+/**
+ * devm_cxl_add_port() - add a cxl_port to the topology
+ * @host: devm context / discovery agent
+ * @parent_port: immediate ancestor towards cxl_root
+ * @port_host: PCI or platform-firmware device hosting this port
+ * @target_id: ordinal id relative to other siblings under @parent_port
+ * @component_regs_phys: CXL component register base address
+ */
+struct cxl_port *devm_cxl_add_port(struct device *host,
+				   struct cxl_port *parent_port,
+				   struct device *port_host, int target_id,
+				   resource_size_t component_regs_phys)
+{
+	struct cxl_port *port;
+	struct device *dev;
+	int rc;
+
+	port = cxl_port_alloc(parent_port, port_host, target_id,
+			      component_regs_phys);
+	if (IS_ERR(port))
+		return port;
+
+	dev = &port->dev;
+	rc = dev_set_name(dev, "port%d", port->id);
+	if (rc)
+		goto err;
+
+	rc = device_add(dev);
+	if (rc)
+		goto err;
+
+	rc = devm_add_action_or_reset(host, unregister_dev, dev);
+	if (rc)
+		return ERR_PTR(rc);
+
+	rc = devm_cxl_link_port(host, port);
+	if (rc)
+		return ERR_PTR(rc);
+
+	return port;
+
+err:
+	put_device(dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(devm_cxl_add_port);
+
 /**
  * cxl_setup_device_regs() - Detect CXL Device register blocks
  * @dev: Host device of the @base mapping
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5cd1173151e5..71a991bdacb7 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -134,5 +134,10 @@  struct cxl_address_space_dev *to_cxl_address_space(struct device *dev);
 struct cxl_root *devm_cxl_add_root(struct device *parent,
 				   struct cxl_address_space *cxl_space,
 				   int nr_spaces);
+struct cxl_port *devm_cxl_add_port(struct device *host,
+				   struct cxl_port *parent_port,
+				   struct device *port_host, int target_id,
+				   resource_size_t component_regs_phys);
+
 extern struct bus_type cxl_bus_type;
 #endif /* __CXL_H__ */