On Fri, 5 Oct 2018 at 23:46, Leif Lindholm <leif.lindh...@linaro.org> wrote: > > On Tue, Aug 21, 2018 at 07:35:13PM +0800, Haojian Zhuang wrote: > > Add Designware USB 2.0 device driver that is used on HiKey platform. > > > > Cc: Leif Lindholm <leif.lindh...@linaro.org> > > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Haojian Zhuang <haojian.zhu...@linaro.org> > > --- > > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec | 45 + > > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf | 52 ++ > > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.h | 655 ++++++++++++++ > > EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.c | 912 ++++++++++++++++++++ > > 4 files changed, 1664 insertions(+) > > Could it be renamed DwUsb2? This seems to match how Synopsys > themselves refer to it, and what the Linux driver is called. > OK
> Other than that, same comments as for DwUsb3Dxe - please move it to > edk2-platforms and convert it to UEFI driver model with > NonDiscoverableDeviceRegistrationLib. UsbDevice isn't supported by NonDiscoverableDeviceRegistrationLib. Could I add a new type? > > Hmm, it also looks to me like there are plenty of things here > hardcoded for the use as a device for fastboot. I don't object to > that being the only support submitted, you made it clear when you > posted it. But the code is completely geared towards this, and I feel > if someone comes along and want to add the functionality to run it in > host mode. I hope to support device first. > > I expect I will find the same when I look at the reworked version of > DwUsb3Dxe. > > Is there anything you can do to break out the generic device > configuration bits from the bits that assume there are two endpoints > and they are being used for fastboot? Let me investigate. > > > > > diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec > > b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec > > new file mode 100644 > > index 000000000000..7eb65e498c04 > > --- /dev/null > > +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec > > @@ -0,0 +1,45 @@ > > +#/** @file > > +# Framework Module Development Environment Industry Standards > > +# > > +# 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> > > +# Copyright (c) 2018, Linaro. All rights reserved.<BR> > > Same comments as for DwUsb3 - please merge these two into a common one > for both drivers (if still needed). > > > +# > > +# 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 = DwUsbDxePkg > > + PACKAGE_GUID = 114a3be9-10f7-4bf1-81ca-09ac52d4c3d5 > > + 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] > > + gDwUsbDxeTokenSpaceGuid = { 0x131c4d02, 0x9449, 0x4ee9, { 0xba, > > 0x3d, 0x69, 0x50, 0x21, 0x89, 0x26, 0x0b }} > > + > > +[Protocols.common] > > + gDwUsbProtocolGuid = { 0x109fa264, 0x7811, 0x4862, { 0xa9, > > 0x73, 0x4a, 0xb2, 0xef, 0x2e, 0xe2, 0xff }} > > + > > +[PcdsFixedAtBuild.common] > > + # DwUsb Driver PCDs > > + gDwUsbDxeTokenSpaceGuid.PcdDwUsbDxeBaseAddress|0x0|UINT32|0x00000001 > > + gDwUsbDxeTokenSpaceGuid.PcdSysCtrlBaseAddress|0x0|UINT32|0x00000002 > > I don't see PcdSysCtrlBaseAddress used anywhere in this patch? It also > doesn't sound like something that should be an aspect of the USB > controller driver. > Yes, PcdSysCtrlBaseAddress isn't used. We could remove it now. > > diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf > > b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf > > new file mode 100644 > > index 000000000000..56d518c27c32 > > --- /dev/null > > +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.inf > > @@ -0,0 +1,52 @@ > > +#/** @file > > +# > > +# Copyright (c) 2018, Linaro. 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 = DwUsbDxe > > + FILE_GUID = 72d78ea6-4dee-11e3-8100-f3842a48d0a0 > > + MODULE_TYPE = UEFI_DRIVER > > + VERSION_STRING = 1.0 > > + ENTRY_POINT = DwUsbEntryPoint > > + > > +[Sources.common] > > + DwUsbDxe.c > > + > > +[LibraryClasses] > > + ArmLib > > This is not an ARM-specific device, so it should not need ArmLib to build. > Because I used ArmDataSynchronizationBarrier(). Do we have the common function on it? > > + CacheMaintenanceLib > > + DebugLib > > + DmaLib > > + IoLib > > + MemoryAllocationLib > > + TimerLib > > + UefiBootServicesTableLib > > + UefiDriverEntryPoint > > + > > +[Protocols] > > + gEfiDriverBindingProtocolGuid > > + gUsbDeviceProtocolGuid > > + > > +[Packages] > > + ArmPkg/ArmPkg.dec > > Hmm? > > > + EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.dec > > + EmbeddedPkg/EmbeddedPkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > + MdePkg/MdePkg.dec > > + > > +[Pcd] > > + gDwUsbDxeTokenSpaceGuid.PcdDwUsbDxeBaseAddress > > + > > +[Depex] > > + TRUE > > diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.h > > b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.h > > new file mode 100644 > > index 000000000000..1595e09a92a4 > > --- /dev/null > > +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.h > > @@ -0,0 +1,655 @@ > > +/** @file > > + > > + Copyright (c) 2018, Linaro. 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_USB_DXE_H__ > > +#define __DW_USB_DXE_H__ > > Preferably expand all DW_ to DESIGNWARE_. OK > > > + > > +#define DW_USB_BASE FixedPcdGet32 > > (PcdDwUsbDxeBaseAddress) > > + > > +#define HPTXFSIZ 0x100 > > +#define DIEPTXF(x) (0x100 + 4 * (x)) > > Why 4 *? > Is this a sizeof (UINT32)? Yes, it's a sizeof (UINT32). > > > +#define DIEPTXF1 0x104 > > +#define DIEPTXF2 0x108 > > +#define DIEPTXF3 0x10C > > +#define DIEPTXF4 0x110 > > +#define DIEPTXF5 0x114 > > +#define DIEPTXF6 0x118 > > +#define DIEPTXF7 0x11C > > +#define DIEPTXF8 0x120 > > +#define DIEPTXF9 0x124 > > +#define DIEPTXF10 0x128 > > +#define DIEPTXF11 0x12C > > +#define DIEPTXF12 0x130 > > +#define DIEPTXF13 0x134 > > +#define DIEPTXF14 0x138 > > +#define DIEPTXF15 0x13C > > + > > +/*** HOST MODE REGISTERS ***/ > > +/* Host Global Registers */ > > +#define HCFG 0x400 > > +#define HFIR 0x404 > > +#define HFNUM 0x408 > > +#define HPTXSTS 0x410 > > +#define HAINT 0x414 > > +#define HAINTMSK 0x418 > > + > > +/* Host Port Control and Status Registers */ > > +#define HPRT 0x440 > > + > > +/* Host Channel-Specific Registers */ > > +#define HCCHAR(x) (0x500 + 0x20 * (x)) > > +#define HCSPLT(x) (0x504 + 0x20 * (x)) > > +#define HCINT(x) (0x508 + 0x20 * (x)) > > +#define HCINTMSK(x) (0x50C + 0x20 * (x)) > > +#define HCTSIZ(x) (0x510 + 0x20 * (x)) > > +#define HCDMA(x) (0x514 + 0x20 * (x)) > > Why 0x20 *? > CHANNEL_REGISTER_BANK_SIZE? Yes. I'll add a new definition of CHANNEL_REGISTER_BANK_SIZE. > > > +#define HCCHAR15 0x6E0 > > +#define HCSPLT15 0x6E4 > > +#define HCINT15 0x6E8 > > +#define HCINTMSK15 0x6EC > > +#define HCTSIZ15 0x6F0 > > +#define HCDMA15 0x6F4 > > Why does this hold both the macros to calculate the offsets for a > given channel and all of the absolute register offsets per channel? > OK. I'll only use one method. > > + > > +/*** DEVICE MODE REGISTERS ***/ > > +/* Device Global Registers */ > > +#define DCFG 0x800 > > +#define DCFG_DESCDMA BIT23 > > +#define DCFG_EPMISCNT_MASK (0x1F << 18) > > +#define DCFG_EPMISCNT_SHIFT 18 > > +#define DCFG_DEVADDR_MASK (0x7F << 4) > > +#define DCFG_DEVADDR_SHIFT 4 > > +#define DCFG_DEVADDR(x) (((x) << DCFG_DEVADDR_SHIFT) & > > DCFG_DEVADDR_MASK) > > +#define DCFG_NZ_STS_OUT_HSHK BIT2 > > + > > +#define DCTL 0x804 > > +#define DCTL_PWRONPRGDONE BIT11 > > +#define DCTL_GNPINNAKSTS BIT2 > > +#define DCTL_SFTDISCON BIT1 > > + > > +#define DSTS 0x808 > > +#define DSTS_ENUMSPD_MASK (0x3 << 1) > > +#define DSTS_ENUMSPD_HIGH (0 << 1) > > +#define DSTS_ENUMSPD_FULL (1 << 1) > > + > > +#define DIEPMSK 0x810 > > +#define DOEPMSK 0x814 > > + > > +#define DXEPMSK_TIMEOUTMSK BIT3 > > +#define DXEPMSK_AHBERMSK BIT2 > > +#define DXEPMSK_XFERCOMPLMSK BIT0 > > + > > +#define DAINT 0x818 > > +#define DAINTMSK 0x81C > > + > > +#define DAINTMSK_OUTEPMSK_SHIFT 16 > > +#define DAINTMSK_INEPMSK_SHIFT 0 > > + > > +#define DTKNQR1 0x820 > > +#define DTKNQR2 0x824 > > +#define DVBUSDIS 0x828 > > +#define DVBUSPULSE 0x82C > > +#define DTHRCTL 0x830 > > + > > +/* Device Logical IN Endpoint-Specific Registers */ > > +#define DIEPCTL(x) (0x900 + 0x20 * (x)) > > +#define DIEPINT(x) (0x908 + 0x20 * (x)) > > +#define DIEPTSIZ(x) (0x910 + 0x20 * (x)) > > +#define DIEPDMA(x) (0x914 + 0x20 * (x)) > > +#define DTXFSTS(x) (0x918 + 0x20 * (x)) > > Same again - please add a #define for that 0x20. OK. > > > + > > +#define DIEPCTL0 0x900 > > +#define DIEPINT0 0x908 > > +#define DIEPTSIZ0 0x910 > > +#define DIEPDMA0 0x914 > > +#define DIEPCTL1 0x920 > > +#define DIEPINT1 0x928 > > +#define DIEPTSIZ1 0x930 > > +#define DIEPDMA1 0x934 > > +#define DIEPCTL2 0x940 > > +#define DIEPINT2 0x948 > > +#define DIEPTSIZ2 0x950 > > +#define DIEPDMA2 0x954 > > +#define DIEPCTL3 0x960 > > +#define DIEPINT3 0x968 > > +#define DIEPTSIZ3 0x970 > > +#define DIEPDMA3 0x974 > > +#define DIEPCTL4 0x980 > > +#define DIEPINT4 0x988 > > +#define DIEPTSIZ4 0x990 > > +#define DIEPDMA4 0x994 > > +#define DIEPCTL5 0x9A0 > > +#define DIEPINT5 0x9A8 > > +#define DIEPTSIZ5 0x9B0 > > +#define DIEPDMA5 0x9B4 > > +#define DIEPCTL6 0x9C0 > > +#define DIEPINT6 0x9C8 > > +#define DIEPTSIZ6 0x9D0 > > +#define DIEPDMA6 0x9D4 > > +#define DIEPCTL7 0x9E0 > > +#define DIEPINT7 0x9E8 > > +#define DIEPTSIZ7 0x9F0 > > +#define DIEPDMA7 0x9F4 > > +#define DIEPCTL8 0xA00 > > +#define DIEPINT8 0xA08 > > +#define DIEPTSIZ8 0xA10 > > +#define DIEPDMA8 0xA14 > > +#define DIEPCTL9 0xA20 > > +#define DIEPINT9 0xA28 > > +#define DIEPTSIZ9 0xA30 > > +#define DIEPDMA9 0xA34 > > +#define DIEPCTL10 0xA40 > > +#define DIEPINT10 0xA48 > > +#define DIEPTSIZ10 0xA50 > > +#define DIEPDMA10 0xA54 > > +#define DIEPCTL11 0xA60 > > +#define DIEPINT11 0xA68 > > +#define DIEPTSIZ11 0xA70 > > +#define DIEPDMA11 0xA74 > > +#define DIEPCTL12 0xA80 > > +#define DIEPINT12 0xA88 > > +#define DIEPTSIZ12 0xA90 > > +#define DIEPDMA12 0xA94 > > +#define DIEPCTL13 0xAA0 > > +#define DIEPINT13 0xAA8 > > +#define DIEPTSIZ13 0xAB0 > > +#define DIEPDMA13 0xAB4 > > +#define DIEPCTL14 0xAC0 > > +#define DIEPINT14 0xAC8 > > +#define DIEPTSIZ14 0xAD0 > > +#define DIEPDMA14 0xAD4 > > +#define DIEPCTL15 0xAE0 > > +#define DIEPINT15 0xAE8 > > +#define DIEPTSIZ15 0xAF0 > > +#define DIEPDMA15 0xAF4 > > And again, why do we have both macros and all of the possible combinations? OK. I'll pick one method only. > > > + > > +/* Device Logical OUT Endpoint-Specific Registers */ > > +#define DOEPCTL(x) (0xB00 + 0x20 * (x)) > > +#define DOEPINT(x) (0xB08 + 0x20 * (x)) > > +#define DOEPTSIZ(x) (0xB10 + 0x20 * (x)) > > + > > +#define DXEPINT_EPDISBLD BIT1 > > +#define DXEPINT_XFERCOMPL BIT0 > > + > > +#define DXEPTSIZ_SUPCNT(x) (((x) & 0x3) << 29) > > +#define DXEPTSIZ_PKTCNT(x) (((x) & 0x3) << 19) > > +#define DXEPTSIZ_XFERSIZE(x) ((x) & 0x7F) > > + > > +#define DOEPDMA(x) (0xB14 + 0x20 * (x)) > > Move this up to below DOEPTSIZ? > Delete all of the below definitions and use the macros instead > And a #define for that 0x20? > OK > > +#define DOEPCTL0 0xB00 > > +#define DOEPINT0 0xB08 > > +#define DOEPTSIZ0 0xB10 > > +#define DOEPDMA0 0xB14 > > +#define DOEPCTL1 0xB20 > > +#define DOEPINT1 0xB28 > > +#define DOEPTSIZ1 0xB30 > > +#define DOEPDMA1 0xB34 > > +#define DOEPCTL2 0xB40 > > +#define DOEPINT2 0xB48 > > +#define DOEPTSIZ2 0xB50 > > +#define DOEPDMA2 0xB54 > > +#define DOEPCTL3 0xB60 > > +#define DOEPINT3 0xB68 > > +#define DOEPTSIZ3 0xB70 > > +#define DOEPDMA3 0xB74 > > +#define DOEPCTL4 0xB80 > > +#define DOEPINT4 0xB88 > > +#define DOEPTSIZ4 0xB90 > > +#define DOEPDMA4 0xB94 > > +#define DOEPCTL5 0xBA0 > > +#define DOEPINT5 0xBA8 > > +#define DOEPTSIZ5 0xBB0 > > +#define DOEPDMA5 0xBB4 > > +#define DOEPCTL6 0xBC0 > > +#define DOEPINT6 0xBC8 > > +#define DOEPTSIZ6 0xBD0 > > +#define DOEPDMA6 0xBD4 > > +#define DOEPCTL7 0xBE0 > > +#define DOEPINT7 0xBE8 > > +#define DOEPTSIZ7 0xBF0 > > +#define DOEPDMA7 0xBF4 > > +#define DOEPCTL8 0xC00 > > +#define DOEPINT8 0xC08 > > +#define DOEPTSIZ8 0xC10 > > +#define DOEPDMA8 0xC14 > > +#define DOEPCTL9 0xC20 > > +#define DOEPINT9 0xC28 > > +#define DOEPTSIZ9 0xC30 > > +#define DOEPDMA9 0xC34 > > +#define DOEPCTL10 0xC40 > > +#define DOEPINT10 0xC48 > > +#define DOEPTSIZ10 0xC50 > > +#define DOEPDMA10 0xC54 > > +#define DOEPCTL11 0xC60 > > +#define DOEPINT11 0xC68 > > +#define DOEPTSIZ11 0xC70 > > +#define DOEPDMA11 0xC74 > > +#define DOEPCTL12 0xC80 > > +#define DOEPINT12 0xC88 > > +#define DOEPTSIZ12 0xC90 > > +#define DOEPDMA12 0xC94 > > +#define DOEPCTL13 0xCA0 > > +#define DOEPINT13 0xCA8 > > +#define DOEPTSIZ13 0xCB0 > > +#define DOEPDMA13 0xCB4 > > +#define DOEPCTL14 0xCC0 > > +#define DOEPINT14 0xCC8 > > +#define DOEPTSIZ14 0xCD0 > > +#define DOEPDMA14 0xCD4 > > +#define DOEPCTL15 0xCE0 > > +#define DOEPINT15 0xCE8 > > +#define DOEPTSIZ15 0xCF0 > > +#define DOEPDMA15 0xCF4 > > + > > +#define DXEPCTL_EPENA BIT31 > > +#define DXEPCTL_SNAK BIT27 > > +#define DXEPCTL_CNAK BIT26 > > +#define DXEPCTL_TXFNUM(x) (((x) & 0xF) << 22) > > +#define DXEPCTL_STALL BIT21 > > +#define DXEPCTL_EPTYPE_MASK (BIT19 | BIT18) > > +#define DXEPCTL_EPTYPE(x) (((x) & 0x3) << 18) > > +#define DXEPCTL_NAKSTS BIT17 > > +#define DXEPCTL_USBACTEP BIT15 > > +#define DXEPCTL_MPS_MASK 0x7FF > > + > > +#define DXEPTSIZN_PKTCNT_MASK (0x3FF << 19) > > +#define DXEPTSIZN_PKTCNT(x) (((x) & 0x3FF) << 19) > > +#define DXEPTSIZN_XFERSIZE_MASK 0x7FFFF > > +#define DXEPTSIZN_XFERSIZE(x) ((x) & 0x7FFFF) > > + > > +/* Power and Clock Gating Register */ > > +#define PCGCCTL 0xE00 > > + > > +#define EP0FIFO 0x1000 > > + > > +/** > > + * This union represents the bit fields in the DMA Descriptor > > + * status quadlet. Read the quadlet into the <i>d32</i> member then > > + * set/clear the bits using the <i>b</i>it, <i>b_iso_out</i> and > > + * <i>b_iso_in</i> elements. > > + */ > > Drop the HTML formatting please. > OK > > +typedef union { > > + /** raw register data */ > > + UINT32 d32; > > + /** quadlet bits */ > > + struct { > > + /** Received number of bytes */ > > + unsigned bytes:16; > > + /** NAK bit - only for OUT EPs */ > > + unsigned nak:1; > > + unsigned reserved17_22:6; > > + /** Multiple Transfer - only for OUT EPs */ > > + unsigned mtrf:1; > > + /** Setup Packet received - only for OUT EPs */ > > + unsigned sr:1; > > + /** Interrupt On Complete */ > > + unsigned ioc:1; > > + /** Short Packet */ > > + unsigned sp:1; > > + /** Last */ > > + unsigned l:1; > > + /** Receive Status */ > > + unsigned sts:2; > > + /** Buffer Status */ > > + unsigned bs:2; > > + } b; > > +} DevDmaDescStatus; > > This struct does not conform to coding style. > OK. I'll use macro definition instead. > > + > > +/** > > + * DMA Descriptor structure > > + * > > + * DMA Descriptor structure contains two quadlets: > > + * Status quadlet and Data buffer pointer. > > + */ > > +typedef struct { > > + /** DMA Descriptor status quadlet */ > > + DevDmaDescStatus status; > > + /** DMA Descriptor data buffer pointer */ > > + UINT32 buf; > > +} DwUsbDevDmaDesc; > > + > > +#endif /* __DW_USB_DXE_H__ */ > > diff --git a/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.c > > b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.c > > new file mode 100644 > > index 000000000000..6e1086190796 > > --- /dev/null > > +++ b/EmbeddedPkg/Drivers/DwUsbDxe/DwUsbDxe.c > > @@ -0,0 +1,912 @@ > > +/** @file > > + > > + Copyright (c) 2018, Linaro. 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> > > Hmm? > > > +#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/UsbDevice.h> > > + > > +#include "DwUsbDxe.h" > > + > > +#define USB_TYPE_LENGTH 16 > > What type? I'll add comment on it. > > > +#define USB_BLOCK_HIGH_SPEED_SIZE 512 > > What size? > > > +#define DATA_SIZE 32768 > > What data? > > > +#define CMD_SIZE 512 > > What command? > > > +#define MATCH_CMD_LITERAL(Cmd, Buf) (!AsciiStrnCmp (Cmd, Buf, sizeof > > (Cmd) - 1)) > > This macro is used exactly once. Please just inline the call (with == > 0 instead of !). > OK > > + > > +// > > +// The time between interrupt polls, in units of 100 nanoseconds > > +// 10 Microseconds > > +// > > +#define DW_INTERRUPT_POLL_PERIOD 10000 > > + > > +#define ENDPOINT0 0 > > +#define ENDPOINT1 1 > > + > > +#define DIR_OUT 0 > > +#define DIR_IN 1 > > + > > +#define DWUSB_BUFFER_PAGE_NUM 16 > > + > > +#define DESCRIPTOR_INDEX_LANG 0 > > +#define DESCRIPTOR_INDEX_MANUFACTURER 1 > > +#define DESCRIPTOR_INDEX_PRODUCT 2 > > +#define DESCRIPTOR_INDEX_SERIALNUMBER 3 > > + > > +#define DESCRIPTOR_TYPE(x) (((x) >> 8) & 0xFF) > > +#define DESCRIPTOR_INDEX(x) ((x) & 0xFF) > > + > > +typedef struct { > > + USB_DEVICE_DESCRIPTOR *DeviceDescriptor; > > + USB_CONFIG_DESCRIPTOR *ConfigDescriptor; > > +} DW_USB_DESCRIPTOR; > > Please move everything after my last comment to here into DwUsbDxe.h. > Or DwUsb2Dxe.h, I guess it should be called. > OK > > + > > +EFI_GUID gDwUsbProtocolGuid = DW_USB_PROTOCOL_GUID; > > Problematic, as mentioned in comment on DwUsb3Dxe. > > > + > > +STATIC DwUsbDevDmaDesc *gDmaDesc,*gDmaDescEp0,*gDmaDescIn; > > +STATIC USB_DEVICE_REQUEST *gCtrlReq; > > +STATIC VOID *RxBuf; > > +STATIC UINTN RxDescBytes; > > +STATIC UINTN mNumDataBytes; > > + > > +STATIC DW_USB_PROTOCOL *DwUsb; > > +STATIC DW_USB_DESCRIPTOR gDwUsbDescriptor; > > Please use 'm' prefix on all STATIC globals. > OK > > + > > +STATIC USB_DEVICE_RX_CALLBACK mDataReceivedCallback; > > +STATIC USB_DEVICE_TX_CALLBACK mDataSentCallback; > > + > > + > > +/* To detect which mode was run, high speed or full speed */ > > +STATIC > > +UINTN > > +UsbDrvPortSpeed ( > > + VOID > > + ) > > +{ > > + return (((READ_REG32 (DSTS) & DSTS_ENUMSPD_MASK) == DSTS_ENUMSPD_HIGH)); > > +} > > + > > +STATIC > > +VOID > > +ResetEndpoints ( > > + VOID > > + ) > > +{ > > + UINT32 Data; > > + > > + /* EP0 IN ACTIVE NEXT=1 */ > > + WRITE_REG32 (DIEPCTL0, DXEPCTL_USBACTEP | BIT11); > > + > > + /* EP0 OUT ACTIVE */ > > + WRITE_REG32 (DOEPCTL0, DXEPCTL_USBACTEP); > > + > > + /* Clear any pending OTG Interrupts */ > > + WRITE_REG32 (GOTGINT, ~0); > > + > > + /* Clear any pending interrupts */ > > + WRITE_REG32 (GINTSTS, ~0); > > + WRITE_REG32 (DIEPINT0, ~0); > > + WRITE_REG32 (DOEPINT0, ~0); > > + WRITE_REG32 (DIEPINT1, ~0); > > + WRITE_REG32 (DOEPINT1, ~0); > > + > > + /* IN EP interrupt mask */ > > + WRITE_REG32 (DIEPMSK, DXEPMSK_TIMEOUTMSK | DXEPMSK_AHBERMSK | > > DXEPMSK_XFERCOMPLMSK); > > + /* OUT EP interrupt mask */ > > + WRITE_REG32 (DOEPMSK, DXEPMSK_TIMEOUTMSK | DXEPMSK_AHBERMSK | > > DXEPMSK_XFERCOMPLMSK); > > + /* Enable interrupts on EndPoint0 */ > > + WRITE_REG32 (DAINTMSK, (1 << DAINTMSK_OUTEPMSK_SHIFT) | (1 << > > DAINTMSK_INEPMSK_SHIFT)); > > + > > + /* EP0 OUT Transfer Size:64 Bytes, 1 Packet, 3 Setup Packet, Read to > > receive setup packet*/ > > + WRITE_REG32 (DOEPTSIZ0, DXEPTSIZ_SUPCNT(3) | DXEPTSIZ_PKTCNT(1) | > > DXEPTSIZ_XFERSIZE(64)); > > Long lines above. > OK > > + > > + // > > + //notes that:the compulsive conversion is expectable. > > What does this mean? > > > + // > > + gDmaDescEp0->status.b.bs = 0x3; > > + gDmaDescEp0->status.b.mtrf = 0; > > + gDmaDescEp0->status.b.sr = 0; > > + gDmaDescEp0->status.b.l = 1; > > + gDmaDescEp0->status.b.ioc = 1; > > + gDmaDescEp0->status.b.sp = 0; > > + gDmaDescEp0->status.b.bytes = 64; > > + gDmaDescEp0->buf = (UINT32)(UINTN)gCtrlReq; > > + gDmaDescEp0->status.b.sts = 0; > > + gDmaDescEp0->status.b.bs = 0x0; > > What do all of these live-coded values mean? > Need #defines or possibly comments. > OK > > + WRITE_REG32 (DOEPDMA0, (UINT32)(UINTN)gDmaDescEp0); > > + /* EP0 OUT ENABLE CLEARNAK */ > > + Data = READ_REG32 (DOEPCTL0); > > + WRITE_REG32 (DOEPCTL0, Data | DXEPCTL_EPENA | DXEPCTL_CNAK); > > +} > > + > > +STATIC > > +VOID > > +EndPointTx ( > > + IN UINT8 EndPoint, > > + IN CONST VOID *Ptr, > > + IN UINTN Len > > + ) > > +{ > > + UINT32 BlockSize; > > + UINT32 Packets; > > + UINT32 Data; > > + > > + /* EPx OUT ACTIVE */ > > + Data = READ_REG32 (DIEPCTL (EndPoint)); > > + WRITE_REG32 (DIEPCTL (EndPoint), Data | DXEPCTL_USBACTEP); > > + if (!EndPoint) { > > + BlockSize = 64; > > + } else { > > + BlockSize = UsbDrvPortSpeed () ? USB_BLOCK_HIGH_SPEED_SIZE : 64; > > I will want #defines for those 64. > Among other things that will tell me if it's the same 64 or whether > they happen to have the same numeric value. > OK > > + } > > + Packets = (Len + BlockSize - 1) / BlockSize; > > + > > + if (!Len) { > > + /* send one empty packet */ > > + gDmaDescIn->status.b.bs = 0x3; > > + gDmaDescIn->status.b.l = 1; > > + gDmaDescIn->status.b.ioc = 1; > > + gDmaDescIn->status.b.sp = 1; > > + gDmaDescIn->status.b.bytes = 0; > > + gDmaDescIn->buf = 0; > > + gDmaDescIn->status.b.sts = 0; > > + gDmaDescIn->status.b.bs = 0x0; > > #defines and/or comments for numbers. > OK > > + > > + WRITE_REG32 (DIEPDMA (EndPoint), (UINT32)(UINTN)gDmaDescIn); > > // DMA Address (DMAAddr) is zero > > + } else { > > + WRITE_REG32 (DIEPTSIZ (EndPoint), Len | DXEPTSIZN_PKTCNT (Packets)); > > + > > + // > > + //flush cache > > + // > > + WriteBackDataCacheRange ((VOID *)Ptr, Len); > > + > > + gDmaDescIn->status.b.bs = 0x3; > > + gDmaDescIn->status.b.l = 1; > > + gDmaDescIn->status.b.ioc = 1; > > + gDmaDescIn->status.b.sp = 1; > > + gDmaDescIn->status.b.bytes = Len; > > + gDmaDescIn->buf = (UINT32)(UINTN)Ptr; > > + gDmaDescIn->status.b.sts = 0; > > + gDmaDescIn->status.b.bs = 0x0; > > #defines and/or comments for numbers. > OK > > + WRITE_REG32 (DIEPDMA (EndPoint), (UINT32)(UINTN)gDmaDescIn); > > // Ptr is DMA address > > + } > > + ArmDataSynchronizationBarrier (); > > MemoryFence ()? > Thanks. I'll check it. > > + /* epena & cnak */ > > + Data = READ_REG32 (DIEPCTL (EndPoint)); > > + WRITE_REG32 (DIEPCTL (EndPoint), Data | DXEPCTL_EPENA | DXEPCTL_CNAK | > > BIT11); > > +} > > + > > +STATIC > > +VOID > > +EndPointRx ( > > + IN UINTN EndPoint, > > + IN UINTN Len > > + ) > > +{ > > + UINT32 Data; > > + > > + /* EPx UNSTALL */ > > + Data = READ_REG32 (DOEPCTL (EndPoint)); > > + WRITE_REG32 (DOEPCTL (EndPoint), Data & ~DXEPCTL_STALL); > > + /* EPx OUT ACTIVE */ > > + Data = READ_REG32 (DOEPCTL (EndPoint)); > > + WRITE_REG32 (DOEPCTL (EndPoint), Data | DXEPCTL_USBACTEP); > > + > > + if (Len >= DATA_SIZE) { > > + RxDescBytes = DATA_SIZE; > > + } else { > > + RxDescBytes = Len; > > + } > > + > > + RxBuf = AllocatePool (DATA_SIZE); > > + if (RxBuf == NULL) { > > + DEBUG ((DEBUG_ERROR, "EndPointRx: failed to allocate buffer\n")); > > + return; > > + } > > + > > + InvalidateDataCacheRange (RxBuf, Len); > > + > > + gDmaDesc->status.b.bs = 0x3; > > + gDmaDesc->status.b.mtrf = 0; > > + gDmaDesc->status.b.sr = 0; > > + gDmaDesc->status.b.l = 1; > > + gDmaDesc->status.b.ioc = 1; > > + gDmaDesc->status.b.sp = 0; > > + gDmaDesc->status.b.bytes = (UINT32)RxDescBytes; > > + gDmaDesc->buf = (UINT32)(UINTN)RxBuf; > > + gDmaDesc->status.b.sts = 0; > > + gDmaDesc->status.b.bs = 0x0; > > #defines and/or comments for numbers. > OK > > + > > + ArmDataSynchronizationBarrier (); > > MemoryFence ()? > OK > > + WRITE_REG32 (DOEPDMA (EndPoint), (UINT32)(UINTN)gDmaDesc); > > + /* EPx OUT ENABLE CLEARNAK */ > > + Data = READ_REG32 (DOEPCTL (EndPoint)); > > + WRITE_REG32 (DOEPCTL (EndPoint), Data | DXEPCTL_EPENA | DXEPCTL_CNAK); > > +} > > + > > +STATIC > > +EFI_STATUS > > +HandleGetDescriptor ( > > + IN USB_DEVICE_REQUEST *Request > > + ) > > +{ > > + UINTN ResponseSize; > > + VOID *ResponseData; > > + EFI_USB_STRING_DESCRIPTOR *Descriptor = NULL; > > + UINTN DescriptorSize; > > + > > + ResponseSize = 0; > > + ResponseData = NULL; > > + > > + // Pretty confused if bmRequestType is anything but this: > > + if (Request->RequestType != USB_DEV_GET_DESCRIPTOR_REQ_TYPE) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // > > + // Choose the response > > + // > > + switch (DESCRIPTOR_TYPE(Request->Value)) { > > + case USB_DESC_TYPE_DEVICE: > > + DEBUG ((DEBUG_INFO, "DwUsbDxe: Got a request for device > > descriptor\n")); > > + ResponseSize = sizeof (USB_DEVICE_DESCRIPTOR); > > + ResponseData = gDwUsbDescriptor.DeviceDescriptor; > > + break; > > + case USB_DESC_TYPE_CONFIG: > > + DEBUG ((DEBUG_INFO, "DwUsbDxe: Got a request for config > > descriptor\n")); > > + ResponseSize = gDwUsbDescriptor.ConfigDescriptor->TotalLength; > > + ResponseData = gDwUsbDescriptor.ConfigDescriptor; > > + break; > > + case USB_DESC_TYPE_STRING: > > + DEBUG (( > > + DEBUG_INFO, > > + "DwUsbDxe: Got a request for String descriptor %d\n", > > + Request->Value & 0xFF > > + )); > > + switch (DESCRIPTOR_INDEX(Request->Value)) { > > Put the nested switch in a separate function, this is unreadable. > OK > > + case 0: > > + DescriptorSize = sizeof (EFI_USB_STRING_DESCRIPTOR) + > > + (LANG_LENGTH + 1) * sizeof (CHAR16); > > + Descriptor = (EFI_USB_STRING_DESCRIPTOR *)AllocateZeroPool > > (DescriptorSize); > > + if (Descriptor == NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + Descriptor->Length = LANG_LENGTH * sizeof (CHAR16); > > + Descriptor->DescriptorType = USB_DESC_TYPE_STRING; > > + DwUsb->GetLang (Descriptor->String, &Descriptor->Length); > > + ResponseSize = Descriptor->Length; > > + ResponseData = Descriptor; > > + break; > > + case 1: > > + DescriptorSize = sizeof (EFI_USB_STRING_DESCRIPTOR) + > > + (MANU_FACTURER_STRING_LENGTH + 1) * sizeof (CHAR16); > > + Descriptor = (EFI_USB_STRING_DESCRIPTOR *)AllocateZeroPool > > (DescriptorSize); > > + if (Descriptor == NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + Descriptor->Length = MANU_FACTURER_STRING_LENGTH * sizeof (CHAR16); > > + Descriptor->DescriptorType = USB_DESC_TYPE_STRING; > > + DwUsb->GetManuFacturer (Descriptor->String, &Descriptor->Length); > > + ResponseSize = Descriptor->Length; > > + ResponseData = Descriptor; > > + break; > > + case 2: > > + DescriptorSize = sizeof (EFI_USB_STRING_DESCRIPTOR) + > > + (PRODUCT_STRING_LENGTH + 1) * sizeof (CHAR16); > > + Descriptor = (EFI_USB_STRING_DESCRIPTOR *)AllocateZeroPool > > (DescriptorSize); > > + if (Descriptor == NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + Descriptor->Length = PRODUCT_STRING_LENGTH * sizeof (CHAR16); > > + Descriptor->DescriptorType = USB_DESC_TYPE_STRING; > > + DwUsb->GetProduct (Descriptor->String, &Descriptor->Length); > > + ResponseSize = Descriptor->Length; > > + ResponseData = Descriptor; > > + break; > > + case 3: > > + DescriptorSize = sizeof (EFI_USB_STRING_DESCRIPTOR) + > > + SERIAL_STRING_LENGTH * sizeof (CHAR16) + 1; > > + Descriptor = (EFI_USB_STRING_DESCRIPTOR *)AllocateZeroPool > > (DescriptorSize); > > + if (Descriptor == NULL) { > > + return EFI_OUT_OF_RESOURCES; > > + } > > + Descriptor->Length = SERIAL_STRING_LENGTH * sizeof (CHAR16); > > + Descriptor->DescriptorType = USB_DESC_TYPE_STRING; > > + DwUsb->GetSerialNo (Descriptor->String, &Descriptor->Length); > > + ResponseSize = Descriptor->Length; > > + ResponseData = Descriptor; > > + break; > > + } > > + break; > > + default: > > + DEBUG (( > > + DEBUG_INFO, > > + "DwUsbDxe: Didn't understand request for descriptor 0x%04x\n", > > + Request->Value > > + )); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // > > + // Send the response > > + // > > + if (Request->Length < ResponseSize) { > > + // > > + // Truncate response > > + // > > + ResponseSize = Request->Length; > > + } else if (Request->Length > ResponseSize) { > > + DEBUG ((DEBUG_INFO, "DwUsbDxe: Info: ResponseSize < wLength\n")); > > + } > > + > > + EndPointTx (ENDPOINT0, ResponseData, ResponseSize); > > + if (Descriptor) { > > + FreePool (Descriptor); > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > +STATIC > > +EFI_STATUS > > +HandleSetAddress ( > > + IN USB_DEVICE_REQUEST *Request > > + ) > > +{ > > + UINT32 Data; > > + > > + // > > + // Pretty confused if bmRequestType is anything but this: > > + // > > + if (Request->RequestType != USB_DEV_SET_ADDRESS_REQ_TYPE) { > > + return EFI_INVALID_PARAMETER; > > + } > > + DEBUG ((DEBUG_INFO, "DwUsbDxe: Setting address to %d\n", > > Request->Value)); > > + ResetEndpoints (); > > + > > + Data = READ_REG32 (DCFG); > > + Data &= ~DCFG_DEVADDR_MASK; > > + Data |= DCFG_DEVADDR (Request->Value); > > + WRITE_REG32 (DCFG, Data); > > + EndPointTx (ENDPOINT0, NULL, 0); > > + > > + return EFI_SUCCESS; > > +} > > + > > +STATIC > > +VOID > > +UsbDrvRequestEndpoint ( > > + IN UINTN Type, > > + IN UINTN Dir > > + ) > > +{ > > + UINTN EndPoint = 1; > > + UINTN NewBits; > > + UINT32 Data; > > + > > + NewBits = DXEPCTL_EPTYPE (Type) | BIT28; > > + > > + /* > > + * (Type << 18):Endpoint Type (EPType) > > + * 0x10000000:Endpoint Enable (EPEna) > > + * 0x000C000:Endpoint Type (EPType);Hardcoded to 00 for control. > > + * (ep<<22):TxFIFO Number (TxFNum) > > + * 0x20000:NAK Status (NAKSts);The core is transmitting NAK handshakes > > on this endpoint. > > + */ > > + if (Dir == DIR_IN) { // IN: to host > > + Data = READ_REG32 (DIEPCTL (EndPoint)); > > + Data &= ~DXEPCTL_EPTYPE_MASK; > > + Data |= NewBits | DXEPCTL_TXFNUM (EndPoint) | DXEPCTL_NAKSTS; > > + WRITE_REG32 (DIEPCTL (EndPoint), Data); > > + } else { // OUT: to device > > + Data = READ_REG32 (DOEPCTL (EndPoint)); > > + Data &= ~DXEPCTL_EPTYPE_MASK; > > + Data |= NewBits; > > + WRITE_REG32 (DOEPCTL (EndPoint), Data); > > + } > > +} > > + > > +STATIC > > +EFI_STATUS > > +HandleSetConfiguration ( > > + IN USB_DEVICE_REQUEST *Request > > + ) > > +{ > > + UINT32 Data; > > + > > + if (Request->RequestType != USB_DEV_SET_CONFIGURATION_REQ_TYPE) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // > > + // Cancel all transfers > > + // > > + ResetEndpoints (); > > + > > + UsbDrvRequestEndpoint (2, DIR_OUT); > > + UsbDrvRequestEndpoint (2, DIR_IN); > > What is 2? > I'll add comment onit. > > + > > + Data = READ_REG32 (DIEPCTL1); > > + WRITE_REG32 (DIEPCTL1, Data | BIT28 | BIT19 | DXEPCTL_USBACTEP | BIT11); > > Please give #defines for those named bits (or the whole command if > more appropriate). > OK > > + > > + /* Enable interrupts on all endpoints */ > > + WRITE_REG32 (DAINTMSK, ~0); > > + > > + EndPointRx (ENDPOINT1, CMD_SIZE); > > + EndPointTx (ENDPOINT0, NULL, 0); > > + return EFI_SUCCESS; > > +} > > + > > + > > +STATIC > > +EFI_STATUS > > +HandleDeviceRequest ( > > + IN USB_DEVICE_REQUEST *Request > > + ) > > +{ > > + EFI_STATUS Status; > > + > > + switch (Request->Request) { > > + case USB_DEV_GET_DESCRIPTOR: > > + Status = HandleGetDescriptor (Request); > > + break; > > + case USB_DEV_SET_ADDRESS: > > + Status = HandleSetAddress (Request); > > + break; > > + case USB_DEV_SET_CONFIGURATION: > > + Status = HandleSetConfiguration (Request); > > + break; > > + default: > > + DEBUG ((DEBUG_ERROR, > > + "Didn't understand RequestType 0x%x Request 0x%x\n", > > + Request->RequestType, Request->Request)); > > + Status = EFI_INVALID_PARAMETER; > > + break; > > + } > > + > > + return Status; > > +} > > + > > +STATIC > > +VOID > > +HandleEndPointInEvent ( > > + VOID > > + ) > > +{ > > + UINT32 EndPointInts; > > + > > + EndPointInts = READ_REG32 (DIEPINT0); > > + WRITE_REG32 (DIEPINT0, EndPointInts); > > + if (EndPointInts & DXEPINT_XFERCOMPL) { > > + DEBUG ((DEBUG_INFO, "INT: IN TX completed.DIEPTSIZ (0) = 0x%x.\n", > > READ_REG32 (DIEPTSIZ0))); > > + } > > + > > + EndPointInts = READ_REG32 (DIEPINT1); > > + WRITE_REG32 (DIEPINT1, EndPointInts); > > + if (EndPointInts & DXEPINT_XFERCOMPL) { > > + DEBUG ((DEBUG_INFO, "ep1: IN TX completed\n")); > > + } > > +} > > + > > +STATIC > > +VOID > > +HandleEndPointOutEvent ( > > + VOID > > + ) > > +{ > > + UINT32 EndPointInts; > > + UINT32 Data; > > + > > + /* > > + * indicates the status of an endpoint > > + * with respect to USB- and AHB-related events. > > + */ > > + EndPointInts = READ_REG32 (DOEPINT0); > > + if (EndPointInts) { > > + WRITE_REG32 (DOEPINT0, EndPointInts); > > + if (EndPointInts & DXEPINT_XFERCOMPL) { > > + DEBUG ((DEBUG_INFO, "INT: EP0 RX completed. DOEPTSIZ(0) = 0x%x.\n", > > READ_REG32 (DOEPTSIZ0))); > > + } > > + /* > > + * > > + * IN Token Received When TxFIFO is Empty (INTknTXFEmp) > > + * Indicates that an IN token was received when the associated TxFIFO > > (periodic/nonperiodic) > > + * was empty. This interrupt is asserted on the endpoint for which the > > IN token > > + * was received. > > + */ > > + if (EndPointInts & BIT3) { /* SETUP phase done */ > > What is BIT3? > I'll try to add comment or use macro definition. > > + Data = READ_REG32 (DIEPCTL0) | DXEPCTL_SNAK; > > + WRITE_REG32 (DIEPCTL0, Data); > > + Data = READ_REG32 (DOEPCTL0) | DXEPCTL_SNAK; > > + WRITE_REG32 (DOEPCTL0, Data); > > + /* clear IN EP intr */ > > + WRITE_REG32 (DIEPINT0, ~0); > > + HandleDeviceRequest((USB_DEVICE_REQUEST *)gCtrlReq); > > + } > > + > > + /* Make sure EP0 OUT is set up to accept the next request */ > > + WRITE_REG32 (DOEPTSIZ0, DXEPTSIZ_SUPCNT(3) | DXEPTSIZ_PKTCNT(1) | > > DXEPTSIZ_XFERSIZE(64)); > > + /* > > + * IN Token Received When TxFIFO is Empty (INTknTXFEmp) > > + * Indicates that an IN token was received when the associated > > + * TxFIFO (periodic/nonperiodic) * was empty. This interrupt is > > + * asserted on the endpoint for which the IN token was received. > > + */ > > + gDmaDescEp0->status.b.bs = 0x3; > > + gDmaDescEp0->status.b.mtrf = 0; > > + gDmaDescEp0->status.b.sr = 0; > > + gDmaDescEp0->status.b.l = 1; > > + gDmaDescEp0->status.b.ioc = 1; > > + gDmaDescEp0->status.b.sp = 0; > > + gDmaDescEp0->status.b.bytes = 64; > > + gDmaDescEp0->buf = (UINT32)(UINTN)gCtrlReq; > > + gDmaDescEp0->status.b.sts = 0; > > + gDmaDescEp0->status.b.bs = 0x0; > > Need some #defines for those numbers. > OK > > + WRITE_REG32 (DOEPDMA0, (UINT32)(UINTN)gDmaDescEp0); > > + // > > + // endpoint enable; clear NAK > > + // > > + WRITE_REG32 (DOEPCTL0, DXEPCTL_EPENA | DXEPCTL_CNAK); > > + } > > + > > + EndPointInts = (READ_REG32 (DOEPINT1)); > > + if (EndPointInts) { > > + WRITE_REG32 (DOEPINT1, EndPointInts); > > + /* Transfer Completed Interrupt (XferCompl);Transfer completed */ > > + if (EndPointInts & DXEPINT_XFERCOMPL) { > > + > > + UINTN Bytes = RxDescBytes - gDmaDesc->status.b.bytes; > > + UINTN Len = 0; > > + > > + ArmDataSynchronizationBarrier (); > > MemoryFence ()? > OK > > + if (MATCH_CMD_LITERAL ("download", RxBuf)) { > > + mNumDataBytes = AsciiStrHexToUint64 (RxBuf + sizeof ("download")); > > + } else { > > + if (mNumDataBytes != 0) { > > + mNumDataBytes -= Bytes; > > + } > > + } > > + > > + mDataReceivedCallback (Bytes, RxBuf); > > + > > + if (mNumDataBytes == 0) { > > + Len = CMD_SIZE; > > + } else if (mNumDataBytes > DATA_SIZE) { > > + Len = DATA_SIZE; > > + } else { > > + Len = mNumDataBytes; > > + } > > + > > + EndPointRx (ENDPOINT1, Len); > > + } > > + } > > +} > > + > > +// > > +// Instead of actually registering interrupt handlers, we poll the > > controller's > > +// interrupt source register in this function. > > +// > > +STATIC > > +VOID > > +CheckInterrupts ( > > + IN EFI_EVENT Event, > > + IN VOID *Context > > + ) > > +{ > > + UINT32 Ints; > > "Ints" gets a bit confusing in C. Please write it out as interrupts. > OK > > + UINT32 Data; > > + > > + // > > + // interrupt register > > + // > > + Ints = READ_REG32 (GINTSTS); > > + > > + /* > > + * bus reset > > + * The core sets this bit to indicate that a reset is detected on the > > USB. > > + */ > > + if (Ints & GINTSTS_USBRST) { > > + WRITE_REG32 (DCFG, DCFG_DESCDMA | DCFG_NZ_STS_OUT_HSHK); > > + ResetEndpoints (); > > + } > > + > > + /* > > + * enumeration done, we now know the speed > > + * The core sets this bit to indicate that speed enumeration is > > complete. The > > + * application must read the Device Status (DSTS) register to obtain the > > + * enumerated speed. > > + */ > > + if (Ints & GINTSTS_ENUMDONE) { > > + /* Set up the maximum packet sizes accordingly */ > > + UINTN MaxPacket = UsbDrvPortSpeed () ? USB_BLOCK_HIGH_SPEED_SIZE : > > MAX_PACKET_SIZE_CONTROL; > > Too long lines. Also an initializer from a ternary. > Please split definition from assignment. > OK > > + // > > + // Set Maximum In Packet Size (MPS) > > + // > > + Data = READ_REG32 (DIEPCTL1); > > + Data &= ~DXEPCTL_MPS_MASK; > > + Data |= MaxPacket; > > + WRITE_REG32 (DIEPCTL1, Data); > > + // > > + //Set Maximum Out Packet Size (MPS) > > + // > > + Data = READ_REG32 (DOEPCTL1); > > + Data &= ~DXEPCTL_MPS_MASK; > > + Data |= MaxPacket; > > + WRITE_REG32 (DOEPCTL1, Data); > > + } > > + > > + /* > > + * IN EP event > > + * The core sets this bit to indicate that an interrupt is pending on > > one of the IN > > + * endpoInts of the core (in Device mode). The application must read the > > + * Device All EndpoInts Interrupt (DAINT) register to determine the exact > > + * number of the IN endpoint on which the interrupt occurred, and then > > read > > + * the corresponding Device IN Endpoint-n Interrupt (DIEPINTn) register > > to > > + * determine the exact cause of the interrupt. The application must > > clear the > > + * appropriate status bit in the corresponding DIEPINTn register to > > clear this bit. > > + */ > > + if (Ints & GINTSTS_IEPINT) { > > + HandleEndPointInEvent (); > > + } > > + > > + /* > > + * OUT EP event > > + * The core sets this bit to indicate that an interrupt is pending on > > one of the > > + * OUT endpoints of the core (in Device mode). The application must read > > the > > + * Device All EndpoInts Interrupt (DAINT) register to determine the exact > > + * number of the OUT endpoint on which the interrupt occurred, and then > > read > > + * the corresponding Device OUT Endpoint-n Interrupt (DOEPINTn) register > > + * to determine the exact cause of the interrupt. The application must > > clear the > > + * appropriate status bit in the corresponding DOEPINTn register to > > clear this bit. > > + */ > > Long lines in comments above, please rewrap. > OK > > + if (Ints & GINTSTS_OEPINT) { > > + HandleEndPointOutEvent (); > > + } > > + > > + // > > + // clear ints > > + // > > + WRITE_REG32 (GINTSTS, Ints); > > +} > > + > > +EFI_STATUS > > +DwUsbSend ( > > + IN UINT8 EndpointIndex, > > + IN UINTN Size, > > + IN CONST VOID *Buffer > > + ) > > +{ > > + EndPointTx (EndpointIndex, Buffer, Size); > > + return EFI_SUCCESS; > > +} > > + > > +STATIC > > +VOID > > +DwUsbInit ( > > + VOID > > + ) > > +{ > > + VOID *Buf; > > + UINT32 Data; > > + EFI_STATUS Status; > > + > > + Status = DmaAllocateBuffer ( > > + EfiBootServicesData, > > + DWUSB_BUFFER_PAGE_NUM, > > + (VOID *)&Buf > > + ); > > + if (EFI_ERROR (Status)) { > > + return; > > + } > > + gDmaDesc = Buf; > > + gDmaDescEp0 = gDmaDesc + sizeof (DwUsbDevDmaDesc); > > + gDmaDescIn = gDmaDescEp0 + sizeof (DwUsbDevDmaDesc); > > + gCtrlReq = (USB_DEVICE_REQUEST *)gDmaDescIn + sizeof (DwUsbDevDmaDesc); > > + > > + ZeroMem (Buf, EFI_PAGE_SIZE * DWUSB_BUFFER_PAGE_NUM); > > + > > + /* Reset usb controller.*/ > > + /* Wait for OTG AHB master idle */ > > + do { > > + Data = READ_REG32 (GRSTCTL) & GRSTCTL_AHBIDLE; > > + } while (Data == 0); > > + > > + /* OTG: Assert Software Reset */ > > + WRITE_REG32 (GRSTCTL, GRSTCTL_CSFTRST); > > + > > + /* Wait for OTG to ack reset */ > > + while ((READ_REG32 (GRSTCTL) & GRSTCTL_CSFTRST) == GRSTCTL_CSFTRST); > > + > > + /* Wait for OTG AHB master idle */ > > + while ((READ_REG32 (GRSTCTL) & GRSTCTL_AHBIDLE) == 0); > > + > > + WRITE_REG32 (GDFIFOCFG, DATA_FIFO_CONFIG); > > + WRITE_REG32 (GRXFSIZ, RX_SIZE); > > + WRITE_REG32 (GNPTXFSIZ, ENDPOINT_TX_SIZE); > > + WRITE_REG32 (DIEPTXF1, DATA_IN_ENDPOINT_TX_FIFO1); > > + WRITE_REG32 (DIEPTXF2, DATA_IN_ENDPOINT_TX_FIFO2); > > + WRITE_REG32 (DIEPTXF3, DATA_IN_ENDPOINT_TX_FIFO3); > > + WRITE_REG32 (DIEPTXF4, DATA_IN_ENDPOINT_TX_FIFO4); > > + WRITE_REG32 (DIEPTXF5, DATA_IN_ENDPOINT_TX_FIFO5); > > + WRITE_REG32 (DIEPTXF6, DATA_IN_ENDPOINT_TX_FIFO6); > > + WRITE_REG32 (DIEPTXF7, DATA_IN_ENDPOINT_TX_FIFO7); > > + WRITE_REG32 (DIEPTXF8, DATA_IN_ENDPOINT_TX_FIFO8); > > + WRITE_REG32 (DIEPTXF9, DATA_IN_ENDPOINT_TX_FIFO9); > > + WRITE_REG32 (DIEPTXF10, DATA_IN_ENDPOINT_TX_FIFO10); > > + WRITE_REG32 (DIEPTXF11, DATA_IN_ENDPOINT_TX_FIFO11); > > + WRITE_REG32 (DIEPTXF12, DATA_IN_ENDPOINT_TX_FIFO12); > > + WRITE_REG32 (DIEPTXF13, DATA_IN_ENDPOINT_TX_FIFO13); > > + WRITE_REG32 (DIEPTXF14, DATA_IN_ENDPOINT_TX_FIFO14); > > + WRITE_REG32 (DIEPTXF15, DATA_IN_ENDPOINT_TX_FIFO15); > > + > > + /* > > + * set Periodic TxFIFO Empty Level, > > + * Non-Periodic TxFIFO Empty Level, > > + * Enable DMA, Unmask Global Intr > > + */ > > + WRITE_REG32 (GAHBCFG, GAHBCFG_CTRL_MASK); > > + > > + /*select 8bit UTMI+, ULPI Inerface*/ > > + WRITE_REG32 (GUSBCFG, GUSBCFG_USBTRDTIM_8BIT); > > + > > + /* Detect usb work mode,host or device? */ > > + do { > > + Data = READ_REG32 (GINTSTS); > > + } while (Data & GINTSTS_CURMODE_HOST); > > + > > + /* Init global and device mode csr register. */ > > + /* set Non-Zero-Length status out handshake */ > > + Data = (0x20 << DCFG_EPMISCNT_SHIFT) | DCFG_NZ_STS_OUT_HSHK; > > Why 0x20? > I'll add comment on it. Best Regards Haojian _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
