[RFC PATCH 0/4] CONFIG_IS_ENABLED magic

[RFC PATCH 0/4] CONFIG_IS_ENABLED magic

[Rasmus Villemoes]

The first patch is just something I suggested to allow zstd support to
move forward. The remaining ones aim to make it more ergonomic to use
CONFIG_IS_ENABLED to exclude things from the build.

While it currently works just fine in C code that one can do

  if (CONFIG_IS_ENABLED(FOO)) {

  }

and have the compiler throw the whole block away, and then later the
linker throw away any functions and/or data that turns out not be used
anyway, it's currently somewhat uglier to exclude items from an array
initializer - it requires three lines to do

  #if CONFIG_IS_ENABLED(FOO)
  { some array item },
  #endif

and grepping for the FOO symbol doesn't really show what it is used
for including/excluding.

With the last patch, one can instead do

  CONFIG_IS_ENABLED(FOO, ({ some array item },))

It's just an RFC; I think this can be useful to reduce the size of
SPL/TPL without too much cluttering of the source, others can
disagree.


Rasmus Villemoes (4):
  common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef
    USE_HOSTCC
  linux/kconfig.h: simplify logic for choosing CONFIG_{SPL_,TPL_,}*
  linux/kconfig.h: remove unused helper macros
  linux/kconfig.h: create two- and three-argument versions of
    CONFIG_IS_ENABLED

 common/image.c               |   2 +
 include/linux/kconfig.h      | 103 ++++++++++++++++++-----------------
 scripts/config_whitelist.txt |   2 -
 3 files changed, 54 insertions(+), 53 deletions(-)

-- 
2.23.0

[RFC PATCH 1/4] common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef USE_HOSTCC

[Rasmus Villemoes]

When building host tools, the CONFIG_GZIP etc. symbols are not defined
anyway, so this does not (should not) change anything [1]. However,
since the host tools also don't include linux/kconfig.h, one cannot
use the CONFIG_IS_ENABLED() smartness in a preprocessor conditional,
which in turn prevents one from adding, say, an

  #if CONFIG_IS_ENABLED(ZSTD)

case.

OTOH, with this, one can do that, and it also makes it possible to
convert say, "#ifdef CONFIG_GZIP" to "#if CONFIG_IS_ENABLED(GZIP)" to
make those other cases SPL-or-not-SPL-aware.

