[U-Boot] [PATCH] MPC5200: workaround data corruption for unaligned local bus accesses

[U-Boot] [PATCH] MPC5200: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

The MPC5200 has a nasty problem that will cause silent data corruption
when performing unaligned 16 or 32 byte accesses when reading from the
local bus - typically this affects reading from flash. The problem can
be easily shown:

=> md fc0c0000 10
fc0c0000: 323e4337 01626f6f 74636d64 3d72756e    2>C7.bootcmd=run
fc0c0010: 206e6574 5f6e6673 00626f6f 7464656c     net_nfs.bootdel
fc0c0020: 61793d35 00626175 64726174 653d3131    ay=5.baudrate=11
fc0c0030: 35323030 00707265 626f6f74 3d656368    5200.preboot=ech
=> md fc0c0001 10
fc0c0001: 65636801 00000074 0000003d 00000020    ech....t...=...
fc0c0011: 0000005f 00000000 00000074 00000061    ..._.......t...a
fc0c0021: 00000000 00000064 00000065 00000035    .......d...e...5
fc0c0031: 00000000 00000062 0000003d 0000006f    .......b...=...o
=> md.w fc0c0001 10
fc0c0001: 0000 3701 0000 6f74 0000 643d 0000 6e20    ..7...ot..d=..n
fc0c0011: 0000 745f 0000 7300 0000 6f74 0000 6c61    ..t_..s...ot..la

This commit implements a workaround at least for the most blatant
problem: using memcpy() from NOR flash. We rename the assembler
routine into __memcpy() and provide a wrapper, which will use a
byte-wise copy loop for source addresses in NOR flash, and branch to
the optimized __memcpy() otherwise.

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
 arch/powerpc/cpu/mpc5xxx/Makefile         |    5 ++
 arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c |   62 +++++++++++++++++++++++++++++
 arch/powerpc/lib/Makefile                 |    5 ++
 3 files changed, 72 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c

diff --git a/arch/powerpc/cpu/mpc5xxx/Makefile b/arch/powerpc/cpu/mpc5xxx/Makefile
index 0ee0611..4ab2b7b 100644
--- a/arch/powerpc/cpu/mpc5xxx/Makefile
+++ b/arch/powerpc/cpu/mpc5xxx/Makefile
@@ -30,6 +30,11 @@ SOBJS	= io.o firmware_sc_task_bestcomm.impl.o
 COBJS	= i2c.o traps.o cpu.o cpu_init.o ide.o interrupts.o \
 	  loadtask.o pci_mpc5200.o serial.o speed.o usb_ohci.o usb.o
 
+# Workaround for local bus unaligned access problem on MPC5200
+#ifdef CONFIG_MPC5200
+COBJS	+= memcpy_mpc5200.o
+#endif
+
 SRCS	:= $(START:.o=.S) $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
 START	:= $(addprefix $(obj),$(START))
diff --git a/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
new file mode 100644
index 0000000..5c2e64f
--- /dev/null
+++ b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
@@ -0,0 +1,62 @@
+/*
+ * (C) Copyright 2010
+ * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/*
+ * This is a workaround for issues on the MPC5200, where unaligned
+ * 32-bit-accesses to the local bus will deliver corrupted data. This
+ * happens for example when trying to use memcpy() from an odd NOR
+ * flash address; the behaviour can be also seen when using "md" on an
+ * odd NOR flash address (but there it is not a bug in U-Boot, which
+ * only shows the behaviour of this processor).
+ *
+ * For memcpy(), we test if the source address is in NOR flash, and
+ * perform byte-wise (slow) copy then; otherwise we use the optimized
+ * (fast) real __memcpy().
+ */
+
+#include <common.h>
+#include <flash.h>
+#include <linux/types.h>
+
+void *memcpy(void *trg, const void *src, size_t len)
+{
+	extern void* __memcpy(void *, const void *, size_t);
+	char *s = (char *)src;
+	char *t = (char *)trg;
+	void *dest = src;
+
+	/*
+	 * Check is source address is in flash:
+	 * If not, we use the fast assembler code
+	 */
+	if (addr2info((ulong)src) == NULL)
+		return __memcpy(trg, src, len);
+
+	/*
+	 * Copying from flash, perform byte by byte copy.
+	 */
+	while (len-- > 0)
+		*t++ = *s++;
+
+	return dest;
+}
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index 5f85502..4ba51b3 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -43,6 +43,11 @@ COBJS-y	+= time.o
 SRCS	:= $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
 
+# Workaround for local bus unaligned access problem on MPC5200
+#ifdef CONFIG_MPC5200
+$(obj)ppcstring.o: AFLAGS += -Dmemcpy=__memcpy
+#endif
+
 $(LIB):	$(obj).depend $(OBJS)
 	@if ! $(CROSS_COMPILE)readelf -S $(OBJS) | grep -q '\.fixup.*PROGBITS';\
 	then \
-- 
1.7.0.1

[U-Boot] [PATCH v2] MPC5200: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

The MPC5200 has a nasty problem that will cause silent data corruption
when performing unaligned 16 or 32 byte accesses when reading from the
local bus - typically this affects reading from flash. The problem can
be easily shown:

