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

From: Jonathan Cameron
Date: Mon May 10 2021 - 11:24:17 EST

Next message: Hyeonggon Yoo: "[PATCH v2] mm: kmalloc_index: make compiler break when size is not supported"
Previous message: Lee Jones: "Re: [PATCH v1 1/1] mfd: intel-lpss: Add Intel Alder Lake-M PCI IDs"
Next in thread: Dan Williams: "Re: [PATCH 7/8] cxl/port: Introduce cxl_port objects"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 7 May 2021 15:51:58 -0700
Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxx>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>

> ---
> 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__ */
>

Next message: Hyeonggon Yoo: "[PATCH v2] mm: kmalloc_index: make compiler break when size is not supported"
Previous message: Lee Jones: "Re: [PATCH v1 1/1] mfd: intel-lpss: Add Intel Alder Lake-M PCI IDs"
Next in thread: Dan Williams: "Re: [PATCH 7/8] cxl/port: Introduce cxl_port objects"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]