From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=216.228.121.65; helo=hqemgate16.nvidia.com; envelope-from=ashishsingha@nvidia.com; receiver=edk2-devel@lists.01.org Received: from hqemgate16.nvidia.com (hqemgate16.nvidia.com [216.228.121.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 079F621196836 for ; Thu, 29 Nov 2018 11:16:34 -0800 (PST) Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqemgate16.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Thu, 29 Nov 2018 11:16:39 -0800 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate101.nvidia.com (PGP Universal service); Thu, 29 Nov 2018 11:16:34 -0800 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Thu, 29 Nov 2018 11:16:34 -0800 Received: from HQMAIL107.nvidia.com (172.20.187.13) by HQMAIL108.nvidia.com (172.18.146.13) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Thu, 29 Nov 2018 19:16:33 +0000 Received: from NAM05-CO1-obe.outbound.protection.outlook.com (104.47.48.53) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1395.4 via Frontend Transport; Thu, 29 Nov 2018 19:16:33 +0000 Received: from BYAPR12MB2743.namprd12.prod.outlook.com (20.177.125.220) by BYAPR12MB2790.namprd12.prod.outlook.com (20.177.126.79) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1294.30; Thu, 29 Nov 2018 19:16:12 +0000 Received: from BYAPR12MB2743.namprd12.prod.outlook.com ([fe80::a4b5:67ae:7698:2f2e]) by BYAPR12MB2743.namprd12.prod.outlook.com ([fe80::a4b5:67ae:7698:2f2e%3]) with mapi id 15.20.1339.027; Thu, 29 Nov 2018 19:16:12 +0000 From: Ashish Singhal To: "Wu, Hao A" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64bit SDMA and ADMA2 support. Thread-Index: AQHUhut4WVEK4XV6bUKEVKsx0Ybm6qVnFDcQgAANh1A= Date: Thu, 29 Nov 2018 19:16:12 +0000 Message-ID: References: <444ed350c110664e0547692c294473503370d3e4.1542659223.git.ashishsingha@nvidia.com> <81ad37c5653bf3a092789118c3fb8078e0f68644.1542659223.git.ashishsingha@nvidia.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_6b558183-044c-4105-8d9c-cea02a2a3d86_Enabled=True; MSIP_Label_6b558183-044c-4105-8d9c-cea02a2a3d86_SiteId=43083d15-7273-40c1-b7db-39efd9ccc17a; MSIP_Label_6b558183-044c-4105-8d9c-cea02a2a3d86_Owner=ashishsingha@nvidia.com; MSIP_Label_6b558183-044c-4105-8d9c-cea02a2a3d86_SetDate=2018-11-29T18:48:14.6034657Z; MSIP_Label_6b558183-044c-4105-8d9c-cea02a2a3d86_Name=Unrestricted; MSIP_Label_6b558183-044c-4105-8d9c-cea02a2a3d86_Application=Microsoft Azure Information Protection; MSIP_Label_6b558183-044c-4105-8d9c-cea02a2a3d86_Extended_MSFT_Method=Automatic; Sensitivity=Unrestricted x-originating-ip: [216.228.112.22] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; BYAPR12MB2790; 6:mBYvcGpxuihRQLCAMEfgPhZF0Adyz3XYusrgDVpMpPitdB0cYKcJoLnXAfQcSkxLKcmzKnHQnsWkMlezbjnEDZfT13BP/ajRCqUtE2ZLMkkta/Nkkctk9pA9Ar0W/kpGtCT0UZO6YUttR+ugqbSudwUmpFyhxiK0MpbvlyzZL9sVshiMtMF6/Mx5stOBRlvx2ZC80MRjj7adeyn9JYKljWwzV1rCJ99ciQ80txeF/BJvy7qoLXQbeAnuRzHSbQVZXMOOyeXQDX8KLiTQm0Il1UvO22q0eTH/o9//wbVPhj6LMl/14zgKxRaGVjKayC+HKk/W7J7v5waWNdQxzDGNjHStHIQBeMyRr7w1gzRXWttX3rJN7F35T2PCF7v4GW6vt03hKVIyuUZtrFxsJUEUucmVt9Qmhprzd3UJ2XkWWedR944Y/KOQsdzWUxgepefbPwr8F4NT47TEv+dcC9pqxQ==; 5:3D3VI+ssYDbYoBowNLcG0KksYI/EwHJ2BjIeoMjaypB/1Y6Do5/ixWvlQZhfXwnklSTszYdqDohbLOH1Nvn4EoMrajpLQlyHnLPS3H5KXBWdrWqcHkbqPnFG7RwzWTZ5zYr1Bf14kucQSL5bXS5AjrOQiILzvnz883RiC1fh+qg=; 7:Yw5arrZ8XCONHTvMaIZ3al0B8MgiSFHr/h/JuZ/xe9ZqgQa19LMVLsItBPof0b4aC5YATqBlg+Lt2+1h1sVz8wuflCoqFIGEAQ4+OKbAUM1oXvau3YBOxDsnqLbD3PauXbSyR0c64DnJpA0PWfDHgA== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: c208fd91-625e-4cd1-1bdc-08d6562f2032 x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390098)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(2017052603328)(7153060)(7193020); SRVR:BYAPR12MB2790; x-ms-traffictypediagnostic: BYAPR12MB2790: authentication-results: spf=none (sender IP is ) smtp.mailfrom=ashishsingha@nvidia.com; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(162533806227266)(228905959029699)(18589796830644); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040522)(2401047)(5005006)(8121501046)(3002001)(3231453)(999002)(944501449)(52105112)(93006095)(93001095)(10201501046)(148016)(149066)(150057)(6041310)(20161123562045)(20161123564045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123558120)(20161123560045)(201708071742011)(7699051)(76991095); SRVR:BYAPR12MB2790; BCL:0; PCL:0; RULEID:; SRVR:BYAPR12MB2790; x-forefront-prvs: 0871917CDA x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6029001)(346002)(376002)(39850400004)(136003)(396003)(366004)(13464003)(51874003)(199004)(189003)(14444005)(19627235002)(256004)(14454004)(105586002)(316002)(2906002)(186003)(102836004)(93886005)(476003)(305945005)(7736002)(478600001)(74316002)(25786009)(486006)(99286004)(229853002)(81156014)(97736004)(8936002)(86362001)(446003)(81166006)(93156006)(8676002)(68736007)(3846002)(2940100002)(7696005)(71190400001)(71200400001)(4744004)(966005)(11346002)(106356001)(5660300001)(6116002)(6506007)(6436002)(66066001)(53936002)(9686003)(6306002)(53546011)(110136005)(76176011)(33656002)(6246003)(26005)(2501003)(55016002)(53946003)(579004)(559001); DIR:OUT; SFP:1101; SCL:1; SRVR:BYAPR12MB2790; H:BYAPR12MB2743.namprd12.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: nvidia.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: cX0tx0jqLRAk99cd4KFKdxRKrfDVqPqVD3qeEc1QcvumFX/gGqxRXb833gsidgvRBlz2QoHCnsJi0OvQRBTYCxjMobUXoOK0zG+bY4m9+2gAld0jFOpR3PtvnX1m5Ry+axExjy+d4Mni76yg0oViBEeYfGUAr3MHt+QSY2DaTyKHW5zIpW9+027HbbMGLx594qyz3OUFrgg1+uBuhJZhWOZBdyDign+36sPgR4U3TFDbAaEI7E7znJB1PRf6tPdBCISIvj22xHUBC/3lM6wszIOakB+cqINO9likWWAsnzBNeaEjQXWpZg0niXI5gsllDi7+s00JiXMiWA6EfXIcbzwMQ2rLH9Rnb0wRLavT0nQ= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: c208fd91-625e-4cd1-1bdc-08d6562f2032 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Nov 2018 19:16:12.1704 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB2790 X-OriginatorOrg: Nvidia.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1543519000; bh=8XosJuPxckmW1EEEbIchqmT5sRqFVYzVvR9tAVOhjco=; h=X-PGP-Universal:From:To:Subject:Thread-Topic:Thread-Index:Date: Message-ID:References:In-Reply-To:Accept-Language:X-MS-Has-Attach: X-MS-TNEF-Correlator:msip_labels:x-originating-ip: x-ms-publictraffictype:x-microsoft-exchange-diagnostics: x-ms-exchange-antispam-srfa-diagnostics: x-ms-office365-filtering-correlation-id:x-microsoft-antispam: x-ms-traffictypediagnostic:authentication-results: x-microsoft-antispam-prvs:x-exchange-antispam-report-test: x-ms-exchange-senderadcheck:x-exchange-antispam-report-cfa-test: x-forefront-prvs:x-forefront-antispam-report:received-spf: x-microsoft-antispam-message-info:spamdiagnosticoutput: spamdiagnosticmetadata:MIME-Version: X-MS-Exchange-CrossTenant-Network-Message-Id: X-MS-Exchange-CrossTenant-originalarrivaltime: X-MS-Exchange-CrossTenant-fromentityheader: X-MS-Exchange-CrossTenant-id: X-MS-Exchange-Transport-CrossTenantHeadersStamped:X-OriginatorOrg: Content-Language:Content-Type:Content-Transfer-Encoding; b=JWxQNxWQJEDyhcptIECfLNJIt/pP5vUF4HLLmuG81e8a7uxE7z+/0MdzQIfgf0G7U aw2JGKReVmgEst6+bsCgmhNk7bcV3DA/8I/xy2czZ2sVWVbfzrpW8r0U8g9nqm5/V8 8XN7K7I2YCY5chCJ1GhGnUN5CmRXs3KtY9WsVVPVafvG9Js4UmFetgN80YrfPEPLyH m/K7bCd7XyVstsaYCcXWBuuZs98v8qJ2uN/X1esl1KCnNco28qRdmZiiTlPXTnZAAS LI8JpyOip/2acCIvOzFmUKhy/w0dnuFO1nJ1K64yMrtjptsOd0LvFXkOqRhoc/7MyV y4VIw/2KPfX4Q== 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: Thu, 29 Nov 2018 19:16:35 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Missed one of your suggested compiler related change in Patch 5. Have pos= ted patch 6. Thanks Ashish Singhal -----Original Message----- From: Ashish Singhal=20 Sent: Thursday, November 29, 2018 11:48 AM To: 'Wu, Hao A' ; edk2-devel@lists.01.org Subject: RE: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: Add V4 64b= it SDMA and ADMA2 support. Hello Hao, Answers to your comments: 1. Patch 5 fixes the issue with accessing SD as well as eMMC files. 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 i= s not possible in some functions for example where we set clock dividers.= =20We either need to use what I have or pass Private or controller versio= n as an argument to functions using it. 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 in PCI layer. So what you are suggesting may not be that helpful.= =20Also, 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 sup= porting similar address width? Also, shouldn't we be enabling 64b DMA in = PCI layer if any of the controller supports 64b addressing? Wouldn't 64b = DMA enabled mean 32b is enabled by default? 4. Patch 5 has been rebased on tip of edk2. Thanks Ashish -----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 64b= it SDMA and ADMA2 support. Hello, Sorry for the delayed response. Apart from inserting the Bugzilla tracker information, several more gener= al level comments: 1. Cannot access the files (content) on SD & eMMC devices 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= =20accessed. There are 1 SD card and 1 embedded eMMC device within the system. Under S= hell, I can see 2 file systems on SD & eMMC devices being detected, but w= hen I try to access the files on those file systems by using the 'ls' com= mand, it fails, saying "ls: File Not Found - 'FS0:\'". I can confirm that= =20files can be listed successfully without applying this patch. 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. = But for eMMC, the command fails with message "dblk: Read file error - 'Bl= ockIo'". 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. 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 f= or Version 4.0 or greater SD controllers. Do you have any hardware with SD controller of version 3.0? Is it working= =20on your side? Please let me know if additional information is needed. 2. On SdMmcHcGetControllerVersion() I found that there is a 'ControllerVersion' version in the data structure= =20SD_MMC_HC_PRIVATE_DATA. But currently, this field seems never used. 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 als= o can avoid calling it multiple times. 3. Setting the SD_MMC_HC_64_ADDR_EN bit in SdMmcHcV4Init64BitSupport() 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. In function SdMmcPciHcDriverBindingStart(): =20 // =20 // Enable 64-bit DMA support in the PCI layer if this controller =20 // supports it. =20 // =20 if (Support64BitDma) { =20 Status =3D PciIo->Attributes ( =20 PciIo, =20 EfiPciIoAttributeOperationEnable, =20 EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, =20 NULL =20 ); =20 if (EFI_ERROR (Status)) { =20 DEBUG ((DEBUG_WARN, "SdMmcPciHcDriverBindingStart: failed to enab= le 64-bit DMA (%r)\n", Status)); =20 } =20 } If the above PCI attribute setting fails, the PCI layer will not support = the 64-bit DMA. Hence, I am thinking to introduce a new BOOLEAN field in the "SD_MMC_HC_P= RIVATE_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. And the setting of the SD_MMC_HC_64_ADDR_EN bit should depend on the BOOL= EAN field as well. 4. Please help to rebase the patch upon the latest master branch. Some changes within the SdMmcPciHcDxe have been pushed recently. Could yo= u help to rebase this patch upon the latest changes. Thanks in advance. Also, some inline comments below. > -----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=20 > 64bit SDMA and ADMA2 support. >=20 > If V4 64 bit address mode is enabled in compatibility register,=20 > program controller to enable V4 host mode. > Use appropriate ADMA2 descriptors supporting 64 bit addresses. > Use appropriate registers for SDMA mode operation. >=20 > 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(-) >=20 > 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 @@ >=20 > Provides some data structure definitions used by the SD/MMC host=20 > controller driver. >=20 > +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=20 > available under the terms and conditions of the BSD License @@ -144,7 > +145,8 @@ typedef struct { > BOOLEAN Started; > UINT64 Timeout; >=20 > - 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 @@ >=20 > It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use. >=20 > + 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=20 > the BSD License @@ -418,6 +419,36 @@ SdMmcHcWaitMmioSet ( } >=20 > /** > + 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=20 > interrupts. >=20 > @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA > instance. > @@ -776,18 +807,18 @@ SdMmcHcClockSupply ( >=20 > DEBUG ((DEBUG_INFO, "BaseClkFreq %dMHz Divisor %d ClockFreq=20 > %dKhz\n", BaseClkFreq, Divisor, ClockFreq)); >=20 > - Status =3D SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_CTRL_VER, TRUE,=20 > 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=20 > 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)=20 > =3D=3D 1)) { > + } else if ((ControllerVer =3D=3D 0) || (ControllerVer =3D=3D 1)) { Could you help to update the above 'else if' statement to use the below v= ersion definitions? SD_MMC_HC_CTRL_VER_100 SD_MMC_HC_CTRL_VER_200 > // > // Only the most significant bit can be used as divisor. > // > @@ -935,6 +966,54 @@ SdMmcHcSetBusWidth ( } >=20 > /** > + 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=20 > + SdMmcHcGetControllerVersion (PciIo, Slot, &ControllerVer); if=20 > + (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) { Apart from the additional check needed comments above, could you help to = refine the above check to: if (Capability.SysBus64V4 !=3D 0) { > + 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, sizeo= f > (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. >=20 > @param[in] PciIo The PCI IO protocol instance. > @@ -1101,6 +1180,11 @@ SdMmcHcInitHost ( > PciIo =3D Private->PciIo; > Capability =3D Private->Capability[Slot]; >=20 > + 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. >=20 > - 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. >=20 > @param[in] Trb The pointer to the SD_MMC_HC_TRB instance.= >=20 > @@ -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_DES= C_LINE); > + VOID *AdmaDesc =3D NULL; Please help to separate the variable declaration and initial value assign= ment. Also, please update the type of variable 'DescSize' from UINTN to UINT32.= =20The Visual Studio 2015 complier complains for: " TableSize =3D (UINTN)MultU64x32 (Entries, DescSize); ": 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,=20 > + &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, > + =20 > + 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))= =20 > { > + // Check for valid ranges in 32bit ADMA Descriptor Table // if > + (AddressingMode64 =3D=3D FALSE && > + ((Data >=3D 0x100000000ul) || ((Data + DataLen) > > + 0x100000000ul))) { Please help to update the above check to: =20 if ((!AddressingMode64) && =20 ((Data >=3D 0x100000000ul) || ((Data + DataLen) > 0x100000000ul))= ) { > return EFI_INVALID_PARAMETER; > } > // > // Address field shall be set on 32-bit boundary (Lower 2-bit is=20 > 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=20 > not aligned to 4 bytes boundary!\n", Data)); > } I found that the above check (also the comments) should be updated. For 3= 2-bit addressing the address of ADMA desc is 4-bytes aligned; for 64-bit = case, the requirement is 8-byte. 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=20 > (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; > } >=20 > - if ((UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) { > + if ((AddressingMode64 =3D=3D FALSE) && > + (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) { Please update the above check to: =20 if ((!AddressingMode64) && =20 (UINT64)(UINTN)Trb->AdmaDescPhy > 0x100000000ul) { > // > // 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; > } >=20 > Remaining =3D DataLen; > - Address =3D (UINT32)Data; > + Address =3D Data; > + if (AddressingMode64 =3D=3D FALSE) { Please help to update the above check to: =20 if (!AddressingMode64) { > + 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) { Please help to update the above check to: =20 if (!AddressingMode64) { > + 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); I think the above 2 " & MAX_UINT32" can be dropped. > + Trb->Adma64Desc[Index].UpperAddress =3D (UINT32)(Address>>32);= > + } > } >=20 > Remaining -=3D ADMA_MAX_DATA_PER_LINE; @@ -1286,7 +1415,7 @@=20 > 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; > } >=20 > @@ -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; 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 ( >=20 > SdMmcHcLedOnOff (PciIo, Trb->Slot, TRUE); >=20 > + Status =3D SdMmcHcGetControllerVersion (PciIo, Trb->Slot,=20 > + &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, > + =20 > + 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)) { Please help to update the above check to: =20 if ((!AddressingMode64) && =20 ((UINT64)(UINTN)Trb->DataPhy >=3D 0x100000000ul)) { > return EFI_INVALID_PARAMETER; > } >=20 > - 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 { Minor coding style comment for the above line: } else { > + 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); } For the below 'Block Count' settings, I think it would be better to add c= omments that quote the SD Host Controller Spec to mention that the 32-bit= =20block count support for all operations in SD mode was introduced at Ve= rsion 4.10. > + 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 { Minor coding style comment for the above line: } else { > + 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; >=20 > 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 > + ); > + } Minor coding style comments for the above line: 1. } else { 2. Please help to refine the space indent of the above SdMmcHcRwMmio() ca= ll Best Regards, Hao Wu > + > if (EFI_ERROR (Status)) { > goto Done; > } > - Trb->DataPhy =3D (UINT32)(UINTN)SdmaAddr; > + Trb->DataPhy =3D (UINT64)(UINTN)SdmaAddr; > } >=20 > 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 @@ >=20 > Provides some data structure definitions used by the SD/MMC host=20 > controller driver. >=20 > +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=20 > available under the terms and conditions of the BSD License @@ -78,6 > +79,9 @@ typedef enum { // > #define ADMA_MAX_DATA_PER_LINE 0x10000 >=20 > +// > +// 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; >=20 > #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 >=20 > +// > +// 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. >=20 > -- > 2.7.4 >=20 > _______________________________________________ > 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 di= stribution is prohibited. If you are not the intended recipient, please contact the= =20sender by reply email and destroy all copies of the original message. -------------------------------------------------------------------------= ----------