[PATCH v3 0/3] spl: allow loading via partition type GUID

[PATCH v3 0/3] spl: allow loading via partition type GUID

[Heinrich Schuchardt]

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.

To avoid using #ifdef in the implementation:

* introduce accessors for fields of struct disk_partition
* provide a macro IF_ENABLED

v3:
	avoid usage of #ifdef
v2:
	avoid if/endif in Kconfig

The necessity of the Kconfig setting was discussed in
https://lore.kernel.org/u-boot/20230216152956.130038-1-heinrich.schuchardt@canonical.com/

Heinrich Schuchardt (3):
  disk: accessors for conditional partition fields
  kconfig: new macro IF_ENABLED()
  spl: allow loading via partition type GUID

 common/spl/Kconfig      | 27 ++++++++++++++++++++++-----
 common/spl/spl_mmc.c    | 18 ++++++++++++++++++
 include/linux/kconfig.h |  7 +++++++
 include/part.h          | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 5 deletions(-)

-- 
2.38.1

[PATCH v3 1/3] disk: accessors for conditional partition fields

[Heinrich Schuchardt]

Structure disk_partition contains some fields whose existence depends on
configuration variables. Provide macros that return a value irrespective of
the value of configuration. This allows to replace #ifdefs by simple ifs
in codes that relies on these fields which is our preferred coding style.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	new patch
---
 include/part.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/include/part.h b/include/part.h
index be75c73549..c0355a3602 100644
--- a/include/part.h
+++ b/include/part.h
@@ -80,6 +80,42 @@ struct disk_partition {
 #endif
 };
 
+/**
+ * get_part_uuid() - get partition UUID
+ *
+ * @a:		disk partition as struct disk_partition
+ * Return:	partition UUID as string
+ */
+#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
+#define get_part_uuid(a) (a.uuid)
+#else
+#define get_part_uuid(a) ("")
+#endif
+
+/**
+ * get_part_type_guid() - get partition type GUID
+ *
+ * @a:		disk partition as struct disk_partition
+ * Return:	partition type GUID as string
+ */
+#ifdef CONFIG_PARTITION_TYPE_GUID
+#define get_part_type_guid(a) (a.type_guid)
+#else
+#define get_part_type_guid(a) ("")
+#endif
+
+/**
+ * get_part_type() - get partition type
+ *
+ * @a:		disk partition as struct disk_partition
+ * Return:	partition type a unsigned char
+ */
+#ifdef CONFIG_DOS_PARTITION
+#define get_part_type(a) (a.sys_ind)
+#else
+#define get_part_type(a) ('\0')
+#endif
+
 struct disk_part {
 	int partnum;
 	struct disk_partition gpt_part_info;
-- 
2.38.1

Re: [PATCH v3 1/3] disk: accessors for conditional partition fields

[Simon Glass]

On Sun, 19 Feb 2023 at 04:36, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Structure disk_partition contains some fields whose existence depends on
> configuration variables. Provide macros that return a value irrespective of
> the value of configuration. This allows to replace #ifdefs by simple ifs
> in codes that relies on these fields which is our preferred coding style.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
>         new patch
> ---
>  include/part.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

But I do think static inlines are better

[PATCH v3 2/3] kconfig: new macro IF_ENABLED()

[Heinrich Schuchardt]

We want to move from using #ifdef to using if in our code. A lot of our
code using #ifdef is structured like:

    #ifdef CONFIG_FOO
        fun(CONFIG_FOO_OPT);
    #endif

In Kconfig you will find

    config FOO
        bool "enable foo"

    config FOO_OPT
        string "value for foo"
        depends on FOO

We cannot use CONFIG_FOO_OPT in our code directly if CONFIG_FOO is not
defined.

     if (IS_ENABLED(CONFIG_FOO)
         fun(CONFIG_FOO_OPT);

This would result in an error due to the undefined symbol CONFIG_FOO_OPT.

The new macro IF_ENABLED(CONFIG_FOO, opt_cfg, def_val) which evaluates
to opt_cfg if CONFIG_FOO is set to 'y' and to def_val otherwise comes to
the rescue:

     if (IS_ENABLED(CONFIG_FOO)
         fun(IF_ENABLED(CONFIG_FOO, CONFIG_FOO_OPT, "");

If CONFIG_FOO is not defined, the compiler will see

    if (0)
        fun("");

and be happy.

If CONFIG_FOO is defined, the compiler will see

    if (1)
        fun(CONFIG_FOO_OPT)

and be equally happy.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	new patch
---
 include/linux/kconfig.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index 2bc704e110..7fea72da1a 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -28,6 +28,13 @@
  */
 #define IS_ENABLED(option)	config_enabled(option, 0)
 
+/*
+ * IF_ENABLED(CONFIG_FOO, opt_cfg, def_val) evalutes to opt_cfg if
+ * CONFIG_FOO is set to 'y' and to def_val otherwise.
+ */
+#define IF_ENABLED(option, opt_cfg, def_val) \
+	config_opt_enabled(IS_ENABLED(option), opt_cfg, def_val)
+
 /*
  * U-Boot add-on: Helper macros to reference to different macros (prefixed by
  * CONFIG_, CONFIG_SPL_, CONFIG_TPL_ or CONFIG_TOOLS_), depending on the build
-- 
2.38.1

Re: [PATCH v3 2/3] kconfig: new macro IF_ENABLED()

[Andre Przywara]

On Sun, 19 Feb 2023 12:36:28 +0100
Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:

Hi,

> We want to move from using #ifdef to using if in our code. A lot of our
> code using #ifdef is structured like:
> 
>     #ifdef CONFIG_FOO
>         fun(CONFIG_FOO_OPT);
>     #endif
> 
> In Kconfig you will find
> 
>     config FOO
>         bool "enable foo"
> 
>     config FOO_OPT
>         string "value for foo"
>         depends on FOO
> 
> We cannot use CONFIG_FOO_OPT in our code directly if CONFIG_FOO is not
> defined.
> 
>      if (IS_ENABLED(CONFIG_FOO)
>          fun(CONFIG_FOO_OPT);
> 
> This would result in an error due to the undefined symbol CONFIG_FOO_OPT.
> 
> The new macro IF_ENABLED(CONFIG_FOO, opt_cfg, def_val) which evaluates
> to opt_cfg if CONFIG_FOO is set to 'y' and to def_val otherwise comes to
> the rescue:
> 
>      if (IS_ENABLED(CONFIG_FOO)
>          fun(IF_ENABLED(CONFIG_FOO, CONFIG_FOO_OPT, "");

Ah, yeah, I like this one, this is indeed the most common pattern that
prevents many "#if to if" conversions.
I briefly thought about unifying those two lines, but it's probably
limiting, as it appears more flexible this way:

> 
> If CONFIG_FOO is not defined, the compiler will see
> 
>     if (0)
>         fun("");
> 
> and be happy.
> 
> If CONFIG_FOO is defined, the compiler will see
> 
>     if (1)
>         fun(CONFIG_FOO_OPT)
> 
> and be equally happy.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> v3:
> 	new patch
> ---
>  include/linux/kconfig.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
> index 2bc704e110..7fea72da1a 100644
> --- a/include/linux/kconfig.h
> +++ b/include/linux/kconfig.h
> @@ -28,6 +28,13 @@
>   */
>  #define IS_ENABLED(option)	config_enabled(option, 0)
>  
> +/*
> + * IF_ENABLED(CONFIG_FOO, opt_cfg, def_val) evalutes to opt_cfg if
> + * CONFIG_FOO is set to 'y' and to def_val otherwise.
> + */
> +#define IF_ENABLED(option, opt_cfg, def_val) \
> +	config_opt_enabled(IS_ENABLED(option), opt_cfg, def_val)
> +
>  /*
>   * U-Boot add-on: Helper macros to reference to different macros (prefixed by
>   * CONFIG_, CONFIG_SPL_, CONFIG_TPL_ or CONFIG_TOOLS_), depending on the build

Re: [PATCH v3 2/3] kconfig: new macro IF_ENABLED()

[Simon Glass]

Hi Heinrich,

On Mon, 20 Feb 2023 at 10:15, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sun, 19 Feb 2023 12:36:28 +0100
> Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote:
>
> Hi,
>
> > We want to move from using #ifdef to using if in our code. A lot of our
> > code using #ifdef is structured like:
> >
> >     #ifdef CONFIG_FOO
> >         fun(CONFIG_FOO_OPT);
> >     #endif
> >
> > In Kconfig you will find
> >
> >     config FOO
> >         bool "enable foo"
> >
> >     config FOO_OPT
> >         string "value for foo"
> >         depends on FOO
> >
> > We cannot use CONFIG_FOO_OPT in our code directly if CONFIG_FOO is not
> > defined.
> >
> >      if (IS_ENABLED(CONFIG_FOO)
> >          fun(CONFIG_FOO_OPT);
> >
> > This would result in an error due to the undefined symbol CONFIG_FOO_OPT.
> >
> > The new macro IF_ENABLED(CONFIG_FOO, opt_cfg, def_val) which evaluates
> > to opt_cfg if CONFIG_FOO is set to 'y' and to def_val otherwise comes to
> > the rescue:
> >
> >      if (IS_ENABLED(CONFIG_FOO)
> >          fun(IF_ENABLED(CONFIG_FOO, CONFIG_FOO_OPT, "");
>
> Ah, yeah, I like this one, this is indeed the most common pattern that
> prevents many "#if to if" conversions.
> I briefly thought about unifying those two lines, but it's probably
> limiting, as it appears more flexible this way:
>
> >
> > If CONFIG_FOO is not defined, the compiler will see
> >
> >     if (0)
> >         fun("");
> >
> > and be happy.
> >
> > If CONFIG_FOO is defined, the compiler will see
> >
> >     if (1)
> >         fun(CONFIG_FOO_OPT)
> >
> > and be equally happy.
> >
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
> > ---
> > v3:
> >       new patch
> > ---
> >  include/linux/kconfig.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
> > index 2bc704e110..7fea72da1a 100644
> > --- a/include/linux/kconfig.h
> > +++ b/include/linux/kconfig.h
> > @@ -28,6 +28,13 @@
> >   */
> >  #define IS_ENABLED(option)   config_enabled(option, 0)
> >
> > +/*
> > + * IF_ENABLED(CONFIG_FOO, opt_cfg, def_val) evalutes to opt_cfg if
> > + * CONFIG_FOO is set to 'y' and to def_val otherwise.
> > + */
> > +#define IF_ENABLED(option, opt_cfg, def_val) \
> > +     config_opt_enabled(IS_ENABLED(option), opt_cfg, def_val)
> > +
> >  /*
> >   * U-Boot add-on: Helper macros to reference to different macros (prefixed by
> >   * CONFIG_, CONFIG_SPL_, CONFIG_TPL_ or CONFIG_TOOLS_), depending on the build
>

This looks good to me, but what should we do with the existing
IF_ENABLED_INT()? Perhaps we should drop that in favour of this one,
or do we need both?

Regards,
Simon

[PATCH v3 3/3] spl: allow loading via partition type GUID

[Heinrich Schuchardt]

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>
---
v3:
	avoid usage of #ifdef
v2:
	avoid if/endif in Kconfig
---
 common/spl/Kconfig   | 27 ++++++++++++++++++++++-----
 common/spl/spl_mmc.c | 18 ++++++++++++++++++
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 3c2af453ab..9af8283cb5 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: by GPT 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
+	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..2417fa43e6 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -191,6 +191,24 @@ static int mmc_load_image_raw_partition(struct spl_image_info *spl_image,
 	struct disk_partition info;
 	int err;
 
+	if (IS_ENABLED(CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE)) {
+		const char *guid =
+			IF_ENABLED(CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE,
+				   CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_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(get_part_type_guid(info), guid,
+				     UUID_STR_LEN)) {
+				partition = i;
+				break;
+			}
+		}
+	}
+
 #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
 	int type_part;
 	/* Only support MBR so DOS_ENTRY_NUMBERS */
-- 
2.38.1