All of lore.kernel.org
 help / color / mirror / Atom feed
From: Masahiro Yamada <yamada.masahiro@socionext.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v3] spl: add support to booting with ATF
Date: Thu, 6 Apr 2017 01:40:22 +0900	[thread overview]
Message-ID: <CAK7LNARwtVAoL9TjVCZ-ExHwXB6r4aivhJCwvH18yuthEYJCmg@mail.gmail.com> (raw)
In-Reply-To: <1491368732-11546-2-git-send-email-kever.yang@rock-chips.com>

Hi.


2017-04-05 14:05 GMT+09:00 Kever Yang <kever.yang@rock-chips.com>:

> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -670,6 +670,20 @@ config SPL_YMODEM_SUPPORT
>           means of transmitting U-Boot over a serial line for using in SPL,
>           with a checksum to ensure correctness.
>
> +config SPL_ATF_SUPPORT
> +       bool "Support ARM Trusted Firmware"
> +       depends on SPL && ARM64
> +       help
> +         ATF(ARM Trusted Firmware) is a component for ARM arch64 which which
> +         is loaded by SPL(which is considered as BL2 in ATF terminology).
> +         More detail at: https://github.com/ARM-software/arm-trusted-firmware

This statement may not be precise.
ATF supports Aarch32 as well as Aarch64.


I thik this is just U-Boot side matter.
As far as I understood from the U-Boot directory structure
(I only see Aarch64 code in arch/arm/cpu/armv8),
U-Boot does not expect 32bit ARM code in ARMv8 generation
(such as Cortex-A32, Coretex-A35, ...).
As for U-Boot, ARM64 == ARMV8
unless I am missing something.



> +config SPL_ATF_TEXT_BASE
> +       depends on SPL_ATF_SUPPORT
> +       hex "ATF TEXT BASE addr"

hex "ATF BL31 base address"



> +       help
> +         This is the base address in memory for ATF text and entry point.

I'd like you to mention "BL31" explicitly
because there are many images in ATF.



>  config TPL_ENV_SUPPORT
>         bool "Support an environment"
>         depends on TPL
> diff --git a/common/spl/Makefile b/common/spl/Makefile
> index 1933cbd..b3b34d6 100644
> --- a/common/spl/Makefile
> +++ b/common/spl/Makefile
> @@ -20,6 +20,7 @@ endif
>  obj-$(CONFIG_SPL_UBI) += spl_ubi.o
>  obj-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
>  obj-$(CONFIG_SPL_MMC_SUPPORT) += spl_mmc.o
> +obj-$(CONFIG_SPL_ATF_SUPPORT) += spl_atf.o
>  obj-$(CONFIG_SPL_USB_SUPPORT) += spl_usb.o
>  obj-$(CONFIG_SPL_FAT_SUPPORT) += spl_fat.o
>  obj-$(CONFIG_SPL_EXT_SUPPORT) += spl_ext.o
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 26bc9ef..7041e1b 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -374,6 +374,9 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>               gd->malloc_ptr / 1024);
>  #endif
>
> +       if (IS_ENABLED(CONFIG_SPL_ATF_SUPPORT))
> +               bl31_entry();
> +
>         debug("loaded - jumping to U-Boot...\n");
>         spl_board_prepare_for_boot();
>         jump_to_image_no_args(&spl_image);




        switch (spl_image.os) {
        case IH_OS_U_BOOT:
                debug("Jumping to U-Boot\n");
                break;
#ifdef CONFIG_SPL_OS_BOOT
        case IH_OS_LINUX:
                debug("Jumping to Linux\n");
                spl_board_prepare_for_linux();
                jump_to_image_linux(&spl_image,
                                    (void *)CONFIG_SYS_SPL_ARGS_ADDR);
#endif
        default:
                debug("Unsupported OS image.. Jumping nevertheless..\n");
        }


Which debug message is printed
before SPL jumps to BL31?

Jumping to U-Boot or Linux or Unsupported OS?








> +void bl31_entry(void)
> +{
> +       struct bl31_params_t *bl31_params;
> +       void (*entry)(struct bl31_params_t *params, void *plat_params) = NULL;
> +
> +       bl31_params = bl2_plat_get_bl31_params();
> +       entry = (void *)CONFIG_SPL_ATF_TEXT_BASE;
> +
> +       raw_write_daif(SPSR_EXCEPTION_MASK);
> +       dcache_disable();
> +
> +       entry(bl31_params, NULL);
> +}

I guess SPL is supposed to load at least BL31 and BL33(=U-Boot full)
because BL31 does not have a facility to load images from non-volatile devices.

I can not see how to do it from this patch.

Relying on Andre's work for multi images in FIT?
http://patchwork.ozlabs.org/patch/745827/


I am not tracking how they are related, though.




> +/*******************************************************************************
> + * Structure used for telling the next BL how much of a particular type of
> + * memory is available for its use and how much is already used.
> + ******************************************************************************/
> +struct aapcs64_params_t {


Please do not add _t suffix for structure.


> +       unsigned long arg0;
> +       unsigned long arg1;
> +       unsigned long arg2;
> +       unsigned long arg3;
> +       unsigned long arg4;
> +       unsigned long arg5;
> +       unsigned long arg6;
> +       unsigned long arg7;
> +};



In ATF, every structure has a shorthand
typedef with _t suffix, like this:

typedef struct aapcs64_params {
        u_register_t arg0;
        u_register_t arg1;
        u_register_t arg2;
        u_register_t arg3;
        u_register_t arg4;
        u_register_t arg5;
        u_register_t arg6;
        u_register_t arg7;
} aapcs64_params_t;


This is bad habit anyway, so
no new typedef:s, no _t suffix in U-boot.





> +/***************************************************************************
> + * This structure provides version information and the size of the
> + * structure, attributes for the structure it represents
> + ***************************************************************************/
> +struct param_header_t {

Please rename to
struct param_header

Likewise for the others.



> +/*******************************************************************************
> + * This structure represents the superset of information that is passed to
> + * BL31, e.g. while passing control to it from BL2, bl31_params
> + * and other platform specific params
> + ******************************************************************************/
> +struct bl2_to_bl31_params_mem_t {
> +       struct bl31_params_t bl31_params;
> +       struct atf_image_info_t bl31_image_info;
> +       struct atf_image_info_t bl32_image_info;
> +       struct atf_image_info_t bl33_image_info;
> +       struct entry_point_info_t bl33_ep_info;
> +       struct entry_point_info_t bl32_ep_info;
> +       struct entry_point_info_t bl31_ep_info;
> +};
> +

This is old interface. (LOAD_IMAGE_V1).


As Dan explained, this was superseded by struct bl_params and bl_params_node.


I'd recommend you to implement LOAD_IMAGE_V2.



-- 
Best Regards
Masahiro Yamada

  parent reply	other threads:[~2017-04-05 16:40 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-05  5:05 [U-Boot] [PATCH v3 0/1] arm64: enable SPL with ATF support Kever Yang
2017-04-05  5:05 ` [U-Boot] [PATCH v3] spl: add support to booting with ATF Kever Yang
2017-04-05  5:35   ` Masahiro Yamada
2017-04-05  8:25     ` Dan Handley
2017-05-05  3:07       ` Kever Yang
2017-04-05 16:40   ` Masahiro Yamada [this message]
2017-05-05  3:33     ` Kever Yang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAK7LNARwtVAoL9TjVCZ-ExHwXB6r4aivhJCwvH18yuthEYJCmg@mail.gmail.com \
    --to=yamada.masahiro@socionext.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.