[U-Boot] [PATCH v1 1/3] add boot_get_loadables() to load listed images

Simon Glass
2015-05-15T20:38:17Z

Hi Karl,

On 15 May 2015 at 14:17, Karl Apsite <Karl.Apsite@dornerworks.com> wrote:
>
> Hi Simon,
>
> On 05/15/2015 09:57 AM, Simon Glass wrote:
> > Hi Karl,
> >
> > On 13 May 2015 at 06:53, Karl Apsite <Karl.Apsite@dornerworks.com> wrote:
> >> From: Karl Apsite <karl.apsite@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.Apsite@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 at 1", "fdt at 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
>

Perhaps. But most of the comments probably need redoing to fit the
right style. So my approach is normally to try to do things nicely for
new code even if old code doesn't.

> >> +#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