public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Leif Lindholm <leif.lindholm@linaro.org>
To: Marcin Wojtas <mw@semihalf.com>
Cc: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	"Nadav Haklai" <nadavh@marvell.com>,
	"Neta Zur Hershkovits" <neta@marvell.com>,
	"Kostya Porotchkin" <kostap@marvell.com>,
	"Hua Jing" <jinghua@marvell.com>, "Jan Dąbroś" <jsd@semihalf.com>
Subject: Re: [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping
Date: Thu, 12 Oct 2017 11:29:59 +0100	[thread overview]
Message-ID: <20171012102959.isozglddgunxmhc6@bivouac.eciton.net> (raw)
In-Reply-To: <CAPv3WKdhYwVbSXQ0_N9agpFBC8_GRE-3ZE1KnjLkb_-mDCTesQ@mail.gmail.com>

On Thu, Oct 12, 2017 at 06:58:37AM +0200, Marcin Wojtas wrote:
> Hi Leif,
> 
> 2017-10-11 20:18 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> > On 11 October 2017 at 18:08, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> Subject: from->for ?
> >>
> >> On Wed, Oct 11, 2017 at 05:40:45PM +0200, Marcin Wojtas wrote:
> >>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>
> >>> The Armada 70x0/80x0 DRAM controller allows a single window of DRAM
> >>> to be remapped to another location in the physical address space. This
> >>> allows us to free up some memory in the 32-bit addressable region for
> >>> peripheral MMIO and PCI MMIO32 and CONFIG spaces. This patch adjusts
> >>> memory blocks to the configuration done in ATF.
> >>
> >> The ARM Trusted Firmware developers tend to frown at that
> >> abbreviation. Please use ARM-TF in official context.
> 
> Ok.
> 
> >>
> >> Which configuration is this? Presumably, if this was changed in
> >> ARM-TF, we would need to change it this end too, so does not hurt to
> >> be explicit. (It uses a FixedPcd, so clearly it is not dynamically
> >> detected, which the commit message can be read as.)
> 
> I checked and I can modify the code, to obtain from registers an
> information about source/target and whether Dram remap is enabled at
> all. This way we would be immune to any further changes in ARM-TF.

That sounds like an excellent improvement.

> However I have a question to that setting, please see below.
> 
> >>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >>> ---
> >>>  Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S | 15 +++++
> >>>  Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf           |  3 +
> >>>  Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c          | 60 ++++++++++++++++----
> >>>  Platform/Marvell/Marvell.dec                                              | 13 +++++
> >>>  4 files changed, 81 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> >>> index 72f8cfc..c5be1a9 100644
> >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/AArch64/ArmPlatformHelper.S
> >>> @@ -17,6 +17,21 @@
> >>>
> >>>  ASM_FUNC(ArmPlatformPeiBootAction)
> >>>    mov   x29, xzr
> >>> +
> >>> +  .if   FixedPcdGet64 (PcdSystemMemoryBase) != 0
> >>> +  .err  PcdSystemMemoryBase should be 0x0 on this platform!
> >>> +  .endif
> >>> +
> >>> +  .if   FixedPcdGet64 (PcdSystemMemorySize) > FixedPcdGet32 (PcdDramRemapTarget)
> >>> +    //
> >>> +    // Use the low range for UEFI itself. The remaining memory will be mapped
> >>> +    // and added to the GCD map later.
> >>> +    //
> >>> +    adr   x0, mSystemMemoryEnd
> >>> +    MOV64 (x1, FixedPcdGet32 (PcdDramRemapTarget) - 1)
> >>> +    str   x1, [x0]
> >>> +  .endif
> >>> +
> 
> The original commit, due lack of dram size detection, increased total
> memory to 4GB, which required above modification with limiting
> mSystemMemoryEnd to dram target (0xc0000000). Now PcdSystemMemorySize
> is left at somewhat lowest reasonable value (1GB), so above code is in
> fact never executed. Are you ok that
> I remove it and leave mSystemMemoryEnd to be set as-is in PrePi.c:
> 
> UINT64 mSystemMemoryEnd = FixedPcdGet64(PcdSystemMemoryBase) +
>                           FixedPcdGet64(PcdSystemMemorySize) - 1;
> 
> ?

Unless Ard tells me we're missing some aspect, that sounds reasonable.

> >>>    ret
> >>>
> >>>  //UINTN
> >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> >>> index 2e198c3..838a670 100644
> >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> >>> @@ -67,5 +67,8 @@
> >>>    gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
> >>>    gArmTokenSpaceGuid.PcdArmPrimaryCore
> >>>
> >>> +  gMarvellTokenSpaceGuid.PcdDramRemapSize
> >>> +  gMarvellTokenSpaceGuid.PcdDramRemapTarget
> >>> +
> >>>  [Ppis]
> >>>    gArmMpCoreInfoPpiGuid
> >>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >>> index 74c9956..2cb2e15 100644
> >>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> >>> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>>  #include <Base.h>
> >>>  #include <Library/ArmPlatformLib.h>
> >>>  #include <Library/DebugLib.h>
> >>> +#include <Library/HobLib.h>
> >>>  #include <Library/MemoryAllocationLib.h>
> >>>
> >>>  // The total number of descriptors, including the final "end-of-table" descriptor.
> >>> @@ -44,6 +45,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>>  #define DDR_ATTRIBUTES_CACHED           ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK
> >>>  #define DDR_ATTRIBUTES_UNCACHED         ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED
> >>>
> >>> +STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
> >>
> >> mVirtualMemoryTable?
> 
> Ok.
> 
> >>
> >>> +
> >>>  /**
> >>>    Return the Virtual Memory Map of your platform
> >>>
> >>> @@ -59,20 +62,41 @@ ArmPlatformGetVirtualMemoryMap (
> >>>    IN ARM_MEMORY_REGION_DESCRIPTOR** VirtualMemoryMap
> >>>    )
> >>>  {
> >>> -  ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> >>>    UINTN                         Index = 0;
> >>> +  UINT64                        MemSize;
> >>> +  UINT64                        MemLowSize;
> >>> +  UINT64                        MemHighStart;
> >>> +  UINT64                        MemHighSize;
> >>> +  EFI_RESOURCE_ATTRIBUTE_TYPE   ResourceAttributes;
> >>>
> >>>    ASSERT (VirtualMemoryMap != NULL);
> >>>
> >>> -  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages(EFI_SIZE_TO_PAGES (sizeof(ARM_MEMORY_REGION_DESCRIPTOR) * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
> >>> -  if (VirtualMemoryTable == NULL) {
> >>> -      return;
> >>> -  }
> >>> +  MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> >>> +  MemLowSize = MIN (FixedPcdGet64 (PcdDramRemapTarget), MemSize);
> >>> +  MemHighStart = (UINT64)FixedPcdGet64 (PcdDramRemapTarget) +
> >>> +                 FixedPcdGet32 (PcdDramRemapSize);
> >>> +  MemHighSize = MemSize - MemLowSize;
> >>> +
> >>> +  ResourceAttributes = (
> >>> +      EFI_RESOURCE_ATTRIBUTE_PRESENT |
> >>> +      EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> >>> +      EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> >>> +      EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> >>> +      EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> >>> +      EFI_RESOURCE_ATTRIBUTE_TESTED
> >>> +  );
> >>> +
> >>> +  BuildResourceDescriptorHob (
> >>> +    EFI_RESOURCE_SYSTEM_MEMORY,
> >>> +    ResourceAttributes,
> >>> +    FixedPcdGet64 (PcdSystemMemoryBase),
> >>> +    MemLowSize
> >>> +    );
> >>>
> >>>    // DDR
> >>> -  VirtualMemoryTable[Index].PhysicalBase    = PcdGet64 (PcdSystemMemoryBase);
> >>> -  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdSystemMemoryBase);
> >>> -  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdSystemMemorySize);
> >>> +  VirtualMemoryTable[Index].PhysicalBase    = FixedPcdGet64 (PcdSystemMemoryBase);
> >>> +  VirtualMemoryTable[Index].VirtualBase     = FixedPcdGet64 (PcdSystemMemoryBase);
> >>> +  VirtualMemoryTable[Index].Length          = MemLowSize;
> >>>    VirtualMemoryTable[Index].Attributes      = DDR_ATTRIBUTES_CACHED;
> >>>
> >>>    // Configuration space 0xF000_0000 - 0xFFFF_FFFF
> >>> @@ -81,13 +105,29 @@ ArmPlatformGetVirtualMemoryMap (
> >>>    VirtualMemoryTable[Index].Length          = 0x10000000;
> >>>    VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >>>
> >>> +  if (MemSize > MemLowSize) {
> >>> +    //
> >>> +    // If we have more than MemLowSize worth of DRAM, the remainder will be
> >>> +    // mapped at the top of the remapped window.
> >>> +    //
> >>> +    VirtualMemoryTable[++Index].PhysicalBase  = MemHighStart;
> >>> +    VirtualMemoryTable[Index].VirtualBase     = MemHighStart;
> >>> +    VirtualMemoryTable[Index].Length          = MemHighSize;
> >>> +    VirtualMemoryTable[Index].Attributes      = DDR_ATTRIBUTES_CACHED;
> >>> +
> >>> +    BuildResourceDescriptorHob (
> >>> +      EFI_RESOURCE_SYSTEM_MEMORY,
> >>> +      ResourceAttributes,
> >>> +      MemHighStart,
> >>> +      MemHighSize
> >>> +      );
> >>> +  }
> >>> +
> >>>    // End of Table
> >>>    VirtualMemoryTable[++Index].PhysicalBase  = 0;
> >>>    VirtualMemoryTable[Index].VirtualBase     = 0;
> >>>    VirtualMemoryTable[Index].Length          = 0;
> >>>    VirtualMemoryTable[Index].Attributes      = 0;
> >>>
> >>> -  ASSERT((Index + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> >>> -
> >>
> >> Why delete this assert?
> 
> Indeed, it looks like unnecessarily removed. I will restore it,
> 
> >>
> >>>    *VirtualMemoryMap = VirtualMemoryTable;
> >>>  }
> >>> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> >>> index 434d6cb..db1c7fa 100644
> >>> --- a/Platform/Marvell/Marvell.dec
> >>> +++ b/Platform/Marvell/Marvell.dec
> >>> @@ -194,6 +194,19 @@
> >>>  #TRNG
> >>>    gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0x0|UINT64|0x50000053
> >>>
> >>> +  #
> >>> +  # DRAM remapping controls.
> >>> +  # On the 70x0/80x0 SOCs, the DRAM is mapped at 0x0, and could be up to
> >>> +  # 16 GB in size. To allow for 32-bit addressable MMIO peripherals or PCI
> >>> +  # windows, a single window of up to 4 GB in size can be remapped elsewhere.
> >>> +  # So let's define a 1 GB window at 0xC000000 by default: this is the minimum
> >>> +  # alignment that Linux can map optimally (i.e., it's section shift is 30 bits)
> >>> +  # and gives us an additional 768 MB (on top of the 256 MB platform MMIO window
> >>> +  # at 0xF0000000) for the PCI MMIO32 and CONFIG spaces.
> >>
> >> If DRAM is mapped at 0, and can be up to 16GB in size, why do we need
> >> to remap some of it below 4GB?
> >> I guess what you're actually doing is mapping it _away_. Where to?
> >>
> >> (And a short-short version of this in the commit message should take
> >> care of that item.)
> >>
> >
> > Indeed. Physical RAM starts at 0x0, and to avoid wasting all 32-bit
> > addressable memory space on RAM, the hardware allows some of it to be
> > remapped elsewhere. So by remapping 1 GB, we make 1 GB of address
> > space available for MMIO, PCI config space and PCI MMIO32 space.
> 
> If we agree to detect remap size and target from registers, those PCDs
> (and comment) will be removed. I will modify the commit log only.

Sounds good to me.

/
    Leif



  reply	other threads:[~2017-10-12 10:26 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-11 15:40 [platforms: PATCH 0/8] Armada 7k/8k - memory improvements Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG Marcin Wojtas
2017-10-11 16:47   ` Leif Lindholm
2017-10-11 18:15     ` Ard Biesheuvel
2017-10-12  4:39       ` Marcin Wojtas
2017-10-12 10:24         ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 2/8] Marvell/Armada: Increase preallocated memory region size Marcin Wojtas
2017-10-11 16:56   ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 3/8] Marvell/Armada: Remove custom reset library residues Marcin Wojtas
2017-10-11 16:56   ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 4/8] Marvell/Armada: Add support from DRAM remapping Marcin Wojtas
2017-10-11 17:08   ` Leif Lindholm
2017-10-11 18:18     ` Ard Biesheuvel
2017-10-12  4:58       ` Marcin Wojtas
2017-10-12 10:29         ` Leif Lindholm [this message]
2017-10-11 15:40 ` [platforms: PATCH 5/8] Marvell/Armada: Add MemoryInitPeiLib that reserves secure region Marcin Wojtas
2017-10-11 17:11   ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 6/8] Marvell/Armada: Enable dynamic DRAM size detection Marcin Wojtas
2017-10-11 17:56   ` Leif Lindholm
2017-10-12  5:47     ` Marcin Wojtas
2017-10-12 10:50       ` Leif Lindholm
2017-10-12 10:58         ` Marcin Wojtas
2017-10-11 15:40 ` [platforms: PATCH 7/8] Marvell/Armada: Armada70x0Lib: Add support for 32-bit ARM Marcin Wojtas
2017-10-11 17:57   ` Leif Lindholm
2017-10-11 15:40 ` [platforms: PATCH 8/8] Marvell/Armada: Add 32-bit ARM support Marcin Wojtas
2017-10-11 17:58   ` Leif Lindholm
2017-10-11 18:20     ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171012102959.isozglddgunxmhc6@bivouac.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox