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

[Marcin Wojtas <mw@semihalf.com>]

2017-10-11 19:56 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Wed, Oct 11, 2017 at 05:40:47PM +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 | 111 +++++++++++++++++++-
>>  Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.h |  49 +++++++++
>>  2 files changed, 159 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> index 2cb2e15..22cbe47 100644
>> --- a/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> +++ b/Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0LibMem.c
>> @@ -36,8 +36,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>  #include <Library/ArmPlatformLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/HobLib.h>
>> +#include <Library/IoLib.h>
>>  #include <Library/MemoryAllocationLib.h>
>>
>> +#include "Armada70x0LibMem.h"
>> +
>>  // The total number of descriptors, including the final "end-of-table" descriptor.
>>  #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
>>
>> @@ -47,6 +50,105 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>>  STATIC ARM_MEMORY_REGION_DESCRIPTOR VirtualMemoryTable[MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS];
>>
>> +// Obtain DRAM size basing on register values filled by early firmware.
>> +STATIC
>> +UINT64
>> +DramSizeGet (
>
> GetDramSize?
>
>> +  UINT64 *MemSize
>
> IN, OUT?
>

2x OK.

>> +  )
>> +{
>> +  UINT64 BaseAddr;
>> +  UINT32 RegVal;
>> +  UINT8 AreaLengthMap;
>> +  UINT8 Cs;
>> +
>> +  *MemSize = 0;
>> +
>> +  for (Cs = 0; Cs < DRAM_MAX_CS_NUM; Cs++) {
>> +
>> +    RegVal = MmioRead32 (DRAM_CH0_MMAP_LOW_REG (Cs));
>> +
>> +    /* Exit loop on first disabled DRAM CS */
>> +    if (!(RegVal & DRAM_CS_VALID_ENABLED_MASK)) {
>> +      break;
>> +    }
>> +
>> +    /*
>> +     * Sanity check for base address of next DRAM block.
>> +     * Only continuous space will be used.
>> +     */
>> +    BaseAddr = ((UINT64)MmioRead32 (DRAM_CH0_MMAP_HIGH_REG (Cs)) <<
>> +                DRAM_START_ADDR_HTOL_OFFS) | (RegVal & DRAM_START_ADDRESS_L_MASK);
>
> Please use macros, temporary variables, more parentheses or whatever
> to help make the above operation readable.
>

Sure.

>> +    if (BaseAddr != *MemSize) {
>> +      DEBUG ((DEBUG_ERROR,
>> +        "DramSizeGet: DRAM blocks are not contiguous, limit size to 0x%llx\n",
>> +        *MemSize));
>> +      return EFI_SUCCESS;
>> +    }
>> +
>> +    /* Decode area length for current CS from register value */
>> +    AreaLengthMap = ((RegVal & DRAM_AREA_LENGTH_MASK) >> DRAM_AREA_LENGTH_OFFS);
>> +    switch (AreaLengthMap) {
>
> Why Map?
>

I stuck to mv-ddr convention, but I agree, 'AreaLength' will be better.

>> +    case 0x0:
>> +      *MemSize += 0x18000000;
>> +      break;
>> +    case 0x1:
>> +      *MemSize += 0x30000000;
>> +      break;
>> +    case 0x2:
>> +      *MemSize += 0x60000000;
>> +      break;
>> +    case 0x3:
>> +      *MemSize += 0xC0000000;
>> +      break;
>
> The above ones look if not devoid of pattern, at least a bit
> unexpected - and 4-6 are comlpetely missing. Is there any
> documentation available for me to read to try to understand what is
> going on?

Do you have a docs for armada 7k or 8k? Please check 0xf0020200 table,
bits 20:16. You are right, I missed '4' (6GB), but 5 and 6 are
reserved (no value possible).

>
> The below all look predictably formatted and possible to calculate
> rather than list one by one.
>
> (1 << ((AreaLengthMap + 16))) if a quick calculation serves me right.

I think we can reduce the switch-case to this:

if  (AreaLength =< 0x4) {
  *MemSize = 0x18000000 << AreaLength;
}  else if (AreaLength >= 0x7 && AreaLength < 0x1B)
  *MemSize = 1 << (AreaLengthMap + 16);
} else {
  DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
CS#%d\n", AreaLength, Cs));
}

Is above ok for you?

>
>> +    case 0x7:
>> +      *MemSize += 0x00800000;
>> +      break;
>> +    case 0x8:
>> +      *MemSize += 0x01000000;
>> +      break;
>> +    case 0x9:
>> +      *MemSize += 0x02000000;
>> +      break;
>> +    case 0xA:
>> +      *MemSize += 0x04000000;
>> +      break;
>> +    case 0xB:
>> +      *MemSize += 0x08000000;
>> +      break;
>> +    case 0xC:
>> +      *MemSize += 0x10000000;
>> +      break;
>> +    case 0xD:
>> +      *MemSize += 0x20000000;
>> +      break;
>> +    case 0xE:
>> +      *MemSize += 0x40000000;
>> +      break;
>> +    case 0xF:
>> +      *MemSize += 0x80000000;
>> +      break;
>> +    case 0x10:
>> +      *MemSize += 0x100000000;
>> +      break;
>> +    case 0x11:
>> +      *MemSize += 0x200000000;
>> +      break;
>> +    case 0x12:
>> +      *MemSize += 0x400000000;
>> +      break;
>> +    case 0x13:
>> +      *MemSize += 0x800000000;
>> +      break;
>> +    default:
>> +      DEBUG ((DEBUG_ERROR, "Invalid area length (0x%x) for CS#%d\n", AreaLengthMap, Cs));
>
> Area length isn't really a helpful debug message.
> "memory module size"?
>

Ok.

Thanks,
Marcin