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

[Marcin Wojtas <mw@semihalf.com>]

2017-10-12 12:50 GMT+02:00 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?)


I guess nowhere, but please bear in mind, there are also custom
modules  (Armada-7040-DB has one) and on-board DRAMs - in such
circumstances it's up to the designer.

>
>
> > >
> > > 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.
>

Ok, I'll try to come up with something readable.

> > } 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.
>

Ok, thanks.
Marcin