Re: [PATCH edk2-platforms 20/27] Silicon/NXP: Add i.MX6 Board init library

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

On Fri, Sep 21, 2018 at 08:26:12AM +0000, Chris Co wrote:
> This adds support for board initialization which is common to
> NXP i.MX6-based platforms.
> 
> 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/Library/iMX6BoardLib/iMX6BoardHelper.S   |  89 ++++++++++++++++
>  Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6BoardMem.c      | 110 ++++++++++++++++++++
>  Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6Common.c        |  88 ++++++++++++++++
>  Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6QBoardCoreDef.c | 107 +++++++++++++++++++
>  4 files changed, 394 insertions(+)
> 
> diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6BoardHelper.S b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6BoardHelper.S
> new file mode 100644
> index 000000000000..979e98888a6b
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6BoardHelper.S
> @@ -0,0 +1,89 @@
> +## @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 <AsmMacroIoLib.h>
> +#include <AutoGen.h>
> +#include <Base.h>
> +#include <Library/ArmLib.h>
> +#include <Library/PcdLib.h>

Are all of these really used?

> +
> +.text
> +.align 2
> +
> +// GIC Cpu interface
> +#define ARM_GIC_ICCICR  0x00  // CPU Interface Control Register
> +#define ARM_GIC_ICCPMR  0x04  // Interrupt Priority Mask Register
> +#define ARM_GIC_ICCBPR  0x08  // Binary Point Register
> +#define ARM_GIC_ICCIAR  0x0C  // Interrupt Acknowledge Register
> +#define ARM_GIC_ICCEIOR 0x10  // End Of Interrupt Register
> +#define ARM_GIC_ICCRPR  0x14  // Running Priority Register
> +#define ARM_GIC_ICCPIR  0x18  // Highest Pending Interrupt Register
> +#define ARM_GIC_ICCABPR 0x1C  // Aliased Binary Point Register
> +#define ARM_GIC_ICCIDR  0xFC  // Identification Register

Why do you need to access these directly from asm?
Hmm, they also look unused?

> +
> +// SRC (System Reset Controller) register offsets & masks
> +#define IMX6_SRC_SCR     0x0
> +#define IMX6_SRC_GPR1    0x20
> +#define IMX6_SRC_GPR2    0x24
> +#define IMX6_SRC_GPR3    0x28
> +#define IMX6_SRC_GPR4    0x2C
> +#define IMX6_SRC_GPR5    0x30
> +#define IMX6_SRC_GPR6    0x34
> +#define IMX6_SRC_GPR7    0x38
> +#define IMX6_SRC_GPR8    0x3C
> +#define IMX6_SRC_GPR9    0x40
> +#define IMX6_SRC_GPR10   0x44

As do these?

> +
> +GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
> +GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> +GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
> +GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
> +
> +//UINTN
> +//ArmPlatformGetPrimaryCoreMpId (
> +//  VOID
> +//  );
> +ASM_PFX(ArmPlatformGetPrimaryCoreMpId):
> +  MOV32 (r0, FixedPcdGet32 (PcdArmPrimaryCore))
> +  ldr   r0, [r0]
> +  bx    lr
> +
> +//UINTN
> +//ArmPlatformIsPrimaryCore (
> +//  IN UINTN MpId
> +//  );
> +ASM_PFX(ArmPlatformIsPrimaryCore):
> +  mrc   p15,0,r0,c0,c0,5
> +  ands  r0,r0,#3
> +  moveq r0,#1
> +  movne r0,#0

Sprinkle some spaces after commas above please.

> +  bx    lr
> +
> +//UINTN
> +//ArmPlatformGetCorePosition (
> +//  IN UINTN MpId
> +//  );
> +ASM_PFX(ArmPlatformGetCorePosition):
> +  and   r0, r0, #ARM_CORE_MASK
> +  bx    lr
> +
> +ASM_PFX(ArmPlatformPeiBootAction):
> +  // enable unaligned access
> +  mrc   p15, 0, r1, c1, c0, 0
> +  bic   r1, r1, #0x2
> +  mcr   p15, 0, r1, c1, c0, 0
> +  isb
> +  bx r14
> +
> +ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6BoardMem.c b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6BoardMem.c
> new file mode 100644
> index 000000000000..a2b601ec9c52
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6BoardMem.c
> @@ -0,0 +1,110 @@
> +/** @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 <Library/ArmPlatformLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +
> +#include "iMX6.h"

<>

> +
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS  8
> +
> +/**
> +  Return the Virtual Memory Map of your platform
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
> +  on your platform.
> +
> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR describing
> +                                    a Physical-to-Virtual Memory mapping. This
> +                                    array must be ended by a zero-filled entry
> +
> +**/
> +VOID
> +ArmPlatformGetVirtualMemoryMap (
> +  IN ARM_MEMORY_REGION_DESCRIPTOR **VirtualMemoryMap
> +  )
> +{
> +  ARM_MEMORY_REGION_ATTRIBUTES  CacheAttributes;
> +  UINTN                         Index = 0;
> +  ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> +
> +  ASSERT (VirtualMemoryMap != NULL);
> +
> +  DEBUG ((DEBUG_VERBOSE, "Enter: ArmPlatformGetVirtualMemoryMap\n"));
> +
> +  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR *)AllocatePages (
> +                         EFI_SIZE_TO_PAGES (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> +                             MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
> +  if (VirtualMemoryTable == NULL) {
> +    return;
> +  }
> +
> +  CacheAttributes = DDR_ATTRIBUTES_CACHED;
> +  DEBUG ((DEBUG_VERBOSE, "CacheAttributes=0x%d\n", CacheAttributes));
> +
> +  // SOC registers region 1 (0x00100000 size 0x00C00000)
> +  VirtualMemoryTable[Index].PhysicalBase   = SOC_REGISTERS_PHYSICAL_BASE1;
> +  VirtualMemoryTable[Index].VirtualBase    = SOC_REGISTERS_PHYSICAL_BASE1;
> +  VirtualMemoryTable[Index].Length         = SOC_REGISTERS_PHYSICAL_LENGTH1;
> +  VirtualMemoryTable[Index].Attributes     = SOC_REGISTERS_ATTRIBUTES;
> +
> +  // PCIE Registers (0x01000000 size 0x01000000)
> +  VirtualMemoryTable[++Index].PhysicalBase = PCIE_REGISTERS_PHYSICAL_BASE;
> +  VirtualMemoryTable[Index].VirtualBase    = PCIE_REGISTERS_PHYSICAL_BASE;
> +  VirtualMemoryTable[Index].Length         = PCIE_REGISTERS_PHYSICAL_LENGTH;
> +  VirtualMemoryTable[Index].Attributes     = SOC_REGISTERS_ATTRIBUTES;
> +
> +  // SOC registers region 2 (excluding EIM) (0x02000000 size 0x00A00000)
> +  VirtualMemoryTable[++Index].PhysicalBase = SOC_REGISTERS_PHYSICAL_BASE2;
> +  VirtualMemoryTable[Index].VirtualBase    = SOC_REGISTERS_PHYSICAL_BASE2;
> +  VirtualMemoryTable[Index].Length         = SOC_REGISTERS_PHYSICAL_LENGTH2;
> +  VirtualMemoryTable[Index].Attributes     = SOC_REGISTERS_ATTRIBUTES;
> +
> +  // Framebuffer
> +  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet32 (PcdFrameBufferBase);
> +  VirtualMemoryTable[Index].VirtualBase    = FixedPcdGet32 (PcdFrameBufferBase);
> +  VirtualMemoryTable[Index].Length         = FixedPcdGet32 (PcdFrameBufferSize);
> +  VirtualMemoryTable[Index].Attributes     = DDR_ATTRIBUTES_UNCACHED;
> +
> +  // Boot (UEFI) DRAM region (kernel.img & boot working DRAM) (0x10800000 size 0x001D0000)
> +  VirtualMemoryTable[++Index].PhysicalBase   = BOOT_IMAGE_PHYSICAL_BASE;
> +  VirtualMemoryTable[Index].VirtualBase      = BOOT_IMAGE_PHYSICAL_BASE;
> +  VirtualMemoryTable[Index].Length           = BOOT_IMAGE_PHYSICAL_LENGTH;
> +  VirtualMemoryTable[Index].Attributes       = BOOT_IMAGE_ATTRIBUTES;
> +
> +  // TrustZone Shared Memory
> +  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdTrustZoneSharedMemoryBase);
> +  VirtualMemoryTable[Index].VirtualBase    = FixedPcdGet64 (PcdTrustZoneSharedMemoryBase);
> +  VirtualMemoryTable[Index].Length         = FixedPcdGet64 (PcdTrustZoneSharedMemorySize);
> +  VirtualMemoryTable[Index].Attributes     = CacheAttributes;
> +
> +  // Base SDRAM
> +  VirtualMemoryTable[++Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[Index].VirtualBase    = PcdGet64 (PcdSystemMemoryBase);
> +  VirtualMemoryTable[Index].Length         = PcdGet64 (PcdSystemMemorySize);
> +  VirtualMemoryTable[Index].Attributes     = CacheAttributes;
> +
> +  // End of Table
> +  VirtualMemoryTable[++Index].PhysicalBase = 0;
> +  VirtualMemoryTable[Index].VirtualBase    = 0;
> +  VirtualMemoryTable[Index].Length         = 0;
> +  VirtualMemoryTable[Index].Attributes     = (ARM_MEMORY_REGION_ATTRIBUTES)0;
> +
> +  ASSERT ((Index + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> +
> +  *VirtualMemoryMap = VirtualMemoryTable;
> +}
> diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6Common.c b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6Common.c
> new file mode 100644
> index 000000000000..3f45527981a7
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6Common.c
> @@ -0,0 +1,88 @@
> +/** @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 <IndustryStandard/Tpm20.h>
> +#include <IndustryStandard/Tpm2Acpi.h>
> +
> +#include <Library/ArmPlatformLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/SerialPortLib.h>
> +#include <Library/TimerLib.h>
> +
> +#include <iMX6.h>
> +#include <iMX6BoardLib.h>
> +#include <iMX6Timer.h>
> +
> +VOID __DoNothing() {}
> +VOID SdhcInit () __attribute__ ((weak, alias ("__DoNothing")));
> +VOID EhciInit () __attribute__ ((weak, alias ("__DoNothing")));
> +VOID EnetInit () __attribute__ ((weak, alias ("__DoNothing")));
> +VOID I2cInit () __attribute__ ((weak, alias ("__DoNothing")));
> +VOID SpiInit () __attribute__ ((weak, alias ("__DoNothing")));
> +VOID PcieInit () __attribute__ ((weak, alias ("__DoNothing")));
> +VOID SetupAudio () __attribute__ ((weak, alias ("__DoNothing")));

This is very unidiomatic for EDK2.
It would be nicer to install a board-specific protocol that was
discovered here and returned a set of function pointers (and a
version?) which could then be called if populated.
And give this library its own .inf, that added a depex on the
board-specific protocol.

/
    Leif

> +
> +/**
> +  Initialize controllers that must setup at the early stage
> +**/
> +RETURN_STATUS
> +ArmPlatformInitialize (
> +  IN  UINTN                     MpId
> +  )
> +{
> +  if (!ArmPlatformIsPrimaryCore (MpId)) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  ImxClkPwrInit ();
> +
> +  // Initialize default UEFI debug port early so we can use its debug output
> +  SerialPortInitialize ();
> +  SerialPortWrite (
> +    (UINT8 *)SERIAL_DEBUG_PORT_INIT_MSG,
> +    (UINTN)sizeof (SERIAL_DEBUG_PORT_INIT_MSG));
> +
> +  // Initialize timer early on because the following init path will be calling
> +  // delay functions. PrePi.c calls ArmPlatformInitialize before it calls
> +  // TimerConstructor to initialize the timer.
> +  TimerConstructor ();
> +
> +  SdhcInit ();
> +  EhciInit ();
> +  EnetInit ();
> +  I2cInit ();
> +  SpiInit ();
> +  PcieInit ();
> +  SetupAudio ();
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Return the current Boot Mode
> +
> +  This function returns the boot reason on the platform
> +
> +**/
> +EFI_BOOT_MODE
> +ArmPlatformGetBootMode (
> +  VOID
> +  )
> +{
> +  return BOOT_WITH_FULL_CONFIGURATION;
> +}
> diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6QBoardCoreDef.c b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6QBoardCoreDef.c
> new file mode 100644
> index 000000000000..b59f9fe6f01f
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6BoardLib/iMX6QBoardCoreDef.c
> @@ -0,0 +1,107 @@
> +/** @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 <Library/ArmPlatformLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +
> +#include <Ppi/ArmMpCoreInfo.h>
> +
> +ARM_CORE_INFO iMX6Ppi[] = {
> +  {
> +    // Cluster 0, Core 0
> +    0x0, 0x0,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value.
> +    // Not used with i.MX6, set to 0
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (UINT64)0
> +  },
> +
> +#if FixedPcdGet32(PcdCoreCount) > 1
> +  {
> +    // Cluster 0, Core 1
> +    0x0, 0x1,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +    // Not used with i.MX6, set to 0
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (UINT64)0
> +  },
> +#endif // FixedPcdGet32(PcdCoreCount) > 1
> +
> +#if FixedPcdGet32(PcdCoreCount) > 2
> +  {
> +    // Cluster 0, Core 2
> +    0x0, 0x2,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +    // Not used with i.MX6, set to 0
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (UINT64)0
> +  },
> +
> +  {
> +    // Cluster 0, Core 3
> +    0x0, 0x3,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +    // Not used with i.MX6, set to 0
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (EFI_PHYSICAL_ADDRESS)0x00000000,
> +    (UINT64)0
> +  }
> +#endif // FixedPcdGet32(PcdCoreCount) > 2
> +};
> +
> +EFI_STATUS
> +PrePeiCoreGetMpCoreInfo (
> +  OUT UINTN                   *CoreCount,
> +  OUT ARM_CORE_INFO           **ArmCoreTable
> +  )
> +{
> +  // Only support one cluster
> +  *CoreCount    = sizeof (iMX6Ppi) / sizeof (ARM_CORE_INFO);
> +  ASSERT (*CoreCount == FixedPcdGet32 (PcdCoreCount));
> +  *ArmCoreTable = iMX6Ppi;
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_GUID mArmMpCoreInfoPpiGuid = ARM_MP_CORE_INFO_PPI_GUID;
> +ARM_MP_CORE_INFO_PPI mMpCoreInfoPpi = { PrePeiCoreGetMpCoreInfo };
> +EFI_PEI_PPI_DESCRIPTOR      gPlatformPpiTable[] = {
> +  {
> +    EFI_PEI_PPI_DESCRIPTOR_PPI,
> +    &mArmMpCoreInfoPpiGuid,
> +    &mMpCoreInfoPpi
> +  }
> +};
> +
> +VOID
> +ArmPlatformGetPlatformPpiList (
> +  OUT UINTN                   *PpiListSize,
> +  OUT EFI_PEI_PPI_DESCRIPTOR  **PpiList
> +  )
> +{
> +  *PpiListSize = sizeof (gPlatformPpiTable);
> +  *PpiList = gPlatformPpiTable;
> +}
> -- 
> 2.16.2.gvfs.1.33.gf5370f1
>