Patchwork [U-Boot,v4,07/13] davinci: Use correct #ifdef around gdata/bdata

login
register
mail settings
Submitter Simon Glass
Date Feb. 21, 2012, 1:32 a.m.
Message ID <1329787975-6695-8-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/142220/
State Deferred
Delegated to: Tom Rini
Headers show

Comments

Simon Glass - Feb. 21, 2012, 1:32 a.m.
This fixes the following warnings in an SPL build when libcommon is
in use:

spl.c:37: warning: 'gdata' defined but not used
spl.c:38: warning: 'bdata' defined but not used

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v4:
- Add new patch to fix davinci build warnings

 arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Tom Rini - Feb. 21, 2012, 3:24 p.m.
On Mon, Feb 20, 2012 at 05:32:49PM -0800, Simon Glass wrote:

> This fixes the following warnings in an SPL build when libcommon is
> in use:
> 
> spl.c:37: warning: 'gdata' defined but not used
> spl.c:38: warning: 'bdata' defined but not used
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Tom Rini <trini@ti.com>
Sughosh Ganu - Feb. 23, 2012, 5:25 p.m.
hi Simon,

On Mon Feb 20, 2012 at 05:32:49PM -0800, Simon Glass wrote:
> This fixes the following warnings in an SPL build when libcommon is
> in use:
> 
> spl.c:37: warning: 'gdata' defined but not used
> spl.c:38: warning: 'bdata' defined but not used
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v4:
> - Add new patch to fix davinci build warnings
> 
>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
> index b1eff26..2861907 100644
> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
> @@ -32,10 +32,12 @@
>  
>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>  
> +#ifdef CONFIG_SPL_SPI_LOAD
>  DECLARE_GLOBAL_DATA_PTR;
>  /* Define global data structure pointer to it*/
>  static gd_t gdata __attribute__ ((section(".data")));
>  static bd_t bdata __attribute__ ((section(".data")));
> +#endif

  Can you specify which boards you get this warning for. With your
  patch to add libcommon to hawkboard's spl image, this is now also
  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

-sughosh
Simon Glass - Feb. 26, 2012, 5:56 p.m.
Hi Sughosh,

On Thu, Feb 23, 2012 at 9:25 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> hi Simon,
>
> On Mon Feb 20, 2012 at 05:32:49PM -0800, Simon Glass wrote:
>> This fixes the following warnings in an SPL build when libcommon is
>> in use:
>>
>> spl.c:37: warning: 'gdata' defined but not used
>> spl.c:38: warning: 'bdata' defined but not used
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> Changes in v4:
>> - Add new patch to fix davinci build warnings
>>
>>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> index b1eff26..2861907 100644
>> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
>> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> @@ -32,10 +32,12 @@
>>
>>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>>
>> +#ifdef CONFIG_SPL_SPI_LOAD
>>  DECLARE_GLOBAL_DATA_PTR;
>>  /* Define global data structure pointer to it*/
>>  static gd_t gdata __attribute__ ((section(".data")));
>>  static bd_t bdata __attribute__ ((section(".data")));
>> +#endif
>
>  Can you specify which boards you get this warning for. With your
>  patch to add libcommon to hawkboard's spl image, this is now also
>  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Perhaps it is any davinci board, with SPI? I don't have any of these -
I was just fixing what I thought was a minor #ifdef bug in the code.

Regards,
Simon

>
> -sughosh
Sughosh Ganu - Feb. 27, 2012, 10:16 a.m.
hi Simon,

