[U-Boot] Falcon boot breaks on DRA7 because of commit b9c8ccab "env_mmc.c: Allow environment to be used within SPL"

Tom Rini trini at konsulko.com
Mon Jan 30 23:44:17 CET 2017 

On Mon, Jan 30, 2017 at 01:44:04PM +0100, Jean-Jacques Hiblot wrote:
> 
> 
> 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().
> 

OK, please make a patch, test it on as much of the TI hardware as you
can (including BBB), and lets move from there, thanks!

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

More information about the U-Boot mailing list