[platforms: PATCH 0/3] Armada7k8k memory handling update

[platforms: PATCH 0/3] Armada7k8k memory handling update

[Marcin Wojtas]

Hi,

This short patchset adjust to the latest Marvell v18.12
ARM-TF sources. They change DRAM configuration
registers to be secure, as well as extend the region,
which is non-accessible by OS. The patches avoid
the issues by shifitng PEI stack base and configuring
the memory map, using SiP services.

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

I'm looking forward to the comments and remarks.

Best regards,
Marcin


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

Marcin Wojtas (1):
  Marvell: Armada7k8k: Shift PEI stack base

 Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc                                  |  4 +-
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf             |  3 ++
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h            | 27 ++--------
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h |  1 +
 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 10 ++++
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c            | 55 +++++---------------
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 17 ++++++
 7 files changed, 51 insertions(+), 66 deletions(-)

-- 
2.7.4

[platforms: PATCH 1/3] 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 and set the PEI stack
base address between both images.

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 2/3] 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 |  1 +
 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 10 ++++++++++
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 17 +++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
index bfc8639..6caee6c 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
@@ -22,6 +22,7 @@
 // Common macros
 //
 #define MV_SOC_CP_BASE(Cp)               (0xF2000000 + ((Cp) * 0x2000000))
+#define MV_SOC_AP806_BASE                0xF0000000
 #define MV_SOC_AP806_COUNT               1
 
 //
diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
index 26b075a..7aec9be 100644
--- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
+++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
@@ -20,6 +20,16 @@
 #include <Protocol/EmbeddedGpio.h>
 
 //