=> md fc0c0000 10
fc0c0000: 323e4337 01626f6f 74636d64 3d72756e    2>C7.bootcmd=run
fc0c0010: 206e6574 5f6e6673 00626f6f 7464656c     net_nfs.bootdel
fc0c0020: 61793d35 00626175 64726174 653d3131    ay=5.baudrate=11
fc0c0030: 35323030 00707265 626f6f74 3d656368    5200.preboot=ech
=> md fc0c0001 10
fc0c0001: 65636801 00000074 0000003d 00000020    ech....t...=...
fc0c0011: 0000005f 00000000 00000074 00000061    ..._.......t...a
fc0c0021: 00000000 00000064 00000065 00000035    .......d...e...5
fc0c0031: 00000000 00000062 0000003d 0000006f    .......b...=...o
=> md.w fc0c0001 10
fc0c0001: 0000 3701 0000 6f74 0000 643d 0000 6e20    ..7...ot..d=..n
fc0c0011: 0000 745f 0000 7300 0000 6f74 0000 6c61    ..t_..s...ot..la

This commit implements a workaround at least for the most blatant
problem: using memcpy() from NOR flash. We rename the assembler
routine into __memcpy() and provide a wrapper, which will use a
byte-wise copy loop for unaligned source or target addresses when
reading from NOR flash, and branch to the optimized __memcpy()
in all other cases, thus minimizing the performance impact.

Tested on lite5200b and TQM5200S.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 arch/powerpc/cpu/mpc5xxx/Makefile         |    5 ++
 arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c |   71 +++++++++++++++++++++++++++++
 arch/powerpc/lib/Makefile                 |    5 ++
 3 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c

diff --git a/arch/powerpc/cpu/mpc5xxx/Makefile b/arch/powerpc/cpu/mpc5xxx/Makefile
index 0ee0611..4ab2b7b 100644
--- a/arch/powerpc/cpu/mpc5xxx/Makefile
+++ b/arch/powerpc/cpu/mpc5xxx/Makefile
@@ -30,6 +30,11 @@ SOBJS	= io.o firmware_sc_task_bestcomm.impl.o
 COBJS	= i2c.o traps.o cpu.o cpu_init.o ide.o interrupts.o \
 	  loadtask.o pci_mpc5200.o serial.o speed.o usb_ohci.o usb.o
 
+# Workaround for local bus unaligned access problem on MPC5200
+#ifdef CONFIG_MPC5200
+COBJS	+= memcpy_mpc5200.o
+#endif
+
 SRCS	:= $(START:.o=.S) $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
 START	:= $(addprefix $(obj),$(START))
diff --git a/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
new file mode 100644
index 0000000..0950354
--- /dev/null
+++ b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
@@ -0,0 +1,71 @@
+/*
+ * (C) Copyright 2010
+ * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/*
+ * This is a workaround for issues on the MPC5200, where unaligned
+ * 32-bit-accesses to the local bus will deliver corrupted data. This
+ * happens for example when trying to use memcpy() from an odd NOR
+ * flash address; the behaviour can be also seen when using "md" on an
+ * odd NOR flash address (but there it is not a bug in U-Boot, which
+ * only shows the behaviour of this processor).
+ *
+ * For memcpy(), we test if either the source or the target address
+ * are not 32 bit aligned, and - if so - if the source address is in
+ * NOR flash: in this case we perform a byte-wise (slow) then; for
+ * aligned operations of non-flash areas we use the optimized (fast)
+ * real __memcpy().  This way we minimize the performance impact of
+ * this workaround.
+ *
+ */
+
+#include <common.h>
+#include <flash.h>
+#include <linux/types.h>
+
+void *memcpy(void *trg, const void *src, size_t len)
+{
+	extern void* __memcpy(void *, const void *, size_t);
+	char *s = (char *)src;
+	char *t = (char *)trg;
+	void *dest = (void *)src;
+
+	/*
+	 * Check is source address is in flash:
+	 * If not, we use the fast assembler code
+	 */
+	if (((((unsigned long)s & 3) == 0)	/* source aligned  */
+		&&				/*	AND	   */
+	     (((unsigned long)t & 3) == 0))	/* target aligned, */
+		||				/*	or	   */
+	    (addr2info((ulong)s) == NULL)) {	/* source not in flash */
+		return __memcpy(trg, src, len);
+	}
+
+	/*
+	 * Copying from flash, perform byte by byte copy.
+	 */
+	while (len-- > 0)
+		*t++ = *s++;
+
+	return dest;
+}
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index 5f85502..4ba51b3 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -43,6 +43,11 @@ COBJS-y	+= time.o
 SRCS	:= $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
 
+# Workaround for local bus unaligned access problem on MPC5200
+#ifdef CONFIG_MPC5200
+$(obj)ppcstring.o: AFLAGS += -Dmemcpy=__memcpy
+#endif
+
 $(LIB):	$(obj).depend $(OBJS)
 	@if ! $(CROSS_COMPILE)readelf -S $(OBJS) | grep -q '\.fixup.*PROGBITS';\
 	then \
-- 
1.7.0.1

[U-Boot] [PATCH v3] MPC5200: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

The MPC5200 has a nasty problem that will cause silent data corruption
when performing unaligned 16 or 32 byte accesses when reading from the
local bus - typically this affects reading from flash. The problem can
be easily shown:

=> md fc0c0000 10
fc0c0000: 323e4337 01626f6f 74636d64 3d72756e    2>C7.bootcmd=run
fc0c0010: 206e6574 5f6e6673 00626f6f 7464656c     net_nfs.bootdel
fc0c0020: 61793d35 00626175 64726174 653d3131    ay=5.baudrate=11
fc0c0030: 35323030 00707265 626f6f74 3d656368    5200.preboot=ech
=> md fc0c0001 10
fc0c0001: 65636801 00000074 0000003d 00000020    ech....t...=...
fc0c0011: 0000005f 00000000 00000074 00000061    ..._.......t...a
fc0c0021: 00000000 00000064 00000065 00000035    .......d...e...5
fc0c0031: 00000000 00000062 0000003d 0000006f    .......b...=...o
=> md.w fc0c0001 10
fc0c0001: 0000 3701 0000 6f74 0000 643d 0000 6e20    ..7...ot..d=..n
fc0c0011: 0000 745f 0000 7300 0000 6f74 0000 6c61    ..t_..s...ot..la

