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

[Pavel Herrmann]

Hi,

On Tuesday 25 December 2012 12:37:55 Albert ARIBAUD wrote:
> 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).

More like it applies to me being stupid, and not checking whether the 
CONFIG_SYS_GBL_DATA_OFFSET is actually used in the code before trying to use 
it to modify the memory layout

> > 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. :)

Technically, the patch was quoted whole, but it goes on top of another, that 
added the DM and early heap macros to the board config, so its of no use as it 
is.

> 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?

We want the DM to replace most of the board-specific code (by providing a 
"driver" that is configured by board-specific values, essentialy encapsulating 
common init code independently on actual board and memory layout), so that the 
board-specific init only loads required drivers with correct parameters, 
instead of directly initializing the hardware (there was a thought of having a 
RAM driver, that would init the main memory when loaded, and provided 
relcation as a method/service, not sure if we want to go this far at the 
moment).
for this we need the DM to run in early (flash-based) phase. The DM itself 
cannot work without a heap, so we need one there.

Pavel Herrmann