[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[Marek Vasut]

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,
-- 
1.8.4.2

[U-Boot] [PATCH 2/3] ARM: pxa: Fix OneNAND SPL builds

[Marek Vasut]

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*)
 	}
-- 
1.8.4.2

[U-Boot] [PATCH 3/3] ARM: pxa: Fix OneNAND window access on VPAC270

[Marek Vasut]

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
-- 
1.8.4.2

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[Rommel G Custodio]

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

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[Scott Wood]

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

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[Marek Vasut]

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

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[Lukasz Majewski]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131228/77c33a29/attachment.pgp>

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[Marek Vasut]

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

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[Lukasz Majewski]

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



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 1/3] mtd: onenand: Fix unaligned access

[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