Hi Wolfgang,

On Fri, 15 Mar 2019 at 02:34, Wolfgang Denk wd@denx.de wrote:

Dear Simon,

In message CAPnjgZ14Sq=OWRbM6hpqO2CLP=1eg=ftQw_1Bqu0DNe+6OxTkQ@mail.gmail.com you wrote:

...

I think it is a reasonable idea to allow the gd region to pass from TPL -> SPL -> U-Boot. But we'll need to remove use of CONFIG_IS_ENABLED(), or put shared things at the beginning of the structure.

Indeed. And/or split things up in "common" stuff and optional / config dependent things.

...

We need the concept of 'am I the first thing to run'. This is implemented in bloblist as u_boot_first_phase() - see spl.h. If this is true, we must set up the data structure. If false we must find one set up by a previous phase and use it. Bloblist handles this, but perhaps gd could as well?

I wonder why we need 4 different ways of doing basically the same thing.

First, we have GD, which exists since the dawn of U-Boot, which was intended to pass data between boot stages (by then, before and after relocation), but apparently it has never been used for passing information between SPL and U-Boot proper.

That's right. It is always zeroed these days at the start of each phase. So I think work is needed if we want to use this.

Also I don't think we can assume that gd stays in the same place through all phases of U-Boot. Probably we can keep it in the same place in TPL, SPL and U-Boot pre-relocation, then move it to SDRAM during relocation.

Then you added the bloblist thingy. It's not really clear what it's intentions are - I see the commits, but I can't find what you want to use it for or what design you have in mind. It's too complicated for passing just a few data, but apparently you find it necessary to make it secure enough that you add version, magic and checksum (which makes it necessary to have CRC32 in SPL...). Also, I wonder how the search mechanism effects boot time...

This is designed for things that need to past structs between phases. For example, verified boot may start processing in TPL and then pass information to SPL and then to U-Boot proper. This may involve quite a bit of info, so it is in a C structure. It isn't really suitable to put this entire structure in gd IMO, since that struct is pretty small.

See doc/README.bloblist:

A bloblist provides a way to store collections of binary information (blobs) in a central structure. Each record of information is assigned a tag so that its owner can find it and update it. Each record is generally described by a C structure defined by the code that owns it.

Maybe this should have more detail?

The version, magic and checksum are to ensure that the data is not corrupted by mistake, which as you know can happen very easily with fixed-position data structures. The search is pretty quick once the checksum is done, just running through a few pointers. I suppose we could make the checksum optional.

An then there is commit b0edea3c27 with the spl_handoff thing. I can't decide whether this is intended as a general feature or a separate, SPL specific mechanism. And more questions - if we pass the handoff pointer in GD, why all the effort - why don't we just make sure GD is passed properly? The fact that there is no use case at all in mainline U-Boot makes it really hard to understand your intentions.

That actually uses the bloblist, putting an SPL struct in there, intended to hold things like the SDRAM config or boot options discovered in SPL. At present we have a few ad-hoc ways of

And finally there is bootstage with it's own mechanism of information passing.

Yes, and this is ad-hoc too. I would like to move it to bloblist.

Can we not unify these, and use one common method, please?

I think we might end up with gd (once this work is done) and bloblist. For now I feel that bloblist has a purpose but let's see how it goes with the gd work.

...

Also consider the scenario where there is a read-only TPL programmed in manufacture that never changes, and a read-write SPL + U-Boot that can be upgraded in the field. In this case they may eventually end up being built with different versions of U-Boot. The bloblist structure is intended to handle this by at least checking that the size matches.

You also have the version field there, right? Who not (also) checking this?

Yes it should check this, at least once we actually have a version 1 :-)

...

Related, I feel that we should figure out how to use registers to pass addresses from SPL to U-Boot. On ARM we could use r0 to pass the value of gd, perhaps.

There is no need to. GD already has a well-defined register which has been reserved exclusively for this use - on ARM, it's R9.

Yes, but I'm not sure this is portable. E.g. on x86 this can't be used in 64-bit mode, so passing info from 32-bit SPL to 64-bit U-Boot is not possible (I believe). Using a normal register might be better, but in any case this is going to be arch-specific.

In any case, I was not even aware that gd was intended to pass info from SPL to U-Boot. It is possible that in my refactoring of board_init() some years ago I actually broken this. Once we get it running again we should add some tests and docs.

