[U-Boot] [PATCH] board: topic: Detect RAM size at boot

[Igor Grinberg]

On 11/22/16 16:54, Mike Looijmans wrote:
> On 22-11-16 12:00, Michal Simek wrote:
>> On 21.11.2016 09:30, Mike Looijmans wrote:
>>> Miami boards can have memory sizes of 256M, 512M or 1GB. To
>>> prevent requiring separate bootloaders for each variant, just
>>> detect the RAM size at boot time instead of relying on the
>>> devicetree information.
>>> 
>>> Signed-off-by: Mike Looijmans <mike.looijmans at topic.nl> --- 
>>> board/topic/zynq/board.c          | 39
>>> +++++++++++++++++++++++++++++++++++++++ 
>>> configs/topic_miami_defconfig     |  1 + 
>>> configs/topic_miamiplus_defconfig |  1 + 3 files changed, 41
>>> insertions(+)
>>> 
>>> diff --git a/board/topic/zynq/board.c b/board/topic/zynq/board.c 
>>> index a95c9d1..8a5765e 100644 --- a/board/topic/zynq/board.c +++
>>> b/board/topic/zynq/board.c @@ -1 +1,40 @@ +/* + * (C) Copyright
>>> 2016 Topic Embedded Products + * + * SPDX-License-Identifier:
>>> GPL-2.0+ + */ + +/* + * Miami boards can have memory sizes of
>>> 256M, 512M or 1GB. To prevent needing + * separate bootloaders
>>> for each variant, just detect the RAM size at boot time + *
>>> instead of relying on the devicetree information. + */ +#define
>>> CONFIG_SYS_SDRAM_BASE    0 +#define CONFIG_SYS_SDRAM_SIZE
>>> topic_get_sdram_size()
>> 
>> I am not happy with this but I see where you go.
> 
> Of the several options I tried to "overload" the memory init
> functions, this was basically the lesser evil...
> 
> Maybe it would be possible to make some changes to the generic Zynq
> board.c file to facilitate overlaying the code?
> 
> Note that the memory detection would work on any board, not just the
> Topic boards. I agree that devicetree is considered the best thing
> since frozen pizza, but for the Zynq it looks as though detection is
> much simpler than static configuration.
> 
>> 
>>> +#define CONFIG_SYS_SDRAM_SIZE_MAX 0x40000000u + +static unsigned
>>> int topic_get_sdram_size(void); + #include
>>> "../../xilinx/zynq/board.c" + +#include <fdt_support.h> + +int
>>> ft_board_setup(void *blob, bd_t *bd) +{ +
>>> fdt_fixup_memory(blob, (u64)CONFIG_SYS_SDRAM_BASE,
>>> (u64)gd->ram_size); + +    return 0; +}
>> 
>> This action is taken at arch_fixup_fdt(). Can you please confirm
>> that this is really needed? And it is not done there? That you
>> don't duplicate stuff here.
> 
> If I omitted this step, the Linux devicetree would not be adjusted
> (during bootm) and would still report its default. I needed both
> arch_fixup_fdt and this extra line to properly adjust the Linux
> devicetree for booting.
> 
> 
>> 
>>> + +void dram_init_banksize(void) +{ +    gd->bd->bi_dram[0].start
>>> = CONFIG_SYS_SDRAM_BASE; +    gd->bd->bi_dram[0].size =
>>> gd->ram_size; +}
>> 
>> This should be fine.
>> 
> 
> A note here, if you compile for Zynq with  CONFIG_SYS_SDRAM_BASE and
> CONFIG_SYS_SDRAM_SIZE set, there will be no implementation of
> 'dram_init_banksize' and the system will fail to boot.
> 
> It might be wise to put this snippet into the zynq/board.c in the
> "#else" clause. I'll send a separate patch for that if you agree.
> 
> 
>>> + +unsigned int topic_get_sdram_size(void) +{ +    /* Detect and
>>> fix ram size */ +    return get_ram_size((void
>>> *)CONFIG_SYS_SDRAM_BASE, +
>>> CONFIG_SYS_SDRAM_SIZE_MAX);
>> 
>> In the first look I didn't know that get_ram_size is u-boot
>> function for memsize detection.
> 
> Indeed, the name "get_ram_size" doesn't really match what it actually
> does, but the function's documentation is quite good.
> 
> I found "get_ram_size" and "ft_board_setup" while browsing the code
> of other boards to see how they handled dynamic configuration.

+1

That's how things works in U-Boot for years...

-- 
Regards,
Igor.