Re: [PATCH v2 1/1] spl: allow loading via partition type GUID

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 2/16/23 21:17, Simon Glass wrote:
> Hi Heinrich,
> 
> On Thu, 16 Feb 2023 at 08:30, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> 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 <heinrich.schuchardt@canonical.com>
>> ---
>> 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?
> 
>> +       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.

> 
> 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.

Best regards

Heinrich

> 
> Regards,
> Simon