Build system: Don't check for CONFIG_SYS_TEXT_BASE being set

PATCH In review Alexey Brodkin 5 participants 15 messages Jan 30, 2018 – Feb 12, 2018 Kbuild

Share & tools

Alexey Brodkin

author

2018-01-30 15:23 UTC #0 lore raw

PATCH 1/1 Build system: Don't check for CONFIG_SYS_TEXT_BASE being set

CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.

Signed-off-by Alexey Brodkin <[email protected]>

Cc Tom Rini <[email protected]>

---
 Makefile | 2 --
 1 file changed, 2 deletions(-)

Makefile +0 -2

index ab3453dcebdc..6f15612b4d07 100644
@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif
 
 # Normally we fill empty space with 0xff
 quiet_cmd_objcopy = OBJCOPY $@

Signature · 1 line

2.14.3

Masahiro Yamada

2018-01-30 16:23 UTC #1 lore raw

PATCH 1/1 Build system: Don't check for CONFIG_SYS_TEXT_BASE being set

2018-01-31 0:23 GMT+09:00 Alexey Brodkin <[email protected]>:

Quoted 15 lines

CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.
Signed-off-by Alexey Brodkin <[email protected]>
Cc Tom Rini <[email protected]>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)
Makefile +0 -0
index ab3453dcebdc..6f15612b4d07 100644

Makefile LDFLAGS_u-boot += $(LDFLAGS_FINAL) · line 825

Quoted 6 lines

Makefile +0 -2

@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif

Please feel free to include historical reason:

This ifneq conditional was added for Blackfin because it did not use CONFIG_SYS_TEXT_BASE.
See.
https://github.com/u-boot/u-boot/blob/v2017.03/arch/blackfin/config.mk#L67

Then Blackfin is gone, so should be OK.

Acked-by Masahiro Yamada <[email protected]>

Quoted 9 lines

Makefile +0 -1
 # Normally we fill empty space with 0xff
 quiet_cmd_objcopy = OBJCOPY $@
--
2.14.3
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Signature · 2 lines

Best Regards
Masahiro Yamada

Alexey Brodkin

author

2018-01-30 16:28 UTC #2 lore raw

PATCH 1/1 Build system: Don't check for CONFIG_SYS_TEXT_BASE being set

Hi MAsahiro-san, Tom,

Quoted 4 lines · Masahiro Yamada

2018-01-31 0:23 GMT+09:00 Alexey Brodkin <[email protected]>:
Quoted 1 line
Please feel free to include historical reason:

Quoted 8 lines

This ifneq conditional was added for Blackfin because it did not use CONFIG_SYS_TEXT_BASE.
See.
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_u-2Dboot_u-2Dboot_blob_v2017.03_arch_blackfin_config.mk-23L67&d=DwIBaQ&c=DPL6_X_6JkX
Fx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=mezwGj3E5EIPuE8Oln2gVeTJ-8q19KNepz7Jo- Kcu1o&s=dmnDXQ05QLyAFKUgnsVyJOS8aybk3rhbDNsgiio6zvA&e=
Then Blackfin is gone, so should be OK.

Should I send a respin with this valuable addition or whoever's going to apply will fix-up this?

-Alexey

Masahiro Yamada

2018-01-31 15:18 UTC #3 lore raw

PATCH 1/1 Build system: Don't check for CONFIG_SYS_TEXT_BASE being set

2018-01-31 1:28 GMT+09:00 Alexey Brodkin <[email protected]>:

Quoted 14 lines

Hi MAsahiro-san, Tom,
Quoted 11 lines · Masahiro Yamada
2018-01-31 0:23 GMT+09:00 Alexey Brodkin <[email protected]>:
Quoted 1 line
Please feel free to include historical reason:
This ifneq conditional was added for Blackfin because it did not use CONFIG_SYS_TEXT_BASE.
See.
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_u-2Dboot_u-2Dboot_blob_v2017.03_arch_blackfin_config.mk-23L67&d=DwIBaQ&c=DPL6_X_6JkX
Fx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=mezwGj3E5EIPuE8Oln2gVeTJ-8q19KNepz7Jo- Kcu1o&s=dmnDXQ05QLyAFKUgnsVyJOS8aybk3rhbDNsgiio6zvA&e=

Quoted 6 lines

Quoted 1 line
Then Blackfin is gone, so should be OK.
Should I send a respin with this valuable addition or whoever's going to apply will fix-up this?
-Alexey

Either will do for me.

Signature · 2 lines

Best Regards
Masahiro Yamada

Alexey Brodkin