On Sun Feb 26, 2012 at 09:56:37AM -0800, Simon Glass wrote:
> Hi Sughosh,
> 
> On Thu, Feb 23, 2012 at 9:25 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> > hi Simon,
> >
> > On Mon Feb 20, 2012 at 05:32:49PM -0800, Simon Glass wrote:
> >> This fixes the following warnings in an SPL build when libcommon is
> >> in use:
> >>
> >> spl.c:37: warning: 'gdata' defined but not used
> >> spl.c:38: warning: 'bdata' defined but not used
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >> Changes in v4:
> >> - Add new patch to fix davinci build warnings
> >>
> >>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> index b1eff26..2861907 100644
> >> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> @@ -32,10 +32,12 @@
> >>
> >>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> >>
> >> +#ifdef CONFIG_SPL_SPI_LOAD
> >>  DECLARE_GLOBAL_DATA_PTR;
> >>  /* Define global data structure pointer to it*/
> >>  static gd_t gdata __attribute__ ((section(".data")));
> >>  static bd_t bdata __attribute__ ((section(".data")));
> >> +#endif
> >
> >  Can you specify which boards you get this warning for. With your
> >  patch to add libcommon to hawkboard's spl image, this is now also
> >  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.
> 
> Perhaps it is any davinci board, with SPI? I don't have any of these -
> I was just fixing what I thought was a minor #ifdef bug in the code.

  I checked the configs for all the davinci boards, and cam_enc_4xx,
  da850* and hawkboard use spl. Out of these the da850* use a spi
  flash, while cam_enc_4xx and hawkboard both use a nand. So we should
  not be using the CONFIG_SPL_SPI_LOAD check to exclude the gdata and
  bdata objects -- these are now needed after adding the libcommon
  support to the hawkboard.

  Also, the cam_enc_4xx board which uses a spl does not have
  CONFIG_SPL_LIBCOMMON_SUPPORT and CONFIG_SPL_LIBGENERIC_SUPPORT
  defined and this patchset does not add these defines for the
  board. Was adding these defines for the board missed out. If so,
  then this patch would no longer be needed.

-sughosh
Christian Riesch - Feb. 27, 2012, 10:39 a.m.
Hi,

On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> hi Simon,
>
> On Sun Feb 26, 2012 at 09:56:37AM -0800, Simon Glass wrote:
>> Hi Sughosh,
>>
>> On Thu, Feb 23, 2012 at 9:25 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> > hi Simon,
>> >
>> > On Mon Feb 20, 2012 at 05:32:49PM -0800, Simon Glass wrote:
>> >> This fixes the following warnings in an SPL build when libcommon is
>> >> in use:
>> >>
>> >> spl.c:37: warning: 'gdata' defined but not used
>> >> spl.c:38: warning: 'bdata' defined but not used
>> >>
>> >> Signed-off-by: Simon Glass <sjg@chromium.org>
>> >> ---
>> >> Changes in v4:
>> >> - Add new patch to fix davinci build warnings
>> >>
>> >>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
>> >>  1 files changed, 2 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> index b1eff26..2861907 100644
>> >> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> @@ -32,10 +32,12 @@
>> >>
>> >>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> >>
>> >> +#ifdef CONFIG_SPL_SPI_LOAD
>> >>  DECLARE_GLOBAL_DATA_PTR;
>> >>  /* Define global data structure pointer to it*/
>> >>  static gd_t gdata __attribute__ ((section(".data")));
>> >>  static bd_t bdata __attribute__ ((section(".data")));
>> >> +#endif

In arch/arm/cpu/arm926ejs/davinci/spl.c gdata and bdata are used for
serial_init() to allow output to the console from the SPL. However,
this is only the case when LIBCOMMON is used. LIBCOMMON is required
for booting from SPI, but not for booting from NAND. Up to now davinci
boards that boot from NAND with SPL did not use LIBCOMMON but used the
puts and putc functions defined in spl.c for console output.

>> >  Can you specify which boards you get this warning for. With your
>> >  patch to add libcommon to hawkboard's spl image, this is now also
>> >  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Simon's patch is for the hawkboard, since due to another patch in his
patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board
that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did
not check the rest of your patchset, why do you need LIBCOMMON on the
hawkboard at all?)

However, the patch fixes the warning, but now we use putc and puts
from LIBCOMMON without initializing them. Thus it breaks the console
output of the hawkboard SPL.

So the correct solution would be to keep the ifdefs around the
definition of gdata and bdata as they are, but change board_init_f()
like this:

