[U-Boot] passing info from SPL to U-Boot

[U-Boot] passing info from SPL to U-Boot

[Heiko Schocher]

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 thought gd info is passed from SPL to U-Boot, but this is not the case!

Looking into

...
  75         bic     r0, r0, #7      /* 8-byte alignment for ABI compliance */
  76         mov     sp, r0
  77         bl      board_init_f_alloc_reserve
  78         mov     sp, r0
  79         /* set up gd here, outside any C code */
  80         mov     r9, r0
  81         bl      board_init_f_init_reserve

and common/init/board_init.c:

  99 void board_init_f_init_reserve(ulong base)
100 {
101         struct global_data *gd_ptr;
102
103         /*
104          * clear GD entirely and set it up.
105          * Use gd_ptr, as gd may not be properly set yet.
106          */
107
108         gd_ptr = (struct global_data *)base;
109         /* zero the area */
110         memset(gd_ptr, '\0', sizeof(*gd));
111         /* set GD unless architecture did it already */
112 #if !defined(CONFIG_ARM)
113         arch_setup_gd(gd_ptr);
114 #endif

gd is always initialized with zeros, no chance for passing infos from
SPL to U-Boot...

I really thought, that gd_t was intentionally designed for passing data
between different U-Boot states, but looking into gd_t definiton in
include/asm-generic/global_data.h it is a big ifdef mess and not useable
as an "API" between TPL/SPL and U-Boot ...

I thought also, that SPL detects for example ramsize and than passes
this info to U-Boot ...

But Ok, I found "common/init/handoff.c" which seems now the way to go, but:

./common/board_f.c

  281 static int setup_spl_handoff(void)
  282 {
  283 #if CONFIG_IS_ENABLED(HANDOFF)
  284         gd->spl_handoff = bloblist_find(BLOBLISTT_SPL_HANDOFF,
  285                                         sizeof(struct spl_handoff));
  286         debug("Found SPL hand-off info %p\n", gd->spl_handoff);
  287 #endif
  288
  289         return 0;
  290 }

There is gd->spl_handoff used ... how could this work at least on arm,
if gd is set to zeros on init ?

Do I miss something obvious?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] passing info from SPL to U-Boot

[Heiko Schocher]

Hello Simon, Tom,

