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

[Alexander Graf]

On 21.02.19 14:22, Philipp Tomsich wrote:
> 
> 
>> 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.

I was actually thinking more along the lines of

#ifdef CONFIG_ARM
  if (arch[0] == 'a') {
    if (arch[3] == '6') /* "arm64" */
      return IH_ARCH_ARM64;
    else
      return IH_ARCH_ARM;
  }
#endif

etc.

If you put that in a generic function that can be used across the board,
you will also less likely have target specific bugs sneak in. And the
resulting code should be much smaller than your strcmp.


Alex