Re: [PATCH 08/19] cxl/port: Enumerate flit mode capability

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Sun, 04 Jun 2023 16:32:21 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Per CXL 3.0 Section 9.14 Back-Invalidation Configuration, in order to
> > enable an HDM-DB range (CXL.mem region with device initiated
> > back-invalidation support), all ports in the path between the endpoint and
> > the host bridge must be in 256-bit flit-mode.
> > 
> > Even for typical Type-3 class devices it is useful to enumerate link
> > capabilities through the chain for debug purposes.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> A few minor comments. In particularly that the field you have in here doesn't
> distinguish between 256 byte flits and otherwise.  That's done with the PCI spec
> field not this one which is about latency optimization.
> 
> > ---
> >  drivers/cxl/core/hdm.c  |    2 +
> >  drivers/cxl/core/pci.c  |   84 +++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/core/port.c |    6 +++
> >  drivers/cxl/cxl.h       |    2 +
> >  drivers/cxl/cxlpci.h    |   25 +++++++++++++-
> >  drivers/cxl/port.c      |    5 +++
> >  6 files changed, 122 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index ca3b99c6eacf..91ab3033c781 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -3,8 +3,10 @@
> >  #include <linux/seq_file.h>
> >  #include <linux/device.h>
> >  #include <linux/delay.h>
> > +#include <linux/pci.h>
> >  
> >  #include "cxlmem.h"
> > +#include "cxlpci.h"
> >  #include "core.h"
> I'm not following why link related patch should change includes in hdm relate c file?
> Maybe later once you use it this makes sense?

Definitely. I missed that this straggled in here.

