14.09.2020 21:50, Tom Rini wrote: > On Mon, Sep 14, 2020 at 08:08:48PM +0300, Denis Pynkin wrote: >> 14.09.2020 17:11, Tom Rini wrote: >>> On Mon, Sep 14, 2020 at 09:29:18AM -0400, Tom Rini wrote: >>>> On Mon, Sep 14, 2020 at 04:20:20PM +0300, Denis Pynkin wrote: >>>>> 14.09.2020 15:33, Tom Rini wroteт: >>>>>> On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote: >>>>> >>>>>>>> The size of the binary created with the default U-boot config is much >>>>>>>> greater than the default offset for environment `0x60000`. If the new >>>>>>>> version is flashed onto SPI it overlaps with the stored environment. >>>>>>>> This leads to U-Boot corruption in case of saving environment onto >>>>>>>> SPI and non-bootable SabreLite board. >>>>>>>> >>>>>>>> Signed-off-by: Denis Pynkin <[email protected]> >>>>>>> >>>>>>> Reviewed-by: Fabio Estevam <[email protected]> >>>>>>> >>>>>>> In case you want to detect errors like this again in the future, you >>>>>>> could add >>>>>>> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time. >>>>>>> >>>>>>> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption >>>>>>> after saving the environment") >>>>>>> >>>>>>> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though. >>>>>> >>>>>> Hold on. Can we shrink the board back down so that we don't need to >>>>>> move the environment? Moving the environment is bad as it will also >>>>>> break existing users. Thanks! >>>>> >>>>> I don't think so to be honest. >>>>> Current sizes even for `u-boot-nodtb.imx` in my builds are: >>>>> - 595308 (0x9156C) -- cross-compilation >>>>> - 470780 (0x72EFC) -- native build >>>> >>>> I'd like to have one thread where we see what on earth is going on >>>> there. That's a rather crazy size difference and very troubling. >>>> >>>>> which is much larger than current offset 393216 (0x60000) >>>>> >>>>> The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in >>>>> commit a09fea1 just a year ago. >>>>> Not sure if it was tested with SabreLite board tbh -- this is a bulk >>>>> commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1 >>>>> commit are also larger than the current offset. >>>> >>>> Ah, so here's what's going on then. Commit a09fea1 wasn't about ARMv8, >>>> it was about migrating options to the defconfig files. In this case, it >>>> should have been set to the value you're changing it to now, at least >>>> from a read of the patch (include/configs/mx6sabre_common.h would set it >>>> to 0xC0000 if CONFIG_ENV_IS_IN_MMC), but I thought I had done that >>>> migration with my hack to make a tool that had the in-use value be >>>> printed. So I'm going to re-check that whole thing to see what else >>>> might be wrong as well. Thanks! >>> >>> Ah, so, mx6qsabrelite falls in to the "nitrogen6x" family and not the >>> rest of the "sabre" board families. As such, it's always had the env >>> offset of 0x60000. Jumping back somewhat arbitrarily to v2014.10, I see >>> with gcc-4.9 a size of 404480 on u-boot.imx which is still bigger. >>> >>> The commit message isn't clear however, as environment is in MMC and not >>> SPI, so SPI booting should be fine. But MMC/eMMC is where this case can >>> happen (I'm not sure which device is SD slot and which is eMMC on this >>> hardware off-hand). Thanks! >> >> To my shame -- I didn't even thought about the booting from MMC, but yes >> -- should be the same issue in case if U-Boot is placed on SD-card. >> >> This device have 2MB SPI NOR flash there the U-Boot could be flashed >> with an environment. We are using this approach, so I attempt to >> describe the issue from this point of view. >> >> Should I try to re-word the description? Or may I ask you to add a >> better description? > > Well, there's a few things going on here then. The defconfig has > ENV_IS_IN_MMC, not ENV_IS_IN_SPI. So SPI booting should be fine we're > putting U-Boot and environment on entirely different media. There's no > conflict. Booting from the first MMC device can be broken as that's > where we say the environment should be and 'saveenv' will scribble all > over it. So can you confirm what the breaking condition you see is? > The commit message talks about SPI, but SPI should work as-is.
You are absolutely right. My fault -- copy-paste the patch from my project as is without additional testing. Did an additional tests with default value 0x60000: - works well with the combination U-Boot on SPI NOR flash + environment on MMC - corrupted U-Boot on MMC after `saveenv` if used U-Boot on MMC - corrupted U-Boot after `saveenv` if it is configured with `CONFIG_ENV_IS_IN_SPI_FLASH=y` and flashed on SPI The value `0xC0000` fixes the issue for both corrupted cases. Thanks a lot for pointing to incorrect description and misunderstanding! -- wbr, Denis
