From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 BBCFF202E5309 for ; Sun, 17 Feb 2019 18:49:49 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Feb 2019 18:49:49 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,382,1544515200"; d="scan'208,217";a="123215862" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga007.fm.intel.com with ESMTP; 17 Feb 2019 18:49:49 -0800 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 17 Feb 2019 18:49:49 -0800 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 17 Feb 2019 18:49:48 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.102]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.162]) with mapi id 14.03.0415.000; Mon, 18 Feb 2019 10:49:46 +0800 From: "Wu, Hao A" To: Jeff Brasen , "edk2-devel@lists.01.org" CC: Edgar Handal Thread-Topic: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths Thread-Index: AQHUuPfkGoiAfNeKak+Wm9jQVLNaX6XKci6w//+QtYCAAIq70IAAKD6AgBo9XUA= Date: Mon, 18 Feb 2019 02:49:45 +0000 Message-ID: References: <7a0d5c95fdeee0e68f54c8a6a0fbe37c85e76774.1548892644.git.jbrasen@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 X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO widths 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: Mon, 18 Feb 2019 02:49:50 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hello Jeff, Sorry for the delayed response. I have verified your patch with the board on my side and it is working fine. And I saw around 3~4 milliseconds performance boost. So I think the patch will bring performance benefit to the driver. Some minor inline comments below: From: Jeff Brasen [mailto:jbrasen@nvidia.com] Sent: Saturday, February 02, 2019 1:53 AM To: Wu, Hao A; edk2-devel@lists.01.org Cc: Edgar Handal Subject: Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO wi= dths ________________________________ From: Wu, Hao A Sent: Friday, February 1, 2019 12:54 AM To: Jeff Brasen; edk2-devel@lists.01.org Cc: Edgar Handal Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO wi= dths > -----Original Message----- > From: Jeff Brasen [mailto:jbrasen@nvidia.com] > Sent: Friday, February 01, 2019 3:12 PM > To: Wu, Hao A; edk2-devel@lists.01.org > Cc: Edgar Handal > Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO > widths > > > > -----Original Message----- > From: Wu, Hao A > Sent: Thursday, January 31, 2019 10:56 PM > To: Jeff Brasen ; edk2-devel@lists.01.org > Cc: Edgar Handal > Subject: RE: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO > widths > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Jeff Brasen > > Sent: Thursday, January 31, 2019 7:59 AM > > To: edk2-devel@lists.01.org > > Cc: Edgar Handal; Jeff Brasen > > Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Use 16/32-bit IO > > widths > > Could you help to file a Bugzilla tracker to state the purpose of this patch and make a reference in the commit log message here? Thanks. > > From: Edgar Handal > > > > Use 16-bit and 32-bit IO widths for SDMMC MMIO to prevent all register > > accesses from being split up into 8-bit accesses. > > > > The SDHCI specification states that the registers shall be accessable > > in byte, word, and double word accesses. 'accessable' -> 'accessible'. Also, could you help to quote the detailed context from the SD HCI spec for the above statement? Lastly, could you help to add your performance evaluation data in the commit log message as well? With the above 3 minor comments handled: Reviewed-by: Hao Wu Best Regards, Hao Wu > > Hi, > > Thanks for the contribution. The change seems good to me. > > Just curious, if the accesses are always slit into byte(8-bit), is there = any issue or > performance impact is encountered during your usage? > > It will be helpful to get more information on the purpose of the patch. > Thanks. > > Best Regards, > Hao Wu > > [JMB] We were working with a simulation module that has some issues when > accessing 16 or 32 bit registers by byte (This should be supported per th= e SDHCI > specification.), and this patch resolves this while we work on getting th= e model > fixed. This should also optimize performance as there will less read/writ= e > instructions (My guess is this is a marginal improvement as most of the S= D > access time would be DMA operations.) Thanks Jeff, Got it. May I know is it possible for you to collect some performance data for the change? [JMB] Sure, did a several boots on silicon platform and see an average boot= time improvement of 15ms. With a range of 10-20ms and never saw the perfor= mance get worse. Meanwhile, I will test this patch with the boards I own and try to collect some data. Please grant me some time for this. Since the current implementation does not violate the spec, let us evaluate whether better performance will be brought. Best Regards, Hao Wu > > Thanks, > Jeff > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jeff Brasen > > --- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 25 > > ++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > index 5aec8c6..82f4493 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > @@ -152,19 +152,36 @@ SdMmcHcRwMmio ( > > ) > > { > > EFI_STATUS Status; > > + EFI_PCI_IO_PROTOCOL_WIDTH Width; > > > > if ((PciIo =3D=3D NULL) || (Data =3D=3D NULL)) { > > return EFI_INVALID_PARAMETER; > > } > > > > - if ((Count !=3D 1) && (Count !=3D 2) && (Count !=3D 4) && (Count != =3D 8)) { > > - return EFI_INVALID_PARAMETER; > > + switch (Count) { > > + case 1: > > + Width =3D EfiPciIoWidthUint8; > > + break; > > + case 2: > > + Width =3D EfiPciIoWidthUint16; > > + Count =3D 1; > > + break; > > + case 4: > > + Width =3D EfiPciIoWidthUint32; > > + Count =3D 1; > > + break; > > + case 8: > > + Width =3D EfiPciIoWidthUint32; > > + Count =3D 2; > > + break; > > + default: > > + return EFI_INVALID_PARAMETER; > > } > > > > if (Read) { > > Status =3D PciIo->Mem.Read ( > > PciIo, > > - EfiPciIoWidthUint8, > > + Width, > > BarIndex, > > (UINT64) Offset, > > Count, > > @@ -173,7 +190,7 @@ SdMmcHcRwMmio ( > > } else { > > Status =3D PciIo->Mem.Write ( > > PciIo, > > - EfiPciIoWidthUint8, > > + Width, > > BarIndex, > > (UINT64) Offset, > > Count, > > -- > > 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. > -------------------------------------------------------------------------= ----------