On Fri, Nov 19, 2021 at 04:02:47PM -0800, Ben Widawsky wrote:
> The CXL port driver is responsible for managing the decoder resources
> contained within the port. It will also provide APIs that other drivers
> will consume for managing these resources.
>
> There are 4 types of ports in a system:
> 1. Platform port. This is a non-programmable entity. Such a port is
> named rootX. It is enumerated by cxl_acpi in an ACPI based system.
Can you mention the ACPI source (static table, namespace PNP ID) here?
> 2. Hostbridge port.
Is "hostbridge" styled as a single word in the spec? I've only seen
"host bridge" elsewhere.
> This ports register access is defined in a platform
> specific way (CHBS for ACPI platforms).
s/This ports/This port's/
This doesn't really make sense, though. Are you saying the register
access *mechanism* is platform specific? Or merely that the
enumeration method (ACPI table, ACPI namespace, DT, etc) is
platform-specific?
I assume "CHBS" is an ACPI static table?
> It has n downstream ports,
> each of which are known as CXL 2.0 root ports.
This sounds like a "host bridge port *contains* these root ports."
That doesn't sound right.
> Once the platform
> specific mechanism to get the offset to the registers is obtained it
> operates just like other CXL components. The enumeration of this
> component is started by cxl_acpi and completed by cxl_port.
> 3. Switch port. A switch port is similar to a hostbridge port except
> register access is defined in the CXL specification in a platform
> agnostic way. The downstream ports for a switch are simply known as
> downstream ports. The enumeration of these are entirely contained
> within cxl_port.
In PCIe, "Downstream Port" includes both Root Ports and Switch
Downstream Ports. It sounds like that's not the case for CXL?
Well, except above you say that a Host Bridge Port has N Downstream
Ports, so I guess "Downstream Port" probably includes both Host Bridge
Ports and Switch Downstream Ports.
Maybe you should just omit the "The downstream ports for a switch are
simply known as downstream ports" sentence.
> 4. Endpoint port. Endpoint ports are similar to switch ports with the
> exception that they have no downstream ports, only the underlying
> media on the device. The enumeration of these are started by cxl_pci,
> and completed by cxl_port.
Does CXL use an "Upstream Port" concept similar to PCIe? In PCIe,
"Upstream Port" includes both Switch Upstream Ports and the Upstream
Port on an Endpoint.
I hope this driver is not modeled on the PCI portdrv. IMO, that was a
design error, and the "port service drivers" (PME, hotplug, AER, etc)
should be directly integrated into the PCI core instead of pretending
to be independent drivers.
> --- a/Documentation/driver-api/cxl/memory-devices.rst
> +++ b/Documentation/driver-api/cxl/memory-devices.rst
> @@ -28,6 +28,11 @@ CXL Memory Device
> .. kernel-doc:: drivers/cxl/pci.c
> :internal:
>
> +CXL Port
> +--------
> +.. kernel-doc:: drivers/cxl/port.c
> + :doc: cxl port
s/cxl port/CXL Port/ ? I don't know exactly how this gets rendered by
ReST.
> CXL Core
> --------
> .. kernel-doc:: drivers/cxl/cxl.h
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index ef05e96f8f97..3aeb33bba5a3 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -77,4 +77,26 @@ config CXL_PMEM
> provisioning the persistent memory capacity of CXL memory expanders.
>
> If unsure say 'm'.
> +
> +config CXL_MEM
> + tristate "CXL.mem: Memory Devices"
> + select CXL_PORT
> + depends on CXL_PCI
> + default CXL_BUS
> + help
> + The CXL.mem protocol allows a device to act as a provider of "System
> + RAM" and/or "Persistent Memory" that is fully coherent as if the
> + memory was attached to the typical CPU memory controller. This is
> + known as HDM "Host-managed Device Memory".
s/was attached/were attached/
> + Say 'y/m' to enable a driver that will attach to CXL.mem devices for
> + memory expansion and control of HDM. See Chapter 9.13 in the CXL 2.0
> + specification for a detailed description of HDM.
> +
> + If unsure say 'm'.
> +
> +
Spurious blank line.
> +config CXL_PORT
> + tristate
> +
> endif
> --- /dev/null
> +++ b/drivers/cxl/port.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include "cxlmem.h"
> +
> +/**
> + * DOC: cxl port
s/cxl port/CXL port/ (I'm assuming this should match usage below)
or maybe "CXL Port" both places to match typical PCIe spec usage?
Capitalization is a useful hint that this term is defined by a spec.
> + *
> + * The port driver implements the set of functionality needed to allow full
> + * decoder enumeration and routing. A CXL port is an abstraction of a CXL
> + * component that implements some amount of CXL decoding of CXL.mem traffic.
> + * As of the CXL 2.0 spec, this includes:
> + *
> + * .. list-table:: CXL Components w/ Ports
> + * :widths: 25 25 50
> + * :header-rows: 1
> + *
> + * * - component
> + * - upstream
> + * - downstream
> + * * - Hostbridge
s/Hostbridge/Host bridge/
> + * - ACPI0016
> + * - root port
s/root port/Root Port/ to match Switch Ports below (and spec usage).
> + * * - Switch
> + * - Switch Upstream Port
> + * - Switch Downstream Port
> + * * - Endpoint
> + * - Endpoint Port
> + * - N/A
What does "N/A" mean here? Is it telling us something useful?
> +static void rescan_ports(struct work_struct *work)
> +{
> + if (bus_rescan_devices(&cxl_bus_type))
> + pr_warn("Failed to rescan\n");
Needs some context. A bare "Failed to rescan" in the dmesg log
without a clue about who emitted it is really annoying.
Maybe you defined pr_fmt() somewhere; I couldn't find it easily.
Bjorn
