From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 24E5C20D7B271 for ; Fri, 30 Nov 2018 00:00:39 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Nov 2018 00:00:38 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,297,1539673200"; d="scan'208";a="114110050" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga001.jf.intel.com with ESMTP; 30 Nov 2018 00:00:38 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 30 Nov 2018 00:00:36 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.203]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.201]) with mapi id 14.03.0415.000; Fri, 30 Nov 2018 16:00:04 +0800 From: "Wu, Hao A" To: Ashish Singhal , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support. Thread-Index: AQHUgErAm86gNGlstkq6syHAPA+mEKVbdenwgAsrkYCAATokoA== Date: Fri, 30 Nov 2018 08:00:03 +0000 Message-ID: References: <444ed350c110664e0547692c294473503370d3e4.1542659223.git.ashishsingha@nvidia.com> <81ad37c5653bf3a092789118c3fb8078e0f68644.1542659223.git.ashishsingha@nvidia.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Nov 2018 08:00:40 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Ashish Singhal > Sent: Friday, November 30, 2018 2:48 AM > To: Wu, Hao A; edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add > V4 64bit SDMA and ADMA2 support. >=20 > Hello Hao, >=20 > Answers to your comments: >=20 > 1. Patch 5 fixes the issue with accessing SD as well as eMMC files. Hello, The issue still exists on my side. Please refer to the comments within the V6 2/2 patch. > 2. Using Private variable for controller version would mean I can only ac= cess it > in functions having an instance of the installed protocol which is not po= ssible > in some functions for example where we set clock dividers. We either need > to use what I have or pass Private or controller version as an argument t= o > functions using it. The solution for this is not very complex. I checked all the callers of SdMmcHcGetControllerVersion() in this patch: SdMmcHcClockSupply() SdMmcHcInitV4Enhancements() BuildAdmaDescTable() SdMmcExecTrb() SdMmcCheckTrbResult() For SdMmcExecTrb() and SdMmcCheckTrbResult(), they already have the access = to 'Private'. For SdMmcHcInitV4Enhancements() and BuildAdmaDescTable(), their callers all have the access to 'Private'. So we can either: 1. Add 'Private' to the input parameter 2. Add 'ControllerVersion' to the input parameter for the above 2 functions. Personally, I prefer the 2nd option. As for SdMmcHcClockSupply(), among its callers, only SdMmcHcInitClockFreq() does not have direct access to 'Private'. However, SdMmcHcInitClockFreq() i= s able to get the ControllerVersion or 'Private' from its caller. Hence, ultimately, we can call function SdMmcHcGetControllerVersion() only = once at function SdMmcPciHcDriverBindingStart() right after the SdMmcHcGetMaxCurrent() call. The result can be saved in the 'ControllerVers= ion' field of the "SD_MMC_HC_PRIVATE_DATA" structure. Also, the type of field 'ControllerVersion' in "SD_MMC_HC_PRIVATE_DATA" can= be updated to UINT16 from UINT32. > 3. I think there is a bigger issue here. During host controller initializ= ation we > need to setup 64b addressing which happens before we initialize 64b DMA i= n > PCI layer. So what you are suggesting may not be that helpful. Also, what= we > are doing right now is that we go through all slots and if controller on = any slot > does not support 64b, we do not enable 64b DMA in PCI layer. What is the > likelihood of all controllers on an SOC not supporting similar address wi= dth? I am not sure on this one. All the devices I own seem to only have 1 slot on the controller. And all t= he SD host controllers in a system seem to have the same address width support= . But since this driver is a general one, let us assume there will be such a case here. > Also, shouldn't we be enabling 64b DMA in PCI layer if any of the control= ler > supports 64b addressing? Wouldn't 64b DMA enabled mean 32b is enabled > by default? I think your handling in the proposed v6 1/2 patch is good. Since the Pci IO protocol is managing all the slots on one SD controller, w= hen 64-bit DMA is enabled in the PCI layer, there will be potential issues if o= ne or more slots only support 32-bit system addressing. But since this driver is a general one, let us assume there will be such a case that controllers may have different address width here. > 4. Patch 5 has been rebased on tip of edk2. Thanks. Best Regards, Hao Wu >=20 > Thanks > Ashish >=20 > -----Original Message----- > From: Wu, Hao A > Sent: Wednesday, November 28, 2018 12:25 AM > To: Ashish Singhal ; edk2-devel@lists.01.org > Subject: RE: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 > 64bit SDMA and ADMA2 support. >=20 > Hello, >=20 > Sorry for the delayed response. >=20 > Apart from inserting the Bugzilla tracker information, several more gener= al > level > comments: >=20 > 1. Cannot access the files (content) on SD & eMMC devices >=20 > After applying (rebasing onto the latest codes) your patches, I found tha= t > though the SD & eMMC devices can be detected, yet the content cannot be > accessed. >=20 > There are 1 SD card and 1 embedded eMMC device within the system. Under > Shell, I can see 2 file systems on SD & eMMC devices being detected, but > when I try to access the files on those file systems by using the 'ls' co= mmand, > it fails, saying "ls: File Not Found - 'FS0:\'". I can confirm that files= can be listed > successfully without applying this patch. >=20 > I also tried the 'dblk' command for display the data via BlockIO protocol= s > created on those devices. I found that for SD, the command works fine. Bu= t > for eMMC, the command fails with message "dblk: Read file error - 'BlockI= o'". >=20 > For the hardware I own, the controllers are all version 3.0. I tested on = both > IA32 and X64 arch image. The results were the same as described above. >=20 > Unfortunately, I do not have access to boards with SD controller with ver= sion > newer than 3.0. So I am not able to perform those tests on my side for > Version > 4.0 or greater SD controllers. >=20 > Do you have any hardware with SD controller of version 3.0? Is it working= on > your side? >=20 > Please let me know if additional information is needed. >=20 >=20 > 2. On SdMmcHcGetControllerVersion() >=20 > I found that there is a 'ControllerVersion' version in the data structure > SD_MMC_HC_PRIVATE_DATA. But currently, this field seems never used. >=20 > I think we can get the controller version information only once and use t= his > field to store it. Hence, this new function can be eliminated and also ca= n > avoid calling it multiple times. >=20 >=20 > 3. Setting the SD_MMC_HC_64_ADDR_EN bit in > SdMmcHcV4Init64BitSupport() >=20 > The setting of the SD_MMC_HC_64_ADDR_EN bit is decided by the > 'SysBus64V4' > field in the CAP register. For me, additional dependency on the 64-bit DM= A > support in the PCI layer is needed as well. >=20 > In function SdMmcPciHcDriverBindingStart(): >=20 > // > // Enable 64-bit DMA support in the PCI layer if this controller > // supports it. > // > if (Support64BitDma) { > Status =3D PciIo->Attributes ( > PciIo, > EfiPciIoAttributeOperationEnable, > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, > NULL > ); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_WARN, "SdMmcPciHcDriverBindingStart: failed to > enable 64-bit DMA (%r)\n", Status)); > } > } >=20 > If the above PCI attribute setting fails, the PCI layer will not support = the 64-bit > DMA. >=20 > Hence, I am thinking to introduce a new BOOLEAN field in the > "SD_MMC_HC_PRIVATE_DATA" data structure (maybe putting the > 'Support64BitDma' > into the data structure). If the above PCI operation succeeds, the BOOLEA= N > field will be set to TRUE, otherwise, FALSE. >=20 > And the setting of the SD_MMC_HC_64_ADDR_EN bit should depend on the > BOOLEAN field as well. >=20 >=20 > 4. Please help to rebase the patch upon the latest master branch. >=20 > Some changes within the SdMmcPciHcDxe have been pushed recently. Could > you help to rebase this patch upon the latest changes. Thanks in advance. >=20 >=20 > Also, some inline comments below. >=20 >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Ashish Singhal > > Sent: Tuesday, November 20, 2018 4:59 AM > > To: edk2-devel@lists.01.org > > Cc: Ashish Singhal > > Subject: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 > > 64bit SDMA and ADMA2 support. > > > > If V4 64 bit address mode is enabled in compatibility register, > > program controller to enable V4 host mode. > > Use appropriate ADMA2 descriptors supporting 64 bit addresses. > > Use appropriate registers for SDMA mode operation. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ashish Singhal > > --- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 4 +- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 273 > > +++++++++++++++++---- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 28 ++- > > 3 files changed, 260 insertions(+), 45 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > index c683600..22795df 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > @@ -2,6 +2,7 @@ > > > > Provides some data structure definitions used by the SD/MMC host > > controller driver. > > > > +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved. > > Copyright (c) 2015, Intel Corporation. All rights reserved.
This > > program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License @@ -144,7 > > +145,8 @@ typedef struct { > > BOOLEAN Started; > > UINT64 Timeout; > > > > - SD_MMC_HC_ADMA_DESC_LINE *AdmaDesc; > > + SD_MMC_HC_ADMA_32_DESC_LINE *Adma32Desc; > > + SD_MMC_HC_ADMA_64_DESC_LINE *Adma64Desc; > > EFI_PHYSICAL_ADDRESS AdmaDescPhy; > > VOID *AdmaMap; > > UINT32 AdmaPages; > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > index e506875..9fef3fb 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > @@ -4,6 +4,7 @@ > > > > It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer > use. > > > > + Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved. > > Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved. > > This program and the accompanying materials > > are licensed and made available under the terms and conditions of > > the BSD License @@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet ( } > > > > /** > > + Get the controller version information from the specified slot. > > + > > + @param[in] PciIo The PCI IO protocol instance. > > + @param[in] Slot The slot number of the SD card to send t= he > > command to. > > + @param[out] Version The buffer to store the version informat= ion. > > + > > + @retval EFI_SUCCESS The operation executes successfully. > > + @retval Others The operation fails. > > + > > +**/ > > +EFI_STATUS > > +SdMmcHcGetControllerVersion ( > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > + IN UINT8 Slot, > > + OUT UINT16 *Version > > + ) > > +{ > > + EFI_STATUS Status; > > + > > + Status =3D SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, > > sizeof (UINT16), Version); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + *Version &=3D 0xFF; > > + > > + return EFI_SUCCESS; > > +} > > + > > +/** > > Software reset the specified SD/MMC host controller and enable all > > interrupts. > > > > @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA > > instance. > > @@ -776,18 +807,18 @@ SdMmcHcClockSupply ( > > > > DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d ClockFreq > > %dKhz\n", BaseClkFreq, Divisor, ClockFreq)); > > > > - Status =3D SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE, > > sizeof (ControllerVer), &ControllerVer); > > + Status =3D SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer)= ; > > if (EFI_ERROR (Status)) { > > return Status; > > } > > // > > // Set SDCLK Frequency Select and Internal Clock Enable fields in > > Clock Control register. > > // > > - if (((ControllerVer & 0xFF) >=3D SD_MMC_HC_CTRL_VER_300) && > > - ((ControllerVer & 0xFF) <=3D SD_MMC_HC_CTRL_VER_420)) { > > + if ((ControllerVer >=3D SD_MMC_HC_CTRL_VER_300) && > > + (ControllerVer <=3D SD_MMC_HC_CTRL_VER_420)) { > > ASSERT (Divisor <=3D 0x3FF); > > ClockCtrl =3D ((Divisor & 0xFF) << 8) | ((Divisor & 0x300) >> 2); > > - } else if (((ControllerVer & 0xFF) =3D=3D 0) || ((ControllerVer & 0x= FF) > > =3D=3D 1)) { > > + } else if ((ControllerVer =3D=3D 0) || (ControllerVer =3D=3D 1)) { >=20 > Could you help to update the above 'else if' statement to use the below > version definitions? >=20 > SD_MMC_HC_CTRL_VER_100 > SD_MMC_HC_CTRL_VER_200 >=20 > > // > > // Only the most significant bit can be used as divisor. > > // > > @@ -935,6 +966,54 @@ SdMmcHcSetBusWidth ( } > > > > /** > > + Configure V4 64 bit system address support at initialization. > > + > > + @param[in] PciIo The PCI IO protocol instance. > > + @param[in] Slot The slot number of the SD card to send the > > command to. > > + @param[in] Capability The capability of the slot. > > + > > + @retval EFI_SUCCESS The clock is supplied successfully. > > + > > +**/ > > +EFI_STATUS > > +SdMmcHcV4Init64BitSupport ( > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > + IN UINT8 Slot, > > + IN SD_MMC_HC_SLOT_CAP Capability > > + ) > > +{ > > + EFI_STATUS Status; > > + UINT16 ControllerVer; > > + UINT16 HostCtrl2; > > + > > + // > > + // Check if V4 64bit support is available // Status =3D > > + SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer); if > > + (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + if (ControllerVer >=3D SD_MMC_HC_CTRL_VER_400) { > > + HostCtrl2 =3D SD_MMC_HC_V4_EN; > > + // > > + // Check if V4 64bit support is available > > + // > > + if (Capability.SysBus64V4 =3D=3D TRUE) { >=20 > Apart from the additional check needed comments above, could you help to > refine the above check to: >=20 > if (Capability.SysBus64V4 !=3D 0) { >=20 > > + HostCtrl2 |=3D SD_MMC_HC_64_ADDR_EN; > > + DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n")); > > + } > > + Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, > sizeof > > (HostCtrl2), &HostCtrl2); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n")); > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > +/** > > Supply SD/MMC card with lowest clock frequency at initialization. > > > > @param[in] PciIo The PCI IO protocol instance. > > @@ -1101,6 +1180,11 @@ SdMmcHcInitHost ( > > PciIo =3D Private->PciIo; > > Capability =3D Private->Capability[Slot]; > > > > + Status =3D SdMmcHcV4Init64BitSupport (PciIo, Slot, Capability); if > > + (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > Status =3D SdMmcHcInitClockFreq (PciIo, Slot, Capability); > > if (EFI_ERROR (Status)) { > > return Status; > > @@ -1169,7 +1253,7 @@ SdMmcHcLedOnOff ( > > /** > > Build ADMA descriptor table for transfer. > > > > - Refer to SD Host Controller Simplified spec 3.0 Section 1.13 for det= ails. > > + Refer to SD Host Controller Simplified spec 4.2 Section 1.13 for det= ails. > > > > @param[in] Trb The pointer to the SD_MMC_HC_TRB instance. > > > > @@ -1187,49 +1271,69 @@ BuildAdmaDescTable ( > > UINT64 Entries; > > UINT32 Index; > > UINT64 Remaining; > > - UINT32 Address; > > + UINT64 Address; > > UINTN TableSize; > > EFI_PCI_IO_PROTOCOL *PciIo; > > EFI_STATUS Status; > > UINTN Bytes; > > + UINT16 ControllerVer; > > + BOOLEAN AddressingMode64 =3D FALSE; > > + UINTN DescSize =3D sizeof > (SD_MMC_HC_ADMA_32_DESC_LINE); > > + VOID *AdmaDesc =3D NULL; >=20 > Please help to separate the variable declaration and initial value assign= ment. >=20 > Also, please update the type of variable 'DescSize' from UINTN to UINT32. > The Visual Studio 2015 complier complains for: >=20 > " TableSize =3D (UINTN)MultU64x32 (Entries, DescSize); ": >=20 > conversion from 'UINTN' to 'UINT32', possible loss of data. >=20 > > > > Data =3D Trb->DataPhy; > > DataLen =3D Trb->DataLen; > > PciIo =3D Trb->Private->PciIo; > > + > > + // > > + // Detect whether 64bit addressing is supported. > > // > > - // Only support 32bit ADMA Descriptor Table > > + Status =3D SdMmcHcGetControllerVersion (PciIo, Trb->Slot, > > + &ControllerVer); if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + if (ControllerVer >=3D SD_MMC_HC_CTRL_VER_400) { > > + Status =3D SdMmcHcCheckMmioSet(PciIo, Trb->Slot, > > SD_MMC_HC_HOST_CTRL2, 0x2, > > + > > + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN, > > SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN); > > + if (!EFI_ERROR (Status)) { > > + AddressingMode64 =3D TRUE; > > + DescSize =3D sizeof (SD_MMC_HC_ADMA_64_DESC_LINE); > > + } > > + } > > // > > - if ((Data >=3D 0x100000000ul) || ((Data + DataLen) > 0x100000000ul)) > > { > > + // Check for valid ranges in 32bit ADMA Descriptor Table // if > > + (AddressingMode64 =3D=3D FALSE && > > + ((Data >=3D 0x100000000ul) || ((Data + DataLen) > > > + 0x100000000ul))) { >=20 > Please help to update the above check to: >=20 > if ((!AddressingMode64) && > ((Data >=3D 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))) = { >=20 > > return EFI_INVALID_PARAMETER; > > } > > // > > // Address field shall be set on 32-bit boundary (Lower 2-bit is > > always set to > > 0) > > - // for 32-bit address descriptor table. > > // > > if ((Data & (BIT0 | BIT1)) !=3D 0) { > > DEBUG ((DEBUG_INFO, "The buffer [0x%x] to construct ADMA desc is > > not aligned to 4 bytes boundary!\n", Data)); > > } >=20 > I found that the above check (also the comments) should be updated. For > 32-bit addressing the address of ADMA desc is 4-bytes aligned; for 64-bit= case, > the requirement is 8-byte. >=20 > Seems the align check for 64-bit addressing is missing here. >=20 > > > > Entries =3D DivU64x32 ((DataLen + ADMA_MAX_DATA_PER_LINE - 1), > > ADMA_MAX_DATA_PER_LINE); > > - TableSize =3D (UINTN)MultU64x32 (Entries, sizeof > > (SD_MMC_HC_ADMA_DESC_LINE)); > > + TableSize =3D (UINTN)MultU64x32 (Entries, DescSize); > > Trb->AdmaPages =3D (UINT32)EFI_SIZE_TO_PAGES (TableSize); > > Status =3D PciIo->AllocateBuffer ( > > PciIo, > > AllocateAnyPages, > > EfiBootServicesData, > > EFI_SIZE_TO_PAGES (TableSize), > > - (VOID **)&Trb->AdmaDesc, > > + (VOID **)&AdmaDesc, > > 0 > > ); > > if (EFI_ERROR (Status)) { > > return EFI_OUT_OF_RESOURCES; > > } > > - ZeroMem (Trb->AdmaDesc, TableSize); > > + ZeroMem (AdmaDesc, TableSize); > > Bytes =3D TableSize; > > Status =3D PciIo->Map ( > > PciIo, > > EfiPciIoOperationBusMasterCommonBuffer, > > - Trb->AdmaDesc, > > + AdmaDesc, > > &Bytes, > > &Trb->AdmaDescPhy, > > &Trb->AdmaMap > > @@ -1242,12 +1346,13 @@ BuildAdmaDescTable ( > > PciIo->FreeBuffer ( > > PciIo, > > EFI_SIZE_TO_PAGES (TableSize), > > - Trb->AdmaDesc > > + AdmaDesc > > ); > > return EFI_OUT_OF_RESOURCES; > > } > > > > - if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) { > > + if ((AddressingMode64 =3D=3D FALSE) && > > + (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) { >=20 > Please update the above check to: >=20 > if ((!AddressingMode64) && > (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) { >=20 > > // > > // The ADMA doesn't support 64bit addressing. > > // > > @@ -1258,25 +1363,49 @@ BuildAdmaDescTable ( > > PciIo->FreeBuffer ( > > PciIo, > > EFI_SIZE_TO_PAGES (TableSize), > > - Trb->AdmaDesc > > + AdmaDesc > > ); > > return EFI_DEVICE_ERROR; > > } > > > > Remaining =3D DataLen; > > - Address =3D (UINT32)Data; > > + Address =3D Data; > > + if (AddressingMode64 =3D=3D FALSE) { >=20 > Please help to update the above check to: >=20 > if (!AddressingMode64) { >=20 > > + Trb->Adma32Desc =3D AdmaDesc; > > + Trb->Adma64Desc =3D NULL; > > + } else { > > + Trb->Adma64Desc =3D AdmaDesc; > > + Trb->Adma32Desc =3D NULL; > > + } > > for (Index =3D 0; Index < Entries; Index++) { > > - if (Remaining <=3D ADMA_MAX_DATA_PER_LINE) { > > - Trb->AdmaDesc[Index].Valid =3D 1; > > - Trb->AdmaDesc[Index].Act =3D 2; > > - Trb->AdmaDesc[Index].Length =3D (UINT16)Remaining; > > - Trb->AdmaDesc[Index].Address =3D Address; > > - break; > > + if (AddressingMode64 =3D=3D FALSE) { >=20 > Please help to update the above check to: >=20 > if (!AddressingMode64) { >=20 > > + if (Remaining < ADMA_MAX_DATA_PER_LINE) { > > + Trb->Adma32Desc[Index].Valid =3D 1; > > + Trb->Adma32Desc[Index].Act =3D 2; > > + Trb->Adma32Desc[Index].Length =3D (UINT16)Remaining; > > + Trb->Adma32Desc[Index].Address =3D (UINT32)Address; > > + break; > > + } else { > > + Trb->Adma32Desc[Index].Valid =3D 1; > > + Trb->Adma32Desc[Index].Act =3D 2; > > + Trb->Adma32Desc[Index].Length =3D 0; > > + Trb->Adma32Desc[Index].Address =3D (UINT32)Address; > > + } > > } else { > > - Trb->AdmaDesc[Index].Valid =3D 1; > > - Trb->AdmaDesc[Index].Act =3D 2; > > - Trb->AdmaDesc[Index].Length =3D 0; > > - Trb->AdmaDesc[Index].Address =3D Address; > > + if (Remaining < ADMA_MAX_DATA_PER_LINE) { > > + Trb->Adma64Desc[Index].Valid =3D 1; > > + Trb->Adma64Desc[Index].Act =3D 2; > > + Trb->Adma64Desc[Index].Length =3D (UINT16)Remaining; > > + Trb->Adma64Desc[Index].LowerAddress =3D (UINT32)(Address & > > MAX_UINT32); > > + Trb->Adma64Desc[Index].UpperAddress =3D (UINT32)(Address>>32); > > + break; > > + } else { > > + Trb->Adma64Desc[Index].Valid =3D 1; > > + Trb->Adma64Desc[Index].Act =3D 2; > > + Trb->Adma64Desc[Index].Length =3D 0; > > + Trb->Adma64Desc[Index].LowerAddress =3D (UINT32)(Address & > > MAX_UINT32); >=20 > I think the above 2 " & MAX_UINT32" can be dropped. >=20 >=20 > > + Trb->Adma64Desc[Index].UpperAddress =3D (UINT32)(Address>>32); > > + } > > } > > > > Remaining -=3D ADMA_MAX_DATA_PER_LINE; @@ -1286,7 +1415,7 @@ > > BuildAdmaDescTable ( > > // > > // Set the last descriptor line as end of descriptor table > > // > > - Trb->AdmaDesc[Index].End =3D 1; > > + AddressingMode64 ? (Trb->Adma64Desc[Index].End =3D 1) : (Trb- > > >Adma32Desc[Index].End =3D 1); > > return EFI_SUCCESS; > > } > > > > @@ -1430,11 +1559,18 @@ SdMmcFreeTrb ( > > Trb->AdmaMap > > ); > > } > > - if (Trb->AdmaDesc !=3D NULL) { > > + if (Trb->Adma32Desc !=3D NULL) { > > PciIo->FreeBuffer ( > > PciIo, > > Trb->AdmaPages, > > - Trb->AdmaDesc > > + Trb->Adma32Desc > > + ); > > + } > > + if (Trb->Adma64Desc !=3D NULL) { > > + PciIo->FreeBuffer ( > > + PciIo, > > + Trb->AdmaPages, > > + Trb->Adma64Desc > > ); > > } > > if (Trb->DataMap !=3D NULL) { > > @@ -1574,12 +1710,14 @@ SdMmcExecTrb ( > > UINT16 Cmd; > > UINT16 IntStatus; > > UINT32 Argument; > > - UINT16 BlkCount; > > + UINT32 BlkCount; > > UINT16 BlkSize; > > UINT16 TransMode; > > UINT8 HostCtrl1; > > - UINT32 SdmaAddr; > > + UINT64 SdmaAddr; > > UINT64 AdmaAddr; > > + UINT16 ControllerVer; > > + BOOLEAN AddressingMode64 =3D FALSE; >=20 > Please help to separate the variable declaration and initial value assign= ment. >=20 > > > > Packet =3D Trb->Packet; > > PciIo =3D Trb->Private->PciIo; > > @@ -1612,13 +1750,33 @@ SdMmcExecTrb ( > > > > SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE); > > > > + Status =3D SdMmcHcGetControllerVersion (PciIo, Trb->Slot, > > + &ControllerVer); if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + if (ControllerVer >=3D SD_MMC_HC_CTRL_VER_400) { > > + Status =3D SdMmcHcCheckMmioSet(PciIo, Trb->Slot, > > SD_MMC_HC_HOST_CTRL2, 0x2, > > + > > + SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN, > > SD_MMC_HC_V4_EN|SD_MMC_HC_64_ADDR_EN); > > + if (!EFI_ERROR (Status)) { > > + AddressingMode64 =3D TRUE; > > + } > > + } > > + > > if (Trb->Mode =3D=3D SdMmcSdmaMode) { > > - if ((UINT64)(UINTN)Trb->DataPhy >=3D 0x100000000ul) { > > + if ((AddressingMode64 =3D=3D FALSE) && > > + ((UINT64)(UINTN)Trb->DataPhy >=3D 0x100000000ul)) { >=20 > Please help to update the above check to: >=20 > if ((!AddressingMode64) && > ((UINT64)(UINTN)Trb->DataPhy >=3D 0x100000000ul)) { >=20 > > return EFI_INVALID_PARAMETER; > > } > > > > - SdmaAddr =3D (UINT32)(UINTN)Trb->DataPhy; > > - Status =3D SdMmcHcRwMmio (PciIo, Trb->Slot, > SD_MMC_HC_SDMA_ADDR, > > FALSE, sizeof (SdmaAddr), &SdmaAddr); > > + SdmaAddr =3D (UINT64)(UINTN)Trb->DataPhy; > > + > > + if (ControllerVer >=3D SD_MMC_HC_CTRL_VER_400) { > > + Status =3D SdMmcHcRwMmio (PciIo, Trb->Slot, > > SD_MMC_HC_ADMA_SYS_ADDR, FALSE, sizeof (UINT64), &SdmaAddr); > > + } > > + else { >=20 > Minor coding style comment for the above line: >=20 > } else { >=20 > > + Status =3D SdMmcHcRwMmio (PciIo, Trb->Slot, > > SD_MMC_HC_SDMA_ADDR, FALSE, sizeof (UINT32), &SdmaAddr); > > + } > > + > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -1648,9 +1806,14 @@ SdMmcExecTrb ( > > // > > // Calcuate Block Count. > > // > > - BlkCount =3D (UINT16)(Trb->DataLen / Trb->BlockSize); > > + BlkCount =3D (Trb->DataLen / Trb->BlockSize); } >=20 > For the below 'Block Count' settings, I think it would be better to add > comments that quote the SD Host Controller Spec to mention that the 32-bi= t > block count support for all operations in SD mode was introduced at Versi= on > 4.10. >=20 > > + if (ControllerVer >=3D SD_MMC_HC_CTRL_VER_410) { > > + Status =3D SdMmcHcRwMmio (PciIo, Trb->Slot, > SD_MMC_HC_SDMA_ADDR, > > FALSE, sizeof (UINT32), &BlkCount); > > + } > > + else { >=20 > Minor coding style comment for the above line: >=20 > } else { >=20 > > + Status =3D SdMmcHcRwMmio (PciIo, Trb->Slot, > SD_MMC_HC_BLK_COUNT, > > FALSE, sizeof (UINT16), &BlkCount); > > } > > - Status =3D SdMmcHcRwMmio (PciIo, Trb->Slot, SD_MMC_HC_BLK_COUNT, > > FALSE, sizeof (BlkCount), &BlkCount); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -1746,10 +1909,11 @@ SdMmcCheckTrbResult ( > > EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet; > > UINT16 IntStatus; > > UINT32 Response[4]; > > - UINT32 SdmaAddr; > > + UINT64 SdmaAddr; > > UINT8 Index; > > UINT8 SwReset; > > UINT32 PioLength; > > + UINT16 ControllerVer; > > > > SwReset =3D 0; > > Packet =3D Trb->Packet; > > @@ -1870,19 +2034,42 @@ SdMmcCheckTrbResult ( > > // > > // Update SDMA Address register. > > // > > - SdmaAddr =3D SD_MMC_SDMA_ROUND_UP ((UINT32)(UINTN)Trb- > > >DataPhy, SD_MMC_SDMA_BOUNDARY); > > - Status =3D SdMmcHcRwMmio ( > > + SdmaAddr =3D SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy, > > SD_MMC_SDMA_BOUNDARY); > > + > > + Status =3D SdMmcHcGetControllerVersion ( > > + Private->PciIo, > > + Trb->Slot, > > + &ControllerVer > > + ); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + if (ControllerVer >=3D SD_MMC_HC_CTRL_VER_400) { > > + Status =3D SdMmcHcRwMmio ( > > Private->PciIo, > > Trb->Slot, > > - SD_MMC_HC_SDMA_ADDR, > > + SD_MMC_HC_ADMA_SYS_ADDR, > > FALSE, > > - sizeof (UINT32), > > + sizeof (UINT64), > > &SdmaAddr > > ); > > + } > > + else { > > + Status =3D SdMmcHcRwMmio ( > > + Private->PciIo, > > + Trb->Slot, > > + SD_MMC_HC_SDMA_ADDR, > > + FALSE, > > + sizeof (UINT32), > > + &SdmaAddr > > + ); > > + } >=20 > Minor coding style comments for the above line: >=20 > 1. } else { > 2. Please help to refine the space indent of the above SdMmcHcRwMmio() > call >=20 >=20 > Best Regards, > Hao Wu >=20 > > + > > if (EFI_ERROR (Status)) { > > goto Done; > > } > > - Trb->DataPhy =3D (UINT32)(UINTN)SdmaAddr; > > + Trb->DataPhy =3D (UINT64)(UINTN)SdmaAddr; > > } > > > > if ((Packet->SdMmcCmdBlk->CommandType !=3D > SdMmcCommandTypeAdtc) && > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > index cc138fc..a6234f1 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > @@ -2,6 +2,7 @@ > > > > Provides some data structure definitions used by the SD/MMC host > > controller driver. > > > > +Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved. > > Copyright (c) 2015, Intel Corporation. All rights reserved.
This > > program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License @@ -78,6 > > +79,9 @@ typedef enum { // > > #define ADMA_MAX_DATA_PER_LINE 0x10000 > > > > +// > > +// ADMA descriptor for 32b addressing. > > +// > > typedef struct { > > UINT32 Valid:1; > > UINT32 End:1; > > @@ -87,7 +91,23 @@ typedef struct { > > UINT32 Reserved1:10; > > UINT32 Length:16; > > UINT32 Address; > > -} SD_MMC_HC_ADMA_DESC_LINE; > > +} SD_MMC_HC_ADMA_32_DESC_LINE; > > + > > +// > > +// ADMA descriptor for 64b addressing. > > +// > > +typedef struct { > > + UINT32 Valid:1; > > + UINT32 End:1; > > + UINT32 Int:1; > > + UINT32 Reserved:1; > > + UINT32 Act:2; > > + UINT32 Reserved1:10; > > + UINT32 Length:16; > > + UINT32 LowerAddress; > > + UINT32 UpperAddress; > > + UINT32 Reserved2; > > +} SD_MMC_HC_ADMA_64_DESC_LINE; > > > > #define SD_MMC_SDMA_BOUNDARY 512 * 1024 > > #define SD_MMC_SDMA_ROUND_UP(x, n) (((x) + n) & ~(n - 1)) > > @@ -145,6 +165,12 @@ typedef struct { > > #define SD_MMC_HC_CTRL_VER_410 0x04 > > #define SD_MMC_HC_CTRL_VER_420 0x05 > > > > +// > > +// SD Host controller V4 Support > > +// > > +#define SD_MMC_HC_V4_EN BIT12 > > +#define SD_MMC_HC_64_ADDR_EN BIT13 > > + > > /** > > Dump the content of SD/MMC host controller's Capability Register. > > > > -- > > 2.7.4 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > -------------------------------------------------------------------------= ---------- > This email message is for the sole use of the intended recipient(s) and m= ay > contain > confidential information. Any unauthorized review, use, disclosure or > distribution > is prohibited. If you are not the intended recipient, please contact the > sender by > reply email and destroy all copies of the original message. > -------------------------------------------------------------------------= ---------- > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel