Re: [Linaro-uefi] [RFC 01/20] Platforms/Marvell: Add initial support for Armada70x0 SOC lib

21 Jun 2016

Please find inline comments.
2016-06-20 17:17 GMT+02:00 Leif Lindholm leif.lindholm@linaro.org:
...
On Fri, Apr 29, 2016 at 07:25:51PM +0200, Marcin Wojtas wrote:
...
From: Yehuda Yitschak yehuday@marvell.com
This patch adds initial support for Armada70x0 SoC's initialization.
It implements empty mandatory hooks which are currently mostly empty
except for Memory mapping information.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yehuda Yitschak yehuday@marvell.com
Signed-off-by: Marcin Wojtas mw@semihalf.com

.../Armada70x0Lib/AArch64/ArmPlatformHelper.S      |  66 +++++++++
 .../Armada/Library/Armada70x0Lib/Armada70x0Lib.c   | 153 +++++++++++++++++++++
 .../Armada/Library/Armada70x0Lib/Armada70x0Lib.inf |  67 +++++++++
 .../Library/Armada70x0Lib/Armada70x0LibMem.c       |  93 +++++++++++++
 4 files changed, 379 insertions(+)
 create mode 100644 Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
 create mode 100644 Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.c
 create mode 100644 Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
 create mode 100644 Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c

diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
new file mode 100644
index 0000000..4ad435c
--- /dev/null
+++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
@@ -0,0 +1,66 @@
+//Based on ArmPlatformPkg/Library/ArmPlatformLibNull/AArch64/ArmPlatformHelper.S
+//
+//  Copyright (c) 2012-2013, ARM Limited. All rights reserved.
+//  Copyright (c) 2016, Marvell. 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 <AsmMacroIoLibV8.h>
+#include <Library/ArmLib.h>



+.text
+.align 2



+GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
+GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
+GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
+GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)



+GCC_ASM_IMPORT(_gPcd_FixedAtBuild_PcdArmPrimaryCore)
+GCC_ASM_IMPORT(_gPcd_FixedAtBuild_PcdArmPrimaryCoreMask)



+ASM_PFX(ArmPlatformPeiBootAction):

ret


+//UINTN
+//ArmPlatformGetCorePosition (
+//  IN UINTN MpId
+//  );
+// With this function: CorePos = (ClusterId * 4) + CoreId
+ASM_PFX(ArmPlatformGetCorePosition):

and   x1, x0, #ARM_CORE_MASK
and   x0, x0, #ARM_CLUSTER_MASK
add   x0, x1, x0, LSR #6
ret


+//UINTN
+//ArmPlatformGetPrimaryCoreMpId (
+//  VOID
+//  );
+ASM_PFX(ArmPlatformGetPrimaryCoreMpId):

LoadConstantToReg (_gPcd_FixedAtBuild_PcdArmPrimaryCore, x0)
ldrh  w0, [x0]
ret


+//UINTN
+//ArmPlatformIsPrimaryCore (
+//  IN UINTN MpId
+//  );
+ASM_PFX(ArmPlatformIsPrimaryCore):

LoadConstantToReg (_gPcd_FixedAtBuild_PcdArmPrimaryCoreMask, x1)
ldrh  w1, [x1]
and   x0, x0, x1
LoadConstantToReg (_gPcd_FixedAtBuild_PcdArmPrimaryCore, x1)
ldrh  w1, [x1]
cmp   w0, w1
mov   x0, #1
mov   x1, #0
csel  x0, x0, x1, eq
ret

diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.c b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.c
new file mode 100644
index 0000000..8609700
--- /dev/null
+++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.c
@@ -0,0 +1,153 @@
+/**Based on ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c
+*
+*  Copyright (c) 2011-2012, ARM Limited. All rights reserved.
+*  Copyright (c) 2016, Marvell International Ltd. 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/ArmLib.h>
+#include <Library/ArmPlatformLib.h>
+#include <Ppi/ArmMpCoreInfo.h>




+ARM_CORE_INFO mArmada7040MpCoreInfoTable[] = {

{
// Cluster 0, Core 0
0x0, 0x0,

// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(UINT64)0xFFFFFFFF
},
{
// Cluster 0, Core 1
0x0, 0x1,

// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(UINT64)0xFFFFFFFF
},
{
// Cluster 0, Core 2
0x0, 0x2,

// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(UINT64)0xFFFFFFFF
},
{
// Cluster 0, Core 3
0x0, 0x3,

// MP Core MailBox Set/Get/Clear Addresses and Clear Value
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(EFI_PHYSICAL_ADDRESS)0,
(UINT64)0xFFFFFFFF
}

+};
First a question of clarification - since this port defines the
multicore functions, does it mean the boot mechanism used is the
parking protocol and that ARM Trusted Firmware is not used?
We are using ARM Trusted Firmware. Basic support (PlatformLibrary) is
based on ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.c,
hence this code is present here. Should this port be removed from
there (placed in ATF)?
...
...



+/**

Return the current Boot Mode

This function returns the boot reason on 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/PrePi or ArmPlatformPkg/PlatformPei
in the PEI phase.


+**/
+RETURN_STATUS
+ArmPlatformInitialize (

IN  UINTN                     MpId
)

+{

if (!ArmPlatformIsPrimaryCore (MpId)) {
return RETURN_SUCCESS;
}

//TODO: Add basic platfrom initialization

return RETURN_SUCCESS;

+}



+/**

Initialize the system (or sometimes called permanent) memory

This memory is generally represented by the DRAM.


+**/
+VOID
+ArmPlatformInitializeSystemMemory (

VOID
)

+{

//TODO: Initialize DRAM controller here

+}



