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 -- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios
