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

On 2017年08月10日 21:04, Leif Lindholm wrote:

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%27s-unkempt-git...

...

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?

No plan yet. So this file will be deleted.

...

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.

Will change it. Is there any coding style readme file? Because I find most of header file use __FILE_NAME_H__ sytle macro.

...

+/*********************************************************************************** +// 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.

This *dec file is unnecessary and will be deleted as Ard suggested. All other comments here will be addressed in next version.

/ Leif

...

+[Guids.common]

• gZx296718TokenSpaceGuid = { 0x7827738f, 0x0651, 0x48ab, {0xad, 0x5c, 0x8d, 0x3d, 0xad, 0x58, 0x0d, 0xa9} }

-- 1.9.1