From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B3A4A1A1E90 for ; Wed, 14 Sep 2016 10:14:20 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 14 Sep 2016 10:14:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,334,1470726000"; d="scan'208";a="1055953681" Received: from orsmsx105.amr.corp.intel.com ([10.22.225.132]) by fmsmga002.fm.intel.com with ESMTP; 14 Sep 2016 10:14:19 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.173]) by ORSMSX105.amr.corp.intel.com ([169.254.2.172]) with mapi id 14.03.0248.002; Wed, 14 Sep 2016 10:14:19 -0700 From: "Kinney, Michael D" To: Leif Lindholm , "Tian, Feng" , "Kinney, Michael D" CC: "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+A= Date: Wed, 14 Sep 2016 17:14:19 +0000 Message-ID: References: <5a0c21d0fe889950fda9e59aaf2272b66d429b8f.1473816993.git.feng.tian@intel.com> <20160914130833.GB16080@bivouac.eciton.net> In-Reply-To: <20160914130833.GB16080@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGE5NzE5OGQtNjY5OC00OTQzLTlkYjYtMmUwNTQzZTlkMWNhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ildva0hlZE4zdTdxYjZucHFTaFk4eG8xdGRucCtQVXplbTVMbml4Z2VEWjA9In0= x-originating-ip: [10.22.254.139] 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 17:14:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Leif, MdePkg/Include/Library/UefiLib.h does have some helper macros for setting t= imer events periods=20 that are in 100 nS units: #define EFI_TIMER_PERIOD_MICROSECONDS(Microseconds) MultU64x32((UINT64)(M= icroseconds), 10) #define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds) MultU64x32((UINT64)(M= illiseconds), 10000) #define EFI_TIMER_PERIOD_SECONDS(Seconds) MultU64x32((UINT64)(S= econds), 10000000) I believe the examples you show are for use with the gBS->Stall() service w= hich 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) * 100= 0) #define EFI_STALL_PERIOD_SECONDS(Seconds) ((Seconds) * 1000000) 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) * 1000= 000) The other method of generating timed delays for PEI/DXE/SMM modules is usin= g the=20 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, maybe= =20 add the following to TimerLib.h: #define DELAY_NANOSECONDS(Nanoseconds) NanoSecondDelay (Nanoseconds) #define DELAY_MICROSECONDS(Microseconds) MicroSecondDelay (Microseconds) #define DELAY_MILLISECONDS(Milliseconds) MicroSecondDelay ((Microseconds)= * 1000) #define DELAY_SECONDS(Seconds) MicroSecondDelay ((Microseconds)= * 1000000)=20 Do you think it would improve the maintenance of the code if macros like th= ese were used consistently? Thanks, Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Le= if 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 acces= s MMIO reg > during reset >=20 > 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. >=20 > 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. >=20 > > 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 may= be found at > > @@ -687,6 +687,10 @@ XhcResetHC ( > > if ((Xhc->DebugCapSupOffset =3D=3D 0xFFFFFFFF) || ((XhcReadExtCapReg= (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 befo= re accessing > any MMIO register during reset. > > + // > > + gBS->Stall (XHC_1_MILLISECOND); >=20 > 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? >=20 > 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) >=20 > And IdeBusPei/PciBusDxe do similar things. >=20 > Could we add something common to Base.h instead, like the SIZE_ ones, > droppping a lot of duplication, gaining uniformity, and correcting > spelling? >=20 > / > Leif >=20 > > Status =3D XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RES= ET, 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