[Linaro-uefi,v2,6/9] Platforms/Hisilicon/HiKey: support designware USB controller

Message ID 1486649511-4149-7-git-send-email-haojian.zhuang@linaro.org
State New
Headers show
Series
  • enable Android Fastboot App on Hikey
Related show

Commit Message

Haojian Zhuang Feb. 9, 2017, 2:11 p.m.
Support Designware USB device controller on HiKey platform.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c      | 266 +++++++++++++++++++++
 .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf    |  46 ++++
 2 files changed, 312 insertions(+)
 create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
 create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf

Comments

Leif Lindholm Feb. 10, 2017, 11:37 a.m. | #1
On Thu, Feb 09, 2017 at 10:11:48PM +0800, Haojian Zhuang wrote:
> Support Designware USB device controller on HiKey platform.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c      | 266 +++++++++++++++++++++
>  .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf    |  46 ++++
>  2 files changed, 312 insertions(+)
>  create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>  create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
> 
> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
> new file mode 100644
> index 0000000..60ad4d6
> --- /dev/null
> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
> @@ -0,0 +1,266 @@
> +/** @file
> +*
> +*  Copyright (c) 2015-2017, 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 <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/TimerLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +
> +#include <Protocol/EmbeddedGpio.h>
> +#include <Protocol/DwUsb.h>
> +
> +#include <Hi6220.h>
> +
> +
> +#define USB_SEL_GPIO0_3          3     // GPIO 0_3
> +#define USB_5V_HUB_EN            7     // GPIO 0_7
> +#define USB_ID_DET_GPIO2_5       21    // GPIO 2_5
> +#define USB_VBUS_DET_GPIO2_6     22    // GPIO 2_6
> +
> +// Jumper on pin5-6 of J15 determines whether boot to fastboot
> +#define DETECT_J15_FASTBOOT      24    // GPIO 3_0
> +
> +STATIC EMBEDDED_GPIO *mGpio;
> +
> +STATIC
> +VOID
> +HiKeyDetectUsbModeInit (
> +  IN VOID
> +  )
> +{
> +  EFI_STATUS     Status;
> +
> +  /* set pullup on both GPIO2_5 & GPIO2_6. It's required for inupt. */
> +  MmioWrite32 (0xf8001864, 1);
> +  MmioWrite32 (0xf8001868, 1);

No magic values please, add #defines.

> +
> +  Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&mGpio);
> +  ASSERT_EFI_ERROR (Status);

This should unconditionally print an error and return if failed.
Also, it should probably return error and be handled at the call site,
instead of continuing as if nothing had happened.

> +  Status = mGpio->Set (mGpio, USB_SEL_GPIO0_3, GPIO_MODE_OUTPUT_0);
> +  ASSERT_EFI_ERROR (Status);
> +  Status = mGpio->Set (mGpio, USB_5V_HUB_EN, GPIO_MODE_OUTPUT_0);
> +  ASSERT_EFI_ERROR (Status);
> +  MicroSecondDelay (1000);

Why this delay? Is a barrier needed? Why was 1ms chosen as the delay?

> +
> +  Status = mGpio->Set (mGpio, USB_ID_DET_GPIO2_5, GPIO_MODE_INPUT);
> +  ASSERT_EFI_ERROR (Status);
> +  Status = mGpio->Set (mGpio, USB_VBUS_DET_GPIO2_6, GPIO_MODE_INPUT);
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +UINTN
> +HiKeyGetUsbMode (
> +  IN VOID
> +  )
> +{
> +#if 0

Please don't include commented-out code blocks.

> +  EFI_STATUS     Status;
> +  UINTN          GpioId, GpioVbus;
> +  UINTN          Value;
> +
> +  Status = mGpio->Get (mGpio, USB_ID_DET_GPIO2_5, &Value);
> +  ASSERT_EFI_ERROR (Status);
> +  GpioId = Value;
> +  Status = mGpio->Get (mGpio, USB_VBUS_DET_GPIO2_6, &Value);
> +  ASSERT_EFI_ERROR (Status);
> +  GpioVbus = Value;
> +
> +DEBUG ((DEBUG_ERROR, "#%a, %d, GpioId:%d, GpioVbus:%d\n", __func__, __LINE__, GpioId, GpioVbus));
> +  if ((GpioId == 1) && (GpioVbus == 0)) {
> +    return USB_DEVICE_MODE;
> +  } else if ((GpioId == 0) && (GpioVbus == 1)) {
> +    return USB_CABLE_NOT_ATTACHED;
> +  }
> +  return USB_HOST_MODE;
> +#else
> +  return USB_DEVICE_MODE;
> +#endif
> +}
> +
> +EFI_STATUS
> +HiKeyUsbPhyInit (
> +  IN UINT8        Mode
> +  )
> +{
> +  UINTN         Value;
> +  UINT32        Data;
> +
> +  HiKeyDetectUsbModeInit ();
> +
> +  //setup clock
> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CLKEN0, BIT4);

What does BIT4 mean in this context?
Can a #define with a more descriptive name be created?

> +  do {
> +       Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CLKSTAT0);
> +  } while ((Value & BIT4) == 0);
> +
> +  //setup phy
> +  Data = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
> +           RST0_USBOTG | RST0_USBOTG_32K;
> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_RSTDIS0, Data);
> +  do {
> +    Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_RSTSTAT0);
> +    Value &= Data;
> +  } while (Value);
> +
> +  Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4);
> +  Value &= ~(CTRL4_PICO_SIDDQ | CTRL4_FPGA_EXT_PHY_SEL |
> +             CTRL4_OTG_PHY_SEL);
> +  Value |=  CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL;
> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4, Value);
> +  MicroSecondDelay (1000);

Why this delay? Is a barrier needed? Why was 1ms chosen as the delay?

> +
> +  //If Mode = 1, USB in Device Mode
> +  //If Mode = 0, USB in Host Mode

You have introduced #defines for these, so don't need to list.
Could just say:
// Verify that controller is in expected mode

> +  if (Mode == USB_DEVICE_MODE) {
> +    if (HiKeyGetUsbMode () == USB_DEVICE_MODE) {
> +      DEBUG ((DEBUG_ERROR, "usb work as device mode.\n"));
> +    } else {
> +      return EFI_INVALID_PARAMETER;
> +    }

May be cleaner written as:

    if (HiKeyGetUsbMode () != USB_DEVICE_MODE) {
      return EFI_INVALID_PARAMETER;
    }

    DEBUG ((DEBUG_ERROR, "usb work as device mode.\n"));

But also, please don't use DEBUG_ERROR for non-error conditions.

> +
> +     Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
> +     Value &= ~CTRL5_PICOPHY_BC_MODE;
> +     MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Value);
> +     MicroSecondDelay (20000);

Why this delay? Is a barrier needed? Why was 20ms chosen as the delay?

> +  } else {
> +    if (HiKeyGetUsbMode () == USB_HOST_MODE) {
> +      DEBUG ((DEBUG_ERROR, "usb work as host mode.\n"));

Please don't use DEBUG_ERROR for non-error conditions.

> +    } else {
> +      return EFI_INVALID_PARAMETER;
> +    }

Ah, but here the pattern repeats again.
So actually, you could do:

  if (HiKeyGetUsbMode () != Mode) {
    return EFI_INVALID_PARAMETER;
  }

once

and then
  if (Mode == USB_DEVICE_MODE) {
    ...
  } else {
    ...
  }

> +
> +    /*CTRL5*/
> +    Data = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
> +    Data &= ~CTRL5_PICOPHY_BC_MODE;
> +    Data |= CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB |
> +            CTRL5_PICOPHY_VDATDETENB | CTRL5_PICOPHY_DCDENB;
> +    MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Data);
> +    MicroSecondDelay (20000);

