Re: [PATCH 03/12] lmb: Add generic arch_lmb_reserve_generic()

[Marek Vasut <marex@denx.de>]

On 9/12/21 9:27 PM, Daniel Schwierzeck wrote:

[...]

>> diff --git a/lib/lmb.c b/lib/lmb.c
>> index 7bd1255f7a..793647724c 100644
>> --- a/lib/lmb.c
>> +++ b/lib/lmb.c
>> @@ -12,6 +12,10 @@
>>   #include <log.h>
>>   #include <malloc.h>
>>   
>> +#include <asm/global_data.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>   #define LMB_ALLOC_ANYWHERE	0
>>   
>>   static void lmb_dump_region(struct lmb_region *rgn, char *name)
>> @@ -113,6 +117,37 @@ void lmb_init(struct lmb *lmb)
>>   	lmb->reserved.cnt = 0;
>>   }
>>   
>> +void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end,
>> ulong align)
>> +{
>> +	ulong bank_end;
>> +	int bank;
>> +
>> +	/*
>> +	 * Reserve memory from aligned address below the bottom of U-
>> Boot stack
>> +	 * until end of U-Boot area using LMB to prevent U-Boot from
>> overwriting
>> +	 * that memory.
>> +	 */
>> +	debug("## Current stack ends at 0x%08lx ", sp);
>> +
>> +	/* adjust sp by 4K to be safe */
> 
> nit: comment doesn't fit anymore
> 
>> +	sp -= align;
>> +	for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> +		if (!gd->bd->bi_dram[bank].size ||
>> +		    sp < gd->bd->bi_dram[bank].start)
>> +			continue;
>> +		/* Watch out for RAM at end of address space! */
>> +		bank_end = gd->bd->bi_dram[bank].start +
>> +			gd->bd->bi_dram[bank].size - 1;
>> +		if (sp > bank_end)
>> +			continue;
>> +		if (bank_end > end)
>> +			bank_end = end - 1;
> 
> isn't that all already taken care of when initializing gd->ram_top as
> well as gd->relocaddr and gd->start_addr_sp (based on gd->ram_top)? So
> gd->ram_top is already guaranteed to be less or equal some DRAM bank
> end address. AFAIK arch_lmb_reserve() should just protect the U-Boot
> area from gd->start_addr_sp up to gd->ram_top as well as the stack area
> from the current stack pointer up to the initial stack pointer in gd-
>> start_addr_sp. Also you changed all callers to always pass gd->ram_top
> in "ulong end". Thus I think arch_lmb_reserve_generic() could omit
> those redundant checks and could be simplified to:
> 
>      sp -= align;
>      lmb_reserve(lmb, sp, gd->ram_top - sp);
> Or am I overlooking something? Is that a valid use case to have U-Boot
This would not allow reserving space at the end of DRAM bank for e.g. 
DSP firmware which just has to be at the end of DRAM bank. That's why 
there is this passing of gd->ram_top into this function and it's not 
part of the function, to prepare for this use case.

> area and stack area in different memory banks? If yes, shouldn't then
> lmb_reserve() be called twice by arch_lmb_reserve() to protect stack
> area AND U-Boot area?

The U-Boot stack is below relocated U-Boot, and there is other stuff 
between those two, so that's why this isn't called twice for stack and 
U-Boot.