[U-Boot] [U-Boot, v3, 07/11] spl: add support to booting with OP-TEE

Thu Jan 4 10:48:04 UTC 2018

> On 4 Jan 2018, at 04:51, Kever Yang <kever.yang at rock-chips.com> wrote:
> 
> 
> 
> On 01/03/2018 01:20 AM, Philipp Tomsich wrote:
>> 
>> 
>> On Tue, 19 Dec 2017, Kever Yang wrote:
>> 
>>> OP-TEE is an open source trusted OS, in armv7, its loading and
>>> running are like this:
>>> loading:
>>> - SPL load both OP-TEE and U-Boot
>>> running:
>>> - SPL run into OP-TEE in secure mode;
>>> - OP-TEE run into U-Boot in non-secure mode;
>>> 
>>> More detail:
>>> https://github.com/OP-TEE/optee_os
>>> and search for 'boot arguments' for detail entry parameter in:
>>> core/arch/arm/kernel/generic_entry_a32.S
>>> 
>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>> 
>> Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>> 
>> See below for requested changed.
>> 
>>> ---
>>> 
>>> Changes in v3: None
>>> Changes in v2:
>>> - Using new image type for op-tee
>>> 
>>> common/spl/Kconfig     |  7 +++++++
>>> common/spl/Makefile    |  1 +
>>> common/spl/spl.c       |  8 ++++++++
>>> common/spl/spl_optee.S | 13 +++++++++++++
>>> include/spl.h          |  9 +++++++++
>>> 5 files changed, 38 insertions(+)
>>> create mode 100644 common/spl/spl_optee.S
>>> 
>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>>> index aef0034..c9a1ebb 100644
>>> --- a/common/spl/Kconfig
>>> +++ b/common/spl/Kconfig
>>> @@ -725,6 +725,13 @@ config SPL_ATF
>>>      is loaded by SPL(which is considered as BL2 in ATF terminology).
>>>      More detail at: https://github.com/ARM-software/arm-trusted-firmware
>>> 
>>> +config SPL_OPTEE
>>> +    bool "Support OP-TEE Trusted OS"
>>> +    depends on ARM
>>> +    help
>>> +      OP-TEE is an open source Trusted OS  which is loaded by SPL.
>>> +      More detail at: https://github.com/OP-TEE/optee_os
>>> +
>>> config TPL
>>>    bool
>>>    depends on SUPPORT_TPL
>>> diff --git a/common/spl/Makefile b/common/spl/Makefile
>>> index 9bf8a2d..9918a2e 100644
>>> --- a/common/spl/Makefile
>>> +++ b/common/spl/Makefile
>>> @@ -23,6 +23,7 @@ obj-$(CONFIG_$(SPL_TPL_)UBI) += spl_ubi.o
>>> obj-$(CONFIG_$(SPL_TPL_)NET_SUPPORT) += spl_net.o
>>> obj-$(CONFIG_$(SPL_TPL_)MMC_SUPPORT) += spl_mmc.o
>>> obj-$(CONFIG_$(SPL_TPL_)ATF) += spl_atf.o
>>> +obj-$(CONFIG_$(SPL_TPL_)OPTEE) += spl_optee.o
>>> obj-$(CONFIG_$(SPL_TPL_)USB_SUPPORT) += spl_usb.o
>>> obj-$(CONFIG_$(SPL_TPL_)FAT_SUPPORT) += spl_fat.o
>>> obj-$(CONFIG_$(SPL_TPL_)EXT_SUPPORT) += spl_ext.o
>>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>>> index 76c1963..2c8942b 100644
>>> --- a/common/spl/spl.c
>>> +++ b/common/spl/spl.c
>>> @@ -436,6 +436,12 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>>>        spl_invoke_atf(&spl_image);
>>>        break;
>>> #endif
>>> +#if CONFIG_IS_ENABLED(OPTEE)
>>> +    case IH_OS_OP_TEE:
>>> +        debug("Jumping to U-Boot via OP-TEE\n");
>>> +        spl_optee_entry(0, 0, 0, (void *)spl_image.entry_point);
>> 
>> Why are you passing '0' for "arg2, device tree address"? This should
>> be the device-tree pointer from the spl_image...
> 
> I will add dt pointer for entry next version.
>> 
>> Also: 0 is not the same as NULL ... the arguments are all defined to
>> have void* type, so 0 is not the correct type.
>> 
>>> +        break;
>>> +#endif
>>> #ifdef CONFIG_SPL_OS_BOOT
>>>    case IH_OS_LINUX:
>>>        debug("Jumping to Linux\n");
>>> @@ -450,6 +456,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>>>    debug("SPL malloc() used %#lx bytes (%ld KB)\n", gd->malloc_ptr,
>>>          gd->malloc_ptr / 1024);
>>> #endif
>>> +
>>> +    debug("loaded - jumping to U-Boot...\n");
>>> #ifdef CONFIG_BOOTSTAGE_STASH
>>>    int ret;
>>> 
>>> diff --git a/common/spl/spl_optee.S b/common/spl/spl_optee.S
>>> new file mode 100644
>>> index 0000000..4f7f8ba
>>> --- /dev/null
>>> +++ b/common/spl/spl_optee.S
>>> @@ -0,0 +1,13 @@
>>> +/*
>>> + * Copyright (C) 2017 Rockchip Electronic Co.,Ltd
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#include <linux/linkage.h>
>>> +#include <asm/assembler.h>
>>> +
>>> +ENTRY(spl_optee_entry)
>>> +    ldr lr, =CONFIG_SYS_TEXT_BASE
>>> +    mov pc, r3
>>> +ENDPROC(spl_optee_entry)
>>> diff --git a/include/spl.h b/include/spl.h
>>> index c14448b..551ce43 100644
>>> --- a/include/spl.h
>>> +++ b/include/spl.h
>>> @@ -288,6 +288,15 @@ int spl_mmc_load_image(struct spl_image_info *spl_image,
>>> void spl_invoke_atf(struct spl_image_info *spl_image);
>>> 
>>> /**
>>> + * spl_optee_entry - entry function for optee
>>> + * entry arg0, pagestore
>>> + * entry arg1, (ARMv7 standard bootarg #1)
>>> + * entry arg2, device tree address, (ARMv7 standard bootarg #2)
>>> + * entry arg3, non-secure entry address (ARMv7 bootarg #0)
>>> + */
>>> +void spl_optee_entry(void *arg0, void *arg1, void *arg2, void *arg3);
>> 
>> This doesn't look like a documentation comment for a U-Boot function.
>> Also: the purpose of arg0 and arg1 are not clear from this comment.
> 
> This is a copy from OP-TEE project, we only use the entry address now.
> https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/generic_entry_a32.S#L208 <https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/generic_entry_a32.S#L208>
You can add a back-reference to the original source (i.e. the link into the OP-TEE
GIT) as well, which may be a good idea so people know what this links up with.

However, you should reformat the comment here anyway:
(a)	U-Boot documentation should be consistent (automated tools may harvest
	the comments documenting our exposed functions)
(b)	The meaning of the arguments should be understandable to anyone reading
	the U-Boot source-code without referring to the source of OP-TEE.

Regards,
Philipp.

>>> +
>>> +/**
>>> * board_return_to_bootrom - allow for boards to continue with the boot ROM
>>> *
>>> * If a board (e.g. the Rockchip RK3368 boards) provide some