Hi Heinrich, On Tue, 21 Feb 2023 at 12:41, Simon Glass <[email protected]> wrote: > > Hi Heinrich, > > On Thu, 16 Feb 2023 at 23:56, Heinrich Schuchardt > <[email protected]> wrote: > > > > > > > > On 2/17/23 03:55, Simon Glass wrote: > > > " properHi Heinrich, > > > > > > On Thu, 16 Feb 2023 at 14:31, Heinrich Schuchardt > > > <[email protected]> wrote: > > >> > > >> > > >> > > >> On 2/16/23 21:17, Simon Glass wrote: > > >>> Hi Heinrich, > > >>> > > >>> On Thu, 16 Feb 2023 at 08:30, Heinrich Schuchardt > > >>> <[email protected]> wrote: > > >>>> > > >>>> Some boards provide main U-Boot as a dedicated partition to SPL. > > >>>> Currently we can define either a fixed partition number or an MBR > > >>>> partition type to define which partition is to be used. > > >>>> > > >>>> Partition numbers tend to conflict with established partitioning > > >>>> schemes > > >>>> of Linux distros. MBR partitioning is more and more replaced by GPT > > >>>> partitioning. > > >>>> > > >>>> Allow defining a partition type GUID identifying the partition to load > > >>>> main U-Boot from. > > >>>> > > >>>> Signed-off-by: Heinrich Schuchardt <[email protected]> > > >>>> --- > > >>>> v2: > > >>>> avoid if/endif in Kconfig > > >>>> --- > > >>>> common/spl/Kconfig | 27 ++++++++++++++++++++++----- > > >>>> common/spl/spl_mmc.c | 13 +++++++++++++ > > >>>> 2 files changed, 35 insertions(+), 5 deletions(-) > > >>>> > > >>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig > > >>>> index 3c2af453ab..9d12b48297 100644 > > >>>> --- a/common/spl/Kconfig > > >>>> +++ b/common/spl/Kconfig > > >>>> @@ -514,19 +514,36 @@ config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION > > >>>> used in raw mode > > >>>> > > >>>> config SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE > > >>>> - bool "MMC raw mode: by partition type" > > >>>> + bool "MMC raw mode: by MBR partition type" > > >>>> depends on DOS_PARTITION && > > >>>> SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION > > >>>> help > > >>>> - Use partition type for specifying U-Boot partition on MMC/SD > > >>>> in > > >>>> + Use MBR partition type for specifying U-Boot partition on > > >>>> MMC/SD in > > >>>> raw mode. U-Boot will be loaded from the first partition > > >>>> of this > > >>>> type to be found. > > >>>> > > >>>> config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION_TYPE > > >>>> - hex "Partition Type on the MMC to load U-Boot from" > > >>>> + hex "MBR Partition Type on the MMC to load U-Boot from" > > >>>> depends on SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE > > >>>> help > > >>>> - Partition Type on the MMC to load U-Boot from, when the MMC > > >>>> is being > > >>>> - used in raw mode. > > >>>> + MBR Partition Type on the MMC to load U-Boot from, when the > > >>>> MMC is > > >>>> + being used in raw mode. > > >>>> + > > >>>> +config SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE > > >>>> + bool "MMC raw mode: GPT by partition type" > > >>>> + depends on PARTITION_TYPE_GUID && > > >>>> SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION > > >>>> + help > > >>>> + Use GPT partition type for specifying U-Boot partition on > > >>>> MMC/SD in > > >>>> + raw mode. U-Boot will be loaded from the first partition of > > >>>> this > > >>>> + type to be found. > > >>>> + > > >>>> +config SYS_MMCSD_RAW_MODE_U_BOOT_GPT_PARTITION_TYPE > > >>>> + string "GPT Partition Type on the MMC to load U-Boot from" > > >>>> + depends on SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE > > >>>> + default d2f002f8-e4e7-4269-b8ac-3bb6fabeaff6 > > >>> > > >>> What is this? Can we have a register of these hideous things and call > > >>> them by name? > > > > > > Further, I don't see any documentation on this in U-Boot. Could you at > > > least add a list of these things? > > > > > >>> > > >>>> + help > > >>>> + GPT Partition Type on the MMC to load U-Boot from, when the > > >>>> MMC is > > >>>> + being used in raw mode. The GUID must be lower case, low > > >>>> endian, > > >>>> + and formatted like d2f002f8-e4e7-4269-b8ac-3bb6fabeaff6. > > >>>> > > >>>> config SUPPORT_EMMC_BOOT_OVERRIDE_PART_CONFIG > > >>>> bool "Override eMMC EXT_CSC_PART_CONFIG by user defined > > >>>> partition" > > >>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c > > >>>> index e4135b2048..69bf1d6e98 100644 > > >>>> --- a/common/spl/spl_mmc.c > > >>>> +++ b/common/spl/spl_mmc.c > > >>>> @@ -191,6 +191,19 @@ static int mmc_load_image_raw_partition(struct > > >>>> spl_image_info *spl_image, > > >>>> struct disk_partition info; > > >>>> int err; > > >>>> > > >>>> +#ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE > > >>>> + for (int i = 1; i <= MAX_SEARCH_PARTITIONS; ++i) { > > >>>> + err = part_get_info(mmc_get_blk_desc(mmc), i, &info); > > >>>> + if (err) > > >>>> + continue; > > >>>> + if (!strncmp(info.type_guid, > > >>>> + > > >>>> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_GPT_PARTITION_TYPE, > > >>>> + UUID_STR_LEN)) { > > >>>> + partition = i; > > >>>> + break; > > >>>> + } > > >>>> + } > > >>>> +#endif > > >>>> #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE > > >>>> int type_part; > > >>>> /* Only support MBR so DOS_ENTRY_NUMBERS */ > > >>>> -- > > >>>> 2.38.1 > > >>>> > > >>> > > >>> Is it possible to avoid using #ifdef here? > > >> > > >> Unfortunately not. Field 'type_guid' is restricted by an #ifdef. So > > >> unconditional compilation would fail. > > > > > > Do you think it is worth adding an accessor as we have done with some > > > global_data things? > > > > There are other places like disk/part_efi.c where we could trade #ifdef > > for if with such an accessor. The accessor itself would require an #ifdef. > > > > We should be careful about the value returned for > > CONFIG_PARTITION_TYPE_GUID=n. Returning NULL would probably lead to > > warnings from GCC and Coverity. We would better return a dummy string > > like "00000000-0000-0000-0000-000000000000". > > > > > > > >> > > >>> > > >>> Longer term, I wonder if we can add a DT schema for all of > > >>> this...these CONFIG options for boot selection seem to be getting out > > >>> of hand! > > >> > > >> Tom just moved a lot of constants hard coded in C code to Kconfig with a > > >> big effort. Now you want to move Kconfig values to hard coded constants > > >> in device-tree. > > >> > > >> Running in circles does not sound like a winning strategy. > > > > > > The values seem to be common across certain boards of the same type, SoC, > > > etc. > > > > As I described above using the same GUID value for different boards only > > makes sense if they are supported by the very same U-Boot binary. > > > > On boards with the same SoC (even from different vendors) I would very > > much prefer a single U-Boot SPL selecting a board specific device-tree > > to having multiple binaries. We should push developers into this direction. > > > > > > > > I'm not sure, but at some point this is all going to get out of hand. > > > Already we have these options: > > > > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_DATA_PART_OFFSET > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION_TYPE > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_EMMC_BOOT_PARTITION > > > common/spl/Kconfig:config SYS_MMCSD_FS_BOOT_PARTITION > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_KERNEL_SECTOR > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_ARGS_SECTOR > > > common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_ARGS_SECTORS > > > > > > That is just for MMC raw mode. > > > > > > For environment we have SYS_MMC_ENV_DEV and _PART. If you look around > > > you'll see loads of these options. > > > > > > I see that rockchip uses u-boot,spl-boot-order as a way to determine > > > the boot order. This makes it configurable without rebuilding > > > U-Boot...although I don't think we need to make the MMC stuff > > > configurable, since I am assuming that the boot ROM determines at > > > least some of it...? > > > > This patch is about SPL loading main U-Boot. So the boot ROM is not > > involved. > > > > > > > > It seems that the whole thing is crying out for a bit of organisation > > > and a proper schema. > > > > The discussion was about hard-coding the values vs configuration. > > Actually my intent was to use names instead of UUIDs, which I consider > to be obfuscation. Then there would be a conversion table somewhere.
Actually there already is one in uuid.c so we could use the strings in that. > > > > > OS distributions should have enough flexibility to deliver an > > installation image with U-Boot for multiple boards on the same medium. > > For the build process it is preferable to use different configurations > > instead of patching source code per U-Boot which might be required if > > hard-coded values for partition GUIDs in the device-trees are used. > > > > I think Tom's approach is right. The U-Boot documentation should give > > guidance on how new boards should find U-Boot SPL and main U-Boot. > > For my understanding, why is U-Boot SPL in one partition and U-Boot > proper in another? ? Regards, Simon