author

2018-01-31 15:21 UTC #4 lore raw

PATCH 1/1 Build system: Don't check for CONFIG_SYS_TEXT_BASE being set

Hi Masahiro-san,

Quoted 19 lines · Masahiro Yamada

2018-01-31 1:28 GMT+09:00 Alexey Brodkin <[email protected]>:
Quoted 18 lines
Hi MAsahiro-san, Tom,
Quoted 14 lines · Masahiro Yamada
2018-01-31 0:23 GMT+09:00 Alexey Brodkin <[email protected]>:
Quoted 1 line
Please feel free to include historical reason:
This ifneq conditional was added for Blackfin because it did not use CONFIG_SYS_TEXT_BASE.
See.
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_u-2Dboot_u-2Dboot_blob_v2017.03_arch_blackfin_config.mk-23L67&d=DwIBaQ&c=DPL6_X_
6JkX Fx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=mezwGj3E5EIPuE8Oln2gVeTJ-8q19KNepz7Jo- Kcu1o&s=dmnDXQ05QLyAFKUgnsVyJOS8aybk3rhbDNsgiio6zvA&e=
Then Blackfin is gone, so should be OK.

Quoted 7 lines

Quoted 4 lines
Should I send a respin with this valuable addition or whoever's going to apply will fix-up this?
-Alexey
Either will do for me.

I'd prefer to let you do this fix-up so we don't spam both mailing list and patchwork with v2.

-Alexey

Masahiro Yamada

2018-02-02 15:01 UTC #5 lore raw

PATCH 1/1 Build system: Don't check for CONFIG_SYS_TEXT_BASE being set

2018-02-01 0:21 GMT+09:00 Alexey Brodkin <[email protected]>:

Quoted 25 lines

Hi Masahiro-san,
Quoted 22 lines · Masahiro Yamada
2018-01-31 1:28 GMT+09:00 Alexey Brodkin <[email protected]>:
Quoted 21 lines
Hi MAsahiro-san, Tom,
Quoted 13 lines · Masahiro Yamada
2018-01-31 0:23 GMT+09:00 Alexey Brodkin <[email protected]>:
Quoted 1 line
Please feel free to include historical reason:
This ifneq conditional was added for Blackfin because it did not use CONFIG_SYS_TEXT_BASE.
See.
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_u-2Dboot_u-2Dboot_blob_v2017.03_arch_blackfin_config.mk-23L67&d=DwIBaQ&c=DPL6_X_
6JkX Fx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=mezwGj3E5EIPuE8Oln2gVeTJ-8q19KNepz7Jo- Kcu1o&s=dmnDXQ05QLyAFKUgnsVyJOS8aybk3rhbDNsgiio6zvA&e=
Quoted 1 line
Then Blackfin is gone, so should be OK.
Should I send a respin with this valuable addition or whoever's going to apply will fix-up this?

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
-Alexey
Either will do for me.
I'd prefer to let you do this fix-up so we don't spam both mailing list and patchwork with v2.
-Alexey

I think your comment in another one is good
(http://patchwork.ozlabs.org/patch/868299/)

If you are reluctant on v2, you can ask Tom to
rephrase the log when this patch is applied.

Signature · 2 lines

Best Regards
Masahiro Yamada

Tom Rini

2018-02-11 20:47 UTC #6 lore raw

In reply to Alexey Brodkin

Quoted 16 lines · Alexey Brodkin

CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.
Signed-off-by Alexey Brodkin <[email protected]>
Cc Tom Rini <[email protected]>
Acked-by Masahiro Yamada <[email protected]>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)
Makefile +0 -0
index ab3453dcebdc..6f15612b4d07 100644

Makefile LDFLAGS_u-boot += $(LDFLAGS_FINAL) · line 825

Quoted 6 lines

Makefile +0 -2

@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif

This then causes xtensa to fail to build as it does not set CONFIG_SYS_TEXT_BASE.

Signature · 1 line

Tom

Alexey Brodkin

author

2018-02-12 13:27 UTC #7 lore raw

Hi Tom, Simon,

Quoted 21 lines · Tom Rini

Quoted 19 lines · Alexey Brodkin
CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.
Signed-off-by Alexey Brodkin <[email protected]>
Cc Tom Rini <[email protected]>
Acked-by Masahiro Yamada <[email protected]>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)
Makefile +0 -0
index ab3453dcebdc..6f15612b4d07 100644
@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 

Quoted 6 lines

