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