Regards, Simon

Hi Wolfgang,

On Sun, 17 Mar 2019 at 20:01, Wolfgang Denk wd@denx.de wrote:

Dear Simon,

In message CAPnjgZ05B7qB0JJn6=cStyW6tj_i5jBZQwEAxTDF-fMp8NHeOQ@mail.gmail.com you wrote:

...

...

First, we have GD, which exists since the dawn of U-Boot, which was intended to pass data between boot stages (by then, before and after relocation), but apparently it has never been used for passing information between SPL and U-Boot proper.

That's right. It is always zeroed these days at the start of each phase. So I think work is needed if we want to use this.

Yes, of course. I just want to make sure we have agreement in which direction we should move.

Yes that's the main thing.

...

Also I don't think we can assume that gd stays in the same place through all phases of U-Boot. Probably we can keep it in the same place in TPL, SPL and U-Boot pre-relocation, then move it to SDRAM during relocation.

Correct - it has never been a requirement that GD remains in place - just that the pointer that is being passed points to valid data. The currentimplementation already allows to move the GD from some restricted resource (like on-chip-RAM or cache) to SDRAM as soon as it becomes available.

OK, make sense. We should make sure it is documented when this work is done.

...

...

Then you added the bloblist thingy. It's not really clear what it's

...

...

This is designed for things that need to past structs between phases. For example, verified boot may start processing in TPL and then pass information to SPL and then to U-Boot proper. This may involve quite a bit of info, so it is in a C structure. It isn't really suitable to put this entire structure in gd IMO, since that struct is pretty small.

So what is your recommendation? Shall we use GD and bloblists side by side? Shall the GD just contain the lob list pointer?

I think that makes sense but I suspect things will become clearer over time. GD already has a bloblist pointer, but it is reinited in every phase. We can fix that once the GD pass-through is implemented.

