Re: [PATCH v2 edk-platforms 2/3] Platform/HiKey960: enable SD controller

[Leif Lindholm <leif.lindholm@linaro.org>]

Apologies for ridiculously slow response,

On Thu, Aug 16, 2018 at 09:49:25AM +0800, Haojian Zhuang wrote:
> Enable DwMmcHcDxe driver on HiKey960 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>
> ---
>  Platform/Hisilicon/HiKey960/HiKey960.dsc                      |   8 ++
>  Platform/Hisilicon/HiKey960/HiKey960.fdf                      |   8 ++
>  Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf       |   1 +
>  Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf |  46 +++++++
>  Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c         |  15 +++
>  Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.c   | 125 ++++++++++++++++++++
>  6 files changed, 203 insertions(+)
> 
> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dsc b/Platform/Hisilicon/HiKey960/HiKey960.dsc
> index be13714b9c19..1ce306da5b50 100644
> --- a/Platform/Hisilicon/HiKey960/HiKey960.dsc
> +++ b/Platform/Hisilicon/HiKey960/HiKey960.dsc
> @@ -193,6 +193,14 @@ [Components.common]
>    #
>    EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
>  
> +  #
> +  # MMC/SD
> +  #
> +  Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
> +  EmbeddedPkg/Universal/NonDiscoverableDeviceDxe/NonDiscoverableDeviceDxe.inf
> +  EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
> +  MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
> +
>    Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
>  
>    #
> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.fdf b/Platform/Hisilicon/HiKey960/HiKey960.fdf
> index d65f77878575..d4fe4c46b818 100644
> --- a/Platform/Hisilicon/HiKey960/HiKey960.fdf
> +++ b/Platform/Hisilicon/HiKey960/HiKey960.fdf
> @@ -128,6 +128,14 @@ [FV.FvMain]
>    #
>    INF EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
>  
> +  #
> +  # MMC/SD
> +  #
> +  INF Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
> +  INF EmbeddedPkg/Universal/NonDiscoverableDeviceDxe/NonDiscoverableDeviceDxe.inf
> +  INF EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
> +  INF MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
> +
>    INF Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
>  
>    #
> diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
> index 54105bdb3c94..9d66c028832e 100644
> --- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
> +++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
> @@ -33,6 +33,7 @@ [LibraryClasses]
>    CacheMaintenanceLib
>    DxeServicesTableLib
>    IoLib
> +  NonDiscoverableDeviceRegistrationLib
>    PcdLib
>    TimerLib
>    UefiBootManagerLib
> diff --git a/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf b/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
> new file mode 100644
> index 000000000000..3e4bc807956e
> --- /dev/null
> +++ b/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
> @@ -0,0 +1,46 @@
> +#/** @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                      = HiKey960MmcDxe
> +  FILE_GUID                      = e10d1dea-2f00-4eea-ac5d-9d6cfa7831a0
> +  MODULE_TYPE                    = UEFI_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = HiKey960MmcEntryPoint
> +
> +[Sources.common]
> +  HiKey960MmcDxe.c
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> +  TimerLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Protocols]
> +  gEfiDriverBindingProtocolGuid
> +  gEmbeddedGpioProtocolGuid
> +  gPlatformDwMmcProtocolGuid
> +
> +[Packages]
> +  EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec

Swap order of the two above lines?

