On 29.11.2007 00:50, Marc Jones wrote:
Carl-Daniel Hailfinger wrote:
On 28.11.2007 23:52, Marc Jones wrote:
Carl-Daniel Hailfinger wrote:
Marc? This has been sitting in my tree for a while now.
On 16.11.2007 16:00, Carl-Daniel Hailfinger wrote:
Hi,
v2 and v3 have almost identical CAR setup code with identical bugs for CAR sizes != {4k,8k,16k}. In v3, the erroneous code paths are not triggered and the bug is latent, but we have at a few boards in v2 which trigger these bugs, resulting in holes and/or smaller size of the CAR area.
Sorry, I have been very busy and I have been putting this off.
I think you are correct that CAR expects power of 2 cache sizes. How about just error if the size isn't power of 2 between 4K and 64K? If you wanted to support non power of 2 you should round up otherwise you might write off the end.
OK, will prepare an updated patch.
What about the bugs which cause 32k CAR to end up as 16k and 64k CAR to have a hole between 16k and 32k?
I am not very familiar with the code but it looks like the size is growing from 0xCFFFF down to 0xC0000. I don't see a gap. The movl %eax, %edx make the entire MSR 0x0606060606060606 which would be 32K and then setting that in both 0x269 and 0x268 would be 64K. But I could be misunderstanding the code.
Quoting from my original mail:
[...]
clear_fixed_var_mtrr_out:
#if CacheSize == 0x10000 /* enable caching for 64K using fixed mtrr */ movl $0x268, %ecx /* fix4k_c0000*/ movl $0x06060606, %eax /* WB IO type */ movl %eax, %edx wrmsr movl $0x269, %ecx wrmsr #endif
OK, 64k are working.
#if CacheSize == 0x8000 /* enable caching for 32K using fixed mtrr */ movl $0x269, %ecx /* fix4k_c8000*/ movl $0x06060606, %eax /* WB IO type */ movl %eax, %edx wrmsr #endif
OK, 32k are working.
Everything is set up correctly until now.
/* enable caching for 16K/8K/4K using fixed mtrr */ movl $0x269, %ecx /* fix4k_cc000*/#if CacheSize == 0x4000 movl $0x06060606, %edx /* WB IO type */ #endif #if CacheSize == 0x2000 movl $0x06060000, %edx /* WB IO type */ #endif #if CacheSize == 0x1000 movl $0x06000000, %edx /* WB IO type */ #endif xorl %eax, %eax wrmsr
This is where the bug is. I'm speaking of the two commands above, executed unconditionally. %ecx is 0x269, %eax is zeroed, %edx keeps its value ($0x06060606 in case of CacheSize>=32k). wrmsr is issued. Is there any reason to assume that this will not disable CAR again between 16k and 32k? And no, that code is not protected by an #ifdef.
Disable CAR between 16k and 32k unconditionally. Even if CacheSize is 32k or 64k. Not nice.
Regards, Carl-Daniel