From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Schocher <hs@denx.de>
Date: Wed, 01 Jun 2011 08:54:30 +0200
Subject: [U-Boot] [PATCH] post, arm,
 memorytest: add support for arm based boards
In-Reply-To: <20110601063728.9F731CF5CA6@gemini.denx.de>
References: <1306909447-19603-1-git-send-email-hs@denx.de>
	<1306909447-19603-2-git-send-email-hs@denx.de>
	<20110601063728.9F731CF5CA6@gemini.denx.de>
Message-ID: <4DE5E226.3090008@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Heiko Schocher,
> 
> In message <1306909447-19603-2-git-send-email-hs@denx.de> you wrote:
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>  post/drivers/memory.c |   20 ++++++++++++++++++++
>>  1 files changed, 20 insertions(+), 0 deletions(-)
>>
>> diff --git a/post/drivers/memory.c b/post/drivers/memory.c
>> index b7943ef..47b312d 100644
>> --- a/post/drivers/memory.c
>> +++ b/post/drivers/memory.c
>> @@ -455,10 +455,30 @@ static int memory_post_tests (unsigned long start, unsigned long size)
>>  __attribute__((weak))
>>  int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
>>  {
>> +#if defined(CONFIG_ARM)
> 
> This is a weak function, so there should be no need to have #ifdef's
> in there.
>
> Just define your own code as you need it.

Yes (I did this for my case, as I use it in nand_spl code,
and therefore I need a "own" function, because there I have no
bd ) ... but, for arm there is no bd->bi_memsize! ... so this
file fails compiling. Independent, if it gets replaced by
another function.

>> +	bd_t *bd = gd->bd;
>> +	int i;
>> +
>> +	*size = 0;
>> +	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
>> +		if (i == 0) {
>> +			*vstart = bd->bi_dram[0].start;
>> +			*size += bd->bi_dram[i].size;
> 
> This is a constant part and should be moved out of the loop.  Then
> you can also get rid of th if...else clause.

Yep, you are right! Change this.

>> +		} else {
>> +			if (bd->bi_dram[i].start ==
>> +			(bd->bi_dram[i - 1].start + bd->bi_dram[i - 1].size)) {
>> +				*size += bd->bi_dram[i].size;
>> +			} else {
>> +				break;
> 
> So how do you handle non-contiguous memory banks?  It appears these
> are quite frequent on ARM these days.

Actually, I could not handle this. Ok, I add a comment, that this
function is actually only valid for contiguous mem banks.

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany