From mboxrd@z Thu Jan 1 00:00:00 1970 From: pwalmsley@nvidia.com (Paul Walmsley) Date: Wed, 1 Apr 2015 13:17:48 -0600 Subject: "ARM: multi_v7_defconfig: Enable shmobile platforms" breaks Tegra20 multi_v7_defconfig boot In-Reply-To: <551C3F25.40105@wwwdotorg.org> References: <55137098.4030903@wwwdotorg.org> <5514569E.2030406@wwwdotorg.org> <551C2768.6060209@wwwdotorg.org> <551C2F7A.7040107@nvidia.com> <551C3F25.40105@wwwdotorg.org> Message-ID: <551C445C.7040107@nvidia.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.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 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