On Thu, Apr 16, 2015 at 03:52:16PM +0200, Albert ARIBAUD wrote:
Hello Matt,
On Tue, 14 Apr 2015 14:07:17 -0400, Matt Porter mporter@konsulko.com wrote:
common/image.c currently implicitly depends on CONFIG_NR_DRAM_BANKS when CONFIG_ARM is enabled. Make this requirement explicit.
Signed-off-by: Matt Porter mporter@konsulko.com
common/image.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image.c b/common/image.c index 162b682..73c24f5 100644 --- a/common/image.c +++ b/common/image.c @@ -461,7 +461,7 @@ phys_size_t getenv_bootm_size(void) tmp = 0;
-#if defined(CONFIG_ARM) +#if defined(CONFIG_ARM) && defined(CONFIG_NR_DRAM_BANKS) return gd->bd->bi_dram[0].size - tmp; #else return gd->bd->bi_memsize - tmp;
I am not entirely fond of a symbol's existence conditioning some code which does not actually use the symbol. I do understand the dependency here -- that bi_dram[0] is meaningful only if CONFIG_NR_DRAM_BANKS is 1 or more -- but then, why does this code not depend on the value of the symbol? Makes me think the patch is not complete and the code should be fixed to depend on the value of CONFIG_NR_DRAM_BANKS.
The problem is that CONFIG_NR_DRAM_BANKS means both "I have DRAM" and "I have X number of DRAM banks". In turn include/asm-generic/u-boot.h will only say we have gd->bd->bi_dram if CONFIG_NR_DRAM_BANKS is set so we can only reference that field when it's also set. Otherwise we get a compile error about no such member.
There's a further argument (as I read and re-read getenv_bootm_size()) that getenv_bootm_clsize() should be cleaned-up / re-worked as it defaults to too large of a value (which is why stuff gets relocated "high" and then Linux doesn't see it and then people do initrd_high=0xffffffff and fdt_high=0xffffffff) but that's unrelated to this patch I think.