> On 4 Jan 2018, at 04:51, Kever Yang <kever.y...@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.y...@rock-chips.com>
>>
>> Reviewed-by: Philipp Tomsich <philipp.toms...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
