On Wed, Oct 25, 2017 at 08:45:28AM +0200, Marcin Wojtas wrote:
> Instead of using hardcoded value in PcdSystemMemorySize PCD,
> obtain DRAM size directly from SoC registers, which are filled
> by firmware during early initialization stage.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <[email protected]>
> ---
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c | 62
> +++++++++++++++++++-
> Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h | 25
> ++++++++
> 2 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> index 978e4d3..bf0ebcf 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
> @@ -50,6 +50,60 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
>
> STATIC ARM_MEMORY_REGION_DESCRIPTOR
> mVirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>
> +// Obtain DRAM size basing on register values filled by early firmware.
> +STATIC
> +UINT64
> +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_CODE_INVALID (RegionCode)) {
> + DEBUG ((DEBUG_ERROR,
> + "%a: Invalid memory region code (0x%x) for CS#%d\n",
> + __FUNCTION__,
> + RegionCode,
> + Cs));
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (RegionCode <= 0x4) {
It would be even nicer this one could be abstracted away in another
macro (now everything else is this clean, it does sort of stand out).
> + *MemSize += GET_DRAM_REGION_SIZE_ODD (RegionCode);
> + } else {
> + *MemSize += GET_DRAM_REGION_SIZE_EVEN (RegionCode);
> + }
I guess we should also check for the reserved ones.
I'm also a fan of the "common case first" approach to if-statements.
So, could we squash these two together?:
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;
}
Where
#define DRAM_REGION_SIZE_ODD(x) ((x) <= 4)
#define DRAM_REGION_SIZE_EVEN(x) (((x) >= 7) && ((x) <= 26))
(I use decimal numbers, because so does the documentation.)
/
Leif
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Return the Virtual Memory Map of your platform
>
> @@ -72,12 +126,18 @@ ArmPlatformGetVirtualMemoryMap (
> UINT64 MemHighSize;
> UINT64 ConfigSpaceBaseAddr;
> EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes;
> + EFI_STATUS Status;
>
> ASSERT (VirtualMemoryMap != NULL);
>
> ConfigSpaceBaseAddr = FixedPcdGet64 (PcdConfigSpaceBaseAddress);
>
> - MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> + // Obtain total memory size from the hardware.
> + Status = GetDramSize (&MemSize);
> + if (EFI_ERROR (Status)) {
> + MemSize = FixedPcdGet64 (PcdSystemMemorySize);
> + DEBUG ((DEBUG_ERROR, "Limit total memory size to %d MB\n", MemSize /
> 1024 / 1024));
> + }
>
> if (DRAM_REMAP_ENABLED) {
> MemLowSize = MIN (DRAM_REMAP_TARGET, MemSize);
> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> index 8101cf3..dd218d9 100644
> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h
> @@ -46,3 +46,28 @@ 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_CODE_INVALID(C) \
> + (((C) > 0x4 && (C) < 0x7) || (C) > 0x1b)
> +#define GET_DRAM_REGION_SIZE_ODD(C) ((UINT64)0x18000000 << (C))
> +#define GET_DRAM_REGION_SIZE_EVEN(C) ((UINT64)1 << ((C) + 16))
> --
> 2.7.4
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
