Skip to contentSkip to conversation

ARM: tegra: Fix misplaced tegra_uart_config in decompressor

PATCH In review Dmitry Osipenko 3 participants 11 messages Dec 15, 2020

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 arch/arm/include/debug/tegra.S | 54 +++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 27 deletions(-)
arch/arm/include/debug/tegra.S +27 -27
index 98daa7f48314..7267516db0ba 100644
@@ -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
Reviewed-by on v1 1/1
#1 lore raw
Quoted 9 lines · Dmitry Osipenko

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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")

Aha I see the problem.

Please put this into Russell's patch tracker as a fix!

Yours,
Linus Walleij

#2 lore raw

15.12.2020 17:28, Linus Walleij пишет:

Quoted 8 lines · Dmitry Osipenko
Quoted 7 lines

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.

Quoted 8 lines
Quoted 3 lines
Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")

Aha I see the problem.

Please put this into Russell's patch tracker as a fix!

Thanks, I sent the patch to the tracker.

#3 lore raw
In reply to Dmitry Osipenko
Quoted 6 lines · Dmitry Osipenko

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.

Quoted 6 lines
Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 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
#4 lore raw

15.12.2020 19:04, Florian Fainelli пишет:

Quoted 16 lines · Dmitry Osipenko
Quoted 6 lines

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.

Quoted 6 lines
Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 arch/arm/include/debug/tegra.S | 54 +++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 27 deletions(-)
Quoted 3 lines

Looks like arch/arm/include/debug/brcmstb.S would need the same treatment since the implementation was copied from tegra.S.

Good catch, will you be able to test the brcm and make a patch?

#5 lore raw
Quoted 17 lines · Dmitry Osipenko

15.12.2020 19:04, Florian Fainelli пишет:

Quoted 16 lines · Dmitry Osipenko
Quoted 12 lines

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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 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.S would need the same treatment since the implementation was copied from tegra.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
#6 lore raw

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_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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 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.S would need the same treatment since the implementation was copied from tegra.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.

#7 lore raw
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_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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 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.S would need the same treatment since the implementation was copied from tegra.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
#8 lore raw

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_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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 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.S would need the same treatment since the implementation was copied from tegra.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_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.

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 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
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_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>
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.S would be subject to the same issue as tegra.S, it would not hurt.

Have you checked whether start/end printed after applying the fix?

#9 lore raw
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_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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 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.S would need the same treatment since the implementation was copied from tegra.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_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.

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 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
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_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>
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.S would be subject to the same issue as tegra.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
#10 lore raw

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_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.

Fixes 2596a72d3384 ("ARM: 9009/1: uncompress: Enable debug in head.S")
---
 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.S would need the same treatment since the implementation was copied from tegra.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_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.

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 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
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 26 lines

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.

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 when CONFIG_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.