Makefile LDFLAGS_u-boot += $(LDFLAGS_FINAL) · line 825
Quoted 3 lines
Makefile +0 -2
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif
This then causes xtensa to fail to build as it does not set CONFIG_SYS_TEXT_BASE.

And that also obviously breaks "efi-x86" target as well because CONFIG_SYS_TEXT_BASE seems to not be defined for EFI and then LD gets a string like "-Ttext -o u-boot" where CONFIG_SYS_TEXT_BASE is supposed to be used as some value.

Frankly I'm not sure what to do with that - probably EFI is just a very special case...

But FWIW I'm not very happy with mandatory "-ttext $(CONFIG_SYS_TEXT_BASE)" in case of ARC and here's why:
1. In case of ARCv2 ISA interrupt vector table is not just another code section with jump

    instructions to corresponding handlers but instead it's just a set of addresses pointing
    to corresponding handlers.

    I.e. that's a traditional IVT (interrupt vector table) which among other architectures is
    used on older ARCompact (AKA ARCv1 ISA):
    ------------------->8-----------------
    .ivt
        jump 0x1000_0000
        jump 0x1000_1000
        ...
    ------------------->8-----------------

    And that's what we have for ARCv2:
    ------------------->8-----------------
    .ivt
        0x1000_0000
        0x1000_1000
        ...
    ------------------->8-----------------

2. Now one may think there's no big difference in 2 cases above except content:

    it is either encoded instructions or literals. But that really matters because
    in case of ARC (regardless ISA version) instructions are encoded in __middle-endian__
    format while literals are normal little-endians.

    Consider the following example:
    ------------------->8-----------------
    .section .ivt
    .word	0x10000000
    .word	0x10001000
    .align	256
    .section .text
    .word	0x10000000
    .word	0x10001000
    ------------------->8-----------------

    That will be compiled into this:
    ------------------->8-----------------
    Disassembly of section .text:
    00000000 <.text>:
       0:	0000 1000           	b	131072	;0x20000
       4:	1000 1000           	ld	r0,[r8]

    Disassembly of section .ivt:
    00000000 <.ivt>:
       0:	00 00 00 10             	.word	0x10000000
       4:	00 10 00 10             	.word	0x10001000
    ------------------->8-----------------

    Note how bytes are swapped in .text section.

In the end that basically means we cannot put IVT in the beginning of .text section how it is usually done. We need to keep .ivt and .text sections as separate substances.

And so far what we used to do we put .ivt section after .text. It was done as a preparation for ARCv2 port introduction here:
http://git.denx.de/?p=u-boot.git;a=commit;h=20a58ac0d8e09d0bf1a74c6b68fea22784512b51

Now here comes another challenge - so far U-Boot was not the first piece of software that was executed by CPU, but what's even more important U-Boot was started by boot-ROM via jump to U-Boot's entry point (which happened to be it's start of .text section).

But now we're going to run U-Boot as the first ever thing on power on and for that we'll put U-Boot in ROM such that CPU starts execution from "reset" vector and that will be U-Boot.

In other words in hardware location of IVT is hard-coded as 0x0000_0000 and that's where we'll put U-Boot. With explanation above I think it's quite clear that we cannot have .text section there at 0x0000_0000 because what's going to happen is CPU will fetch the first "data" word from ROM and will attempt to junp at address it "sees" there. Obviously that won't be a correct address and so CPU will just jump into some unexpected location.

Which basically means we need to put .ivt section in the very beginning of the image and have .text section at say 0x0000_1000. I.e. now we'll need to keep in mind at least 2 things:
1) CONFIG_SYS_TEXT_BASE is not the base-address of the uboot.img
2) .ivt's base-address is something just a couple of kB below CONFIG_SYS_TEXT_BASE

So if "-Ttext CONFIG_SYS_TEXT_BASE" is not used for each and every board I may use CONFIG_SYS_TEXT_BASE directly in linker just as we have it now, see
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arc/cpu/u-boot.lds#l14
and then there might be any section like .ivt, .text, .myfunkysection etc.

In fact in the Linux kernel "-Ttext XXX" is not used for everybody - some arches like MIPS and PPC indeed set it but others do other things.

The simplest thing might be is to add another #ifdef for ARC and X86 which both use CONFIG_SYS_TEXT_BASE directly in their linker scripts like that:
------------------->8-----------------

Makefile +2 -1