Why this delay? Is a barrier needed? Why was 20ms chosen as the delay?

> +    MmioWrite32 (PERI_CTRL_BASE + 0x018, 0x70533483); //EYE_PATTERN

No magic values, please.
(What's an eye pattern?)

> +
> +    MicroSecondDelay (5000);

Why this delay? Is a barrier needed? Why was 5ms chosen as the delay?

> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyUsbGetLang (
> +  OUT CHAR16            *Lang,
> +  OUT UINT8             *Length
> +  )
> +{
> +  if ((Lang == NULL) || (Length == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  Lang[0] = 0x409;

No magic values, please.

> +  *Length = sizeof (CHAR16);
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyUsbGetManuFacturer (

Manufacturer is a single word, so the 'f' should not be capitalised
and the #define should be "MANUFACTURER_". (To be corrected also in
3/9, where I missed it on the first pass.)

> +  OUT CHAR16            *ManuFacturer,
> +  OUT UINT8             *Length
> +  )
> +{
> +  UINTN                  VariableSize;
> +  CHAR16                 DataUnicode[MANU_FACTURER_STRING_LENGTH];
> +
> +  if ((ManuFacturer == NULL) || (Length == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  VariableSize = MANU_FACTURER_STRING_LENGTH * sizeof (CHAR16);
> +  ZeroMem (DataUnicode, MANU_FACTURER_STRING_LENGTH * sizeof(CHAR16));
> +  StrCpy (DataUnicode, L"96Boards");
> +  CopyMem (ManuFacturer, DataUnicode, VariableSize);
> +  *Length = VariableSize;
> +  return EFI_SUCCESS;
> +}

OK, the mechanism used here to set manufacturer, product and serial
strings is hideously over-complicated.

In fact, ignore my comment that the string lenghts should be corrected
also in 3/9 - they should never exist as defines in the first place.

This file holds all of the information about which strings are going
into the data structure, and that should not be exported anywhere.

These functions should return a pointer to a string allocated
dynamically in this file (or NULL if allocation fails).
Since these functions create '0'-terminated strings. the length can
then be determined using StrLen.

> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyUsbGetProduct (
> +  OUT CHAR16            *Product,
> +  OUT UINT8             *Length
> +  )
> +{
> +  UINTN                  VariableSize;
> +  CHAR16                 DataUnicode[PRODUCT_STRING_LENGTH];
> +
> +  if ((Product == NULL) || (Length == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  VariableSize = PRODUCT_STRING_LENGTH * sizeof (CHAR16);
> +  ZeroMem (DataUnicode, PRODUCT_STRING_LENGTH * sizeof(CHAR16));
> +  StrCpy (DataUnicode, L"HiKey");
> +  CopyMem (Product, DataUnicode, VariableSize);
> +  *Length = VariableSize;
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyUsbGetSerialNo (
> +  OUT CHAR16            *SerialNo,
> +  OUT UINT8             *Length
> +  )
> +{
> +  UINTN                  VariableSize;
> +  CHAR16                 DataUnicode[SERIAL_STRING_LENGTH];
> +
> +  if ((SerialNo == NULL) || (Length == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  VariableSize = SERIAL_STRING_LENGTH * sizeof (CHAR16);
> +  ZeroMem (DataUnicode, SERIAL_STRING_LENGTH * sizeof(CHAR16));
> +  StrCpy (DataUnicode, L"0123456789abcdef");

A hardcoded serial number will make you no friends in validation labs.
I would suggest that as a start, this fake serial number is broken out
to be retreived from a helper function (which for now can return only
a made up one). Then at a later date, you can figure out how to
provide it with some data that lets you actually tell different
devices apart.

> +  CopyMem (SerialNo, DataUnicode, VariableSize);
> +  *Length = VariableSize;
> +  return EFI_SUCCESS;
> +}
> +
> +DW_USB_PROTOCOL mDwUsbDevice = {
> +  HiKeyUsbGetLang,
> +  HiKeyUsbGetManuFacturer,
> +  HiKeyUsbGetProduct,
> +  HiKeyUsbGetSerialNo,
> +  HiKeyUsbPhyInit
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyUsbEntryPoint (
> +  IN EFI_HANDLE                            ImageHandle,
> +  IN EFI_SYSTEM_TABLE                      *SystemTable
> +  )
> +{
> +  EFI_STATUS        Status;
> +
> +  Status = gBS->InstallProtocolInterface (
> +                  &ImageHandle,
> +                  &gDwUsbProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  &mDwUsbDevice
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }

The code returns status anyway, so this EFI_ERROR test has no purpose.

> +  return Status;
> +}
> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
> new file mode 100644
> index 0000000..1cb70c9
> --- /dev/null
> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
> @@ -0,0 +1,46 @@
> +#/** @file
> +#
> +#  Copyright (c) 2015-2017, 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                      = HiKeyUsbDxe
> +  FILE_GUID                      = c5c7089e-9b00-448c-8b23-a552688e2833
> +  MODULE_TYPE                    = UEFI_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = HiKeyUsbEntryPoint
> +
> +[Sources.common]
> +  HiKeyUsbDxe.c
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> +  TimerLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Protocols]
> +  gDwUsbProtocolGuid
> +  gEfiDriverBindingProtocolGuid
> +  gEmbeddedGpioProtocolGuid
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  OpenPlatformPkg/Drivers/Usb/DwUsbDxe/DwUsbDxe.dec
> +  OpenPlatformPkg/Platforms/Hisilicon/HiKey/HiKey.dec

Please sort packages alphabetically.

> +
> +[Depex]
> +  TRUE
> -- 
> 2.7.4
>
Ard Biesheuvel Feb. 10, 2017, 11:45 a.m. | #2
> On 10 Feb 2017, at 11:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 
>> On Thu, Feb 09, 2017 at 10:11:48PM +0800, Haojian Zhuang wrote:
>> Support Designware USB device controller on HiKey platform.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>> .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c      | 266 +++++++++++++++++++++
>> .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf    |  46 ++++
>> 2 files changed, 312 insertions(+)
>> create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
>> 
>> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> new file mode 100644
>> index 0000000..60ad4d6
>> --- /dev/null
>> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> @@ -0,0 +1,266 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2015-2017, 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 <Library/BaseMemoryLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/IoLib.h>
>> +#include <Library/TimerLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
>> +
>> +#include <Protocol/EmbeddedGpio.h>
>> +#include <Protocol/DwUsb.h>
>> +
>> +#include <Hi6220.h>
>> +
>> +
>> +#define USB_SEL_GPIO0_3          3     // GPIO 0_3
>> +#define USB_5V_HUB_EN            7     // GPIO 0_7
>> +#define USB_ID_DET_GPIO2_5       21    // GPIO 2_5
>> +#define USB_VBUS_DET_GPIO2_6     22    // GPIO 2_6
>> +
>> +// Jumper on pin5-6 of J15 determines whether boot to fastboot
>> +#define DETECT_J15_FASTBOOT      24    // GPIO 3_0
>> +
>> +STATIC EMBEDDED_GPIO *mGpio;
>> +
>> +STATIC
>> +VOID
>> +HiKeyDetectUsbModeInit (
>> +  IN VOID
>> +  )
>> +{
>> +  EFI_STATUS     Status;
>> +
>> +  /* set pullup on both GPIO2_5 & GPIO2_6. It's required for inupt. */
>> +  MmioWrite32 (0xf8001864, 1);
>> +  MmioWrite32 (0xf8001868, 1);
> 
> No magic values please, add #defines.
> 
>> +
>> +  Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&mGpio);
>> +  ASSERT_EFI_ERROR (Status);
> 
> This should unconditionally print an error and return if failed.
> Also, it should probably return error and be handled at the call site,
> instead of continuing as if nothing had happened.
> 

If this driver has a depex on this protocol, it is reasonable (and idiomatic in edk2) to use this pattern imo

>> +  Status = mGpio->Set (mGpio, USB_SEL_GPIO0_3, GPIO_MODE_OUTPUT_0);
>> +  ASSERT_EFI_ERROR (Status);
>> +  Status = mGpio->Set (mGpio, USB_5V_HUB_EN, GPIO_MODE_OUTPUT_0);
>> +  ASSERT_EFI_ERROR (Status);
>> +  MicroSecondDelay (1000);
> 
> Why this delay? Is a barrier needed? Why was 1ms chosen as the delay?
> 
>> +
>> +  Status = mGpio->Set (mGpio, USB_ID_DET_GPIO2_5, GPIO_MODE_INPUT);
>> +  ASSERT_EFI_ERROR (Status);
>> +  Status = mGpio->Set (mGpio, USB_VBUS_DET_GPIO2_6, GPIO_MODE_INPUT);
>> +  ASSERT_EFI_ERROR (Status);
>> +}
>> +
>> +UINTN
>> +HiKeyGetUsbMode (
>> +  IN VOID
>> +  )
>> +{
>> +#if 0
> 
> Please don't include commented-out code blocks.
> 
>> +  EFI_STATUS     Status;
>> +  UINTN          GpioId, GpioVbus;
>> +  UINTN          Value;
>> +
>> +  Status = mGpio->Get (mGpio, USB_ID_DET_GPIO2_5, &Value);
>> +  ASSERT_EFI_ERROR (Status);
>> +  GpioId = Value;
>> +  Status = mGpio->Get (mGpio, USB_VBUS_DET_GPIO2_6, &Value);
>> +  ASSERT_EFI_ERROR (Status);
>> +  GpioVbus = Value;
>> +
>> +DEBUG ((DEBUG_ERROR, "#%a, %d, GpioId:%d, GpioVbus:%d\n", __func__, __LINE__, GpioId, GpioVbus));
>> +  if ((GpioId == 1) && (GpioVbus == 0)) {
>> +    return USB_DEVICE_MODE;
>> +  } else if ((GpioId == 0) && (GpioVbus == 1)) {
>> +    return USB_CABLE_NOT_ATTACHED;
>> +  }
>> +  return USB_HOST_MODE;
>> +#else
>> +  return USB_DEVICE_MODE;
>> +#endif
>> +}
>> +
>> +EFI_STATUS
>> +HiKeyUsbPhyInit (
>> +  IN UINT8        Mode
>> +  )
>> +{
>> +  UINTN         Value;
>> +  UINT32        Data;
>> +
>> +  HiKeyDetectUsbModeInit ();
>> +
>> +  //setup clock
>> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CLKEN0, BIT4);
> 
> What does BIT4 mean in this context?
> Can a #define with a more descriptive name be created?
> 
>> +  do {
>> +       Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CLKSTAT0);
>> +  } while ((Value & BIT4) == 0);
>> +
>> +  //setup phy
>> +  Data = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
>> +           RST0_USBOTG | RST0_USBOTG_32K;
>> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_RSTDIS0, Data);
>> +  do {
>> +    Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_RSTSTAT0);
>> +    Value &= Data;
>> +  } while (Value);
>> +
>> +  Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4);
>> +  Value &= ~(CTRL4_PICO_SIDDQ | CTRL4_FPGA_EXT_PHY_SEL |
>> +             CTRL4_OTG_PHY_SEL);
>> +  Value |=  CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL;
>> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4, Value);
>> +  MicroSecondDelay (1000);
> 
> Why this delay? Is a barrier needed? Why was 1ms chosen as the delay?
> 
>> +
>> +  //If Mode = 1, USB in Device Mode
>> +  //If Mode = 0, USB in Host Mode
> 
> You have introduced #defines for these, so don't need to list.
> Could just say:
> // Verify that controller is in expected mode
> 
>> +  if (Mode == USB_DEVICE_MODE) {
>> +    if (HiKeyGetUsbMode () == USB_DEVICE_MODE) {
>> +      DEBUG ((DEBUG_ERROR, "usb work as device mode.\n"));
>> +    } else {
>> +      return EFI_INVALID_PARAMETER;
>> +    }
> 
> May be cleaner written as:
> 
>    if (HiKeyGetUsbMode () != USB_DEVICE_MODE) {
>      return EFI_INVALID_PARAMETER;
>    }
> 
>    DEBUG ((DEBUG_ERROR, "usb work as device mode.\n"));
> 
> But also, please don't use DEBUG_ERROR for non-error conditions.
> 
>> +
>> +     Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
>> +     Value &= ~CTRL5_PICOPHY_BC_MODE;
>> +     MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Value);
>> +     MicroSecondDelay (20000);
> 
> Why this delay? Is a barrier needed? Why was 20ms chosen as the delay?
> 
>> +  } else {
>> +    if (HiKeyGetUsbMode () == USB_HOST_MODE) {
>> +      DEBUG ((DEBUG_ERROR, "usb work as host mode.\n"));
> 
> Please don't use DEBUG_ERROR for non-error conditions.
> 
>> +    } else {
>> +      return EFI_INVALID_PARAMETER;
>> +    }
> 
> Ah, but here the pattern repeats again.
> So actually, you could do:
> 
>  if (HiKeyGetUsbMode () != Mode) {
>    return EFI_INVALID_PARAMETER;
>  }
> 
> once
> 
> and then
>  if (Mode == USB_DEVICE_MODE) {
>    ...
>  } else {
>    ...
>  }
> 
>> +
>> +    /*CTRL5*/
>> +    Data = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
>> +    Data &= ~CTRL5_PICOPHY_BC_MODE;
>> +    Data |= CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB |
>> +            CTRL5_PICOPHY_VDATDETENB | CTRL5_PICOPHY_DCDENB;
>> +    MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Data);
>> +    MicroSecondDelay (20000);
> 
> Why this delay? Is a barrier needed? Why was 20ms chosen as the delay?
> 
>> +    MmioWrite32 (PERI_CTRL_BASE + 0x018, 0x70533483); //EYE_PATTERN
> 
> No magic values, please.
> (What's an eye pattern?)
> 
>> +
>> +    MicroSecondDelay (5000);
> 
> Why this delay? Is a barrier needed? Why was 5ms chosen as the delay?
> 
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +HiKeyUsbGetLang (
>> +  OUT CHAR16            *Lang,
>> +  OUT UINT8             *Length
>> +  )
>> +{
>> +  if ((Lang == NULL) || (Length == NULL)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  Lang[0] = 0x409;
> 
> No magic values, please.
> 
>> +  *Length = sizeof (CHAR16);
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +HiKeyUsbGetManuFacturer (
> 
> Manufacturer is a single word, so the 'f' should not be capitalised
> and the #define should be "MANUFACTURER_". (To be corrected also in
> 3/9, where I missed it on the first pass.)
> 
>> +  OUT CHAR16            *ManuFacturer,
>> +  OUT UINT8             *Length
>> +  )
>> +{
>> +  UINTN                  VariableSize;
>> +  CHAR16                 DataUnicode[MANU_FACTURER_STRING_LENGTH];
>> +
>> +  if ((ManuFacturer == NULL) || (Length == NULL)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  VariableSize = MANU_FACTURER_STRING_LENGTH * sizeof (CHAR16);
>> +  ZeroMem (DataUnicode, MANU_FACTURER_STRING_LENGTH * sizeof(CHAR16));
>> +  StrCpy (DataUnicode, L"96Boards");
>> +  CopyMem (ManuFacturer, DataUnicode, VariableSize);
>> +  *Length = VariableSize;
>> +  return EFI_SUCCESS;
>> +}
> 
> OK, the mechanism used here to set manufacturer, product and serial
> strings is hideously over-complicated.
> 
> In fact, ignore my comment that the string lenghts should be corrected
> also in 3/9 - they should never exist as defines in the first place.
> 
> This file holds all of the information about which strings are going
> into the data structure, and that should not be exported anywhere.
> 
> These functions should return a pointer to a string allocated
> dynamically in this file (or NULL if allocation fails).
> Since these functions create '0'-terminated strings. the length can
> then be determined using StrLen.
> 
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +HiKeyUsbGetProduct (
>> +  OUT CHAR16            *Product,
>> +  OUT UINT8             *Length
>> +  )
>> +{
>> +  UINTN                  VariableSize;
>> +  CHAR16                 DataUnicode[PRODUCT_STRING_LENGTH];
>> +
>> +  if ((Product == NULL) || (Length == NULL)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  VariableSize = PRODUCT_STRING_LENGTH * sizeof (CHAR16);
>> +  ZeroMem (DataUnicode, PRODUCT_STRING_LENGTH * sizeof(CHAR16));
>> +  StrCpy (DataUnicode, L"HiKey");
>> +  CopyMem (Product, DataUnicode, VariableSize);
>> +  *Length = VariableSize;
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +HiKeyUsbGetSerialNo (
>> +  OUT CHAR16            *SerialNo,
>> +  OUT UINT8             *Length
>> +  )
>> +{
>> +  UINTN                  VariableSize;
>> +  CHAR16                 DataUnicode[SERIAL_STRING_LENGTH];
>> +
>> +  if ((SerialNo == NULL) || (Length == NULL)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  VariableSize = SERIAL_STRING_LENGTH * sizeof (CHAR16);
>> +  ZeroMem (DataUnicode, SERIAL_STRING_LENGTH * sizeof(CHAR16));
>> +  StrCpy (DataUnicode, L"0123456789abcdef");
> 
> A hardcoded serial number will make you no friends in validation labs.
> I would suggest that as a start, this fake serial number is broken out
> to be retreived from a helper function (which for now can return only
> a made up one). Then at a later date, you can figure out how to
> provide it with some data that lets you actually tell different
> devices apart.
> 
>> +  CopyMem (SerialNo, DataUnicode, VariableSize);
>> +  *Length = VariableSize;
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +DW_USB_PROTOCOL mDwUsbDevice = {
>> +  HiKeyUsbGetLang,
>> +  HiKeyUsbGetManuFacturer,
>> +  HiKeyUsbGetProduct,
>> +  HiKeyUsbGetSerialNo,
>> +  HiKeyUsbPhyInit
>> +};
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +HiKeyUsbEntryPoint (
>> +  IN EFI_HANDLE                            ImageHandle,
>> +  IN EFI_SYSTEM_TABLE                      *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS        Status;
>> +
>> +  Status = gBS->InstallProtocolInterface (
>> +                  &ImageHandle,
>> +                  &gDwUsbProtocolGuid,
>> +                  EFI_NATIVE_INTERFACE,
>> +                  &mDwUsbDevice
>> +                  );
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
> 
> The code returns status anyway, so this EFI_ERROR test has no purpose.
> 
>> +  return Status;
>> +}
>> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
>> new file mode 100644
>> index 0000000..1cb70c9
>> --- /dev/null
>> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
>> @@ -0,0 +1,46 @@
>> +#/** @file
>> +#
>> +#  Copyright (c) 2015-2017, 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                      = HiKeyUsbDxe
>> +  FILE_GUID                      = c5c7089e-9b00-448c-8b23-a552688e2833
>> +  MODULE_TYPE                    = UEFI_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  ENTRY_POINT                    = HiKeyUsbEntryPoint
>> +
>> +[Sources.common]
>> +  HiKeyUsbDxe.c
>> +
>> +[LibraryClasses]
>> +  DebugLib
>> +  IoLib
>> +  TimerLib
>> +  UefiBootServicesTableLib
>> +  UefiDriverEntryPoint
>> +
>> +[Protocols]
>> +  gDwUsbProtocolGuid
>> +  gEfiDriverBindingProtocolGuid
>> +  gEmbeddedGpioProtocolGuid
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  OpenPlatformPkg/Drivers/Usb/DwUsbDxe/DwUsbDxe.dec
>> +  OpenPlatformPkg/Platforms/Hisilicon/HiKey/HiKey.dec
> 
> Please sort packages alphabetically.
> 
>> +
>> +[Depex]
>> +  TRUE
>> -- 
>> 2.7.4
>>
Leif Lindholm Feb. 10, 2017, 1:32 p.m. | #3
On Fri, Feb 10, 2017 at 11:45:20AM +0000, Ard Biesheuvel wrote:
> 
> > On 10 Feb 2017, at 11:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > 
> >> On Thu, Feb 09, 2017 at 10:11:48PM +0800, Haojian Zhuang wrote:
> >> Support Designware USB device controller on HiKey platform.
> >> 
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> >> ---
> >> .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c      | 266 +++++++++++++++++++++
> >> .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf    |  46 ++++
> >> 2 files changed, 312 insertions(+)
> >> create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
> >> create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
> >> 
> >> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
> >> new file mode 100644
> >> index 0000000..60ad4d6
> >> --- /dev/null
> >> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
> >> @@ -0,0 +1,266 @@
> >> +/** @file
> >> +*
> >> +*  Copyright (c) 2015-2017, 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 <Library/BaseMemoryLib.h>
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/IoLib.h>
> >> +#include <Library/TimerLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +#include <Library/UefiLib.h>
> >> +#include <Library/UefiRuntimeServicesTableLib.h>
> >> +
> >> +#include <Protocol/EmbeddedGpio.h>
> >> +#include <Protocol/DwUsb.h>
> >> +
> >> +#include <Hi6220.h>
> >> +
> >> +
> >> +#define USB_SEL_GPIO0_3          3     // GPIO 0_3
> >> +#define USB_5V_HUB_EN            7     // GPIO 0_7
> >> +#define USB_ID_DET_GPIO2_5       21    // GPIO 2_5
> >> +#define USB_VBUS_DET_GPIO2_6     22    // GPIO 2_6
> >> +
> >> +// Jumper on pin5-6 of J15 determines whether boot to fastboot
> >> +#define DETECT_J15_FASTBOOT      24    // GPIO 3_0
> >> +
> >> +STATIC EMBEDDED_GPIO *mGpio;
> >> +
> >> +STATIC
> >> +VOID
> >> +HiKeyDetectUsbModeInit (
> >> +  IN VOID
> >> +  )
> >> +{
> >> +  EFI_STATUS     Status;
> >> +
> >> +  /* set pullup on both GPIO2_5 & GPIO2_6. It's required for inupt. */
> >> +  MmioWrite32 (0xf8001864, 1);
> >> +  MmioWrite32 (0xf8001868, 1);
> > 
> > No magic values please, add #defines.
> > 
> >> +
> >> +  Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&mGpio);
> >> +  ASSERT_EFI_ERROR (Status);
> > 
> > This should unconditionally print an error and return if failed.
> > Also, it should probably return error and be handled at the call site,
> > instead of continuing as if nothing had happened.
> 
> If this driver has a depex on this protocol, it is reasonable (and
> idiomatic in edk2) to use this pattern imo

Right, I didn't notice that aspect.
On the basis that this is an "impossible" error?

I still don't feel great about _not_ returning before dereferencing a
NULL pointer, and I see both ways of dealing with this in edk2.

But I think if the code stays as it is, there should at least be a
comment added - I shouldn't need to refer to other files in order to
determine whether a condition is impossible or not.

/
    Leif

> >> +  Status = mGpio->Set (mGpio, USB_SEL_GPIO0_3, GPIO_MODE_OUTPUT_0);
> >> +  ASSERT_EFI_ERROR (Status);
> >> +  Status = mGpio->Set (mGpio, USB_5V_HUB_EN, GPIO_MODE_OUTPUT_0);
> >> +  ASSERT_EFI_ERROR (Status);
> >> +  MicroSecondDelay (1000);
> > 
> > Why this delay? Is a barrier needed? Why was 1ms chosen as the delay?
> > 
> >> +
> >> +  Status = mGpio->Set (mGpio, USB_ID_DET_GPIO2_5, GPIO_MODE_INPUT);
> >> +  ASSERT_EFI_ERROR (Status);
> >> +  Status = mGpio->Set (mGpio, USB_VBUS_DET_GPIO2_6, GPIO_MODE_INPUT);
> >> +  ASSERT_EFI_ERROR (Status);
> >> +}
> >> +
> >> +UINTN
> >> +HiKeyGetUsbMode (
> >> +  IN VOID
> >> +  )
> >> +{
> >> +#if 0
> > 
> > Please don't include commented-out code blocks.
> > 
> >> +  EFI_STATUS     Status;
> >> +  UINTN          GpioId, GpioVbus;
> >> +  UINTN          Value;
> >> +
> >> +  Status = mGpio->Get (mGpio, USB_ID_DET_GPIO2_5, &Value);
> >> +  ASSERT_EFI_ERROR (Status);
> >> +  GpioId = Value;
> >> +  Status = mGpio->Get (mGpio, USB_VBUS_DET_GPIO2_6, &Value);
> >> +  ASSERT_EFI_ERROR (Status);
> >> +  GpioVbus = Value;
> >> +
> >> +DEBUG ((DEBUG_ERROR, "#%a, %d, GpioId:%d, GpioVbus:%d\n", __func__, __LINE__, GpioId, GpioVbus));
> >> +  if ((GpioId == 1) && (GpioVbus == 0)) {
> >> +    return USB_DEVICE_MODE;
> >> +  } else if ((GpioId == 0) && (GpioVbus == 1)) {
> >> +    return USB_CABLE_NOT_ATTACHED;
> >> +  }
> >> +  return USB_HOST_MODE;
> >> +#else
> >> +  return USB_DEVICE_MODE;
> >> +#endif
> >> +}
> >> +
> >> +EFI_STATUS
> >> +HiKeyUsbPhyInit (
> >> +  IN UINT8        Mode
> >> +  )
> >> +{
> >> +  UINTN         Value;
> >> +  UINT32        Data;
> >> +
> >> +  HiKeyDetectUsbModeInit ();
> >> +
> >> +  //setup clock
> >> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CLKEN0, BIT4);
> > 
> > What does BIT4 mean in this context?
> > Can a #define with a more descriptive name be created?
> > 
> >> +  do {
> >> +       Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CLKSTAT0);
> >> +  } while ((Value & BIT4) == 0);
> >> +
> >> +  //setup phy
> >> +  Data = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
> >> +           RST0_USBOTG | RST0_USBOTG_32K;
> >> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_RSTDIS0, Data);
> >> +  do {
> >> +    Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_RSTSTAT0);
> >> +    Value &= Data;
> >> +  } while (Value);
> >> +
> >> +  Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4);
> >> +  Value &= ~(CTRL4_PICO_SIDDQ | CTRL4_FPGA_EXT_PHY_SEL |
> >> +             CTRL4_OTG_PHY_SEL);
> >> +  Value |=  CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL;
> >> +  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4, Value);
> >> +  MicroSecondDelay (1000);
> > 
> > Why this delay? Is a barrier needed? Why was 1ms chosen as the delay?
> > 
> >> +
> >> +  //If Mode = 1, USB in Device Mode
> >> +  //If Mode = 0, USB in Host Mode
> > 
> > You have introduced #defines for these, so don't need to list.
> > Could just say:
> > // Verify that controller is in expected mode
> > 
> >> +  if (Mode == USB_DEVICE_MODE) {
> >> +    if (HiKeyGetUsbMode () == USB_DEVICE_MODE) {
> >> +      DEBUG ((DEBUG_ERROR, "usb work as device mode.\n"));
> >> +    } else {
> >> +      return EFI_INVALID_PARAMETER;
> >> +    }
> > 
> > May be cleaner written as:
> > 
> >    if (HiKeyGetUsbMode () != USB_DEVICE_MODE) {
> >      return EFI_INVALID_PARAMETER;
> >    }
> > 
> >    DEBUG ((DEBUG_ERROR, "usb work as device mode.\n"));
> > 
> > But also, please don't use DEBUG_ERROR for non-error conditions.
> > 
> >> +
> >> +     Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
> >> +     Value &= ~CTRL5_PICOPHY_BC_MODE;
> >> +     MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Value);
> >> +     MicroSecondDelay (20000);
> > 
> > Why this delay? Is a barrier needed? Why was 20ms chosen as the delay?
> > 
> >> +  } else {
> >> +    if (HiKeyGetUsbMode () == USB_HOST_MODE) {
> >> +      DEBUG ((DEBUG_ERROR, "usb work as host mode.\n"));
> > 
> > Please don't use DEBUG_ERROR for non-error conditions.
> > 
> >> +    } else {
> >> +      return EFI_INVALID_PARAMETER;
> >> +    }
> > 
> > Ah, but here the pattern repeats again.
> > So actually, you could do:
> > 
> >  if (HiKeyGetUsbMode () != Mode) {
> >    return EFI_INVALID_PARAMETER;
> >  }
> > 
> > once
> > 
> > and then
> >  if (Mode == USB_DEVICE_MODE) {
> >    ...
> >  } else {
> >    ...
> >  }
> > 
> >> +
> >> +    /*CTRL5*/
> >> +    Data = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
> >> +    Data &= ~CTRL5_PICOPHY_BC_MODE;
> >> +    Data |= CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB |
> >> +            CTRL5_PICOPHY_VDATDETENB | CTRL5_PICOPHY_DCDENB;
> >> +    MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Data);
> >> +    MicroSecondDelay (20000);
> > 
> > Why this delay? Is a barrier needed? Why was 20ms chosen as the delay?
> > 
> >> +    MmioWrite32 (PERI_CTRL_BASE + 0x018, 0x70533483); //EYE_PATTERN
> > 
> > No magic values, please.
> > (What's an eye pattern?)
> > 
> >> +
> >> +    MicroSecondDelay (5000);
> > 
> > Why this delay? Is a barrier needed? Why was 5ms chosen as the delay?
> > 
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +HiKeyUsbGetLang (
> >> +  OUT CHAR16            *Lang,
> >> +  OUT UINT8             *Length
> >> +  )
> >> +{
> >> +  if ((Lang == NULL) || (Length == NULL)) {
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +  Lang[0] = 0x409;
> > 
> > No magic values, please.
> > 
> >> +  *Length = sizeof (CHAR16);
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +HiKeyUsbGetManuFacturer (
> > 
> > Manufacturer is a single word, so the 'f' should not be capitalised
> > and the #define should be "MANUFACTURER_". (To be corrected also in
> > 3/9, where I missed it on the first pass.)
> > 
> >> +  OUT CHAR16            *ManuFacturer,
> >> +  OUT UINT8             *Length
> >> +  )
> >> +{
> >> +  UINTN                  VariableSize;
> >> +  CHAR16                 DataUnicode[MANU_FACTURER_STRING_LENGTH];
> >> +
> >> +  if ((ManuFacturer == NULL) || (Length == NULL)) {
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +  VariableSize = MANU_FACTURER_STRING_LENGTH * sizeof (CHAR16);
> >> +  ZeroMem (DataUnicode, MANU_FACTURER_STRING_LENGTH * sizeof(CHAR16));
> >> +  StrCpy (DataUnicode, L"96Boards");
> >> +  CopyMem (ManuFacturer, DataUnicode, VariableSize);
> >> +  *Length = VariableSize;
> >> +  return EFI_SUCCESS;
> >> +}
> > 
> > OK, the mechanism used here to set manufacturer, product and serial
> > strings is hideously over-complicated.
> > 
> > In fact, ignore my comment that the string lenghts should be corrected
> > also in 3/9 - they should never exist as defines in the first place.
> > 
> > This file holds all of the information about which strings are going
> > into the data structure, and that should not be exported anywhere.
> > 
> > These functions should return a pointer to a string allocated
> > dynamically in this file (or NULL if allocation fails).
> > Since these functions create '0'-terminated strings. the length can
> > then be determined using StrLen.
> > 
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +HiKeyUsbGetProduct (
> >> +  OUT CHAR16            *Product,
> >> +  OUT UINT8             *Length
> >> +  )
> >> +{
> >> +  UINTN                  VariableSize;
> >> +  CHAR16                 DataUnicode[PRODUCT_STRING_LENGTH];
> >> +
> >> +  if ((Product == NULL) || (Length == NULL)) {
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +  VariableSize = PRODUCT_STRING_LENGTH * sizeof (CHAR16);
> >> +  ZeroMem (DataUnicode, PRODUCT_STRING_LENGTH * sizeof(CHAR16));
> >> +  StrCpy (DataUnicode, L"HiKey");
> >> +  CopyMem (Product, DataUnicode, VariableSize);
> >> +  *Length = VariableSize;
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +HiKeyUsbGetSerialNo (
> >> +  OUT CHAR16            *SerialNo,
> >> +  OUT UINT8             *Length
> >> +  )
> >> +{
> >> +  UINTN                  VariableSize;
> >> +  CHAR16                 DataUnicode[SERIAL_STRING_LENGTH];
> >> +
> >> +  if ((SerialNo == NULL) || (Length == NULL)) {
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +  VariableSize = SERIAL_STRING_LENGTH * sizeof (CHAR16);
> >> +  ZeroMem (DataUnicode, SERIAL_STRING_LENGTH * sizeof(CHAR16));
> >> +  StrCpy (DataUnicode, L"0123456789abcdef");
> > 
> > A hardcoded serial number will make you no friends in validation labs.
> > I would suggest that as a start, this fake serial number is broken out
> > to be retreived from a helper function (which for now can return only
> > a made up one). Then at a later date, you can figure out how to
> > provide it with some data that lets you actually tell different
> > devices apart.
> > 
> >> +  CopyMem (SerialNo, DataUnicode, VariableSize);
> >> +  *Length = VariableSize;
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +DW_USB_PROTOCOL mDwUsbDevice = {
> >> +  HiKeyUsbGetLang,
> >> +  HiKeyUsbGetManuFacturer,
> >> +  HiKeyUsbGetProduct,
> >> +  HiKeyUsbGetSerialNo,
> >> +  HiKeyUsbPhyInit
> >> +};
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +HiKeyUsbEntryPoint (
> >> +  IN EFI_HANDLE                            ImageHandle,
> >> +  IN EFI_SYSTEM_TABLE                      *SystemTable
> >> +  )
> >> +{
> >> +  EFI_STATUS        Status;
> >> +
> >> +  Status = gBS->InstallProtocolInterface (
> >> +                  &ImageHandle,
> >> +                  &gDwUsbProtocolGuid,
> >> +                  EFI_NATIVE_INTERFACE,
> >> +                  &mDwUsbDevice
> >> +                  );
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> > 
> > The code returns status anyway, so this EFI_ERROR test has no purpose.
> > 
> >> +  return Status;
> >> +}
> >> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
> >> new file mode 100644
> >> index 0000000..1cb70c9
> >> --- /dev/null
> >> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
> >> @@ -0,0 +1,46 @@
> >> +#/** @file
> >> +#
> >> +#  Copyright (c) 2015-2017, 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                      = HiKeyUsbDxe
> >> +  FILE_GUID                      = c5c7089e-9b00-448c-8b23-a552688e2833
> >> +  MODULE_TYPE                    = UEFI_DRIVER
> >> +  VERSION_STRING                 = 1.0
> >> +  ENTRY_POINT                    = HiKeyUsbEntryPoint
> >> +
> >> +[Sources.common]
> >> +  HiKeyUsbDxe.c
> >> +
> >> +[LibraryClasses]
> >> +  DebugLib
> >> +  IoLib
> >> +  TimerLib
> >> +  UefiBootServicesTableLib
> >> +  UefiDriverEntryPoint
> >> +
> >> +[Protocols]
> >> +  gDwUsbProtocolGuid
> >> +  gEfiDriverBindingProtocolGuid
> >> +  gEmbeddedGpioProtocolGuid
> >> +
> >> +[Packages]
> >> +  MdePkg/MdePkg.dec
> >> +  MdeModulePkg/MdeModulePkg.dec
> >> +  EmbeddedPkg/EmbeddedPkg.dec
> >> +  OpenPlatformPkg/Drivers/Usb/DwUsbDxe/DwUsbDxe.dec
> >> +  OpenPlatformPkg/Platforms/Hisilicon/HiKey/HiKey.dec
> > 
> > Please sort packages alphabetically.
> > 
> >> +
> >> +[Depex]
> >> +  TRUE
> >> -- 
> >> 2.7.4
> >>
Ard Biesheuvel Feb. 10, 2017, 2:16 p.m. | #4
On 10 February 2017 at 13:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Feb 10, 2017 at 11:45:20AM +0000, Ard Biesheuvel wrote:
>>
>> > On 10 Feb 2017, at 11:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> >
>> >> On Thu, Feb 09, 2017 at 10:11:48PM +0800, Haojian Zhuang wrote:
>> >> Support Designware USB device controller on HiKey platform.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.0
>> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> >> ---
>> >> .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c      | 266 +++++++++++++++++++++
>> >> .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf    |  46 ++++
>> >> 2 files changed, 312 insertions(+)
>> >> create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> >> create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
>> >>
>> >> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> >> new file mode 100644
>> >> index 0000000..60ad4d6
>> >> --- /dev/null
>> >> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> >> @@ -0,0 +1,266 @@
>> >> +/** @file
>> >> +*
>> >> +*  Copyright (c) 2015-2017, 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 <Library/BaseMemoryLib.h>
>> >> +#include <Library/DebugLib.h>
>> >> +#include <Library/IoLib.h>
>> >> +#include <Library/TimerLib.h>
>> >> +#include <Library/UefiBootServicesTableLib.h>
>> >> +#include <Library/UefiLib.h>
>> >> +#include <Library/UefiRuntimeServicesTableLib.h>
>> >> +
>> >> +#include <Protocol/EmbeddedGpio.h>
>> >> +#include <Protocol/DwUsb.h>
>> >> +
>> >> +#include <Hi6220.h>
>> >> +
>> >> +
>> >> +#define USB_SEL_GPIO0_3          3     // GPIO 0_3
>> >> +#define USB_5V_HUB_EN            7     // GPIO 0_7
>> >> +#define USB_ID_DET_GPIO2_5       21    // GPIO 2_5
>> >> +#define USB_VBUS_DET_GPIO2_6     22    // GPIO 2_6
>> >> +
>> >> +// Jumper on pin5-6 of J15 determines whether boot to fastboot
>> >> +#define DETECT_J15_FASTBOOT      24    // GPIO 3_0
>> >> +
>> >> +STATIC EMBEDDED_GPIO *mGpio;
>> >> +
>> >> +STATIC
>> >> +VOID
>> >> +HiKeyDetectUsbModeInit (
>> >> +  IN VOID
>> >> +  )
>> >> +{
>> >> +  EFI_STATUS     Status;
>> >> +
>> >> +  /* set pullup on both GPIO2_5 & GPIO2_6. It's required for inupt. */
>> >> +  MmioWrite32 (0xf8001864, 1);
>> >> +  MmioWrite32 (0xf8001868, 1);
>> >
>> > No magic values please, add #defines.
>> >
>> >> +
>> >> +  Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&mGpio);
>> >> +  ASSERT_EFI_ERROR (Status);
>> >
>> > This should unconditionally print an error and return if failed.
>> > Also, it should probably return error and be handled at the call site,
>> > instead of continuing as if nothing had happened.
>>
>> If this driver has a depex on this protocol, it is reasonable (and
>> idiomatic in edk2) to use this pattern imo
>
> Right, I didn't notice that aspect.
> On the basis that this is an "impossible" error?
>

Yes, but only if you declare the dependency in the [Depex], which this
driver does not do.

The DXE core will only dispatch your driver if the protocol is
installed, so your firmware is *badly* broken if you end up here and
the protocol is not here.

> I still don't feel great about _not_ returning before dereferencing a
> NULL pointer, and I see both ways of dealing with this in edk2.
>
> But I think if the code stays as it is, there should at least be a
> comment added - I shouldn't need to refer to other files in order to
> determine whether a condition is impossible or not.
>

I agree that it makes sense to annotate a LocateProtocol() call as
involving a depex'ed upon protocol.
Haojian Zhuang Feb. 13, 2017, 7:51 a.m. | #5
On 10 February 2017 at 19:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Feb 09, 2017 at 10:11:48PM +0800, Haojian Zhuang wrote:
>> Support Designware USB device controller on HiKey platform.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c      | 266 +++++++++++++++++++++
>>  .../Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf    |  46 ++++
>>  2 files changed, 312 insertions(+)
>>  create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>>  create mode 100644 Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
>>
>> diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> new file mode 100644
>> index 0000000..60ad4d6
>> --- /dev/null
>> +++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +HiKeyUsbGetProduct (
>> +  OUT CHAR16            *Product,
>> +  OUT UINT8             *Length
>> +  )
>> +{
>> +  UINTN                  VariableSize;
>> +  CHAR16                 DataUnicode[PRODUCT_STRING_LENGTH];
>> +
>> +  if ((Product == NULL) || (Length == NULL)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  VariableSize = PRODUCT_STRING_LENGTH * sizeof (CHAR16);
>> +  ZeroMem (DataUnicode, PRODUCT_STRING_LENGTH * sizeof(CHAR16));
>> +  StrCpy (DataUnicode, L"HiKey");
>> +  CopyMem (Product, DataUnicode, VariableSize);
>> +  *Length = VariableSize;
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +HiKeyUsbGetSerialNo (
>> +  OUT CHAR16            *SerialNo,
>> +  OUT UINT8             *Length
>> +  )
>> +{
>> +  UINTN                  VariableSize;
>> +  CHAR16                 DataUnicode[SERIAL_STRING_LENGTH];
>> +
>> +  if ((SerialNo == NULL) || (Length == NULL)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  VariableSize = SERIAL_STRING_LENGTH * sizeof (CHAR16);
>> +  ZeroMem (DataUnicode, SERIAL_STRING_LENGTH * sizeof(CHAR16));
>> +  StrCpy (DataUnicode, L"0123456789abcdef");
>
> A hardcoded serial number will make you no friends in validation labs.
> I would suggest that as a start, this fake serial number is broken out
> to be retreived from a helper function (which for now can return only
> a made up one). Then at a later date, you can figure out how to
> provide it with some data that lets you actually tell different
> devices apart.
>

It's simplified at here. I'll add code to fill custom serial number
later. For this
patch, it's used to make fastboot app working.

Patch hide | download patch | download mbox

diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
new file mode 100644
index 0000000..60ad4d6
--- /dev/null
+++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.c
@@ -0,0 +1,266 @@ 
+/** @file
+*
+*  Copyright (c) 2015-2017, 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 <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/IoLib.h>
+#include <Library/TimerLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
+
+#include <Protocol/EmbeddedGpio.h>
+#include <Protocol/DwUsb.h>
+
+#include <Hi6220.h>
+
+
+#define USB_SEL_GPIO0_3          3     // GPIO 0_3
+#define USB_5V_HUB_EN            7     // GPIO 0_7
+#define USB_ID_DET_GPIO2_5       21    // GPIO 2_5
+#define USB_VBUS_DET_GPIO2_6     22    // GPIO 2_6
+
+// Jumper on pin5-6 of J15 determines whether boot to fastboot
+#define DETECT_J15_FASTBOOT      24    // GPIO 3_0
+
+STATIC EMBEDDED_GPIO *mGpio;
+
+STATIC
+VOID
+HiKeyDetectUsbModeInit (
+  IN VOID
+  )
+{
+  EFI_STATUS     Status;
+
+  /* set pullup on both GPIO2_5 & GPIO2_6. It's required for inupt. */
+  MmioWrite32 (0xf8001864, 1);
+  MmioWrite32 (0xf8001868, 1);
+
+  Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&mGpio);
+  ASSERT_EFI_ERROR (Status);
+  Status = mGpio->Set (mGpio, USB_SEL_GPIO0_3, GPIO_MODE_OUTPUT_0);
+  ASSERT_EFI_ERROR (Status);
+  Status = mGpio->Set (mGpio, USB_5V_HUB_EN, GPIO_MODE_OUTPUT_0);
+  ASSERT_EFI_ERROR (Status);
+  MicroSecondDelay (1000);
+
+  Status = mGpio->Set (mGpio, USB_ID_DET_GPIO2_5, GPIO_MODE_INPUT);
+  ASSERT_EFI_ERROR (Status);
+  Status = mGpio->Set (mGpio, USB_VBUS_DET_GPIO2_6, GPIO_MODE_INPUT);
+  ASSERT_EFI_ERROR (Status);
+}
+
+UINTN
+HiKeyGetUsbMode (
+  IN VOID
+  )
+{
+#if 0
+  EFI_STATUS     Status;
+  UINTN          GpioId, GpioVbus;
+  UINTN          Value;
+
+  Status = mGpio->Get (mGpio, USB_ID_DET_GPIO2_5, &Value);
+  ASSERT_EFI_ERROR (Status);
+  GpioId = Value;
+  Status = mGpio->Get (mGpio, USB_VBUS_DET_GPIO2_6, &Value);
+  ASSERT_EFI_ERROR (Status);
+  GpioVbus = Value;
+
+DEBUG ((DEBUG_ERROR, "#%a, %d, GpioId:%d, GpioVbus:%d\n", __func__, __LINE__, GpioId, GpioVbus));
+  if ((GpioId == 1) && (GpioVbus == 0)) {
+    return USB_DEVICE_MODE;
+  } else if ((GpioId == 0) && (GpioVbus == 1)) {
+    return USB_CABLE_NOT_ATTACHED;
+  }
+  return USB_HOST_MODE;
+#else
+  return USB_DEVICE_MODE;
+#endif
+}
+
+EFI_STATUS
+HiKeyUsbPhyInit (
+  IN UINT8        Mode
+  )
+{
+  UINTN         Value;
+  UINT32        Data;
+
+  HiKeyDetectUsbModeInit ();
+
+  //setup clock
+  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CLKEN0, BIT4);
+  do {
+       Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CLKSTAT0);
+  } while ((Value & BIT4) == 0);
+
+  //setup phy
+  Data = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
+           RST0_USBOTG | RST0_USBOTG_32K;
+  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_RSTDIS0, Data);
+  do {
+    Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_RSTSTAT0);
+    Value &= Data;
+  } while (Value);
+
+  Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4);
+  Value &= ~(CTRL4_PICO_SIDDQ | CTRL4_FPGA_EXT_PHY_SEL |
+             CTRL4_OTG_PHY_SEL);
+  Value |=  CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL;
+  MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL4, Value);
+  MicroSecondDelay (1000);
+
+  //If Mode = 1, USB in Device Mode
+  //If Mode = 0, USB in Host Mode
+  if (Mode == USB_DEVICE_MODE) {
+    if (HiKeyGetUsbMode () == USB_DEVICE_MODE) {
+      DEBUG ((DEBUG_ERROR, "usb work as device mode.\n"));
+    } else {
+      return EFI_INVALID_PARAMETER;
+    }
+
+     Value = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
+     Value &= ~CTRL5_PICOPHY_BC_MODE;
+     MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Value);
+     MicroSecondDelay (20000);
+  } else {
+    if (HiKeyGetUsbMode () == USB_HOST_MODE) {
+      DEBUG ((DEBUG_ERROR, "usb work as host mode.\n"));
+    } else {
+      return EFI_INVALID_PARAMETER;
+    }
+
+    /*CTRL5*/
+    Data = MmioRead32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5);
+    Data &= ~CTRL5_PICOPHY_BC_MODE;
+    Data |= CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB |
+            CTRL5_PICOPHY_VDATDETENB | CTRL5_PICOPHY_DCDENB;
+    MmioWrite32 (PERI_CTRL_BASE + SC_PERIPH_CTRL5, Data);
+    MicroSecondDelay (20000);
+    MmioWrite32 (PERI_CTRL_BASE + 0x018, 0x70533483); //EYE_PATTERN
+
+    MicroSecondDelay (5000);
+  }
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+HiKeyUsbGetLang (
+  OUT CHAR16            *Lang,
+  OUT UINT8             *Length
+  )
+{
+  if ((Lang == NULL) || (Length == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+  Lang[0] = 0x409;
+  *Length = sizeof (CHAR16);
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+HiKeyUsbGetManuFacturer (
+  OUT CHAR16            *ManuFacturer,
+  OUT UINT8             *Length
+  )
+{
+  UINTN                  VariableSize;
+  CHAR16                 DataUnicode[MANU_FACTURER_STRING_LENGTH];
+
+  if ((ManuFacturer == NULL) || (Length == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+  VariableSize = MANU_FACTURER_STRING_LENGTH * sizeof (CHAR16);
+  ZeroMem (DataUnicode, MANU_FACTURER_STRING_LENGTH * sizeof(CHAR16));
+  StrCpy (DataUnicode, L"96Boards");
+  CopyMem (ManuFacturer, DataUnicode, VariableSize);
+  *Length = VariableSize;
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+HiKeyUsbGetProduct (
+  OUT CHAR16            *Product,
+  OUT UINT8             *Length
+  )
+{
+  UINTN                  VariableSize;
+  CHAR16                 DataUnicode[PRODUCT_STRING_LENGTH];
+
+  if ((Product == NULL) || (Length == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+  VariableSize = PRODUCT_STRING_LENGTH * sizeof (CHAR16);
+  ZeroMem (DataUnicode, PRODUCT_STRING_LENGTH * sizeof(CHAR16));
+  StrCpy (DataUnicode, L"HiKey");
+  CopyMem (Product, DataUnicode, VariableSize);
+  *Length = VariableSize;
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+HiKeyUsbGetSerialNo (
+  OUT CHAR16            *SerialNo,
+  OUT UINT8             *Length
+  )
+{
+  UINTN                  VariableSize;
+  CHAR16                 DataUnicode[SERIAL_STRING_LENGTH];
+
+  if ((SerialNo == NULL) || (Length == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+  VariableSize = SERIAL_STRING_LENGTH * sizeof (CHAR16);
+  ZeroMem (DataUnicode, SERIAL_STRING_LENGTH * sizeof(CHAR16));
+  StrCpy (DataUnicode, L"0123456789abcdef");
+  CopyMem (SerialNo, DataUnicode, VariableSize);
+  *Length = VariableSize;
+  return EFI_SUCCESS;
+}
+
+DW_USB_PROTOCOL mDwUsbDevice = {
+  HiKeyUsbGetLang,
+  HiKeyUsbGetManuFacturer,
+  HiKeyUsbGetProduct,
+  HiKeyUsbGetSerialNo,
+  HiKeyUsbPhyInit
+};
+
+EFI_STATUS
+EFIAPI
+HiKeyUsbEntryPoint (
+  IN EFI_HANDLE                            ImageHandle,
+  IN EFI_SYSTEM_TABLE                      *SystemTable
+  )
+{
+  EFI_STATUS        Status;
+
+  Status = gBS->InstallProtocolInterface (
+                  &ImageHandle,
+                  &gDwUsbProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  &mDwUsbDevice
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  return Status;
+}
diff --git a/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
new file mode 100644
index 0000000..1cb70c9
--- /dev/null
+++ b/Platforms/Hisilicon/HiKey/HiKeyUsbDxe/HiKeyUsbDxe.inf
@@ -0,0 +1,46 @@ 
+#/** @file
+#
+#  Copyright (c) 2015-2017, 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                      = HiKeyUsbDxe
+  FILE_GUID                      = c5c7089e-9b00-448c-8b23-a552688e2833
+  MODULE_TYPE                    = UEFI_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = HiKeyUsbEntryPoint
+
+[Sources.common]
+  HiKeyUsbDxe.c
+
+[LibraryClasses]
+  DebugLib
+  IoLib
+  TimerLib
+  UefiBootServicesTableLib
+  UefiDriverEntryPoint
+
+[Protocols]
+  gDwUsbProtocolGuid
+  gEfiDriverBindingProtocolGuid
+  gEmbeddedGpioProtocolGuid
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
+  OpenPlatformPkg/Drivers/Usb/DwUsbDxe/DwUsbDxe.dec
+  OpenPlatformPkg/Platforms/Hisilicon/HiKey/HiKey.dec
+
+[Depex]
+  TRUE