... > > > > +MODULE_IMPORT_NS(CXL); > > > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > > > new file mode 100644 > > > > index 000000000000..beb03921e6da > > > > --- /dev/null > > > > +++ b/drivers/cxl/pci.h > > > > @@ -0,0 +1,15 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > +// Copyright(c) 2020 Intel Corporation. All rights reserved. > > > > +#ifndef __CXL_PCI_H__ > > > > +#define __CXL_PCI_H__ > > > > + > > > > +#define PCI_CLASS_MEMORY_CXL 0x050210 > > > > + > > > > +#define PCI_EXT_CAP_ID_DVSEC 0x23 > > > > +#define PCI_DVSEC_VENDOR_CXL 0x1E98 > > > > > > Hmm. The magic question of what to call a vendor ID that isn't a vendor > > > ID but just a magic number that talks like a duck and quacks like a duck > > > (for anyone wondering what I'm talking about, there is a nice bit of legal > > > boilerplate on this in the CXL spec) > > > > > > This name is definitely not accurate however. > > > > > > PCI_UNIQUE_VALUE_CXL maybe? It is used for other things than DVSEC (VDMs etc), > > > though possibly this is the only software visible use. > > > > Finally working my way back through this review to make the changes. > > If 0x1E98 becomes visible to software somewhere else then this can > > become something like the following: > > > > #define PCI_DVSEC_VENDOR_CXL PCI_UNIQUE_VALUE_CXL > > > > ...or whatever the generic name is, but this field per the > > specification is the DVSEC-vendor-id and calling it > > PCI_UNIQUE_VALUE_CXL does not have any basis in the spec. There is a big statement about it as a footnote to 3.1.2 in CXL 2.0 "The Unique Value that is provided in this specification for use in ... Designated Vendor Specific Extended Capabilities.." And for extra amusement in the "Notice Regarding PCI-SIG Unique Value" that forms part of the click through https://www.computeexpresslink.org/download-the-specification (that's the only use of "PCI-SIG Unique Value" that Google finds but I know of one other similar statement) However, I agree it's being used in DVSEC field only (from software point of view) so fair enough to name it after where it is used rather than what it is. > > > > I will rename it though to: > > > > PCI_DVSEC_VENDOR_ID_CXL > > > > ...since include/linux/pci_ids.h includes the _ID_ part. > > > > > > > > > > > > +#define PCI_DVSEC_VENDOR_OFFSET 0x4 > > > > +#define PCI_DVSEC_ID_OFFSET 0x8 > > > > > > Put a line break here perhaps and maybe a spec reference to where to find > > > the various DVSEC IDs. > > > > Ok. > > > > > > > > > +#define PCI_DVSEC_ID_CXL 0x0 > > > > > > That's definitely a confusing name as well. > > > > Yeah, should be PCI_DVSEC_DEVICE_ID_CXL > > Actually, no, the spec calls this the "DVSEC id" so PCI_DVSEC_ID_CXL > seems suitable to me. This is from: > > Table 126. PCI Express DVSEC Register Settings for CXL Device > > In the CXL 2.0 Specification. The DVSEC ID naming is straight from the PCI spec so that part is fine, my issue is this is one of a whole bunch of CXL related DVSEC ID so it needs a more specific name. PCI_DVSEC_ID_CXL_DEVICE would work in line with table 124. I'm not that bothered though. Jonathan
