Please refer to the inline comments below:
> -----Original Message----- > From: Chiu, Ian <ian.c...@intel.com> > Sent: Monday, April 25, 2022 9:45 PM > To: devel@edk2.groups.io > Cc: Chiu, Ian <ian.c...@intel.com>; Huang, Jenny <jenny.hu...@intel.com>; > Shih, More <more.s...@intel.com>; Wu, Hao A <hao.a...@intel.com>; Ni, Ray > <ray...@intel.com> > Subject: [PATCH v2] MdeModulePkg/XhciDxe: Add access xHCI Extended > Capabilities Pointer > > From: Ian Chiu <ian.c...@intel.com> > > Add support process Port Speed field value of PORTSC according to Supported > Protocol Capability > (new design in xHCI spec 1.2 2019) Please help to remove the above line. My take is that 'Supported Protocol Capability' contents already exist in the XHCI Spec 1.1 version. > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3914 > > The value of Port Speed field in PORTSC bit[10:13] (xHCI spec 1.2 2019 section > 5.4.8) > should be change to use this value to query thru Protocol Speed ID (PSI) > (xHCI spec 1.2 2019 section 7.2.1) in xHCI Supported Protocol Capability and > return the value according the Protocol Speed ID (PSIV) Dword. Could you help to add a brief summary on the PSI check when mapping to different port speeds (how the checks are mapping to low speed, high speed and super speed) in the log message? > > Cc: Jenny Huang <jenny.hu...@intel.com> > Cc: More Shih <more.s...@intel.com> > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Signed-off-by: Ian Chiu <ian.c...@intel.com> > --- > MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 41 ++++-- > MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h | 2 + > MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 147 ++++++++++++++++++++ > MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h | 87 ++++++++++++ > 4 files changed, 262 insertions(+), 15 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > index b79499e225..f5b99210c9 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > @@ -398,25 +398,32 @@ XhcGetRootHubPortStatus ( > State = XhcReadOpReg (Xhc, Offset); > > > > // > > - // According to XHCI 1.1 spec November 2017, > > - // bit 10~13 of the root port status register identifies the speed of the > attached device. > > + // According to XHCI 1.2 spec November 2019, I think you can still use the reference information from the XHCI 1.1 spec. The Support Protocol Capability related content already exists in this version of spec. > > + // Section 7.2 xHCI Support Protocol Capability > > // > > - switch ((State & XHC_PORTSC_PS) >> 10) { > > - case 2: > > - PortStatus->PortStatus |= USB_PORT_STAT_LOW_SPEED; > > - break; > > + PortStatus->PortStatus = XhcCheckUsbPortSpeedUsedPsic (Xhc, ((State & > XHC_PORTSC_PS) >> 10)); > > + if (PortStatus->PortStatus == 0) { > > + // > > + // According to XHCI 1.1 spec November 2017, > > + // bit 10~13 of the root port status register identifies the speed of the > attached device. > > + // > > + switch ((State & XHC_PORTSC_PS) >> 10) { > > + case 2: > > + PortStatus->PortStatus |= USB_PORT_STAT_LOW_SPEED; > > + break; > > > > - case 3: > > - PortStatus->PortStatus |= USB_PORT_STAT_HIGH_SPEED; > > - break; > > + case 3: > > + PortStatus->PortStatus |= USB_PORT_STAT_HIGH_SPEED; > > + break; > > > > - case 4: > > - case 5: > > - PortStatus->PortStatus |= USB_PORT_STAT_SUPER_SPEED; > > - break; > > + case 4: > > + case 5: > > + PortStatus->PortStatus |= USB_PORT_STAT_SUPER_SPEED; > > + break; > > > > - default: > > - break; > > + default: > > + break; > > + } > > } > > > > // > > @@ -1820,6 +1827,8 @@ XhcCreateUsbHc ( > Xhc->ExtCapRegBase = ExtCapReg << 2; > > Xhc->UsbLegSupOffset = XhcGetCapabilityAddr (Xhc, XHC_CAP_USB_LEGACY); > > Xhc->DebugCapSupOffset = XhcGetCapabilityAddr (Xhc, > XHC_CAP_USB_DEBUG); > > + Xhc->Usb2SupOffset = XhcGetUsbSupportedCapabilityAddr (Xhc, > USB_SUPPORT_PROTOCOL_USB2_MAJOR_VER); > > + Xhc->UsbSsSupOffset = XhcGetUsbSupportedCapabilityAddr (Xhc, > USB_SUPPORT_PROTOCOL_USB3_MAJOR_VER); > > > > DEBUG ((DEBUG_INFO, "XhcCreateUsb3Hc: Capability length 0x%x\n", Xhc- > >CapLength)); > > DEBUG ((DEBUG_INFO, "XhcCreateUsb3Hc: HcSParams1 0x%x\n", Xhc- > >HcSParams1)); > > @@ -1829,6 +1838,8 @@ XhcCreateUsbHc ( > DEBUG ((DEBUG_INFO, "XhcCreateUsb3Hc: RTSOff 0x%x\n", Xhc->RTSOff)); > > DEBUG ((DEBUG_INFO, "XhcCreateUsb3Hc: UsbLegSupOffset 0x%x\n", Xhc- > >UsbLegSupOffset)); > > DEBUG ((DEBUG_INFO, "XhcCreateUsb3Hc: DebugCapSupOffset 0x%x\n", Xhc- > >DebugCapSupOffset)); > > + DEBUG ((DEBUG_INFO, "XhcCreateUsb3Hc: Usb2SupOffset 0x%x\n", Xhc- > >Usb2SupOffset)); > > + DEBUG ((DEBUG_INFO, "XhcCreateUsb3Hc: UsbSsSupOffset 0x%x\n", Xhc- > >UsbSsSupOffset)); > > > > // > > // Create AsyncRequest Polling Timer > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > index 5054d796b1..7eed7bd15e 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h > @@ -227,6 +227,8 @@ struct _USB_XHCI_INSTANCE { > UINT32 ExtCapRegBase; > > UINT32 UsbLegSupOffset; > > UINT32 DebugCapSupOffset; > > + UINT32 Usb2SupOffset; > > + UINT32 UsbSsSupOffset; How about renaming to the above field to 'Usb3SupOffset'? > > UINT64 *DCBAA; > > VOID *DCBAAMap; > > UINT32 MaxSlotsEn; > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > index 80be3311d4..5bff698edb 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > @@ -564,7 +564,57 @@ XhcGetCapabilityAddr ( > if ((Data & 0xFF) == CapId) { > > return ExtCapOffset; > > } > > + // > > + // If not, then traverse all of the ext capability registers till > finding out it. > > + // > > + NextExtCapReg = (UINT8)((Data >> 8) & 0xFF); > > + ExtCapOffset += (NextExtCapReg << 2); > > + } while (NextExtCapReg != 0); > > + > > + return 0xFFFFFFFF; > > +} > > > > +/** > > + Calculate the offset of the xHCI Supported Protocol Capability. > > + > > + @param Xhc The XHCI Instance. > > + @param MajorVersion The USB Major Version in xHCI Support Protocol > Capability Field > > + > > + @return The offset of xHCI Supported Protocol capability register. > > + > > +**/ > > +UINT32 > > +XhcGetUsbSupportedCapabilityAddr ( Could you help to rename the function to XhcGetSupportedProtocolCapabilityAddr()? > > + IN USB_XHCI_INSTANCE *Xhc, > > + IN UINT8 MajorVersion > > + ) > > +{ > > + UINT32 ExtCapOffset; > > + UINT8 NextExtCapReg; > > + UINT32 Data; > > + UINT32 NameString; > > + XHC_SUPPORTED_PROTOCOL_DW0 UsbSupportDw0; > > + > > + if (Xhc == NULL) { > > + return 0; > > + } > > + > > + ExtCapOffset = 0; > > + > > + do { > > + // > > + // Check if the extended capability register's capability id is USB > Legacy > Support. > > + // > > + Data = XhcReadExtCapReg (Xhc, ExtCapOffset); > > + UsbSupportDw0.Dword = Data; > > + if ((Data & 0xFF) == XHC_CAP_USB_SUPPORTED) { > > + if (UsbSupportDw0.Data.RevMajor == MajorVersion) { > > + NameString = XhcReadExtCapReg (Xhc, ExtCapOffset + > USB_SUPPORTED_NAME_STRING_OFFSET); > > + if (NameString == USB_SUPPORTED_PROTOCOL_NAME_STRING) { > > + return ExtCapOffset; > > + } > > + } > > + } > > // > > // If not, then traverse all of the ext capability registers till > finding out it. > > // > > @@ -575,6 +625,103 @@ XhcGetCapabilityAddr ( > return 0xFFFFFFFF; > > } > > > > +/** > > + Find SpeedField value match with Port Speed ID value. > > + > > + @param Xhc The XHCI Instance. > > + @param ExtCapOffset The USB Major Version in xHCI Support Protocol > Capability Field > > + @param SpeedField The Port Speed filed in USB PortSc register > > + > > + @return The Protocol Speed ID xHCI Supported Protocol capability register. > > + > > +**/ > > +UINT32 > > +XhciPsivGetPsid ( > > + IN USB_XHCI_INSTANCE *Xhc, > > + IN UINT32 ExtCapOffset, > > + IN UINT8 SpeedField > > + ) > > +{ > > + XHC_SUPPORTED_PROTOCOL_DW2 PortId; > > + XHC_SUPPORTED_PROTOCOL_FIELD Reg; > > + UINT32 Count; > > + > > + if ((Xhc == NULL) || (ExtCapOffset == 0xFFFFFFFF)) { > > + return 0; > > + } > > + > > + // > > + // According to XHCI 1.2 spec November 2019, > > + // Section 7.2 xHCI Supported Protocol Capability > > + // 1. Get the PSIC(Protocol Speed ID Count) Value. > > + // 2. The PSID register boundary should be Base address + PSIC * 0x04 > > + // > > + PortId.Dword = XhcReadExtCapReg (Xhc, ExtCapOffset + > USB_SUPPORTED_PORT_ID_OFFSET); > > + > > + for (Count = 0; Count < PortId.Data.Psic; Count++) { > > + Reg.Dword = XhcReadExtCapReg (Xhc, ExtCapOffset + > USB_SUPPORT_SPEED_ID_OFFSET + (Count << 2)); > > + if (Reg.Data.Psiv == SpeedField) { > > + return Reg.Dword; > > + } > > + } > > + return 0; > > +} > > + > > +/** > > + Find SpeedField value match with Port Speed ID value. > > + > > + @param Xhc The XHCI Instance. > > + @param Speed The Port Speed filed in USB PortSc register > > + > > + @return The USB Port Speed. > > + > > +**/ > > +UINT16 > > +XhcCheckUsbPortSpeedUsedPsic ( > > + IN USB_XHCI_INSTANCE *Xhc, > > + IN UINT8 Speed > > + ) > > +{ > > + XHC_SUPPORTED_PROTOCOL_FIELD SpField; > > + UINT16 ReturnSpeed; > > + > > + if (Xhc == NULL) { > > + return 0; > > + } > > + > > + SpField.Dword = 0; > > + ReturnSpeed = 0; > > + // > > + // Check USB3 Protocol Speed ID if ReturnSpeed didn't get match speed. > > + // > > + if ((ReturnSpeed == 0) && (Xhc->UsbSsSupOffset != 0xFFFFFFFF)) { > > + SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->UsbSsSupOffset, Speed); > > + if (SpField.Dword != 0) { > > + // Super Speed > > + ReturnSpeed = USB_PORT_STAT_SUPER_SPEED; > > + } > > + } > > + > > + // > > + // Check USB2 Protocol Speed ID if ReturnSpeed didn't get match speed. > > + // > > + if ((ReturnSpeed == 0) && (Xhc->Usb2SupOffset != 0xFFFFFFFF)) { > > + SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb2SupOffset, Speed); > > + if (SpField.Dword != 0) { > > + if (SpField.Data.Psie == 2) { > > + if (SpField.Data.Mantissa == > USB_SUPPORT_PROTOCOL_USB2_HIGH_SPEED_PSIM) { > > + // High Speed > > + ReturnSpeed = USB_PORT_STAT_HIGH_SPEED; > > + } > > + } else if (SpField.Data.Psie == 1) { > > + // Low speed > > + ReturnSpeed = USB_PORT_STAT_LOW_SPEED; I suggest to add a similar Psim field check (whether equals to 1500) as the high speed check above. > > + } > > + } > > + } > > + return ReturnSpeed; > > +} > > + > > /** > > Whether the XHCI host controller is halted. > > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h > index 4950eed272..4f83b49027 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h > @@ -27,6 +27,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > #define XHC_CAP_USB_LEGACY 0x01 > > #define XHC_CAP_USB_DEBUG 0x0A > > +#define XHC_CAP_USB_SUPPORTED 0x02 Could you help to rename the above macro to XHC_CAP_USB_SUPPORTED_PROTOCOL? > > > > // ============================================// > > // XHCI register offset // > > @@ -74,6 +75,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #define USBLEGSP_BIOS_SEMAPHORE BIT16 // HC BIOS Owned > Semaphore > > #define USBLEGSP_OS_SEMAPHORE BIT24 // HC OS Owned Semaphore > > > > +// > > +// xHCI Supported Protocol Capability > > +// > > +#define USB_SUPPORTED_PROTOCOL_NAME_STRING 0x20425355 > > +#define USB_SUPPORTED_NAME_STRING_OFFSET 0x04 > > +#define USB_SUPPORTED_PORT_ID_OFFSET 0x08 > > +#define USB_SUPPORT_SPEED_ID_OFFSET 0x10 > > +#define USB_SUPPORT_PROTOCOL_USB2_MAJOR_VER 0x02 > > +#define USB_SUPPORT_PROTOCOL_USB3_MAJOR_VER 0x03 > > +#define USB_SUPPORT_PROTOCOL_USB2_HIGH_SPEED_PSIM 480 Could you please help to update the above macro definitions to: #define XHC_SUPPORTED_PROTOCOL_DW0_MAJOR_REVISION_USB2 0x02 #define XHC_SUPPORTED_PROTOCOL_DW0_MAJOR_REVISION_USB3 0x03 #define XHC_SUPPORTED_PROTOCOL_NAME_STRING_OFFSET 0x04 #define XHC_SUPPORTED_PROTOCOL_NAME_STRING_VALUE 0x20425355 #define XHC_SUPPORTED_PROTOCOL_DW2_OFFSET 0x08 #define XHC_SUPPORTED_PROTOCOL_PSI_OFFSET 0x10 #define XHC_SUPPORTED_PROTOCOL_USB2_HIGH_SPEED_PSIM 480 > > + > > #pragma pack (1) > > typedef struct { > > UINT8 MaxSlots; // Number of Device Slots > > @@ -130,6 +142,52 @@ typedef union { > HCCPARAMS Data; > > } XHC_HCCPARAMS; > > > > +// > > +// xHCI Supported Protocol Cabability > > +// > > +typedef struct { > > + UINT8 CapId; > > + UINT8 NextExtCapReg; > > + UINT8 RevMinor; > > + UINT8 RevMajor; > > +} SUPP_PROTOCOL_DW0; Could you help to rename the above structure name to SUPPORTED_PROTOCOL_DW0? > > + > > +typedef union { > > + UINT32 Dword; > > + SUPP_PROTOCOL_DW0 Data; > > +} XHC_SUPPORTED_PROTOCOL_DW0; > > + > > +typedef struct { > > + UINT32 NameString; > > +} XHC_SUPPORTED_PROTOCOL_DW1; > > + > > +typedef struct { > > + UINT8 CompPortOffset : 8; > > + UINT8 CompPortCount : 8; > > + UINT16 ProtocolDef :12; > > + UINT16 Psic : 4; > > +} SUPP_PROTOCOL_DW2; Could you help to refine the above structure definition to: typedef struct { UINT8 CompPortOffset; UINT8 CompPortCount; UINT16 ProtocolDef :12; UINT16 Psic : 4; } SUPPORTED_PROTOCOL_DW2; > > + > > +typedef union { > > + UINT32 Dword; > > + SUPP_PROTOCOL_DW2 Data; > > +} XHC_SUPPORTED_PROTOCOL_DW2; > > + > > +typedef struct { > > + UINT16 Psiv : 4; > > + UINT16 Psie : 2; > > + UINT16 Plt : 2; > > + UINT16 Pfd : 1; > > + UINT16 RsvdP : 5; > > + UINT16 Lp : 2; > > + UINT16 Mantissa :16; > > +} XHCI_PROTOCOL_FIELD; Could you help to refine the above structure definition to: typedef struct { UINT16 Psiv : 4; UINT16 Psie : 2; UINT16 Plt : 2; UINT16 Pfd : 1; UINT16 RsvdP : 5; UINT16 Lp : 2; UINT16 Psim; } SUPPORTED_PROTOCOL_PROTOCOL_SPEED_ID; > > + > > +typedef union { > > + UINT32 Dword; > > + XHCI_PROTOCOL_FIELD Data; > > +} XHC_SUPPORTED_PROTOCOL_FIELD; Could you help to refine the above structure definition to: typedef union { UINT32 Dword; SUPPORTED_PROTOCOL_PROTOCOL_SPEED_ID Data; } XHC_SUPPORTED_PROTOCOL_PROTOCOL_SPEED_ID; Best Regards, Hao Wu > > + > > #pragma pack () > > > > // > > @@ -546,4 +604,33 @@ XhcGetCapabilityAddr ( > IN UINT8 CapId > > ); > > > > +/** > > + Calculate the offset of the xHCI Supported Protocol Capability. > > + > > + @param Xhc The XHCI Instance. > > + @param MajorVersion The USB Major Version in xHCI Support Protocol > Capability Field > > + > > + @return The offset of xHCI Supported Protocol capability register. > > + > > +**/ > > +UINT32 > > +XhcGetUsbSupportedCapabilityAddr ( > > + IN USB_XHCI_INSTANCE *Xhc, > > + IN UINT8 MajorVersion > > + ); > > + > > +/** > > + Find SpeedField value match with Port Speed ID value. > > + > > + @param Xhc The XHCI Instance. > > + @param Speed The Port Speed filed in USB PortSc register > > + > > + @return The USB Port Speed. > > + > > +**/ > > +UINT16 > > +XhcCheckUsbPortSpeedUsedPsic ( > > + IN USB_XHCI_INSTANCE *Xhc, > > + IN UINT8 Speed > > + ); > > #endif > > -- > 2.26.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89553): https://edk2.groups.io/g/devel/message/89553 Mute This Topic: https://groups.io/mt/90684916/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
