From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-in4.apple.com (mail-out4.apple.com [17.151.62.26]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 02AD21A1E91 for ; Wed, 14 Sep 2016 10:37:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1473874633; x=2337788233; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-transfer-encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=3kAW30DVSCiEGWyz39B8PDGZ0DoaIkhYqVRFqV85OlY=; b=ZP9aSg5qEt/st1FGzJOQcuVLIiI4V2RoHvp14sLuQT582wluqGSFfqJdPGEhfJ9n zdDnpSVl4fLyOi0AD1fNn279V60MU2qU5c3ytdRq0IirY/U+QZOeI+RhB8hTFdLa lYOuNh/tqIG6bolkoulHxCC3/LN4WqBSTbt5/ejSxkYZyCNkoltB7CZAhoK1Dojv Tj+rns49UemqtRBFuukxu9kyQZM4I3VC+aIs4rYYar8Q7nopbagIr9SMTJDPupZC KHAIwnYOTQQftLuEzEj4D+kzSLl+e5QgoeGUNStYFHJlTBHIt8rAXyOtty6ZiDhD Gku9Iv8nkbpZpTIH6N3Bww==; Received: from relay3.apple.com (relay3.apple.com [17.128.113.83]) by mail-in4.apple.com (Apple Secure Mail Relay) with SMTP id 07.5C.07433.9CA89D75; Wed, 14 Sep 2016 10:37:13 -0700 (PDT) X-AuditID: 11973e12-f79b16d000001d09-0a-57d98ac9da82 Received: from nwk-mmpp-sz08.apple.com (nwk-mmpp-sz08.apple.com [17.128.115.25]) by relay3.apple.com (Apple SCV relay) with SMTP id CC.00.13773.9CA89D75; Wed, 14 Sep 2016 10:37:13 -0700 (PDT) MIME-version: 1.0 Received: from [17.114.155.95] by nwk-mmpp-sz08.apple.com (Oracle Communications Messaging Server 8.0.1.1.0 64bit (built Jun 15 2016)) with ESMTPSA id <0ODI003748Y0Y700@nwk-mmpp-sz08.apple.com>; Wed, 14 Sep 2016 10:37:13 -0700 (PDT) Sender: afish@apple.com From: Andrew Fish In-reply-to: Date: Wed, 14 Sep 2016 10:37:12 -0700 Cc: Leif Lindholm , "Tian, Feng" , "edk2-devel@lists.01.org" , "Zeng, Star" Message-id: <20A29753-8302-4AE0-91FF-F0368C1D7E35@apple.com> References: <5a0c21d0fe889950fda9e59aaf2272b66d429b8f.1473816993.git.feng.tian@intel.com> <20160914130833.GB16080@bivouac.eciton.net> To: Mike Kinney X-Mailer: Apple Mail (2.3112) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrOLMWRmVeSWpSXmKPExsUi2FAYrHuy62a4wbFbshZ7Dh1ltpi0m93i 0+49LBYdHf+YLPb1Wjuweize85LJ4861PWwe3bP/sQQwR3HZpKTmZJalFunbJXBlvD+4kq1g rX7F1RNTmBsY96t1MXJySAiYSPy9OY8JwhaTuHBvPVsXIxeHkMBeRokvT24AORxgRXceJELE DzJKPJ/8iBWkgVdAUOLH5HssIDXMAvISB8/LgoSZBbQkvj9qZYGof8co8WfeRGaQhLCAuMS7 M5uYQeqFBZIk2mYEgITZBJQlVsz/wA5icwqESVx5PpERxGYRUJXY0beHCWQOs8B6Ron7y34z Qey1kTh15jCYLSRwmVHi+LR8kJkiAjoS3SujIX6Rldi3YQHYLxIC19kkNmybwDqBUWQWkrNn IZw9C8nZCxiZVzEK5SZm5uhm5pnoJRYU5KTqJefnbmIExcR0O6EdjKdWWR1iFOBgVOLhbfC7 GS7EmlhWXJl7iFGag0VJnDfu7Y1wIYH0xJLU7NTUgtSi+KLSnNTiQ4xMHJxSDYzVW8o6tsze auWkqD3l1Zu9dXZ8xuKX7AKUuso7p6Uu2aohoHOm7uoCl+nrBA0mCG2JtK6MOFexKo5bXdAw 1Io9apF9i8iF9y9qwkKTQ+X/eyokf7pyTl9K3zjCWqvQ3mdXefnt3Y/EnvX4fhf6GrBR4+Z1 kyaFtQwTV/zn/ughK+4UUv50mhJLcUaioRZzUXEiANPlucFqAgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprEIsWRmVeSWpSXmKPExsUi2FAsqXuy62a4wZqjkhZ7Dh1ltpi0m93i 0+49LBYdHf+YLPb1Wjuweize85LJ4861PWwe3bP/sQQwR3HZpKTmZJalFunbJXBlvD+4kq1g rX7F1RNTmBsY96t1MXJwSAiYSNx5kNjFyAlkiklcuLeerYuRi0NI4CCjxPPJj1hBErwCghI/ Jt9jAalnFpCXOHheFiTMLKAl8f1RKwtE/TtGiT/zJjKDJIQFxCXendnEDFIvLJAk0TYjACTM JqAssWL+B3YQm1MgTOLK84mMIDaLgKrEjr49TCBzmAXWM0rcX/abCWKvjcSpM4fBbCGBy4wS x6flg8wUEdCR6F4ZDXGzrMS+DQvYJjAKzkJy6SyES2chuXQBI/MqRoGi1JzESmO9xIKCnFS9 5PzcTYzgEC4M3sH4Z5nVIUYBDkYlHt4bATfDhVgTy4orcw8xSnAwK4nwWgIjQIg3JbGyKrUo P76oNCe1+BBjMtD9E5mlRJPzgfGVVxJvaGJiYGJsbGZsbG5iTpqwkjivlNr1cCGB9MSS1OzU 1ILUIpgtTBycUg2M02fuumH27ED25Aq27knv/y9X1z9pdFrDJOJu0vf28IVTn80rYS5nvcVU 127h16/o+stwi9TWFM8d1y9eP/k8L00uaGLBWbPOjOTPdzZVhhr9Fnq++3RK+AVuPuGnMU1S 4WW1pX/aTYR81EucLdffWX/74+qEr+xiVlM5fNexWDyIXtcnuk1TiaU4I9FQi7moOBEAbIdT X6UCAAA= 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:37:14 -0000 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII > On Sep 14, 2016, at 10:14 AM, Kinney, Michael D wrote: > > Leif, > > MdePkg/Include/Library/UefiLib.h does have some helper macros for setting 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() service 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) * 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) * 1000000) > > 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, maybe > 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) > > Do you think it would improve the maintenance of the code if macros like these > were used consistently? > MIke, We have some APIs that are 100ns vs. MIcroSeconds and that causes confusion 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. Thanks, Andrew Fish > 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 access 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 may be found at >>> @@ -687,6 +687,10 @@ XhcResetHC ( >>> if ((Xhc->DebugCapSupOffset == 0xFFFFFFFF) || ((XhcReadExtCapReg (Xhc, Xhc- >>> DebugCapSupOffset) & 0xFF) != XHC_CAP_USB_DEBUG) || >>> ((XhcReadExtCapReg (Xhc, Xhc->DebugCapSupOffset + XHC_DC_DCCTRL) & BIT0) == >> 0)) { >>> XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET); >>> + // >>> + // some XHCI host controllers require to have extra 1ms delay before 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 = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET, 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