@@ -820,9 +820,11 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
+ifeq($(CONFIG_ARC)$(CONFIG_X86),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
+endif
 
 # Normally we fill empty space with 0xff
 quiet_cmd_objcopy = OBJCOPY $@
------------------->8-----------------

Any thoughts?

-Alexey

Tom Rini

2018-02-12 14:23 UTC #8 lore raw

Quoted 112 lines · Alexey Brodkin

Hi Tom, Simon,
Quoted 21 lines · Tom Rini
Quoted 19 lines · Alexey Brodkin
CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.
Signed-off-by Alexey Brodkin <[email protected]>
Cc Tom Rini <[email protected]>
Acked-by Masahiro Yamada <[email protected]>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)
Makefile +0 -0
index ab3453dcebdc..6f15612b4d07 100644
@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
Quoted 6 lines
Makefile LDFLAGS_u-boot += $(LDFLAGS_FINAL) · line 825
Quoted 3 lines
Makefile +0 -2
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif
This then causes xtensa to fail to build as it does not set CONFIG_SYS_TEXT_BASE.
And that also obviously breaks "efi-x86" target as well because CONFIG_SYS_TEXT_BASE seems to not be defined for EFI and then LD gets a string like "-Ttext -o u-boot" where CONFIG_SYS_TEXT_BASE is supposed to be used as some value.
Frankly I'm not sure what to do with that - probably EFI is just a very special case...
But FWIW I'm not very happy with mandatory "-ttext $(CONFIG_SYS_TEXT_BASE)" in case of ARC and here's why:
1. In case of ARCv2 ISA interrupt vector table is not just another code section with jump
    instructions to corresponding handlers but instead it's just a set of addresses pointing
    to corresponding handlers.

    I.e. that's a traditional IVT (interrupt vector table) which among other architectures is
    used on older ARCompact (AKA ARCv1 ISA):
    ------------------->8-----------------
    .ivt
        jump 0x1000_0000
        jump 0x1000_1000
        ...
    ------------------->8-----------------

    And that's what we have for ARCv2:
    ------------------->8-----------------
    .ivt
        0x1000_0000
        0x1000_1000
        ...
    ------------------->8-----------------
2. Now one may think there's no big difference in 2 cases above except content:
    it is either encoded instructions or literals. But that really matters because
    in case of ARC (regardless ISA version) instructions are encoded in __middle-endian__
    format while literals are normal little-endians.

    Consider the following example:
    ------------------->8-----------------
    .section .ivt
    .word	0x10000000
    .word	0x10001000
    .align	256
    .section .text
    .word	0x10000000
    .word	0x10001000
    ------------------->8-----------------

    That will be compiled into this:
    ------------------->8-----------------
    Disassembly of section .text:
    00000000 <.text>:
       0:	0000 1000           	b	131072	;0x20000
       4:	1000 1000           	ld	r0,[r8]

    Disassembly of section .ivt:
    00000000 <.ivt>:
       0:	00 00 00 10             	.word	0x10000000
       4:	00 10 00 10             	.word	0x10001000
    ------------------->8-----------------

    Note how bytes are swapped in .text section.
In the end that basically means we cannot put IVT in the beginning of .text section how it is usually done. We need to keep .ivt and .text sections as separate substances.
And so far what we used to do we put .ivt section after .text. It was done as a preparation for ARCv2 port introduction here:
http://git.denx.de/?p=u-boot.git;a=commit;h=20a58ac0d8e09d0bf1a74c6b68fea22784512b51
Now here comes another challenge - so far U-Boot was not the first piece of software that was executed by CPU, but what's even more important U-Boot was started by boot-ROM via jump to U-Boot's entry point (which happened to be it's start of .text section).
But now we're going to run U-Boot as the first ever thing on power on and for that we'll put U-Boot in ROM such that CPU starts execution from "reset" vector and that will be U-Boot.
In other words in hardware location of IVT is hard-coded as 0x0000_0000 and that's where we'll put U-Boot. With explanation above I think it's quite clear that we cannot have .text section there at 0x0000_0000 because what's going to happen is CPU will fetch the first "data" word from ROM and will attempt to junp at address it "sees" there. Obviously that won't be a correct address and so CPU will just jump into some unexpected location.

Quoted 4 lines

Which basically means we need to put .ivt section in the very beginning of the image and have .text section at say 0x0000_1000. I.e. now we'll need to keep in mind at least 2 things:
1) CONFIG_SYS_TEXT_BASE is not the base-address of the uboot.img
2) .ivt's base-address is something just a couple of kB below CONFIG_SYS_TEXT_BASE

Thanks for explaining what's going on with ARC.

Quoted 25 lines

