Re: [PATCH 1/4] Platforms: Add Sanchip Zx296718 basic library

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

You've reworked this to be targeted for edk2-platforms, which is
excellent! However:
- Please update subject line to match.
- Please also cc edk2-devel@lists.01.org (since you are with Linaro,
  it makes sense to still cc linaro-uefi)

To get the appropriate subject line for edk2-devel, as desribed by
https://github.com/tianocore/edk2-platforms/blob/about/Readme.md it
would also be helpful to add
--subject-prefix="edk2-platforms/master][PATCH" to git format-patch
command line (unless Laszlo can point out a better way of achieving
the same).

On Wed, Aug 09, 2017 at 10:12:36PM +0800, Jun Nie wrote:
> Add Sanchip Zx296718 basic library files for Zx296718 SoC
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../Library/Zx296718EvbLib/Zx296718Evb.c           | 132 +++++++++++++++++++++
>  .../Library/Zx296718EvbLib/Zx296718EvbHelper.S     |  50 ++++++++
>  .../Library/Zx296718EvbLib/Zx296718EvbLib.inf      |  53 +++++++++
>  .../Library/Zx296718EvbLib/Zx296718EvbMem.c        | 107 +++++++++++++++++
>  Silicon/Sanchip/Zx296718/Include/Zx296718.h        |  30 +++++
>  Silicon/Sanchip/Zx296718/Zx296718.dec              |  32 +++++

It is more clear (especially when submitting new ports) to generate
the patches with --stat=1000, to make the full file paths visible.

See Laszlo's guide for helpful steps when submitting patches:
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

>  6 files changed, 404 insertions(+)
>  create mode 100644 Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
>  create mode 100644 Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbHelper.S
>  create mode 100644 Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbLib.inf
>  create mode 100644 Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbMem.c
>  create mode 100644 Silicon/Sanchip/Zx296718/Include/Zx296718.h
>  create mode 100644 Silicon/Sanchip/Zx296718/Zx296718.dec

Since this ends up with a .dec, please rename
Silicon/Sanchip/Zx296718/ -> Silicon/Sanchip/Zx296718Pkg/.

That said, I don't see this .dec file providing any information that
is subsequently used enywhere. Is this in preparation for further
patches coming after this initial platform support?

> diff --git a/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
> new file mode 100644
> index 0000000..4e4eb54
> --- /dev/null
> +++ b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
> @@ -0,0 +1,132 @@
> +/** @file
> +*
> +*  Copyright (C) 2017 Sanechips Technology Co., Ltd.

Just a clarification: Sanchip/Sanechips?
"The Internet" suggests Sanechips is the appropriate company name, so
should the directory name change?

> +*  Copyright (c) 2017, Linaro Ltd.
> +*
> +*  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/IoLib.h>
> +#include <Library/ArmPlatformLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>

Please sort include files alphabetically.

> +
> +#include <Ppi/ArmMpCoreInfo.h>
> +
> +#include <Zx296718.h>
> +
> +ARM_CORE_INFO mZx296718EvbInfoTable[] = {
> +  {
> +    // Cluster 0, Core 0
> +    0x0, 0x0,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +    (UINT64)0xFFFFFFFF
> +  },
> +  {
> +    // Cluster 0, Core 1
> +    0x0, 0x1,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +    (UINT64)0xFFFFFFFF
> +  },
> +  {
> +    // Cluster 0, Core 2
> +    0x0, 0x2,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +    (UINT64)0xFFFFFFFF
> +  },
> +  {
> +    // Cluster 0, Core 3
> +    0x0, 0x3,
> +
> +    // MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +    (UINT64)0xFFFFFFFF
> +  },
> +};
> +
> +/**
> +  Return the current Boot Mode
> +
> +  This function returns the boot reason on the platform
> +
> +  @return   Return the current Boot Mode of the platform
> +
> +**/
> +EFI_BOOT_MODE
> +ArmPlatformGetBootMode (
> +  VOID
> +  )
> +{
> +  return BOOT_WITH_FULL_CONFIGURATION;
> +}
> +
> +/**
> +  Initialize controllers that must setup in the normal world
> +
> +  This function is called by the ArmPlatformPkg/Pei or ArmPlatformPkg/Pei/PlatformPeim
> +  in the PEI phase.
> +
> +**/
> +RETURN_STATUS
> +ArmPlatformInitialize (
> +  IN  UINTN                     MpId
> +  )
> +{
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Initialize the system (or sometimes called permanent) memory
> +
> +  This memory is generally represented by the DRAM.
> +
> +**/
> +VOID
> +ArmPlatformInitializeSystemMemory (
> +  VOID
> +  )
> +{
> +}
> +
> +EFI_STATUS
> +PrePeiCoreGetMpCoreInfo (
> +  OUT UINTN                   *CoreCount,
> +  OUT ARM_CORE_INFO           **ArmCoreTable
> +  )
> +{
> +  // Only support one cluster
> +  *CoreCount    = sizeof(mZx296718EvbInfoTable) / sizeof(ARM_CORE_INFO);

Do a global search and replace of "sizeof(" to "sizeof (".

> +  *ArmCoreTable = mZx296718EvbInfoTable;
> +  return EFI_SUCCESS;
> +}
> +
> +// Needs to be declared in the file. Otherwise gArmMpCoreInfoPpiGuid is undefined in the contect of PrePeiCore
> +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;
> +}
> diff --git a/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbHelper.S b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbHelper.S
> new file mode 100644
> index 0000000..829d9ef
> --- /dev/null
> +++ b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbHelper.S
> @@ -0,0 +1,50 @@
> +#
> +#  Copyright (C) 2017 Sanechips Technology Co., Ltd.
> +#  Copyright (c) 2017, Linaro Ltd.
> +#
> +#  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 <AsmMacroIoLibV8.h>
> +#include <Library/ArmLib.h>
> +
> +ASM_FUNC(ArmPlatformPeiBootAction)
> +  ret
> +
> +//UINTN
> +//ArmPlatformGetCorePosition (
> +//  IN UINTN MpId
> +//  );
> +// With this function: CorePos = (ClusterId * 4) + CoreId
> +ASM_FUNC(ArmPlatformGetCorePosition)
> +  and   x1, x0, #ARM_CORE_MASK
> +  and   x0, x0, #ARM_CLUSTER_MASK
> +  add   x0, x1, x0, LSR #6
> +  ret
> +
> +//UINTN
> +//ArmPlatformGetPrimaryCoreMpId (
> +//  VOID
> +//  );
> +ASM_FUNC(ArmPlatformGetPrimaryCoreMpId)
> +  MOV32 (w0, FixedPcdGet32 (PcdArmPrimaryCore))
> +  ret
> +
> +//UINTN
> +//ArmPlatformIsPrimaryCore (
> +//  IN UINTN MpId
> +//  );
> +ASM_FUNC(ArmPlatformIsPrimaryCore)
> +  MOV32 (w1, FixedPcdGet32 (PcdArmPrimaryCoreMask))
> +  and   x0, x0, x1
> +  MOV32 (w1, FixedPcdGet32 (PcdArmPrimaryCore))
> +  cmp   w0, w1
> +  cset  x0, eq
> +  ret
> diff --git a/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbLib.inf b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbLib.inf
> new file mode 100644
> index 0000000..18b383b
> --- /dev/null
> +++ b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbLib.inf
> @@ -0,0 +1,53 @@
> +#
> +#  Copyright (C) 2017 Sanechips Technology Co., Ltd.
> +#  Copyright (c) 2017, Linaro Ltd.
> +#
> +#  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.
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005

0x00010019 is the current version.

> +  BASE_NAME                      = Zx296718EvbLib
> +  FILE_GUID                      = e1903cfc-f842-4f9a-a6ed-856bae38a902
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmPlatformLib
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec

M (MdeM) before P (MdeP).

> +  Platform/Sanchip/Zx296718Evb/Zx296718Evb.dec
> +  Silicon/Sanchip/Zx296718/Zx296718.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  IoLib
> +  HobLib

H before I.

