Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts to get TSC Freq

2016-08-17 Thread Kinney, Michael D 
Reviewed-by: Michael Kinney 

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, August 16, 2016 7:27 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Kinney, Michael D 
> ;
> Gao, Liming ; Paolo Bonzini ; 
> Lohr, Paul A
> 
> Subject: [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts to 
> get TSC
> Freq
> 
> Compute the number of ticks to wait to measure TSC frequency.
> Instead of (ACPI_TIMER_FREQUENCY / 1) = 357 and 357 * 1 = 357,
> use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
> 363 counts is a calibration time of 101.4 uS.
> 
> The idea comes from Michael and Paolo.
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Paolo Bonzini 
> Cc: Paul A Lohr 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng 
> ---
>  PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c | 32 
> +-
>  .../Library/AcpiTimerLib/BaseAcpiTimerLib.c| 14 +-
>  .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 14 +-
>  3 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> index e6fea231123d..020031e3f4a5 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> @@ -340,13 +340,13 @@ GetTimeInNanoSecond (
>Calculate TSC frequency.
> 
>The TSC counting frequency is determined by comparing how far it counts
> -  during a 100us period as determined by the ACPI timer. The ACPI timer is
> -  used because it counts at a known frequency.
> -  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
> -  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
> -  difference multiplied by 1 is the TSC frequency. There will be a small
> -  error because of the overhead of reading the ACPI timer. An attempt is
> -  made to determine and compensate for this error.
> +  during a 101.4 us period as determined by the ACPI timer.
> +  The ACPI timer is used because it counts at a known frequency.
> +  The TSC is sampled, followed by waiting 363 counts of the ACPI timer,
> +  or 101.4 us. The TSC is then sampled again. The difference multiplied by
> +  9861 is the TSC frequency. There will be a small error because of the
> +  overhead of reading the ACPI timer. An attempt is made to determine and
> +  compensate for this error.
> 
>@return The number of TSC counts per second.
> 
> @@ -366,22 +366,28 @@ InternalCalculateTscFrequency (
>InterruptState = SaveAndDisableInterrupts ();
> 
>TimerAddr = InternalAcpiGetAcpiTimerIoPort ();
> -  Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 1);// Set 
> Ticks to
> 100us in the future
> +  //
> +  // Compute the number of ticks to wait to measure TSC frequency.
> +  // Use 363 * 9861 = 3579543 Hz which is within 2 Hz of 
> ACPI_TIMER_FREQUENCY.
> +  // 363 counts is a calibration time of 101.4 uS.
> +  //
> +  Ticks = IoRead32 (TimerAddr) + 363;
> 
>StartTSC = AsmReadTsc (); // Get 
> base
> value for the TSC
>//
> -  // Wait until the ACPI timer has counted 100us.
> +  // Wait until the ACPI timer has counted 101.4 us.
>// Timer wrap-arounds are handled correctly by this function.
> -  // When the current ACPI timer value is greater than 'Ticks', the while 
> loop will
> exit.
> +  // When the current ACPI timer value is greater than 'Ticks',
> +  // the while loop will exit.
>//
>while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) {
>  CpuPause();
>}
> -  EndTSC = AsmReadTsc ();   // TSC 
> value
> 100us later
> +  EndTSC = AsmReadTsc ();   // TSC 
> value
> 101.4 us later
> 
>TscFrequency = MultU64x32 (
> -   (EndTSC - StartTSC), // 
> Number of TSC
> counts in 100us
> -   1// 
> Number of
> 100us in a second
> +   (EndTSC - StartTSC), // 
> Number of TSC
> counts in 101.4 us
> +   9861 // 
> Number of
> 101.4 us in a second
> );
> 
>SetInterruptState (InterruptState);
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> index 8819ebcfccef..29521f8b220b 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> @@

Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts to get TSC Freq

2016-08-17 Thread Kinney, Michael D 
Hi Paolo,

We did some experiments to wait for the next ACPI Timer tick, and they did 
not improve the results.

On real HW, the I/O Port read of the ACPI Timer takes ~1 uS, and an ACPI Timer
tick is about 0.25 uS, so the action to read the counter takes 4 times longer
than a single count.

If the I/O port read was much faster than the tick period, then adding the loop
would make sense.

Mike



> -Original Message-
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo 
> Bonzini
> Sent: Wednesday, August 17, 2016 3:31 AM
> To: Zeng, Star <star.z...@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> <liming....@intel.com>
> Subject: Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer 
> counts
> to get TSC Freq
> 
> 
> 
> On 17/08/2016 04:26, Star Zeng wrote:
> > Compute the number of ticks to wait to measure TSC frequency.
> > Instead of (ACPI_TIMER_FREQUENCY / 1) = 357 and 357 * 1 = 357,
> > use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
> > 363 counts is a calibration time of 101.4 uS.
> >
> > The idea comes from Michael and Paolo.
> >
> > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > Cc: Liming Gao <liming@intel.com>
> > Cc: Paolo Bonzini <pbonz...@redhat.com>
> > Cc: Paul A Lohr <paul.a.l...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Star Zeng <star.z...@intel.com>
> > ---
> >  PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c | 32 
> > +-
> >  .../Library/AcpiTimerLib/BaseAcpiTimerLib.c| 14 +-
> >  .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 14 +-
> >  3 files changed, 33 insertions(+), 27 deletions(-)
> >
> > diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> > index e6fea231123d..020031e3f4a5 100644
> > --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> > +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> > @@ -340,13 +340,13 @@ GetTimeInNanoSecond (
> >Calculate TSC frequency.
> >
> >The TSC counting frequency is determined by comparing how far it counts
> > -  during a 100us period as determined by the ACPI timer. The ACPI timer is
> > -  used because it counts at a known frequency.
> > -  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
> > -  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
> > -  difference multiplied by 1 is the TSC frequency. There will be a 
> > small
> > -  error because of the overhead of reading the ACPI timer. An attempt is
> > -  made to determine and compensate for this error.
> > +  during a 101.4 us period as determined by the ACPI timer.
> > +  The ACPI timer is used because it counts at a known frequency.
> > +  The TSC is sampled, followed by waiting 363 counts of the ACPI timer,
> > +  or 101.4 us. The TSC is then sampled again. The difference multiplied by
> > +  9861 is the TSC frequency. There will be a small error because of the
> > +  overhead of reading the ACPI timer. An attempt is made to determine and
> > +  compensate for this error.
> >
> >@return The number of TSC counts per second.
> >
> > @@ -366,22 +366,28 @@ InternalCalculateTscFrequency (
> >InterruptState = SaveAndDisableInterrupts ();
> >
> >TimerAddr = InternalAcpiGetAcpiTimerIoPort ();
> > -  Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 1);// Set 
> > Ticks
> to 100us in the future
> > +  //
> > +  // Compute the number of ticks to wait to measure TSC frequency.
> > +  // Use 363 * 9861 = 3579543 Hz which is within 2 Hz of 
> > ACPI_TIMER_FREQUENCY.
> > +  // 363 counts is a calibration time of 101.4 uS.
> > +  //
> > +  Ticks = IoRead32 (TimerAddr) + 363;
> >
> >StartTSC = AsmReadTsc (); // Get 
> > base
> value for the TSC
> >//
> > -  // Wait until the ACPI timer has counted 100us.
> > +  // Wait until the ACPI timer has counted 101.4 us.
> >// Timer wrap-arounds are handled correctly by this function.
> > -  // When the current ACPI timer value is greater than 'Ticks', the while 
> > loop
> will exit.
> > +  // When the current ACPI timer value is greater than 'Ticks',
> > +  // the while loop will exit.
> >//
> >while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) {
> >

Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts to get TSC Freq

2016-08-17 Thread Paolo Bonzini 


On 17/08/2016 04:26, Star Zeng wrote:
> Compute the number of ticks to wait to measure TSC frequency.
> Instead of (ACPI_TIMER_FREQUENCY / 1) = 357 and 357 * 1 = 357,
> use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
> 363 counts is a calibration time of 101.4 uS.
> 
> The idea comes from Michael and Paolo.
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Paolo Bonzini 
> Cc: Paul A Lohr 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng 
> ---
>  PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c | 32 
> +-
>  .../Library/AcpiTimerLib/BaseAcpiTimerLib.c| 14 +-
>  .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 14 +-
>  3 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> index e6fea231123d..020031e3f4a5 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> @@ -340,13 +340,13 @@ GetTimeInNanoSecond (
>Calculate TSC frequency.
>  
>The TSC counting frequency is determined by comparing how far it counts
> -  during a 100us period as determined by the ACPI timer. The ACPI timer is
> -  used because it counts at a known frequency.
> -  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
> -  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
> -  difference multiplied by 1 is the TSC frequency. There will be a small
> -  error because of the overhead of reading the ACPI timer. An attempt is
> -  made to determine and compensate for this error.
> +  during a 101.4 us period as determined by the ACPI timer.
> +  The ACPI timer is used because it counts at a known frequency.
> +  The TSC is sampled, followed by waiting 363 counts of the ACPI timer,
> +  or 101.4 us. The TSC is then sampled again. The difference multiplied by
> +  9861 is the TSC frequency. There will be a small error because of the
> +  overhead of reading the ACPI timer. An attempt is made to determine and
> +  compensate for this error.
>  
>@return The number of TSC counts per second.
>  
> @@ -366,22 +366,28 @@ InternalCalculateTscFrequency (
>InterruptState = SaveAndDisableInterrupts ();
>  
>TimerAddr = InternalAcpiGetAcpiTimerIoPort ();
> -  Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 1);// Set 
> Ticks to 100us in the future
> +  //
> +  // Compute the number of ticks to wait to measure TSC frequency.
> +  // Use 363 * 9861 = 3579543 Hz which is within 2 Hz of 
> ACPI_TIMER_FREQUENCY.
> +  // 363 counts is a calibration time of 101.4 uS.
> +  //
> +  Ticks = IoRead32 (TimerAddr) + 363;
>  
>StartTSC = AsmReadTsc (); // Get 
> base value for the TSC
>//
> -  // Wait until the ACPI timer has counted 100us.
> +  // Wait until the ACPI timer has counted 101.4 us.
>// Timer wrap-arounds are handled correctly by this function.
> -  // When the current ACPI timer value is greater than 'Ticks', the while 
> loop will exit.
> +  // When the current ACPI timer value is greater than 'Ticks',
> +  // the while loop will exit.
>//
>while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) {
>  CpuPause();
>}
> -  EndTSC = AsmReadTsc ();   // TSC 
> value 100us later
> +  EndTSC = AsmReadTsc ();   // TSC 
> value 101.4 us later
>  
>TscFrequency = MultU64x32 (
> -   (EndTSC - StartTSC), // 
> Number of TSC counts in 100us
> -   1// 
> Number of 100us in a second
> +   (EndTSC - StartTSC), // 
> Number of TSC counts in 101.4 us
> +   9861 // 
> Number of 101.4 us in a second
> );
>  
>SetInterruptState (InterruptState);
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> index 8819ebcfccef..29521f8b220b 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> @@ -20,13 +20,13 @@
>Calculate TSC frequency.
>  
>The TSC counting frequency is determined by comparing how far it counts
> -  during a 100us period as determined by the ACPI timer. The ACPI timer is
> -  used because it counts at a known frequency.
> -  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
> -  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
> -  difference multiplied by 1 is the

[edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts to get TSC Freq

2016-08-16 Thread Star Zeng 
Compute the number of ticks to wait to measure TSC frequency.
Instead of (ACPI_TIMER_FREQUENCY / 1) = 357 and 357 * 1 = 357,
use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
363 counts is a calibration time of 101.4 uS.

The idea comes from Michael and Paolo.

Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Paolo Bonzini 
Cc: Paul A Lohr 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c | 32 +-
 .../Library/AcpiTimerLib/BaseAcpiTimerLib.c| 14 +-
 .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 14 +-
 3 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c 
b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
index e6fea231123d..020031e3f4a5 100644
--- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
@@ -340,13 +340,13 @@ GetTimeInNanoSecond (
   Calculate TSC frequency.
 
   The TSC counting frequency is determined by comparing how far it counts
-  during a 100us period as determined by the ACPI timer. The ACPI timer is
-  used because it counts at a known frequency.
-  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
-  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
-  difference multiplied by 1 is the TSC frequency. There will be a small
-  error because of the overhead of reading the ACPI timer. An attempt is
-  made to determine and compensate for this error.
+  during a 101.4 us period as determined by the ACPI timer.
+  The ACPI timer is used because it counts at a known frequency.
+  The TSC is sampled, followed by waiting 363 counts of the ACPI timer,
+  or 101.4 us. The TSC is then sampled again. The difference multiplied by
+  9861 is the TSC frequency. There will be a small error because of the
+  overhead of reading the ACPI timer. An attempt is made to determine and
+  compensate for this error.
 
   @return The number of TSC counts per second.
 
@@ -366,22 +366,28 @@ InternalCalculateTscFrequency (
   InterruptState = SaveAndDisableInterrupts ();
 
   TimerAddr = InternalAcpiGetAcpiTimerIoPort ();
-  Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 1);// Set 
Ticks to 100us in the future
+  //
+  // Compute the number of ticks to wait to measure TSC frequency.
+  // Use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
+  // 363 counts is a calibration time of 101.4 uS.
+  //
+  Ticks = IoRead32 (TimerAddr) + 363;
 
   StartTSC = AsmReadTsc (); // Get 
base value for the TSC
   //
-  // Wait until the ACPI timer has counted 100us.
+  // Wait until the ACPI timer has counted 101.4 us.
   // Timer wrap-arounds are handled correctly by this function.
-  // When the current ACPI timer value is greater than 'Ticks', the while loop 
will exit.
+  // When the current ACPI timer value is greater than 'Ticks',
+  // the while loop will exit.
   //
   while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) {
 CpuPause();
   }
-  EndTSC = AsmReadTsc ();   // TSC 
value 100us later
+  EndTSC = AsmReadTsc ();   // TSC 
value 101.4 us later
 
   TscFrequency = MultU64x32 (
-   (EndTSC - StartTSC), // Number 
of TSC counts in 100us
-   1// Number 
of 100us in a second
+   (EndTSC - StartTSC), // Number 
of TSC counts in 101.4 us
+   9861 // Number 
of 101.4 us in a second
);
 
   SetInterruptState (InterruptState);
diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c 
b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
index 8819ebcfccef..29521f8b220b 100644
--- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
@@ -20,13 +20,13 @@
   Calculate TSC frequency.
 
   The TSC counting frequency is determined by comparing how far it counts
-  during a 100us period as determined by the ACPI timer. The ACPI timer is
-  used because it counts at a known frequency.
-  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
-  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
-  difference multiplied by 1 is the TSC frequency. There will be a small
-  error because of the overhead of reading the ACPI timer. An attempt is
-  made to determine and compensate for this error.
+  during a 101.4 us period as determined by the ACPI timer.
+  The ACPI timer is used