So if "-Ttext CONFIG_SYS_TEXT_BASE" is not used for each and every board I may use CONFIG_SYS_TEXT_BASE directly in linker just as we have it now, see
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arc/cpu/u-boot.lds#l14
and then there might be any section like .ivt, .text, .myfunkysection etc.
In fact in the Linux kernel "-Ttext XXX" is not used for everybody - some arches like MIPS and PPC indeed set it but others do other things.
The simplest thing might be is to add another #ifdef for ARC and X86 which both use CONFIG_SYS_TEXT_BASE directly in their linker scripts like that:
------------------->8-----------------
Makefile +2 -1
@@ -820,9 +820,11 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
+ifeq($(CONFIG_ARC)$(CONFIG_X86),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
+endif
 
 # Normally we fill empty space with 0xff
 quiet_cmd_objcopy = OBJCOPY $@
------------------->8-----------------
 

Quoted 1 line

Any thoughts?

I'm largely ok with the above, but:
- You need to exclude CONFIG_NIOS2 in the above as well, or convert the usage of CONFIG_SYS_MONITOR_BASE to CONFIG_SYS_TEXT_BASE (Thomas?)
- For Xtensa (Max?), CONFIG_SYS_TEXT_ADDR needs to be renamed to CONFIG_SYS_TEXT_BASE there

Thanks!

Signature · 1 line

Tom

Marek Vasut

2018-02-12 14:46 UTC #9 lore raw

Quoted 145 lines · Tom Rini

Quoted 112 lines · Alexey Brodkin
Hi Tom, Simon,
Quoted 21 lines · Tom Rini
Quoted 19 lines · Alexey Brodkin
CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.
Signed-off-by Alexey Brodkin <[email protected]>
Cc Tom Rini <[email protected]>
Acked-by Masahiro Yamada <[email protected]>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)
Makefile +0 -0
index ab3453dcebdc..6f15612b4d07 100644
@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
Quoted 6 lines
Makefile LDFLAGS_u-boot += $(LDFLAGS_FINAL) · line 825
Quoted 3 lines
Makefile +0 -2
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif
This then causes xtensa to fail to build as it does not set CONFIG_SYS_TEXT_BASE.
And that also obviously breaks "efi-x86" target as well because CONFIG_SYS_TEXT_BASE seems to not be defined for EFI and then LD gets a string like "-Ttext -o u-boot" where CONFIG_SYS_TEXT_BASE is supposed to be used as some value.
Frankly I'm not sure what to do with that - probably EFI is just a very special case...
But FWIW I'm not very happy with mandatory "-ttext $(CONFIG_SYS_TEXT_BASE)" in case of ARC and here's why:
1. In case of ARCv2 ISA interrupt vector table is not just another code section with jump
    instructions to corresponding handlers but instead it's just a set of addresses pointing
    to corresponding handlers.

    I.e. that's a traditional IVT (interrupt vector table) which among other architectures is
    used on older ARCompact (AKA ARCv1 ISA):
    ------------------->8-----------------
    .ivt
        jump 0x1000_0000
        jump 0x1000_1000
        ...
    ------------------->8-----------------

    And that's what we have for ARCv2:
    ------------------->8-----------------
    .ivt
        0x1000_0000
        0x1000_1000
        ...
    ------------------->8-----------------
2. Now one may think there's no big difference in 2 cases above except content:
    it is either encoded instructions or literals. But that really matters because
    in case of ARC (regardless ISA version) instructions are encoded in __middle-endian__
    format while literals are normal little-endians.

    Consider the following example:
    ------------------->8-----------------
    .section .ivt
    .word	0x10000000
    .word	0x10001000
    .align	256
    .section .text
    .word	0x10000000
    .word	0x10001000
    ------------------->8-----------------

    That will be compiled into this:
    ------------------->8-----------------
    Disassembly of section .text:
    00000000 <.text>:
       0:	0000 1000           	b	131072	;0x20000
       4:	1000 1000           	ld	r0,[r8]

    Disassembly of section .ivt:
    00000000 <.ivt>:
       0:	00 00 00 10             	.word	0x10000000
       4:	00 10 00 10             	.word	0x10001000
    ------------------->8-----------------

    Note how bytes are swapped in .text section.
