My response inline.
Regards
Ashraf
> -----Original Message-----
> From: Gao, Liming <liming....@intel.com>
> Sent: Monday, July 27, 2020 8:25 PM
> To: devel@edk2.groups.io; l...@nuviainc.com; Javeed, Ashraf
> <ashraf.jav...@intel.com>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> <liming....@intel.com>
> Subject: RE: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard:
> CXL 1.1 Registers
>
> Lefi:
> Thanks for your comments.
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif
> > Lindholm
> > Sent: Monday, July 27, 2020 10:25 PM
> > To: devel@edk2.groups.io; Javeed, Ashraf <ashraf.jav...@intel.com>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> > <liming....@intel.com>
> > Subject: Re: [edk2-devel] [PATCH V4 1/2]
> > MdePkg/Include/IndustryStandard: CXL 1.1 Registers
> >
> > Hi Ashraf, but also Mike, Liming.
> >
> > I just saw this patch go in and have some comments.
> >
> > On Fri, Jul 24, 2020 at 23:56:12 +0530, Javeed, Ashraf wrote:
> > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2611
> > >
> > > Register definitions from chapter 7 of Compute Express Link
> > > Specification Revision 1.1 are ported into the new Cxl11.h.
> > > The CXL Flex Bus registers are based on the PCIe Extended Capability
> > > DVSEC structure header, led to the inclusion of upgraded Pci.h.
> > >
> > > Signed-off-by: Ashraf Javeed <ashraf.jav...@intel.com>
> > > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > > Cc: Liming Gao <liming....@intel.com>
> > > --
> > >
> > > V4: fix code style
> > >
> > > V3: Copyright date fix
> > >
> > > V2: Indentation and double declaration fix, copyright date update
> > > ---
> > > MdePkg/Include/IndustryStandard/Cxl11.h | 569
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++
> > +++++++++
> > > MdePkg/Include/IndustryStandard/Pci.h | 6 ++----
> > > 2 files changed, 571 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/MdePkg/Include/IndustryStandard/Cxl11.h
> > > b/MdePkg/Include/IndustryStandard/Cxl11.h
> > > new file mode 100644
> > > index 0000000000..933c1ab817
> > > --- /dev/null
> > > +++ b/MdePkg/Include/IndustryStandard/Cxl11.h
> > > @@ -0,0 +1,569 @@
> > > +/** @file
> > > + CXL 1.1 Register definitions
> > > +
> > > + This file contains the register definitions based on the Compute
> > > + Express Link
> > > + (CXL) Specification Revision 1.1.
> > > +
> > > +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> > > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#ifndef _CXL11_H_
> > > +#define _CXL11_H_
> >
> > We should not be adding macros with a leading _ - these are intended
> > for toolchain use.
>
> This style is align to other header file. This is the file header macro to
> make sure
> the header file be included more than once.
>
> >
> > > +
> > > +#include <IndustryStandard/Pci.h>
> > > +//
> > > +// DVSEC Vendor ID
> > > +// Compute Express Link Specification Revision: 1.1 - Chapter 7.1.1
> > > +- Table 58 // (subject to change as per CXL assigned Vendor ID) //
> > > +#define INTEL_CXL_DVSEC_VENDOR_ID
> > > 0x8086
> >
> > This is Cxl11.h - surely this should be CXL_DVSEC_VENDOR_ID_INTEL?
>
> Ashraf: is it defined in spec?
>
Yes, the Intel Vendor ID is defined in CXL 1.1 Specification, section 7.1.1.
> >
> > > +
> > > +//
> > > +// CXL Flex Bus Device default device and function number //
> > > +Compute Express Link Specification Revision: 1.1 - Chapter 7.1.1 //
> > > +#define CXL_DEV_DEV 0
> > > +#define CXL_DEV_FUNC 0
> > > +
> > > +//
> > > +// Ensure proper structure formats
> > > +//
> > > +#pragma pack(1)
> >
> > And this pragma has no function whatsoever with regards to any of the
> > register definition structs below. It would be much better if the
> > structs requiring packing (_DEVICE, _PORT, ...) were grouped together
> > and only those were given this treatment.
> >
> > #pragma pack(1) is *not* a safe default.
> >
>
> I know pack(1) is for the compact structure layout.
>
> > > +
> > > +///
> > > +/// The PCIe DVSEC for Flex Bus Device ///@{ typedef union {
> > > + struct {
> > > + UINT16 CacheCapable : 1; //
> > > bit 0
> > > + UINT16 IoCapable : 1; //
> > > bit 1
> > > + UINT16 MemCapable : 1; //
> > > bit 2
> > > + UINT16 MemHwInitMode : 1; //
> > > bit 3
> > > + UINT16 HdmCount : 2; //
> > > bit 4..5
> > > + UINT16 Reserved1 : 8; //
> > > bit 6..13
> > > + UINT16 ViralCapable : 1; //
> > > bit 14
> > > + UINT16 Reserved2 : 1; //
> > > bit 15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_CAPABILITY;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 CacheEnable : 1; //
> > > bit 0
> > > + UINT16 IoEnable : 1; //
> > > bit 1
> > > + UINT16 MemEnable : 1; //
> > > bit 2
> > > + UINT16 CacheSfCoverage : 5; //
> > > bit 3..7
> > > + UINT16 CacheSfGranularity : 3; //
> > > bit 8..10
> > > + UINT16 CacheCleanEviction : 1; //
> > > bit 11
> > > + UINT16 Reserved1 : 2; //
> > > bit 12..13
> > > + UINT16 ViralEnable : 1; //
> > > bit 14
> > > + UINT16 Reserved2 : 1; //
> > > bit 15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_CONTROL;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 Reserved1 : 14; //
> > > bit 0..13
> > > + UINT16 ViralStatus : 1; //
> > > bit 14
> > > + UINT16 Reserved2 : 1; //
> > > bit 15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_STATUS;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 Reserved1 : 1; //
> > > bit 0
> > > + UINT16 Reserved2 : 1; //
> > > bit 1
> > > + UINT16 Reserved3 : 1; //
> > > bit 2
> > > + UINT16 Reserved4 : 13; //
> > > bit 3..15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_1_1_DVSEC_FLEX_BUS_DEVICE_CONTROL2;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 Reserved1 : 1; //
> > > bit 0
> > > + UINT16 Reserved2 : 1; //
> > > bit 1
> > > + UINT16 Reserved3 : 14; //
> > > bit 2..15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_1_1_DVSEC_FLEX_BUS_DEVICE_STATUS2;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 ConfigLock : 1; //
> > > bit 0
> > > + UINT16 Reserved1 : 15; //
> > > bit 1..15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_LOCK;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 MemorySizeHigh : 32; //
> > > bit 0..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_SIZE_HIGH;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 MemoryInfoValid : 1; //
> > > bit 0
> > > + UINT32 MemoryActive : 1; //
> > > bit 1
> > > + UINT32 MediaType : 3; //
> > > bit 2..4
> > > + UINT32 MemoryClass : 3; //
> > > bit 5..7
> > > + UINT32 DesiredInterleave : 3; //
> > > bit 8..10
> > > + UINT32 Reserved : 17; //
> > > bit 11..27
> > > + UINT32 MemorySizeLow : 4; //
> > > bit 28..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_SIZE_LOW;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 MemoryBaseHigh : 32; //
> > > bit 0..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_BASE_HIGH;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 Reserved : 28; //
> > > bit 0..27
> > > + UINT32 MemoryBaseLow : 4; //
> > > bit 28..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_BASE_LOW;
> > > +
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 MemorySizeHigh : 32; //
> > > bit 0..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_SIZE_HIGH;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 MemoryInfoValid : 1; //
> > > bit 0
> > > + UINT32 MemoryActive : 1; //
> > > bit 1
> > > + UINT32 MediaType : 3; //
> > > bit 2..4
> > > + UINT32 MemoryClass : 3; //
> > > bit 5..7
> > > + UINT32 DesiredInterleave : 3; //
> > > bit 8..10
> > > + UINT32 Reserved : 17; //
> > > bit 11..27
> > > + UINT32 MemorySizeLow : 4; //
> > > bit 28..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_SIZE_LOW;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 MemoryBaseHigh : 32; //
> > > bit 0..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_BASE_HIGH;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 Reserved : 28; //
> > > bit 0..27
> > > + UINT32 MemoryBaseLow : 4; //
> > > bit 28..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_BASE_LOW;
> > > +
> > > +//
> > > +// Flex Bus Device DVSEC ID
> > > +// Compute Express Link Specification Revision: 1.1 - Chapter
> > > +7.1.1, Table 58 //
> > > +#define FLEX_BUS_DEVICE_DVSEC_ID 0
> > > +
> > > +//
> > > +// PCIe DVSEC for Flex Bus Device
> > > +// Compute Express Link Specification Revision: 1.1 - Chapter
> > > +7.1.1, Figure 95 // typedef struct {
> > > + PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER Header;
> // offset 0
> > > + PCI_EXPRESS_DESIGNATED_VENDOR_SPECIFIC_HEADER_1
> DesignatedVendorSpecificHeader1; // offset 4
> > > + PCI_EXPRESS_DESIGNATED_VENDOR_SPECIFIC_HEADER_2
> DesignatedVendorSpecificHeader2; // offset 8
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_CAPABILITY
> DeviceCapability; // offset 10
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_CONTROL
> > > DeviceControl;
> // offset 12
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_STATUS
> > > DeviceStatus;
> // offset 14
> > > + CXL_1_1_DVSEC_FLEX_BUS_DEVICE_CONTROL2
> DeviceControl2; // offset 16
> > > + CXL_1_1_DVSEC_FLEX_BUS_DEVICE_STATUS2
> DeviceStatus2; // offset 18
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_LOCK
> > > DeviceLock;
> // offset 20
> > > + UINT16
> > > Reserved; // offset 22
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_SIZE_HIGH
> DeviceRange1SizeHigh; // offset 24
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_SIZE_LOW
> DeviceRange1SizeLow; // offset 28
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_BASE_HIGH
> DeviceRange1BaseHigh; // offset 32
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE1_BASE_LOW
> DeviceRange1BaseLow; // offset 36
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_SIZE_HIGH
> DeviceRange2SizeHigh; // offset 40
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_SIZE_LOW
> DeviceRange2SizeLow; // offset 44
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_BASE_HIGH
> DeviceRange2BaseHigh; // offset 48
> > > + CXL_DVSEC_FLEX_BUS_DEVICE_RANGE2_BASE_LOW
> DeviceRange2BaseLow; // offset 52
> > > +} CXL_1_1_DVSEC_FLEX_BUS_DEVICE;
> > > +///@}
> > > +
> > > +///
> > > +/// PCIe DVSEC for FLex Bus Port
> > > +///@{
> > > +typedef union {
> > > + struct {
> > > + UINT16 CacheCapable : 1; //
> > > bit 0
> > > + UINT16 IoCapable : 1; //
> > > bit 1
> > > + UINT16 MemCapable : 1; //
> > > bit 2
> > > + UINT16 Reserved : 13; //
> > > bit 3..15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_1_1_DVSEC_FLEX_BUS_PORT_CAPABILITY;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 CacheEnable : 1; //
> > > bit 0
> > > + UINT16 IoEnable : 1; //
> > > bit 1
> > > + UINT16 MemEnable : 1; //
> > > bit 2
> > > + UINT16 CxlSyncBypassEnable : 1; //
> > > bit 3
> > > + UINT16 DriftBufferEnable : 1; //
> > > bit 4
> > > + UINT16 Reserved : 3; //
> > > bit 5..7
> > > + UINT16 Retimer1Present : 1; //
> > > bit 8
> > > + UINT16 Retimer2Present : 1; //
> > > bit 9
> > > + UINT16 Reserved2 : 6; //
> > > bit 10..15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_1_1_DVSEC_FLEX_BUS_PORT_CONTROL;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 CacheEnable : 1; //
> > > bit 0
> > > + UINT16 IoEnable : 1; //
> > > bit 1
> > > + UINT16 MemEnable : 1; //
> > > bit 2
> > > + UINT16 CxlSyncBypassEnable : 1; //
> > > bit 3
> > > + UINT16 DriftBufferEnable : 1; //
> > > bit 4
> > > + UINT16 Reserved : 3; //
> > > bit 5..7
> > > + UINT16 CxlCorrectableProtocolIdFramingError : 1; //
> > > bit 8
> > > + UINT16 CxlUncorrectableProtocolIdFramingError : 1; //
> > > bit 9
> > > + UINT16 CxlUnexpectedProtocolIdDropped : 1; //
> > > bit 10
> > > + UINT16 Reserved2 : 5; //
> > > bit 11..15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_1_1_DVSEC_FLEX_BUS_PORT_STATUS;
> > > +
> > > +//
> > > +// Flex Bus Port DVSEC ID
> > > +// Compute Express Link Specification Revision: 1.1 - Chapter
> > > +7.2.1.3, Table 62 //
> > > +#define FLEX_BUS_PORT_DVSEC_ID 7
> > > +
> > > +//
> > > +// PCIe DVSEC for Flex Bus Port
> > > +// Compute Express Link Specification Revision: 1.1 - Chapter
> > > +7.2.1.3, Figure 99 // typedef struct {
> > > + PCI_EXPRESS_EXTENDED_CAPABILITIES_HEADER Header;
> // offset 0
> > > + PCI_EXPRESS_DESIGNATED_VENDOR_SPECIFIC_HEADER_1
> DesignatedVendorSpecificHeader1; // offset 4
> > > + PCI_EXPRESS_DESIGNATED_VENDOR_SPECIFIC_HEADER_2
> DesignatedVendorSpecificHeader2; // offset 8
> > > + CXL_1_1_DVSEC_FLEX_BUS_PORT_CAPABILITY
> PortCapability; // offset 10
> > > + CXL_1_1_DVSEC_FLEX_BUS_PORT_CONTROL
> > > PortControl;
> // offset 12
> > > + CXL_1_1_DVSEC_FLEX_BUS_PORT_STATUS
> > > PortStatus;
> // offset 14
> > > +} CXL_1_1_DVSEC_FLEX_BUS_PORT;
> > > +///@}
> > > +
> > > +///
> > > +/// CXL 1.1 Upstream and Downstream Port Subsystem Component
> > > +registers ///
> > > +
> > > +/// The CXL.Cache and CXL.Memory Architectural register definitions
> > > +/// Based on chapter 7.2.2 of Compute Express Link Specification
> > > +Revision: 1.1 ///@{
> > > +
> > > +#define CXL_CAPABILITY_HEADER_OFFSET 0
> > > +typedef union {
> > > + struct {
> > > + UINT32 CxlCapabilityId : 16; //
> > > bit 0..15
> > > + UINT32 CxlCapabilityVersion : 4; //
> > > bit 16..19
> > > + UINT32 CxlCacheMemVersion : 4; //
> > > bit 20..23
> > > + UINT32 ArraySize : 8; //
> > > bit 24..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_CAPABILITY_HEADER;
> > > +
> > > +#define CXL_RAS_CAPABILITY_HEADER_OFFSET 4
> > > +typedef union {
> > > + struct {
> > > + UINT32 CxlCapabilityId : 16; //
> > > bit 0..15
> > > + UINT32 CxlCapabilityVersion : 4; //
> > > bit 16..19
> > > + UINT32 CxlRasCapabilityPointer : 12; //
> > > bit 20..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_RAS_CAPABILITY_HEADER;
> > > +
> > > +#define CXL_SECURITY_CAPABILITY_HEADER_OFFSET 8
> > > +typedef union {
> > > + struct {
> > > + UINT32 CxlCapabilityId : 16; //
> > > bit 0..15
> > > + UINT32 CxlCapabilityVersion : 4; //
> > > bit 16..19
> > > + UINT32 CxlSecurityCapabilityPointer : 12; //
> > > bit 20..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_SECURITY_CAPABILITY_HEADER;
> > > +
> > > +#define CXL_LINK_CAPABILITY_HEADER_OFFSET 0xC
> > > +typedef union {
> > > + struct {
> > > + UINT32 CxlCapabilityId : 16; //
> > > bit 0..15
> > > + UINT32 CxlCapabilityVersion : 4; //
> > > bit 16..19
> > > + UINT32 CxlLinkCapabilityPointer : 12; //
> > > bit 20..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_LINK_CAPABILITY_HEADER;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 CacheDataParity : 1; //
> > > bit 0..0
> > > + UINT32 CacheAddressParity : 1; //
> > > bit 1..1
> > > + UINT32 CacheByteEnableParity : 1; //
> > > bit 2..2
> > > + UINT32 CacheDataEcc : 1; //
> > > bit 3..3
> > > + UINT32 MemDataParity : 1; //
> > > bit 4..4
> > > + UINT32 MemAddressParity : 1; //
> > > bit 5..5
> > > + UINT32 MemByteEnableParity : 1; //
> > > bit 6..6
> > > + UINT32 MemDataEcc : 1; //
> > > bit 7..7
> > > + UINT32 ReInitThreshold : 1; //
> > > bit 8..8
> > > + UINT32 RsvdEncodingViolation : 1; //
> > > bit 9..9
> > > + UINT32 PoisonReceived : 1; //
> > > bit 10..10
> > > + UINT32 ReceiverOverflow : 1; //
> > > bit 11..11
> > > + UINT32 Reserved : 20; //
> > > bit 12..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_1_1_UNCORRECTABLE_ERROR_STATUS;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 CacheDataParityMask : 1; //
> > > bit 0..0
> > > + UINT32 CacheAddressParityMask : 1; //
> > > bit 1..1
> > > + UINT32 CacheByteEnableParityMask : 1; //
> > > bit 2..2
> > > + UINT32 CacheDataEccMask : 1; //
> > > bit 3..3
> > > + UINT32 MemDataParityMask : 1; //
> > > bit 4..4
> > > + UINT32 MemAddressParityMask : 1; //
> > > bit 5..5
> > > + UINT32 MemByteEnableParityMask : 1; //
> > > bit 6..6
> > > + UINT32 MemDataEccMask : 1; //
> > > bit 7..7
> > > + UINT32 ReInitThresholdMask : 1; //
> > > bit 8..8
> > > + UINT32 RsvdEncodingViolationMask : 1; //
> > > bit 9..9
> > > + UINT32 PoisonReceivedMask : 1; //
> > > bit 10..10
> > > + UINT32 ReceiverOverflowMask : 1; //
> > > bit 11..11
> > > + UINT32 Reserved : 20; //
> > > bit 12..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_1_1_UNCORRECTABLE_ERROR_MASK;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 CacheDataParitySeverity : 1; //
> > > bit 0..0
> > > + UINT32 CacheAddressParitySeverity : 1; //
> > > bit 1..1
> > > + UINT32 CacheByteEnableParitySeverity : 1; //
> > > bit 2..2
> > > + UINT32 CacheDataEccSeverity : 1; //
> > > bit 3..3
> > > + UINT32 MemDataParitySeverity : 1; //
> > > bit 4..4
> > > + UINT32 MemAddressParitySeverity : 1; //
> > > bit 5..5
> > > + UINT32 MemByteEnableParitySeverity : 1; //
> > > bit 6..6
> > > + UINT32 MemDataEccSeverity : 1; //
> > > bit 7..7
> > > + UINT32 ReInitThresholdSeverity : 1; //
> > > bit 8..8
> > > + UINT32 RsvdEncodingViolationSeverity : 1; //
> > > bit 9..9
> > > + UINT32 PoisonReceivedSeverity : 1; //
> > > bit 10..10
> > > + UINT32 ReceiverOverflowSeverity : 1; //
> > > bit 11..11
> > > + UINT32 Reserved : 20; //
> > > bit 12..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_1_1_UNCORRECTABLE_ERROR_SEVERITY;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 CacheDataEcc : 1; //
> > > bit 0..0
> > > + UINT32 MemoryDataEcc : 1; //
> > > bit 1..1
> > > + UINT32 CrcThreshold : 1; //
> > > bit 2..2
> > > + UINT32 RetryThreshold : 1; //
> > > bit 3..3
> > > + UINT32 CachePoisonReceived : 1; //
> > > bit 4..4
> > > + UINT32 MemoryPoisonReceived : 1; //
> > > bit 5..5
> > > + UINT32 PhysicalLayerError : 1; //
> > > bit 6..6
> > > + UINT32 Reserved : 25; //
> > > bit 7..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_CORRECTABLE_ERROR_STATUS;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 CacheDataEccMask : 1; //
> > > bit 0..0
> > > + UINT32 MemoryDataEccMask : 1; //
> > > bit 1..1
> > > + UINT32 CrcThresholdMask : 1; //
> > > bit 2..2
> > > + UINT32 RetryThresholdMask : 1; //
> > > bit 3..3
> > > + UINT32 CachePoisonReceivedMask : 1; //
> > > bit 4..4
> > > + UINT32 MemoryPoisonReceivedMask : 1; //
> > > bit 5..5
> > > + UINT32 PhysicalLayerErrorMask : 1; //
> > > bit 6..6
> > > + UINT32 Reserved : 25; //
> > > bit 7..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_CORRECTABLE_ERROR_MASK;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 FirstErrorPointer : 4; //
> > > bit 0..3
> > > + UINT32 Reserved1 : 5; //
> > > bit 4..8
> > > + UINT32 MultipleHeaderRecordingCapability : 1; //
> > > bit 9..9
> > > + UINT32 Reserved2 : 3; //
> > > bit 10..12
> > > + UINT32 PoisonEnabled : 1; //
> > > bit 13..13
> > > + UINT32 Reserved3 : 18; //
> > > bit 14..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_ERROR_CAPABILITIES_AND_CONTROL;
> > > +
> > > +typedef struct {
> > > + CXL_1_1_UNCORRECTABLE_ERROR_STATUS
> UncorrectableErrorStatus;
> > > + CXL_1_1_UNCORRECTABLE_ERROR_MASK
> UncorrectableErrorMask;
> > > + CXL_1_1_UNCORRECTABLE_ERROR_SEVERITY
> UncorrectableErrorSeverity;
> > > + CXL_CORRECTABLE_ERROR_STATUS
> CorrectableErrorStatus;
> > > + CXL_CORRECTABLE_ERROR_MASK
> CorrectableErrorMask;
> > > + CXL_ERROR_CAPABILITIES_AND_CONTROL
> ErrorCapabilitiesAndControl;
> > > + UINT32
> > > HeaderLog[16];
> > > +} CXL_1_1_RAS_CAPABILITY_STRUCTURE;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 DeviceTrustLevel : 2; //
> > > bit 0..1
> > > + UINT32 Reserved : 30; //
> > > bit 2..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_1_1_SECURITY_POLICY;
> > > +
> > > +typedef struct {
> > > + CXL_1_1_SECURITY_POLICY
> > > SecurityPolicy;
> > > +} CXL_1_1_SECURITY_CAPABILITY_STRUCTURE;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT64 CxlLinkVersionSupported : 4; //
> > > bit 0..3
> > > + UINT64 CxlLinkVersionReceived : 4; //
> > > bit 4..7
> > > + UINT64 LlrWrapValueSupported : 8; //
> > > bit 8..15
> > > + UINT64 LlrWrapValueReceived : 8; //
> > > bit 16..23
> > > + UINT64 NumRetryReceived : 5; //
> > > bit 24..28
> > > + UINT64 NumPhyReinitReceived : 5; //
> > > bit 29..33
> > > + UINT64 WrPtrReceived : 8; //
> > > bit 34..41
> > > + UINT64 EchoEseqReceived : 8; //
> > > bit 42..49
> > > + UINT64 NumFreeBufReceived : 8; //
> > > bit 50..57
> > > + UINT64 Reserved : 6; //
> > > bit 58..63
> > > + } Bits;
> > > + UINT64 Uint64;
> > > +} CXL_LINK_LAYER_CAPABILITY;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT16 LlReset : 1; //
> > > bit 0..0
> > > + UINT16 LlInitStall : 1; //
> > > bit 1..1
> > > + UINT16 LlCrdStall : 1; //
> > > bit 2..2
> > > + UINT16 InitState : 2; //
> > > bit 3..4
> > > + UINT16 LlRetryBufferConsumed : 8; //
> > > bit 5..12
> > > + UINT16 Reserved : 3; //
> > > bit 13..15
> > > + } Bits;
> > > + UINT16 Uint16;
> > > +} CXL_LINK_LAYER_CONTROL_AND_STATUS;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT64 CacheReqCredits : 10; //
> > > bit 0..9
> > > + UINT64 CacheRspCredits : 10; //
> > > bit 10..19
> > > + UINT64 CacheDataCredits : 10; //
> > > bit 20..29
> > > + UINT64 MemReqRspCredits : 10; //
> > > bit 30..39
> > > + UINT64 MemDataCredits : 10; //
> > > bit 40..49
> > > + } Bits;
> > > + UINT64 Uint64;
> > > +} CXL_LINK_LAYER_RX_CREDIT_CONTROL;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT64 CacheReqCredits : 10; //
> > > bit 0..9
> > > + UINT64 CacheRspCredits : 10; //
> > > bit 10..19
> > > + UINT64 CacheDataCredits : 10; //
> > > bit 20..29
> > > + UINT64 MemReqRspCredits : 10; //
> > > bit 30..39
> > > + UINT64 MemDataCredits : 10; //
> > > bit 40..49
> > > + } Bits;
> > > + UINT64 Uint64;
> > > +} CXL_LINK_LAYER_RX_CREDIT_RETURN_STATUS;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT64 CacheReqCredits : 10; //
> > > bit 0..9
> > > + UINT64 CacheRspCredits : 10; //
> > > bit 10..19
> > > + UINT64 CacheDataCredits : 10; //
> > > bit 20..29
> > > + UINT64 MemReqRspCredits : 10; //
> > > bit 30..39
> > > + UINT64 MemDataCredits : 10; //
> > > bit 40..49
> > > + } Bits;
> > > + UINT64 Uint64;
> > > +} CXL_LINK_LAYER_TX_CREDIT_STATUS;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 AckForceThreshold : 8; //
> > > bit 0..7
> > > + UINT32 AckFLushRetimer : 10; //
> > > bit 8..17
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_LINK_LAYER_ACK_TIMER_CONTROL;
> > > +
> > > +typedef union {
> > > + struct {
> > > + UINT32 MdhDisable : 1; //
> > > bit 0..0
> > > + UINT32 Reserved : 31; //
> > > bit 1..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_LINK_LAYER_DEFEATURE;
> > > +
> > > +typedef struct {
> > > + CXL_LINK_LAYER_CAPABILITY
> > > LinkLayerCapability;
> > > + CXL_LINK_LAYER_CONTROL_AND_STATUS
> LinkLayerControlStatus;
> > > + CXL_LINK_LAYER_RX_CREDIT_CONTROL
> LinkLayerRxCreditControl;
> > > + CXL_LINK_LAYER_RX_CREDIT_RETURN_STATUS
> LinkLayerRxCreditReturnStatus;
> > > + CXL_LINK_LAYER_TX_CREDIT_STATUS
> LinkLayerTxCreditStatus;
> > > + CXL_LINK_LAYER_ACK_TIMER_CONTROL
> LinkLayerAckTimerControl;
> > > + CXL_LINK_LAYER_DEFEATURE
> > > LinkLayerDefeature;
> > > +} CXL_1_1_LINK_CAPABILITY_STRUCTURE;
> > > +
> > > +#define CXL_IO_ARBITRATION_CONTROL_OFFSET 0x180
> > > +typedef union {
> > > + struct {
> > > + UINT32 Reserved1 : 4; //
> > > bit 0..3
> > > + UINT32 WeightedRoundRobinArbitrationWeight : 4; //
> > > bit 4..7
> > > + UINT32 Reserved2 : 24; //
> > > bit 8..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_IO_ARBITRATION_CONTROL;
> > > +
> > > +#define CXL_CACHE_MEMORY_ARBITRATION_CONTROL_OFFSET
> 0x1C0
> > > +typedef union {
> > > + struct {
> > > + UINT32 Reserved1 : 4; //
> > > bit 0..3
> > > + UINT32 WeightedRoundRobinArbitrationWeight : 4; //
> > > bit 4..7
> > > + UINT32 Reserved2 : 24; //
> > > bit 8..31
> > > + } Bits;
> > > + UINT32 Uint32;
> > > +} CXL_CACHE_MEMORY_ARBITRATION_CONTROL;
> > > +///@}
> > > +
> > > +/// The CXL.RCRB base register definition /// Based on chapter 7.3
> > > +of Compute Express Link Specification Revision: 1.1 ///@{ typedef
> > > +union {
> > > + struct {
> > > + UINT64 RcrbEnable : 1; //
> > > bit 0..0
> > > + UINT64 Reserved : 12; //
> > > bit 1..12
> > > + UINT64 RcrbBaseAddress : 51; //
> > > bit 13..63
> > > + } Bits;
> > > + UINT64 Uint64;
> > > +} CXL_RCRB_BASE;
> > > +///@}
> > > +
> > > +#pragma pack()
> > > +
> > > +//
> > > +// CXL Downstream / Upstream Port RCRB space register offsets //
> > > +Compute Express Link Specification Revision: 1.1 - Chapter 7.2.1.1
> > > +- Figure 97 //
> > > +#define CXL_PORT_RCRB_MEMBAR0_LOW_OFFSET
> > > 0x010
> > > +#define CXL_PORT_RCRB_MEMBAR0_HIGH_OFFSET
> > > 0x014
> > > +#define CXL_PORT_RCRB_EXTENDED_CAPABILITY_BASE_OFFSET
> 0x100
> > > +
> > > +#endif
> > > diff --git a/MdePkg/Include/IndustryStandard/Pci.h
> > > b/MdePkg/Include/IndustryStandard/Pci.h
> > > index 8ed96b992a..42c00ac762 100644
> > > --- a/MdePkg/Include/IndustryStandard/Pci.h
> > > +++ b/MdePkg/Include/IndustryStandard/Pci.h
> >
> > But here is the thing that really disappoints me getting through
> > review - this change is completely, fundamentally unrelated to the
> > rest of this patch, and not even mentioned in the commit message. This
> > should have been a separate patch preceding this one.
> >
> > > @@ -1,7 +1,7 @@
> > > /** @file
> > > Support for the latest PCI standard.
> > >
> > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > **/
> > > @@ -9,9 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > #ifndef _PCI_H_ #define _PCI_H_
> > >
> > > -#include <IndustryStandard/Pci30.h> -#include
> > > <IndustryStandard/PciExpress21.h> -#include
> > > <IndustryStandard/PciExpress30.h>
> > > +#include <IndustryStandard/PciExpress50.h>
> > > #include <IndustryStandard/PciCodeId.h>
> > >
> > > #endif
> >
> > Now, one final comment - and this is more of a project feature
> > suggestion:
> > Industry standard headers is something fairly special, even in
> > comparison with the rest of MdePkg. *I* would certainly like to ensure
> > I don't miss changes or additions to them.
> > Could we set up a dedicated group of reviewers for this folder only?
> > This need not affect the actual maintainership of MdePkg, just ensure
> > more eyeballs (or screen readers, braille terminals, ...) hit updates
> > here?
> >
> > i.e. something like the below to Maintainers.txt:
> >
> > F: MdePkg/Include/IndustryStandard/
> > R: Leif ...
> > R: ...
> > R: ...
> >
>
> This is a good suggestion. IndustryStandard needs more feedback.
> Can you send the patch to update Maintainers.txt?
>
> Thanks
> Liming
> > /
> > Leif
> >
> > > --
> > > 2.21.0.windows.1
> > >
> > >
> > >
> > >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#63389): https://edk2.groups.io/g/devel/message/63389
Mute This Topic: https://groups.io/mt/75823529/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
