| Message ID | 20180130152313.28099-1-abrodkin@synopsys.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] Build system: Don't check for CONFIG_SYS_TEXT_BASE being set | expand |
2018-01-31 0:23 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > 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 <abrodkin@synopsys.com> > Cc: Tom Rini <trini@konsulko.com> > --- > Makefile | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/Makefile b/Makefile > index ab3453dcebdc..6f15612b4d07 100644 > --- a/Makefile > +++ b/Makefile > @@ -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 <yamada.masahiro@socionext.com> > # Normally we fill empty space with 0xff > quiet_cmd_objcopy = OBJCOPY $@ > -- > 2.14.3 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi MAsahiro-san, Tom, On Wed, 2018-01-31 at 01:23 +0900, Masahiro Yamada wrote: > 2018-01-31 0:23 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > > > 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. Should I send a respin with this valuable addition or whoever's going to apply will fix-up this? -Alexey
2018-01-31 1:28 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > Hi MAsahiro-san, Tom, > > On Wed, 2018-01-31 at 01:23 +0900, Masahiro Yamada wrote: >> 2018-01-31 0:23 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: >> > >> 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. > > 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.
Hi Masahiro-san, On Thu, 2018-02-01 at 00:18 +0900, Masahiro Yamada wrote: > 2018-01-31 1:28 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > > Hi MAsahiro-san, Tom, > > > > On Wed, 2018-01-31 at 01:23 +0900, Masahiro Yamada wrote: > > > 2018-01-31 0:23 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > > > > > > > > > > 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. > > > > 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
2018-02-01 0:21 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: > Hi Masahiro-san, > > On Thu, 2018-02-01 at 00:18 +0900, Masahiro Yamada wrote: >> 2018-01-31 1:28 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: >> > Hi MAsahiro-san, Tom, >> > >> > On Wed, 2018-01-31 at 01:23 +0900, Masahiro Yamada wrote: >> > > 2018-01-31 0:23 GMT+09:00 Alexey Brodkin <Alexey.Brodkin@synopsys.com>: >> > > > >> > > >> > > 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. >> > >> > 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 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.
On Tue, Jan 30, 2018 at 06:23:13PM +0300, Alexey Brodkin wrote: > 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 <abrodkin@synopsys.com> > Cc: Tom Rini <trini@konsulko.com> > Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > Makefile | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/Makefile b/Makefile > index ab3453dcebdc..6f15612b4d07 100644 > --- a/Makefile > +++ b/Makefile > @@ -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.
Hi Tom, Simon, On Sun, 2018-02-11 at 15:47 -0500, Tom Rini wrote: > On Tue, Jan 30, 2018 at 06:23:13PM +0300, Alexey Brodkin wrote: > > > 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 <abrodkin@synopsys.com> > > Cc: Tom Rini <trini@konsulko.com> > > Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > --- > > Makefile | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index ab3453dcebdc..6f15612b4d07 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -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. 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----------------- --- a/Makefile +++ b/Makefile @@ -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
On Mon, Feb 12, 2018 at 01:27:30PM +0000, Alexey Brodkin wrote: > Hi Tom, Simon, > > On Sun, 2018-02-11 at 15:47 -0500, Tom Rini wrote: > > On Tue, Jan 30, 2018 at 06:23:13PM +0300, Alexey Brodkin wrote: > > > > > 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 <abrodkin@synopsys.com> > > > Cc: Tom Rini <trini@konsulko.com> > > > Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > --- > > > Makefile | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/Makefile b/Makefile > > > index ab3453dcebdc..6f15612b4d07 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -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. > > 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 Thanks for explaining what's going on with ARC. > 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----------------- > --- a/Makefile > +++ b/Makefile > @@ -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? 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!
On 02/12/2018 03:23 PM, Tom Rini wrote: > On Mon, Feb 12, 2018 at 01:27:30PM +0000, Alexey Brodkin wrote: >> Hi Tom, Simon, >> >> On Sun, 2018-02-11 at 15:47 -0500, Tom Rini wrote: >>> On Tue, Jan 30, 2018 at 06:23:13PM +0300, Alexey Brodkin wrote: >>> >>>> 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 <abrodkin@synopsys.com> >>>> Cc: Tom Rini <trini@konsulko.com> >>>> Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com> >>>> --- >>>> Makefile | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/Makefile b/Makefile >>>> index ab3453dcebdc..6f15612b4d07 100644 >>>> --- a/Makefile >>>> +++ b/Makefile >>>> @@ -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. >> >> 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 > > Thanks for explaining what's going on with ARC. > >> 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----------------- >> --- a/Makefile >> +++ b/Makefile >> @@ -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? > > 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 . > - For Xtensa (Max?), CONFIG_SYS_TEXT_ADDR needs to be renamed to > CONFIG_SYS_TEXT_BASE there > > Thanks! >
On Mon, 2018-02-12 at 09:23 -0500, Tom Rini wrote: > On Mon, Feb 12, 2018 at 01:27:30PM +0000, Alexey Brodkin wrote: > > Hi Tom, Simon, > > > > On Sun, 2018-02-11 at 15:47 -0500, Tom Rini wrote: > > > On Tue, Jan 30, 2018 at 06:23:13PM +0300, Alexey Brodkin wrote: > > > > > > > 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 <abrodkin@synopsys.com> > > > > Cc: Tom Rini <trini@konsulko.com> > > > > Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > --- > > > > Makefile | 2 -- > > > > 1 file changed, 2 deletions(-) > > > > > > > > diff --git a/Makefile b/Makefile > > > > index ab3453dcebdc..6f15612b4d07 100644 > > > > --- a/Makefile > > > > +++ b/Makefile > > > > @@ -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. > > > > 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] > > 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----------------- > > --- a/Makefile > > +++ b/Makefile > > @@ -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? > > 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
On Mon, Feb 12, 2018 at 04:21:09PM +0000, Alexey Brodkin wrote: > On Mon, 2018-02-12 at 09:23 -0500, Tom Rini wrote: > > On Mon, Feb 12, 2018 at 01:27:30PM +0000, Alexey Brodkin wrote: > > > Hi Tom, Simon, > > > > > > On Sun, 2018-02-11 at 15:47 -0500, Tom Rini wrote: > > > > On Tue, Jan 30, 2018 at 06:23:13PM +0300, Alexey Brodkin wrote: > > > > > > > > > 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 <abrodkin@synopsys.com> > > > > > Cc: Tom Rini <trini@konsulko.com> > > > > > Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > > --- > > > > > Makefile | 2 -- > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > index ab3453dcebdc..6f15612b4d07 100644 > > > > > --- a/Makefile > > > > > +++ b/Makefile > > > > > @@ -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. > > > > > > 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] > > > > 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----------------- > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -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? > > > > 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? 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.
Hello, On Tue, Jan 30, 2018 at 7:23 AM, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote: > 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. On Mon, Feb 12, 2018 at 6:23 AM, Tom Rini <trini@konsulko.com> wrote: > 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)'
On Mon, Feb 12, 2018 at 12:48:02PM -0800, Max Filippov wrote: > Hello, > > On Tue, Jan 30, 2018 at 7:23 AM, Alexey Brodkin > <Alexey.Brodkin@synopsys.com> wrote: > > 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. > > On Mon, Feb 12, 2018 at 6:23 AM, Tom Rini <trini@konsulko.com> wrote: > > 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)' 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!
On Mon, Feb 12, 2018 at 1:03 PM, Tom Rini <trini@konsulko.com> wrote: >> On Mon, Feb 12, 2018 at 6:23 AM, Tom Rini <trini@konsulko.com> wrote: >> > 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. > > 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. > 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.
diff --git a/Makefile b/Makefile index ab3453dcebdc..6f15612b4d07 100644 --- a/Makefile +++ b/Makefile @@ -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 $@
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 <abrodkin@synopsys.com> Cc: Tom Rini <trini@konsulko.com> --- Makefile | 2 -- 1 file changed, 2 deletions(-)