Re: [edk2-devel] [edk2-platforms][PATCH 1/2] Platform/RaspberryPi/Drivers/ConfigDxe: fix bug in 3GB RAM logic

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

On 3/5/20 12:38 AM, Pete Batard wrote:
> On 2020.03.04 22:31, Andrei Warkentin wrote:
>> The original change*** used positive logic (PcdPi4GBEnabled), while the
>> upstreamed change uses negative logic (PcdRamLimitTo3GB), which
>> requires an additional condition, or it blows up on 1GiB and 2GiB boards.
> 
> Good catch!
> 
>> Tested on 2GB and 4GB boards (with limiting and without)
>>
>> *** https://github.com/pftf/edk2-platforms/\
>>      commit/968451beb7c9302517098abf72f7e42b57a0e024
>>
>> Signed-off-by: Andrei Warkentin <awarkentin@vmware.com>
>> ---
>>   Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c 
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index ccac8daa..5fca3c7a 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -288,7 +288,8 @@ ApplyVariables (
>>       DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
>>     }
>> -  if (mModelFamily >= 4 && PcdGet32 (PcdRamLimitTo3GB) == 0) {
>> +  if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) &&
> 
> Just a super minor nitpick, but if we want to be consistent with the 
> next line, where we use the comparison to the integer value as the 
> boolean, instead of using the result of the PcdGet operation itself, 
> then the condition above should be:
> 
>       if (mModelFamily >= 4 && PcdGet32 (PcdRamMoreThan3GB) != 0 &&

Yes please. Can the maintainer amend that change? It can be quicker to 
simply repost. With the change:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> Since it doesn't matter either way, I'm 100% fine with the patch as it 
> stands. It's just that I remember trying to go for an explicit integer 
> comparison for code clarity, on account that we went for integer PCDs 
> rather than boolean ones...
> 
>> +      PcdGet32 (PcdRamLimitTo3GB) == 0) {
>>       /*
>>        * Similar to how we compute the > 3 GB RAM segment's size in 
>> PlatformLib/
>>        * RaspberryPiMem.c, with some overlap protection for the 
>> Bcm2xxx register
>>
> 
> Reviewed-by: Pete Batard <pete@akeo.ie>
> 
> 
> 
>