* [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey
@ 2018-04-28 5:24 Haojian Zhuang
2018-04-28 5:24 ` [PATCH edk2-platforms 2/2] Platform/HiKey960: enable SD controller Haojian Zhuang
2018-04-28 7:00 ` [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Ard Biesheuvel
0 siblings, 2 replies; 4+ messages in thread
From: Haojian Zhuang @ 2018-04-28 5:24 UTC (permalink / raw)
To: edk2-devel; +Cc: Haojian Zhuang, Leif Lindholm, Ard Biesheuvel
Replace DwEmmcDxe driver by DwMmcHcDxe driver on HiKey platform. Since
the new driver could work on both eMMC and SD controller.
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/HiKey/HiKey.dsc | 19 ++-
Platform/Hisilicon/HiKey/HiKey.fdf | 15 ++-
Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c | 26 ++++
Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf | 1 +
Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c | 139 +++++++++++++++++++++
.../Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf | 45 +++++++
6 files changed, 228 insertions(+), 17 deletions(-)
create mode 100644 Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
create mode 100644 Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
diff --git a/Platform/Hisilicon/HiKey/HiKey.dsc b/Platform/Hisilicon/HiKey/HiKey.dsc
index 9c1fc2e1b40d..0635e16c4141 100644
--- a/Platform/Hisilicon/HiKey/HiKey.dsc
+++ b/Platform/Hisilicon/HiKey/HiKey.dsc
@@ -69,7 +69,7 @@ [LibraryClasses.common.SEC]
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
[BuildOptions]
- GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/Include -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include
+ GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/Include -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include -I$(WORKSPACE)/ArmPkg/Include -I$(WORKSPACE)/EmbeddedPkg/Include
################################################################################
#
@@ -126,12 +126,6 @@ [PcdsFixedAtBuild.common]
gEmbeddedTokenSpaceGuid.PcdMetronomeTickPeriod|1000
#
- # DW MMC/SD card controller
- #
- gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0xF723D000
- gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|100000000
-
- #
#
# Fastboot
#
@@ -204,13 +198,16 @@ [Components.common]
#
EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
- Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
-
#
# MMC/SD
#
- EmbeddedPkg/Universal/MmcDxe/MmcDxe.inf
- EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
+ Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
+ MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
+ EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
+ MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
+ MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
+
+ Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
#
# USB Host Support
diff --git a/Platform/Hisilicon/HiKey/HiKey.fdf b/Platform/Hisilicon/HiKey/HiKey.fdf
index 2bca7232b6e5..afc6a1a6e6e1 100644
--- a/Platform/Hisilicon/HiKey/HiKey.fdf
+++ b/Platform/Hisilicon/HiKey/HiKey.fdf
@@ -128,15 +128,18 @@ [FV.FvMain]
#
INF EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
+ #
+ # MMC/SD
+ #
+ INF Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
+ INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
+ INF EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
+ INF MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
+ INF MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
+
INF Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
#
- # Multimedia Card Interface
- #
- INF EmbeddedPkg/Universal/MmcDxe/MmcDxe.inf
- INF EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
-
- #
# USB Host Support
#
INF MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf
diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
index fec43f3fb4c1..5f1904cec805 100644
--- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
+++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
@@ -18,6 +18,7 @@
#include <Library/DevicePathLib.h>
#include <Library/IoLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/NonDiscoverableDeviceRegistrationLib.h>
#include <Library/PcdLib.h>
#include <Library/PrintLib.h>
#include <Library/UefiBootManagerLib.h>
@@ -378,6 +379,31 @@ HiKeyEntryPoint (
return Status;
}
+ Status = RegisterNonDiscoverableMmioDevice (
+ NonDiscoverableDeviceTypeSdhci,
+ NonDiscoverableDeviceDmaTypeNonCoherent,
+ NULL,
+ NULL,
+ 1,
+ 0xF723D000, // eMMC
+ SIZE_4KB
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = RegisterNonDiscoverableMmioDevice (
+ NonDiscoverableDeviceTypeSdhci,
+ NonDiscoverableDeviceDmaTypeNonCoherent,
+ NULL,
+ NULL,
+ 1,
+ 0xF723E000, // SD
+ SIZE_4KB
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
Status = gBS->InstallProtocolInterface (
&ImageHandle,
&gPlatformVirtualKeyboardProtocolGuid,
diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
index e97afab8785a..a217e2fb7033 100644
--- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
+++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
@@ -37,6 +37,7 @@ [LibraryClasses]
DxeServicesTableLib
FdtLib
IoLib
+ NonDiscoverableDeviceRegistrationLib
PcdLib
PrintLib
SerialPortLib
diff --git a/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
new file mode 100644
index 000000000000..390c23018bc6
--- /dev/null
+++ b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
@@ -0,0 +1,139 @@
+/** @file
+*
+* Copyright (c) 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/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 <Hi6220.h>
+
+#define DETECT_SD_CARD 8 // GPIO 1_0
+
+DW_MMC_HC_SLOT_CAP
+DwMmcCapability[2] = {
+ {
+ .Ddr50 = 1,
+ .HighSpeed = 1,
+ .BusWidth = 8,
+ .SlotType = EmbeddedSlot,
+ .CardType = EmmcCardType,
+ .BaseClkFreq = 100000
+ }, {
+ .HighSpeed = 1,
+ .BusWidth = 4,
+ .SlotType = RemovableSlot,
+ .CardType = SdCardType,
+ .Voltage30 = 1,
+ .BaseClkFreq = 100000
+ }
+};
+
+EFI_STATUS
+EFIAPI
+HiKeyGetCapability (
+ IN EFI_HANDLE Controller,
+ IN UINT8 Slot,
+ OUT DW_MMC_HC_SLOT_CAP *Capability
+ )
+{
+ if (Capability == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+ if (DwMmcCapability[0].Controller == 0) {
+ 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 if (DwMmcCapability[1].Controller == 0) {
+ DwMmcCapability[1].Controller = Controller;
+ CopyMem (Capability, &DwMmcCapability[1], sizeof (DW_MMC_HC_SLOT_CAP));
+ } else if (DwMmcCapability[1].Controller == Controller) {
+ CopyMem (Capability, &DwMmcCapability[1], sizeof (DW_MMC_HC_SLOT_CAP));
+ } else {
+ return EFI_INVALID_PARAMETER;
+ }
+ return EFI_SUCCESS;
+}
+
+BOOLEAN
+EFIAPI
+HiKeyCardDetect (
+ IN EFI_HANDLE Controller,
+ IN UINT8 Slot
+ )
+{
+ EFI_STATUS Status;
+ EMBEDDED_GPIO *Gpio;
+ UINTN Value;
+
+ if (DwMmcCapability[0].Controller == Controller) {
+ return TRUE;
+ } else if (DwMmcCapability[1].Controller == Controller) {
+ Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&Gpio);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to get GPIO protocol: %r\n", Status));
+ return FALSE;
+ }
+ 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));
+ return FALSE;
+ }
+ Status = Gpio->Get (Gpio, DETECT_SD_CARD, &Value);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Failed to get GPIO value: %r\n", Status));
+ return FALSE;
+ }
+ if (Value == 0) {
+ return TRUE;
+ }
+ return FALSE;
+ }
+ return FALSE;
+}
+
+PLATFORM_DW_MMC_PROTOCOL mDwMmcDevice = {
+ HiKeyGetCapability,
+ HiKeyCardDetect
+};
+
+EFI_STATUS
+EFIAPI
+HiKeyMmcEntryPoint (
+ 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;
+}
diff --git a/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
new file mode 100644
index 000000000000..1b78d3228ccf
--- /dev/null
+++ b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
@@ -0,0 +1,45 @@
+#/** @file
+#
+# Copyright (c) 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 = HiKeyMmcDxe
+ FILE_GUID = a4f9bfb1-b3f8-4d4d-8c04-9539173fc1f2
+ MODULE_TYPE = UEFI_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = HiKeyMmcEntryPoint
+
+[Sources.common]
+ HiKeyMmcDxe.c
+
+[LibraryClasses]
+ DebugLib
+ IoLib
+ TimerLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+
+[Protocols]
+ gEfiDriverBindingProtocolGuid
+ gEmbeddedGpioProtocolGuid
+ gPlatformDwMmcProtocolGuid
+
+[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ OpenPlatformPkg/Drivers/SdMmc/DwMmcHcDxe/DwMmcHcDxe.dec
+
+[Depex]
+ TRUE
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH edk2-platforms 2/2] Platform/HiKey960: enable SD controller
2018-04-28 5:24 [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Haojian Zhuang
@ 2018-04-28 5:24 ` Haojian Zhuang
2018-04-28 7:00 ` [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Ard Biesheuvel
1 sibling, 0 replies; 4+ messages in thread
From: Haojian Zhuang @ 2018-04-28 5:24 UTC (permalink / raw)
To: edk2-devel; +Cc: Haojian Zhuang, Leif Lindholm, Ard Biesheuvel
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 | 10 +-
Platform/Hisilicon/HiKey960/HiKey960.fdf | 8 ++
.../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c | 13 +++
.../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf | 1 +
.../HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.c | 125 +++++++++++++++++++++
.../HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf | 45 ++++++++
6 files changed, 201 insertions(+), 1 deletion(-)
create mode 100644 Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.c
create mode 100644 Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dsc b/Platform/Hisilicon/HiKey960/HiKey960.dsc
index 4097fbfd77f5..646de759d46c 100644
--- a/Platform/Hisilicon/HiKey960/HiKey960.dsc
+++ b/Platform/Hisilicon/HiKey960/HiKey960.dsc
@@ -69,7 +69,7 @@ [LibraryClasses.common.SEC]
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
[BuildOptions]
- GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi3660/Include
+ GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi3660/Include -I$(WORKSPACE)/ArmPkg/Include -I$(WORKSPACE)/EmbeddedPkg/Include
################################################################################
#
@@ -196,6 +196,14 @@ [Components.common]
#
EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
+ #
+ # MMC/SD
+ #
+ Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
+ MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.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..61fd9121ac3e 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 MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.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.c b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
index b6a88daf4f99..cc27f0b9c8f8 100644
--- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
+++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
@@ -641,6 +641,19 @@ HiKey960EntryPoint (
return Status;
}
+ Status = RegisterNonDiscoverableMmioDevice (
+ NonDiscoverableDeviceTypeSdhci,
+ NonDiscoverableDeviceDmaTypeNonCoherent,
+ NULL,
+ NULL,
+ 1,
+ 0xFF37F000, // SD
+ SIZE_4KB
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
Status = gBS->InstallProtocolInterface (
&ImageHandle,
&gPlatformVirtualKeyboardProtocolGuid,
diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
index 5adedd79d3e8..56836590ddea 100644
--- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
+++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
@@ -34,6 +34,7 @@ [LibraryClasses]
DxeServicesTableLib
FdtLib
IoLib
+ NonDiscoverableDeviceRegistrationLib
PcdLib
PrintLib
SerialPortLib
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
+
+DW_MMC_HC_SLOT_CAP
+DwMmcCapability[] = {
+ {
+ .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
+ )
+{
+ if (Capability == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+ if (DwMmcCapability[0].Controller == 0) {
+ 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;
+}
diff --git a/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf b/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
new file mode 100644
index 000000000000..fabafaf479ba
--- /dev/null
+++ b/Platform/Hisilicon/HiKey960/HiKey960MmcDxe/HiKey960MmcDxe.inf
@@ -0,0 +1,45 @@
+#/** @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
+
+[Depex]
+ TRUE
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey
2018-04-28 5:24 [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Haojian Zhuang
2018-04-28 5:24 ` [PATCH edk2-platforms 2/2] Platform/HiKey960: enable SD controller Haojian Zhuang
@ 2018-04-28 7:00 ` Ard Biesheuvel
2018-04-28 8:28 ` Haojian Zhuang
1 sibling, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2018-04-28 7:00 UTC (permalink / raw)
To: Haojian Zhuang; +Cc: edk2-devel@lists.01.org, Leif Lindholm
Hello Haojian,
On 28 April 2018 at 07:24, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
> Replace DwEmmcDxe driver by DwMmcHcDxe driver on HiKey platform. Since
> the new driver could work on both eMMC and SD controller.
>
I am very happy you did this work, since it allows us to move to the
UEFI SD/MMC protocols instead of using the old non-standard one we
have in EmbeddedPkg
However, there are some problems with the approach, please see below.
> 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/HiKey/HiKey.dsc | 19 ++-
> Platform/Hisilicon/HiKey/HiKey.fdf | 15 ++-
> Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c | 26 ++++
> Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf | 1 +
> Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c | 139 +++++++++++++++++++++
> .../Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf | 45 +++++++
> 6 files changed, 228 insertions(+), 17 deletions(-)
> create mode 100644 Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
> create mode 100644 Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
>
> diff --git a/Platform/Hisilicon/HiKey/HiKey.dsc b/Platform/Hisilicon/HiKey/HiKey.dsc
> index 9c1fc2e1b40d..0635e16c4141 100644
> --- a/Platform/Hisilicon/HiKey/HiKey.dsc
> +++ b/Platform/Hisilicon/HiKey/HiKey.dsc
> @@ -69,7 +69,7 @@ [LibraryClasses.common.SEC]
> PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
>
> [BuildOptions]
> - GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/Include -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include
> + GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/Include -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include -I$(WORKSPACE)/ArmPkg/Include -I$(WORKSPACE)/EmbeddedPkg/Include
>
Please drop this. If you need this, it means one of your modules lacks
a reference to EmbeddedPkg.dec in its .inf
> ################################################################################
> #
> @@ -126,12 +126,6 @@ [PcdsFixedAtBuild.common]
> gEmbeddedTokenSpaceGuid.PcdMetronomeTickPeriod|1000
>
> #
> - # DW MMC/SD card controller
> - #
> - gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0xF723D000
> - gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|100000000
> -
> - #
> #
> # Fastboot
> #
> @@ -204,13 +198,16 @@ [Components.common]
> #
> EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
>
> - Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> -
> #
> # MMC/SD
> #
> - EmbeddedPkg/Universal/MmcDxe/MmcDxe.inf
> - EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> + Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> + MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
Please drop this.
You are exposing the SD and MMC controllers as generic SDHCI
controllers, and then abusing the PCI SDHCI class codes to bind your
driver to (in the other patch).
This is incorrect. This device is not a PCI device, and so there is no
need to pretend it is for a new driver.
Instead, you should invent your own non-discoverable device with its
own GUID, and register that directly. Your driver should bind to
gEdkiiNonDiscoverableDeviceProtocolGuid directly, and match the device
type against the GUID you generated for this particular piece of
hardware.
Unfortunately, this implies that you need to rewrite the driver itself
to drop any references to EFI_PCI_IO_PROTOCOL, which is not a small
task. Instead, you should use DmaLib directly for DMA operations, and
discover the base address from the non-discoverable device's resource
descriptor.
The driver itself is a large piece of code, and so both Leif and I
will likely have more detailed review comments as well, but given the
impact of this feedback, I needed to point this out first.
--
Ard.
> + EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
> + MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
> + MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
> +
> + Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
>
> #
> # USB Host Support
> diff --git a/Platform/Hisilicon/HiKey/HiKey.fdf b/Platform/Hisilicon/HiKey/HiKey.fdf
> index 2bca7232b6e5..afc6a1a6e6e1 100644
> --- a/Platform/Hisilicon/HiKey/HiKey.fdf
> +++ b/Platform/Hisilicon/HiKey/HiKey.fdf
> @@ -128,15 +128,18 @@ [FV.FvMain]
> #
> INF EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
>
> + #
> + # MMC/SD
> + #
> + INF Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> + INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
> + INF EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
> + INF MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
> + INF MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
> +
> INF Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
>
> #
> - # Multimedia Card Interface
> - #
> - INF EmbeddedPkg/Universal/MmcDxe/MmcDxe.inf
> - INF EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> -
> - #
> # USB Host Support
> #
> INF MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf
> diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
> index fec43f3fb4c1..5f1904cec805 100644
> --- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
> +++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
> @@ -18,6 +18,7 @@
> #include <Library/DevicePathLib.h>
> #include <Library/IoLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/NonDiscoverableDeviceRegistrationLib.h>
> #include <Library/PcdLib.h>
> #include <Library/PrintLib.h>
> #include <Library/UefiBootManagerLib.h>
> @@ -378,6 +379,31 @@ HiKeyEntryPoint (
> return Status;
> }
>
> + Status = RegisterNonDiscoverableMmioDevice (
> + NonDiscoverableDeviceTypeSdhci,
> + NonDiscoverableDeviceDmaTypeNonCoherent,
> + NULL,
> + NULL,
> + 1,
> + 0xF723D000, // eMMC
> + SIZE_4KB
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = RegisterNonDiscoverableMmioDevice (
> + NonDiscoverableDeviceTypeSdhci,
> + NonDiscoverableDeviceDmaTypeNonCoherent,
> + NULL,
> + NULL,
> + 1,
> + 0xF723E000, // SD
> + SIZE_4KB
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> Status = gBS->InstallProtocolInterface (
> &ImageHandle,
> &gPlatformVirtualKeyboardProtocolGuid,
> diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> index e97afab8785a..a217e2fb7033 100644
> --- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> +++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> @@ -37,6 +37,7 @@ [LibraryClasses]
> DxeServicesTableLib
> FdtLib
> IoLib
> + NonDiscoverableDeviceRegistrationLib
> PcdLib
> PrintLib
> SerialPortLib
> diff --git a/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
> new file mode 100644
> index 000000000000..390c23018bc6
> --- /dev/null
> +++ b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
> @@ -0,0 +1,139 @@
> +/** @file
> +*
> +* Copyright (c) 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/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 <Hi6220.h>
> +
> +#define DETECT_SD_CARD 8 // GPIO 1_0
> +
> +DW_MMC_HC_SLOT_CAP
> +DwMmcCapability[2] = {
> + {
> + .Ddr50 = 1,
> + .HighSpeed = 1,
> + .BusWidth = 8,
> + .SlotType = EmbeddedSlot,
> + .CardType = EmmcCardType,
> + .BaseClkFreq = 100000
> + }, {
> + .HighSpeed = 1,
> + .BusWidth = 4,
> + .SlotType = RemovableSlot,
> + .CardType = SdCardType,
> + .Voltage30 = 1,
> + .BaseClkFreq = 100000
> + }
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyGetCapability (
> + IN EFI_HANDLE Controller,
> + IN UINT8 Slot,
> + OUT DW_MMC_HC_SLOT_CAP *Capability
> + )
> +{
> + if (Capability == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> + if (DwMmcCapability[0].Controller == 0) {
> + 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 if (DwMmcCapability[1].Controller == 0) {
> + DwMmcCapability[1].Controller = Controller;
> + CopyMem (Capability, &DwMmcCapability[1], sizeof (DW_MMC_HC_SLOT_CAP));
> + } else if (DwMmcCapability[1].Controller == Controller) {
> + CopyMem (Capability, &DwMmcCapability[1], sizeof (DW_MMC_HC_SLOT_CAP));
> + } else {
> + return EFI_INVALID_PARAMETER;
> + }
> + return EFI_SUCCESS;
> +}
> +
> +BOOLEAN
> +EFIAPI
> +HiKeyCardDetect (
> + IN EFI_HANDLE Controller,
> + IN UINT8 Slot
> + )
> +{
> + EFI_STATUS Status;
> + EMBEDDED_GPIO *Gpio;
> + UINTN Value;
> +
> + if (DwMmcCapability[0].Controller == Controller) {
> + return TRUE;
> + } else if (DwMmcCapability[1].Controller == Controller) {
> + Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&Gpio);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Failed to get GPIO protocol: %r\n", Status));
> + return FALSE;
> + }
> + 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));
> + return FALSE;
> + }
> + Status = Gpio->Get (Gpio, DETECT_SD_CARD, &Value);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Failed to get GPIO value: %r\n", Status));
> + return FALSE;
> + }
> + if (Value == 0) {
> + return TRUE;
> + }
> + return FALSE;
> + }
> + return FALSE;
> +}
> +
> +PLATFORM_DW_MMC_PROTOCOL mDwMmcDevice = {
> + HiKeyGetCapability,
> + HiKeyCardDetect
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyMmcEntryPoint (
> + 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;
> +}
> diff --git a/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> new file mode 100644
> index 000000000000..1b78d3228ccf
> --- /dev/null
> +++ b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> @@ -0,0 +1,45 @@
> +#/** @file
> +#
> +# Copyright (c) 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 = HiKeyMmcDxe
> + FILE_GUID = a4f9bfb1-b3f8-4d4d-8c04-9539173fc1f2
> + MODULE_TYPE = UEFI_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = HiKeyMmcEntryPoint
> +
> +[Sources.common]
> + HiKeyMmcDxe.c
> +
> +[LibraryClasses]
> + DebugLib
> + IoLib
> + TimerLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> +
> +[Protocols]
> + gEfiDriverBindingProtocolGuid
> + gEmbeddedGpioProtocolGuid
> + gPlatformDwMmcProtocolGuid
> +
> +[Packages]
> + EmbeddedPkg/EmbeddedPkg.dec
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + OpenPlatformPkg/Drivers/SdMmc/DwMmcHcDxe/DwMmcHcDxe.dec
> +
> +[Depex]
> + TRUE
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey
2018-04-28 7:00 ` [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Ard Biesheuvel
@ 2018-04-28 8:28 ` Haojian Zhuang
0 siblings, 0 replies; 4+ messages in thread
From: Haojian Zhuang @ 2018-04-28 8:28 UTC (permalink / raw)
To: Ard Biesheuvel, Haojian Zhuang; +Cc: edk2-devel@lists.01.org, Leif Lindholm
Hi Ard,
Thanks a lot. I'll update it with your comments.
Best Regards
Haojian
________________________________________
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Sent: Saturday, April 28, 2018 7:00 AM
To: Haojian Zhuang
Cc: edk2-devel@lists.01.org; Leif Lindholm
Subject: Re: [edk2][PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey
Hello Haojian,
On 28 April 2018 at 07:24, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
> Replace DwEmmcDxe driver by DwMmcHcDxe driver on HiKey platform. Since
> the new driver could work on both eMMC and SD controller.
>
I am very happy you did this work, since it allows us to move to the
UEFI SD/MMC protocols instead of using the old non-standard one we
have in EmbeddedPkg
However, there are some problems with the approach, please see below.
> 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/HiKey/HiKey.dsc | 19 ++-
> Platform/Hisilicon/HiKey/HiKey.fdf | 15 ++-
> Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c | 26 ++++
> Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf | 1 +
> Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c | 139 +++++++++++++++++++++
> .../Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf | 45 +++++++
> 6 files changed, 228 insertions(+), 17 deletions(-)
> create mode 100644 Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
> create mode 100644 Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
>
> diff --git a/Platform/Hisilicon/HiKey/HiKey.dsc b/Platform/Hisilicon/HiKey/HiKey.dsc
> index 9c1fc2e1b40d..0635e16c4141 100644
> --- a/Platform/Hisilicon/HiKey/HiKey.dsc
> +++ b/Platform/Hisilicon/HiKey/HiKey.dsc
> @@ -69,7 +69,7 @@ [LibraryClasses.common.SEC]
> PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
>
> [BuildOptions]
> - GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/Include -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include
> + GCC:*_*_*_PLATFORM_FLAGS = -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/Include -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include -I$(WORKSPACE)/ArmPkg/Include -I$(WORKSPACE)/EmbeddedPkg/Include
>
Please drop this. If you need this, it means one of your modules lacks
a reference to EmbeddedPkg.dec in its .inf
> ################################################################################
> #
> @@ -126,12 +126,6 @@ [PcdsFixedAtBuild.common]
> gEmbeddedTokenSpaceGuid.PcdMetronomeTickPeriod|1000
>
> #
> - # DW MMC/SD card controller
> - #
> - gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeBaseAddress|0xF723D000
> - gEmbeddedTokenSpaceGuid.PcdDwEmmcDxeClockFrequencyInHz|100000000
> -
> - #
> #
> # Fastboot
> #
> @@ -204,13 +198,16 @@ [Components.common]
> #
> EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
>
> - Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> -
> #
> # MMC/SD
> #
> - EmbeddedPkg/Universal/MmcDxe/MmcDxe.inf
> - EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> + Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> + MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
Please drop this.
You are exposing the SD and MMC controllers as generic SDHCI
controllers, and then abusing the PCI SDHCI class codes to bind your
driver to (in the other patch).
This is incorrect. This device is not a PCI device, and so there is no
need to pretend it is for a new driver.
Instead, you should invent your own non-discoverable device with its
own GUID, and register that directly. Your driver should bind to
gEdkiiNonDiscoverableDeviceProtocolGuid directly, and match the device
type against the GUID you generated for this particular piece of
hardware.
Unfortunately, this implies that you need to rewrite the driver itself
to drop any references to EFI_PCI_IO_PROTOCOL, which is not a small
task. Instead, you should use DmaLib directly for DMA operations, and
discover the base address from the non-discoverable device's resource
descriptor.
The driver itself is a large piece of code, and so both Leif and I
will likely have more detailed review comments as well, but given the
impact of this feedback, I needed to point this out first.
--
Ard.
> + EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
> + MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
> + MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
> +
> + Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
>
> #
> # USB Host Support
> diff --git a/Platform/Hisilicon/HiKey/HiKey.fdf b/Platform/Hisilicon/HiKey/HiKey.fdf
> index 2bca7232b6e5..afc6a1a6e6e1 100644
> --- a/Platform/Hisilicon/HiKey/HiKey.fdf
> +++ b/Platform/Hisilicon/HiKey/HiKey.fdf
> @@ -128,15 +128,18 @@ [FV.FvMain]
> #
> INF EmbeddedPkg/Drivers/VirtualKeyboardDxe/VirtualKeyboardDxe.inf
>
> + #
> + # MMC/SD
> + #
> + INF Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> + INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
> + INF EmbeddedPkg/Drivers/DwMmcHcDxe/DwMmcHcDxe.inf
> + INF MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.inf
> + INF MdeModulePkg/Bus/Sd/SdDxe/SdDxe.inf
> +
> INF Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
>
> #
> - # Multimedia Card Interface
> - #
> - INF EmbeddedPkg/Universal/MmcDxe/MmcDxe.inf
> - INF EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.inf
> -
> - #
> # USB Host Support
> #
> INF MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBusDxe.inf
> diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
> index fec43f3fb4c1..5f1904cec805 100644
> --- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
> +++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
> @@ -18,6 +18,7 @@
> #include <Library/DevicePathLib.h>
> #include <Library/IoLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/NonDiscoverableDeviceRegistrationLib.h>
> #include <Library/PcdLib.h>
> #include <Library/PrintLib.h>
> #include <Library/UefiBootManagerLib.h>
> @@ -378,6 +379,31 @@ HiKeyEntryPoint (
> return Status;
> }
>
> + Status = RegisterNonDiscoverableMmioDevice (
> + NonDiscoverableDeviceTypeSdhci,
> + NonDiscoverableDeviceDmaTypeNonCoherent,
> + NULL,
> + NULL,
> + 1,
> + 0xF723D000, // eMMC
> + SIZE_4KB
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = RegisterNonDiscoverableMmioDevice (
> + NonDiscoverableDeviceTypeSdhci,
> + NonDiscoverableDeviceDmaTypeNonCoherent,
> + NULL,
> + NULL,
> + 1,
> + 0xF723E000, // SD
> + SIZE_4KB
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> Status = gBS->InstallProtocolInterface (
> &ImageHandle,
> &gPlatformVirtualKeyboardProtocolGuid,
> diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> index e97afab8785a..a217e2fb7033 100644
> --- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> +++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
> @@ -37,6 +37,7 @@ [LibraryClasses]
> DxeServicesTableLib
> FdtLib
> IoLib
> + NonDiscoverableDeviceRegistrationLib
> PcdLib
> PrintLib
> SerialPortLib
> diff --git a/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
> new file mode 100644
> index 000000000000..390c23018bc6
> --- /dev/null
> +++ b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.c
> @@ -0,0 +1,139 @@
> +/** @file
> +*
> +* Copyright (c) 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/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 <Hi6220.h>
> +
> +#define DETECT_SD_CARD 8 // GPIO 1_0
> +
> +DW_MMC_HC_SLOT_CAP
> +DwMmcCapability[2] = {
> + {
> + .Ddr50 = 1,
> + .HighSpeed = 1,
> + .BusWidth = 8,
> + .SlotType = EmbeddedSlot,
> + .CardType = EmmcCardType,
> + .BaseClkFreq = 100000
> + }, {
> + .HighSpeed = 1,
> + .BusWidth = 4,
> + .SlotType = RemovableSlot,
> + .CardType = SdCardType,
> + .Voltage30 = 1,
> + .BaseClkFreq = 100000
> + }
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyGetCapability (
> + IN EFI_HANDLE Controller,
> + IN UINT8 Slot,
> + OUT DW_MMC_HC_SLOT_CAP *Capability
> + )
> +{
> + if (Capability == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> + if (DwMmcCapability[0].Controller == 0) {
> + 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 if (DwMmcCapability[1].Controller == 0) {
> + DwMmcCapability[1].Controller = Controller;
> + CopyMem (Capability, &DwMmcCapability[1], sizeof (DW_MMC_HC_SLOT_CAP));
> + } else if (DwMmcCapability[1].Controller == Controller) {
> + CopyMem (Capability, &DwMmcCapability[1], sizeof (DW_MMC_HC_SLOT_CAP));
> + } else {
> + return EFI_INVALID_PARAMETER;
> + }
> + return EFI_SUCCESS;
> +}
> +
> +BOOLEAN
> +EFIAPI
> +HiKeyCardDetect (
> + IN EFI_HANDLE Controller,
> + IN UINT8 Slot
> + )
> +{
> + EFI_STATUS Status;
> + EMBEDDED_GPIO *Gpio;
> + UINTN Value;
> +
> + if (DwMmcCapability[0].Controller == Controller) {
> + return TRUE;
> + } else if (DwMmcCapability[1].Controller == Controller) {
> + Status = gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (VOID **)&Gpio);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Failed to get GPIO protocol: %r\n", Status));
> + return FALSE;
> + }
> + 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));
> + return FALSE;
> + }
> + Status = Gpio->Get (Gpio, DETECT_SD_CARD, &Value);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Failed to get GPIO value: %r\n", Status));
> + return FALSE;
> + }
> + if (Value == 0) {
> + return TRUE;
> + }
> + return FALSE;
> + }
> + return FALSE;
> +}
> +
> +PLATFORM_DW_MMC_PROTOCOL mDwMmcDevice = {
> + HiKeyGetCapability,
> + HiKeyCardDetect
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +HiKeyMmcEntryPoint (
> + 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;
> +}
> diff --git a/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> new file mode 100644
> index 000000000000..1b78d3228ccf
> --- /dev/null
> +++ b/Platform/Hisilicon/HiKey/HiKeyMmcDxe/HiKeyMmcDxe.inf
> @@ -0,0 +1,45 @@
> +#/** @file
> +#
> +# Copyright (c) 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 = HiKeyMmcDxe
> + FILE_GUID = a4f9bfb1-b3f8-4d4d-8c04-9539173fc1f2
> + MODULE_TYPE = UEFI_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = HiKeyMmcEntryPoint
> +
> +[Sources.common]
> + HiKeyMmcDxe.c
> +
> +[LibraryClasses]
> + DebugLib
> + IoLib
> + TimerLib
> + UefiBootServicesTableLib
> + UefiDriverEntryPoint
> +
> +[Protocols]
> + gEfiDriverBindingProtocolGuid
> + gEmbeddedGpioProtocolGuid
> + gPlatformDwMmcProtocolGuid
> +
> +[Packages]
> + EmbeddedPkg/EmbeddedPkg.dec
> + MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + OpenPlatformPkg/Drivers/SdMmc/DwMmcHcDxe/DwMmcHcDxe.dec
> +
> +[Depex]
> + TRUE
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-28 8:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-28 5:24 [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Haojian Zhuang
2018-04-28 5:24 ` [PATCH edk2-platforms 2/2] Platform/HiKey960: enable SD controller Haojian Zhuang
2018-04-28 7:00 ` [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Ard Biesheuvel
2018-04-28 8:28 ` Haojian Zhuang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox