Hi Wolfgang,
On Sat, Nov 3, 2012 at 5:29 AM, Wolfgang Denk <w...@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1351813330-23741-1-git-send-email-...@chromium.org> you wrote:
>> It is good to have these functions written in C instead of assembler,
>> but with -O0 the cache_disable() function doesn't return. Rather than
>> revert to assembler, this fix just forces this to be built with -O2.
>
> NAK.
>
> This is vodoo programming to fix a problem which is obviously not
> correctly understood (and fixed), so the real cause remains unfixed.
>
>> +/*
>> + * Big hack warning!
>> + *
>> + * Devs like to compile with -O0 to get a nice debugging illusion. But this
>
> We don't use -O0 normally, and actually there are more places in the
> code that are likely to cause problems or to actually break when
> using -O0.
>
>> + * function does not survive that since -O0 causes the compiler to read the
>> + * PC back from the stack after the dcache flush. Might it be possible to
>> fix
>> + * this by flushing the write buffer?
>> + */
>
> "compiler to read the PC back from the stack after the dcache flush" -
> can you please explain what exactly this means, and which exact
> problem it causes?
This is the code without the patch (armv7) using -O0:
00000248 <cache_disable>:
248: e92d4810 push {r4, fp, lr}
24c: e28db008 add fp, sp, #8
250: e24dd01c sub sp, sp, #28
254: e50b0020 str r0, [fp, #-32]
258: ee114f10 mrc 15, 0, r4, cr1, cr0, {0}
25c: e50b4014 str r4, [fp, #-20]
260: e51b3014 ldr r3, [fp, #-20]
264: e50b3010 str r3, [fp, #-16]
268: ebffff69 bl 14 <cp_delay>
26c: e51b3020 ldr r3, [fp, #-32]
270: e3530004 cmp r3, #4
274: 1a000007 bne 298 <cache_disable+0x50>
278: e51b3010 ldr r3, [fp, #-16]
27c: e2033004 and r3, r3, #4
280: e3530000 cmp r3, #0
284: 0a00000b beq 2b8 <cache_disable+0x70>
288: e51b3020 ldr r3, [fp, #-32]
28c: e3833001 orr r3, r3, #1
290: e50b3020 str r3, [fp, #-32]
294: ebfffffe bl 0 <flush_dcache_all>
298: e51b3020 ldr r3, [fp, #-32]
^^^^^^^^^^^^^
29c: e1e02003 mvn r2, r3
2a0: e51b3010 ldr r3, [fp, #-16]
2a4: e0023003 and r3, r2, r3
2a8: e50b3018 str r3, [fp, #-24]
2ac: e51b3018 ldr r3, [fp, #-24]
2b0: ee013f10 mcr 15, 0, r3, cr1, cr0, {0}
2b4: ea000000 b 2bc <cache_disable+0x74>
2b8: e1a00000 nop ; (mov r0, r0)
2bc: e24bd008 sub sp, fp, #8
2c0: e8bd8810 pop {r4, fp, pc}
this is the code with the patch:
00000124 <dcache_disable>:
124: e92d4010 push {r4, lr}
128: ebffffb4 bl 0 <cp_delay>
12c: ee114f10 mrc 15, 0, r4, cr1, cr0, {0}
130: e3140004 tst r4, #4
134: 08bd8010 popeq {r4, pc}
138: ebfffffe bl 0 <flush_dcache_all>
13c: e3c44005 bic r4, r4, #5
140: ee014f10 mcr 15, 0, r4, cr1, cr0, {0}
144: e8bd8010 pop {r4, pc}
I have marked with ^^^^^^^^^^ the line that I think it dies. I did not
spent a lot of time looking at it - just found the problem with an ICE
and then tried to fix it. I suspect it can be fixed with some sort of
dsb() in flush_dcache_all(), but I am not sure.
Unfortunately things have moved on and I can't easily test this (now
using Thumb2 where -O0 isn't quite so extreme).
>
>> +static void cache_disable(uint32_t cache_bit) __attribute__ ((optimize(2)));
>
> Sorry, I will not accept this.
OK. If I hit it again next time I will try a bit harder to root cause it.
Regards,
Simon
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
> backups: always in season, never out of style.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
