| Message ID | 20230421105656.1062558-7-festevam@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [v2,01/13] smegw01: Enable setting additional boot params | expand |
On Friday 21 April 2023 07:56:50 Fabio Estevam wrote: > +uint mmc_get_env_part(struct mmc *mmc) > +{ > + uint part; > + > + if (mmc->part_config == MMCPART_NOAVAILABLE) { > + part = 0; > + } else { > + switch (EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) { > + case 0: /* Booting from this eMMC device is disabled */ > + printf("Error - Booting from this eMMC device is disabled\n"); > + printf("Hint: Use 'mmc partconf' command to choose boot partition\n"); > + return -ENODEV; This does not look to be correct. Return type of the function is uint but here you are trying to return negative number. I think that there is some layering or API issue. Caller of this function probably does not expect any failure and use returned value as partition number. But you are reading partition number from the source which may return failure (as the source does not have to contain it). I'm not sure what is the correct way how to handle these kind of error. I hope that this is something which you would know. Which partition want to choose? Same from which was u-boot/spl loaded at runtime? If yes then maybe this my patch series for mvebu may be interested for you: https://lore.kernel.org/u-boot/20230413205750.10641-1-pali@kernel.org/t/#u I added there ability to store emmc partition from which was bootloader loaded into access bits of mmc->part_config variable and then via macro EXT_CSD_EXTRACT_PARTITION_ACCESS() it can be extracted. See function spl_mmc_emmc_boot_partition() in that mvebu patch series. But I'm not sure how reliable access bits of mmc->part_config are on other platforms. As currently only mvebu in that patch series is doing to use it. > + case 1: /* Boot partition 1 is used for booting */ > + part = 1; > + break; > + case 2: /* Boot partition 2 is used for booting */ > + part = 2; > + break; > + case 7: /* User area is used for booting */ > + part = 0; > + break; > + default: /* Other values are reserved / unsupported */ > + printf("Error - This eMMC device has configured Reserved boot option\n"); > + printf("Hint: Use 'mmc partconf' command to choose boot partition\n"); > + return -ENODEV; > + } > + } > + > + return part; > +}
Hi Pali, On Fri, Apr 21, 2023 at 3:11 PM Pali Rohár <pali@kernel.org> wrote: > This does not look to be correct. Return type of the function is uint > but here you are trying to return negative number. > > I think that there is some layering or API issue. Caller of this > function probably does not expect any failure and use returned value as > partition number. But you are reading partition number from the source > which may return failure (as the source does not have to contain it). > > I'm not sure what is the correct way how to handle these kind of error. > I hope that this is something which you would know. Thanks for your feedback. Talked to Eduard and we prefer to go with a simpler implementation of board_mmc_get_env_part(), just like the one from board/purism/librem5/librem5.c. The librem5 board is based on the imx8mq, which has the same esdhc controller as the imx7d and it works well in our case. Will send a v3.
diff --git a/board/storopack/smegw01/smegw01.c b/board/storopack/smegw01/smegw01.c index e6bff80e5565..db6069a02722 100644 --- a/board/storopack/smegw01/smegw01.c +++ b/board/storopack/smegw01/smegw01.c @@ -17,6 +17,7 @@ #include <asm/arch/crm_regs.h> #include <asm/setup.h> #include <asm/bootm.h> +#include <mmc.h> DECLARE_GLOBAL_DATA_PTR; @@ -93,3 +94,34 @@ int board_late_init(void) return 0; } + +uint mmc_get_env_part(struct mmc *mmc) +{ + uint part; + + if (mmc->part_config == MMCPART_NOAVAILABLE) { + part = 0; + } else { + switch (EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) { + case 0: /* Booting from this eMMC device is disabled */ + printf("Error - Booting from this eMMC device is disabled\n"); + printf("Hint: Use 'mmc partconf' command to choose boot partition\n"); + return -ENODEV; + case 1: /* Boot partition 1 is used for booting */ + part = 1; + break; + case 2: /* Boot partition 2 is used for booting */ + part = 2; + break; + case 7: /* User area is used for booting */ + part = 0; + break; + default: /* Other values are reserved / unsupported */ + printf("Error - This eMMC device has configured Reserved boot option\n"); + printf("Hint: Use 'mmc partconf' command to choose boot partition\n"); + return -ENODEV; + } + } + + return part; +} diff --git a/configs/smegw01_defconfig b/configs/smegw01_defconfig index b3580d5d6e54..54cf1cfc1f1b 100644 --- a/configs/smegw01_defconfig +++ b/configs/smegw01_defconfig @@ -30,6 +30,7 @@ CONFIG_CMD_MEMTEST=y CONFIG_CMD_UNZIP=y CONFIG_CMD_DFU=y CONFIG_CMD_GPIO=y +CONFIG_CMD_GPT=y CONFIG_CMD_MMC=y CONFIG_CMD_PART=y CONFIG_CMD_DHCP=y @@ -44,6 +45,7 @@ CONFIG_OF_CONTROL=y CONFIG_ENV_OVERWRITE=y CONFIG_SYS_REDUNDAND_ENVIRONMENT=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y +CONFIG_SYS_MMC_ENV_DEV=1 CONFIG_NET_RANDOM_ETHADDR=y CONFIG_BOUNCE_BUFFER=y CONFIG_BOOTCOUNT_LIMIT=y diff --git a/include/configs/smegw01.h b/include/configs/smegw01.h index 277c828d0e07..71f2d9c8e85c 100644 --- a/include/configs/smegw01.h +++ b/include/configs/smegw01.h @@ -32,17 +32,21 @@ "mmcpart=1\0" \ "mmcpart_committed=1\0" \ "mmcargs=setenv bootargs console=${console},${baudrate} " \ - "root=/dev/mmcblk0p${mmcpart_committed} rootwait rw " \ - __stringify(EXTRA_BOOTPARAMS) "\0" \ + "root=/dev/mmcblk${mmcdev}p${gpt_partition_entry} rootwait rw " \ + __stringify(EXTRA_BOOTPARAMS) " SM_ROOT_DEV=${mmcdev} SM_ROOT_PART=${gpt_partition_entry} SM_BOOT_PART=${boot_part}\0" \ "commit_mmc=if test \"${ustate}\" = 1 -a \"${mmcpart}\" != \"${mmcpart_committed}\"; then " \ "setenv mmcpart_committed ${mmcpart};" \ "saveenv;" \ "fi;\0" \ "bootlimit=3\0" \ - "loadimage=load mmc ${mmcdev}:${mmcpart_committed} ${loadaddr} boot/${image}\0" \ - "loadfdt=load mmc ${mmcdev}:${mmcpart_committed} ${fdt_addr} boot/${fdtfile}\0" \ + "loadimage=load mmc ${mmcdev}#rootfs-${mmcpart_committed} ${loadaddr} boot/${image}\0" \ + "loadfdt=load mmc ${mmcdev}#rootfs-${mmcpart_committed} ${fdt_addr} boot/${fdtfile}\0" \ + "loadpart=gpt setenv mmc ${mmcdev} rootfs-${mmcpart_committed}\0" \ + "loadbootpart=mmc partconf 1 boot_part\0" \ "mmcboot=echo Booting from mmc ...; " \ "run commit_mmc; " \ + "run loadpart; " \ + "run loadbootpart; " \ "run mmcargs; " \ "if run loadfdt; then " \ "if bootz ${loadaddr} - ${fdt_addr}; then " \