Re: [platforms: PATCH v2 6/8] Marvell/Armada: Enable dynamic DRAM size detection

[Leif Lindholm <leif.lindholm@linaro.org>]

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 <mw@semihalf.com>
> ---
>  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
>