On Mon, Aug 27, 2012 at 11:16:45AM -0500, Scott Wood wrote:
> On 08/27/2012 09:37 AM, Tom Rini wrote:
> > On 08/24/2012 05:09 PM, Scott Wood wrote:
> >> On 08/24/2012 06:58 PM, Tom Rini wrote:
> >>> Takes the load function from arch/arm/cpu/armv7/omap-common/spl_nand.c
> >>> instead. This will allow for easier integration of SPL-boots-Linux code
> >>> on
> >>> other arches.
> >>>
> >>> Signed-off-by: Tom Rini <tr...@ti.com>
> >>> ---
> >>> Changes in v4:
> >>> - Leave nand_spl_load.c alone, move the new load into nand_spl_simple.c
> >> [snip]
> >>> +void spl_nand_load_image(void)
> >>> +{
> >>> + struct image_header *header;
> >>> + int *src __attribute__((unused));
> >>> + int *dst __attribute__((unused));
> >>> +
> >>> + nand_init();
> >>> +
> >>> + /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> >>> + header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> >>> +#ifdef CONFIG_SPL_OS_BOOT
> >>> + if (!spl_start_uboot()) {
> >>> + /*
> >>> + * load parameter image
> >>> + * load to temp position since nand_spl_load_image reads
> >>> + * a whole block which is typically larger than
> >>> + * CONFIG_CMD_SPL_WRITE_SIZE therefore may overwrite
> >>> + * following sections like BSS
> >>> + */
> >>> + nand_spl_load_image(CONFIG_CMD_SPL_NAND_OFS,
> >>> + CONFIG_CMD_SPL_WRITE_SIZE,
> >>> + (void *)CONFIG_SYS_TEXT_BASE);
> >>> + /* copy to destintion */
> >>> + for (dst = (int *)CONFIG_SYS_SPL_ARGS_ADDR,
> >>> + src = (int *)CONFIG_SYS_TEXT_BASE;
> >>> + src < (int *)(CONFIG_SYS_TEXT_BASE +
> >>> + CONFIG_CMD_SPL_WRITE_SIZE);
> >>> + src++, dst++) {
> >>> + writel(readl(src), dst);
> >>> + }
> >>> +
> >>> + /* load linux */
> >>> + nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
> >>> + CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> + spl_parse_image_header(header);
> >>> + if (header->ih_os == IH_OS_LINUX) {
> >>> + /* happy - was a linux */
> >>> + nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
> >>> + spl_image.size, (void *)spl_image.load_addr);
> >>> + nand_deselect();
> >>> + return;
> >>> + } else {
> >>> + puts("The Expected Linux image was not "
> >>> + "found. Please check your NAND "
> >>> + "configuration.\n");
> >>> + puts("Trying to start u-boot now...\n");
> >>> + }
> >>> + }
> >>> +#endif
> >>> +#ifdef CONFIG_NAND_ENV_DST
> >>> + nand_spl_load_image(CONFIG_ENV_OFFSET,
> >>> + CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> + spl_parse_image_header(header);
> >>> + nand_spl_load_image(CONFIG_ENV_OFFSET, spl_image.size,
> >>> + (void *)spl_image.load_addr);
> >>> +#ifdef CONFIG_ENV_OFFSET_REDUND
> >>> + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND,
> >>> + CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> + spl_parse_image_header(header);
> >>> + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, spl_image.size,
> >>> + (void *)spl_image.load_addr);
> >>> +#endif
> >>> +#endif
> >>> + /* Load u-boot */
> >>> + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
> >>> + CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> + spl_parse_image_header(header);
> >>> + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
> >>> + spl_image.size, (void *)spl_image.load_addr);
> >>> + nand_deselect();
> >>> +}
> >>
> >> Will this refuse to link if spl_parse_image_header is not present, or
> >> will gc-sections remove it before the error is given? Does this
> >> function leave any anonymous data that isn't cleaned up by gc-sections?
> >> Again, this file must not grow for users that don't need the new features.
> >
> > Yes, spl_nand_load_image will be garbage collected and not link-error if
> > not called. But note that all users of this file have been converted to
> > CONFIG_SPL_FRAMEWORK and would be using this function.
>
> There are still a lot of nand_spl targets that have not yet been
> converted, some of which will be future users of this file (such as ppc
> 4xx). This file is a replacement for nand_spl/nand_boot.c and will be
> used by the same SPLs.
>
> >> What is the benefit of putting this in nand_spl_simple.c versus another
> >> file? What if someone wants to use this with a different NAND boot
> >> implementation?
> >
> > I would start by questioning the need of a 3rd SPL framework.
>
> The "simple" driver does not work for all hardware. This is why we have
> nand_spl/nand_boot_fsl_elbc.c and others in addition to
> nand_spl/nand_boot.c. It's not a "3rd SPL framework", just a different
> NAND implementation.
The question boils down to, what are your size constraints? I guess
what I'm saying is, if it's <4kb, it's not using this file nor the
framework. If we've got more than 4kb to work with, it's using the
framework (with changes if needed, of course) and I guess we could move
the function to common/spl/spl_nand.c and add
drivers/mtd/nand/nand_spl_fsl_elbc.c and so on. Now that I've had more
coffee, do I follow your suggestion right?
--
Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
