Re: [edk2-devel] [edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 10/14/19 1:44 PM, Pete Batard wrote:
> Hi Philippe,
> 
> On 2019.10.14 11:01, Philippe Mathieu-Daudé wrote:
>> Hi Pete,
>>
>> On 10/11/19 1:07 PM, Pete Batard wrote:
>>> The board revision is the proper channel to use to detect the amount of
>>> RAM available as bits [20-22] report the effective RAM size for the 
>>> board
>>> starting with 256 MB (000b) and doubling in size for each value.
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>> Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>> | 18 ++++++++++++------
>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git 
>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>
>>> index b5dcff897a59..5abc82b8d363 100644
>>> --- 
>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>
>>> +++ 
>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>
>>> @@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
>>>     )
>>>   {
>>>     EFI_STATUS Status;
>>> -  UINT32 Base;
>>> -  UINT32 Size;
>>> +  UINT32 BoardRevision = 0;
>>> -  Status = mFwProtocol->GetArmMem (&Base, &Size);
>>> +  // Note: Type 19 addresses are expressed in KB, not bytes
>>> +  mMemArrMapInfoType19.StartingAddress = 0;
>>
>> Now you assume the ARM base RAM address is always 0, why?
> 
> Because, in the case that is of interest to us here (Broadcom SoCs used 
> for the various Raspberry Pi platforms), this is what the documentation 
> says.
> 
> If you look at something like 
> https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf 
> (which, as per the Pi Foundation, also applies to the later models) and 
> especially the memory layout graphic that you see early in the document, 
> you can find that the ARM base RAM address is indeed set to 0 always.
> 
> At this stage, we have no reason to think that we are going to contend 
> with a model where the RAM base address isn't 0.

OK. I have no idea what are Broadcom plans, I read somewhere they were 
going to use tricks to allow more than 4GB of RAM on the Raspberry Pi 4, 
so I was wondering, since they provide a firmware API call to get the 
RAM base address.

Maybe you can add a comment that we expect all following boards to use 
RAM base at 0 (simply replying to this email, and eventually Leif would 
amend it previous to push).

Otherwise your patch looks OK.

Regards,

Phil.

>>> +  // The minimum RAM size used on any Raspberry Pi model is 256 MB
>>> +  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
>>> +  Status = mFwProtocol->GetModelRevision (&BoardRevision);
>>>     if (Status != EFI_SUCCESS) {
>>> -    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", 
>>> Status));
>>> +    DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - 
>>> defaulting to 256 MB: %r\n", Status));
>>>     } else {
>>> -    mMemArrMapInfoType19.StartingAddress = Base / 1024;
>>> -    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
>>> +    // 
>>> www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md 
>>>
>>> +    // Bits [20-22] indicate the amount of memory starting with 
>>> 256MB (000b)
>>> +    // and doubling in size for each value (001b = 512 MB, 010b = 
>>> 1GB, etc.)
>>> +    mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 
>>> 0x07;
>>>     }
>>> +  mMemArrMapInfoType19.EndingAddress -= 1;
>>>     LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, 
>>> mMemArrMapInfoType19Strings, NULL);
>>>   }
>>>
>>
> 
> 
> 
>