Re: "ARM: multi_v7_defconfig: Enable shmobile platforms" breaks Tegra20 multi_v7_defconfig boot

[Paul Walmsley <pwalmsley-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 04/01/2015 12:55 PM, Stephen Warren wrote:
> (Dropping people likely not interested in Tegra U-Boot from explicit Cc)
>
> On 04/01/2015 11:48 AM, Paul Walmsley wrote:
>> On 04/01/2015 11:14 AM, Stephen Warren wrote:
>>> On 03/26/2015 12:57 PM, Stephen Warren wrote:
>>>> On 03/26/2015 12:35 PM, Paul Walmsley wrote:
>>>>> On Wed, 25 Mar 2015, Stephen Warren wrote:
>>>>>
>>>>>> On 03/25/2015 04:00 PM, Paul Walmsley wrote:
>>>>>>> On Wed, 25 Mar 2015, Tyler Baker wrote:
>>>>>>>> On 25 March 2015 at 11:03, Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>> Looks like commit 4a3a6f86693922b29cf829c63f652b057f14619e ("ARM:
>>>>>>>>> multi_v7_defconfig: Enable shmobile platforms") breaks Tegra20
>>>>>>>>> multi_v7_defconfig boot.
>>> ...
>>>>>>>> Can you try to shift your kernel load address around a bit? From
>>>>>>>> experience with the boards from kernelci.org we find that as the
>>>>>>>> multi
>>>>>>>> v7 kernel size increases they can clobber memory when they get
>>>>>>>> decompressed.
>>>>>>>
>>>>>>> Thanks, that was it:
>>>>>>>
>>>>>>> http://nvt.pwsan.com/pub/pwalmsley-tester/testlogs/test_20150325144058_6af714b069dc278d5d8e1b7afc13568f71d9aba8/20150325144058/boot/tegra20-trimslice/tegra20-trimslice/multi_v7_defconfig_log.txt 
>>>>>>>
>>>>>>>
>>> ...
>>>>>> Interesting. Do the values in U-Boot's default environment work
>>>>>> correctly
>>>>>
>>>>> No idea, I haven't tried.  (The load addresses I used are 
>>>>> observable in
>>>>> the boot logs above.)
>>>>
>>>> Sure. I was hoping you'd try it out since you already had the setup to
>>>> repro the issue.
>>>>
>>>> It'd be good if your test-bed used the built-in U-Boot variables 
>>>> too, so
>>>> we're testing them.
>>>
>>> I've reproduced the original problem, and then validated that using
>>> the addresses from U-Boot's default environment (at least with a ToT
>>> U-Boot that I just built) does indeed solve it.
>>>
>>> I'd like to re-iterate that the test bed should be using the values
>>> from U-Boot's environment rather than making up its own. That way:
>>>
>>> * The value the test bed uses for the kernel load address likely
>>> overlaps where the kernel decompressor writes to for even slightly
>>> large kernels. This means the decompressor must move itself before
>>> decompressing. IIUC, there's zero guarantee that moving the
>>> decompressor won't overwrite the DTB, since IIUC the decompressor uses
>>> zero knowledge of the DTB location. In other words, if the compressed
>>> kernel is loaded somewhere that's likely to require it to move itself,
>>> there's a real risk of the exact same problem cropping up again in the
>>> future if the kernel happens to overwrite the DTB during relocation.
>>>
>>> * The values are part of every Tegra U-Boot port (and hopefully for
>>> other SoCs too given any distro using boot.scr with
>>> config_distro_bootcmd.h will expect the variables to exist), so for
>>> future (Tegra) chips you won't have to adjust the test bed if RAM
>>> layout changes.
>>>
>>> * Those values get testing, so we'll find out if the ever don't work.
>>> We get more test coverage.
>>
>> Sounds reasonable, I'll try to get to this at some point soon.
>>
>> BTW, it might be worth changing U-boot CONFIG_LOADADDR to point to the
>> value you define for kernel_addr_r.  That would reinforce to folks who
>> aren't using the U-boot scripts that they should use that address for
>> loading their kernel.
>
> Hmm. Our values for CONFIG_LOADADDR and CONFIG_SYS_LOADADDR in U-Boot 
> seem to be a mess.
>
> Essentially they are used for the same semantic purpose, it's just 
> that different U-Boot features use one or the other. I propose we make 
> them the same value. There are certainly many other boards that do 
> (and many that don't, strangely).

That sounds fine to me.  I got totally confused trying to figure out the 
difference between CONFIG_LOADADDR and CONFIG_SYS_LOAD_ADDR.

>
> I'm not sure that pointing those at the same location as kernel_addr_r 
> is best. I'd expect modern scripts to explicitly use one of the 
> type-specific (kernel, DT, script, initrd, ...) variables and never 
> rely on simple $loadaddr. How about we point these variables somewhere 
> that doesn't overlap with any of the memory regions 
> "reserved"/intended for a specific purpose?
>
> In other words, I'd suggest 0x90200000 as the value for Tegra30+; 
> that's consistent with the default values of $scriptaddr and 
> $pxefile_addr_r, i.e. 1MB beyond the latter, and well out of the way 
> from anything else.
>
> Does all that sound reasonable? If so, I'll send a patch for U-Boot.

I always used to treat CONFIG_LOADADDR as the "recommended" load address 
for the kernel, as did my colleagues.  But that was years ago in the 
days of uImages and things have changed a lot since then.  So to be 
perfectly honest, I don't know what the right path forward is.  I'd hate 
to add more work to your plate, but maybe we should ask on the U-boot 
mailing lists to see what folks there would recommend?  If you're busy 
I'd be happy to send the initial query, just let me know.

Otherwise, I think you probably know U-boot better than I, so ultimately 
I'm fine with your discretion.

- Paul