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

[Bryan O'Donoghue]

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

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

I leave it to Kever to decide next steps for his patches.

---
bod