subject:"\[edk2\] \[patch\] MdeModulePkg\/Xhci\: add 1ms delay before access MMIO reg during reset"

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-19 Thread 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-boun...@lists.01.org] On Behalf Of Leif 
Lindholm
Sent: Thursday, September 15, 2016 6:20 PM
To: Kinney, Michael D <michael.d.kin...@intel.com>
Cc: Tian, Feng <feng.t...@intel.com>; edk2-devel@lists.01.org; Zeng, Star 
<star.z...@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 +, 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), 1)
>   #define EFI_TIMER_PERIOD_SECONDS(Seconds)   
> MultU64x32((UINT64)(Seconds), 1000)
> 
> 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) * 100)
> 
> 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) * 
> 100)
> 

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) * 
> 100) 

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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-15 Thread Leif Lindholm

On Wed, Sep 14, 2016 at 05:14:19PM +, 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), 1)
>   #define EFI_TIMER_PERIOD_SECONDS(Seconds)   
> MultU64x32((UINT64)(Seconds), 1000)
> 
> 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) * 100)
> 
> 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) * 
> 100)
> 

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) * 
> 100) 

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

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-14 Thread Kinney, Michael D

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: af...@apple.com [mailto:af...@apple.com]
> Sent: Wednesday, September 14, 2016 10:37 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; Tian, Feng 
> <feng.t...@intel.com>; edk2-
> de...@lists.01.org; Zeng, Star <star.z...@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.kin...@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), 1)
> >  #define EFI_TIMER_PERIOD_SECONDS(Seconds)   
> > MultU64x32((UINT64)(Seconds),
> 1000)
> >
> > 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) * 100)
> >
> > 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) * 
> > 100)
> >
> > 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) *
> 100)
> >
> > 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-boun...@lists.01.org] On Behalf Of Leif
> Lindholm
> >> Sent: Wednesday, September 14, 2016 6:09 AM
> >> To: Tian, Feng <feng.t...@intel.com>
> >> Cc: edk2-devel@lists.01.org; Zeng, Star <star.z...@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.z...@intel.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Feng Tian <feng.t...@intel.com>
> >>> -

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-14 Thread Andrew Fish


> On Sep 14, 2016, at 10:14 AM, Kinney, Michael D <michael.d.kin...@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), 1)
>  #define EFI_TIMER_PERIOD_SECONDS(Seconds)   
> MultU64x32((UINT64)(Seconds), 1000)
> 
> 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) * 100)
> 
> 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) * 100)
> 
> 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) * 
> 100) 
> 
> 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-boun...@lists.01.org] On Behalf Of Leif 
>> Lindholm
>> Sent: Wednesday, September 14, 2016 6:09 AM
>> To: Tian, Feng <feng.t...@intel.com>
>> Cc: edk2-devel@lists.01.org; Zeng, Star <star.z...@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.z...@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Feng Tian <feng.t...@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.
>>> +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 == 0x) || ((XhcReadExtCapReg (Xhc, 
>>> Xhc-
>>> DebugCapSupOffset) & 0xFF) != XHC_CAP_USB_DEBUG)

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-14 Thread Kinney, Michael D

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), 1)
  #define EFI_TIMER_PERIOD_SECONDS(Seconds)   
MultU64x32((UINT64)(Seconds), 1000)

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) * 100)

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) * 100)

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) * 
100) 

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-boun...@lists.01.org] On Behalf Of Leif 
> Lindholm
> Sent: Wednesday, September 14, 2016 6:09 AM
> To: Tian, Feng <feng.t...@intel.com>
> Cc: edk2-devel@lists.01.org; Zeng, Star <star.z...@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.z...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Feng Tian <feng.t...@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.
> > +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 == 0x) || ((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)
> 
>

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-14 Thread Leif Lindholm

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 == 0x) || ((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

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-13 Thread 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 
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 

Thanks,
Star
-Original Message-
From: Tian, Feng 
Sent: Wednesday, September 14, 2016 9:37 AM
To: Zeng, Star 
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 
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 == 0x) || ((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

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-13 Thread Zeng, Star

How about to use XhciDxe instead of Xhci in the title to be more differentiated 
with another change in XhciPei?

Reviewed-by: Star Zeng 

Thanks,
Star
-Original Message-
From: Tian, Feng 
Sent: Wednesday, September 14, 2016 9:37 AM
To: Zeng, Star 
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 
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 == 0x) || ((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

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-13 Thread Feng Tian

Some XHCI host controllers require to have extra 1ms delay before
accessing any MMIO register during HC reset.

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 == 0x) || ((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

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

[edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

9 matches

Site Navigation

Mail list logo

Footer information