Hi Simon,
On 05/15/2015 09:57 AM, Simon Glass wrote:
> Hi Karl,
>
> On 13 May 2015 at 06:53, Karl Apsite <karl.aps...@dornerworks.com> wrote:
>> From: Karl Apsite <karl.aps...@dornerworks.com>
>>
>> Added a trimmed down instance of boot_get_<thing>() to satisfy the
>> minimum requierments of the added feature. The function follows the
>> normal patterns set by other boot_get<thing>'s, which should make it a
>> bit easier to combine them all together into one boot_get_image()
>> function in a later refactor.
>>
>> Documentation for the new function can be found in source:
>> common/image.c
>>
>> Signed-off-by: Karl Apsite <karl.aps...@dornerworks.com>
>> ---
>>
>> common/bootm.c | 22 +++++++++++++
>> common/image-fit.c | 8 ++++-
>> common/image.c | 93
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/bootstage.h | 1 +
>> include/image.h | 5 ++-
>> 5 files changed, 127 insertions(+), 2 deletions(-)
>>
>
> This looks fine to me. Can you please add documentation - doc/uImage.FIT
>
This will be included in one of the missing commits
> A few bits below.
>
>> diff --git a/common/bootm.c b/common/bootm.c
>> index 6842029..f04e49b 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -240,6 +240,23 @@ static int bootm_find_fdt(int flag, int argc, char *
>> const argv[])
>> }
>> #endif
>>
>> +#if defined(CONFIG_FIT)
>> +static int bootm_find_loadables(int flag, int argc, char * const argv[])
>> +{
>> + int ret;
>> +
>> + /* find all of the loadables */
>> + ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT,
>> + NULL, NULL);
>> + if (ret) {
>> + puts("Loadable(s) is corrupt or invalid\n");
>
> printf() here (we are trying to avoid using puts() so that one day we
> can make it compatible with the C library puts in that it
> automatically outputs \n).
>
I will convert puts to printf in all 3 cases.
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[])
>> {
>> if (bootm_find_ramdisk(flag, argc, argv))
>> @@ -250,6 +267,11 @@ int bootm_find_ramdisk_fdt(int flag, int argc, char *
>> const argv[])
>> return 1;
>> #endif
>>
>> +#if defined(CONFIG_FIT)
>> + if (bootm_find_loadables(flag, argc, argv))
>> + return 1;
>> +#endif
>> +
>> return 0;
>> }
>>
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index 245264d..fe23c13 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -1544,6 +1544,8 @@ static const char *fit_get_image_type_property(int
>> type)
>> return FIT_RAMDISK_PROP;
>> case IH_TYPE_X86_SETUP:
>> return FIT_SETUP_PROP;
>> + case IH_TYPE_LOADABLE:
>> + return FIT_LOADABLE_PROP;
>> }
>>
>> return "unknown";
>> @@ -1661,7 +1663,11 @@ int fit_image_load(bootm_headers_t *images, ulong
>> addr,
>> os_ok = image_type == IH_TYPE_FLATDT ||
>> fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
>> fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
>> - if (!type_ok || !os_ok) {
>> +
>> + /* If either of the checks fail, we should report an error, but
>
> /*
> * If either ...
> * what it is
> */
>
I'll correct the comment
>> + * if the image type is coming from the "loadables" field, we
>> + * don't care about what it is */
>> + if ((!type_ok || !os_ok) && image_type != IH_TYPE_LOADABLE) {
>> fit_image_get_os(fit, noffset, &os);
>> printf("No %s %s %s Image\n",
>> genimg_get_os_name(os),
>> diff --git a/common/image.c b/common/image.c
>> index fdec496..dc7e795 100644
>> --- a/common/image.c
>> +++ b/common/image.c
>> @@ -1165,6 +1165,99 @@ int boot_get_setup(bootm_headers_t *images, uint8_t
>> arch,
>> #endif
>> }
>>
>> +/**
>> + * boot_get_loadable - routine to load a list of binaries to memory
>> + * @argc: Ignored Argument
>> + * @argv: Ignored Argument
>> + * @images: pointer to the bootm images structure
>> + * @arch: expected architecture for the image
>> + * @ld_start: Ignored Argument
>> + * @ld_len: Ignored Argument
>> + *
>> + * boot_get_loadable() will take the given FIT configuration, and look
>> + * for a field named "loadables". Loadables, is a list of elements in
>> + * the FIT given as strings. exe:
>> + * loadables = "linux_kernel@1", "fdt@2";
>> + * this function will attempt to parse each string, and load the
>> + * corresponding element from the FIT into memory. Once placed,
>> + * no aditional actions are taken.
>> + *
>> + * returns:
>
> @return
I did not see @return in the linked documentation:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt
I will change it to @return
>
>> + * 0, if only valid images or no images are found
>> + * error code, if an error occurs during fit_image_load
>
> Great docs.
>
>> + */
>> +#if defined(CONFIG_FIT)
>> +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t
>> *images,
>> + uint8_t arch, const ulong *ld_start, ulong * const ld_len)
>> +{
>> + /*
>> + * These variabels are used to hold the current image location
>> + * in system memory.
>> + */
s/variabels/variables
>> + ulong tmp_img_addr;
>> + /* These are not used */
I will make this comment a bit more clear
>> + ulong img_data, img_len;
>> + void *buf;
>> + char *uname;
>> + char *uname_end;
>> + int len, fit_img_result;
>
> blank line
>
I looked, and didn't see a blank line, so I take this to mean you WANT a line
here.
>> + /* Check to see if the images struct has a FIT configuration */
>> + if (genimg_has_config(images)) {
>
> Can we do...
>
> if (!genimg_has_config(images)) {
> debug("## FIT configuration was not specified\n");
> return 0;
> }
>
> just to limit the indent level?
>
Totally!
>> + /*
>> + * Obtain the os FIT header from the images struct
>> + * copy from dataflash if needed
>> + */
>> + tmp_img_addr = map_to_sysmem(images->fit_hdr_os);
>> + tmp_img_addr = genimg_get_image(tmp_img_addr);
>> + buf = map_sysmem(tmp_img_addr, 0);
>> + /*
>> + * Check image type. For FIT images get FIT node
>> + * and attempt to locate a generic binary.
>> + */
>> + switch (genimg_get_format(buf)) {
>> + case IMAGE_FORMAT_FIT:
>> + /*
>> + * Try to obtain the 'loadables' field from the
>> + * configuration If it exists, then try and load
>> + * all of the listed strings
>> + *
>> + * This logic assumes a character is one byte
>> + */
>> + uname = (char *)fdt_getprop(buf,
>
> do you need the cast?
>
It is a (const void *) without the cast. An error is tossed if I don't have the
cast because I am discarding the ‘const’ qualifier. Probably a moot point.
>> + fit_conf_get_node(buf,
>> images->fit_uname_cfg),
>> + FIT_LOADABLE_PROP, &len);
>> + uname_end = uname + len;
>> + if (NULL != uname) {
>
> if (uname)
>
> but perhaps you should use:
>
> for (index = 0; !fdt_get_string_index(...index...); index++) {
> process string
>
Sure!
>> [snip]
>> +#endif
>> +
>> #ifdef CONFIG_SYS_BOOT_GET_CMDLINE
>> /**
>> * boot_get_cmdline - allocate and initialize kernel cmdline
>> diff --git a/include/bootstage.h b/include/bootstage.h
>> index be44014..4e2e0fb 100644
>> --- a/include/bootstage.h
>> +++ b/include/bootstage.h
>> @@ -168,6 +168,7 @@ enum bootstage_id {
>> BOOTSTAGE_ID_NAND_FIT_READ = 150,
>> BOOTSTAGE_ID_NAND_FIT_READ_OK,
>>
>> + BOOTSTAGE_ID_FIT_LOADS_START = 160, /* for Loadable Images */
>
> FIT_LOADABLE_START?
>
The name is getting a bit long, but I can change it.
>> /*
>> * These boot stages are new, higher level, and not directly related
>> * to the old boot progress numbers. They are useful for recording
>> diff --git a/include/image.h b/include/image.h
>> index 97b96b3..0b0b55b 100644
>> --- a/include/image.h
>> +++ b/include/image.h
>> @@ -244,6 +244,7 @@ struct lmb;
>> #define IH_TYPE_SOCFPGAIMAGE 19 /* Altera SOCFPGA Preloader */
>> #define IH_TYPE_X86_SETUP 20 /* x86 setup.bin Image */
>> #define IH_TYPE_LPC32XXIMAGE 21 /* x86 setup.bin Image */
>> +#define IH_TYPE_LOADABLE 22 /* A list of typeless images */
>>
>> /*
>> * Compression Types
>> @@ -455,7 +456,9 @@ ulong genimg_get_image(ulong img_addr);
>>
>> int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
>> uint8_t arch, ulong *rd_start, ulong *rd_end);
>> -#endif
>> +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t
>> *images,
>> + uint8_t arch, const ulong *ld_start, ulong * const ld_len);
>
> Function comment should go here since this is the API definition (move
> it from the C file).
>
I agree, however by that logic, ALL of the comments on non-static functions in
common/image.c should move. If that's correct, then it might be a good idea to
defer this until someone can get around to moving the lot of them together, to
maintain consistency.
Current examples:
http://git.denx.de/?p=u-boot.git;a=blob;f=common/image.c;h=fdec496c4bf0e936f26e8876f621d9103c19595c;hb=HEAD#l852
http://git.denx.de/?p=u-boot.git;a=blob;f=common/image-fdt.c;h=7e2da7b3b7218d10c40167e1d80c0d678ff47c1a;hb=HEAD#l201
>> +#endif /* !USE_HOSTCC */
>>
>> int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch,
>> ulong *setup_start, ulong *setup_len);
>> --
>> 2.3.7
>>
>
> Regards,
> Simon
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
