On Tue, Mar 12, 2019 at 06:08:32PM +0100, Wolfgang Denk wrote:
Dear Tom,
In message 20190312140417.GJ4690@bill-the-cat you wrote:
To answer that, no, I don't suppose there's a problem with auditing the code to make sure that we can pass in gd, rather than U-Boot proper assuming it's the first thing to touch gd, if configured.
But that to me is a different ball of wax. On this SoC, if you're at the point where you're trampling over the defined scratch space that is used for other things that need scratch space, you may have other problems too.
I think you were misled by Heiko's description. What he really ment was just that the addresses where the boot ROM stored the information about the boot device etc. gets overwriteen when the SPL
For clarity, that's not _quite_ it. The ROM passes the value in a register and we move that to scratch space, see arch/arm/mach-omap2/lowlevel_init.S and save_boot_params. This is done on every 32bit Cortex-A TI platform.
sets up his stack. This is what Heiko meant when he wrote: "On am437x the bootmode info get overwritten from SPL stack." But at that time the SPL has already loaded this information and maintains it elsewhere.
OK, but here's the problem. We define off, iirc, 1KiB of that SRAM space as not-stack but scratch space to store stuff in. The first problem you will hit, if something else touches that scratch space is what Heiko found, the boot value got blown away. But there's other stuff we do in there, more on other SoCs than others, but you're asking for trouble. To be clearer, IMHO, arch/arm/mach-omap2/u-boot-spl.lds is missing a build-time check to make sure things can't bleed into that area. You can't use that scratch space for two non-cooperating uses. If we've malloced out that area, we better not need that allocated area when hw_data_init() scribbles in there. That's my point.
I am not aware of any other problems. Of course one has to re-think where to place the GD - but this is the same problem as when using the bloblist and spl_handoff data.
I just feel it makes a lot of sense to use an existing mechanism across all the boot stages SPL -> ( TPL -> ) U-Boot before relocation -> U-Boot after relocation, instead of implementing several different hooks for the same purpose.
[And yes, it might be also time to clean up GD from a lot of mess that has accumulated there over the last decade. I doubt many of these data are really needed there. But that's another topic, IMO.]
I agree here. Fixing things up such that we can pass things we know from one stage to another in a defined manner, rather than ad-hoc manner, is good. We could even, probably, re-work most of that use of scratch space to be done in another way, or make it safe to re-use later.