From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 9C2A51A1E92 for ; Wed, 14 Sep 2016 11:07:33 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga105.jf.intel.com with ESMTP; 14 Sep 2016 11:07:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,335,1470726000"; d="scan'208";a="8399663" Received: from orsmsx110.amr.corp.intel.com ([10.22.240.8]) by fmsmga006.fm.intel.com with ESMTP; 14 Sep 2016 11:07:32 -0700 Received: from orsmsx160.amr.corp.intel.com (10.22.226.43) by ORSMSX110.amr.corp.intel.com (10.22.240.8) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 14 Sep 2016 11:07:31 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.173]) by ORSMSX160.amr.corp.intel.com ([10.22.226.43]) with mapi id 14.03.0248.002; Wed, 14 Sep 2016 11:07:30 -0700 From: "Kinney, Michael D" To: "afish@apple.com" , "Kinney, Michael D" CC: Leif Lindholm , "Tian, Feng" , "edk2-devel@lists.01.org" , "Zeng, Star" Thread-Topic: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset Thread-Index: AQHSDiiixR7EED5N70OIlOz44hLDOaB5aziA///LA+CAAIAMAP//knjg Date: Wed, 14 Sep 2016 18:07:26 +0000 Message-ID: References: <5a0c21d0fe889950fda9e59aaf2272b66d429b8f.1473816993.git.feng.tian@intel.com> <20160914130833.GB16080@bivouac.eciton.net> <20A29753-8302-4AE0-91FF-F0368C1D7E35@apple.com> In-Reply-To: <20A29753-8302-4AE0-91FF-F0368C1D7E35@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOGM3NDRlMWQtZGJiMC00ZjlhLTlkZDctZDNmZjMwYzdlZWUwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkMrOEc3TTcrcVNabVUwck5QOE83SFJZOWkxT0xVeExkYnN4RG9USmM3Tzg9In0= x-originating-ip: [10.22.254.138] MIME-Version: 1.0 Subject: Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Sep 2016 18:07:33 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Andrew, The 100 ns vs. 1 us differences are what I tried to highlight here and prop= ose some helpers to hide that difference. Do you have some alternatives in mind? Mike > -----Original Message----- > From: afish@apple.com [mailto:afish@apple.com] > Sent: Wednesday, September 14, 2016 10:37 AM > To: Kinney, Michael D > Cc: Leif Lindholm ; Tian, Feng ; edk2- > devel@lists.01.org; Zeng, Star > Subject: Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before acces= s MMIO reg > during reset >=20 >=20 > > On Sep 14, 2016, at 10:14 AM, Kinney, Michael D wrote: > > > > Leif, > > > > MdePkg/Include/Library/UefiLib.h does have some helper macros for setti= ng timer > events periods > > that are in 100 nS units: > > > > #define EFI_TIMER_PERIOD_MICROSECONDS(Microseconds) > MultU64x32((UINT64)(Microseconds), 10) > > #define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds) > MultU64x32((UINT64)(Milliseconds), 10000) > > #define EFI_TIMER_PERIOD_SECONDS(Seconds) MultU64x32((UINT64= )(Seconds), > 10000000) > > > > I believe the examples you show are for use with the gBS->Stall() servi= ce which is > in 1 uS units. > > > > Maybe we should consider some additional macros in UefiLib.h > > > > #define EFI_STALL_PERIOD_MICROSECONDS(Microseconds) (Microseconds) > > #define EFI_STALL_PERIOD_MILLISECONDS(Milliseconds) ((Milliseconds) * = 1000) > > #define EFI_STALL_PERIOD_SECONDS(Seconds) ((Seconds) * 10000= 00) > > > > Or maybe some macros that actually do the call to gBS->Stall() too. > > > > #define EFI_STALL_MICROSECONDS(Microseconds) gBS->Stall (Microseconds) > > #define EFI_STALL_MILLISECONDS(Milliseconds) gBS->Stall ((Milliseconds= ) * 1000) > > #define EFI_STALL_SECONDS(Seconds) gBS->Stall ((Seconds) * 1= 000000) > > > > The other method of generating timed delays for PEI/DXE/SMM modules is = using the > > Services in MdePkg/Include/Library/TimerLib.h: > > > > UINTN > > EFIAPI > > NanoSecondDelay ( > > IN UINTN NanoSeconds > > ); > > > > UINTN > > EFIAPI > > MicroSecondDelay ( > > IN UINTN MicroSeconds > > ); > > > > If we wanted macros helper to use these services to match UEFI ones, ma= ybe > > add the following to TimerLib.h: > > > > #define DELAY_NANOSECONDS(Nanoseconds) NanoSecondDelay (Nanoseconds) > > #define DELAY_MICROSECONDS(Microseconds) MicroSecondDelay (Microsecond= s) > > #define DELAY_MILLISECONDS(Milliseconds) MicroSecondDelay ((Microsecon= ds) * 1000) > > #define DELAY_SECONDS(Seconds) MicroSecondDelay ((Microsecon= ds) * > 1000000) > > > > Do you think it would improve the maintenance of the code if macros lik= e these > > were used consistently? > > >=20 > MIke, >=20 > We have some APIs that are 100ns vs. MIcroSeconds and that causes confusi= on for > folks. Maybe we should have some helpers for both APIs and comments about= which are > which to help with the confusion. For example list the APIs that take the= 100ns > argument, and list the APIs that take Microseconds. >=20 > Thanks, >=20 > Andrew Fish >=20 >=20 > > Thanks, > > > > Mike > > > > > >> -----Original Message----- > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of= Leif > Lindholm > >> Sent: Wednesday, September 14, 2016 6:09 AM > >> To: Tian, Feng > >> Cc: edk2-devel@lists.01.org; Zeng, Star > >> Subject: Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before ac= cess MMIO > reg > >> during reset > >> > >> On Wed, Sep 14, 2016 at 09:37:13AM +0800, Feng Tian wrote: > >>> Some XHCI host controllers require to have extra 1ms delay before > >>> accessing any MMIO register during HC reset. > >> > >> Is this compliant with the XHCI specification or a bug workaround? > >> I am not going to argue about the delay, but I would like to see it > >> spelled out in the commit message. > >> > >>> Cc: Star Zeng > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Feng Tian > >>> --- > >>> MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 6 +++++- > >>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > >> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > >>> index d0f2205..cb822a6 100644 > >>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > >>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > >>> @@ -2,7 +2,7 @@ > >>> > >>> The XHCI register operation routines. > >>> > >>> -Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved. > >>> +Copyright (c) 2011 - 2016, 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 > >>> which accompanies this distribution. The full text of the license ma= y be found > at > >>> @@ -687,6 +687,10 @@ XhcResetHC ( > >>> if ((Xhc->DebugCapSupOffset =3D=3D 0xFFFFFFFF) || ((XhcReadExtCapRe= g (Xhc, Xhc- > >>> DebugCapSupOffset) & 0xFF) !=3D XHC_CAP_USB_DEBUG) || > >>> ((XhcReadExtCapReg (Xhc, Xhc->DebugCapSupOffset + XHC_DC_DCCTRL= ) & BIT0) =3D=3D > >> 0)) { > >>> XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET); > >>> + // > >>> + // some XHCI host controllers require to have extra 1ms delay be= fore > accessing > >> any MMIO register during reset. > >>> + // > >>> + gBS->Stall (XHC_1_MILLISECOND); > >> > >> OK, so this is not actually a comment on this patch, but why do we > >> have so many separate definitions of how many microseconds go in a > >> milisecond or second? > >> > >> USB_BUS_1_MILLISECOND (Pei and Dxe) > >> XHC_1_MILLISECOND (Pei and Dxe) > >> EHC_1_MILLISECOND (Pei and Dxe) > >> UHC_1_MILLISECOND (Dxe, Pei defines STALL_1_MILLI_SECOND instead) > >> > >> And IdeBusPei/PciBusDxe do similar things. > >> > >> Could we add something common to Base.h instead, like the SIZE_ ones, > >> droppping a lot of duplication, gaining uniformity, and correcting > >> spelling? > >> > >> / > >> Leif > >> > >>> Status =3D XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RE= SET, FALSE, > >> Timeout); > >>> } > >>> > >>> -- > >>> 2.7.1.windows.2 > >>> > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org > >>> https://lists.01.org/mailman/listinfo/edk2-devel > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel