[U-Boot] [PATCH] keystone2: use detected ddr3a size

[Vitaly Andrianov]

On 06/18/2015 11:57 AM, Tom Rini wrote:
> On Mon, Jun 15, 2015 at 12:42:49PM -0400, Vitaly Andrianov wrote:
>>
>>
>> On 06/15/2015 10:17 AM, Tom Rini wrote:
>>> On Mon, Jun 15, 2015 at 08:48:01AM -0400, Vitaly Andrianov wrote:
>>>
>>>> KS2 u-boot detects the ddr3a size installed to EVM. The detected size can
>>>> be used instead of environment variable. Because the ddr3 configuration is
>>>> done before relocation we cannot use a global variable to pass the
>>>> ddr3_size to ft_board_setup(). Instead we have to use the global data
>>>> structure.
>>>>
>>>> Because KS2 u-boot works in 32 bit address space the existing ram_size
>>>> global data filed cannot be used. The maximum, which the get_ram_size()
>>>> can detect is 2GB only. This patch creates the ddr3_size filed in the
>>>> arch_global_data structure, which is used for that purpose.
>>>>
>>>> Signed-off-by: Vitaly Andrianov <vitalya at ti.com>
>>>
>>> So we've got a few possibilities here, yes?  Since we have the ability
>>> to change the DDR modules on the board and read the sizes in the SPD
>>> information U-Boot is the place where the board can find out if we have
>>> say 1GB or 2GB of memory and thus has to be the one to correctly
>>> populate the device tree.  So the "fix" that we're talking about for
>>> Calxeda can't be applied here.
>>>
>>> But this also brings up http://patchwork.ozlabs.org/patch/281094/ (and
>>> the follow-up of http://patchwork.ozlabs.org/patch/291219/ and
>>> http://patchwork.ozlabs.org/patch/291247/) where no, we have a problem
>>> that we need to fix.
>>>
>> Hi Tom,
>>
>> If I understand correctly the patches above are about changing long
>> to unsigned long to accommodate possible 2GB of DDR size. Or to use
>> phys_addr_t for 64bit architecture. Did I miss something?
>
> No, but that is part of your actual problem.  You have 2GB of DDR (or
> more in some cases) that you want to report.
>
>> The problem with KS2 platforms is that it is a 32 bit architecture
>> which uses LPAE. So, the EVMs may have more than 2GB memory
>> (typically 4 or 8 GB), but u-boot sees only 2GB maximum. That is
>> what get_ram_size() can detect.
>
> Right.  So you're in the same problem area as the highbank board (and
> some Tegra boards too I think).
>
>> Also it is not always possible to use SPD data to detect the DDR
>> size because not all EVMs use SODIMM. Some of them use DDR3 chips
>> populated to the main board.
>
> Right.  But on the ones you added support for the SPD data to, you do,
> right?
>

On EVM that has SODIMMs with SPD eeprom we already read SPD data and 
detected the size. That happen before relocation. Why do we need to 
repeat that step again after relocation. Also it make the 
"after-relocation" code conditional. The EVM w/o SPD info has to get the 
size by some different means. Having one simple variable and the global 
data structure simply solve the issue. This variable is Keystone2 
specific and doesn't affect any other architecture. If you don't like 
that the size is represented in GB we can make the variable 64 bit.

>> Even if we uses SPD data, we detect the DDR3 size before relocation.
>> So, I believe, instead of reading the SPD EEPROM and calculate the
>> size again, it is easier just to pass the ddr3 size through the
>> global_data.
>
> Well we need to do something, certainly.  The problem is that we need to
> populate the device tree for the kernel with the correct amount of
> memory.  Today we have a system that essentially forces what we have
> stored today in gd to be what we populate.  This is wrong in the LPAE
> case. In the case of highbank, something else has already correctly
> populated the DT with the memory sizes and a patch has been made to say
> "lets just set CONFIG_NR_DRAM_BANKS to 0 so we can avoid that 'fixup'".
> This won't help Keystone as U-Boot is where we somehow know how much
> memory there really is.
>
> Today, a bit further down in board/ti/ks2_evm/board.c than this patch
> shows you play some games to correct the DT node.  And part of the
> problem is that if we add "ddr3_size" to just the keystone DT we've made
> a very specific work-around for this general problem.  You're still
> having to play games to know that you shoved a >32bit value into a 32bit
> variable.
>
> So yes, I think you need to structure the code such that you can call a
> function to read the SPD information and see how big your memory is, and
> then go poke arch/arm/lib/bootm-fdt.c::arch_fixup_fdt() to be something
> like:
> __weak void board_calc_memory_size(u64 *start, u64 *end)
> {
>    .. current for-loop
> }
>
> int arch_fixup_fdt(void *blob)
> {
>    board_calc_memory_size(&start, &end);
>    fdt_fixup_memory_banks(...);
>    ...
> }
>
> And then have keystone fill in a board_calc_memory_size() and even
> populate the real # of banks and such if you want.
>

We have situations when not all EVM DDR3 memory is available for Linux 
kernel. Some portion (sometimes more that 50%) of the memory can be 
dedicated for DSPs. In that case kernel even doesn't have to know about 
that memory. In that case u-boot is responsible to configure DDR3 
controller for the entire DDR3 memory, but Linux shouldn't see the 
entire memory. We still have to "play some game". Some customers ask for 
board specific environment variables to override default memory 
configuration in fdt.

The board_calc_memory_size(), which you offered is an excellent solution 
to perform the custom arch_fdt_fixup. It is even possible to use it for 
Keystone2 requirements. But it will require significant redesign and 
testing of existing code, and still "playing some game".

So, if you strongly disagree to add one (for Keystone only) variable to 
global data, forget about it. I'll work on using your approach later.

Thanks,
-Vitaly