ARM: tegra: Fix misplaced tegra_uart_config in decompressor
Subsystems (2)
PATCH 1/1 ARM: tegra: Fix misplaced tegra_uart_config in decompressor
The tegra_uart_config of the DEBUG_LL code is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore if DEBUG_LL is enabled since tegra_uart_config data is executes as a code. Fix the misplaced tegra_uart_config storage by embedding it into the code.
--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)
arch/arm/include/debug/tegra.S +27 -27
@@ -149,7 +149,34 @@ .align 99: .word . +#if defined(ZIMAGE) + .word . + 4 +/* + * Storage for the state maintained by the macro. + * + * In the kernel proper, this data is located in arch/arm/mach-tegra/tegra.c. + * That's because this header is included from multiple files, and we only + * want a single copy of the data. In particular, the UART probing code above + * assumes it's running using physical addresses. This is true when this file + * is included from head.o, but not when included from debug.o. So we need + * to share the probe results between the two copies, rather than having + * to re-run the probing again later. + * + * In the decompressor, we put the storage right here, since common.c + * isn't included in the decompressor build. This storage data gets put in + * .text even though it's really data, since .data is discarded from the + * decompressor. Luckily, .text is writeable in the decompressor, unless + * CONFIG_ZBOOT_ROM. That dependency is handled in arch/arm/Kconfig.debug. + */ + /* Debug UART initialization required */ + .word 1 + /* Debug UART physical address */ + .word 0 + /* Debug UART virtual address */ + .word 0 +#else .word tegra_uart_config +#endif .ltorg /* Load previously selected UART address */
@@ -189,30 +216,3 @@ .macro waituarttxrdy,rd,rx .endm - -/* - * Storage for the state maintained by the macros above. - * - * In the kernel proper, this data is located in arch/arm/mach-tegra/tegra.c. - * That's because this header is included from multiple files, and we only - * want a single copy of the data. In particular, the UART probing code above - * assumes it's running using physical addresses. This is true when this file - * is included from head.o, but not when included from debug.o. So we need - * to share the probe results between the two copies, rather than having - * to re-run the probing again later. - * - * In the decompressor, we put the symbol/storage right here, since common.c - * isn't included in the decompressor build. This symbol gets put in .text - * even though it's really data, since .data is discarded from the - * decompressor. Luckily, .text is writeable in the decompressor, unless - * CONFIG_ZBOOT_ROM. That dependency is handled in arch/arm/Kconfig.debug. - */ -#if defined(ZIMAGE) -tegra_uart_config: - /* Debug UART initialization required */ - .word 1 - /* Debug UART physical address */ - .word 0 - /* Debug UART virtual address */ - .word 0 -#endif
Signature · 1 line
2.29.2
Quoted 9 lines · Dmitry Osipenko
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>
Aha I see the problem.
Please put this into Russell's patch tracker as a fix!
Yours,
Linus Walleij
15.12.2020 17:28, Linus Walleij пишет:
Quoted 8 lines · Dmitry Osipenko
Quoted 7 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.
Quoted 8 lines
Quoted 3 lines
Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>Aha I see the problem.
Reviewed-by Linus Walleij <[email protected]>Please put this into Russell's patch tracker as a fix!
Thanks, I sent the patch to the tracker.
Quoted 6 lines · Dmitry Osipenko
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.
Quoted 6 lines
Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)
Looks like arch/arm/include/debug/brcmstb.S would need the same treatment since the implementation was copied from tegra.S.
Signature · 1 line
Florian
15.12.2020 19:04, Florian Fainelli пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 6 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Quoted 6 lines
Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)
Quoted 3 lines
Looks like
arch/arm/include/debug/brcmstb.Swould need the same treatment since the implementation was copied fromtegra.S.
Good catch, will you be able to test the brcm and make a patch?
Quoted 17 lines · Dmitry Osipenko
15.12.2020 19:04, Florian Fainelli пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 12 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)
Quoted 5 lines
Quoted 3 lines
Looks like
arch/arm/include/debug/brcmstb.Swould need the same treatment since the implementation was copied fromtegra.S.Good catch, will you be able to test the brcm and make a patch?
Yes, absolutely, building a kernel to test right now.
Signature · 1 line
Florian
15.12.2020 19:40, Florian Fainelli пишет:
Quoted 20 lines · Dmitry Osipenko
Quoted 17 lines
15.12.2020 19:04, Florian Fainelli пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 12 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Looks like
arch/arm/include/debug/brcmstb.Swould need the same treatment since the implementation was copied fromtegra.S.Good catch, will you be able to test the brcm and make a patch?
Yes, absolutely, building a kernel to test right now.
Thank you.
BTW, I noticed that the problem is more visible on a thumb2 kernel build, i.e. you should get a more reliable hang on boot. On a non-thumb2 kernel the hanging behaviour seems depends on a device / bootloader. I haven't tried to figure out what exactly makes the difference, perhaps it should be a memory layout / state.
Quoted 30 lines · Dmitry Osipenko
15.12.2020 19:40, Florian Fainelli пишет:
Quoted 20 lines · Dmitry Osipenko
Quoted 17 lines
15.12.2020 19:04, Florian Fainelli пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 12 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Looks like
arch/arm/include/debug/brcmstb.Swould need the same treatment since the implementation was copied fromtegra.S.Good catch, will you be able to test the brcm and make a patch?
Yes, absolutely, building a kernel to test right now.
Quoted 7 lines
Thank you.
BTW, I noticed that the problem is more visible on a thumb2 kernel build, i.e. you should get a more reliable hang on boot. On a non-thumb2 kernel the hanging behaviour seems depends on a device / bootloader. I haven't tried to figure out what exactly makes the difference, perhaps it should be a memory layout / state.
To build with a CONFIG_THUMB2_KERNEL I had to fetch:
https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=9018/2
to avoid a build error, too bad this missed v5.10 final but hopefully it can make it soon.
With CONFIG_THUMB2_KERNEL=y, I am not getting the head.S output where it prints the start/end of the compressed kernel:
C:0x420800C0-0x4321B0E0->0x4212AB00-0x432C5B20
Uncompressing Linux... done, booting the kernel.
[ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Linux version 5.10.0-g148842c98a24
(fainelli@fainelli-desktop) (arm-linux-gcc (GCC) 8.3.0, GNU ld (GNU Binutils) 2.32) #71 SMP Tue Dec 15 09:53:09 PST 2020
I am only getting:
Uncompressing Linux... done, booting the kernel.
Is that the same for you?
Looking at the disassembly of head.o it definitively has brcmstb_uart_config in the .text section as the beginning just like you mentioned in your commit message.
Disassembly of section .text:
00000000 <brcmstb_uart_config>:
0: 00000001 andeq r0, r0, r1
... c: 467c mov r4, pc e: f004 4478 and.w r4, r4, #4160749568 ; 0xf8000000 12: f504 4400 add.w r4, r4, #32768 ; 0x8000 16: 4678 mov r0, pc 18: 42a0 cmp r0, r4 1a: bf3f itttt cc 1c: 48d4 ldrcc r0, [pc, #848] ; (370 <LC1+0x8>) 1e: 4478 addcc r0, pc 20: 4284 cmpcc r4, r0 22: f044 0401 orrcc.w r4, r4, #1 26: bf28 it cs 28: f000 f9aa blcs 380 <cache_on>
however after applying a fix similar to yours, we do end-up with the expected data embedded within the code and given brcmstb.S would be subject to the same issue as tegra.S, it would not hurt.
Signature · 1 line
Florian
15.12.2020 21:22, Florian Fainelli пишет:
Quoted 41 lines · Dmitry Osipenko
Quoted 30 lines
15.12.2020 19:40, Florian Fainelli пишет:
Quoted 20 lines · Dmitry Osipenko
Quoted 17 lines
15.12.2020 19:04, Florian Fainelli пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 12 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Looks like
arch/arm/include/debug/brcmstb.Swould need the same treatment since the implementation was copied fromtegra.S.Good catch, will you be able to test the brcm and make a patch?
Yes, absolutely, building a kernel to test right now.
Quoted 7 lines
Thank you.
BTW, I noticed that the problem is more visible on a thumb2 kernel build, i.e. you should get a more reliable hang on boot. On a non-thumb2 kernel the hanging behaviour seems depends on a device / bootloader. I haven't tried to figure out what exactly makes the difference, perhaps it should be a memory layout / state.
Quoted 6 lines
To build with a
CONFIG_THUMB2_KERNELI had to fetch:https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=9018/2
to avoid a build error, too bad this missed v5.10 final but hopefully it can make it soon.
The VFP fix was applied to the -next very recently, it should propagate to v5.10 eventually.
Quoted 7 lines
With
CONFIG_THUMB2_KERNEL=y, I am not getting thehead.Soutput where it prints the start/end of the compressed kernel:C:0x420800C0-0x4321B0E0->0x4212AB00-0x432C5B20
Uncompressing Linux... done, booting the kernel.[ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Linux version 5.10.0-g148842c98a24
Quoted 8 lines
(fainelli@fainelli-desktop) (arm-linux-gcc (GCC) 8.3.0, GNU ld (GNU Binutils) 2.32) #71 SMP Tue Dec 15 09:53:09 PST 2020
I am only getting:
Uncompressing Linux... done, booting the kernel.
Is that the same for you?
No, start/end are printed for both THUMB2 and ARM kernels here.
Quoted 22 lines
Looking at the disassembly of head.o it definitively has
brcmstb_uart_configin the .text section as the beginning just like you mentioned in your commit message.Disassembly of section .text:
00000000 <
brcmstb_uart_config>:0: 00000001 andeq r0, r0, r1 ... c: 467c mov r4, pc e: f004 4478 and.w r4, r4, #4160749568 ; 0xf8000000 12: f504 4400 add.w r4, r4, #32768 ; 0x8000 16: 4678 mov r0, pc 18: 42a0 cmp r0, r4 1a: bf3f itttt cc 1c: 48d4 ldrcc r0, [pc, #848] ; (370 <LC1+0x8>) 1e: 4478 addcc r0, pc 20: 4284 cmpcc r4, r0 22: f044 0401 orrcc.w r4, r4, #1 26: bf28 it cs 28: f000 f9aa blcs 380 <cache_on>
Quoted 4 lines
however after applying a fix similar to yours, we do end-up with the expected data embedded within the code and given
brcmstb.Swould be subject to the same issue astegra.S, it would not hurt.
Have you checked whether start/end printed after applying the fix?
Quoted 92 lines · Dmitry Osipenko
15.12.2020 21:22, Florian Fainelli пишет:
Quoted 41 lines · Dmitry Osipenko
Quoted 30 lines
15.12.2020 19:40, Florian Fainelli пишет:
Quoted 20 lines · Dmitry Osipenko
Quoted 17 lines
15.12.2020 19:04, Florian Fainelli пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 12 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Looks like
arch/arm/include/debug/brcmstb.Swould need the same treatment since the implementation was copied fromtegra.S.Good catch, will you be able to test the brcm and make a patch?
Yes, absolutely, building a kernel to test right now.
Quoted 7 lines
Thank you.
BTW, I noticed that the problem is more visible on a thumb2 kernel build, i.e. you should get a more reliable hang on boot. On a non-thumb2 kernel the hanging behaviour seems depends on a device / bootloader. I haven't tried to figure out what exactly makes the difference, perhaps it should be a memory layout / state.
Quoted 6 lines
To build with a
CONFIG_THUMB2_KERNELI had to fetch:https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=9018/2
to avoid a build error, too bad this missed v5.10 final but hopefully it can make it soon.
The VFP fix was applied to the -next very recently, it should propagate to v5.10 eventually.
Quoted 7 lines
With
CONFIG_THUMB2_KERNEL=y, I am not getting thehead.Soutput where it prints the start/end of the compressed kernel:C:0x420800C0-0x4321B0E0->0x4212AB00-0x432C5B20
Uncompressing Linux... done, booting the kernel.[ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Linux version 5.10.0-g148842c98a24Quoted 8 lines
(fainelli@fainelli-desktop) (arm-linux-gcc (GCC) 8.3.0, GNU ld (GNU Binutils) 2.32) #71 SMP Tue Dec 15 09:53:09 PST 2020
I am only getting:
Uncompressing Linux... done, booting the kernel.
Is that the same for you?
No, start/end are printed for both THUMB2 and ARM kernels here.
Quoted 22 lines
Looking at the disassembly of head.o it definitively has
brcmstb_uart_configin the .text section as the beginning just like you mentioned in your commit message.Disassembly of section .text:
00000000 <
brcmstb_uart_config>:0: 00000001 andeq r0, r0, r1 ... c: 467c mov r4, pc e: f004 4478 and.w r4, r4, #4160749568 ; 0xf8000000 12: f504 4400 add.w r4, r4, #32768 ; 0x8000 16: 4678 mov r0, pc 18: 42a0 cmp r0, r4 1a: bf3f itttt cc 1c: 48d4 ldrcc r0, [pc, #848] ; (370 <LC1+0x8>) 1e: 4478 addcc r0, pc 20: 4284 cmpcc r4, r0 22: f044 0401 orrcc.w r4, r4, #1 26: bf28 it cs 28: f000 f9aa blcs 380 <cache_on>
Quoted 6 lines
Quoted 4 lines
however after applying a fix similar to yours, we do end-up with the expected data embedded within the code and given
brcmstb.Swould be subject to the same issue astegra.S, it would not hurt.Have you checked whether start/end printed after applying the fix?
Yes I did, it is not printed when CONFIG_THUMB2_KERNEL=y, but it is when CONFIG_THUMB2_KERNEL=n. I don't have a JTAG adapter on this board right now to single step.
Signature · 1 line
Florian
15.12.2020 22:20, Florian Fainelli пишет:
Quoted 100 lines · Dmitry Osipenko
Quoted 97 lines
15.12.2020 21:22, Florian Fainelli пишет:
Quoted 41 lines · Dmitry Osipenko
Quoted 30 lines
15.12.2020 19:40, Florian Fainelli пишет:
Quoted 20 lines · Dmitry Osipenko
Quoted 17 lines
15.12.2020 19:04, Florian Fainelli пишет:
Quoted 16 lines · Dmitry Osipenko
Quoted 12 lines
The
tegra_uart_configof theDEBUG_LLcode is now placed right at the start of the .text section after commit which enabled debug output in the decompressor. Tegra devices are not booting anymore ifDEBUG_LLis enabled sincetegra_uart_configdata is executes as a code. Fix the misplacedtegra_uart_configstorage by embedding it into the code.Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")Signed-off-by Dmitry Osipenko <[email protected]>--- arch/arm/include/debug/tegra.S | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-)Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Looks like
arch/arm/include/debug/brcmstb.Swould need the same treatment since the implementation was copied fromtegra.S.Good catch, will you be able to test the brcm and make a patch?
Yes, absolutely, building a kernel to test right now.
Quoted 7 lines
Thank you.
BTW, I noticed that the problem is more visible on a thumb2 kernel build, i.e. you should get a more reliable hang on boot. On a non-thumb2 kernel the hanging behaviour seems depends on a device / bootloader. I haven't tried to figure out what exactly makes the difference, perhaps it should be a memory layout / state.
Quoted 6 lines
To build with a
CONFIG_THUMB2_KERNELI had to fetch:https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=9018/2
to avoid a build error, too bad this missed v5.10 final but hopefully it can make it soon.
The VFP fix was applied to the -next very recently, it should propagate to v5.10 eventually.
Quoted 7 lines
With
CONFIG_THUMB2_KERNEL=y, I am not getting thehead.Soutput where it prints the start/end of the compressed kernel:C:0x420800C0-0x4321B0E0->0x4212AB00-0x432C5B20
Uncompressing Linux... done, booting the kernel.[ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Linux version 5.10.0-g148842c98a24Quoted 8 lines
(fainelli@fainelli-desktop) (arm-linux-gcc (GCC) 8.3.0, GNU ld (GNU Binutils) 2.32) #71 SMP Tue Dec 15 09:53:09 PST 2020
I am only getting:
Uncompressing Linux... done, booting the kernel.
Is that the same for you?
No, start/end are printed for both THUMB2 and ARM kernels here.
Quoted 26 lines
Looking at the disassembly of head.o it definitively has
brcmstb_uart_configin the .text section as the beginning just like you mentioned in your commit message.Disassembly of section .text:
00000000 <
brcmstb_uart_config>:0: 00000001 andeq r0, r0, r1 ... c: 467c mov r4, pc e: f004 4478 and.w r4, r4, #4160749568 ; 0xf8000000 12: f504 4400 add.w r4, r4, #32768 ; 0x8000 16: 4678 mov r0, pc 18: 42a0 cmp r0, r4 1a: bf3f itttt cc 1c: 48d4 ldrcc r0, [pc, #848] ; (370 <LC1+0x8>) 1e: 4478 addcc r0, pc 20: 4284 cmpcc r4, r0 22: f044 0401 orrcc.w r4, r4, #1 26: bf28 it cs 28: f000 f9aa blcs 380 <cache_on>however after applying a fix similar to yours, we do end-up with the expected data embedded within the code and given
brcmstb.Swould be subject to the same issue astegra.S, it would not hurt.
Quoted 6 lines
Quoted 1 line
Have you checked whether start/end printed after applying the fix?
Yes I did, it is not printed when
CONFIG_THUMB2_KERNEL=y, but it is whenCONFIG_THUMB2_KERNEL=n. I don't have a JTAG adapter on this board right now to single step.
Okay, I don't know what's wrong there.
I also noticed that brcmstb doesn't check whether UART address is 0x0 in senduart / busyuart, perhaps this needs to be fixed too.