[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned.
I managed to trigger the problem with regular ELDK 5.4 GCC compiler.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Scott Wood scottwood@freescale.com Cc: Tom Rini trini@ti.com --- drivers/mtd/onenand/onenand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = { .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } };
-static const unsigned char ffchars[] = { +/* + * Warning! This array is used with the memcpy_16() function, thus + * it must be aligned to 2 bytes. GCC can make this array unaligned + * as the array is made of unsigned char, which memcpy16() doesn't + * like and will cause unaligned access. + */ +static const unsigned char __aligned(2) ffchars[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /* 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
The OneNAND SPL used on PXA is slightly obscure. Due to the OneNAND limitation, where we have only the first 1KiB of the OneNAND available upon power-up as a memory-mapped area, from which the CPU starts executing, we place only the most essential code into this first 1KiB . This code copies the rest of the SPL into SRAM and jumps to it. This code is stored in section .text.0 .
The rest of the SPL is stored in section .text.1 . When running the OBJCOPY on the SPL, it will preserve only .text section, but the .text.0 and .text.1 are stripped away from the result, thus making the SPL binary empty. The patch adds additional -j parameters to the OBJCOPY for PXA during the SPL build, which will preserve the .text.0 and .text.1 sections.
Moreover, this patch also adds missing functions into the .text.0 section, since otherwise the PXA270 with 1KiB-window OneNAND won't be able to boot.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Tom Rini trini@ti.com --- arch/arm/cpu/pxa/config.mk | 13 +++++++++++++ board/vpac270/u-boot-spl.lds | 1 + 2 files changed, 14 insertions(+)
diff --git a/arch/arm/cpu/pxa/config.mk b/arch/arm/cpu/pxa/config.mk index d8d263d..f2befbe 100644 --- a/arch/arm/cpu/pxa/config.mk +++ b/arch/arm/cpu/pxa/config.mk @@ -14,3 +14,16 @@ PLATFORM_CPPFLAGS += -mcpu=xscale # ======================================================================== PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT) + +# +# !WARNING! +# The PXA's OneNAND SPL uses .text.0 and .text.1 segments to allow booting from +# really small OneNAND memories where the mmap'd window is only 1KiB big. The +# .text.0 contains only the bare minimum needed to load the real SPL into SRAM. +# Add .text.0 and .text.1 into OBJFLAGS, so when the SPL is being objcopy'd, +# they are not discarded. +# + +#ifdef CONFIG_SPL_BUILD +OBJCFLAGS += -j .text.0 -j .text.1 +#endif diff --git a/board/vpac270/u-boot-spl.lds b/board/vpac270/u-boot-spl.lds index 02d107c..b6fdde4 100644 --- a/board/vpac270/u-boot-spl.lds +++ b/board/vpac270/u-boot-spl.lds @@ -20,6 +20,7 @@ SECTIONS .text.0 : { arch/arm/cpu/pxa/start.o (.text*) + arch/arm/lib/built-in.o (.text*) board/vpac270/built-in.o (.text*) drivers/mtd/onenand/built-in.o (.text*) }
Access the OneNAND 1KiB window on the VPAC270 as an SRAM instead of accessing it as a burst-RAM. This fixes a problem where the board failed to reboot sometimes as the CPU couldn't start executing from the OneNAND 1KiB window.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Tom Rini trini@ti.com --- include/configs/vpac270.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/configs/vpac270.h b/include/configs/vpac270.h index c3ac612..46acac4 100644 --- a/include/configs/vpac270.h +++ b/include/configs/vpac270.h @@ -287,7 +287,7 @@ /* * Memory settings */ -#define CONFIG_SYS_MSC0_VAL 0x3ffc95fa +#define CONFIG_SYS_MSC0_VAL 0x3ffc95f9 #define CONFIG_SYS_MSC1_VAL 0x02ccf974 #define CONFIG_SYS_MSC2_VAL 0x00000000 #ifdef CONFIG_RAM_256M
Dear Marek Vasut,
Marek Vasut <marex <at> denx.de> writes:
+/*
- Warning! This array is used with the memcpy_16() function, thus
- it must be aligned to 2 bytes. GCC can make this array unaligned
- as the array is made of unsigned char, which memcpy16() doesn't
- like and will cause unaligned access.
- */
This is just really a nitpick but memcpy16() -> memcpy_16().
All the best, Rommel
On Thu, 2013-12-26 at 01:01 +0100, Marek Vasut wrote:
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned.
I managed to trigger the problem with regular ELDK 5.4 GCC compiler.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Scott Wood scottwood@freescale.com Cc: Tom Rini trini@ti.com
I don't see the onenand custodian on that CC list.
-Scott
On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote:
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned.
I managed to trigger the problem with regular ELDK 5.4 GCC compiler.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Scott Wood scottwood@freescale.com Cc: Tom Rini trini@ti.com
drivers/mtd/onenand/onenand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = { .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } };
-static const unsigned char ffchars[] = { +/*
- Warning! This array is used with the memcpy_16() function, thus
- it must be aligned to 2 bytes. GCC can make this array unaligned
- as the array is made of unsigned char, which memcpy16() doesn't
- like and will cause unaligned access.
- */
+static const unsigned char __aligned(2) ffchars[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /* 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Lukasz, can you please review this one?
Best regards, Marek Vasut
Hi Marek,
On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote:
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned.
I managed to trigger the problem with regular ELDK 5.4 GCC compiler.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Scott Wood scottwood@freescale.com Cc: Tom Rini trini@ti.com
drivers/mtd/onenand/onenand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = { .oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} } };
-static const unsigned char ffchars[] = { +/*
- Warning! This array is used with the memcpy_16() function, thus
- it must be aligned to 2 bytes. GCC can make this array unaligned
- as the array is made of unsigned char, which memcpy16() doesn't
- like and will cause unaligned access.
- */
+static const unsigned char __aligned(2) ffchars[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /* 16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Lukasz, can you please review this one?
No problem, I will review the patch when I come back to the office (and be able to test it on the proper HW).
Best regards, Marek Vasut
Best regards, Lukasz
On Saturday, December 28, 2013 at 05:06:28 PM, Lukasz Majewski wrote:
Hi Marek,
On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote:
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned.
I managed to trigger the problem with regular ELDK 5.4 GCC compiler.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Scott Wood scottwood@freescale.com Cc: Tom Rini trini@ti.com
drivers/mtd/onenand/onenand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = {
.oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} }
};
-static const unsigned char ffchars[] = { +/*
- Warning! This array is used with the memcpy_16() function, thus
- it must be aligned to 2 bytes. GCC can make this array unaligned
- as the array is made of unsigned char, which memcpy16() doesn't
- like and will cause unaligned access.
- */
+static const unsigned char __aligned(2) ffchars[] = {
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /*
16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Lukasz, can you please review this one?
No problem, I will review the patch when I come back to the office (and be able to test it on the proper HW).
Thanks, I'd love to get this fix into 2014.01 . Maybe the real fix here would be to fix the memcpy_16() instead tho.
Best regards, Marek Vasut
Hi Marek,
On Saturday, December 28, 2013 at 05:06:28 PM, Lukasz Majewski wrote:
Hi Marek,
On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote:
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned.
I managed to trigger the problem with regular ELDK 5.4 GCC compiler.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Scott Wood scottwood@freescale.com Cc: Tom Rini trini@ti.com
drivers/mtd/onenand/onenand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = {
.oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} }
};
-static const unsigned char ffchars[] = { +/*
- Warning! This array is used with the memcpy_16() function,
thus
- it must be aligned to 2 bytes. GCC can make this array
unaligned
- as the array is made of unsigned char, which memcpy16()
doesn't
- like and will cause unaligned access.
- */
+static const unsigned char __aligned(2) ffchars[] = {
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, /*
16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Lukasz, can you please review this one?
No problem, I will review the patch when I come back to the office (and be able to test it on the proper HW).
Thanks, I'd love to get this fix into 2014.01 . Maybe the real fix here would be to fix the memcpy_16() instead tho.
I've tested it with eldk-5.4 toolchain on GONI device. It seems that no regression is caused by this patch.
As a side note - this problem didn't show up for GONI board. Apparently I was lucky.
I will scrutinize this code in respect of code alignment.
Applied to u-boot-onenand.
I will send pull request to Tom ASAP.
Best regards, Marek Vasut
On Tuesday, December 31, 2013 at 11:43:48 AM, Lukasz Majewski wrote:
Hi Marek,
On Saturday, December 28, 2013 at 05:06:28 PM, Lukasz Majewski wrote:
Hi Marek,
On Thursday, December 26, 2013 at 01:01:24 AM, Marek Vasut wrote:
Fix unaligned access in OneNAND core. The problem is that the ffchars[] array is an array of "unsigned char", but in onenand_write_ops_nolock() can be passed to the memcpy_16() function. The memcpy_16() function will treat the buffer as an array of "unsigned short", thus triggering unaligned access if the compiler decided ffchars[] to be not aligned.
I managed to trigger the problem with regular ELDK 5.4 GCC compiler.
Signed-off-by: Marek Vasut marex@denx.de Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Scott Wood scottwood@freescale.com Cc: Tom Rini trini@ti.com
drivers/mtd/onenand/onenand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 979e4af..e33e8d3 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -91,7 +91,13 @@ static struct nand_ecclayout onenand_oob_32 = {
.oobfree = { {2, 3}, {14, 2}, {18, 3}, {30, 2} }
};
-static const unsigned char ffchars[] = { +/*
- Warning! This array is used with the memcpy_16() function,
thus
- it must be aligned to 2 bytes. GCC can make this array
unaligned
- as the array is made of unsigned char, which memcpy16()
doesn't
- like and will cause unaligned access.
- */
+static const unsigned char __aligned(2) ffchars[] = {
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, /*
16 */ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
Lukasz, can you please review this one?
No problem, I will review the patch when I come back to the office (and be able to test it on the proper HW).
Thanks, I'd love to get this fix into 2014.01 . Maybe the real fix here would be to fix the memcpy_16() instead tho.
I've tested it with eldk-5.4 toolchain on GONI device. It seems that no regression is caused by this patch.
As a side note - this problem didn't show up for GONI board. Apparently I was lucky.
I will scrutinize this code in respect of code alignment.
Applied to u-boot-onenand.
Thank you very much!
Have a happy new year, guys !
Best regards, Marek Vasut
participants (5)
-
Lukasz Majewski -
Lukasz Majewski -
Marek Vasut -
Rommel G Custodio -
Scott Wood