[U-Boot] [PATCH] MPC5200: workaround data corruption for unaligned local bus accesses
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@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 \
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@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 \
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@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 \
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@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
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
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
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
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
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()?
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
Wolfgang Denk wd@denx.de wrote on 2010/06/29 14:55:44:
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 ;-)
memcpy_align_src()?
Jocke
Wolfgang Denk wd@denx.de wrote on 2010/06/29 14:55:44:
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 ;-)
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; }
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Wolfgang Denk Sent: Tuesday, June 29, 2010 7:56 AM To: Joakim Tjernlund Cc: u-boot@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)
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
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
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
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
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
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;
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
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
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
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
participants (6)
-
Anatolij Gustschin -
Detlev Zundel -
Joakim Tjernlund -
Mike Frysinger -
Steve Deiters -
Wolfgang Denk