On Mon, Jun 08, 2026 at 04:22:41PM +0200, Quentin Schulz wrote: > > > On 6/8/26 4:04 PM, Tom Rini wrote: > > On Mon, Jun 08, 2026 at 03:59:11PM +0200, Quentin Schulz wrote: > > > Hi Daniel, > > > > > > On 6/8/26 3:06 PM, Daniel Palmer wrote: > > > > - CONFIG_LOADB causes the xyzModem code to get built and that code > > > > depends > > > > on CONFIG_CRC16. If you don't have something else that selects > > > > CONFIG_CRC16 > > > > you can unselect it and then the final link will fail: > > > > > > > > LD u-boot > > > > /usr/bin/m68k-linux-gnu-ld.bfd: common/xyzModem.o: in function > > > > `xyzModem_get_hdr': > > > > u-boot/common/xyzModem.c:388:(.text.xyzModem_get_hdr+0x17a): undefined > > > > reference to `crc16_ccitt' > > > > > > > > - Add CONFIG_YMODEM_SUPPORT, make that select CONFIG_CRC16 and make > > > > CONFIG_LOADB select CONFIG_YMODEM_SUPPORT. > > > > > > > > - SPL/TPL already have Kconfig symbols for ymodem support but the spl > > > > ymodem code has issues building in the TPL phase and doesn't seem to > > > > be used: > > > > > > > > /usr/bin/aarch64-linux-gnu-ld.bfd: common/spl/spl_ymodem.o: in function > > > > `spl_ymodem_load_image': > > > > u-boot/common/spl/spl_ymodem.c:138:(.text.spl_ymodem_load_image+0xdc): > > > > undefined reference to `spl_load_simple_fit' > > > > u-boot/common/spl/spl_ymodem.c:138:(.text.spl_ymodem_load_image+0xdc): > > > > relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol > > > > `spl_load_simple_fit' > > > > make[1]: *** [scripts/Makefile.xpl:546: tpl/u-boot-tpl] Error 1 > > > > > > > > - Remove the ymodem symbol entirely for TPL. > > > > > > > > - Select CONFIG_SPL_CRC16 for SPL. > > > > > > > > - Finally, since everyone is now using the ymodem support symbols remove > > > > the special rule for loadb in the makefile. > > > > > > > > > > When you need to list what you're doing in a commit log, it usually means > > > you should be splitting the content into multiple separate patches (in a > > > single series is usually fine/recommended). > > > > > > What I could identify (slightly reworked, which better match how *I* would > > > write the patches): > > > > > > - changes made to common/spl/Kconfig.tpl with the justification given > > > above, > > And I also see a mention of CONFIG_TPL_YMODEM_SUPPORT=y in > doc/usage/spl_boot.rst that would need removal > > > > - adding the missing CRC16 dependency to CMD_LOADB, > > > - adding missing SPL_CRC16 dependency to SPL_YMODEM_SUPPORT (possibly > > > squashed with the CMD_LOADB+CRC16 patch above, I'm undecided on what's > > > best > > > here), > > > - create new YMODEM_SUPPORT symbol and migrate CMD_LOADB + common/Makefile > > > to use that, > > > > > > This makes it clearer what the purpose of each individual commit is: > > > - remove dead code > > > - fix existing support for CMD_LOADB, > > > - fix existing support for SPL_YMODEM_SUPPORT, > > > - quality of life improvement by migrating the CRC16 dependency from > > > CMD_LOADB into a new symbol + making use of that new symbol in > > > common/Makefile instead of "hardlinking" CMD_LOADB with building > > > xyzModem.o > > > (which is quite honestly confusing, so good to see this fixed!), > > > > > > What do you think? > > > > > > > Signed-off-by: Daniel Palmer <[email protected]> > > > > --- > > > > > > > > v2: Fix the SPL/TPL stuff as brought up by Tom. This opened a can of > > > > worms when I tested if the TPL builds or not so I decided to just > > > > remove the TPL ymodem stuff. It didn't seem to be used in any > > > > defconfig and there isn't a CONFIG_TPL_CRC16 to enable the needed > > > > crc16 code anyhow so maybe it hasn't been used for a while? > > > > > > > > cmd/Kconfig | 1 + > > > > common/Kconfig | 4 ++++ > > > > common/Makefile | 1 - > > > > common/spl/Kconfig | 1 + > > > > common/spl/Kconfig.tpl | 11 +---------- > > > > 5 files changed, 7 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig > > > > index c71c6824a196..503fc51f6bb4 100644 > > > > --- a/cmd/Kconfig > > > > +++ b/cmd/Kconfig > > > > @@ -1412,6 +1412,7 @@ config CMD_W1 > > > > config CMD_LOADB > > > > bool "loadb" > > > > default y > > > > + select YMODEM_SUPPORT > > > > help > > > > Load a binary file over serial line. > > > > diff --git a/common/Kconfig b/common/Kconfig > > > > index 8e8c733aa295..0db74aa580e4 100644 > > > > --- a/common/Kconfig > > > > +++ b/common/Kconfig > > > > @@ -1274,3 +1274,7 @@ config IO_TRACE > > > > confirm that the driver behaves the same way before and after > > > > a code > > > > change. To add support for your architecture, add '#include > > > > <iotrace.h>' to the bottom of arch/<arch>/include/asm/io.h > > > > and test. > > > > + > > > > +config YMODEM_SUPPORT > > > > + bool > > > > + select CRC16 > > > > > > Is there any reason we wouldn't want the user to be able to select this > > > independently from CMD_LOADB? > > > > > > I'm asking because this is both invisible (no prompt) and has no help > > > text. > > > > I think this should stay hidden as it's *not* hidden for xPL as it's the > > equivalent of CMD_LOADB but it's the symbol for the library portion. As > > a maybe clearer result, maybe the library symbol should be > > XYZMODEM_SUPPORT / xPL_XYZMODEM_SUPPORT, to control building > > common/xyzModem.o ? > > > > I'm not sure to understand how the rename would help here? I'm assuming we > have an issue of the symbol having a different meaning in xPL and in proper.
To try and be clear, we don't have a symbol for common/xyzModem.o today, I was suggesting introducing one as part of fixing Daniel's problem. > If I understood correctly, YMODEM_SUPPORT in proper is for building the In this v2, it would be for that, yes. > library and using it, while in xPL it's both for building the library and > add support for loading the next U-Boot stage over UART. Is this what you Correct, xPL_YMODEM_SUPPORT covers both common/xyzModem.o (library) and common/spl/spl_ymodem.o (user). > meant? Are you suggesting we split "build library code" and "load next stage > from UART" into separate symbols? Thinking harder about the problem, yes. That would make things clearer in the long term. Maybe even with a move of common/xyzModem.c to lib/xyzModem.c (and yes bad filename, but matches the ancient upstream its from I bet) and introduction of LIB_XYZMODEM (and xPL_LIBXYZMODEM) which in turn selects the required CRC16 symbols. > Also, lib/Makefile has > > obj-$(CONFIG_SPL_YMODEM_SUPPORT) += crc16-ccitt.o > > which is.... odd? Shouldn't this handle the case of "missing" SPL_CRC16 > dependency? So: does it really not build if SPL_CRC16=n but > SPL_YMODEM_SUPPORT=y? If it does not build, is bringing SPL_CRC16 too much? > We only need crc16-ccitt.o I think, and this will also bring crc16.o into > the SPL, which may not be desired (size increase). This indeed looks to be another odd case to look in to and figure out the right solution for. -- Tom
signature.asc
Description: PGP signature
