Hi Wolfgang,
On Tue, Jul 14, 2020 at 1:18 AM Wolfgang Denk wd@denx.de wrote:
Dear Bin Meng,
In message 1594378641-26360-1-git-send-email-bmeng.cn@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