> +  Silicon/Hisilicon/Hi3660/Hi3660.dec
> +
> +[Depex]
> +  TRUE
> diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
> index 47ec879861bd..75627d5d6665 100644
> --- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
> +++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
> @@ -23,6 +23,7 @@
>  #include <Library/DevicePathLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/NonDiscoverableDeviceRegistrationLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/SerialPortLib.h>
>  #include <Library/TimerLib.h>
> @@ -33,6 +34,7 @@
>  #include <Protocol/DevicePathFromText.h>
>  #include <Protocol/EmbeddedGpio.h>
>  #include <Protocol/LoadedImage.h>
> +#include <Protocol/NonDiscoverableDevice.h>
>  #include <Protocol/PlatformBootManager.h>
>  #include <Protocol/PlatformVirtualKeyboard.h>
>  
> @@ -447,6 +449,19 @@ HiKey960EntryPoint (
>      return Status;
>    }
>  
> +  Status = RegisterNonDiscoverableMmioDevice (
> +             NonDiscoverableDeviceTypeSdhci,
> +             NonDiscoverableDeviceDmaTypeNonCoherent,
> +             NULL,
> +             NULL,
> +             1,

Can you either make a #define or add a comment explaining why there's
a live-coded integer here?

> +             0xFF37F000, // SD

And could you add the appropriate #defines to
Silicon/Hisilicon/Hi3660/Include/Hi3660.h and use that instead?

> +             SIZE_4KB
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>    Status = gBS->InstallProtocolInterface (
>                    &ImageHandle,
>                    &gPlatformVirtualKeyboardProtocolGuid,
> diff --git a/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.c b/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.c
> new file mode 100644
> index 000000000000..0a2b60edf352
> --- /dev/null
> +++ b/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.c
> @@ -0,0 +1,125 @@
> +/** @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 <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.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/PlatformDwMmc.h>
> +
> +#include <Hi3660.h>
> +
> +#define DETECT_SD_CARD           203     // GPIO 25_3

And could you move this to a local .h file?

> +
> +DW_MMC_HC_SLOT_CAP

Where is this struct defined?

> +DwMmcCapability[] = {

'm' prefix for global variable.
And STATIC unless it is intended for use outside this file.

> +  {
> +    .HighSpeed   = 1,
> +    .BusWidth    = 4,
> +    .SlotType    = RemovableSlot,
> +    .CardType    = SdCardType,
> +    .Voltage30   = 1,
> +    .BaseClkFreq = 3200
> +  }
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +HiKey960GetCapability (
> +  IN     EFI_HANDLE           Controller,
> +  IN     UINT8                Slot,
> +     OUT DW_MMC_HC_SLOT_CAP   *Capability

Is the placement of that OUT intentional?
I don't think I've seen that before, but it sort of makes sense.

> +  )
> +{
> +  if (Capability == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if (DwMmcCapability[0].Controller == 0) {

Pretty sure that should be NULL?
Why is DwMmcCapability being treated as a single-entry array?
And why is Slot ignored?

/
    Leif

> +    DwMmcCapability[0].Controller = Controller;
> +    CopyMem (Capability, &DwMmcCapability[0], sizeof (DW_MMC_HC_SLOT_CAP));
> +  } else if (DwMmcCapability[0].Controller == Controller) {
> +    CopyMem (Capability, &DwMmcCapability[0], sizeof (DW_MMC_HC_SLOT_CAP));
> +  } else {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +BOOLEAN
> +EFIAPI
> +HiKey960CardDetect (
> +  IN EFI_HANDLE               Controller,
> +  IN UINT8                    Slot
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EMBEDDED_GPIO         *Gpio;
> +  UINTN                 Value;
> +
> +  if (Slot == 0) {
> +    Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&Gpio);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failed to get GPIO protocol: %r\n", Status));
> +      goto Exit;
> +    }
> +    Status = Gpio->Set (Gpio, DETECT_SD_CARD, GPIO_MODE_INPUT);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failed to sed GPIO as input mode: %r\n", Status));
> +      goto Exit;
> +    }
> +    Status = Gpio->Get (Gpio, DETECT_SD_CARD, &Value);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failed to get GPIO value: %r\n", Status));
> +      goto Exit;
> +    }
> +    if (Value == 0) {
> +      return TRUE;
> +    }
> +  }
> +Exit:
> +  return FALSE;
> +}
> +
> +PLATFORM_DW_MMC_PROTOCOL mDwMmcDevice = {
> +  HiKey960GetCapability,
> +  HiKey960CardDetect
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +HiKey960MmcEntryPoint (
> +  IN EFI_HANDLE                            ImageHandle,
> +  IN EFI_SYSTEM_TABLE                      *SystemTable
> +  )
> +{
> +  EFI_STATUS        Status;
> +
> +  Status = gBS->InstallProtocolInterface (
> +                  &ImageHandle,
> +                  &gPlatformDwMmcProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  &mDwMmcDevice
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  return Status;
> +}
> -- 
> 2.7.4
>