This commit implements a workaround at least for the most blatant
problem: using memcpy() from NOR flash. We rename the assembler
routine into __memcpy() and provide a wrapper, which will use a
byte-wise copy loop for unaligned source or target addresses when
reading from NOR flash, and branch to the optimized __memcpy()
in all other cases, thus minimizing the performance impact.

Tested on lite5200b and TQM5200S.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
v2:
	Check alignment of source and target addresses, use byte-wise
	copy only when needed (i. e. unaligned, and reading from flash)
v3:
	Fix Makefile typo which will break all non-MPC5200 boards

 arch/powerpc/cpu/mpc5xxx/Makefile         |    5 ++
 arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c |   71 +++++++++++++++++++++++++++++
 arch/powerpc/lib/Makefile                 |    5 ++
 3 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c

diff --git a/arch/powerpc/cpu/mpc5xxx/Makefile b/arch/powerpc/cpu/mpc5xxx/Makefile
index 0ee0611..4ab2b7b 100644
--- a/arch/powerpc/cpu/mpc5xxx/Makefile
+++ b/arch/powerpc/cpu/mpc5xxx/Makefile
@@ -30,6 +30,11 @@ SOBJS	= io.o firmware_sc_task_bestcomm.impl.o
 COBJS	= i2c.o traps.o cpu.o cpu_init.o ide.o interrupts.o \
 	  loadtask.o pci_mpc5200.o serial.o speed.o usb_ohci.o usb.o
 
+# Workaround for local bus unaligned access problem on MPC5200
+ifdef CONFIG_MPC5200
+COBJS	+= memcpy_mpc5200.o
+endif
+
 SRCS	:= $(START:.o=.S) $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
 START	:= $(addprefix $(obj),$(START))
diff --git a/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
new file mode 100644
index 0000000..0950354
--- /dev/null
+++ b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
@@ -0,0 +1,71 @@
+/*
+ * (C) Copyright 2010
+ * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/*
+ * This is a workaround for issues on the MPC5200, where unaligned
+ * 32-bit-accesses to the local bus will deliver corrupted data. This
+ * happens for example when trying to use memcpy() from an odd NOR
+ * flash address; the behaviour can be also seen when using "md" on an
+ * odd NOR flash address (but there it is not a bug in U-Boot, which
+ * only shows the behaviour of this processor).
+ *
+ * For memcpy(), we test if either the source or the target address
+ * are not 32 bit aligned, and - if so - if the source address is in
+ * NOR flash: in this case we perform a byte-wise (slow) then; for
+ * aligned operations of non-flash areas we use the optimized (fast)
+ * real __memcpy().  This way we minimize the performance impact of
+ * this workaround.
+ *
+ */
+
+#include <common.h>
+#include <flash.h>
+#include <linux/types.h>
+
+void *memcpy(void *trg, const void *src, size_t len)
+{
+	extern void* __memcpy(void *, const void *, size_t);
+	char *s = (char *)src;
+	char *t = (char *)trg;
+	void *dest = (void *)src;
+
+	/*
+	 * Check is source address is in flash:
+	 * If not, we use the fast assembler code
+	 */
+	if (((((unsigned long)s & 3) == 0)	/* source aligned  */
+		&&				/*	AND	   */
+	     (((unsigned long)t & 3) == 0))	/* target aligned, */
+		||				/*	or	   */
+	    (addr2info((ulong)s) == NULL)) {	/* source not in flash */
+		return __memcpy(trg, src, len);
+	}
+
+	/*
+	 * Copying from flash, perform byte by byte copy.
+	 */
+	while (len-- > 0)
+		*t++ = *s++;
+
+	return dest;
+}
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index 5f85502..4ba51b3 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -43,6 +43,11 @@ COBJS-y	+= time.o
 SRCS	:= $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
 
+# Workaround for local bus unaligned access problem on MPC5200
+#ifdef CONFIG_MPC5200
+$(obj)ppcstring.o: AFLAGS += -Dmemcpy=__memcpy
+#endif
+
 $(LIB):	$(obj).depend $(OBJS)
 	@if ! $(CROSS_COMPILE)readelf -S $(OBJS) | grep -q '\.fixup.*PROGBITS';\
 	then \
-- 
1.7.0.1

[U-Boot] [PATCH v3] MPC5200: workaround data corruption for unaligned local bus accesses

[Detlev Zundel]

Hi Wolfgang,

Now that we keep the old speed on aligned access, I'm ok with the change
and only see one typo in the comment.

> The MPC5200 has a nasty problem that will cause silent data corruption
> when performing unaligned 16 or 32 byte accesses when reading from the
> local bus - typically this affects reading from flash. The problem can
> be easily shown:
>
> => md fc0c0000 10
> fc0c0000: 323e4337 01626f6f 74636d64 3d72756e    2>C7.bootcmd=run
> fc0c0010: 206e6574 5f6e6673 00626f6f 7464656c     net_nfs.bootdel
> fc0c0020: 61793d35 00626175 64726174 653d3131    ay=5.baudrate=11
> fc0c0030: 35323030 00707265 626f6f74 3d656368    5200.preboot=ech
> => md fc0c0001 10
> fc0c0001: 65636801 00000074 0000003d 00000020    ech....t...=...
> fc0c0011: 0000005f 00000000 00000074 00000061    ..._.......t...a
> fc0c0021: 00000000 00000064 00000065 00000035    .......d...e...5
> fc0c0031: 00000000 00000062 0000003d 0000006f    .......b...=...o
> => md.w fc0c0001 10
> fc0c0001: 0000 3701 0000 6f74 0000 643d 0000 6e20    ..7...ot..d=..n
> fc0c0011: 0000 745f 0000 7300 0000 6f74 0000 6c61    ..t_..s...ot..la
>
> This commit implements a workaround at least for the most blatant
> problem: using memcpy() from NOR flash. We rename the assembler
> routine into __memcpy() and provide a wrapper, which will use a
> byte-wise copy loop for unaligned source or target addresses when
> reading from NOR flash, and branch to the optimized __memcpy()
> in all other cases, thus minimizing the performance impact.
>
> Tested on lite5200b and TQM5200S.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Detlev Zundel <dzu@denx.de>

