* [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset @ 2016-09-14 1:37 Feng Tian 2016-09-14 2:58 ` Zeng, Star 2016-09-14 13:08 ` Leif Lindholm 0 siblings, 2 replies; 10+ messages in thread From: Feng Tian @ 2016-09-14 1:37 UTC (permalink / raw) To: star.zeng; +Cc: edk2-devel Some XHCI host controllers require to have extra 1ms delay before accessing any MMIO register during HC reset. Cc: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Feng Tian <feng.tian@intel.com> --- 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.<BR> +Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> 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); Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET, FALSE, Timeout); } -- 2.7.1.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 1:37 [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset Feng Tian @ 2016-09-14 2:58 ` Zeng, Star 2016-09-14 2:59 ` Tian, Feng 2016-09-14 13:08 ` Leif Lindholm 1 sibling, 1 reply; 10+ messages in thread From: Zeng, Star @ 2016-09-14 2:58 UTC (permalink / raw) To: Tian, Feng; +Cc: edk2-devel@lists.01.org How about to use XhciDxe instead of Xhci in the title to be more differentiated with another change in XhciPei? Reviewed-by: Star Zeng <star.zeng@intel.com> Thanks, Star -----Original Message----- From: Tian, Feng Sent: Wednesday, September 14, 2016 9:37 AM To: Zeng, Star <star.zeng@intel.com> Cc: edk2-devel@lists.01.org Subject: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset Some XHCI host controllers require to have extra 1ms delay before accessing any MMIO register during HC reset. Cc: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Feng Tian <feng.tian@intel.com> --- 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.<BR> +Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> 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); Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET, FALSE, Timeout); } -- 2.7.1.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 2:58 ` Zeng, Star @ 2016-09-14 2:59 ` Tian, Feng 0 siblings, 0 replies; 10+ messages in thread From: Tian, Feng @ 2016-09-14 2:59 UTC (permalink / raw) To: Zeng, Star; +Cc: edk2-devel@lists.01.org, Tian, Feng Ok, I will update the commit title to highlight it's a change in XhciDxe. Thanks Feng -----Original Message----- From: Zeng, Star Sent: Wednesday, September 14, 2016 10:58 AM To: Tian, Feng <feng.tian@intel.com> Cc: edk2-devel@lists.01.org Subject: RE: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset How about to use XhciDxe instead of Xhci in the title to be more differentiated with another change in XhciPei? Reviewed-by: Star Zeng <star.zeng@intel.com> Thanks, Star -----Original Message----- From: Tian, Feng Sent: Wednesday, September 14, 2016 9:37 AM To: Zeng, Star <star.zeng@intel.com> Cc: edk2-devel@lists.01.org Subject: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset Some XHCI host controllers require to have extra 1ms delay before accessing any MMIO register during HC reset. Cc: Star Zeng <star.zeng@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Feng Tian <feng.tian@intel.com> --- 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.<BR> +Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> 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); Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET, FALSE, Timeout); } -- 2.7.1.windows.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 1:37 [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset Feng Tian 2016-09-14 2:58 ` Zeng, Star @ 2016-09-14 13:08 ` Leif Lindholm 2016-09-14 17:14 ` Kinney, Michael D 2016-09-20 1:39 ` Tian, Feng 1 sibling, 2 replies; 10+ messages in thread From: Leif Lindholm @ 2016-09-14 13:08 UTC (permalink / raw) To: Feng Tian; +Cc: star.zeng, edk2-devel 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 <star.zeng@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Feng Tian <feng.tian@intel.com> > --- > 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.<BR> > +Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 13:08 ` Leif Lindholm @ 2016-09-14 17:14 ` Kinney, Michael D 2016-09-14 17:37 ` Andrew Fish 2016-09-15 10:19 ` Leif Lindholm 2016-09-20 1:39 ` Tian, Feng 1 sibling, 2 replies; 10+ messages in thread From: Kinney, Michael D @ 2016-09-14 17:14 UTC (permalink / raw) To: Leif Lindholm, Tian, Feng, Kinney, Michael D Cc: edk2-devel@lists.01.org, Zeng, Star 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? 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 <feng.tian@intel.com> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com> > 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 <star.zeng@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Feng Tian <feng.tian@intel.com> > > --- > > 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.<BR> > > +Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 17:14 ` Kinney, Michael D @ 2016-09-14 17:37 ` Andrew Fish 2016-09-14 18:07 ` Kinney, Michael D 2016-09-15 10:19 ` Leif Lindholm 1 sibling, 1 reply; 10+ messages in thread From: Andrew Fish @ 2016-09-14 17:37 UTC (permalink / raw) To: Mike Kinney Cc: Leif Lindholm, Tian, Feng, edk2-devel@lists.01.org, Zeng, Star > On Sep 14, 2016, at 10:14 AM, Kinney, Michael D <michael.d.kinney@intel.com> 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 <feng.tian@intel.com> >> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com> >> 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 <star.zeng@intel.com> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Feng Tian <feng.tian@intel.com> >>> --- >>> 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.<BR> >>> +Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> >>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 17:37 ` Andrew Fish @ 2016-09-14 18:07 ` Kinney, Michael D 0 siblings, 0 replies; 10+ messages in thread From: Kinney, Michael D @ 2016-09-14 18:07 UTC (permalink / raw) To: afish@apple.com, Kinney, Michael D Cc: Leif Lindholm, Tian, Feng, edk2-devel@lists.01.org, Zeng, Star Andrew, The 100 ns vs. 1 us differences are what I tried to highlight here and propose 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 <michael.d.kinney@intel.com> > Cc: Leif Lindholm <leif.lindholm@linaro.org>; Tian, Feng <feng.tian@intel.com>; edk2- > devel@lists.01.org; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg > during reset > > > > On Sep 14, 2016, at 10:14 AM, Kinney, Michael D <michael.d.kinney@intel.com> 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 <feng.tian@intel.com> > >> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com> > >> 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 <star.zeng@intel.com> > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Feng Tian <feng.tian@intel.com> > >>> --- > >>> 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.<BR> > >>> +Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> > >>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 17:14 ` Kinney, Michael D 2016-09-14 17:37 ` Andrew Fish @ 2016-09-15 10:19 ` Leif Lindholm 2016-09-20 2:01 ` Tian, Feng 1 sibling, 1 reply; 10+ messages in thread From: Leif Lindholm @ 2016-09-15 10:19 UTC (permalink / raw) To: Kinney, Michael D; +Cc: Tian, Feng, edk2-devel@lists.01.org, Zeng, Star On Wed, Sep 14, 2016 at 05:14:19PM +0000, Kinney, Michael D wrote: > 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. Correct. > 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) > Either (or both) of those two look good to me. The latter has the benefit of a smaller call site, at the cost of perhaps obscuring the dependency on UefiRuntimeServicesTableLib. > 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) I'm less concerned about those, but it could make sense for completeness. > Do you think it would improve the maintenance of the code if macros > like these were used consistently? It would certainly be good to reduce duplication, and consistency would help with the readability of the code. (Which is good for reviewing.) Regards, Leif ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-15 10:19 ` Leif Lindholm @ 2016-09-20 2:01 ` Tian, Feng 0 siblings, 0 replies; 10+ messages in thread From: Tian, Feng @ 2016-09-20 2:01 UTC (permalink / raw) To: Leif Lindholm, Kinney, Michael D Cc: edk2-devel@lists.01.org, Zeng, Star, Tian, Feng Leif & Mike I will help to submit another tracker to Bugzilla to track this request. 1. create below new clear time definitions 2. remove those separate definitions in modules to get code clean. Thanks Feng -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif Lindholm Sent: Thursday, September 15, 2016 6:20 PM To: Kinney, Michael D <michael.d.kinney@intel.com> Cc: Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset On Wed, Sep 14, 2016 at 05:14:19PM +0000, Kinney, Michael D wrote: > 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. Correct. > 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) > Either (or both) of those two look good to me. The latter has the benefit of a smaller call site, at the cost of perhaps obscuring the dependency on UefiRuntimeServicesTableLib. > 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) I'm less concerned about those, but it could make sense for completeness. > Do you think it would improve the maintenance of the code if macros > like these were used consistently? It would certainly be good to reduce duplication, and consistency would help with the readability of the code. (Which is good for reviewing.) Regards, Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset 2016-09-14 13:08 ` Leif Lindholm 2016-09-14 17:14 ` Kinney, Michael D @ 2016-09-20 1:39 ` Tian, Feng 1 sibling, 0 replies; 10+ messages in thread From: Tian, Feng @ 2016-09-20 1:39 UTC (permalink / raw) To: Leif Lindholm; +Cc: Zeng, Star, edk2-devel@lists.01.org, Tian, Feng Lefi, This delay is just a workaround for some XHCI HCs, I will add such info into commit log. Thanks Feng -----Original Message----- From: Leif Lindholm [mailto:leif.lindholm@linaro.org] Sent: Wednesday, September 14, 2016 9:09 PM To: Tian, Feng <feng.tian@intel.com> Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org 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 <star.zeng@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Feng Tian <feng.tian@intel.com> > --- > 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.<BR> > +Copyright (c) 2011 - 2016, Intel Corporation. All rights > +reserved.<BR> > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-09-20 2:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-14 1:37 [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset Feng Tian 2016-09-14 2:58 ` Zeng, Star 2016-09-14 2:59 ` Tian, Feng 2016-09-14 13:08 ` Leif Lindholm 2016-09-14 17:14 ` Kinney, Michael D 2016-09-14 17:37 ` Andrew Fish 2016-09-14 18:07 ` Kinney, Michael D 2016-09-15 10:19 ` Leif Lindholm 2016-09-20 2:01 ` Tian, Feng 2016-09-20 1:39 ` Tian, Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox