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

From: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
To: Marek Vasut <marek.vasut@gmail.com>, u-boot@lists.denx.de
Cc: Marek Vasut <marek.vasut+renesas@gmail.com>,
	Alexey Brodkin <alexey.brodkin@synopsys.com>,
	Angelo Dureghello <angelo@sysam.it>,
	Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>,
	Hai Pham <hai.pham.ud@renesas.com>,
	Michal Simek <monstr@monstr.eu>,
	Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>,
	Tom Rini <trini@konsulko.com>, Wolfgang Denk <wd@denx.de>
Subject: Re: [PATCH 03/12] lmb: Add generic arch_lmb_reserve_generic()
Date: Sun, 12 Sep 2021 21:27:10 +0200	[thread overview]
Message-ID: <4d503594c9da872302e9afc79f070f7037ff2316.camel@gmail.com> (raw)
In-Reply-To: <20210910204718.17765-3-marek.vasut+renesas@gmail.com>

Am Freitag, dem 10.09.2021 um 22:47 +0200 schrieb Marek Vasut:
> The arc/arm/m68k/microblaze/mips/ppc arch_lmb_reserve()
> implementations
> are all mostly the same, except for a couple of details. Implement a
> generic arch_lmb_reserve_generic() function which can be parametrized
> enough to cater for those differences between architectures. This can
> also be parametrized enough so it can handle cases where U-Boot is
> not
> relocated to the end of DRAM e.g. because there is some other
> reserved
> memory past U-Boot (e.g. unmovable firmware for coprocessor), it is
> not
> relocated at all, and other such use cases.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Alexey Brodkin <alexey.brodkin@synopsys.com>
> Cc: Angelo Dureghello <angelo@sysam.it>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Cc: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> Cc: Hai Pham <hai.pham.ud@renesas.com>
> Cc: Michal Simek <monstr@monstr.eu>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
> V2: Reword code comment
> ---
>  include/lmb.h |  1 +
>  lib/lmb.c     | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/include/lmb.h b/include/lmb.h
> index 3c4afdf9f0..1984291132 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -122,6 +122,7 @@ lmb_size_bytes(struct lmb_region *type, unsigned
> long region_nr)
>  
>  void board_lmb_reserve(struct lmb *lmb);
>  void arch_lmb_reserve(struct lmb *lmb);
> +void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end,
> ulong align);
>  
>  /* Low level functions */
>  
> 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
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?

> +
> +		lmb_reserve(lmb, sp, bank_end - sp + 1);
> +		break;
> +	}
> +}
> +
>  static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  {
>  	arch_lmb_reserve(lmb);
-- 
- Daniel