void board_init_r(gd_t *id, ulong dummy)
{
#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
        mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN,
                        CONFIG_SYS_MALLOC_LEN);

        gd = &gdata;
        gd->bd = &bdata;
        gd->flags |= GD_FLG_RELOC;
        gd->baudrate = CONFIG_BAUDRATE;
        serial_init();          /* serial communications setup */
        gd->have_console = 1;
#endif
#ifdef CONFIG_SPL_NAND_LOAD
        nand_init();
        puts("Nand boot...\n");
        nand_boot();
#endif
#ifdef CONFIG_SPL_SPI_LOAD
        puts("SPI boot...\n");
        spi_boot();
#endif
}

Regards, Christian
Sughosh Ganu - Feb. 27, 2012, 10:56 a.m.
hi Christian,

On Mon Feb 27, 2012 at 11:39:42AM +0100, Christian Riesch wrote:
> Hi,
> 
> On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:

<snip>

> >> >>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
> >> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >> >>
> >> >> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> >> index b1eff26..2861907 100644
> >> >> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> >> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> >> @@ -32,10 +32,12 @@
> >> >>
> >> >>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> >> >>
> >> >> +#ifdef CONFIG_SPL_SPI_LOAD
> >> >>  DECLARE_GLOBAL_DATA_PTR;
> >> >>  /* Define global data structure pointer to it*/
> >> >>  static gd_t gdata __attribute__ ((section(".data")));
> >> >>  static bd_t bdata __attribute__ ((section(".data")));
> >> >> +#endif

<snip>

> >> >  Can you specify which boards you get this warning for. With your
> >> >  patch to add libcommon to hawkboard's spl image, this is now also
> >> >  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.
> 
> Simon's patch is for the hawkboard, since due to another patch in his
> patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board
> that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did
> not check the rest of your patchset, why do you need LIBCOMMON on the
> hawkboard at all?)

  LIBCOMMON is now needed as the generic relocation based functions
  are part of the libcommon.o, which are being enabled in the same
  patchset for all arm boards. So if i understand correct, all arm
  board based spl's now need libcommon and libgeneric.

  The only thing i see is that libcommon and libgeneric are not
  defined for cam_enc_4xx board which uses spl, and this patchset does
  not add it either. Not sure whether it got missed.


> However, the patch fixes the warning, but now we use putc and puts
> from LIBCOMMON without initializing them. Thus it breaks the console
> output of the hawkboard SPL.
> 
> So the correct solution would be to keep the ifdefs around the
> definition of gdata and bdata as they are, but change board_init_f()
> like this:

  I have a patch for this, which i will send today. There is only one
  question i have though.

> 
> void board_init_r(gd_t *id, ulong dummy)
> {
> #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>         mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN,
>                         CONFIG_SYS_MALLOC_LEN);

  Can you please explain why we need the mem_malloc_init. I did not
  include this, and spl boots up just fine on my board.

-sughosh
Christian Riesch - Feb. 27, 2012, 11:37 a.m.
Hi Sughosh,

On Mon, Feb 27, 2012 at 11:56 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> hi Christian,
>
> On Mon Feb 27, 2012 at 11:39:42AM +0100, Christian Riesch wrote:
>> Hi,
>>
>> On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>
> <snip>
>
>> >> >>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
>> >> >>  1 files changed, 2 insertions(+), 0 deletions(-)
>> >> >>
>> >> >> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> >> index b1eff26..2861907 100644
>> >> >> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> >> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> >> @@ -32,10 +32,12 @@
>> >> >>
>> >> >>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> >> >>
>> >> >> +#ifdef CONFIG_SPL_SPI_LOAD
>> >> >>  DECLARE_GLOBAL_DATA_PTR;
>> >> >>  /* Define global data structure pointer to it*/
>> >> >>  static gd_t gdata __attribute__ ((section(".data")));
>> >> >>  static bd_t bdata __attribute__ ((section(".data")));
>> >> >> +#endif
>
> <snip>
>
>> >> >  Can you specify which boards you get this warning for. With your
>> >> >  patch to add libcommon to hawkboard's spl image, this is now also
>> >> >  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.
>>
>> Simon's patch is for the hawkboard, since due to another patch in his
>> patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board
>> that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did
>> not check the rest of your patchset, why do you need LIBCOMMON on the
>> hawkboard at all?)
>
>  LIBCOMMON is now needed as the generic relocation based functions
>  are part of the libcommon.o, which are being enabled in the same
>  patchset for all arm boards. So if i understand correct, all arm
>  board based spl's now need libcommon and libgeneric.
>
>  The only thing i see is that libcommon and libgeneric are not
>  defined for cam_enc_4xx board which uses spl, and this patchset does
>  not add it either. Not sure whether it got missed.

When I asked Heiko Schocher a few month ago why he defined putc and
puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could
not use LIBCOMMON due to size limitations for the SPL. So I guess that
this board will not be able to use the generic relocation functions,
unless the SPL is smaller than 16kB, right? Simon's patchset will
break this board then, right?

However, I we can make all davinci boards use LIBCOMMON, we can remove
the putc/puts functions from spl.c.

>> However, the patch fixes the warning, but now we use putc and puts
>> from LIBCOMMON without initializing them. Thus it breaks the console
>> output of the hawkboard SPL.
>>
>> So the correct solution would be to keep the ifdefs around the
>> definition of gdata and bdata as they are, but change board_init_f()
>> like this:
>
>  I have a patch for this, which i will send today. There is only one
>  question i have though.
>
>>
>> void board_init_r(gd_t *id, ulong dummy)
>> {
>> #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>>         mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN,
>>                         CONFIG_SYS_MALLOC_LEN);
>
>  Can you please explain why we need the mem_malloc_init. I did not
>  include this, and spl boots up just fine on my board.
>

malloc is required for the SPI code only, so you could also put it
within #ifdef CONFIG_SPL_SPI_LOAD

Regards, Christian
Sughosh Ganu - Feb. 27, 2012, 12:02 p.m.
hi Christian,

On Mon Feb 27, 2012 at 12:37:16PM +0100, Christian Riesch wrote:
> Hi Sughosh,
> 
> On Mon, Feb 27, 2012 at 11:56 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> > hi Christian,
> >
> > On Mon Feb 27, 2012 at 11:39:42AM +0100, Christian Riesch wrote:
> >> Hi,
> >>
> >> On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> >
> > <snip>
> >
> >> >> >>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
> >> >> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >> >> >>
> >> >> >> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> >> >> index b1eff26..2861907 100644
> >> >> >> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> >> >> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
> >> >> >> @@ -32,10 +32,12 @@
> >> >> >>
> >> >> >>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> >> >> >>
> >> >> >> +#ifdef CONFIG_SPL_SPI_LOAD
> >> >> >>  DECLARE_GLOBAL_DATA_PTR;
> >> >> >>  /* Define global data structure pointer to it*/
> >> >> >>  static gd_t gdata __attribute__ ((section(".data")));
> >> >> >>  static bd_t bdata __attribute__ ((section(".data")));
> >> >> >> +#endif
> >
> > <snip>
> >
> >> >> >  Can you specify which boards you get this warning for. With your
> >> >> >  patch to add libcommon to hawkboard's spl image, this is now also
> >> >> >  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.
> >>
> >> Simon's patch is for the hawkboard, since due to another patch in his
> >> patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board
> >> that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did
> >> not check the rest of your patchset, why do you need LIBCOMMON on the
> >> hawkboard at all?)
> >
> >  LIBCOMMON is now needed as the generic relocation based functions
> >  are part of the libcommon.o, which are being enabled in the same
> >  patchset for all arm boards. So if i understand correct, all arm
> >  board based spl's now need libcommon and libgeneric.
> >
> >  The only thing i see is that libcommon and libgeneric are not
> >  defined for cam_enc_4xx board which uses spl, and this patchset does
> >  not add it either. Not sure whether it got missed.
> 
> When I asked Heiko Schocher a few month ago why he defined putc and
> puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could
> not use LIBCOMMON due to size limitations for the SPL. So I guess that
> this board will not be able to use the generic relocation functions,
> unless the SPL is smaller than 16kB, right? Simon's patchset will
> break this board then, right?

  That is exactly what i reported in one of the threads in response to
  addition of libcommon and libgeneric to the hawkboard's spl. In
  fact, this might cause problems on quite a few boards with spl size
  restrictions. I am not sure, whether the generic relocation feature
  should be turned on by default on all boards or should be a config
  option -- at least for the spl builds. Another option would be to
  move it to a place where it is not needed to compile in the entire
  libcommon/libgeneric support that is not needed for the generic
  relocation code. I think that would help us keep the generic
  relocation without the size bloat that we see right now.

  http://lists.denx.de/pipermail/u-boot/2012-February/118567.html

<snip>

> >> void board_init_r(gd_t *id, ulong dummy)
> >> {
> >> #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> >>         mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN,
> >>                         CONFIG_SYS_MALLOC_LEN);
> >
> >  Can you please explain why we need the mem_malloc_init. I did not
> >  include this, and spl boots up just fine on my board.
> >
> 
> malloc is required for the SPI code only, so you could also put it
> within #ifdef CONFIG_SPL_SPI_LOAD

  Ok, i will move the common changes to a static function, and call
  it from both the nand and spi load cases. Or, should i wait till a
  consensus is drawn on whether to enable this feature for spl images
  also. In case this is not needed for spl, then we don't need this
  change.

-sughosh
Albert ARIBAUD - Feb. 28, 2012, 9:55 p.m.
Le 27/02/2012 13:02, Sughosh Ganu a écrit :

>> When I asked Heiko Schocher a few month ago why he defined putc and
>> puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could
>> not use LIBCOMMON due to size limitations for the SPL. So I guess that
>> this board will not be able to use the generic relocation functions,
>> unless the SPL is smaller than 16kB, right? Simon's patchset will
>> break this board then, right?
>
>    That is exactly what i reported in one of the threads in response to
>    addition of libcommon and libgeneric to the hawkboard's spl. In
>    fact, this might cause problems on quite a few boards with spl size
>    restrictions. I am not sure, whether the generic relocation feature
>    should be turned on by default on all boards or should be a config
>    option -- at least for the spl builds. Another option would be to
>    move it to a place where it is not needed to compile in the entire
>    libcommon/libgeneric support that is not needed for the generic
>    relocation code. I think that would help us keep the generic
>    relocation without the size bloat that we see right now.
>
>    http://lists.denx.de/pipermail/u-boot/2012-February/118567.html

Sorry for appearing dumb, but can someone explain to me how SPL relates 
to relocation in the first place? I thought SPL was meant to be a 
preloader for the full(er) U-boot, small enough to be loaded by some 
SoCs' ROM code and possibly even to fit in SRAM. Why does it need 
relocation? And if it does not, how come it is affected by a rework of 
the relocation feature? I really would like a heads-up on this.

Amicalement,
Scott Wood - Feb. 28, 2012, 10:03 p.m.
On 02/28/2012 03:55 PM, Albert ARIBAUD wrote:
> Le 27/02/2012 13:02, Sughosh Ganu a écrit :
> 
>>> When I asked Heiko Schocher a few month ago why he defined putc and
>>> puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could
>>> not use LIBCOMMON due to size limitations for the SPL. So I guess that
>>> this board will not be able to use the generic relocation functions,
>>> unless the SPL is smaller than 16kB, right? Simon's patchset will
>>> break this board then, right?
>>
>>    That is exactly what i reported in one of the threads in response to
>>    addition of libcommon and libgeneric to the hawkboard's spl. In
>>    fact, this might cause problems on quite a few boards with spl size
>>    restrictions. I am not sure, whether the generic relocation feature
>>    should be turned on by default on all boards or should be a config
>>    option -- at least for the spl builds. Another option would be to
>>    move it to a place where it is not needed to compile in the entire
>>    libcommon/libgeneric support that is not needed for the generic
>>    relocation code. I think that would help us keep the generic
>>    relocation without the size bloat that we see right now.
>>
>>    http://lists.denx.de/pipermail/u-boot/2012-February/118567.html
> 
> Sorry for appearing dumb, but can someone explain to me how SPL relates
> to relocation in the first place? I thought SPL was meant to be a
> preloader for the full(er) U-boot, small enough to be loaded by some
> SoCs' ROM code and possibly even to fit in SRAM. Why does it need
> relocation? And if it does not, how come it is affected by a rework of
> the relocation feature? I really would like a heads-up on this.

SPL may need relocation to vacate a buffer that will be used for further
I/O, such as on Freescale PPC (NAND boot execution begins in the NAND
controller's SRAM), or possibly to allow the memory map to be
transformed to what the final image is going to expect, etc.

Even in cases where the SPL text itself doesn't really need to move, you
may need some other things that typically happen along with relocation,
such as moving the stack to a larger memory after that memory is
initialized, zeroing BSS, etc.

-Scott
Simon Glass - March 3, 2012, 8:22 p.m.
Hi,

On Mon, Feb 27, 2012 at 4:02 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> hi Christian,
>
> On Mon Feb 27, 2012 at 12:37:16PM +0100, Christian Riesch wrote:
>> Hi Sughosh,
>>
>> On Mon, Feb 27, 2012 at 11:56 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> > hi Christian,
>> >
>> > On Mon Feb 27, 2012 at 11:39:42AM +0100, Christian Riesch wrote:
>> >> Hi,
>> >>
>> >> On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> >
>> > <snip>
>> >
>> >> >> >>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
>> >> >> >>  1 files changed, 2 insertions(+), 0 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> >> >> index b1eff26..2861907 100644
>> >> >> >> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> >> >> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
>> >> >> >> @@ -32,10 +32,12 @@
>> >> >> >>
>> >> >> >>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> >> >> >>
>> >> >> >> +#ifdef CONFIG_SPL_SPI_LOAD
>> >> >> >>  DECLARE_GLOBAL_DATA_PTR;
>> >> >> >>  /* Define global data structure pointer to it*/
>> >> >> >>  static gd_t gdata __attribute__ ((section(".data")));
>> >> >> >>  static bd_t bdata __attribute__ ((section(".data")));
>> >> >> >> +#endif
>> >
>> > <snip>
>> >
>> >> >> >  Can you specify which boards you get this warning for. With your
>> >> >> >  patch to add libcommon to hawkboard's spl image, this is now also
>> >> >> >  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.
>> >>
>> >> Simon's patch is for the hawkboard, since due to another patch in his
>> >> patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board
>> >> that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did
>> >> not check the rest of your patchset, why do you need LIBCOMMON on the
>> >> hawkboard at all?)
>> >
>> >  LIBCOMMON is now needed as the generic relocation based functions
>> >  are part of the libcommon.o, which are being enabled in the same
>> >  patchset for all arm boards. So if i understand correct, all arm
>> >  board based spl's now need libcommon and libgeneric.
>> >
>> >  The only thing i see is that libcommon and libgeneric are not
>> >  defined for cam_enc_4xx board which uses spl, and this patchset does
>> >  not add it either. Not sure whether it got missed.
>>
>> When I asked Heiko Schocher a few month ago why he defined putc and
>> puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could
>> not use LIBCOMMON due to size limitations for the SPL. So I guess that
>> this board will not be able to use the generic relocation functions,
>> unless the SPL is smaller than 16kB, right? Simon's patchset will
>> break this board then, right?

If it pushes it over 16KB, although it's not clear that it will. So
far I haven't seen this and I would hope that builds would fail in
this case (normally I got a 'cannot move program counter backwards'
error).

But clearly putting this code in spl.c is a bit ugly because it
hard-codes the driver and duplicates code in common/console.c:

void putc(char c)
{
	if (c == '\n')
		NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), '\r');

	NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), c);
}

Can someone please check what is the maximum SPL size on one of these
boards? Generic relocation increases the spl from 5.9KB to 10KB. Is
there an 8KB limit?

One option if this really is a problem is to move common/reloc.c to
lib/reloc.c, although there isn't really a huge amount of
justification for that given that it is U-Boot code. Perhaps we could
claim that it is a relocation library and not U-Boot-specific?

