RPi4 fixes to 3GB RAM limit logic

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

Posted by Andrei Warkentin via Groups.Io 5 years, 11 months ago

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.

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) &&
+      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
-- 
2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55459): https://edk2.groups.io/g/devel/message/55459
Mute This Topic: https://groups.io/mt/71737117/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Pete Batard 5 years, 11 months ago

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>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55461): https://edk2.groups.io/g/devel/message/55461
Mute This Topic: https://groups.io/mt/71737117/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Philippe Mathieu-Daudé 5 years, 11 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#55502): https://edk2.groups.io/g/devel/message/55502
Mute This Topic: https://groups.io/mt/71737117/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-