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

[Kever Yang <kever.yang@rock-chips.com>]

Hi


On 04/06/2017 12:40 AM, Masahiro Yamada wrote:
> 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.

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
>
>