| Message ID | 1306909447-19603-2-git-send-email-hs@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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
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
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 --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)
Signed-off-by: Heiko Schocher <hs@denx.de> --- post/drivers/memory.c | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)