Re: [PATCH V2] PcAtChipsetPkg AcpiTimerLib: Clear bits [31:24] after reading by IoRead32()

["Zeng, Star" <star.zeng@intel.com>]

Mike,

Just get your point, that means the [31:24] will be always ignored regardless of the ACPI timer is 24Bits or 32Bits.

How about updating the comments like below?

"Note: The implementation only uses the lower 24-bits of the ACPI timer and can be compatible with both 24-bits and 32-bits ACPI timers."


Thanks,
Star
-----Original Message-----
From: Kinney, Michael D 
Sent: Wednesday, September 28, 2016 10:58 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH V2] PcAtChipsetPkg AcpiTimerLib: Clear bits [31:24] after reading by IoRead32()

Star,

I think the comment should be updated.  This updated algorithm only uses the lower 24-bits of the ACPI timer.  It is compatible with both 24-bit and 32-bit ACPI timers.

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Star Zeng
> Sent: Wednesday, September 28, 2016 5:49 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: [edk2] [PATCH V2] PcAtChipsetPkg AcpiTimerLib: Clear bits 
> [31:24] after reading by IoRead32()
> 
> From: "Zeng, Star" <star.zeng@intel.com>
> 
> Clear bits [31:24] when reading ACPI timer count by IoRead32(), also 
> add comments "Note: The library only supports 24Bits ACPI timer" in INF.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c       | 8 ++++----
>  PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf | 6 ++++--  
> PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf  | 8 +++++---
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> index 020031e3f4a5..792781a33f3f 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> @@ -162,14 +162,14 @@ InternalAcpiDelay (
>      //
>      // The target timer count is calculated here
>      //
> -    Ticks = IoRead32 (Port) + Delay;
> +    Ticks = IoBitFieldRead32 (Port, 0, 23) + Delay;
>      Delay = BIT22;
>      //
>      // Wait until time out
>      // Delay >= 2^23 could not be handled by this function
>      // Timer wrap-arounds are handled correctly by this function
>      //
> -    while (((Ticks - IoRead32 (Port)) & BIT23) == 0) {
> +    while (((Ticks - IoBitFieldRead32 (Port, 0, 23)) & BIT23) == 0) {
>        CpuPause ();
>      }
>    } while (Times-- > 0);
> @@ -371,7 +371,7 @@ InternalCalculateTscFrequency (
>    // 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;
> +  Ticks = IoBitFieldRead32 (TimerAddr, 0, 23) + 363;
> 
>    StartTSC = AsmReadTsc ();                                         // Get base value
> for the TSC
>    //
> @@ -380,7 +380,7 @@ InternalCalculateTscFrequency (
>    // When the current ACPI timer value is greater than 'Ticks',
>    // the while loop will exit.
>    //
> -  while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) {
> +  while (((Ticks - IoBitFieldRead32 (TimerAddr, 0, 23)) & BIT23) == 
> + 0) {
>      CpuPause();
>    }
>    EndTSC = AsmReadTsc ();                                           // TSC value 101.4
> us later
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> index 48caebff1354..913fbfe7d30e 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> @@ -2,9 +2,11 @@
>  #  Base ACPI Timer Library
>  #
>  #  Provides basic timer support using the ACPI timer hardware.  The 
> performance -#  counter features are provided by the processors time stamp counter.
> +#  counter features are provided by the processors time stamp counter.
>  #
> -#  Copyright (c) 2013 - 2015, Intel Corporation. All rights 
> reserved.<BR>
> +#  Note: The library only supports 24Bits ACPI timer.
> +#
> +#  Copyright (c) 2013 - 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 diff --git 
> a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> index 3446c03eda21..33d4970afc57 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> @@ -2,9 +2,11 @@
>  #  DXE ACPI Timer Library
>  #
>  #  Provides basic timer support using the ACPI timer hardware.  The 
> performance -#  counter features are provided by the processors time stamp counter.
> +#  counter features are provided by the processors time stamp counter.
>  #
> -#  Copyright (c) 2013 - 2015, Intel Corporation. All rights 
> reserved.<BR>
> +#  Note: The library only supports 24Bits ACPI timer.
> +#
> +#  Copyright (c) 2013 - 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 @@ -49,4 +51,4 @@ [Pcd]
>    gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPciBarRegisterOffset     ## CONSUMES
>    gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPortBaseAddress          ## CONSUMES
>    gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiPm1TmrOffset               ## CONSUMES
> -  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPortBaseAddressMask      ## CONSUMES
> \ No newline at end of file
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdAcpiIoPortBaseAddressMask      ## CONSUMES
> --
> 2.8.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel