[U-Boot] [PATCH 21/24] tegra124: Implement spl_was_boot_source()

[Stephen Warren <swarren@wwwdotorg.org>]

On 05/05/2015 10:02 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 5 May 2015 at 09:54, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/04/2015 11:31 AM, Simon Glass wrote:
>>>
>>> Add an implementation of this function for Tegra.
>>
>>
>>> diff --git a/arch/arm/mach-tegra/board.c b/arch/arm/mach-tegra/board.c
>>
>>
>>> +#ifndef CONFIG_SPL_BUILD
>>> +void save_boot_params(u32 r0, u32 r1, u32 r2, u32 r3)
>>> +{
>>> +       from_spl = r0 != SPL_RUNNING_FROM_UBOOT;
>>> +       save_boot_params_ret();
>>> +}
>>> +#endif
>>
>>
>> (Using terminology from:
>> https://patchwork.ozlabs.org/patch/467771/
>> arm: spl: Enable detecting when U-Boot is started from SPL
>> )
>>
>> That doesn't look right. Surely (at least on Tegra), if the r/o U-Boot
>> chain-loads to the r/w U-Boot, then the chain-loaded U-Boot has no SPL and
>> is just the main CPU build of U-Boot.
>>
>> Hence, "SPL_RUNNING_FROM_UBOOT" seems incorrectly named, since the r/o
>> U-Boot doesn't chain to SPL but rather to U-Boot.
>
> What name do you suggest? I was trying to add a prefix indicating that
> it relates to non-SPL start-up of U-Boot.

Well, that name specifically states that it's SPL that's running, 
whereas the exact opposite is true.

Perhaps UBOOT_CHAIN_LOADED_FROM_UBOOT?

>> This approach sounds a little brittle; what happens if r0 just happens to
>> have that value. Won't the code get confused?
>
> Yes, but SPL does not set that value in r0, and we have control over this.
>
>> Why does U-Boot care whether it's been chain-loaded? Shouldn't it always
>> behave identically in all cases, so it's independent of what caused it to
>> run?
>
> In the case of read-only U-Boot it must find the read-write one to
> jump to. In the case of read-write U-Boot it must boot a kernel.

Surely that should be taken care of by placing the correct boot scripts 
into the U-Boot environment, rather than hard-coding specific boot 
behaviour into the U-Boot binary?

This feature seems really use-case-specific; I wonder if it's 
useful/generic-enough to upstream even?