[platforms: PATCH v2 0/4] Armada7k8k memory handling update

[platforms: PATCH v2 0/4] Armada7k8k memory handling update

[Marcin Wojtas]

Hi,

The second version of the patchset introduces the new common
header for Marvell SMC ID's, and answers other remarks
pointed out in v1 review. The details can be found in the
changelog below.

Patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/dram-upstream-r20190122

I'm looking forward to the comments and remarks.

Best regards,
Marcin

Changelog:
v1 -> v2:
* 1/4
  - Improve commit log - mention single area size and new PEI stack base

* 2/4 (new patch)
  - Add common header for Marvell SMC ID's

* 3/4
  - Add function description comment
  - Define and use ARMADA7K8K_AP806_INDEX
  - Change function argument to EFI_PHYSICAL_ADDRESS

* 4/4
  - Move new SMC ID to MvSmc.h
  - Include ArmadaSoCDescLib.h directly (instead indirectly via BoardDesc.h)
  - Remove ARMADA7K8K_AP806_INDEX macro

Grzegorz Jaszczyk (2):
  Marvell/Library: ArmadaSoCDescLib: Add North Bridge description
  Marvell/Armada7k8k: Read DRAM settings from ARM-TF

Marcin Wojtas (2):
  Marvell/Armada7k8k: Shift PEI stack base
  Marvell/Library: Introduce common header for the SMC ID's

 Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc                                  |  4 +-
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf             |  3 +
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h            | 25 --------
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h |  6 ++
 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 28 +++++++++
 Silicon/Marvell/Include/Library/MvSmc.h                                        | 24 ++++++++
 Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h                               |  8 +--
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c            | 60 ++++++--------------
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 34 +++++++++++
 Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c                                | 14 ++---
 10 files changed, 125 insertions(+), 81 deletions(-)
 create mode 100644 Silicon/Marvell/Include/Library/MvSmc.h

-- 
2.7.4

[platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Marcin Wojtas]

Recent changes in the ARM-TF configure its runtime serices region
as protected, hence the hitherto PEI stack base address (0x41F0000)
violated it.

