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

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

On Thu, Oct 12, 2017 at 07:47:52AM +0200, Marcin Wojtas wrote:
> 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).

(Where do I find .75G, 1.5G, 3G and 6G DIMMs?)

> >
> > 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);

I would prefer for the arithmetic to be hidden away in a macro.
Both cases.

Also, the tests themselves: IS_VALID_xxx_AREA(), IS_VALID_yyy_AREA().
Sorry, can't think of good real names - if the documentation has it,
use that.

> } else {
>   DEBUG ((DEBUG_ERROR, "Invalid memory module size (0x%x) for
> CS#%d\n", AreaLength, Cs));
> }
> 
> Is above ok for you?

Yeah, it's a big improvement over listing each individual option.

/
    Leif