On Sat, Jan 07, 2023 at 06:38:58PM +0100, Pali Rohár wrote:
On Saturday 07 January 2023 12:32:12 Tom Rini wrote:
On Sat, Jan 07, 2023 at 05:26:45PM +0100, Pali Rohár wrote:
On Friday 06 January 2023 17:51:56 Tom Rini wrote:
On Fri, Jan 06, 2023 at 11:09:30PM +0100, Pali Rohár wrote:
On Friday 06 January 2023 16:45:41 Tom Rini wrote:
On Fri, Jan 06, 2023 at 04:14:08PM -0500, Tom Rini wrote: > On Fri, Jan 06, 2023 at 09:22:56PM +0100, Pali Rohár wrote: > > On Friday 06 January 2023 12:25:24 Tom Rini wrote: > > > On Fri, Jan 06, 2023 at 05:45:43PM +0100, Pali Rohár wrote: > > > > On Friday 06 January 2023 10:51:43 Tom Rini wrote: > > > > > This reverts commit 777aaaa706bcfe08c284aed06886db7d482af3f8. > > > > > > > > > > The changes to this generic function, which is intended to help with > > > > > 32bit platforms with large amounts of memory has unintended side effects > > > > > (which in turn lead to boot failures) on other platforms which were > > > > > previously functional. > > > > > > > > As mentioned previously, unfortunately this revert breaks 32-bit u-boot > > > > on 36-bit mpc85xx boards with 32-bit e500v2 cores and 4GB DDR module. > > > > > > > > Which platforms currently have broken u-boot without this revert? The > > > > only one which was reported is stm32mp but for it there different > > > > workaround patch waiting in the queue. > > > > > > Are you able to test on one of these PowerPC platforms currently? As > > > the stm32 problem shows, not everything is getting tested frequently > > > enough, so how many other cases are lurking out there. And, I think > > > overall issue is that the overflow check-and-change you introduce here > > > should just be in the CONFIG_MAX_MEM_MAPPED==true case. As that's the > > > case you're dealing with, yes? > > > > I was planning to do big retest again after all powerpc patches are > > reviewed and merged... > > Yes, but can you test one of them now, to see if my idea works?
Ok, I will try to look at during the weekend.
OK, good, thanks.
> > Anyway, if the issue here is with ram_size and its reduction was needed > > for mpc85xx (at the time of introduction of that patch), what about > > putting mpc85xx ifdef around ram_size reduction? For arm boards it would > > have same behavior as revert of that commit and for mpc85xx it would be > > no change.
This is what I mean:
#ifdef CONFIG_MPC85xx if (gd->ram_base + ram_size < gd->ram_base) ram_size = ...; #endif
> > I agree that this code needs to be revisited, together with ram_top > > issue and also code which fills DDR banks. Because really mapped memory > > for u-boot and real size of DDR are two different things here. > > The issue here is that we see two now (given Fabio's reminder about > another thread I had forgotten) of unintended consequences, on 32bit > platforms trying to normally have 2GB of memory, which does not require > special treatment.
Running git grep get_effective_memsize and git grep 'gd->ram_top' shows the root of this issue: Different platforms, boards and common code use these things differently. This needs to be "fixed" = unified in whole codebase. We need a function which returns mappable memory for u-boot (intptr_t type is enough) and another function (or structure or whatever) which says total size of RAM as u64 type (to ensure that it would work also for 4GB SODIMM modules on pure 32-bit platforms). And then each place in u-boot code has to be modified to use the correct function.
Second issue is then gd->ram_top. Either say 4GB for 32-bit ram_top type is not supported or say that zero value is special and represents 4GB. And also every place in u-boot code needs to be adjusted by this decision / code.
Fixing both issues make easily break lot of boards (if done improperly) as it touches whole u-boot code base. So not easy task.
What I'm leaning towards right now even, is that since it's hard to test the non-36bit platforms that do set CONFIG_MAX_MEM_MAPPED, to see if their behavior also changed here, the 36bit platforms should just be overriding get_effective_memsize.
There are (at least) 3 situation:
- if RAM is mapped to the end of physical address space (possibly just small e.g. 1GB)
- if platform is >32-bit but running in 32-bit mode (so physical address is u64 because we do not have e.g. int36_t; but void* is 32-bit)
- if RAM is exactly 4GB and u-boot is 32-bit
And every one has different edge cases and cause different problems. Now, as I pointed above that every platform / board is using get_effective_memsize() differently, plus we have CONFIG_MAX_MEM_MAPPED option, it means that number of test matrix is really huge.
It looks like that ARM issues are caused by the fact that RAM is mapped to the end of the physical address space (so it does not matter how big or small it is). And powerpc issue is 4GB of RAM together with running in 32-bit mode.
Anyway, has U-Boot support for 32-bit x86 CPUs with PAE support? If yes, then I bet that there can be other edge cases when e.g. 8GB of DDR is connected.
Yes, it's very much a mess, so for this release I'd like to return to either:
- Status quo of v2022.10 (revert this patch)
- Change only CONFIG_MAX_MEM_MAPPED being set behavior (should keep PowerPC 36bit working, may have unexpected impact on other platforms, still, but very few at least).
Yea, it is a mess. I'm looking at the CONFIG_MAX_MEM_MAPPED again and it is for different situation. CONFIG_MAX_MEM_MAPPED says maximal mapped memory. For mpc85xx it is by default set to 2GB for a very long time. And if base physical address od the RAM is at 2GB or higher then it also hits this 4GB limit. So CONFIG_MAX_MEM_MAPPED does not help there...
Right, it's a mess. So, to try and end up with the least number of broken platforms for the coming release, would you rather a full revert, or just moving your changes under CONFIG_MAX_MEM_MAPPED being set, which I believe you're saying is the case for 36bit PowerPC ?
CONFIG_MAX_MEM_MAPPED is by default set for all powerpc boards (see arch/powerpc/include/asm/config.h) and also for some ARM plat.
As I suggested above, rather move ram_size modification under CONFIG_MPC85xx which is set only for powerpc mpc85xx platform which I tested.
CONFIG_MAX_MEM_MAPPED option is not too strict as CONFIG_MPC85xx.
Right, so which option for the release on Monday do you prefer at this point? We should indeed sort this mess out, for v2023.04.