[1] The gzip.h header is only included under !USE_HOSTCC, and removing
the CONFIG_GZIP conditional hence both gives an "no declaration of
gunzip" warning as well as breaks the link of the host tools, since
the gunzip code is indeed not linked in.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/image.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/image.c b/common/image.c
index e1ca1a7905..73f6845274 100644
--- a/common/image.c
+++ b/common/image.c
@@ -458,6 +458,7 @@ int image_decomp(int comp, ulong load, ulong image_start, int type,
 		else
 			ret = -ENOSPC;
 		break;
+#ifndef USE_HOSTCC
 #ifdef CONFIG_GZIP
 	case IH_COMP_GZIP: {
 		ret = gunzip(load_buf, unc_len, image_buf, &image_len);
@@ -508,6 +509,7 @@ int image_decomp(int comp, ulong load, ulong image_start, int type,
 		break;
 	}
 #endif /* CONFIG_LZ4 */
+#endif /* !USE_HOSTCC */
 	default:
 		printf("Unimplemented compression type %d\n", comp);
 		return -ENOSYS;
-- 
2.23.0

[RFC PATCH 2/4] linux/kconfig.h: simplify logic for choosing CONFIG_{SPL_, TPL_, }*

[Rasmus Villemoes]

Instead of using the arg1_or_junk trick to pick between two choices,
with a bit of duplication between the branches (and most of the
CONFIG_TPL_BUILD case being redundant, as _IS_TPL is known to be
defined to 1 in that case), simply define a prefix that we inject
between CONFIG_ and the given config symbol.

This only requires one level of indirection (to get the
_CONFIG_PREFIX macro expanded before the token concatenation takes
place), and makes it easy to, say, introduce a CONFIG_HOSTTOOL_
prefix. [I would expect most HOSTTOOL_ symbols to just be def_bool y,
but it would allow us to clean up some of the ifdef HOSTCC mess in the
sources shared between U-Boot and host tools.]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/linux/kconfig.h | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index 3a2da738c4..56b8e5d787 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -47,30 +47,19 @@
  * U-Boot add-on: Helper macros to reference to different macros
  * (CONFIG_ or CONFIG_SPL_ prefixed), depending on the build context.
  */
-#ifdef CONFIG_SPL_BUILD
-#define _IS_SPL 1
-#endif
-
-#ifdef CONFIG_TPL_BUILD
-#define _IS_TPL 1
-#endif
 
 #if defined(CONFIG_TPL_BUILD)
-#define config_val(cfg) _config_val(_IS_TPL, cfg)
-#define _config_val(x, cfg) __config_val(x, cfg)
-#define __config_val(x, cfg) ___config_val(__ARG_PLACEHOLDER_##x, cfg)
-#define ___config_val(arg1_or_junk, cfg)  \
-	____config_val(arg1_or_junk CONFIG_TPL_##cfg, CONFIG_##cfg)
-#define ____config_val(__ignored, val, ...) val
+#define _CONFIG_PREFIX TPL_
+#elif defined(CONFIG_SPL_BUILD)
+#define _CONFIG_PREFIX SPL_
 #else
-#define config_val(cfg) _config_val(_IS_SPL, cfg)
-#define _config_val(x, cfg) __config_val(x, cfg)
-#define __config_val(x, cfg) ___config_val(__ARG_PLACEHOLDER_##x, cfg)
-#define ___config_val(arg1_or_junk, cfg)  \
-	____config_val(arg1_or_junk CONFIG_SPL_##cfg, CONFIG_##cfg)
-#define ____config_val(__ignored, val, ...) val
+#define _CONFIG_PREFIX
 #endif
 
+#define   config_val(cfg)       _config_val(_CONFIG_PREFIX, cfg)
+#define  _config_val(pfx, cfg) __config_val(pfx, cfg)
+#define __config_val(pfx, cfg) CONFIG_ ## pfx ## cfg
+
 /*
  * CONFIG_VAL(FOO) evaluates to the value of
  *  CONFIG_FOO if CONFIG_SPL_BUILD is undefined,
-- 
2.23.0

[RFC PATCH 3/4] linux/kconfig.h: remove unused helper macros

[Rasmus Villemoes]

U-Boot does not have loadable modules, and nothing currently uses any
of the (CONFIG_)?IS_(BUILTIN|MODULE) macros - only
the (CONFIG_)?IS_ENABLED variants are ever used.

While I understand the desire to keep this somewhat synchronized with
linux, we've already departed by the introduction of the
CONFIG_IS_ENABLED extra logic, and deleting these makes the next patch
much simpler, since I won't have to duplicate a lot of logic for no
real gain (as there are no users).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/linux/kconfig.h      | 40 +++++-------------------------------
 scripts/config_whitelist.txt |  2 --
 2 files changed, 5 insertions(+), 37 deletions(-)

diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index 56b8e5d787..caea505a0e 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -23,25 +23,12 @@
 #define ___config_enabled(__ignored, val, ...) val
 
 /*
- * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
+ * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y',
  * 0 otherwise.
  *
  */
 #define IS_ENABLED(option) \
-	(config_enabled(option) || config_enabled(option##_MODULE))
-
-/*
- * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
- * otherwise. For boolean options, this is equivalent to
- * IS_ENABLED(CONFIG_FOO).
- */
-#define IS_BUILTIN(option) config_enabled(option)
-
-/*
- * IS_MODULE(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'm', 0
- * otherwise.
- */
-#define IS_MODULE(option) config_enabled(option##_MODULE)
+	(config_enabled(option))
 
 /*
  * U-Boot add-on: Helper macros to reference to different macros
@@ -70,29 +57,12 @@
 
 /*
  * CONFIG_IS_ENABLED(FOO) evaluates to
- *  1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y' or 'm',
- *  1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y' or 'm',
- *  1 if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y' or 'm',
- *  0 otherwise.
- */
-#define CONFIG_IS_ENABLED(option) \
-	(config_enabled(CONFIG_VAL(option)) ||		\
-	 config_enabled(CONFIG_VAL(option##_MODULE)))
-
-/*
- * CONFIG_IS_BUILTIN(FOO) evaluates to
  *  1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y',
  *  1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y',
+ *  1 if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
  *  0 otherwise.
  */
-#define CONFIG_IS_BUILTIN(option) config_enabled(CONFIG_VAL(option))
-
-/*
- * CONFIG_IS_MODULE(FOO) evaluates to
- *  1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'm',
- *  1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'm',
- *  0 otherwise.
- */
-#define CONFIG_IS_MODULE(option) config_enabled(CONFIG_VAL(option##_MODULE))
+#define CONFIG_IS_ENABLED(option) \
+	(config_enabled(CONFIG_VAL(option)))
 
 #endif /* __LINUX_KCONFIG_H */
diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index 2210f46e44..352db2b57a 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -875,9 +875,7 @@ CONFIG_IRAM_SIZE
 CONFIG_IRAM_STACK
 CONFIG_IRAM_TOP
 CONFIG_IRDA_BASE
-CONFIG_IS_BUILTIN
 CONFIG_IS_ENABLED
-CONFIG_IS_MODULE
 CONFIG_JFFS2_CMDLINE
 CONFIG_JFFS2_DEV
 CONFIG_JFFS2_LZO
-- 
2.23.0

[RFC PATCH 4/4] linux/kconfig.h: create two- and three-argument versions of CONFIG_IS_ENABLED

[Rasmus Villemoes]

This adds a bunch of preprocessor magic to extend the capabilities of
CONFIG_IS_ENABLED. The existing semantics of

  CONFIG_IS_ENABLED(FOO)

expanding to a 1 or 0 (depending on build context and the defined-ness
or not of the appropriate CONFIG_FOO/CONFIG_SPL_FOO/CONFIG_TPL_FOO)
are of course preserved. With this, one is also allowed a two-argument
form

  CONFIG_IS_ENABLED(FOO, (something))

which expands to something precisely when CONFIG_IS_ENABLED(FOO) would
expand to 1, and expands to nothing otherwise. It is, in other words,
completely equivalent to the three lines

  #if CONFIG_IS_ENABLED(FOO)
  something
  #endif

The second argument must be parenthesized in order to allow any
tokens, including a trailing comma, to appear - one use case for this
is precisely to make it a bit more ergonomic to build an array and
only include certain items depending on .config. That should increase
both readability and not least "git grep"ability.

A third variant is also introduced,

  CONFIG_IS_ENABLED(FOO, (xxx), (yyy))

which corresponds to

  #if CONFIG_IS_ENABLED(FOO)
  xxx
  #else
  yyy
  #endif

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/linux/kconfig.h | 48 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index caea505a0e..d109ed3119 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -56,13 +56,55 @@
 #define CONFIG_VAL(option)  config_val(option)
 
 /*
- * CONFIG_IS_ENABLED(FOO) evaluates to
+ * Count number of arguments to a variadic macro. Currently only need
+ * it for 1, 2 or 3 arguments.
+ */
+#define __arg6(a1, a2, a3, a4, a5, a6, ...) a6
+#define __count_args(...) __arg6(dummy, ##__VA_ARGS__, 4, 3, 2, 1, 0)
+
+#define __concat(a, b)   ___concat(a, b)
+#define ___concat(a, b)  a ## b
+
+#define __unwrap(...) __VA_ARGS__
+#define __unwrap1(case1, case0) __unwrap case1
+#define __unwrap0(case1, case0) __unwrap case0
+
+#define __CONFIG_IS_ENABLED_1(option)        __CONFIG_IS_ENABLED_3(option, (1), (0))
+#define __CONFIG_IS_ENABLED_2(option, case1) __CONFIG_IS_ENABLED_3(option, case1, ())
+#define __CONFIG_IS_ENABLED_3(option, case1, case0) \
+	__concat(__unwrap, config_enabled(CONFIG_VAL(option))) (case1, case0)
+
+/*
+ * CONFIG_IS_ENABLED(FOO) expands to
  *  1 if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y',
  *  1 if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y',
  *  1 if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
  *  0 otherwise.
+ *
+ * CONFIG_IS_ENABLED(FOO, (abc)) expands to
+ *  abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y',
+ *  abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y',
+ *  abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
+ *  nothing otherwise.
+ *
+ * CONFIG_IS_ENABLED(FOO, (abc), (def)) expands to
+ *  abc if CONFIG_SPL_BUILD is undefined and CONFIG_FOO is set to 'y',
+ *  abc if CONFIG_SPL_BUILD is defined and CONFIG_SPL_FOO is set to 'y',
+ *  abc if CONFIG_TPL_BUILD is defined and CONFIG_TPL_FOO is set to 'y',
+ *  def otherwise.
+ *
+ * The optional second and third arguments must be parenthesized; that
+ * allows one to include a trailing comma, e.g. for use in
+ *
+ * CONFIG_IS_ENABLED(ACME, ({.compatible = "acme,frobnozzle"},))
+ *
+ * which adds an entry to the array being defined if CONFIG_ACME (or
+ * CONFIG_SPL_ACME/CONFIG_TPL_ACME, depending on build context) is
+ * set, and nothing otherwise.
  */
-#define CONFIG_IS_ENABLED(option) \
-	(config_enabled(CONFIG_VAL(option)))
+
+#define CONFIG_IS_ENABLED(option, ...)					\
+	__concat(__CONFIG_IS_ENABLED_, __count_args(option, ##__VA_ARGS__)) (option, ##__VA_ARGS__)
+
 
 #endif /* __LINUX_KCONFIG_H */
-- 
2.23.0

[RFC PATCH 4/4] linux/kconfig.h: create two- and three-argument versions of CONFIG_IS_ENABLED

[Simon Glass]

Hi Rasmus,

On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> This adds a bunch of preprocessor magic to extend the capabilities of
> CONFIG_IS_ENABLED. The existing semantics of
>
>   CONFIG_IS_ENABLED(FOO)
>
> expanding to a 1 or 0 (depending on build context and the defined-ness
> or not of the appropriate CONFIG_FOO/CONFIG_SPL_FOO/CONFIG_TPL_FOO)
> are of course preserved. With this, one is also allowed a two-argument
> form
>
>   CONFIG_IS_ENABLED(FOO, (something))
>
> which expands to something precisely when CONFIG_IS_ENABLED(FOO) would
> expand to 1, and expands to nothing otherwise. It is, in other words,
> completely equivalent to the three lines
>
>   #if CONFIG_IS_ENABLED(FOO)
>   something
>   #endif
>
> The second argument must be parenthesized in order to allow any
> tokens, including a trailing comma, to appear - one use case for this
> is precisely to make it a bit more ergonomic to build an array and
> only include certain items depending on .config. That should increase
> both readability and not least "git grep"ability.
>
> A third variant is also introduced,
>
>   CONFIG_IS_ENABLED(FOO, (xxx), (yyy))
>
> which corresponds to
>
>   #if CONFIG_IS_ENABLED(FOO)
>   xxx
>   #else
>   yyy
>   #endif
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/linux/kconfig.h | 48 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 3 deletions(-)

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

See also this:

http://patchwork.ozlabs.org/project/uboot/patch/20200522020223.230834-2-sjg at chromium.org/

but I think your idea is a lot nicer.

Regards,
Simon

[RFC PATCH 1/4] common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef USE_HOSTCC

[Simon Glass]

Hi Rasmus,

On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> When building host tools, the CONFIG_GZIP etc. symbols are not defined
> anyway, so this does not (should not) change anything [1]. However,
> since the host tools also don't include linux/kconfig.h, one cannot
> use the CONFIG_IS_ENABLED() smartness in a preprocessor conditional,
> which in turn prevents one from adding, say, an
>
>   #if CONFIG_IS_ENABLED(ZSTD)
>
> case.
>
> OTOH, with this, one can do that, and it also makes it possible to
> convert say, "#ifdef CONFIG_GZIP" to "#if CONFIG_IS_ENABLED(GZIP)" to
> make those other cases SPL-or-not-SPL-aware.
>
> [1] The gzip.h header is only included under !USE_HOSTCC, and removing
> the CONFIG_GZIP conditional hence both gives an "no declaration of
> gunzip" warning as well as breaks the link of the host tools, since
> the gunzip code is indeed not linked in.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  common/image.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/common/image.c b/common/image.c
> index e1ca1a7905..73f6845274 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -458,6 +458,7 @@ int image_decomp(int comp, ulong load, ulong image_start, int type,
>                 else
>                         ret = -ENOSPC;
>                 break;
> +#ifndef USE_HOSTCC

In general I'm not a fan of the USE_HOSTCC stuff. I suspect HOST
should be a Kconfig like anything else, although perhaps magically
inserted somehow. Then we can do more things at compile-time.

>  #ifdef CONFIG_GZIP
>         case IH_COMP_GZIP: {
>                 ret = gunzip(load_buf, unc_len, image_buf, &image_len);
> @@ -508,6 +509,7 @@ int image_decomp(int comp, ulong load, ulong image_start, int type,
>                 break;
>         }
>  #endif /* CONFIG_LZ4 */
> +#endif /* !USE_HOSTCC */
>         default:
>                 printf("Unimplemented compression type %d\n", comp);
>                 return -ENOSYS;
> --
> 2.23.0
>

Regards,
Simon

[RFC PATCH 2/4] linux/kconfig.h: simplify logic for choosing CONFIG_{SPL_, TPL_, }*

[Simon Glass]

On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Instead of using the arg1_or_junk trick to pick between two choices,
> with a bit of duplication between the branches (and most of the
> CONFIG_TPL_BUILD case being redundant, as _IS_TPL is known to be
> defined to 1 in that case), simply define a prefix that we inject
> between CONFIG_ and the given config symbol.
>
> This only requires one level of indirection (to get the
> _CONFIG_PREFIX macro expanded before the token concatenation takes
> place), and makes it easy to, say, introduce a CONFIG_HOSTTOOL_
> prefix. [I would expect most HOSTTOOL_ symbols to just be def_bool y,
> but it would allow us to clean up some of the ifdef HOSTCC mess in the
> sources shared between U-Boot and host tools.]
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/linux/kconfig.h | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
>

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

[RFC PATCH 3/4] linux/kconfig.h: remove unused helper macros

[Simon Glass]

On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> U-Boot does not have loadable modules, and nothing currently uses any
> of the (CONFIG_)?IS_(BUILTIN|MODULE) macros - only
> the (CONFIG_)?IS_ENABLED variants are ever used.
>
> While I understand the desire to keep this somewhat synchronized with
> linux, we've already departed by the introduction of the
> CONFIG_IS_ENABLED extra logic, and deleting these makes the next patch
> much simpler, since I won't have to duplicate a lot of logic for no
> real gain (as there are no users).
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  include/linux/kconfig.h      | 40 +++++-------------------------------
>  scripts/config_whitelist.txt |  2 --
>  2 files changed, 5 insertions(+), 37 deletions(-)

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

[RFC PATCH 1/4] common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef USE_HOSTCC

[Rasmus Villemoes]

On 17/06/2020 05.11, Simon Glass wrote:
> Hi Rasmus,
> 
> On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> When building host tools, the CONFIG_GZIP etc. symbols are not defined
>> anyway, so this does not (should not) change anything [1]. However,
>> since the host tools also don't include linux/kconfig.h, one cannot
>> use the CONFIG_IS_ENABLED() smartness in a preprocessor conditional,
>> which in turn prevents one from adding, say, an
>>
>>   #if CONFIG_IS_ENABLED(ZSTD)
>>
>> case.
>>
>> OTOH, with this, one can do that, and it also makes it possible to
>> convert say, "#ifdef CONFIG_GZIP" to "#if CONFIG_IS_ENABLED(GZIP)" to
>> make those other cases SPL-or-not-SPL-aware.
>>
>> [1] The gzip.h header is only included under !USE_HOSTCC, and removing
>> the CONFIG_GZIP conditional hence both gives an "no declaration of
>> gunzip" warning as well as breaks the link of the host tools, since
>> the gunzip code is indeed not linked in.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  common/image.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/common/image.c b/common/image.c
>> index e1ca1a7905..73f6845274 100644
>> --- a/common/image.c
>> +++ b/common/image.c
>> @@ -458,6 +458,7 @@ int image_decomp(int comp, ulong load, ulong image_start, int type,
>>                 else
>>                         ret = -ENOSPC;
>>                 break;
>> +#ifndef USE_HOSTCC
> 
> In general I'm not a fan of the USE_HOSTCC stuff. I suspect HOST
> should be a Kconfig like anything else, although perhaps magically
> inserted somehow. Then we can do more things at compile-time.

Yes, as you can see in 2/4, I agree that the hostcc stuff is messy. I
imagine we can introduce a Kconfig.hosttools at some point which can
have a lot of def_bool y symbols (and of course one could eventually
allow them to be actually choosable, e.g. to allow the user to not build
some tool or functionality that he doesn't use and which requires some
external library dependency).

So for example, to make the fit_check_sign actually also check the
decompression, we'd add a

CONFIG_HOSTTOOL_GZIP
  def_bool y

move this USE_HOSTCC a bit further down, and change the #ifdef
CONFIG_GZIP to #if CONFIG_IS_ENABLED(GZIP) - all after checking all the
other places CONFIG_GZIP is tested... so not something that's just done
mechanically or over night, but I think it can be done mostly piecemeal.

Rasmus

[RFC PATCH 1/4] common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef USE_HOSTCC

[Tom Rini]

On Wed, Jun 17, 2020 at 09:07:20AM +0200, Rasmus Villemoes wrote:
> On 17/06/2020 05.11, Simon Glass wrote:
> > Hi Rasmus,
> > 
> > On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> When building host tools, the CONFIG_GZIP etc. symbols are not defined
> >> anyway, so this does not (should not) change anything [1]. However,
> >> since the host tools also don't include linux/kconfig.h, one cannot
> >> use the CONFIG_IS_ENABLED() smartness in a preprocessor conditional,
> >> which in turn prevents one from adding, say, an
> >>
> >>   #if CONFIG_IS_ENABLED(ZSTD)
> >>
> >> case.
> >>
> >> OTOH, with this, one can do that, and it also makes it possible to
> >> convert say, "#ifdef CONFIG_GZIP" to "#if CONFIG_IS_ENABLED(GZIP)" to
> >> make those other cases SPL-or-not-SPL-aware.
> >>
> >> [1] The gzip.h header is only included under !USE_HOSTCC, and removing
> >> the CONFIG_GZIP conditional hence both gives an "no declaration of
> >> gunzip" warning as well as breaks the link of the host tools, since
> >> the gunzip code is indeed not linked in.
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >> ---
> >>  common/image.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/common/image.c b/common/image.c
> >> index e1ca1a7905..73f6845274 100644
> >> --- a/common/image.c
> >> +++ b/common/image.c
> >> @@ -458,6 +458,7 @@ int image_decomp(int comp, ulong load, ulong image_start, int type,
> >>                 else
> >>                         ret = -ENOSPC;
> >>                 break;
> >> +#ifndef USE_HOSTCC
> > 
> > In general I'm not a fan of the USE_HOSTCC stuff. I suspect HOST
> > should be a Kconfig like anything else, although perhaps magically
> > inserted somehow. Then we can do more things at compile-time.
> 
> Yes, as you can see in 2/4, I agree that the hostcc stuff is messy. I
> imagine we can introduce a Kconfig.hosttools at some point which can
> have a lot of def_bool y symbols (and of course one could eventually
> allow them to be actually choosable, e.g. to allow the user to not build
> some tool or functionality that he doesn't use and which requires some
> external library dependency).
> 
> So for example, to make the fit_check_sign actually also check the
> decompression, we'd add a
> 
> CONFIG_HOSTTOOL_GZIP
>   def_bool y
> 
> move this USE_HOSTCC a bit further down, and change the #ifdef
> CONFIG_GZIP to #if CONFIG_IS_ENABLED(GZIP) - all after checking all the
> other places CONFIG_GZIP is tested... so not something that's just done
> mechanically or over night, but I think it can be done mostly piecemeal.

I agree, we can improve this area piecemeal.  We need to perhaps give a
harder think to what code should be shared directly and where it's more
of a pain than it's worth too.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200617/eb46c62c/attachment.sig>

[RFC PATCH 0/4] CONFIG_IS_ENABLED magic

[Tom Rini]

On Fri, Jun 12, 2020 at 01:02:12PM +0200, Rasmus Villemoes wrote:

> The first patch is just something I suggested to allow zstd support to
> move forward. The remaining ones aim to make it more ergonomic to use
> CONFIG_IS_ENABLED to exclude things from the build.
> 
> While it currently works just fine in C code that one can do
> 
>   if (CONFIG_IS_ENABLED(FOO)) {
> 
>   }
> 
> and have the compiler throw the whole block away, and then later the
> linker throw away any functions and/or data that turns out not be used
> anyway, it's currently somewhat uglier to exclude items from an array
> initializer - it requires three lines to do
> 
>   #if CONFIG_IS_ENABLED(FOO)
>   { some array item },
>   #endif
> 
> and grepping for the FOO symbol doesn't really show what it is used
> for including/excluding.
> 
> With the last patch, one can instead do
> 
>   CONFIG_IS_ENABLED(FOO, ({ some array item },))
> 
> It's just an RFC; I think this can be useful to reduce the size of
> SPL/TPL without too much cluttering of the source, others can
> disagree.
> 
> 
> Rasmus Villemoes (4):
>   common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef
>     USE_HOSTCC
>   linux/kconfig.h: simplify logic for choosing CONFIG_{SPL_,TPL_,}*
>   linux/kconfig.h: remove unused helper macros
>   linux/kconfig.h: create two- and three-argument versions of
>     CONFIG_IS_ENABLED
> 
>  common/image.c               |   2 +
>  include/linux/kconfig.h      | 103 ++++++++++++++++++-----------------
>  scripts/config_whitelist.txt |   2 -
>  3 files changed, 54 insertions(+), 53 deletions(-)

This is I believe a good step forward and should let us clean up our
code in a few areas.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200619/2d3791db/attachment.sig>

[RFC PATCH 4/4] linux/kconfig.h: create two- and three-argument versions of CONFIG_IS_ENABLED

[Simon Glass]

Hi,

On Tue, 16 Jun 2020 at 17:31, Simon Glass <sjg@google.com> wrote:
>
> Hi Rasmus,
>
> On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
> >
> > This adds a bunch of preprocessor magic to extend the capabilities of
> > CONFIG_IS_ENABLED. The existing semantics of
> >
> >   CONFIG_IS_ENABLED(FOO)
> >
> > expanding to a 1 or 0 (depending on build context and the defined-ness
> > or not of the appropriate CONFIG_FOO/CONFIG_SPL_FOO/CONFIG_TPL_FOO)
> > are of course preserved. With this, one is also allowed a two-argument
> > form
> >
> >   CONFIG_IS_ENABLED(FOO, (something))
> >
> > which expands to something precisely when CONFIG_IS_ENABLED(FOO) would
> > expand to 1, and expands to nothing otherwise. It is, in other words,
> > completely equivalent to the three lines
> >
> >   #if CONFIG_IS_ENABLED(FOO)
> >   something
> >   #endif
> >
> > The second argument must be parenthesized in order to allow any
> > tokens, including a trailing comma, to appear - one use case for this
> > is precisely to make it a bit more ergonomic to build an array and
> > only include certain items depending on .config. That should increase
> > both readability and not least "git grep"ability.
> >
> > A third variant is also introduced,
> >
> >   CONFIG_IS_ENABLED(FOO, (xxx), (yyy))
> >
> > which corresponds to
> >
> >   #if CONFIG_IS_ENABLED(FOO)
> >   xxx
> >   #else
> >   yyy
> >   #endif
> >
> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> >  include/linux/kconfig.h | 48 ++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 45 insertions(+), 3 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> See also this:
>
> http://patchwork.ozlabs.org/project/uboot/patch/20200522020223.230834-2-sjg at chromium.org/
>
> but I think your idea is a lot nicer.

I'd like to pick up these three Kconfig patches for dm/next. Any objection?

Regards,
SImon

[RFC PATCH 4/4] linux/kconfig.h: create two- and three-argument versions of CONFIG_IS_ENABLED

[Tom Rini]

On Fri, Jul 03, 2020 at 10:22:17AM -0600, Simon Glass wrote:
> Hi,
> 
> On Tue, 16 Jun 2020 at 17:31, Simon Glass <sjg@google.com> wrote:
> >
> > Hi Rasmus,
> >
> > On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> > >
> > > This adds a bunch of preprocessor magic to extend the capabilities of
> > > CONFIG_IS_ENABLED. The existing semantics of
> > >
> > >   CONFIG_IS_ENABLED(FOO)
> > >
> > > expanding to a 1 or 0 (depending on build context and the defined-ness
> > > or not of the appropriate CONFIG_FOO/CONFIG_SPL_FOO/CONFIG_TPL_FOO)
> > > are of course preserved. With this, one is also allowed a two-argument
> > > form
> > >
> > >   CONFIG_IS_ENABLED(FOO, (something))
> > >
> > > which expands to something precisely when CONFIG_IS_ENABLED(FOO) would
> > > expand to 1, and expands to nothing otherwise. It is, in other words,
> > > completely equivalent to the three lines
> > >
> > >   #if CONFIG_IS_ENABLED(FOO)
> > >   something
> > >   #endif
> > >
> > > The second argument must be parenthesized in order to allow any
> > > tokens, including a trailing comma, to appear - one use case for this
> > > is precisely to make it a bit more ergonomic to build an array and
> > > only include certain items depending on .config. That should increase
> > > both readability and not least "git grep"ability.
> > >
> > > A third variant is also introduced,
> > >
> > >   CONFIG_IS_ENABLED(FOO, (xxx), (yyy))
> > >
> > > which corresponds to
> > >
> > >   #if CONFIG_IS_ENABLED(FOO)
> > >   xxx
> > >   #else
> > >   yyy
> > >   #endif
> > >
> > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > ---
> > >  include/linux/kconfig.h | 48 ++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 45 insertions(+), 3 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > See also this:
> >
> > http://patchwork.ozlabs.org/project/uboot/patch/20200522020223.230834-2-sjg at chromium.org/
> >
> > but I think your idea is a lot nicer.
> 
> I'd like to pick up these three Kconfig patches for dm/next. Any objection?

I'll take them through master soon.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200703/51e0e83d/attachment.sig>