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

[Dr. Philipp Tomsich]

Bryan,

> On 21 Feb 2018, at 04:27, Bryan O'Donoghue <bryan.odonoghue at linaro.org> wrote:
> 
> On 19/02/18 15:44, Tom Rini wrote:
>> On Fri, Feb 02, 2018 at 04:56:57PM +0100, Dr. Philipp Tomsich wrote:
>>> Bryan,
>>> 
>>>> On 2 Feb 2018, at 16:37, Bryan O'Donoghue <bryan.odonoghue at linaro.org> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 02/02/18 15:02, Dr. Philipp Tomsich wrote:
>>>>> Where do we stand on this: can we reuse IH_TYPE_TEE, will be use IH_TYPE_OPTEE or will there be a new IH_TYPE_OPTEE_SPL?
>>>> 
>>>> I think because you aren't doing anything different with the image type you can reuse IH_TYPE_TEE
>>>> 
>>>> This
>>>> 
>>>> +#if CONFIG_IS_ENABLED(OPTEE)
>>>> +	case IH_OS_OP_TEE:
>>>> +		debug("Jumping to U-Boot via OP-TEE\n");
>>>> +		spl_optee_entry(NULL, NULL, spl_image->fdt_addr,
>>>> +				(void *)spl_image.entry_point);
>>>> +		break;
>>>> +#endif
>>>> 
>>>> could just as easily be this
>>>> 
>>>> +#if CONFIG_IS_ENABLED(OPTEE)
>>>> +	case IH_TYPE_TEE:
>>>> +		debug("Jumping to U-Boot via OP-TEE\n");
>>>> +		spl_optee_entry(NULL, NULL, spl_image->fdt_addr,
>>>> +				(void *)spl_image.entry_point);
>>>> +		break;
>>>> +#endif
>>>> 
>>>> should be a matter of just replacing the call to mkimage to use
>>>> 
>>>> mkimage -A arm -T tee
>>>> 
>>>> instead of
>>>> 
>>>> mkimage -A arm -T optee
>>>> 
>>>> and the suggested change above.. case IH_OS_OP_TEE -> case IH_TYPE_TEE
>>> 
>>> Thanks for summarising your suggestion.
>>> 
>>> However, my mail was intended to test the waters to see what the consensus was.
>>> A it appears that none has yet emerged between all the involved parties (including
>>> our colleagues from TI that had also chimed in on the discussion).
>>> So for now, I’ll sit back and wait until some sort of consensus (or at least a majority
>>> for one solution or the other) emerges.
>>> 
>>> Personally, I am not happy with having a ‘tee’ and an ‘optee’ both refering to OP-TEE
>>> and the upstream OP-TEE documentation suggesting that their envisioned boot
>>> process was to boot through the OP-TEE (i.e. what the ‘tee’ image type does).
>>> However, with the ‘tee’ image type already being defined, we seem to have already
>>> backed ourselves into a situation where the naming is non-intuitive.
>> Alright, I'm a little confused here.  I guess first, there's a
>> disconnect in upstream OP-TEE land about how 32bit ARM should be done?
>> I guess we have three different implemented and upstreamed flows:
>> - SPL->U-Boot->OP-TEE->U-Boot->Linux
>> - SPL->U-Boot->OP-TEE->Linux
>> - SPL->OP-TEE->U-Boot->Linux
> 
> I'd call that
> 
> - SPL/BootROM->U-Boot->OP-TEE->U-Boot->Linux
> - SPL/BootROM->U-Boot->OP-TEE->Linux
> - SPL/BootROM->OP-TEE->U-Boot->Linux
> 
> But yes - fundamentally your flow is correct.
> 
>> And in this last case we have a combined image that is passed from SPL
>> to OP-TEE.
>> Since all 3 of these flows are in upstream OP-TEE, we need to support
>> them all. 
> 
> Agreed.
> 
>> The biggest constraint is that we have the first flow already
>> in and named "tee" (my fault, I should have made sure everyone would use
>> the same flow).  So we need to have descriptive enough names for the
>> other flows that we're going to add so that it's clear what's what.  How
>> about "tee-standalone" for "U-Boot starts OP-TEE, and is done"
> 
>> "tee-combo" for "SPL gives OP-TEE an image to deal with".  This could
>> even in theory I suspect be SPL gives OP-TEE a Linux kernel to boot.
>> I'm also happy to hear better suffixes but I don't want "tee" and
>> "optee".  And if we can cover two flows under the same name, that's good
>> too, we just need to name the last flow "tee-something".  Thanks all!
> 
> Yes I take your point "tee" and "optee" are the opposite of descriptive names.
> 
> - SPL/BootROM->U-Boot->OP-TEE->U-Boot->Linux
>  Currently called "tee" - has type IH_TEE - maintained as is for
>  compatibility - deserves some documentation.
> 
> - SPL/BootROM->U-Boot->OP-TEE->Linux
>  New type "tee-standalone" this would be the type I've proposed
>  in my patch set. New type IH_TYPE_TEE_STANDALONE
> 
> - SPL/BootROM->OP-TEE->U-Boot->Linux
>  New type "tee-combo" this would be Kever's type IH_TYPE_TEE_COMBO
> 
> I've suggested to Kever that he doesn't actually need a separate type (though I could be wrong).

My guess is that TEE_COMBO should cover Kever’s case.
We’ll probably need to wait and see if he encounters issues with this, once
he’s back after the Chinese New Year holiday.

> I resend my previous patchset renaming "optee" to "tee-standalone" and add the type IH_TYPE_TEE_STANDALONE.

As it looks as you’ll have your series revised first—could you add some
documentation that summarises the various the ways of how OPTEE
and some info on your use-case?

I’d then make sure (prior to merging) that Kever provides an update
against this file that provides more info on his use-case and implementation.

Thanks,
Philipp.