[U-Boot] [PATCH v2 06/11] common/board_f: factor out reserve_stacks

[Simon Glass]

Hi Andreas,

On 29 January 2015 at 16:07, Andreas Bießmann
<andreas.devel at googlemail.com> wrote:
> Require each architecture to provide an arch_reserve_stacks() function to
> setup the required stacks for the architecture.
>
> Signed-off-by: Andreas Bießmann <andreas.devel at googlemail.com>
> ---
> This patch is _not_ fully compile tested for all ppc/arm boards!
>
> Changes in v2:
> - new since v1
>
> Changes in v1: None
>
>  arch/arm/lib/Makefile     |    1 +
>  arch/arm/lib/stack.c      |   44 +++++++++++++++++++++++++++++++++++++++++++
>  arch/powerpc/lib/Makefile |    1 +
>  arch/powerpc/lib/stack.c  |   33 ++++++++++++++++++++++++++++++++
>  common/board_f.c          |   46 +--------------------------------------------
>  include/common.h          |    9 +++++++++
>  6 files changed, 89 insertions(+), 45 deletions(-)
>  create mode 100644 arch/arm/lib/stack.c
>  create mode 100644 arch/powerpc/lib/stack.c
>
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index d74e4b8..da8ed72 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -35,6 +35,7 @@ endif
>  obj-$(CONFIG_SEMIHOSTING) += semihosting.o
>
>  obj-y  += sections.o
> +obj-y  += stack.o
>  ifdef CONFIG_ARM64
>  obj-y  += gic_64.o
>  obj-y  += interrupts_64.o
> diff --git a/arch/arm/lib/stack.c b/arch/arm/lib/stack.c
> new file mode 100644
> index 0000000..b1050a5
> --- /dev/null
> +++ b/arch/arm/lib/stack.c
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (c) 2015 Andreas Bießmann <andreas.devel at googlemail.com>
> + *
> + * Copyright (c) 2011 The Chromium OS Authors.
> + * (C) Copyright 2002-2006
> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> + *
> + * (C) Copyright 2002
> + * Sysgo Real-Time Solutions, GmbH <www.elinos.com>
> + * Marius Groeger <mgroeger at sysgo.de>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +#include <common.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int arch_reserve_stacks(void)
> +{
> +#ifdef CONFIG_SPL_BUILD
> +       gd->start_addr_sp -= 128;       /* leave 32 words for abort-stack */
> +       gd->irq_sp = gd->start_addr_sp;
> +#else
> +       /* setup stack pointer for exceptions */
> +       gd->start_addr_sp -= 16;
> +       gd->start_addr_sp &= ~0xf;
> +       gd->irq_sp = gd->start_addr_sp;
> +
> +# if !defined(CONFIG_ARM64)
> +#  ifdef CONFIG_USE_IRQ
> +       gd->start_addr_sp -= (CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ);
> +       debug("Reserving %zu Bytes for IRQ stack at: %08lx\n",
> +             CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ, gd->start_addr_sp);
> +
> +       /* 8-byte alignment for ARM ABI compliance */
> +       gd->start_addr_sp &= ~0x07;
> +#  endif
> +       /* leave 3 words for abort-stack, plus 1 for alignment */
> +       gd->start_addr_sp -= 16;
> +# endif
> +#endif
> +
> +       return 0;
> +}
> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index 0f62982..05b22bb 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -40,6 +40,7 @@ obj-y += extable.o
>  obj-y  += interrupts.o
>  obj-$(CONFIG_CMD_KGDB) += kgdb.o
>  obj-$(CONFIG_CMD_IDE) += ide.o
> +obj-y  += stack.o
>  obj-y  += time.o
>
>  # Don't include the MPC5xxx special memcpy into the
> diff --git a/arch/powerpc/lib/stack.c b/arch/powerpc/lib/stack.c
> new file mode 100644
> index 0000000..3a6c91a
> --- /dev/null
> +++ b/arch/powerpc/lib/stack.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (c) 2015 Andreas Bießmann <andreas.devel at googlemail.com>
> + *
> + * Copyright (c) 2011 The Chromium OS Authors.
> + * (C) Copyright 2002-2006
> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> + *
> + * (C) Copyright 2002
> + * Sysgo Real-Time Solutions, GmbH <www.elinos.com>
> + * Marius Groeger <mgroeger at sysgo.de>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +#include <common.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int arch_reserve_stacks(void)
> +{
> +       ulong *s;
> +
> +       /* setup stack pointer for exceptions */
> +       gd->start_addr_sp -= 16;
> +       gd->start_addr_sp &= ~0xf;
> +       gd->irq_sp = gd->start_addr_sp;
> +
> +       /* Clear initial stack frame */
> +       s = (ulong *)gd->start_addr_sp;
> +       *s = 0; /* Terminate back chain */
> +       *++s = 0; /* NULL return address */
> +
> +       return 0;
> +}
> diff --git a/common/board_f.c b/common/board_f.c
> index 215108b..03f1529 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -573,50 +573,6 @@ static int reserve_fdt(void)
>         return 0;
>  }
>
> -static int reserve_stacks(void)
> -{
> -#ifdef CONFIG_SPL_BUILD
> -# ifdef CONFIG_ARM
> -       gd->start_addr_sp -= 128;       /* leave 32 words for abort-stack */
> -       gd->irq_sp = gd->start_addr_sp;
> -# endif
> -#else
> -# ifdef CONFIG_PPC
> -       ulong *s;
> -# endif
> -
> -       /* setup stack pointer for exceptions */
> -       gd->start_addr_sp -= 16;
> -       gd->start_addr_sp &= ~0xf;
> -       gd->irq_sp = gd->start_addr_sp;
> -
> -       /*
> -        * Handle architecture-specific things here
> -        * TODO(sjg at chromium.org): Perhaps create arch_reserve_stack()
> -        * to handle this and put in arch/xxx/lib/stack.c
> -        */
> -# if defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
> -#  ifdef CONFIG_USE_IRQ
> -       gd->start_addr_sp -= (CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ);
> -       debug("Reserving %zu Bytes for IRQ stack at: %08lx\n",
> -               CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ, gd->start_addr_sp);
> -
> -       /* 8-byte alignment for ARM ABI compliance */
> -       gd->start_addr_sp &= ~0x07;
> -#  endif
> -       /* leave 3 words for abort-stack, plus 1 for alignment */
> -       gd->start_addr_sp -= 16;
> -# elif defined(CONFIG_PPC)
> -       /* Clear initial stack frame */
> -       s = (ulong *) gd->start_addr_sp;
> -       *s = 0; /* Terminate back chain */
> -       *++s = 0; /* NULL return address */
> -# endif /* Architecture specific code */
> -
> -       return 0;
> -#endif
> -}

