From: Haojian Zhuang <haojian.zhuang@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH v1 2/2] EmbeddedPkg/Drivers: add DwUsb3Dxe driver
Date: Mon, 22 Oct 2018 10:51:22 +0800 [thread overview]
Message-ID: <CAD6h2NRZ898Cp3L59yuyiKt0b7R++uypXxKbpvRAqNfEiyW9sQ@mail.gmail.com> (raw)
In-Reply-To: <20181004163207.ydfxtrsdxaf6ifgf@bivouac.eciton.net>
On Fri, 5 Oct 2018 at 00:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Mon, Aug 20, 2018 at 06:31:25PM +0800, Haojian Zhuang wrote:
> > Add Designware USB 3.0 device driver. It's focus on USB device
> > functionality, not USB Host functionality. The USB driver is
> > mainly used for Android Fastboot application.
> >
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> > ---
> > EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.dec | 44 +
> > EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.inf | 52 +
> > EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.h | 632 +++++
> > EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.c | 2434 ++++++++++++++++++++
> > 4 files changed, 3162 insertions(+)
> >
> > diff --git a/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.dec b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.dec
> > new file mode 100644
> > index 000000000000..038e4881a948
> > --- /dev/null
> > +++ b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.dec
> > @@ -0,0 +1,44 @@
> > +#/** @file
> > +# Framework Module Development Environment Industry Standards
>
> Please change this to reflect this package rather than EmbeddedPkg one
> (which appears to have stolen it from an old version of MdePkg.dec :)
>
OK
> > +#
> > +# This Package provides headers and libraries that conform to EFI/PI Industry standards.
> > +# Copyright (c) 2007, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2012-2014, ARM Ltd. All rights reserved.<BR>
>
> I see no need to keep these copyright statements.
>
OK
> > +# Copyright (c) 2018, Linaro. All rights reserved.<BR>
> > +#
> > +# This program and the accompanying materials are licensed and made available under
> > +# the terms and conditions of the BSD License which accompanies this distribution.
> > +# The full text of the license may be found at
> > +# http://opensource.org/licenses/bsd-license.php
> > +#
> > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +#
> > +#**/
> > +
> > +[Defines]
> > + DEC_SPECIFICATION = 0x00010019
> > + PACKAGE_NAME = DwUsb3DxePkg
> > + PACKAGE_GUID = 9b7aa6fe-405b-4955-af1f-5faf183aedec
> > + PACKAGE_VERSION = 0.1
> > +
> > +
> > +################################################################################
> > +#
> > +# Include Section - list of Include Paths that are provided by this package.
> > +# Comments are used for Keywords and Module Types.
> > +#
> > +# Supported Module Types:
> > +# BASE SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION
> > +#
> > +################################################################################
> > +
> > +[Guids.common]
> > + gDwUsb3DxeTokenSpaceGuid = { 0x098a50d3, 0x92e2, 0x461a, { 0xb6, 0xba, 0xf8, 0x5d, 0x9d, 0x89, 0x49, 0xf5 }}
> > +
> > +[Protocols.common]
> > + gDwUsbProtocolGuid = { 0x109fa264, 0x7811, 0x4862, { 0xa9, 0x73, 0x4a, 0xb2, 0xef, 0x2e, 0xe2, 0xff }}
>
> Please make this file a common .dec for DwUsb and DwUsb3 instead of
> duplicating guid information.
> (If we weren't moving it to edk2-platforms, it would still have been
> better to add it to EmbeddedPkg.dec.)
>
OK
> > +
> > +[PcdsFixedAtBuild.common]
> > + # DwUsb Driver PCDs
> > + gDwUsb3DxeTokenSpaceGuid.PcdDwUsb3DxeBaseAddress|0x0|UINT32|0x00000001
> > diff --git a/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.inf b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.inf
> > new file mode 100644
> > index 000000000000..510b51a34de7
> > --- /dev/null
> > +++ b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.inf
> > @@ -0,0 +1,52 @@
> > +#/** @file
> > +#
> > +# Copyright (c) 2018, Linaro Limited. All rights reserved.
> > +#
> > +# This program and the accompanying materials
> > +# are licensed and made available under the terms and conditions of the BSD License
> > +# which accompanies this distribution. The full text of the license may be found at
> > +# http://opensource.org/licenses/bsd-license.php
> > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +#
> > +#
> > +#**/
> > +
> > +[Defines]
> > + INF_VERSION = 0x00010019
> > + BASE_NAME = DwUsb3Dxe
> > + FILE_GUID = 0879cd34-c399-4315-9891-56024072e711
> > + MODULE_TYPE = UEFI_DRIVER
> > + VERSION_STRING = 1.0
> > + ENTRY_POINT = DwUsb3EntryPoint
> > +
> > +[Sources.common]
> > + DwUsb3Dxe.c
> > +
> > +[LibraryClasses]
> > + CacheMaintenanceLib
> > + DebugLib
> > + DmaLib
> > + IoLib
> > + MemoryAllocationLib
> > + TimerLib
> > + UefiBootServicesTableLib
> > + UefiDriverEntryPoint
> > + UsbSerialNumberLib
>
> I don't see this library anywhere in the tree?
>
I'll add it into the patch series for the next revision.
> > +
> > +[Protocols]
> > + gEfiDriverBindingProtocolGuid
> > + gUsbDeviceProtocolGuid
> > +
> > +[Packages]
> > + ArmPkg/ArmPkg.dec
> > + EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.dec
> > + EmbeddedPkg/EmbeddedPkg.dec
> > + MdePkg/MdePkg.dec
> > + MdeModulePkg/MdeModulePkg.dec
> > +
> > +[Pcd]
> > + gDwUsb3DxeTokenSpaceGuid.PcdDwUsb3DxeBaseAddress
> > +
> > +[Depex]
> > + TRUE
> > diff --git a/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.h b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.h
> > new file mode 100644
> > index 000000000000..92b610553169
> > --- /dev/null
> > +++ b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.h
> > @@ -0,0 +1,632 @@
> > +/** @file
> > +
> > + Copyright (c) 2017, Linaro Limited. All rights reserved.
> > +
> > + This program and the accompanying materials
> > + are licensed and made available under the terms and conditions of the BSD License
> > + which accompanies this distribution. The full text of the license may be found at
> > + http://opensource.org/licenses/bsd-license.php
> > +
> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef __DW_USB3_DXE_H__
> > +#define __DW_USB3_DXE_H__
>
> DW -> DESIGNWARE (throughout).
>
OK
> > +
> > +#define DW_USB3_BASE FixedPcdGet32 (PcdDwUsb3DxeBaseAddress)
> > +
> > +#define GSBUCFG0 (DW_USB3_BASE + 0xC100)
> > +#define GCTL (DW_USB3_BASE + 0xC110)
> > +
> > +#define GCTL_PWRDNSCALE_MASK (0x1FFF << 19)
> > +#define GCTL_PWRDNSCALE(x) (((x) & 0x1FFF) << 19)
> > +#define GCTL_U2RSTECN BIT16
> > +#define GCTL_PRTCAPDIR_MASK (BIT13 | BIT12)
> > +#define GCTL_PRTCAPDIR_HOST BIT12
> > +#define GCTL_PRTCAPDIR_DEVICE BIT13
> > +#define GCTL_PRTCAPDIR_OTG (BIT13 | BIT12)
> > +#define GCTL_U2EXIT_LFPS BIT2
> > +
> > +#define GUSB2PHYCFG(x) (DW_USB3_BASE + 0xC200 + (((x) & 0xF) << 2))
> > +
> > +#define GUSB2PHYCFG_USBTRDTIM_MASK (0xF << 10)
> > +#define GUSB2PHYCFG_USBTRDTIM(x) (((x) & 0xF) << 10)
> > +#define GUSB2PHYCFG_SUSPHY BIT6
> > +
> > +#define GUSB3PIPECTL(x) (DW_USB3_BASE + 0xC2C0 + (((x) & 0x3) << 2))
> > +
> > +#define PIPECTL_DELAYP1TRANS BIT18
> > +#define PIPECTL_SUSPEND_EN BIT17
> > +#define PIPECTL_LFPS_FILTER BIT9
> > +#define PIPECTL_TX_DEMPH_MASK (0x3 << 1)
> > +#define PIPECTL_TX_DEMPH(x) (((x) & 0x3) << 1)
> > +
> > +#define GTXFIFOSIZ(x) (DW_USB3_BASE + 0xC300 + (((x) & 0x1F) << 2))
> > +#define GRXFIFOSIZ(x) (DW_USB3_BASE + 0xC380 + (((x) & 0x1F) << 2))
> > +
> > +#define FIFOSIZ_ADDR(x) (((x) & 0xFFFF) << 16)
> > +#define FIFOSIZ_DEP(x) ((x) & 0xFFFF)
> > +
> > +#define GEVNTADRL(x) (DW_USB3_BASE + 0xC400 + (((x) & 0x1F) << 2))
> > +#define GEVNTADRH(x) (DW_USB3_BASE + 0xC404 + (((x) & 0x1F) << 2))
> > +#define GEVNTSIZ(x) (DW_USB3_BASE + 0xC408 + (((x) & 0x1F) << 2))
> > +
> > +#define GEVNTSIZ_EVNTINTMASK BIT31
> > +#define GEVNTSIZ_EVNTSIZ_MASK (0xFFFF)
> > +#define GEVNTSIZ_EVNTSIZ(x) ((x) & 0xFFFF)
> > +
> > +#define GEVNTCOUNT(x) (DW_USB3_BASE + 0xC40C + (((x) & 0x1F) << 2))
> > +#define GEVNTCOUNT_EVNTCOUNT_MASK (0xFFFF)
> > +#define GEVNTCOUNT_EVNTCOUNT(x) ((x) & 0xFFFF)
> > +
> > +// Non-Endpoint specific event flag
> > +#define GEVNT_INTTYPE_MASK (0x7F << 1)
> > +#define GEVNT_INTTYPE(x) (((x) & 0x7F) << 1)
> > +#define EVENT_I2C_INT 4
> > +#define EVENT_CARKIT_INT 3
> > +#define EVENT_OTG_INT 1
> > +#define EVENT_DEV_INT 0
> > +
> > +#define GEVNT_NON_EP BIT0
> > +// Endpoint specific event flag
> > +#define GEVNT_DEPEVT_INTTYPE_MASK (0xF << 6)
> > +#define GEVNT_DEPEVT_INTTYPE(x) (((x) & 0xF) << 6)
> > +#define GEVNT_DEPEVT_INTTYPE_EPCMD_CMPL (7 << 6)
> > +#define GEVNT_DEPEVT_INTTYPE_STRM_EVT (6 << 6)
> > +#define GEVNT_DEPEVT_INTTYPE_FIFOXRUN (4 << 6)
> > +#define GEVNT_DEPEVT_INTTYPE_XFER_NRDY (3 << 6)
> > +#define GEVNT_DEPEVT_INTTYPE_XFER_IN_PROG (2 << 6)
> > +#define GEVNT_DEPEVT_INTTYPE_XFER_CMPL (1 << 6)
> > +#define GEVNT_DEPEVT_EPNUM_MASK (0x1F << 1)
> > +#define GEVNT_DEPEVT_EPNUM_SHIFT 1
> > +#define GEVNT_DEPEVT_EPNUM(x) (((x) & 0x1F) << 1)
> > +// Devices specific event flag
> > +#define GEVNT_DEVT_MASK (0xF << 8)
> > +#define GEVNT_DEVT_SHIFT 8
> > +#define GEVNT_DEVT(x) (((x) & 0xF) << 8)
> > +#define GEVNT_DEVT_INACT_TIMEOUT_RCVD (0x15 << 8)
> > +#define GEVNT_DEVT_VNDR_DEV_TST_RCVD (0x14 << 8)
> > +#define GEVNT_DEVT_OVERFLOW (0x13 << 8)
> > +#define GEVNT_DEVT_CMD_CMPL (0x12 << 8)
> > +#define GEVNT_DEVT_ERRATICERR (0x11 << 8)
> > +#define GEVNT_DEVT_SOF (0x7 << 8)
> > +#define GEVNT_DEVT_EOPF (0x6 << 8)
> > +#define GEVNT_DEVT_HIBER_REQ (0x5 << 8)
> > +#define GEVNT_DEVT_WKUP (0x4 << 8)
> > +#define GEVNT_DEVT_ULST_CHNG (0x3 << 8)
> > +#define GEVNT_DEVT_CONNDONE (0x2 << 8)
> > +#define GEVNT_DEVT_USBRESET (0x1 << 8)
> > +#define GEVNT_DEVT_DISCONN (0x0 << 8)
> > +
> > +#define DCFG (DW_USB3_BASE + 0xC700)
> > +
> > +#define DCFG_NUMP_MASK (0x1F << 17)
> > +#define DCFG_NUMP(x) (((x) & 0x1F) << 17)
> > +#define DCFG_DEVADDR_MASK (0x7F << 3)
> > +#define DCFG_DEVADDR(x) (((x) & 0x7F) << 3)
> > +#define DCFG_DEVSPD_MASK (0x7)
> > +#define DCFG_DEVSPD(x) ((x) & 0x7)
> > +#define DEVSPD_HS_PHY_30MHZ_OR_60MHZ 0
> > +#define DEVSPD_FS_PHY_30MHZ_OR_60MHZ 1
> > +#define DEVSPD_LS_PHY_6MHZ 2
> > +#define DEVSPD_FS_PHY_48MHZ 3
> > +#define DEVSPD_SS_PHY_125MHZ_OR_250MHZ 4
> > +
> > +#define DCTL (DW_USB3_BASE + 0xC704)
> > +
> > +#define DCTL_RUN_STOP BIT31
> > +#define DCTL_CSFTRST BIT30
> > +#define DCTL_INIT_U2_EN BIT12
> > +#define DCTL_ACCEPT_U2_EN BIT11
> > +#define DCTL_INIT_U1_EN BIT10
> > +#define DCTL_ACCEPT_U1_EN BIT9
> > +
> > +#define DEVTEN (DW_USB3_BASE + 0xC708)
> > +#define DEVTEN_CONNECTDONEEN BIT2
> > +#define DEVTEN_USBRSTEN BIT1
> > +#define DEVTEN_DISCONNEN BIT0
> > +
> > +#define DSTS (DW_USB3_BASE + 0xC70C)
> > +#define DSTS_GET_DEVSPD(x) ((x) & 0x7)
> > +
> > +#define DALEPENA (DW_USB3_BASE + 0xC720)
> > +
> > +#define DEPCMDPAR2(x) (DW_USB3_BASE + 0xC800 + ((x) & 0x1F) * 0x10)
> > +#define DEPCMDPAR1(x) (DW_USB3_BASE + 0xC804 + ((x) & 0x1F) * 0x10)
> > +#define DEPCMDPAR0(x) (DW_USB3_BASE + 0xC808 + ((x) & 0x1F) * 0x10)
> > +#define DEPCMD(x) (DW_USB3_BASE + 0xc80C + ((x) & 0x1F) * 0x10)
> > +
> > +#define DEPCMD_COMMANDPARAM_MASK (0xFFFF << 16)
> > +#define DEPCMD_COMMANDPARAM(x) (((x) & 0xFFFF) << 16)
> > +/* Stream Number or uFrame (input) */
> > +#define DEPCMD_STR_NUM_OR_UF_MASK (0xFFFF << 16)
> > +#define DEPCMD_STR_NUM_OR_UF(x) (((x) & 0xFFFF) << 16)
> > +/* Transfer Resource Index (output) */
> > +#define DEPCMD_XFER_RSRC_IDX_SHIFT 16
> > +#define DEPCMD_XFER_RSRC_IDX_MASK (0x7F << 16)
> > +#define DEPCMD_XFER_RSRC_IDX(x) (((x) & 0x7F) << 16)
> > +#define GET_DEPCMD_XFER_RSRC_IDX(x) (((x) >> 16) & 0x7F)
> > +#define DEPCMD_CMDACT BIT10
> > +#define DEPCMD_CMDTYPE_MASK 0xFF
> > +#define DEPCMD_CMDTYPE(x) ((x) & 0xFF)
> > +
> > +/* EP registers range as: OUT0, IN0, OUT1, IN1, ... */
> > +#define EP_OUT_IDX(x) ((x) * 2)
> > +#define EP_IN_IDX(x) (((x) * 2) + 1)
> > +
> > +#define CMDTYPE_SET_EP_CFG 1
> > +#define CMDTYPE_SET_XFER_CFG 2
> > +#define CMDTYPE_GET_EP_STATE 3
> > +#define CMDTYPE_SET_STALL 4
> > +#define CMDTYPE_CLR_STALL 5
> > +#define CMDTYPE_START_XFER 6
> > +#define CMDTYPE_UPDATE_XFER 7
> > +#define CMDTYPE_END_XFER 8
> > +#define CMDTYPE_START_NEW_CFG 9
> > +
> > +#define EPTYPE_CONTROL 0
> > +#define EPTYPE_ISOC 1
> > +#define EPTYPE_BULK 2
> > +#define EPTYPE_INTR 3
> > +
> > +#define CFG_ACTION_INIT 0
> > +#define CFG_ACTION_RESTORE 1
> > +#define CFG_ACTION_MODIFY 2
> > +
> > +#define EPCFG0_CFG_ACTION_MASK (0x3 << 30)
> > +#define EPCFG0_CFG_ACTION(x) (((x) & 0x3) << 30)
> > +#define EPCFG0_BRSTSIZ_MASK (0xF << 22)
> > +#define EPCFG0_BRSTSIZ(x) (((x) & 0xF) << 22)
> > +#define EPCFG0_TXFNUM_MASK (0x1F << 17)
> > +#define EPCFG0_TXFNUM(x) (((x) & 0x1F) << 17)
> > +#define EPCFG0_MPS_MASK (0x7FF << 3)
> > +#define EPCFG0_MPS(x) (((x) & 0x7FF) << 3)
> > +#define EPCFG0_EPTYPE_MASK (0x3 << 1)
> > +#define EPCFG0_EPTYPE_SHIFT 1
> > +#define EPCFG0_EPTYPE(x) (((x) & 0x3) << 1)
> > +
> > +/* Endpoint Number */
> > +#define EPCFG1_EP_NUM_MASK (0xF << 26)
> > +#define EPCFG1_EP_NUM(x) (((x) & 0xF) << 26)
> > +/* Endpoint Direction */
> > +#define EPCFG1_EP_DIR_IN BIT25
> > +/* Stream Not Ready */
> > +#define EPCFG1_XFER_NRDY BIT10
> > +/* XferInProgress Enable */
> > +#define EPCFG1_XFER_IN_PROG BIT9
> > +/* Stream Completed */
> > +#define EPCFG1_XFER_CMPL BIT8
> > +
> > +#define USB_SPEED_UNKNOWN 0
> > +#define USB_SPEED_LOW 1
> > +#define USB_SPEED_FULL 2
> > +#define USB_SPEED_HIGH 3
> > +#define USB_SPEED_VARIABLE 4
> > +#define USB_SPEED_SUPER 5
> > +
> > +// DMA registers
> > +#define DSCSTS_TRBRSP_MASK (0xF << 28)
> > +#define DSCSTS_TRBRSP(x) (((x) & 0xF) << 28)
> > +#define GET_DSCSTS_TRBRSP(x) (((x) >> 28) & 0xF)
> > +#define TRBRSP_MISSED_ISOC_IN 1
> > +#define TRBRSP_SETUP_PEND 2
> > +#define TRBRSP_XFER_IN_PROG 4
> > +#define DSCSTS_PCM1_MASK (0x3 << 24)
> > +#define DSCSTS_PCM1(x) (((x) & 0x3) << 24)
> > +#define DSCSTS_XFERCNT_MASK 0xFFFFFF
> > +#define DSCSTS_XFERCNT(x) ((x) & 0xFFFFFF)
> > +#define GET_DSCSTS_XFERCNT(x) ((x) & 0xFFFFFF)
> > +
> > +#define DSCCTL_STRMID_SOFN(x) (((x) & 0xFFFF) << 14)
> > +#define DSCCTL_IOC BIT11
> > +#define DSCCTL_ISP BIT10
> > +#define DSCCTL_TRBCTL_MASK (0x3F << 4)
> > +#define DSCCTL_TRBCTL(x) (((x) & 0x3F) << 4)
> > +#define DSCCTL_LST BIT1
> > +#define DSCCTL_HWO BIT0
> > +#define TRBCTL_NORMAL 1
> > +#define TRBCTL_SETUP 2
> > +#define TRBCTL_STATUS_2 3
> > +#define TRBCTL_STATUS_3 4
> > +#define TRBCTL_CTLDATA_1ST 5
> > +#define TRBCTL_ISOC_1ST 6
> > +#define TRBCTL_ISOC 7
> > +#define TRBCTL_LINK 8
> > +#define TRBCTL_NORMAL_ZLP 9
> > +
> > +
> > +#define UE_DIR_IN 0x80
> > +#define UE_DIR_OUT 0
> > +#define UE_SET_DIR(a, d) ((a) | (((d) & 1) << 7))
> > +#define UE_GET_DIR(a) ((a) & 0x80)
> > +#define UE_ADDR 0x0F
> > +#define UE_GET_ADDR(a) ((a) & UE_ADDR)
> > +
> > +#define UT_GET_DIR(a) ((a) & 0x80)
> > +#define UT_WRITE 0x00
> > +#define UT_READ 0x80
> > +
> > +#define UT_GET_TYPE(a) ((a) & 0x60)
> > +#define UT_STANDARD 0x00
> > +#define UT_CLASS 0x20
> > +#define UT_VENDOR 0x40
> > +
> > +#define UT_GET_RECIPIENT(a) ((a) & 0x1f)
> > +#define UT_DEVICE 0x00
> > +#define UT_INTERFACE 0x01
> > +#define UT_ENDPOINT 0x02
> > +#define UT_OTHER 0x03
> > +
> > +#define UR_GET_STATUS 0x00
> > +#define UR_CLEAR_FEATURE 0x01
> > +#define UR_SET_FEATURE 0x03
> > +#define UR_SET_ADDRESS 0x05
> > +#define UR_GET_DESCRIPTOR 0x06
> > +#define UR_SET_DESCRIPTOR 0x07
> > +#define UR_GET_CONFIG 0x08
> > +#define UR_SET_CONFIG 0x09
> > +#define UR_GET_INTERFACE 0x0A
> > +#define UR_SET_INTERFACE 0x0B
> > +#define UR_SYNCH_FRAME 0x0C
> > +#define UR_SET_SEL 0x30
> > +#define UR_SET_ISOC_DELAY 0x31
> > +
>
> Somewhere around here starts a bunch of redefinitions of things
> already described in MdePkg/Include/IndustryStandard/Usb.h.
> Can you please reuse that include file as much as possible.
>
OK
> > +/* Feature numbers */
> > +#define UF_ENDPOINT_HALT 0
> > +#define UF_DEVICE_REMOTE_WAKEUP 1
> > +#define UF_TEST_MODE 2
> > +#define UF_DEVICE_B_HNP_ENABLE 3
> > +#define UF_DEVICE_A_HNP_SUPPORT 4
> > +#define UF_DEVICE_A_ALT_HNP_SUPPORT 5
> > +#define UF_FUNCTION_SUSPEND 0
> > +#define UF_U1_ENABLE 48
> > +#define UF_U2_ENABLE 49
> > +#define UF_LTM_ENABLE 50
> > +
> > +#define UDESC_DEVICE 0x01
> > +#define UDESC_CONFIG 0x02
> > +#define UDESC_STRING 0x03
> > +#define UDESC_INTERFACE 0x04
> > +#define UDESC_ENDPOINT 0x05
> > +#define UDESC_SS_USB_COMPANION 0x30
> > +#define UDESC_DEVICE_QUALIFIER 0x06
> > +#define UDESC_BOS 0x0f
> > +#define UDESC_DEVICE_CAPABILITY 0x10
> > +
> > +#define STRING_LANGUAGE 0
> > +#define STRING_MANUFACTURER 1
> > +#define STRING_PRODUCT 2
> > +#define STRING_SERIAL 3
> > +
> > +#define CONFIG_VALUE 1
> > +
> > +#define USB3_BULK_IN_EP 1
> > +#define USB3_BULK_OUT_EP 1
> > +
> > +#define USB_ENUM_ADB_PORT_VID 0x18D1
> > +#define USB_ENUM_ADB_PORT_PID 0xD00D
> > +#define USB_ENUM_INTERFACE_ADB_SUBCLASS 0x42
> > +#define USB_ENUM_INTERFACE_ADB_PROTOCOL 0x03
> > +
> > +struct usb3_pcd;
> > +
> > +typedef enum pcd_state {
> > + USB3_STATE_UNCONNECTED, /* no host */
> > + USB3_STATE_DEFAULT,
> > + USB3_STATE_ADDRESSED,
> > + USB3_STATE_CONFIGURED,
> > +} pcdstate_e;
>
> These structs also break the coding style completely.
> Will using IndustryStandard/Usb.h get rid of all of that?
>
OK
> > +
> > +typedef enum ep0_state {
> > + EP0_IDLE,
> > + EP0_IN_DATA_PHASE,
> > + EP0_OUT_DATA_PHASE,
> > + EP0_IN_WAIT_NRDY,
> > + EP0_OUT_WAIT_NRDY,
> > + EP0_IN_STATUS_PHASE,
> > + EP0_OUT_STATUS_PHASE,
> > + EP0_STALL,
> > +} ep0state_e;
> > +
> > +typedef struct usb3_dma_desc {
> > + /** Buffer Pointer - Low address quadlet */
> > + UINT32 bptl;
> > +
> > + /** Buffer Pointer - High address quadlet */
> > + UINT32 bpth;
> > +
> > + /** Status quadlet. Fields defined in enum @ref desc_sts_data. */
> > + UINT32 status;
> > +
> > + /** Control quadlet. Fields defined in enum @ref desc_ctl_data. */
> > + UINT32 control;
> > +} usb3_dma_desc_t;
> > +
> > +typedef struct usb3_pcd_req {
> > + usb3_dma_desc_t *trb;
> > + UINT64 trbdma;
> > +
> > + UINT32 length;
> > + UINT32 actual;
> > +
> > + UINT64 *bufdma;
> > + int (*complete)(unsigned actual, int status);
> > +} usb3_pcd_req_t;
> > +
> > +typedef struct usb_device_request {
> > + UINT8 bmRequestType;
> > + UINT8 bRequest;
> > + UINT16 wValue;
> > + UINT16 wIndex;
> > + UINT16 wLength;
> > +} usb_device_request_t;
> > +
> > +#pragma pack(1)
> > +/** USB_DT_DEVICE: Device descriptor */
> > +typedef struct usb_device_descriptor {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > +
> > + UINT16 bcdUSB;
> > +#define USB_CLASS_COMM 0x02
> > +#define USB_CLASS_VENDOR_SPEC 0xFF
> > +#define USB_SC_VENDOR_SPEC 0xFF
> > +#define USB_PR_VENDOR_SPEC 0xFF
> > + UINT8 bDeviceClass;
> > + UINT8 bDeviceSubClass;
> > + UINT8 bDeviceProtocol;
> > + UINT8 bMaxPacketSize0;
> > + UINT16 idVendor;
> > + UINT16 idProduct;
> > + UINT16 bcdDevice;
> > + UINT8 iManufacturer;
> > + UINT8 iProduct;
> > + UINT8 iSerialNumber;
> > + UINT8 bNumConfigurations;
> > +} usb_device_descriptor_t;
> > +
> > +/* USB_DT_CONFIG: Config descriptor */
> > +typedef struct usb_config_descriptor {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > +
> > + UINT16 wTotalLength;
> > + UINT8 bNumInterfaces;
> > +#define CONFIG_VALUE 1
> > + UINT8 bConfigurationValue;
> > + UINT8 iConfiguration;
> > +#define USB_CONFIG_ATT_ONE (1 << 7)
> > + UINT8 bmAttributes;
> > +#define USB_CONFIG_VBUS_DRAW (0xFA)
> > + UINT8 bMaxPower;
> > +} usb_config_descriptor_t;
> > +
> > +/* USB_DT_DEVICE_QUALIFIER: Device Qualifier descriptor */
> > +typedef struct usb_qualifier_descriptor {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > +
> > + UINT16 bcdUSB;
> > + UINT8 bDeviceClass;
> > + UINT8 bDeviceSubClass;
> > + UINT8 bDeviceProtocol;
> > + UINT8 bMaxPacketSize0;
> > + UINT8 bNumConfigurations;
> > + UINT8 bRESERVED;
> > +} usb_qualifier_descriptor_t;
> > +
> > +/* USB_DT_INTERFACE: Interface descriptor */
> > +typedef struct usb_interface_descriptor {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > +
> > + UINT8 bInterfaceNumber;
> > + UINT8 bAlternateSetting;
> > + UINT8 bNumEndpoints;
> > + UINT8 bInterfaceClass;
> > + UINT8 bInterfaceSubClass;
> > + UINT8 bInterfaceProtocol;
> > + UINT8 iInterface;
> > +} usb_interface_descriptor_t;
> > +
> > +/* USB_DT_ENDPOINT: Endpoint descriptor */
> > +typedef struct usb_endpoint_descriptor {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > +
> > + UINT8 bEndpointAddress;
> > + UINT8 bmAttributes;
> > +#define USB_ENDPOINT_XFER_CONTROL 0x00
> > +#define USB_ENDPOINT_XFER_ISOC 0x01
> > +#define USB_ENDPOINT_XFER_BULK 0x02
> > +#define USB_ENDPOINT_XFER_INT 0x03
> > + UINT16 wMaxPacketSize;
> > + UINT8 bInterval;
> > +} usb_endpoint_descriptor_t;
> > +
> > +/* USB_DT_SS_ENDPOINT_COMP: SuperSpeed Endpoint Companion descriptor */
> > +typedef struct usb_ss_ep_comp_descriptor {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > +
> > + UINT8 bMaxBurst;
> > + UINT8 bmAttributes;
> > + UINT16 wBytesPerInterval;
> > +} usb_ss_ep_comp_descriptor_t;
> > +
> > +/* WUSB BOS Descriptor (Binary device Object Store) */
> > +typedef struct wusb_bos_desc {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > + UINT16 wTotalLength;
> > + UINT8 bNumDeviceCaps;
> > +} wusb_bos_desc_t;
> > +
> > +#define USB_DEVICE_CAPABILITY_20_EXTENSION 0x02
> > +typedef struct usb_dev_cap_20_ext_desc {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > + UINT8 bDevCapabilityType;
> > +#define USB_20_EXT_LPM 0x02
> > + UINT32 bmAttributes;
> > +} usb_dev_cap_20_ext_desc_t;
> > +
> > +#define USB_DEVICE_CAPABILITY_SS_USB 0x03
> > +typedef struct usb_dev_cap_ss_usb {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > + UINT8 bDevCapabilityType;
> > +#define USB_DC_SS_USB_LTM_CAPABLE 0x02
> > + UINT8 bmAttributes;
> > +#define USB_DC_SS_USB_SPEED_SUPPORT_LOW 0x01
> > +#define USB_DC_SS_USB_SPEED_SUPPORT_FULL 0x02
> > +#define USB_DC_SS_USB_SPEED_SUPPORT_HIGH 0x04
> > +#define USB_DC_SS_USB_SPEED_SUPPORT_SS 0x08
> > + UINT32 wSpeedsSupported;
> > + UINT8 bFunctionalitySupport;
> > + UINT8 bU1DevExitLat;
> > + UINT32 wU2DevExitLat;
> > +} usb_dev_cap_ss_usb_t;
> > +
> > +#define USB_DEVICE_CAPABILITY_CONTAINER_ID 0x04
> > +typedef struct usb_dev_cap_container_id {
> > + UINT8 bLength;
> > + UINT8 bDescriptorType;
> > + UINT8 bDevCapabilityType;
> > + UINT8 bReserved;
> > + UINT8 containerID[16];
> > +} usb_dev_cap_container_id_t;
> > +#pragma pack()
> > +
> > +typedef union usb_setup_pkt {
> > + usb_device_request_t req;
> > + UINT32 d32[2];
> > + UINT8 d8[8];
> > +} usb_setup_pkt_t;
> > +
> > +typedef struct usb3_pcd_ep {
> > + struct usb3_pcd *pcd;
> > +
> > + UINT8 EpInIdx;
> > + UINT8 EpOutIdx;
> > + UINT8 phys;
> > +
> > + //UINT8 phys;
> > + UINT8 num;
> > + UINT8 type;
> > + UINT8 maxburst;
> > + UINT16 maxpacket;
> > + /* Tx FIFO # for IN EPs */
> > + UINT8 tx_fifo_num;
> > +
> > + /* The Transfer Resource Index from the Start Transfer command */
> > + UINT8 tri_out;
> > + UINT8 tri_in;
> > +
> > + UINT8 stopped;
> > + /* Send ZLP */
> > + UINT8 send_zlp;
> > + /* True if 3-stage control transfer */
> > + UINT8 three_stage;
> > + /* True if transfer has been started on EP */
> > + UINT8 xfer_started;
> > + /* EP direction 0 = OUT */
> > + UINT8 is_in;
> > + /* True if endpoint is active */
> > + UINT8 active;
> > + /* Initial data pid of bulk endpoint */
> > + UINT8 data_pid_start;
> > +
> > + /* ep_desc (excluding ep0) */
> > + usb3_dma_desc_t *ep_desc;
> > +
> > +#if 0
> > + /* TRB descriptor must be aligned to 16 bytes */
> > + UINT8 epx_desc[32];
> > +#endif
> > +
> > + /* request (excluding ep0) */
> > + usb3_pcd_req_t req;
> > +} usb3_pcd_ep_t;
> > +
> > +typedef struct usb3_pcd {
> > + //struct usb3_device *usb3_dev;
> > +
> > + INT32 link_state;
> > + pcdstate_e state;
> > + UINT8 new_config;
> > + ep0state_e ep0state;
> > +
> > + UINT32 eps_enabled;
> > + UINT32 ltm_enable;
> > +
> > + usb3_pcd_ep_t ep0;
> > + usb3_pcd_ep_t out_ep;
> > + usb3_pcd_ep_t in_ep;
> > +
> > + /*
> > + usb3_dev_global_regs_t *dev_global_regs;
> > + usb3_dev_ep_regs_t *out_ep_regs;
> > + usb3_dev_ep_regs_t *in_ep_regs;
> > + */
> > +
> > + usb3_pcd_req_t ep0_req;
> > +
> > + UINT8 speed;
> > +
> > + usb3_dma_desc_t *ep0_setup_desc;
> > + usb3_dma_desc_t *ep0_in_desc;
> > + usb3_dma_desc_t *ep0_out_desc;
> > +
> > + /* TRB descriptor must be aligned to 16 bytes */
> > +#if 0
> > + UINT8 ep0_setup[32];
> > + UINT8 ep0_in[32];
> > + UINT8 ep0_out[32];
> > +
> > + usb_setup_pkt_t ep0_setup_pkt[5];
> > +
> > +#define USB3_STATUS_BUF_SIZE 512
> > + UINT8 ep0_status_buf[USB3_STATUS_BUF_SIZE];
> > +
> > +#define USB3_BULK_BUF_SIZE 2048
> > + UINT8 ss_bulk_buf[USB3_BULK_BUF_SIZE];
> > +#endif
> > +
> > + UINT32 file_type;
> > + UINT32 file_address;
> > + UINT32 file_capacity;
> > + UINT32 file_total_frame;
> > + UINT32 file_curr_frame;
> > + UINT32 file_next_frame;
> > + UINT32 file_received;
> > + UINT32 file_complete;
> > +
> > + UINT16 test_mode_nr;
> > + UINT16 test_mode;
> > +} usb3_pcd_t;
> > +
> > +struct usb_enum_port_param {
> > + UINT16 idVendor;
> > + UINT16 idProduct;
> > + UINT8 bInterfaceSubClass;
> > + UINT8 bInterfaceProtocol;
> > +};
> > +
> > +#if 0
>
> If not used, leave it out.
>
OK
> > +typedef struct usb3_pcd_req {
> > + usb3_dma_desc_t *trb;
> > + UINT64 trbdma;
> > +
> > + UINT32 length;
> > + UINT32 actual;
> > +
> > + UINT64 *bufdma;
> > + int (*complete)(unsigned actual, int status);
> > +} usb3_pcd_req_t;
> > +
> > +#endif
> > +
> > +#endif /* __DW_USB3_DXE_H__ */
> > diff --git a/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.c b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.c
> > new file mode 100644
> > index 000000000000..83d5e4736de0
> > --- /dev/null
> > +++ b/EmbeddedPkg/Drivers/DwUsb3Dxe/DwUsb3Dxe.c
> > @@ -0,0 +1,2434 @@
> > +/** @file
> > +
> > + Copyright (c) 2018, Linaro Limited. All rights reserved.
> > +
> > + This program and the accompanying materials are licensed and made available
> > + under the terms and conditions of the BSD License which accompanies this
> > + distribution. The full text of the license may be found at
> > + http://opensource.org/licenses/bsd-license.php
> > +
> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#include <IndustryStandard/Usb.h>
> > +#include <Library/ArmLib.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/CacheMaintenanceLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/DmaLib.h>
> > +#include <Library/IoLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/TimerLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiDriverEntryPoint.h>
> > +#include <Library/UefiRuntimeServicesTableLib.h>
> > +#include <Protocol/DwUsb.h>
> > +#include <Protocol/DriverBinding.h>
> > +#include <Protocol/UsbDevice.h>
> > +
> > +#include "DwUsb3Dxe.h"
> > +
> > +#define FIFO_DIR_TX 0
> > +#define FIFO_DIR_RX 1
> > +
> > +#define TX_FIFO_ADDR 0
> > +#define RX_FIFO_ADDR 0
> > +
> > +#define RAM_WIDTH 8
> > +#define RAM_TX0_DEPTH 2048
> > +#define RAM_TX1_DEPTH 4096
> > +#define RAM_RX_DEPTH 8192
>
> Should these not also go into DwUsb3Dxe.h?
>
OK
> > +
> > +#define USB_TYPE_LENGTH 16
> > +#define USB_BLOCK_HIGH_SPEED_SIZE 512
> > +#define DATA_SIZE 131072
> > +#define CMD_SIZE 512
>
> What do the above definitions mean?
>
I'll add comment.
> > +#define MATCH_CMD_LITERAL(Cmd, Buf) !AsciiStrnCmp (Cmd, Buf, sizeof (Cmd) - 1)
>
> This one does not appear to be used?
>
OK
> > +
> > +#define ALIGN(x, a) (((x) + ((a) - 1)) & ~((a) - 1))
>
> Use available macros in Base.h.
>
OK
> > +
> > +//
> > +// The time between interrupt polls, in units of 100 nanoseconds
> > +// 10 Microseconds
> > +//
> > +#define DW_INTERRUPT_POLL_PERIOD 100
> > +
> > +#define DWUSB3_EVENT_BUF_SIZE 256
> > +
> > +//
> > +// Maxpacket size for EP0, defined by USB3 spec
> > +//
> > +#define USB3_MAX_EP0_SIZE 512
> > +
> > +//
> > +// Maxpacket size for any EP, defined by USB3 spec
> > +//
> > +#define USB3_MAX_PACKET_SIZE 1024
> > +#define USB2_HS_MAX_PACKET_SIZE 512
> > +#define USB2_FS_MAX_PACKET_SIZE 64
> > +
> > +#define USB3_STATE_UNCONNECTED 0
> > +#define USB3_STATE_DEFAULT 1
> > +#define USB3_STATE_ADDRESSED 2
> > +#define USB3_STATE_CONFIGURED 3
> > +
> > +#define USB3_STATUS_BUF_SIZE 512
>
> If these are defined by USB3 spec, please add them to
> MdePkg/Include/IndustryStandard/Usb.h instead of defining them
> locally.
>
OK
> > +
> > +#define GET_EVENTBUF_COUNT() \
> > + (GEVNTCOUNT_EVNTCOUNT (MmioRead32 (GEVNTCOUNT (0))))
> > +#define UPDATE_EVENTBUF_COUNT(x) \
> > + (MmioWrite32 (GEVNTCOUNT (0), GEVNTCOUNT_EVNTCOUNT (x)))
> > +
> > +#define SET_DEVADDR(x) \
> > + (MmioAndThenOr32 (DCFG, ~DCFG_DEVADDR_MASK, DCFG_DEVADDR (x)))
>
> Should these not also move to DwUsb3Dxe.h?
>
OK
> > +
> > +EFI_GUID gDwUsbProtocolGuid = DW_USB_PROTOCOL_GUID;
>
> This looks less than ideal - this makes it impossible to include
> DwUsb2 and DwUsb3 drivers in the same image.
>
On the view of hardware, DwUsb2 and DwUsb3 doesn't exist at the same time.
> > +
> > +STATIC DW_USB_PROTOCOL *DwUsb;
> > +
> > +STATIC usb3_pcd_t gPcd;
>
> This seems a very unfortunate choice for variable name.
>
OK. I'll rename it.
> > +STATIC UINT32 *gEventBuf, *gEventPtr;
> > +STATIC struct usb_device_descriptor gDwUsb3DevDesc;
> > +STATIC VOID *gRxBuf;
> > +
> > +STATIC usb_setup_pkt_t *gEndPoint0SetupPacket;
> > +STATIC UINT8 *gEndPoint0StatusBuf;
>
> Although most if not all of these will disappear when converting to
> UEFI driver model, why do they have 'g' prefix in the first place?
> 'g' is for globally visible symbols. Variables that are "global" in
> this file only are better off using 'm'.
OK. I'll fix it.
>
> > +STATIC USB_DEVICE_RX_CALLBACK mDataReceivedCallback;
> > +STATIC UINTN mDataBufferSize;
> > +
> > +struct usb_interface_descriptor intf = {
> > + sizeof (struct usb_interface_descriptor),
> > + UDESC_INTERFACE,
> > + 0,
> > + 0,
> > + 2,
> > + USB_CLASS_VENDOR_SPEC,
> > + 0x42,
> > + 0x03,
> > + 0
> > +};
> > +
> > +const struct usb_ss_ep_comp_descriptor ep_comp = {
> > + sizeof (struct usb_ss_ep_comp_descriptor),
> > + UDESC_SS_USB_COMPANION,
> > + 0,
> > + 0,
> > + 0
> > +};
> > +
> > +const struct usb_endpoint_descriptor hs_bulk_in = {
> > + sizeof (struct usb_endpoint_descriptor),
> > + UDESC_ENDPOINT,
> > + UE_DIR_IN | USB3_BULK_IN_EP,
> > + USB_ENDPOINT_XFER_BULK,
> > + 0x200,
> > + 0
> > +};
> > +
> > +const struct usb_endpoint_descriptor
> > +hs_bulk_out = {
> > + sizeof(struct usb_endpoint_descriptor), /* bLength */
> > + UDESC_ENDPOINT, /* bDescriptorType */
> > +
> > + UE_DIR_OUT | USB3_BULK_OUT_EP, /* bEndpointAddress */
> > + USB_ENDPOINT_XFER_BULK, /* bmAttributes */
> > + 0x200, /* wMaxPacketSize: 512 of high-speed */
> > + 1, /* bInterval */
> > +};
> > +
> > +const struct usb_endpoint_descriptor ss_bulk_in = {
> > + sizeof(struct usb_endpoint_descriptor), /* bLength */
> > + UDESC_ENDPOINT, /* bDescriptorType */
> > +
> > + UE_DIR_IN | USB3_BULK_IN_EP, /* bEndpointAddress */
> > + USB_ENDPOINT_XFER_BULK, /* bmAttributes */
> > + 0x400, /* wMaxPacketSize: 1024 of super-speed */
> > + 0, /* bInterval */
> > +};
> > +
> > +const struct usb_endpoint_descriptor ss_bulk_out = {
> > + sizeof(struct usb_endpoint_descriptor), /* bLength */
> > + UDESC_ENDPOINT, /* bDescriptorType */
> > +
> > + UE_DIR_OUT | USB3_BULK_OUT_EP, /* bEndpointAddress */
> > + USB_ENDPOINT_XFER_BULK, /* bmAttributes */
> > + 0x400, /* wMaxPacketSize: 1024 of super-speed */
> > + 0, /* bInterval */
> > +};
> > +
> > +/** The BOS Descriptor */
>
> What's a BOS?
>
Let me add comment on it.
> > +
> > +const struct usb_dev_cap_20_ext_desc cap1 = {
> > + sizeof(struct usb_dev_cap_20_ext_desc), /* bLength */
> > + UDESC_DEVICE_CAPABILITY, /* bDescriptorType */
> > + USB_DEVICE_CAPABILITY_20_EXTENSION, /* bDevCapabilityType */
> > + 0x2, /* bmAttributes */
> > +};
> > +
> > +const struct usb_dev_cap_ss_usb
> > +cap2 = {
> > + sizeof(struct usb_dev_cap_ss_usb), /* bLength */
> > + UDESC_DEVICE_CAPABILITY, /* bDescriptorType */
> > + USB_DEVICE_CAPABILITY_SS_USB, /* bDevCapabilityType */
> > + 0x0, /* bmAttributes */
> > + (USB_DC_SS_USB_SPEED_SUPPORT_SS |
> > + USB_DC_SS_USB_SPEED_SUPPORT_HIGH), /* wSpeedsSupported */
> > + 0x2, /* bFunctionalitySupport */
> > + 0xa, /* bU1DevExitLat */
> > + 0x100, /* wU2DevExitLat */
> > +};
> > +
> > +const struct usb_dev_cap_container_id
> > +cap3 = {
> > + sizeof(struct usb_dev_cap_container_id),/* bLength */
> > + UDESC_DEVICE_CAPABILITY, /* bDescriptorType */
> > + USB_DEVICE_CAPABILITY_CONTAINER_ID, /* bDevCapabilityType */
> > + 0, /* bReserved */
> > + {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* containerID */
> > +};
> > +
> > +const struct wusb_bos_desc
> > +bos = {
> > + sizeof(struct wusb_bos_desc), /* bLength */
> > + UDESC_BOS, /* bDescriptorType */
> > + (sizeof(struct wusb_bos_desc) + \
> > + sizeof(cap1) + sizeof(cap2) + \
> > + sizeof(cap3)), /* wTotalLength */
> > + 3, /* bNumDeviceCaps */
> > +};
> > +
> > +STATIC struct usb_enum_port_param usb_port_activity_config = {
> > + .idVendor = USB_ENUM_ADB_PORT_VID,
> > + .idProduct = USB_ENUM_ADB_PORT_PID,
> > + .bInterfaceSubClass = USB_ENUM_INTERFACE_ADB_SUBCLASS,
> > + .bInterfaceProtocol = USB_ENUM_INTERFACE_ADB_PROTOCOL
> > +};
> > +
> > +STATIC
> > +UINT32
> > +DwUsb3GetEventBufEvent (
> > + IN UINTN Size
> > + )
> > +{
> > + UINT32 Event;
> > +
> > + Event = *gEventPtr++;
> > + if ((UINT32)(UINTN)gEventPtr >= (UINT32)(UINTN)gEventBuf + Size) {
> > + gEventPtr = gEventBuf;
> > + }
> > + return Event;
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3SetFifoSize (
> > + IN UINT32 Addr,
> > + IN UINT32 Depth,
> > + IN UINT32 Dir,
> > + IN UINT32 FifoNum
> > + )
> > +{
> > + UINT32 Reg = 0;
> > +
> > + if (Dir == FIFO_DIR_TX) {
> > + Reg = GTXFIFOSIZ (FifoNum);
> > + } else if (Dir == FIFO_DIR_RX) {
> > + Reg = GRXFIFOSIZ (FifoNum);
> > + } else {
> > + ASSERT (0);
> > + }
> > + MmioWrite32 (Reg, FIFOSIZ_DEP (Depth) | FIFOSIZ_ADDR (Addr));
> > +}
> > +
> > +STATIC
> > +UINT32
> > +Handshake (
>
> What does this function do? "Handshake" does not tell me enough to
> figure out.
>
> > + IN UINT32 Reg,
> > + IN UINT32 Mask,
> > + IN UINT32 Done
> > + )
> > +{
> > + UINT32 Timeout = 100000;
> > +
> > + do {
> > + if ((MmioRead32 (Reg) & Mask) == Done) {
> > + return 1;
> > + }
> > + MicroSecondDelay (1);
> > + } while (Timeout-- > 0);
> > + return 0;
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3FillDesc (
> > + IN usb3_dma_desc_t *desc,
> > + IN UINT64 dma_addr,
> > + IN UINT32 dma_len,
> > + IN UINT32 stream,
> > + IN UINT32 type,
> > + IN UINT32 ctrlbits,
> > + IN UINT32 own
> > + )
> > +{
> > + desc->bptl = (UINT32)(dma_addr & 0xFFFFFFFF);
> > + desc->bpth = (UINT32)(dma_addr >> 32);
>
> Could you possibly create LOW_32BIT and HIGH_32BIT macros for
> MdePkg/Base.h instead? We are already duplicating these operations all
> over the place. Including all over MdeModulePkg.
>
OK
> > + desc->status = DSCSTS_XFERCNT (dma_len);
> > + if (type) {
> > + desc->control = DSCCTL_TRBCTL (type);
> > + }
> > + desc->control |= DSCCTL_STRMID_SOFN (stream) | ctrlbits;
> > + ArmDataSynchronizationBarrier ();
>
> Please use MemoryFence() instead.
> This controller is not ARM-specific.
>
> (DSB? That sounds excessive. DMB should suffice?)
>
OK
> > + /* must execute this operation at last */
> > + if (own) {
> > + desc->control |= DSCCTL_HWO;
> > + }
> > + ArmDataSynchronizationBarrier ();
>
> As above.
>
OK
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3DepStartNewCfg (
> > + IN UINT32 EpIdx,
> > + IN UINT32 RsrcIdx
> > + )
> > +{
> > + /* start the command */
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_XFER_RSRC_IDX (RsrcIdx) | DEPCMD_CMDTYPE (CMDTYPE_START_NEW_CFG) | \
> > + DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3DepCfg (
>
> Please no abbreviations in function names. This name tells me nothing
> about what the function does (and there is no comment describing its
> function).
>
OK
> > + IN UINT32 EpIdx,
> > + IN UINT32 DepCfg0,
> > + IN UINT32 DepCfg1,
> > + IN UINT32 DepCfg2
> > + )
> > +{
> > + MmioWrite32 (DEPCMDPAR2 (EpIdx), DepCfg2);
> > + MmioWrite32 (DEPCMDPAR1 (EpIdx), DepCfg1);
> > + MmioWrite32 (DEPCMDPAR0 (EpIdx), DepCfg0);
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_CMDTYPE (CMDTYPE_SET_EP_CFG) | DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3DepXferCfg (
>
> Dep<something>TransferConfig.
> Please address throughout.
>
> > + IN UINT32 EpIdx,
> > + IN UINT32 DepStrmCfg
> > + )
> > +{
> > + MmioWrite32 (DEPCMDPAR0 (EpIdx), DepStrmCfg);
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_CMDTYPE (CMDTYPE_SET_XFER_CFG) | DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > +}
> > +
> > +STATIC
> > +UINT8
> > +DwUsb3DepStartXfer (
> > + IN UINT32 EpIdx,
> > + IN UINT64 DmaAddr,
> > + IN UINT32 StreamOrUf
> > + )
> > +{
> > + UINT32 Data;
> > +
> > + MmioWrite32 (DEPCMDPAR1 (EpIdx), (UINT32)DmaAddr);
> > + MmioWrite32 (DEPCMDPAR0 (EpIdx), (UINT32)(DmaAddr >> 32));
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_STR_NUM_OR_UF (StreamOrUf) | DEPCMD_CMDTYPE (CMDTYPE_START_XFER) | \
> > + DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > + Data = MmioRead32 (DEPCMD (EpIdx));
> > + return GET_DEPCMD_XFER_RSRC_IDX(Data);
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3DepStopXfer (
> > + IN UINT32 EpIdx,
> > + IN UINT32 Tri
> > + )
> > +{
> > + MmioWrite32 (DEPCMDPAR2 (EpIdx), 0);
> > + MmioWrite32 (DEPCMDPAR1 (EpIdx), 0);
> > + MmioWrite32 (DEPCMDPAR0 (EpIdx), 0);
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_XFER_RSRC_IDX (Tri) | DEPCMD_CMDTYPE (CMDTYPE_END_XFER) | \
> > + DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > +}
> > +
> > +VOID
> > +DwUsb3DepUpdateXfer (
> > + IN UINT32 EpIdx,
> > + IN UINT32 Tri
> > + )
> > +{
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_XFER_RSRC_IDX (Tri) | DEPCMD_CMDTYPE (CMDTYPE_UPDATE_XFER) | \
> > + DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3DepClearStall (
> > + IN UINTN EpIdx
> > + )
> > +{
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_CMDTYPE (CMDTYPE_CLR_STALL) | DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > +}
> > +
> > +
> > +STATIC
> > +VOID
> > +DwUsb3DepSetStall (
> > + IN UINTN EpIdx
> > + )
> > +{
> > + MmioWrite32 (
> > + DEPCMD (EpIdx),
> > + DEPCMD_CMDTYPE (CMDTYPE_SET_STALL) | DEPCMD_CMDACT
> > + );
> > + Handshake (DEPCMD (EpIdx), DEPCMD_CMDACT, 0);
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3EnableEp (
>
> EnableEndpoint
>
OK
> > + IN UINT32 EpIdx,
>
> EndpointIndex.
>
OK
> > + IN usb3_pcd_ep_t *ep
>
> Endpoint.
>
OK
> > + )
> > +{
> > + UINT32 Dalepena;
>
> What?
>
> > +
> > + Dalepena = MmioRead32 (DALEPENA);
> > + /* If the EP is already enabled, skip to set it again. */
>
> endpoint
>
> > + if (Dalepena & (1 << EpIdx)) {
> > + return;
> > + }
> > + Dalepena |= 1 << EpIdx;
> > + MmioWrite32 (DALEPENA, Dalepena);
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3Ep0Activate (
>
> (EP0 is special, so may not need expanding.)
>
> > + IN OUT usb3_pcd_t *pcd
> > + )
> > +{
> > + /* issue DEPCFG command to EP0 OUT */
>
> ... except for in comments.
>
> > + DwUsb3DepStartNewCfg (EP_OUT_IDX (0), 0);
> > + DwUsb3DepCfg (
> > + EP_OUT_IDX (0),
> > + EPCFG0_EPTYPE (EPTYPE_CONTROL) | EPCFG0_MPS (512),
> > + EPCFG1_XFER_CMPL | EPCFG1_XFER_NRDY,
> > + 0
> > + );
> > + /* issue DEPSTRMCFG command to EP0 OUT */
> > + DwUsb3DepXferCfg (EP_OUT_IDX (0), 1); // one stream
> > + /* issue DEPCFG command to EP0 IN */
> > + DwUsb3DepCfg (
> > + EP_IN_IDX (0),
> > + EPCFG0_EPTYPE (EPTYPE_CONTROL) | EPCFG0_MPS (512) | \
> > + EPCFG0_TXFNUM (pcd->ep0.tx_fifo_num),
> > + EPCFG1_XFER_NRDY | EPCFG1_XFER_CMPL | EPCFG1_EP_DIR_IN,
> > + 0
> > + );
> > + /* issue DEPSTRMCFG command to EP0 IN */
> > + DwUsb3DepXferCfg (EP_IN_IDX (0), 1); // one stream
> > + pcd->ep0.active = 1;
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3EpActivate (
> > + IN OUT usb3_pcd_t *pcd,
> > + IN OUT usb3_pcd_ep_t *ep
> > + )
> > +{
> > + UINT32 EpIdx, DepCfg0, DepCfg1;
> > + if (ep->is_in) {
> > + EpIdx = EP_IN_IDX (ep->num);
> > + } else {
> > + EpIdx = EP_OUT_IDX (ep->num);
> > + }
> > +
> > + /* Start a new configurate when enable the first EP. */
> > + if (!pcd->eps_enabled) {
> > + pcd->eps_enabled = 1;
> > + /* Issue DEPCFG command to physical EP1 (logical EP0 IN) first.
> > + * It resets the core's Tx FIFO mapping table.
> > + */
> > + DepCfg0 = EPCFG0_EPTYPE (EPTYPE_CONTROL);
> > + DepCfg0 |= EPCFG0_CFG_ACTION (CFG_ACTION_MODIFY);
> > + DepCfg1 = EPCFG1_XFER_CMPL | EPCFG1_XFER_NRDY | EPCFG1_EP_DIR_IN;
> > +
> > + switch (pcd->speed) {
> > + case USB_SPEED_SUPER:
> > + DepCfg0 |= EPCFG0_MPS (512);
> > + break;
> > + case USB_SPEED_HIGH:
> > + case USB_SPEED_FULL:
> > + DepCfg0 |= EPCFG0_MPS (64);
> > + break;
> > + case USB_SPEED_LOW:
> > + DepCfg0 |= EPCFG0_MPS (8);
>
> No live coding of values.
> Add #defines for those speed setting values.
>
> > + break;
> > + default:
> > + ASSERT (0);
> > + break;
> > + }
> > + DwUsb3DepCfg (EP_IN_IDX (0), DepCfg0, DepCfg1, 0);
>
> What does that final 0 do?
>
> > + DwUsb3DepStartNewCfg (EP_OUT_IDX (0), 2);
>
> And that 2?
>
> Please address throughout.
>
> > + }
> > + /* issue DEPCFG command to EP */
> > + DepCfg0 = EPCFG0_EPTYPE (ep->type);
> > + DepCfg0 |= EPCFG0_MPS (ep->maxpacket);
> > + if (ep->is_in) {
> > + DepCfg0 |= EPCFG0_TXFNUM (ep->tx_fifo_num);
> > + }
> > + DepCfg0 |= EPCFG0_BRSTSIZ (ep->maxburst);
> > + DepCfg1 = EPCFG1_EP_NUM (ep->num);
> > + if (ep->is_in) {
> > + DepCfg1 |= EPCFG1_EP_DIR_IN;
> > + } else {
> > + DepCfg1 |= EPCFG1_XFER_CMPL;
> > + }
> > + DwUsb3DepCfg (EpIdx, DepCfg0, DepCfg1, 0);
> > + /* issue DEPSTRMCFG command to EP */
> > + DwUsb3DepXferCfg (EpIdx, 1);
> > + DwUsb3EnableEp (EpIdx, ep);
> > + ep->active = 1;
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3Ep0OutStart (
> > + IN usb3_pcd_t *pcd
> > + )
> > +{
> > + usb3_dma_desc_t *desc;
> > +
> > + /* Get the SETUP packet DMA Descriptor (TRB) */
> > + desc = pcd->ep0_setup_desc;
> > +
> > + /* DMA Descriptor setup */
> > + DwUsb3FillDesc (
> > + desc,
> > + (UINT64)gEndPoint0SetupPacket,
> > + pcd->ep0.maxpacket,
> > + 0,
> > + TRBCTL_SETUP,
> > + DSCCTL_IOC | DSCCTL_ISP | DSCCTL_LST,
> > + 1
> > + );
> > +
> > + /* issue DEPSTRTXFER command to EP0 OUT */
> > + pcd->ep0.tri_out = DwUsb3DepStartXfer (EP_OUT_IDX (0), (UINT64)desc, 0);
> > +}
> > +
> > +STATIC
> > +VOID
> > +DwUsb3Init (
> > + VOID
> > + )
> > +{
> > + UINT32 Data, Addr;
>
> Address.
>
> > + usb3_pcd_t *pcd = &gPcd;
> > +
> > + /* soft reset the usb core */
> > + do {
> > + MmioAndThenOr32 (DCTL, ~DCTL_RUN_STOP, DCTL_CSFTRST);
> > +
> > + do {
> > + MicroSecondDelay (1000);
> > + Data = MmioRead32 (DCTL);
> > + } while (Data & DCTL_CSFTRST);
> > + //
> > + // wait for at least 3 PHY clocks
> > + //
> > + MicroSecondDelay (1000);
> > + } while (0);
> > +
> > + pcd->link_state = 0;
> > +
> > + /* TI PHY: Set Turnaround Time = 9 (8-bit UTMI+ / ULPI) */
> > + MmioAndThenOr32 (
> > + GUSB2PHYCFG (0),
> > + ~GUSB2PHYCFG_USBTRDTIM_MASK,
> > + GUSB2PHYCFG_USBTRDTIM (9)
>
> How long is 9?
>
> I will stop my review here and give a full review on v2 when the
> overarching comments have been addressed.
>
OK
Best Regards
Haojian
next prev parent reply other threads:[~2018-10-22 2:51 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-20 10:31 [PATCH v1 0/2] add DwUsb3Dxe driver Haojian Zhuang
2018-08-20 10:31 ` [PATCH v1 1/2] EmbeddedPkg: add DwUsb protocol Haojian Zhuang
2018-10-04 14:49 ` Leif Lindholm
2018-10-22 2:39 ` Haojian Zhuang
2018-08-20 10:31 ` [PATCH v1 2/2] EmbeddedPkg/Drivers: add DwUsb3Dxe driver Haojian Zhuang
2018-10-04 16:32 ` Leif Lindholm
2018-10-22 2:51 ` Haojian Zhuang [this message]
2018-10-04 14:15 ` [PATCH v1 0/2] " Leif Lindholm
2018-10-22 2:09 ` Haojian Zhuang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAD6h2NRZ898Cp3L59yuyiKt0b7R++uypXxKbpvRAqNfEiyW9sQ@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox