xPL Proposal
Tom Rini
trini at konsulko.com
Thu Feb 13 19:02:14 CET 2025
On Tue, Feb 11, 2025 at 08:03:20AM -0700, Simon Glass wrote:
> Hi,
>
> I just wanted to send a note to (re-)introduce my ideas[1] for the
> next iteration of xPL.
>
> A recent series introduced 'xPL' as the name for the various
> pre-U-Boot phases, so now CONFIG_XPL_BUILD means that this is any xPL
> phase and CONFIG_SPL means this really is the SPL phase, not TPL. We
> still use filenames and function naming which uses 'spl', but could
> potentially adjust that.
>
> The major remaining problem IMO is that it is quite tricky and
> expensive (in terms of time) to add a new phase. We also have some
> medium-sized problems:
>
> a. The $(PHASE_), $(SPL_) rules in the Makefile are visually ugly and
> can be confusing, particularly when combined with ifdef and ifneq
>
> b. We have both CONFIG_IS_ENABLED() and IS_ENABLED() and they mean
> different things. For any given option, some code uses one and some
> the other, depending on what problems people have met along the way.
>
> c. An option like CONFIG_FOO is ambiguous, in that it could mean that
> the option is enabled in one or more xPL phases, or just in U-Boot
> proper. The only way to know is to look for $(PHASE_) etc. in the
> Makefiles and CONFIG_IS_ENABLED() in the code. This is very confusing
> and has not scaled well.
>
> d. We need to retain an important feature: options from different
> phases can depend on each other. As an example, we might want to
> enable MMC in SPL by default, if MMC is enabled in U-Boot proper. We
> may also want to share values between phases, such as TEXT_BASE. We
> can do this easily today, just by adding Kconfig rules.
>
> Proposal
>
> 1. Adjust kconf to generate separate autoconf.h files for each phase.
> These contain the values for each Kconfig option for that phase. For
> example CONFIG_TEXT_BASE in autoconf_spl.h is SPL's text base.
>
> 2. Add a file to resolve the ambiguity in (c) above, listing the
> Kconfig options which should not be enabled/valid in any xPL build.
> There are around 200 of these.
>
> 3. Introduce CONFIG_PPL as a new prefix, meaning U-Boot proper (only),
> useful in rare cases. This indicates that the option applies only to
> U-Boot proper and is not defined in any xPL build. It is analogous to
> CONFIG_TPL_xxx meaning 'enabled in TPL'. Only a dozen of these are
> needed at present, basically to allow access to the value for another
> phase, e.g. SPL wanting to find CONFIG_PPL_TEXT_BASE so that it knows
> the address to which U-Boot should be loaded.
>
> 4. There is no change to the existing defconfig files, or 'make
> menuconfig', which works just as today, including dependencies between
> options across all phases.
>
> 5. (next) Expand the Kconfig language[2] to support declaring phases
> (SPL, TPL, etc.) and remove the need for duplicating options (DM_MMC,
> SPL_DM_MMC, TPL_DM_MMC, VPL_DM_MMC), so allowing an option to be
> declared once for any/all phases. We can then drop the file in 2
> above.
>
> With this, maintaining Kconfig options, Makefiles and adding a new
> phase should be considerably easier.
>
> Regards,
> Simon
>
> [1] https://lore.kernel.org/u-boot/20230131152702.249197-1-sjg@chromium.org/
> [2] https://lore.kernel.org/u-boot/CAK7LNARQ-PgxiCh+gm2efpfXmNBkdTp18OTk3sHtqNsk6by5-Q@mail.gmail.com/
I'm not sure where to bring this up, so I'm going back to the top. I
wondered if your proposal fixed the problem described here:
https://lore.kernel.org/all/01020194f07b459d-184820e6-90c2-44fe-9850-f57fc2ebaafe-000000@eu-west-1.amazonses.com/
which goes back to here:
https://lore.kernel.org/u-boot/20250211043335.92538-1-naoki@radxa.com/T/#m15a3058ef1ec8afb76528e1ed3edf864c8a77ef0
So I took:
https://source.denx.de/u-boot/custodians/u-boot-dm/-/tree/splc-working?ref_type=heads
and then modified pinebook-pro-rk3399_defconfig as described.
This fails to build to start with as:
CC spl/drivers/usb/gadget/g_dnl.o
In file included from /home/trini/work/u-boot/u-boot/drivers/usb/gadget/g_dnl.c:25:
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/composite.c: In function 'config_buf':
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/composite.c:214:47: error: 'CONFIG_USB_GADGET_VBUS_DRAW' undeclared (first use in this function)
214 | c->bMaxPower = config->bMaxPower ? : (CONFIG_USB_GADGET_VBUS_DRAW / 2);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/composite.c:214:47: note: each undeclared identifier is reported only once for each function it appears in
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/composite.c: In function 'set_config':
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/composite.c:462:53: error: 'CONFIG_USB_GADGET_VBUS_DRAW' undeclared (first use in this function)
462 | power = c->bMaxPower ? (2 * c->bMaxPower) : CONFIG_USB_GADGET_VBUS_DRAW;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/g_dnl.c: At top level:
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/g_dnl.c:49:36: error: 'CONFIG_USB_GADGET_MANUFACTURER' undeclared here (not in a function)
49 | static const char manufacturer[] = CONFIG_USB_GADGET_MANUFACTURER;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/trini/work/u-boot/u-boot/arch/arm/include/asm/byteorder.h:29,
from /home/trini/work/u-boot/u-boot/include/linux/libfdt_env.h:15,
from /home/trini/work/u-boot/u-boot/include/linux/libfdt.h:6,
from /home/trini/work/u-boot/u-boot/include/fdtdec.h:17,
from /home/trini/work/u-boot/u-boot/include/usb.h:14,
from /home/trini/work/u-boot/u-boot/drivers/usb/gadget/g_dnl.c:15:
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/g_dnl.c:65:44: error: 'CONFIG_USB_GADGET_VENDOR_NUM' undeclared here (not in a function)
65 | .idVendor = __constant_cpu_to_le16(CONFIG_USB_GADGET_VENDOR_NUM),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/trini/work/u-boot/u-boot/include/linux/byteorder/little_endian.h:24:60: note: in definition of macro '__constant_cpu_to_le16'
24 | #define __constant_cpu_to_le16(x) ((__force __le16)(__u16)(x))
| ^
/home/trini/work/u-boot/u-boot/drivers/usb/gadget/g_dnl.c:66:45: error: 'CONFIG_USB_GADGET_PRODUCT_NUM' undeclared here (not in a function)
66 | .idProduct = __constant_cpu_to_le16(CONFIG_USB_GADGET_PRODUCT_NUM),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/trini/work/u-boot/u-boot/include/linux/byteorder/little_endian.h:24:60: note: in definition of macro '__constant_cpu_to_le16'
24 | #define __constant_cpu_to_le16(x) ((__force __le16)(__u16)(x))
| ^
make[4]: *** [/home/trini/work/u-boot/u-boot/scripts/Makefile.build:267: spl/drivers/usb/gadget/g_dnl.o] Error 1
make[3]: *** [/home/trini/work/u-boot/u-boot/scripts/Makefile.build:407: spl/drivers/usb/gadget] Error 2
make[2]: *** [/home/trini/work/u-boot/u-boot/scripts/Makefile.spl:523: spl/drivers] Error 2
make[1]: *** [/home/trini/work/u-boot/u-boot/Makefile:2055: spl/u-boot-spl] Error 2
make[1]: Leaving directory '/tmp/pinebook-pro-rk3399'
make: *** [Makefile:177: sub-make] Error 2
I tried adding the following:
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index e120efeb0073..c28ff53f3770 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -47,6 +47,16 @@ config USB_GADGET_MANUFACTURER
Vendor name of the USB device emulated, reported to the host device.
This is usually either the manufacturer of the device or the SoC.
+config SPL_USB_GADGET_MANUFACTURER
+ string
+ depends on USB_GADGET_MANUFACTURER
+ default USB_GADGET_MANUFACTURER
+
+config TPL_USB_GADGET_MANUFACTURER
+ string
+ depends on USB_GADGET_MANUFACTURER
+ default USB_GADGET_MANUFACTURER
+
config USB_GADGET_VENDOR_NUM
hex "Vendor ID of the USB device"
default 0x1f3a if ARCH_SUNXI
@@ -57,6 +67,16 @@ config USB_GADGET_VENDOR_NUM
This is usually the board or SoC vendor's, unless you've registered
for one.
+config SPL_USB_GADGET_VENDOR_NUM
+ depends on USB_GADGET_VENDOR_NUM
+ default USB_GADGET_VENDOR_NUM
+ hex
+
+config TPL_USB_GADGET_VENDOR_NUM
+ depends on USB_GADGET_VENDOR_NUM
+ default USB_GADGET_VENDOR_NUM
+ hex
+
config USB_GADGET_PRODUCT_NUM
hex "Product ID of the USB device"
default 0x1010 if ARCH_SUNXI
@@ -70,6 +90,16 @@ config USB_GADGET_PRODUCT_NUM
help
Product ID of the USB device emulated, reported to the host device.
+config SPL_USB_GADGET_PRODUCT_NUM
+ hex
+ depends on USB_GADGET_PRODUCT_NUM
+ default USB_GADGET_PRODUCT_NUM
+
+config TPL_USB_GADGET_PRODUCT_NUM
+ hex
+ depends on USB_GADGET_PRODUCT_NUM
+ default USB_GADGET_PRODUCT_NUM
+
config USB_GADGET_ATMEL_USBA
bool "Atmel USBA"
select USB_GADGET_DUALSPEED
@@ -149,6 +179,11 @@ config USB_GADGET_VBUS_DRAW
This value will be used except for system-specific gadget
drivers that have more specific information.
+config SPL_USB_GADGET_VBUS_DRAW
+ int
+ depends on USB_GADGET_VBUS_DRAW
+ default USB_GADGET_VBUS_DRAW
+
config SDP_LOADADDR
hex "Default load address at SDP_WRITE and SDP_JUMP"
default 0
But I guess there's just a bug with that iteration of the series and
converting non-boolean options to all their xPL variants.
It also I think highlights two big problems.
One, without the Kconfig language modification as well we're going to
have an even worse problem of duplicating questions.
Two, it doesn't solve the problem of it being confusing to avoid
building FOO when FOO isn't needed on a phase. In this example we're
still building drivers/usb/dwc3/ in TPL and need 2/2 in that series to
disable a warning.
--
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/20250213/7c8099a3/attachment.sig>
More information about the U-Boot
mailing list