[PATCH] arm: mach-snapdragon: pinctrl: Place pin_name in .data section
According to arch/arm/lib/crt0_64.S, the BSS section is "UNAVAILABLE" and uninitialized before relocation. Also, it overlaps with the appended DTB before relocation, so writing data into a variable in the BSS section might corrupt the appended DTB.
Unfortunately, pinctrl-apq8016.c and pinctrl-apq8096.c do place the "pin_name" variable in the BSS section (since it's uninitialized). It's also used before relocation, when setting up the pinctrl for the serial driver.
On DB410c this causes "GPIO_5" to be written into some part of an appended DTB, e.g.:
80111820: edfe0dd0 9f100000 38000000 c00e0000 ...........8.... 80111830: 28000000 11000000 10000000 00000000 ...(............ 80111840: 4f495047 8800355f 00000000 00000000 GPIO_5.......... 80111850: 00000000 00000000 01000000 00000000 ................ 80111860: 03000000 04000000 00000000 02000000 ................ 80111870: 03000000 04000000 0f000000 02000000 ................ 80111880: 03000000 2d000000 1b000000 6c617551 .......-....Qual 80111890: 6d6d6f63 63655420 6c6f6e68 6569676f comm Technologie
Depending on the part of the DTB that is corrupted this might not cause any problems, but it can also result in strange reboots without any serial output.
Fortunately, in practice this does not cause issues on DB410c yet because board_fdt_blob_setup() in dragonboard410c.c currently overrides the appended DTB with the one passed by the previous bootloader (LK) (which does not get corrupted).
DB820c does not have board_fdt_blob_setup() so I would expect it to be affected by this problem. Perhaps everyone was just fortunate to not compile an U-Boot configuration where the pin_name corrupts an important part of the DTB.
Make sure "pin_name" is explicitly placed in the .data section instead of .bss to fix this.
Cc: Ramon Fried rfried.dev@gmail.com Signed-off-by: Stephan Gerhold stephan@gerhold.net ---
arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +-- arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c index 1042b564c3..70c0be0bca 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD", @@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = { .get_function_mux = apq8016_get_function_mux, .get_pin_name = apq8016_get_pin_name, }; - diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c index 20a71c319b..45462f01c2 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD",
On Mon, Jul 5, 2021 at 3:19 PM Stephan Gerhold stephan@gerhold.net wrote:
According to arch/arm/lib/crt0_64.S, the BSS section is "UNAVAILABLE" and uninitialized before relocation. Also, it overlaps with the appended DTB before relocation, so writing data into a variable in the BSS section might corrupt the appended DTB.
Unfortunately, pinctrl-apq8016.c and pinctrl-apq8096.c do place the "pin_name" variable in the BSS section (since it's uninitialized). It's also used before relocation, when setting up the pinctrl for the serial driver.
On DB410c this causes "GPIO_5" to be written into some part of an appended DTB, e.g.:
80111820: edfe0dd0 9f100000 38000000 c00e0000 ...........8.... 80111830: 28000000 11000000 10000000 00000000 ...(............ 80111840: 4f495047 8800355f 00000000 00000000 GPIO_5.......... 80111850: 00000000 00000000 01000000 00000000 ................ 80111860: 03000000 04000000 00000000 02000000 ................ 80111870: 03000000 04000000 0f000000 02000000 ................ 80111880: 03000000 2d000000 1b000000 6c617551 .......-....Qual 80111890: 6d6d6f63 63655420 6c6f6e68 6569676f comm Technologie
Depending on the part of the DTB that is corrupted this might not cause any problems, but it can also result in strange reboots without any serial output.
Fortunately, in practice this does not cause issues on DB410c yet because board_fdt_blob_setup() in dragonboard410c.c currently overrides the appended DTB with the one passed by the previous bootloader (LK) (which does not get corrupted).
DB820c does not have board_fdt_blob_setup() so I would expect it to be affected by this problem. Perhaps everyone was just fortunate to not compile an U-Boot configuration where the pin_name corrupts an important part of the DTB.
Make sure "pin_name" is explicitly placed in the .data section instead of .bss to fix this.
Cc: Ramon Fried rfried.dev@gmail.com Signed-off-by: Stephan Gerhold stephan@gerhold.net
arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +-- arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c index 1042b564c3..70c0be0bca 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD", @@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = { .get_function_mux = apq8016_get_function_mux, .get_pin_name = apq8016_get_pin_name, };
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c index 20a71c319b..45462f01c2 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD", -- 2.32.0
Reviewed-by: Ramon Fried rfried.dev@gmail.com
On 06/07/21, Ramon Fried wrote:
On Mon, Jul 5, 2021 at 3:19 PM Stephan Gerhold stephan@gerhold.net wrote:
According to arch/arm/lib/crt0_64.S, the BSS section is "UNAVAILABLE" and uninitialized before relocation. Also, it overlaps with the appended DTB before relocation, so writing data into a variable in the BSS section might corrupt the appended DTB.
Unfortunately, pinctrl-apq8016.c and pinctrl-apq8096.c do place the "pin_name" variable in the BSS section (since it's uninitialized). It's also used before relocation, when setting up the pinctrl for the serial driver.
On DB410c this causes "GPIO_5" to be written into some part of an appended DTB, e.g.:
80111820: edfe0dd0 9f100000 38000000 c00e0000 ...........8.... 80111830: 28000000 11000000 10000000 00000000 ...(............ 80111840: 4f495047 8800355f 00000000 00000000 GPIO_5.......... 80111850: 00000000 00000000 01000000 00000000 ................ 80111860: 03000000 04000000 00000000 02000000 ................ 80111870: 03000000 04000000 0f000000 02000000 ................ 80111880: 03000000 2d000000 1b000000 6c617551 .......-....Qual 80111890: 6d6d6f63 63655420 6c6f6e68 6569676f comm Technologie
Depending on the part of the DTB that is corrupted this might not cause any problems, but it can also result in strange reboots without any serial output.
Fortunately, in practice this does not cause issues on DB410c yet because board_fdt_blob_setup() in dragonboard410c.c currently overrides the appended DTB with the one passed by the previous bootloader (LK) (which does not get corrupted).
DB820c does not have board_fdt_blob_setup() so I would expect it to be affected by this problem. Perhaps everyone was just fortunate to not compile an U-Boot configuration where the pin_name corrupts an important part of the DTB.
I'll try to confirm during the week.
After Ramon's pinctrl changes db820c did indeed break - was able to test a few months back- but we didnt have a board to test back then (and nobody complained)
will come back to you on this but from what you are saying, this is probably the fix we needed for db820c
Make sure "pin_name" is explicitly placed in the .data section instead of .bss to fix this.
Cc: Ramon Fried rfried.dev@gmail.com Signed-off-by: Stephan Gerhold stephan@gerhold.net
arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +-- arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c index 1042b564c3..70c0be0bca 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD", @@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = { .get_function_mux = apq8016_get_function_mux, .get_pin_name = apq8016_get_pin_name, };
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c index 20a71c319b..45462f01c2 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD", -- 2.32.0
Reviewed-by: Ramon Fried rfried.dev@gmail.com
On 15/07/21, Jorge Ramirez-Ortiz, Gmail wrote:
On 06/07/21, Ramon Fried wrote:
On Mon, Jul 5, 2021 at 3:19 PM Stephan Gerhold stephan@gerhold.net wrote:
According to arch/arm/lib/crt0_64.S, the BSS section is "UNAVAILABLE" and uninitialized before relocation. Also, it overlaps with the appended DTB before relocation, so writing data into a variable in the BSS section might corrupt the appended DTB.
Unfortunately, pinctrl-apq8016.c and pinctrl-apq8096.c do place the "pin_name" variable in the BSS section (since it's uninitialized). It's also used before relocation, when setting up the pinctrl for the serial driver.
On DB410c this causes "GPIO_5" to be written into some part of an appended DTB, e.g.:
80111820: edfe0dd0 9f100000 38000000 c00e0000 ...........8.... 80111830: 28000000 11000000 10000000 00000000 ...(............ 80111840: 4f495047 8800355f 00000000 00000000 GPIO_5.......... 80111850: 00000000 00000000 01000000 00000000 ................ 80111860: 03000000 04000000 00000000 02000000 ................ 80111870: 03000000 04000000 0f000000 02000000 ................ 80111880: 03000000 2d000000 1b000000 6c617551 .......-....Qual 80111890: 6d6d6f63 63655420 6c6f6e68 6569676f comm Technologie
Depending on the part of the DTB that is corrupted this might not cause any problems, but it can also result in strange reboots without any serial output.
Fortunately, in practice this does not cause issues on DB410c yet because board_fdt_blob_setup() in dragonboard410c.c currently overrides the appended DTB with the one passed by the previous bootloader (LK) (which does not get corrupted).
DB820c does not have board_fdt_blob_setup() so I would expect it to be affected by this problem. Perhaps everyone was just fortunate to not compile an U-Boot configuration where the pin_name corrupts an important part of the DTB.
I'll try to confirm during the week.
After Ramon's pinctrl changes db820c did indeed break - was able to test a few months back- but we didnt have a board to test back then (and nobody complained)
will come back to you on this but from what you are saying, this is probably the fix we needed for db820c
yep, all good. just tested it. thanks for the fix!
U-Boot 2021.07-00467-gc11f5abce8 (Jul 15 2021 - 09:20:22 +0200) Qualcomm-DragonBoard 820C
DRAM: 3 GiB PSCI: v1.0 MMC: sdhci@74a4900: 0 Loading Environment from EXT4... MMC: no card present In: serial@75b0000 Out: serial@75b0000 Err: serial@75b0000 Net: Net Initialization Skipped No ethernet found. Hit any key to stop autoboot: 0 MMC: no card present dragonboard820c => dragonboard820c => bdinfo boot_params = 0x0000000000000000 DRAM bank = 0x0000000000000000 -> start = 0x0000000080000000 -> size = 0x0000000060000000 DRAM bank = 0x0000000000000001 -> start = 0x0000000100000000 -> size = 0x000000005ea4ffff flashstart = 0x0000000000000000 flashsize = 0x0000000000000000 flashoffset = 0x0000000000000000 baudrate = 115200 bps relocaddr = 0x000000013ff70000 reloc off = 0x00000000bfef0000 Build = 64-bit current eth = unknown ethaddr = (not set) IP addr = <NULL> fdt_blob = 0x000000013f7673c0 new_fdt = 0x000000013f7673c0 fdt_size = 0x0000000000000a20 lmb_dump_all: memory.cnt = 0x2 memory[0] [0x80000000-0xdfffffff], 0x60000000 bytes flags: 0 memory[1] [0x100000000-0x15ea4fffe], 0x5ea4ffff bytes flags: 0 reserved.cnt = 0x1 reserved[0] [0x86300000-0x864fffff], 0x00200000 bytes flags: 4 arch_number = 0x0000000000000000 TLB addr = 0x000000013fff0000 irq_sp = 0x000000013f7673b0 sp start = 0x000000013f7673b0 Early malloc usage: 5d8 / 2000
Make sure "pin_name" is explicitly placed in the .data section instead of .bss to fix this.
Cc: Ramon Fried rfried.dev@gmail.com Signed-off-by: Stephan Gerhold stephan@gerhold.net
arch/arm/mach-snapdragon/pinctrl-apq8016.c | 3 +-- arch/arm/mach-snapdragon/pinctrl-apq8096.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8016.c b/arch/arm/mach-snapdragon/pinctrl-apq8016.c index 1042b564c3..70c0be0bca 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8016.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8016.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD", @@ -59,4 +59,3 @@ struct msm_pinctrl_data apq8016_data = { .get_function_mux = apq8016_get_function_mux, .get_pin_name = apq8016_get_pin_name, };
diff --git a/arch/arm/mach-snapdragon/pinctrl-apq8096.c b/arch/arm/mach-snapdragon/pinctrl-apq8096.c index 20a71c319b..45462f01c2 100644 --- a/arch/arm/mach-snapdragon/pinctrl-apq8096.c +++ b/arch/arm/mach-snapdragon/pinctrl-apq8096.c @@ -10,7 +10,7 @@ #include <common.h>
#define MAX_PIN_NAME_LEN 32 -static char pin_name[MAX_PIN_NAME_LEN]; +static char pin_name[MAX_PIN_NAME_LEN] __section(".data"); static const char * const msm_pinctrl_pins[] = { "SDC1_CLK", "SDC1_CMD", -- 2.32.0
Reviewed-by: Ramon Fried rfried.dev@gmail.com
On Mon, Jul 05, 2021 at 02:18:47PM +0200, Stephan Gerhold wrote:
According to arch/arm/lib/crt0_64.S, the BSS section is "UNAVAILABLE" and uninitialized before relocation. Also, it overlaps with the appended DTB before relocation, so writing data into a variable in the BSS section might corrupt the appended DTB.
Unfortunately, pinctrl-apq8016.c and pinctrl-apq8096.c do place the "pin_name" variable in the BSS section (since it's uninitialized). It's also used before relocation, when setting up the pinctrl for the serial driver.
On DB410c this causes "GPIO_5" to be written into some part of an appended DTB, e.g.:
80111820: edfe0dd0 9f100000 38000000 c00e0000 ...........8.... 80111830: 28000000 11000000 10000000 00000000 ...(............ 80111840: 4f495047 8800355f 00000000 00000000 GPIO_5.......... 80111850: 00000000 00000000 01000000 00000000 ................ 80111860: 03000000 04000000 00000000 02000000 ................ 80111870: 03000000 04000000 0f000000 02000000 ................ 80111880: 03000000 2d000000 1b000000 6c617551 .......-....Qual 80111890: 6d6d6f63 63655420 6c6f6e68 6569676f comm Technologie
Depending on the part of the DTB that is corrupted this might not cause any problems, but it can also result in strange reboots without any serial output.
Fortunately, in practice this does not cause issues on DB410c yet because board_fdt_blob_setup() in dragonboard410c.c currently overrides the appended DTB with the one passed by the previous bootloader (LK) (which does not get corrupted).
DB820c does not have board_fdt_blob_setup() so I would expect it to be affected by this problem. Perhaps everyone was just fortunate to not compile an U-Boot configuration where the pin_name corrupts an important part of the DTB.
Make sure "pin_name" is explicitly placed in the .data section instead of .bss to fix this.
Cc: Ramon Fried rfried.dev@gmail.com Signed-off-by: Stephan Gerhold stephan@gerhold.net Reviewed-by: Ramon Fried rfried.dev@gmail.com
Applied to u-boot/master, thanks!
participants (4)
-
Jorge Ramirez-Ortiz, Gmail -
Ramon Fried -
Stephan Gerhold -
Tom Rini