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 X-Spam-Level: X-Spam-Status: No, score=-7.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1837AC4338F for ; Mon, 9 Aug 2021 19:11:26 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 37E7161052 for ; Mon, 9 Aug 2021 19:11:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 37E7161052 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5E094807F3; Mon, 9 Aug 2021 21:11:22 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="KzwhwCh8"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id D8DED8262E; Mon, 9 Aug 2021 21:11:20 +0200 (CEST) Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 113368200B for ; Mon, 9 Aug 2021 21:11:16 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-qk1-x72b.google.com with SMTP id e14so19606117qkg.3 for ; Mon, 09 Aug 2021 12:11:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=wgH0nFKpOHxFobgmjLz2GAXovrNyWbSxNoLYu6YuEbc=; b=KzwhwCh8o08SiuDvf6eJS43M7HpF5Aqj/9kY9IhrHIDJOT4DhDepmHTfG+lFf6BhSq z4r3/bttxyII9dpWoGDBja6nvoWsjD9X8Aa4lM4CgDfN/N9WFybimMFw5FVcSy2ItxLI 7MyVnrYmg5P+AWP5g3/M7gpvfJ9kpUw740pA8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=wgH0nFKpOHxFobgmjLz2GAXovrNyWbSxNoLYu6YuEbc=; b=LJyrT0P8RCM5KbDTp+KCP0ToU4HvghBOPhMfKTjw4oU7MP6Tvh1DZYQ2i2KHZdLWk7 eK1prEpnd1oTCoIdN+PdSbvdc5oAup3NuZP+lB0Jc3ce9PiEf10wDCBPOhvnIXvbctD1 hlvmo76mXkRqaJekzSZna2XcnvvdV2GIZsYc65dFzqZ2o7jdy9PztFO77zikS5X3K5PF ogQBIx4OzaDRygCWq0Xf3HBsmRT+jm/y2dgrFxzXyI77orLwHz/JGdeh+9NjMiPmIoqf VGQFyGn107IGWdl4ln6LexpJZPekqAxHup05jDtHdjsuYze/U+drxnRVzYw2YyqE0Z2d wQXQ== X-Gm-Message-State: AOAM5310WOKt7jpAmJZ3O0z8TOYF5TWC0Rjg4mp/eYUSpAKlTmnrHFKH jkA4ZUnTgD/y3xzRPqsKH+5agw== X-Google-Smtp-Source: ABdhPJzlzFDFBKn3ZEJX2jXjxjKh+4v9TgSlt68SFsv2z/jcOYAUpzQIrvSm+HyAQihsIYhBKfvb5w== X-Received: by 2002:a05:620a:199f:: with SMTP id bm31mr11930460qkb.157.1628536274745; Mon, 09 Aug 2021 12:11:14 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b01-cbda-b1cf-f982-fc10-d612.res6.spectrum.com. [2603:6081:7b01:cbda:b1cf:f982:fc10:d612]) by smtp.gmail.com with ESMTPSA id f3sm7202295qti.65.2021.08.09.12.11.13 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Mon, 09 Aug 2021 12:11:13 -0700 (PDT) Date: Mon, 9 Aug 2021 15:11:11 -0400 From: Tom Rini To: Simon Glass Cc: U-Boot Mailing List , Marek Vasut , Masahiro Yamada , Heinrich Schuchardt , Bin Meng , Stefan Roese , Marek =?iso-8859-1?Q?Beh=FAn?= , Sean Anderson , Aaron Williams Subject: Re: RFC: Support for U-Boot phases in Kconfig Message-ID: <20210809191111.GD858@bill-the-cat> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="OF9salbnfkEgqvaK" Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 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.2 at phobos.denx.de X-Virus-Status: Clean --OF9salbnfkEgqvaK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote: > Hi, >=20 > U-Boot can be configured to build the source multiple times to product mu= ltiple > 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary Prog= ram > Loader) build can produce a cut-down image only suitable for loading U-Bo= ot > proper. >=20 > SPL does not want to use the same Kconfig options, since that would produ= ce the > same binary. Instead we have two copies of some options, one with an SPL = prefix, > that can be configured independently. In the source code we can use a mac= ro to > see if code should be run: >=20 > if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { > ... > } >=20 > This expands to check either checking SYS_STDIO_DEREGISTER or > SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built. >=20 > U-Boot also has TPL (Tertiary Program Loader) which works a similar way. = This > is causing quite an expansion of the Kconfig source, with quite a bit of > duplication. Each time a new feature needs to be supported in SPL, it inv= olves > a patch to add the same options again but for SPL. >=20 >=20 > Here are some proposed changes to make it easier to deal with SPL/TPL: >=20 > 1. Kconfig support >=20 > At present we do things like this when we need to control an option separ= ately > in U-Boot proper and SPL: >=20 > config SYS_STDIO_DEREGISTER > bool "Allow deregistering stdio devices" > depends on DM_DEVICE_REMOVE > default y if USB_KEYBOARD > help > Generally there is no need to deregister stdio devices since they > are never deactivated. But if a stdio device is used which can be > removed (for example a USB keyboard) then this option can be > enabled to ensure this is handled correctly. >=20 > config SPL_SYS_STDIO_DEREGISTER > bool "Allow deregistering stdio devices in SPL" > depends on SPL_DM_DEVICE_REMOVE > help > Generally there is no need to deregister stdio devices since they > are never deactivated. But if a stdio device is used which can be > removed (for example a USB keyboard) then this option can be > enabled to ensure this is handled correctly. This is very rarely > needed in SPL. >=20 > This is a pain. Apart from the duplication, sometimes the SPL version is = in a > different file or a different part of the file, making it hard to find re= lated > options or update them in sync. >=20 > Instead, we can add a 'phase' command to the Kconfig language, so we can = do: >=20 > config SYS_STDIO_DEREGISTER > bool "Allow deregistering stdio devices" > phases > depends on p.DM_DEVICE_REMOVE > phase MAIN default y if USB_KEYBOARD > help > Generally there is no need to deregister stdio devices since they > are never deactivated. But if a stdio device is used which can be > removed (for example a USB keyboard) then this option can be > enabled to ensure this is handled correctly. >=20 > 'phases' means this Kconfig option exists in all phases. You can also say > 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL. >=20 > 'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U= -Boot > (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty= ) and > for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMO= VE > (p is SPL_). This is somewhat similar in style to the special-case > 'depends on m' in Kconfig. >=20 > To make this work, we tell Kconfig that SPL is a phase with 'def_phase': >=20 > config SPL > def_phase > depends on SUPPORT_SPL > prompt "Enable SPL" > help > If you want to build SPL as well as the normal image, say Y. >=20 > It works just the same as a bool, but kconfig also uses it to automatical= ly add > new Kconfigs for each phase. In the above example it creates both > SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has th= e text > '(SPL) ' shown before the SPL option. >=20 > This can easily handle Kconfigs with similar dependencies and even differ= ent > ones. If the Kconfig options are not actually very similar we can still > create two separate copies instead, as now. >=20 > This allows us to substantially reduce the size and duplication in the Kc= onfig > defintions. It also reduces the pain of adding another phase to U-Boot. >=20 > Note: This change needs to be done in Linux, which owns Kconfig upstream. >=20 >=20 > 2.Rename SPL_TPL_ >=20 > This Makefile variable is used to reduce the number of duplicate rules in > makefiles: >=20 > obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) +=3D fdt_region.o >=20 > The SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the = other > phases. >=20 > This is confusing though, since CONFIG_SPL_BUILD it set even for TPL buil= ds, so > for example. with: >=20 > obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) +=3D fdt_region.o >=20 > the file is built for both SPL and TPL. >=20 > To help with this, We can rename SPL_TPL to PHASE_: >=20 > obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) +=3D fdt_region.o >=20 > or perhaps P_ which is more readable: >=20 > obj-$(CONFIG_$(P_)FIT_SIGNATURE) +=3D fdt_region.o >=20 >=20 > 3. Rename CONFIG_IS_ENABLED() >=20 > This macro is used to determine whether an option is enabled in the curre= nt > build phase: >=20 > if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { > printf("## Checking hash(es) for Image %s ... ", > fit_get_name(fit, node, NULL)); >=20 > It is quite long-winded and people sometimes add CONFIG_ to the option in= side > the brakets by mistake. It is also a bit confusing that IS_ENABLED() and > CONFIG_IS_ENABLED() mean completely different things. >=20 > Instead we can rename it to CONFIG: >=20 > if (CONFIG(FIT_SIGNATURE)) { > printf("## Checking hash(es) for Image %s ... ", > fit_get_name(fit, node, NULL)); >=20 > This is shorter and looks more like CONFIG_FIT_SIGATURE so people should = find > it easier to understand. Being shorter is a big help when converting from > use #if to if(), since an indentation is always enabled. This change makes > the CONFIG() check no longer than IS_ENABLED(). >=20 > It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which ma= kes > things much more convenient, since ideally if the toolchain permitted it,= we > would just use CONFIG_OPTION in the code. This is not possible at present= since > the option may not be defined, so can cause a compiler error. >=20 > Over time, perhaps the existing IS_ENABLED() will phase out, since in many > cases SPL will have its own options. We already see that CONFIG_IS_ENABLE= D is > more popular / useful: >=20 > $ git grep -w IS_ENABLED |wc -l > 902 > $ git grep -w CONFIG_IS_ENABLED |wc -l > 2282 >=20 >=20 > 4. Add macros to help avoid more #ifdefs >=20 > We sometimes have to use #ifdefs in structs or drivers: >=20 > struct spl_image_loader { > #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > const char *name; > #endif > ... > }; >=20 > UCLASS_DRIVER(spi) =3D { > .id =3D UCLASS_SPI, > .name =3D "spi", > .flags =3D DM_UC_FLAG_SEQ_ALIAS, > #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) > .post_bind =3D dm_scan_fdt_dev, > #endif > ... > }; >=20 > This is a pain. We can add an IF_CONFIG macro to help with this: >=20 > struct spl_image_loader { > IF_CONFIG(LIBCOMMON_SUPPORT, const char *name;) > ... > }; >=20 > UCLASS_DRIVER(spi) =3D { > .id =3D UCLASS_SPI, > .name =3D "spi", > .flags =3D DM_UC_FLAG_SEQ_ALIAS, > IF_CONFIG(REAL, .post_bind =3D dm_scan_fdt_dev,) > ... > }; >=20 > It still isn't wonderfully readable but it seems like an improvement. The > IF_CONFIG() macros could be implemented easily with the current > CONFIG_IS_EANBLED() macro. >=20 >=20 > 5. IF_CONFIG_INT() or similar >=20 > See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html >=20 >=20 > 6. Discarding static functions >=20 > We sometimes see code like this: >=20 > #if CONFIG_IS_ENABLED(OF_REAL) > static const struct udevice_id apl_ns16550_serial_ids[] =3D { > { .compatible =3D "intel,apl-ns16550" }, > { }, > }; > #endif >=20 > U_BOOT_DRIVER(intel_apl_ns16550) =3D { > .name =3D "intel_apl_ns16550", > .id =3D UCLASS_SERIAL, > .of_match =3D of_match_ptr(apl_ns16550_serial_ids), > .plat_auto =3D sizeof(struct apl_ns16550_plat), > .priv_auto =3D sizeof(struct ns16550), > ... > }; >=20 > The of_match_ptr() avoids an #ifdef in the driver declaration since it ev= aluates > to NULL if !CONFIG_IS_ENABLED(OF_REAL) but we still need an #ifdef around= the > function, since it is static and would otherwise produce a warning. >=20 > One solution is to drop the 'static'. But this is not very nice, since the > structure clearly should not be used from another file. >=20 > We can add STATIC_IF_CONFIG() to help with this: >=20 > STATIC_IF_CONFIG(OF_REAL) const struct udevice_id > apl_ns16550_serial_ids[] =3D { > { .compatible =3D "intel,apl-ns16550" }, > { }, > }; > #endif >=20 > It expands to 'static' if CONFIG(OF_REAL) is enabled, otherwise it expand= to > nothing, in the hope that the compiler drops the data. Failing that it wo= uld > also be possible to have it expand to '__section(".discard.config")' so a= t least > the struct is discarded, even if the compatible string is not. The behavi= our of > gcc/binutils in this area is not always as might be hoped. >=20 >=20 > Comments welcome! I think what this is really showing is that Yamada-san was right. All the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=3Daarch64-linux- $ make O=3Dobj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=3Dobj/spl khadas-edge-v-rk3399_spl_config all $ make O=3Dobj/khadas-edge-v-rk3399 TPL=3Dobj/tpl/u-boot.bin \ SPL=3Dobj/spl/u-boot.bin khadas-edge-v-rk3399_config all But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) +=3D foo.o would be a win. --=20 Tom --OF9salbnfkEgqvaK Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmERfcgACgkQFHw5/5Y0 tywaLAv9GQBQgkt0/X8SMwM4J+lB7v8xP/1i53IPcJmCclRzD7caJN1dUOoiSnsH 0zMWaKbYj837sIoKvge8JvioqOl/ITEi5OuOIXq96kpSKU3H1Q0mKUb27g/sjg87 l4crDRDHfHCLGG9u7EAooUODxERCH4XcYKXJnxEr30HK4xOk/Tk1AOxfeh+SxyQp yyWlH4LKbNnwyun9eMdeHxl8sy3BHHOL2PEk8jXn30CJL4fLS94Gfd0EifAFdEO1 JdM8Osietht1OwB/PT7gLcbWp+pZf1NzNHK/c0APdu6SnvKhkPuEvlz7+EeX8J4p 9XhWM8X+UefsYXCR5tIPJUoJuY8k++7H9LMJLtsmWr9kTio8I20N3frlSCsfGK4/ 3ch/gC7Yzxilrfgb30MGQF/0/Ipa3ixU34kt05MW8465Rl4vZjpkLJNZg8tndSHz VVp6Fz+WMx+A84k3WDpHbuZeCnjKud16fHlORjAzVvMmHz0uVWsnGIX8WfkW+FtN tahr7kGh =jjWO -----END PGP SIGNATURE----- --OF9salbnfkEgqvaK--