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().
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