Or shall we move all things have been thrown (without much thought, as it seems) into GD to smaller structs and convert to blobs, so the GD pointer actually becomes a pointer to the blob list (which would increase code and executin time, I'm afraid).

What is your recommendation?

A scan of GD suggests that everything there is pretty small, generally a ulong or a pointer. I suppose if we had rarely access stuff we could package it up and put it in a bloblist, but I don't see a lot of benefit to that. As you say, there is a cost. So I'd suggest leaving GD as it is for now.

...

The version, magic and checksum are to ensure that the data is not corrupted by mistake, which as you know can happen very easily with fixed-position data structures. The search is pretty quick once the checksum is done, just running through a few pointers. I suppose we could make the checksum optional.

But still it is another increase in code size - as is, for accessing a field in GD we just dereference a pointer which is already in a register; for a blob list, we need to call (at least) a function...

Yes GD is much more efficient.

One thing to note with bloblist is that changing anything in it makes it invalid for the next phase, until the checksum is recalculated (which happens at the end of each phase).

...

...

An then there is commit b0edea3c27 with the spl_handoff thing. I

...

...

That actually uses the bloblist, putting an SPL struct in there, intended to hold things like the SDRAM config or boot options discovered in SPL. At present we have a few ad-hoc ways of

Again: this is exactly what GD was intended for - now we have more than one implementation and should decide what to do...

Right but here I think we have a use case. For example, if exynos wants to store its RAM config from SPL and pass it to U-Boot, that might be 100 bytes. Also it means putting arch-specific stuff in GD. Perhaps bloblist is a better place for that.

...

...

And finally there is bootstage with it's own mechanism of information passing.

Yes, and this is ad-hoc too. I would like to move it to bloblist.

So bloblist would be the way to go for GD, too?

No, I mean that bootstage data should go in a bloblist. I don't think we should get rid of GD.

...

...

Can we not unify these, and use one common method, please?

I think we might end up with gd (once this work is done) and bloblist. For now I feel that bloblist has a purpose but let's see how it goes with the gd work.

Why do we need GD _and_ bloblist? I would like to have one solution only.

Well, my thinking is that GD is actually a set of pointers, with the actual data stored elsewhere. The nice thing about bloblist is that it is a contiguous block of a set size, which holds data used by different parts of U-Boot (e.g. could be drivers, arch-specific code, vboot, features like android boot, EFI, etc.). Some of the pointers in GD could move to bloblist, like bootstage as already discussed.

But if you want one solution, then it has to be GD I think. Perhaps we can defer this until the GD work is done and we have a few more users?

Regards, Simon

On Tue, Mar 12, 2019 at 12:37:20PM +0100, Heiko Schocher wrote:

Hello Tom,

Am 12.03.2019 um 12:16 schrieb Tom Rini:

...

On Tue, Mar 12, 2019 at 09:21:36AM +0100, Heiko Schocher wrote:

...

Hello Simon, Tom,

I am just stumbeld on an am437x basd board over the problem to pass the bootmode from SPL to U-Boot. On am437x the bootmode info get overwritten from SPL stack, and I need this info in U-Boot.

Hack would be to move SPL stack to another address, but we loose than 0xa000 size for stack ... I do not want to go this way..

I think you need to fix it being over-written as it sounds like we're running over our pre-defined scratch space area and that can lead to other problems as well, depending on how much your platform is deviating from how the TI ref platforms are written. Thanks!

Yes, doable ... but the question is more, do we really want to read such infos twice, instead reading them in SPL and passing them from SPL to U-Boot ?

Probably so, yes. Since we're talking about SPL and a "lots of features, medium sized SRAM" SoC where we've had to carefully tweak configs before to get all desired features within the limits. If you've bled into SRAM_SCRATCH_SPACE_ADDR range you may run into other problems too.

On Tue, Mar 12, 2019 at 02:42:02PM +0100, Wolfgang Denk wrote:

Dear Tom,

In message 20190312121957.GI4690@bill-the-cat you wrote:

...

...

Yes, doable ... but the question is more, do we really want to read such infos twice, instead reading them in SPL and passing them from SPL to U-Boot ?

Probably so, yes. Since we're talking about SPL and a "lots of features, medium sized SRAM" SoC where we've had to carefully tweak configs before to get all desired features within the limits. If you've bled into SRAM_SCRATCH_SPACE_ADDR range you may run into other problems too.

I think you misunderstand.

Heiko is not asking for any new data, We are already using the struct global_data in SPL. And actually this is also where Simon added the pointers for the bloblist and the spl_handoff data.

The question is - why do we actually _need_ this spl_handoff, why don;t we just make sure the global_data is passed from SPL (to TPL and from there) to U-oot proper?

We already have reserved this memory, we already have all needed information, so why not just passing this on instead of throwing it away?

this will not grow data size, and it will not require any additional code size compared to the spl_handoff stuff, which then would be no longer needed (if I understnad the code correctly).

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 would swear the EEPROM reading stuff makes use of that but I suspect your platform doesn't bother with that path at all. The other thing that may bite you is the stuff around hw_data.

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.

Hello Wolfgang,

Am 12.03.2019 um 18:46 schrieb Wolfgang Denk:

Dear Tom,

In message 20190312173125.GP4690@bill-the-cat you wrote:

...

...

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.

...

...

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.

I see. Well, I think it's best if Heiko explains in detail; what he has observed, and what when which part of the information got lost. I was just interpreting his mail, so I may easily have misunderstood this.

@ Heiko: Can you please elucidate?

arch/arm/include/asm/arch-am33xx/omap.h

19 #ifdef CONFIG_AM33XX 20 #define NON_SECURE_SRAM_START 0x402F0400 21 #define NON_SECURE_SRAM_END 0x40310000 22 #define NON_SECURE_SRAM_IMG_END 0x4030B800 23 #elif defined(CONFIG_TI816X) || defined(CONFIG_TI814X) 24 #define NON_SECURE_SRAM_START 0x40300000 25 #define NON_SECURE_SRAM_END 0x40320000 26 #define NON_SECURE_SRAM_IMG_END 0x4031B800 27 #elif defined(CONFIG_AM43XX) 28 #define NON_SECURE_SRAM_START 0x402F0400 29 #define NON_SECURE_SRAM_END 0x40340000 30 #define NON_SECURE_SRAM_IMG_END 0x40337DE0 31 #define QSPI_BASE 0x47900000 32 #endif 33 #define SRAM_SCRATCH_SPACE_ADDR (NON_SECURE_SRAM_IMG_END - SZ_1K) 34

and with ./include/configs/ti_armv7_common.h

69 #ifndef CONFIG_SYS_INIT_SP_ADDR 70 #define CONFIG_SYS_INIT_SP_ADDR (NON_SECURE_SRAM_END - \ 71 GENERATED_GBL_DATA_SIZE) 72 #endif 73

include/generated/generic-asm-offsets.h

9 #define GENERATED_GBL_DATA_SIZE 224 /* (sizeof(struct global_data) + 15) & ~15 @ */ 10 #define GENERATED_BD_INFO_SIZE 112 /* (sizeof(struct bd_info) + 15) & ~15 @ */ 11 #define GD_SIZE 224 /* sizeof(struct global_data) @ */

-> CONFIG_SYS_INIT_SP_ADDR = 0x40340000 - 0xe0 = 0x4033ff20 -> SRAM_SCRATCH_SPACE_ADDR = 0x40337DE0 - 0x400 = 0x403379e0

./arch/arm/include/asm/omap_common.h: 816 #define OMAP_SRAM_SCRATCH_BOOT_PARAMS (SRAM_SCRATCH_SPACE_ADDR + 0x24)

OMAP_SRAM_SCRATCH_BOOT_PARAMS = 0x40337a04

So stack is on a higher address than the scratch space. Stack addresses decrement on usage, so may they overwrite scratch space, as SPL functionality grows more and more ...

Hmm... I see, the NON_SECURE_SRAM_IMG_END is fix defined, and we cannot move it.

Ok, looking on my own now on the hardware:

=> md 40337a04 40337a04: 40338dc4 8f943b1e 8138beca 4ea1b2c2 ..3@.;....8.... ^ pointer to bootparms struct

=> md.b 40338dc4 40338dc4: ff ff ff ff 08 8f 33 40 07 01 00 00 00 00 00 00 ......3@........ ^^ bootmode 0x07 -> mmc0

Nothing overwritten here! Puuh...

But I have a bad feeling reading the bootmode again from U-Boot, as the boot_params info is may in space, where stack can overwrite it ...

...

I agree here. Fixing things up such that we can pass things we know =66rom 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.

Thanks a lot! Let's go for it.

To be safe here, we should really pass the bootmode (or more common, the infos collected already in GD) from SPL to U-Boot ...

bye, Heiko

Hello Stefano,

Am 13.03.2019 um 09:51 schrieb Stefano Babic:

Hi Heiko,

On 13/03/19 08:44, Heiko Schocher wrote:

...

Hello Wolfgang,

Am 12.03.2019 um 18:46 schrieb Wolfgang Denk:

...

Dear Tom,

In message 20190312173125.GP4690@bill-the-cat you wrote:

...

...

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.

...

...

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.

I see.  Well, I think it's best if Heiko explains in detail; what he has observed, and what when which part of the information got lost. I was just interpreting his mail, so I may easily have misunderstood this.

@ Heiko:  Can you please elucidate?

arch/arm/include/asm/arch-am33xx/omap.h

19 #ifdef CONFIG_AM33XX  20 #define NON_SECURE_SRAM_START   0x402F0400  21 #define NON_SECURE_SRAM_END     0x40310000  22 #define NON_SECURE_SRAM_IMG_END 0x4030B800  23 #elif defined(CONFIG_TI816X) || defined(CONFIG_TI814X)  24 #define NON_SECURE_SRAM_START   0x40300000  25 #define NON_SECURE_SRAM_END     0x40320000  26 #define NON_SECURE_SRAM_IMG_END 0x4031B800  27 #elif defined(CONFIG_AM43XX)  28 #define NON_SECURE_SRAM_START   0x402F0400  29 #define NON_SECURE_SRAM_END     0x40340000  30 #define NON_SECURE_SRAM_IMG_END 0x40337DE0  31 #define QSPI_BASE              0x47900000  32 #endif  33 #define SRAM_SCRATCH_SPACE_ADDR (NON_SECURE_SRAM_IMG_END - SZ_1K)  34

and with ./include/configs/ti_armv7_common.h

69 #ifndef CONFIG_SYS_INIT_SP_ADDR  70 #define CONFIG_SYS_INIT_SP_ADDR         (NON_SECURE_SRAM_END - \  71 GENERATED_GBL_DATA_SIZE)  72 #endif  73

include/generated/generic-asm-offsets.h

9 #define GENERATED_GBL_DATA_SIZE 224 /* (sizeof(struct global_data) + 15) & ~15  @ */  10 #define GENERATED_BD_INFO_SIZE 112 /* (sizeof(struct bd_info) + 15) & ~15       @ */  11 #define GD_SIZE 224 /* sizeof(struct global_data)       @ */

-> CONFIG_SYS_INIT_SP_ADDR = 0x40340000 - 0xe0  = 0x4033ff20 -> SRAM_SCRATCH_SPACE_ADDR = 0x40337DE0 - 0x400 = 0x403379e0

./arch/arm/include/asm/omap_common.h: 816 #define OMAP_SRAM_SCRATCH_BOOT_PARAMS   (SRAM_SCRATCH_SPACE_ADDR + 0x24)

OMAP_SRAM_SCRATCH_BOOT_PARAMS = 0x40337a04

So stack is on a higher address than the scratch space. Stack addresses decrement on usage, so may they overwrite scratch space, as SPL functionality grows more and more ...

What about to move this area after the initial SP ? This is the same way we do with GENERATED_GBL_DATA_SIZE to avoid to be overwritten.

If I am not wrong, the value of SRAM_SCRATCH_SPACE_ADDR is not changeable as it comes from the ROM bootloader.

bye, Heiko

Stefano

...

Hmm... I see, the NON_SECURE_SRAM_IMG_END is fix defined, and we cannot move it.

Ok, looking on my own now on the hardware:

=> md 40337a04 40337a04: 40338dc4 8f943b1e 8138beca 4ea1b2c2    ..3@.;....8....           ^           pointer to bootparms struct

=> md.b 40338dc4 40338dc4: ff ff ff ff 08 8f 33 40 07 01 00 00 00 00 00 00 ......3@........                                   ^^                                   bootmode 0x07 -> mmc0

Nothing overwritten here! Puuh...

But I have a bad feeling reading the bootmode again from U-Boot, as the boot_params info is may in space, where stack can overwrite it ...

...

...

I agree here.  Fixing things up such that we can pass things we know =66rom 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.

Thanks a lot!  Let's go for it.

To be safe here, we should really pass the bootmode (or more common, the infos collected already in GD) from SPL to U-Boot ...

bye, Heiko

Hello Tom,

Am 13.03.2019 um 19:19 schrieb Tom Rini:

On Wed, Mar 13, 2019 at 10:18:06AM +0100, Heiko Schocher wrote:

...

Hello Stefano,

Am 13.03.2019 um 09:51 schrieb Stefano Babic:

...

Hi Heiko,

On 13/03/19 08:44, Heiko Schocher wrote:

...

Hello Wolfgang,

Am 12.03.2019 um 18:46 schrieb Wolfgang Denk:

...

Dear Tom,

In message 20190312173125.GP4690@bill-the-cat you wrote:

...

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

...

...

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.

I see.  Well, I think it's best if Heiko explains in detail; what he has observed, and what when which part of the information got lost. I was just interpreting his mail, so I may easily have misunderstood this.

@ Heiko:  Can you please elucidate?

arch/arm/include/asm/arch-am33xx/omap.h

19 #ifdef CONFIG_AM33XX  20 #define NON_SECURE_SRAM_START   0x402F0400  21 #define NON_SECURE_SRAM_END     0x40310000  22 #define NON_SECURE_SRAM_IMG_END 0x4030B800  23 #elif defined(CONFIG_TI816X) || defined(CONFIG_TI814X)  24 #define NON_SECURE_SRAM_START   0x40300000  25 #define NON_SECURE_SRAM_END     0x40320000  26 #define NON_SECURE_SRAM_IMG_END 0x4031B800  27 #elif defined(CONFIG_AM43XX)  28 #define NON_SECURE_SRAM_START   0x402F0400  29 #define NON_SECURE_SRAM_END     0x40340000  30 #define NON_SECURE_SRAM_IMG_END 0x40337DE0  31 #define QSPI_BASE              0x47900000  32 #endif  33 #define SRAM_SCRATCH_SPACE_ADDR (NON_SECURE_SRAM_IMG_END - SZ_1K)  34

and with ./include/configs/ti_armv7_common.h

69 #ifndef CONFIG_SYS_INIT_SP_ADDR  70 #define CONFIG_SYS_INIT_SP_ADDR         (NON_SECURE_SRAM_END - \  71 GENERATED_GBL_DATA_SIZE)  72 #endif  73

include/generated/generic-asm-offsets.h

9 #define GENERATED_GBL_DATA_SIZE 224 /* (sizeof(struct global_data) + 15) & ~15  @ */  10 #define GENERATED_BD_INFO_SIZE 112 /* (sizeof(struct bd_info) + 15) & ~15       @ */  11 #define GD_SIZE 224 /* sizeof(struct global_data)       @ */

-> CONFIG_SYS_INIT_SP_ADDR = 0x40340000 - 0xe0  = 0x4033ff20 -> SRAM_SCRATCH_SPACE_ADDR = 0x40337DE0 - 0x400 = 0x403379e0

./arch/arm/include/asm/omap_common.h: 816 #define OMAP_SRAM_SCRATCH_BOOT_PARAMS   (SRAM_SCRATCH_SPACE_ADDR + 0x24)

OMAP_SRAM_SCRATCH_BOOT_PARAMS = 0x40337a04

So stack is on a higher address than the scratch space. Stack addresses decrement on usage, so may they overwrite scratch space, as SPL functionality grows more and more ...

What about to move this area after the initial SP ? This is the same way we do with GENERATED_GBL_DATA_SIZE to avoid to be overwritten.

If I am not wrong, the value of SRAM_SCRATCH_SPACE_ADDR is not changeable as it comes from the ROM bootloader.

No, we define the scratch space, but there's important restrictions.

Ah, with ./arch/arm/mach-omap2/lowlevel_init.S

20 #ifdef CONFIG_SPL 21 ENTRY(save_boot_params) 22 ldr r1, =OMAP_SRAM_SCRATCH_BOOT_PARAMS 23 str r0, [r1] 24 b save_boot_params_ret 25 ENDPROC(save_boot_params)

and in the AM437x TRM "5.2.5.1 Overview" I read: """ When ROM transfers control to the ISW, it passes a parameter to a Boot Parameter Structure in R0. The Boot Parameter Structure can be used to determine the boot device, reset reason, etc. The fields of this structure are described in Table 5-9 """

this makes sense now ... thanks for clarifying.

I think, in the first step we do not need to change this here.

I try to investigate some time for passing gd_t between TPL/SPL and U-Boot ... may this makes sense for variables like dram start, size bootparameters, baudrate ... but gd_t needs some cleanup restructuring here.

bye, Heiko

On Wed, Mar 13, 2019 at 08:44:45AM +0100, Heiko Schocher wrote:

Hello Wolfgang,

Am 12.03.2019 um 18:46 schrieb Wolfgang Denk:

...

Dear Tom,

In message 20190312173125.GP4690@bill-the-cat you wrote:

...

...

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.

...

...

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.

I see. Well, I think it's best if Heiko explains in detail; what he has observed, and what when which part of the information got lost. I was just interpreting his mail, so I may easily have misunderstood this.

@ Heiko: Can you please elucidate?

arch/arm/include/asm/arch-am33xx/omap.h

19 #ifdef CONFIG_AM33XX 20 #define NON_SECURE_SRAM_START 0x402F0400 21 #define NON_SECURE_SRAM_END 0x40310000 22 #define NON_SECURE_SRAM_IMG_END 0x4030B800 23 #elif defined(CONFIG_TI816X) || defined(CONFIG_TI814X) 24 #define NON_SECURE_SRAM_START 0x40300000 25 #define NON_SECURE_SRAM_END 0x40320000 26 #define NON_SECURE_SRAM_IMG_END 0x4031B800 27 #elif defined(CONFIG_AM43XX) 28 #define NON_SECURE_SRAM_START 0x402F0400 29 #define NON_SECURE_SRAM_END 0x40340000 30 #define NON_SECURE_SRAM_IMG_END 0x40337DE0 31 #define QSPI_BASE 0x47900000 32 #endif 33 #define SRAM_SCRATCH_SPACE_ADDR (NON_SECURE_SRAM_IMG_END - SZ_1K) 34

For reference, the AM437x TRM is at https://www.ti.com/lit/pdf/spruhl7 and as of this writing is at rev H and section 5.2.3 "Memory Map" under section 5 which is "Initialization" is helpful to understand these values. The AM335x TRM is at https://www.ti.com/lit/pdf/spruh73 and also helpful to understand how and why things are written how they are. The other parts of this family have similar chapters and breakdowns.

So, lets comment on these, with the TRM too. We can see from the table that we use 0x402F0400 for NON_SECURE_SRAM_START as it's the best value for all silicon versions as we must not touch that HS area.

Lets also take an aside to note that while the AM335x TRM does not note that some areas are reserved for ROM the AM437x TRM does and it's a good assumption that it's better documenting things that are likely true on older SoCs.

We say that 0x40340000 is NON_SECURE_SRAM_END is the top of SRAM (as the HS area is below the non-HS area). Finally we say 0x4031B800 is NON_SECURE_SRAM_IMG_END as that is the top of "Downloaded image" area.

Finally, we put the scratch space into the download area and give ourselves a "generous" 1K for future use. This is because of either concerns or actual problems with using space elsewhere in SRAM and being able to access it later. Or just to try and separate out the area between stack and this scratch space as much as possible.

and with ./include/configs/ti_armv7_common.h

69 #ifndef CONFIG_SYS_INIT_SP_ADDR 70 #define CONFIG_SYS_INIT_SP_ADDR (NON_SECURE_SRAM_END - \ 71 GENERATED_GBL_DATA_SIZE) 72 #endif

Here's where some troubles now start. As this was written versus the am335x TRM, I decided that "public stack", which those TRMs did not say was "reserved for ROM use", along with anything else outside the download area was fair game. We cannot download anything larger than that otherwise the ROM fails. This is why I put the stack pointer there. One could argue that's wrong now, given the guidance of the AM437x TRM.

73

include/generated/generic-asm-offsets.h

9 #define GENERATED_GBL_DATA_SIZE 224 /* (sizeof(struct global_data) + 15) & ~15 @ */ 10 #define GENERATED_BD_INFO_SIZE 112 /* (sizeof(struct bd_info) + 15) & ~15 @ */ 11 #define GD_SIZE 224 /* sizeof(struct global_data) @ */

-> CONFIG_SYS_INIT_SP_ADDR = 0x40340000 - 0xe0 = 0x4033ff20 -> SRAM_SCRATCH_SPACE_ADDR = 0x40337DE0 - 0x400 = 0x403379e0

./arch/arm/include/asm/omap_common.h: 816 #define OMAP_SRAM_SCRATCH_BOOT_PARAMS (SRAM_SCRATCH_SPACE_ADDR + 0x24)

OMAP_SRAM_SCRATCH_BOOT_PARAMS = 0x40337a04

So stack is on a higher address than the scratch space. Stack addresses decrement on usage, so may they overwrite scratch space, as SPL functionality grows more and more ...

Yes, there is a trade-off between image features and space constraints.

Hmm... I see, the NON_SECURE_SRAM_IMG_END is fix defined, and we cannot move it.

Ok, looking on my own now on the hardware:

=> md 40337a04 40337a04: 40338dc4 8f943b1e 8138beca 4ea1b2c2 ..3@.;....8.... ^ pointer to bootparms struct

=> md.b 40338dc4 40338dc4: ff ff ff ff 08 8f 33 40 07 01 00 00 00 00 00 00 ......3@........ ^^ bootmode 0x07 -> mmc0

Nothing overwritten here! Puuh...

But I have a bad feeling reading the bootmode again from U-Boot, as the boot_params info is may in space, where stack can overwrite it ...

Now, here's where you at least have a possibly easy answer. AM437x has the most SRAM available in the "download image" area, so you could indeed move the stack pointer to be below the scratch area, and checks so that we know that we've reserved 8KiB (or something) for stack in there too. The instructions in doc/README.SPL for estimating stack usage are still correct I'm pretty sure. The problems start to come on AM335x where non-HS has 110KiB and HS has 46KiB.

...

...

I agree here. Fixing things up such that we can pass things we know =66rom 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.

Thanks a lot! Let's go for it.

To be safe here, we should really pass the bootmode (or more common, the infos collected already in GD) from SPL to U-Boot ...

I don't object here, but you're going to run into memory constraints I strongly suspect if we want to use bloblist and there's some safety checking needed to make sure that if we pass GD along that it's a valid thing and not garbage in a register.