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

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

Laszlo,

I agree that is a better idea, please check the V2 patch.

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, September 28, 2016 6:51 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: edk2-devel@ml01.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Clear bits [31:24] after reading by IoRead32()

Star,

On 09/28/16 12:17, Star Zeng wrote:
> 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>
> 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(-)

Just a question: would the IoBitFieldRead32() function apply in this case? If it is applicable, then I think it could make the code easier to
read:

  IoBitFieldRead32 (Port, 0, 23)

Just an idea.

Thanks
Laszlo

> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> index 020031e3f4a5..67f165ed20ac 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 = (IoRead32 (Port) & (BIT24 - 1)) + 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 - (IoRead32 (Port) & (BIT24 - 1))) & 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 = (IoRead32 (TimerAddr) & (BIT24 - 1)) + 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 - (IoRead32 (TimerAddr) & (BIT24 - 1))) & 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
>