From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kever Yang Date: Fri, 5 May 2017 11:33:18 +0800 Subject: [U-Boot] [PATCH v3] spl: add support to booting with ATF In-Reply-To: References: <1491368732-11546-1-git-send-email-kever.yang@rock-chips.com> <1491368732-11546-2-git-send-email-kever.yang@rock-chips.com> Message-ID: <930ac82b-8008-6703-ae04-096c5ca6a40b@rock-chips.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi On 04/06/2017 12:40 AM, Masahiro Yamada wrote: > Hi. > > > 2017-04-05 14:05 GMT+09:00 Kever Yang : > >> --- 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. I would like to leave it as is now, people can remove this if there is aarch32 want to support ATF, at least I didn't see any of them now. > > >> +config SPL_ATF_TEXT_BASE >> + depends on SPL_ATF_SUPPORT >> + hex "ATF TEXT BASE addr" > hex "ATF BL31 base address" Will fix, thanks. > > > >> + 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. Will fix, thanks. > > > >> 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? It's Jumping to U-Boot now, how about add one message before jump to BL31 like this: debug("loaded - jumping to U-Boot via ATF BL31.\n"); > > > > > > > >> +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/ Yes, I use Andre's patch to load multi image in FIT, this can handle flexible number of ATF target. > > > 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. OK, will do it. > > > >> +/******************************************************************************* >> + * 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. Yes, there is new IMAGE_V2 available now, but I would like to support V1 first, because there already have bl31 binary release from Rockchip which is V1 format. Thanks, - Kever > >