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?

> 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.
> 
> 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.

-- 
Tom