[edk2-devel] [PATCH edk2-platforms v4 12/24] Silicon/NXP: Move RAM retrieval from SocLib
Pankaj Bansal
pankaj.bansal at nxp.com
Thu May 7 07:28:30 UTC 2020
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58776): https://edk2.groups.io/g/devel/message/58776
Mute This Topic: https://groups.io/mt/73370132/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list