+EFI_STATUS
+PrePeiCoreGetMpCoreInfo (

OUT UINTN                   *CoreCount,
OUT ARM_CORE_INFO           **ArmCoreTable
)

+{

if (ArmIsMpCore()) {
*CoreCount    = sizeof(mArmada7040MpCoreInfoTable) / sizeof(ARM_CORE_INFO);
*ArmCoreTable = mArmada7040MpCoreInfoTable;
return EFI_SUCCESS;
} else {
return EFI_UNSUPPORTED;
}

+}



+ARM_MP_CORE_INFO_PPI mMpCoreInfoPpi = { PrePeiCoreGetMpCoreInfo };



+EFI_PEI_PPI_DESCRIPTOR      gPlatformPpiTable[] = {

{
EFI_PEI_PPI_DESCRIPTOR_PPI,
&gArmMpCoreInfoPpiGuid,
&mMpCoreInfoPpi
}

+};



+VOID
+ArmPlatformGetPlatformPpiList (

OUT UINTN                   *PpiListSize,
OUT EFI_PEI_PPI_DESCRIPTOR  **PpiList
)

+{

if (ArmIsMpCore()) {
*PpiListSize = sizeof(gPlatformPpiTable);
*PpiList = gPlatformPpiTable;
} else {
*PpiListSize = 0;
*PpiList = NULL;
}

+}




diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
new file mode 100644
index 0000000..2e2120e
--- /dev/null
+++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
@@ -0,0 +1,67 @@
+# Copyright (C) 2016 Marvell International Ltd.
+#
+# Marvell BSD License Option
+#
+# If you received this File from Marvell, you may opt to use, redistribute
+# and/or modify this File under the following licensing terms.
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are met:
+#
+# * Redistributions of source code must retain the above copyright notice,
+# this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Marvell nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE
+# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#



+[Defines]

INF_VERSION                    = 0x00010005
BASE_NAME                      = Armada7040Lib
FILE_GUID                      = 3f29b642-4a49-4dfd-8f4a-205dd38432bb
MODULE_TYPE                    = BASE
VERSION_STRING                 = 1.0
LIBRARY_CLASS                  = ArmPlatformLib


+[Packages]

MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
ArmPkg/ArmPkg.dec
ArmPlatformPkg/ArmPlatformPkg.dec


+[LibraryClasses]

ArmLib
DebugLib
MemoryAllocationLib


+[Sources.common]

Armada70x0Lib.c
Armada70x0LibMem.c


+[Sources.AArch64]

AArch64/ArmPlatformHelper.S


+[FixedPcd]

gArmTokenSpaceGuid.PcdSystemMemoryBase
gArmTokenSpaceGuid.PcdSystemMemorySize

gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
gArmTokenSpaceGuid.PcdArmPrimaryCore


+[Ppis]

gArmMpCoreInfoPpiGuid

diff --git a/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
new file mode 100644
index 0000000..74c9956
--- /dev/null
+++ b/Platforms/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
@@ -0,0 +1,93 @@
+/*******************************************************************************
+Copyright (C) 2016 Marvell International Ltd.



+Marvell BSD License Option



+If you received this File from Marvell, you may opt to use, redistribute and/or
+modify this File under the following licensing terms.
+Redistribution and use in source and binary forms, with or without modification,
+are permitted provided that the following conditions are met:



+* Redistributions of source code must retain the above copyright notice,

this list of conditions and the following disclaimer.


+* Redistributions in binary form must reproduce the above copyright

notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.


+* Neither the name of Marvell nor the names of its contributors may be

used to endorse or promote products derived from this software without
specific prior written permission.


+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.



+*******************************************************************************/



+#include <Base.h>
+#include <Library/ArmPlatformLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>



+// The total number of descriptors, including the final "end-of-table" descriptor.
+#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16



+// DDR attributes
+#define DDR_ATTRIBUTES_CACHED           ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_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_DESCRIPTOR  *VirtualMemoryTable;
UINTN                         Index = 0;

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;


}

// DDR
VirtualMemoryTable[Index].PhysicalBase    = PcdGet64 (PcdSystemMemoryBase);
VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdSystemMemoryBase);
VirtualMemoryTable[Index].Length          = PcdGet64 (PcdSystemMemorySize);
VirtualMemoryTable[Index].Attributes      = DDR_ATTRIBUTES_CACHED;

// Configuration space 0xF000_0000 - 0xFFFF_FFFF
VirtualMemoryTable[++Index].PhysicalBase  = 0xF0000000;
VirtualMemoryTable[Index].VirtualBase     = 0xF0000000;
VirtualMemoryTable[Index].Length          = 0x10000000;
VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

// End of Table
VirtualMemoryTable[++Index].PhysicalBase  = 0;
VirtualMemoryTable[Index].VirtualBase     = 0;
VirtualMemoryTable[Index].Length          = 0;
VirtualMemoryTable[Index].Attributes      = 0;

ASSERT((Index + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);

*VirtualMemoryMap = VirtualMemoryTable;

+}
The issues I have with this file have nothing to do with your
contribution, but rather with the code it is based on. The worst part
being the ASSERT checking whether or not
ArmPlatformGetVirtualMemoryMap writes beyond the
MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS it has allocated being sort of
pointless, since it only applies for DEBUG builds and only after the
locations have already been written to.
(But until we have a better template for how to do this, I'm happy to
leave it in.)
Beyond that, since on the 7040 platform, RAM is socketed - is
there no way to detect the amount at runtime rather than the
MemoryBase/MemorySize?
It's Marvell specific RAM socket and its characteristic cannot be
detected at runtime.
...
Regards,
Leif
Regards,
Jan

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

Re: [Linaro-uefi] [RFC 01/20] Platforms/Marvell: Add initial support for Armada70x0 SOC lib