Acked-by: Detlev Zundel <dzu@denx.de>


[...]

> diff --git a/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
> new file mode 100644
> index 0000000..0950354
> --- /dev/null
> +++ b/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
> @@ -0,0 +1,71 @@
> +/*
> + * (C) Copyright 2010
> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +/*
> + * This is a workaround for issues on the MPC5200, where unaligned
> + * 32-bit-accesses to the local bus will deliver corrupted data. This
> + * happens for example when trying to use memcpy() from an odd NOR
> + * flash address; the behaviour can be also seen when using "md" on an
> + * odd NOR flash address (but there it is not a bug in U-Boot, which
> + * only shows the behaviour of this processor).
> + *
> + * For memcpy(), we test if either the source or the target address
> + * are not 32 bit aligned, and - if so - if the source address is in
> + * NOR flash: in this case we perform a byte-wise (slow) then; for

                                                           ^
                                                           +- copy

> + * aligned operations of non-flash areas we use the optimized (fast)
> + * real __memcpy().  This way we minimize the performance impact of
> + * this workaround.
> + *
> + */

[...]

Cheers
  Detlev

-- 
WARNING: The external boundaries of India as depicted in map(s) are neither
correct nor authentic.  Other external boundaries as depicted in the map(s)
may neither be correct nor authentic.
                                                 -- Garmin MapSource manual
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

Commit 460c2ce3 "MPC5200: workaround data corruption for unaligned
local bus accesses" fixed the problem for MPC5200 only, but MPC512x is
affected as well, so apply the same fix here, too.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
Cc: Anatolij Gustschin <agust@denx.de>
---
 arch/powerpc/cpu/mpc5xxx/Makefile                  |    5 -----
 arch/powerpc/lib/Makefile                          |   16 ++++++++++++----
 arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c |    0
 3 files changed, 12 insertions(+), 9 deletions(-)
 rename arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c (100%)

diff --git a/arch/powerpc/cpu/mpc5xxx/Makefile b/arch/powerpc/cpu/mpc5xxx/Makefile
index 4ab2b7b..0ee0611 100644
--- a/arch/powerpc/cpu/mpc5xxx/Makefile
+++ b/arch/powerpc/cpu/mpc5xxx/Makefile
@@ -30,11 +30,6 @@ SOBJS	= io.o firmware_sc_task_bestcomm.impl.o
 COBJS	= i2c.o traps.o cpu.o cpu_init.o ide.o interrupts.o \
 	  loadtask.o pci_mpc5200.o serial.o speed.o usb_ohci.o usb.o
 
-# Workaround for local bus unaligned access problem on MPC5200
-#ifdef CONFIG_MPC5200
-COBJS	+= memcpy_mpc5200.o
-#endif
-
 SRCS	:= $(START:.o=.S) $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
 START	:= $(addprefix $(obj),$(START))
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index bf23790..2065b6d 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -40,14 +40,22 @@ COBJS-y	+= interrupts.o
 COBJS-$(CONFIG_CMD_KGDB) += kgdb.o
 COBJS-y	+= time.o
 
-SRCS	:= $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
-OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
-
-# Workaround for local bus unaligned access problem on MPC5200
+# Workaround for local bus unaligned access problems
+# on MPC512x and MPC5200
+ifdef CONFIG_MPC512X
+$(obj)ppcstring.o: AFLAGS += -Dmemcpy=__memcpy
+COBJS-y += memcpy_mpc5200.o
+endif
 ifdef CONFIG_MPC5200
 $(obj)ppcstring.o: AFLAGS += -Dmemcpy=__memcpy
+COBJS-y += memcpy_mpc5200.o
 endif
 
+COBJS	+= $(sort $(COBJS-y))
+
+SRCS	:= $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
+OBJS	:= $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
+
 $(LIB):	$(obj).depend $(OBJS)
 	@if ! $(CROSS_COMPILE)readelf -S $(OBJS) | grep -q '\.fixup.*PROGBITS';\
 	then \
diff --git a/arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c b/arch/powerpc/lib/memcpy_mpc5200.c
similarity index 100%
rename from arch/powerpc/cpu/mpc5xxx/memcpy_mpc5200.c
rename to arch/powerpc/lib/memcpy_mpc5200.c
-- 
1.7.0.1

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Detlev Zundel]

Hi Wolfgang,

> Commit 460c2ce3 "MPC5200: workaround data corruption for unaligned
> local bus accesses" fixed the problem for MPC5200 only, but MPC512x is
> affected as well, so apply the same fix here, too.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Detlev Zundel <dzu@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
>  arch/powerpc/cpu/mpc5xxx/Makefile                  |    5 -----
>  arch/powerpc/lib/Makefile                          |   16 ++++++++++++----
>  arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c |    0
>  3 files changed, 12 insertions(+), 9 deletions(-)
>  rename arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c (100%)

Acked-by: Detlev Zundel <dzu@denx.de>