>
>  That is exactly what i reported in one of the threads in response to
>  addition of libcommon and libgeneric to the hawkboard's spl. In
>  fact, this might cause problems on quite a few boards with spl size
>  restrictions. I am not sure, whether the generic relocation feature
>  should be turned on by default on all boards or should be a config
>  option -- at least for the spl builds. Another option would be to
>  move it to a place where it is not needed to compile in the entire
>  libcommon/libgeneric support that is not needed for the generic
>  relocation code. I think that would help us keep the generic
>  relocation without the size bloat that we see right now.
>
>  http://lists.denx.de/pipermail/u-boot/2012-February/118567.html

Well it would be nice to find out what the limit is. I might be able
to squeeze it a bit more even in common. But failing that the putc()
hack and lib/ might be the most expedient solution at this stage.

>
> <snip>
>
>> >> void board_init_r(gd_t *id, ulong dummy)
>> >> {
>> >> #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> >>         mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN,
>> >>                         CONFIG_SYS_MALLOC_LEN);
>> >
>> >  Can you please explain why we need the mem_malloc_init. I did not
>> >  include this, and spl boots up just fine on my board.
>> >
>>
>> malloc is required for the SPI code only, so you could also put it
>> within #ifdef CONFIG_SPL_SPI_LOAD
>
>  Ok, i will move the common changes to a static function, and call
>  it from both the nand and spi load cases. Or, should i wait till a
>  consensus is drawn on whether to enable this feature for spl images
>  also. In case this is not needed for spl, then we don't need this
>  change.

My series removes the old relocation code, so it really does need to
work for SPL also. I have put a bit of effort in here to tidy this up
and I don't think it is hard to resolve this last problem.

I will investigate this a bit more and then suggest a solution.

Regards,
Simon

>
> -sughosh
Simon Glass - March 3, 2012, 8:29 p.m.
Hi,

