[U-Boot] [PATCH] common/memsize.c: restore content of the base address

[Wolfgang Denk]

Dear Patrick,

In message <885aaa3abdfe440591ea271f92ab42ff at SFHDAG6NODE3.st.com> you wrote:
> 
> > You mean you are running this code from the very memory you are sizing?  This
> > is fundamentally broken.  You must not do this!
> 
> Yes I do it, sorry if it is a error.
> 
> But it is recommended in ./doc/README.arm-relocation:

Is it?  I don't think so.

>      At board level:
> 
> 	dram_init(): bd pointer is now at this point not accessible, so only
> 	detect the real dramsize, and store it in gd->ram_size. Bst detected
> 	with get_ram_size().

Where do  you read that this allows runing the code from the same
memory which is sized/tested?  I cannot see any such claim.  It is
always supposed that you run from a totally different, independent
memory type - in the olden days usually parallel NOR flash, nor some
on-chip-memory, but never ever the RAM you are testing!

> And before to setup by board I check on many device with ARM architecture.
> Most the time U-Boot loaded at beginning of the RAM, relocated at the end of the RAM 
> and dram_init use get_ram_size

But definitely not in this order!  The sequence should always be:
initialize the RAM and determine it's size; load U-Boot and relocate
it to the (now known) end of RAM (resp. to some lower address,
depending on any reservations made for shared display buffer, shared
log buffer, PRAM, etc.).

> Example1 :
>   ./board/toradex/colibri_imx6/colibri_imx6.c
>   int dram_init(void)
>   {
> 	/* use the DDR controllers configured size */
> 	gd->ram_size = get_ram_size((void *)CONFIG_SYS_SDRAM_BASE,
> 				    (ulong)imx_ddr_size());
> 
> 	return 0;
>   }

What do you mean to show by this?  It just means that dram_init()
must be running from some other memory (OCM), too.

> So I just do the same for my arm/armv7 platform.

No, apparently not.

> On ARM device device, I think we don't have the banks...

So you must run this (typically int he SPL) fom some on-chip memory.

>  I agree that the function is not safe in this case.
> So I will change my board to remove this call....

That's the wrong conclusion. get_ram_size() is not only useful to
measure the size of the RAM, it also catches 95% of the typical
in-field memory errors.  When you see a board stuk or crashing after
reporting a memory size that differs from what you expect, you don't
have to search long for the culprit.

> > For the test to work correctly, max test size should be chosen at least twice as
> > big as the maximum possible memory configuration.
> 
> This requirement seems strange to me

Then you have not thought about what the code does in this case,
especially on memory controllers where the initialization of the
cntroller includes setting the maximum access size.  Especially on
board which can be shipped with different memory types / sizes
installed.  Here you can find the real end of the memory only by
verifying that there is no memory any more "above" the expected
range, sothe test range must be (at least) twice as big. [And things
get funny when you _do_ find memory at such locations - then again
it's time to call the hardware guys to fixz the board.]

> I have 32 bits ARM platform, with addressable RAM area 1GiB

Not all memory controllers are so primitive.

> Ok, I will remove the call of the code on my board, even if over arm platform use this function in the same context.

You should keep the functionality, but move it to where it belongs,
i. e. to the SPL running from OCM.

> But it is the case that you indicate it is not working:
> 	max test size should be chosen at least twice as big as the maximum possible memory configuration.

But when we tested it all the many times in the past, no such
problem could be found.  And the supposed "fix" caused boards to
hang/crash.

> 	When no overlap is detected, return maxsize but don't restore *base

get_ram_size() is supposed to be non-destructive, i. e. _all_ memory
locations should be the same before and after running this code. In
our tests this was the case (but I admit that this has not been
tested for a long time - probably not since the last discussion
about this "fix").

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
WARNING:  This Product Attracts Every Other Piece  of  Matter in  the
Universe, Including the Products of Other Manufacturers, with a Force
Proportional  to the Product of the Masses and Inversely Proportional
to the Distance Between Them.