The only thing I wonder is the filename 'memcpy_mpc5200.c' as the code
doesn't really have any 5200 specifics in it.  What about
'memcpy_align32wrap' or something likew that to express the more general
nature of the code?

If one thinks further along this line, why not move the wrapper to lib/? 

Cheers
  Detlev

-- 
LISP is the most powerful programming language, and if you want an inter-
preter, LISP is the best.  None of the other languages come anywhere near
LISP in their power.  The most exciting things about LISP are read, eval,
and print.  If you look at other languages,  they have no equivalent for
any of those.                             -- Richard Stallman
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

Dear Detlev Zundel,

In message <m2vd9283ew.fsf@ohwell.denx.de> you wrote:
> 
> Acked-by: Detlev Zundel <dzu@denx.de>

thanks.

> The only thing I wonder is the filename 'memcpy_mpc5200.c' as the code
> doesn't really have any 5200 specifics in it.  What about

As far as I understand this behaviour is specific to the MPC512x and
MPC5200 processors; I did not notice it on other cores yet.
Unfortunately we do not have a generic name that includes these
processors (5xxx is taken by something else).

> 'memcpy_align32wrap' or something likew that to express the more general
> nature of the code?

I could not come up with a better name... What is "align32wrap"
supposed to mean?

> If one thinks further along this line, why not move the wrapper to lib/? 

Please re-check what my patch does - exactly that:

>  arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c |    0


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The value of marriage is not that adults produce children, but  that
children produce adults."                            - Peter De Vries

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Detlev Zundel]

Hi Wolfgang,

>> The only thing I wonder is the filename 'memcpy_mpc5200.c' as the code
>> doesn't really have any 5200 specifics in it.  What about
>
> As far as I understand this behaviour is specific to the MPC512x and
> MPC5200 processors; I did not notice it on other cores yet.
> Unfortunately we do not have a generic name that includes these
> processors (5xxx is taken by something else).

I believe that other architectures may also suffer from such unaligned
byte copies, but maybe in those cases 'memcpy' already takes car of
that, I don't know.  The point I wanted to make is that the file in
question has no powerpc specific code in it at all.

>> 'memcpy_align32wrap' or something likew that to express the more general
>> nature of the code?
>
> I could not come up with a better name... What is "align32wrap"
> supposed to mean?

It was meant to mean somehow to express the fact that for some condition
the original memcpy was called and for some conditions not - hence
"wrap".  The condition which was passed on to the original memcpy is
connected to correct 32 bit alignment, hence the proposal.

>> If one thinks further along this line, why not move the wrapper to lib/? 
>
> Please re-check what my patch does - exactly that:
>
>>  arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c |    0

Actually I was aiming for "lib" outside of any architecure for the
reason noted above.

But as I said, this is only place for improvement, I surely will not nak
the patch ;)

Cheers
  Detlev

-- 
System going down at 1:45 this afternoon for disk crashing.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Joakim Tjernlund]

>
> Dear Detlev Zundel,
>
> In message <m2vd9283ew.fsf@ohwell.denx.de> you wrote:
> >
> > Acked-by: Detlev Zundel <dzu@denx.de>
>
> thanks.
>
> > The only thing I wonder is the filename 'memcpy_mpc5200.c' as the code
> > doesn't really have any 5200 specifics in it.  What about
>
> As far as I understand this behaviour is specific to the MPC512x and
> MPC5200 processors; I did not notice it on other cores yet.
> Unfortunately we do not have a generic name that includes these
> processors (5xxx is taken by something else).
>
> > 'memcpy_align32wrap' or something likew that to express the more general
> > nature of the code?
>
> I could not come up with a better name... What is "align32wrap"
> supposed to mean?

mpc5200_memcpy_fromio() resp. mpc5200_memcpy_toio()?

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <OFEB4E68BC.F6B8C9D0-ONC1257751.0045AE08-C1257751.0045DA56@transmode.se> you wrote:
>
> > I could not come up with a better name... What is "align32wrap"
> > supposed to mean?
> 
> mpc5200_memcpy_fromio() resp. mpc5200_memcpy_toio()?

No. It's not only MPC5200, but also MPC521x. It's not I/O in general,
but only I/O from the Local Bus. And even then only unaliged read
access.

But memcpy_for_unaligned_read_from_local_bus() was too long for me,
and memcpy_furflb() too cryptic ;-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"There is nothing new under the sun, but there are lots of old things
we don't know yet."                                  - Ambrose Bierce

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 2010/06/29 14:55:44:
>
> Dear Joakim Tjernlund,
>
> In message <OFEB4E68BC.F6B8C9D0-ONC1257751.0045AE08-C1257751.
> 0045DA56 at transmode.se> you wrote:
> >
> > > I could not come up with a better name... What is "align32wrap"
> > > supposed to mean?
> >
> > mpc5200_memcpy_fromio() resp. mpc5200_memcpy_toio()?
>
> No. It's not only MPC5200, but also MPC521x. It's not I/O in general,
> but only I/O from the Local Bus. And even then only unaliged read
> access.
>
> But memcpy_for_unaligned_read_from_local_bus() was too long for me,
> and memcpy_furflb() too cryptic ;-)

memcpy_align_src()?

   Jocke

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Joakim Tjernlund]

>
> Wolfgang Denk <wd@denx.de> wrote on 2010/06/29 14:55:44:
> >
> > Dear Joakim Tjernlund,
> >
> > In message <OFEB4E68BC.F6B8C9D0-ONC1257751.0045AE08-C1257751.
> > 0045DA56 at transmode.se> you wrote:
> > >
> > > > I could not come up with a better name... What is "align32wrap"
> > > > supposed to mean?
> > >
> > > mpc5200_memcpy_fromio() resp. mpc5200_memcpy_toio()?
> >
> > No. It's not only MPC5200, but also MPC521x. It's not I/O in general,
> > but only I/O from the Local Bus. And even then only unaliged read
> > access.
> >
> > But memcpy_for_unaligned_read_from_local_bus() was too long for me,
> > and memcpy_furflb() too cryptic ;-)
>
> memcpy_align_src()?
>
>    Jocke

