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.2 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 E2105C4338F for ; Tue, 10 Aug 2021 19:38:28 +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 E2C6F60F38 for ; Tue, 10 Aug 2021 19:38:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E2C6F60F38 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 7065982BEF; Tue, 10 Aug 2021 21:38:25 +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="XjOoHW3u"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 31A0382BEF; Tue, 10 Aug 2021 21:38:23 +0200 (CEST) Received: from mail-qt1-x82f.google.com (mail-qt1-x82f.google.com [IPv6:2607:f8b0:4864:20::82f]) (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 B3AD682BFD for ; Tue, 10 Aug 2021 21:38:13 +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-qt1-x82f.google.com with SMTP id l24so41089qtj.4 for ; Tue, 10 Aug 2021 12:38:13 -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=9PLnsP7+fAcemqU1wzE2u/6OkQyg2W7OExDVs7lSwxM=; b=XjOoHW3uJlRB3nwSX2BSTCaOQ087N+q7N8ySFMvAGV8AoRePpXd2TRrC5LOaqCpzJ5 puMBbyVx1+q1buSmouBcY2VIr9jWX7buj6wP7bmoUmUiXZbzommicXJmzqVrlJM6p4Hl X8T2DSLWdVB5M1hYqOUCsLUd0Yp3pvsjEQ+XY= 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=9PLnsP7+fAcemqU1wzE2u/6OkQyg2W7OExDVs7lSwxM=; b=D8/hvHt+3CqgUqOZ3iytNtI8SIZ3zQ4OKlnUqwjcEq7fuXepnM3v5JC6uw55DVleNl YurD4wQQ3DgMjLXFQ3wvX75wbGnIA/8LFZ3XOivLBjWFVXTGDeHJpvSSVOpDCH8hY2Rh maR0GKef8oobqWa73my6/IdRK3O9hx0am5KW6fo2DkfafYafpEJYdsOU84ElK8/5iFTA lrOwriQssnrp/m15uR4xutlRh50IEARzD7tzK0wv3dGNnV6bA0nimDY1bC7aRjUgcgYB QhbglxXX3eKyBMMuo3Jb6s9Xm8E5pUnuTG/RVcf902t4Db2BQ3zzOVES93l4M4t9mk/H zdYw== X-Gm-Message-State: AOAM530AV3Ft0NkxEjWhd8cCbV+FBkOoJ+m1Ln0SG7Auqfv2P+8AfFSr uW5Mvthy+0oTOLk4c0H+TGrGJQ== X-Google-Smtp-Source: ABdhPJwqiZdgAxG0B0YvGbi918nrzq7WMVXiWGWj1Djy76s8fYBJIz/2AaPBeo/jqm69Dqdb5s1nCg== X-Received: by 2002:a05:622a:174e:: with SMTP id l14mr26393326qtk.26.1628624292227; Tue, 10 Aug 2021 12:38:12 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b01-cbda-2899-f679-6391-9304.res6.spectrum.com. [2603:6081:7b01:cbda:2899:f679:6391:9304]) by smtp.gmail.com with ESMTPSA id k24sm6010172qki.11.2021.08.10.12.38.10 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Tue, 10 Aug 2021 12:38:11 -0700 (PDT) Date: Tue, 10 Aug 2021 15:38:09 -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: <20210810193809.GL858@bill-the-cat> References: <20210809191111.GD858@bill-the-cat> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="9v+t0xRk6rtskISq" 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 --9v+t0xRk6rtskISq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 10, 2021 at 08:58:46AM -0600, Simon Glass wrote: > Hi Tom, >=20 > On Mon, 9 Aug 2021 at 13:11, Tom Rini wrote: > > > > On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote: > > > Hi, > > > > > > U-Boot can be configured to build the source multiple times to produc= t multiple > > > 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary = Program > > > Loader) build can produce a cut-down image only suitable for loading = U-Boot > > > proper. > > > > > > SPL does not want to use the same Kconfig options, since that would p= roduce 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= macro to > > > see if code should be run: > > > > > > if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { > > > ... > > > } > > > > > > This expands to check either checking SYS_STDIO_DEREGISTER or > > > SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built. > > > > > > U-Boot also has TPL (Tertiary Program Loader) which works a similar w= ay. 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= involves > > > a patch to add the same options again but for SPL. > > > > > > > > > Here are some proposed changes to make it easier to deal with SPL/TPL: > > > > > > 1. Kconfig support > > > > > > At present we do things like this when we need to control an option s= eparately > > > in U-Boot proper and SPL: > > > > > > 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 ca= n be > > > removed (for example a USB keyboard) then this option can be > > > enabled to ensure this is handled correctly. > > > > > > 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 ca= n be > > > removed (for example a USB keyboard) then this option can be > > > enabled to ensure this is handled correctly. This is very rar= ely > > > needed in SPL. > > > > > > 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 fin= d related > > > options or update them in sync. > > > > > > Instead, we can add a 'phase' command to the Kconfig language, so we = can do: > > > > > > 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 ca= n be > > > removed (for example a USB keyboard) then this option can be > > > enabled to ensure this is handled correctly. > > > > > > 'phases' means this Kconfig option exists in all phases. You can also= say > > > 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL. > > > > > > 'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so f= or U-Boot > > > (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is e= mpty) and > > > for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_= REMOVE > > > (p is SPL_). This is somewhat similar in style to the special-case > > > 'depends on m' in Kconfig. > > > > > > To make this work, we tell Kconfig that SPL is a phase with 'def_phas= e': > > > > > > 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. > > > > > > It works just the same as a bool, but kconfig also uses it to automat= ically add > > > new Kconfigs for each phase. In the above example it creates both > > > SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name ha= s the text > > > '(SPL) ' shown before the SPL option. > > > > > > This can easily handle Kconfigs with similar dependencies and even di= fferent > > > ones. If the Kconfig options are not actually very similar we can sti= ll > > > create two separate copies instead, as now. > > > > > > This allows us to substantially reduce the size and duplication in th= e Kconfig > > > defintions. It also reduces the pain of adding another phase to U-Boo= t. > > > > > > Note: This change needs to be done in Linux, which owns Kconfig upstr= eam. > > > > > > > > > 2.Rename SPL_TPL_ > > > > > > This Makefile variable is used to reduce the number of duplicate rule= s in > > > makefiles: > > > > > > obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) +=3D fdt_region.o > > > > > > The SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for = the other > > > phases. > > > > > > This is confusing though, since CONFIG_SPL_BUILD it set even for TPL = builds, so > > > for example. with: > > > > > > obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) +=3D fdt_region.o > > > > > > the file is built for both SPL and TPL. > > > > > > To help with this, We can rename SPL_TPL to PHASE_: > > > > > > obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) +=3D fdt_region.o > > > > > > or perhaps P_ which is more readable: > > > > > > obj-$(CONFIG_$(P_)FIT_SIGNATURE) +=3D fdt_region.o > > > > > > > > > 3. Rename CONFIG_IS_ENABLED() > > > > > > This macro is used to determine whether an option is enabled in the c= urrent > > > build phase: > > > > > > if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { > > > printf("## Checking hash(es) for Image %s ... ", > > > fit_get_name(fit, node, NULL)); > > > > > > It is quite long-winded and people sometimes add CONFIG_ to the optio= n inside > > > the brakets by mistake. It is also a bit confusing that IS_ENABLED() = and > > > CONFIG_IS_ENABLED() mean completely different things. > > > > > > Instead we can rename it to CONFIG: > > > > > > if (CONFIG(FIT_SIGNATURE)) { > > > printf("## Checking hash(es) for Image %s ... ", > > > fit_get_name(fit, node, NULL)); > > > > > > This is shorter and looks more like CONFIG_FIT_SIGATURE so people sho= uld find > > > it easier to understand. Being shorter is a big help when converting = =66rom > > > use #if to if(), since an indentation is always enabled. This change = makes > > > the CONFIG() check no longer than IS_ENABLED(). > > > > > > It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, whic= h makes > > > 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 pre= sent since > > > the option may not be defined, so can cause a compiler error. > > > > > > 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_EN= ABLED is > > > more popular / useful: > > > > > > $ git grep -w IS_ENABLED |wc -l > > > 902 > > > $ git grep -w CONFIG_IS_ENABLED |wc -l > > > 2282 > > > > > > > > > 4. Add macros to help avoid more #ifdefs > > > > > > We sometimes have to use #ifdefs in structs or drivers: > > > > > > struct spl_image_loader { > > > #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > > > const char *name; > > > #endif > > > ... > > > }; > > > > > > 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_PLATDA= TA) > > > .post_bind =3D dm_scan_fdt_dev, > > > #endif > > > ... > > > }; > > > > > > This is a pain. We can add an IF_CONFIG macro to help with this: > > > > > > struct spl_image_loader { > > > IF_CONFIG(LIBCOMMON_SUPPORT, const char *name;) > > > ... > > > }; > > > > > > 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,) > > > ... > > > }; > > > > > > 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. > > > > > > > > > 5. IF_CONFIG_INT() or similar > > > > > > See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html > > > > > > > > > 6. Discarding static functions > > > > > > We sometimes see code like this: > > > > > > #if CONFIG_IS_ENABLED(OF_REAL) > > > static const struct udevice_id apl_ns16550_serial_ids[] =3D { > > > { .compatible =3D "intel,apl-ns16550" }, > > > { }, > > > }; > > > #endif > > > > > > 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), > > > ... > > > }; > > > > > > The of_match_ptr() avoids an #ifdef in the driver declaration since i= t evaluates > > > to NULL if !CONFIG_IS_ENABLED(OF_REAL) but we still need an #ifdef ar= ound the > > > function, since it is static and would otherwise produce a warning. > > > > > > One solution is to drop the 'static'. But this is not very nice, sinc= e the > > > structure clearly should not be used from another file. > > > > > > We can add STATIC_IF_CONFIG() to help with this: > > > > > > STATIC_IF_CONFIG(OF_REAL) const struct udevice_id > > > apl_ns16550_serial_ids[] =3D { > > > { .compatible =3D "intel,apl-ns16550" }, > > > { }, > > > }; > > > #endif > > > > > > It expands to 'static' if CONFIG(OF_REAL) is enabled, otherwise it ex= pand to > > > nothing, in the hope that the compiler drops the data. Failing that i= t would > > > also be possible to have it expand to '__section(".discard.config")' = so at least > > > the struct is discarded, even if the compatible string is not. The be= haviour of > > > gcc/binutils in this area is not always as might be hoped. > > > > > > > > > Comments welcome! > > > > I think what this is really showing is that Yamada-san was right. All >=20 > One thread where this was discussed was here I think: >=20 > https://yhbt.net/lore/all/20140624192425.9368.AA925319@jp.panasonic.com/T/ >=20 > I cannot find all the arguments for either side now. Do you have a > pointer to them? I don't off-hand. I'm pretty sure it's come up more than once tho. > > 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 >=20 > We also need to think about the tools which are presumably built separate= ly. I'd like to see a way to build less host tools by default, but I fear the problem of distro folks if we go that way as it's more of a world builder / CI person problem that we build so many tools every time. > We might have to build in the opposite order, because SPL needs to > grep the full devicetree, although I suppose we could just recompile > it. If we use dtb files from one stage in another stage directly (rather than being an included / packaged file) that seems like a potential problem. > > 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 > That definitely looks nice. >=20 > But how much of a win is this? I think a good sized one. Especially since it will let us remove some customization in the build code. > I understand how it could work and I think we did talk about this at > the time. But there are problems too that I'd like to review if we can > find them. Some I can think of: >=20 > - maintaining three or more separate defconfig files for each board Somewhat, yes. The SPL/TPL configs should be small. With some not overly judicious use of imply's in board/.../Kconfig it shouldn't be very much. I think it might emphasize that we do need a good way / place to set defaults for SoCs and boards. > - not sure how to handle dependencies between phases (e.g. > SPL_BLOBLIST has ''default y if BLOBLIST', or one phase expecting an > image to be in there) My first, but perhaps bad idea would be that we have say TARGET_AM335X_EVM in arch/arm/mach-omap2/am33xx/Kconfig still and a new TARGET_AM335X_EVM_OPTS in board/ti/am335x/Kconfig that would select/imply things that need to be enabled in all stages. > - running 'make menuconfig' updates one phase but not the others, > making things harder to understand I'm not sure this is a problem so much. TPL/SPL shouldn't have much configuring to them, and even less re-configuring. > - splitting up of the build as you note above, making it harder for > people to understand This I think is debatable. That we build and configure things the way we do isn't always obvious. More and better documentation, either way, would be good. > Interesting to see this comment: >=20 > http://u-boot.10912.n7.nabble.com/PATCH-v8-0-13-Kconfig-for-U-Boot-tt1853= 09.html#a185306 >=20 > "It would take really long term (one year? two year? I don't know) > to migrate from board headers to Kconfig. >=20 > So, two different infractructure must coexist in the interim." >=20 > That was 2014! I think we need a way to remove old CONFIGs and let > board maintainers add them back in Kconfig if needed. I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now. --=20 Tom --9v+t0xRk6rtskISq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmES1ZoACgkQFHw5/5Y0 tyy59gv/TgQa8+7YjrQnDQFk/myu9n1ZIGFez03q96krB65KUeK7UWoo+CTkXGVk 00Ti5gI2+3vYAHEKWEcnraoq259U9+6aMT2PKME4qG35gY7YQQzStW51n0WrdI5D KbabRD6x5L+aI0GsLg/Zneg6VBNjXUMgk+Iu3P2Mq+rfQ5SEDIkgZpIhVueMt+PN +6zA2Uiavxx5AaRfn7GsBJZ3taRpHfsK1tAAPULw1u6TXtHrGipSn1H5xjXKIbS8 qG8MiY1bQmBq4zEjxKmpxpL2eyD642YpuN3xLlnxuL462zMRtIIHPzThqH24ihKw 6W1C+0Nyacy5L/gk8z3NQnor+SEDLKn4rjJOMlKw5RBHJCOuwHwoiCj9TWYH5rqe ggWpKpl70ysgmM3fWvk0MDpPIg6+8uXr8a509wP/vObLu2Kx3gLtFoFz21Xo/s1f 4kaPrCI5xUNKBpmfuxUJs5QxgXmyLuUXQDB1qL9x6gwtDHUBQPtQAXo3uux/wtIO ejOigVq8 =l/Uj -----END PGP SIGNATURE----- --9v+t0xRk6rtskISq--