[U-Boot] [PATCH v3] spl: add support to booting with ATF

[Masahiro Yamada]

Hi.


2017-04-05 14:05 GMT+09:00 Kever Yang <kever.yang at 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