In order to fix this, extend the region which is non-accessible
by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
(0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
Set the PEI stack base address between both images (0x43F0000).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
index eafcd6e..c8c597f 100644
--- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
+++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
@@ -376,12 +376,12 @@
 
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
 
-  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
+  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
   gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
 
   # Secure region reservation
   gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
-  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
+  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
 
   # TRNG
   gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
-- 
2.7.4

[platforms: PATCH v2 2/4] Marvell/Library: Introduce common header for the SMC ID's

[Marcin Wojtas]

Marvell firmware allows to use SiP services other than
for ComPhy handling. In order to avoid spreading the SMC
ID's definitions across many files, introduce common header
for that purpose.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Include/Library/MvSmc.h          | 23 ++++++++++++++++++++
 Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h |  8 +++----
 Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c  | 14 ++++++------
 3 files changed, 33 insertions(+), 12 deletions(-)
 create mode 100644 Silicon/Marvell/Include/Library/MvSmc.h

diff --git a/Silicon/Marvell/Include/Library/MvSmc.h b/Silicon/Marvell/Include/Library/MvSmc.h
new file mode 100644
index 0000000..2d1542a
--- /dev/null
+++ b/Silicon/Marvell/Include/Library/MvSmc.h
@@ -0,0 +1,23 @@
+/**
+*
+*  Copyright (C) 2019, Marvell International Ltd. and its affiliates.
+*
+*  This program and the accompanying materials are licensed and made available
+*  under the terms and conditions of the BSD License which accompanies this
+*  distribution. The full text of the license may be found at
+*  http://opensource.org/licenses/bsd-license.php
+*
+*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+*
+**/
+
+#ifndef __MV_SMC_H__
+#define __MV_SMC_H__
+
+/* Marvell SiP services SMC ID's */
+#define MV_SMC_ID_COMPHY_POWER_ON         0x82000001
+#define MV_SMC_ID_COMPHY_POWER_OFF        0x82000002
+#define MV_SMC_ID_COMPHY_PLL_LOCK         0x82000003
+
+#endif //__MV_SMC_H__
diff --git a/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h b/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
index d156af6..f6ac65b 100644
--- a/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
+++ b/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
@@ -35,16 +35,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef __COMPHY_SIP_SVC_H__
 #define __COMPHY_SIP_SVC_H__
 
+#include <Library/MvSmc.h>
+
 /*
  * All values in this file are defined externally and used
  * for the SerDes configuration via SiP services.
  */
 
-/* Firmware related definitions used for SMC calls */
-#define MV_SIP_COMPHY_POWER_ON      0x82000001
-#define MV_SIP_COMPHY_POWER_OFF     0x82000002
-#define MV_SIP_COMPHY_PLL_LOCK      0x82000003
-
+/* Helper macros for passing ComPhy parameters to the EL3 */
 #define COMPHY_FW_MODE_FORMAT(mode) (mode << 12)
 #define COMPHY_FW_FORMAT(mode, idx, speeds) \
                                     ((mode << 12) | (idx << 8) | (speeds << 2))
diff --git a/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c b/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
index 2abb006..4f85676 100755
--- a/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
+++ b/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
@@ -163,7 +163,7 @@ ComPhySataPowerUp (
 
   ComPhySataMacPowerDown (Desc[ChipId].SoC->AhciBaseAddress);
 
-  Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
+  Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
              ComPhyBase,
              Lane,
              COMPHY_FW_FORMAT (COMPHY_SATA_MODE,
@@ -175,7 +175,7 @@ ComPhySataPowerUp (
 
   ComPhySataPhyPowerUp (Desc[ChipId].SoC->AhciBaseAddress);
 
-  Status = ComPhySmc (MV_SIP_COMPHY_PLL_LOCK,
+  Status = ComPhySmc (MV_SMC_ID_COMPHY_PLL_LOCK,
              ComPhyBase,
              Lane,
              COMPHY_FW_FORMAT (COMPHY_SATA_MODE,
@@ -234,7 +234,7 @@ ComPhyCp110Init (
     case COMPHY_TYPE_PCIE1:
     case COMPHY_TYPE_PCIE2:
     case COMPHY_TYPE_PCIE3:
-      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
+      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
                  PtrChipCfg->ComPhyBaseAddr,
                  Lane,
                  COMPHY_FW_PCIE_FORMAT (PcieWidth,
@@ -269,7 +269,7 @@ ComPhyCp110Init (
       break;
     case COMPHY_TYPE_USB3_HOST0:
     case COMPHY_TYPE_USB3_HOST1:
-      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
+      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
                  PtrChipCfg->ComPhyBaseAddr,
                  Lane,
                  COMPHY_FW_MODE_FORMAT (COMPHY_USB3H_MODE));
@@ -278,7 +278,7 @@ ComPhyCp110Init (
     case COMPHY_TYPE_SGMII1:
     case COMPHY_TYPE_SGMII2:
     case COMPHY_TYPE_SGMII3:
-      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
+      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
                  PtrChipCfg->ComPhyBaseAddr,
                  Lane,
                  COMPHY_FW_FORMAT (COMPHY_SGMII_MODE,
@@ -286,7 +286,7 @@ ComPhyCp110Init (
                    PtrComPhyMap->Speed));
       break;
     case COMPHY_TYPE_SFI:
-      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
+      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
                  PtrChipCfg->ComPhyBaseAddr,
                  Lane,
                  COMPHY_FW_FORMAT (COMPHY_SFI_MODE,
@@ -295,7 +295,7 @@ ComPhyCp110Init (
       break;
     case COMPHY_TYPE_RXAUI0:
     case COMPHY_TYPE_RXAUI1:
-      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
+      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
                  PtrChipCfg->ComPhyBaseAddr,
                  Lane,
                  COMPHY_FW_MODE_FORMAT (COMPHY_RXAUI_MODE));
-- 
2.7.4

[platforms: PATCH v2 3/4] Marvell/Library: ArmadaSoCDescLib: Add North Bridge description

[Marcin Wojtas]

From: Grzegorz Jaszczyk <jaz@semihalf.com>

For upcomming patch there is need to get AP806 base, provide required
getter function for it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h |  6 ++++
 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 28 ++++++++++++++++
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 34 ++++++++++++++++++++
 3 files changed, 68 insertions(+)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
index bfc8639..c2d7933 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
@@ -22,9 +22,15 @@
 // Common macros
 //
 #define MV_SOC_CP_BASE(Cp)               (0xF2000000 + ((Cp) * 0x2000000))
+#define MV_SOC_AP806_BASE                0xF0000000
 #define MV_SOC_AP806_COUNT               1
 
 //
+// Armada7k8k default North Bridge index
+//
+#define ARMADA7K8K_AP806_INDEX           0
+
+//
 // Platform description of AHCI controllers
 //
 #define MV_SOC_AHCI_BASE(Cp)             (MV_SOC_CP_BASE (Cp) + 0x540000)
diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
index 26b075a..fc17c3a 100644
--- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
+++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
@@ -20,6 +20,34 @@
 #include <Protocol/EmbeddedGpio.h>
 
 //
+// North Bridge description
+//
+
+/**
+
+Routine Description:
+
+  Get base address of the SoC North Bridge.
+
+Arguments:
+
+  ApBase  - Base address of the North Bridge.
+  ApIndex - Index of the North Bridge.
+
+Returns:
+
+  EFI_SUCCESS           - Proper base address is returned.
+  EFI_INVALID_PARAMETER - The index is out of range.
+
+**/
+EFI_STATUS
+EFIAPI
+ArmadaSoCAp8xxBaseGet (
+  IN OUT EFI_PHYSICAL_ADDRESS  *ApBase,
+  IN UINTN                      ApIndex
+  );
+
+//
 // ComPhy SoC description
 //
 typedef struct {
diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
index 5b72c20..584f445 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
@@ -28,6 +28,40 @@
 
 #include "Armada7k8kSoCDescLib.h"
 
+/**
+
+Routine Description:
+
+  Get base address of the SoC North Bridge.
+
+Arguments:
+
+  ApBase  - Base address of the North Bridge.
+  ApIndex - Index of the North Bridge.
+
+Returns:
+
+  EFI_SUCCESS           - Proper base address is returned.
+  EFI_INVALID_PARAMETER - The index is out of range.
+
+**/
+EFI_STATUS
+EFIAPI
+ArmadaSoCAp8xxBaseGet (
+  IN OUT EFI_PHYSICAL_ADDRESS  *ApBase,
+  IN UINTN                      ApIndex
+  )
+{
+  if (ApIndex != ARMADA7K8K_AP806_INDEX) {
+    DEBUG ((DEBUG_ERROR, "%a: Only one AP806 in A7K/A8K SoC\n", __FUNCTION__));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *ApBase = MV_SOC_AP806_BASE;
+
+  return EFI_SUCCESS;
+}
+
 EFI_STATUS
 EFIAPI
 ArmadaSoCDescComPhyGet (
-- 
2.7.4

[platforms: PATCH v2 4/4] Marvell/Armada7k8k: Read DRAM settings from ARM-TF

[Marcin Wojtas]

From: Grzegorz Jaszczyk <jaz@semihalf.com>

The memory controller registers are marked as secure in the latest
ARM-TF for Armada SoCs. It is available however get the DRAM
information via SiP services in the EL3, so use it instead
of accessing the registers directly.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf  |  3 +
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h | 25 --------
 Silicon/Marvell/Include/Library/MvSmc.h                             |  1 +
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c | 60 ++++++--------------
 4 files changed, 22 insertions(+), 67 deletions(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
index e888566..0c7f320 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
@@ -41,12 +41,15 @@
 [Packages]
   ArmPkg/ArmPkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
   MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
   Silicon/Marvell/Marvell.dec
 
 [LibraryClasses]
+  ArmadaSoCDescLib
   ArmLib
+  ArmSmcLib
   DebugLib
   MemoryAllocationLib
   MppLib
diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
index cc30e4a..8101cf3 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
@@ -46,28 +46,3 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
           (MmioRead32 (CCU_MC_RCR_REG) & REMAP_SIZE_MASK) + SIZE_1MB
 #define DRAM_REMAP_TARGET \
           (MmioRead32 (CCU_MC_RTBR_REG) << TARGET_BASE_OFFS)
-
-#define DRAM_CH0_MMAP_LOW_REG(cs)       (0xf0020200 + (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_REG(cs)      (0xf0020204 + (cs) * 0x8)
-#define DRAM_START_ADDR_HTOL_OFFS       32
-
-#define DRAM_MAX_CS_NUM                 8
-
-#define DRAM_CS_ENABLED(Cs) \
-          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_CS_VALID_ENABLED_MASK)
-#define GET_DRAM_REGION_BASE(Cs) \
-          ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG ((Cs))) << \
-           DRAM_START_ADDR_HTOL_OFFS) | \
-          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_START_ADDRESS_L_MASK);
-#define GET_DRAM_REGION_SIZE_CODE(Cs) \
-          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG ((Cs))) & \
-           DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS
-#define DRAM_REGION_SIZE_EVEN(C)        (((C) >= 7) && ((C) <= 26))
-#define GET_DRAM_REGION_SIZE_EVEN(C)    ((UINT64)1 << ((C) + 16))
-#define DRAM_REGION_SIZE_ODD(C)         ((C) <= 4)
-#define GET_DRAM_REGION_SIZE_ODD(C)     ((UINT64)0x18000000 << (C))
diff --git a/Silicon/Marvell/Include/Library/MvSmc.h b/Silicon/Marvell/Include/Library/MvSmc.h
index 2d1542a..0c90f11 100644
--- a/Silicon/Marvell/Include/Library/MvSmc.h
+++ b/Silicon/Marvell/Include/Library/MvSmc.h
@@ -19,5 +19,6 @@
 #define MV_SMC_ID_COMPHY_POWER_ON         0x82000001
 #define MV_SMC_ID_COMPHY_POWER_OFF        0x82000002
 #define MV_SMC_ID_COMPHY_PLL_LOCK         0x82000003
+#define MV_SMC_ID_DRAM_SIZE               0x82000010
 
 #endif //__MV_SMC_H__
diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
index 2a4f5ad..8517deb 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
@@ -32,12 +32,18 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 *******************************************************************************/
 
-#include <Base.h>
+#include <Uefi.h>
+
+#include <IndustryStandard/ArmStdSmc.h>
+
+#include <Library/ArmadaSoCDescLib.h>
 #include <Library/ArmPlatformLib.h>
+#include <Library/ArmSmcLib.h>
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/IoLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/MvSmc.h>
 
 #include "Armada7k8kLibMem.h"
 
@@ -57,49 +63,19 @@ GetDramSize (
   IN OUT UINT64 *MemSize
   )
 {
-  UINT64 BaseAddr;
-  UINT8 RegionCode;
-  UINT8 Cs;
-
-  *MemSize = 0;
-
-  for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
-
-    /* Exit loop on first disabled DRAM CS */
-    if (!DRAM_CS_ENABLED (Cs)) {
-      break;
-    }
-
-    /*
-     * Sanity check for base address of next DRAM block.
-     * Only continuous space will be used.
-     */
-    BaseAddr = GET_DRAM_REGION_BASE (Cs);
-    if (BaseAddr != *MemSize) {
-      DEBUG ((DEBUG_ERROR,
-        "%a: DRAM blocks are not contiguous, limit size to 0x%llx\n",
-        __FUNCTION__,
-        *MemSize));
-      return EFI_SUCCESS;
-    }
-
-    /* Decode area length for current CS from register value */
-    RegionCode = GET_DRAM_REGION_SIZE_CODE (Cs);
-
-    if (DRAM_REGION_SIZE_EVEN (RegionCode)) {
-      *MemSize += GET_DRAM_REGION_SIZE_EVEN (RegionCode);
-    } else if (DRAM_REGION_SIZE_ODD (RegionCode)) {
-      *MemSize += GET_DRAM_REGION_SIZE_ODD (RegionCode);
-    } else {
-      DEBUG ((DEBUG_ERROR,
-        "%a: Invalid memory region code (0x%x) for CS#%d\n",
-        __FUNCTION__,
-        RegionCode,
-        Cs));
-      return EFI_INVALID_PARAMETER;
-    }
+  ARM_SMC_ARGS SmcRegs = {0};
+  EFI_STATUS Status;
+
+  SmcRegs.Arg0 = MV_SMC_ID_DRAM_SIZE;
+  Status = ArmadaSoCAp8xxBaseGet (&SmcRegs.Arg1, 0);
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
+  ArmCallSmc (&SmcRegs);
+
+  *MemSize = SmcRegs.Arg0;
+
   return EFI_SUCCESS;
 }
 
-- 
2.7.4

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Leif Lindholm]

On Tue, Jan 22, 2019 at 02:32:19AM +0100, Marcin Wojtas wrote:
> Recent changes in the ARM-TF configure its runtime serices region
> as protected, hence the hitherto PEI stack base address (0x41F0000)
> violated it.
> 
> In order to fix this, extend the region which is non-accessible
> by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
> (0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
> Set the PEI stack base address between both images (0x43F0000).

OK, that is a much better description.
But I'm getting slight cognitive dissonance from placing the PEI stack
inside something we've just claimed belongs to Secure world...

Could you instead break this out into two separate protected regions?
PcdSecureOpteeBase/Size and PcdSecureTfBase/Size?

Alternatively, nudge the stackbase to 0x5400000?

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index eafcd6e..c8c597f 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -376,12 +376,12 @@
>  
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
>  
> -  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
> +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
>    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
>  
>    # Secure region reservation
>    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> -  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> +  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
>  
>    # TRNG
>    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 2/4] Marvell/Library: Introduce common header for the SMC ID's

[Leif Lindholm]

On Tue, Jan 22, 2019 at 02:32:20AM +0100, Marcin Wojtas wrote:
> Marvell firmware allows to use SiP services other than
> for ComPhy handling. In order to avoid spreading the SMC
> ID's definitions across many files, introduce common header
> for that purpose.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Include/Library/MvSmc.h          | 23 ++++++++++++++++++++

Final nitpick: This isn't a library.
IndustryStandard is probably a better approximation. (You are
effectively extending ArmPkg/Include/IndustryStandard/ArmStdSmc.h with
vendor-specific bits.)

With that change:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

>  Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h |  8 +++----
>  Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c  | 14 ++++++------
>  3 files changed, 33 insertions(+), 12 deletions(-)
>  create mode 100644 Silicon/Marvell/Include/Library/MvSmc.h
> 
> diff --git a/Silicon/Marvell/Include/Library/MvSmc.h b/Silicon/Marvell/Include/Library/MvSmc.h
> new file mode 100644
> index 0000000..2d1542a
> --- /dev/null
> +++ b/Silicon/Marvell/Include/Library/MvSmc.h
> @@ -0,0 +1,23 @@
> +/**
> +*
> +*  Copyright (C) 2019, Marvell International Ltd. and its affiliates.
> +*
> +*  This program and the accompanying materials are licensed and made available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#ifndef __MV_SMC_H__
> +#define __MV_SMC_H__
> +
> +/* Marvell SiP services SMC ID's */
> +#define MV_SMC_ID_COMPHY_POWER_ON         0x82000001
> +#define MV_SMC_ID_COMPHY_POWER_OFF        0x82000002
> +#define MV_SMC_ID_COMPHY_PLL_LOCK         0x82000003
> +
> +#endif //__MV_SMC_H__
> diff --git a/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h b/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
> index d156af6..f6ac65b 100644
> --- a/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
> +++ b/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
> @@ -35,16 +35,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #ifndef __COMPHY_SIP_SVC_H__
>  #define __COMPHY_SIP_SVC_H__
>  
> +#include <Library/MvSmc.h>
> +
>  /*
>   * All values in this file are defined externally and used
>   * for the SerDes configuration via SiP services.
>   */
>  
> -/* Firmware related definitions used for SMC calls */
> -#define MV_SIP_COMPHY_POWER_ON      0x82000001
> -#define MV_SIP_COMPHY_POWER_OFF     0x82000002
> -#define MV_SIP_COMPHY_PLL_LOCK      0x82000003
> -
> +/* Helper macros for passing ComPhy parameters to the EL3 */
>  #define COMPHY_FW_MODE_FORMAT(mode) (mode << 12)
>  #define COMPHY_FW_FORMAT(mode, idx, speeds) \
>                                      ((mode << 12) | (idx << 8) | (speeds << 2))
> diff --git a/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c b/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
> index 2abb006..4f85676 100755
> --- a/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
> +++ b/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
> @@ -163,7 +163,7 @@ ComPhySataPowerUp (
>  
>    ComPhySataMacPowerDown (Desc[ChipId].SoC->AhciBaseAddress);
>  
> -  Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> +  Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
>               ComPhyBase,
>               Lane,
>               COMPHY_FW_FORMAT (COMPHY_SATA_MODE,
> @@ -175,7 +175,7 @@ ComPhySataPowerUp (
>  
>    ComPhySataPhyPowerUp (Desc[ChipId].SoC->AhciBaseAddress);
>  
> -  Status = ComPhySmc (MV_SIP_COMPHY_PLL_LOCK,
> +  Status = ComPhySmc (MV_SMC_ID_COMPHY_PLL_LOCK,
>               ComPhyBase,
>               Lane,
>               COMPHY_FW_FORMAT (COMPHY_SATA_MODE,
> @@ -234,7 +234,7 @@ ComPhyCp110Init (
>      case COMPHY_TYPE_PCIE1:
>      case COMPHY_TYPE_PCIE2:
>      case COMPHY_TYPE_PCIE3:
> -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
>                   PtrChipCfg->ComPhyBaseAddr,
>                   Lane,
>                   COMPHY_FW_PCIE_FORMAT (PcieWidth,
> @@ -269,7 +269,7 @@ ComPhyCp110Init (
>        break;
>      case COMPHY_TYPE_USB3_HOST0:
>      case COMPHY_TYPE_USB3_HOST1:
> -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
>                   PtrChipCfg->ComPhyBaseAddr,
>                   Lane,
>                   COMPHY_FW_MODE_FORMAT (COMPHY_USB3H_MODE));
> @@ -278,7 +278,7 @@ ComPhyCp110Init (
>      case COMPHY_TYPE_SGMII1:
>      case COMPHY_TYPE_SGMII2:
>      case COMPHY_TYPE_SGMII3:
> -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
>                   PtrChipCfg->ComPhyBaseAddr,
>                   Lane,
>                   COMPHY_FW_FORMAT (COMPHY_SGMII_MODE,
> @@ -286,7 +286,7 @@ ComPhyCp110Init (
>                     PtrComPhyMap->Speed));
>        break;
>      case COMPHY_TYPE_SFI:
> -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
>                   PtrChipCfg->ComPhyBaseAddr,
>                   Lane,
>                   COMPHY_FW_FORMAT (COMPHY_SFI_MODE,
> @@ -295,7 +295,7 @@ ComPhyCp110Init (
>        break;
>      case COMPHY_TYPE_RXAUI0:
>      case COMPHY_TYPE_RXAUI1:
> -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
>                   PtrChipCfg->ComPhyBaseAddr,
>                   Lane,
>                   COMPHY_FW_MODE_FORMAT (COMPHY_RXAUI_MODE));
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 3/4] Marvell/Library: ArmadaSoCDescLib: Add North Bridge description

[Leif Lindholm]

On Tue, Jan 22, 2019 at 02:32:21AM +0100, Marcin Wojtas wrote:
> From: Grzegorz Jaszczyk <jaz@semihalf.com>
> 
> For upcomming patch there is need to get AP806 base, provide required
> getter function for it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h |  6 ++++
>  Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 28 ++++++++++++++++
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 34 ++++++++++++++++++++
>  3 files changed, 68 insertions(+)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> index bfc8639..c2d7933 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> @@ -22,9 +22,15 @@
>  // Common macros
>  //
>  #define MV_SOC_CP_BASE(Cp)               (0xF2000000 + ((Cp) * 0x2000000))
> +#define MV_SOC_AP806_BASE                0xF0000000
>  #define MV_SOC_AP806_COUNT               1
>  
>  //
> +// Armada7k8k default North Bridge index
> +//
> +#define ARMADA7K8K_AP806_INDEX           0
> +
> +//
>  // Platform description of AHCI controllers
>  //
>  #define MV_SOC_AHCI_BASE(Cp)             (MV_SOC_CP_BASE (Cp) + 0x540000)
> diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> index 26b075a..fc17c3a 100644
> --- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> +++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> @@ -20,6 +20,34 @@
>  #include <Protocol/EmbeddedGpio.h>
>  
>  //
> +// North Bridge description
> +//
> +
> +/**
> +
> +Routine Description:
> +
> +  Get base address of the SoC North Bridge.
> +
> +Arguments:
> +
> +  ApBase  - Base address of the North Bridge.
> +  ApIndex - Index of the North Bridge.
> +
> +Returns:
> +
> +  EFI_SUCCESS           - Proper base address is returned.
> +  EFI_INVALID_PARAMETER - The index is out of range.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +ArmadaSoCAp8xxBaseGet (
> +  IN OUT EFI_PHYSICAL_ADDRESS  *ApBase,
> +  IN UINTN                      ApIndex
> +  );
> +
> +//
>  // ComPhy SoC description
>  //
>  typedef struct {
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> index 5b72c20..584f445 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> @@ -28,6 +28,40 @@
>  
>  #include "Armada7k8kSoCDescLib.h"
>  
> +/**
> +
> +Routine Description:
> +
> +  Get base address of the SoC North Bridge.
> +
> +Arguments:
> +
> +  ApBase  - Base address of the North Bridge.
> +  ApIndex - Index of the North Bridge.
> +
> +Returns:
> +
> +  EFI_SUCCESS           - Proper base address is returned.
> +  EFI_INVALID_PARAMETER - The index is out of range.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +ArmadaSoCAp8xxBaseGet (
> +  IN OUT EFI_PHYSICAL_ADDRESS  *ApBase,
> +  IN UINTN                      ApIndex
> +  )
> +{
> +  if (ApIndex != ARMADA7K8K_AP806_INDEX) {
> +    DEBUG ((DEBUG_ERROR, "%a: Only one AP806 in A7K/A8K SoC\n", __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *ApBase = MV_SOC_AP806_BASE;
> +
> +  return EFI_SUCCESS;
> +}
> +
>  EFI_STATUS
>  EFIAPI
>  ArmadaSoCDescComPhyGet (
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 4/4] Marvell/Armada7k8k: Read DRAM settings from ARM-TF

[Leif Lindholm]

On Tue, Jan 22, 2019 at 02:32:22AM +0100, Marcin Wojtas wrote:
> From: Grzegorz Jaszczyk <jaz@semihalf.com>
> 
> The memory controller registers are marked as secure in the latest
> ARM-TF for Armada SoCs. It is available however get the DRAM
> information via SiP services in the EL3, so use it instead
> of accessing the registers directly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

(With the required include path change based on feedback on other patch:)
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf  |  3 +
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h | 25 --------
>  Silicon/Marvell/Include/Library/MvSmc.h                             |  1 +
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c | 60 ++++++--------------
>  4 files changed, 22 insertions(+), 67 deletions(-)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
> index e888566..0c7f320 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
> @@ -41,12 +41,15 @@
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmPlatformPkg/ArmPlatformPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    Silicon/Marvell/Marvell.dec
>  
>  [LibraryClasses]
> +  ArmadaSoCDescLib
>    ArmLib
> +  ArmSmcLib
>    DebugLib
>    MemoryAllocationLib
>    MppLib
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> index cc30e4a..8101cf3 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> @@ -46,28 +46,3 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>            (MmioRead32 (CCU_MC_RCR_REG) & REMAP_SIZE_MASK) + SIZE_1MB
>  #define DRAM_REMAP_TARGET \
>            (MmioRead32 (CCU_MC_RTBR_REG) << TARGET_BASE_OFFS)
> -
> -#define DRAM_CH0_MMAP_LOW_REG(cs)       (0xf0020200 + (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_REG(cs)      (0xf0020204 + (cs) * 0x8)
> -#define DRAM_START_ADDR_HTOL_OFFS       32
> -
> -#define DRAM_MAX_CS_NUM                 8
> -
> -#define DRAM_CS_ENABLED(Cs) \
> -          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_CS_VALID_ENABLED_MASK)
> -#define GET_DRAM_REGION_BASE(Cs) \
> -          ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG ((Cs))) << \
> -           DRAM_START_ADDR_HTOL_OFFS) | \
> -          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs)) & DRAM_START_ADDRESS_L_MASK);
> -#define GET_DRAM_REGION_SIZE_CODE(Cs) \
> -          (MmioRead32 (DRAM_CH0_MMAP_LOW_REG ((Cs))) & \
> -           DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS
> -#define DRAM_REGION_SIZE_EVEN(C)        (((C) >= 7) && ((C) <= 26))
> -#define GET_DRAM_REGION_SIZE_EVEN(C)    ((UINT64)1 << ((C) + 16))
> -#define DRAM_REGION_SIZE_ODD(C)         ((C) <= 4)
> -#define GET_DRAM_REGION_SIZE_ODD(C)     ((UINT64)0x18000000 << (C))
> diff --git a/Silicon/Marvell/Include/Library/MvSmc.h b/Silicon/Marvell/Include/Library/MvSmc.h
> index 2d1542a..0c90f11 100644
> --- a/Silicon/Marvell/Include/Library/MvSmc.h
> +++ b/Silicon/Marvell/Include/Library/MvSmc.h
> @@ -19,5 +19,6 @@
>  #define MV_SMC_ID_COMPHY_POWER_ON         0x82000001
>  #define MV_SMC_ID_COMPHY_POWER_OFF        0x82000002
>  #define MV_SMC_ID_COMPHY_PLL_LOCK         0x82000003
> +#define MV_SMC_ID_DRAM_SIZE               0x82000010
>  
>  #endif //__MV_SMC_H__
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> index 2a4f5ad..8517deb 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> @@ -32,12 +32,18 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  *******************************************************************************/
>  
> -#include <Base.h>
> +#include <Uefi.h>
> +
> +#include <IndustryStandard/ArmStdSmc.h>
> +
> +#include <Library/ArmadaSoCDescLib.h>
>  #include <Library/ArmPlatformLib.h>
> +#include <Library/ArmSmcLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
>  #include <Library/IoLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/MvSmc.h>
>  
>  #include "Armada7k8kLibMem.h"
>  
> @@ -57,49 +63,19 @@ GetDramSize (
>    IN OUT UINT64 *MemSize
>    )
>  {
> -  UINT64 BaseAddr;
> -  UINT8 RegionCode;
> -  UINT8 Cs;
> -
> -  *MemSize = 0;
> -
> -  for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
> -
> -    /* Exit loop on first disabled DRAM CS */
> -    if (!DRAM_CS_ENABLED (Cs)) {
> -      break;
> -    }
> -
> -    /*
> -     * Sanity check for base address of next DRAM block.
> -     * Only continuous space will be used.
> -     */
> -    BaseAddr = GET_DRAM_REGION_BASE (Cs);
> -    if (BaseAddr != *MemSize) {
> -      DEBUG ((DEBUG_ERROR,
> -        "%a: DRAM blocks are not contiguous, limit size to 0x%llx\n",
> -        __FUNCTION__,
> -        *MemSize));
> -      return EFI_SUCCESS;
> -    }
> -
> -    /* Decode area length for current CS from register value */
> -    RegionCode = GET_DRAM_REGION_SIZE_CODE (Cs);
> -
> -    if (DRAM_REGION_SIZE_EVEN (RegionCode)) {
> -      *MemSize += GET_DRAM_REGION_SIZE_EVEN (RegionCode);
> -    } else if (DRAM_REGION_SIZE_ODD (RegionCode)) {
> -      *MemSize += GET_DRAM_REGION_SIZE_ODD (RegionCode);
> -    } else {
> -      DEBUG ((DEBUG_ERROR,
> -        "%a: Invalid memory region code (0x%x) for CS#%d\n",
> -        __FUNCTION__,
> -        RegionCode,
> -        Cs));
> -      return EFI_INVALID_PARAMETER;
> -    }
> +  ARM_SMC_ARGS SmcRegs = {0};
> +  EFI_STATUS Status;
> +
> +  SmcRegs.Arg0 = MV_SMC_ID_DRAM_SIZE;
> +  Status = ArmadaSoCAp8xxBaseGet (&SmcRegs.Arg1, 0);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
>    }
>  
> +  ArmCallSmc (&SmcRegs);
> +
> +  *MemSize = SmcRegs.Arg0;
> +
>    return EFI_SUCCESS;
>  }
>  
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 2/4] Marvell/Library: Introduce common header for the SMC ID's

[Marcin Wojtas]

Hi Leif,

wt., 22 sty 2019 o 18:36 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Tue, Jan 22, 2019 at 02:32:20AM +0100, Marcin Wojtas wrote:
> > Marvell firmware allows to use SiP services other than
> > for ComPhy handling. In order to avoid spreading the SMC
> > ID's definitions across many files, introduce common header
> > for that purpose.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Include/Library/MvSmc.h          | 23 ++++++++++++++++++++
>
> Final nitpick: This isn't a library.
> IndustryStandard is probably a better approximation. (You are
> effectively extending ArmPkg/Include/IndustryStandard/ArmStdSmc.h with
> vendor-specific bits.)
>

Ok, I will move MvSmc.h to such location.

Thanks,
Marcin

> With that change:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> /
>     Leif
>
> >  Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h |  8 +++----
> >  Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c  | 14 ++++++------
> >  3 files changed, 33 insertions(+), 12 deletions(-)
> >  create mode 100644 Silicon/Marvell/Include/Library/MvSmc.h
> >
> > diff --git a/Silicon/Marvell/Include/Library/MvSmc.h b/Silicon/Marvell/Include/Library/MvSmc.h
> > new file mode 100644
> > index 0000000..2d1542a
> > --- /dev/null
> > +++ b/Silicon/Marvell/Include/Library/MvSmc.h
> > @@ -0,0 +1,23 @@
> > +/**
> > +*
> > +*  Copyright (C) 2019, Marvell International Ltd. and its affiliates.
> > +*
> > +*  This program and the accompanying materials are licensed and made available
> > +*  under the terms and conditions of the BSD License which accompanies this
> > +*  distribution. The full text of the license may be found at
> > +*  http://opensource.org/licenses/bsd-license.php
> > +*
> > +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +*
> > +**/
> > +
> > +#ifndef __MV_SMC_H__
> > +#define __MV_SMC_H__
> > +
> > +/* Marvell SiP services SMC ID's */
> > +#define MV_SMC_ID_COMPHY_POWER_ON         0x82000001
> > +#define MV_SMC_ID_COMPHY_POWER_OFF        0x82000002
> > +#define MV_SMC_ID_COMPHY_PLL_LOCK         0x82000003
> > +
> > +#endif //__MV_SMC_H__
> > diff --git a/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h b/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
> > index d156af6..f6ac65b 100644
> > --- a/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
> > +++ b/Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h
> > @@ -35,16 +35,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  #ifndef __COMPHY_SIP_SVC_H__
> >  #define __COMPHY_SIP_SVC_H__
> >
> > +#include <Library/MvSmc.h>
> > +
> >  /*
> >   * All values in this file are defined externally and used
> >   * for the SerDes configuration via SiP services.
> >   */
> >
> > -/* Firmware related definitions used for SMC calls */
> > -#define MV_SIP_COMPHY_POWER_ON      0x82000001
> > -#define MV_SIP_COMPHY_POWER_OFF     0x82000002
> > -#define MV_SIP_COMPHY_PLL_LOCK      0x82000003
> > -
> > +/* Helper macros for passing ComPhy parameters to the EL3 */
> >  #define COMPHY_FW_MODE_FORMAT(mode) (mode << 12)
> >  #define COMPHY_FW_FORMAT(mode, idx, speeds) \
> >                                      ((mode << 12) | (idx << 8) | (speeds << 2))
> > diff --git a/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c b/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
> > index 2abb006..4f85676 100755
> > --- a/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
> > +++ b/Silicon/Marvell/Library/ComPhyLib/ComPhyCp110.c
> > @@ -163,7 +163,7 @@ ComPhySataPowerUp (
> >
> >    ComPhySataMacPowerDown (Desc[ChipId].SoC->AhciBaseAddress);
> >
> > -  Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> > +  Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
> >               ComPhyBase,
> >               Lane,
> >               COMPHY_FW_FORMAT (COMPHY_SATA_MODE,
> > @@ -175,7 +175,7 @@ ComPhySataPowerUp (
> >
> >    ComPhySataPhyPowerUp (Desc[ChipId].SoC->AhciBaseAddress);
> >
> > -  Status = ComPhySmc (MV_SIP_COMPHY_PLL_LOCK,
> > +  Status = ComPhySmc (MV_SMC_ID_COMPHY_PLL_LOCK,
> >               ComPhyBase,
> >               Lane,
> >               COMPHY_FW_FORMAT (COMPHY_SATA_MODE,
> > @@ -234,7 +234,7 @@ ComPhyCp110Init (
> >      case COMPHY_TYPE_PCIE1:
> >      case COMPHY_TYPE_PCIE2:
> >      case COMPHY_TYPE_PCIE3:
> > -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> > +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
> >                   PtrChipCfg->ComPhyBaseAddr,
> >                   Lane,
> >                   COMPHY_FW_PCIE_FORMAT (PcieWidth,
> > @@ -269,7 +269,7 @@ ComPhyCp110Init (
> >        break;
> >      case COMPHY_TYPE_USB3_HOST0:
> >      case COMPHY_TYPE_USB3_HOST1:
> > -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> > +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
> >                   PtrChipCfg->ComPhyBaseAddr,
> >                   Lane,
> >                   COMPHY_FW_MODE_FORMAT (COMPHY_USB3H_MODE));
> > @@ -278,7 +278,7 @@ ComPhyCp110Init (
> >      case COMPHY_TYPE_SGMII1:
> >      case COMPHY_TYPE_SGMII2:
> >      case COMPHY_TYPE_SGMII3:
> > -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> > +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
> >                   PtrChipCfg->ComPhyBaseAddr,
> >                   Lane,
> >                   COMPHY_FW_FORMAT (COMPHY_SGMII_MODE,
> > @@ -286,7 +286,7 @@ ComPhyCp110Init (
> >                     PtrComPhyMap->Speed));
> >        break;
> >      case COMPHY_TYPE_SFI:
> > -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> > +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
> >                   PtrChipCfg->ComPhyBaseAddr,
> >                   Lane,
> >                   COMPHY_FW_FORMAT (COMPHY_SFI_MODE,
> > @@ -295,7 +295,7 @@ ComPhyCp110Init (
> >        break;
> >      case COMPHY_TYPE_RXAUI0:
> >      case COMPHY_TYPE_RXAUI1:
> > -      Status = ComPhySmc (MV_SIP_COMPHY_POWER_ON,
> > +      Status = ComPhySmc (MV_SMC_ID_COMPHY_POWER_ON,
> >                   PtrChipCfg->ComPhyBaseAddr,
> >                   Lane,
> >                   COMPHY_FW_MODE_FORMAT (COMPHY_RXAUI_MODE));
> > --
> > 2.7.4
> >

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Marcin Wojtas]

Hi Leif,

wt., 22 sty 2019 o 18:26 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Tue, Jan 22, 2019 at 02:32:19AM +0100, Marcin Wojtas wrote:
> > Recent changes in the ARM-TF configure its runtime serices region
> > as protected, hence the hitherto PEI stack base address (0x41F0000)
> > violated it.
> >
> > In order to fix this, extend the region which is non-accessible
> > by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
> > (0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
> > Set the PEI stack base address between both images (0x43F0000).
>
> OK, that is a much better description.
> But I'm getting slight cognitive dissonance from placing the PEI stack
> inside something we've just claimed belongs to Secure world...
>
> Could you instead break this out into two separate protected regions?
> PcdSecureOpteeBase/Size and PcdSecureTfBase/Size?
>
> Alternatively, nudge the stackbase to 0x5400000?

As discussed some time ago with Ard, when the PEI stack base was
introduced, it is recommended that this stack is placed in the
location, which is not accessible by OS. Most preferred is to have it
in the SRAM (cannot do it on Armada7k8k) or in a reserved region - cut
out from the memory map passed to the OS.

Currently we have a single region (a "hole") that covers:
2MB for EL3 runtime services
2MB of nothing
16MB for OPTEE image

The 2MB space between images IMO seems perfect for PEI stack to place.
If it was placed e.g. @0x5400000 and we kept the reserved regions
separate, the outcome would be:
2MB for EL3 runtime services
2MB of DRAM normal memory
16MB + 64kB for Optee and PEI stack base.

This is the reason, I'd like to keep original setting, proposed in the
patch. Please let know your opinion.

Best regards,
Marcin


>
> /
>     Leif
>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > index eafcd6e..c8c597f 100644
> > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > @@ -376,12 +376,12 @@
> >
> >    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
> >
> > -  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
> > +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
> >    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
> >
> >    # Secure region reservation
> >    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> > -  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> > +  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
> >
> >    # TRNG
> >    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> > --
> > 2.7.4
> >

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Leif Lindholm]

On Tue, Jan 22, 2019 at 07:26:58PM +0100, Marcin Wojtas wrote:
> Hi Leif,
> 
> wt., 22 sty 2019 o 18:26 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> >
> > On Tue, Jan 22, 2019 at 02:32:19AM +0100, Marcin Wojtas wrote:
> > > Recent changes in the ARM-TF configure its runtime serices region
> > > as protected, hence the hitherto PEI stack base address (0x41F0000)
> > > violated it.
> > >
> > > In order to fix this, extend the region which is non-accessible
> > > by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
> > > (0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
> > > Set the PEI stack base address between both images (0x43F0000).
> >
> > OK, that is a much better description.
> > But I'm getting slight cognitive dissonance from placing the PEI stack
> > inside something we've just claimed belongs to Secure world...
> >
> > Could you instead break this out into two separate protected regions?
> > PcdSecureOpteeBase/Size and PcdSecureTfBase/Size?
> >
> > Alternatively, nudge the stackbase to 0x5400000?
> 
> As discussed some time ago with Ard, when the PEI stack base was
> introduced, it is recommended that this stack is placed in the
> location, which is not accessible by OS. Most preferred is to have it
> in the SRAM (cannot do it on Armada7k8k) or in a reserved region - cut
> out from the memory map passed to the OS.
> 
> Currently we have a single region (a "hole") that covers:
> 2MB for EL3 runtime services
> 2MB of nothing
> 16MB for OPTEE image
> 
> The 2MB space between images IMO seems perfect for PEI stack to place.
> If it was placed e.g. @0x5400000 and we kept the reserved regions
> separate, the outcome would be:
> 2MB for EL3 runtime services
> 2MB of DRAM normal memory
> 16MB + 64kB for Optee and PEI stack base.
> 
> This is the reason, I'd like to keep original setting, proposed in the
> patch. Please let know your opinion.

I have no issue with the placement of the PEI stack between the ARM-TF
region and the Op-TEE region. I _have_ an issue with the PEI stack
being placed between PcdSecureRegionBase and (PcdSecureRegionBase +
PcdSecureRegionSize). I.e. something that we describe as "the Secure
region".

I think I gave my suggestion for the resolution of this problem (with
moving StackBase to 0x05400000 as the alternative) in my previous
reply.

Best Regards,

Leif

> 
> Best regards,
> Marcin
> 
> 
> >
> > /
> >     Leif
> >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > ---
> > >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > index eafcd6e..c8c597f 100644
> > > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > @@ -376,12 +376,12 @@
> > >
> > >    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
> > >
> > > -  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
> > > +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
> > >    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
> > >
> > >    # Secure region reservation
> > >    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> > > -  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> > > +  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
> > >
> > >    # TRNG
> > >    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> > > --
> > > 2.7.4
> > >

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Marcin Wojtas]

Hi Leif,

wt., 22 sty 2019 o 20:06 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Tue, Jan 22, 2019 at 07:26:58PM +0100, Marcin Wojtas wrote:
> > Hi Leif,
> >
> > wt., 22 sty 2019 o 18:26 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> > >
> > > On Tue, Jan 22, 2019 at 02:32:19AM +0100, Marcin Wojtas wrote:
> > > > Recent changes in the ARM-TF configure its runtime serices region
> > > > as protected, hence the hitherto PEI stack base address (0x41F0000)
> > > > violated it.
> > > >
> > > > In order to fix this, extend the region which is non-accessible
> > > > by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
> > > > (0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
> > > > Set the PEI stack base address between both images (0x43F0000).
> > >
> > > OK, that is a much better description.
> > > But I'm getting slight cognitive dissonance from placing the PEI stack
> > > inside something we've just claimed belongs to Secure world...
> > >
> > > Could you instead break this out into two separate protected regions?
> > > PcdSecureOpteeBase/Size and PcdSecureTfBase/Size?
> > >
> > > Alternatively, nudge the stackbase to 0x5400000?
> >
> > As discussed some time ago with Ard, when the PEI stack base was
> > introduced, it is recommended that this stack is placed in the
> > location, which is not accessible by OS. Most preferred is to have it
> > in the SRAM (cannot do it on Armada7k8k) or in a reserved region - cut
> > out from the memory map passed to the OS.
> >
> > Currently we have a single region (a "hole") that covers:
> > 2MB for EL3 runtime services
> > 2MB of nothing
> > 16MB for OPTEE image
> >
> > The 2MB space between images IMO seems perfect for PEI stack to place.
> > If it was placed e.g. @0x5400000 and we kept the reserved regions
> > separate, the outcome would be:
> > 2MB for EL3 runtime services
> > 2MB of DRAM normal memory
> > 16MB + 64kB for Optee and PEI stack base.
> >
> > This is the reason, I'd like to keep original setting, proposed in the
> > patch. Please let know your opinion.
>
> I have no issue with the placement of the PEI stack between the ARM-TF
> region and the Op-TEE region. I _have_ an issue with the PEI stack
> being placed between PcdSecureRegionBase and (PcdSecureRegionBase +
> PcdSecureRegionSize). I.e. something that we describe as "the Secure
> region".
>
> I think I gave my suggestion for the resolution of this problem (with
> moving StackBase to 0x05400000 as the alternative) in my previous
> reply.
>

Yes, and I answered, presenting the alternative memory map with
additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
not fancy, given available space inside the 20MB chunk.

Because in fact this region is not entirely secure (EL3 runtime
services are exectued in NS context for example), how about I:
- rename the PCD's to be more generic (e.g.
gMarvellTokenSpaceGuid.PcdReservedRegionBase)
- add proper comment in Armada7k8k.dsc.inc for the default reserved
memory (+ maybe in Armada7k8kLib, where the PCD's are used)
?

Best regards,
Marcin

> >
> > Best regards,
> > Marcin
> >
> >
> > >
> > > /
> > >     Leif
> > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > > ---
> > > >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > index eafcd6e..c8c597f 100644
> > > > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > @@ -376,12 +376,12 @@
> > > >
> > > >    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
> > > >
> > > > -  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
> > > > +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
> > > >    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
> > > >
> > > >    # Secure region reservation
> > > >    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> > > > -  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> > > > +  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
> > > >
> > > >    # TRNG
> > > >    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> > > > --
> > > > 2.7.4
> > > >

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Leif Lindholm]

On Tue, Jan 22, 2019 at 08:27:10PM +0100, Marcin Wojtas wrote:
> > > > > In order to fix this, extend the region which is non-accessible
> > > > > by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
> > > > > (0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
> > > > > Set the PEI stack base address between both images (0x43F0000).
> > > >
> > > > OK, that is a much better description.
> > > > But I'm getting slight cognitive dissonance from placing the PEI stack
> > > > inside something we've just claimed belongs to Secure world...
> > > >
> > > > Could you instead break this out into two separate protected regions?
> > > > PcdSecureOpteeBase/Size and PcdSecureTfBase/Size?
> > > >
> > > > Alternatively, nudge the stackbase to 0x5400000?
> > >
> > > As discussed some time ago with Ard, when the PEI stack base was
> > > introduced, it is recommended that this stack is placed in the
> > > location, which is not accessible by OS. Most preferred is to have it
> > > in the SRAM (cannot do it on Armada7k8k) or in a reserved region - cut
> > > out from the memory map passed to the OS.
> > >
> > > Currently we have a single region (a "hole") that covers:
> > > 2MB for EL3 runtime services
> > > 2MB of nothing
> > > 16MB for OPTEE image
> > >
> > > The 2MB space between images IMO seems perfect for PEI stack to place.
> > > If it was placed e.g. @0x5400000 and we kept the reserved regions
> > > separate, the outcome would be:
> > > 2MB for EL3 runtime services
> > > 2MB of DRAM normal memory
> > > 16MB + 64kB for Optee and PEI stack base.
> > >
> > > This is the reason, I'd like to keep original setting, proposed in the
> > > patch. Please let know your opinion.
> >
> > I have no issue with the placement of the PEI stack between the ARM-TF
> > region and the Op-TEE region. I _have_ an issue with the PEI stack
> > being placed between PcdSecureRegionBase and (PcdSecureRegionBase +
> > PcdSecureRegionSize). I.e. something that we describe as "the Secure
> > region".
> >
> > I think I gave my suggestion for the resolution of this problem (with
> > moving StackBase to 0x05400000 as the alternative) in my previous
> > reply.
> >
> 
> Yes, and I answered, presenting the alternative memory map with
> additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
> not fancy, given available space inside the 20MB chunk.

Please go back and reread my first and my second email.
Then please point out where I have, other than as an alternative
solution, suggested growing the cutout size.

Then perhaps we can rewind this conversation and try again?

Best Regards,

Leif

> Because in fact this region is not entirely secure (EL3 runtime
> services are exectued in NS context for example), how about I:
> - rename the PCD's to be more generic (e.g.
> gMarvellTokenSpaceGuid.PcdReservedRegionBase)
> - add proper comment in Armada7k8k.dsc.inc for the default reserved
> memory (+ maybe in Armada7k8kLib, where the PCD's are used)
> ?
> 
> Best regards,
> Marcin
> 
> > >
> > > Best regards,
> > > Marcin
> > >
> > >
> > > >
> > > > /
> > > >     Leif
> > > >
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > > > ---
> > > > >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > index eafcd6e..c8c597f 100644
> > > > > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > @@ -376,12 +376,12 @@
> > > > >
> > > > >    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
> > > > >
> > > > > -  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
> > > > > +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
> > > > >    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
> > > > >
> > > > >    # Secure region reservation
> > > > >    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> > > > > -  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> > > > > +  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
> > > > >
> > > > >    # TRNG
> > > > >    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> > > > > --
> > > > > 2.7.4
> > > > >

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Marcin Wojtas]

Hi Leif,

wt., 22 sty 2019 o 21:26 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Tue, Jan 22, 2019 at 08:27:10PM +0100, Marcin Wojtas wrote:
> > > > > > In order to fix this, extend the region which is non-accessible
> > > > > > by the OS to cover both the ARM-TF (0x4000000 - 0x4200000) and OPTEE
> > > > > > (0x4400000 - 0x5400000) within a single area (0x4000000 - 0x5400000).
> > > > > > Set the PEI stack base address between both images (0x43F0000).
> > > > >
> > > > > OK, that is a much better description.
> > > > > But I'm getting slight cognitive dissonance from placing the PEI stack
> > > > > inside something we've just claimed belongs to Secure world...
> > > > >
> > > > > Could you instead break this out into two separate protected regions?
> > > > > PcdSecureOpteeBase/Size and PcdSecureTfBase/Size?
> > > > >
> > > > > Alternatively, nudge the stackbase to 0x5400000?
> > > >
> > > > As discussed some time ago with Ard, when the PEI stack base was
> > > > introduced, it is recommended that this stack is placed in the
> > > > location, which is not accessible by OS. Most preferred is to have it
> > > > in the SRAM (cannot do it on Armada7k8k) or in a reserved region - cut
> > > > out from the memory map passed to the OS.
> > > >
> > > > Currently we have a single region (a "hole") that covers:
> > > > 2MB for EL3 runtime services
> > > > 2MB of nothing
> > > > 16MB for OPTEE image
> > > >
> > > > The 2MB space between images IMO seems perfect for PEI stack to place.
> > > > If it was placed e.g. @0x5400000 and we kept the reserved regions
> > > > separate, the outcome would be:
> > > > 2MB for EL3 runtime services
> > > > 2MB of DRAM normal memory
> > > > 16MB + 64kB for Optee and PEI stack base.
> > > >
> > > > This is the reason, I'd like to keep original setting, proposed in the
> > > > patch. Please let know your opinion.
> > >
> > > I have no issue with the placement of the PEI stack between the ARM-TF
> > > region and the Op-TEE region. I _have_ an issue with the PEI stack
> > > being placed between PcdSecureRegionBase and (PcdSecureRegionBase +
> > > PcdSecureRegionSize). I.e. something that we describe as "the Secure
> > > region".
> > >
> > > I think I gave my suggestion for the resolution of this problem (with
> > > moving StackBase to 0x05400000 as the alternative) in my previous
> > > reply.
> > >
> >
> > Yes, and I answered, presenting the alternative memory map with
> > additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
> > not fancy, given available space inside the 20MB chunk.
>
> Please go back and reread my first and my second email.
> Then please point out where I have, other than as an alternative
> solution, suggested growing the cutout size.
>
> Then perhaps we can rewind this conversation and try again?

Ok. So would it be sufficient to replace
gMarvellTokenSpaceGuid.PcdSecureRegionBase with two sets of separate
PCDs for ARM-TF runtime services and OPTEE leaving the PEI stack base
@0x43f0000?

Best regards,
Marcin

>
> Best Regards,
>
> Leif
>
> > Because in fact this region is not entirely secure (EL3 runtime
> > services are exectued in NS context for example), how about I:
> > - rename the PCD's to be more generic (e.g.
> > gMarvellTokenSpaceGuid.PcdReservedRegionBase)
> > - add proper comment in Armada7k8k.dsc.inc for the default reserved
> > memory (+ maybe in Armada7k8kLib, where the PCD's are used)
> > ?
> >
> > Best regards,
> > Marcin
> >
> > > >
> > > > Best regards,
> > > > Marcin
> > > >
> > > >
> > > > >
> > > > > /
> > > > >     Leif
> > > > >
> > > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > > > > ---
> > > > > >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > > index eafcd6e..c8c597f 100644
> > > > > > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > > @@ -376,12 +376,12 @@
> > > > > >
> > > > > >    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|36
> > > > > >
> > > > > > -  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x41F0000
> > > > > > +  gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x43F0000
> > > > > >    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize|0x10000
> > > > > >
> > > > > >    # Secure region reservation
> > > > > >    gMarvellTokenSpaceGuid.PcdSecureRegionBase|0x4000000
> > > > > > -  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x0200000
> > > > > > +  gMarvellTokenSpaceGuid.PcdSecureRegionSize|0x1400000
> > > > > >
> > > > > >    # TRNG
> > > > > >    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
> > > > > > --
> > > > > > 2.7.4
> > > > > >

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Leif Lindholm]

On Tue, Jan 22, 2019 at 09:56:14PM +0100, Marcin Wojtas wrote:
> > > > I think I gave my suggestion for the resolution of this problem (with
> > > > moving StackBase to 0x05400000 as the alternative) in my previous
> > > > reply.
> > > >
> > >
> > > Yes, and I answered, presenting the alternative memory map with
> > > additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
> > > not fancy, given available space inside the 20MB chunk.
> >
> > Please go back and reread my first and my second email.
> > Then please point out where I have, other than as an alternative
> > solution, suggested growing the cutout size.
> >
> > Then perhaps we can rewind this conversation and try again?
> 
> Ok. So would it be sufficient to replace
> gMarvellTokenSpaceGuid.PcdSecureRegionBase with two sets of separate
> PCDs for ARM-TF runtime services and OPTEE leaving the PEI stack base
> @0x43f0000?

That would be lovely, thank you :)

(Although your reference to wanting to keep the PEI stack area out of
the hands of the operating system might mean that you want three? I'll
leave that to your discretion.)

Best Regards,

Leif

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Marcin Wojtas]

Hi Leif,

wt., 22 sty 2019 o 22:10 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Tue, Jan 22, 2019 at 09:56:14PM +0100, Marcin Wojtas wrote:
> > > > > I think I gave my suggestion for the resolution of this problem (with
> > > > > moving StackBase to 0x05400000 as the alternative) in my previous
> > > > > reply.
> > > > >
> > > >
> > > > Yes, and I answered, presenting the alternative memory map with
> > > > additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
> > > > not fancy, given available space inside the 20MB chunk.
> > >
> > > Please go back and reread my first and my second email.
> > > Then please point out where I have, other than as an alternative
> > > solution, suggested growing the cutout size.
> > >
> > > Then perhaps we can rewind this conversation and try again?
> >
> > Ok. So would it be sufficient to replace
> > gMarvellTokenSpaceGuid.PcdSecureRegionBase with two sets of separate
> > PCDs for ARM-TF runtime services and OPTEE leaving the PEI stack base
> > @0x43f0000?
>
> That would be lovely, thank you :)
>
> (Although your reference to wanting to keep the PEI stack area out of
> the hands of the operating system might mean that you want three? I'll
> leave that to your discretion.)
>

PEI stack has its own PCDs:
   gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase
   gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize

I want to keep it simple (and btw aligned with U-Boot booting the
mainline DTB with single 20MB reserved memory area), so what I intend
to do is to limit reserved region in Armada7k8kMemoryInitPeiLib.c with
PcdArmTFRegionBase (@0x4000000) up to PcdOpteeRegionBase +
PcdOpteeRegionSize (@0x5400000).

Best regards,
Marcin

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Leif Lindholm]

On Wed, Jan 23, 2019 at 09:28:40AM +0100, Marcin Wojtas wrote:
> wt., 22 sty 2019 o 22:10 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> >
> > On Tue, Jan 22, 2019 at 09:56:14PM +0100, Marcin Wojtas wrote:
> > > > > > I think I gave my suggestion for the resolution of this problem (with
> > > > > > moving StackBase to 0x05400000 as the alternative) in my previous
> > > > > > reply.
> > > > > >
> > > > >
> > > > > Yes, and I answered, presenting the alternative memory map with
> > > > > additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
> > > > > not fancy, given available space inside the 20MB chunk.
> > > >
> > > > Please go back and reread my first and my second email.
> > > > Then please point out where I have, other than as an alternative
> > > > solution, suggested growing the cutout size.
> > > >
> > > > Then perhaps we can rewind this conversation and try again?
> > >
> > > Ok. So would it be sufficient to replace
> > > gMarvellTokenSpaceGuid.PcdSecureRegionBase with two sets of separate
> > > PCDs for ARM-TF runtime services and OPTEE leaving the PEI stack base
> > > @0x43f0000?
> >
> > That would be lovely, thank you :)
> >
> > (Although your reference to wanting to keep the PEI stack area out of
> > the hands of the operating system might mean that you want three? I'll
> > leave that to your discretion.)
> >
> 
> PEI stack has its own PCDs:
>    gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase
>    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize
> 
> I want to keep it simple (and btw aligned with U-Boot booting the
> mainline DTB with single 20MB reserved memory area), so what I intend
> to do is to limit reserved region in Armada7k8kMemoryInitPeiLib.c with
> PcdArmTFRegionBase (@0x4000000) up to PcdOpteeRegionBase +
> PcdOpteeRegionSize (@0x5400000).

I am totally online with you wanting to align the reservation of 20MB
of RAM with U-Boot.

If you want to remove the 2MB gap between ARM-TF and Optee from use by
the OS, you need to reserve that 2MB window. Not pretend that it forms
part of an adjacent region that you also happen to want to keep out of
the hands of the OS.

The point of the source code is not wiggling the correct signal lines
to create an expected behaviour. Were that the case, we'd be hacking
programs directly into binary.
The point of the source code is to describe what is being done such
that someone else can come in and understand it.

Saving 15 (or 30, or whatever) lines of boilerplate text by making the
code misleading is not a win.

You want to solve this by making PcdCPUCorePrimaryStackSize 2MB?
Fine. It's not misleading, and you could always shrink it if you need
the remainder for something else.

You want to solve this by setting up a third reserved area of
(2MB - PcdCPUCorePrimaryStackSize)?
Fine.

You want to solve this by making the source code say that a memory
region is simultaneously reserved for Secure world and where our
Non-secure stack resides?
Not fine. That is what I mean by semantic sense.

Best Regards,

Leif

Re: [platforms: PATCH v2 1/4] Marvell/Armada7k8k: Shift PEI stack base

[Marcin Wojtas]

Hi Leif,

śr., 23 sty 2019 o 10:42 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Wed, Jan 23, 2019 at 09:28:40AM +0100, Marcin Wojtas wrote:
> > wt., 22 sty 2019 o 22:10 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> > >
> > > On Tue, Jan 22, 2019 at 09:56:14PM +0100, Marcin Wojtas wrote:
> > > > > > > I think I gave my suggestion for the resolution of this problem (with
> > > > > > > moving StackBase to 0x05400000 as the alternative) in my previous
> > > > > > > reply.
> > > > > > >
> > > > > >
> > > > > > Yes, and I answered, presenting the alternative memory map with
> > > > > > additional 64kB "cut out" on top of 20MB "hole" of memory, which I'm
> > > > > > not fancy, given available space inside the 20MB chunk.
> > > > >
> > > > > Please go back and reread my first and my second email.
> > > > > Then please point out where I have, other than as an alternative
> > > > > solution, suggested growing the cutout size.
> > > > >
> > > > > Then perhaps we can rewind this conversation and try again?
> > > >
> > > > Ok. So would it be sufficient to replace
> > > > gMarvellTokenSpaceGuid.PcdSecureRegionBase with two sets of separate
> > > > PCDs for ARM-TF runtime services and OPTEE leaving the PEI stack base
> > > > @0x43f0000?
> > >
> > > That would be lovely, thank you :)
> > >
> > > (Although your reference to wanting to keep the PEI stack area out of
> > > the hands of the operating system might mean that you want three? I'll
> > > leave that to your discretion.)
> > >
> >
> > PEI stack has its own PCDs:
> >    gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase
> >    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize
> >
> > I want to keep it simple (and btw aligned with U-Boot booting the
> > mainline DTB with single 20MB reserved memory area), so what I intend
> > to do is to limit reserved region in Armada7k8kMemoryInitPeiLib.c with
> > PcdArmTFRegionBase (@0x4000000) up to PcdOpteeRegionBase +
> > PcdOpteeRegionSize (@0x5400000).
>
> I am totally online with you wanting to align the reservation of 20MB
> of RAM with U-Boot.
>
> If you want to remove the 2MB gap between ARM-TF and Optee from use by
> the OS, you need to reserve that 2MB window. Not pretend that it forms
> part of an adjacent region that you also happen to want to keep out of
> the hands of the OS.
>
> The point of the source code is not wiggling the correct signal lines
> to create an expected behaviour. Were that the case, we'd be hacking
> programs directly into binary.
> The point of the source code is to describe what is being done such
> that someone else can come in and understand it.
>
> Saving 15 (or 30, or whatever) lines of boilerplate text by making the
> code misleading is not a win.
>
> You want to solve this by making PcdCPUCorePrimaryStackSize 2MB?
> Fine. It's not misleading, and you could always shrink it if you need
> the remainder for something else.
>
> You want to solve this by setting up a third reserved area of
> (2MB - PcdCPUCorePrimaryStackSize)?
> Fine.
>
> You want to solve this by making the source code say that a memory
> region is simultaneously reserved for Secure world and where our
> Non-secure stack resides?
> Not fine. That is what I mean by semantic sense.
>

Thank you for your input. I will explicitly handle each region then.

Best regards,
Marcin