> +  MemoryAllocationLib
> +  SerialPortLib
> +
> +[Sources.common]
> +  Zx296718Evb.c
> +  Zx296718EvbMem.c
> +
> +[Sources.AARCH64]
> +  Zx296718EvbHelper.S | GCC
> +
> +[FeaturePcd]
> +  gEmbeddedTokenSpaceGuid.PcdCacheEnable
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdArmPrimaryCore
> +  gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> diff --git a/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbMem.c b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbMem.c
> new file mode 100644
> index 0000000..c319f4d
> --- /dev/null
> +++ b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbMem.c
> @@ -0,0 +1,107 @@
> +/** @file
> +*
> +*  Copyright (C) 2017 Sanechips Technology Co., Ltd.
> +*  Copyright (c) 2017, Linaro Ltd.
> +*
> +*  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/HobLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>

Please sort includes alphabetically.

> +
> +#include <Zx296718.h>
> +
> +// The total number of descriptors, including the final "end-of-table" descriptor.
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 5
> +
> +// DDR attributes
> +#define DDR_ATTRIBUTES_CACHED           ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> +#define DDR_ATTRIBUTES_UNCACHED         ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
> +
> +/**
> +  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;
> +  EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAttributes;
> +
> +  ResourceAttributes = (
> +      EFI_RESOURCE_ATTRIBUTE_PRESENT |
> +      EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> +      EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> +      EFI_RESOURCE_ATTRIBUTE_TESTED
> +  );
> +
> +  // Create initial Base Hob for system memory.
> +  BuildResourceDescriptorHob (
> +      EFI_RESOURCE_SYSTEM_MEMORY,
> +      ResourceAttributes,
> +      PcdGet64 (PcdSystemMemoryBase),
> +      PcdGet64 (PcdSystemMemorySize)
> +  );
> +
> +  ASSERT (VirtualMemoryMap != NULL);
> +  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages(
> +      EFI_SIZE_TO_PAGES (sizeof(ARM_MEMORY_REGION_DESCRIPTOR) *
> +                         MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
> +  if (VirtualMemoryTable == NULL) {
> +      return;
> +  }
> +
> +  if (FeaturePcdGet(PcdCacheEnable) == TRUE) {
> +      CacheAttributes = DDR_ATTRIBUTES_CACHED;
> +  } else {
> +      CacheAttributes = DDR_ATTRIBUTES_UNCACHED;
> +  }
> +
> +  Index = 0;
> +
> +  // Zx296718 SOC peripherals
> +  VirtualMemoryTable[Index].PhysicalBase    = ZX296718_PERIPH_BASE;
> +  VirtualMemoryTable[Index].VirtualBase     = ZX296718_PERIPH_BASE;
> +  VirtualMemoryTable[Index].Length          = ZX296718_PERIPH_SZ;
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // DDR - predefined 1GB size
> +  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/Sanchip/Zx296718/Include/Zx296718.h b/Silicon/Sanchip/Zx296718/Include/Zx296718.h
> new file mode 100644
> index 0000000..3ace9ab
> --- /dev/null
> +++ b/Silicon/Sanchip/Zx296718/Include/Zx296718.h
> @@ -0,0 +1,30 @@
> +/** @file
> +*
> +*  Copyright (c) 2016, Linaro Limited. All rights reserved.

Guess 2016-2017 by now?

> +*
> +*  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 __ZX296718_H__
> +#define __ZX296718_H__

Coding style specifies:
"Every header file must have a ‘#ifndef FILE_NAME_H_’ and ‘#endif’ guard" 
so you could drop 3 of those underscores.

> +
> +/***********************************************************************************
> +// Platform Memory Map
> +************************************************************************************/
> +
> +// SOC peripherals (UART, I2C, I2S, USB, etc)
> +#define ZX296718_PERIPH_BASE         0x110000
> +#define ZX296718_PERIPH_SZ           0x3000000
> +
> +#define SYS_CFG_BASE_ADDR            0x01463000
> +#define TOP_CRM_BASE_ADDR            0x01461000
> +#define AON_SYS_CTRL_BASE_ADDR       0x116000
> +
> +#endif /* __ZX296718_H__ */
> diff --git a/Silicon/Sanchip/Zx296718/Zx296718.dec b/Silicon/Sanchip/Zx296718/Zx296718.dec
> new file mode 100644
> index 0000000..9ecd11d
> --- /dev/null
> +++ b/Silicon/Sanchip/Zx296718/Zx296718.dec
> @@ -0,0 +1,32 @@
> +#
> +#  Copyright (c) 2016, Linaro Limited. 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.
> +#
> +
> +[Defines]
> +  DEC_SPECIFICATION              = 0x00010005
> +  PACKAGE_NAME                   = Zx296718
> +  PACKAGE_GUID                   = 6b180aeb-7bf7-4cf1-9310-dcd403d5dfcb
> +  PACKAGE_VERSION                = 0.1
> +
> +################################################################################
> +#
> +# Include Section - list of Include Paths that are provided by this package.
> +#                   Comments are used for Keywords and Module Types.
> +#
> +# Supported Module Types:
> +#  BASE SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION
> +#
> +################################################################################
> +[Includes.common]
> +  Include                        # Root include for the package

This path does not exist, which breaks the build.

/
    Leif

> +
> +[Guids.common]
> +  gZx296718TokenSpaceGuid      =  { 0x7827738f, 0x0651, 0x48ab, {0xad, 0x5c, 0x8d, 0x3d, 0xad, 0x58, 0x0d, 0xa9} }
> -- 
> 1.9.1
>