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

[Mike Looijmans]

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.

> Thanks,
> Michal
>




Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail