From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 61C26C05027 for ; Fri, 17 Feb 2023 06:56:13 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1FA1385BA7; Fri, 17 Feb 2023 07:56:10 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=canonical.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=canonical.com header.i=@canonical.com header.b="Za4qD1FH"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 90D0185BED; Fri, 17 Feb 2023 07:56:07 +0100 (CET) Received: from smtp-relay-internal-1.canonical.com (smtp-relay-internal-1.canonical.com [185.125.188.123]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E1A6C85BA7 for ; Fri, 17 Feb 2023 07:56:01 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=canonical.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=heinrich.schuchardt@canonical.com Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id EFB2B3F212 for ; Fri, 17 Feb 2023 06:56:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1676616960; bh=stHJ2/1tdmehZlGOKvGLUtkOnAjPLYCsnz6JFTNfx1Q=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Za4qD1FH3sH8P1jJTc+6/1xsFVQUrHTYnYxJkOYQ7jr/JRZmwStTx2ygGZlURNtlK //NE8KpozSvvldhIaFSjlyDTqM2THIkT9O3xmiON8WFTDWvxrll2NHTRhpdCyiUFut 726BSMdlEDsLRleOcihs9nRDt2Z6solNzAhg/cL8r04i/rTo1pGOZC6BdAW+DdbXrI TB+oSR5qBOY75POAavDZLigrQ3uUiibE0tcTbHINDGd1n6pdRsOornGflyOqYP5AV3 pvHv/HlkrxTqaCvsZeEJQbt6cGLxtbsIYqPQZ5+AcmKkBy1Bq2hhzfzqaeJgKsk/Yl mPTVGsO/OJXxg== Received: by mail-wm1-f72.google.com with SMTP id l36-20020a05600c1d2400b003dfe4bae099so192276wms.0 for ; Thu, 16 Feb 2023 22:56:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=stHJ2/1tdmehZlGOKvGLUtkOnAjPLYCsnz6JFTNfx1Q=; b=aN0c8nk1V5LSqpYPI5mHG57ilIXbV/mHE8I5dZz/Oa8EbBxgwX+Dk3IC++gnwwuCcx xfBbT+ceeBsVI5tvIpi4/TvoCdgRwOA6veLqGLe8CV6WJ4b9x0VM52u3pL6bKIWhQSQU QkjqS9DSCICblDoA9CLhO3NnGKCQiOpWt9u+RmmxDE7zqTVMIGUAfgQpZhtddqBuSegY s9K2sK1ivA+oybY6qtERHMfb69JZhVyoIUHp8Iy4oL/NykBccSDNNt+4ape4LzvmOokq Q8pi+g1woi97i2yqs6CCaT9Gee885Ht+pYhoDsJGcPygXkzhHEcnOIeQLgH1CEtLl9xI pabA== X-Gm-Message-State: AO0yUKX9r0+isi6CI7k8pJISO0BMn4Gw5wscXxI2YjHxaOIgCC9jmXga SRrv4tBbQBxaNObuKj0myXvpdd2ZtBfr+MSFzDO/0jF/DhE/1ZjgdVjjLuQYvygLWeuR7qDmgLG EXRa3mH4qa9q7l4nhDY9CHXouFKZSjeQ= X-Received: by 2002:a05:600c:130f:b0:3df:ffab:a391 with SMTP id j15-20020a05600c130f00b003dfffaba391mr262253wmf.24.1676616959741; Thu, 16 Feb 2023 22:55:59 -0800 (PST) X-Google-Smtp-Source: AK7set8lSb7RjMeRSqYj8lNrWEKBNKt9r68u6bbMdeWEQ3nOF/GfanfJCKJFYgvjF+Mngat3n+BykQ== X-Received: by 2002:a05:600c:130f:b0:3df:ffab:a391 with SMTP id j15-20020a05600c130f00b003dfffaba391mr262239wmf.24.1676616959325; Thu, 16 Feb 2023 22:55:59 -0800 (PST) Received: from [192.168.123.94] (ip-088-152-145-137.um26.pools.vodafone-ip.de. [88.152.145.137]) by smtp.gmail.com with ESMTPSA id c190-20020a1c35c7000000b003e21558ee9dsm3625778wma.2.2023.02.16.22.55.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Feb 2023 22:55:58 -0800 (PST) Message-ID: <39edd2e5-47fd-485f-7ce4-ad708667dfa2@canonical.com> Date: Fri, 17 Feb 2023 07:55:58 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 Subject: Re: [PATCH v2 1/1] spl: allow loading via partition type GUID Content-Language: en-US To: Simon Glass Cc: Tom Rini , Yanhong Wang , Andrew Davis , Alper Nebi Yasak , Stefan Roese , Andre Przywara , =?UTF-8?B?SsOpcsO0bWUgQ2FycmV0ZXJv?= , Harald Seiler , u-boot@lists.denx.de References: <20230216152956.130038-1-heinrich.schuchardt@canonical.com> From: Heinrich Schuchardt In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 2/17/23 03:55, Simon Glass wrote: > " properHi Heinrich, > > On Thu, 16 Feb 2023 at 14:31, Heinrich Schuchardt > wrote: >> >> >> >> On 2/16/23 21:17, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Thu, 16 Feb 2023 at 08:30, Heinrich Schuchardt >>> 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 >>>> --- >>>> 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. 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. Best regards Heinrich