[U-Boot] Falcon boot breaks on DRA7 because of commit b9c8ccab "env_mmc.c: Allow environment to be used within SPL"
Jean-Jacques Hiblot
jjhiblot at ti.com
Mon Jan 30 13:44:04 CET 2017
On 27/01/2017 00:36, Tom Rini wrote:
> On Wed, Jan 25, 2017 at 11:21:32AM +0100, Jean-Jacques Hiblot wrote:
>>
>> On 24/01/2017 20:11, Tom Rini wrote:
>>> On Tue, Jan 24, 2017 at 06:04:47PM +0100, Jean-Jacques Hiblot wrote:
>>>> On 24/01/2017 16:46, Tom Rini wrote:
>>>>>> I had noticed that it's quite old indeed. I didn't mean that it's a
>>>>>> regression. I'm just puzzled by the commit. what is its purpose ?
>>>>>> why is SPL not using CONFIG_SYS_MMC_ENV_DEV ?
>>>>> Because in SPL we do not have both MMC devices initialized.
>>>> That is not always the case. Actually in spl_mmc.c the code requires
>>>> us to register more than one MMC device to work properly when
>>>> multiple MMC boot devices can be used (see
>>>> spl_mmc_get_device_index())
>>>> I did the test of registering only MMC2 when booting from eMMC, the
>>>> SPL fails because it can't find device 1:
>>>> Trying to boot from MMC2_2
>>>> MMC Device 1 not found
>>>> spl: could not find mmc device. error: -19
>>>>
>>>>> We register
>>>>> the one we booted from and thus it is device 0 to U-Boot in this case.
>>>>> I suspect the rest of the issues stem from this quirk, or something
>>>>> having broken around this quirk. Thanks!
>>> Right. So I suspect the problem is that some level of the env_mmc.c
>>> code needs to be adapted again for the change in how SPL now works with
>>> the possibility of multiple devices. It's possible that the change you
>>> found is the right fix, please investigate a bit more and confirm
>>> things before submitting a proper patch, thanks!
>> I did more tests and it turns out that there I find no real benefit
>> of registering only the controller for the boot device.
>> The initialization of the MMC/SD/eMMC is done only prior accessing
>> it, not when it's registered. So in terms of boot time the impact of
>> registering many controllers is not significant.
>> By registering the same controllers in SPL and in u-boot, we would
>> get the same mapping for the MMC devices in SPL and u-boot. It would
>> remove a source of confusion and of #ifdef CONFIG_SPL_BUILD
>>
>> The catch is that many boards register only one MMC controller in
>> the SPL, depending on what the boot source is (ex: board_mmc_init()
>> in board/freescale/mx6slevk/mx6slevk.c)
>> To reduce the risk of regression, we could deal with those boards in
>> 2 steps:
>> 1) Don't change the code of the board except to override the weak
>> function mmc_get_env_dev() and make it return 0. This is not likely
>> to introduce a regression
>> 2) One by one, change the code of the boards to register all the
>> controllers in SPL as done in u-boot. Also we need to adapt
>> spl_boot_device() to return the right boot device. There has been a
>> partial attempt at this ""ARM: mx6: add MMC2 boot device detection
>> support in SPL" but had to be reverted probably because it was not
>> coherent with the registration of the controllers.
> Due to the issue you mention in #2, we probably need to do #1 and with
> care and testing, as there's enough places that assume SPL is a single
> MMC device that it'll be problematic to do them one at a time.
I made a survey of the code in 2017.01 to identify the platforms that
may be impacted by the change in env_mmc.c
Here is the method:
1) * find the platforms that override mmc_get_env_dev(). Those may not
rely on CONFIG_SYS_MMC_ENV_DEV to get the MMC device where the env is
stored.
* find the platforms that define CONFIG_SYS_MMC_ENV_DEV as not 0
2) Find the paltforms that use CONFIG_SPL_ENV_SUPPORT
3) Cross the info from 1 and 2 to get the platforms that may be impacted
by the change
1) Platforms that my use a device that is not dev 0 for en storage:
$ git grep -w mmc_get_env_dev
arch/arm/cpu/armv7/mx6/soc.c:int mmc_get_env_dev(void)
arch/arm/cpu/armv7/mx7/soc.c:int mmc_get_env_dev(void)
arch/arm/mach-uniphier/boot-mode/boot-mode.c:int mmc_get_env_dev(void)
board/freescale/mx7dsabresd/mx7dsabresd.c: u32 dev_no =
mmc_get_env_dev();
common/env_mmc.c:__weak int mmc_get_env_dev(void)
common/env_mmc.c: int dev = mmc_get_env_dev();
common/env_mmc.c: int dev = mmc_get_env_dev();
common/env_mmc.c: int dev = mmc_get_env_dev();
common/env_mmc.c: int dev = mmc_get_env_dev();
common/env_mmc.c: int dev = mmc_get_env_dev();
include/mmc.h:int mmc_get_env_dev(void);
$ git grep -e 'ne\sCONFIG_SYS_MMC_ENV_DEV\s*[1-9a-zA-Z\(\)]' include/
include/configs/am335x_evm.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/am335x_shc.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/am335x_sl50.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/bav335x.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/cm_t54.h:#define CONFIG_SYS_MMC_ENV_DEV 1
/* SLOT2: eMMC(1) */
include/configs/dra7xx_evm.h:#define CONFIG_SYS_MMC_ENV_DEV
1 /* SLOT2: eMMC(1) */
include/configs/el6x_common.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/embestmx6boards.h:#define
CONFIG_SYS_MMC_ENV_DEV 2 /* SDHC4 */
include/configs/evb_rk3288.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/evb_rk3399.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/fennec_rk3288.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/mx6qarm2.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/mx6sabresd.h:#define CONFIG_SYS_MMC_ENV_DEV
1 /* SDHC3 */
include/configs/mx6slevk.h:#define CONFIG_SYS_MMC_ENV_DEV
1 /* SDHC2*/
include/configs/mx6sxsabresd.h:#define CONFIG_SYS_MMC_ENV_DEV
2 /*USDHC4*/
include/configs/mx6ul_14x14_evk.h:#define
CONFIG_SYS_MMC_ENV_DEV 1 /* USDHC2 */
include/configs/mx6ullevk.h:#define CONFIG_SYS_MMC_ENV_DEV
1 /* USDHC2 */
include/configs/odroid.h:#define CONFIG_SYS_MMC_ENV_DEV
CONFIG_MMC_DEFAULT_DEV
include/configs/omap4_sdp4430.h:#define CONFIG_SYS_MMC_ENV_DEV
1 /* SLOT2: eMMC(1) */
include/configs/omap5_uevm.h:#define CONFIG_SYS_MMC_ENV_DEV
1 /* SLOT2: eMMC(1) */
include/configs/popmetal_rk3288.h:#define CONFIG_SYS_MMC_ENV_DEV 1
include/configs/s5p_goni.h:#define CONFIG_SYS_MMC_ENV_DEV
CONFIG_MMC_DEFAULT_DEV
include/configs/s5pc210_universal.h:#define
CONFIG_SYS_MMC_ENV_DEV CONFIG_MMC_DEFAULT_DEV
include/configs/tbs2910.h:#define CONFIG_SYS_MMC_ENV_DEV
2 /* overwritten on SD boot */
include/configs/trats.h:#define CONFIG_SYS_MMC_ENV_DEV
CONFIG_MMC_DEFAULT_DEV
include/configs/trats2.h:#define CONFIG_SYS_MMC_ENV_DEV
CONFIG_MMC_DEFAULT_DEV
They can be sorted as such:
* exynos based: use CONFIG_MMC_DEFAULT_DEV wich is 0.
* uniphier : the code in u-boot locates the first MMC (not SD) and use
it for the env. But this mechanism is not used in the SPL case => it
probably doesn't work with SPL_ENV_SUPPORT.
* TI based: The code used to register the controllers is the same for
all and all suffer from the problem I described. They will benefit from
removing "dev = 0"
* Rockchip based : It looks like those platforms use DT to register the
controllers. So I expect that they too would benefit from removing "dev = 0"
* iMX6/IMX7 based : This is a more complex situation:
- imx6/imx7 common code provide relies on board_mmc_get_env_dev() to
get the dev where the env is stored if booting from SD/MMC. When
implemented this function is usually returns the boot device.The default
(weak) implementation returns CONFIG_SYS_MMC_ENV_DEV.
- platforms register several MMC controllers in u-boot and usually
only one in SPL though there are exceptions (embestmx6boards, mx6qarm2,
tbs2910, imx7sabre, etc.).
- Removing "dev = 0" in env_mmc.c, would break SPL_ENV_SUPPORT for
most of the mx6 based boards
2) platform that use SPL env in their default config
$ git grep -e "CONFIG_SPL_ENV_SUPPORT=y"
configs/B4420QDS_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/B4860QDS_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PA_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PA_36BIT_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PA_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PA_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PB_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PB_36BIT_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1010RDB-PB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020MBG-PC_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020MBG-PC_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020RDB-PC_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020RDB-PC_36BIT_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020RDB-PC_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020RDB-PC_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020RDB-PD_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020RDB-PD_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020UTM-PC_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1020UTM-PC_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1021RDB-PC_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1021RDB-PC_36BIT_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1021RDB-PC_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1021RDB-PC_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1022DS_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1022DS_36BIT_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1022DS_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1022DS_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1024RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1024RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1025RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P1025RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P2020RDB-PC_36BIT_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P2020RDB-PC_36BIT_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P2020RDB-PC_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/P2020RDB-PC_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1023RDB_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1023RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1023RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1024QDS_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1024QDS_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1024QDS_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1024RDB_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1024RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1024RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1040D4RDB_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1040D4RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1040D4RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1040RDB_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1040RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1040RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1042D4RDB_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1042D4RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1042D4RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1042RDB_PI_NAND_SECURE_BOOT_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1042RDB_PI_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1042RDB_PI_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T1042RDB_PI_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2080QDS_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2080QDS_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2080QDS_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2080RDB_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2080RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2080RDB_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2081QDS_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2081QDS_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T2081QDS_SPIFLASH_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T4160QDS_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T4160QDS_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T4240QDS_NAND_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T4240QDS_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/T4240RDB_SDCARD_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/a3m071_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/a4m2k_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/am335x_shc_netboot_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/am335x_sl50_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1021aqds_nand_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1021aqds_sdcard_ifc_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1021aqds_sdcard_qspi_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1021atwr_sdcard_ifc_SECURE_BOOT_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1021atwr_sdcard_ifc_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1021atwr_sdcard_qspi_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1043aqds_nand_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1043aqds_sdcard_ifc_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1043aqds_sdcard_qspi_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1043ardb_nand_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls1043ardb_sdcard_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls2080aqds_nand_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/ls2080ardb_nand_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/pcm051_rev1_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/pcm051_rev3_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/pengwyn_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/sandbox_spl_defconfig:CONFIG_SPL_ENV_SUPPORT=y
configs/udoo_neo_defconfig:CONFIG_SPL_ENV_SUPPORT=y
$ git grep -e "define\sCONFIG_SPL_ENV_SUPPORT"
include/configs/ls1021aiot.h:#define CONFIG_SPL_ENV_SUPPORT
include/configs/ls1046a_common.h:#define CONFIG_SPL_ENV_SUPPORT
include/configs/ls1046a_common.h:#define CONFIG_SPL_ENV_SUPPORT
include/configs/xilinx_zynqmp.h:# define CONFIG_SPL_ENV_SUPPORT
$ git grep -A2 "SPL_ENV_SUPPORT" | grep -e 'default\sy'
board/ti/am335x/Kconfig- default y
board/ti/common/Kconfig- default y
3) Cross info.
- udoo_neo is the only iMX6 based platform that uses SPL_ENV. But as it
register only one SDHC port, there will be no problem. All other iMX6/7
based platforms don't use SPL_ENV_SUPPORT in their default configuration.
- all TI platforms are impacted
=> IMO it's safe to make the remove all the 'dev = 0;' from env_mmc and
rely on mmc_get_env_dev().
>
More information about the U-Boot
mailing list