Re: [PATHv11 28/43] configs/turris_omnia_defconfig: enable LTO
On 2023-11-27 06:57, Maxim Uvarov wrote: Decrease allowed binary size to fit lwip code. u-boot-with-spl.kwb exceeds file size limit: limit: 0xf6000 bytes actual: 0xf8600 bytes excess: 0x2600 bytes Signed-off-by: Maxim Uvarov --- configs/turris_omnia_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig index afcd4a1eb7..4e965c795a 100644 --- a/configs/turris_omnia_defconfig +++ b/configs/turris_omnia_defconfig @@ -118,3 +118,4 @@ CONFIG_USB_EHCI_HCD=y CONFIG_WDT=y CONFIG_WDT_ORION=y CONFIG_EXT4_WRITE=y +CONFIG_LTO=y The last time I tried enabling LTO for omnia, the mvneta driver stopped working correctly. So this first need to test everything thoroughly. Marek
Re: [PATCH] arm: mvebu: a38x: Define supported UART baudrates
Reviewed-by: Marek Behún
Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
On Mon, 26 Jul 2021 16:58:04 +0200 Pali Rohár wrote: > On Monday 26 July 2021 16:55:22 Marek Behun wrote: > > On Mon, 26 Jul 2021 14:58:59 +0200 > > Pali Rohár wrote: > > > > > Static inline function _debug_uart_init() should avoid calling external > > > (non-inline) functions. > > > > Why? > > Function is called in stage when stack is not fully initialized and > documentation suggest to avoid stack usage and other functions. OK, but maybe we should use the macro names for register constants. Could you move (in a separate patch) the corresponding macros from arch/arm/mach-mvebu/armada3700/cpu.c to arch/arm/mach-mvebu/include/mach/soc.h and then in this patch include in the serial driver and use the constant names? Thanks. Marek
Re: [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART
On Mon, 26 Jul 2021 14:58:58 +0200 Pali Rohár wrote: > CONFIG_BAUDRATE should be used for setting the baudrate for the early debug > UART. This replaces current hardcoded 115200 value. > > Signed-off-by: Pali Rohár Reviewed-by: Marek Behun
Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár wrote: > Static inline function _debug_uart_init() should avoid calling external > (non-inline) functions. Why?
Re: [PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart
Hi Jagan, On Wed, 21 Jul 2021 21:46:56 +0530 Jagan Teki wrote: > Found the build error with CI [1], would you please check? > > [1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345 > > Jagan. OK I think I've found out what is the problem. I've pushed new version into github CI to check if it builds correctly. The problem seems to be that after this series the function spi_nor_erase() calls mtd_erase_callback(), which is declared in the header file include/linux/mtd/mtd.h, if CONFIG_MTD_PARTITIONS is enabled, and defined as a static inline function otherwise. The problem is that for some boards we have CONFIG_MTD_PARTITIONS together with CONFIG_SPL_SPI_FLASH_SUPPORT. But in SPL, mtdpart.c (where mtd_erase_callback() is defined) is not compiled at all. Thus this leads to undefined reference to mtd_erase_callback(). This is another proof that the whole mtd subsystem has become a gross spaghetti code where hacks upon hacks were introduced by different people to solve different purposes, and the result makes me angry. :-D We really need to rewrite this. Anyway, for now I will just send v2 of this series with another patch fixing this issue, once CI ends smoothly. Marek
Re: [PATCH RESEND u-boot-spi 0/8] Fix `mtd erase` when used with mtdpart
On Wed, 21 Jul 2021 21:46:56 +0530 Jagan Teki wrote: > Hi Marek, > > On Thu, Jul 15, 2021 at 5:21 AM Marek Behún wrote: > > > > Hello, > > > > I accidentally forgot to send this series to U-Boot's mailing list last > > time, meaning it did not end up in patchwork, so now I am resending it. > > Sorry for this mess. > > > > The original cover letter said: > > > > this patch series fixes the `mtd erase` command when used with mtdpart > > with a partition of non-zero offset. > > > > Currently when the `mtd erase` command is used for such a partition, > > it does not erase all blocks. Instead after a block is erased, the next > > block address not current block address + block size, but current block > > address + block size + partition offset, due to spi_nor_erase() not > > calling mtd_erase_callback(): > > => mtd erase "Rescue system" > > Erasing 0x ... 0x006f (1792 eraseblock(s)) > > jedec_spi_nor spi-nor@0: at 0x10, len 4096 > > jedec_spi_nor spi-nor@0: at 0x201000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x302000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x403000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x504000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x605000, len 4096 > > jedec_spi_nor spi-nor@0: at 0x706000, len 4096 > > > > This series adds some fixes to spi_nor_erase() function, then adds > > calling of mtd_erase_callback() to fix this bug. > > > > The series also contains an improvement - adding the posibility to > > interrupt spi_nor_erase() with Ctrl+C; and another one - making mtdpart's > > _erase() method more sane so that the above mentioned bug will not occur > > even if underlying driver does not call mtd_erase_callback(). > > > > Moreover I would also like to start a discussion regarding the MTD > > subsystem: > > - U-Boot's MTD subsystem is based on Linux's, with many #ifdef __U_BOOT__ > > macros > > - this was done to make it easier to port Linux's patches to U-Boot > > - the problem is that it seems nobody did port Linux's MTD patches to > > U-Boot for a long time, the code is many times different from Linux', > > and it would be very hard to align it > > - therefore I propose to get rid of all the #ifdefs, remove the Linux > > specific code, and continue developing the code independently from > > Linux. This would make it impossible to apply Linux patches in some > > kind of automatic way, but this is currently already impossible > > anyway > > What do you guys think? > > > > Marek > > > > Marek Behún (8): > > mtd: spi-nor-core: Try cleaning up in case writing BAR failed > > mtd: spi-nor-core: Check return value of write_enable() in > > spi_nor_erase() > > mtd: spi-nor-core: Don't overwrite return value if it is non-zero > > mtd: spi-nor-core: Check return value of write_disable() in > > spi_nor_erase() > > mtd: spi-nor-core: Don't check for zero length in spi_nor_erase() > > mtd: spi-nor-core: Call mtd_erase_callback() from spi_nor_erase() > > mtd: spi-nor-core: Check for ctrlc() in spi_nor_erase() > > mtd: mtdpart: Make mtdpart's _erase method sane > > Found the build error with CI [1], would you please check? > > [1] https://source.denx.de/u-boot/custodians/u-boot-spi/-/pipelines/8345 > > Jagan. Jagan, I am unable to get the output of the failed CI tests. Probably because I do not have an account at source.denx.de. I tried to run "sandbox with clang" CI scenario on my local machine and it is successful. Can you send me outputs of the failed scenarios? Marek
Re: [RESEND PATCH] build: remove the variable NM in gen_ll_addressable_symbols.sh
On Wed, 21 Jul 2021 09:56:07 +0200 Patrick Delaunay wrote: > With LTO activated, the buildman tools failed with an error on my > configuration (Ubuntu 20.04, stm32mp15_trusted_defconfig) with the error: > > ../arm-linux-gnueabi/bin/nm: > scripts/gen_ll_addressable_symbols.sh: file format not recognized > > It seems the shell variable initialization NM=$(NM) is not correctly > interpreted when shell is started in the Makefile, but I have not this > issue when I compile the same target without buildman. > > I don't found the root reason of the problem but I solve it by > providing $(NM) as script parameter instead using a shell variable. > > The command executed is identical: > > cmd_keep-syms-lto.c := NM=arm-none-linux-gnueabihf-gcc-nm \ > u-boot/scripts/gen_ll_addressable_symbols.sh arch/arm/cpu/built-in.o \ > net/built-in.o >keep-syms-lto.c > > cmd_keep-syms-lto.c := u-boot/scripts/gen_ll_addressable_symbols.sh \ > arm-none-linux-gnueabihf-gcc-nm arch/arm/cpu/built-in.o \ > ... net/built-in.o > keep-syms-lto.c > > Reviewed-by: Simon Glass > Signed-off-by: Patrick Delaunay > --- > Resend with correct commit message for patman > s/Serie-cc/Series-cc/ > > > Makefile | 2 +- > scripts/Makefile.spl | 2 +- > scripts/gen_ll_addressable_symbols.sh | 5 - > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Makefile b/Makefile > index ca2432c8ce..140dea09f4 100644 > --- a/Makefile > +++ b/Makefile > @@ -1736,7 +1736,7 @@ u-boot-keep-syms-lto_c := $(patsubst > %.o,%.c,$(u-boot-keep-syms-lto)) > > quiet_cmd_keep_syms_lto = KSL $@ >cmd_keep_syms_lto = \ > - NM=$(NM) $(srctree)/scripts/gen_ll_addressable_symbols.sh $^ >$@ > + $(srctree)/scripts/gen_ll_addressable_symbols.sh $(NM) $^ > $@ > > quiet_cmd_keep_syms_lto_cc = KSLCC $@ >cmd_keep_syms_lto_cc = \ > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > index 5be1a9ba1b..25a3e7fa52 100644 > --- a/scripts/Makefile.spl > +++ b/scripts/Makefile.spl > @@ -459,7 +459,7 @@ u-boot-spl-keep-syms-lto_c := \ > > quiet_cmd_keep_syms_lto = KSL $@ >cmd_keep_syms_lto = \ > - NM=$(NM) $(srctree)/scripts/gen_ll_addressable_symbols.sh $^ >$@ > + $(srctree)/scripts/gen_ll_addressable_symbols.sh $(NM) $^ > $@ > > quiet_cmd_keep_syms_lto_cc = KSLCC $@ >cmd_keep_syms_lto_cc = \ > diff --git a/scripts/gen_ll_addressable_symbols.sh > b/scripts/gen_ll_addressable_symbols.sh > index 3978a39d97..b8840dd011 100755 > --- a/scripts/gen_ll_addressable_symbols.sh > +++ b/scripts/gen_ll_addressable_symbols.sh > @@ -5,8 +5,11 @@ > # Generate __ADDRESSABLE(symbol) for every linker list entry symbol, so that > LTO > # does not optimize these symbols away > > +# The expected parameter of this script is the command requested to have > +# the U-Boot symbols to parse, for example: $(NM) $(u-boot-main) > + > set -e > > echo '#include ' > -$NM "$@" 2>/dev/null | grep -oe > '_u_boot_list_2_[a-zA-Z0-9_]*_2_[a-zA-Z0-9_]*' | \ > +$@ 2>/dev/null | grep -oe '_u_boot_list_2_[a-zA-Z0-9_]*_2_[a-zA-Z0-9_]*' | \ > sort -u | sed -e 's/^\(.*\)/extern char \1[];\n__ADDRESSABLE(\1);/' Shouldn't we use "$@" ? In case the arguments contain spaces? Marek
Re: non-DM SPI_FLASH removal?
On Mon, 12 Jul 2021 10:10:14 -0400 Tom Rini wrote: > On Mon, Jul 12, 2021 at 04:01:13PM +0200, Marek Behun wrote: > > > Tom, > > > > there are 16 boards left with CONFIG_SPI_FLASH > > (git grep CONFIG_SPI_FLASH=y) > > and Makefile says this was supposed to be deprecated in v2019.07. > > > > Are we going to remove them so that we can simplify the SPI subsystem? > > The warning is on CONFIG_SPI_FLASH without CONFIG_DM_SPI_FLASH and of > course outside of SPL. When I asked Jagan about this off-list last > month or so, he said everything had been migrated, and I couldn't make > any boards fail by making the migration warning in the makefile be an > error. I think that means that if you have SPI subsystem cleanups you > want to do / post, please do so, thanks! > None of the boards from git grep CONFIG_SPI_FLASH=y have CONFIG_DM_SPI_FLASH defined. These boards are not migrated. Marek
Re: non-DM SPI_FLASH removal?
On Mon, 12 Jul 2021 16:01:13 +0200 Marek Behun wrote: > Tom, > > there are 16 boards left with CONFIG_SPI_FLASH > (git grep CONFIG_SPI_FLASH=y) > and Makefile says this was supposed to be deprecated in v2019.07. > > Are we going to remove them so that we can simplify the SPI subsystem? > > Marek Alternatively it seems all those boards have devicetrees, and at91-vinco even has "jedec,spi-nor" compatible device defined. The m68k devices don't have SPI-NOR device under their spi controller in device-tree, though.
non-DM SPI_FLASH removal?
Tom, there are 16 boards left with CONFIG_SPI_FLASH (git grep CONFIG_SPI_FLASH=y) and Makefile says this was supposed to be deprecated in v2019.07. Are we going to remove them so that we can simplify the SPI subsystem? Marek
Re: [PATCH v2 3/6] dts: synquacer: Add partition information to the spi-nor
On Sun, 11 Jul 2021 09:46:19 +0900
Masami Hiramatsu wrote:
> Add partition information to the spi-nor flash.
> This is required for accessing NOR flash via mtdparts.
>
> Signed-off-by: Masami Hiramatsu
> ---
> Changes in v2:
> - Add new lines to separate the partitions.
> ---
> .../dts/synquacer-sc2a11-developerbox-u-boot.dtsi | 49
>
> 1 file changed, 49 insertions(+)
>
> diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> index 2f13a42235..7a56116d6f 100644
> --- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> +++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> @@ -31,6 +31,55 @@
> spi-max-frequency = <3125>;
> spi-rx-bus-width = <0x1>;
> spi-tx-bus-width = <0x1>;
> +
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + partition@0 {
> + label = "BootStrap-BL1";
> + reg = <0x0 0x7>;
> + read-only;
> + };
> +
> + partition@7 {
> + label = "Flash-Writer";
> + reg = <0x7 0x9>;
> + read-only;
> + };
> +
> + partition@10 {
> + label = "SCP-BL2";
> + reg = <0x10 0x8>;
> + read-only;
> + };
> +
> + partition@18 {
> + label = "FIP-TFA";
> + reg = <0x18 0x78000>;
> + };
> +
> + partition@1f8000 {
> + label = "Stage2Tables";
> + reg = <0x1f8000 0x8000>;
> + };
> +
> + partition@20 {
> + label = "U-Boot";
> + reg = <0x20 0x10>;
> + };
> +
> + partition@30 {
> + label = "UBoot-Env";
> + reg = <0x30 0x10>;
> + };
> +
> + partition@50 {
> + label = "Ex-OPTEE";
> + reg = <0x50 0x20>;
> + };
> + };
> };
> };
>
>
Reviewed-by: Marek Behún
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Tom, Sean, Wolfgang and others, here are some of my opinions for this discussion - I agree with Wolfgang that there are far better options than a Tcl-like shell, if we want to add another language - I also think that instead of adding another language, it is more preferable to improve the existing one. Adding a new language will cause more problems in the future: - I think it can end up with OS distributions needing to write boot scripts in both languages, because they can't be sure which will be compiled into U-Boot - we will certainly end up with more bugs - userbase will fragment between the two languages - I think we can start improving the current U-Boot's shell in ways that are incompatible with upstream Hush. The idea back then, as I understand it, was to minimize man-hours invested into the CLI code, and so an existing shell was incorporated (with many #ifdef guards). But U-Boot has since evolved so much that it is very probable it would be more economic to simply fork from upsteam Hush, remove all the #ifdefs and start developing features we want in U-Boot. Is upstream Hush even maintained properly? What is the upstream repository? Is it https://github.com/sheumann/hush? - even if we decide to stay with upstream Hush and just upgrade U-Boot's Hush to upstream (since it supports functions, arithmetic with $((...)), command substitution with $(...), these are all nice features), it is IMO still better than adding a new language - one of the points Sean mentioned with LIL is that when compiled, it's size does not exceed the size of U-Boot's Hush. If we were to add new features into U-Boot's Hush, the code size would certainly increase. I think we should implement these new features, and instead of adding a new language, we should work on minimizing the code size / resulting U-Boot image size. This is where U-Boot will gain most not only with it's CLI, but also everywhere else. Regarding this, - we already have LTO - Simon worked on dtoc so that devicetrees can be compiled into C code - we can start playing with compression - either we can compress the whole image for machines with enough RAM but small place for U-Boot (Nokia N900 for example has only 256 KiB space for U-Boot) - or we can try to invent a way to decompress code when it is needed, for machines with small RAM Marek
Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation
On Tue, 29 Jun 2021 09:41:25 + "Roland Gaudig (OSS)" wrote: > I think just passing the format string directly to sprintf should be > avoided because it is unsafe. For example > > => setexpr foo fmt %s 0x > > would surely lead to access on memory location outside the variable > where 0x is stored. +1. I guess Wolfgang's rationale was that in U-Boot we already have pretty serious means to break the system, so allowing the user to directly pass wrong parameters to sprintf is not that much of a problem since we can say that the user should know what they are doing. But implementing a dedicated format parser for this that is also safe is a simple exercise, imho, so I think we should do this properly, if at all. > > This was actually one of my intentions when making this suggestion - > > to be able to construct any kind of data from pieces; say, for > > example: > > > > => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d > > I think the only way to support such expressions in a save way would > be implementing an own format string parser for setexpr with > corresponding checks if access is possible, instead of just directly > passing all values unchecked to sprintf. We can properly implement %s with field width, justification %c integral types (everything 64-bits, no reason for length modifiers, imho) with field width, precision, zero padding, sign forcing, etc... We don't need floating points nor out of order arguments. Marek
Announcing a tool for Armada 3720 firmware
Hello, this is announcement of mox-imager [1], a firmware uploader / manipulator for Marvell's Armada 3720 SOC. For most of you who use the SOC on boards other than Turris MOX, the most useful feature probably is that it can upload a firmware via UART at higher baudrates than Marvell's original WtpDownloader, which is useful when debugging or during board manufacture. Features that should be interesting for you: - ability to upload firmware over UART at baudrates up to 6 MBaud. This was tested on ESPRESSObin, but should also work on other boards (uDPU, ESPRESSObin Ultra) Uploading at 3 MBaud: $ ./mox-imager -b 300 -D /dev/ttyUSB0 flash-image.bin - ability to upload flash-image.bin firmware over UART When TF-A builds for A3720, it creates image for SPI/eMMC called flash-image.bin, and a directory uart-images containing 3 binaries that you have to use instead of flash-image.bin if you want to boot via UART. mox-imager can work with both methods. When given flash-image.bin, it updates BootFlashSign in the TIM header so that the BootROM will accept the image when given over UART. So both of these work: $ ./mox-imager -D /dev/ttyUSB0 flash-image.bin $ ./mox-imager -D /dev/ttyUSB0 TIM_ATF.bin wtmi_h.bin boot-image_h.bin (This of course does not work if the image needs to be signed cryptographically and the singing key is not present. But AFAIK only Turris MOX uses this feature of the SOC.) - mox-imager has an implementation of a mini-terminal (code from U-Boot's kwboot), which can be used instead of minicom/kermit, once the firmware is uploaded. Although this mini-terminal does not support uploading images over Y-MODEM or other protocols to U-Boot, it can still be useful. If not for anything else, then at least for the ability to not lose any output which the SOC might have sent over UART. (When using minicom/kermit, there is a small window when the /dev/ttyX device is closed and some output is lost.) Use the -t flag to invoke this mini-terminal. Other features that we use on Turris MOX and may be useful to you, but need implementation for your boards: - we use one firmware image for all versions of our board: DDR3 512 MiB, DDR3 1024 MiB, DDR4 This simplifies development of updates significantly. We achieve this by burning board version info into the SOC's OTP. The GPP code contained in the TIM header can read OTP and decide how to initialize clock and DDR registers depending on the values there. The GPP code also works when OTP is empty. In that case it first tries to initialize the registers for DDR3, tests if DDR works, and if not, switches to DDR4. Then it determines RAM size. Note that we do not use this method in production, this was only tested on a few boards that we use for development. (We do not know for example, whether it is safe for the DDR4 chip if we first try to communicate with it in DDR3 mode, since the protocols are different.) You can look at the GPP code at https://gitlab.nic.cz/turris/mox-imager/-/blob/master/gpp/ddr.gpp - mox-imager can create trusted firmware image signed with ECDSA signature and can write the SOC's OTP so that only images signed with a specific ECDSA key will boot. We use this on Turris MOX. Related software: - we have significantly modified Marvell's original WTMI firmware that runs on the Cortex-M3 secure coprocessor. You can look at this modified firmware at our mox-boot-builder repository [2] in directory wtmi. Perhaps the most interesting feature for you is that this firmware exposes the SOC's internal HW RNG via the mailbox to Linux, and that upstream Linux has a driver (turris-mox-rwtm) that registers this random number generator via Linux' crypto API. Another feature our firmware supports is that it can utilize the crypto engine in the secure processor for creating ECDSA signatures. Every Turris MOX has an ECDSA key generated and burned into OTP when manufactured. When done correctly, it is impossible to read this key. It is only possible to use it via the crypto engine for ECDSA signatures. (The firmware has to be in trusted mode for this. If the user has the ability to upload any firmware into their device, they can read the private key from OTP.) This firmware can be built as a standalone replacement of Marvell's WTMI firmware from A3700-utils-marvell repository, but also as an wtmi_app.bin payload application for Marvell's firmware (in this case DDR and clocks are initialized by Marvell's code and HW RNG is provided by wtmi_app.bin). There are instructions on how to use this firmware on ESPRESSObin in mox-boot-builder's README.md. - turris-mox-rwtm driver in Linux [3] communicates with our WTMI firmware (see the point above) and exposes to Linux: - the SOC's HWRNG - the SOC's ECDSA signing engine with singing key stored in OTP. This can be used for authorizing the device, for example via SSH
Re: [PATCH u-boot-marvell] arm: mvebu: turris_{omnia,mox}: ensure running bootcmd_rescue always works
Hi Stefan, Pali discovered this issue with the bootcmd_rescue code for Omnia & MOX. The patch is a therefore a fix. Is is still possible to send this for v2021.07 ? Sorry for the inconvenience. Marek
Re: [PATCH u-boot-marvell v2 5/6] arm: mvebu: dts: turris_mox: add nodes for SPI NOR partitions
On Thu, 10 Jun 2021 16:07:05 +0200 Pali Rohár wrote: > On Thursday 10 June 2021 07:12:53 Stefan Roese wrote: > > Hi Pali, > > > > On 08.06.21 11:51, Pali Rohár wrote: > > > On Monday 07 June 2021 16:34:50 Marek Behún wrote: > > > > Add nodes for SPI NOR partitions to the device tree of Turris MOX, as > > > > are in Linux' device tree. > > > > > > This patch is not needed (for now) as U-Boot cannot parse SPI NOR > > > partitions from DT yet. U-Boot for SPI NOR currently supports specifying > > > partitions only via CONFIG_MTDPARTS_DEFAULT compile option. > > > > But do you agree that this patch "does not hurt"? I'm asking to check, > > if I should include this patch in a potential pull request. > > Yes. This does not hurt and currently does nothing. And after patches > for SPI NOR parsing are merged then this patch can be useful. > > So if you are fine with merging which which currently does noting and in > future is useful then there is no problem with it. Moreover this definition is also in kernel's DTS. I am going to try to work on the U-Boot's A3720 comphy driver so that we can completely align U-Boot's A3720 DTS files with kernel's. Marek
Re: problems with boards with CONFIG_DM disabled
On Wed, 26 May 2021 01:27:56 +0200 Marek Behun wrote: > Tom, Simon, > > now that LTO is merged I am working on > Support SPI NORs and OF partitions in `mtd list` > > but CI fails for some boards, see > https://github.com/u-boot/u-boot/pull/55 > > The reason is that there are still several boards which do not use > CONFIG_DM. > > On the previous version Simon commented that I should use > if (IS_ENABLED(...)) > instead of > #if > but this does not currently work for those boards with CONFIG_DM > disabled (struct udevice's members are not visible at all, and > functions from dm/device.h do not exist). > > There are multiple possible workarounds: > - use #if (until all boards are at CONFIG_DM) > - create static inline versions of functions from dm/device.h returning > failures when CONFIG_DM is not set (this would be rather big :( ) > - wait till all those boards with CONFIG_DM disabled are removed > - ... Since there is rather a large number of defconfigs with CONFIG_DM disabled, and since the relevant code was rather complex if (!is_part && dev && mtd->dev == dev) || !strcmp(name, mtd->name) || (is_part && mtd->dev && !strcmp(name, mtd->dev->name)) I moved the code into a separate name matching function and for now created a non-DM version. Hopefully this will be acceptable and pass CI. Marek
problems with boards with CONFIG_DM disabled
Tom, Simon, now that LTO is merged I am working on Support SPI NORs and OF partitions in `mtd list` but CI fails for some boards, see https://github.com/u-boot/u-boot/pull/55 The reason is that there are still several boards which do not use CONFIG_DM. On the previous version Simon commented that I should use if (IS_ENABLED(...)) instead of #if but this does not currently work for those boards with CONFIG_DM disabled (struct udevice's members are not visible at all, and functions from dm/device.h do not exist). There are multiple possible workarounds: - use #if (until all boards are at CONFIG_DM) - create static inline versions of functions from dm/device.h returning failures when CONFIG_DM is not set (this would be rather big :( ) - wait till all those boards with CONFIG_DM disabled are removed - ... What should I do? Thanks, Marek
Re: [PATCH u-boot v4 36/36] ARM: enable LTO for some boards
On Mon, 24 May 2021 13:44:38 -0400 Tom Rini wrote: > On Mon, May 24, 2021 at 01:09:19PM -0400, Tom Rini wrote: > > On Mon, May 24, 2021 at 05:58:55PM +0200, Marek Behun wrote: > > > On Mon, 24 May 2021 11:40:53 -0400 > > > Tom Rini wrote: > > > > > > > On Fri, May 21, 2021 at 12:56:41PM -0400, Tom Rini wrote: > > > > > On Fri, May 21, 2021 at 06:00:31PM +0200, Marek Behún wrote: > > > > > > On Fri, 21 May 2021 10:11:47 -0400 > > > > > > Tom Rini wrote: > > > > > > > > > > > > > On Thu, May 20, 2021 at 01:56:29PM -0500, Adam Ford wrote: > > > > > > > > On Thu, May 20, 2021 at 6:25 AM Marek Behún > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Enable LTO for some boards that were tested by people on > > > > > > > > > U-Boot > > > > > > > > > Mailing List. > > > > > > > > > > > > > > > > > > Signed-off-by: Marek Behún > > > > > > > > > Tested-by: Adam Ford > > > > > > > > > Tested-by: Pali Rohár > > > > > > > > > Tested-by: Tim Harvey > > > > > > > > > > > > > > > > Since the imx8mm beacon boards and the imx8mm venice board both > > > > > > > > show > > > > > > > > promise, does it make sense to 'imply' the LTO for anything > > > > > > > > enabling > > > > > > > > imx8mm? > > > > > > > > Same thing for the various omap3 boards, and potentially the > > > > > > > > renesas > > > > > > > > RZ/G2 boards. I know Tom went through to remove a bunch of > > > > > > > > boards > > > > > > > > that were never converted to DM. Most of the boards remaining > > > > > > > > boards have minimal board files and most of code is common to > > > > > > > > other > > > > > > > > boards in the same platforms. > > > > > > > > > > > > > > > > I have an l138_lcdk that I can use to test which I expect to be > > > > > > > > similar to the da850evm. > > > > > > > > > > > > > > As much as I am eager to move everything, quickly, over to LTO by > > > > > > > default, I think the problems that we've seen thus far show it's > > > > > > > best > > > > > > > to really make it an explicit enable per board at least for the > > > > > > > first > > > > > > > release or two. Once we've hopefully gotten more boards tested > > > > > > > and > > > > > > > enabled we can see what makes sense for defaults, give a release > > > > > > > worth > > > > > > > of heads up, and then go. > > > > > > > > > > > > Tom, are there some other issues aside from the one failing CI > > > > > > scenario > > > > > > (sandbox_clang)? Would you be willing to merge this if I resolved > > > > > > that > > > > > > one fail by disabling LTO for that scenario (until I resolve it)? It > > > > > > would help me not having to maintain all 30+ patches... > > > > > > > > > > Yeah, CI needs to keep passing, so if we need to disable > > > > > sandbox+clang+lto for now, OK. > > > > > > > > Ah, I see the problem now. I've worked out a fix after looking at the > > > > Linux kernel a bit and I'll post something for us and upstream dtc as > > > > well. > > > > > > > > > > What do you mean? The problem is in dtc? I see 2 problems: > > > - one with DM test > > > - one with stack protector test > > > > I don't have a full answer about the stack protector test just yet, but > > it almost seems like it's too simple and maybe something is happening > > with it being optimized to not a problem? > > Yeah, so clang with LTO optimizes away that memset call, and so the test > passes. I'll do something to make sure the array is used so it won't be > optimized away. > I am unable to make the compiler to protect the stack of that function even with GCC on my local machine. It seems that at least on my gentoo with gcc-10.2, when compiling with -ffreestanding, the call to __stack_chk_fail is not made at all. I even started reading sources of gcc on thursday because of this, but it didn't lead anywhere... When you compile sandbox_defconfig with gcc, does the test pass on your local machine? Marek
Re: [PATCH u-boot v4 36/36] ARM: enable LTO for some boards
On Mon, 24 May 2021 11:40:53 -0400 Tom Rini wrote: > On Fri, May 21, 2021 at 12:56:41PM -0400, Tom Rini wrote: > > On Fri, May 21, 2021 at 06:00:31PM +0200, Marek Behún wrote: > > > On Fri, 21 May 2021 10:11:47 -0400 > > > Tom Rini wrote: > > > > > > > On Thu, May 20, 2021 at 01:56:29PM -0500, Adam Ford wrote: > > > > > On Thu, May 20, 2021 at 6:25 AM Marek Behún > > > > > wrote: > > > > > > > > > > > > Enable LTO for some boards that were tested by people on U-Boot > > > > > > Mailing List. > > > > > > > > > > > > Signed-off-by: Marek Behún > > > > > > Tested-by: Adam Ford > > > > > > Tested-by: Pali Rohár > > > > > > Tested-by: Tim Harvey > > > > > > > > > > Since the imx8mm beacon boards and the imx8mm venice board both show > > > > > promise, does it make sense to 'imply' the LTO for anything enabling > > > > > imx8mm? > > > > > Same thing for the various omap3 boards, and potentially the renesas > > > > > RZ/G2 boards. I know Tom went through to remove a bunch of boards > > > > > that were never converted to DM. Most of the boards remaining > > > > > boards have minimal board files and most of code is common to other > > > > > boards in the same platforms. > > > > > > > > > > I have an l138_lcdk that I can use to test which I expect to be > > > > > similar to the da850evm. > > > > > > > > As much as I am eager to move everything, quickly, over to LTO by > > > > default, I think the problems that we've seen thus far show it's best > > > > to really make it an explicit enable per board at least for the first > > > > release or two. Once we've hopefully gotten more boards tested and > > > > enabled we can see what makes sense for defaults, give a release worth > > > > of heads up, and then go. > > > > > > Tom, are there some other issues aside from the one failing CI scenario > > > (sandbox_clang)? Would you be willing to merge this if I resolved that > > > one fail by disabling LTO for that scenario (until I resolve it)? It > > > would help me not having to maintain all 30+ patches... > > > > Yeah, CI needs to keep passing, so if we need to disable > > sandbox+clang+lto for now, OK. > > Ah, I see the problem now. I've worked out a fix after looking at the > Linux kernel a bit and I'll post something for us and upstream dtc as > well. > What do you mean? The problem is in dtc? I see 2 problems: - one with DM test - one with stack protector test
Re: [PATCH u-boot v4 01/36] regmap: fix a serious pointer casting bug
> I don't see a changelog here but this is v4. Are you using patman? Changelog is in cover letter. Unfortunately I am not using patman yet. Marek
Re: [PATCH] fs: btrfs: Add missing cache aligned allocation
On Tue, 18 May 2021 00:39:39 +0200 Marek Vasut wrote: > The superblock buffer must be cache aligned, since it might be used > in DMA context, allocate it using ALLOC_CACHE_ALIGN_BUFFER() just > like it was done in btrfs_read_superblock() and read_tree_node(). > > This fixes this output on boot and non-working btrfs on iMX53: > CACHE: Misaligned operation at range [ced299d0, ced2a9d0] > > Signed-off-by: Marek Vasut > Cc: Marek Behún > Cc: Qu Wenruo > --- Reviewed-by: Marek Behún
Re: [RFC PATCH u-boot 00/12] U-Boot LTO (Sandbox + ARM Nokia RX-51)
On Sun, 9 May 2021 09:14:14 -0500 Adam Ford wrote: > On Sat, Mar 6, 2021 at 10:06 PM Marek Behun wrote: > > > > On Sat, 6 Mar 2021 21:45:02 -0600 > > Adam Ford wrote: > > > > > On Sat, Mar 6, 2021 at 3:49 PM Marek Behun wrote: > > > > > > > > On Sat, 6 Mar 2021 22:38:52 +0100 > > > > Pali Rohár wrote: > > > > > > > > > On Saturday 06 March 2021 22:19:22 Marek Behun wrote: > > > > > > On Sat, 6 Mar 2021 22:00:45 +0100 > > > > > > Pali Rohár wrote: > > > > > > > > > > > > > On Saturday 06 March 2021 21:54:00 Marek Behun wrote: > > > > > > > > On Sat, 6 Mar 2021 21:41:14 +0100 > > > > > > > > Pali Rohár wrote: > > > > > > > > > > > > > > > > > On Saturday 06 March 2021 15:08:13 Tom Rini wrote: > > > > > > > > > > Perhaps we'll default to yes on some SoCs. The omap3 thing > > > > > > > > > > is a bit > > > > > > > > > > odd, but we'll see what happens on real N900 hardware. > > > > > > > > > > > > > > > > > > Hello! > > > > > > > > > > > > > > > > > > Could you send me a link to git repo / branch and tell me > > > > > > > > > from which > > > > > > > > > commit should I do tests on real N900 hardware? I will test > > > > > > > > > it and let > > > > > > > > > you know results. > > > > > > > > > > > > > > > > > > Adding maemo ML to the loop as on the maemo list are more > > > > > > > > > people with > > > > > > > > > N900 HW and U-Boot. > > > > > > > > > > > > > > > > https://github.com/elkablo/u-boot branch lto > > > > > > > > > > > > > > Sorry, compilation is failing :-( > > > > > > > > > > > > > > $ git clone https://github.com/elkablo/u-boot -b lto --depth=100 > > > > > > > Cloning into 'u-boot'... > > > > > > > remote: Enumerating objects: 33644, done. > > > > > > > remote: Counting objects: 100% (33644/33644), done. > > > > > > > remote: Compressing objects: 100% (20116/20116), done. > > > > > > > remote: Total 33644 (delta 15838), reused 19947 (delta 13018), > > > > > > > pack-reused 0 > > > > > > > Receiving objects: 100% (33644/33644), 26.28 MiB | 10.21 MiB/s, > > > > > > > done. > > > > > > > Resolving deltas: 100% (15838/15838), done. > > > > > > > > > > > > > > $ cd u-boot > > > > > > > > > > > > > > $ make CROSS_COMPILE=arm-linux-gnueabi- nokia_rx51_config > > > > > > > HOSTCC scripts/basic/fixdep > > > > > > > HOSTCC scripts/kconfig/conf.o > > > > > > > YACCscripts/kconfig/zconf.tab.c > > > > > > > LEX scripts/kconfig/zconf.lex.c > > > > > > > HOSTCC scripts/kconfig/zconf.tab.o > > > > > > > HOSTLD scripts/kconfig/conf > > > > > > > # > > > > > > > # configuration written to .config > > > > > > > # > > > > > > > > > > > > > > $ make CROSS_COMPILE=arm-linux-gnueabi- u-boot.bin > > > > > > > ... > > > > > > > LTO u-boot > > > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > > > > > > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > > > DWARF error: offset (1258291444) greater than or equal to > > > > > > > .debug_str size (676) > > > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > > > DWARF error: offset (1459618036) greater than or equal to > > > > > > > .debug_str size (676) > > > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > > > DWARF error: could not find abbrev number 48028
Re: [PATCH] cmd: mvebu: Rename rx_training to mvebu_comphy_rx_training
On Wed, 5 May 2021 09:15:10 +0200 Stefan Roese wrote: > Rename the misleading cmd "rx_training" to "mvebu_comphy_rx_training" to > avoid confusion and mixup with DDR3/4 training. This makes it clear, > that this command is platform specific and handles the COMPHY RX > training. > > Also depend this cmd on ARMADA_8K and not TARGET_MVEBU_ARMADA_8K to make > is available for OcteonTX2 CN913x. Acked-by: Marek Behún
Re: [PATCH v1 00/23] phy: marvell: Sync Armada 3k/7k/8k SERDES code with Marvell version
On Wed, 5 May 2021 11:19:13 +0200 Pali Rohár wrote: > > "bubt" is special and cannot be changed easily without breaking update > > scripts using it AFAICT. As it's pretty old and used in the Marvell code > > base for quite some time - including all the documentation about > > updating. > > I see. This needs to say in current form. bubt can theoretically be implemented for other platforms. Its purpose is to update u-boot. This is useful for other platforms. Marek
Re: [PATCH 04/49] btrfs: Use U-Boot API for decompression
On Mon, 3 May 2021 17:10:51 -0600 Simon Glass wrote: > Use the common function to avoid code duplication. > > Signed-off-by: Simon Glass Is this tested? Why only zstd? marek
Re: [PATCH v1 00/23] phy: marvell: Sync Armada 3k/7k/8k SERDES code with Marvell version
On Thu, 29 Apr 2021 08:46:36 +0200 Stefan Roese wrote: > >phy: marvell: add RX training command > > Applied to u-boot-marvell/master > > Thanks, > Stefan Stefan, do you think it would make sense to at least create a special mechanism for these platform commands? For example via a top-level command "plat", i.e. instead of > rx_training params we would call > plat rx_training params The plat command could list all registered platform specific commands... Marek
Re: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes
On Thu, 8 Apr 2021 19:18:09 +
Stefan Chulski wrote:
> >
> > Stefan, you suggest to drop this define from PHY_INTERFACE enum which we
> > can't easily do with other drivers (like NXP) also referencing this macro.
> >
> > How to continue then?
> >
> > Thanks,
> > Stefan
>
> Probably we should drop SGMII_2500 from this series, introduce "manage"
> devicetree property(that would indicate if inband supported or not).
> For example this is armada-8040-mcbin.dtsi In kernel, when in-band supported:
> _eth2 {
> /* CPS Lane 5 */
> status = "okay";
> /* Network PHY */
> phy-mode = "2500base-x";
> managed = "in-band-status";
> /* Generic PHY, providing serdes lanes */
> phys = <_comphy5 2>;
> sfp = <_eth3>;
> };
>
> If in-band not supported(for example PPv2 MAC connected to 88E2110 in 2.5G
> speed) we would use default managed = "auto" and fixed link property.
Such DTS properties should first be proposed to device-tree ML
and documented in devicetree bindings documentation.
Marek
Re: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes
On Thu, 8 Apr 2021 10:24:22 +0200
Stefan Roese wrote:
> Hi Stefan,
> Hi Marek,
>
> On 25.03.21 13:59, Stefan Chulski wrote:
> >> Could you please ask internally at Marvell?
> >> We are trying to get to the bottom of this because we are stuck in
> >> development of code for Amethyst. We need to get 2500base-x to work, we
> >> need to know whether it does or does not support AN, or maybe does but
> >> only for some devices. Otherwise it may happen that some SFPs won't link
> >> with our hardware.
> >>
> >> Thank you.
> >>
> >> Marek
> >
> > To avoid confusions, I suggest we take this issue directly with Marvell
> > SoHo switch FAE. I'm willing to start another thread to discuss this(I
> > will check who can assist you).
> >
> > Regarding to this patch series. Let's drop PHY_INTERFACE_MODE_SGMII_2500,
> > from PHY_INTERFACE enum and we would handle SGMII mode in 2.5G speed
> > differently in PPv2 driver.
>
> I'm just now getting back to this patch.
>
> JFYI: PHY_INTERFACE_MODE_SGMII_2500 is used in NXP ethernet drivers as
> well:
>
> $ git grep PHY_INTERFACE_MODE_SGMII_2500
> board/freescale/ls1012aqds/eth.c:
> PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1012aqds/eth.c:
> PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1012ardb/eth.c:
> PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1012ardb/eth.c:
> PHY_INTERFACE_MODE_SGMII_2500);
> board/freescale/ls1043aqds/eth.c:
> PHY_INTERFACE_MODE_SGMII_2500) {
> board/freescale/ls1043aqds/eth.c: case
> PHY_INTERFACE_MODE_SGMII_2500:
> board/freescale/ls1043aqds/eth.c: } else if
> (interface == PHY_INTERFACE_MODE_SGMII_2500) {
> board/freescale/ls1046aqds/eth.c: } else if
> (fm_info_get_enet_if(port) == PHY_INTERFACE_MODE_SGMII_2500) {
> board/freescale/t102xrdb/eth_t102xrdb.c:case
> PHY_INTERFACE_MODE_SGMII_2500:
> board/freescale/t102xrdb/eth_t102xrdb.c:if
> (((fm_info_get_enet_if(port) == PHY_INTERFACE_MODE_SGMII_2500) ||
> drivers/net/fm/eth.c: PHY_INTERFACE_MODE_SGMII_2500) ?
> true : false;
> drivers/net/fm/eth.c: fm_eth->enet_if ==
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/fm/eth.c:(fm_eth->enet_if ==
> PHY_INTERFACE_MODE_SGMII_2500) ||
> drivers/net/fm/eth.c: if (fm_eth->enet_if ==
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/fm/eth.c: case PHY_INTERFACE_MODE_SGMII_2500:
> drivers/net/fm/ls1043.c:return
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fm/ls1043.c:return
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fm/ls1046.c:return
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fm/memac.c: case PHY_INTERFACE_MODE_SGMII_2500:
> drivers/net/fm/t1024.c: return
> PHY_INTERFACE_MODE_SGMII_2500;
> drivers/net/fsl_enetc.c:if (priv->if_type ==
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/fsl_enetc.c:case PHY_INTERFACE_MODE_SGMII_2500:
> drivers/net/mscc_eswitch/felix_switch.c:phy->interface ==
> PHY_INTERFACE_MODE_SGMII_2500)
> drivers/net/mscc_eswitch/felix_switch.c:case
> PHY_INTERFACE_MODE_SGMII_2500:
> ...
>
> Stefan, you suggest to drop this define from PHY_INTERFACE enum which
> we can't easily do with other drivers (like NXP) also referencing this
> macro.
>
> How to continue then?
I think NXP also uses this macro incorrectly, they should instead use
2500BASEX. SGMII_2500 does not exist in Linux. We can look at the
corresponding NXP driver in Linux (if it exists) to see if the
corresponging mode is 2500BASEX.
Marek
Re: regmap bug fix
On Thu, 25 Mar 2021 15:41:55 +1300
Simon Glass wrote:
> eHi Marek,
>
> On Thu, 25 Mar 2021 at 13:46, Marek Behun wrote:
> >
> > On Thu, 25 Mar 2021 13:38:13 +1300
> > Simon Glass wrote:
> >
> > > Hi Marek,
> > >
> > > On Wed, 17 Mar 2021 at 04:19, Marek Behun wrote:
> > > >
> > > > Simon, Heiko, Bin,
> > > >
> > > > Pratyush discovered that the solution implemented by the patch
> > > > regmap: fix a serious pointer casting bug
> > > > is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct
> > > > position, but on big endian machines it also reverses byte order.
> > > > Somehow this went right through my head when I thought this up.
> > > >
> > > > I have sent a new version, with subject
> > > > [PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug
> > > >
> > > > The new solution utilizes an union { u8; u16; u32; u64; }, since all
> > > > members of an union start at the same address.
> > > >
> > > > Could you please review this? Thanks.
> > >
> > > You already have my review tag on that.
> > >
> > > I dropped it from u-boot-dm/next. The problem was that it v1 was in my
> > > queue, but not later versions.
> > >
> > > Regards,
> > > Simon
> >
> > I don't have your review for version v3.1 :) to which you just replied.
> >
> > I had your review tag for v3, and sent v3.1 as a reply to
> > Pratyush'scomplains on v3.
>
> Fractional versions? In that case I'm going to hold out for version 3.125.
:) I sent it as a reply for v3, and did not want to call it v4
because for v4 I am going to send the whole series again. Sorry.
Anyway I probably will send v4 next week, there are still some problems
and I am working on something different now.
Marek
Re: regmap bug fix
On Thu, 25 Mar 2021 13:38:13 +1300
Simon Glass wrote:
> Hi Marek,
>
> On Wed, 17 Mar 2021 at 04:19, Marek Behun wrote:
> >
> > Simon, Heiko, Bin,
> >
> > Pratyush discovered that the solution implemented by the patch
> > regmap: fix a serious pointer casting bug
> > is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct
> > position, but on big endian machines it also reverses byte order.
> > Somehow this went right through my head when I thought this up.
> >
> > I have sent a new version, with subject
> > [PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug
> >
> > The new solution utilizes an union { u8; u16; u32; u64; }, since all
> > members of an union start at the same address.
> >
> > Could you please review this? Thanks.
>
> You already have my review tag on that.
>
> I dropped it from u-boot-dm/next. The problem was that it v1 was in my
> queue, but not later versions.
>
> Regards,
> Simon
I don't have your review for version v3.1 :) to which you just replied.
I had your review tag for v3, and sent v3.1 as a reply to
Pratyush'scomplains on v3.
Marek
Re: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes
On Wed, 24 Mar 2021 16:36:10 + Stefan Chulski wrote: > > > > > > SGMII uses the same coding as 1000base-x, but the latter works > > > > > > only with one speed (1000mbps), while the former can also work > > > > > > in 10mbps and 100mbps (by repeating each byte 100 or 10 times, > > respectively). > > > > > > > > > > > > Then there is 2500base-x, which is the same as 1000base-x, but > > > > > > with the clock being at 2.5x the speed of 1000base-x clock. > > > > > > > > > > > > But there is no analogue of the SGMII protocol (i.e. the > > > > > > repearing of bytes in order to achieve lower speed) for the > > > > > > 2500base- > > x. > > > > > > > > > > > > So what I am confused about here is what is supposed to be the > > > > > > difference between > > > > > > PHY_INTERFACE_MODE_SGMII_2500 > > > > > > and > > > > > > PHY_INTERFACE_MODE_2500BASEX > > > > > > ? > > > > > > > > > > > > Marek > > > > > > > > I not sure what is correct naming for these mode. PHY_INTERFACE > > > > includes both MAC2PHY interfaces(MII, RGMII and etc), PHY2PHY > > > > interfaces(like > > > > BASEX) and SGMII(which is kind of both). > > > > For both 2500BASEX and SGMII_2500 Serdes lanes set to HS-SGMII in > > > > 3.125G speed, but MAC configured differently and autoneg cannot be > > supported. > > > > > > > > Regards, > > > > Stefan. > > > > > > Since we already has PHY_INTERFACE_MODE_SGMII and > > > PHY_INTERFACE_MODE_QSGMII (5G mode), maybe we should call > > PHY_INTERFACE_MODE_HSGMII - High-Serial Gigabit Media Independent > > Interface (HSGMII, 3.125Gbps). > > > > And we have now autonegotiation in this discussion. Just today I sent a > > question to Marvell about 2500base-x and why inband autonegotiation is not > > supported there, while it is for 1000base-x. > > To clear thing out, inband autonegotiation is negotiation of pause, speed and > duplex over PCS(usually between MAC and PHY). > > > So are you saying that 2500base-x mode is like 1000base-x, but at 2.5g > > speed, > > and without inband autonegotiation? > > From Serdes configurations point of view - lanes configured to 3_125G in > 2500base-x and to 1_25G in 1000base-x. > MAC doesn't support inband autonegotiation in base-x mode. Hi Stefan, This is where you are wrong. MAC _does_ support inband autonegotiation in base-x mode, specifically in 1000base-x mode (on the SerDes connection between MAC and PHY). This is supported by Linux' mvneta driver and also by Even Marvell's documentation for various PHYs says that inband autonegotiation is supported for 1000base-x mode. Look for example at document MV-S111371-00 Rev E Marvell Alaska M 88E2181/88E2180/88E2111/88E2110 section 1.3.3 "H Unit 1000BASE-X/2500BASE-X Register Description" There is register 4.0x2000 "1000BASE-X/SGMII Control Register" bit 12 of this register is called "1000BASE-X Auto-Negotiation Enable" And then look for example at datasheet for 88E2110 (doc MV-S111947-00 Rev A) section 5.2.2 "2500BASE-X". It says: 2500BASE-X is identical to 1000GBASE-X operation except at 2.5 times speed. Auto-Negotiation is not supported in 2500BASE-X. Then look at section 5.2.3.2, which says SGMII uses 1000BASE-X coding to send data as well as Auto-Negotiation information between the PHY and the MAC. However, the contents of the SGMII Auto-Negotiation are different than the 1000BASE-X Auto-Negotiation. So clearly there is AN for 1000base-x, but not for 2500base-x. So I think there is still misunderstanding. I am going to explain to you how I understand the whole thing. Note that all this time we are talking about auto-negotiation on the link between MAC and PHY (i.e. how mvneta driver connects to a PHY, for example). - 1000base-x PCS is a SerDes protocol using 8b/10b encoding, and is capable of auto-negotiation. It allows to auto-negotiate flow-control (Pauses) and duplexicity (but is always full-duplex) - sgmii extends this AN to also support speed autonegotiation - you do not need to configure AN in SerDes code - no such thing is done by the comphy driver, only by the MAC driver for the MAC and PHY drivers for the PHY - MAC can negotiate pause with the PHY in 1000base-x mode (mvneta ethernet driver + marvell10g PHY driver in kernel could do this, for example) - 2500base-x PCS is the same as 1000base-x, i.e. a SerDes protocol using 8b/10b encoding, via which you can again connect a MAC with a PHY, or a PHY with another PHY, or even a MAC with another MAC. It's only difference from 1000base-x should be clock speed: it runs at 2.5x the speed of 1000base-x But for some reason, Marvell documentation for various PHYs says that AN is _NOT_ supported in this mode (although it is in 1000base-x). Reality is that Marvell's Armada 3720 MAC (mvneta driver) can enable AN in 2500base-x mode, and SerDes PHY in the Marvell 88E6141 Switch (Topaz) can also enable AN in 2500base-x mode, and they will link correctly, and the registers will look as
Re: [PATCH v1 08/23] phy: marvell: add RX training command
On Wed, 24 Mar 2021 15:06:34 +0100 Stefan Roese wrote: > From: Igal Liberman > > This patch adds support for running RX training using new command called > "rx_training" > Usage: > rx_training - rx_training > > RX training allows to improve link quality (for SFI mode) > by running training sequence between us and the link partner, > this allows to reach better link quality then using static configuration. PLEASE do not add another vendor specific command. PLEASE ! Create a generic command, with name 'ddr' or something, with API and documentation. Also the name "rx_training" is the worst thing ever. It could be interpreted as training for RX on ethernet PHY, or even on UART. Maybe people from Marvell could work more on their cooperation with community. It is not terrible, but it is bad. There are hundreds if not thousand of patches in their SDKs for both U-Boot and Linux. It is a nightmare. Marek
Re: [EXT] Re: [PATCH v1 02/23] phy: marvell: rename comphy related definitions to COMPHY_XX
Please be also aware the we have the following patch U-Boot
commit 545591132aa701ff1262bb309fbcd0c3ff0acd75
Author: Marek Behún
Date: Wed Aug 19 11:57:25 2020 +0200
arm64: dts: armada-3720-espressobin: fix COMPHY nodes
This commit fixes initialization of COMPHY on EspressoBin.
Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
The lanes are defined in comphy_a3700.c as described in functional
specification, that is:
lane 0 is SGMII1 or USB3
lane 1 is PCIe or SGMII0
lane 2 is SATA or USB3
But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
lane 1, which is wrong in the sense of the specification and doesn't
work with the comphy_mux code, which is 2 years now (the aardvark driver
causes synchronous abort in U-Boot).
It worked till the above mentioned commit, because the code for powering
up PCIe PHY doesn't work with lane number at all, and the code for
powering up USB3 PHY works differently only if USB3 is on lane 2, ie.
the check goes like:
if (lane == 2)
something
else
something else
so it does not differentiate between lanes 0 and 1.
In the future I shall post patches that remove the comphy_a3700 driver
and add comphy driver which uses calls to ATF, like Linux' driver does.
This will have the advantage of same DTS bindings as Linux', but till
this is done, we need this patch.
Signed-off-by: Marek Behún
Tested-by: Pali Rohár
Cc: Stefan Roese
Reviewed-by: Stefan Roese
Tested-by: Andre Heider
diff --git a/arch/arm/dts/armada-3720-espressobin.dts
b/arch/arm/dts/armada-3720-espressobin.dts
index 84e2c2adba..50381e979e 100644
--- a/arch/arm/dts/armada-3720-espressobin.dts
+++ b/arch/arm/dts/armada-3720-espressobin.dts
@@ -72,13 +72,13 @@
{
max-lanes = <3>;
phy0 {
- phy-type = ;
- phy-speed = ;
+ phy-type = ;
+ phy-speed = ;
};
phy1 {
- phy-type = ;
- phy-speed = ;
+ phy-type = ;
+ phy-speed = ;
};
phy2 {
Re: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes
On Wed, 24 Mar 2021 13:09:00 + Stefan Chulski wrote: > > > > > > > > SGMII uses the same coding as 1000base-x, but the latter works only > > > > with one speed (1000mbps), while the former can also work in 10mbps > > > > and 100mbps (by repeating each byte 100 or 10 times, respectively). > > > > > > > > Then there is 2500base-x, which is the same as 1000base-x, but with > > > > the clock being at 2.5x the speed of 1000base-x clock. > > > > > > > > But there is no analogue of the SGMII protocol (i.e. the repearing > > > > of bytes in order to achieve lower speed) for the 2500base-x. > > > > > > > > So what I am confused about here is what is supposed to be the > > > > difference between > > > > PHY_INTERFACE_MODE_SGMII_2500 > > > > and > > > > PHY_INTERFACE_MODE_2500BASEX > > > > ? > > > > > > > > Marek > > > > I not sure what is correct naming for these mode. PHY_INTERFACE includes > > both MAC2PHY interfaces(MII, RGMII and etc), PHY2PHY interfaces(like > > BASEX) and SGMII(which is kind of both). > > For both 2500BASEX and SGMII_2500 Serdes lanes set to HS-SGMII in 3.125G > > speed, but MAC configured differently and autoneg cannot be supported. > > > > Regards, > > Stefan. > > Since we already has PHY_INTERFACE_MODE_SGMII and PHY_INTERFACE_MODE_QSGMII > (5G mode), maybe we should call > PHY_INTERFACE_MODE_HSGMII - High-Serial Gigabit Media Independent Interface > (HSGMII, 3.125Gbps). And we have now autonegotiation in this discussion. Just today I sent a question to Marvell about 2500base-x and why inband autonegotiation is not supported there, while it is for 1000base-x. So are you saying that 2500base-x mode is like 1000base-x, but at 2.5g speed, and without inband autonegotiation? And meanwhile HS-SGMII mode is like SGMII, but at 2.5g speed, and _WITH_ autonegotiation? And what does this autonegotiation support? Does is support only negotiation of Pause? Or does it support negotiation of duplexicity and speed as well? Thank you. Marek
Re: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes
On Wed, 24 Mar 2021 09:55:04 + Kostya Porotchkin wrote: > Hi, Marek, > > > -Original Message- > > From: Marek Behun > > Sent: Wednesday, March 24, 2021 12:44 > > To: Stefan Roese > > Cc: u-boot@lists.denx.de; Kostya Porotchkin ; Stefan > > Chulski ; Ramon Fried ; > > Nadav Haklai ; Joe Hershberger > > ; Marcin Wojtas ; sa_ip-sw- > > jenkins ; Igal Liberman ; > > Simon Glass ; Yan Markman > > Subject: [EXT] Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use > > both 2.5GHz modes > > > > External Email > > > > -- > > On Wed, 24 Mar 2021 10:20:05 +0100 > > Stefan Roese wrote: > > > > > PHY_INTERFACE_MODE_SGMII_2500 > > > > What the hell is this mode??? > > > > AFAIK something like this does not actually exist. > [KP] I think you are wrong. These modes are definitely exist > https://en.wikipedia.org/wiki/2.5GBASE-T_and_5GBASE-T > > Regards > Kosta Hi Kosta, the wikipedia page you linked specifies copper modes, not PHY modes. We are discussing PHY modes here. What I am confused about is that this patch adds check for PHY_INTERFACE_MODE_SGMII_2500 in addition to PHY_INTERFACE_MODE_2500BASEX But what is the difference between these two? Marvell named this protocol HS-SGMII in some of their datasheets and code. I guess this was done because of the similarities with 1000base-x and SGMII. Marvell uses the names SGMII and 1000base-x interchangably, although this is not correct. I guess they are similarily using names 2500base-x and HS-SGMII (and now SGMII_2500) interchangably, which is also not correct. SGMII uses the same coding as 1000base-x, but the latter works only with one speed (1000mbps), while the former can also work in 10mbps and 100mbps (by repeating each byte 100 or 10 times, respectively). Then there is 2500base-x, which is the same as 1000base-x, but with the clock being at 2.5x the speed of 1000base-x clock. But there is no analogue of the SGMII protocol (i.e. the repearing of bytes in order to achieve lower speed) for the 2500base-x. So what I am confused about here is what is supposed to be the difference between PHY_INTERFACE_MODE_SGMII_2500 and PHY_INTERFACE_MODE_2500BASEX ? Marek
Re: [PATCH v1 2/5] net: phy: marvell: extend 88E2110 to use both 2.5GHz modes
On Wed, 24 Mar 2021 10:20:05 +0100 Stefan Roese wrote: > PHY_INTERFACE_MODE_SGMII_2500 What the hell is this mode??? AFAIK something like this does not actually exist.
Re: [PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug
On Tue, 16 Mar 2021 22:04:17 +0530
Pratyush Yadav wrote:
> > + switch (map->width) {
> > + case REGMAP_SIZE_8:
> > + *valp = u.v8;
> > + break;
> > + case REGMAP_SIZE_16:
> > + *valp = u.v16;
> > + break;
> > + case REGMAP_SIZE_32:
> > + *valp = u.v32;
> > + break;
> > + case REGMAP_SIZE_64:
> > + *valp = u.v64;
> > + break;
>
> I think this should fix the problem you are trying to solve.
>
> But I see another problem with this code. What if someone wants to read
> 8 bytes? IIUC, since valp points to a uint, *valp = u.v64 will result in
> 4 bytes being truncated from the result.
>
> I see two options:
>
> - Change the uint pointer to u64 pointer and update every driver to use
> a u64 when using the regmap.
>
> - Change the uint pointer to void pointer and expect every driver to
> pass a container with exactly the required size, based on map->width.
> Update the ones that don't follow this.
>
> I prefer the latter option.
If only these two options are possible, then the first option is
better. The regmap_read function ought to be simple, so no void *
pointer.
There is another option: if a value greater than ULONG_MAX is read, the
regmap_read() function will return -ERANGE. This way this function
will remain simple. ulong is 64-bit wide on 64-bit devices, so this
only is a problem when a 64-bit wide regmap is used on a 32-bit device.
I think we should keep regmap_read() simple, and for people who use a
64-bit wide regmap on 32-bit device, there is regmap_raw_read().
But I think this problem should be solved (however we decide) in a
follow-up patch. This patch fixes the pointer casting bug, and commits
should remain atomic (fixing one thing).
Marek
Re: [PATCH u-boot v3 01/39] regmap: fix a serious pointer casting bug
On Tue, 16 Mar 2021 20:59:55 +0530
Pratyush Yadav wrote:
> On 16/03/21 03:15PM, Marek Behun wrote:
> > On Tue, 16 Mar 2021 19:28:46 +0530
> > Pratyush Yadav wrote:
> >
> > > On 16/03/21 01:25PM, Marek Behún wrote:
> > > > There is a serious bug in regmap_read() and regmap_write() functions
> > > > where an uint pointer is cast to (void *) which is then cast to (u8 *),
> > > > (u16 *), (u32 *) or (u64 *), depending on register width of the map.
> > > >
> > > > For example given a regmap with 16-bit register width the code
> > > > int val = 0x1234;
> > > > regmap_read(map, 0, );
> > > > only changes the lower 16 bits of val on little-endian machines.
> > > > The upper 16 bits will remain 0x1234.
> > > >
> > > > Nobody noticed this probably because this bug can be triggered with
> > > > regmap_write() only on big-endian architectures (which are not used by
> > > > many people anymore), and on little endian this bug has consequences
> > > > only if register width is 8 or 16 bits and also the memory place to
> > > > which regmap_read() should store it's result has non-zero upper bits,
> > > > which it seems doesn't happen anywhere in U-Boot normally. CI managed to
> > > > trigger this bug in unit test of dm_test_devm_regmap_field when compiled
> > > > for sandbox_defconfig using LTO.
> > > >
> > > > Fix this simply by taking into account that regmap_raw_read() and
> > > > regmap_raw_write() behave as if the data given to these functions were
> > > > in little-endian format, i.e. use cpu_to_le32() / le32_to_cpu(). In
> > > > regmap_read() also zero out the space so that we don't get invalid
> > > > result if regmap_raw_read() does not fill the whole object.
> > > >
> > > > Signed-off-by: Marek Behún
> > > > Reviewed-by: Simon Glass
> > > > Reviewed-by: Heiko Schocher
> > > > Reviewed-by: Bin Meng
> > > > ---
> > > > drivers/core/regmap.c | 13 -
> > > > 1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > > > index b51ce108c1..5d37006fff 100644
> > > > --- a/drivers/core/regmap.c
> > > > +++ b/drivers/core/regmap.c
> > > > @@ -435,7 +435,16 @@ int regmap_raw_read(struct regmap *map, uint
> > > > offset, void *valp, size_t val_len)
> > > >
> > > > int regmap_read(struct regmap *map, uint offset, uint *valp)
> > > > {
> > > > - return regmap_raw_read(map, offset, valp, map->width);
> > > > + int res;
> > > > +
> > > > + *valp = 0;
> > > > + res = regmap_raw_read(map, offset, valp, map->width);
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + *valp = le32_to_cpu(*valp);
> > >
> > > Looks like I'm a bit late to the party and Simon has already applied
> > > this patch.
> >
> > Where did he apply? I don't see it applied in u-boot-dm.
>
> Message-ID:
>
>
> The quoting is off but look at the last line, it says "applied to
> u-boot-dm/next".
>
Ah, my mail client failed to parse that mail.
Simon, will you drop the patch or shall I write a fix?
Marek
regmap bug fix
Simon, Heiko, Bin,
Pratyush discovered that the solution implemented by the patch
regmap: fix a serious pointer casting bug
is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct
position, but on big endian machines it also reverses byte order.
Somehow this went right through my head when I thought this up.
I have sent a new version, with subject
[PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug
The new solution utilizes an union { u8; u16; u32; u64; }, since all
members of an union start at the same address.
Could you please review this? Thanks.
Marek
Re: [PATCH u-boot v3 01/39] regmap: fix a serious pointer casting bug
On Tue, 16 Mar 2021 19:28:46 +0530
Pratyush Yadav wrote:
> On 16/03/21 01:25PM, Marek Behún wrote:
> > There is a serious bug in regmap_read() and regmap_write() functions
> > where an uint pointer is cast to (void *) which is then cast to (u8 *),
> > (u16 *), (u32 *) or (u64 *), depending on register width of the map.
> >
> > For example given a regmap with 16-bit register width the code
> > int val = 0x1234;
> > regmap_read(map, 0, );
> > only changes the lower 16 bits of val on little-endian machines.
> > The upper 16 bits will remain 0x1234.
> >
> > Nobody noticed this probably because this bug can be triggered with
> > regmap_write() only on big-endian architectures (which are not used by
> > many people anymore), and on little endian this bug has consequences
> > only if register width is 8 or 16 bits and also the memory place to
> > which regmap_read() should store it's result has non-zero upper bits,
> > which it seems doesn't happen anywhere in U-Boot normally. CI managed to
> > trigger this bug in unit test of dm_test_devm_regmap_field when compiled
> > for sandbox_defconfig using LTO.
> >
> > Fix this simply by taking into account that regmap_raw_read() and
> > regmap_raw_write() behave as if the data given to these functions were
> > in little-endian format, i.e. use cpu_to_le32() / le32_to_cpu(). In
> > regmap_read() also zero out the space so that we don't get invalid
> > result if regmap_raw_read() does not fill the whole object.
> >
> > Signed-off-by: Marek Behún
> > Reviewed-by: Simon Glass
> > Reviewed-by: Heiko Schocher
> > Reviewed-by: Bin Meng
> > ---
> > drivers/core/regmap.c | 13 -
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > index b51ce108c1..5d37006fff 100644
> > --- a/drivers/core/regmap.c
> > +++ b/drivers/core/regmap.c
> > @@ -435,7 +435,16 @@ int regmap_raw_read(struct regmap *map, uint offset,
> > void *valp, size_t val_len)
> >
> > int regmap_read(struct regmap *map, uint offset, uint *valp)
> > {
> > - return regmap_raw_read(map, offset, valp, map->width);
> > + int res;
> > +
> > + *valp = 0;
> > + res = regmap_raw_read(map, offset, valp, map->width);
> > + if (res)
> > + return res;
> > +
> > + *valp = le32_to_cpu(*valp);
>
> Looks like I'm a bit late to the party and Simon has already applied
> this patch.
Where did he apply? I don't see it applied in u-boot-dm.
> Anyway, I don't see why this is correct. regmap_raw_read()
> calls regmap_raw_read_range(), which calls the helpers __read_16(),
> __read_32() and so on.
>
> Take __read_16() for example. It takes the regmap's endianness and then
> based on that calls in_le16() or in_be16(). These calls translate to
> le16_to_cpu(__raw_readw(a)) or be16_to_cpu(__raw_readw(a)). Or the
> regmap is native endian in which case it is a simple readw(a).
>
> In all 3 cases the value returned is in cpu endianness. But you claim
> that "regmap_raw_read() and regmap_raw_write() behave as if the data
> given to these functions were in little-endian format".
>
> This is fine on a little endian cpu but on a big endian cpu you would
> reverse the byte order, no? Same for writes.
I made a mistake.
Somehow I thought that le32_to_cpu() will fix this, because it will
move the read bytes into the correct position. But somehow I forgot that
it will also reverse the byte order /o\ :D
I shall fix this and send a new version :D
Re: [PATCH u-boot v2.1 38/38] ARM: enable LTO for some boards
On Mon, 15 Mar 2021 21:12:32 -0400 Tom Rini wrote: > On Mon, Mar 15, 2021 at 10:42:31AM +0100, Marek Behun wrote: > > On Fri, 12 Mar 2021 15:47:12 -0500 > > Tom Rini wrote: > > > > > On Fri, Mar 12, 2021 at 06:36:05PM +0100, Marek Behun wrote: > > > > On Fri, 12 Mar 2021 18:19:08 +0100 > > > > Heinrich Schuchardt wrote: > > > > > > > > > On 12.03.21 18:03, Marek Behun wrote: > > > > > > On Fri, 12 Mar 2021 08:34:41 -0800 > > > > > > Tim Harvey wrote: > > > > > > > > > > > >> On Fri, Mar 12, 2021 at 5:47 AM Marek Behún > > > > > >> wrote: > > > > > >>> > > > > > >>> Enable LTO for some boards that were tested by people on U-Boot > > > > > >>> Mailing > > > > > >>> List. > > > > > >>> > > > > > >>> Signed-off-by: Marek Behún > > > > > >>> Tested-by: Adam Ford > > > > > >>> Tested-by: Pali Rohár > > > > > >>> Tested-by: Tim Harvey > > > > > >>> --- > > > > > >>> configs/am3517_evm_defconfig | 1 + > > > > > >>> configs/da850evm_defconfig| 1 + > > > > > >>> configs/da850evm_direct_nor_defconfig | 1 + > > > > > >>> configs/da850evm_nand_defconfig | 1 + > > > > > >>> configs/imx6q_logic_defconfig | 1 + > > > > > >>> configs/imx8mm_beacon_defconfig | 1 + > > > > > >>> configs/imx8mm_venice_defconfig | 1 + > > > > > >>> configs/imx8mn_beacon_2g_defconfig| 1 + > > > > > >>> configs/imx8mn_beacon_defconfig | 1 + > > > > > >>> configs/nokia_rx51_defconfig | 1 + > > > > > >>> configs/omap3_logic_defconfig | 1 + > > > > > >>> configs/r8a774a1_beacon_defconfig | 1 + > > > > > >>> configs/r8a774b1_beacon_defconfig | 1 + > > > > > >>> configs/r8a774e1_beacon_defconfig | 1 + > > > > > >>> configs/turris_mox_defconfig | 1 + > > > > > >>> configs/turris_omnia_defconfig| 1 + > > > > > >>> 16 files changed, 16 insertions(+) > > > > > >>> > > > > > >>> diff --git a/configs/am3517_evm_defconfig > > > > > >>> b/configs/am3517_evm_defconfig > > > > > >>> index bae0e0af35..d61eec94a4 100644 > > > > > >>> --- a/configs/am3517_evm_defconfig > > > > > >>> +++ b/configs/am3517_evm_defconfig > > > > > >>> @@ -1,3 +1,4 @@ > > > > > >>> +CONFIG_LTO=y > > > > > >>> CONFIG_ARM=y > > > > > >>> # CONFIG_SPL_USE_ARCH_MEMCPY is not set > > > > > >>> # CONFIG_SPL_USE_ARCH_MEMSET is not set > > > > > >>> diff --git a/configs/da850evm_defconfig > > > > > >>> b/configs/da850evm_defconfig > > > > > >>> index 26e76a2929..6ff5e21bc6 100644 > > > > > >>> --- a/configs/da850evm_defconfig > > > > > >>> +++ b/configs/da850evm_defconfig > > > > > >>> @@ -1,3 +1,4 @@ > > > > > >>> +CONFIG_LTO=y > > > > > >>> CONFIG_ARM=y > > > > > >>> CONFIG_SYS_THUMB_BUILD=y > > > > > >>> CONFIG_ARCH_DAVINCI=y > > > > > >>> diff --git a/configs/da850evm_direct_nor_defconfig > > > > > >>> b/configs/da850evm_direct_nor_defconfig > > > > > >>> index d3860a963d..06c7ce7c47 100644 > > > > > >>> --- a/configs/da850evm_direct_nor_defconfig > > > > > >>> +++ b/configs/da850evm_direct_nor_defconfig > > > > > >>> @@ -1,3 +1,4 @@ > > > > > >>> +CONFIG_LTO=y > > > > > >>> CONFIG_ARM=y > > > > > >>> CONFIG_ARCH_CPU_INIT=y > > > > > >>> CONFIG_ARCH_DAVINCI=y > > > > > >>> diff --git a/configs/da850evm_nand_defconfig > > > > > >>> b/configs/da850evm_nand_defconfig > > > > > >>> index 0d0e9a148d..be737564e1
Re: [PATCH u-boot v2.1 38/38] ARM: enable LTO for some boards
On Fri, 12 Mar 2021 10:01:34 -0800 Tim Harvey wrote: > Marek / Heinrich, > > Yes, 'make -j1' does work. > > Tim Tim, could you try make -j8, but change the toplevel Makefile: find string "-flto=jobserver" and change it to "-flto". Does make -j8 fail then? Thank you. Marek
Re: [PATCH u-boot v2 10/38] efi_loader: fix warning when linking with LTO
On Fri, 12 Mar 2021 17:42:35 +0100
Heinrich Schuchardt wrote:
> On 12.03.21 11:34, Marek Behún wrote:
> > When linking with LTO, the compiler complains about type mismatch of
> > variables `__efi_runtime_start`, `__efi_runtime_stop`,
> > `__efi_runtime_rel_start` and `__efi_runtime_rel_stop`:
> >
> > include/efi_loader.h:218:21: warning: type of ‘__efi_runtime_start’
> >does not match original
> >declaration [-Wlto-type-mismatch]
> > 218 | extern unsigned int __efi_runtime_start, __efi_runtime_stop;
> > | ^
> > arch/sandbox/lib/sections.c:7:6: note: ‘__efi_runtime_start’ was
> > previously declared here
> > 7 | char __efi_runtime_start[0] __attribute__((section(".__efi_run
> > | ^
> >
> > Change the type to char[] in include/efi_loader.h.
> >
> > Signed-off-by: Marek Behún
> > Reviewed-by: Bin Meng
>
> This patch leaves us with definition differences:
>
> We have:
>
> arch/arm/lib/sections.c:31:
> char __efi_runtime_start[0]
> __attribute__((section(".__efi_runtime_start")));
>
> arch/x86/lib/sections.c:6:char __efi_runtime_start[0]
> __attribute__((section(".__efi_runtime_start")));
>
> We should use [] everywhere. [0] was needed by elder GCC versions.
No, these two things are different: in the header file we have an
extern declaration, while in the sections.c files those are definitions.
We cannot use char __efi_runtime_start[] for definition, the compiler
will complain:
warning: array ‘__efi_runtime_start’ assumed to have one element
On the other hand the sections.c file explains why those symbols need
to be declared in a C file and why a 0-size array is chosen.
Marek
Re: [PATCH u-boot v2.1 38/38] ARM: enable LTO for some boards
On Fri, 12 Mar 2021 15:47:12 -0500 Tom Rini wrote: > On Fri, Mar 12, 2021 at 06:36:05PM +0100, Marek Behun wrote: > > On Fri, 12 Mar 2021 18:19:08 +0100 > > Heinrich Schuchardt wrote: > > > > > On 12.03.21 18:03, Marek Behun wrote: > > > > On Fri, 12 Mar 2021 08:34:41 -0800 > > > > Tim Harvey wrote: > > > > > > > >> On Fri, Mar 12, 2021 at 5:47 AM Marek Behún > > > >> wrote: > > > >>> > > > >>> Enable LTO for some boards that were tested by people on U-Boot > > > >>> Mailing > > > >>> List. > > > >>> > > > >>> Signed-off-by: Marek Behún > > > >>> Tested-by: Adam Ford > > > >>> Tested-by: Pali Rohár > > > >>> Tested-by: Tim Harvey > > > >>> --- > > > >>> configs/am3517_evm_defconfig | 1 + > > > >>> configs/da850evm_defconfig| 1 + > > > >>> configs/da850evm_direct_nor_defconfig | 1 + > > > >>> configs/da850evm_nand_defconfig | 1 + > > > >>> configs/imx6q_logic_defconfig | 1 + > > > >>> configs/imx8mm_beacon_defconfig | 1 + > > > >>> configs/imx8mm_venice_defconfig | 1 + > > > >>> configs/imx8mn_beacon_2g_defconfig| 1 + > > > >>> configs/imx8mn_beacon_defconfig | 1 + > > > >>> configs/nokia_rx51_defconfig | 1 + > > > >>> configs/omap3_logic_defconfig | 1 + > > > >>> configs/r8a774a1_beacon_defconfig | 1 + > > > >>> configs/r8a774b1_beacon_defconfig | 1 + > > > >>> configs/r8a774e1_beacon_defconfig | 1 + > > > >>> configs/turris_mox_defconfig | 1 + > > > >>> configs/turris_omnia_defconfig| 1 + > > > >>> 16 files changed, 16 insertions(+) > > > >>> > > > >>> diff --git a/configs/am3517_evm_defconfig > > > >>> b/configs/am3517_evm_defconfig > > > >>> index bae0e0af35..d61eec94a4 100644 > > > >>> --- a/configs/am3517_evm_defconfig > > > >>> +++ b/configs/am3517_evm_defconfig > > > >>> @@ -1,3 +1,4 @@ > > > >>> +CONFIG_LTO=y > > > >>> CONFIG_ARM=y > > > >>> # CONFIG_SPL_USE_ARCH_MEMCPY is not set > > > >>> # CONFIG_SPL_USE_ARCH_MEMSET is not set > > > >>> diff --git a/configs/da850evm_defconfig b/configs/da850evm_defconfig > > > >>> index 26e76a2929..6ff5e21bc6 100644 > > > >>> --- a/configs/da850evm_defconfig > > > >>> +++ b/configs/da850evm_defconfig > > > >>> @@ -1,3 +1,4 @@ > > > >>> +CONFIG_LTO=y > > > >>> CONFIG_ARM=y > > > >>> CONFIG_SYS_THUMB_BUILD=y > > > >>> CONFIG_ARCH_DAVINCI=y > > > >>> diff --git a/configs/da850evm_direct_nor_defconfig > > > >>> b/configs/da850evm_direct_nor_defconfig > > > >>> index d3860a963d..06c7ce7c47 100644 > > > >>> --- a/configs/da850evm_direct_nor_defconfig > > > >>> +++ b/configs/da850evm_direct_nor_defconfig > > > >>> @@ -1,3 +1,4 @@ > > > >>> +CONFIG_LTO=y > > > >>> CONFIG_ARM=y > > > >>> CONFIG_ARCH_CPU_INIT=y > > > >>> CONFIG_ARCH_DAVINCI=y > > > >>> diff --git a/configs/da850evm_nand_defconfig > > > >>> b/configs/da850evm_nand_defconfig > > > >>> index 0d0e9a148d..be737564e1 100644 > > > >>> --- a/configs/da850evm_nand_defconfig > > > >>> +++ b/configs/da850evm_nand_defconfig > > > >>> @@ -1,3 +1,4 @@ > > > >>> +CONFIG_LTO=y > > > >>> CONFIG_ARM=y > > > >>> CONFIG_SYS_THUMB_BUILD=y > > > >>> CONFIG_ARCH_DAVINCI=y > > > >>> diff --git a/configs/imx6q_logic_defconfig > > > >>> b/configs/imx6q_logic_defconfig > > > >>> index 36dc24d080..0f8aea6983 100644 > > > >>> --- a/configs/imx6q_logic_defconfig > > > >>> +++ b/configs/imx6q_logic_defconfig > > > >>> @@ -1,3 +1,4 @@ > > > >>> +CONFIG_LTO=y > > > >>> CONFIG_ARM=y > > > >>> CONFIG_ARCH_MX6=y > > > >>&g
Re: [PATCH u-boot v2 32/38] ARM: omap3: fix LTO for DM3730 (and possibly other omap3 boards)
On Sat, 13 Mar 2021 09:23:05 -0600 Adam Ford wrote: > I have tested this on omap35_logic_somlv and the LTO flag removal > isn't necessary for the clock.o > Having the clock built with LTO saves 239 bytes in SPL with my > compiler. It's not a huge savings, but in SPL, we need as much as > possible. So I can drop this patch? Marek
Re: [PATCH u-boot v2.1 38/38] ARM: enable LTO for some boards
On Fri, 12 Mar 2021 18:19:08 +0100 Heinrich Schuchardt wrote: > On 12.03.21 18:03, Marek Behun wrote: > > On Fri, 12 Mar 2021 08:34:41 -0800 > > Tim Harvey wrote: > > > >> On Fri, Mar 12, 2021 at 5:47 AM Marek Behún wrote: > >>> > >>> Enable LTO for some boards that were tested by people on U-Boot Mailing > >>> List. > >>> > >>> Signed-off-by: Marek Behún > >>> Tested-by: Adam Ford > >>> Tested-by: Pali Rohár > >>> Tested-by: Tim Harvey > >>> --- > >>> configs/am3517_evm_defconfig | 1 + > >>> configs/da850evm_defconfig| 1 + > >>> configs/da850evm_direct_nor_defconfig | 1 + > >>> configs/da850evm_nand_defconfig | 1 + > >>> configs/imx6q_logic_defconfig | 1 + > >>> configs/imx8mm_beacon_defconfig | 1 + > >>> configs/imx8mm_venice_defconfig | 1 + > >>> configs/imx8mn_beacon_2g_defconfig| 1 + > >>> configs/imx8mn_beacon_defconfig | 1 + > >>> configs/nokia_rx51_defconfig | 1 + > >>> configs/omap3_logic_defconfig | 1 + > >>> configs/r8a774a1_beacon_defconfig | 1 + > >>> configs/r8a774b1_beacon_defconfig | 1 + > >>> configs/r8a774e1_beacon_defconfig | 1 + > >>> configs/turris_mox_defconfig | 1 + > >>> configs/turris_omnia_defconfig| 1 + > >>> 16 files changed, 16 insertions(+) > >>> > >>> diff --git a/configs/am3517_evm_defconfig b/configs/am3517_evm_defconfig > >>> index bae0e0af35..d61eec94a4 100644 > >>> --- a/configs/am3517_evm_defconfig > >>> +++ b/configs/am3517_evm_defconfig > >>> @@ -1,3 +1,4 @@ > >>> +CONFIG_LTO=y > >>> CONFIG_ARM=y > >>> # CONFIG_SPL_USE_ARCH_MEMCPY is not set > >>> # CONFIG_SPL_USE_ARCH_MEMSET is not set > >>> diff --git a/configs/da850evm_defconfig b/configs/da850evm_defconfig > >>> index 26e76a2929..6ff5e21bc6 100644 > >>> --- a/configs/da850evm_defconfig > >>> +++ b/configs/da850evm_defconfig > >>> @@ -1,3 +1,4 @@ > >>> +CONFIG_LTO=y > >>> CONFIG_ARM=y > >>> CONFIG_SYS_THUMB_BUILD=y > >>> CONFIG_ARCH_DAVINCI=y > >>> diff --git a/configs/da850evm_direct_nor_defconfig > >>> b/configs/da850evm_direct_nor_defconfig > >>> index d3860a963d..06c7ce7c47 100644 > >>> --- a/configs/da850evm_direct_nor_defconfig > >>> +++ b/configs/da850evm_direct_nor_defconfig > >>> @@ -1,3 +1,4 @@ > >>> +CONFIG_LTO=y > >>> CONFIG_ARM=y > >>> CONFIG_ARCH_CPU_INIT=y > >>> CONFIG_ARCH_DAVINCI=y > >>> diff --git a/configs/da850evm_nand_defconfig > >>> b/configs/da850evm_nand_defconfig > >>> index 0d0e9a148d..be737564e1 100644 > >>> --- a/configs/da850evm_nand_defconfig > >>> +++ b/configs/da850evm_nand_defconfig > >>> @@ -1,3 +1,4 @@ > >>> +CONFIG_LTO=y > >>> CONFIG_ARM=y > >>> CONFIG_SYS_THUMB_BUILD=y > >>> CONFIG_ARCH_DAVINCI=y > >>> diff --git a/configs/imx6q_logic_defconfig b/configs/imx6q_logic_defconfig > >>> index 36dc24d080..0f8aea6983 100644 > >>> --- a/configs/imx6q_logic_defconfig > >>> +++ b/configs/imx6q_logic_defconfig > >>> @@ -1,3 +1,4 @@ > >>> +CONFIG_LTO=y > >>> CONFIG_ARM=y > >>> CONFIG_ARCH_MX6=y > >>> CONFIG_SYS_TEXT_BASE=0x1780 > >>> diff --git a/configs/imx8mm_beacon_defconfig > >>> b/configs/imx8mm_beacon_defconfig > >>> index 045b19f4f3..e8bb44eea6 100644 > >>> --- a/configs/imx8mm_beacon_defconfig > >>> +++ b/configs/imx8mm_beacon_defconfig > >>> @@ -1,3 +1,4 @@ > >>> +CONFIG_LTO=y > >>> CONFIG_ARM=y > >>> CONFIG_ARCH_IMX8M=y > >>> CONFIG_SYS_TEXT_BASE=0x4020 > >>> diff --git a/configs/imx8mm_venice_defconfig > >>> b/configs/imx8mm_venice_defconfig > >>> index a15c3641f6..dff8f64540 100644 > >>> --- a/configs/imx8mm_venice_defconfig > >>> +++ b/configs/imx8mm_venice_defconfig > >>> @@ -1,3 +1,4 @@ > >>> +CONFIG_LTO=y > >>> CONFIG_ARM=y > >>> CONFIG_ARCH_IMX8M=y > >>> CONFIG_SYS_TEXT_BASE=0x4020 > >>> diff --git a/configs/imx8mn_beacon_2g_defconfig > >&
Re: [PATCH u-boot v2.1 38/38] ARM: enable LTO for some boards
On Fri, 12 Mar 2021 08:34:41 -0800 Tim Harvey wrote: > On Fri, Mar 12, 2021 at 5:47 AM Marek Behún wrote: > > > > Enable LTO for some boards that were tested by people on U-Boot Mailing > > List. > > > > Signed-off-by: Marek Behún > > Tested-by: Adam Ford > > Tested-by: Pali Rohár > > Tested-by: Tim Harvey > > --- > > configs/am3517_evm_defconfig | 1 + > > configs/da850evm_defconfig| 1 + > > configs/da850evm_direct_nor_defconfig | 1 + > > configs/da850evm_nand_defconfig | 1 + > > configs/imx6q_logic_defconfig | 1 + > > configs/imx8mm_beacon_defconfig | 1 + > > configs/imx8mm_venice_defconfig | 1 + > > configs/imx8mn_beacon_2g_defconfig| 1 + > > configs/imx8mn_beacon_defconfig | 1 + > > configs/nokia_rx51_defconfig | 1 + > > configs/omap3_logic_defconfig | 1 + > > configs/r8a774a1_beacon_defconfig | 1 + > > configs/r8a774b1_beacon_defconfig | 1 + > > configs/r8a774e1_beacon_defconfig | 1 + > > configs/turris_mox_defconfig | 1 + > > configs/turris_omnia_defconfig| 1 + > > 16 files changed, 16 insertions(+) > > > > diff --git a/configs/am3517_evm_defconfig b/configs/am3517_evm_defconfig > > index bae0e0af35..d61eec94a4 100644 > > --- a/configs/am3517_evm_defconfig > > +++ b/configs/am3517_evm_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > # CONFIG_SPL_USE_ARCH_MEMCPY is not set > > # CONFIG_SPL_USE_ARCH_MEMSET is not set > > diff --git a/configs/da850evm_defconfig b/configs/da850evm_defconfig > > index 26e76a2929..6ff5e21bc6 100644 > > --- a/configs/da850evm_defconfig > > +++ b/configs/da850evm_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_SYS_THUMB_BUILD=y > > CONFIG_ARCH_DAVINCI=y > > diff --git a/configs/da850evm_direct_nor_defconfig > > b/configs/da850evm_direct_nor_defconfig > > index d3860a963d..06c7ce7c47 100644 > > --- a/configs/da850evm_direct_nor_defconfig > > +++ b/configs/da850evm_direct_nor_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_ARCH_CPU_INIT=y > > CONFIG_ARCH_DAVINCI=y > > diff --git a/configs/da850evm_nand_defconfig > > b/configs/da850evm_nand_defconfig > > index 0d0e9a148d..be737564e1 100644 > > --- a/configs/da850evm_nand_defconfig > > +++ b/configs/da850evm_nand_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_SYS_THUMB_BUILD=y > > CONFIG_ARCH_DAVINCI=y > > diff --git a/configs/imx6q_logic_defconfig b/configs/imx6q_logic_defconfig > > index 36dc24d080..0f8aea6983 100644 > > --- a/configs/imx6q_logic_defconfig > > +++ b/configs/imx6q_logic_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_ARCH_MX6=y > > CONFIG_SYS_TEXT_BASE=0x1780 > > diff --git a/configs/imx8mm_beacon_defconfig > > b/configs/imx8mm_beacon_defconfig > > index 045b19f4f3..e8bb44eea6 100644 > > --- a/configs/imx8mm_beacon_defconfig > > +++ b/configs/imx8mm_beacon_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_ARCH_IMX8M=y > > CONFIG_SYS_TEXT_BASE=0x4020 > > diff --git a/configs/imx8mm_venice_defconfig > > b/configs/imx8mm_venice_defconfig > > index a15c3641f6..dff8f64540 100644 > > --- a/configs/imx8mm_venice_defconfig > > +++ b/configs/imx8mm_venice_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_ARCH_IMX8M=y > > CONFIG_SYS_TEXT_BASE=0x4020 > > diff --git a/configs/imx8mn_beacon_2g_defconfig > > b/configs/imx8mn_beacon_2g_defconfig > > index 58b8e49486..1c8cbc2c89 100644 > > --- a/configs/imx8mn_beacon_2g_defconfig > > +++ b/configs/imx8mn_beacon_2g_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_ARCH_IMX8M=y > > CONFIG_SYS_TEXT_BASE=0x4020 > > diff --git a/configs/imx8mn_beacon_defconfig > > b/configs/imx8mn_beacon_defconfig > > index d6a3385d8d..6457b9409a 100644 > > --- a/configs/imx8mn_beacon_defconfig > > +++ b/configs/imx8mn_beacon_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > CONFIG_ARCH_IMX8M=y > > CONFIG_SYS_TEXT_BASE=0x4020 > > diff --git a/configs/nokia_rx51_defconfig b/configs/nokia_rx51_defconfig > > index 312ca3a1a9..85ca627790 100644 > > --- a/configs/nokia_rx51_defconfig > > +++ b/configs/nokia_rx51_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > # CONFIG_SYS_THUMB_BUILD is not set > > CONFIG_ARCH_OMAP2PLUS=y > > diff --git a/configs/omap3_logic_defconfig b/configs/omap3_logic_defconfig > > index 4e37237b86..cc4b727a2c 100644 > > --- a/configs/omap3_logic_defconfig > > +++ b/configs/omap3_logic_defconfig > > @@ -1,3 +1,4 @@ > > +CONFIG_LTO=y > > CONFIG_ARM=y > > # CONFIG_SPL_USE_ARCH_MEMCPY is not set > > # CONFIG_SPL_USE_ARCH_MEMSET is not set > > diff --git a/configs/r8a774a1_beacon_defconfig > > b/configs/r8a774a1_beacon_defconfig > > index 2f45edd92e..9dd5d9192e 100644 > > --- a/configs/r8a774a1_beacon_defconfig > > +++ b/configs/r8a774a1_beacon_defconfig >
Re: [PATCH u-boot v2 00/38] U-Boot LTO (Sandbox + Some ARM boards)
On Fri, 12 Mar 2021 16:11:13 +0100 Harald Seiler wrote: > On Fri, 2021-03-12 at 16:07 +0100, Harald Seiler wrote: > > On Fri, 2021-03-12 at 15:26 +0100, Marek Behun wrote: > > > On Fri, 12 Mar 2021 15:21:05 +0100 > > > Harald Seiler wrote: > > > > > > > Hi Marek, > > > > > > > > On Fri, 2021-03-12 at 11:33 +0100, Marek Behún wrote: > > > > > Hello, > > > > > > > > > > I am sending version 2 of patches adding support for LTO to U-Boot. > > > > > > > > > > This series was tested by Github/Azure CI at > > > > > https://github.com/u-boot/u-boot/pull/57 > > > > > > > > > > Code reduction is on average 4.23% for u-boot.bin and 13.58% for > > > > > u-boot-spl.bin. > > > > > > > > > > I am currently running a build test for all 1077 ARM defconfigs. > > > > > Of the first 232 defconfigs, 2 are failing when LTO is enabled > > > > > (chromebook_jerry and chromebook_speedy). Note that this series > > > > > only enables LTO for tested boards. > > > > > > > > > > Changes since v1: > > > > > - remove patches applied into u-boot-marvell > > > > > - added Reviewed-by tags > > > > > - addressed some issues discovered by Bin Meng, Marek Vasut, > > > > > Heinrich Schuchardt > > > > > - added more ARM boards (thanks to Adam Ford, Tim Harvey and Bin Meng) > > > > > - removed --gc-sections for ARM if internal libgcc is used > > > > > - remove -fwhole-program in final LTO LDFLAGS > > > > > - declared all 4 functions (memcpy, memset, memcmp, memmove) __used, > > > > > (these are mentioned in GCC man page for option -nodefaultlibs that > > > > > the compiler may generate; this seems to be a bug in GCC that > > > > > linking > > > > > fails with LTO even if these functions are present, because the > > > > > symbols can be renamed on some targets by optimization) > > > > > > > > I'm hitting a compiler error when building with imx6q_logic_defconfig: > > > > > > > > real-ld: error: no memory region specified for loadable section > > > > `.note.gnu.build-id' > > > > > > > > It seems this is caused by calling the linker through a gcc invocation > > > > which adds a `--build-id` commandline flag. I think the linker script > > > > which is used for SPL in this case (arch/arm/mach-omap2/u-boot-spl.lds) > > > > isn't properly set up to deal with a build-id. > > > > > > > > I'm not sure how to deal with this. One could either add > > > > `--build-id=none` to the GCC commandline to suppress generation of this > > > > section entirely (it is not emitted in non-LTO builds right now anyway) > > > > or > > > > include it in .text in said linker script so it is visible on the > > > > target. > > > > What do you think? > > > > > > > > I should note that I am using a Yocto-generated toolchain. I suppose > > > > most > > > > standard toolchains' behavior regarding the `--build-id` flag probably > > > > differs. > > > > > > > > Regards, > > > > > > I encountered this with Debian's cross toolchain, but since this did > > > not happen on my station with Gentoo crossdev toolchain, nor on Azure > > > CI, I ignored it. > > > > > > What is the purpose of --build-id? Why do people use it? > > > > I'm not entirely sure but I think it acts as a unique identifier for > > a certain binary. So you can match up a core-dump with its debug info for > > example. > > > > But I am unsure if anyone in the firmware space is actively using this > > feature... At least U-Boot does not actually include the build-id on the > > target - it is not generated for SPL at all and U-Boot proper only > > contains it in the ELF file, it is not exported into the raw binary. > > This is the origin of --build-id: > > https://fedoraproject.org/wiki/Releases/FeatureBuildId > Tom, do we want build-id stored in binaries? Maybe it can be useful for something. Marek
Re: [PATCH u-boot v2 00/38] U-Boot LTO (Sandbox + Some ARM boards)
On Fri, 12 Mar 2021 15:21:05 +0100 Harald Seiler wrote: > Hi Marek, > > On Fri, 2021-03-12 at 11:33 +0100, Marek Behún wrote: > > Hello, > > > > I am sending version 2 of patches adding support for LTO to U-Boot. > > > > This series was tested by Github/Azure CI at > > https://github.com/u-boot/u-boot/pull/57 > > > > Code reduction is on average 4.23% for u-boot.bin and 13.58% for > > u-boot-spl.bin. > > > > I am currently running a build test for all 1077 ARM defconfigs. > > Of the first 232 defconfigs, 2 are failing when LTO is enabled > > (chromebook_jerry and chromebook_speedy). Note that this series > > only enables LTO for tested boards. > > > > Changes since v1: > > - remove patches applied into u-boot-marvell > > - added Reviewed-by tags > > - addressed some issues discovered by Bin Meng, Marek Vasut, > > Heinrich Schuchardt > > - added more ARM boards (thanks to Adam Ford, Tim Harvey and Bin Meng) > > - removed --gc-sections for ARM if internal libgcc is used > > - remove -fwhole-program in final LTO LDFLAGS > > - declared all 4 functions (memcpy, memset, memcmp, memmove) __used, > > (these are mentioned in GCC man page for option -nodefaultlibs that > > the compiler may generate; this seems to be a bug in GCC that linking > > fails with LTO even if these functions are present, because the > > symbols can be renamed on some targets by optimization) > > I'm hitting a compiler error when building with imx6q_logic_defconfig: > > real-ld: error: no memory region specified for loadable section > `.note.gnu.build-id' > > It seems this is caused by calling the linker through a gcc invocation > which adds a `--build-id` commandline flag. I think the linker script > which is used for SPL in this case (arch/arm/mach-omap2/u-boot-spl.lds) > isn't properly set up to deal with a build-id. > > I'm not sure how to deal with this. One could either add > `--build-id=none` to the GCC commandline to suppress generation of this > section entirely (it is not emitted in non-LTO builds right now anyway) or > include it in .text in said linker script so it is visible on the target. > What do you think? > > I should note that I am using a Yocto-generated toolchain. I suppose most > standard toolchains' behavior regarding the `--build-id` flag probably > differs. > > Regards, I encountered this with Debian's cross toolchain, but since this did not happen on my station with Gentoo crossdev toolchain, nor on Azure CI, I ignored it. What is the purpose of --build-id? Why do people use it? Marek
Re: [PATCH u-boot 37/39] ARM: don't use -ffunction-sections/-fdata-sections with LTO build
On Fri, 12 Mar 2021 08:18:44 -0500 Tom Rini wrote: > On Fri, Mar 12, 2021 at 08:29:05AM +0100, Marek Behun wrote: > > On Fri, 12 Mar 2021 15:19:26 +0800 > > Bin Meng wrote: > > > > > On Fri, Mar 12, 2021 at 3:11 PM Marek Behun wrote: > > > > > > > > On Fri, 12 Mar 2021 14:48:04 +0800 > > > > Bin Meng wrote: > > > > > > > > > On Fri, Mar 12, 2021 at 2:45 PM Marek Behun > > > > > wrote: > > > > > > > > > > > > On Wed, 10 Mar 2021 11:40:42 +0800 > > > > > > Bin Meng wrote: > > > > > > > > > > > > > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún > > > > > > > wrote: > > > > > > > > > > > > > > > > When building with LTO, using > > > > > > > > -ffunction-sections/-fdata-sections is not > > > > > > > > useful anymore. > > > > > > > > > > > > > > > > Signed-off-by: Marek Behún > > > > > > > > --- > > > > > > > > arch/arm/config.mk | 8 ++-- > > > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > I believe we should also remove --gc-sections. > > > > > > > > > > > > It seems that --gc-sections cannot be removed, otherwise some > > > > > > builds, > > > > > > for example turris_mox_defconfig, fail with > > > > > > > > > > > > LTO u-boot > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(lse-init.o): > > > > > > in function `init_have_lse_atomics': > > > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/config/aarch64/lse-init.c:44: > > > > > > undefined reference to `__getauxval' > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): > > > > > > in function `__absvdi2': > > > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:228: > > > > > > undefined reference to `abort' > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): > > > > > > in function `__absvsi2': > > > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:246: > > > > > > undefined reference to `abort' > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvdi2.o): > > > > > > in function `__absvti2': > > > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:267: > > > > > > undefined reference to `abort' > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): > > > > > > in function `__addvdi3': > > > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:81: > > > > > > undefined reference to `abort' > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): > > > > > > in function `__addvsi3': > > > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:92: > > > > > > undefined reference to `abort' > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvdi3.o):/var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:106: > > > > > > more undefined references to `abort' follow > > > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): > > > > > >
Re: [PATCH u-boot 12/39] string: make memcpy() and memset() visible to fix LTO linking errors
I created a gcc bug for this https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99559
Re: [PATCH u-boot 37/39] ARM: don't use -ffunction-sections/-fdata-sections with LTO build
On Fri, 12 Mar 2021 15:19:26 +0800 Bin Meng wrote: > On Fri, Mar 12, 2021 at 3:11 PM Marek Behun wrote: > > > > On Fri, 12 Mar 2021 14:48:04 +0800 > > Bin Meng wrote: > > > > > On Fri, Mar 12, 2021 at 2:45 PM Marek Behun wrote: > > > > > > > > On Wed, 10 Mar 2021 11:40:42 +0800 > > > > Bin Meng wrote: > > > > > > > > > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún > > > > > wrote: > > > > > > > > > > > > When building with LTO, using -ffunction-sections/-fdata-sections > > > > > > is not > > > > > > useful anymore. > > > > > > > > > > > > Signed-off-by: Marek Behún > > > > > > --- > > > > > > arch/arm/config.mk | 8 ++-- > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > I believe we should also remove --gc-sections. > > > > > > > > It seems that --gc-sections cannot be removed, otherwise some builds, > > > > for example turris_mox_defconfig, fail with > > > > > > > > LTO u-boot > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(lse-init.o): in > > > > function `init_have_lse_atomics': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/config/aarch64/lse-init.c:44: > > > > undefined reference to `__getauxval' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in > > > > function `__absvdi2': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:228: > > > > undefined reference to `abort' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in > > > > function `__absvsi2': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:246: > > > > undefined reference to `abort' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvdi2.o): in > > > > function `__absvti2': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:267: > > > > undefined reference to `abort' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in > > > > function `__addvdi3': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:81: > > > > undefined reference to `abort' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in > > > > function `__addvsi3': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:92: > > > > undefined reference to `abort' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvdi3.o):/var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:106: > > > > more undefined references to `abort' follow > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > > > function `__eprintf': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: > > > > undefined reference to `stderr' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: > > > > undefined reference to `stderr' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > > > function `fprintf': > > > > /usr/aarch64-unknown-linux-gnu/sys-include/bits/stdio2.h:103: undefined > > > > reference to `__fprintf_chk' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > > > function `__eprintf': > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2153: > > > > undefined reference to `fflush' > > > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2154: > > > > undefined reference to `abort' > > > > > > Ouch! How compiler behaves when it comes to LTO and works with all > > > these compiler/linker options is really a mystery ... > > > > OK, it seems that on aarch64 we are actually using system's libgcc :) > > Thanks. > > > Not the internal one. So it seems we need --gc-sections to throw away > > garbade that is not used. > > Needed only when CONFIG_USE_PRIVATE_LIBGCC is off? Seems that way.
Re: [PATCH u-boot 37/39] ARM: don't use -ffunction-sections/-fdata-sections with LTO build
On Fri, 12 Mar 2021 14:48:04 +0800 Bin Meng wrote: > On Fri, Mar 12, 2021 at 2:45 PM Marek Behun wrote: > > > > On Wed, 10 Mar 2021 11:40:42 +0800 > > Bin Meng wrote: > > > > > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún wrote: > > > > > > > > When building with LTO, using -ffunction-sections/-fdata-sections is not > > > > useful anymore. > > > > > > > > Signed-off-by: Marek Behún > > > > --- > > > > arch/arm/config.mk | 8 ++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > I believe we should also remove --gc-sections. > > > > It seems that --gc-sections cannot be removed, otherwise some builds, > > for example turris_mox_defconfig, fail with > > > > LTO u-boot > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(lse-init.o): in > > function `init_have_lse_atomics': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/config/aarch64/lse-init.c:44: > > undefined reference to `__getauxval' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in > > function `__absvdi2': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:228: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in > > function `__absvsi2': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:246: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvdi2.o): in > > function `__absvti2': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:267: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in > > function `__addvdi3': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:81: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in > > function `__addvsi3': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:92: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvdi3.o):/var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:106: > > more undefined references to `abort' follow > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > function `__eprintf': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: > > undefined reference to `stderr' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: > > undefined reference to `stderr' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > function `fprintf': > > /usr/aarch64-unknown-linux-gnu/sys-include/bits/stdio2.h:103: undefined > > reference to `__fprintf_chk' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > function `__eprintf': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2153: > > undefined reference to `fflush' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2154: > > undefined reference to `abort' > > Ouch! How compiler behaves when it comes to LTO and works with all > these compiler/linker options is really a mystery ... OK, it seems that on aarch64 we are actually using system's libgcc :) Not the internal one. So it seems we need --gc-sections to throw away garbade that is not used. Marek
Re: [PATCH u-boot 37/39] ARM: don't use -ffunction-sections/-fdata-sections with LTO build
On Fri, 12 Mar 2021 14:48:04 +0800 Bin Meng wrote: > On Fri, Mar 12, 2021 at 2:45 PM Marek Behun wrote: > > > > On Wed, 10 Mar 2021 11:40:42 +0800 > > Bin Meng wrote: > > > > > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún wrote: > > > > > > > > When building with LTO, using -ffunction-sections/-fdata-sections is not > > > > useful anymore. > > > > > > > > Signed-off-by: Marek Behún > > > > --- > > > > arch/arm/config.mk | 8 ++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > I believe we should also remove --gc-sections. > > > > It seems that --gc-sections cannot be removed, otherwise some builds, > > for example turris_mox_defconfig, fail with > > > > LTO u-boot > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(lse-init.o): in > > function `init_have_lse_atomics': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/config/aarch64/lse-init.c:44: > > undefined reference to `__getauxval' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in > > function `__absvdi2': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:228: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in > > function `__absvsi2': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:246: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvdi2.o): in > > function `__absvti2': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:267: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in > > function `__addvdi3': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:81: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in > > function `__addvsi3': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:92: > > undefined reference to `abort' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvdi3.o):/var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:106: > > more undefined references to `abort' follow > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > function `__eprintf': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: > > undefined reference to `stderr' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: > > undefined reference to `stderr' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > function `fprintf': > > /usr/aarch64-unknown-linux-gnu/sys-include/bits/stdio2.h:103: undefined > > reference to `__fprintf_chk' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in > > function `__eprintf': > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2153: > > undefined reference to `fflush' > > /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: > > /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2154: > > undefined reference to `abort' > > Ouch! How compiler behaves when it comes to LTO and works with all > these compiler/linker options is really a mystery ... Anyway this is weird. __fprintf_chk is a glibc function, so how it ended here I don't know... I am going to investigate this
Re: [PATCH u-boot 37/39] ARM: don't use -ffunction-sections/-fdata-sections with LTO build
On Wed, 10 Mar 2021 11:40:42 +0800 Bin Meng wrote: > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún wrote: > > > > When building with LTO, using -ffunction-sections/-fdata-sections is not > > useful anymore. > > > > Signed-off-by: Marek Behún > > --- > > arch/arm/config.mk | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > I believe we should also remove --gc-sections. It seems that --gc-sections cannot be removed, otherwise some builds, for example turris_mox_defconfig, fail with LTO u-boot /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(lse-init.o): in function `init_have_lse_atomics': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/config/aarch64/lse-init.c:44: undefined reference to `__getauxval' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in function `__absvdi2': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:228: undefined reference to `abort' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvsi2.o): in function `__absvsi2': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:246: undefined reference to `abort' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_absvdi2.o): in function `__absvti2': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:267: undefined reference to `abort' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in function `__addvdi3': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:81: undefined reference to `abort' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvsi3.o): in function `__addvsi3': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:92: undefined reference to `abort' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_addvdi3.o):/var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:106: more undefined references to `abort' follow /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in function `__eprintf': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: undefined reference to `stderr' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2152: undefined reference to `stderr' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in function `fprintf': /usr/aarch64-unknown-linux-gnu/sys-include/bits/stdio2.h:103: undefined reference to `__fprintf_chk' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /usr/lib/gcc/aarch64-unknown-linux-gnu/10.2.0/libgcc.a(_eprintf.o): in function `__eprintf': /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2153: undefined reference to `fflush' /usr/libexec/gcc/aarch64-unknown-linux-gnu/ld: /var/tmp/portage/cross-aarch64-unknown-linux-gnu/gcc-10.2.0-r5/work/gcc-10.2.0/libgcc/libgcc2.c:2154: undefined reference to `abort'
Re: [PATCH u-boot 18/39] build: LTO: move platform libs into --start-group list
On Tue, 9 Mar 2021 23:24:01 +0800 Bin Meng wrote: > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún wrote: > > > > When building with LTO, move $(PLATFORM_LIBS) into the --start-group / > > --end-group list. > > This should be --whole-archive > > > Otherwise some functions declared in assembly may not be resolved and > > linking may fail. > > I am not sure if this is caused by we build these lib functions with -flto? It is caused by using thin archives, not LTO
Re: [PATCH u-boot 17/39] build: support building with Link Time Optimizations
On Tue, 9 Mar 2021 21:30:26 +0800 Bin Meng wrote: > > +config LTO > > + bool "Enable Link Time Optimizations" > > + depends on ARCH_SUPPORTS_LTO > > + default n > > nits: this line is not needed as the default value is n if omitted This is for consistency. The other options in this file also use "default n" > > +ifdef CONFIG_LTO > > + LTO_CFLAGS := -flto > > + LTO_FINAL_LDFLAGS := -fwhole-program > > This one should not be necessary per my read of the GCC doc. Also in > your patch, it is only used in the SPL build. I shall do some tests. > > +ifdef CONFIG_LTO > > +quiet_cmd_u-boot__ ?= LTO $@ > > + cmd_u-boot__ ?= > > \ > > + $(CC) -nostdlib -nostartfiles > > \ > > + $(LTO_FINAL_CFLAGS) $(c_flags) > > \ > > LTO_FINAL_CFLAGS is not defined anywhere. I guess you wanted to use > LTO_FINAL_LDFLAGS? THX, I forgot to change it here. We will see if this variable is needed (since it only contains -fwhole-program, which may be dropped) > > > + $(KBUILD_LDFLAGS:%=-Wl,%) $(LDFLAGS_u-boot:%=-Wl,%) -o $@ > > \ > > + -T u-boot.lds $(u-boot-init) > > \ > > + -Wl,--start-group -Wl,--whole-archive > > \ > > --start-group should be dropped Will test
Re: [PATCH u-boot 16/39] build: use thin archives instead of incremental linking
On Tue, 9 Mar 2021 21:00:00 +0800 Bin Meng wrote: > > --start-group is useless now. > > > $(u-boot-main) > > \ > > - --end-group > > \ > > + --no-whole-archive --end-group > > \ > > and --end-group > I will test this > > - rm -f $@; $(AR) rcs$(KBUILD_ARFLAGS) $@) > > + rm -f $@; $(AR) cDPrsT $@ $(filter $(obj-y), $^), \ > > + rm -f $@; $(AR) cPrsT$(KBUILD_ARFLAGS) $@) > > nits: should we use D for the empty one for consistency? OK > > > > $(builtin-target): $(obj-y) FORCE > > $(call if_changed,link_o_target) > > @@ -362,7 +361,7 @@ $(modorder-target): $(subdir-ym) FORCE > > # > > ifdef lib-target > > quiet_cmd_link_l_target = AR $@ > > -cmd_link_l_target = rm -f $@; $(AR) rcs$(KBUILD_ARFLAGS) $@ $(lib-y) > > +cmd_link_l_target = rm -f $@; $(AR) cPrs$(KBUILD_ARFLAGS) $@ $(lib-y) > > It looks this line change is not needed Hmm. I will look into this, maybe I added it just for consistency. > Otherwise LGTM: > Reviewed-by: Bin Meng THX
Re: [PATCH u-boot 16/39] build: use thin archives instead of incremental linking
On Tue, 9 Mar 2021 11:42:08 +0800 Bin Meng wrote: > Hi Marek, > > On Tue, Mar 9, 2021 at 9:24 AM Bin Meng wrote: > > > > Hi Marek, > > > > On Mon, Mar 8, 2021 at 11:22 PM Marek Behún wrote: > > > > > > On Mon, 8 Mar 2021 22:30:17 +0800 > > > Bin Meng wrote: > > > > > > > Hi Marek, > > > > > > > > On Mon, Mar 8, 2021 at 9:24 PM Marek Behún wrote: > > > > > > > > > > On Mon, 8 Mar 2021 19:32:10 +0800 > > > > > Bin Meng wrote: > > > > > > > > > > > On Mon, Mar 8, 2021 at 7:18 PM Marek Behun > > > > > > wrote: > > > > > > > > > > > > > > On Mon, 8 Mar 2021 18:44:58 +0800 > > > > > > > Bin Meng wrote: > > > > > > > > > > > > > > > Could you investigate why? > > > > > > > > > > > > > > I could, but I don't understand why exactly I should > > > > > > > - Linux is also using --whole-archive > > > > > > > - it is working > > > > > > > - I have other things I would like to work on > > > > > > > > > > > > > > Maybe you could look into this? :) > > > > > > > > > > > > Yes, I can look into this. I wonder if you already knew this which > > > > > > could save some time as this is a normal review process, asking > > > > > > for clarifications if something isn't clear. > > > > > > > > > > Bin, CI is failing without the --whole-archive option. > > > > > > > > > > What is interesting is that the binaries build successfully, but > > > > > testing them fails :) > > > > > > > > > > You can try this (with and without the --whole-archive options) > > > > > (note that this is without LTO) > > > > > make qemu_arm_defconfig > > > > > CROSS_COMPILE=arm-compiler- make -j8 > > > > > qemu-system-arm -M virt -nographic \ > > > > > -netdev user,id=net0,tftp=$(pwd) \ > > > > > -device e1000,netdev=net0 -device virtio-rng-pci \ > > > > > -bios u-boot.bin -serial mon:stdio > > > > > > > > > > With --whole-archive option this boots successfully into U-Boot, > > > > > without --whole-archive it just hangs. > > > > > > > > Thanks for reporting. My initnial build testing on qemu_arm_defconfig > > > > with this series succeeded but there are some warnings: > > > > > > > > /opt/armv7-linux/bin/arm-linux-ld.bfd: > > > > lib/efi_selftest/efi_selftest_miniapp_exception.o: plugin needed to > > > > handle lto object > > > > /opt/armv7-linux/bin/arm-linux-ld.bfd: > > > > examples/standalone/hello_world.o: plugin needed to handle lto object > > > > /opt/armv7-linux/bin/arm-linux-ld.bfd: examples/standalone/libstubs.o: > > > > plugin needed to handle lto object > > > > /opt/armv7-linux/bin/arm-linux-ld.bfd: warning: cannot find entry > > > > symbol hello_world; defaulting to 0c10 > > > > > > > > It looks we should update the make rules to remove "-flto" for these > > > > targets. > > > > > > > > Applying the following diff to remove --whole-archive, I got a build > > > > error: > > > > > > > > diff --git a/Makefile b/Makefile > > > > index 0f538270d7..127630e060 100644 > > > > --- a/Makefile > > > > +++ b/Makefile > > > > @@ -1780,7 +1780,7 @@ quiet_cmd_u-boot__ ?= LTO $@ > > > > $(LTO_FINAL_CFLAGS) $(c_flags) > > > > \ > > > > $(KBUILD_LDFLAGS:%=-Wl,%) $(LDFLAGS_u-boot:%=-Wl,%) -o > > > > $@ \ > > > > -T u-boot.lds $(u-boot-init) > > > > \ > > > > - -Wl,--start-group -Wl,--whole-archive > > > > \ > > > > + -Wl,--start-group > > > > \ > > > > $(u-boot-main) > > > > \ > > > > $(PLATFORM_LIBS) > > > > \ > > > > -Wl,--no-whole-archive -Wl,--end-group > > > > \ > > > > @@ -1790,9 +1790,9 @@ else > > &
Re: [PATCH u-boot 16/39] build: use thin archives instead of incremental linking
On Mon, 8 Mar 2021 19:32:10 +0800 Bin Meng wrote: > On Mon, Mar 8, 2021 at 7:18 PM Marek Behun wrote: > > > > On Mon, 8 Mar 2021 18:44:58 +0800 > > Bin Meng wrote: > > > > > Could you investigate why? > > > > I could, but I don't understand why exactly I should > > - Linux is also using --whole-archive > > - it is working > > - I have other things I would like to work on > > > > Maybe you could look into this? :) > > Yes, I can look into this. I wonder if you already knew this which > could save some time as this is a normal review process, asking for > clarifications if something isn't clear. If I knew what? Whether it is used in Linux also? To be honest I wasn't sure, I looked into Linux' sources before replying. The thing is, when I first worked on these patches, I just started with some old LTO patches for Linux (which were not even merged then), and played with the options until LTO worked for sandbox. Then the things started to get complicated and I rebased the series many times, so I did not remember which came from where. Marek
Re: [PATCH u-boot 12/39] string: make memcpy() and memset() visible to fix LTO linking errors
On Mon, 8 Mar 2021 11:55:32 +0100
Pali Rohár wrote:
> On Monday 08 March 2021 18:40:58 Bin Meng wrote:
> > On Mon, Mar 8, 2021 at 6:23 PM Pali Rohár wrote:
> > >
> > > On Monday 08 March 2021 11:19:33 Marek Behun wrote:
> > > > On Mon, 8 Mar 2021 15:56:01 +0800
> > > > Bin Meng wrote:
> > > >
> > > > > Hi Marek,
> > > > >
> > > > > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún
> > > > > wrote:
> > > > > >
> > > > > > It seems that sometimes (happening on ARM64, for example with
> > > > > > turris_mox_defconfig) GCC, when linking with LTO, changes the symbol
> > > > > > names of some functions, for example lib/string.c's memcpy()
> > > > > > function to
> > > > > > memcpy.isra.0.
> > > > > >
> > > > > > This is a problem however when GCC for a code such as this:
> > > > > > struct some_struct *info = get_some_struct();
> > > > > > struct some struct tmpinfo;
> > > > > > tmpinfo = *info;
> > > > > > emits a call to memcpy() by builtin behaviour, to copy *info to
> > > > > > tmpinfo.
> > > > > > memset() can be generated sometimes as well.
> > > > > >
> > > > > > This then results in the following linking error:
> > > > > > .../lz4.c:93: undefined reference to `memcpy'
> > > > > > .../uuid.c:206: more undefined references to `memcpy' follow
> > > > > >
> > > > > > Make memcpy() and memset() visible by using the __used macro to
> > > > > > avoid
> > > > > > this error.
> > > > >
> > > > > This sounds like a GCC bug of using -fno-builtin and -flto. Could you
> > > > > file a bugzilla to GCC people to get some comments?
> > > >
> > > > This is not LTO related. -fno-builtin still generates memcpy() call for
> > > > the following code:
> > > >
> > > > typedef struct {
> > > > int a[40];
> > > > char b[50];
> > > > int c[60];
> > > > } a;
> > > >
> > > > void cp(a *d, const a *s) {
> > > > *d = *s;
> > > > }
> > > >
> > > > when compiled with
> > > > armv7a-hardfloat-linux-gnueabi-gcc -O2 -fno-builtin -ffreestanding \
> > > >-nostdlib -S
> > > >
> > > > it produces code
> > > >
> > > > push{r4, lr}
> > > > mov ip, #4096
> > > > sub ip, sp, ip
> > > > str r0, [ip, #4088]
> > > > mov r2, #452
> > > > bl memcpy
> > > > pop {r4, pc}
> > > > .size cp, .-cp
> > > >
> > > > I don't think this is a bug. Or if it is, it is a wontfix. Just
> > > > implement memcpy into your code, so that gcc does not have to emit
> > > > shitstorms of instructions because of assignment operator :)
> > > >
> > > > Marek
> > >
> > > This is not a bug but rather a feature of gcc. Documentation for
> > > -nodefaultlibs and -nostdlib contains:
> > >
> > > The compiler may generate calls to "memcmp", "memset", "memcpy" and
> > > "memmove". These entries are usually resolved by entries in libc.
> > > These entry points should be supplied through some other mechanism when
> > > this option is specified.
> >
> > Yeah I know this. My question was why when LTO is enabled, we have to
> > explicitly mark these APIs as __used? I should have asked clearly, is
> > this a bug of LTO?
>
> I see... What is the _exact_ command line arguments called when this
> linking issue happen? I know that when gcc's LTO is enabled it depends
> on other of objects and libraries which are linking. And I'm not sure if
> all -whole-archive option can make order of archives independent when
> dealing with these builtin function references. At least I cannot find
> exact gcc documentation about linking order.
Soo, it seems that the compiler generates a copy of memcpy called
memcpy.isra.0 which is a version with -fipa-sra optimization.
Then it thinks the original memcpy is not used, since it replaces all
calls to memcpy.isra.0, but the original memcpy is still being called
for assignment operator. So maybe it is a bug of gcc, and gcc should
use memcpy.isra.0 for the assignment operator as well...
Re: [PATCH u-boot 12/39] string: make memcpy() and memset() visible to fix LTO linking errors
On Mon, 8 Mar 2021 11:55:32 +0100
Pali Rohár wrote:
> On Monday 08 March 2021 18:40:58 Bin Meng wrote:
> > On Mon, Mar 8, 2021 at 6:23 PM Pali Rohár wrote:
> > >
> > > On Monday 08 March 2021 11:19:33 Marek Behun wrote:
> > > > On Mon, 8 Mar 2021 15:56:01 +0800
> > > > Bin Meng wrote:
> > > >
> > > > > Hi Marek,
> > > > >
> > > > > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún
> > > > > wrote:
> > > > > >
> > > > > > It seems that sometimes (happening on ARM64, for example with
> > > > > > turris_mox_defconfig) GCC, when linking with LTO, changes the symbol
> > > > > > names of some functions, for example lib/string.c's memcpy()
> > > > > > function to
> > > > > > memcpy.isra.0.
> > > > > >
> > > > > > This is a problem however when GCC for a code such as this:
> > > > > > struct some_struct *info = get_some_struct();
> > > > > > struct some struct tmpinfo;
> > > > > > tmpinfo = *info;
> > > > > > emits a call to memcpy() by builtin behaviour, to copy *info to
> > > > > > tmpinfo.
> > > > > > memset() can be generated sometimes as well.
> > > > > >
> > > > > > This then results in the following linking error:
> > > > > > .../lz4.c:93: undefined reference to `memcpy'
> > > > > > .../uuid.c:206: more undefined references to `memcpy' follow
> > > > > >
> > > > > > Make memcpy() and memset() visible by using the __used macro to
> > > > > > avoid
> > > > > > this error.
> > > > >
> > > > > This sounds like a GCC bug of using -fno-builtin and -flto. Could you
> > > > > file a bugzilla to GCC people to get some comments?
> > > >
> > > > This is not LTO related. -fno-builtin still generates memcpy() call for
> > > > the following code:
> > > >
> > > > typedef struct {
> > > > int a[40];
> > > > char b[50];
> > > > int c[60];
> > > > } a;
> > > >
> > > > void cp(a *d, const a *s) {
> > > > *d = *s;
> > > > }
> > > >
> > > > when compiled with
> > > > armv7a-hardfloat-linux-gnueabi-gcc -O2 -fno-builtin -ffreestanding \
> > > >-nostdlib -S
> > > >
> > > > it produces code
> > > >
> > > > push{r4, lr}
> > > > mov ip, #4096
> > > > sub ip, sp, ip
> > > > str r0, [ip, #4088]
> > > > mov r2, #452
> > > > bl memcpy
> > > > pop {r4, pc}
> > > > .size cp, .-cp
> > > >
> > > > I don't think this is a bug. Or if it is, it is a wontfix. Just
> > > > implement memcpy into your code, so that gcc does not have to emit
> > > > shitstorms of instructions because of assignment operator :)
> > > >
> > > > Marek
> > >
> > > This is not a bug but rather a feature of gcc. Documentation for
> > > -nodefaultlibs and -nostdlib contains:
> > >
> > > The compiler may generate calls to "memcmp", "memset", "memcpy" and
> > > "memmove". These entries are usually resolved by entries in libc.
> > > These entry points should be supplied through some other mechanism when
> > > this option is specified.
> >
> > Yeah I know this. My question was why when LTO is enabled, we have to
> > explicitly mark these APIs as __used? I should have asked clearly, is
> > this a bug of LTO?
>
> I see... What is the _exact_ command line arguments called when this
> linking issue happen? I know that when gcc's LTO is enabled it depends
> on other of objects and libraries which are linking. And I'm not sure if
> all -whole-archive option can make order of archives independent when
> dealing with these builtin function references. At least I cannot find
> exact gcc documentation about linking order.
remove the __used flag for memcpy, add -save-temps to the final linking
command in order not to delete ltrans assembly files,
compile for turris_mox_defconfig with V=1
the final linking fails and you can study the assembly file it
generated
Re: [PATCH u-boot 16/39] build: use thin archives instead of incremental linking
On Mon, 8 Mar 2021 18:44:58 +0800 Bin Meng wrote: > Could you investigate why? I could, but I don't understand why exactly I should - Linux is also using --whole-archive - it is working - I have other things I would like to work on Maybe you could look into this? :) Marek
Re: [PATCH u-boot 11/39] binman: declare symbols externally visible
On Mon, 8 Mar 2021 18:31:26 +0800 Bin Meng wrote: > On Mon, Mar 8, 2021 at 5:26 PM Marek Behun wrote: > > > > On Mon, 8 Mar 2021 15:47:38 +0800 > > Bin Meng wrote: > > > > > Hi Marek, > > > > > > On Sun, Mar 7, 2021 at 12:59 PM Marek Behun wrote: > > > > > > > > I forgot to drop this patch. It is not needed, please ignore it. > > > > > > > > > > Why is this not needed anymore? > > > > It was not needed at all, binman does not fail when its symbols are not > > explicitly declared to be visible. > > Then why was it needed in the previous patch? Or is this due to > undeterminism of LTO? It wasn't needed. I did not test binman then, I just grepped for all occurances of __attribute__, found occurances where it looked like it would break LTO and applied, just to be sure.
Re: [PATCH u-boot 07/39] compiler.h: align the __ADDRESSABLE macro with Linux' version
On Mon, 8 Mar 2021 18:29:29 +0800
Bin Meng wrote:
> On Mon, Mar 8, 2021 at 5:23 PM Marek Behun wrote:
> >
> > On Mon, 8 Mar 2021 15:27:41 +0800
> > Bin Meng wrote:
> >
> > > > #define __ADDRESSABLE(sym) \
> > > > static void * __section(".discard.addressable") __used \
> > > > - __PASTE(__addressable_##sym, __LINE__) = (void *)
> > > > + __UNIQUE_ID(__PASTE(__addressable_,sym)) = (void
> > > > *)
> > >
> > > nits: need one space after ,
> >
> > This is copy-paster from Linux, so it should be first fixed there.
>
> Do you mean this whole file is copy-paster from Linux? Is this the
> only place that U-Boot's version is different? If not, probably we
> need to do a sync with the Linux one.
Currently it is different from Linux's version, but yes, this and other
files are periodically synced with Linux' version.
I am not going to do a whole sync of all these headers in this series,
though.
Marek
Re: [PATCH u-boot 12/39] string: make memcpy() and memset() visible to fix LTO linking errors
On Mon, 8 Mar 2021 15:56:01 +0800
Bin Meng wrote:
> Hi Marek,
>
> On Sun, Mar 7, 2021 at 12:26 PM Marek Behún wrote:
> >
> > It seems that sometimes (happening on ARM64, for example with
> > turris_mox_defconfig) GCC, when linking with LTO, changes the symbol
> > names of some functions, for example lib/string.c's memcpy() function to
> > memcpy.isra.0.
> >
> > This is a problem however when GCC for a code such as this:
> > struct some_struct *info = get_some_struct();
> > struct some struct tmpinfo;
> > tmpinfo = *info;
> > emits a call to memcpy() by builtin behaviour, to copy *info to tmpinfo.
> > memset() can be generated sometimes as well.
> >
> > This then results in the following linking error:
> > .../lz4.c:93: undefined reference to `memcpy'
> > .../uuid.c:206: more undefined references to `memcpy' follow
> >
> > Make memcpy() and memset() visible by using the __used macro to avoid
> > this error.
>
> This sounds like a GCC bug of using -fno-builtin and -flto. Could you
> file a bugzilla to GCC people to get some comments?
This is not LTO related. -fno-builtin still generates memcpy() call for
the following code:
typedef struct {
int a[40];
char b[50];
int c[60];
} a;
void cp(a *d, const a *s) {
*d = *s;
}
when compiled with
armv7a-hardfloat-linux-gnueabi-gcc -O2 -fno-builtin -ffreestanding \
-nostdlib -S
it produces code
push{r4, lr}
mov ip, #4096
sub ip, sp, ip
str r0, [ip, #4088]
mov r2, #452
bl memcpy
pop {r4, pc}
.size cp, .-cp
I don't think this is a bug. Or if it is, it is a wontfix. Just
implement memcpy into your code, so that gcc does not have to emit
shitstorms of instructions because of assignment operator :)
Marek
Re: [PATCH u-boot 16/39] build: use thin archives instead of incremental linking
On Mon, 8 Mar 2021 17:16:03 +0800 Bin Meng wrote: > Hi Marek, > > On Sun, Mar 7, 2021 at 12:26 PM Marek Behún wrote: > > > > Currently we use incremental linking (ld -r) to link several object > > files from one directory into one built-in.o object file containing the > > linked code from that directory (and its subdirectories). > > > > Linux has, some time ago, moved to thin archives instead. > > > > Thin archives are archives (.a) that do not really contain the object > > files, only references to them. > > > > Using thin archives instead of incremental linking > > - saves disk space > > - apparently works better with dead code elimination > > - makes things easier for LTO > > > > The third point is the important one for us. With incremental linking > > there are several options how to do LTO, and that would unnecessarily > > complicate things. > > > > On the other hand, by using thin archives we can make (via the > > --whole-archive use flag) the final linking behave as if we passed all > > the object files from the archives to the linking program as arguments. > > I don't think --whole-archive is required for LTO to work. It is. Linking fails if it is not used, for example for nokia_rx51_defconfig. > Switching to --whole-archive should be made conditionally when LTO is on, > otherwise for targets that don't have > -ffunction-sections/data-sections/--gc-sections specified, it will > create unnecessary bloat. OK I will push into CI without this flag for non-LTO and if it passes all tests I shall remove this flag for non-LTO. Marek
Re: [PATCH u-boot 11/39] binman: declare symbols externally visible
On Mon, 8 Mar 2021 15:47:38 +0800 Bin Meng wrote: > Hi Marek, > > On Sun, Mar 7, 2021 at 12:59 PM Marek Behun wrote: > > > > I forgot to drop this patch. It is not needed, please ignore it. > > > > Why is this not needed anymore? It was not needed at all, binman does not fail when its symbols are not explicitly declared to be visible.
Re: [PATCH u-boot 07/39] compiler.h: align the __ADDRESSABLE macro with Linux' version
On Mon, 8 Mar 2021 15:27:41 +0800
Bin Meng wrote:
> > #define __ADDRESSABLE(sym) \
> > static void * __section(".discard.addressable") __used \
> > - __PASTE(__addressable_##sym, __LINE__) = (void *)
> > + __UNIQUE_ID(__PASTE(__addressable_,sym)) = (void *)
>
> nits: need one space after ,
This is copy-paster from Linux, so it should be first fixed there.
Re: [PATCH u-boot 04/39] api: fix a potential serious bug caused by undef CONFIG_SYS_64BIT_LBA
On Mon, 8 Mar 2021 15:09:51 +0800
Bin Meng wrote:
> Hi Marek,
>
> On Sun, Mar 7, 2021 at 12:26 PM Marek Behún wrote:
> >
> > The api_public.h header file undefined macro CONFIG_SYS_64BIT_LBA.
> >
> > But api/api_storage.c includes this header before including part.h,
> > causing the type of lbaint_t and subsequently the type signature of
> > blk_dread() and blk_dwrite() functions to change from the rest of U-Boot
> > (if CONFIG_SYS_64BIT_LBA is defined for the board).
> >
> > This is of course wrong, because the call to blk_dread() / blk_dwrite()
> > will recieve mangled arguments.
>
> typo: receive
>
> >
> > Fix this by removing the undef of macro CONFIG_SYS_64BIT_LBA and instead
> > make the immediate code do what it would do as if the macro was not
> > defined.
> >
> > Add a FIXME to whoever is maintaining this code.
> >
> > CI managed to trigger this bug when compiling for lsxhl_defconfig, which
> > has CONFIG_API selected. The compiler complained about blk_dwrite() and
> > blk_dread() not matching original declarations:
> >
> > include/blk.h:280:15: warning: type of ‘blk_dwrite’ does not match
> > original declaration
> > [-Wlto-type-mismatch]
> > 280 | unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t st
> > | ^
> > drivers/block/blk-uclass.c:456:15: note: type mismatch in parameter 2
> > 456 | unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t st
> > | ^
> >
> > Signed-off-by: Marek Behún
> > ---
> > include/api_public.h | 23 ++-
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/api_public.h b/include/api_public.h
> > index def103ce22..5a4465ea89 100644
> > --- a/include/api_public.h
> > +++ b/include/api_public.h
> > @@ -70,12 +70,25 @@ struct sys_info {
> > int mr_no; /* number of memory regions */
> > };
> >
> > -#undef CONFIG_SYS_64BIT_LBA
> > -#ifdef CONFIG_SYS_64BIT_LBA
> > -typedefu_int64_t lbasize_t;
> > -#else
> > +/*
> > + * FIXME: Previously this code was:
> > + *
> > + * #undef CONFIG_SYS_64BIT_LBA
> > + * #ifdef CONFIG_SYS_64BIT_LBA
> > + * typedef u_int64_t lbasize_t;
> > + * #else
> > + * typedef unsigned long lbasize_t;
> > + * #endif
> > + *
> > + * But we cannot just undefine CONFIG_SYS_64BIT_LBA, because then in
> > + * api/api_storage.c the type signature of lbaint_t will be different if
> > + * CONFIG_SYS_64BIT_LBA is enabled for the board, which can result in
> > various
> > + * bugs.
> > + * So simply define lbasize_t as an unsigned long, since this was what was
> > done
> > + * anyway for at least 13 years, but don't undefine CONFIG_SYS_64BIT_LBA.
> > + */
> > typedef unsigned long lbasize_t;
>
> Does "typedef lbaint_t labsize_t" help?
That could break API applications...
Re: [PATCH u-boot-marvell 02/39] ddr: marvell: axp: fix array types have different bounds warning
On Mon, 8 Mar 2021 07:58:30 +0100 Stefan Roese wrote: > Hi Marek, > > On 08.03.21 07:46, Marek Behun wrote: > > And BTW do you have time to test this series on some ARM boards? > > I can test this on the theadorable Armada XP board, which also uses > SPL. Do you have a git repo I should use? Or is the patch series from > yesterday the "latest and greatest"? > > Thanks, > Stefan https://github.com/elkablo/u-boot branch lto but you need to also add the last patch from this series on the mailing list, which enables LTO for all ARM boards, or enable CONFIG_LTO=y manually after defconfig Marek
Re: [PATCH u-boot-marvell 02/39] ddr: marvell: axp: fix array types have different bounds warning
And BTW do you have time to test this series on some ARM boards?
Re: [PATCH u-boot-marvell 02/39] ddr: marvell: axp: fix array types have different bounds warning
> Reviewed-by: Stefan Roese Thanks, Stefan. Do you want to merge this into your repo u-boot-marvell, or shall Tom merge this once this series is mature? Marek
Re: [PATCH u-boot 14/39] lib: crc32: put the crc_table variable into efi_runtime_rodata section
On Sun, 7 Mar 2021 14:04:35 +0100 Marek Behun wrote: > Hmm, maybe this should be split into 2 commits. I have divided this into 2 commits and pushed into my github branch
Re: [PATCH u-boot 38/39] ARM: enable LTO for some boards
On Sun, 7 Mar 2021 10:14:07 -0600 Adam Ford wrote: > On Sat, Mar 6, 2021 at 10:26 PM Marek Behún wrote: > > > > Enable LTO for some boards that were tested by people on U-Boot Mailing > > List. > > > > I have also confirmed this works on the r8a774a1_beacon board as well > and boots without issues. The r8a774b1_beacon and r8a774e1_beacon > should be safe since the only real difference between them is the > device tree reference. pushed into my github branch
Re: [PATCH u-boot 14/39] lib: crc32: put the crc_table variable into efi_runtime_rodata section
Hmm, maybe this should be split into 2 commits.
Re: [PATCH u-boot 14/39] lib: crc32: make the crc_table variable non-const
On Sun, 7 Mar 2021 13:31:11 +0100 Pali Rohár wrote: > On Sunday 07 March 2021 13:26:36 Marek Behun wrote: > > On Sun, 7 Mar 2021 06:02:16 +0100 > > Marek Vasut wrote: > > > > > On 3/7/21 5:58 AM, Marek Behun wrote: > > > > On Sun, 7 Mar 2021 05:46:24 +0100 > > > > Marek Vasut wrote: > > > > > > > >> On 3/7/21 5:25 AM, Marek Behún wrote: > > > >>> When compiling with LTO, the compiler fails with an error saying that > > > >>> `crc_table` causes a section type conflict with `efi_var_buf`. > > > >>> > > > >>> This is because both are declared to be in the same section (via macro > > > >>> `__efi_runtime_data`), but one is const while the other is not. > > > >>> > > > >>> Make this variable non-const in order to fix this. > > > >> > > > >> This does not look right, the crc32 array is constant. > > > >> Maybe what you want to do instead if create some __efi_constant_data > > > >> section ? > > > > > > > > Yes, this was the easier solution, and maybe is not ideal. > > > > > > > > I thought it would not be much of a problem since this array can be > > > > nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is > > > > enabled. > > > > > > > > Anyway I don't much understand the EFI code so I wanted to poke into it > > > > as little as possible. > > > > > > Isn't the compiler capable of better optimization on constant stuff ? > > > That's pretty much what prompted my suggestion to add separate section. > > > > Yes, but > > - for this case I don't think the compiler actually can do any > > significat optimizations when declaring the table const, since it has > > to access > > tab[(crc ^ (x)) & 255] > > I tried with arm compiler just now, with -O2 and -Os, and it > > generated the same code either way > > - when using LTO, the compiler theoretically should be able to deduce > > that the table is never written to > > > > But if people insist on declaring the table const, I will look into > > this... > > If you try to overwrite a const variable from the same code unit then > compiler throw an error. So declaring read-only variable as a const > could prevent people to unintentionally overwrite read-only variable. > And detect possible bad code. OK it seems all linker scripts also mention .rodata.efi_runtime*, not just .data.efi_runtime*. I shall put it into the .rodata.efi_runtime section. Marek
Re: [PATCH u-boot 14/39] lib: crc32: make the crc_table variable non-const
On Sun, 7 Mar 2021 06:02:16 +0100 Marek Vasut wrote: > On 3/7/21 5:58 AM, Marek Behun wrote: > > On Sun, 7 Mar 2021 05:46:24 +0100 > > Marek Vasut wrote: > > > >> On 3/7/21 5:25 AM, Marek Behún wrote: > >>> When compiling with LTO, the compiler fails with an error saying that > >>> `crc_table` causes a section type conflict with `efi_var_buf`. > >>> > >>> This is because both are declared to be in the same section (via macro > >>> `__efi_runtime_data`), but one is const while the other is not. > >>> > >>> Make this variable non-const in order to fix this. > >> > >> This does not look right, the crc32 array is constant. > >> Maybe what you want to do instead if create some __efi_constant_data > >> section ? > > > > Yes, this was the easier solution, and maybe is not ideal. > > > > I thought it would not be much of a problem since this array can be > > nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is > > enabled. > > > > Anyway I don't much understand the EFI code so I wanted to poke into it > > as little as possible. > > Isn't the compiler capable of better optimization on constant stuff ? > That's pretty much what prompted my suggestion to add separate section. Yes, but - for this case I don't think the compiler actually can do any significat optimizations when declaring the table const, since it has to access tab[(crc ^ (x)) & 255] I tried with arm compiler just now, with -O2 and -Os, and it generated the same code either way - when using LTO, the compiler theoretically should be able to deduce that the table is never written to But if people insist on declaring the table const, I will look into this...
Re: [PATCH u-boot 30/39] ARM: imx6m: fix imx_eqos_txclk_set_rate() type mismatch for LTO
THX
Re: [PATCH u-boot 11/39] binman: declare symbols externally visible
I forgot to drop this patch. It is not needed, please ignore it. Marek
Re: [PATCH u-boot 14/39] lib: crc32: make the crc_table variable non-const
On Sun, 7 Mar 2021 05:46:24 +0100 Marek Vasut wrote: > On 3/7/21 5:25 AM, Marek Behún wrote: > > When compiling with LTO, the compiler fails with an error saying that > > `crc_table` causes a section type conflict with `efi_var_buf`. > > > > This is because both are declared to be in the same section (via macro > > `__efi_runtime_data`), but one is const while the other is not. > > > > Make this variable non-const in order to fix this. > > This does not look right, the crc32 array is constant. > Maybe what you want to do instead if create some __efi_constant_data > section ? Yes, this was the easier solution, and maybe is not ideal. I thought it would not be much of a problem since this array can be nonconstant (generated after boot) if CONFIG_DYNAMIC_CRC_TABLE is enabled. Anyway I don't much understand the EFI code so I wanted to poke into it as little as possible. Marek
Re: [PATCH u-boot 05/39] checkpatch: require quotes around section name in the __section() macro
On Sun, 7 Mar 2021 05:47:56 +0100
Marek Vasut wrote:
> On 3/7/21 5:25 AM, Marek Behún wrote:
> > This is how Linux does this now, see Linux commit 339f29d91acf.
> >
> > Signed-off-by: Marek Behún
> > ---
> > scripts/checkpatch.pl | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 755f4802a4..fd1e9c4d24 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -6065,7 +6065,7 @@ sub process {
> > my $old = substr($rawline, $-[1], $+[1] - $-[1]);
> > my $new = substr($old, 1, -1);
> > if (WARN("PREFER_SECTION",
> > -"__section($new) is preferred over
> > __attribute__((section($old)))\n" . $herecurr) &&
> > +"__section(\"$new\") is preferred over
> > __attribute__((section($old)))\n" . $herecurr) &&
> > $fix) {
> > $fixed[$fixlinenr] =~
> > s/\b__attribute__\s*\(\s*\(\s*_*section_*\s*\(\s*\Q$old\E\s*\)\s*\)\s*\)/__section($new)/;
> >
>
> Shouldn't some of the patches which are clearly fixes be sent as
> separate fixes, so they can be picked while the LTO support is being
> worked on ?
Yes, ideally it would be better, but:
this patch is connected to patch 6 of this series, and patch 6 needs to
be in this series because otherwise people trying to apply this series
would get an error.
The first 4 patches are also fixes for something else, but they
were discovered thanks to LTO and without them users will get
warnings/errors when trying to build for some boards.
Tom, should I send these patches separately? Also the first 3 patches
should maybe be applied via Stefan and Simon, via their trees...
Marek
Re: [RFC PATCH u-boot 00/12] U-Boot LTO (Sandbox + ARM Nokia RX-51)
On Sat, 6 Mar 2021 21:45:02 -0600 Adam Ford wrote: > On Sat, Mar 6, 2021 at 3:49 PM Marek Behun wrote: > > > > On Sat, 6 Mar 2021 22:38:52 +0100 > > Pali Rohár wrote: > > > > > On Saturday 06 March 2021 22:19:22 Marek Behun wrote: > > > > On Sat, 6 Mar 2021 22:00:45 +0100 > > > > Pali Rohár wrote: > > > > > > > > > On Saturday 06 March 2021 21:54:00 Marek Behun wrote: > > > > > > On Sat, 6 Mar 2021 21:41:14 +0100 > > > > > > Pali Rohár wrote: > > > > > > > > > > > > > On Saturday 06 March 2021 15:08:13 Tom Rini wrote: > > > > > > > > Perhaps we'll default to yes on some SoCs. The omap3 thing is > > > > > > > > a bit > > > > > > > > odd, but we'll see what happens on real N900 hardware. > > > > > > > > > > > > > > Hello! > > > > > > > > > > > > > > Could you send me a link to git repo / branch and tell me from > > > > > > > which > > > > > > > commit should I do tests on real N900 hardware? I will test it > > > > > > > and let > > > > > > > you know results. > > > > > > > > > > > > > > Adding maemo ML to the loop as on the maemo list are more people > > > > > > > with > > > > > > > N900 HW and U-Boot. > > > > > > > > > > > > https://github.com/elkablo/u-boot branch lto > > > > > > > > > > Sorry, compilation is failing :-( > > > > > > > > > > $ git clone https://github.com/elkablo/u-boot -b lto --depth=100 > > > > > Cloning into 'u-boot'... > > > > > remote: Enumerating objects: 33644, done. > > > > > remote: Counting objects: 100% (33644/33644), done. > > > > > remote: Compressing objects: 100% (20116/20116), done. > > > > > remote: Total 33644 (delta 15838), reused 19947 (delta 13018), > > > > > pack-reused 0 > > > > > Receiving objects: 100% (33644/33644), 26.28 MiB | 10.21 MiB/s, done. > > > > > Resolving deltas: 100% (15838/15838), done. > > > > > > > > > > $ cd u-boot > > > > > > > > > > $ make CROSS_COMPILE=arm-linux-gnueabi- nokia_rx51_config > > > > > HOSTCC scripts/basic/fixdep > > > > > HOSTCC scripts/kconfig/conf.o > > > > > YACCscripts/kconfig/zconf.tab.c > > > > > LEX scripts/kconfig/zconf.lex.c > > > > > HOSTCC scripts/kconfig/zconf.tab.o > > > > > HOSTLD scripts/kconfig/conf > > > > > # > > > > > # configuration written to .config > > > > > # > > > > > > > > > > $ make CROSS_COMPILE=arm-linux-gnueabi- u-boot.bin > > > > > ... > > > > > LTO u-boot > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > DWARF error: offset (1258291444) greater than or equal to .debug_str > > > > > size (676) > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > DWARF error: offset (1459618036) greater than or equal to .debug_str > > > > > size (676) > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > DWARF error: could not find abbrev number 48028 > > > > > /tmp/cc8l0QSQ.ltrans3.ltrans.o: in function `omap3_set_aux_cr_secure': > > > > > :(.text+0x6eb8): undefined reference to > > > > > `do_omap3_emu_romcode_call' > > > > > collect2: error: ld returned 1 exit status > > > > > make: *** [Makefile:1808: u-boot] Error 1 > > > > > > > > > > > > > > > I'm using arm-linux-gnueabi-gcc version 8.3.0 which is available in > > > > > current Debian stable (Debian 10 Buster). > > > > > > > > Fixed and force-pushed, it seems ar needs the P flag that Bin Meng > > > > questioned. > > > > > > Problem is fixed, now compilation succeeded. u-boot.bin has size 243788 > > > bytes. > > > > > > And seems that compiled U-Boot is working fin
Re: [RFC PATCH u-boot 02/12] sandbox: errno: avoid conflict with libc's errno
On Fri, 5 Mar 2021 18:21:51 +0100 Heinrich Schuchardt wrote: > On 05.03.21 16:37, Marek Behun wrote: > > On Fri, 5 Mar 2021 11:00:45 +0800 > > Bin Meng wrote: > > > >> On Wed, Mar 3, 2021 at 12:13 PM Marek Behún wrote: > >>> > >>> When building with LTO, the system libc's `errno` variable used in > >>> arch/sandbox/cpu/os.c conflicts with U-Boot's `errno` (defined in > >>> lib/errno.c) with the following error: > >>> .../ld: errno@@GLIBC_PRIVATE: TLS definition in /lib64/libc.so.6 > >>> section .tbss mismatches non-TLS reference in > >>> /tmp/u-boot.EQlEXz.ltrans0.ltrans.o > >> > >> Do you know if this is the expected behavior when enabling LTO on the > >> compiler? > > > > I don't, but this is a bug anyway. The symbol clashes with the symbol > > from glibc. Does somebody know whether the usage of this symbol in os.c > > does really use glibc's version or U-Boot's one? > > > > Hello Marek, > > Why do you resort to assembler in your patch instead of simply using: > > #define errno __uboot_errno > > to substitute the symbol? Meh. :D That would just make error messages from gcc more complicated, if suddenly the compiler spat out 2 more lines, saying "in expansion of macro...". I think that using attributes, static inline functions and everything else the compiler provides instead of macros is better. > Why explicitly set errno = 0? Globals are automatically initialized to zero. I just added the symbol renaming part, the = 0 assignment was already there. I don't think this commit should remove it. If we want that, we can make it in another commit. Marek
Re: U-Boot CI error, unable to reproduce locally
On Sat, 6 Mar 2021 21:11:34 -0500 Tom Rini wrote: > On Sun, Mar 07, 2021 at 02:27:33AM +0100, Marek Behun wrote: > > > Hello Tom, > > > > I seem to run into an error on Azure's CI for U-Boot that I cannot > > reproduce locally. > > > > It concerns my LTO work https://github.com/u-boot/u-boot/pull/57 > > > > The test > > u-boot.u-boot (test.py xilinx_zynq_virt) > > is failing with this log > > https://dev.azure.com/u-boot/a1096300-2999-4ec4-a21a-4c22075e3771/_apis/build/builds/1936/logs/283 > > > > The error seems to be > > make[3]: *** read jobs pipe: No such file or directory. Stop. > > lto-wrapper: fatal error: make returned 2 exit status > > as if make's jobserver died, or something. > > > > But when I build with buildman on my local machine, everything is OK. > > Seems like there's still some build races on fast machines? > Can this happen even if only 2 jobs are used? According to that log -j2 was used.
request for i.MX8MM Venice test
[sorry for the spam, I accidentally sent this e-mail from my personal address] Hello Tim, you are listed as maintainer of i.MX8MM Venice board in U-Boot. I am currently working on LTO support for U-Boot, and I have encountered a problem with i.MX8MM Venice board: when LTO is enabled, the linking process for SPL does not throw away relocation information, making the resulting SPL image too big for that board. I have added a patch that discards symbols from .rela* section to my patch series, but I would like you to test whether the patch series works for your board and does not break anything. Could you please clone https://github.com/elkablo/u-boot branch lto, build for imx8mm_venice_defconfig and test whether it boots on your board and maybe test some U-Boot commands (disk reads, kernel booting, ...)? If it does not work, could you also please check with current U-Boot master, to see if it got broken with my patches or with something different? Thank you. Marek
U-Boot CI error, unable to reproduce locally
Hello Tom, I seem to run into an error on Azure's CI for U-Boot that I cannot reproduce locally. It concerns my LTO work https://github.com/u-boot/u-boot/pull/57 The test u-boot.u-boot (test.py xilinx_zynq_virt) is failing with this log https://dev.azure.com/u-boot/a1096300-2999-4ec4-a21a-4c22075e3771/_apis/build/builds/1936/logs/283 The error seems to be make[3]: *** read jobs pipe: No such file or directory. Stop. lto-wrapper: fatal error: make returned 2 exit status as if make's jobserver died, or something. But when I build with buildman on my local machine, everything is OK. How should I proceed? Marek
request for i.MX8MM Venice test
Hello Tim, you are listed as maintainer of i.MX8MM Venice board in U-Boot. I am currently working on LTO support for U-Boot, and I have encountered a problem with i.MX8MM Venice board: when LTO is enabled, the linking process for SPL does not throw away relocation information, making the resulting SPL image too big for that board. I have added a patch that discards symbols from .rela* section to my patch series, but I would like you to test whether the patch series works for your board and does not break anything. Could you please clone https://github.com/elkablo/u-boot branch lto, build for imx8mm_venice_defconfig and test whether it boots on your board and maybe test some U-Boot commands (disk reads, kernel booting, ...)? If it does not work, could you also please check with current U-Boot master, to see if it got broken with my patches or with something different? Thank you. Marek
Re: [RFC PATCH u-boot 00/12] U-Boot LTO (Sandbox + ARM Nokia RX-51)
On Sat, 6 Mar 2021 22:38:52 +0100 Pali Rohár wrote: > On Saturday 06 March 2021 22:19:22 Marek Behun wrote: > > On Sat, 6 Mar 2021 22:00:45 +0100 > > Pali Rohár wrote: > > > > > On Saturday 06 March 2021 21:54:00 Marek Behun wrote: > > > > On Sat, 6 Mar 2021 21:41:14 +0100 > > > > Pali Rohár wrote: > > > > > > > > > On Saturday 06 March 2021 15:08:13 Tom Rini wrote: > > > > > > Perhaps we'll default to yes on some SoCs. The omap3 thing is a bit > > > > > > odd, but we'll see what happens on real N900 hardware. > > > > > > > > > > Hello! > > > > > > > > > > Could you send me a link to git repo / branch and tell me from which > > > > > commit should I do tests on real N900 hardware? I will test it and let > > > > > you know results. > > > > > > > > > > Adding maemo ML to the loop as on the maemo list are more people with > > > > > N900 HW and U-Boot. > > > > > > > > https://github.com/elkablo/u-boot branch lto > > > > > > Sorry, compilation is failing :-( > > > > > > $ git clone https://github.com/elkablo/u-boot -b lto --depth=100 > > > Cloning into 'u-boot'... > > > remote: Enumerating objects: 33644, done. > > > remote: Counting objects: 100% (33644/33644), done. > > > remote: Compressing objects: 100% (20116/20116), done. > > > remote: Total 33644 (delta 15838), reused 19947 (delta 13018), > > > pack-reused 0 > > > Receiving objects: 100% (33644/33644), 26.28 MiB | 10.21 MiB/s, done. > > > Resolving deltas: 100% (15838/15838), done. > > > > > > $ cd u-boot > > > > > > $ make CROSS_COMPILE=arm-linux-gnueabi- nokia_rx51_config > > > HOSTCC scripts/basic/fixdep > > > HOSTCC scripts/kconfig/conf.o > > > YACCscripts/kconfig/zconf.tab.c > > > LEX scripts/kconfig/zconf.lex.c > > > HOSTCC scripts/kconfig/zconf.tab.o > > > HOSTLD scripts/kconfig/conf > > > # > > > # configuration written to .config > > > # > > > > > > $ make CROSS_COMPILE=arm-linux-gnueabi- u-boot.bin > > > ... > > > LTO u-boot > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > DWARF error: offset (1258291444) greater than or equal to .debug_str > > > size (676) > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > DWARF error: offset (1459618036) greater than or equal to .debug_str > > > size (676) > > > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > > > DWARF error: could not find abbrev number 48028 > > > /tmp/cc8l0QSQ.ltrans3.ltrans.o: in function `omap3_set_aux_cr_secure': > > > :(.text+0x6eb8): undefined reference to > > > `do_omap3_emu_romcode_call' > > > collect2: error: ld returned 1 exit status > > > make: *** [Makefile:1808: u-boot] Error 1 > > > > > > > > > I'm using arm-linux-gnueabi-gcc version 8.3.0 which is available in > > > current Debian stable (Debian 10 Buster). > > > > Fixed and force-pushed, it seems ar needs the P flag that Bin Meng > > questioned. > > Problem is fixed, now compilation succeeded. u-boot.bin has size 243788 > bytes. > > And seems that compiled U-Boot is working fine! > > Nokia RX-51 # version > U-Boot 2021.04-rc3-00338-g88d0a5042c97 (Mar 06 2021 - 22:19:08 +0100) > > arm-linux-gnueabi-gcc (Debian 8.3.0-2) 8.3.0 > GNU ld (GNU Binutils for Debian) 2.31.1 > > I can send binary files via usbtty and 'loadb' command. I can boot linux > kernel via 'bootm'. I can chainload to another U-Boot binary (loaded by > 'loadb') via 'go' command. Also 'ext4ls' and 'fatls' commands are > working. Also 'onenand dump bootloader' is working. > > Do you need something more to test? > > If not you can add my 'Tested-by: Pali Rohár ' line for > all patches which are up to the commit 88d0a5042c97. > > Good job! Thanks. I am still working on these patches, since I have discovered some more defconfigs which fail to build for one reason or another. I will send a patch enabling LTO for Nokia N900 though. marek
Re: [RFC PATCH u-boot 01/12] build: use thin archives instead of incremental linking
On Fri, 5 Mar 2021 08:37:28 -0500 Tom Rini wrote: > On Fri, Mar 05, 2021 at 09:34:42PM +0800, Bin Meng wrote: > > Hi Marek, > > > > On Fri, Mar 5, 2021 at 2:17 AM Marek Behun wrote: > > > > > > On Thu, 4 Mar 2021 18:57:11 +0800 > > > Bin Meng wrote: > > > > > > > Hi Marek, > > > > > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún wrote: > > > > > > > > > > > > > > Using thin archives instead of incremental linking > > > > > - saves disk space > > > > > - works better with dead code elimination > > > > > - prepares for potential LTO > > > > > > > > The commit message is a little bit confusing. This commit actually > > > > does 2 things: don't do incremental linking (using --whole-archive), > > > > and use thin archive (passing T to ar). I believe they are for > > > > different purposes, so we cannot say "using thin archives instead of > > > > incremental linking". > > > > > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > > > > > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group > > > > > \ > > > > > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) > > > > > -Wl,--no-whole-archive \ > > > > > + -Wl,--start-group $(patsubst > > > > > $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > > > > > u-boot-spl-platdata is still within --start-group, --end-group, is > > > > this intentional? > > > > > > I confess that I did not really study these options, I have made these > > > changes according to old LTO patches for Linux. But you are right that > > > it does not make sense. I have fixed this for the next version of this > > > patch. > > > > > > > Is P required to make everything work? > > > > > > It is not. Removed in next version. > > > > I did more investigation on this. > > > > The Linux kernel specially added P to ar, in below commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a6cfca4f4130444 > > > > So it looks like we should keep P here? > > > > But I don't get the point of switching to thin archives. Based on my > > experiment, LTO does not rely on thin archives. The Linux kernel did > > not introduce thin archives for LTO. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5967db9af51a84f > > > > So technically it would just be part of dealing with the backlog of > kbuild-resync to take it in this series I guess. > It seems the P flag is needed for ar, otherwise final linking may fail, for example for nokia rx51. Since Linux uses this as well I am just gonna put it there.
Re: [RFC PATCH u-boot 00/12] U-Boot LTO (Sandbox + ARM Nokia RX-51)
On Sat, 6 Mar 2021 22:00:45 +0100 Pali Rohár wrote: > On Saturday 06 March 2021 21:54:00 Marek Behun wrote: > > On Sat, 6 Mar 2021 21:41:14 +0100 > > Pali Rohár wrote: > > > > > On Saturday 06 March 2021 15:08:13 Tom Rini wrote: > > > > Perhaps we'll default to yes on some SoCs. The omap3 thing is a bit > > > > odd, but we'll see what happens on real N900 hardware. > > > > > > Hello! > > > > > > Could you send me a link to git repo / branch and tell me from which > > > commit should I do tests on real N900 hardware? I will test it and let > > > you know results. > > > > > > Adding maemo ML to the loop as on the maemo list are more people with > > > N900 HW and U-Boot. > > > > https://github.com/elkablo/u-boot branch lto > > Sorry, compilation is failing :-( > > $ git clone https://github.com/elkablo/u-boot -b lto --depth=100 > Cloning into 'u-boot'... > remote: Enumerating objects: 33644, done. > remote: Counting objects: 100% (33644/33644), done. > remote: Compressing objects: 100% (20116/20116), done. > remote: Total 33644 (delta 15838), reused 19947 (delta 13018), pack-reused 0 > Receiving objects: 100% (33644/33644), 26.28 MiB | 10.21 MiB/s, done. > Resolving deltas: 100% (15838/15838), done. > > $ cd u-boot > > $ make CROSS_COMPILE=arm-linux-gnueabi- nokia_rx51_config > HOSTCC scripts/basic/fixdep > HOSTCC scripts/kconfig/conf.o > YACCscripts/kconfig/zconf.tab.c > LEX scripts/kconfig/zconf.lex.c > HOSTCC scripts/kconfig/zconf.tab.o > HOSTLD scripts/kconfig/conf > # > # configuration written to .config > # > > $ make CROSS_COMPILE=arm-linux-gnueabi- u-boot.bin > ... > LTO u-boot > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > DWARF error: offset (1258291444) greater than or equal to .debug_str size > (676) > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > DWARF error: offset (1459618036) greater than or equal to .debug_str size > (676) > /usr/lib/gcc-cross/arm-linux-gnueabi/8/../../../../arm-linux-gnueabi/bin/ld: > DWARF error: could not find abbrev number 48028 > /tmp/cc8l0QSQ.ltrans3.ltrans.o: in function `omap3_set_aux_cr_secure': > :(.text+0x6eb8): undefined reference to > `do_omap3_emu_romcode_call' > collect2: error: ld returned 1 exit status > make: *** [Makefile:1808: u-boot] Error 1 > > > I'm using arm-linux-gnueabi-gcc version 8.3.0 which is available in > current Debian stable (Debian 10 Buster). Fixed and force-pushed, it seems ar needs the P flag that Bin Meng questioned.
Re: [RFC PATCH u-boot 00/12] U-Boot LTO (Sandbox + ARM Nokia RX-51)
On Sat, 6 Mar 2021 21:41:14 +0100 Pali Rohár wrote: > On Saturday 06 March 2021 15:08:13 Tom Rini wrote: > > Perhaps we'll default to yes on some SoCs. The omap3 thing is a bit > > odd, but we'll see what happens on real N900 hardware. > > Hello! > > Could you send me a link to git repo / branch and tell me from which > commit should I do tests on real N900 hardware? I will test it and let > you know results. > > Adding maemo ML to the loop as on the maemo list are more people with > N900 HW and U-Boot. https://github.com/elkablo/u-boot branch lto
Re: [RFC PATCH u-boot 00/12] U-Boot LTO (Sandbox + ARM Nokia RX-51)
On Sat, 6 Mar 2021 05:12:12 -0600 Adam Ford wrote: > On Fri, Mar 5, 2021 at 9:03 PM Adam Ford wrote: > > > > On Fri, Mar 5, 2021 at 11:10 AM Adam Ford wrote: > > > > > > On Fri, Mar 5, 2021 at 6:31 AM Stefan Roese wrote: > > > > > > > > On 05.03.21 12:25, Adam Ford wrote: > > > > > On Thu, Mar 4, 2021 at 4:33 PM Marek Behun > > > > > wrote: > > > > >> > > > > >> On Thu, 4 Mar 2021 16:18:03 -0600 > > > > >> Adam Ford wrote: > > > > >> > > > > >>> diff --git a/arch/arm/mach-omap2/omap3/Makefile > > > > >>> b/arch/arm/mach-omap2/omap3/Makefile > > > > >>> index 91ed8ebc9f..a2cc21c6d2 100644 > > > > >>> --- a/arch/arm/mach-omap2/omap3/Makefile > > > > >>> +++ b/arch/arm/mach-omap2/omap3/Makefile > > > > >>> @@ -6,6 +6,8 @@ > > > > >>> # If clock.c is compiled for Thumb2, then it fails on OMAP3530 > > > > >>> CFLAGS_clock.o += -marm > > > > >>> > > > > >>> +CFLAGS_REMOVE_file.o := $(LTO_CFLAGS) > > > > >> > > > > >> Eh? There is no file.c in arch/arm/mach-omap2/omap3/ directory. > > > > > > > > > > It must be from something else that had changed when I did a git pull > > > > > pull on your repo. I guess that's better news. > > > > > > > > It might be a misunderstanding. Marek means that you need to replace > > > > "file" with your real filename, like: > > > > > > > > driver_spi.c -> > > > > > > > > +CFLAGS_REMOVE_driver_spi.o := $(LTO_CFLAGS) > > > > > > I first did a git pull from his git repo, and then blindly copy-pasted > > > the suggested link to that above directly. I don't play with > > > Makefiles often, so I didn't really notice that I needed to match the > > > line to an actual file. When I rebuilt, my initial issue with showing > > > too much DDR and hanging went away, so I wrongly assumed that this > > > fixed it. In reality, I think it was a patch that had been applied to > > > his git repo that got pulled in at the same time. > > > > > > On the testing front, > > > I started testing the da850evm (ARM9) this morning. I ran into an > > > issue that I apparently caused some time ago, and I'm trying to > > > resolve that now. I have it working, but I need to clean it up. I'll > > > push the clean-up to the ML then try the LTO on that board to see if > > > U-Boot and/or SPL work with LTO enabled. > > > > > > As of now, I have one board that seems to fully boot (imx6q_logic) and > > > a family of boards that work in U-Boot but not SPL (omap3_logic). > > > > > > After the da850em, I'll test a 64-bit Renesas board without SPL and a > > > 64-bit NXP board with SPL to test. > > > > > > On the build-testing I have done, I am seeing an average of a 10-20% > > > reduction in size for SPL, and a ~ 3% reduction in size in U-Boot. > > > > > > > With a patch that I've already sent to the mailing list for the > > da850evm, it's booting both SPL and U-Boot > > > > With the compiler I have, the code went from: > > SPL24305 > > U-Boot 381930 > > > > To: > > SPL20937 > > U-Boot 358780 > > > > For a Reduction of: > > SPL-3368 (-13.86%) > > U-Boot -23150 (-6.06%) > > > > I didn't test the NOR or NAND booting versions of the da850evm, but I > > will try to do that when the time comes. > > > > I ran the same tests on the imx8mn_beacon board, and this board boots. > > Without LTO > SPL 82487 > U-Boot 704477 > > With LTO: > SPL 74526 > U-Boot 670859 > > For a reduction of: > SPL -7961 (-9.65%) > U-Boot -33618 (-4.77%) Thank you Adam for these tests. I think you are right in that we should not enable LTO for all ARM boards, but only those that are tested, at least until for example about 80% of them are tested. Marek
Re: [RFC PATCH u-boot 02/12] sandbox: errno: avoid conflict with libc's errno
On Fri, 5 Mar 2021 09:58:34 -0700 Simon Glass wrote: > Hi Marek, > > On Fri, 5 Mar 2021 at 09:50, Marek Behun wrote: > > > > On Fri, 5 Mar 2021 09:39:53 -0700 > > Simon Glass wrote: > > > > > Hi Marek, > > > > > > On Fri, 5 Mar 2021 at 08:37, Marek Behun wrote: > > > > > > > > On Fri, 5 Mar 2021 11:00:45 +0800 > > > > Bin Meng wrote: > > > > > > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún > > > > > wrote: > > > > > > > > > > > > When building with LTO, the system libc's `errno` variable used in > > > > > > arch/sandbox/cpu/os.c conflicts with U-Boot's `errno` (defined in > > > > > > lib/errno.c) with the following error: > > > > > > .../ld: errno@@GLIBC_PRIVATE: TLS definition in /lib64/libc.so.6 > > > > > > section .tbss mismatches non-TLS reference in > > > > > > /tmp/u-boot.EQlEXz.ltrans0.ltrans.o > > > > > > > > > > Do you know if this is the expected behavior when enabling LTO on the > > > > > compiler? > > > > > > > > I don't, but this is a bug anyway. The symbol clashes with the symbol > > > > from glibc. Does somebody know whether the usage of this symbol in os.c > > > > does really use glibc's version or U-Boot's one? > > > > > > It is intended to use glibc's version. In fact I don't think U-Boot > > > should have an errno. We return errors in each case, as does Linux. > > > > The problem is that libc defines errno as a thread-local variable or, > > in older version, as a macro expading to a function dereference, i.e. > > #define errno (*__get_threads_errno()) > > But U-Boot usis the errno symbol defined in include/errno.h as a symbol. > > > > So in order for these two symbols not to clash (in case libc is using > > thread-local symbol with name errno), we need to rename the U-Boot > > errno variable's symbol name. > > Rename is OK, but can we delete it instead? I really don't think it > should be there. We can't simply delete it. The whole u-boot is using the errno symbol from include/errno.h and if we want the whole u-boot to use libc's symbol we need to code include/errno.h to declare it in the same way as libc, which may be different for different libcs.
Re: [RFC PATCH u-boot 02/12] sandbox: errno: avoid conflict with libc's errno
On Fri, 5 Mar 2021 09:39:53 -0700 Simon Glass wrote: > Hi Marek, > > On Fri, 5 Mar 2021 at 08:37, Marek Behun wrote: > > > > On Fri, 5 Mar 2021 11:00:45 +0800 > > Bin Meng wrote: > > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún wrote: > > > > > > > > When building with LTO, the system libc's `errno` variable used in > > > > arch/sandbox/cpu/os.c conflicts with U-Boot's `errno` (defined in > > > > lib/errno.c) with the following error: > > > > .../ld: errno@@GLIBC_PRIVATE: TLS definition in /lib64/libc.so.6 > > > > section .tbss mismatches non-TLS reference in > > > > /tmp/u-boot.EQlEXz.ltrans0.ltrans.o > > > > > > Do you know if this is the expected behavior when enabling LTO on the > > > compiler? > > > > I don't, but this is a bug anyway. The symbol clashes with the symbol > > from glibc. Does somebody know whether the usage of this symbol in os.c > > does really use glibc's version or U-Boot's one? > > It is intended to use glibc's version. In fact I don't think U-Boot > should have an errno. We return errors in each case, as does Linux. The problem is that libc defines errno as a thread-local variable or, in older version, as a macro expading to a function dereference, i.e. #define errno (*__get_threads_errno()) But U-Boot usis the errno symbol defined in include/errno.h as a symbol. So in order for these two symbols not to clash (in case libc is using thread-local symbol with name errno), we need to rename the U-Boot errno variable's symbol name.
Re: [RFC PATCH u-boot 03/12] linker_lists: declare entries and lists externally visible
On Fri, 5 Mar 2021 11:04:08 +0800 Bin Meng wrote: > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún wrote: > > > > Use the `__visible` macro to declare entires and lists declared by > > ll_entry_declare() and ll_entry_declare_list() externally visible, so > > that when building with LTO the compiler does not optimize this data > > away. > > > > __visible is defined like this: > > /* > * Optional: not supported by clang > * > * gcc: > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-externally_005fvisible-function-attribute > */ > #if __has_attribute(__externally_visible__) > # define __visible > __attribute__((__externally_visible__)) > #else > # define __visible > #endif > > It says clang does not support this. So what about clang? > > > Signed-off-by: Marek Behún > > --- > > include/linker_lists.h | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > Regards, > Bin Bin, this is already changed to something different on my github. I will send new version once I am satisfied with CI tests. Marek
Re: [RFC PATCH u-boot 01/12] build: use thin archives instead of incremental linking
On Fri, 5 Mar 2021 21:34:42 +0800 Bin Meng wrote: > Hi Marek, > > On Fri, Mar 5, 2021 at 2:17 AM Marek Behun wrote: > > > > On Thu, 4 Mar 2021 18:57:11 +0800 > > Bin Meng wrote: > > > > > Hi Marek, > > > > > > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún wrote: > > > > > > > > Using thin archives instead of incremental linking > > > > - saves disk space > > > > - works better with dead code elimination > > > > - prepares for potential LTO > > > > > > The commit message is a little bit confusing. This commit actually > > > does 2 things: don't do incremental linking (using --whole-archive), > > > and use thin archive (passing T to ar). I believe they are for > > > different purposes, so we cannot say "using thin archives instead of > > > incremental linking". > > > > - -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-main)) \ > > > > - $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) -Wl,--end-group \ > > > > + -Wl,--whole-archive $(patsubst $(obj)/%,%,$(u-boot-spl-main)) > > > > -Wl,--no-whole-archive \ > > > > + -Wl,--start-group $(patsubst $(obj)/%,%,$(u-boot-spl-platdata)) > > > > -Wl,--end-group \ > > > > > > u-boot-spl-platdata is still within --start-group, --end-group, is > > > this intentional? > > > > I confess that I did not really study these options, I have made these > > changes according to old LTO patches for Linux. But you are right that > > it does not make sense. I have fixed this for the next version of this > > patch. > > > > > Is P required to make everything work? > > > > It is not. Removed in next version. > > I did more investigation on this. > > The Linux kernel specially added P to ar, in below commit: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9a6cfca4f4130444 > > So it looks like we should keep P here? > > But I don't get the point of switching to thin archives. Based on my > experiment, LTO does not rely on thin archives. The Linux kernel did > not introduce thin archives for LTO. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a5967db9af51a84f It does not matter whether we use thin archives or real archives. But we did not use any of this before, instead we linked the object files in a directory into one object file in that directory. And to do this with LTO would cause unnecessary complications. Marek
Re: [RFC PATCH u-boot 02/12] sandbox: errno: avoid conflict with libc's errno
On Fri, 5 Mar 2021 11:00:45 +0800 Bin Meng wrote: > On Wed, Mar 3, 2021 at 12:13 PM Marek Behún wrote: > > > > When building with LTO, the system libc's `errno` variable used in > > arch/sandbox/cpu/os.c conflicts with U-Boot's `errno` (defined in > > lib/errno.c) with the following error: > > .../ld: errno@@GLIBC_PRIVATE: TLS definition in /lib64/libc.so.6 > > section .tbss mismatches non-TLS reference in > > /tmp/u-boot.EQlEXz.ltrans0.ltrans.o > > Do you know if this is the expected behavior when enabling LTO on the > compiler? I don't, but this is a bug anyway. The symbol clashes with the symbol from glibc. Does somebody know whether the usage of this symbol in os.c does really use glibc's version or U-Boot's one?
