efi_loader: replace board_get_usable_ram_top by gd->ram_top

As gd->ram_top = board_get_usable_ram_top() in board_r
the EFI loader don't need to call this function again and after relocation.

This patch avoid issue if board assumed that this function is called
only one time and before relocation.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

https://source.denx.de/u-boot/custodians/u-boot-stm/-/pipelines/7399


 lib/efi_loader/efi_memory.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

On 7/9/21 12:46 PM, Patrick Delaunay wrote:
> As gd->ram_top = board_get_usable_ram_top() in board_r
> the EFI loader don't need to call this function again and after relocation.
>
> This patch avoid issue if board assumed that this function is called
> only one time and before relocation.

Hello Patrick,

Which implementation of board_get_usable_ram_top() assumes that it is
called only once before relocation? Please, mention this in the commit
message.

gd->ram_top is set in multiple places:

arch/arm/mach-rockchip/spl.c:149:       gd->ram_top = gd->ram_base +
get_effective_memsize();
arch/arm/mach-rockchip/spl.c:150:       gd->ram_top =
board_get_usable_ram_top(gd->ram_size);
arch/arm/cpu/armv8/fsl-layerscape/spl.c:114:    gd->ram_top =
gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size;

I guess you refer to:

common/board_f.c:345:   gd->ram_top = gd->ram_base +
get_effective_memsize();
common/board_f.c:346:   gd->ram_top = board_get_usable_ram_top(gd->mon_len);

I would not expect board_get_usable_ram_top(gd->mon_len) and
board_get_usable_ram_top(0) to be the same. So, please, describe in your
patch why you assume that board_get_usable_ram_top(gd->mon_len) is the
value we should use.

Best regards

Heinrich

>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
> https://source.denx.de/u-boot/custodians/u-boot-stm/-/pipelines/7399
>
>
>   lib/efi_loader/efi_memory.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index be2f655dff..f5bc37a20a 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -7,7 +7,6 @@
>
>   #include <common.h>
>   #include <efi_loader.h>
> -#include <init.h>
>   #include <malloc.h>
>   #include <mapmem.h>
>   #include <watchdog.h>
> @@ -731,7 +730,7 @@ efi_status_t efi_add_conventional_memory_map(u64 ram_start, u64 ram_end,
>
>   __weak void efi_add_known_memory(void)
>   {
> -	u64 ram_top = board_get_usable_ram_top(0) & ~EFI_PAGE_MASK;
> +	u64 ram_top = gd->ram_top & ~EFI_PAGE_MASK;
>   	int i;
>
>   	/*
>

Hi Heinrich

On 7/18/21 9:29 AM, Heinrich Schuchardt wrote:
>
>
> On 7/9/21 12:46 PM, Patrick Delaunay wrote:
>> As gd->ram_top = board_get_usable_ram_top() in board_r
>> the EFI loader don't need to call this function again and after 
>> relocation.
>>
>> This patch avoid issue if board assumed that this function is called
>> only one time and before relocation.
>
> Hello Patrick,
>
> Which implementation of board_get_usable_ram_top() assumes that it is
> called only once before relocation? Please, mention this in the commit
> message.
>
I see the issue occur in stm32mp platform, in 
arch/arm/mach-stm32mp/dram_init.c

I made some treatment in this function = parsing of device tree to found the

reserved memory - reserved for OP-TEE - and cache attribute update.


Before the commit 7dc6068fc10d ("stm32mp: Increase the reserved memory in

board_get_usable_ram_top"), mmu_set_region_dcache_behaviour was called

unconditionally, and cause issue when the board_get_usable_ram_top()

was called in efi code (after relocation)


So I already correct this issue with the added test:

+    /* before relocation, mark the U-Boot memory as cacheable by default */
+    if (!(gd->flags & GD_FLG_RELOC))
+        mmu_set_region_dcache_behaviour(reg, size, DCACHE_DEFAULT_OPTION);


And I propose the patch to avoid issue on other platform...


But I don't correctly handle the case total_size = 0....


> gd->ram_top is set in multiple places:
>
> arch/arm/mach-rockchip/spl.c:149:       gd->ram_top = gd->ram_base +
> get_effective_memsize();
> arch/arm/mach-rockchip/spl.c:150: gd->ram_top =
> board_get_usable_ram_top(gd->ram_size);

yes

=> void board_init_f(ulong dummy)

> arch/arm/cpu/armv8/fsl-layerscape/spl.c:114: gd->ram_top =
> gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size;
>
yes

=>voidboard_init_f(ulong dummy)


The booth case are only for SPL not for U-Boot proper ...

where ram_top is only handle in common/board_f.c


> I guess you refer to:
>
> common/board_f.c:345:   gd->ram_top = gd->ram_base +
> get_effective_memsize();
> common/board_f.c:346:   gd->ram_top = 
> board_get_usable_ram_top(gd->mon_len);
>

Yes

in  first analysis, I assume that the computation is done one time

and we don't have reason to do it again.


> I would not expect board_get_usable_ram_top(gd->mon_len) and
> board_get_usable_ram_top(0) to be the same. So, please, describe in your
> patch why you assume that board_get_usable_ram_top(gd->mon_len) is the
> value we should use.


No, I don't assume it should be the same value....

and it shoul be the case for stm32mp arch.


When I propose the patch, I don't think that board_get_usable_ram_top should

use, except to select the U-Boot relocation address.


But after reflection and a other check I agree that for EFI point of 
view ram_top

should be respected (to present correct memory mapping to kernle)

and it is more a issue in stm32mp platform...


=> I need to support correctly board_get_usable_ram_top(0)



>
> Best regards
>
> Heinrich
>
>>

conclusion: I abandon this patch.


Patrick