> 
> 
> >  
> >  /**
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 67f4ab6daa34..b62ec17ccdde 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -519,6 +519,90 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> 
> > +
> > +int cxl_probe_link(struct cxl_port *port)
> > +{
> > +	struct pci_dev *pdev = cxl_port_to_pci(port);
> > +	u16 cap, en, parent_features;
> > +	struct cxl_port *parent_port;
> > +	struct device *dev;
> > +	int rc, dvsec;
> > +	u32 hdr;
> > +
> > +	if (!pdev) {
> > +		/*
> > +		 * Assume host bridges support all features, the root
> > +		 * port will dictate the actual enabled set to endpoints.
> > +		 */
> > +		return 0;
> > +	}
> > +
> > +	dev = &pdev->dev;
> > +	dvsec = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> > +					  CXL_DVSEC_FLEXBUS_PORT);
> > +	if (!dvsec) {
> > +		dev_err(dev, "Failed to enumerate port capabilities\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	/*
> > +	 * Cache the link features for future determination of HDM-D or
> > +	 * HDM-DB support
> > +	 */
> > +	rc = pci_read_config_dword(pdev, dvsec + PCI_DVSEC_HEADER1, &hdr);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = pci_read_config_word(pdev, dvsec + CXL_DVSEC_FLEXBUS_CAP_OFFSET,
> > +				  &cap);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = pci_read_config_word(pdev, dvsec + CXL_DVSEC_FLEXBUS_STATUS_OFFSET,
> > +				  &en);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (PCI_DVSEC_HEADER1_REV(hdr) < 2)
> > +		cap &= ~CXL_DVSEC_FLEXBUS_REV2_MASK;
> > +
> > +	if (PCI_DVSEC_HEADER1_REV(hdr) < 1)
> > +		cap &= ~CXL_DVSEC_FLEXBUS_REV1_MASK;
> 
> I talk about this below, but I'd not normally expect to see this.
> Anyone who used those bits out of usage defined by later specs has buggy 
> hardware and should quirk it rather than having it built in here.

True, makes sense.

> 
> > +
> > +	en &= cap;
> > +	parent_port = to_cxl_port(port->dev.parent);
> > +	parent_features = parent_port->features;
> > +
> > +	/* Enforce port features are plumbed through to the host bridge */
> > +	port->features = en & CXL_DVSEC_FLEXBUS_ENABLE_MASK & parent_features;
> > +
> > +	dev_dbg(dev, "features:%s%s%s%s%s%s%s\n",
> > +		en & CXL_DVSEC_FLEXBUS_CACHE_ENABLED ? " cache" : "",
> > +		en & CXL_DVSEC_FLEXBUS_IO_ENABLED ? " io" : "",
> > +		en & CXL_DVSEC_FLEXBUS_MEM_ENABLED ? " mem" : "",
> > +		en & CXL_DVSEC_FLEXBUS_FLIT68_ENABLED ? " flit68" : "",
> > +		en & CXL_DVSEC_FLEXBUS_MLD_ENABLED ? " mld" : "",
> > +		en & CXL_DVSEC_FLEXBUS_FLIT256_ENABLED ? " flit256" : "",
> 
> Definitely want that text to be more explicit about latency optimized

Ok, see below, I think dropping flit size altogether from these names
makes sense.

> 
> > +		en & CXL_DVSEC_FLEXBUS_PBR_ENABLED ? " pbr" : "");
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_probe_link, CXL);
> > +
> >  #define CXL_DOE_TABLE_ACCESS_REQ_CODE		0x000000ff
> >  #define   CXL_DOE_TABLE_ACCESS_REQ_CODE_READ	0
> >  #define CXL_DOE_TABLE_ACCESS_TABLE_TYPE		0x0000ff00
> 
> 
> > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> > index 7c02e55b8042..7f82ffb5b4be 100644
> > --- a/drivers/cxl/cxlpci.h
> > +++ b/drivers/cxl/cxlpci.h
> > @@ -45,8 +45,28 @@
> >  /* CXL 2.0 8.1.7: GPF DVSEC for CXL Device */
> >  #define CXL_DVSEC_DEVICE_GPF					5
> >  
> > -/* CXL 2.0 8.1.8: PCIe DVSEC for Flex Bus Port */
> > -#define CXL_DVSEC_PCIE_FLEXBUS_PORT				7
> > +/* CXL 3.0 8.2.1.3: PCIe DVSEC for Flex Bus Port */
> > +#define CXL_DVSEC_FLEXBUS_PORT					7
> > +#define   CXL_DVSEC_FLEXBUS_CAP_OFFSET		0xA
> > +#define     CXL_DVSEC_FLEXBUS_CACHE_CAPABLE	BIT(0)
> > +#define     CXL_DVSEC_FLEXBUS_IO_CAPABLE	BIT(1)
> > +#define     CXL_DVSEC_FLEXBUS_MEM_CAPABLE	BIT(2)
> > +#define     CXL_DVSEC_FLEXBUS_FLIT68_CAPABLE	BIT(5)
> 
> This one includes the stuff that makes it 2.0 rather than 1.1  Might need a longer
> name to avoid miss use? (I checked the 1.1 spec and reserved so would be 0).

So maybe I will drop the flit size from the name until a conflict
arises. I.e. this bit is relevant for all known flit sizes that support
VH topologies.

> 
> > +#define     CXL_DVSEC_FLEXBUS_MLD_CAPABLE	BIT(6)
> > +#define     CXL_DVSEC_FLEXBUS_REV1_MASK		GENMASK(6, 5)
> 
> Unusual approach.. Shouldn't be needed as those bits were RsvdP so
> no one should have set them and now we are supporting the new bits
> so should be good without masking.

Agree.

> 
> > +#define     CXL_DVSEC_FLEXBUS_FLIT256_CAPABLE	BIT(13)
> 
> Not just flit256, but the latency optimized one (split in two kind of
> with separate CRCs)  So this name needs to be something like
> FLEXBUS_LAT_OPT_FLIT256_CAPABLE

Until a non-flit256 latency optimized mechanism is added the flit size
is redundant in this name.

> 
> 
> > +#define     CXL_DVSEC_FLEXBUS_PBR_CAPABLE	BIT(14)
> > +#define     CXL_DVSEC_FLEXBUS_REV2_MASK		GENMASK(14, 13)
> > +#define   CXL_DVSEC_FLEXBUS_STATUS_OFFSET	0xE
> > +#define     CXL_DVSEC_FLEXBUS_CACHE_ENABLED	BIT(0)
> > +#define     CXL_DVSEC_FLEXBUS_IO_ENABLED	BIT(1)
> > +#define     CXL_DVSEC_FLEXBUS_MEM_ENABLED	BIT(2)
> > +#define     CXL_DVSEC_FLEXBUS_FLIT68_ENABLED	BIT(5)
> 
> Again, not just FLIT68, but the VH stuff from CXL 2.0 as well.
> 
> > +#define     CXL_DVSEC_FLEXBUS_MLD_ENABLED	BIT(6)
> > +#define     CXL_DVSEC_FLEXBUS_FLIT256_ENABLED	BIT(13)
> Also latency optimized is key here, not 256 bit (though you need
> that as well).
> 
> > +#define     CXL_DVSEC_FLEXBUS_PBR_ENABLED	BIT(14)
> > +#define     CXL_DVSEC_FLEXBUS_ENABLE_MASK \
> > +	(GENMASK(2, 0) | GENMASK(6, 5) | GENMASK(14, 13))
> Ok - I guess the resvP requires this dance.
> >  
>