| Message ID | 1384196535-19289-1-git-send-email-juhosg@openwrt.org |
|---|---|
| State | Rejected |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
2013/11/11 Gabor Juhos <juhosg@openwrt.org>: > The relocation code uses the CONFIG_SYS_MONITOR_BASE > constant for calculating the reserved memory size and > relocation offset values. Along with these predefined > values the code also uses several other constants which > are computed by the linker from the CONFIG_SYS_TEXT_BASE > value. Due to this, the relocation code works incorreclty > if the CONFIG_SYS_TEXT_BASE and CONFIG_SYS_MONITOR_BASE > values are different. > > Change the relocation code to use the CONFIG_SYS_TEXT_BASE > constant in the calculations to avoid the problem. > > Only tested in qemu with the 'malta' and 'qemu_mips{,el}' > targets. > > Signed-off-by: Gabor Juhos <juhosg@openwrt.org> > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > Cc: Paul Burton <paul.burton@imgtec.com> > --- > Daniel, > > This should be merged before my 'malta: use unmapped flash base address' > patch. > > Thanks, > Gabor thanks, I'll test it on real hardware.
2013/11/11 Gabor Juhos <juhosg@openwrt.org>: > The relocation code uses the CONFIG_SYS_MONITOR_BASE > constant for calculating the reserved memory size and > relocation offset values. Along with these predefined > values the code also uses several other constants which > are computed by the linker from the CONFIG_SYS_TEXT_BASE > value. Due to this, the relocation code works incorreclty > if the CONFIG_SYS_TEXT_BASE and CONFIG_SYS_MONITOR_BASE > values are different. to be consistent with all other architectures, we should keep CONFIG_SYS_MONITOR_BASE. I think it is neither valid nor intentional to use a value different from CONFIG_SYS_TEXT_BASE. Instead we should change include/configs/malta.h: -#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_FLASH_BASE +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE Comments?
2013.11.11. 20:36 keltezéssel, Daniel Schwierzeck írta: > 2013/11/11 Gabor Juhos <juhosg@openwrt.org>: >> The relocation code uses the CONFIG_SYS_MONITOR_BASE >> constant for calculating the reserved memory size and >> relocation offset values. Along with these predefined >> values the code also uses several other constants which >> are computed by the linker from the CONFIG_SYS_TEXT_BASE >> value. Due to this, the relocation code works incorreclty >> if the CONFIG_SYS_TEXT_BASE and CONFIG_SYS_MONITOR_BASE >> values are different. > > to be consistent with all other architectures, we should keep > CONFIG_SYS_MONITOR_BASE. I think it is neither valid nor intentional > to use a value different from CONFIG_SYS_TEXT_BASE. If it is neither valid nor intentional, the CONFIG_SYS_MONITOR_BASE constant should not exist at all and CONFIG_SYS_TEXT_BASE should be used everywhere instead IMHO. Additionally, we have this check in arch/mips/lib/board.c: > #if CONFIG_SYS_MONITOR_BASE == CONFIG_SYS_FLASH_BASE > bd->bi_flashoffset = monitor_flash_len; /* reserved area for U-Boot */ > #else > bd->bi_flashoffset = 0; > #endif If it is not allowed to use different values for the two constants, the condition and the #else branch should be removed. > Instead we should change include/configs/malta.h: > > -#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_FLASH_BASE > +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE > > > Comments? I have tried this already. It is working as well, however with this change the flash sectors containing the bootloader are not protected correctly: > malta # flinfo > > Bank # 1: CFI conformant flash (32 x 32) Size: 4 MB in 64 Sectors > Intel Extended command set, Manufacturer ID: 0x00, Device ID: 0x00 > Erase timeout: 16384 ms, write timeout: 3 ms > Buffer write timeout: 3 ms, buffer size: 2048 bytes > > Sector Start Addresses: > BE000000 BE010000 BE020000 BE030000 BE040000 > BE050000 BE060000 BE070000 BE080000 BE090000 > BE0A0000 BE0B0000 BE0C0000 BE0D0000 BE0E0000 > BE0F0000 BE100000 BE110000 BE120000 BE130000 > BE140000 BE150000 BE160000 BE170000 BE180000 > BE190000 BE1A0000 BE1B0000 BE1C0000 BE1D0000 > BE1E0000 BE1F0000 BE200000 BE210000 BE220000 > BE230000 BE240000 BE250000 BE260000 BE270000 > BE280000 BE290000 BE2A0000 BE2B0000 BE2C0000 > BE2D0000 BE2E0000 BE2F0000 BE300000 BE310000 > BE320000 BE330000 BE340000 BE350000 BE360000 > BE370000 BE380000 BE390000 BE3A0000 BE3B0000 > BE3C0000 BE3D0000 BE3E0000 RO BE3F0000 RO > malta # For reference, this is the output of flinfo with my change: > malta # flinfo > > Bank # 1: CFI conformant flash (32 x 32) Size: 4 MB in 64 Sectors > Intel Extended command set, Manufacturer ID: 0x00, Device ID: 0x00 > Erase timeout: 16384 ms, write timeout: 3 ms > Buffer write timeout: 3 ms, buffer size: 2048 bytes > > Sector Start Addresses: > BE000000 RO BE010000 RO BE020000 RO BE030000 BE040000 > BE050000 BE060000 BE070000 BE080000 BE090000 > BE0A0000 BE0B0000 BE0C0000 BE0D0000 BE0E0000 > BE0F0000 BE100000 BE110000 BE120000 BE130000 > BE140000 BE150000 BE160000 BE170000 BE180000 > BE190000 BE1A0000 BE1B0000 BE1C0000 BE1D0000 > BE1E0000 BE1F0000 BE200000 BE210000 BE220000 > BE230000 BE240000 BE250000 BE260000 BE270000 > BE280000 BE290000 BE2A0000 BE2B0000 BE2C0000 > BE2D0000 BE2E0000 BE2F0000 BE300000 BE310000 > BE320000 BE330000 BE340000 BE350000 BE360000 > BE370000 BE380000 BE390000 BE3A0000 BE3B0000 > BE3C0000 BE3D0000 BE3E0000 RO BE3F0000 RO > malta # Any idea how can we resolve this properly? -Gabor
2013/11/11 Gabor Juhos <juhosg@openwrt.org>: > 2013.11.11. 20:36 keltezéssel, Daniel Schwierzeck írta: >> 2013/11/11 Gabor Juhos <juhosg@openwrt.org>: >>> The relocation code uses the CONFIG_SYS_MONITOR_BASE >>> constant for calculating the reserved memory size and >>> relocation offset values. Along with these predefined >>> values the code also uses several other constants which >>> are computed by the linker from the CONFIG_SYS_TEXT_BASE >>> value. Due to this, the relocation code works incorreclty >>> if the CONFIG_SYS_TEXT_BASE and CONFIG_SYS_MONITOR_BASE >>> values are different. >> >> to be consistent with all other architectures, we should keep >> CONFIG_SYS_MONITOR_BASE. I think it is neither valid nor intentional >> to use a value different from CONFIG_SYS_TEXT_BASE. > > If it is neither valid nor intentional, the CONFIG_SYS_MONITOR_BASE constant > should not exist at all and CONFIG_SYS_TEXT_BASE should be used everywhere > instead IMHO. By now it is redundant. Once CONFIG_SYS_TEXT_BASE was only a make variable named TEXT_BASE and defined in board-specific config.mk. This has been converted with commit 14d0a02a168b36e87665b8d7f42fa3e88263d26d. BTW the README states: - CONFIG_SYS_MONITOR_BASE: Physical start address of boot monitor code (set by make config files to be same as the text base address (CONFIG_SYS_TEXT_BASE) used when linking) - same as CONFIG_SYS_FLASH_BASE when booting from flash. > > Additionally, we have this check in arch/mips/lib/board.c: > >> #if CONFIG_SYS_MONITOR_BASE == CONFIG_SYS_FLASH_BASE >> bd->bi_flashoffset = monitor_flash_len; /* reserved area for U-Boot */ >> #else >> bd->bi_flashoffset = 0; >> #endif > > If it is not allowed to use different values for the two constants, > the condition and the #else branch should be removed. no that is still needed if a board has NOR flash but boots from NAND or SPI flash or other media (e.g. SoC evaluation boards). In that case CONFIG_SYS_TEXT_BASE points usually to an SRAM address. > >> Instead we should change include/configs/malta.h: >> >> -#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_FLASH_BASE >> +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE >> >> >> Comments? > > I have tried this already. It is working as well, however with this change the > flash sectors containing the bootloader are not protected correctly: > >> malta # flinfo >> >> Bank # 1: CFI conformant flash (32 x 32) Size: 4 MB in 64 Sectors >> Intel Extended command set, Manufacturer ID: 0x00, Device ID: 0x00 >> Erase timeout: 16384 ms, write timeout: 3 ms >> Buffer write timeout: 3 ms, buffer size: 2048 bytes >> >> Sector Start Addresses: >> BE000000 BE010000 BE020000 BE030000 BE040000 >> BE050000 BE060000 BE070000 BE080000 BE090000 >> BE0A0000 BE0B0000 BE0C0000 BE0D0000 BE0E0000 >> BE0F0000 BE100000 BE110000 BE120000 BE130000 >> BE140000 BE150000 BE160000 BE170000 BE180000 >> BE190000 BE1A0000 BE1B0000 BE1C0000 BE1D0000 >> BE1E0000 BE1F0000 BE200000 BE210000 BE220000 >> BE230000 BE240000 BE250000 BE260000 BE270000 >> BE280000 BE290000 BE2A0000 BE2B0000 BE2C0000 >> BE2D0000 BE2E0000 BE2F0000 BE300000 BE310000 >> BE320000 BE330000 BE340000 BE350000 BE360000 >> BE370000 BE380000 BE390000 BE3A0000 BE3B0000 >> BE3C0000 BE3D0000 BE3E0000 RO BE3F0000 RO >> malta # > > For reference, this is the output of flinfo with my change: > >> malta # flinfo >> >> Bank # 1: CFI conformant flash (32 x 32) Size: 4 MB in 64 Sectors >> Intel Extended command set, Manufacturer ID: 0x00, Device ID: 0x00 >> Erase timeout: 16384 ms, write timeout: 3 ms >> Buffer write timeout: 3 ms, buffer size: 2048 bytes >> >> Sector Start Addresses: >> BE000000 RO BE010000 RO BE020000 RO BE030000 BE040000 >> BE050000 BE060000 BE070000 BE080000 BE090000 >> BE0A0000 BE0B0000 BE0C0000 BE0D0000 BE0E0000 >> BE0F0000 BE100000 BE110000 BE120000 BE130000 >> BE140000 BE150000 BE160000 BE170000 BE180000 >> BE190000 BE1A0000 BE1B0000 BE1C0000 BE1D0000 >> BE1E0000 BE1F0000 BE200000 BE210000 BE220000 >> BE230000 BE240000 BE250000 BE260000 BE270000 >> BE280000 BE290000 BE2A0000 BE2B0000 BE2C0000 >> BE2D0000 BE2E0000 BE2F0000 BE300000 BE310000 >> BE320000 BE330000 BE340000 BE350000 BE360000 >> BE370000 BE380000 BE390000 BE3A0000 BE3B0000 >> BE3C0000 BE3D0000 BE3E0000 RO BE3F0000 RO >> malta # > > Any idea how can we resolve this properly? > > -Gabor following seems to work (in both variants with -bios and -pflash) #define CONFIG_SYS_TEXT_BASE 0xbe000000 #define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE
2013.11.11. 23:36 keltezéssel, Daniel Schwierzeck írta: <...> >>> to be consistent with all other architectures, we should keep >>> CONFIG_SYS_MONITOR_BASE. I think it is neither valid nor intentional >>> to use a value different from CONFIG_SYS_TEXT_BASE. >> >> If it is neither valid nor intentional, the CONFIG_SYS_MONITOR_BASE constant >> should not exist at all and CONFIG_SYS_TEXT_BASE should be used everywhere >> instead IMHO. > > By now it is redundant. Once CONFIG_SYS_TEXT_BASE was only a make > variable named TEXT_BASE and defined in board-specific config.mk. This > has been converted with commit > 14d0a02a168b36e87665b8d7f42fa3e88263d26d. > > BTW the README states: > > - CONFIG_SYS_MONITOR_BASE: > Physical start address of boot monitor code (set by > make config files to be same as the text base address > (CONFIG_SYS_TEXT_BASE) used when linking) - same as > CONFIG_SYS_FLASH_BASE when booting from flash. I see. Thank you for the explanation. >> >> Additionally, we have this check in arch/mips/lib/board.c: >> >>> #if CONFIG_SYS_MONITOR_BASE == CONFIG_SYS_FLASH_BASE >>> bd->bi_flashoffset = monitor_flash_len; /* reserved area for U-Boot */ >>> #else >>> bd->bi_flashoffset = 0; >>> #endif >> >> If it is not allowed to use different values for the two constants, >> the condition and the #else branch should be removed. > > no that is still needed if a board has NOR flash but boots from NAND > or SPI flash or other media (e.g. SoC evaluation boards). In that case > CONFIG_SYS_TEXT_BASE points usually to an SRAM address. Ok. > >> >>> Instead we should change include/configs/malta.h: >>> >>> -#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_FLASH_BASE >>> +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE >>> >>> >>> Comments? >> >> I have tried this already. It is working as well, however with this change the >> flash sectors containing the bootloader are not protected correctly: >> >>> malta # flinfo >>> >>> Bank # 1: CFI conformant flash (32 x 32) Size: 4 MB in 64 Sectors >>> Intel Extended command set, Manufacturer ID: 0x00, Device ID: 0x00 >>> Erase timeout: 16384 ms, write timeout: 3 ms >>> Buffer write timeout: 3 ms, buffer size: 2048 bytes >>> >>> Sector Start Addresses: >>> BE000000 BE010000 BE020000 BE030000 BE040000 >>> BE050000 BE060000 BE070000 BE080000 BE090000 >>> BE0A0000 BE0B0000 BE0C0000 BE0D0000 BE0E0000 >>> BE0F0000 BE100000 BE110000 BE120000 BE130000 >>> BE140000 BE150000 BE160000 BE170000 BE180000 >>> BE190000 BE1A0000 BE1B0000 BE1C0000 BE1D0000 >>> BE1E0000 BE1F0000 BE200000 BE210000 BE220000 >>> BE230000 BE240000 BE250000 BE260000 BE270000 >>> BE280000 BE290000 BE2A0000 BE2B0000 BE2C0000 >>> BE2D0000 BE2E0000 BE2F0000 BE300000 BE310000 >>> BE320000 BE330000 BE340000 BE350000 BE360000 >>> BE370000 BE380000 BE390000 BE3A0000 BE3B0000 >>> BE3C0000 BE3D0000 BE3E0000 RO BE3F0000 RO >>> malta # >> >> For reference, this is the output of flinfo with my change: >> >>> malta # flinfo >>> >>> Bank # 1: CFI conformant flash (32 x 32) Size: 4 MB in 64 Sectors >>> Intel Extended command set, Manufacturer ID: 0x00, Device ID: 0x00 >>> Erase timeout: 16384 ms, write timeout: 3 ms >>> Buffer write timeout: 3 ms, buffer size: 2048 bytes >>> >>> Sector Start Addresses: >>> BE000000 RO BE010000 RO BE020000 RO BE030000 BE040000 >>> BE050000 BE060000 BE070000 BE080000 BE090000 >>> BE0A0000 BE0B0000 BE0C0000 BE0D0000 BE0E0000 >>> BE0F0000 BE100000 BE110000 BE120000 BE130000 >>> BE140000 BE150000 BE160000 BE170000 BE180000 >>> BE190000 BE1A0000 BE1B0000 BE1C0000 BE1D0000 >>> BE1E0000 BE1F0000 BE200000 BE210000 BE220000 >>> BE230000 BE240000 BE250000 BE260000 BE270000 >>> BE280000 BE290000 BE2A0000 BE2B0000 BE2C0000 >>> BE2D0000 BE2E0000 BE2F0000 BE300000 BE310000 >>> BE320000 BE330000 BE340000 BE350000 BE360000 >>> BE370000 BE380000 BE390000 BE3A0000 BE3B0000 >>> BE3C0000 BE3D0000 BE3E0000 RO BE3F0000 RO >>> malta # >> >> Any idea how can we resolve this properly? >> >> -Gabor > > following seems to work (in both variants with -bios and -pflash) > > #define CONFIG_SYS_TEXT_BASE 0xbe000000 > #define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE Yeah, this definitely looks better than my solution. :) Do you want me to incorporate this into the 'malta: use unmapped flash base address' patch, or do you want to apply this separately? -Gabor >
2013/11/12 Gabor Juhos <juhosg@openwrt.org>: > 2013.11.11. 23:36 keltezéssel, Daniel Schwierzeck írta: > ... >>> >>> Any idea how can we resolve this properly? >>> >>> -Gabor >> >> following seems to work (in both variants with -bios and -pflash) >> >> #define CONFIG_SYS_TEXT_BASE 0xbe000000 >> #define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE > > Yeah, this definitely looks better than my solution. :) > > Do you want me to incorporate this into the 'malta: use unmapped flash base > address' patch, yes, one patch is sufficiently > or do you want to apply this separately? >
2013.11.12. 16:40 keltezéssel, Daniel Schwierzeck írta: > 2013/11/12 Gabor Juhos <juhosg@openwrt.org>: >> 2013.11.11. 23:36 keltezéssel, Daniel Schwierzeck írta: >> > ... >>>> >>>> Any idea how can we resolve this properly? >>>> >>>> -Gabor >>> >>> following seems to work (in both variants with -bios and -pflash) >>> >>> #define CONFIG_SYS_TEXT_BASE 0xbe000000 >>> #define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE >> >> Yeah, this definitely looks better than my solution. :) >> >> Do you want me to incorporate this into the 'malta: use unmapped flash base >> address' patch, > > yes, one patch is sufficiently Ok.
diff --git a/arch/mips/cpu/mips32/start.S b/arch/mips/cpu/mips32/start.S index 68e59b5..13e2de7 100644 --- a/arch/mips/cpu/mips32/start.S +++ b/arch/mips/cpu/mips32/start.S @@ -159,7 +159,7 @@ relocate_code: move s0, a1 # save gd in s0 move s2, a2 # save destination address in s2 - li t0, CONFIG_SYS_MONITOR_BASE + li t0, CONFIG_SYS_TEXT_BASE sub s1, s2, t0 # s1 <-- relocation offset la t3, in_ram diff --git a/arch/mips/cpu/mips64/start.S b/arch/mips/cpu/mips64/start.S index 92954e1..4fa8bb1 100644 --- a/arch/mips/cpu/mips64/start.S +++ b/arch/mips/cpu/mips64/start.S @@ -153,7 +153,7 @@ relocate_code: move s0, a1 # save gd in s0 move s2, a2 # save destination address in s2 - dli t0, CONFIG_SYS_MONITOR_BASE + dli t0, CONFIG_SYS_TEXT_BASE dsub s1, s2, t0 # s1 <-- relocation offset dla t3, in_ram diff --git a/arch/mips/cpu/xburst/start.S b/arch/mips/cpu/xburst/start.S index 10dffb4..e9d9679 100644 --- a/arch/mips/cpu/xburst/start.S +++ b/arch/mips/cpu/xburst/start.S @@ -50,7 +50,7 @@ relocate_code: move s0, a1 # save gd in s0 move s2, a2 # save destination address in s2 - li t0, CONFIG_SYS_MONITOR_BASE + li t0, CONFIG_SYS_TEXT_BASE sub s1, s2, t0 # s1 <-- relocation offset la t3, in_ram diff --git a/arch/mips/lib/board.c b/arch/mips/lib/board.c index 9e6ba15..82efa5a 100644 --- a/arch/mips/lib/board.c +++ b/arch/mips/lib/board.c @@ -160,7 +160,7 @@ void board_init_f(ulong bootflag) /* Reserve memory for U-Boot code, data & bss * round down to next 16 kB limit */ - len = bss_end() - CONFIG_SYS_MONITOR_BASE; + len = bss_end() - CONFIG_SYS_TEXT_BASE; addr -= len; addr &= ~(16 * 1024 - 1);
The relocation code uses the CONFIG_SYS_MONITOR_BASE constant for calculating the reserved memory size and relocation offset values. Along with these predefined values the code also uses several other constants which are computed by the linker from the CONFIG_SYS_TEXT_BASE value. Due to this, the relocation code works incorreclty if the CONFIG_SYS_TEXT_BASE and CONFIG_SYS_MONITOR_BASE values are different. Change the relocation code to use the CONFIG_SYS_TEXT_BASE constant in the calculations to avoid the problem. Only tested in qemu with the 'malta' and 'qemu_mips{,el}' targets. Signed-off-by: Gabor Juhos <juhosg@openwrt.org> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> Cc: Paul Burton <paul.burton@imgtec.com> --- Daniel, This should be merged before my 'malta: use unmapped flash base address' patch. Thanks, Gabor --- arch/mips/cpu/mips32/start.S | 2 +- arch/mips/cpu/mips64/start.S | 2 +- arch/mips/cpu/xburst/start.S | 2 +- arch/mips/lib/board.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)