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
