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 1/1] EmbeddedPkg/Drivers: add DwUsbDxe
Date: Mon, 22 Oct 2018 10:32:16 +0800 [thread overview]
Message-ID: <CAD6h2NQ59Hg1FMnu_YDVfPu7cJGaNQkRshd5z-tO8DK-wh+txA@mail.gmail.com> (raw)
In-Reply-To: <20181005154648.ot3uvjf72uzcmtr7@bivouac.eciton.net>
On Fri, 5 Oct 2018 at 23:46, Leif Lindholm <leif.lindholm@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.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/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
prev parent reply other threads:[~2018-10-22 2:32 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-21 11:35 [PATCH v1 1/1] EmbeddedPkg/Drivers: add DwUsbDxe Haojian Zhuang
2018-10-05 15:46 ` Leif Lindholm
2018-10-22 2:32 ` Haojian Zhuang [this message]
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=CAD6h2NQ59Hg1FMnu_YDVfPu7cJGaNQkRshd5z-tO8DK-wh+txA@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