In the end that basically means we cannot put IVT in the beginning of .text section how it is usually done. We need to keep .ivt and .text sections as separate substances.
And so far what we used to do we put .ivt section after .text. It was done as a preparation for ARCv2 port introduction here:
http://git.denx.de/?p=u-boot.git;a=commit;h=20a58ac0d8e09d0bf1a74c6b68fea22784512b51
Now here comes another challenge - so far U-Boot was not the first piece of software that was executed by CPU, but what's even more important U-Boot was started by boot-ROM via jump to U-Boot's entry point (which happened to be it's start of .text section).
But now we're going to run U-Boot as the first ever thing on power on and for that we'll put U-Boot in ROM such that CPU starts execution from "reset" vector and that will be U-Boot.
In other words in hardware location of IVT is hard-coded as 0x0000_0000 and that's where we'll put U-Boot. With explanation above I think it's quite clear that we cannot have .text section there at 0x0000_0000 because what's going to happen is CPU will fetch the first "data" word from ROM and will attempt to junp at address it "sees" there. Obviously that won't be a correct address and so CPU will just jump into some unexpected location.
Quoted 4 lines
Which basically means we need to put .ivt section in the very beginning of the image and have .text section at say 0x0000_1000. I.e. now we'll need to keep in mind at least 2 things:
1) CONFIG_SYS_TEXT_BASE is not the base-address of the uboot.img
2) .ivt's base-address is something just a couple of kB below CONFIG_SYS_TEXT_BASE
Thanks for explaining what's going on with ARC.
Quoted 25 lines
So if "-Ttext CONFIG_SYS_TEXT_BASE" is not used for each and every board I may use CONFIG_SYS_TEXT_BASE directly in linker just as we have it now, see
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arc/cpu/u-boot.lds#l14
and then there might be any section like .ivt, .text, .myfunkysection etc.
In fact in the Linux kernel "-Ttext XXX" is not used for everybody - some arches like MIPS and PPC indeed set it but others do other things.
The simplest thing might be is to add another #ifdef for ARC and X86 which both use CONFIG_SYS_TEXT_BASE directly in their linker scripts like that:
------------------->8-----------------
Makefile +2 -1
@@ -820,9 +820,11 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
+ifeq($(CONFIG_ARC)$(CONFIG_X86),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
+endif
 
 # Normally we fill empty space with 0xff
 quiet_cmd_objcopy = OBJCOPY $@
------------------->8-----------------
 

Quoted 5 lines

Quoted 1 line
Any thoughts?
I'm largely ok with the above, but:
- You need to exclude CONFIG_NIOS2 in the above as well, or convert the usage of CONFIG_SYS_MONITOR_BASE to CONFIG_SYS_TEXT_BASE (Thomas?)

I _think_ converting nios2 from monitor_base to text_base should be easy enough, U-Boot is linked to monitor_base via the linker script anyway, so just change that to text_base .

Quoted 5 lines

- For Xtensa (Max?), CONFIG_SYS_TEXT_ADDR needs to be renamed to CONFIG_SYS_TEXT_BASE there
Thanks!

Signature · 2 lines

Best regards,
Marek Vasut

Alexey Brodkin

author

2018-02-12 16:21 UTC #10 lore raw

In reply to Tom Rini

Quoted 35 lines · Tom Rini

Quoted 34 lines · Alexey Brodkin
Hi Tom, Simon,
Quoted 21 lines · Tom Rini
Quoted 19 lines · Alexey Brodkin
CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.
Signed-off-by Alexey Brodkin <[email protected]>
Cc Tom Rini <[email protected]>
Acked-by Masahiro Yamada <[email protected]>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)
Makefile +0 -0
index ab3453dcebdc..6f15612b4d07 100644
@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
Quoted 6 lines
Makefile LDFLAGS_u-boot += $(LDFLAGS_FINAL) · line 825
Quoted 3 lines
Makefile +0 -2
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif
This then causes xtensa to fail to build as it does not set CONFIG_SYS_TEXT_BASE.
And that also obviously breaks "efi-x86" target as well because CONFIG_SYS_TEXT_BASE seems to not be defined for EFI and then LD gets a string like "-Ttext -o u-boot" where CONFIG_SYS_TEXT_BASE is supposed to be used as some value.

[snip]

Quoted 27 lines

Quoted 27 lines
So if "-Ttext CONFIG_SYS_TEXT_BASE" is not used for each and every board I may use CONFIG_SYS_TEXT_BASE directly in linker just as we have it now, see
https://urldefense.proofpoint.com/v2/url?u=http-3A__git.denx.de_-3Fp-3Du-2Dboot.git-3Ba-3Dblob-3Bf-3Darch_arc_cpu_u-2Dboot.lds-23l14&d=DwIBAg&c=DP
L6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=3oKsgMcKriZJ81auV4XDwr4ovIElxycDMpRlTqS7nhc&s=Yq0YW- MQvofxwQ961IRHzIS5xWYtHHzxMp_Fwn91h5s&e=
and then there might be any section like .ivt, .text, .myfunkysection etc.
In fact in the Linux kernel "-Ttext XXX" is not used for everybody - some arches like MIPS and PPC indeed set it but others do other things.
The simplest thing might be is to add another #ifdef for ARC and X86 which both use CONFIG_SYS_TEXT_BASE directly in their linker scripts like that:
------------------->8-----------------
Makefile +2 -1
@@ -820,9 +820,11 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
+ifeq($(CONFIG_ARC)$(CONFIG_X86),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
+endif
 
 # Normally we fill empty space with 0xff
 quiet_cmd_objcopy = OBJCOPY $@
------------------->8-----------------
 

Quoted 5 lines

Quoted 1 line
Any thoughts?
I'm largely ok with the above, but:
- You need to exclude CONFIG_NIOS2 in the above as well, or convert the usage of CONFIG_SYS_MONITOR_BASE to CONFIG_SYS_TEXT_BASE (Thomas?)

Looks like NIOS2 builds are not yet supported in TravisCI which is quite unfortunate... are there any instructions on how to build for Nios2 so that I may test my changes before posting them?

-Alexey

Tom Rini

2018-02-12 16:29 UTC #11 lore raw

Quoted 72 lines · Alexey Brodkin

Quoted 35 lines · Tom Rini
Quoted 34 lines · Alexey Brodkin
Hi Tom, Simon,
Quoted 21 lines · Tom Rini
Quoted 19 lines · Alexey Brodkin
CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks so there's no point in keeping this check araound just in one place.
Signed-off-by Alexey Brodkin <[email protected]>
Cc Tom Rini <[email protected]>
Acked-by Masahiro Yamada <[email protected]>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)
Makefile +0 -0
index ab3453dcebdc..6f15612b4d07 100644
@@ -820,9 +820,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
Quoted 6 lines
Makefile LDFLAGS_u-boot += $(LDFLAGS_FINAL) · line 825
Quoted 3 lines
Makefile +0 -2
-ifneq ($(CONFIG_SYS_TEXT_BASE),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
-endif
This then causes xtensa to fail to build as it does not set CONFIG_SYS_TEXT_BASE.
And that also obviously breaks "efi-x86" target as well because CONFIG_SYS_TEXT_BASE seems to not be defined for EFI and then LD gets a string like "-Ttext -o u-boot" where CONFIG_SYS_TEXT_BASE is supposed to be used as some value.
[snip]
Quoted 27 lines
Quoted 27 lines
So if "-Ttext CONFIG_SYS_TEXT_BASE" is not used for each and every board I may use CONFIG_SYS_TEXT_BASE directly in linker just as we have it now, see
https://urldefense.proofpoint.com/v2/url?u=http-3A__git.denx.de_-3Fp-3Du-2Dboot.git-3Ba-3Dblob-3Bf-3Darch_arc_cpu_u-2Dboot.lds-23l14&d=DwIBAg&c=DP
L6_X_6JkXFx7AXWqB0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=3oKsgMcKriZJ81auV4XDwr4ovIElxycDMpRlTqS7nhc&s=Yq0YW- MQvofxwQ961IRHzIS5xWYtHHzxMp_Fwn91h5s&e=
and then there might be any section like .ivt, .text, .myfunkysection etc.
In fact in the Linux kernel "-Ttext XXX" is not used for everybody - some arches like MIPS and PPC indeed set it but others do other things.
The simplest thing might be is to add another #ifdef for ARC and X86 which both use CONFIG_SYS_TEXT_BASE directly in their linker scripts like that:
------------------->8-----------------
Makefile +2 -1
@@ -820,9 +820,11 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL)
 # Avoid 'Not enough room for program headers' error on binutils 2.28 onwards.
 LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker)
 
+ifeq($(CONFIG_ARC)$(CONFIG_X86),)
 LDFLAGS_u-boot += -Ttext $(CONFIG_SYS_TEXT_BASE)
+endif
 
 # Normally we fill empty space with 0xff
 quiet_cmd_objcopy = OBJCOPY $@
------------------->8-----------------
 
Quoted 5 lines
Quoted 1 line
Any thoughts?
I'm largely ok with the above, but:
- You need to exclude CONFIG_NIOS2 in the above as well, or convert the usage of CONFIG_SYS_MONITOR_BASE to CONFIG_SYS_TEXT_BASE (Thomas?)

Quoted 3 lines

Looks like NIOS2 builds are not yet supported in TravisCI which is quite unfortunate... are there any instructions on how to build for Nios2 so that I may test my changes before posting them?

