* [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 16:47 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 2/8] Marvell/Armada: Increase preallocated memory region size Marcin Wojtas
` (6 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Add an implementation of EFI_RNG_PROTOCOL so that the OS loader has
access to entropy for KASLR and other purposes (i.e., seeding the OS's
entropy pool very early on).
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada.dsc.inc | 4 +
Platform/Marvell/Armada/Armada70x0.fdf | 1 +
Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c | 254 ++++++++++++++++++++
Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf | 47 ++++
Platform/Marvell/Marvell.dec | 3 +
5 files changed, 309 insertions(+)
diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index 1aa485c..ec24d76 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -364,6 +364,9 @@
gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
gArmTokenSpaceGuid.PcdArmScr|0x531
+ # TRNG
+ gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
+
################################################################################
#
# Components Section - list of all EDK II Modules needed by this Platform
@@ -400,6 +403,7 @@
Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
+ Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
# Network support
MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
index 933c3ed..a94a9ff 100644
--- a/Platform/Marvell/Armada/Armada70x0.fdf
+++ b/Platform/Marvell/Armada/Armada70x0.fdf
@@ -113,6 +113,7 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
INF Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
INF Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
INF Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
+ INF Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
# Network support
INF MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
new file mode 100644
index 0000000..dca6dcc
--- /dev/null
+++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
@@ -0,0 +1,254 @@
+/** @file
+
+ This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
+
+ Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
+
+ 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/BaseMemoryLib.h>
+#include <Library/IoLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/Rng.h>
+
+#define TRNG_OUTPUT_REG mTrngBaseAddress
+#define TRNG_OUTPUT_SIZE 0x10
+
+#define TRNG_STATUS_REG (mTrngBaseAddress + 0x10)
+#define TRNG_STATUS_READY BIT0
+
+#define TRNG_INTACK_REG (mTrngBaseAddress + 0x10)
+#define TRNG_INTACK_READY BIT0
+
+#define TRNG_CONTROL_REG (mTrngBaseAddress + 0x14)
+#define TRNG_CONTROL_REG_ENABLE BIT10
+
+#define TRNG_CONFIG_REG (mTrngBaseAddress + 0x18)
+#define __MIN_REFILL_SHIFT 0
+#define __MAX_REFILL_SHIFT 16
+#define TRNG_CONFIG_MIN_REFILL_CYCLES (0x05 << __MIN_REFILL_SHIFT)
+#define TRNG_CONFIG_MAX_REFILL_CYCLES (0x22 << __MAX_REFILL_SHIFT)
+
+#define TRNG_FRODETUNE_REG (mTrngBaseAddress + 0x24)
+#define TRNG_FRODETUNE_MASK 0x0
+
+#define TRNG_FROENABLE_REG (mTrngBaseAddress + 0x20)
+#define TRNG_FROENABLE_MASK 0xffffff
+
+#define TRNG_MAX_RETRIES 20
+
+STATIC EFI_PHYSICAL_ADDRESS mTrngBaseAddress;
+
+/**
+ Returns information about the random number generation implementation.
+
+ @param[in] This A pointer to the EFI_RNG_PROTOCOL
+ instance.
+ @param[in,out] RNGAlgorithmListSize On input, the size in bytes of
+ RNGAlgorithmList.
+ On output with a return code of
+ EFI_SUCCESS, the size in bytes of the
+ data returned in RNGAlgorithmList. On
+ output with a return code of
+ EFI_BUFFER_TOO_SMALL, the size of
+ RNGAlgorithmList required to obtain the
+ list.
+ @param[out] RNGAlgorithmList A caller-allocated memory buffer filled
+ by the driver with one EFI_RNG_ALGORITHM
+ element for each supported RNG algorithm.
+ The list must not change across multiple
+ calls to the same driver. The first
+ algorithm in the list is the default
+ algorithm for the driver.
+
+ @retval EFI_SUCCESS The RNG algorithm list was returned
+ successfully.
+ @retval EFI_UNSUPPORTED The services is not supported by this
+ driver.
+ @retval EFI_DEVICE_ERROR The list of algorithms could not be
+ retrieved due to a hardware or firmware
+ error.
+ @retval EFI_INVALID_PARAMETER One or more of the parameters are
+ incorrect.
+ @retval EFI_BUFFER_TOO_SMALL The buffer RNGAlgorithmList is too small
+ to hold the result.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+Armada70x0RngGetInfo (
+ IN EFI_RNG_PROTOCOL *This,
+ IN OUT UINTN *RNGAlgorithmListSize,
+ OUT EFI_RNG_ALGORITHM *RNGAlgorithmList
+ )
+{
+ if (This == NULL || RNGAlgorithmListSize == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
+ *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ if (RNGAlgorithmList == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+ CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+GetTrngData (
+ IN UINTN Length,
+ OUT UINT8 *Bits
+ )
+{
+ UINTN Tries;
+ UINT32 Buf[TRNG_OUTPUT_SIZE / sizeof (UINT32)];
+ UINTN Index;
+
+ for (Tries = 0; Tries < TRNG_MAX_RETRIES; Tries++) {
+ if (MmioRead32 (TRNG_STATUS_REG) & TRNG_STATUS_READY) {
+ for (Index = 0; Index < ARRAY_SIZE (Buf); Index++) {
+ Buf[Index] = MmioRead32 (TRNG_OUTPUT_REG + Index * sizeof (UINT32));
+ }
+ CopyMem (Bits, Buf, Length);
+ MmioWrite32 (TRNG_INTACK_REG, TRNG_INTACK_READY);
+
+ return EFI_SUCCESS;
+ }
+ gBS->Stall (10);
+ }
+ return EFI_DEVICE_ERROR;
+}
+
+/**
+ Produces and returns an RNG value using either the default or specified RNG
+ algorithm.
+
+ @param[in] This A pointer to the EFI_RNG_PROTOCOL
+ instance.
+ @param[in] RNGAlgorithm A pointer to the EFI_RNG_ALGORITHM that
+ identifies the RNG algorithm to use. May
+ be NULL in which case the function will
+ use its default RNG algorithm.
+ @param[in] RNGValueLength The length in bytes of the memory buffer
+ pointed to by RNGValue. The driver shall
+ return exactly this numbers of bytes.
+ @param[out] RNGValue A caller-allocated memory buffer filled
+ by the driver with the resulting RNG
+ value.
+
+ @retval EFI_SUCCESS The RNG value was returned successfully.
+ @retval EFI_UNSUPPORTED The algorithm specified by RNGAlgorithm
+ is not supported by this driver.
+ @retval EFI_DEVICE_ERROR An RNG value could not be retrieved due
+ to a hardware or firmware error.
+ @retval EFI_NOT_READY There is not enough random data available
+ to satisfy the length requested by
+ RNGValueLength.
+ @retval EFI_INVALID_PARAMETER RNGValue is NULL or RNGValueLength is
+ zero.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+Armada70x0RngGetRNG (
+ IN EFI_RNG_PROTOCOL *This,
+ IN EFI_RNG_ALGORITHM *RNGAlgorithm, OPTIONAL
+ IN UINTN RNGValueLength,
+ OUT UINT8 *RNGValue
+ )
+{
+ UINTN Length;
+ EFI_STATUS Status;
+
+ if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // We only support the raw algorithm, so reject requests for anything else
+ //
+ if (RNGAlgorithm != NULL &&
+ !CompareGuid (RNGAlgorithm, &gEfiRngAlgorithmRaw)) {
+ return EFI_UNSUPPORTED;
+ }
+
+ do {
+ Length = MIN (RNGValueLength, TRNG_OUTPUT_SIZE);
+ Status = GetTrngData (Length, RNGValue);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ RNGValue += Length;
+ RNGValueLength -= Length;
+ } while (RNGValueLength > 0);
+
+ return EFI_SUCCESS;
+}
+
+STATIC EFI_RNG_PROTOCOL mArmada70x0RngProtocol = {
+ Armada70x0RngGetInfo,
+ Armada70x0RngGetRNG
+};
+
+//
+// Entry point of this driver.
+//
+EFI_STATUS
+EFIAPI
+Armada70x0RngDxeEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ mTrngBaseAddress = PcdGet64 (PcdEip76TrngBaseAddress);
+
+ //
+ // Disable the TRNG before updating its configuration
+ //
+ MmioAnd32 (TRNG_CONTROL_REG, ~TRNG_CONTROL_REG_ENABLE);
+
+ //
+ // Configure the internal conditioning parameters of the TRNG
+ //
+ MmioWrite32 (TRNG_CONFIG_REG, TRNG_CONFIG_MIN_REFILL_CYCLES |
+ TRNG_CONFIG_MAX_REFILL_CYCLES);
+
+ //
+ // Configure the FROs
+ //
+ MmioWrite32 (TRNG_FRODETUNE_REG, TRNG_FRODETUNE_MASK);
+ MmioWrite32 (TRNG_FROENABLE_REG, TRNG_FROENABLE_MASK);
+
+ //
+ // Enable the TRNG
+ //
+ MmioOr32 (TRNG_CONTROL_REG, TRNG_CONTROL_REG_ENABLE);
+
+ return SystemTable->BootServices->InstallMultipleProtocolInterfaces (
+ &ImageHandle,
+ &gEfiRngProtocolGuid,
+ &mArmada70x0RngProtocol,
+ NULL
+ );
+}
diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
new file mode 100644
index 0000000..189ffc5
--- /dev/null
+++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
@@ -0,0 +1,47 @@
+## @file
+# This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
+#
+# Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
+#
+# 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 = 0x00010019
+ BASE_NAME = Armada70x0RngDxe
+ FILE_GUID = dd87096a-cae5-4328-bec1-2ddb755f2e08
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = Armada70x0RngDxeEntryPoint
+
+[Sources]
+ Armada70x0RngDxe.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ Platform/Marvell/Marvell.dec
+
+[LibraryClasses]
+ BaseMemoryLib
+ IoLib
+ PcdLib
+ UefiDriverEntryPoint
+
+[Pcd]
+ gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress
+
+[Protocols]
+ gEfiRngProtocolGuid ## PRODUCES
+
+[Guids]
+ gEfiRngAlgorithmRaw
+
+[Depex]
+ TRUE
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index e7d7c2c..78f5e53 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -195,6 +195,9 @@
#RTC
gMarvellTokenSpaceGuid.PcdRtcEnabled|{ 0x0 }|VOID*|0x40000052
+#TRNG
+ gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
+
[Protocols]
gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG
2017-10-11 15:40 ` [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG Marcin Wojtas
@ 2017-10-11 16:47 ` Leif Lindholm
2017-10-11 18:15 ` Ard Biesheuvel
0 siblings, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 16:47 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Wed, Oct 11, 2017 at 05:40:42PM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Add an implementation of EFI_RNG_PROTOCOL so that the OS loader has
> access to entropy for KASLR and other purposes (i.e., seeding the OS's
> entropy pool very early on).
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Armada/Armada.dsc.inc | 4 +
> Platform/Marvell/Armada/Armada70x0.fdf | 1 +
> Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c | 254 ++++++++++++++++++++
> Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf | 47 ++++
> Platform/Marvell/Marvell.dec | 3 +
> 5 files changed, 309 insertions(+)
>
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index 1aa485c..ec24d76 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -364,6 +364,9 @@
> gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
> gArmTokenSpaceGuid.PcdArmScr|0x531
>
> + # TRNG
> + gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> +
> ################################################################################
> #
> # Components Section - list of all EDK II Modules needed by this Platform
> @@ -400,6 +403,7 @@
> Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
> Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
> + Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>
> # Network support
> MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
> index 933c3ed..a94a9ff 100644
> --- a/Platform/Marvell/Armada/Armada70x0.fdf
> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
> @@ -113,6 +113,7 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
> INF Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
> INF Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
> INF Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
> + INF Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>
> # Network support
> INF MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
> diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
> new file mode 100644
> index 0000000..dca6dcc
> --- /dev/null
> +++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
> @@ -0,0 +1,254 @@
> +/** @file
> +
> + This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
> +
> + Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
> +
> + 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/BaseMemoryLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include <Protocol/Rng.h>
> +
> +#define TRNG_OUTPUT_REG mTrngBaseAddress
> +#define TRNG_OUTPUT_SIZE 0x10
> +
> +#define TRNG_STATUS_REG (mTrngBaseAddress + 0x10)
> +#define TRNG_STATUS_READY BIT0
> +
> +#define TRNG_INTACK_REG (mTrngBaseAddress + 0x10)
> +#define TRNG_INTACK_READY BIT0
> +
> +#define TRNG_CONTROL_REG (mTrngBaseAddress + 0x14)
> +#define TRNG_CONTROL_REG_ENABLE BIT10
> +
> +#define TRNG_CONFIG_REG (mTrngBaseAddress + 0x18)
> +#define __MIN_REFILL_SHIFT 0
> +#define __MAX_REFILL_SHIFT 16
> +#define TRNG_CONFIG_MIN_REFILL_CYCLES (0x05 << __MIN_REFILL_SHIFT)
> +#define TRNG_CONFIG_MAX_REFILL_CYCLES (0x22 << __MAX_REFILL_SHIFT)
> +
> +#define TRNG_FRODETUNE_REG (mTrngBaseAddress + 0x24)
> +#define TRNG_FRODETUNE_MASK 0x0
> +
> +#define TRNG_FROENABLE_REG (mTrngBaseAddress + 0x20)
> +#define TRNG_FROENABLE_MASK 0xffffff
> +
> +#define TRNG_MAX_RETRIES 20
> +
> +STATIC EFI_PHYSICAL_ADDRESS mTrngBaseAddress;
> +
> +/**
> + Returns information about the random number generation implementation.
> +
> + @param[in] This A pointer to the EFI_RNG_PROTOCOL
> + instance.
> + @param[in,out] RNGAlgorithmListSize On input, the size in bytes of
> + RNGAlgorithmList.
> + On output with a return code of
> + EFI_SUCCESS, the size in bytes of the
> + data returned in RNGAlgorithmList. On
> + output with a return code of
> + EFI_BUFFER_TOO_SMALL, the size of
> + RNGAlgorithmList required to obtain the
> + list.
> + @param[out] RNGAlgorithmList A caller-allocated memory buffer filled
> + by the driver with one EFI_RNG_ALGORITHM
> + element for each supported RNG algorithm.
> + The list must not change across multiple
> + calls to the same driver. The first
> + algorithm in the list is the default
> + algorithm for the driver.
> +
> + @retval EFI_SUCCESS The RNG algorithm list was returned
> + successfully.
> + @retval EFI_UNSUPPORTED The services is not supported by this
> + driver.
> + @retval EFI_DEVICE_ERROR The list of algorithms could not be
> + retrieved due to a hardware or firmware
> + error.
> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
> + incorrect.
> + @retval EFI_BUFFER_TOO_SMALL The buffer RNGAlgorithmList is too small
> + to hold the result.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Armada70x0RngGetInfo (
> + IN EFI_RNG_PROTOCOL *This,
> + IN OUT UINTN *RNGAlgorithmListSize,
> + OUT EFI_RNG_ALGORITHM *RNGAlgorithmList
> + )
> +{
> + if (This == NULL || RNGAlgorithmListSize == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
> + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
> + return EFI_BUFFER_TOO_SMALL;
> + }
> +
> + if (RNGAlgorithmList == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
> + CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +GetTrngData (
> + IN UINTN Length,
> + OUT UINT8 *Bits
> + )
> +{
> + UINTN Tries;
> + UINT32 Buf[TRNG_OUTPUT_SIZE / sizeof (UINT32)];
> + UINTN Index;
> +
> + for (Tries = 0; Tries < TRNG_MAX_RETRIES; Tries++) {
> + if (MmioRead32 (TRNG_STATUS_REG) & TRNG_STATUS_READY) {
> + for (Index = 0; Index < ARRAY_SIZE (Buf); Index++) {
> + Buf[Index] = MmioRead32 (TRNG_OUTPUT_REG + Index * sizeof (UINT32));
> + }
> + CopyMem (Bits, Buf, Length);
> + MmioWrite32 (TRNG_INTACK_REG, TRNG_INTACK_READY);
> +
> + return EFI_SUCCESS;
> + }
> + gBS->Stall (10);
Why? Why 10? Please add a comment.
No other comments on this patch.
/
Leif
> + }
> + return EFI_DEVICE_ERROR;
> +}
> +
> +/**
> + Produces and returns an RNG value using either the default or specified RNG
> + algorithm.
> +
> + @param[in] This A pointer to the EFI_RNG_PROTOCOL
> + instance.
> + @param[in] RNGAlgorithm A pointer to the EFI_RNG_ALGORITHM that
> + identifies the RNG algorithm to use. May
> + be NULL in which case the function will
> + use its default RNG algorithm.
> + @param[in] RNGValueLength The length in bytes of the memory buffer
> + pointed to by RNGValue. The driver shall
> + return exactly this numbers of bytes.
> + @param[out] RNGValue A caller-allocated memory buffer filled
> + by the driver with the resulting RNG
> + value.
> +
> + @retval EFI_SUCCESS The RNG value was returned successfully.
> + @retval EFI_UNSUPPORTED The algorithm specified by RNGAlgorithm
> + is not supported by this driver.
> + @retval EFI_DEVICE_ERROR An RNG value could not be retrieved due
> + to a hardware or firmware error.
> + @retval EFI_NOT_READY There is not enough random data available
> + to satisfy the length requested by
> + RNGValueLength.
> + @retval EFI_INVALID_PARAMETER RNGValue is NULL or RNGValueLength is
> + zero.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Armada70x0RngGetRNG (
> + IN EFI_RNG_PROTOCOL *This,
> + IN EFI_RNG_ALGORITHM *RNGAlgorithm, OPTIONAL
> + IN UINTN RNGValueLength,
> + OUT UINT8 *RNGValue
> + )
> +{
> + UINTN Length;
> + EFI_STATUS Status;
> +
> + if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // We only support the raw algorithm, so reject requests for anything else
> + //
> + if (RNGAlgorithm != NULL &&
> + !CompareGuid (RNGAlgorithm, &gEfiRngAlgorithmRaw)) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + do {
> + Length = MIN (RNGValueLength, TRNG_OUTPUT_SIZE);
> + Status = GetTrngData (Length, RNGValue);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + RNGValue += Length;
> + RNGValueLength -= Length;
> + } while (RNGValueLength > 0);
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC EFI_RNG_PROTOCOL mArmada70x0RngProtocol = {
> + Armada70x0RngGetInfo,
> + Armada70x0RngGetRNG
> +};
> +
> +//
> +// Entry point of this driver.
> +//
> +EFI_STATUS
> +EFIAPI
> +Armada70x0RngDxeEntryPoint (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + mTrngBaseAddress = PcdGet64 (PcdEip76TrngBaseAddress);
> +
> + //
> + // Disable the TRNG before updating its configuration
> + //
> + MmioAnd32 (TRNG_CONTROL_REG, ~TRNG_CONTROL_REG_ENABLE);
> +
> + //
> + // Configure the internal conditioning parameters of the TRNG
> + //
> + MmioWrite32 (TRNG_CONFIG_REG, TRNG_CONFIG_MIN_REFILL_CYCLES |
> + TRNG_CONFIG_MAX_REFILL_CYCLES);
> +
> + //
> + // Configure the FROs
> + //
> + MmioWrite32 (TRNG_FRODETUNE_REG, TRNG_FRODETUNE_MASK);
> + MmioWrite32 (TRNG_FROENABLE_REG, TRNG_FROENABLE_MASK);
> +
> + //
> + // Enable the TRNG
> + //
> + MmioOr32 (TRNG_CONTROL_REG, TRNG_CONTROL_REG_ENABLE);
> +
> + return SystemTable->BootServices->InstallMultipleProtocolInterfaces (
> + &ImageHandle,
> + &gEfiRngProtocolGuid,
> + &mArmada70x0RngProtocol,
> + NULL
> + );
> +}
> diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
> new file mode 100644
> index 0000000..189ffc5
> --- /dev/null
> +++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
> @@ -0,0 +1,47 @@
> +## @file
> +# This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
> +#
> +# Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
> +#
> +# 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 = 0x00010019
> + BASE_NAME = Armada70x0RngDxe
> + FILE_GUID = dd87096a-cae5-4328-bec1-2ddb755f2e08
> + MODULE_TYPE = DXE_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = Armada70x0RngDxeEntryPoint
> +
> +[Sources]
> + Armada70x0RngDxe.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + Platform/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> + BaseMemoryLib
> + IoLib
> + PcdLib
> + UefiDriverEntryPoint
> +
> +[Pcd]
> + gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress
> +
> +[Protocols]
> + gEfiRngProtocolGuid ## PRODUCES
> +
> +[Guids]
> + gEfiRngAlgorithmRaw
> +
> +[Depex]
> + TRUE
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index e7d7c2c..78f5e53 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -195,6 +195,9 @@
> #RTC
> gMarvellTokenSpaceGuid.PcdRtcEnabled|{ 0x0 }|VOID*|0x40000052
>
> +#TRNG
> + gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
> +
> [Protocols]
> gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
> gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG
2017-10-11 16:47 ` Leif Lindholm
@ 2017-10-11 18:15 ` Ard Biesheuvel
2017-10-12 4:39 ` Marcin Wojtas
0 siblings, 1 reply; 27+ messages in thread
From: Ard Biesheuvel @ 2017-10-11 18:15 UTC (permalink / raw)
To: Leif Lindholm
Cc: Marcin Wojtas, edk2-devel@lists.01.org, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
On 11 October 2017 at 17:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Oct 11, 2017 at 05:40:42PM +0200, Marcin Wojtas wrote:
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Add an implementation of EFI_RNG_PROTOCOL so that the OS loader has
>> access to entropy for KASLR and other purposes (i.e., seeding the OS's
>> entropy pool very early on).
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Armada/Armada.dsc.inc | 4 +
>> Platform/Marvell/Armada/Armada70x0.fdf | 1 +
>> Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c | 254 ++++++++++++++++++++
>> Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf | 47 ++++
>> Platform/Marvell/Marvell.dec | 3 +
>> 5 files changed, 309 insertions(+)
>>
>> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
>> index 1aa485c..ec24d76 100644
>> --- a/Platform/Marvell/Armada/Armada.dsc.inc
>> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
>> @@ -364,6 +364,9 @@
>> gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
>> gArmTokenSpaceGuid.PcdArmScr|0x531
>>
>> + # TRNG
>> + gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
>> +
>> ################################################################################
>> #
>> # Components Section - list of all EDK II Modules needed by this Platform
>> @@ -400,6 +403,7 @@
>> Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
>> Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
>> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
>> + Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>>
>> # Network support
>> MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
>> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
>> index 933c3ed..a94a9ff 100644
>> --- a/Platform/Marvell/Armada/Armada70x0.fdf
>> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
>> @@ -113,6 +113,7 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>> INF Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
>> INF Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
>> INF Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
>> + INF Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>>
>> # Network support
>> INF MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
>> diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
>> new file mode 100644
>> index 0000000..dca6dcc
>> --- /dev/null
>> +++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
>> @@ -0,0 +1,254 @@
>> +/** @file
>> +
>> + This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
>> +
>> + Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
>> +
>> + 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/BaseMemoryLib.h>
>> +#include <Library/IoLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#include <Protocol/Rng.h>
>> +
>> +#define TRNG_OUTPUT_REG mTrngBaseAddress
>> +#define TRNG_OUTPUT_SIZE 0x10
>> +
>> +#define TRNG_STATUS_REG (mTrngBaseAddress + 0x10)
>> +#define TRNG_STATUS_READY BIT0
>> +
>> +#define TRNG_INTACK_REG (mTrngBaseAddress + 0x10)
>> +#define TRNG_INTACK_READY BIT0
>> +
>> +#define TRNG_CONTROL_REG (mTrngBaseAddress + 0x14)
>> +#define TRNG_CONTROL_REG_ENABLE BIT10
>> +
>> +#define TRNG_CONFIG_REG (mTrngBaseAddress + 0x18)
>> +#define __MIN_REFILL_SHIFT 0
>> +#define __MAX_REFILL_SHIFT 16
>> +#define TRNG_CONFIG_MIN_REFILL_CYCLES (0x05 << __MIN_REFILL_SHIFT)
>> +#define TRNG_CONFIG_MAX_REFILL_CYCLES (0x22 << __MAX_REFILL_SHIFT)
>> +
>> +#define TRNG_FRODETUNE_REG (mTrngBaseAddress + 0x24)
>> +#define TRNG_FRODETUNE_MASK 0x0
>> +
>> +#define TRNG_FROENABLE_REG (mTrngBaseAddress + 0x20)
>> +#define TRNG_FROENABLE_MASK 0xffffff
>> +
>> +#define TRNG_MAX_RETRIES 20
>> +
>> +STATIC EFI_PHYSICAL_ADDRESS mTrngBaseAddress;
>> +
>> +/**
>> + Returns information about the random number generation implementation.
>> +
>> + @param[in] This A pointer to the EFI_RNG_PROTOCOL
>> + instance.
>> + @param[in,out] RNGAlgorithmListSize On input, the size in bytes of
>> + RNGAlgorithmList.
>> + On output with a return code of
>> + EFI_SUCCESS, the size in bytes of the
>> + data returned in RNGAlgorithmList. On
>> + output with a return code of
>> + EFI_BUFFER_TOO_SMALL, the size of
>> + RNGAlgorithmList required to obtain the
>> + list.
>> + @param[out] RNGAlgorithmList A caller-allocated memory buffer filled
>> + by the driver with one EFI_RNG_ALGORITHM
>> + element for each supported RNG algorithm.
>> + The list must not change across multiple
>> + calls to the same driver. The first
>> + algorithm in the list is the default
>> + algorithm for the driver.
>> +
>> + @retval EFI_SUCCESS The RNG algorithm list was returned
>> + successfully.
>> + @retval EFI_UNSUPPORTED The services is not supported by this
>> + driver.
>> + @retval EFI_DEVICE_ERROR The list of algorithms could not be
>> + retrieved due to a hardware or firmware
>> + error.
>> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
>> + incorrect.
>> + @retval EFI_BUFFER_TOO_SMALL The buffer RNGAlgorithmList is too small
>> + to hold the result.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +Armada70x0RngGetInfo (
>> + IN EFI_RNG_PROTOCOL *This,
>> + IN OUT UINTN *RNGAlgorithmListSize,
>> + OUT EFI_RNG_ALGORITHM *RNGAlgorithmList
>> + )
>> +{
>> + if (This == NULL || RNGAlgorithmListSize == NULL) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
>> + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
>> + return EFI_BUFFER_TOO_SMALL;
>> + }
>> +
>> + if (RNGAlgorithmList == NULL) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
>> + CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
>> +
>> + return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +GetTrngData (
>> + IN UINTN Length,
>> + OUT UINT8 *Bits
>> + )
>> +{
>> + UINTN Tries;
>> + UINT32 Buf[TRNG_OUTPUT_SIZE / sizeof (UINT32)];
>> + UINTN Index;
>> +
>> + for (Tries = 0; Tries < TRNG_MAX_RETRIES; Tries++) {
>> + if (MmioRead32 (TRNG_STATUS_REG) & TRNG_STATUS_READY) {
>> + for (Index = 0; Index < ARRAY_SIZE (Buf); Index++) {
>> + Buf[Index] = MmioRead32 (TRNG_OUTPUT_REG + Index * sizeof (UINT32));
>> + }
>> + CopyMem (Bits, Buf, Length);
>> + MmioWrite32 (TRNG_INTACK_REG, TRNG_INTACK_READY);
>> +
>> + return EFI_SUCCESS;
>> + }
>> + gBS->Stall (10);
>
> Why? Why 10? Please add a comment.
>
> No other comments on this patch.
>
I don't remember the details, and I think the 1 ms delay was chosen
arbitrarily, but the purpose of the delay is to allow the hardware to
assume the 'ready' state.
>> + }
>> + return EFI_DEVICE_ERROR;
>> +}
>> +
>> +/**
>> + Produces and returns an RNG value using either the default or specified RNG
>> + algorithm.
>> +
>> + @param[in] This A pointer to the EFI_RNG_PROTOCOL
>> + instance.
>> + @param[in] RNGAlgorithm A pointer to the EFI_RNG_ALGORITHM that
>> + identifies the RNG algorithm to use. May
>> + be NULL in which case the function will
>> + use its default RNG algorithm.
>> + @param[in] RNGValueLength The length in bytes of the memory buffer
>> + pointed to by RNGValue. The driver shall
>> + return exactly this numbers of bytes.
>> + @param[out] RNGValue A caller-allocated memory buffer filled
>> + by the driver with the resulting RNG
>> + value.
>> +
>> + @retval EFI_SUCCESS The RNG value was returned successfully.
>> + @retval EFI_UNSUPPORTED The algorithm specified by RNGAlgorithm
>> + is not supported by this driver.
>> + @retval EFI_DEVICE_ERROR An RNG value could not be retrieved due
>> + to a hardware or firmware error.
>> + @retval EFI_NOT_READY There is not enough random data available
>> + to satisfy the length requested by
>> + RNGValueLength.
>> + @retval EFI_INVALID_PARAMETER RNGValue is NULL or RNGValueLength is
>> + zero.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +Armada70x0RngGetRNG (
>> + IN EFI_RNG_PROTOCOL *This,
>> + IN EFI_RNG_ALGORITHM *RNGAlgorithm, OPTIONAL
>> + IN UINTN RNGValueLength,
>> + OUT UINT8 *RNGValue
>> + )
>> +{
>> + UINTN Length;
>> + EFI_STATUS Status;
>> +
>> + if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + //
>> + // We only support the raw algorithm, so reject requests for anything else
>> + //
>> + if (RNGAlgorithm != NULL &&
>> + !CompareGuid (RNGAlgorithm, &gEfiRngAlgorithmRaw)) {
>> + return EFI_UNSUPPORTED;
>> + }
>> +
>> + do {
>> + Length = MIN (RNGValueLength, TRNG_OUTPUT_SIZE);
>> + Status = GetTrngData (Length, RNGValue);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> +
>> + RNGValue += Length;
>> + RNGValueLength -= Length;
>> + } while (RNGValueLength > 0);
>> +
>> + return EFI_SUCCESS;
>> +}
>> +
>> +STATIC EFI_RNG_PROTOCOL mArmada70x0RngProtocol = {
>> + Armada70x0RngGetInfo,
>> + Armada70x0RngGetRNG
>> +};
>> +
>> +//
>> +// Entry point of this driver.
>> +//
>> +EFI_STATUS
>> +EFIAPI
>> +Armada70x0RngDxeEntryPoint (
>> + IN EFI_HANDLE ImageHandle,
>> + IN EFI_SYSTEM_TABLE *SystemTable
>> + )
>> +{
>> + mTrngBaseAddress = PcdGet64 (PcdEip76TrngBaseAddress);
>> +
>> + //
>> + // Disable the TRNG before updating its configuration
>> + //
>> + MmioAnd32 (TRNG_CONTROL_REG, ~TRNG_CONTROL_REG_ENABLE);
>> +
>> + //
>> + // Configure the internal conditioning parameters of the TRNG
>> + //
>> + MmioWrite32 (TRNG_CONFIG_REG, TRNG_CONFIG_MIN_REFILL_CYCLES |
>> + TRNG_CONFIG_MAX_REFILL_CYCLES);
>> +
>> + //
>> + // Configure the FROs
>> + //
>> + MmioWrite32 (TRNG_FRODETUNE_REG, TRNG_FRODETUNE_MASK);
>> + MmioWrite32 (TRNG_FROENABLE_REG, TRNG_FROENABLE_MASK);
>> +
>> + //
>> + // Enable the TRNG
>> + //
>> + MmioOr32 (TRNG_CONTROL_REG, TRNG_CONTROL_REG_ENABLE);
>> +
>> + return SystemTable->BootServices->InstallMultipleProtocolInterfaces (
>> + &ImageHandle,
>> + &gEfiRngProtocolGuid,
>> + &mArmada70x0RngProtocol,
>> + NULL
>> + );
>> +}
>> diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>> new file mode 100644
>> index 0000000..189ffc5
>> --- /dev/null
>> +++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>> @@ -0,0 +1,47 @@
>> +## @file
>> +# This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
>> +#
>> +# Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
>> +#
>> +# 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 = 0x00010019
>> + BASE_NAME = Armada70x0RngDxe
>> + FILE_GUID = dd87096a-cae5-4328-bec1-2ddb755f2e08
>> + MODULE_TYPE = DXE_DRIVER
>> + VERSION_STRING = 1.0
>> + ENTRY_POINT = Armada70x0RngDxeEntryPoint
>> +
>> +[Sources]
>> + Armada70x0RngDxe.c
>> +
>> +[Packages]
>> + MdePkg/MdePkg.dec
>> + Platform/Marvell/Marvell.dec
>> +
>> +[LibraryClasses]
>> + BaseMemoryLib
>> + IoLib
>> + PcdLib
>> + UefiDriverEntryPoint
>> +
>> +[Pcd]
>> + gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress
>> +
>> +[Protocols]
>> + gEfiRngProtocolGuid ## PRODUCES
>> +
>> +[Guids]
>> + gEfiRngAlgorithmRaw
>> +
>> +[Depex]
>> + TRUE
>> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
>> index e7d7c2c..78f5e53 100644
>> --- a/Platform/Marvell/Marvell.dec
>> +++ b/Platform/Marvell/Marvell.dec
>> @@ -195,6 +195,9 @@
>> #RTC
>> gMarvellTokenSpaceGuid.PcdRtcEnabled|{ 0x0 }|VOID*|0x40000052
>>
>> +#TRNG
>> + gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
>> +
>> [Protocols]
>> gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
>> gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG
2017-10-11 18:15 ` Ard Biesheuvel
@ 2017-10-12 4:39 ` Marcin Wojtas
2017-10-12 10:24 ` Leif Lindholm
0 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-12 4:39 UTC (permalink / raw)
To: Leif Lindholm
Cc: Ard Biesheuvel, edk2-devel@lists.01.org, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
2017-10-11 20:15 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 11 October 2017 at 17:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Wed, Oct 11, 2017 at 05:40:42PM +0200, Marcin Wojtas wrote:
>>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>> Add an implementation of EFI_RNG_PROTOCOL so that the OS loader has
>>> access to entropy for KASLR and other purposes (i.e., seeding the OS's
>>> entropy pool very early on).
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>> Platform/Marvell/Armada/Armada.dsc.inc | 4 +
>>> Platform/Marvell/Armada/Armada70x0.fdf | 1 +
>>> Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c | 254 ++++++++++++++++++++
>>> Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf | 47 ++++
>>> Platform/Marvell/Marvell.dec | 3 +
>>> 5 files changed, 309 insertions(+)
>>>
>>> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
>>> index 1aa485c..ec24d76 100644
>>> --- a/Platform/Marvell/Armada/Armada.dsc.inc
>>> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
>>> @@ -364,6 +364,9 @@
>>> gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
>>> gArmTokenSpaceGuid.PcdArmScr|0x531
>>>
>>> + # TRNG
>>> + gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
>>> +
>>> ################################################################################
>>> #
>>> # Components Section - list of all EDK II Modules needed by this Platform
>>> @@ -400,6 +403,7 @@
>>> Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
>>> Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
>>> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
>>> + Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>>>
>>> # Network support
>>> MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
>>> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
>>> index 933c3ed..a94a9ff 100644
>>> --- a/Platform/Marvell/Armada/Armada70x0.fdf
>>> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
>>> @@ -113,6 +113,7 @@ FvNameGuid = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>>> INF Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
>>> INF Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
>>> INF Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
>>> + INF Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>>>
>>> # Network support
>>> INF MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
>>> diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
>>> new file mode 100644
>>> index 0000000..dca6dcc
>>> --- /dev/null
>>> +++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
>>> @@ -0,0 +1,254 @@
>>> +/** @file
>>> +
>>> + This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
>>> +
>>> + Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
>>> +
>>> + 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/BaseMemoryLib.h>
>>> +#include <Library/IoLib.h>
>>> +#include <Library/PcdLib.h>
>>> +#include <Library/UefiBootServicesTableLib.h>
>>> +
>>> +#include <Protocol/Rng.h>
>>> +
>>> +#define TRNG_OUTPUT_REG mTrngBaseAddress
>>> +#define TRNG_OUTPUT_SIZE 0x10
>>> +
>>> +#define TRNG_STATUS_REG (mTrngBaseAddress + 0x10)
>>> +#define TRNG_STATUS_READY BIT0
>>> +
>>> +#define TRNG_INTACK_REG (mTrngBaseAddress + 0x10)
>>> +#define TRNG_INTACK_READY BIT0
>>> +
>>> +#define TRNG_CONTROL_REG (mTrngBaseAddress + 0x14)
>>> +#define TRNG_CONTROL_REG_ENABLE BIT10
>>> +
>>> +#define TRNG_CONFIG_REG (mTrngBaseAddress + 0x18)
>>> +#define __MIN_REFILL_SHIFT 0
>>> +#define __MAX_REFILL_SHIFT 16
>>> +#define TRNG_CONFIG_MIN_REFILL_CYCLES (0x05 << __MIN_REFILL_SHIFT)
>>> +#define TRNG_CONFIG_MAX_REFILL_CYCLES (0x22 << __MAX_REFILL_SHIFT)
>>> +
>>> +#define TRNG_FRODETUNE_REG (mTrngBaseAddress + 0x24)
>>> +#define TRNG_FRODETUNE_MASK 0x0
>>> +
>>> +#define TRNG_FROENABLE_REG (mTrngBaseAddress + 0x20)
>>> +#define TRNG_FROENABLE_MASK 0xffffff
>>> +
>>> +#define TRNG_MAX_RETRIES 20
>>> +
>>> +STATIC EFI_PHYSICAL_ADDRESS mTrngBaseAddress;
>>> +
>>> +/**
>>> + Returns information about the random number generation implementation.
>>> +
>>> + @param[in] This A pointer to the EFI_RNG_PROTOCOL
>>> + instance.
>>> + @param[in,out] RNGAlgorithmListSize On input, the size in bytes of
>>> + RNGAlgorithmList.
>>> + On output with a return code of
>>> + EFI_SUCCESS, the size in bytes of the
>>> + data returned in RNGAlgorithmList. On
>>> + output with a return code of
>>> + EFI_BUFFER_TOO_SMALL, the size of
>>> + RNGAlgorithmList required to obtain the
>>> + list.
>>> + @param[out] RNGAlgorithmList A caller-allocated memory buffer filled
>>> + by the driver with one EFI_RNG_ALGORITHM
>>> + element for each supported RNG algorithm.
>>> + The list must not change across multiple
>>> + calls to the same driver. The first
>>> + algorithm in the list is the default
>>> + algorithm for the driver.
>>> +
>>> + @retval EFI_SUCCESS The RNG algorithm list was returned
>>> + successfully.
>>> + @retval EFI_UNSUPPORTED The services is not supported by this
>>> + driver.
>>> + @retval EFI_DEVICE_ERROR The list of algorithms could not be
>>> + retrieved due to a hardware or firmware
>>> + error.
>>> + @retval EFI_INVALID_PARAMETER One or more of the parameters are
>>> + incorrect.
>>> + @retval EFI_BUFFER_TOO_SMALL The buffer RNGAlgorithmList is too small
>>> + to hold the result.
>>> +
>>> +**/
>>> +STATIC
>>> +EFI_STATUS
>>> +EFIAPI
>>> +Armada70x0RngGetInfo (
>>> + IN EFI_RNG_PROTOCOL *This,
>>> + IN OUT UINTN *RNGAlgorithmListSize,
>>> + OUT EFI_RNG_ALGORITHM *RNGAlgorithmList
>>> + )
>>> +{
>>> + if (This == NULL || RNGAlgorithmListSize == NULL) {
>>> + return EFI_INVALID_PARAMETER;
>>> + }
>>> +
>>> + if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
>>> + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
>>> + return EFI_BUFFER_TOO_SMALL;
>>> + }
>>> +
>>> + if (RNGAlgorithmList == NULL) {
>>> + return EFI_INVALID_PARAMETER;
>>> + }
>>> +
>>> + *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
>>> + CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
>>> +
>>> + return EFI_SUCCESS;
>>> +}
>>> +
>>> +STATIC
>>> +EFI_STATUS
>>> +GetTrngData (
>>> + IN UINTN Length,
>>> + OUT UINT8 *Bits
>>> + )
>>> +{
>>> + UINTN Tries;
>>> + UINT32 Buf[TRNG_OUTPUT_SIZE / sizeof (UINT32)];
>>> + UINTN Index;
>>> +
>>> + for (Tries = 0; Tries < TRNG_MAX_RETRIES; Tries++) {
>>> + if (MmioRead32 (TRNG_STATUS_REG) & TRNG_STATUS_READY) {
>>> + for (Index = 0; Index < ARRAY_SIZE (Buf); Index++) {
>>> + Buf[Index] = MmioRead32 (TRNG_OUTPUT_REG + Index * sizeof (UINT32));
>>> + }
>>> + CopyMem (Bits, Buf, Length);
>>> + MmioWrite32 (TRNG_INTACK_REG, TRNG_INTACK_READY);
>>> +
>>> + return EFI_SUCCESS;
>>> + }
>>> + gBS->Stall (10);
>>
>> Why? Why 10? Please add a comment.
>>
>> No other comments on this patch.
>>
>
> I don't remember the details, and I think the 1 ms delay was chosen
> arbitrarily, but the purpose of the delay is to allow the hardware to
> assume the 'ready' state.
>
I don't have EIP150 docs, but how about a comment:
// Polling interval for obtaining TRNG data is 10us
gBS->Stall (10);
I know it's not perfect, but would you accept it?:)
Best regards,
Marcin
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG
2017-10-12 4:39 ` Marcin Wojtas
@ 2017-10-12 10:24 ` Leif Lindholm
0 siblings, 0 replies; 27+ messages in thread
From: Leif Lindholm @ 2017-10-12 10:24 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Ard Biesheuvel, edk2-devel@lists.01.org, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
On Thu, Oct 12, 2017 at 06:39:46AM +0200, Marcin Wojtas wrote:
> 2017-10-11 20:15 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> > On 11 October 2017 at 17:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> On Wed, Oct 11, 2017 at 05:40:42PM +0200, Marcin Wojtas wrote:
> >>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> + }
> >>> + gBS->Stall (10);
> >>
> >> Why? Why 10? Please add a comment.
> >>
> >> No other comments on this patch.
> >>
> >
> > I don't remember the details, and I think the 1 ms delay was chosen
> > arbitrarily, but the purpose of the delay is to allow the hardware to
> > assume the 'ready' state.
> >
>
> I don't have EIP150 docs, but how about a comment:
>
> // Polling interval for obtaining TRNG data is 10us
> gBS->Stall (10);
>
> I know it's not perfect, but would you accept it?:)
Nearly. "Polling interval ... is" sounds a bit too authoritative
(unless that is actually what is documented).
"Wait for more TRNG data to arrive" would be sufficient.
/
Leif
^ permalink raw reply [flat|nested] 27+ messages in thread
* [platforms: PATCH 2/8] Marvell/Armada: Increase preallocated memory region size
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 16:56 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 3/8] Marvell/Armada: Remove custom reset library residues Marcin Wojtas
` (5 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
In order to prevent fragmentation of the UEFI memory map, increase the
sizes of the preallocated regions. Note that this does not increase the
memory footprint of UEFI, it just modifies it allocation policy to keep
similar region types together.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada.dsc.inc | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index ec24d76..56d8941 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -341,10 +341,10 @@
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0
- gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|50
- gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|20
- gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesCode|400
- gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData|20000
+ gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|1000
+ gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|1000
+ gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesCode|2000
+ gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData|35000
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode|20
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData|0
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 2/8] Marvell/Armada: Increase preallocated memory region size
2017-10-11 15:40 ` [platforms: PATCH 2/8] Marvell/Armada: Increase preallocated memory region size Marcin Wojtas
@ 2017-10-11 16:56 ` Leif Lindholm
0 siblings, 0 replies; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 16:56 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Wed, Oct 11, 2017 at 05:40:43PM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> In order to prevent fragmentation of the UEFI memory map, increase the
> sizes of the preallocated regions. Note that this does not increase the
> memory footprint of UEFI, it just modifies it allocation policy to keep
> similar region types together.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/Marvell/Armada/Armada.dsc.inc | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index ec24d76..56d8941 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -341,10 +341,10 @@
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType|0
> - gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|50
> - gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|20
> - gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesCode|400
> - gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData|20000
> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData|1000
> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode|1000
> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesCode|2000
> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData|35000
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode|20
> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData|0
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [platforms: PATCH 3/8] Marvell/Armada: Remove custom reset library residues
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 2/8] Marvell/Armada: Increase preallocated memory region size Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 16:56 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping Marcin Wojtas
` (4 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
When switching to generic PSCI reset library, obsolete parts
of previous custom reset library (PCDs, documentation) remained.
Remove them.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada70x0.dsc | 4 ----
Platform/Marvell/Marvell.dec | 4 ----
Silicon/Marvell/Documentation/PortingGuide.txt | 9 ---------
3 files changed, 17 deletions(-)
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index 430803c..946c93e 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -138,9 +138,5 @@
gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1, 0x0 }
gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1 }
- #ResetLib
- gMarvellTokenSpaceGuid.PcdResetRegAddress|0xf06f0084
- gMarvellTokenSpaceGuid.PcdResetRegMask|0x1
-
#RTC
gMarvellTokenSpaceGuid.PcdRtcEnabled|{ 0x1 }
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index 78f5e53..434d6cb 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -188,10 +188,6 @@
gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x0 }|VOID*|0x3000034
gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x3000035
-#ResetLib
- gMarvellTokenSpaceGuid.PcdResetRegAddress|0|UINT64|0x40000050
- gMarvellTokenSpaceGuid.PcdResetRegMask|0|UINT32|0x4000051
-
#RTC
gMarvellTokenSpaceGuid.PcdRtcEnabled|{ 0x0 }|VOID*|0x40000052
diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
index 66ec918..cbe3bed 100644
--- a/Silicon/Marvell/Documentation/PortingGuide.txt
+++ b/Silicon/Marvell/Documentation/PortingGuide.txt
@@ -383,15 +383,6 @@ Set pin 6 and 7 to 0xa function:
gMarvellTokenSpaceGuid.PcdChip0MppSel0|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xa, 0xa, 0x0, 0x0 }
-MarvellResetSystemLib configuration
-===================================
-This simple library allows to mask given bits in given reg at UEFI 'reset'
-command call. These variables are configurable through PCDs:
-
- - gMarvellTokenSpaceGuid.PcdResetRegAddress
- - gMarvellTokenSpaceGuid.PcdResetRegMask
-
-
Ramdisk configuration
=====================
There is one PCD available for Ramdisk configuration
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 3/8] Marvell/Armada: Remove custom reset library residues
2017-10-11 15:40 ` [platforms: PATCH 3/8] Marvell/Armada: Remove custom reset library residues Marcin Wojtas
@ 2017-10-11 16:56 ` Leif Lindholm
0 siblings, 0 replies; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 16:56 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Wed, Oct 11, 2017 at 05:40:44PM +0200, Marcin Wojtas wrote:
> When switching to generic PSCI reset library, obsolete parts
> of previous custom reset library (PCDs, documentation) remained.
> Remove them.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Yes, please.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/Marvell/Armada/Armada70x0.dsc | 4 ----
> Platform/Marvell/Marvell.dec | 4 ----
> Silicon/Marvell/Documentation/PortingGuide.txt | 9 ---------
> 3 files changed, 17 deletions(-)
>
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 430803c..946c93e 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -138,9 +138,5 @@
> gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1, 0x0 }
> gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1 }
>
> - #ResetLib
> - gMarvellTokenSpaceGuid.PcdResetRegAddress|0xf06f0084
> - gMarvellTokenSpaceGuid.PcdResetRegMask|0x1
> -
> #RTC
> gMarvellTokenSpaceGuid.PcdRtcEnabled|{ 0x1 }
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index 78f5e53..434d6cb 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -188,10 +188,6 @@
> gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x0 }|VOID*|0x3000034
> gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x0 }|VOID*|0x3000035
>
> -#ResetLib
> - gMarvellTokenSpaceGuid.PcdResetRegAddress|0|UINT64|0x40000050
> - gMarvellTokenSpaceGuid.PcdResetRegMask|0|UINT32|0x4000051
> -
> #RTC
> gMarvellTokenSpaceGuid.PcdRtcEnabled|{ 0x0 }|VOID*|0x40000052
>
> diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
> index 66ec918..cbe3bed 100644
> --- a/Silicon/Marvell/Documentation/PortingGuide.txt
> +++ b/Silicon/Marvell/Documentation/PortingGuide.txt
> @@ -383,15 +383,6 @@ Set pin 6 and 7 to 0xa function:
> gMarvellTokenSpaceGuid.PcdChip0MppSel0|{ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xa, 0xa, 0x0, 0x0 }
>
>
> -MarvellResetSystemLib configuration
> -===================================
> -This simple library allows to mask given bits in given reg at UEFI 'reset'
> -command call. These variables are configurable through PCDs:
> -
> - - gMarvellTokenSpaceGuid.PcdResetRegAddress
> - - gMarvellTokenSpaceGuid.PcdResetRegMask
> -
> -
> Ramdisk configuration
> =====================
> There is one PCD available for Ramdisk configuration
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
` (2 preceding siblings ...)
2017-10-11 15:40 ` [platforms: PATCH 3/8] Marvell/Armada: Remove custom reset library residues Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 17:08 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 5/8] Marvell/Armada: Add MemoryInitPeiLib that reserves secure region Marcin Wojtas
` (3 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
The Armada 70x0/80x0 DRAM controller allows a single window of DRAM
to be remapped to another location in the physical address space. This
allows us to free up some memory in the 32-bit addressable region for
peripheral MMIO and PCI MMIO32 and CONFIG spaces. This patch adjusts
memory blocks to the configuration done in ATF.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S | 15 +++++
Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 +
Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 60 ++++++++++++++++----
Platform/Marvell/Marvell.dec | 13 +++++
4 files changed, 81 insertions(+), 10 deletions(-)
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
index 72f8cfc..c5be1a9 100644
--- a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
@@ -17,6 +17,21 @@
ASM_FUNC(ArmPlatformPeiBootAction)
mov x29, xzr
+
+ .if FixedPcdGet64 (PcdSystemMemoryBase) != 0
+ .err PcdSystemMemoryBase should be 0x0 on this platform!
+ .endif
+
+ .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
+ //
+ // Use the low range for UEFI itself. The remaining memory will be mapped
+ // and added to the GCD map later.
+ //
+ adr x0, mSystemMemoryEnd
+ MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
+ str x1, [x0]
+ .endif
+
ret
//UINTN
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
index 2e198c3..838a670 100644
--- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
@@ -67,5 +67,8 @@
gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
gArmTokenSpaceGuid.PcdArmPrimaryCore
+ gMarvellTokenSpaceGuid.PcdDramRemapSize
+ gMarvellTokenSpaceGuid.PcdDramRemapTarget
+
[Ppis]
gArmMpCoreInfoPpiGuid
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
index 74c9956..2cb2e15 100644
--- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
@@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Base.h>
#include <Library/ArmPlatformLib.h>
#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
#include <Library/MemoryAllocationLib.h>
// The total number of descriptors, including the final "end-of-table" descriptor.
@@ -44,6 +45,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK
#define DDR_ATTRIBUTES_UNCACHED ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
+STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
+
/**
Return the Virtual Memory Map of your platform
@@ -59,20 +62,41 @@ ArmPlatformGetVirtualMemoryMap (
IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
)
{
- ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
UINTN Index = 0;
+ UINT64 MemSize;
+ UINT64 MemLowSize;
+ UINT64 MemHighStart;
+ UINT64 MemHighSize;
+ EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
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;
- }
+ MemSize = FixedPcdGet64 (PcdSystemMemorySize);
+ MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
+ MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
+ FixedPcdGet32 (PcdDramRemapSize);
+ MemHighSize = MemSize - MemLowSize;
+
+ ResourceAttributes = (
+ EFI_RESOURCE_ATTRIBUTE_PRESENT |
+ EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+ EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
+ EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
+ EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
+ EFI_RESOURCE_ATTRIBUTE_TESTED
+ );
+
+ BuildResourceDescriptorHob (
+ EFI_RESOURCE_SYSTEM_MEMORY,
+ ResourceAttributes,
+ FixedPcdGet64 (PcdSystemMemoryBase),
+ MemLowSize
+ );
// DDR
- VirtualMemoryTable[Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
- VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdSystemMemoryBase);
- VirtualMemoryTable[Index].Length = PcdGet64 (PcdSystemMemorySize);
+ VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdSystemMemoryBase);
+ VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdSystemMemoryBase);
+ VirtualMemoryTable[Index].Length = MemLowSize;
VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
// Configuration space 0xF000_0000 - 0xFFFF_FFFF
@@ -81,13 +105,29 @@ ArmPlatformGetVirtualMemoryMap (
VirtualMemoryTable[Index].Length = 0x10000000;
VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+ if (MemSize > MemLowSize) {
+ //
+ // If we have more than MemLowSize worth of DRAM, the remainder will be
+ // mapped at the top of the remapped window.
+ //
+ VirtualMemoryTable[++Index].PhysicalBase = MemHighStart;
+ VirtualMemoryTable[Index].VirtualBase = MemHighStart;
+ VirtualMemoryTable[Index].Length = MemHighSize;
+ VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
+
+ BuildResourceDescriptorHob (
+ EFI_RESOURCE_SYSTEM_MEMORY,
+ ResourceAttributes,
+ MemHighStart,
+ MemHighSize
+ );
+ }
+
// 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;
}
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index 434d6cb..db1c7fa 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -194,6 +194,19 @@
#TRNG
gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
+ #
+ # DRAM remapping controls.
+ # On the 70x0/80x0 SOCs, the DRAM is mapped at 0x0, and could be up to
+ # 16 GB in size. To allow for 32-bit addressable MMIO peripherals or PCI
+ # windows, a single window of up to 4 GB in size can be remapped elsewhere.
+ # So let's define a 1 GB window at 0xC000000 by default: this is the minimum
+ # alignment that Linux can map optimally (i.e., it's section shift is 30 bits)
+ # and gives us an additional 768 MB (on top of the 256 MB platform MMIO window
+ # at 0xF0000000) for the PCI MMIO32 and CONFIG spaces.
+ #
+ gMarvellTokenSpaceGuid.PcdDramRemapSize|0x40000000|UINT32|0x50000004
+ gMarvellTokenSpaceGuid.PcdDramRemapTarget|0xC0000000|UINT32|0x50000003
+
[Protocols]
gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping
2017-10-11 15:40 ` [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping Marcin Wojtas
@ 2017-10-11 17:08 ` Leif Lindholm
2017-10-11 18:18 ` Ard Biesheuvel
0 siblings, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 17:08 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
Subject: from->for ?
On Wed, Oct 11, 2017 at 05:40:45PM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> The Armada 70x0/80x0 DRAM controller allows a single window of DRAM
> to be remapped to another location in the physical address space. This
> allows us to free up some memory in the 32-bit addressable region for
> peripheral MMIO and PCI MMIO32 and CONFIG spaces. This patch adjusts
> memory blocks to the configuration done in ATF.
The ARM Trusted Firmware developers tend to frown at that
abbreviation. Please use ARM-TF in official context.
Which configuration is this? Presumably, if this was changed in
ARM-TF, we would need to change it this end too, so does not hurt to
be explicit. (It uses a FixedPcd, so clearly it is not dynamically
detected, which the commit message can be read as.)
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S | 15 +++++
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 +
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 60 ++++++++++++++++----
> Platform/Marvell/Marvell.dec | 13 +++++
> 4 files changed, 81 insertions(+), 10 deletions(-)
>
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> index 72f8cfc..c5be1a9 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> @@ -17,6 +17,21 @@
>
> ASM_FUNC(ArmPlatformPeiBootAction)
> mov x29, xzr
> +
> + .if FixedPcdGet64 (PcdSystemMemoryBase) != 0
> + .err PcdSystemMemoryBase should be 0x0 on this platform!
> + .endif
> +
> + .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
> + //
> + // Use the low range for UEFI itself. The remaining memory will be mapped
> + // and added to the GCD map later.
> + //
> + adr x0, mSystemMemoryEnd
> + MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> + str x1, [x0]
> + .endif
> +
> ret
>
> //UINTN
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> index 2e198c3..838a670 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> @@ -67,5 +67,8 @@
> gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
> gArmTokenSpaceGuid.PcdArmPrimaryCore
>
> + gMarvellTokenSpaceGuid.PcdDramRemapSize
> + gMarvellTokenSpaceGuid.PcdDramRemapTarget
> +
> [Ppis]
> gArmMpCoreInfoPpiGuid
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> index 74c9956..2cb2e15 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> #include <Base.h>
> #include <Library/ArmPlatformLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> #include <Library/MemoryAllocationLib.h>
>
> // The total number of descriptors, including the final "end-of-table" descriptor.
> @@ -44,6 +45,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> #define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK
> #define DDR_ATTRIBUTES_UNCACHED ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
>
> +STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
mVirtualMemoryTable?
> +
> /**
> Return the Virtual Memory Map of your platform
>
> @@ -59,20 +62,41 @@ ArmPlatformGetVirtualMemoryMap (
> IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
> )
> {
> - ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
> UINTN Index = 0;
> + UINT64 MemSize;
> + UINT64 MemLowSize;
> + UINT64 MemHighStart;
> + UINT64 MemHighSize;
> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
>
> 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;
> - }
> + MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> + MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
> + MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
> + FixedPcdGet32 (PcdDramRemapSize);
> + MemHighSize = MemSize - MemLowSize;
> +
> + ResourceAttributes = (
> + EFI_RESOURCE_ATTRIBUTE_PRESENT |
> + EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> + EFI_RESOURCE_ATTRIBUTE_TESTED
> + );
> +
> + BuildResourceDescriptorHob (
> + EFI_RESOURCE_SYSTEM_MEMORY,
> + ResourceAttributes,
> + FixedPcdGet64 (PcdSystemMemoryBase),
> + MemLowSize
> + );
>
> // DDR
> - VirtualMemoryTable[Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> - VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdSystemMemoryBase);
> - VirtualMemoryTable[Index].Length = PcdGet64 (PcdSystemMemorySize);
> + VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdSystemMemoryBase);
> + VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdSystemMemoryBase);
> + VirtualMemoryTable[Index].Length = MemLowSize;
> VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
>
> // Configuration space 0xF000_0000 - 0xFFFF_FFFF
> @@ -81,13 +105,29 @@ ArmPlatformGetVirtualMemoryMap (
> VirtualMemoryTable[Index].Length = 0x10000000;
> VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>
> + if (MemSize > MemLowSize) {
> + //
> + // If we have more than MemLowSize worth of DRAM, the remainder will be
> + // mapped at the top of the remapped window.
> + //
> + VirtualMemoryTable[++Index].PhysicalBase = MemHighStart;
> + VirtualMemoryTable[Index].VirtualBase = MemHighStart;
> + VirtualMemoryTable[Index].Length = MemHighSize;
> + VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
> +
> + BuildResourceDescriptorHob (
> + EFI_RESOURCE_SYSTEM_MEMORY,
> + ResourceAttributes,
> + MemHighStart,
> + MemHighSize
> + );
> + }
> +
> // 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);
> -
Why delete this assert?
> *VirtualMemoryMap = VirtualMemoryTable;
> }
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index 434d6cb..db1c7fa 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -194,6 +194,19 @@
> #TRNG
> gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
>
> + #
> + # DRAM remapping controls.
> + # On the 70x0/80x0 SOCs, the DRAM is mapped at 0x0, and could be up to
> + # 16 GB in size. To allow for 32-bit addressable MMIO peripherals or PCI
> + # windows, a single window of up to 4 GB in size can be remapped elsewhere.
> + # So let's define a 1 GB window at 0xC000000 by default: this is the minimum
> + # alignment that Linux can map optimally (i.e., it's section shift is 30 bits)
> + # and gives us an additional 768 MB (on top of the 256 MB platform MMIO window
> + # at 0xF0000000) for the PCI MMIO32 and CONFIG spaces.
If DRAM is mapped at 0, and can be up to 16GB in size, why do we need
to remap some of it below 4GB?
I guess what you're actually doing is mapping it _away_. Where to?
(And a short-short version of this in the commit message should take
care of that item.)
/
Leif
> + #
> + gMarvellTokenSpaceGuid.PcdDramRemapSize|0x40000000|UINT32|0x50000004
> + gMarvellTokenSpaceGuid.PcdDramRemapTarget|0xC0000000|UINT32|0x50000003
> +
> [Protocols]
> gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
> gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping
2017-10-11 17:08 ` Leif Lindholm
@ 2017-10-11 18:18 ` Ard Biesheuvel
2017-10-12 4:58 ` Marcin Wojtas
0 siblings, 1 reply; 27+ messages in thread
From: Ard Biesheuvel @ 2017-10-11 18:18 UTC (permalink / raw)
To: Leif Lindholm
Cc: Marcin Wojtas, edk2-devel@lists.01.org, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
On 11 October 2017 at 18:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Subject: from->for ?
>
> On Wed, Oct 11, 2017 at 05:40:45PM +0200, Marcin Wojtas wrote:
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> The Armada 70x0/80x0 DRAM controller allows a single window of DRAM
>> to be remapped to another location in the physical address space. This
>> allows us to free up some memory in the 32-bit addressable region for
>> peripheral MMIO and PCI MMIO32 and CONFIG spaces. This patch adjusts
>> memory blocks to the configuration done in ATF.
>
> The ARM Trusted Firmware developers tend to frown at that
> abbreviation. Please use ARM-TF in official context.
>
> Which configuration is this? Presumably, if this was changed in
> ARM-TF, we would need to change it this end too, so does not hurt to
> be explicit. (It uses a FixedPcd, so clearly it is not dynamically
> detected, which the commit message can be read as.)
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S | 15 +++++
>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 +
>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 60 ++++++++++++++++----
>> Platform/Marvell/Marvell.dec | 13 +++++
>> 4 files changed, 81 insertions(+), 10 deletions(-)
>>
>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>> index 72f8cfc..c5be1a9 100644
>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>> @@ -17,6 +17,21 @@
>>
>> ASM_FUNC(ArmPlatformPeiBootAction)
>> mov x29, xzr
>> +
>> + .if FixedPcdGet64 (PcdSystemMemoryBase) != 0
>> + .err PcdSystemMemoryBase should be 0x0 on this platform!
>> + .endif
>> +
>> + .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
>> + //
>> + // Use the low range for UEFI itself. The remaining memory will be mapped
>> + // and added to the GCD map later.
>> + //
>> + adr x0, mSystemMemoryEnd
>> + MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
>> + str x1, [x0]
>> + .endif
>> +
>> ret
>>
>> //UINTN
>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>> index 2e198c3..838a670 100644
>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>> @@ -67,5 +67,8 @@
>> gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
>> gArmTokenSpaceGuid.PcdArmPrimaryCore
>>
>> + gMarvellTokenSpaceGuid.PcdDramRemapSize
>> + gMarvellTokenSpaceGuid.PcdDramRemapTarget
>> +
>> [Ppis]
>> gArmMpCoreInfoPpiGuid
>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> index 74c9956..2cb2e15 100644
>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> #include <Base.h>
>> #include <Library/ArmPlatformLib.h>
>> #include <Library/DebugLib.h>
>> +#include <Library/HobLib.h>
>> #include <Library/MemoryAllocationLib.h>
>>
>> // The total number of descriptors, including the final "end-of-table" descriptor.
>> @@ -44,6 +45,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> #define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK
>> #define DDR_ATTRIBUTES_UNCACHED ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
>>
>> +STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>
> mVirtualMemoryTable?
>
>> +
>> /**
>> Return the Virtual Memory Map of your platform
>>
>> @@ -59,20 +62,41 @@ ArmPlatformGetVirtualMemoryMap (
>> IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
>> )
>> {
>> - ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
>> UINTN Index = 0;
>> + UINT64 MemSize;
>> + UINT64 MemLowSize;
>> + UINT64 MemHighStart;
>> + UINT64 MemHighSize;
>> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
>>
>> 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;
>> - }
>> + MemSize = FixedPcdGet64 (PcdSystemMemorySize);
>> + MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
>> + MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
>> + FixedPcdGet32 (PcdDramRemapSize);
>> + MemHighSize = MemSize - MemLowSize;
>> +
>> + ResourceAttributes = (
>> + EFI_RESOURCE_ATTRIBUTE_PRESENT |
>> + EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
>> + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>> + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>> + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
>> + EFI_RESOURCE_ATTRIBUTE_TESTED
>> + );
>> +
>> + BuildResourceDescriptorHob (
>> + EFI_RESOURCE_SYSTEM_MEMORY,
>> + ResourceAttributes,
>> + FixedPcdGet64 (PcdSystemMemoryBase),
>> + MemLowSize
>> + );
>>
>> // DDR
>> - VirtualMemoryTable[Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
>> - VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdSystemMemoryBase);
>> - VirtualMemoryTable[Index].Length = PcdGet64 (PcdSystemMemorySize);
>> + VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdSystemMemoryBase);
>> + VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdSystemMemoryBase);
>> + VirtualMemoryTable[Index].Length = MemLowSize;
>> VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
>>
>> // Configuration space 0xF000_0000 - 0xFFFF_FFFF
>> @@ -81,13 +105,29 @@ ArmPlatformGetVirtualMemoryMap (
>> VirtualMemoryTable[Index].Length = 0x10000000;
>> VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>
>> + if (MemSize > MemLowSize) {
>> + //
>> + // If we have more than MemLowSize worth of DRAM, the remainder will be
>> + // mapped at the top of the remapped window.
>> + //
>> + VirtualMemoryTable[++Index].PhysicalBase = MemHighStart;
>> + VirtualMemoryTable[Index].VirtualBase = MemHighStart;
>> + VirtualMemoryTable[Index].Length = MemHighSize;
>> + VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
>> +
>> + BuildResourceDescriptorHob (
>> + EFI_RESOURCE_SYSTEM_MEMORY,
>> + ResourceAttributes,
>> + MemHighStart,
>> + MemHighSize
>> + );
>> + }
>> +
>> // 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);
>> -
>
> Why delete this assert?
>
>> *VirtualMemoryMap = VirtualMemoryTable;
>> }
>> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
>> index 434d6cb..db1c7fa 100644
>> --- a/Platform/Marvell/Marvell.dec
>> +++ b/Platform/Marvell/Marvell.dec
>> @@ -194,6 +194,19 @@
>> #TRNG
>> gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
>>
>> + #
>> + # DRAM remapping controls.
>> + # On the 70x0/80x0 SOCs, the DRAM is mapped at 0x0, and could be up to
>> + # 16 GB in size. To allow for 32-bit addressable MMIO peripherals or PCI
>> + # windows, a single window of up to 4 GB in size can be remapped elsewhere.
>> + # So let's define a 1 GB window at 0xC000000 by default: this is the minimum
>> + # alignment that Linux can map optimally (i.e., it's section shift is 30 bits)
>> + # and gives us an additional 768 MB (on top of the 256 MB platform MMIO window
>> + # at 0xF0000000) for the PCI MMIO32 and CONFIG spaces.
>
> If DRAM is mapped at 0, and can be up to 16GB in size, why do we need
> to remap some of it below 4GB?
> I guess what you're actually doing is mapping it _away_. Where to?
>
> (And a short-short version of this in the commit message should take
> care of that item.)
>
Indeed. Physical RAM starts at 0x0, and to avoid wasting all 32-bit
addressable memory space on RAM, the hardware allows some of it to be
remapped elsewhere. So by remapping 1 GB, we make 1 GB of address
space available for MMIO, PCI config space and PCI MMIO32 space.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping
2017-10-11 18:18 ` Ard Biesheuvel
@ 2017-10-12 4:58 ` Marcin Wojtas
2017-10-12 10:29 ` Leif Lindholm
0 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-12 4:58 UTC (permalink / raw)
To: Leif Lindholm
Cc: Ard Biesheuvel, edk2-devel@lists.01.org, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
Hi Leif,
2017-10-11 20:18 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 11 October 2017 at 18:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> Subject: from->for ?
>>
>> On Wed, Oct 11, 2017 at 05:40:45PM +0200, Marcin Wojtas wrote:
>>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>> The Armada 70x0/80x0 DRAM controller allows a single window of DRAM
>>> to be remapped to another location in the physical address space. This
>>> allows us to free up some memory in the 32-bit addressable region for
>>> peripheral MMIO and PCI MMIO32 and CONFIG spaces. This patch adjusts
>>> memory blocks to the configuration done in ATF.
>>
>> The ARM Trusted Firmware developers tend to frown at that
>> abbreviation. Please use ARM-TF in official context.
Ok.
>>
>> Which configuration is this? Presumably, if this was changed in
>> ARM-TF, we would need to change it this end too, so does not hurt to
>> be explicit. (It uses a FixedPcd, so clearly it is not dynamically
>> detected, which the commit message can be read as.)
I checked and I can modify the code, to obtain from registers an
information about source/target and whether Dram remap is enabled at
all. This way we would be immune to any further changes in ARM-TF.
However I have a question to that setting, please see below.
>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>> Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S | 15 +++++
>>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 +
>>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 60 ++++++++++++++++----
>>> Platform/Marvell/Marvell.dec | 13 +++++
>>> 4 files changed, 81 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>>> index 72f8cfc..c5be1a9 100644
>>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
>>> @@ -17,6 +17,21 @@
>>>
>>> ASM_FUNC(ArmPlatformPeiBootAction)
>>> mov x29, xzr
>>> +
>>> + .if FixedPcdGet64 (PcdSystemMemoryBase) != 0
>>> + .err PcdSystemMemoryBase should be 0x0 on this platform!
>>> + .endif
>>> +
>>> + .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
>>> + //
>>> + // Use the low range for UEFI itself. The remaining memory will be mapped
>>> + // and added to the GCD map later.
>>> + //
>>> + adr x0, mSystemMemoryEnd
>>> + MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
>>> + str x1, [x0]
>>> + .endif
>>> +
The original commit, due lack of dram size detection, increased total
memory to 4GB, which required above modification with limiting
mSystemMemoryEnd to dram target (0xc0000000). Now PcdSystemMemorySize
is left at somewhat lowest reasonable value (1GB), so above code is in
fact never executed. Are you ok that
I remove it and leave mSystemMemoryEnd to be set as-is in PrePi.c:
UINT64 mSystemMemoryEnd = FixedPcdGet64(PcdSystemMemoryBase) +
FixedPcdGet64(PcdSystemMemorySize) - 1;
?
>>> ret
>>>
>>> //UINTN
>>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>>> index 2e198c3..838a670 100644
>>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>>> @@ -67,5 +67,8 @@
>>> gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
>>> gArmTokenSpaceGuid.PcdArmPrimaryCore
>>>
>>> + gMarvellTokenSpaceGuid.PcdDramRemapSize
>>> + gMarvellTokenSpaceGuid.PcdDramRemapTarget
>>> +
>>> [Ppis]
>>> gArmMpCoreInfoPpiGuid
>>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>>> index 74c9956..2cb2e15 100644
>>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>>> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>> #include <Base.h>
>>> #include <Library/ArmPlatformLib.h>
>>> #include <Library/DebugLib.h>
>>> +#include <Library/HobLib.h>
>>> #include <Library/MemoryAllocationLib.h>
>>>
>>> // The total number of descriptors, including the final "end-of-table" descriptor.
>>> @@ -44,6 +45,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>> #define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK
>>> #define DDR_ATTRIBUTES_UNCACHED ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
>>>
>>> +STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>>
>> mVirtualMemoryTable?
Ok.
>>
>>> +
>>> /**
>>> Return the Virtual Memory Map of your platform
>>>
>>> @@ -59,20 +62,41 @@ ArmPlatformGetVirtualMemoryMap (
>>> IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
>>> )
>>> {
>>> - ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
>>> UINTN Index = 0;
>>> + UINT64 MemSize;
>>> + UINT64 MemLowSize;
>>> + UINT64 MemHighStart;
>>> + UINT64 MemHighSize;
>>> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
>>>
>>> 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;
>>> - }
>>> + MemSize = FixedPcdGet64 (PcdSystemMemorySize);
>>> + MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
>>> + MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
>>> + FixedPcdGet32 (PcdDramRemapSize);
>>> + MemHighSize = MemSize - MemLowSize;
>>> +
>>> + ResourceAttributes = (
>>> + EFI_RESOURCE_ATTRIBUTE_PRESENT |
>>> + EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
>>> + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>>> + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>>> + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
>>> + EFI_RESOURCE_ATTRIBUTE_TESTED
>>> + );
>>> +
>>> + BuildResourceDescriptorHob (
>>> + EFI_RESOURCE_SYSTEM_MEMORY,
>>> + ResourceAttributes,
>>> + FixedPcdGet64 (PcdSystemMemoryBase),
>>> + MemLowSize
>>> + );
>>>
>>> // DDR
>>> - VirtualMemoryTable[Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
>>> - VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdSystemMemoryBase);
>>> - VirtualMemoryTable[Index].Length = PcdGet64 (PcdSystemMemorySize);
>>> + VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdSystemMemoryBase);
>>> + VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdSystemMemoryBase);
>>> + VirtualMemoryTable[Index].Length = MemLowSize;
>>> VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
>>>
>>> // Configuration space 0xF000_0000 - 0xFFFF_FFFF
>>> @@ -81,13 +105,29 @@ ArmPlatformGetVirtualMemoryMap (
>>> VirtualMemoryTable[Index].Length = 0x10000000;
>>> VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>>
>>> + if (MemSize > MemLowSize) {
>>> + //
>>> + // If we have more than MemLowSize worth of DRAM, the remainder will be
>>> + // mapped at the top of the remapped window.
>>> + //
>>> + VirtualMemoryTable[++Index].PhysicalBase = MemHighStart;
>>> + VirtualMemoryTable[Index].VirtualBase = MemHighStart;
>>> + VirtualMemoryTable[Index].Length = MemHighSize;
>>> + VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
>>> +
>>> + BuildResourceDescriptorHob (
>>> + EFI_RESOURCE_SYSTEM_MEMORY,
>>> + ResourceAttributes,
>>> + MemHighStart,
>>> + MemHighSize
>>> + );
>>> + }
>>> +
>>> // 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);
>>> -
>>
>> Why delete this assert?
Indeed, it looks like unnecessarily removed. I will restore it,
>>
>>> *VirtualMemoryMap = VirtualMemoryTable;
>>> }
>>> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
>>> index 434d6cb..db1c7fa 100644
>>> --- a/Platform/Marvell/Marvell.dec
>>> +++ b/Platform/Marvell/Marvell.dec
>>> @@ -194,6 +194,19 @@
>>> #TRNG
>>> gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
>>>
>>> + #
>>> + # DRAM remapping controls.
>>> + # On the 70x0/80x0 SOCs, the DRAM is mapped at 0x0, and could be up to
>>> + # 16 GB in size. To allow for 32-bit addressable MMIO peripherals or PCI
>>> + # windows, a single window of up to 4 GB in size can be remapped elsewhere.
>>> + # So let's define a 1 GB window at 0xC000000 by default: this is the minimum
>>> + # alignment that Linux can map optimally (i.e., it's section shift is 30 bits)
>>> + # and gives us an additional 768 MB (on top of the 256 MB platform MMIO window
>>> + # at 0xF0000000) for the PCI MMIO32 and CONFIG spaces.
>>
>> If DRAM is mapped at 0, and can be up to 16GB in size, why do we need
>> to remap some of it below 4GB?
>> I guess what you're actually doing is mapping it _away_. Where to?
>>
>> (And a short-short version of this in the commit message should take
>> care of that item.)
>>
>
> Indeed. Physical RAM starts at 0x0, and to avoid wasting all 32-bit
> addressable memory space on RAM, the hardware allows some of it to be
> remapped elsewhere. So by remapping 1 GB, we make 1 GB of address
> space available for MMIO, PCI config space and PCI MMIO32 space.
If we agree to detect remap size and target from registers, those PCDs
(and comment) will be removed. I will modify the commit log only.
Best regards,
Marcin
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping
2017-10-12 4:58 ` Marcin Wojtas
@ 2017-10-12 10:29 ` Leif Lindholm
0 siblings, 0 replies; 27+ messages in thread
From: Leif Lindholm @ 2017-10-12 10:29 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Ard Biesheuvel, edk2-devel@lists.01.org, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
On Thu, Oct 12, 2017 at 06:58:37AM +0200, Marcin Wojtas wrote:
> Hi Leif,
>
> 2017-10-11 20:18 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> > On 11 October 2017 at 18:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> Subject: from->for ?
> >>
> >> On Wed, Oct 11, 2017 at 05:40:45PM +0200, Marcin Wojtas wrote:
> >>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>
> >>> The Armada 70x0/80x0 DRAM controller allows a single window of DRAM
> >>> to be remapped to another location in the physical address space. This
> >>> allows us to free up some memory in the 32-bit addressable region for
> >>> peripheral MMIO and PCI MMIO32 and CONFIG spaces. This patch adjusts
> >>> memory blocks to the configuration done in ATF.
> >>
> >> The ARM Trusted Firmware developers tend to frown at that
> >> abbreviation. Please use ARM-TF in official context.
>
> Ok.
>
> >>
> >> Which configuration is this? Presumably, if this was changed in
> >> ARM-TF, we would need to change it this end too, so does not hurt to
> >> be explicit. (It uses a FixedPcd, so clearly it is not dynamically
> >> detected, which the commit message can be read as.)
>
> I checked and I can modify the code, to obtain from registers an
> information about source/target and whether Dram remap is enabled at
> all. This way we would be immune to any further changes in ARM-TF.
That sounds like an excellent improvement.
> However I have a question to that setting, please see below.
>
> >>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >>> ---
> >>> Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S | 15 +++++
> >>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 +
> >>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 60 ++++++++++++++++----
> >>> Platform/Marvell/Marvell.dec | 13 +++++
> >>> 4 files changed, 81 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> >>> index 72f8cfc..c5be1a9 100644
> >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> >>> @@ -17,6 +17,21 @@
> >>>
> >>> ASM_FUNC(ArmPlatformPeiBootAction)
> >>> mov x29, xzr
> >>> +
> >>> + .if FixedPcdGet64 (PcdSystemMemoryBase) != 0
> >>> + .err PcdSystemMemoryBase should be 0x0 on this platform!
> >>> + .endif
> >>> +
> >>> + .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
> >>> + //
> >>> + // Use the low range for UEFI itself. The remaining memory will be mapped
> >>> + // and added to the GCD map later.
> >>> + //
> >>> + adr x0, mSystemMemoryEnd
> >>> + MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> >>> + str x1, [x0]
> >>> + .endif
> >>> +
>
> The original commit, due lack of dram size detection, increased total
> memory to 4GB, which required above modification with limiting
> mSystemMemoryEnd to dram target (0xc0000000). Now PcdSystemMemorySize
> is left at somewhat lowest reasonable value (1GB), so above code is in
> fact never executed. Are you ok that
> I remove it and leave mSystemMemoryEnd to be set as-is in PrePi.c:
>
> UINT64 mSystemMemoryEnd = FixedPcdGet64(PcdSystemMemoryBase) +
> FixedPcdGet64(PcdSystemMemorySize) - 1;
>
> ?
Unless Ard tells me we're missing some aspect, that sounds reasonable.
> >>> ret
> >>>
> >>> //UINTN
> >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> >>> index 2e198c3..838a670 100644
> >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> >>> @@ -67,5 +67,8 @@
> >>> gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
> >>> gArmTokenSpaceGuid.PcdArmPrimaryCore
> >>>
> >>> + gMarvellTokenSpaceGuid.PcdDramRemapSize
> >>> + gMarvellTokenSpaceGuid.PcdDramRemapTarget
> >>> +
> >>> [Ppis]
> >>> gArmMpCoreInfoPpiGuid
> >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >>> index 74c9956..2cb2e15 100644
> >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >>> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>> #include <Base.h>
> >>> #include <Library/ArmPlatformLib.h>
> >>> #include <Library/DebugLib.h>
> >>> +#include <Library/HobLib.h>
> >>> #include <Library/MemoryAllocationLib.h>
> >>>
> >>> // The total number of descriptors, including the final "end-of-table" descriptor.
> >>> @@ -44,6 +45,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>> #define DDR_ATTRIBUTES_CACHED ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK
> >>> #define DDR_ATTRIBUTES_UNCACHED ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
> >>>
> >>> +STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
> >>
> >> mVirtualMemoryTable?
>
> Ok.
>
> >>
> >>> +
> >>> /**
> >>> Return the Virtual Memory Map of your platform
> >>>
> >>> @@ -59,20 +62,41 @@ ArmPlatformGetVirtualMemoryMap (
> >>> IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
> >>> )
> >>> {
> >>> - ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable;
> >>> UINTN Index = 0;
> >>> + UINT64 MemSize;
> >>> + UINT64 MemLowSize;
> >>> + UINT64 MemHighStart;
> >>> + UINT64 MemHighSize;
> >>> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> >>>
> >>> 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;
> >>> - }
> >>> + MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> >>> + MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
> >>> + MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
> >>> + FixedPcdGet32 (PcdDramRemapSize);
> >>> + MemHighSize = MemSize - MemLowSize;
> >>> +
> >>> + ResourceAttributes = (
> >>> + EFI_RESOURCE_ATTRIBUTE_PRESENT |
> >>> + EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> >>> + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> >>> + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> >>> + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> >>> + EFI_RESOURCE_ATTRIBUTE_TESTED
> >>> + );
> >>> +
> >>> + BuildResourceDescriptorHob (
> >>> + EFI_RESOURCE_SYSTEM_MEMORY,
> >>> + ResourceAttributes,
> >>> + FixedPcdGet64 (PcdSystemMemoryBase),
> >>> + MemLowSize
> >>> + );
> >>>
> >>> // DDR
> >>> - VirtualMemoryTable[Index].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
> >>> - VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdSystemMemoryBase);
> >>> - VirtualMemoryTable[Index].Length = PcdGet64 (PcdSystemMemorySize);
> >>> + VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdSystemMemoryBase);
> >>> + VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdSystemMemoryBase);
> >>> + VirtualMemoryTable[Index].Length = MemLowSize;
> >>> VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
> >>>
> >>> // Configuration space 0xF000_0000 - 0xFFFF_FFFF
> >>> @@ -81,13 +105,29 @@ ArmPlatformGetVirtualMemoryMap (
> >>> VirtualMemoryTable[Index].Length = 0x10000000;
> >>> VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >>>
> >>> + if (MemSize > MemLowSize) {
> >>> + //
> >>> + // If we have more than MemLowSize worth of DRAM, the remainder will be
> >>> + // mapped at the top of the remapped window.
> >>> + //
> >>> + VirtualMemoryTable[++Index].PhysicalBase = MemHighStart;
> >>> + VirtualMemoryTable[Index].VirtualBase = MemHighStart;
> >>> + VirtualMemoryTable[Index].Length = MemHighSize;
> >>> + VirtualMemoryTable[Index].Attributes = DDR_ATTRIBUTES_CACHED;
> >>> +
> >>> + BuildResourceDescriptorHob (
> >>> + EFI_RESOURCE_SYSTEM_MEMORY,
> >>> + ResourceAttributes,
> >>> + MemHighStart,
> >>> + MemHighSize
> >>> + );
> >>> + }
> >>> +
> >>> // 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);
> >>> -
> >>
> >> Why delete this assert?
>
> Indeed, it looks like unnecessarily removed. I will restore it,
>
> >>
> >>> *VirtualMemoryMap = VirtualMemoryTable;
> >>> }
> >>> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> >>> index 434d6cb..db1c7fa 100644
> >>> --- a/Platform/Marvell/Marvell.dec
> >>> +++ b/Platform/Marvell/Marvell.dec
> >>> @@ -194,6 +194,19 @@
> >>> #TRNG
> >>> gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
> >>>
> >>> + #
> >>> + # DRAM remapping controls.
> >>> + # On the 70x0/80x0 SOCs, the DRAM is mapped at 0x0, and could be up to
> >>> + # 16 GB in size. To allow for 32-bit addressable MMIO peripherals or PCI
> >>> + # windows, a single window of up to 4 GB in size can be remapped elsewhere.
> >>> + # So let's define a 1 GB window at 0xC000000 by default: this is the minimum
> >>> + # alignment that Linux can map optimally (i.e., it's section shift is 30 bits)
> >>> + # and gives us an additional 768 MB (on top of the 256 MB platform MMIO window
> >>> + # at 0xF0000000) for the PCI MMIO32 and CONFIG spaces.
> >>
> >> If DRAM is mapped at 0, and can be up to 16GB in size, why do we need
> >> to remap some of it below 4GB?
> >> I guess what you're actually doing is mapping it _away_. Where to?
> >>
> >> (And a short-short version of this in the commit message should take
> >> care of that item.)
> >>
> >
> > Indeed. Physical RAM starts at 0x0, and to avoid wasting all 32-bit
> > addressable memory space on RAM, the hardware allows some of it to be
> > remapped elsewhere. So by remapping 1 GB, we make 1 GB of address
> > space available for MMIO, PCI config space and PCI MMIO32 space.
>
> If we agree to detect remap size and target from registers, those PCDs
> (and comment) will be removed. I will modify the commit log only.
Sounds good to me.
/
Leif
^ permalink raw reply [flat|nested] 27+ messages in thread
* [platforms: PATCH 5/8] Marvell/Armada: Add MemoryInitPeiLib that reserves secure region
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
` (3 preceding siblings ...)
2017-10-11 15:40 ` [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 17:11 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection Marcin Wojtas
` (2 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
The default MemoryInitPeiLib implementation insists on reserving the
region occupied by our own FV, while this is not necessary at all (the
compressed payload is uncompressed elsewhere, so the moment we enter
DXE core, we don't care about the FV contents in memory)
So clone MemoryInitPeiLib and modify it to suit our needs.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada.dsc.inc | 6 +-
Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c | 158 ++++++++++++++++++++
Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf | 46 ++++++
Platform/Marvell/Marvell.dec | 8 +
4 files changed, 217 insertions(+), 1 deletion(-)
diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index 56d8941..b0a8240 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -153,7 +153,7 @@
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
- MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
+ MemoryInitPeiLib|Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
[LibraryClasses.common.DXE_CORE]
@@ -364,6 +364,10 @@
gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
gArmTokenSpaceGuid.PcdArmScr|0x531
+ # Secure region reservation
+ gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
+ gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
+
# TRNG
gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
diff --git a/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c
new file mode 100644
index 0000000..53119f4
--- /dev/null
+++ b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c
@@ -0,0 +1,158 @@
+/** @file
+*
+* Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+* Copyright (c) 2017, ARM 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.
+*
+**/
+
+#include <PiPei.h>
+
+#include <Library/ArmMmuLib.h>
+#include <Library/ArmPlatformLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/PcdLib.h>
+
+VOID
+BuildMemoryTypeInformationHob (
+ VOID
+ );
+
+STATIC
+VOID
+InitMmu (
+ IN ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable
+ )
+{
+
+ VOID *TranslationTableBase;
+ UINTN TranslationTableSize;
+ RETURN_STATUS Status;
+
+ Status = ArmConfigureMmu (MemoryTable,
+ &TranslationTableBase,
+ &TranslationTableSize);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Error: Failed to enable MMU\n"));
+ }
+}
+
+/*++
+
+Routine Description:
+
+
+
+Arguments:
+
+ FileHandle - Handle of the file being invoked.
+ PeiServices - Describes the list of possible PEI Services.
+
+Returns:
+
+ Status - EFI_SUCCESS if the boot mode could be set
+
+--*/
+EFI_STATUS
+EFIAPI
+MemoryPeim (
+ IN EFI_PHYSICAL_ADDRESS UefiMemoryBase,
+ IN UINT64 UefiMemorySize
+ )
+{
+ ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
+ EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
+ UINT64 ResourceLength;
+ EFI_PEI_HOB_POINTERS NextHob;
+ EFI_PHYSICAL_ADDRESS SecureTop;
+ EFI_PHYSICAL_ADDRESS ResourceTop;
+
+ // Get Virtual Memory Map from the Platform Library
+ ArmPlatformGetVirtualMemoryMap (&MemoryTable);
+
+ SecureTop = (EFI_PHYSICAL_ADDRESS)FixedPcdGet64 (PcdSecureRegionBase) +
+ FixedPcdGet32 (PcdSecureRegionSize);
+
+ //
+ // Search for System Memory Hob that covers the secure firmware,
+ // and punch a hole in it
+ //
+ for (NextHob.Raw = GetHobList ();
+ NextHob.Raw != NULL;
+ NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
+ NextHob.Raw)) {
+
+ if ((NextHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) &&
+ (FixedPcdGet64 (PcdSecureRegionBase) >= NextHob.ResourceDescriptor->PhysicalStart) &&
+ (SecureTop <= NextHob.ResourceDescriptor->PhysicalStart +
+ NextHob.ResourceDescriptor->ResourceLength))
+ {
+ ResourceAttributes = NextHob.ResourceDescriptor->ResourceAttribute;
+ ResourceLength = NextHob.ResourceDescriptor->ResourceLength;
+ ResourceTop = NextHob.ResourceDescriptor->PhysicalStart + ResourceLength;
+
+ if (FixedPcdGet64 (PcdSecureRegionBase) == NextHob.ResourceDescriptor->PhysicalStart) {
+ //
+ // This region starts right at the start of the reserved region, so we
+ // can simply move its start pointer and reduce its length by the same
+ // value
+ //
+ NextHob.ResourceDescriptor->PhysicalStart += FixedPcdGet32 (PcdSecureRegionSize);
+ NextHob.ResourceDescriptor->ResourceLength -= FixedPcdGet32 (PcdSecureRegionSize);
+
+ } else if ((NextHob.ResourceDescriptor->PhysicalStart +
+ NextHob.ResourceDescriptor->ResourceLength) == SecureTop) {
+
+ //
+ // This region ends right at the end of the reserved region, so we
+ // can simply reduce its length by the size of the region.
+ //
+ NextHob.ResourceDescriptor->ResourceLength -= FixedPcdGet32 (PcdSecureRegionSize);
+
+ } else {
+ //
+ // This region covers the reserved region. So split it into two regions,
+ // each one touching the reserved region at either end, but not covering
+ // it.
+ //
+ NextHob.ResourceDescriptor->ResourceLength = FixedPcdGet64 (PcdSecureRegionBase) -
+ NextHob.ResourceDescriptor->PhysicalStart;
+
+ // Create the System Memory HOB for the remaining region (top of the FD)
+ BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
+ ResourceAttributes,
+ SecureTop,
+ ResourceTop - SecureTop);
+ }
+
+ //
+ // Reserve the memory space occupied by the secure firmware
+ //
+ BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED,
+ 0,
+ FixedPcdGet64 (PcdSecureRegionBase),
+ FixedPcdGet32 (PcdSecureRegionSize));
+
+ break;
+ }
+ NextHob.Raw = GET_NEXT_HOB (NextHob);
+ }
+
+ // Build Memory Allocation Hob
+ InitMmu (MemoryTable);
+
+ if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) {
+ // Optional feature that helps prevent EFI memory map fragmentation.
+ BuildMemoryTypeInformationHob ();
+ }
+
+ return EFI_SUCCESS;
+}
diff --git a/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf
new file mode 100644
index 0000000..ebaed01
--- /dev/null
+++ b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf
@@ -0,0 +1,46 @@
+#/** @file
+#
+# Copyright (c) 2011-2014, ARM Ltd. All rights reserved.<BR>
+# Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
+#
+# 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 = 0x00010019
+ BASE_NAME = Armada70x0MemoryInitPeiLib
+ FILE_GUID = abc4e8a7-89a7-4aea-92bc-0e9421c4a473
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = MemoryInitPeiLib|SEC PEIM
+
+[Sources]
+ Armada70x0MemoryInitPeiLib.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ ArmPlatformPkg/ArmPlatformPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Platform/Marvell/Marvell.dec
+
+[LibraryClasses]
+ ArmPlatformLib
+ DebugLib
+ HobLib
+ ArmMmuLib
+
+[FeaturePcd]
+ gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
+
+[FixedPcd]
+ gMarvellTokenSpaceGuid.PcdSecureRegionBase
+ gMarvellTokenSpaceGuid.PcdSecureRegionSize
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index db1c7fa..63ea071 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -207,6 +207,14 @@
gMarvellTokenSpaceGuid.PcdDramRemapSize|0x40000000|UINT32|0x50000004
gMarvellTokenSpaceGuid.PcdDramRemapTarget|0xC0000000|UINT32|0x50000003
+ #
+ # The secure firmware may occupy a DRAM region that is accessible by the
+ # normal world. These PCDs describe such a region, which will be converted
+ # to 'reserved' memory before DXE is entered.
+ #
+ gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x0|UINT64|0x50000000
+ gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0|UINT32|0x50000001
+
[Protocols]
gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 5/8] Marvell/Armada: Add MemoryInitPeiLib that reserves secure region
2017-10-11 15:40 ` [platforms: PATCH 5/8] Marvell/Armada: Add MemoryInitPeiLib that reserves secure region Marcin Wojtas
@ 2017-10-11 17:11 ` Leif Lindholm
0 siblings, 0 replies; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 17:11 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Wed, Oct 11, 2017 at 05:40:46PM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> The default MemoryInitPeiLib implementation insists on reserving the
> region occupied by our own FV, while this is not necessary at all (the
> compressed payload is uncompressed elsewhere, so the moment we enter
> DXE core, we don't care about the FV contents in memory)
>
> So clone MemoryInitPeiLib and modify it to suit our needs.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/Marvell/Armada/Armada.dsc.inc | 6 +-
> Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c | 158 ++++++++++++++++++++
> Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf | 46 ++++++
> Platform/Marvell/Marvell.dec | 8 +
> 4 files changed, 217 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index 56d8941..b0a8240 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -153,7 +153,7 @@
> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>
> [LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
> - MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
> + MemoryInitPeiLib|Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf
> BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>
> [LibraryClasses.common.DXE_CORE]
> @@ -364,6 +364,10 @@
> gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
> gArmTokenSpaceGuid.PcdArmScr|0x531
>
> + # Secure region reservation
> + gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> + gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> +
> # TRNG
> gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
>
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c
> new file mode 100644
> index 0000000..53119f4
> --- /dev/null
> +++ b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.c
> @@ -0,0 +1,158 @@
> +/** @file
> +*
> +* Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +* Copyright (c) 2017, ARM 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.
> +*
> +**/
> +
> +#include <PiPei.h>
> +
> +#include <Library/ArmMmuLib.h>
> +#include <Library/ArmPlatformLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PcdLib.h>
> +
> +VOID
> +BuildMemoryTypeInformationHob (
> + VOID
> + );
> +
> +STATIC
> +VOID
> +InitMmu (
> + IN ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable
> + )
> +{
> +
> + VOID *TranslationTableBase;
> + UINTN TranslationTableSize;
> + RETURN_STATUS Status;
> +
> + Status = ArmConfigureMmu (MemoryTable,
> + &TranslationTableBase,
> + &TranslationTableSize);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Error: Failed to enable MMU\n"));
> + }
> +}
> +
> +/*++
> +
> +Routine Description:
> +
> +
> +
> +Arguments:
> +
> + FileHandle - Handle of the file being invoked.
> + PeiServices - Describes the list of possible PEI Services.
> +
> +Returns:
> +
> + Status - EFI_SUCCESS if the boot mode could be set
> +
> +--*/
> +EFI_STATUS
> +EFIAPI
> +MemoryPeim (
> + IN EFI_PHYSICAL_ADDRESS UefiMemoryBase,
> + IN UINT64 UefiMemorySize
> + )
> +{
> + ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable;
> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> + UINT64 ResourceLength;
> + EFI_PEI_HOB_POINTERS NextHob;
> + EFI_PHYSICAL_ADDRESS SecureTop;
> + EFI_PHYSICAL_ADDRESS ResourceTop;
> +
> + // Get Virtual Memory Map from the Platform Library
> + ArmPlatformGetVirtualMemoryMap (&MemoryTable);
> +
> + SecureTop = (EFI_PHYSICAL_ADDRESS)FixedPcdGet64 (PcdSecureRegionBase) +
> + FixedPcdGet32 (PcdSecureRegionSize);
> +
> + //
> + // Search for System Memory Hob that covers the secure firmware,
> + // and punch a hole in it
> + //
> + for (NextHob.Raw = GetHobList ();
> + NextHob.Raw != NULL;
> + NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> + NextHob.Raw)) {
> +
> + if ((NextHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) &&
> + (FixedPcdGet64 (PcdSecureRegionBase) >= NextHob.ResourceDescriptor->PhysicalStart) &&
> + (SecureTop <= NextHob.ResourceDescriptor->PhysicalStart +
> + NextHob.ResourceDescriptor->ResourceLength))
> + {
> + ResourceAttributes = NextHob.ResourceDescriptor->ResourceAttribute;
> + ResourceLength = NextHob.ResourceDescriptor->ResourceLength;
> + ResourceTop = NextHob.ResourceDescriptor->PhysicalStart + ResourceLength;
> +
> + if (FixedPcdGet64 (PcdSecureRegionBase) == NextHob.ResourceDescriptor->PhysicalStart) {
> + //
> + // This region starts right at the start of the reserved region, so we
> + // can simply move its start pointer and reduce its length by the same
> + // value
> + //
> + NextHob.ResourceDescriptor->PhysicalStart += FixedPcdGet32 (PcdSecureRegionSize);
> + NextHob.ResourceDescriptor->ResourceLength -= FixedPcdGet32 (PcdSecureRegionSize);
> +
> + } else if ((NextHob.ResourceDescriptor->PhysicalStart +
> + NextHob.ResourceDescriptor->ResourceLength) == SecureTop) {
> +
> + //
> + // This region ends right at the end of the reserved region, so we
> + // can simply reduce its length by the size of the region.
> + //
> + NextHob.ResourceDescriptor->ResourceLength -= FixedPcdGet32 (PcdSecureRegionSize);
> +
> + } else {
> + //
> + // This region covers the reserved region. So split it into two regions,
> + // each one touching the reserved region at either end, but not covering
> + // it.
> + //
> + NextHob.ResourceDescriptor->ResourceLength = FixedPcdGet64 (PcdSecureRegionBase) -
> + NextHob.ResourceDescriptor->PhysicalStart;
> +
> + // Create the System Memory HOB for the remaining region (top of the FD)
> + BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> + ResourceAttributes,
> + SecureTop,
> + ResourceTop - SecureTop);
> + }
> +
> + //
> + // Reserve the memory space occupied by the secure firmware
> + //
> + BuildResourceDescriptorHob (EFI_RESOURCE_MEMORY_RESERVED,
> + 0,
> + FixedPcdGet64 (PcdSecureRegionBase),
> + FixedPcdGet32 (PcdSecureRegionSize));
> +
> + break;
> + }
> + NextHob.Raw = GET_NEXT_HOB (NextHob);
> + }
> +
> + // Build Memory Allocation Hob
> + InitMmu (MemoryTable);
> +
> + if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) {
> + // Optional feature that helps prevent EFI memory map fragmentation.
> + BuildMemoryTypeInformationHob ();
> + }
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf
> new file mode 100644
> index 0000000..ebaed01
> --- /dev/null
> +++ b/Platform/Marvell/Armada/Library/Armada70x0MemoryInitPeiLib/Armada70x0MemoryInitPeiLib.inf
> @@ -0,0 +1,46 @@
> +#/** @file
> +#
> +# Copyright (c) 2011-2014, ARM Ltd. All rights reserved.<BR>
> +# Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
> +#
> +# 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 = 0x00010019
> + BASE_NAME = Armada70x0MemoryInitPeiLib
> + FILE_GUID = abc4e8a7-89a7-4aea-92bc-0e9421c4a473
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = MemoryInitPeiLib|SEC PEIM
> +
> +[Sources]
> + Armada70x0MemoryInitPeiLib.c
> +
> +[Packages]
> + ArmPkg/ArmPkg.dec
> + ArmPlatformPkg/ArmPlatformPkg.dec
> + EmbeddedPkg/EmbeddedPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + MdePkg/MdePkg.dec
> + Platform/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> + ArmPlatformLib
> + DebugLib
> + HobLib
> + ArmMmuLib
> +
> +[FeaturePcd]
> + gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob
> +
> +[FixedPcd]
> + gMarvellTokenSpaceGuid.PcdSecureRegionBase
> + gMarvellTokenSpaceGuid.PcdSecureRegionSize
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index db1c7fa..63ea071 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -207,6 +207,14 @@
> gMarvellTokenSpaceGuid.PcdDramRemapSize|0x40000000|UINT32|0x50000004
> gMarvellTokenSpaceGuid.PcdDramRemapTarget|0xC0000000|UINT32|0x50000003
>
> + #
> + # The secure firmware may occupy a DRAM region that is accessible by the
> + # normal world. These PCDs describe such a region, which will be converted
> + # to 'reserved' memory before DXE is entered.
> + #
> + gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x0|UINT64|0x50000000
> + gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0|UINT32|0x50000001
> +
> [Protocols]
> gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
> gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
` (4 preceding siblings ...)
2017-10-11 15:40 ` [platforms: PATCH 5/8] Marvell/Armada: Add MemoryInitPeiLib that reserves secure region Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 17:56 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 7/8] Marvell/Armada: Armada70x0Lib: Add support for 32-bit ARM Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support Marcin Wojtas
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Instead of using hardcoded value in PcdSystemMemorySize PCD,
obtain DRAM size directly from SoC registers, which are filled
by firmware during early initialization stage.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
2 files changed, 159 insertions(+), 1 deletion(-)
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
index 2cb2e15..22cbe47 100644
--- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
@@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Library/ArmPlatformLib.h>
#include <Library/DebugLib.h>
#include <Library/HobLib.h>
+#include <Library/IoLib.h>
#include <Library/MemoryAllocationLib.h>
+#include "Armada70x0LibMem.h"
+
// The total number of descriptors, including the final "end-of-table" descriptor.
#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
@@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
+// Obtain DRAM size basing on register values filled by early firmware.
+STATIC
+UINT64
+DramSizeGet (
+ UINT64 *MemSize
+ )
+{
+ UINT64 BaseAddr;
+ UINT32 RegVal;
+ UINT8 AreaLengthMap;
+ UINT8 Cs;
+
+ *MemSize = 0;
+
+ for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
+
+ RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
+
+ /* Exit loop on first disabled DRAM CS */
+ if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
+ break;
+ }
+
+ /*
+ * Sanity check for base address of next DRAM block.
+ * Only continuous space will be used.
+ */
+ BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
+ DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
+ if (BaseAddr != *MemSize) {
+ DEBUG ((DEBUG_ERROR,
+ "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
+ *MemSize));
+ return EFI_SUCCESS;
+ }
+
+ /* Decode area length for current CS from register value */
+ AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
+ switch (AreaLengthMap) {
+ case 0x0:
+ *MemSize += 0x18000000;
+ break;
+ case 0x1:
+ *MemSize += 0x30000000;
+ break;
+ case 0x2:
+ *MemSize += 0x60000000;
+ break;
+ case 0x3:
+ *MemSize += 0xC0000000;
+ break;
+ case 0x7:
+ *MemSize += 0x00800000;
+ break;
+ case 0x8:
+ *MemSize += 0x01000000;
+ break;
+ case 0x9:
+ *MemSize += 0x02000000;
+ break;
+ case 0xA:
+ *MemSize += 0x04000000;
+ break;
+ case 0xB:
+ *MemSize += 0x08000000;
+ break;
+ case 0xC:
+ *MemSize += 0x10000000;
+ break;
+ case 0xD:
+ *MemSize += 0x20000000;
+ break;
+ case 0xE:
+ *MemSize += 0x40000000;
+ break;
+ case 0xF:
+ *MemSize += 0x80000000;
+ break;
+ case 0x10:
+ *MemSize += 0x100000000;
+ break;
+ case 0x11:
+ *MemSize += 0x200000000;
+ break;
+ case 0x12:
+ *MemSize += 0x400000000;
+ break;
+ case 0x13:
+ *MemSize += 0x800000000;
+ break;
+ default:
+ DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs));
+ return EFI_INVALID_PARAMETER;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Return the Virtual Memory Map of your platform
@@ -68,10 +170,17 @@ ArmPlatformGetVirtualMemoryMap (
UINT64 MemHighStart;
UINT64 MemHighSize;
EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
+ EFI_STATUS Status;
ASSERT (VirtualMemoryMap != NULL);
- MemSize = FixedPcdGet64 (PcdSystemMemorySize);
+ // Obtain total memory size from the hardware.
+ Status = DramSizeGet (&MemSize);
+ if (EFI_ERROR (Status)) {
+ MemSize = FixedPcdGet64 (PcdSystemMemorySize);
+ DEBUG ((DEBUG_ERROR, "Limit total memory size to %d MB\n", MemSize / 1024 / 1024));
+ }
+
MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
FixedPcdGet32 (PcdDramRemapSize);
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
new file mode 100644
index 0000000..b81fd1d
--- /dev/null
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
@@ -0,0 +1,49 @@
+/*******************************************************************************
+Copyright (C) 2017 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.
+
+*******************************************************************************/
+
+#define DRAM_CONF_BASE 0xf0020000
+
+#define DRAM_CH0_MMAP_LOW_BASE (DRAM_CONF_BASE + 0x200)
+#define DRAM_CH0_MMAP_LOW_REG(cs) (DRAM_CH0_MMAP_LOW_BASE + (cs) * 0x8)
+#define DRAM_CS_VALID_ENABLED_MASK 0x1
+#define DRAM_AREA_LENGTH_OFFS 16
+#define DRAM_AREA_LENGTH_MASK (0x1f << DRAM_AREA_LENGTH_OFFS)
+#define DRAM_START_ADDRESS_L_OFFS 23
+#define DRAM_START_ADDRESS_L_MASK (0x1ff << DRAM_START_ADDRESS_L_OFFS)
+
+#define DRAM_CH0_MMAP_HIGH_BASE (DRAM_CONF_BASE + 0x204)
+#define DRAM_CH0_MMAP_HIGH_REG(cs) (DRAM_CH0_MMAP_HIGH_BASE + (cs) * 0x8)
+#define DRAM_START_ADDR_HTOL_OFFS 32
+
+#define DRAM_MAX_CS_NUM 8
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection
2017-10-11 15:40 ` [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection Marcin Wojtas
@ 2017-10-11 17:56 ` Leif Lindholm
2017-10-12 5:47 ` Marcin Wojtas
0 siblings, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 17:56 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
> Instead of using hardcoded value in PcdSystemMemorySize PCD,
> obtain DRAM size directly from SoC registers, which are filled
> by firmware during early initialization stage.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
> 2 files changed, 159 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> index 2cb2e15..22cbe47 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> #include <Library/ArmPlatformLib.h>
> #include <Library/DebugLib.h>
> #include <Library/HobLib.h>
> +#include <Library/IoLib.h>
> #include <Library/MemoryAllocationLib.h>
>
> +#include "Armada70x0LibMem.h"
> +
> // The total number of descriptors, including the final "end-of-table" descriptor.
> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
>
> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>
> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>
> +// Obtain DRAM size basing on register values filled by early firmware.
> +STATIC
> +UINT64
> +DramSizeGet (
GetDramSize?
> + UINT64 *MemSize
IN, OUT?
> + )
> +{
> + UINT64 BaseAddr;
> + UINT32 RegVal;
> + UINT8 AreaLengthMap;
> + UINT8 Cs;
> +
> + *MemSize = 0;
> +
> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> +
> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
> +
> + /* Exit loop on first disabled DRAM CS */
> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
> + break;
> + }
> +
> + /*
> + * Sanity check for base address of next DRAM block.
> + * Only continuous space will be used.
> + */
> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
Please use macros, temporary variables, more parentheses or whatever
to help make the above operation readable.
> + if (BaseAddr != *MemSize) {
> + DEBUG ((DEBUG_ERROR,
> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> + *MemSize));
> + return EFI_SUCCESS;
> + }
> +
> + /* Decode area length for current CS from register value */
> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
> + switch (AreaLengthMap) {
Why Map?
> + case 0x0:
> + *MemSize += 0x18000000;
> + break;
> + case 0x1:
> + *MemSize += 0x30000000;
> + break;
> + case 0x2:
> + *MemSize += 0x60000000;
> + break;
> + case 0x3:
> + *MemSize += 0xC0000000;
> + break;
The above ones look if not devoid of pattern, at least a bit
unexpected - and 4-6 are comlpetely missing. Is there any
documentation available for me to read to try to understand what is
going on?
The below all look predictably formatted and possible to calculate
rather than list one by one.
(1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
> + case 0x7:
> + *MemSize += 0x00800000;
> + break;
> + case 0x8:
> + *MemSize += 0x01000000;
> + break;
> + case 0x9:
> + *MemSize += 0x02000000;
> + break;
> + case 0xA:
> + *MemSize += 0x04000000;
> + break;
> + case 0xB:
> + *MemSize += 0x08000000;
> + break;
> + case 0xC:
> + *MemSize += 0x10000000;
> + break;
> + case 0xD:
> + *MemSize += 0x20000000;
> + break;
> + case 0xE:
> + *MemSize += 0x40000000;
> + break;
> + case 0xF:
> + *MemSize += 0x80000000;
> + break;
> + case 0x10:
> + *MemSize += 0x100000000;
> + break;
> + case 0x11:
> + *MemSize += 0x200000000;
> + break;
> + case 0x12:
> + *MemSize += 0x400000000;
> + break;
> + case 0x13:
> + *MemSize += 0x800000000;
> + break;
> + default:
> + DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs));
Area length isn't really a helpful debug message.
"memory module size"?
/
Leif
> + return EFI_INVALID_PARAMETER;
> + }
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Return the Virtual Memory Map of your platform
>
> @@ -68,10 +170,17 @@ ArmPlatformGetVirtualMemoryMap (
> UINT64 MemHighStart;
> UINT64 MemHighSize;
> EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> + EFI_STATUS Status;
>
> ASSERT (VirtualMemoryMap != NULL);
>
> - MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> + // Obtain total memory size from the hardware.
> + Status = DramSizeGet (&MemSize);
> + if (EFI_ERROR (Status)) {
> + MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> + DEBUG ((DEBUG_ERROR, "Limit total memory size to %d MB\n", MemSize / 1024 / 1024));
> + }
> +
> MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
> MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
> FixedPcdGet32 (PcdDramRemapSize);
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> new file mode 100644
> index 0000000..b81fd1d
> --- /dev/null
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> @@ -0,0 +1,49 @@
> +/*******************************************************************************
> +Copyright (C) 2017 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.
> +
> +*******************************************************************************/
> +
> +#define DRAM_CONF_BASE 0xf0020000
> +
> +#define DRAM_CH0_MMAP_LOW_BASE (DRAM_CONF_BASE + 0x200)
> +#define DRAM_CH0_MMAP_LOW_REG(cs) (DRAM_CH0_MMAP_LOW_BASE + (cs) * 0x8)
> +#define DRAM_CS_VALID_ENABLED_MASK 0x1
> +#define DRAM_AREA_LENGTH_OFFS 16
> +#define DRAM_AREA_LENGTH_MASK (0x1f << DRAM_AREA_LENGTH_OFFS)
> +#define DRAM_START_ADDRESS_L_OFFS 23
> +#define DRAM_START_ADDRESS_L_MASK (0x1ff << DRAM_START_ADDRESS_L_OFFS)
> +
> +#define DRAM_CH0_MMAP_HIGH_BASE (DRAM_CONF_BASE + 0x204)
> +#define DRAM_CH0_MMAP_HIGH_REG(cs) (DRAM_CH0_MMAP_HIGH_BASE + (cs) * 0x8)
> +#define DRAM_START_ADDR_HTOL_OFFS 32
> +
> +#define DRAM_MAX_CS_NUM 8
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection
2017-10-11 17:56 ` Leif Lindholm
@ 2017-10-12 5:47 ` Marcin Wojtas
2017-10-12 10:50 ` Leif Lindholm
0 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-12 5:47 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
2017-10-11 19:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
>> Instead of using hardcoded value in PcdSystemMemorySize PCD,
>> obtain DRAM size directly from SoC registers, which are filled
>> by firmware during early initialization stage.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
>> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
>> 2 files changed, 159 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> index 2cb2e15..22cbe47 100644
>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> #include <Library/ArmPlatformLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/HobLib.h>
>> +#include <Library/IoLib.h>
>> #include <Library/MemoryAllocationLib.h>
>>
>> +#include "Armada70x0LibMem.h"
>> +
>> // The total number of descriptors, including the final "end-of-table" descriptor.
>> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
>>
>> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>>
>> +// Obtain DRAM size basing on register values filled by early firmware.
>> +STATIC
>> +UINT64
>> +DramSizeGet (
>
> GetDramSize?
>
>> + UINT64 *MemSize
>
> IN, OUT?
>
2x OK.
>> + )
>> +{
>> + UINT64 BaseAddr;
>> + UINT32 RegVal;
>> + UINT8 AreaLengthMap;
>> + UINT8 Cs;
>> +
>> + *MemSize = 0;
>> +
>> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
>> +
>> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
>> +
>> + /* Exit loop on first disabled DRAM CS */
>> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
>> + break;
>> + }
>> +
>> + /*
>> + * Sanity check for base address of next DRAM block.
>> + * Only continuous space will be used.
>> + */
>> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
>> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
>
> Please use macros, temporary variables, more parentheses or whatever
> to help make the above operation readable.
>
Sure.
>> + if (BaseAddr != *MemSize) {
>> + DEBUG ((DEBUG_ERROR,
>> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
>> + *MemSize));
>> + return EFI_SUCCESS;
>> + }
>> +
>> + /* Decode area length for current CS from register value */
>> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
>> + switch (AreaLengthMap) {
>
> Why Map?
>
I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better.
>> + case 0x0:
>> + *MemSize += 0x18000000;
>> + break;
>> + case 0x1:
>> + *MemSize += 0x30000000;
>> + break;
>> + case 0x2:
>> + *MemSize += 0x60000000;
>> + break;
>> + case 0x3:
>> + *MemSize += 0xC0000000;
>> + break;
>
> The above ones look if not devoid of pattern, at least a bit
> unexpected - and 4-6 are comlpetely missing. Is there any
> documentation available for me to read to try to understand what is
> going on?
Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table,
bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are
reserved (no value possible).
>
> The below all look predictably formatted and possible to calculate
> rather than list one by one.
>
> (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
I think we can reduce the switch-case to this:
if (AreaLength =< 0x4) {
*MemSize = 0x18000000 << AreaLength;
} else if (AreaLength >= 0x7 && AreaLength < 0x1B)
*MemSize = 1 << (AreaLengthMap + 16);
} else {
DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
CS#%d\n", AreaLength, Cs));
}
Is above ok for you?
>
>> + case 0x7:
>> + *MemSize += 0x00800000;
>> + break;
>> + case 0x8:
>> + *MemSize += 0x01000000;
>> + break;
>> + case 0x9:
>> + *MemSize += 0x02000000;
>> + break;
>> + case 0xA:
>> + *MemSize += 0x04000000;
>> + break;
>> + case 0xB:
>> + *MemSize += 0x08000000;
>> + break;
>> + case 0xC:
>> + *MemSize += 0x10000000;
>> + break;
>> + case 0xD:
>> + *MemSize += 0x20000000;
>> + break;
>> + case 0xE:
>> + *MemSize += 0x40000000;
>> + break;
>> + case 0xF:
>> + *MemSize += 0x80000000;
>> + break;
>> + case 0x10:
>> + *MemSize += 0x100000000;
>> + break;
>> + case 0x11:
>> + *MemSize += 0x200000000;
>> + break;
>> + case 0x12:
>> + *MemSize += 0x400000000;
>> + break;
>> + case 0x13:
>> + *MemSize += 0x800000000;
>> + break;
>> + default:
>> + DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs));
>
> Area length isn't really a helpful debug message.
> "memory module size"?
>
Ok.
Thanks,
Marcin
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection
2017-10-12 5:47 ` Marcin Wojtas
@ 2017-10-12 10:50 ` Leif Lindholm
2017-10-12 10:58 ` Marcin Wojtas
0 siblings, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2017-10-12 10:50 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
On Thu, Oct 12, 2017 at 07:47:52AM +0200, Marcin Wojtas wrote:
> 2017-10-11 19:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
> >> Instead of using hardcoded value in PcdSystemMemorySize PCD,
> >> obtain DRAM size directly from SoC registers, which are filled
> >> by firmware during early initialization stage.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
> >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
> >> 2 files changed, 159 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> index 2cb2e15..22cbe47 100644
> >> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >> #include <Library/ArmPlatformLib.h>
> >> #include <Library/DebugLib.h>
> >> #include <Library/HobLib.h>
> >> +#include <Library/IoLib.h>
> >> #include <Library/MemoryAllocationLib.h>
> >>
> >> +#include "Armada70x0LibMem.h"
> >> +
> >> // The total number of descriptors, including the final "end-of-table" descriptor.
> >> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
> >>
> >> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>
> >> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
> >>
> >> +// Obtain DRAM size basing on register values filled by early firmware.
> >> +STATIC
> >> +UINT64
> >> +DramSizeGet (
> >
> > GetDramSize?
> >
> >> + UINT64 *MemSize
> >
> > IN, OUT?
> >
>
> 2x OK.
>
> >> + )
> >> +{
> >> + UINT64 BaseAddr;
> >> + UINT32 RegVal;
> >> + UINT8 AreaLengthMap;
> >> + UINT8 Cs;
> >> +
> >> + *MemSize = 0;
> >> +
> >> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> >> +
> >> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
> >> +
> >> + /* Exit loop on first disabled DRAM CS */
> >> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
> >> + break;
> >> + }
> >> +
> >> + /*
> >> + * Sanity check for base address of next DRAM block.
> >> + * Only continuous space will be used.
> >> + */
> >> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
> >> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
> >
> > Please use macros, temporary variables, more parentheses or whatever
> > to help make the above operation readable.
> >
>
> Sure.
>
> >> + if (BaseAddr != *MemSize) {
> >> + DEBUG ((DEBUG_ERROR,
> >> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> >> + *MemSize));
> >> + return EFI_SUCCESS;
> >> + }
> >> +
> >> + /* Decode area length for current CS from register value */
> >> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
> >> + switch (AreaLengthMap) {
> >
> > Why Map?
> >
>
> I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better.
>
> >> + case 0x0:
> >> + *MemSize += 0x18000000;
> >> + break;
> >> + case 0x1:
> >> + *MemSize += 0x30000000;
> >> + break;
> >> + case 0x2:
> >> + *MemSize += 0x60000000;
> >> + break;
> >> + case 0x3:
> >> + *MemSize += 0xC0000000;
> >> + break;
> >
> > The above ones look if not devoid of pattern, at least a bit
> > unexpected - and 4-6 are comlpetely missing. Is there any
> > documentation available for me to read to try to understand what is
> > going on?
>
> Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table,
> bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are
> reserved (no value possible).
(Where do I find .75G, 1.5G, 3G and 6G DIMMs?)
> >
> > The below all look predictably formatted and possible to calculate
> > rather than list one by one.
> >
> > (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
>
> I think we can reduce the switch-case to this:
>
> if (AreaLength =< 0x4) {
> *MemSize = 0x18000000 << AreaLength;
> } else if (AreaLength >= 0x7 && AreaLength < 0x1B)
> *MemSize = 1 << (AreaLengthMap + 16);
I would prefer for the arithmetic to be hidden away in a macro.
Both cases.
Also, the tests themselves: IS_VALID_xxx_AREA(), IS_VALID_yyy_AREA().
Sorry, can't think of good real names - if the documentation has it,
use that.
> } else {
> DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
> CS#%d\n", AreaLength, Cs));
> }
>
> Is above ok for you?
Yeah, it's a big improvement over listing each individual option.
/
Leif
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection
2017-10-12 10:50 ` Leif Lindholm
@ 2017-10-12 10:58 ` Marcin Wojtas
0 siblings, 0 replies; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-12 10:58 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
2017-10-12 12:50 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>
> On Thu, Oct 12, 2017 at 07:47:52AM +0200, Marcin Wojtas wrote:
> > 2017-10-11 19:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > > On Wed, Oct 11, 2017 at 05:40:47PM +0200, Marcin Wojtas wrote:
> > >> Instead of using hardcoded value in PcdSystemMemorySize PCD,
> > >> obtain DRAM size directly from SoC registers, which are filled
> > >> by firmware during early initialization stage.
> > >>
> > >> Contributed-under: TianoCore Contribution Agreement 1.1
> > >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > >> ---
> > >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 111 +++++++++++++++++++-
> > >> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 49 +++++++++
> > >> 2 files changed, 159 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> > >> index 2cb2e15..22cbe47 100644
> > >> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> > >> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> > >> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > >> #include <Library/ArmPlatformLib.h>
> > >> #include <Library/DebugLib.h>
> > >> #include <Library/HobLib.h>
> > >> +#include <Library/IoLib.h>
> > >> #include <Library/MemoryAllocationLib.h>
> > >>
> > >> +#include "Armada70x0LibMem.h"
> > >> +
> > >> // The total number of descriptors, including the final "end-of-table" descriptor.
> > >> #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
> > >>
> > >> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > >>
> > >> STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
> > >>
> > >> +// Obtain DRAM size basing on register values filled by early firmware.
> > >> +STATIC
> > >> +UINT64
> > >> +DramSizeGet (
> > >
> > > GetDramSize?
> > >
> > >> + UINT64 *MemSize
> > >
> > > IN, OUT?
> > >
> >
> > 2x OK.
> >
> > >> + )
> > >> +{
> > >> + UINT64 BaseAddr;
> > >> + UINT32 RegVal;
> > >> + UINT8 AreaLengthMap;
> > >> + UINT8 Cs;
> > >> +
> > >> + *MemSize = 0;
> > >> +
> > >> + for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> > >> +
> > >> + RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
> > >> +
> > >> + /* Exit loop on first disabled DRAM CS */
> > >> + if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
> > >> + break;
> > >> + }
> > >> +
> > >> + /*
> > >> + * Sanity check for base address of next DRAM block.
> > >> + * Only continuous space will be used.
> > >> + */
> > >> + BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
> > >> + DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
> > >
> > > Please use macros, temporary variables, more parentheses or whatever
> > > to help make the above operation readable.
> > >
> >
> > Sure.
> >
> > >> + if (BaseAddr != *MemSize) {
> > >> + DEBUG ((DEBUG_ERROR,
> > >> + "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> > >> + *MemSize));
> > >> + return EFI_SUCCESS;
> > >> + }
> > >> +
> > >> + /* Decode area length for current CS from register value */
> > >> + AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
> > >> + switch (AreaLengthMap) {
> > >
> > > Why Map?
> > >
> >
> > I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better.
> >
> > >> + case 0x0:
> > >> + *MemSize += 0x18000000;
> > >> + break;
> > >> + case 0x1:
> > >> + *MemSize += 0x30000000;
> > >> + break;
> > >> + case 0x2:
> > >> + *MemSize += 0x60000000;
> > >> + break;
> > >> + case 0x3:
> > >> + *MemSize += 0xC0000000;
> > >> + break;
> > >
> > > The above ones look if not devoid of pattern, at least a bit
> > > unexpected - and 4-6 are comlpetely missing. Is there any
> > > documentation available for me to read to try to understand what is
> > > going on?
> >
> > Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table,
> > bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are
> > reserved (no value possible).
>
> (Where do I find .75G, 1.5G, 3G and 6G DIMMs?)
I guess nowhere, but please bear in mind, there are also custom
modules (Armada-7040-DB has one) and on-board DRAMs - in such
circumstances it's up to the designer.
>
>
> > >
> > > The below all look predictably formatted and possible to calculate
> > > rather than list one by one.
> > >
> > > (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.
> >
> > I think we can reduce the switch-case to this:
> >
> > if (AreaLength =< 0x4) {
> > *MemSize = 0x18000000 << AreaLength;
> > } else if (AreaLength >= 0x7 && AreaLength < 0x1B)
> > *MemSize = 1 << (AreaLengthMap + 16);
>
> I would prefer for the arithmetic to be hidden away in a macro.
> Both cases.
>
> Also, the tests themselves: IS_VALID_xxx_AREA(), IS_VALID_yyy_AREA().
> Sorry, can't think of good real names - if the documentation has it,
> use that.
>
Ok, I'll try to come up with something readable.
> > } else {
> > DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
> > CS#%d\n", AreaLength, Cs));
> > }
> >
> > Is above ok for you?
>
> Yeah, it's a big improvement over listing each individual option.
>
Ok, thanks.
Marcin
^ permalink raw reply [flat|nested] 27+ messages in thread
* [platforms: PATCH 7/8] Marvell/Armada: Armada70x0Lib: Add support for 32-bit ARM
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
` (5 preceding siblings ...)
2017-10-11 15:40 ` [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 17:57 ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support Marcin Wojtas
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Add an ARM implementation of ArmPlatformHelper.S.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S | 77 ++++++++++++++++++++
Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 +
2 files changed, 80 insertions(+)
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
new file mode 100644
index 0000000..21459e5
--- /dev/null
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
@@ -0,0 +1,77 @@
+//Based on ArmPlatformPkg/Library/ArmPlatformLibNull/AArch64/ArmPlatformHelper.S
+//
+// Copyright (c) 2012-2013, ARM Limited. All rights reserved.
+// Copyright (c) 2016, Marvell. All rights reserved.
+// Copyright (c) 2017, 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
+//
+
+#include <AsmMacroIoLib.h>
+#include <Library/ArmLib.h>
+
+#define CCU_MC_BASE 0xF0001700
+#define CCU_MC_RCR_OFFSET 0x0
+#define CCU_MC_RCR_REMAP_EN BIT0
+#define CCU_MC_RCR_REMAP_SIZE(Size) (((Size) - 1) ^ (SIZE_1MB - 1))
+
+#define CCU_MC_RSBR_OFFSET 0x4
+#define CCU_MC_RSBR_SOURCE_BASE(Base) (((Base) >> 20) << 10)
+#define CCU_MC_RTBR_OFFSET 0x8
+#define CCU_MC_RTBR_TARGET_BASE(Base) (((Base) >> 20) << 10)
+
+ASM_FUNC(ArmPlatformPeiBootAction)
+ .if FixedPcdGet64 (PcdSystemMemoryBase) != 0
+ .err PcdSystemMemoryBase should be 0x0 on this platform!
+ .endif
+
+ .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
+ //
+ // Use the low range for UEFI itself. The remaining memory will be mapped
+ // and added to the GCD map later.
+ //
+ ADRL (r0, mSystemMemoryEnd)
+ MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
+ mov r3, #0
+ strd r2, r3, [r0]
+ .endif
+
+ bx lr
+
+//UINTN
+//ArmPlatformGetCorePosition (
+// IN UINTN MpId
+// );
+// With this function: CorePos = (ClusterId * 2) + CoreId
+ASM_FUNC(ArmPlatformGetCorePosition)
+ and r1, r0, #ARM_CORE_MASK
+ and r0, r0, #ARM_CLUSTER_MASK
+ add r0, r1, r0, LSR #7
+ bx lr
+
+//UINTN
+//ArmPlatformGetPrimaryCoreMpId (
+// VOID
+// );
+ASM_FUNC(ArmPlatformGetPrimaryCoreMpId)
+ MOV32 (r0, FixedPcdGet32(PcdArmPrimaryCore))
+ bx lr
+
+//UINTN
+//ArmPlatformIsPrimaryCore (
+// IN UINTN MpId
+// );
+ASM_FUNC(ArmPlatformIsPrimaryCore)
+ MOV32 (r1, FixedPcdGet32(PcdArmPrimaryCoreMask))
+ and r0, r0, r1
+ MOV32 (r1, FixedPcdGet32(PcdArmPrimaryCore))
+ cmp r0, r1
+ moveq r0, #1
+ movne r0, #0
+ bx lr
diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
index 838a670..0dabd4b 100644
--- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
+++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
@@ -60,6 +60,9 @@
[Sources.AArch64]
AArch64/ArmPlatformHelper.S
+[Sources.ARM]
+ ARM/ArmPlatformHelper.S
+
[FixedPcd]
gArmTokenSpaceGuid.PcdSystemMemoryBase
gArmTokenSpaceGuid.PcdSystemMemorySize
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 7/8] Marvell/Armada: Armada70x0Lib: Add support for 32-bit ARM
2017-10-11 15:40 ` [platforms: PATCH 7/8] Marvell/Armada: Armada70x0Lib: Add support for 32-bit ARM Marcin Wojtas
@ 2017-10-11 17:57 ` Leif Lindholm
0 siblings, 0 replies; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 17:57 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Wed, Oct 11, 2017 at 05:40:48PM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Add an ARM implementation of ArmPlatformHelper.S.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S | 77 ++++++++++++++++++++
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf | 3 +
> 2 files changed, 80 insertions(+)
>
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> new file mode 100644
> index 0000000..21459e5
> --- /dev/null
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/ARM/ArmPlatformHelper.S
> @@ -0,0 +1,77 @@
> +//Based on ArmPlatformPkg/Library/ArmPlatformLibNull/AArch64/ArmPlatformHelper.S
> +//
> +// Copyright (c) 2012-2013, ARM Limited. All rights reserved.
> +// Copyright (c) 2016, Marvell. All rights reserved.
> +// Copyright (c) 2017, 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
> +//
> +
> +#include <AsmMacroIoLib.h>
> +#include <Library/ArmLib.h>
> +
> +#define CCU_MC_BASE 0xF0001700
> +#define CCU_MC_RCR_OFFSET 0x0
> +#define CCU_MC_RCR_REMAP_EN BIT0
> +#define CCU_MC_RCR_REMAP_SIZE(Size) (((Size) - 1) ^ (SIZE_1MB - 1))
> +
> +#define CCU_MC_RSBR_OFFSET 0x4
> +#define CCU_MC_RSBR_SOURCE_BASE(Base) (((Base) >> 20) << 10)
> +#define CCU_MC_RTBR_OFFSET 0x8
> +#define CCU_MC_RTBR_TARGET_BASE(Base) (((Base) >> 20) << 10)
> +
> +ASM_FUNC(ArmPlatformPeiBootAction)
> + .if FixedPcdGet64 (PcdSystemMemoryBase) != 0
> + .err PcdSystemMemoryBase should be 0x0 on this platform!
> + .endif
> +
> + .if FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
> + //
> + // Use the low range for UEFI itself. The remaining memory will be mapped
> + // and added to the GCD map later.
> + //
> + ADRL (r0, mSystemMemoryEnd)
> + MOV32 (r2, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> + mov r3, #0
> + strd r2, r3, [r0]
> + .endif
> +
> + bx lr
> +
> +//UINTN
> +//ArmPlatformGetCorePosition (
> +// IN UINTN MpId
> +// );
> +// With this function: CorePos = (ClusterId * 2) + CoreId
> +ASM_FUNC(ArmPlatformGetCorePosition)
> + and r1, r0, #ARM_CORE_MASK
> + and r0, r0, #ARM_CLUSTER_MASK
> + add r0, r1, r0, LSR #7
> + bx lr
> +
> +//UINTN
> +//ArmPlatformGetPrimaryCoreMpId (
> +// VOID
> +// );
> +ASM_FUNC(ArmPlatformGetPrimaryCoreMpId)
> + MOV32 (r0, FixedPcdGet32(PcdArmPrimaryCore))
> + bx lr
> +
> +//UINTN
> +//ArmPlatformIsPrimaryCore (
> +// IN UINTN MpId
> +// );
> +ASM_FUNC(ArmPlatformIsPrimaryCore)
> + MOV32 (r1, FixedPcdGet32(PcdArmPrimaryCoreMask))
> + and r0, r0, r1
> + MOV32 (r1, FixedPcdGet32(PcdArmPrimaryCore))
> + cmp r0, r1
> + moveq r0, #1
> + movne r0, #0
> + bx lr
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> index 838a670..0dabd4b 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> @@ -60,6 +60,9 @@
> [Sources.AArch64]
> AArch64/ArmPlatformHelper.S
>
> +[Sources.ARM]
> + ARM/ArmPlatformHelper.S
> +
> [FixedPcd]
> gArmTokenSpaceGuid.PcdSystemMemoryBase
> gArmTokenSpaceGuid.PcdSystemMemorySize
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
` (6 preceding siblings ...)
2017-10-11 15:40 ` [platforms: PATCH 7/8] Marvell/Armada: Armada70x0Lib: Add support for 32-bit ARM Marcin Wojtas
@ 2017-10-11 15:40 ` Marcin Wojtas
2017-10-11 17:58 ` Leif Lindholm
7 siblings, 1 reply; 27+ messages in thread
From: Marcin Wojtas @ 2017-10-11 15:40 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Update the included components and library classes to make this platform
build for 32-bit ARM.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Armada/Armada.dsc.inc | 3 +--
Platform/Marvell/Armada/Armada70x0.dsc | 4 ++--
Platform/Marvell/Armada/Armada70x0.fdf | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index b0a8240..b9fc384 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -132,7 +132,6 @@
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
-[LibraryClasses.AARCH64]
ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
@@ -362,7 +361,7 @@
# ARM Pcds
gArmTokenSpaceGuid.PcdSystemMemoryBase|0
gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
- gArmTokenSpaceGuid.PcdArmScr|0x531
+ gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|36
# Secure region reservation
gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index 946c93e..0396e8e 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -39,8 +39,8 @@
PLATFORM_GUID = f837e231-cfc7-4f56-9a0f-5b218d746ae3
PLATFORM_VERSION = 0.1
DSC_SPECIFICATION = 0x00010005
- OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME)
- SUPPORTED_ARCHITECTURES = AARCH64
+ OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME)-$(ARCH)
+ SUPPORTED_ARCHITECTURES = AARCH64|ARM
BUILD_TARGETS = DEBUG|RELEASE
SKUID_IDENTIFIER = DEFAULT
FLASH_DEFINITION = Platform/Marvell/Armada/Armada70x0.fdf
diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
index a94a9ff..ec2c368 100644
--- a/Platform/Marvell/Armada/Armada70x0.fdf
+++ b/Platform/Marvell/Armada/Armada70x0.fdf
@@ -237,7 +237,7 @@ READ_LOCK_STATUS = TRUE
#
############################################################################
-[Rule.AARCH64.SEC]
+[Rule.Common.SEC]
FILE SEC = $(NAMED_GUID) RELOCS_STRIPPED FIXED {
TE TE Align = Auto $(INF_OUTPUT)/$(MODULE_NAME).efi
}
--
2.7.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support
2017-10-11 15:40 ` [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support Marcin Wojtas
@ 2017-10-11 17:58 ` Leif Lindholm
2017-10-11 18:20 ` Ard Biesheuvel
0 siblings, 1 reply; 27+ messages in thread
From: Leif Lindholm @ 2017-10-11 17:58 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Wed, Oct 11, 2017 at 05:40:49PM +0200, Marcin Wojtas wrote:
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Update the included components and library classes to make this platform
> build for 32-bit ARM.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Very neat!
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/Marvell/Armada/Armada.dsc.inc | 3 +--
> Platform/Marvell/Armada/Armada70x0.dsc | 4 ++--
> Platform/Marvell/Armada/Armada70x0.fdf | 2 +-
> 3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index b0a8240..b9fc384 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -132,7 +132,6 @@
> CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
> FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
>
> -[LibraryClasses.AARCH64]
> ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
> ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
> ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
> @@ -362,7 +361,7 @@
> # ARM Pcds
> gArmTokenSpaceGuid.PcdSystemMemoryBase|0
> gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
> - gArmTokenSpaceGuid.PcdArmScr|0x531
> + gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|36
>
> # Secure region reservation
> gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 946c93e..0396e8e 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -39,8 +39,8 @@
> PLATFORM_GUID = f837e231-cfc7-4f56-9a0f-5b218d746ae3
> PLATFORM_VERSION = 0.1
> DSC_SPECIFICATION = 0x00010005
> - OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME)
> - SUPPORTED_ARCHITECTURES = AARCH64
> + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME)-$(ARCH)
> + SUPPORTED_ARCHITECTURES = AARCH64|ARM
> BUILD_TARGETS = DEBUG|RELEASE
> SKUID_IDENTIFIER = DEFAULT
> FLASH_DEFINITION = Platform/Marvell/Armada/Armada70x0.fdf
> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
> index a94a9ff..ec2c368 100644
> --- a/Platform/Marvell/Armada/Armada70x0.fdf
> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
> @@ -237,7 +237,7 @@ READ_LOCK_STATUS = TRUE
> #
> ############################################################################
>
> -[Rule.AARCH64.SEC]
> +[Rule.Common.SEC]
> FILE SEC = $(NAMED_GUID) RELOCS_STRIPPED FIXED {
> TE TE Align = Auto $(INF_OUTPUT)/$(MODULE_NAME).efi
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support
2017-10-11 17:58 ` Leif Lindholm
@ 2017-10-11 18:20 ` Ard Biesheuvel
0 siblings, 0 replies; 27+ messages in thread
From: Ard Biesheuvel @ 2017-10-11 18:20 UTC (permalink / raw)
To: Leif Lindholm
Cc: Marcin Wojtas, edk2-devel@lists.01.org, Nadav Haklai,
Neta Zur Hershkovits, Kostya Porotchkin, Hua Jing,
Jan Dąbroś
On 11 October 2017 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Oct 11, 2017 at 05:40:49PM +0200, Marcin Wojtas wrote:
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Update the included components and library classes to make this platform
>> build for 32-bit ARM.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>
> Very neat!
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
There are two caveats though:
- it is up to ARM-TF to enter the non-secure world in the right mode
(EL2 or SVC)
- HYP mode cannot be used, since it mandates long descriptors, while
UEFI mandates short descriptors, so we lose KVM functionality when
booting 32-bit Linux via UEFI.
^ permalink raw reply [flat|nested] 27+ messages in thread