+// North Bridge description
+//
+EFI_STATUS
+EFIAPI
+ArmadaSoCAp8xxBaseGet (
+  IN OUT UINT64  *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..089ac2d 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
@@ -30,6 +30,23 @@
 
 EFI_STATUS
 EFIAPI
+ArmadaSoCAp8xxBaseGet (
+  IN OUT UINT64  *ApBase,
+  IN UINTN        ApIndex
+  )
+{
+  if (ApIndex != 0) {
+    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 (
   IN OUT MV_SOC_COMPHY_DESC  **ComPhyDesc,
   IN OUT UINTN                *DescCount
-- 
2.7.4

[platforms: PATCH 3/3] 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 | 27 ++--------
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c | 55 +++++---------------
 3 files changed, 21 insertions(+), 64 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..8a46df6 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
@@ -47,27 +47,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #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
+/* Armada7k8k North Bridge index */
+#define ARMADA7K8K_AP806_INDEX          0
 
-#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))
+/* Firmware related definition used for SMC calls */
+#define MV_SIP_DRAM_SIZE                0x82000010
diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
index 2a4f5ad..62e8467 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
@@ -33,11 +33,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 *******************************************************************************/
 
 #include <Base.h>
+#include <IndustryStandard/ArmStdSmc.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 <Protocol/BoardDesc.h>
 
 #include "Armada7k8kLibMem.h"
 
@@ -57,49 +60,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_SIP_DRAM_SIZE;
+  Status = ArmadaSoCAp8xxBaseGet (&SmcRegs.Arg1, ARMADA7K8K_AP806_INDEX);
+  if (EFI_ERROR (Status)) {
+    return Status;
   }
 
+  ArmCallSmc (&SmcRegs);
+
+  *MemSize = SmcRegs.Arg0;
+
   return EFI_SUCCESS;
 }
 
-- 
2.7.4

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

[Leif Lindholm]

On Mon, Jan 21, 2019 at 11:52:09AM +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 and set the PEI stack

What is the single area?

> base address between both images.
> 
> 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 2/3] Marvell/Library: ArmadaSoCDescLib: Add North Bridge description

[Leif Lindholm]

On Mon, Jan 21, 2019 at 11:52:10AM +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>
> ---
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h |  1 +
>  Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 10 ++++++++++
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 17 +++++++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> index bfc8639..6caee6c 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> @@ -22,6 +22,7 @@
>  // Common macros
>  //
>  #define MV_SOC_CP_BASE(Cp)               (0xF2000000 + ((Cp) * 0x2000000))
> +#define MV_SOC_AP806_BASE                0xF0000000
>  #define MV_SOC_AP806_COUNT               1
>  
>  //
> diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> index 26b075a..7aec9be 100644
> --- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> +++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> @@ -20,6 +20,16 @@
>  #include <Protocol/EmbeddedGpio.h>
>  
>  //
> +// North Bridge description
> +//
> +EFI_STATUS
> +EFIAPI
> +ArmadaSoCAp8xxBaseGet (
> +  IN OUT UINT64  *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..089ac2d 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> @@ -30,6 +30,23 @@
>  

As I said, I was going to get stricter about adding function
description comments - please add one here. (And clone it to
ArmadaSoCDescLib.h, for the obious place to go look for a template
when implementing a variant for a new platform.)

>  EFI_STATUS
>  EFIAPI
> +ArmadaSoCAp8xxBaseGet (
> +  IN OUT UINT64  *ApBase,
> +  IN UINTN        ApIndex
> +  )
> +{
> +  if (ApIndex != 0) {

This test should be using ARMADA7K8K_AP806_INDEX, and that definition
should be added to this patch.

> +    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 (
>    IN OUT MV_SOC_COMPHY_DESC  **ComPhyDesc,
>    IN OUT UINTN                *DescCount
> -- 
> 2.7.4
> 

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

[Leif Lindholm]

On Mon, Jan 21, 2019 at 11:52:11AM +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>
> ---
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf  |  3 ++
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h | 27 ++--------
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c | 55 +++++---------------
>  3 files changed, 21 insertions(+), 64 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..8a46df6 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> @@ -47,27 +47,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #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
> +/* Armada7k8k North Bridge index */
> +#define ARMADA7K8K_AP806_INDEX          0
>  
> -#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))
> +/* Firmware related definition used for SMC calls */
> +#define MV_SIP_DRAM_SIZE                0x82000010

Hmm...
This would end us up with having Marvell SMC calls spread across
multiple files. Could you insert an intermediate patch which breaks out
the ones from Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h into a
separate (MarvellSmc.h?) include file?

If you could further add _SMC_ID to the #defines (like in edk2
ArmPkg/Include/IndustryStandard/ArmStdSmc.h), that would make me very
happy. (I'd be happy for you to drop _SIP, or keep it either side of
the addition, as per your preference - we don't seem to have precedent
here yet.)

Regards,

Leif

> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> index 2a4f5ad..62e8467 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> @@ -33,11 +33,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  *******************************************************************************/
>  
>  #include <Base.h>
> +#include <IndustryStandard/ArmStdSmc.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 <Protocol/BoardDesc.h>
>  
>  #include "Armada7k8kLibMem.h"
>  
> @@ -57,49 +60,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_SIP_DRAM_SIZE;
> +  Status = ArmadaSoCAp8xxBaseGet (&SmcRegs.Arg1, ARMADA7K8K_AP806_INDEX);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
>    }
>  
> +  ArmCallSmc (&SmcRegs);
> +
> +  *MemSize = SmcRegs.Arg0;
> +
>    return EFI_SUCCESS;
>  }
>  
> -- 
> 2.7.4
> 

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

[Marcin Wojtas]

Hi Leif,


pon., 21 sty 2019 o 12:26 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Mon, Jan 21, 2019 at 11:52:09AM +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 and set the PEI stack
>
> What is the single area?

The single region is set to:
0x4000000 - 0x5400000

PEI stack base is shifted right below the OPTEE region, i.e. to:
0x43F0000

Do you wish to add above to the commit message as well?

Best regards,
Marcin

>
> > base address between both images.
> >
> > 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 2/3] Marvell/Library: ArmadaSoCDescLib: Add North Bridge description

[Marcin Wojtas]

Hi Leif,

pon., 21 sty 2019 o 12:32 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Mon, Jan 21, 2019 at 11:52:10AM +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>
> > ---
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h |  1 +
> >  Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 10 ++++++++++
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 17 +++++++++++++++++
> >  3 files changed, 28 insertions(+)
> >
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> > index bfc8639..6caee6c 100644
> > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> > @@ -22,6 +22,7 @@
> >  // Common macros
> >  //
> >  #define MV_SOC_CP_BASE(Cp)               (0xF2000000 + ((Cp) * 0x2000000))
> > +#define MV_SOC_AP806_BASE                0xF0000000
> >  #define MV_SOC_AP806_COUNT               1
> >
> >  //
> > diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> > index 26b075a..7aec9be 100644
> > --- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> > +++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> > @@ -20,6 +20,16 @@
> >  #include <Protocol/EmbeddedGpio.h>
> >
> >  //
> > +// North Bridge description
> > +//
> > +EFI_STATUS
> > +EFIAPI
> > +ArmadaSoCAp8xxBaseGet (
> > +  IN OUT UINT64  *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..089ac2d 100644
> > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> > @@ -30,6 +30,23 @@
> >
>
> As I said, I was going to get stricter about adding function
> description comments - please add one here. (And clone it to
> ArmadaSoCDescLib.h, for the obious place to go look for a template
> when implementing a variant for a new platform.)

Sure, I will add it.

>
> >  EFI_STATUS
> >  EFIAPI
> > +ArmadaSoCAp8xxBaseGet (
> > +  IN OUT UINT64  *ApBase,
> > +  IN UINTN        ApIndex
> > +  )
> > +{
> > +  if (ApIndex != 0) {
>
> This test should be using ARMADA7K8K_AP806_INDEX, and that definition
> should be added to this patch.
>

Ok.

Thanks,
Marcin

> > +    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 (
> >    IN OUT MV_SOC_COMPHY_DESC  **ComPhyDesc,
> >    IN OUT UINTN                *DescCount
> > --
> > 2.7.4
> >

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

[Leif Lindholm]

On Mon, Jan 21, 2019 at 04:34:39PM +0100, Marcin Wojtas wrote:
> Hi Leif,
> 
> 
> pon., 21 sty 2019 o 12:26 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> >
> > On Mon, Jan 21, 2019 at 11:52:09AM +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 and set the PEI stack
> >
> > What is the single area?
> 
> The single region is set to:
> 0x4000000 - 0x5400000
> 
> PEI stack base is shifted right below the OPTEE region, i.e. to:
> 0x43F0000
> 
> Do you wish to add above to the commit message as well?

Yes please.

Best Regards,

Leif

> Best regards,
> Marcin
> 
> >
> > > base address between both images.
> > >
> > > 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 3/3] Marvell/Armada7k8k: Read DRAM settings from ARM-TF

[Marcin Wojtas]

Hi Leif,

pon., 21 sty 2019 o 12:51 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Mon, Jan 21, 2019 at 11:52:11AM +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>
> > ---
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf  |  3 ++
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h | 27 ++--------
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c | 55 +++++---------------
> >  3 files changed, 21 insertions(+), 64 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..8a46df6 100644
> > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.h
> > @@ -47,27 +47,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  #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
> > +/* Armada7k8k North Bridge index */
> > +#define ARMADA7K8K_AP806_INDEX          0
> >
> > -#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))
> > +/* Firmware related definition used for SMC calls */
> > +#define MV_SIP_DRAM_SIZE                0x82000010
>
> Hmm...
> This would end us up with having Marvell SMC calls spread across
> multiple files. Could you insert an intermediate patch which breaks out
> the ones from Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h into a
> separate (MarvellSmc.h?) include file?

How about MvSmc.h ? I try to use 'Mv' prefix consistently, especially
when adding new code.

>
> If you could further add _SMC_ID to the #defines (like in edk2
> ArmPkg/Include/IndustryStandard/ArmStdSmc.h), that would make me very
> happy. (I'd be happy for you to drop _SIP, or keep it either side of
> the addition, as per your preference - we don't seem to have precedent
> here yet.)
>

How about below:
#define MV_SMC_ID_COMPHY_POWER_ON       0x82000001
#define MV_SMC_ID_COMPHY_POWER_OFF     0x82000002
#define MV_SMC_ID_COMPHY_PLL_LOCK          0x82000003
?

Thanks,
Marcin


> Regards,
>
> Leif
>
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > index 2a4f5ad..62e8467 100644
> > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > @@ -33,11 +33,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  *******************************************************************************/
> >
> >  #include <Base.h>
> > +#include <IndustryStandard/ArmStdSmc.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 <Protocol/BoardDesc.h>
> >
> >  #include "Armada7k8kLibMem.h"
> >
> > @@ -57,49 +60,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_SIP_DRAM_SIZE;
> > +  Status = ArmadaSoCAp8xxBaseGet (&SmcRegs.Arg1, ARMADA7K8K_AP806_INDEX);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> >    }
> >
> > +  ArmCallSmc (&SmcRegs);
> > +
> > +  *MemSize = SmcRegs.Arg0;
> > +
> >    return EFI_SUCCESS;
> >  }
> >
> > --
> > 2.7.4
> >

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

[Leif Lindholm]

On Mon, Jan 21, 2019 at 04:47:31PM +0100, Marcin Wojtas wrote:
> > > -#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))
> > > +/* Firmware related definition used for SMC calls */
> > > +#define MV_SIP_DRAM_SIZE                0x82000010
> >
> > Hmm...
> > This would end us up with having Marvell SMC calls spread across
> > multiple files. Could you insert an intermediate patch which breaks out
> > the ones from Silicon/Marvell/Library/ComPhyLib/ComPhySipSvc.h into a
> > separate (MarvellSmc.h?) include file?
> 
> How about MvSmc.h ? I try to use 'Mv' prefix consistently, especially
> when adding new code.

Sure.

> >
> > If you could further add _SMC_ID to the #defines (like in edk2
> > ArmPkg/Include/IndustryStandard/ArmStdSmc.h), that would make me very
> > happy. (I'd be happy for you to drop _SIP, or keep it either side of
> > the addition, as per your preference - we don't seem to have precedent
> > here yet.)
> >
> 
> How about below:
> #define MV_SMC_ID_COMPHY_POWER_ON       0x82000001
> #define MV_SMC_ID_COMPHY_POWER_OFF     0x82000002
> #define MV_SMC_ID_COMPHY_PLL_LOCK          0x82000003
> ?

Yeah, that works.

Regards,

Leif

> Thanks,
> Marcin
> 
> 
> > Regards,
> >
> > Leif
> >
> > > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > > index 2a4f5ad..62e8467 100644
> > > --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLibMem.c
> > > @@ -33,11 +33,14 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > >  *******************************************************************************/
> > >
> > >  #include <Base.h>
> > > +#include <IndustryStandard/ArmStdSmc.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 <Protocol/BoardDesc.h>
> > >
> > >  #include "Armada7k8kLibMem.h"
> > >
> > > @@ -57,49 +60,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_SIP_DRAM_SIZE;
> > > +  Status = ArmadaSoCAp8xxBaseGet (&SmcRegs.Arg1, ARMADA7K8K_AP806_INDEX);
> > > +  if (EFI_ERROR (Status)) {
> > > +    return Status;
> > >    }
> > >
> > > +  ArmCallSmc (&SmcRegs);
> > > +
> > > +  *MemSize = SmcRegs.Arg0;
> > > +
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > > --
> > > 2.7.4
> > >