Hi Andre, 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. > >> >>> 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 :-) Regards, Simon > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot