[U-Boot] [PATCH 3/4] fix memory corruption on versatile

[Albert ARIBAUD]

Hi Pavel,

On Mon, 24 Dec 2012 15:57:30 +0100, Pavel Herrmann
<morpheus.ibis at gmail.com> wrote:
> Hi,
> 
> On Monday 24 December 2012 14:56:03 Albert ARIBAUD wrote:
> > Hi Pavel,
> > 
> > On Mon, 24 Dec 2012 02:27:53 +0100, Marek Vasut <marex at denx.de> wrote:
> > > Dear Pavel Herrmann,
> > > 
> > > > ARM board.c doesnt respect CONFIG_SYS_GBL_DATA_OFFSET, nor do all boards
> > > > set it, so reorganize the memory a bit to avoid overlaps.
> > > > 
> > > > Signed-off-by: Pavel Herrmann <morpheus.ibis at gmail.com>
> > > 
> > > Ccing Albert
> > > 
> > > > ---
> > > > 
> > > >  include/configs/versatile.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/configs/versatile.h b/include/configs/versatile.h
> > > > index c9aed72..27ffffe 100644
> > > > --- a/include/configs/versatile.h
> > > > +++ b/include/configs/versatile.h
> > > > @@ -35,7 +35,7 @@
> > > > 
> > > >  #define CONFIG_DM_BLOCK
> > > >  #define CONFIG_SYS_EARLY_MALLOC
> > > >  #define CONFIG_SYS_EARLY_HEAP_ADDR	(CONFIG_SYS_INIT_RAM_ADDR + \
> > > > 
> > > > -						CONFIG_SYS_GBL_DATA_OFFSET - \
> > > > +						CONFIG_SYS_INIT_RAM_SIZE - \
> > > > 
> > > >  						CONFIG_SYS_EARLY_HEAP_SIZE - 64)
> > > >  
> > > >  #define CONFIG_SYS_EARLY_HEAP_SIZE	1024
> > > 
> > > Best regards,
> > > Marek Vasut
> > 
> > I can't seem to find this patch in my inbox or in the list archives.
> > 
> > Pavel, can you please repost it?
> > 
> > Amicalement,
> 
> I dont really understand why Marek CCed you, this patch is meant to go in the 
> DM tree, and i dont see why it should be considered for upstream.

Patches should not be meant for a tree in the first place; they should
be meant for the tree where they make most sense. IIUC, this patch
fixes an issue related not to DM per se but to ARM, which is why Marek
CC:ed me (and the list).

> The problem this is trying to solve is that some boards (like the versatile i 
> use, as it has upstream qemu support) do define a suspicious looking 
> CONFIG_SYS_GBL_DATA_OFFSET, but that macro is not used in the board_init_f. 
> instead the GD is placed under the bottom of stack.
> 
> This is perfectly fine when we only have GD and stack in memory, but I would 
> like to have the early heap in there somewhere, ideally between GD and stack. 
> When I set the macros (not knowing that it is actually ignored) to represent 
> this layout, the boot failed because the stack was overlapping the GD, so this 
> patch changed the layout to [stack][GD][heap][end].
> 
> the "optimal" solution would be to have universal (as in that all archs 
> board_init_f()s would respect them, and all board configs would have them) 
> macros for address of GD, early heap and stack independently (with the obvious 
> exception that for stack you have the highest address, while for other you 
> have the lowest), so that we can have the originally intended layout ([stack]
> [heap][GD][end]), or any other layout for that matter.
> 
> This, of course, may not be possible due to various limitations of various 
> archs, so I did not attempt to make any such patch, instead just quick-fix the 
> board i am using to test early DM initialization.
> 
> Hope this clears the confusion

It does; I think I understand the problem you are stating. The only
problem I have is that I am still unable to tell if Marek did quote the
whole patch or not. :)

Based on the assumption that this patch is complete as quoted, and on
your comments above, my comment would be (to both Marek and you) why do
you nead a heap during flash-based inits?

> Pavel Herrmann

Amicalement,
-- 
Albert.