Re: [PATCH edk2-platforms 15/27] Silicon/NXP: Add i.MX6 GPT Timer library

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Sep 21, 2018 at 08:26:07AM +0000, Chris Co wrote:
> This adds support for GPT Timer on NXP i.MX6 SoCs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co <christopher.co@microsoft.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h           |  24 ++
>  Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c   | 246 ++++++++++++++++++++
>  Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.inf |  45 ++++
>  3 files changed, 315 insertions(+)
> 
> diff --git a/Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h b/Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h
> new file mode 100644
> index 000000000000..fbac9d2a61c0
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h
> @@ -0,0 +1,24 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft 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
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#ifndef _IMX6_TIMER_H_
> +#define _IMX6_TIMER_H_
> +
> +RETURN_STATUS
> +EFIAPI
> +TimerConstructor (
> +  VOID
> +  );
> +
> +#endif  /* _IMX6_TIMER_H_ */
> diff --git a/Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c b/Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c
> new file mode 100644
> index 000000000000..fa55cee242ef
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c
> @@ -0,0 +1,246 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft 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
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#include <Base.h>
> +#include <Uefi.h>
> +
> +#include <Library/ArmLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/TimerLib.h>
> +
> +#include <common_gpt.h>
> +#include <iMX6.h>
> +#include <iMX6Timer.h>
> +
> +RETURN_STATUS
> +EFIAPI
> +TimerConstructor (
> +  VOID
> +  )
> +{
> +  PCSP_GPT_REGS pGpt;
> +  UINT32 FreqPreScale;
> +
> +  pGpt = (PCSP_GPT_REGS)CSP_BASE_REG_PA_GPT;
> +
> +  ASSERT (SOC_OSC_FREQUENCY_REF_HZ >= PcdGet32 (PcdArmArchTimerFreqInHz));

This line strikes me as slightly counterintuitive.
Are you reusing the Arch Timer Pcd for a custom timer?

If this is the case, we should still be able to share some of the code
from the ArmPkg ArmArchTimerLib by breaking it out and sharing it
(like we did with TimeBaseLib).

> +
> +  // Calculate the scale factor since we are using the 24Mhz oscillator
> +  // as reference.
> +  FreqPreScale = SOC_OSC_FREQUENCY_REF_HZ / PcdGet32 (PcdArmArchTimerFreqInHz);
> +  ASSERT (FreqPreScale <= (1 << GPT_PR_PRESCALER_WID));
> +
> +  // Set the frequency scale
> +  MmioWrite32 ((UINTN)&pGpt->PR, FreqPreScale - 1);
> +
> +#if defined(CPU_IMX6DQ) || defined (CPU_IMX6DQP)
> +  // Set GPT configuration:
> +  // - GPT Enabled
> +  // - Use the 24Mhz oscillator source
> +  MmioWrite32 ((UINTN)&pGpt->CR,
> +            (GPT_CR_EN_ENABLE << GPT_CR_EN_LSH) |

Can you do a global search and replace _LSH/_SHIFT?

/
    Leif