Re: [PATCH edk2-platforms v4 12/24] Silicon/NXP: Move RAM retrieval from SocLib

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

Hi Leif,

> > +  ARM_SMC_ARGS  ArmSmcArgs;
> >
> > -Routine Description:
> > +  ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> > +  ArmSmcArgs.Arg1 = -1;
> 
> Should this be SMC_UNK?

No. SMC_OK / SMC_UNK is returned values.
While x0, x1 are arguments.
I have explained this in the MemoryInitPeiLib.h

// This SMC call works in this way:
// x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size
// x1 >= number of DRAM regions to which DDR RAM is mapped : return x0: SMC_UNK
// 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return
//           x0: SMC_OK, x1: Base address of DRAM region,
//           x2: Size of DRAM region

> 
> >
> > +  ArmCallSmc (&ArmSmcArgs);
> >
> > +  if (ArmSmcArgs.Arg0 == SMC_OK) {
> > +    return ArmSmcArgs.Arg1;
> > +  }
> >

....

> > -      {
> > -        Found = TRUE;
> > -        break;
> > -      }
> > -      NextHob.Raw = GET_NEXT_HOB (NextHob);
> > +  Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> > +  ASSERT_EFI_ERROR (Status);
> 
> Slightly concerned here because we end up with a variable Status that
> is only *used* in DEBUG builds. That could lead to toolchain warnings.

I don't think this would cause build warnings in RELEASE builds. I have tested it.
Also the Status is frequently being handled in this way in other places in edk2:

https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/PlatformPeim.c#L90

> 
> And I was completely serious last time around, I am OK with the return
> value being cast away explicitly. What I meant with that is:
>   (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> 

I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from GetDramRegionsInfo,
But If we discard the return value it means we are OK with some RAM not being reported to UEFI firmware
and subsequently to OS. Isn't this a critical error ?

> /
>     Leif
> 
> > +
> > +  FdBase = (UINTN)FixedPcdGet64 (PcdFdBaseAddress);
> > +  FdTop = FdBase + (UINTN)FixedPcdGet32 (PcdFdSize);
> > +
> > +  // Declare memory regions to system