ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Subsystems (3)
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
Reading C99 6.7.3 8 and the comment 114) there, I think it is a bug of that gcc version to ignore the volatile type qualifier used e.g. in __arch_getl(). Anyway, using a definition as in the kernel headers avoids such optimizations when gcc 4.5.1 is used.
Maybe the headers as used in the current linux-kernel should be used, but to avoid large changes, I've just added a small change to the current headers.
--- arch/arm/include/asm/io.h | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
arch/arm/include/asm/io.h +14 -6
@@ -125,13 +125,21 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen)#define __raw_readw (a) __arch_getw(a)#define __raw_readl (a) __arch_getl(a) -#define writeb (v,a) __arch_putb(v,a) -#define writew (v,a) __arch_putw(v,a) -#define writel (v,a) __arch_putl(v,a) +/* +* TODO: The kernel offers some more advanced versions of barriers, it might +* have some advantages to use them instead of the simple one here. +*/ +#define dmb () __asm__ __volatile__ ("" : : : "memory") +#define __iormb () dmb() +#define __iowmb () dmb() + +#define writeb (v,c) ({ __iowmb(); __arch_putb(v,c); }) +#define writew (v,c) ({ __iowmb(); __arch_putw(v,c); }) +#define writel (v,c) ({ __iowmb(); __arch_putl(v,c); }) -#define readb (a) __arch_getb(a) -#define readw (a) __arch_getw(a) -#define readl (a) __arch_getl(a) +#define readb (c) ({ u8 __v = __arch_getb(c); __iormb(); __v; }) +#define readw (c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) +#define readl (c) ({ u32 __v = __arch_getl(c); __iormb(); __v; })/* * The compiler seems to be incapable of optimising constants
Signature · 1 line
1.7.2.2
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 35 lines · Alexander Holler
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
Reading C99 6.7.3 8 and the comment 114) there, I think it is a bug of that gcc version to ignore the volatile type qualifier used e.g. in
__arch_getl(). Anyway, using a definition as in the kernel headers avoids such optimizations when gcc 4.5.1 is used.Maybe the headers as used in the current linux-kernel should be used, but to avoid large changes, I've just added a small change to the current headers.
Signed-off-by Alexander Holler<[email protected]>--- arch/arm/include/asm/io.h | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)arch/arm/include/asm/io.h +8
-3@@ -125,13 +125,21 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen)#define __raw_readw (a) __arch_getw(a)#define __raw_readl (a) __arch_getl(a) -#define writeb (v,a) __arch_putb(v,a) -#define writew (v,a) __arch_putw(v,a) -#define writel (v,a) __arch_putl(v,a) +/* +* TODO: The kernel offers some more advanced versions of barriers, it might +* have some advantages to use them instead of the simple one here. +*/ +#define dmb () __asm__ __volatile__ ("" : : : "memory") +#define __iormb () dmb() +#define __iowmb () dmb() +
arch/arm/include/asm/io.h extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) · line 142
Quoted 10 lines
arch/arm/include/asm/io.h +6
-3+#define writeb (v,c) ({ __iowmb(); __arch_putb(v,c); }) +#define writew (v,c) ({ __iowmb(); __arch_putw(v,c); }) +#define writel (v,c) ({ __iowmb(); __arch_putl(v,c); }) -#define readb (a) __arch_getb(a) -#define readw (a) __arch_getw(a) -#define readl (a) __arch_getl(a) +#define readb (c) ({ u8 __v = __arch_getb(c); __iormb(); __v; }) +#define readw (c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) +#define readl (c) ({ u32 __v = __arch_getl(c); __iormb(); __v; })
Using the test code below [1] and then looking at the disassembly from the two tool chains gcc version 4.3.3 (Sourcery G++ Lite 2009q1-203) versus gcc version 4.5.1 (Sourcery G++ Lite 2010.09-50): Yes, without the additional dmb() the gcc 4.5.1 just creates
00000000 <main>:
0: e3a00000 mov r0, #0
4: e12fff1e bx lrwhile with the additional dmb() it creates
00000000 <main>:
0: e59f300c ldr r3, [pc, #12] ; 14 <main+0x14>
4: e5932028 ldr r2, [r3, #40] ; 0x28
8: e5930028 ldr r0, [r3, #40] ; 0x28
c: e0620000 rsb r0, r2, r0 10: e12fff1e bx lr
14: 48318000
what looks correct. And 4.3.3 does the same code for both readl() versions. So:
Thanks
Dirk
[1]
arm-none-linux-gnueabi-gcc -Wall -O2 -c foo.c -o foo.o arm-none-linux-gnueabi-objdump -D foo.o > foo.dis
-- foo.c --
struct gptimer {
unsigned int tidr; /* 0x00 r */
unsigned char res[0xc];
unsigned int tiocp_cfg; /* 0x10 rw */
unsigned int tistat; /* 0x14 r */
unsigned int tisr; /* 0x18 rw */
unsigned int tier; /* 0x1c rw */
unsigned int twer; /* 0x20 rw */
unsigned int tclr; /* 0x24 rw */
unsigned int tcrr; /* 0x28 rw */
unsigned int tldr; /* 0x2c rw */
unsigned int ttgr; /* 0x30 rw */
unsigned int twpc; /* 0x34 r*/
unsigned int tmar; /* 0x38 rw*/
unsigned int tcar1; /* 0x3c r */
unsigned int tcicr; /* 0x40 rw */
unsigned int tcar2; /* 0x44 r */
};#define dmb() __asm__ __volatile__ ("" : : : "memory")
#define __iormb() dmb()
#define __arch_getl(a) (*(volatile unsigned int *)(a))
#define readl(a) __arch_getl(a)
//#define readl(c) ({ unsigned int __v = __arch_getl(c); __iormb();__v; })
int main(void) {
struct gptimer *gpt1_base = (struct gptimer *)0x48318000; unsigned int cdiff, cstart, cend;
cstart = readl(&gpt1_base->tcrr); cend = readl(&gpt1_base->tcrr); cdiff = cend - cstart;
return cdiff;
}
-- foo.c --
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Hello,
Quoted 6 lines · Dirk Behme
Quoted 3 lines · Alexander Holler
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
Acked-by Dirk Behme <[email protected]>
Thanks for the ack, but I have to say, that those barriers are having side effects here. Reading NAND now fails on my BeagleBoard. Regardless if I use gcc 4.3.5 or gcc 4.5.1, after applying that patch the wrong ID of the NAND is read. In nand_get_flash_type() (drivers/mtd/nand/nand_base.c) without that patch I will get the following:
*maf_id: 44, dev_id: 186
with the patch the following is read:
*maf_id: 128, dev_id: 85
Which just is wrong.
I haven't looked further up to now, maybe thats just a side effect of some wrong clock settings because of different timings through those barrieres or whatever.
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Le 19/12/2010 11:22, Alexander Holler a ?crit :
Quoted 20 lines
Hello,
Quoted 6 lines · Dirk Behme
Quoted 3 lines · Alexander Holler
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
Acked-by Dirk Behme<[email protected]>Thanks for the ack, but I have to say, that those barriers are having side effects here. Reading NAND now fails on my BeagleBoard. Regardless if I use gcc 4.3.5 or gcc 4.5.1, after applying that patch the wrong ID of the NAND is read. In
nand_get_flash_type()(drivers/mtd/nand/nand_base.c) without that patch I will get the following:
*maf_id: 44,dev_id: 186with the patch the following is read:
Quoted 7 lines
*maf_id: 128,dev_id: 85Which just is wrong.
I haven't looked further up to now, maybe thats just a side effect of some wrong clock settings because of different timings through those barrieres or whatever.
IIUC, the patch is adding barriers to every HW register write and read, which makes the compiler stop reordering these, and keep them ordered as in the source code. Are you sure that the order as laid out in the source is the right one? Maybe you were just luck so far that the reordering masked a bug.
Amicalement,
Signature · 1 line
Albert.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Hello,
Am 19.12.2010 12:28, schrieb Albert ARIBAUD:
Quoted 29 lines
Le 19/12/2010 11:22, Alexander Holler a ?crit :
Quoted 20 lines
Hello,
Quoted 6 lines · Dirk Behme
Quoted 3 lines · Alexander Holler
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
Acked-by Dirk Behme<[email protected]>Thanks for the ack, but I have to say, that those barriers are having side effects here. Reading NAND now fails on my BeagleBoard. Regardless if I use gcc 4.3.5 or gcc 4.5.1, after applying that patch the wrong ID of the NAND is read. In
nand_get_flash_type()(drivers/mtd/nand/nand_base.c) without that patch I will get the following:
*maf_id: 44,dev_id: 186with the patch the following is read:
Quoted 7 lines
*maf_id: 128,dev_id: 85Which just is wrong.
I haven't looked further up to now, maybe thats just a side effect of some wrong clock settings because of different timings through those barrieres or whatever.
Quoted 5 lines
IIUC, the patch is adding barriers to every HW register write and read, which makes the compiler stop reordering these, and keep them ordered as in the source code. Are you sure that the order as laid out in the source is the right one? Maybe you were just luck so far that the reordering masked a bug.
I don't know much about all the stuff the omap3-drivers are doing. E.g. I've already wondered why it's necessary to measure a clock frequency in the code which got (wrongly) optimized without that patch. I would think such isn't necessary because I would assume the drivers are actually there to set the clock settings, and not to measure them. ;)
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to Alexander HollerQuoted 10 lines · Alexander Holler
side effects here. Reading NAND now fails on my BeagleBoard. Regardless if I use gcc 4.3.5 or gcc 4.5.1, after applying that patch the wrong ID of the NAND is read. In
nand_get_flash_type()(drivers/mtd/nand/nand_base.c) without that patch I will get the following:
*maf_id: 44,dev_id: 186with the patch the following is read:
*maf_id: 128,dev_id: 85
The nand_get_flash_type routine reads these id's twice and compares them. Do your see an error message like this?
second ID read did not match
Quoted 15 lines
Which just is wrong.
I haven't looked further up to now, maybe thats just a side effect of some wrong clock settings because of different timings through those barrieres or whatever.
Regards,
Alexander
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Am 19.12.2010 19:45, schrieb John Rigby:
Quoted 8 lines · Alexander Holler
Quoted 7 lines
side effects here. Reading NAND now fails on my BeagleBoard. Regardless if I use gcc 4.3.5 or gcc 4.5.1, after applying that patch the wrong ID of the NAND is read. In
nand_get_flash_type()(drivers/mtd/nand/nand_base.c) without that patch I will get the following:
*maf_id: 44,dev_id: 186
Quoted 8 lines
Quoted 3 lines
with the patch the following is read:
*maf_id: 128,dev_id: 85The
nand_get_flash_typeroutine reads these id's twice and compares them. Do your see an error message like this?second ID read did not match
No, the output is
without memory barrier in read*/write*:
--------------------------------
U-Boot 2010.12-rc3-00013-g71aab09 (Dec 19 2010 - 01:19:38)
OMAP3530-GP ES3.1, CPU-OPP2, L3-165MHz, Max CPU Clock 720 mHz OMAP3 Beagle board + LPDDR/NAND
I2C: ready DRAM: 256 MiB NAND: 256 MiB MMC: OMAP SD/MMC: 0 In: serial Out: serial Err: serial
Beagle Rev C4
timed out in wait_for_pin: I2C_STAT=0
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: 0
OMAP3 beagleboard.org # nand info
Device 0: nand0, sector size 128 KiB
--------------------------------
with memory barrier in read*/write*:
--------------------------------
U-Boot 2010.12-rc3-00014-gde0126f (Dec 19 2010 - 01:25:11)
OMAP3530-GP ES3.1, CPU-OPP2, L3-165MHz, Max CPU Clock 720 mHz OMAP3 Beagle board + LPDDR/NAND
I2C: ready DRAM: 256 MiB NAND: 32 MiB MMC: OMAP SD/MMC: 0
NAND read from offset 260000 failed -74
*** Warning - readenv() failed, using default environment
In: serial Out: serial Err: serial
Beagle Rev C4
timed out in wait_for_pin: I2C_STAT=0
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: 0
OMAP3 beagleboard.org # nand info
Device 0: nand0, sector size 16 KiB
--------------------------------
And with the memory barrier the kernel gets started but hangs afterwards (at least it looks so). I still haven't searched further and can only offer speculations like clocks are screwed up, memory setup is broken or such. Finding the reason and not just the impact will need some more time on my side.
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
On Sun, Dec 19, 2010 at 12:59 PM, Alexander Holler <[email protected]> wrote:
...
Quoted 8 lines
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: ?0
OMAP3 beagleboard.org # nand infoDevice 0: nand0, sector size 16 KiB
--------------------------------
I get the same output without your change. My gcc is linaro 4.4.5. I'll do some bisecting and try to find out what is going on.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Am 20.12.2010 01:39, schrieb John Rigby:
Quoted 7 lines · Alexander Holler
...
Quoted 5 lines
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: 0
OMAP3 beagleboard.org # nand info
Quoted 5 lines
Quoted 3 lines
Device 0: nand0, sector size 16 KiB
--------------------------------I get the same output without your change. My gcc is linaro 4.4.5. I'll do some bisecting and try to find out what is going on.
Bisecting won't help you here. Not if the problem was always there (which is what I assume).
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 11 lines · Alexander Holler
Am 20.12.2010 01:39, schrieb John Rigby:
Quoted 10 lines
On Sun, Dec 19, 2010 at 12:59 PM, Alexander Holler<[email protected]> ?wrote:
...Quoted 6 lines
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: ?0
OMAP3 beagleboard.org # nand info
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Device 0: nand0, sector size 16 KiB
--------------------------------I get the same output ?without your change. ?My gcc is linaro 4.4.5. I'll do some bisecting and try to find out what is going on.
Bisecting won't help you here. Not if the problem was always there (which is what I assume
Sorry, I was confused about my results.
If I replace include <asm/io.h> in drivers/mtd/nand/omap_gpmc.c with a copy of the original called orig_io.h:
#include "orig_io.h"
Nand starts working again. So the problem seems to be isolated to this file.
Quoted 5 lines
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 20 lines · John Rigby
Quoted 11 lines · Alexander Holler
Am 20.12.2010 01:39, schrieb John Rigby:
Quoted 10 lines
On Sun, Dec 19, 2010 at 12:59 PM, Alexander Holler<[email protected]> ?wrote:
...Quoted 6 lines
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: ?0
OMAP3 beagleboard.org # nand infoQuoted 8 lines
Quoted 5 lines
Quoted 3 lines
Device 0: nand0, sector size 16 KiB
--------------------------------I get the same output ?without your change. ?My gcc is linaro 4.4.5. I'll do some bisecting and try to find out what is going on.
Bisecting won't help you here. Not if the problem was always there (which is what I assume
Quoted 13 lines
Sorry, I was confused about my results.
If I replace include <
asm/io.h> indrivers/mtd/nand/omap_gpmc.cwith a copy of the original calledorig_io.h:
#include "orig_io.h"Nand starts working again. ?So the problem seems to be isolated to this file.
Quoted 5 lines
Regards,
Alexander
With your patch and the following hack nand works:
drivers/mtd/nand/omap_gpmc.c +2 -0
@@ -29,6 +29,8 @@#include <linux/mtd/nand_ecc.h> #include <nand.h> +#define origwriteb (v,a) __arch_putb(v,a) +static uint8_t cs;static struct nand_ecclayout hw_nand_oob =GPMC_NAND_HW_ECC_LAYOUT ;
@@ -58,7 +60,7 @@ static void omap_nand_hwcontrol(struct mtd_info
*mtd, int32_t cmd,
}
if (cmd != NAND_CMD_NONE)- writeb(cmd, this->IO_ADDR_W);
+ origwriteb(cmd, this->IO_ADDR_W);
}
/*
The working assembly looks like this:
if (cmd != NAND_CMD_NONE) 80024d28: e3710001 cmn r1, #1 origwriteb(cmd, this->IO_ADDR_W);
80024d2c: 15933004 ldrne r3, [r3, #4]80024d30: 120110ff andne r1, r1, #255 ; 0xff80024d34: 15c31000 strbne r1, [r3]80024d38: e8bd8010 pop {r4, pc}
The broken assembly looks like this:
if (cmd != NAND_CMD_NONE)
80024d28: e3710001 cmn r1, #1
80024d2c: 08bd8010 popeq {r4, pc}
writeb(cmd, this->IO_ADDR_W);80024d30: e5933004 ldr r3, [r3, #4]80024d34: e20110ff and r1, r1, #255 ; 0xff80024d38: e5c31000 strb r1, [r3]80024d3c: e5d33000 ldrb r3, [r3]80024d40: e8bd8010 pop {r4, pc}
This is with gcc version "(Ubuntu/Linaro 4.4.4-14ubuntu4) 4.4.5". I'll try a 4.5.2 version next and see what happens.
br,
John
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 78 lines · John Rigby
Quoted 20 lines · John Rigby
Quoted 11 lines · Alexander Holler
Am 20.12.2010 01:39, schrieb John Rigby:
Quoted 10 lines
On Sun, Dec 19, 2010 at 12:59 PM, Alexander Holler<[email protected]>
wrote:
...Quoted 6 lines
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: 0
OMAP3 beagleboard.org # nand infoQuoted 8 lines
Quoted 5 lines
Quoted 3 lines
Device 0: nand0, sector size 16 KiB
--------------------------------I get the same output without your change. My gcc is linaro 4.4.5. I'll do some bisecting and try to find out what is going on.
Bisecting won't help you here. Not if the problem was always there (which is what I assume
Quoted 13 lines
Sorry, I was confused about my results.
If I replace include<asm/io.h> in
drivers/mtd/nand/omap_gpmc.cwith a copy of the original calledorig_io.h:
#include "orig_io.h"Nand starts working again. So the problem seems to be isolated to this file.
Quoted 5 lines
Regards,
Alexander
With your patch and the following hack nand works:
drivers/mtd/nand/omap_gpmc.c +3
-1@@ -29,6 +29,8 @@#include <linux/mtd/nand_ecc.h> #include <nand.h> +#define origwriteb (v,a) __arch_putb(v,a) +static uint8_t cs;static struct nand_ecclayout hw_nand_oob =GPMC_NAND_HW_ECC_LAYOUT ;@@ -58,7 +60,7 @@ static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd, }if (cmd !=NAND_CMD_NONE ) -writeb (cmd, this->IO_ADDR_W); +origwriteb (cmd, this->IO_ADDR_W); }/*
The working assembly looks like this:
if (cmd != NAND_CMD_NONE) 80024d28: e3710001 cmn r1, #1 origwriteb(cmd, this->IO_ADDR_W);
80024d2c: 15933004 ldrne r3, [r3, #4]80024d30:120110ffandne r1, r1, #255 ;0xff80024d34:15c31000strbne r1, [r3]80024d38:e8bd8010pop {r4, pc}The broken assembly looks like this:
if (cmd != NAND_CMD_NONE) 80024d28: e3710001 cmn r1, #1 80024d2c: 08bd8010 popeq {r4, pc} writeb(cmd, this->IO_ADDR_W);
Quoted 5 lines
80024d30:e5933004ldr r3, [r3, #4]80024d34:e20110ffand r1, r1, #255 ;0xff80024d38:e5c31000strb r1, [r3]80024d3c:e5d33000ldrb r3, [r3]80024d40:e8bd8010pop {r4, pc}
Hmm. From functionality point of view, the 'broken' assembly below should to the same as the working assembly, above. The main difference
is the 'popeq {r4, pc}' and the additional 'ldrb r3, [r3]'. The write
to the HW 'strb r1, [r3]' is there, so it should work. Is this
understanding correct?
If it's correct, the question is, what breaks the below assembly? The popeq or the additional ldrb? The popeq looks correct, but why is the additional ldrb there?
For some further debugging, I had two ideas: Modifying the resulting binary with a hex editor and replacing the ldrb with a nop (if I remember correctly the hex code for a nop is ffffffff?). Or modifying the the C code and adding a barrier behind the writeb(). E.g.
if (cmd != NAND_CMD_NONE);
writeb(cmd, this->IO_ADDR_W);
__asm__ __volatile__ ("dsb" : : : "memory");
}Best regards
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 23 lines · Dirk Behme
Quoted 12 lines · John Rigby
The working assembly looks like this:
? ? ? ?if (cmd !=
NAND_CMD_NONE)80024d28: ? ? ?e3710001? ? ? ?cmn ? ? r1, #1
? ? ? ? ? ? ? ?origwriteb(cmd,this->IO_ADDR_W);80024d2c: ? ? ? 15933004 ? ? ? ?ldrne ? r3, [r3, #4]80024d30: ? ? ?120110ff? ? ? ?andne ? r1, r1, #255 ? ?;0xff80024d34: ? ? ?15c31000? ? ? ?strbne ?r1, [r3]80024d38: ? ? ?e8bd8010? ? ? ?pop ? ? {r4, pc}The broken assembly looks like this:
Quoted 9 lines
? ? ? ?if (cmd !=
NAND_CMD_NONE)80024d28: ? ? ?e3710001? ? ? ?cmn ? ? r1, #180024d2c: ? ? ?08bd8010? ? ? ?popeq ? {r4, pc}
? ? ? ? ? ? ? ?writeb(cmd,this->IO_ADDR_W);80024d30: ? ? ?e5933004? ? ? ?ldr ? ? r3, [r3, #4]80024d34: ? ? ?e20110ff? ? ? ?and ? ? r1, r1, #255 ? ?;0xff80024d38: ? ? ?e5c31000? ? ? ?strb ? ?r1, [r3]80024d3c: ? ? ?e5d33000? ? ? ?ldrb ? ?r3, [r3]80024d40: ? ? ?e8bd8010? ? ? ?pop ? ? {r4, pc}
Quoted 9 lines
Hmm. From functionality point of view, the 'broken' assembly below should to the same as the working assembly, above. The main difference is the 'popeq {r4, pc}' and the additional 'ldrb ? ? ?r3, [r3]'. The write to the HW 'strb ? ?r1, [r3]' is there, so it should work. Is this understanding correct?
If it's correct, the question is, what breaks the below assembly? The popeq or the additional ldrb? The popeq looks correct, but why is the additional ldrb there?
I can't answer why the ldrb is there but I'm pretty sure it is the problem. From the TRM
http://focus.ti.com/lit/ug/spruf98m/spruf98m.pdf:
Only write accesses must be issued to these locations, but the GPMC does not discard any read access. Accessing a NAND device with nOE and CLE or ALE asserted (read access) can produce undefined results.
br,
John
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 3 lines · Earlier in this thread Alexander
I haven't add the definitions which are using a memory barrier because I haven't found a place in the kernel where they were actually enabled (
CONFIG_ARM_DMA_MEM_BUFFERABLE).
I think this is the problem because it is indeed defined for all v6 and v7 arm platforms. Here is the config snippet from arch/arm/mm/Kconfig:
config ARM_DMA_MEM_BUFFERABLE
bool "Use non-cacheable memory for DMA" if CPU_V6 && !CPU_V7 depends on !(MACH_REALVIEW_PB1176 || REALVIEW_EB_ARM11MP || \
MACH_REALVIEW_PB11MP)
default y if CPU_V6 || CPU_V7
help
Historically, the kernel has used strongly ordered mappings to provide DMA coherent memory. With the advent of ARMv7, mapping memory with differing types results in unpredictable behaviour, so on these CPUs, this option is forced on.
Multiple mappings with differing attributes is also unpredictable on ARMv6 CPUs, but since they do not have aggressive speculative prefetch, no harm appears to occur.
However, drivers may be missing the necessary barriers for ARMv6, and therefore turning this on may result in unpredictable driver behaviour. Therefore, we offer this as an option.
You are recommended say 'Y' here and debug any affected drivers.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 9 lines · John Rigby
Quoted 3 lines · Earlier in this thread Alexander
I haven't add the definitions which are using a memory barrier because I haven't found a place in the kernel where they were actually enabled (
CONFIG_ARM_DMA_MEM_BUFFERABLE).I think this is the problem because it is indeed defined for all v6 and v7 arm platforms. ?Here is the config snippet from
arch/arm/mm/Kconfig:
Quoted 10 lines
config
ARM_DMA_MEM_BUFFERABLE
? ? ? ?bool "Use non-cacheable memory for DMA" ifCPU_V6&& !CPU_V7 ? ? ? ?depends on !(MACH_REALVIEW_PB1176 ||REALVIEW_EB_ARM11MP|| \
? ? ? ? ? ? ? ? ? ?MACH_REALVIEW_PB11MP)
? ? ? ?default y ifCPU_V6||CPU_V7
? ? ? ?help
? ? ? ? ?Historically, the kernel has used strongly ordered mappings to ? ? ? ? ?provide DMA coherent memory. ?With the advent of ARMv7, mapping ? ? ? ? ?memory with differing types results in unpredictable behaviour, ? ? ? ? ?so on these CPUs, this option is forced on.
On second thought maybe this is noise for us in u-boot without cacheable mappings? Sorry for the noise.
br,
John
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to John RigbyHello,
Am 20.12.2010 17:08, schrieb John Rigby:
Quoted 4 lines
Quoted 3 lines · Earlier in this thread Alexander
I haven't add the definitions which are using a memory barrier because I haven't found a place in the kernel where they were actually enabled (
CONFIG_ARM_DMA_MEM_BUFFERABLE).
Because I've just run again into such a "search problem":
Don't use "git grep" on a kernel configured for x86, when you are searching an option for another architecture. ;)
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to John RigbyHello,
Am 20.12.2010 07:07, schrieb John Rigby:
Quoted 43 lines
With your patch and the following hack nand works:
drivers/mtd/nand/omap_gpmc.c +3
-1@@ -29,6 +29,8 @@#include <linux/mtd/nand_ecc.h> #include <nand.h> +#define origwriteb (v,a) __arch_putb(v,a) +static uint8_t cs;static struct nand_ecclayout hw_nand_oob =GPMC_NAND_HW_ECC_LAYOUT ;@@ -58,7 +60,7 @@ static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd, }if (cmd !=NAND_CMD_NONE ) -writeb (cmd, this->IO_ADDR_W); +origwriteb (cmd, this->IO_ADDR_W); }/*
The working assembly looks like this:
if (cmd != NAND_CMD_NONE) 80024d28: e3710001 cmn r1, #1 origwriteb(cmd, this->IO_ADDR_W);
80024d2c: 15933004 ldrne r3, [r3, #4]80024d30:120110ffandne r1, r1, #255 ;0xff80024d34:15c31000strbne r1, [r3]80024d38:e8bd8010pop {r4, pc}The broken assembly looks like this:
if (cmd != NAND_CMD_NONE) 80024d28: e3710001 cmn r1, #1 80024d2c: 08bd8010 popeq {r4, pc} writeb(cmd, this->IO_ADDR_W);
Quoted 8 lines
80024d30:e5933004ldr r3, [r3, #4]80024d34:e20110ffand r1, r1, #255 ;0xff80024d38:e5c31000strb r1, [r3]80024d3c:e5d33000ldrb r3, [r3]80024d40:e8bd8010pop {r4, pc}This is with gcc version "(Ubuntu/Linaro 4.4.4-14ubuntu4) 4.4.5". I'll try a 4.5.2 version next and see what happens.
There must be more problems. Using gcc 4.5.1, the read*/write*-patch and your hack, my kernel doesn't boot. Using gcc 4.3.5 and the same source to compile u-boot the kernel comes up. Here is the output for the non-working u-boot:
----------------
U-Boot 2010.12-rc3-00015-g3ae9687-dirty (Dec 20 2010 - 18:01:41, gcc 4.5.1)
OMAP3530-GP ES3.1, CPU-OPP2, L3-165MHz, Max CPU Clock 720 mHz OMAP3 Beagle board + LPDDR/NAND
I2C: ready DRAM: 256 MiB NAND: 256 MiB MMC: OMAP SD/MMC: 0 In: serial Out: serial Err: serial
Beagle Rev C4
timed out in wait_for_pin: I2C_STAT=0
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: 0
reading boot.scr
422 bytes read
Running bootscript from mmc ...
## Executing script at 82000000
reading uImage
2419940 bytes read
Booting from mmc ...
## Booting kernel from Legacy Image at 82000000 ...
Image Name: Linux-2.6.37-rc5-beagleboard-000
Image Type: ARM Linux Kernel Image (uncompressed)
Data Size: 2419876 Bytes = 2.3 MiB Load Address: 80008000
Entry Point: 80008000
Verifying Checksum ... OK
Loading Kernel Image ... OK
OK
----------------
Nothing else.
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 42 lines · Alexander Holler
There must be more problems. Using gcc 4.5.1, the read*/write*-patch and your hack, my kernel doesn't boot. Using gcc 4.3.5 and the same source to compile u-boot the kernel comes up. Here is the output for the non-working u-boot:
----------------
U-Boot 2010.12-rc3-00015-g3ae9687-dirty (Dec 20 2010 - 18:01:41, gcc 4.5.1)OMAP3530-GP ES3.1, CPU-OPP2, L3-165MHz, Max CPU Clock 720 mHz OMAP3 Beagle board + LPDDR/NAND
I2C: ? ready
DRAM: ?256 MiB
NAND: ?256 MiB
MMC: ? OMAP SD/MMC: 0
In: ? ?serial
Out: ? serial
Err: ? serial
Beagle Rev C4
timed out inwait_for_pin: I2C_STAT=0
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: ?0
reading boot.scr422 bytes read
Running bootscript from mmc ...
## Executing script at 82000000
reading uImage2419940 bytes read
Booting from mmc ...
## Booting kernel from Legacy Image at 82000000 ... ? Image Name: ? Linux-2.6.37-rc5-beagleboard-000 ? Image Type: ? ARM Linux Kernel Image (uncompressed) ? Data Size: ? ?2419876 Bytes = 2.3 MiB
? Load Address: 80008000
? Entry Point: ?80008000
? Verifying Checksum ... OK
? Loading Kernel Image ... OK
OK
----------------
Quoted 6 lines
Nothing else.
Regards,
Alexander
Yes, you are correct, I see the same here with 4.5.2. I noticed that bd did not have correct values of machine type and boot params:
bd address = 0x8FF24FE0arch_number = 0xFF0084FFboot_params = 0xBB2000FE
DRAM bank = 0x00000000 -> start = 0x80000000 -> size = 0x08000000 DRAM bank = 0x00000001 -> start = 0x88000000 -> size = 0x08000000 baudrate = 115200 bps TLB addr = 0x8FFF0000 relocaddr = 0x8FF85000 reloc off = 0x0FF7D000 irq_sp = 0x8FF24F68 sp start = 0x8FF24F60 FB base = 0x00000000
If we then look at board_init in beagle.c the problem is obvious:
800331ac <board_init>:
800331ac: e92d4008 push {r3, lr}
800331b0: ebff5a74 bl 80009b88 <gpmc_init>
800331b4: e3a00000 mov r0, #0
800331b8: e5983000 ldr r3, [r8]
800331bc: e5983000 ldr r3, [r8]
800331c0: e8bd8008 pop {r3, pc}Here is with source mingled in:800331ac <board_init>:
/*
* Routine: board_init
* Description: Early hardware init.
*/
int board_init(void)
{800331ac: e92d4008 push {r3, lr}
DECLARE_GLOBAL_DATA_PTR;
gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
800331b0: ebff5a74 bl 80009b88 <gpmc_init>
gd->bd->bi_arch_number = MACH_TYPE_OMAP3_BEAGLE;
/* boot param addr */
gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100);
return 0;
}
800331b4: e3a00000 mov r0, #0
{
DECLARE_GLOBAL_DATA_PTR;
gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
/* board id for Linux */
gd->bd->bi_arch_number = MACH_TYPE_OMAP3_BEAGLE;
800331b8: e5983000 ldr r3, [r8]
/* boot param addr */
gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100);800331bc: e5983000 ldr r3, [r8]
return 0; }
800331c0: e8bd8008 pop {r3, pc}
br,
John
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 73 lines · John Rigby
Quoted 42 lines · Alexander Holler
There must be more problems. Using gcc 4.5.1, the read*/write*-patch and your hack, my kernel doesn't boot. Using gcc 4.3.5 and the same source to compile u-boot the kernel comes up. Here is the output for the non-working u-boot:
----------------
U-Boot 2010.12-rc3-00015-g3ae9687-dirty (Dec 20 2010 - 18:01:41, gcc 4.5.1)OMAP3530-GP ES3.1, CPU-OPP2, L3-165MHz, Max CPU Clock 720 mHz OMAP3 Beagle board + LPDDR/NAND
I2C: ? ready
DRAM: ?256 MiB
NAND: ?256 MiB
MMC: ? OMAP SD/MMC: 0
In: ? ?serial
Out: ? serial
Err: ? serial
Beagle Rev C4
timed out inwait_for_pin: I2C_STAT=0
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: ?0
reading boot.scr422 bytes read
Running bootscript from mmc ...
## Executing script at 82000000
reading uImage2419940 bytes read
Booting from mmc ...
## Booting kernel from Legacy Image at 82000000 ... ? Image Name: ? Linux-2.6.37-rc5-beagleboard-000 ? Image Type: ? ARM Linux Kernel Image (uncompressed) ? Data Size: ? ?2419876 Bytes = 2.3 MiB
? Load Address: 80008000
? Entry Point: ?80008000
? Verifying Checksum ... OK
? Loading Kernel Image ... OK
OK
----------------Quoted 6 lines
Nothing else.
Regards,
Alexander
Yes, you are correct, I see the same here with 4.5.2. ?I noticed that bd did not have correct values of machine type and boot params:
bd address ?=
0x8FF24FE0arch_number=0xFF0084FFboot_params=0xBB2000FE
DRAM bank ? =0x00000000
-> start ? ?=0x80000000
-> size ? ? =0x08000000
DRAM bank ? =0x00000001
-> start ? ?=0x88000000
-> size ? ? =0x08000000
baudrate ? ?= 115200 bps
TLB addr ? ?=0x8FFF0000
relocaddr ? =0x8FF85000
reloc off ? =0x0FF7D000irq_sp? ? ?=0x8FF24F68
sp start ? ?=0x8FF24F60
FB base ? ? =0x00000000If we then look at
board_initinbeagle.cthe problem is obvious:
Quoted 8 lines
800331ac<board_init>:800331ac: ? ? ?e92d4008? ? ? ?push ? ?{r3, lr}800331b0: ? ? ?ebff5a74? ? ? ?bl ? ? ?80009b88 <gpmc_init>800331b4: ? ? ?e3a00000? ? ? ?mov ? ? r0, #0800331b8: ? ? ?e5983000? ? ? ?ldr ? ? r3, [r8]800331bc: ? ? ?e5983000? ? ? ?ldr ? ? r3, [r8]800331c0: ? ? ?e8bd8008? ? ? ?pop ? ? {r3, pc}
Apparently this is a known issue mentioned in README:
NOTE: DECLARE_GLOBAL_DATA_PTR must be used with file-global scope, or current versions of GCC may "optimize" the code too much.
With this fix I can boot again:
board/ti/beagle/beagle.c +2 -2
@@ -51,6 +51,8 @@#define BEAGLE_NO_EEPROM 0xffffffff +DECLARE_GLOBAL_DATA_PTR ; + static struct { unsigned int device_vendor; unsigned char revision;
@@ -66,8 +68,6 @@ static struct {*/ int board_init (void ) { -DECLARE_GLOBAL_DATA_PTR ; -gpmc_init ();/* in SRAM or SDRAM, finish GPMC */ /* board id for Linux */ gd->bd->bi_arch_number =MACH_TYPE_OMAP3_BEAGLE ;
Please let me know if you find any other problems.
br,
John
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 114 lines · John Rigby
Quoted 73 lines · John Rigby
Quoted 42 lines · Alexander Holler
There must be more problems. Using gcc 4.5.1, the read*/write*-patch and your hack, my kernel doesn't boot. Using gcc 4.3.5 and the same source to compile u-boot the kernel comes up. Here is the output for the non-working u-boot:
----------------
U-Boot 2010.12-rc3-00015-g3ae9687-dirty (Dec 20 2010 - 18:01:41, gcc 4.5.1)OMAP3530-GP ES3.1, CPU-OPP2, L3-165MHz, Max CPU Clock 720 mHz OMAP3 Beagle board + LPDDR/NAND
I2C: ready DRAM: 256 MiB NAND: 256 MiB MMC: OMAP SD/MMC: 0 In: serial Out: serial Err: serialBeagle Rev C4
timed out inwait_for_pin: I2C_STAT=0
No EEPROM on expansion board
Die ID #062a000400000000040365fa16019019
Hit any key to stop autoboot: 0
reading boot.scr422 bytes read
Running bootscript from mmc ...
## Executing script at 82000000
reading uImage2419940 bytes read
Booting from mmc ...
## Booting kernel from Legacy Image at 82000000 ...Image Name: Linux-2.6.37-rc5-beagleboard-000 Image Type: ARM Linux Kernel Image (uncompressed) Data Size: 2419876 Bytes = 2.3 MiBLoad Address: 80008000
Entry Point: 80008000
Verifying Checksum ... OK
Loading Kernel Image ... OK
OK
----------------Quoted 6 lines
Nothing else.
Regards,
Alexander
Yes, you are correct, I see the same here with 4.5.2. I noticed that bd did not have correct values of machine type and boot params:
bd address =
0x8FF24FE0arch_number=0xFF0084FFboot_params=0xBB2000FE
DRAM bank =0x00000000-> start = 0x80000000 -> size = 0x08000000 DRAM bank = 0x00000001 -> start = 0x88000000 -> size = 0x08000000 baudrate = 115200 bps TLB addr = 0x8FFF0000 relocaddr = 0x8FF85000 reloc off = 0x0FF7D000 irq_sp = 0x8FF24F68 sp start = 0x8FF24F60 FB base = 0x00000000If we then look at
board_initinbeagle.cthe problem is obvious:Quoted 8 lines
800331ac<board_init>:
800331ac: e92d4008 push {r3, lr} 800331b0: ebff5a74 bl 80009b88<gpmc_init> 800331b4: e3a00000 mov r0, #0 800331b8: e5983000 ldr r3, [r8] 800331bc: e5983000 ldr r3, [r8] 800331c0: e8bd8008 pop {r3, pc}Apparently this is a known issue mentioned in README:
NOTE:
DECLARE_GLOBAL_DATA_PTRmust be used with file-global scope, or current versions of GCC may "optimize" the code too much.With this fix I can boot again:
board/ti/beagle/beagle.c +2
-2@@ -51,6 +51,8 @@#define BEAGLE_NO_EEPROM 0xffffffff +DECLARE_GLOBAL_DATA_PTR ; + static struct { unsigned int device_vendor; unsigned char revision;@@ -66,8 +68,6 @@ static struct {*/ int board_init (void ) { -DECLARE_GLOBAL_DATA_PTR ; -gpmc_init ();/* in SRAM or SDRAM, finish GPMC */ /* board id for Linux */ gd->bd->bi_arch_number =MACH_TYPE_OMAP3_BEAGLE ;
Quoted 1 line
Please let me know if you find any other problems.
Just to not loose the overview:
This is fixed by your patch
http://patchwork.ozlabs.org/patch/76250/
?
But the issue with drivers/mtd/nand/omap_gpmc.c (i.e. the additional
ldrb r3, [r3]) is still open? Has anybody tried to replace it with
a nop in the binary to be sure this is the root cause?
Thanks
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Hi Dirk,
Le 21/12/2010 08:11, Dirk Behme a ?crit :
Quoted 3 lines
But the issue with
drivers/mtd/nand/omap_gpmc.c(i.e. the additional
ldrb r3, [r3]) is still open? Has anybody tried to replace it with
a nop in the binary to be sure this is the root cause?
Can you try and preprocess the C file for both the broken and working cases, then post the preprocessed C extract? Differences at the C level may help understanding differences at the asm level.
Quoted 3 lines
Thanks
Dirk
Amicalement,
Signature · 1 line
Albert.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 11 lines · Albert ARIBAUD
Hi Dirk,
Le 21/12/2010 08:11, Dirk Behme a ?crit :
Quoted 3 lines
But the issue with
drivers/mtd/nand/omap_gpmc.c(i.e. the additional
ldrb r3, [r3]) is still open? Has anybody tried to replace it with
a nop in the binary to be sure this is the root cause?Can you try and preprocess the C file for both the broken and working cases, then post the preprocessed C extract? Differences at the C level may help understanding differences at the asm level.
gcc version 4.5.1 (Sourcery G++ Lite 2010.09-50)
Work:
====
static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
uint32_t ctrl)
{
register struct nand_chip *this = mtd->priv;
...
if (cmd != -1)
(*(volatile unsigned char *)(this->IO_ADDR_W) = (cmd));
}
if (cmd != NAND_CMD_NONE) 84: e3710001 cmn r1, #1 origwriteb(cmd, this->IO_ADDR_W); 88: 15933004 ldrne r3, [r3, #4] 8c: 120110ff andne r1, r1, #255 ; 0xff 90: 15c31000 strbne r1, [r3] 94: e12fff1e bx lr ...
Broken:
======
static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
uint32_t ctrl)
{
register struct nand_chip *this = mtd->priv;
...
if (cmd != -1)
({ do { } while (0); (*(volatile unsigned char *)(this->IO_ADDR_W)
= (cmd)); });
}
if (cmd != NAND_CMD_NONE) 84: e3710001 cmn r1, #1 writeb(cmd, this->IO_ADDR_W); 88: 15933004 ldrne r3, [r3, #4] 8c: 120110ff andne r1, r1, #255 ; 0xff 90: 15c31000 strbne r1, [r3] 94: 15d33000 ldrbne r3, [r3] 98: e12fff1e bx lr ...
The issue seems to be the additional 'ldrbne r3, [r3]' added by the
compiler in the broken version.
Best regards
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Dear Dirk Behme,
Quoted 3 lines · In message <[email protected]> you wrote:
Broken:
======
...
Quoted 8 lines
static void omap_nand_hwcontrol(struct
mtd_info*mtd,int32_tcmd,uint32_t ctrl){
register structnand_chip*this =mtd->priv;
...
if (cmd != -1)
({ do { } while (0); (*(volatile unsigned char *)(this->IO_ADDR_W) >
= (cmd)); });
But this is the old, discarded version of the patch.
I though we had agreed that we have to use the
__asm__ __volatile__ ("" : : : "memory")version instead?
Best regards,
Wolfgang Denk
Signature · 6 lines
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 Program maintenance is an entropy-increasing process, and even its most skilfull execution only delays the subsidence of the system into unfixable obsolescence. - Fred Brooks, "The Mythical Man Month"
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 16 lines · Wolfgang Denk
Dear Dirk Behme,
Quoted 3 lines · In message<[email protected]> you wrote:
Broken:
======...
Quoted 8 lines
static void omap_nand_hwcontrol(struct
mtd_info*mtd,int32_tcmd,uint32_t ctrl) { register struct nand_chip *this = mtd->priv; ... if (cmd != -1) ({ do { } while (0); (*(volatile unsigned char *)(this->IO_ADDR_W)>= (cmd)); });
Quoted 7 lines
But this is the old, discarded version of the patch.
I though we had agreed that we have to use the
__asm__ __volatile__ ("" : : : "memory")version instead?
Yes.
I mixed the patches :(
Sorry for the noise. I just sent the hopefully correct version some minutes ago.
Thanks
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to Albert ARIBAUD(Resend with corrected broken example)
Quoted 11 lines · Albert ARIBAUD
Hi Dirk,
Le 21/12/2010 08:11, Dirk Behme a ?crit :
Quoted 3 lines
But the issue with
drivers/mtd/nand/omap_gpmc.c(i.e. the additional
ldrb r3, [r3]) is still open? Has anybody tried to replace it with
a nop in the binary to be sure this is the root cause?Can you try and preprocess the C file for both the broken and working cases, then post the preprocessed C extract? Differences at the C level may help understanding differences at the asm level.
gcc version 4.5.1 (Sourcery G++ Lite 2010.09-50)
Work:
====
static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
uint32_t ctrl)
{
register struct nand_chip *this = mtd->priv;
...
if (cmd != -1)
(*(volatile unsigned char *)(this->IO_ADDR_W) = (cmd));
}
if (cmd != NAND_CMD_NONE) 84: e3710001 cmn r1, #1 origwriteb(cmd, this->IO_ADDR_W); 88: 15933004 ldrne r3, [r3, #4] 8c: 120110ff andne r1, r1, #255 ; 0xff 90: 15c31000 strbne r1, [r3] 94: e12fff1e bx lr ...
Broken:
======
static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
uint32_t ctrl)
{
register struct nand_chip *this = mtd->priv;
...
if (cmd != -1)
({ __asm__ __volatile__ ("" : : : "memory"); (*(volatile unsigned
char *)(this->IO_ADDR_W) = (cmd)); });
}
if (cmd != NAND_CMD_NONE) 84: e3710001 cmn r1, #1 88: 012fff1e bxeq lr writeb(cmd, this->IO_ADDR_W);
8c: e5933004 ldr r3, [r3, #4]
90: e20110ff and r1, r1, #255 ; 0xff
94: e5c31000 strb r1, [r3]
98: e5d33000 ldrb r3, [r3]
9c: e12fff1e bx lr
The issue seems to be the additional 'ldrb r3, [r3]' added by the
compiler in the broken version.
Best regards
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 67 lines · Dirk Behme
(Resend with corrected broken example)
Quoted 11 lines · Albert ARIBAUD
Hi Dirk,
Le 21/12/2010 08:11, Dirk Behme a ?crit :
Quoted 3 lines
But the issue with
drivers/mtd/nand/omap_gpmc.c(i.e. the additional ldrb ? ?r3, [r3]) is still open? Has anybody tried to replace it with a nop in the binary to be sure this is the root cause?Can you try and preprocess the C file for both the broken and working cases, then post the preprocessed C extract? Differences at the C level may help understanding differences at the asm level.
gcc version 4.5.1 (Sourcery G++ Lite 2010.09-50)
Work:
====static void
omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd, ? ? uint32_t ctrl)
{
?register structnand_chip*this =mtd->priv;
?...
?if (cmd != -1)? (*(volatile unsigned char *)(this->IO_ADDR_W) = (cmd)); }
? ? ? ?if (cmd !=
NAND_CMD_NONE)
? 84: ?e3710001 ? ? ? ?cmn ? ? r1, #1
? ? ? ? ? ? ? ?origwriteb(cmd,this->IO_ADDR_W); ? 88: ?15933004 ? ? ? ?ldrne ? r3, [r3, #4]
? 8c: ?120110ff ? ? ? ?andne ? r1, r1, #255 ? ?;0xff? 90: ?15c31000 ? ? ? ?strbne ?r1, [r3]
? 94: ?e12fff1e ? ? ? ?bx ? ? ?lr
? ? ? ?...Broken:
======static void
omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd, ? ? uint32_t ctrl)
{
?register structnand_chip*this =mtd->priv;...
?if (cmd != -1)
? ({__asm____volatile__("" : : : "memory"); (*(volatile unsigned char *)(this->IO_ADDR_W) = (cmd)); });
}? ? ? ?if (cmd !=
NAND_CMD_NONE)
? 84: ?e3710001 ? ? ? ?cmn ? ? r1, #1
? 88: ?012fff1e ? ? ? ?bxeq ? ?lr
? ? ? ? ? ? ? ?writeb(cmd,this->IO_ADDR_W); ? 8c: ?e5933004 ? ? ? ?ldr ? ? r3, [r3, #4]
? 90: ?e20110ff ? ? ? ?and ? ? r1, r1, #255 ? ?;0xff? 94: ?e5c31000 ? ? ? ?strb ? ?r1, [r3]
? 98: ?e5d33000 ? ? ? ?ldrb ? ?r3, [r3]
? 9c: ?e12fff1e ? ? ? ?bx ? ? ?lr
Quoted 3 lines
The issue seems to be the additional 'ldrb ? ? ?r3, [r3]' added by the compiler in the broken version.
And I at your suggestion tried modifying the binary changing the extra ldrb to a nop and it works.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Le 21/12/2010 09:46, John Rigby a ?crit :
Quoted 68 lines · Dirk Behme
Quoted 67 lines
(Resend with corrected broken example)
Quoted 11 lines · Albert ARIBAUD
Hi Dirk,
Le 21/12/2010 08:11, Dirk Behme a ?crit :
Quoted 3 lines
But the issue with
drivers/mtd/nand/omap_gpmc.c(i.e. the additional
ldrb r3, [r3]) is still open? Has anybody tried to replace it with
a nop in the binary to be sure this is the root cause?Can you try and preprocess the C file for both the broken and working cases, then post the preprocessed C extract? Differences at the C level may help understanding differences at the asm level.
gcc version 4.5.1 (Sourcery G++ Lite 2010.09-50)
Work:
====static void omap_nand_hwcontrol(struct
mtd_info*mtd,int32_tcmd,uint32_t ctrl){
register structnand_chip*this =mtd->priv;
...
if (cmd != -1)(*(volatile unsigned char *)(this->IO_ADDR_W) = (cmd)); }if (cmd != NAND_CMD_NONE) 84: e3710001 cmn r1, #1 origwriteb(cmd, this->IO_ADDR_W); 88: 15933004 ldrne r3, [r3, #4] 8c: 120110ff andne r1, r1, #255 ; 0xff 90: 15c31000 strbne r1, [r3] 94: e12fff1e bx lr ...Broken:
======static void omap_nand_hwcontrol(struct
mtd_info*mtd,int32_tcmd,uint32_t ctrl){
register structnand_chip*this =mtd->priv;...
if (cmd != -1)
({__asm____volatile__("" : : : "memory"); (*(volatile unsigned
char *)(this->IO_ADDR_W) = (cmd)); });
}if (cmd != NAND_CMD_NONE) 84: e3710001 cmn r1, #1 88: 012fff1e bxeq lr writeb(cmd, this->IO_ADDR_W); 8c: e5933004 ldr r3, [r3, #4] 90: e20110ff and r1, r1, #255 ; 0xff 94: e5c31000 strb r1, [r3] 98: e5d33000 ldrb r3, [r3] 9c: e12fff1e bx lr
Quoted 6 lines
Quoted 3 lines
The issue seems to be the additional 'ldrb r3, [r3]' added by the
compiler in the broken version.And I at your suggestion tried modifying the binary changing the extra ldrb to a nop and it works.
Seems like a compiler issue to me, as the preprocessed C source is the same for the register access and does not call for a re-read (that is what I wanted to see in the preprocessed version), yet the ASM sequence does the re-read.
Amicalement,
Signature · 1 line
Albert.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Dear Albert ARIBAUD,
Quoted 8 lines · In message <[email protected]> you wrote:
Quoted 3 lines
And I at your suggestion tried modifying the binary changing the extra ldrb to a nop and it works.
Seems like a compiler issue to me, as the preprocessed C source is the same for the register access and does not call for a re-read (that is what I wanted to see in the preprocessed version), yet the ASM sequence does the re-read.
I also tend to think this is a compiler problem. Searching the gcc bugzilla entries for "ldrb" turns up quite a number of hits. I'm not sure which of these we are running into here, but there are enough of them so you can chose freely :-(
Best regards,
Wolfgang Denk
Signature · 8 lines
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 Every revolutionary idea - in science, politics, art, or whatever - evokes three stages of reaction in a hearer: 1. It is completely impossible - don't waste my time. 2. It is possible, but it is not worth doing. 3. I said it was a good idea all along.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Am 21.12.2010 11:53, schrieb Wolfgang Denk:
Quoted 12 lines
Dear Albert ARIBAUD,
Quoted 8 lines · In message<[email protected]> you wrote:
Quoted 3 lines
And I at your suggestion tried modifying the binary changing the extra ldrb to a nop and it works.
Seems like a compiler issue to me, as the preprocessed C source is the same for the register access and does not call for a re-read (that is what I wanted to see in the preprocessed version), yet the ASM sequence does the re-read.
Quoted 4 lines
I also tend to think this is a compiler problem. Searching the gcc bugzilla entries for "ldrb" turns up quite a number of hits. I'm not sure which of these we are running into here, but there are enough of them so you can chose freely :-(
Hmm, is there actual somethinbg which should forbid the compiler to generate such code which rereads something? It might not be nice, but I don't think that it is forbidden for a compiler to do so. So the proper way to handle such, might be to use asm to avoid that the compiler touches that register.
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Le 21/12/2010 13:35, Alexander Holler a ?crit :
Quoted 5 lines
Hmm, is there actual somethinbg which should forbid the compiler to generate such code which rereads something? It might not be nice, but I don't think that it is forbidden for a compiler to do so. So the proper way to handle such, might be to use asm to avoid that the compiler touches that register.
Yes there is something that should prevent a compiler from inserting
reads: these accesses are to hardware, not memory, and may cause side effects even on read (these could be acknowledges, for instance; I've seen instances of that myself on some HW).
Another way to look at it is that the semantics of " *ptr = value " is a pure write and should not result in a write-then-read.
Quoted 3 lines
Regards,
Alexander
Amicalement,
Signature · 1 line
Albert.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Am 21.12.2010 13:51, schrieb Albert ARIBAUD:
Quoted 8 lines
Le 21/12/2010 13:35, Alexander Holler a ?crit :
Quoted 5 lines
Hmm, is there actual somethinbg which should forbid the compiler to generate such code which rereads something? It might not be nice, but I don't think that it is forbidden for a compiler to do so. So the proper way to handle such, might be to use asm to avoid that the compiler touches that register.
Quoted 7 lines
Yes there is something that should prevent a compiler from inserting
reads: these accesses are to hardware, not memory, and may cause side effects even on read (these could be acknowledges, for instance; I've seen instances of that myself on some HW).Another way to look at it is that the semantics of " *ptr = value " is a pure write and should not result in a write-then-read.
I think it's something like atomic_read. E.g. when reading an 32bit int (uint32_t i = *bla;), nothing forbids that the compiler generates code which reads those 4 bytes byte by byte (and so becoming a non-atomic operation). It's unusual to do so on 32bit architectures but valid.
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Le 21/12/2010 14:30, Alexander Holler a ?crit :
Quoted 14 lines
Am 21.12.2010 13:51, schrieb Albert ARIBAUD:
Quoted 13 lines
Le 21/12/2010 13:35, Alexander Holler a ?crit :
Quoted 5 lines
Hmm, is there actual somethinbg which should forbid the compiler to generate such code which rereads something? It might not be nice, but I don't think that it is forbidden for a compiler to do so. So the proper way to handle such, might be to use asm to avoid that the compiler touches that register.
Yes there is something that should prevent a compiler from inserting
reads: these accesses are to hardware, not memory, and may cause side effects even on read (these could be acknowledges, for instance; I've seen instances of that myself on some HW).
Quoted 7 lines
Quoted 2 lines
Another way to look at it is that the semantics of " *ptr = value " is a pure write and should not result in a write-then-read.
I think it's something like
atomic_read. E.g. when reading an 32bit int (uint32_ti = *bla;), nothing forbids that the compiler generates code which reads those 4 bytes byte by byte (and so becoming a non-atomic operation). It's unusual to do so on 32bit architectures but valid.
OTOH, this still respects the semantics (the compiler is allowed to do a non-atomic read) while the bug we're seeing does not repect the semantics (the compiler is not asked to do any read but does one).
Quoted 3 lines
Regards,
Alexander
Amicalement,
Signature · 1 line
Albert.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Dear Albert & friends,
what is your opinion? Should we include the memory barrier patch into the upcoming release (and eventually delay it for further testing), or release as is and solve this issue in the next release?
I tend to leave it as is, as I expect that most people will disappear in the next few days for holidays, so no much testing will be done anyway, and we then can solve this with less pressure in the next release - but I'm not really sure if this is a good idea?
Best regards,
Wolfgang Denk
Signature · 5 lines
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
A committee is a life form with six or more legs and no brain.
-- Lazarus Long, "Time Enough For Love"RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 10 lines · Wolfgang Denk
Dear Albert& friends,
what is your opinion? Should we include the memory barrier patch into the upcoming release (and eventually delay it for further testing), or release as is and solve this issue in the next release?
I tend to leave it as is, as I expect that most people will disappear in the next few days for holidays, so no much testing will be done anyway, and we then can solve this with less pressure in the next release - but I'm not really sure if this is a good idea?
I somehow tend to leave it as is, too.
We have issues with some recent compilers. For these we found a fix using the io.h somehow the same way the Linux kernel does. But this introduces new issues for us, we haven't found a proper fix yet (except changing the code to the 'old' io.h style). But we don't know where we might have this issue additionally, yet.
I would like to talk with some tool chain guys about this, too.
Could we add a readme or a note somewhere about the issues with the recent tool chains in this release?
Best regards
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Le 21/12/2010 21:04, Dirk Behme a ?crit :
Quoted 1 line
I somehow tend to leave it as is, too.
Agree.
Amicalement,
Signature · 1 line
Albert.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to Dirk BehmeAm 21.12.2010 21:04, schrieb Dirk Behme:
Quoted 12 lines · Wolfgang Denk
Quoted 10 lines
Dear Albert& friends,
what is your opinion? Should we include the memory barrier patch into the upcoming release (and eventually delay it for further testing), or release as is and solve this issue in the next release?
I tend to leave it as is, as I expect that most people will disappear in the next few days for holidays, so no much testing will be done anyway, and we then can solve this with less pressure in the next release - but I'm not really sure if this is a good idea?
Quoted 7 lines
I somehow tend to leave it as is, too.
We have issues with some recent compilers. For these we found a fix using the
io.hsomehow the same way the Linux kernel does. But this introduces new issues for us, we haven't found a proper fix yet (except changing the code to the 'old'io.hstyle). But we don't know where we might have this issue additionally, yet.
The only real problem found with that patch was one with a register which doesn't like an (unmotivated) read after write.
On the other side, without that patch, using gcc >= 4.5.x (at least on arm) proved to fail. In contrast to that problem of gcc 4.5.x ignoring that volatile, 4.5.x still fixes many bugs for arm and gcc >= 4.5.x is necessary for hardfloat. So it's likely that more people will start using 4.5.x (4.5.2 was just released).
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Le 22/12/2010 01:11, Alexander Holler a ?crit :
Quoted 15 lines
Am 21.12.2010 21:04, schrieb Dirk Behme:
Quoted 14 lines · Wolfgang Denk
Quoted 10 lines
Dear Albert& friends,
what is your opinion? Should we include the memory barrier patch into the upcoming release (and eventually delay it for further testing), or release as is and solve this issue in the next release?
I tend to leave it as is, as I expect that most people will disappear in the next few days for holidays, so no much testing will be done anyway, and we then can solve this with less pressure in the next release - but I'm not really sure if this is a good idea?
I somehow tend to leave it as is, too.
Quoted 8 lines
Quoted 5 lines
We have issues with some recent compilers. For these we found a fix using the
io.hsomehow the same way the Linux kernel does. But this introduces new issues for us, we haven't found a proper fix yet (except changing the code to the 'old'io.hstyle). But we don't know where we might have this issue additionally, yet.The only real problem found with that patch was one with a register which doesn't like an (unmotivated) read after write.
Yes, and this is enough for me to not want it right away: we caught this one, but how many others, so far unseen, will creep up?
Quoted 5 lines
On the other side, without that patch, using gcc>= 4.5.x (at least on arm) proved to fail. In contrast to that problem of gcc 4.5.x ignoring that volatile, 4.5.x still fixes many bugs for arm and gcc>= 4.5.x is necessary for hardfloat. So it's likely that more people will start using 4.5.x (4.5.2 was just released).
Do we need hard floating point in u-boot? IIRC, and unless this changed, the kernel does not want floating point, so I wonder why u-boot would.
As for getting 4.5 to work, for the next cycle people can still use pre 4.5 gccs / toolchains, so this is important but not urgent to the point of rushing decisions.
Quoted 3 lines
Regards,
Alexander
Amicalement,
Signature · 1 line
Albert.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 34 lines · Albert ARIBAUD
Le 22/12/2010 01:11, Alexander Holler a ?crit :
Quoted 15 lines
Am 21.12.2010 21:04, schrieb Dirk Behme:
Quoted 14 lines · Wolfgang Denk
Quoted 10 lines
Dear Albert& friends,
what is your opinion? Should we include the memory barrier patch into the upcoming release (and eventually delay it for further testing), or release as is and solve this issue in the next release?
I tend to leave it as is, as I expect that most people will disappear in the next few days for holidays, so no much testing will be done anyway, and we then can solve this with less pressure in the next release - but I'm not really sure if this is a good idea?
I somehow tend to leave it as is, too.
Quoted 8 lines
Quoted 5 lines
We have issues with some recent compilers. For these we found a fix using the
io.hsomehow the same way the Linux kernel does. But this introduces new issues for us, we haven't found a proper fix yet (except changing the code to the 'old'io.hstyle). But we don't know where we might have this issue additionally, yet.The only real problem found with that patch was one with a register which doesn't like an (unmotivated) read after write.
Yes, and this is enough for me to not want it right away: we caught this one, but how many others, so far unseen, will creep up?
Quoted 5 lines
On the other side, without that patch, using gcc>= 4.5.x (at least on arm) proved to fail. In contrast to that problem of gcc 4.5.x ignoring that volatile, 4.5.x still fixes many bugs for arm and gcc>= 4.5.x is necessary for hardfloat. So it's likely that more people will start using 4.5.x (4.5.2 was just released).
Quoted 6 lines
Do we need hard floating point in u-boot? IIRC, and unless this changed, the kernel does not want floating point, so I wonder why u-boot would.
As for getting 4.5 to work, for the next cycle people can still use pre 4.5 gccs / toolchains, so this is important but not urgent to the point of rushing decisions.
Agree.
Btw, I tried to send a summary of our issues to the Codesourcery mailing list:
http://www.codesourcery.com/archives/arm-gnu/msg03989.html
Let's see if we get an answer.
Best regards
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Dear Dirk Behme,
Quoted 8 lines · In message <[email protected]> you wrote:
Btw, I tried to send a summary of our issues to the Codesourcery =
mailing list:
http://www.codesourcery.com/archives/arm-gnu/msg03989.html
Let's see if we get an answer.
You got one:
http://www.codesourcery.com/archives/arm-gnu/msg03990.html
And it sounds like a reasonable explanation to me.
Best regards,
Wolfgang Denk
Signature · 5 lines
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 Cigarette, n.: A fire at one end, a fool at the other, and a bit of tobacco in between.
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 8 lines · Wolfgang Denk
Dear Dirk Behme,
Quoted 5 lines · In message<[email protected]> you wrote:
Btw, I tried to send a summary of our issues to the Codesourcery =
mailing list:
Quoted 8 lines
Quoted 3 lines
http://www.codesourcery.com/archives/arm-gnu/msg03989.html
Let's see if we get an answer.
You got one:
http://www.codesourcery.com/archives/arm-gnu/msg03990.htmlAnd it sounds like a reasonable explanation to me.
An additional answer, mainly covering the question why the volatile is optimized away:
http://www.codesourcery.com/archives/arm-gnu/msg03999.html
Best regards
Dirk
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to Albert ARIBAUDAm 22.12.2010 08:02, schrieb Albert ARIBAUD:
Quoted 28 lines
Le 22/12/2010 01:11, Alexander Holler a ?crit :
Quoted 15 lines
Am 21.12.2010 21:04, schrieb Dirk Behme:
Quoted 14 lines · Wolfgang Denk
Quoted 10 lines
Dear Albert& friends,
what is your opinion? Should we include the memory barrier patch into the upcoming release (and eventually delay it for further testing), or release as is and solve this issue in the next release?
I tend to leave it as is, as I expect that most people will disappear in the next few days for holidays, so no much testing will be done anyway, and we then can solve this with less pressure in the next release - but I'm not really sure if this is a good idea?
I somehow tend to leave it as is, too.
Quoted 8 lines
Quoted 5 lines
We have issues with some recent compilers. For these we found a fix using the
io.hsomehow the same way the Linux kernel does. But this introduces new issues for us, we haven't found a proper fix yet (except changing the code to the 'old'io.hstyle). But we don't know where we might have this issue additionally, yet.The only real problem found with that patch was one with a register which doesn't like an (unmotivated) read after write.
Yes, and this is enough for me to not want it right away: we caught this one, but how many others, so far unseen, will creep up?
Quoted 8 lines
Quoted 5 lines
On the other side, without that patch, using gcc>= 4.5.x (at least on arm) proved to fail. In contrast to that problem of gcc 4.5.x ignoring that volatile, 4.5.x still fixes many bugs for arm and gcc>= 4.5.x is necessary for hardfloat. So it's likely that more people will start using 4.5.x (4.5.2 was just released).
Do we need hard floating point in u-boot? IIRC, and unless this changed, the kernel does not want floating point, so I wonder why u-boot would.
Most people won't use U-Boot as base for the decision which compiler version to use.
Quoted 3 lines
As for getting 4.5 to work, for the next cycle people can still use pre 4.5 gccs / toolchains, so this is important but not urgent to the point of rushing decisions.
I've just written down my opinion.
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to John RigbyHello,
Am 21.12.2010 01:25, schrieb John Rigby:
Quoted 3 lines · Alexander Holler
Quoted 2 lines
There must be more problems. Using gcc 4.5.1, the read*/write*-patch and
...
Quoted 4 lines
Yes, you are correct, I see the same here with 4.5.2. I noticed that bd did not have correct values of machine type and boot params:
bd address =
0x8FF24FE0
...
Great!
Thanks a lot for searching and finding that.
I've just tested an u-boot for the BeagleBoard with your "Move DECLARE..."-patch compiled with gcc 4.5.1 and it now boots. ;)
I remember having seen those two DECLARE_GLOBAL_DATA_PTR in beagle.c too, but I was to lazy at that time to check if that has (bad) consequences. ;)
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to Alexander HollerDear Alexander Holler,
Quoted 2 lines · In message <[email protected]> you wrote:
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
...
arch/arm/include/asm/io.h extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) · line 138
Quoted 3 lines
arch/arm/include/asm/io.h +3
-0+#define writeb (v,c) ({ __iowmb(); __arch_putb(v,c); }) +#define writew (v,c) ({ __iowmb(); __arch_putw(v,c); }) +#define writel (v,c) ({ __iowmb(); __arch_putl(v,c); })
http://www.codesourcery.com/archives/arm-gnu/msg03990.html explains why this construct is causing errors in cases where an additional read from the address is unsupported.
Can you please try the following patch instead?
-------------------------------------------------------------------------
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Hello,
Am 22.12.2010 09:02, schrieb Wolfgang Denk:
Quoted 12 lines
Quoted 2 lines · In message<[email protected]> you wrote:
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
...
arch/arm/include/asm/io.h extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) · line 138
Quoted 3 lines
arch/arm/include/asm/io.h +3
-0+#define writeb (v,c) ({ __iowmb(); __arch_putb(v,c); }) +#define writew (v,c) ({ __iowmb(); __arch_putw(v,c); }) +#define writel (v,c) ({ __iowmb(); __arch_putl(v,c); })http://www.codesourcery.com/archives/arm-gnu/msg03990.html explains why this construct is causing errors in cases where an additional read from the address is unsupported.
Quoted 1 line
Can you please try the following patch instead?
Indeed. Using do {} while (0) instead of that "GCC statement-expression" fixes the problem with the read after write.
I didn't know about that "GCC statement-expression" and my usage of ({...}) was based on the writeb in the kernel-headers. But they (seem to) expand to something returning (void) which might avoid the problem.
Good that I've added the sentence that using the kernel-headers instead of that patch might be a better solution. But this might bring in much more changes, including real memory barriers. ;)
Anyway, now the master (including the GLOBAL...-patch) + patch v3 for read*/write* is good here. Just tested with both gcc 4.3.5 and gcc 4.5.1 using binutils 2.20.1.
Regards,
Alexander
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to Wolfgang DenkQuoted 5 lines · Wolfgang Denk
Dear Alexander Holler,
Quoted 2 lines · In message<[email protected]> you wrote:
gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
Quoted 8 lines
...
arch/arm/include/asm/io.h extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) · line 138
Quoted 3 lines
arch/arm/include/asm/io.h +3
-0+#define writeb (v,c) ({ __iowmb(); __arch_putb(v,c); }) +#define writew (v,c) ({ __iowmb(); __arch_putw(v,c); }) +#define writel (v,c) ({ __iowmb(); __arch_putl(v,c); })http://www.codesourcery.com/archives/arm-gnu/msg03990.html explains why this construct is causing errors in cases where an additional read from the address is unsupported.
Just for the record:
The trick is to ensure that the __arch_putx() containing the volatile is not the last statement in the GCC statement-expression. So, using something like
#define writeb(v,c) ({ __iowmb(); __arch_putb(v,c); v;})
(note the additional 'v;') should result in correct code, too.
The patches sent by Wolfgang and Alexander using
#define writeb(v,c) do { __iowmb(); __arch_putb(v,c); } while (0)
do the same with a slightly different syntax, so these patches are fine, too.
Thanks
Dirk
Quoted 72 lines
Can you please try the following patch instead?
-------------------------------------------------------------------------
Quoted 1 line
From
4672bbddaf8ce7e17a99ba737782cc527d46e5ebMon Sep 17 00:00:00 2001From: Alexander Holler<[email protected]>
Date: Sat, 18 Dec 2010 23:27:10 +0100
Subject: [PATCH] ARM: Avoid compiler optimization for readb, writeb and friends.gcc 4.5.1 seems to ignore (at least some) volatile definitions, avoid that as done in the kernel.
Reading C99 6.7.3 8 and the comment 114) there, I think it is a bug of that gcc version to ignore the volatile type qualifier used e.g. in
__arch_getl(). Anyway, using a definition as in the kernel headers avoids such optimizations when gcc 4.5.1 is used.Maybe the headers as used in the current linux-kernel should be used, but to avoid large changes, I've just added a small change to the current headers.
Signed-off-by Alexander Holler<[email protected]>Signed-off-by Wolfgang Denk<[email protected]>--- arch/arm/include/asm/io.h | 32 ++++++++++++++++++++------------ 1 files changed, 20 insertions(+), 12 deletions(-)arch/arm/include/asm/io.h +20
-12@@ -117,21 +117,29 @@ extern inline void __raw_readsl(unsigned int addr, void *data, int longlen) *buf++ =__arch_getl (addr); } -#define __raw_writeb (v,a) __arch_putb(v,a) -#define __raw_writew (v,a) __arch_putw(v,a) -#define __raw_writel (v,a) __arch_putl(v,a) +#define __raw_writeb (v,a) __arch_putb(v,a) +#define __raw_writew (v,a) __arch_putw(v,a) +#define __raw_writel (v,a) __arch_putl(v,a) -#define __raw_readb (a) __arch_getb(a) -#define __raw_readw (a) __arch_getw(a) -#define __raw_readl (a) __arch_getl(a) +#define __raw_readb (a) __arch_getb(a) +#define __raw_readw (a) __arch_getw(a) +#define __raw_readl (a) __arch_getl(a) -#define writeb (v,a) __arch_putb(v,a) -#define writew (v,a) __arch_putw(v,a) -#define writel (v,a) __arch_putl(v,a) +/* +* TODO: The kernel offers some more advanced versions of barriers, it might +* have some advantages to use them instead of the simple one here. +*/ +#define dmb () __asm__ __volatile__ ("" : : : "memory") +#define __iormb () dmb() +#define __iowmb () dmb() + +#define writeb (v,c) do { __iowmb(); __arch_putb(v,c); } while (0) +#define writew (v,c) do { __iowmb(); __arch_putw(v,c); } while (0) +#define writel (v,c) do { __iowmb(); __arch_putl(v,c); } while (0) -#define readb (a) __arch_getb(a) -#define readw (a) __arch_getw(a) -#define readl (a) __arch_getl(a) +#define readb (c) ({ u8 __v = __arch_getb(c); __iormb(); __v; }) +#define readw (c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) +#define readl (c) ({ u32 __v = __arch_getl(c); __iormb(); __v; })/* * The compiler seems to be incapable of optimising constants
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Dirk Behme:
Quoted 9 lines
Just for the record:
The trick is to ensure that the
__arch_putx()containing the volatile is not the last statement in the GCC statement-expression. So, using something like#define
writeb(v,c)({__iowmb();__arch_putb(v,c); v;})(note the additional 'v;') should result in correct code, too.
Yes, that's good. Also "0" may work, and may be more readable, (or not, according to who reads it).
Quoted 6 lines
The patches sent by Wolfgang and Alexander using
#define
writeb(v,c)do {__iowmb();__arch_putb(v,c); } while (0)do the same with a slightly different syntax, so these patches are fine, too.
It's not just different syntax, it's different semantics.
The ({...}) trick turns statements into expressions, while the "do {...} while(0)" does not. I'd better not forbid statements like
while (reg = readb(addr), reg != VALUE) { .... }or
if (readb(addr) == VALUE) { ... } or
swtich (readb(addr)) { ... }While I agree they may in general not be clean, I can forsee meaningful uses. Moreover, I'd better allow expression-looking macros to really behave like expressions -- otherwise error messages are quite hard to understand for the unaquainted.
IMHO, the only reason to use "do {} while(0)" over statemente expressions is being portable but in u-boot we are gcc-specific anyways.
/alessandro
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
Quoted 7 lines · Alessandro Rubini
Dirk Behme:
Quoted 6 lines
Just for the record:
The trick is to ensure that the
__arch_putx()containing the volatile is not the last statement in the GCC statement-expression. So, using something like
Quoted 6 lines
Quoted 3 lines
#define
writeb(v,c)({__iowmb();__arch_putb(v,c); v;})(note the additional 'v;') should result in correct code, too.
Yes, that's good. Also "0" may work, and may be more readable, (or not, according to who reads it).
Yes, indeed,
#define writeb(v,c) ({ __iowmb(); __arch_putb(v,c); 0;})
seems to work, too :)
Thanks
Dirk
Quoted 32 lines
Quoted 6 lines
The patches sent by Wolfgang and Alexander using
#define
writeb(v,c)do {__iowmb();__arch_putb(v,c); } while (0)do the same with a slightly different syntax, so these patches are fine, too.
It's not just different syntax, it's different semantics.
The ({...}) trick turns statements into expressions, while the "do {...} while(0)" does not. I'd better not forbid statements like
while (reg = readb(addr), reg != VALUE) { .... }or
if (readb(addr) == VALUE) { ... }or
swtich (readb(addr)) { ... }While I agree they may in general not be clean, I can forsee meaningful uses. Moreover, I'd better allow expression-looking macros to really behave like expressions -- otherwise error messages are quite hard to understand for the unaquainted.
IMHO, the only reason to use "do {} while(0)" over statemente expressions is being portable but in u-boot we are gcc-specific anyways.
/alessandro
RFC PATCH v2 1/1 ARM: Avoid compiler optimization for usages of readb, writeb and friends.
In reply to Alessandro RubiniDear Alessandro Rubini,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 4 lines
#define
writeb(v,c)({__iowmb();__arch_putb(v,c); v;})(note the additional 'v;') should result in correct code, too.
Yes, that's good. Also "0" may work, and may be more readable, (or not, according to who reads it).
'0' looks wrong to me, as it changes behaviour. Keep in mind that the previous implemention usually looks like this:
#define writeb(val,addr) (*(volatile unsigned char *)(addr)) = (val)
The value of this should be "val", not 0.
Quoted 6 lines
Quoted 4 lines
#define
writeb(v,c)do {__iowmb();__arch_putb(v,c); } while (0)do the same with a slightly different syntax, so these patches are fine, too.
It's not just different syntax, it's different semantics.
This is true. Thanks for pointing out.
Quoted 11 lines
The ({...}) trick turns statements into expressions, while the "do {...} while(0)" does not. I'd better not forbid statements like
while (reg = readb(addr), reg != VALUE) { .... }or
if (readb(addr) == VALUE) { ... }or
swtich (readb(addr)) { ... }
Here you are mixing things up. There is no issue with the read*() code, only the write*() code is affected.
And while no person ion a sane mind of state should use write*() in such a context, I agree that for the sake of backward compatibility we should change the code. Patch follows.
Quoted 4 lines
While I agree they may in general not be clean, I can forsee meaningful uses. Moreover, I'd better allow expression-looking macros to really behave like expressions -- otherwise error messages are quite hard to understand for the unaquainted.
Agreed. But the write*() "functions" are considered to return void, so there should not be any such issues.
Quoted 3 lines
IMHO, the only reason to use "do {} while(0)" over statemente expressions is being portable but in u-boot we are gcc-specific anyways.
This is wrong interpretation, too; nothing in this construct is GCC specific.
Best regards,
Wolfgang Denk
Signature · 5 lines
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 Documentation is the castor oil of programming. Managers know it must be good because the programmers hate it so much.