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

["Pete Batard" <pete@akeo.ie>]

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

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>