It's nice to see this clean-up.

This does force every arch to provide this function. For some
architectures I suspect that this will be enough:

       gd->start_addr_sp -= 16;
       gd->start_addr_sp &= ~0xf;

So I think it might be better to put that code in reserve_stacks() and
then call a weak arch_reserve_stacks() after that.

> -
>  static int display_new_sp(void)
>  {
>         debug("New Stack Pointer is: %08lx\n", gd->start_addr_sp);
> @@ -977,7 +933,7 @@ static init_fnc_t init_sequence_f[] = {
>         reserve_global_data,
>         reserve_fdt,
>         reserve_arch,
> -       reserve_stacks,
> +       arch_reserve_stacks,
>         setup_dram_config,
>         show_dram_config,
>  #ifdef CONFIG_PPC
> diff --git a/include/common.h b/include/common.h
> index 4b3e0d3..dcf8255 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -251,6 +251,15 @@ int update_flash_size(int flash_size);
>  int arch_early_init_r(void);
>
>  /**
> + * Reserve all necessary stacks
> + *
> + * This is used in generic board init sequence in common/board_f.c. Each
> + * architecture should provide this function and set the gd->start_addr_sp and
> + * gd->irq_sp accordingly.

Should add a little more detail here:

On entry gd->start_addr_sp is pointing to the suggested top of the stack
On exit this should be updated to the position of the stack. The stack
pointer will be set to this later
gd->irq_sp should be set if this is needed by the arch.

also @return 0 if no error

> + */
> +int arch_reserve_stacks(void);
> +
> +/**
>   * Show the DRAM size in a board-specific way
>   *
>   * This is used by boards to display DRAM information in their own way.
> --
> 1.7.10.4
>