BTW, perhaps this memcpy I wrote for uClibc long time ago
is useful? You can easily change dst or src alignment:

void *memcpy(void *to, const void *from, size_t len)
{
	unsigned long rem, chunks, tmp1, tmp2;
	unsigned char *tmp_to;
	unsigned char *tmp_from = (unsigned char *)from;

	chunks = len / 8;
	tmp_from -= 4;
	tmp_to = to - 4;
	if (!chunks)
		goto lessthan8;
	rem = (unsigned long )tmp_to % 4;
	if (rem)
		goto align;
 copy_chunks:
	do {
		/* make gcc to load all data, then store it */
		tmp1 = *(unsigned long *)(tmp_from+4);
		tmp_from += 8;
		tmp2 = *(unsigned long *)tmp_from;
		*(unsigned long *)(tmp_to+4) = tmp1;
		tmp_to += 8;
		*(unsigned long *)tmp_to = tmp2;
	} while (--chunks);
 lessthan8:
	len = len % 8;
	if (len >= 4) {
		tmp_from += 4;
		tmp_to += 4;
		*(unsigned long *)(tmp_to) = *(unsigned long *)(tmp_from);
		len -= 4;
	}
	if (!len)
		return to;
	tmp_from += 3;
	tmp_to += 3;
	do {
		*++tmp_to = *++tmp_from;
	} while (--len);

	return to;
 align:
	/* ???: Do we really need to generate the carry flag here? If not, then:
	rem -= 4; */
	rem = 4 - rem;
	len -= rem;
	do {
		*(tmp_to+4) = *(tmp_from+4);
		++tmp_from;
		++tmp_to;
	} while (--rem);
	chunks = len / 8;
	if (chunks)
		goto copy_chunks;
	goto lessthan8;
}

[U-Boot] [PATCH] MPC512x: workaround data corruption forunaligned local bus accesses

[Steve Deiters]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Wolfgang Denk
> Sent: Tuesday, June 29, 2010 7:56 AM
> To: Joakim Tjernlund
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] MPC512x: workaround data 
> corruption forunaligned local bus accesses
> 
> Dear Joakim Tjernlund,
> 
> In message 
> <OFEB4E68BC.F6B8C9D0-ONC1257751.0045AE08-C1257751.0045DA56@tra
> nsmode.se> you wrote:
> >
> > > I could not come up with a better name... What is "align32wrap"
> > > supposed to mean?
> > 
> > mpc5200_memcpy_fromio() resp. mpc5200_memcpy_toio()?
> 
> No. It's not only MPC5200, but also MPC521x. It's not I/O in 
> general, but only I/O from the Local Bus. And even then only 
> unaliged read access.
> 
> But memcpy_for_unaligned_read_from_local_bus() was too long 
> for me, and memcpy_furflb() too cryptic ;-)
> 
> Best regards,
> 
> Wolfgang Denk

I just posted a patch on the linuxppc-dev list that simply uses a
slightly modified version of memcpy to always keep the source address
aligned.  I had conditionals in that one so it only used it for MPC512x
or MPC52xx but you should be able to replace the regular memcpy with
this version.  This way you can avoid the wrappers and extra checks.  It
is a simple enough change in that case:


diff --git a/arch/powerpc/lib/ppcstring.S b/arch/powerpc/lib/ppcstring.S
index 97023a0..4e17265 100644
--- a/arch/powerpc/lib/ppcstring.S
+++ b/arch/powerpc/lib/ppcstring.S
@@ -114,7 +114,7 @@ memcpy:
 	addi	r6,r3,-4
 	addi	r4,r4,-4
 	beq	2f			/* if less than 8 bytes to do */
-	andi.	r0,r6,3			/* get dest word aligned */
+	andi.	r0,r4,3			/* get src word aligned */
 	mtctr	r7
 	bne	5f
 1:	lwz	r7,4(r4)
@@ -125,6 +125,8 @@ memcpy:
 	andi.	r5,r5,7
 2:	cmplwi	0,r5,4
 	blt	3f
+	andi.	r0,r4,3
+	bne	3f
 	lwzu	r0,4(r4)
 	addi	r5,r5,-4
 	stwu	r0,4(r6)

[U-Boot] [PATCH] MPC512x: workaround data corruption forunaligned local bus accesses

[Wolfgang Denk]

Dear "Steve Deiters",

In message <181804936ABC2349BE503168465576460F272CE9@exchserver.basler.com> you wrote:
>
> I just posted a patch on the linuxppc-dev list that simply uses a
> slightly modified version of memcpy to always keep the source address
> aligned.  I had conditionals in that one so it only used it for MPC512x
> or MPC52xx but you should be able to replace the regular memcpy with
> this version.  This way you can avoid the wrappers and extra checks. It
> is a simple enough change in that case:

Thanks. I'll keep this queued and try to run some tests with it on
different architectures - I don't know which side effects might be
caused by changing the alignment from target to source address, so I
will not introduce such a change at this point in the release cycle,
literally minutes before a release.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If you hear an onion ring, answer it.

[U-Boot] [PATCH] MPC512x: workaround data corruption forunaligned local bus accesses

[Joakim Tjernlund]

