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

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