[U-Boot] [RFC PATCH 08/10] SPL: read and store arch property from U-Boot image

[Simon Glass]

Hi Andre,

On 19 November 2016 at 09:35, André Przywara <andre.przywara at arm.com> wrote:
> On 19/11/16 13:49, Simon Glass wrote:
>
> Hi Simon,
>
>> On 17 November 2016 at 18:50, André Przywara <andre.przywara at arm.com> wrote:
>>> On 05/11/16 16:10, Simon Glass wrote:
>>>
>>> Hi Simon,
>>>
>>>> On 2 November 2016 at 19:36, Andre Przywara <andre.przywara at arm.com> 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>
>>>>> ---
>>>>>  common/spl/spl.c     | 1 +
>>>>>  common/spl/spl_fit.c | 8 ++++++++
>>>>>  include/spl.h        | 3 ++-
>>>>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>
>>> Thanks!
>>>
>>>>
>>>>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>>>>> index bdb165a..f76ddd2 100644
>>>>> --- a/common/spl/spl.c
>>>>> +++ b/common/spl/spl.c
>>>>> @@ -114,6 +114,7 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
>>>>>                                 header_size;
>>>>>                 }
>>>>>                 spl_image->os = image_get_os(header);
>>>>> +               spl_image->arch = image_get_arch(header);
>>>>>                 spl_image->name = image_get_name(header);
>>>>>                 debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
>>>>>                         (int)sizeof(spl_image->name), spl_image->name,
>>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>>> index aae556f..a5d903b 100644
>>>>> --- a/common/spl/spl_fit.c
>>>>> +++ b/common/spl/spl_fit.c
>>>>> @@ -123,6 +123,11 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size,
>>>>>         return (data_size + info->bl_len - 1) / info->bl_len;
>>>>>  }
>>>>>
>>>>> +__weak u8 spl_genimg_get_arch_id(const char *arch_str)
>>>>> +{
>>>>> +       return IH_ARCH_DEFAULT;
>>>>> +}
>>>>> +
>>>>
>>>> Do we need this weak function, or could we just add a normal function
>>>> in the ARM code somewhere?
>>>
>>> Mmh, but this here is generic code. In a later patch I provide an ARM
>>> specific function under arch/arm, but so far this weak function just
>>> mimics the current behaviour: return the current architecture.
>>>
>>>> If you don't think we need much error checking you could do something like:
>>>>
>>>> #ifdef CONFIG_ARM
>>>> ... possible strcmp() here
>>>> return IH_ARCH_ARM;
>>>> #endif
>>>
>>> So I found the weak function more elegant than #ifdef-ing architecture
>>> specific code into a generic file.
>>
>> Fair enough.
>>
>>> Is there any issue with weak functions, shall we avoid them?
>>
>> Well they can be confusing since it's hard to know which function gets
>> linked in. We have things like CONFIG_MISC_INIT_F, rather than make
>> misc_init() weak.
>
> I see. But I think in this case a weak function is neater. I will add a
> comment to the non-weak definition pointing out its nature, if that helps.

OK, well more comments help.

>
>>>>
>>>>>  int spl_load_simple_fit(struct spl_image_info *spl_image,
>>>>>                         struct spl_load_info *info, ulong sector, void *fit)
>>>>>  {
>>>>> @@ -136,6 +141,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>>>>         int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
>>>>>         int src_sector;
>>>>>         void *dst, *src;
>>>>> +       const char *arch_str;
>>>>>
>>>>>         /*
>>>>>          * Figure out where the external images start. This is the base for the
>>>>> @@ -184,10 +190,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>>>>         data_offset = fdt_getprop_u32(fit, node, "data-offset");
>>>>>         data_size = fdt_getprop_u32(fit, node, "data-size");
>>>>>         load = fdt_getprop_u32(fit, node, "load");
>>>>> +       arch_str = fdt_getprop(fit, node, "arch", NULL);
>>>>>         debug("data_offset=%x, data_size=%x\n", data_offset, data_size);
>>>>>         spl_image->load_addr = load;
>>>>>         spl_image->entry_point = load;
>>>>>         spl_image->os = IH_OS_U_BOOT;
>>>>> +       spl_image->arch = spl_genimg_get_arch_id(arch_str);
>>>>>
>>>>>         /*
>>>>>          * Work out where to place the image. We read it so that the first
>>>>> diff --git a/include/spl.h b/include/spl.h
>>>>> index e080a82..6a9d2fb 100644
>>>>> --- a/include/spl.h
>>>>> +++ b/include/spl.h
>>>>> @@ -22,11 +22,12 @@
>>>>>
>>>>>  struct spl_image_info {
>>>>>         const char *name;
>>>>> -       u8 os;
>>>>>         u32 load_addr;
>>>>>         u32 entry_point;
>>>>>         u32 size;
>>>>>         u32 flags;
>>>>> +       u8 os;
>>>>> +       u8 arch;
>>>>
>>>> Can you please add a struct comment?
>>>
>>> Is that something specific / a special documentation format or do you
>>> mean just document the structure members?
>>
>> The latter - see struct spl_load_info for an example. If we improve
>> the code we change everyone wins :-)
>
> Yeah, sorry for that stupid question: When I just wanted to add
> something I saw what you meant in the struct next to it ;-)

Yes that's what I mean, thanks.

Regards,
Simon