diff mbox

[U-Boot] post, arm, memorytest: add support for arm based boards

Message ID 1306909447-19603-2-git-send-email-hs@denx.de
State Changes Requested
Headers show

Commit Message

Heiko Schocher June 1, 2011, 6:24 a.m. UTC
Signed-off-by: Heiko Schocher <hs@denx.de>
---
 post/drivers/memory.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

Comments

Wolfgang Denk June 1, 2011, 6:37 a.m. UTC | #1
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.

> +	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.

> +		} 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.

Best regards,

Wolfgang Denk
Heiko Schocher June 1, 2011, 6:54 a.m. UTC | #2
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
Mike Frysinger June 1, 2011, 1:51 p.m. UTC | #3
On Wednesday, June 01, 2011 02:54:30 Heiko Schocher wrote:
> Wolfgang Denk wrote:
> > Heiko Schocher wrote:
> >> --- 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.

so add bi_memsize to arm ?  it's the only arch that lacks it.
-mike
Heiko Schocher June 2, 2011, 5:53 a.m. UTC | #4
Hello Mike,

Mike Frysinger wrote:
> On Wednesday, June 01, 2011 02:54:30 Heiko Schocher wrote:
>> Wolfgang Denk wrote:
>>> Heiko Schocher wrote:
>>>> --- 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.
> 
> so add bi_memsize to arm ?  it's the only arch that lacks it.

Hmm.. I thought of that too, but wouldn;t it be better to use
gd->ram_size in post/drivers/memory.c, as this is defined in
global_data for all archs?

bye,
Heiko
Wolfgang Denk June 2, 2011, 12:40 p.m. UTC | #5
Dear Heiko Schocher,

In message <4DE72566.9040008@denx.de> you wrote:
> 
> Hmm.. I thought of that too, but wouldn;t it be better to use
> gd->ram_size in post/drivers/memory.c, as this is defined in
> global_data for all archs?

Indeed.  Can you do this, please?

Best regards,

Wolfgang Denk
Mike Frysinger June 2, 2011, 3:08 p.m. UTC | #6
On Thursday, June 02, 2011 01:53:42 Heiko Schocher wrote:
> Mike Frysinger wrote:
> > so add bi_memsize to arm ?  it's the only arch that lacks it.
> 
> Hmm.. I thought of that too, but wouldn;t it be better to use
> gd->ram_size in post/drivers/memory.c, as this is defined in
> global_data for all archs?

makes me wonder why we have bd->bi_memsize in the first place.

and how can this possibly work ?
arch/arm/lib/board.c:
	sprintf ((char *)memsz, "%ldk", (bd->bi_memsize / 1024) - pram);
-mike
Heiko Schocher June 3, 2011, 5:39 a.m. UTC | #7
Hello Mike,

Mike Frysinger wrote:
> On Thursday, June 02, 2011 01:53:42 Heiko Schocher wrote:
>> Mike Frysinger wrote:
>>> so add bi_memsize to arm ?  it's the only arch that lacks it.
>> Hmm.. I thought of that too, but wouldn;t it be better to use
>> gd->ram_size in post/drivers/memory.c, as this is defined in
>> global_data for all archs?
> 
> makes me wonder why we have bd->bi_memsize in the first place.
> 
> and how can this possibly work ?
> arch/arm/lib/board.c:
> 	sprintf ((char *)memsz, "%ldk", (bd->bi_memsize / 1024) - pram);
> -mike

Yep, good question ... maybe, no arm based board has defined

"#if defined(CONFIG_PRAM) || defined(CONFIG_LOGBUFFER)"

?

I can make a fix and change this to gd->ram_size?

bye,
Heiko
Wolfgang Denk June 3, 2011, 6:30 a.m. UTC | #8
Dear Heiko Schocher,

In message <4DE8739D.2040400@denx.de> you wrote:
> 
> Yep, good question ... maybe, no arm based board has defined
> 
> "#if defined(CONFIG_PRAM) || defined(CONFIG_LOGBUFFER)"

That's actually very likely.  ARM systems tended to be simple and not
use any such fancy features.

> I can make a fix and change this to gd->ram_size?

Please do!

Best regards,

Wolfgang Denk
diff mbox

Patch

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)
+	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;
+		} 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;
+			}
+		}
+	}
+#else
 	bd_t *bd = gd->bd;
 	*vstart = CONFIG_SYS_SDRAM_BASE;
 	*size = (bd->bi_memsize >= 256 << 20 ?
 			256 << 20 : bd->bi_memsize) - (1 << 20);
+#endif
 
 	/* Limit area to be tested with the board info struct */
 	if ((*vstart) + (*size) > (ulong)bd)