On 19/11/16 13:49, Simon Glass wrote:
Hi Simon,
> On 17 November 2016 at 18:50, André Przywara <andre.przyw...@arm.com> wrote:
>> On 05/11/16 16:10, Simon Glass wrote:
>>
>> Hi Simon,
>>
>>> On 2 November 2016 at 19:36, Andre Przywara <andre.przyw...@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.przyw...@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 <s...@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.
>>>
>>>> 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 ;-)
Cheers,
Andre.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
