| Message ID | 20210910204718.17765-3-marek.vasut+renesas@gmail.com |
|---|---|
| State | Accepted |
| Commit | 1274698d13ce1dfd00275b821b512e17cdc88d98 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [01/12] lmb: Always compile arch_lmb_reserve() into U-Boot on arm | expand |
On Fri, Sep 10, 2021 at 10:47:09PM +0200, Marek Vasut wrote: > 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> Reviewed-by: Tom Rini <trini@konsulko.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);
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.
On Fri, Sep 10, 2021 at 10:47:09PM +0200, Marek Vasut wrote: > 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> > Reviewed-by: Tom Rini <trini@konsulko.com> Applied to u-boot/next, thanks!
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 */ + 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; + + lmb_reserve(lmb, sp, bank_end - sp + 1); + break; + } +} + static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob) { arch_lmb_reserve(lmb);
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(+)