| Message ID | 1500889647-26472-1-git-send-email-andy.yan@rock-chips.com |
|---|---|
| State | Accepted |
| Commit | 2b71d098eefbd396771d19bc18c4ec36142976fa |
| Delegated to: | Philipp Tomsich |
| Headers | show |
> Some platforms has very small sram to run spl code, so > it may have no enough sapce for so much malloc pool before > relocation in spl stage as the normal u-boot stage. > Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > --- > > Changes in v3: > - use CONFIG_VAL(), which suggested by Simon > > Changes in v2: None > > arch/powerpc/cpu/mpc83xx/start.S | 8 ++++---- > arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------ > 2 files changed, 9 insertions(+), 10 deletions(-) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
On Mon, 24 Jul 2017, Andy Yan wrote: > Some platforms has very small sram to run spl code, so > it may have no enough sapce for so much malloc pool before > relocation in spl stage as the normal u-boot stage. > Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > > Changes in v3: > - use CONFIG_VAL(), which suggested by Simon > > Changes in v2: None > > arch/powerpc/cpu/mpc83xx/start.S | 8 ++++---- > arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------ > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S > index 2fed4a1..1c3c737 100644 > --- a/arch/powerpc/cpu/mpc83xx/start.S > +++ b/arch/powerpc/cpu/mpc83xx/start.S > @@ -274,14 +274,14 @@ in_flash: > cmplw r3, r4 > bne 1b > > -#ifdef CONFIG_SYS_MALLOC_F_LEN > +#if CONFIG_VAL(SYS_MALLOC_F_LEN) > > -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE > -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." > +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE > +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." > #endif > > /* r3 = new stack pointer / pre-reloc malloc area */ > - subi r3, r3, CONFIG_SYS_MALLOC_F_LEN > + subi r3, r3, CONFIG_VAL(SYS_MALLOC_F_LEN) > > /* Set pointer to pre-reloc malloc area in GD */ > stw r3, GD_MALLOC_BASE(r4) > diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S > index 63fdffd..58cb9fc 100644 > --- a/arch/powerpc/cpu/mpc85xx/start.S > +++ b/arch/powerpc/cpu/mpc85xx/start.S > @@ -1183,14 +1183,13 @@ _start_cont: > lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h > ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ > > -#ifdef CONFIG_SYS_MALLOC_F_LEN > - > -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE > -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." > +#if CONFIG_VAL(SYS_MALLOC_F_LEN) > +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE > +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." > #endif > > /* Leave 16+ byte for back chain termination and NULL return address */ > - subi r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf) > + subi r3,r3,((CONFIG_VAL(SYS_MALLOC_F_LEN)+16+15)&~0xf) > #endif Could we now just drop the outermost "#if CONFIG_VAL(SYS_MALLOC_F_LEN)" guard? (And as a question to someone with knowledge of the history of this code:) Why are those 16 bytes ("for back chain termination and NULL return address") consumed only when CONFIG_SYS_MALLOC_F_LEN is defined? Shouldn't this be required both with SYS_MALLOC_F_LEN and without? > > /* End of RAM */ > @@ -1204,7 +1203,7 @@ _start_cont: > cmplw r4,r3 > bne 1b > > -#ifdef CONFIG_SYS_MALLOC_F_LEN > +#if CONFIG_VAL(SYS_MALLOC_F_LEN) > lis r4,(CONFIG_SYS_INIT_RAM_ADDR)@h > ori r4,r4,(CONFIG_SYS_GBL_DATA_OFFSET)@l > >
Hi Phillip, On Thu, Jul 27, 2017 at 12:48 PM, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > > > On Mon, 24 Jul 2017, Andy Yan wrote: > >> Some platforms has very small sram to run spl code, so >> it may have no enough sapce for so much malloc pool before >> relocation in spl stage as the normal u-boot stage. >> Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition. >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >> >> --- >> >> Changes in v3: >> - use CONFIG_VAL(), which suggested by Simon >> >> Changes in v2: None >> >> arch/powerpc/cpu/mpc83xx/start.S | 8 ++++---- >> arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------ >> 2 files changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/arch/powerpc/cpu/mpc83xx/start.S >> b/arch/powerpc/cpu/mpc83xx/start.S >> index 2fed4a1..1c3c737 100644 >> --- a/arch/powerpc/cpu/mpc83xx/start.S >> +++ b/arch/powerpc/cpu/mpc83xx/start.S >> @@ -274,14 +274,14 @@ in_flash: >> cmplw r3, r4 >> bne 1b >> >> -#ifdef CONFIG_SYS_MALLOC_F_LEN >> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) >> >> -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > >> CONFIG_SYS_INIT_RAM_SIZE >> -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." >> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > >> CONFIG_SYS_INIT_RAM_SIZE >> +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." >> #endif >> >> /* r3 = new stack pointer / pre-reloc malloc area */ >> - subi r3, r3, CONFIG_SYS_MALLOC_F_LEN >> + subi r3, r3, CONFIG_VAL(SYS_MALLOC_F_LEN) >> >> /* Set pointer to pre-reloc malloc area in GD */ >> stw r3, GD_MALLOC_BASE(r4) >> diff --git a/arch/powerpc/cpu/mpc85xx/start.S >> b/arch/powerpc/cpu/mpc85xx/start.S >> index 63fdffd..58cb9fc 100644 >> --- a/arch/powerpc/cpu/mpc85xx/start.S >> +++ b/arch/powerpc/cpu/mpc85xx/start.S >> @@ -1183,14 +1183,13 @@ _start_cont: >> lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h >> ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to >> 16 */ >> >> -#ifdef CONFIG_SYS_MALLOC_F_LEN >> - >> -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > >> CONFIG_SYS_INIT_RAM_SIZE >> -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." >> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) >> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > >> CONFIG_SYS_INIT_RAM_SIZE >> +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." >> #endif >> >> /* Leave 16+ byte for back chain termination and NULL return >> address */ >> - subi r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf) >> + subi r3,r3,((CONFIG_VAL(SYS_MALLOC_F_LEN)+16+15)&~0xf) >> #endif > > > Could we now just drop the outermost "#if CONFIG_VAL(SYS_MALLOC_F_LEN)" > guard? > > (And as a question to someone with knowledge of the history of this code:) > Why are those 16 bytes ("for back chain termination and NULL return > address") consumed only when CONFIG_SYS_MALLOC_F_LEN is defined? Shouldn't > this be required both with SYS_MALLOC_F_LEN and without? > The code is a bit tricky here: The actual back chain termination and NULL return address setting is further down: stw r0,0(r3) /* Terminate Back Chain * stw r0,+4(r3) /* NULL return address. */ Now, if we don't have a pre-reloc malloc area, the r3 we computed via lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ will just become our stack pointer. The global data are starts right after the stack area (CONFIG_SYS_INIT_SP_OFFSET == CONFIG_SYS_GBL_DATA_OFFSET), which is OK at first glance, since the stack "grows downwards", but since the back chain termination and null address setting write beyond the stack pointer (see above), they would write into the GD area (and might be overwritten). But since we subtracted 16 in the above statement, we have room left to store them. The case where we have a pre-reloc malloc area is similar: Here we subtract the pre-reloc malloc area's size from the computed r3 and also a bit more to keep enough space for the back chain termination and null address: subi r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf) Otherwise, the addresses of these would lie in the pre-reloc malloc area, and might be overwritten. So the statement basically exists to "fix" what the substraction of the pre-reloc malloc area size disturbed. If we don't have pre-reloc malloc, the initial setting of r3 (with the -16 offset) takes care of that already. >> >> /* End of RAM */ >> @@ -1204,7 +1203,7 @@ _start_cont: >> cmplw r4,r3 >> bne 1b >> >> -#ifdef CONFIG_SYS_MALLOC_F_LEN >> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) >> lis r4,(CONFIG_SYS_INIT_RAM_ADDR)@h >> ori r4,r4,(CONFIG_SYS_GBL_DATA_OFFSET)@l >> >> Best regards, Mario
Mario, > On 27 Jul 2017, at 13:45, Mario Six <mariosix1986@gmail.com> wrote: > > Hi Phillip, > > On Thu, Jul 27, 2017 at 12:48 PM, Philipp Tomsich > <philipp.tomsich@theobroma-systems.com> wrote: >> >> >> On Mon, 24 Jul 2017, Andy Yan wrote: >> >>> Some platforms has very small sram to run spl code, so >>> it may have no enough sapce for so much malloc pool before >>> relocation in spl stage as the normal u-boot stage. >>> Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition. >>> >>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >>> >>> --- >>> >>> Changes in v3: >>> - use CONFIG_VAL(), which suggested by Simon >>> >>> Changes in v2: None >>> >>> arch/powerpc/cpu/mpc83xx/start.S | 8 ++++---- >>> arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------ >>> 2 files changed, 9 insertions(+), 10 deletions(-) >>> >>> diff --git a/arch/powerpc/cpu/mpc83xx/start.S >>> b/arch/powerpc/cpu/mpc83xx/start.S >>> index 2fed4a1..1c3c737 100644 >>> --- a/arch/powerpc/cpu/mpc83xx/start.S >>> +++ b/arch/powerpc/cpu/mpc83xx/start.S >>> @@ -274,14 +274,14 @@ in_flash: >>> cmplw r3, r4 >>> bne 1b >>> >>> -#ifdef CONFIG_SYS_MALLOC_F_LEN >>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) >>> >>> -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > >>> CONFIG_SYS_INIT_RAM_SIZE >>> -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." >>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > >>> CONFIG_SYS_INIT_RAM_SIZE >>> +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." >>> #endif >>> >>> /* r3 = new stack pointer / pre-reloc malloc area */ >>> - subi r3, r3, CONFIG_SYS_MALLOC_F_LEN >>> + subi r3, r3, CONFIG_VAL(SYS_MALLOC_F_LEN) >>> >>> /* Set pointer to pre-reloc malloc area in GD */ >>> stw r3, GD_MALLOC_BASE(r4) >>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S >>> b/arch/powerpc/cpu/mpc85xx/start.S >>> index 63fdffd..58cb9fc 100644 >>> --- a/arch/powerpc/cpu/mpc85xx/start.S >>> +++ b/arch/powerpc/cpu/mpc85xx/start.S >>> @@ -1183,14 +1183,13 @@ _start_cont: >>> lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h >>> ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to >>> 16 */ >>> >>> -#ifdef CONFIG_SYS_MALLOC_F_LEN >>> - >>> -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > >>> CONFIG_SYS_INIT_RAM_SIZE >>> -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." >>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) >>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > >>> CONFIG_SYS_INIT_RAM_SIZE >>> +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." >>> #endif >>> >>> /* Leave 16+ byte for back chain termination and NULL return >>> address */ >>> - subi r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf) >>> + subi r3,r3,((CONFIG_VAL(SYS_MALLOC_F_LEN)+16+15)&~0xf) >>> #endif >> >> >> Could we now just drop the outermost "#if CONFIG_VAL(SYS_MALLOC_F_LEN)" >> guard? >> >> (And as a question to someone with knowledge of the history of this code:) >> Why are those 16 bytes ("for back chain termination and NULL return >> address") consumed only when CONFIG_SYS_MALLOC_F_LEN is defined? Shouldn't >> this be required both with SYS_MALLOC_F_LEN and without? >> > > The code is a bit tricky here: The actual back chain termination and NULL > return address setting is further down: > > stw r0,0(r3) /* Terminate Back Chain * > stw r0,+4(r3) /* NULL return address. */ > > Now, if we don't have a pre-reloc malloc area, the r3 we computed via > > lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h > ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ > > will just become our stack pointer. > > The global data are starts right after the stack area > (CONFIG_SYS_INIT_SP_OFFSET == CONFIG_SYS_GBL_DATA_OFFSET), which is OK at first > glance, since the stack "grows downwards", but since the back chain termination > and null address setting write beyond the stack pointer (see above), they would > write into the GD area (and might be overwritten). But since we subtracted 16 > in the above statement, we have room left to store them. > > The case where we have a pre-reloc malloc area is similar: Here we subtract the > pre-reloc malloc area's size from the computed r3 and also a bit more to keep > enough space for the back chain termination and null address: > > subi r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf) > > Otherwise, the addresses of these would lie in the pre-reloc malloc area, and > might be overwritten. > > So the statement basically exists to "fix" what the substraction of the > pre-reloc malloc area size disturbed. If we don't have pre-reloc malloc, the > initial setting of r3 (with the -16 offset) takes care of that already. Thanks for reviewing. So how do you want to handle this: should I just merge this as-is (as it’s in a series that will go through the rockchip tree) or should we try to simplify this code in the same go? > >>> >>> /* End of RAM */ >>> @@ -1204,7 +1203,7 @@ _start_cont: >>> cmplw r4,r3 >>> bne 1b >>> >>> -#ifdef CONFIG_SYS_MALLOC_F_LEN >>> +#if CONFIG_VAL(SYS_MALLOC_F_LEN) >>> lis r4,(CONFIG_SYS_INIT_RAM_ADDR)@h >>> ori r4,r4,(CONFIG_SYS_GBL_DATA_OFFSET)@l >>> >>> > > Best regards, > > Mario
> Some platforms has very small sram to run spl code, so > it may have no enough sapce for so much malloc pool before > relocation in spl stage as the normal u-boot stage. > Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > > Changes in v3: > - use CONFIG_VAL(), which suggested by Simon > > Changes in v2: None > > arch/powerpc/cpu/mpc83xx/start.S | 8 ++++---- > arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------ > 2 files changed, 9 insertions(+), 10 deletions(-) > Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Some platforms has very small sram to run spl code, so > it may have no enough sapce for so much malloc pool before > relocation in spl stage as the normal u-boot stage. > Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > > Changes in v3: > - use CONFIG_VAL(), which suggested by Simon > > Changes in v2: None > > arch/powerpc/cpu/mpc83xx/start.S | 8 ++++---- > arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------ > 2 files changed, 9 insertions(+), 10 deletions(-) > Applied to u-boot-rockchip, thanks!
diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S index 2fed4a1..1c3c737 100644 --- a/arch/powerpc/cpu/mpc83xx/start.S +++ b/arch/powerpc/cpu/mpc83xx/start.S @@ -274,14 +274,14 @@ in_flash: cmplw r3, r4 bne 1b -#ifdef CONFIG_SYS_MALLOC_F_LEN +#if CONFIG_VAL(SYS_MALLOC_F_LEN) -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." #endif /* r3 = new stack pointer / pre-reloc malloc area */ - subi r3, r3, CONFIG_SYS_MALLOC_F_LEN + subi r3, r3, CONFIG_VAL(SYS_MALLOC_F_LEN) /* Set pointer to pre-reloc malloc area in GD */ stw r3, GD_MALLOC_BASE(r4) diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S index 63fdffd..58cb9fc 100644 --- a/arch/powerpc/cpu/mpc85xx/start.S +++ b/arch/powerpc/cpu/mpc85xx/start.S @@ -1183,14 +1183,13 @@ _start_cont: lis r3,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r3,r3,((CONFIG_SYS_INIT_SP_OFFSET-16)&~0xf)@l /* Align to 16 */ -#ifdef CONFIG_SYS_MALLOC_F_LEN - -#if CONFIG_SYS_MALLOC_F_LEN + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE -#error "CONFIG_SYS_MALLOC_F_LEN too large to fit into initial RAM." +#if CONFIG_VAL(SYS_MALLOC_F_LEN) +#if CONFIG_VAL(SYS_MALLOC_F_LEN) + GENERATED_GBL_DATA_SIZE > CONFIG_SYS_INIT_RAM_SIZE +#error "SYS_MALLOC_F_LEN too large to fit into initial RAM." #endif /* Leave 16+ byte for back chain termination and NULL return address */ - subi r3,r3,((CONFIG_SYS_MALLOC_F_LEN+16+15)&~0xf) + subi r3,r3,((CONFIG_VAL(SYS_MALLOC_F_LEN)+16+15)&~0xf) #endif /* End of RAM */ @@ -1204,7 +1203,7 @@ _start_cont: cmplw r4,r3 bne 1b -#ifdef CONFIG_SYS_MALLOC_F_LEN +#if CONFIG_VAL(SYS_MALLOC_F_LEN) lis r4,(CONFIG_SYS_INIT_RAM_ADDR)@h ori r4,r4,(CONFIG_SYS_GBL_DATA_OFFSET)@l
Some platforms has very small sram to run spl code, so it may have no enough sapce for so much malloc pool before relocation in spl stage as the normal u-boot stage. Use CONFIG_VAL(SYS_MALLOC_F_LEN) to fit this condition. Signed-off-by: Andy Yan <andy.yan@rock-chips.com> --- Changes in v3: - use CONFIG_VAL(), which suggested by Simon Changes in v2: None arch/powerpc/cpu/mpc83xx/start.S | 8 ++++---- arch/powerpc/cpu/mpc85xx/start.S | 11 +++++------ 2 files changed, 9 insertions(+), 10 deletions(-)