Re: [PATCH 20/23] cxl/port: Introduce a port driver

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Nov 23, 2021 at 02:03:15PM -0800, Ben Widawsky wrote:
> On 21-11-23 12:21:28, Bjorn Helgaas wrote:
> > On Fri, Nov 19, 2021 at 04:02:47PM -0800, Ben Widawsky wrote:

> > > 2. Hostbridge port. 
> > > ...
> > >    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.
> 
> What sounds better? A CXL 2.0 Root Port is CXL capabilities on top
> of the PCIe component which has the PCI_EXP_TYPE_ROOT_PORT cap. In
> my mental model, a host bridge does contain the root ports. Perhaps
> I am wrong about that?

"A host bridge contains the root ports" makes sense to me.
"A host bridge *port* contains root ports" does not.

The PCIe spec would describe this as a "Root Complex may support one
or more [Root] Ports" (see PCIe r5.0, sec 1.3.1).

In PCIe, a "Port" is "an interface between a component and a PCI
Express Link."  It doesn't contain other Ports.

Sounds like CXL is the same here, and using the same terminology
(assuming that's what the CXL spec does) will reduce confusion.

> > > 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?
> 
> In CXL 2.0, it's fairly close to true that switch downstream ports
> and root ports are equivalent. From today's driver perspective they
> are equivalent. Root ports have some capabilities which switch
> downstream ports do not.

Same as PCIe.

> > > 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.
> 
> Not really. Endpoints aren't known as ports in the spec and they
> have a decent amount of divergence from upstream ports. The main
> area where they do converge is in the memory decoding capabilities.
> In fact, it might come to a point where we find adding an endpoint
> as a port does not make sense, but for now it does.

Since a PCIe "Port" refers to the interface between a component and a
Link, PCIe Endpoints have Upstream Ports just like switches do.  I'm
guessing CXL is the same.

The part about "endpoint ports have no downstream ports" is what
doesn't read well to me because ports don't have other ports.

This section is about the four types of ports in a system.  I'm
trying to match those up with spec terms, either PCIe or CXL.  It
sounds like you intend bullet 3 to include both Switch Upstream Ports
and Switch Downstream Ports, and bullet 4 to be only Endpoint Ports
(which are Upstream Ports).

> > 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.
> 
> I'd like to understand a bit about why you think it was a design
> error. The port driver is intended to be a port services driver,
> however I see the services provided as quite different than the ones
> you mention. The primary service cxl_port will provide from here on
> out is the ability to manage HDM decoder resources for a port. Other
> independent drivers that want to use HDM decoder resources would
> rely on the port driver for this.

I'll continue this part in the later part of the thread.

> > > + *		* - Switch
> > > + *		  - Switch Upstream Port
> > > + *		  - Switch Downstream Port
> > > + *		* - Endpoint
> > > + *		  - Endpoint Port
> > > + *		  - N/A
> > 
> > What does "N/A" mean here?  Is it telling us something useful?
> 
> This gets rendered into a table that looks like the following:
> 
> | component  | upstream             | downstream             |
> | ---------  | --------             | ----------             |
> | Hostbridge | ACPI0016             | Root Port              |
> | Switch     | Switch Upstream Port | Switch Downstream Port |
> | Endpoint   | Endpoint Port        | N/A                    |

Makes sense, thanks.  I didn't know how to read the ReST and thought
this was just a list, where N/A wouldn't make much sense.

Bjorn