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

[Tom Rini]

On Tue, Jun 23, 2015 at 08:24:23AM -0400, Vitaly Andrianov wrote:
> 
> 
> 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.

I want to answer your last two points together.  First, gd is nominally
used for things like "I cannot get this data again after relocation" or
"I must have this information passed around prior to relocation".

Next, there's already an infrastructure in the generic code and PowerPC
code for saying that we have a 32bit CPU but extra large memory.  Given
how the arch/*/lib/board.c consolidation went I'm not sure that there's
much more work needed to enable this for ARM than fixing up
arch/arm/include/asm/types.h (based on the PowerPC one) and setting a
few CONFIG options (which need to be Kconfig'd).

All that would then leave is needing to add a generic way to make sure
that we carve up the memory node in the DT to exclude what needs
excluding.  And this too is a very generic problem as many SoCs need
some way to cut out DDR and use it for some DSP or another.

So yes, I'd like to see this problem solved in a way that will benefit
not just Keystone 2 but other TI parts (I have vague recollections of
other parts having large amounts of memory being possible) and other
vendors too (which is good, since you can leverage a large amount of
work they already did, to make it happen :)).  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150623/0d43522e/attachment.sig>