On Sat, Mar 3, 2012 at 12:22 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On Mon, Feb 27, 2012 at 4:02 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> hi Christian,
>>
>> On Mon Feb 27, 2012 at 12:37:16PM +0100, Christian Riesch wrote:
>>> Hi Sughosh,
>>>
>>> On Mon, Feb 27, 2012 at 11:56 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>>> > hi Christian,
>>> >
>>> > On Mon Feb 27, 2012 at 11:39:42AM +0100, Christian Riesch wrote:
>>> >> Hi,
>>> >>
>>> >> On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>>> >
>>> > <snip>
>>> >
>>> >> >> >>  arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
>>> >> >> >>  1 files changed, 2 insertions(+), 0 deletions(-)
>>> >> >> >>
>>> >> >> >> diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
>>> >> >> >> index b1eff26..2861907 100644
>>> >> >> >> --- a/arch/arm/cpu/arm926ejs/davinci/spl.c
>>> >> >> >> +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
>>> >> >> >> @@ -32,10 +32,12 @@
>>> >> >> >>
>>> >> >> >>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>>> >> >> >>
>>> >> >> >> +#ifdef CONFIG_SPL_SPI_LOAD
>>> >> >> >>  DECLARE_GLOBAL_DATA_PTR;
>>> >> >> >>  /* Define global data structure pointer to it*/
>>> >> >> >>  static gd_t gdata __attribute__ ((section(".data")));
>>> >> >> >>  static bd_t bdata __attribute__ ((section(".data")));
>>> >> >> >> +#endif
>>> >
>>> > <snip>
>>> >
>>> >> >> >  Can you specify which boards you get this warning for. With your
>>> >> >> >  patch to add libcommon to hawkboard's spl image, this is now also
>>> >> >> >  needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.
>>> >>
>>> >> Simon's patch is for the hawkboard, since due to another patch in his
>>> >> patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board
>>> >> that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did
>>> >> not check the rest of your patchset, why do you need LIBCOMMON on the
>>> >> hawkboard at all?)
>>> >
>>> >  LIBCOMMON is now needed as the generic relocation based functions
>>> >  are part of the libcommon.o, which are being enabled in the same
>>> >  patchset for all arm boards. So if i understand correct, all arm
>>> >  board based spl's now need libcommon and libgeneric.
>>> >
>>> >  The only thing i see is that libcommon and libgeneric are not
>>> >  defined for cam_enc_4xx board which uses spl, and this patchset does
>>> >  not add it either. Not sure whether it got missed.
>>>
>>> When I asked Heiko Schocher a few month ago why he defined putc and
>>> puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could
>>> not use LIBCOMMON due to size limitations for the SPL. So I guess that
>>> this board will not be able to use the generic relocation functions,
>>> unless the SPL is smaller than 16kB, right? Simon's patchset will
>>> break this board then, right?
>
> If it pushes it over 16KB, although it's not clear that it will. So
> far I haven't seen this and I would hope that builds would fail in
> this case (normally I got a 'cannot move program counter backwards'
> error).
>
> But clearly putting this code in spl.c is a bit ugly because it
> hard-codes the driver and duplicates code in common/console.c:
>
> void putc(char c)
> {
>        if (c == '\n')
>                NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), '\r');
>
>        NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), c);
> }
>
> Can someone please check what is the maximum SPL size on one of these
> boards? Generic relocation increases the spl from 5.9KB to 10KB. Is
> there an 8KB limit?
>
> One option if this really is a problem is to move common/reloc.c to
> lib/reloc.c, although there isn't really a huge amount of
> justification for that given that it is U-Boot code. Perhaps we could
> claim that it is a relocation library and not U-Boot-specific?
>
>>
>>  That is exactly what i reported in one of the threads in response to
>>  addition of libcommon and libgeneric to the hawkboard's spl. In
>>  fact, this might cause problems on quite a few boards with spl size
>>  restrictions. I am not sure, whether the generic relocation feature
>>  should be turned on by default on all boards or should be a config
>>  option -- at least for the spl builds. Another option would be to
>>  move it to a place where it is not needed to compile in the entire
>>  libcommon/libgeneric support that is not needed for the generic
>>  relocation code. I think that would help us keep the generic
>>  relocation without the size bloat that we see right now.
>>
>>  http://lists.denx.de/pipermail/u-boot/2012-February/118567.html
>
> Well it would be nice to find out what the limit is. I might be able
> to squeeze it a bit more even in common. But failing that the putc()
> hack and lib/ might be the most expedient solution at this stage.
>
>>
>> <snip>
>>
>>> >> void board_init_r(gd_t *id, ulong dummy)
>>> >> {
>>> >> #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>>> >>         mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN,
>>> >>                         CONFIG_SYS_MALLOC_LEN);
>>> >
>>> >  Can you please explain why we need the mem_malloc_init. I did not
>>> >  include this, and spl boots up just fine on my board.
>>> >
>>>
>>> malloc is required for the SPI code only, so you could also put it
>>> within #ifdef CONFIG_SPL_SPI_LOAD
>>
>>  Ok, i will move the common changes to a static function, and call
>>  it from both the nand and spi load cases. Or, should i wait till a
>>  consensus is drawn on whether to enable this feature for spl images
>>  also. In case this is not needed for spl, then we don't need this
>>  change.
>
> My series removes the old relocation code, so it really does need to
> work for SPL also. I have put a bit of effort in here to tidy this up
> and I don't think it is hard to resolve this last problem.
>
> I will investigate this a bit more and then suggest a solution.

Actually further to this it seems that it is just that raise() is
calling printf(). Seems like we can punt this unless anyone has
objections. Alternatively we could use puts() and just print a general
message. I will send a patch.

Regards,
Simon

>
> Regards,
> Simon
>
>>
>> -sughosh

Patch

diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c
index b1eff26..2861907 100644
--- a/arch/arm/cpu/arm926ejs/davinci/spl.c
+++ b/arch/arm/cpu/arm926ejs/davinci/spl.c
@@ -32,10 +32,12 @@ 
 
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 
+#ifdef CONFIG_SPL_SPI_LOAD
 DECLARE_GLOBAL_DATA_PTR;
 /* Define global data structure pointer to it*/
 static gd_t gdata __attribute__ ((section(".data")));
 static bd_t bdata __attribute__ ((section(".data")));
+#endif
 
 #else