[U-Boot] [PATCH 3/9] SPL: read and store arch property from U-Boot image

[Philipp Tomsich]

> On 21.02.2019, at 13:00, Andre Przywara <andre.przywara at arm.com> wrote:
> 
> On Thu, 21 Feb 2019 12:47:04 +0100
> Alexander Graf <agraf at suse.de> wrote:
> 
> Hi Alex,
> 
>> On 02/21/2019 02:30 AM, Andre Przywara wrote:
>>> Read the specified "arch" value from a legacy or FIT U-Boot image and
>>> store it in our SPL data structure.
>>> This allows loaders to take the target architecture in account for
>>> custom loading procedures.
>>> Having the complete string -> arch mapping for FIT based images in the
>>> SPL would be too big, so we leave it up to architectures (or boards) to
>>> overwrite the weak function that does the actual translation, possibly
>>> covering only the required subset there.
>>> 
>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>  
>> 
>> Reviewed-by: Alexander Graf <agraf at suse.de>
> 
> Thanks for having a look!
> 
>> I don't fully buy the argument that the generic mapping would be too big 
>> though. Realistically you should be able to get away with 1 or 2 
>> branches per case, no? So that would be maybe 40 instructions?
> 
> You are apparently not in the Allwinner SPL mindset ;-)

I am sometimes struggling against the 192KB limit on the RK3399.  So while
my current problems (SPL on the A31 was a different story) may sound like
“luxury” to you, I do understand the pain.

> I get excited when I find a way of saving 100 bytes (see the writel_relaxed
> patch), so wasting 160 bytes when we will probably never need to return
> IH_ARCH_X86_64 doesn't sound right to me.
> 
> And Philipp is right: the canonical way would be to use uimage_arch[] from
> common/image.c, which is quite big.
> 
> We could have a big #ifdef cascade, something like:
> +#ifdef CONFIG_ARM
> +	if (!strcmp(arch_str, "arm"))
> +		return IH_ARCH_ARM;
> +
> +	if (!strcmp(arch_str, "arm64"))
> +		return IH_ARCH_ARM64;
> +#endif
> +#ifdef CONFIG_X86
> +	if (!strcmp(arch_str, , "x86"))
> +		return IH_ARCH_I386;
> +
> +	if (!strcmp(arch_str, "x86_64"))
> +		return IH_ARCH_X86_64;
> +#endif

Could we have a preprocessor-guarded table of the form
	ONLY_ON(ARM)( { “arm”, IH_ARCH_ARM }, )
	ONLY_ON(ARM)( { “arm64”, IH_ARCH_ARM64 }, )
for this?

Then the function would be
	for ( …, curr_element = …, ... ) {
		if (!strcmp(arch_str, curr_element.arch_str))
			return curr_element.ih_value;
which should be less than the hypothetical 40 instructions, I would hope.

Then again, I didn’t send this through a compiler to look at the assembly
generated.  And I have been surprised by code-size on AArch64 before.

I usually am in favor of cleaning up the common code paths, so my personal
preference would be in figuring out a way how we can change uimage_arch[]
to become useful in a space-constrained SPL context.

Phil.