On 14.07.20 12:35, Bin Meng wrote:
> Hi Wolfgang,
>
> On Tue, Jul 14, 2020 at 1:18 AM Wolfgang Denk <w...@denx.de> wrote:
>>
>> Dear Bin Meng,
>>
>> In message <1594378641-26360-1-git-send-email-bmeng...@gmail.com> you wrote:
>>>
>>> Currently get_ram_size() only works with certain RAM size like 1GiB,
>>> 2GiB, 4GiB, 8GiB, etc. Chanage the codes to work with any RAM size.
>>
>> I'm afraid I don't understand this change, Can you please explain a
>> bit more detailed what "any RAM size" means?
>
> I meant "any RAM size" that is not power of two.
>
>>
>> The existing code should work fine with any RAM size that is a power
>> of two (and the algoithm used is based on this assumption, so the
>> code would fail if it is not met).
>
> Unfortunately this limitation is not documented clearly.
>
>>
>>> - long save[BITS_PER_LONG - 1];
>>> - long save_base;
>>> - long cnt;
>>> - long val;
>>> - long size;
>>> - int i = 0;
>>> + long save[BITS_PER_LONG - 1];
>>> + long save_base;
>>> + long cnt;
>>> + long val;
>>> + long size;
>>> + int i = 0;
>>
>> Why do you change the formatting here? I can see no need for that?
>
> I see the above are the only places in this file that do not follow
> our coding practices. Since I was modifying this function, I think
> it's fine to do the updates while we are here.
>
>>
>>
>>> + long n = maxsize / sizeof(long);
>>>
>>> - for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) {
>>> + n = __ffs(n);
>>> + n = BIT(n);
>>> +
>>> + for (cnt = n >> 1; cnt > 0; cnt >>= 1) {
>>
>> I can only speculate - but do you think that this will work for a
>> memory size of - for example - 2.5 GiB as might result from combining
>> two banks of 2 GiB resp. 512 MiB ? I bet it doesn't.
>
> Correct. This issue was exposed when I was testing U-Boot with QEMU on
> RISC-V. With QEMU we can instantiate arbitrary RAM size for a given
> machine.
>
>>
>> For correct operation (as originally intended) you would always
>> specify a maxsize twice as large as the maximum possible memory size
>> of a bank of memory, and the function would return the real size it
>> found.
>
> I don't think this function can work as expected. Even if I pass a
> power-of-two RAM size to this function, it could still fail. Imagine
> the target has 2GiB memory size, and I pass 8GiB as the maxsize to
> this function. The function will hang when it tries to read/write
> something at an address that is beyond 2GiB.
>
>>
>> Any other use, especially not checking one bank of memory at a time,
>> will not work as intended. And I have yet to see systems where
>> the size of a bank of memory is not a power of two.
>>
>> So I feel what you are doing here is not an improvement, but a
>> "workaround" for some incorrect usage.
>>
>
> Given what you mentioned the function limitation, we should update the
> codes with the above power of two assumptions documented.
>
>> I don't think we should accept this patch.
>>
>
> Regards,
> Bin
>
If we want a fast algorithm to determine the last supported address even
if the start or size is not a power of two:
unsigned long n = sizeof(unsigned long) * 8 - 1;
unsigned long addr = 0;
n = 1UL << n;
for (; n; n >>= 1) {
unsigned long next = addr + n;
if (next < start || next <= start + size - 1 && test(next))
addr = next;
}
Best regards
Heinrich