Wolfgang Denk <wd@denx.de> wrote on 2010/06/29 21:24:03:
>
> Dear "Steve Deiters",
>
> In message <181804936ABC2349BE503168465576460F272CE9@exchserver.basler.com> you wrote:
> >
> > I just posted a patch on the linuxppc-dev list that simply uses a
> > slightly modified version of memcpy to always keep the source address
> > aligned.  I had conditionals in that one so it only used it for MPC512x
> > or MPC52xx but you should be able to replace the regular memcpy with
> > this version.  This way you can avoid the wrappers and extra checks. It
> > is a simple enough change in that case:
>
> Thanks. I'll keep this queued and try to run some tests with it on
> different architectures - I don't know which side effects might be
> caused by changing the alignment from target to source address, so I
> will not introduce such a change at this point in the release cycle,
> literally minutes before a release.

Try the C memcpy I sent. It will/should compile into the same
assembler(it did when I wrote it). It is far more generic and
is easier to understand.

 Jocke

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Anatolij Gustschin]

Hi Detlev, Hi Wolfgang,

On Tue, 29 Jun 2010 13:49:11 +0200
Detlev Zundel <dzu@denx.de> wrote:

> > Commit 460c2ce3 "MPC5200: workaround data corruption for unaligned
> > local bus accesses" fixed the problem for MPC5200 only, but MPC512x is
> > affected as well, so apply the same fix here, too.
> >
> > Signed-off-by: Wolfgang Denk <wd@denx.de>
> > Cc: Detlev Zundel <dzu@denx.de>
> > Cc: Anatolij Gustschin <agust@denx.de>
> > ---
> >  arch/powerpc/cpu/mpc5xxx/Makefile                  |    5 -----
> >  arch/powerpc/lib/Makefile                          |   16 ++++++++++++----
> >  arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c |    0
> >  3 files changed, 12 insertions(+), 9 deletions(-)
> >  rename arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c (100%)
> 
> Acked-by: Detlev Zundel <dzu@denx.de>
> 
> The only thing I wonder is the filename 'memcpy_mpc5200.c' as the code
> doesn't really have any 5200 specifics in it.  What about
> 'memcpy_align32wrap' or something likew that to express the more general
> nature of the code?

'memcpy_align32wrap' isn't really good since the fixed memcpy
also fixes 16-bit accesses, too.

BTW, shouldn't we fix print_buffer() also? do_mem_md() doesn't use
memcpy() and the issue with corrupted dumps still remains here.
I'm testing a patch to fix it. Will submit it soon.

Best regards,
Anatolij

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

Dear Anatolij,

In message <20100629142343.7ecadfaa@wker> you wrote:
> 
> 'memcpy_align32wrap' isn't really good since the fixed memcpy
> also fixes 16-bit accesses, too.

we discussed this a bit more internally, and could not find a good
name yet, either. Guess I'll stick with the current one until someone
comes up with a really good idea.

> BTW, shouldn't we fix print_buffer() also? do_mem_md() doesn't use
> memcpy() and the issue with corrupted dumps still remains here.
> I'm testing a patch to fix it. Will submit it soon.

I though about this, too. But then I decided against it. "md" is
intended to to exactly what the user requests - as is, we can easily
demonstrate the issue. I consider this an extremely useful debug
feature. If I command the memory to be read in units of 32 bits I
really mean that. A "fix" ins print_buffer() whould hush up the
symptoms, so the problem would be much harder to detect.

Let's keep this as is, please: if you need a memory dump, you can
either align your start address, or use "md.b".

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"How to make a million dollars:  First, get a million dollars."
- Steve Martin

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Anatolij Gustschin]

Dear Wolfgang,

On Tue, 29 Jun 2010 14:47:14 +0200
Wolfgang Denk <wd@denx.de> wrote:

...
> > BTW, shouldn't we fix print_buffer() also? do_mem_md() doesn't use
> > memcpy() and the issue with corrupted dumps still remains here.
> > I'm testing a patch to fix it. Will submit it soon.
> 
> I though about this, too. But then I decided against it. "md" is
> intended to to exactly what the user requests - as is, we can easily
> demonstrate the issue. I consider this an extremely useful debug
> feature. If I command the memory to be read in units of 32 bits I
> really mean that. A "fix" ins print_buffer() whould hush up the
> symptoms, so the problem would be much harder to detect.

Thanks for the explanation, I didn't thought about it. This is
indeed a reasonable debug feature.

Best regards,
Anatolij

[U-Boot] [PATCH] Use memcpy in print_buffer to fix unaligned dumps

[Anatolij Gustschin]

Unaligned 32-/16-bit accesses to local bus on MPC5200
and MPC512x deliver corrupted data:

=> md F0000000 10
f0000000: 27051956 552d426f 6f742032 3031302e    '..VU-Boot 2010.
f0000010: 30362d72 63332d30 38303336 2d676665    06-rc3-08036-gfe
f0000020: 38663238 362d6469 72747920 284a756e    8f286-dirty (Jun
f0000030: 20323920 32303130 202d2031 343a3235     29 2010 - 14:25
=> md F0000001 10
f0000001: 00005655 00006f6f 00003230 00002e30    ..VU..oo..20...0
f0000011: 00007263 00003038 0000362d 00006538    ..rc..08..6-..e8
f0000021: 00003836 00006972 00002028 00006e20    ..86..ir.. (..n
f0000031: 00002032 00003020 00003134 0000353a    .. 2..0 ..14..5:
=> md.w F0000001 20
f0000001: 0000 5655 0000 6f6f 0000 3230 0000 2e30    ..VU..oo..20...0
f0000011: 0000 7263 0000 3038 0000 362d 0000 6538    ..rc..08..6-..e8
f0000021: 0000 3836 0000 6972 0000 2028 0000 6e20    ..86..ir.. (..n
f0000031: 0000 2032 0000 3020 0000 3134 0000 353a    .. 2..0 ..14..5:

Use memcpy in print_buffer() to fix the problem.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 lib/display_options.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/lib/display_options.c b/lib/display_options.c
index a711425..c0b89b2 100644
--- a/lib/display_options.c
+++ b/lib/display_options.c
@@ -122,10 +122,24 @@ int print_buffer (ulong addr, void* data, uint width, uint count, uint linelen)
 		/* Copy from memory into linebuf and print hex values */
 		for (i = 0; i < linelen; i++) {
 			if (width == 4) {
+#if defined(CONFIG_MPC5200) || defined(CONFIG_MPC512X)
+				/*
+				 * workaround for issues on the MPC5200/MPC512X,
+				 * where unaligned 32-/16-bit-accesses to the
+				 * local bus will deliver corrupted data. Just
+				 * use fixed memcpy here.
+				 */
+				memcpy(&uip[i], data, 4);
+#else
 				uip[i] = *(volatile uint32_t *)data;
+#endif
 				printf(" %08x", uip[i]);
 			} else if (width == 2) {
+#if defined(CONFIG_MPC5200) || defined(CONFIG_MPC512X)
+				memcpy(&usp[i], data, 2);
+#else
 				usp[i] = *(volatile uint16_t *)data;
+#endif
 				printf(" %04x", usp[i]);
 			} else {
 				ucp[i] = *(volatile uint8_t *)data;
-- 
1.7.0.4

[U-Boot] [PATCH] Use memcpy in print_buffer to fix unaligned dumps

[Wolfgang Denk]

Dear Anatolij Gustschin,

In message <1277815514-32120-1-git-send-email-agust@denx.de> you wrote:
> Unaligned 32-/16-bit accesses to local bus on MPC5200
> and MPC512x deliver corrupted data:

Right, and the current version of print_buffer() shopws the real data,
i. e. you can see that such a corruption happens.

> Use memcpy in print_buffer() to fix the problem.

NAK. This violates the design of the command.

>  		for (i = 0; i < linelen; i++) {
>  			if (width == 4) {

The "width == 4" part means that we want to access the memory with 32
bit accesses - nothing else.

> +#if defined(CONFIG_MPC5200) || defined(CONFIG_MPC512X)
> +				/*
> +				 * workaround for issues on the MPC5200/MPC512X,
> +				 * where unaligned 32-/16-bit-accesses to the
> +				 * local bus will deliver corrupted data. Just
> +				 * use fixed memcpy here.
> +				 */
> +				memcpy(&uip[i], data, 4);
> +#else
>  				uip[i] = *(volatile uint32_t *)data;
> +#endif

If we would go this route, we could drop the #ifdef and always
perform a memcpy(), but then we have absolutley no guarantee about
which kind of accesses would be used.

No, please leave as is - this part of U-Boot is working exactly as
intended.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Be kind to unkind people - they need it the most.

[U-Boot] [PATCH] Use memcpy in print_buffer to fix unaligned dumps

[Wolfgang Denk]

Dear Anatolij,

I wrote:

> > Use memcpy in print_buffer() to fix the problem.
> 
> NAK. This violates the design of the command.
> 
> >  		for (i = 0; i < linelen; i++) {
> >  			if (width == 4) {
> 
> The "width == 4" part means that we want to access the memory with 32
> bit accesses - nothing else.

Also note that the fix would be incomplete - the same issue happens
with unaligned 16 bit accesses:

=> md.w FC0C0000 10
fc0c0000: 7012 ab65 0161 6464 636f 6e73 3d73 6574    p..e.addcons=set
fc0c0010: 656e 7620 626f 6f74 6172 6773 2024 7b62    env bootargs ${b
=> md.w FC0C0001 10
fc0c0001: 007b 6501 0000 6463 0000 733d 0000 7465    .{e...dc..s=..te
fc0c0011: 0000 2062 0000 7461 0000 7320 0000 626f    .. b..ta..s ..bo


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Politics:  A  strife  of  interests  masquerading  as  a  contest  of
principles. The conduct of public affairs for private advantage.
- Ambrose Bierce

[U-Boot] [PATCH] Use memcpy in print_buffer to fix unaligned dumps

[Mike Frysinger]

On Tuesday, June 29, 2010 08:53:06 Wolfgang Denk wrote:
> Anatolij Gustschin wrote:
> > Unaligned 32-/16-bit accesses to local bus on MPC5200
> > and MPC512x deliver corrupted data:
>
> Right, and the current version of print_buffer() shopws the real data,
> i. e. you can see that such a corruption happens.

i also prefer the current behavior.  on Blackfin systems, you get unaligned 
exceptions when you try to do 'md.l 1' and it's a nice way to quickly exercise 
those code paths ;).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100629/2260d730/attachment.pgp 

[U-Boot] [PATCH] MPC512x: workaround data corruption for unaligned local bus accesses

[Wolfgang Denk]

In message <1277804892-453-1-git-send-email-wd@denx.de> you wrote:
> Commit 460c2ce3 "MPC5200: workaround data corruption for unaligned
> local bus accesses" fixed the problem for MPC5200 only, but MPC512x is
> affected as well, so apply the same fix here, too.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Detlev Zundel <dzu@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
>  arch/powerpc/cpu/mpc5xxx/Makefile                  |    5 -----
>  arch/powerpc/lib/Makefile                          |   16 ++++++++++++----
>  arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c |    0
>  3 files changed, 12 insertions(+), 9 deletions(-)
>  rename arch/powerpc/{cpu/mpc5xxx => lib}/memcpy_mpc5200.c (100%)

Applied


Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Men of peace usually are [brave].
	-- Spock, "The Savage Curtain", stardate 5906.5