So, part of the issue is that we need newer kernel.org toolchains
(https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/7.3.0/) in order for buildman to be able to grab a nios2 toolchain, and that in turn has its own issues. You can however grab one of those (or the 6.4.0) version manually, extract into ~/.buildman/ and then just let buildman build the arch.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180212/8fd13625/attachment.sig>

Max Filippov

2018-02-12 20:48 UTC #12 lore raw

In reply to Tom Rini

Hello,

Quoted 2 lines · Alexey Brodkin

CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks

Why? xtensa doesn't use any of it.

Quoted 3 lines · Tom Rini

I'm largely ok with the above, but:
- For Xtensa (Max?), CONFIG_SYS_TEXT_ADDR needs to be renamed to CONFIG_SYS_TEXT_BASE there

For xtensa that address is defined as an expression, like (CONFIG_SYS_MEMORY_TOP - CONFIG_SYS_MONITOR_LEN),
and for a single board it may vary with the CPU core on that board. If I just do this replacement then linking fails because of this symbolic definition:

xtensa-dc233c-elf-ld.bfd --gc-sections -Bstatic
--no-dynamic-linker -Ttext "(CONFIG_SYS_MEMORY_TOP - CONFIG_SYS_MONITOR_LEN)"
...
xtensa-dc233c-elf-ld.bfd: invalid hex number `(CONFIG_SYS_MEMORY_TOP
- CONFIG_SYS_MONITOR_LEN)'

Signature · 2 lines

Thanks.
-- Max

Tom Rini

2018-02-12 21:03 UTC #13 lore raw

Quoted 20 lines · Max Filippov

Hello,
Quoted 2 lines · Alexey Brodkin
CONFIG_SYS_TEXT_BASE must be set anyways and then it is used in many places in the same Makefile without any checks
Why? xtensa doesn't use any of it.
Quoted 3 lines · Tom Rini
I'm largely ok with the above, but:
- For Xtensa (Max?), CONFIG_SYS_TEXT_ADDR needs to be renamed to CONFIG_SYS_TEXT_BASE there
For xtensa that address is defined as an expression, like (CONFIG_SYS_MEMORY_TOP - CONFIG_SYS_MONITOR_LEN),
and for a single board it may vary with the CPU core on that board. If I just do this replacement then linking fails because of this symbolic definition:

Quoted 6 lines

xtensa-dc233c-elf-ld.bfd --gc-sections -Bstatic
--no-dynamic-linker -Ttext "(CONFIG_SYS_MEMORY_TOP - CONFIG_SYS_MONITOR_LEN)"
...
xtensa-dc233c-elf-ld.bfd: invalid hex number `(CONFIG_SYS_MEMORY_TOP
- CONFIG_SYS_MONITOR_LEN)'

OK. But are those also really dynamic values? SYS_MEMORY_TOP, SYS_MONITOR_LEN and SYS_TEXT_ADDR need to be converted to Kconfig, or removed from CONFIG namespace, whatever makes the most sense. If the notion of CONFIG_SYS_TEXT_BASE is just pointless to Xtensa, we add them to the ifneq(...,) test for the change Alexey is doing. Thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180212/40a6d8ba/attachment.sig>

Max Filippov

2018-02-12 21:48 UTC #14 lore raw

Quoted 5 lines · Tom Rini

Quoted 5 lines · Tom Rini
Quoted 3 lines
I'm largely ok with the above, but:
- For Xtensa (Max?), CONFIG_SYS_TEXT_ADDR needs to be renamed to CONFIG_SYS_TEXT_BASE there

Quoted 7 lines

Quoted 3 lines
For xtensa that address is defined as an expression, like (CONFIG_SYS_MEMORY_TOP - CONFIG_SYS_MONITOR_LEN),
and for a single board it may vary with the CPU core on that board.
OK. But are those also really dynamic values? SYS_MEMORY_TOP, SYS_MONITOR_LEN and SYS_TEXT_ADDR need to be converted to Kconfig, or removed from CONFIG namespace, whatever makes the most sense. If the

At least two of the three -- yes, in a sense that they cannot be calculated without the core-specific configuration overlay.
I'll move SYS_MONITOR_LEN to the Kconfig and move SYS_MEMORY_TOP and SYS_TEXT_ADDR from the CONFIG namespace.

Quoted 2 lines

notion of CONFIG_SYS_TEXT_BASE is just pointless to Xtensa, we add them to the ifneq(...,) test for the change Alexey is doing. Thanks!

Yes, please.

Signature · 2 lines

Thanks.
-- Max