Am 12.03.2019 um 09:21 schrieb Heiko Schocher:
> 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 thought gd info is passed from SPL to U-Boot, but this is not the case!
> 
> Looking into
> 
> ...
>   75         bic     r0, r0, #7      /* 8-byte alignment for ABI compliance */
>   76         mov     sp, r0
>   77         bl      board_init_f_alloc_reserve
>   78         mov     sp, r0
>   79         /* set up gd here, outside any C code */
>   80         mov     r9, r0
>   81         bl      board_init_f_init_reserve
> 
> and common/init/board_init.c:
> 
>   99 void board_init_f_init_reserve(ulong base)
> 100 {
> 101         struct global_data *gd_ptr;
> 102
> 103         /*
> 104          * clear GD entirely and set it up.
> 105          * Use gd_ptr, as gd may not be properly set yet.
> 106          */
> 107
> 108         gd_ptr = (struct global_data *)base;
> 109         /* zero the area */
> 110         memset(gd_ptr, '\0', sizeof(*gd));
> 111         /* set GD unless architecture did it already */
> 112 #if !defined(CONFIG_ARM)
> 113         arch_setup_gd(gd_ptr);
> 114 #endif
> 
> gd is always initialized with zeros, no chance for passing infos from
> SPL to U-Boot...
> 
> I really thought, that gd_t was intentionally designed for passing data
> between different U-Boot states, but looking into gd_t definiton in
> include/asm-generic/global_data.h it is a big ifdef mess and not useable
> as an "API" between TPL/SPL and U-Boot ...
> 
> I thought also, that SPL detects for example ramsize and than passes
> this info to U-Boot ...
> 
> But Ok, I found "common/init/handoff.c" which seems now the way to go, but:
> 
> ./common/board_f.c
> 
>   281 static int setup_spl_handoff(void)
>   282 {
>   283 #if CONFIG_IS_ENABLED(HANDOFF)
>   284         gd->spl_handoff = bloblist_find(BLOBLISTT_SPL_HANDOFF,
>   285                                         sizeof(struct spl_handoff));
>   286         debug("Found SPL hand-off info %p\n", gd->spl_handoff);
>   287 #endif
>   288
>   289         return 0;
>   290 }
> 
> There is gd->spl_handoff used ... how could this work at least on arm,
> if gd is set to zeros on init ?
> 
> Do I miss something obvious?

Sorry for being so stupid, with:

common/board_f.c

  853 #ifdef CONFIG_BLOBLIST
  854         bloblist_init,
  855 #endif
  856         setup_spl_handoff,

and common/bloblist.c

216 int bloblist_init(void)
217 {
218         bool expected;
219         int ret = -ENOENT;
220
221         /**
222          * Wed expect to find an existing bloblist in the first phase of U-Boot
223          * that runs
224          */
225         expected = !u_boot_first_phase();
226         if (expected)
227                 ret = bloblist_check(CONFIG_BLOBLIST_ADDR,
228                                      CONFIG_BLOBLIST_SIZE);

gd->spl_handoff gets setup through bloblist_find() ...

But beside sandbox there is no current user currently, or?

$ grep -lr BLOBLIST_ADDR .
./test/bloblist.c
./include/bloblist.h
./common/bloblist.c
./common/Kconfig
./board/sandbox/README.sandbox
$

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] passing info from SPL to U-Boot

[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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190312/78522579/attachment.sig>

[U-Boot] passing info from SPL to U-Boot

[Heiko Schocher]

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 ?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] passing info from SPL to U-Boot

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190312/0a069391/attachment.sig>

[U-Boot] passing info from SPL to U-Boot

[Wolfgang Denk]

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"...one of the main causes of the fall of the Roman Empire was  that,
lacking  zero,  they had no way to indicate successful termination of
their C programs."                                     - Robert Firth

[U-Boot] passing info from SPL to U-Boot

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190312/94cc1e10/attachment.sig>

[U-Boot] passing info from SPL to U-Boot

[Wolfgang Denk]

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

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You don't have to stay up nights to succeed; you have to  stay  awake
days.

[U-Boot] passing info from SPL to U-Boot

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190312/c4529491/attachment.sig>

[U-Boot] passing info from SPL to U-Boot

[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?


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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No man knows what true happiness is until he gets married.  By  then,
of course, its too late.

[U-Boot] passing info from SPL to U-Boot

[Heiko Schocher]

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 at .;....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 at ........
                                   ^^
                                   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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] passing info from SPL to U-Boot

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

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 at .;....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 at ........
>                                   ^^
>                                   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


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] passing info from SPL to U-Boot

[Heiko Schocher]

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 at .;....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 at ........
>>                                    ^^
>>                                    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
> 
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] passing info from SPL to U-Boot

[Tom Rini]

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 at .;....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 at ........
>                                   ^^
>                                   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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190313/c265c406/attachment.sig>

[U-Boot] passing info from SPL to U-Boot

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190313/f7e5c4a5/attachment.sig>

[U-Boot] passing info from SPL to U-Boot

[Simon Glass]

Hi Heiko,

On Tue, 12 Mar 2019 at 02:50, Heiko Schocher <hs@denx.de> wrote:
>
> Hello Simon, Tom,
>
> Am 12.03.2019 um 09:21 schrieb Heiko Schocher:
> > 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 thought gd info is passed from SPL to U-Boot, but this is not the case!
> >
> > Looking into
> >
> > ...
> >   75         bic     r0, r0, #7      /* 8-byte alignment for ABI compliance */
> >   76         mov     sp, r0
> >   77         bl      board_init_f_alloc_reserve
> >   78         mov     sp, r0
> >   79         /* set up gd here, outside any C code */
> >   80         mov     r9, r0
> >   81         bl      board_init_f_init_reserve
> >
> > and common/init/board_init.c:
> >
> >   99 void board_init_f_init_reserve(ulong base)
> > 100 {
> > 101         struct global_data *gd_ptr;
> > 102
> > 103         /*
> > 104          * clear GD entirely and set it up.
> > 105          * Use gd_ptr, as gd may not be properly set yet.
> > 106          */
> > 107
> > 108         gd_ptr = (struct global_data *)base;
> > 109         /* zero the area */
> > 110         memset(gd_ptr, '\0', sizeof(*gd));
> > 111         /* set GD unless architecture did it already */
> > 112 #if !defined(CONFIG_ARM)
> > 113         arch_setup_gd(gd_ptr);
> > 114 #endif
> >
> > gd is always initialized with zeros, no chance for passing infos from
> > SPL to U-Boot...
> >
> > I really thought, that gd_t was intentionally designed for passing data
> > between different U-Boot states, but looking into gd_t definiton in
> > include/asm-generic/global_data.h it is a big ifdef mess and not useable
> > as an "API" between TPL/SPL and U-Boot ...
> >
> > I thought also, that SPL detects for example ramsize and than passes
> > this info to U-Boot ...
> >
> > But Ok, I found "common/init/handoff.c" which seems now the way to go, but:
> >
> > ./common/board_f.c
> >
> >   281 static int setup_spl_handoff(void)
> >   282 {
> >   283 #if CONFIG_IS_ENABLED(HANDOFF)
> >   284         gd->spl_handoff = bloblist_find(BLOBLISTT_SPL_HANDOFF,
> >   285                                         sizeof(struct spl_handoff));
> >   286         debug("Found SPL hand-off info %p\n", gd->spl_handoff);
> >   287 #endif
> >   288
> >   289         return 0;
> >   290 }
> >
> > There is gd->spl_handoff used ... how could this work at least on arm,
> > if gd is set to zeros on init ?
> >
> > Do I miss something obvious?
>
> Sorry for being so stupid, with:
>
> common/board_f.c
>
>   853 #ifdef CONFIG_BLOBLIST
>   854         bloblist_init,
>   855 #endif
>   856         setup_spl_handoff,
>
> and common/bloblist.c
>
> 216 int bloblist_init(void)
> 217 {
> 218         bool expected;
> 219         int ret = -ENOENT;
> 220
> 221         /**
> 222          * Wed expect to find an existing bloblist in the first phase of U-Boot
> 223          * that runs
> 224          */
> 225         expected = !u_boot_first_phase();
> 226         if (expected)
> 227                 ret = bloblist_check(CONFIG_BLOBLIST_ADDR,
> 228                                      CONFIG_BLOBLIST_SIZE);
>
> gd->spl_handoff gets setup through bloblist_find() ...
>
> But beside sandbox there is no current user currently, or?
>
> $ grep -lr BLOBLIST_ADDR .
> ./test/bloblist.c
> ./include/bloblist.h
> ./common/bloblist.c
> ./common/Kconfig
> ./board/sandbox/README.sandbox
> $

Yes that's right, it is not used outside sandbox, although there are
patches to use it on x86.

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.

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?

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.

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.

Regards,
Simon

[U-Boot] passing info from SPL to U-Boot

[Heiko Schocher]

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
-- 
-- DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] passing info from SPL to U-Boot

[Heiko Schocher]

Hello Simon,

Am 14.03.2019 um 11:02 schrieb Simon Glass:
> Hi Heiko,
> 
> On Tue, 12 Mar 2019 at 02:50, Heiko Schocher <hs@denx.de> wrote:
>>
>> Hello Simon, Tom,
>>
>> Am 12.03.2019 um 09:21 schrieb Heiko Schocher:
>>> 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 thought gd info is passed from SPL to U-Boot, but this is not the case!
>>>
>>> Looking into
>>>
>>> ...
>>>    75         bic     r0, r0, #7      /* 8-byte alignment for ABI compliance */
>>>    76         mov     sp, r0
>>>    77         bl      board_init_f_alloc_reserve
>>>    78         mov     sp, r0
>>>    79         /* set up gd here, outside any C code */
>>>    80         mov     r9, r0
>>>    81         bl      board_init_f_init_reserve
>>>
>>> and common/init/board_init.c:
>>>
>>>    99 void board_init_f_init_reserve(ulong base)
>>> 100 {
>>> 101         struct global_data *gd_ptr;
>>> 102
>>> 103         /*
>>> 104          * clear GD entirely and set it up.
>>> 105          * Use gd_ptr, as gd may not be properly set yet.
>>> 106          */
>>> 107
>>> 108         gd_ptr = (struct global_data *)base;
>>> 109         /* zero the area */
>>> 110         memset(gd_ptr, '\0', sizeof(*gd));
>>> 111         /* set GD unless architecture did it already */
>>> 112 #if !defined(CONFIG_ARM)
>>> 113         arch_setup_gd(gd_ptr);
>>> 114 #endif
>>>
>>> gd is always initialized with zeros, no chance for passing infos from
>>> SPL to U-Boot...
>>>
>>> I really thought, that gd_t was intentionally designed for passing data
>>> between different U-Boot states, but looking into gd_t definiton in
>>> include/asm-generic/global_data.h it is a big ifdef mess and not useable
>>> as an "API" between TPL/SPL and U-Boot ...
>>>
>>> I thought also, that SPL detects for example ramsize and than passes
>>> this info to U-Boot ...
>>>
>>> But Ok, I found "common/init/handoff.c" which seems now the way to go, but:
>>>
>>> ./common/board_f.c
>>>
>>>    281 static int setup_spl_handoff(void)
>>>    282 {
>>>    283 #if CONFIG_IS_ENABLED(HANDOFF)
>>>    284         gd->spl_handoff = bloblist_find(BLOBLISTT_SPL_HANDOFF,
>>>    285                                         sizeof(struct spl_handoff));
>>>    286         debug("Found SPL hand-off info %p\n", gd->spl_handoff);
>>>    287 #endif
>>>    288
>>>    289         return 0;
>>>    290 }
>>>
>>> There is gd->spl_handoff used ... how could this work at least on arm,
>>> if gd is set to zeros on init ?
>>>
>>> Do I miss something obvious?
>>
>> Sorry for being so stupid, with:
>>
>> common/board_f.c
>>
>>    853 #ifdef CONFIG_BLOBLIST
>>    854         bloblist_init,
>>    855 #endif
>>    856         setup_spl_handoff,
>>
>> and common/bloblist.c
>>
>> 216 int bloblist_init(void)
>> 217 {
>> 218         bool expected;
>> 219         int ret = -ENOENT;
>> 220
>> 221         /**
>> 222          * Wed expect to find an existing bloblist in the first phase of U-Boot
>> 223          * that runs
>> 224          */
>> 225         expected = !u_boot_first_phase();
>> 226         if (expected)
>> 227                 ret = bloblist_check(CONFIG_BLOBLIST_ADDR,
>> 228                                      CONFIG_BLOBLIST_SIZE);
>>
>> gd->spl_handoff gets setup through bloblist_find() ...
>>
>> But beside sandbox there is no current user currently, or?
>>
>> $ grep -lr BLOBLIST_ADDR .
>> ./test/bloblist.c
>> ./include/bloblist.h
>> ./common/bloblist.c
>> ./common/Kconfig
>> ./board/sandbox/README.sandbox
>> $
> 
> Yes that's right, it is not used outside sandbox, although there are
> patches to use it on x86.

Thanks for clarifying!

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

I thought about putting interchangeable things at the beginning
of gd_t.

The big advantage of a bloblist is, that you only need to parse it
and you know, what values are really setup from the previous stage.

Passing structs is not ideal, but small ... but may error prone?

Can we always be sure, previous stage sets up all info in gd_t
next stage expects?

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

Yes, this could be used.

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

Just experimenting with:
diff --git a/common/init/board_init.c b/common/init/board_init.c
index 526fee35ff..bdb4b6364d 100644
--- a/common/init/board_init.c
+++ b/common/init/board_init.c
@@ -96,9 +96,10 @@ ulong board_init_f_alloc_reserve(ulong top)
   * (seemingly useless) incrementation causes no code increase.
   */

-void board_init_f_init_reserve(ulong base)
+void board_init_f_init_reserve(ulong base, ulong oldgd)
  {
         struct global_data *gd_ptr;
+       struct global_data *gd_ptr_old;

         /*
          * clear GD entirely and set it up.
@@ -106,8 +107,22 @@ void board_init_f_init_reserve(ulong base)
          */

         gd_ptr = (struct global_data *)base;
-       /* zero the area */
-       memset(gd_ptr, '\0', sizeof(*gd));
+       gd_ptr_old = (struct global_data *)oldgd;
+       if (u_boot_first_phase() ||
+           oldgd == 0 ||
+           !(((gd_ptr->magic == UBOOT_GD_MAGIC) &&
+                (gd_ptr->gd_size == sizeof(struct global_data))))) {
+               /* setup gd, zero the area ... */
+               memset(gd_ptr, '\0', sizeof(*gd));
+               /* ... and set magic and size information */
+               gd_ptr->magic = UBOOT_GD_MAGIC;
+               gd_ptr->gd_size = sizeof(struct global_data);
+       } else {
+               /*
+                * in case we get an already initialised gd_pointer
+                * use the info from already existing oldgd.
+                */
+       }
         /* set GD unless architecture did it already */
  #if !defined(CONFIG_ARM)
         arch_setup_gd(gd_ptr);
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 78dcf40bff..3a5063dbe1 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -23,8 +23,11 @@
  #include <membuff.h>
  #include <linux/list.h>

+#define UBOOT_GD_MAGIC 0x19730517
  typedef struct global_data {
         bd_t *bd;
+       unsigned long magic;
+       int gd_size;
         unsigned long flags;
         unsigned int baudrate;
         unsigned long cpu_clk;          /* CPU clock in Hz!             */

(after moving u_boot_first_phase() to include/common.h)

But it is WIP, nothing functional yet...

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

Perhaps, yes.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] passing info from SPL to U-Boot

[Wolfgang Denk]

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.

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

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.

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


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


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

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



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Little known fact about Middle Earth:   The Hobbits had a very sophi-
sticated computer network!   It was a Tolkien Ring...

[U-Boot] passing info from SPL to U-Boot

[Tom Rini]

On Fri, Mar 15, 2019 at 05:41:36AM +0100, Heiko Schocher wrote:
> 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
> """

Yup.  And please bear in mind that what you see there has been true
since OMAP2 days, iirc, and is true for all 32bit Cortex-A TI platforms,
basically.  It's possible that the keystone platforms are the exception
to the rule here, actually.  But lots and lots of SoCs with a large age
range on them.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190315/d81c0d23/attachment.sig>

[U-Boot] passing info from SPL to U-Boot

[Simon Glass]

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

[U-Boot] passing info from SPL to U-Boot

[Wolfgang Denk]

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.

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

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

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?

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

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

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

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


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The algorithm to do that is extremely nasty. You might want  to  mug
someone with it."                   - M. Devine, Computer Science 340

[U-Boot] passing info from SPL to U-Boot

[Simon Glass]

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

[U-Boot] passing info from SPL to U-Boot

[Philipp Tomsich]

> On 17.03.2019, at 15:12, Simon Glass <sjg@chromium.org> wrote:
> 
> 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?

For some RK33xx devices, we inject the original boot-location into
the FDT passed to the full U-Boot stage.  While this requires FIT in
SPL (and may thus not be generally applicable), it is one convenient
way for such use-cases today.

Thanks,
Philipp.