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

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

[Feng Tian]

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

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

[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 <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

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

[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

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

[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 <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

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

[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), 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

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

[Andrew Fish]

> 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

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

[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: 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

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

[Leif Lindholm]

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

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

[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

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

[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