From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c00::243; helo=mail-pf0-x243.google.com; envelope-from=haojian.zhuang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf0-x243.google.com (mail-pf0-x243.google.com [IPv6:2607:f8b0:400e:c00::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F32BF2034C8DA for ; Sat, 28 Apr 2018 01:28:30 -0700 (PDT) Received: by mail-pf0-x243.google.com with SMTP id f189so3172066pfa.7 for ; Sat, 28 Apr 2018 01:28:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:thread-topic:thread-index:date:message-id :references:in-reply-to:accept-language:content-language :content-transfer-encoding:mime-version; bh=LFofkpThPhzSyMT53SO1BQBXzZGYcmcOfJjhjNVPlNs=; b=QXz3E+TRLulrnQ4HoBWPI5sZE0i6BjQHWHaICZM20rXoqpDjtf6x5pUCYEz02pTB7W MHTBD86Rzd2aaXKzAtspe4z7aQ1M18GFrnVjlPjZlcFBjbxGai+OrnOI5yYu+TjXxOFe ftXz0qv5p0oc1ry+mshCgpsNynZzNpljxitnc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:thread-topic:thread-index :date:message-id:references:in-reply-to:accept-language :content-language:content-transfer-encoding:mime-version; bh=LFofkpThPhzSyMT53SO1BQBXzZGYcmcOfJjhjNVPlNs=; b=sfDPLx3DIR5vKnvZw9uvTFaDcgDkk8WR/UfPLUXJmoEOt6pUmT+U7UNiidrnzYEvkS rjHmV1p+HsTWI8yQoSDjB89xsC2O9MO9SfakE9dbTBW/IZcacx+JHyJp+3vpYW4G5Hg7 u1i3o37v6dIBSQmcHn5wPZzZeHMFwVWBeYXjd++CKXn99wClbHbr8O7ze9OxFeTln4L1 2al8yAxa/mmsc5OcRY5JJJLs+yGZvvUXsnozS9fzFsyI+niMXlvkuZGKYdB7uaXI8fxh DEtWr9J0CAbYMulTjKLnHRziBCIEGTnvlb5opO42ZOWmZw4kkpWEtR4y/tdVrl3SPAcA mDPw== X-Gm-Message-State: ALQs6tCodN/VeKsfvgHaDEP5zjfsr6ZMTSLx355aiLDAcZg+osCbyHY0 59EoOt88IIN+1hO9AeljKWkGZQ== X-Google-Smtp-Source: AB8JxZpAqe9ZkkVrTs7kRnU9PKZb/2JuiO279wH2CvZVx2zEBaAnEg9BG7nDtE/PvdxkOIW/M5G+Kw== X-Received: by 2002:a17:902:9344:: with SMTP id g4-v6mr5306672plp.10.1524904110354; Sat, 28 Apr 2018 01:28:30 -0700 (PDT) Received: from CY1PR15MB0730.namprd15.prod.outlook.com ([132.245.253.237]) by smtp.gmail.com with ESMTPSA id j74sm6246062pfk.25.2018.04.28.01.28.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 28 Apr 2018 01:28:29 -0700 (PDT) From: Haojian Zhuang To: Ard Biesheuvel , Haojian Zhuang CC: "edk2-devel@lists.01.org" , Leif Lindholm Thread-Topic: [edk2][PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey Thread-Index: AQHT3rEvJ438AsMqREmpBVP3z5nYrqQVv7SAgAAYjl4= X-MS-Exchange-MessageSentRepresentingType: 2 Date: Sat, 28 Apr 2018 08:28:27 +0000 Message-ID: References: <1524893061-3080-1-git-send-email-haojian.zhuang@linaro.org>, In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-Exchange-Organization-SCL: -1 X-MS-TNEF-Correlator: X-MS-Exchange-Organization-RecordReviewCfmType: 0 MIME-Version: 1.0 Subject: Re: [PATCH edk2-platforms 1/2] Platform/HiKey: enable SD/MMC controller on HiKey X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Apr 2018 08:28:31 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ard, Thanks a lot. I'll update it with your comments. Best Regards Haojian ________________________________________ From: Ard Biesheuvel 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 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 > Cc: Ard Biesheuvel > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Haojian Zhuang > --- > 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/HiKe= y/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/P= rePiHobListPointerLib.inf > > [BuildOptions] > - GCC:*_*_*_PLATFORM_FLAGS =3D -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/I= nclude -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include > + GCC:*_*_*_PLATFORM_FLAGS =3D -I$(WORKSPACE)/Silicon/Hisilicon/Hi6220/I= nclude -I$(WORKSPACE)/Platform/Hisilicon/HiKey/Include -I$(WORKSPACE)/ArmPk= g/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/NonDiscoverablePciDev= iceDxe.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/HiKe= y/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/NonDiscoverablePc= iDeviceDxe.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/Hisi= licon/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 > #include > #include > +#include > #include > #include > #include > @@ -378,6 +379,31 @@ HiKeyEntryPoint ( > return Status; > } > > + Status =3D RegisterNonDiscoverableMmioDevice ( > + NonDiscoverableDeviceTypeSdhci, > + NonDiscoverableDeviceDmaTypeNonCoherent, > + NULL, > + NULL, > + 1, > + 0xF723D000, // eMMC > + SIZE_4KB > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Status =3D RegisterNonDiscoverableMmioDevice ( > + NonDiscoverableDeviceTypeSdhci, > + NonDiscoverableDeviceDmaTypeNonCoherent, > + NULL, > + NULL, > + 1, > + 0xF723E000, // SD > + SIZE_4KB > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > Status =3D gBS->InstallProtocolInterface ( > &ImageHandle, > &gPlatformVirtualKeyboardProtocolGuid, > diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf b/Platform/Hi= silicon/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/Platfor= m/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 ma= y 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#include > + > +#define DETECT_SD_CARD 8 // GPIO 1_0 > + > +DW_MMC_HC_SLOT_CAP > +DwMmcCapability[2] =3D { > + { > + .Ddr50 =3D 1, > + .HighSpeed =3D 1, > + .BusWidth =3D 8, > + .SlotType =3D EmbeddedSlot, > + .CardType =3D EmmcCardType, > + .BaseClkFreq =3D 100000 > + }, { > + .HighSpeed =3D 1, > + .BusWidth =3D 4, > + .SlotType =3D RemovableSlot, > + .CardType =3D SdCardType, > + .Voltage30 =3D 1, > + .BaseClkFreq =3D 100000 > + } > +}; > + > +EFI_STATUS > +EFIAPI > +HiKeyGetCapability ( > + IN EFI_HANDLE Controller, > + IN UINT8 Slot, > + OUT DW_MMC_HC_SLOT_CAP *Capability > + ) > +{ > + if (Capability =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + if (DwMmcCapability[0].Controller =3D=3D 0) { > + DwMmcCapability[0].Controller =3D Controller; > + CopyMem (Capability, &DwMmcCapability[0], sizeof (DW_MMC_HC_SLOT_CAP= )); > + } else if (DwMmcCapability[0].Controller =3D=3D Controller) { > + CopyMem (Capability, &DwMmcCapability[0], sizeof (DW_MMC_HC_SLOT_CAP= )); > + } else if (DwMmcCapability[1].Controller =3D=3D 0) { > + DwMmcCapability[1].Controller =3D Controller; > + CopyMem (Capability, &DwMmcCapability[1], sizeof (DW_MMC_HC_SLOT_CAP= )); > + } else if (DwMmcCapability[1].Controller =3D=3D 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 =3D=3D Controller) { > + return TRUE; > + } else if (DwMmcCapability[1].Controller =3D=3D Controller) { > + Status =3D gBS->LocateProtocol (&gEmbeddedGpioProtocolGuid, NULL, (V= OID **)&Gpio); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to get GPIO protocol: %r\n", Status))= ; > + return FALSE; > + } > + Status =3D 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", Sta= tus)); > + return FALSE; > + } > + Status =3D 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 =3D=3D 0) { > + return TRUE; > + } > + return FALSE; > + } > + return FALSE; > +} > + > +PLATFORM_DW_MMC_PROTOCOL mDwMmcDevice =3D { > + HiKeyGetCapability, > + HiKeyCardDetect > +}; > + > +EFI_STATUS > +EFIAPI > +HiKeyMmcEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status =3D 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/Platf= orm/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 =3D 0x00010019 > + BASE_NAME =3D HiKeyMmcDxe > + FILE_GUID =3D a4f9bfb1-b3f8-4d4d-8c04-9539173fc1f= 2 > + MODULE_TYPE =3D UEFI_DRIVER > + VERSION_STRING =3D 1.0 > + ENTRY_POINT =3D 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 >