| Message ID | 1345852714-13138-20-git-send-email-trini@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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 <trini@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. 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? -Scott
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 <trini@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. > 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. We need to be able to support the 4KB case, and we do (by not touching anything). We need to be able to support the 16KB case (cam_4xx_enc, others) and we do in CONFIG_SPL_FRAMEWORK. We also fit in the 8KB case (hawkboard) with CONFIG_SPL_FRAMEWORK. And of course, we fit in the "lots" of space and big feature case.
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 <trini@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. -Scott
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 <trini@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?
On 08/27/2012 12:07 PM, Tom Rini wrote: > 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: >>>> 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. 4K SPLs will use nand_spl_simple.c. It is pretty much a copy of nand_spl/nand_boot.c which 4K SPLs use, and Wolfgang is insisting that no new boards be added to nand_spl, so they must use the new SPL (even if there are no new 4xx boards, presumably such a stance by Wolfgang indicates a desire to see nand_spl go away entirely at some point). > 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? I think so. eLBC is 4K-limited, but IFC is similar and can do an 8K SPL (though we currently don't), and who knows what controllers will come along in the future. -Scott
On Mon, Aug 27, 2012 at 12:14:30PM -0500, Scott Wood wrote: > On 08/27/2012 12:07 PM, Tom Rini wrote: > > 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: > >>>> 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. > > 4K SPLs will use nand_spl_simple.c. It is pretty much a copy of > nand_spl/nand_boot.c which 4K SPLs use, and Wolfgang is insisting that > no new boards be added to nand_spl, so they must use the new SPL (even > if there are no new 4xx boards, presumably such a stance by Wolfgang > indicates a desire to see nand_spl go away entirely at some point). > > > 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? > > I think so. eLBC is 4K-limited, but IFC is similar and can do an 8K SPL > (though we currently don't), and who knows what controllers will come > along in the future. When do you plan to try and do the conversion? :) I kludged (but think it would still work) hawkboard to 887 bytes over 4kb and I see bamboo is 736 bytes under, leaving a 151 byte gap (in this very quick and somewhat silly SWAG). So maybe we can use this framework for 4KB systems. And, I'll split things out for now so we can move past this.
On 08/27/2012 12:50 PM, Tom Rini wrote: > On Mon, Aug 27, 2012 at 12:14:30PM -0500, Scott Wood wrote: >> On 08/27/2012 12:07 PM, Tom Rini wrote: >>> 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: >>>>>> 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. >> >> 4K SPLs will use nand_spl_simple.c. It is pretty much a copy of >> nand_spl/nand_boot.c which 4K SPLs use, and Wolfgang is insisting that >> no new boards be added to nand_spl, so they must use the new SPL (even >> if there are no new 4xx boards, presumably such a stance by Wolfgang >> indicates a desire to see nand_spl go away entirely at some point). >> >>> 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? >> >> I think so. eLBC is 4K-limited, but IFC is similar and can do an 8K SPL >> (though we currently don't), and who knows what controllers will come >> along in the future. > > When do you plan to try and do the conversion? :) I started a conversion of an eLBC board recently, but ran into some bugs that I couldn't squash by the end of the merge window -- at which point the timeslice expiration hit and its priority dropped. I may be able to resume next week (this week is Linux Plumbers). > I kludged (but think it would still work) hawkboard to 887 bytes over 4kb and I see bamboo is > 736 bytes under, leaving a 151 byte gap (in this very quick and somewhat > silly SWAG). So maybe we can use this framework for 4KB systems. Perhaps for some of them. How much does the framework add? > And, I'll split things out for now so we can move past this. OK, thanks. -Scott
On Mon, Aug 27, 2012 at 01:02:56PM -0500, Scott Wood wrote: > On 08/27/2012 12:50 PM, Tom Rini wrote: > > On Mon, Aug 27, 2012 at 12:14:30PM -0500, Scott Wood wrote: > >> On 08/27/2012 12:07 PM, Tom Rini wrote: > >>> 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: > >>>>>> 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. > >> > >> 4K SPLs will use nand_spl_simple.c. It is pretty much a copy of > >> nand_spl/nand_boot.c which 4K SPLs use, and Wolfgang is insisting that > >> no new boards be added to nand_spl, so they must use the new SPL (even > >> if there are no new 4xx boards, presumably such a stance by Wolfgang > >> indicates a desire to see nand_spl go away entirely at some point). > >> > >>> 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? > >> > >> I think so. eLBC is 4K-limited, but IFC is similar and can do an 8K SPL > >> (though we currently don't), and who knows what controllers will come > >> along in the future. > > > > When do you plan to try and do the conversion? :) > > I started a conversion of an eLBC board recently, but ran into some bugs > that I couldn't squash by the end of the merge window -- at which point > the timeslice expiration hit and its priority dropped. > > I may be able to resume next week (this week is Linux Plumbers). > > > I kludged (but think it would still work) hawkboard to 887 bytes over 4kb and I see bamboo is > > 736 bytes under, leaving a 151 byte gap (in this very quick and somewhat > > silly SWAG). So maybe we can use this framework for 4KB systems. > > Perhaps for some of them. How much does the framework add? For hawkboard (davinci and NAND) we grew by 894 bytes. Doing some re-organization of the code just now I've got that growth down to 766 bytes. Some of that reduction I had in the other quick swag I was doing however.
On Mon, Aug 27, 2012 at 12:08:14PM -0700, Tom Rini wrote: > On Mon, Aug 27, 2012 at 01:02:56PM -0500, Scott Wood wrote: > > On 08/27/2012 12:50 PM, Tom Rini wrote: > > > On Mon, Aug 27, 2012 at 12:14:30PM -0500, Scott Wood wrote: > > >> On 08/27/2012 12:07 PM, Tom Rini wrote: > > >>> 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: > > >>>>>> 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. > > >> > > >> 4K SPLs will use nand_spl_simple.c. It is pretty much a copy of > > >> nand_spl/nand_boot.c which 4K SPLs use, and Wolfgang is insisting that > > >> no new boards be added to nand_spl, so they must use the new SPL (even > > >> if there are no new 4xx boards, presumably such a stance by Wolfgang > > >> indicates a desire to see nand_spl go away entirely at some point). > > >> > > >>> 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? > > >> > > >> I think so. eLBC is 4K-limited, but IFC is similar and can do an 8K SPL > > >> (though we currently don't), and who knows what controllers will come > > >> along in the future. > > > > > > When do you plan to try and do the conversion? :) > > > > I started a conversion of an eLBC board recently, but ran into some bugs > > that I couldn't squash by the end of the merge window -- at which point > > the timeslice expiration hit and its priority dropped. > > > > I may be able to resume next week (this week is Linux Plumbers). > > > > > I kludged (but think it would still work) hawkboard to 887 bytes over 4kb and I see bamboo is > > > 736 bytes under, leaving a 151 byte gap (in this very quick and somewhat > > > silly SWAG). So maybe we can use this framework for 4KB systems. > > > > Perhaps for some of them. How much does the framework add? > > For hawkboard (davinci and NAND) we grew by 894 bytes. Doing some > re-organization of the code just now I've got that growth down to 766 > bytes. Some of that reduction I had in the other quick swag I was doing > however. Whack out a few more prints (enabled still for debug) and we're at 711 byte growth now. Will be part of v5.
diff --git a/arch/arm/cpu/arm926ejs/davinci/spl.c b/arch/arm/cpu/arm926ejs/davinci/spl.c index 8a6855e..a0c9119 100644 --- a/arch/arm/cpu/arm926ejs/davinci/spl.c +++ b/arch/arm/cpu/arm926ejs/davinci/spl.c @@ -79,7 +79,7 @@ void spl_board_init(void) u32 spl_boot_device(void) { -#ifdef CONFIG_SPL_NAND_LOAD +#ifdef CONFIG_SPL_NAND_SIMPLE return BOOT_DEVICE_NAND; #elif defined(CONFIG_SPL_SPI_LOAD) return BOOT_DEVICE_SPI; diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile index 837e22e..0e0b641 100644 --- a/arch/arm/cpu/armv7/omap-common/Makefile +++ b/arch/arm/cpu/armv7/omap-common/Makefile @@ -45,12 +45,6 @@ COBJS += boot-common.o SOBJS += lowlevel_init.o endif -ifdef CONFIG_SPL_BUILD -ifdef CONFIG_SPL_NAND_SUPPORT -COBJS += spl_nand.o -endif -endif - ifndef CONFIG_SPL_BUILD ifneq ($(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX),) COBJS += mem-common.o diff --git a/arch/arm/cpu/armv7/omap-common/spl_nand.c b/arch/arm/cpu/armv7/omap-common/spl_nand.c deleted file mode 100644 index 9e9206c..0000000 --- a/arch/arm/cpu/armv7/omap-common/spl_nand.c +++ /dev/null @@ -1,102 +0,0 @@ -/* - * Copyright (C) 2011 - * Corscience GmbH & Co. KG - Simon Schwarz <schwarz@corscience.de> - * - * See file CREDITS for list of people who contributed to this - * project. - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License as - * published by the Free Software Foundation; either version 2 of - * the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, - * MA 02111-1307 USA - */ -#include <common.h> -#include <spl.h> -#include <asm/u-boot.h> -#include <asm/utils.h> -#include <asm/io.h> -#include <nand.h> -#include <version.h> - -void spl_nand_load_image(void) -{ - struct image_header *header; - int *src __attribute__((unused)); - int *dst __attribute__((unused)); - - debug("spl: nand - using hw ecc\n"); - 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 { - printf("The Expected Linux image was not" - "found. Please check your NAND" - "configuration.\n"); - printf("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(); -} diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c index 4a4d02f..05483e0 100644 --- a/drivers/mtd/nand/nand_spl_simple.c +++ b/drivers/mtd/nand/nand_spl_simple.c @@ -2,6 +2,9 @@ * (C) Copyright 2006-2008 * Stefan Roese, DENX Software Engineering, sr@denx.de. * + * Copyright (C) 2011 + * Corscience GmbH & Co. KG - Simon Schwarz <schwarz@corscience.de> + * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as * published by the Free Software Foundation; either version 2 of @@ -20,6 +23,7 @@ #include <common.h> #include <nand.h> +#include <spl.h> #include <asm/io.h> #include <linux/mtd/nand_ecc.h> @@ -281,3 +285,75 @@ void nand_deselect(void) if (nand_chip.select_chip) nand_chip.select_chip(&mtd, -1); } + +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(); +} diff --git a/include/configs/cam_enc_4xx.h b/include/configs/cam_enc_4xx.h index fe28278..34aac8c 100644 --- a/include/configs/cam_enc_4xx.h +++ b/include/configs/cam_enc_4xx.h @@ -222,7 +222,6 @@ #define CONFIG_SPL_LIBGENERIC_SUPPORT #define CONFIG_SPL_NAND_SUPPORT #define CONFIG_SPL_NAND_SIMPLE -#define CONFIG_SPL_NAND_LOAD #define CONFIG_SYS_NAND_HW_ECC_OOBFIRST #define CONFIG_SPL_SERIAL_SUPPORT #define CONFIG_SPL_POST_MEM_SUPPORT diff --git a/include/configs/hawkboard.h b/include/configs/hawkboard.h index 838e572..5f2aa62 100644 --- a/include/configs/hawkboard.h +++ b/include/configs/hawkboard.h @@ -63,7 +63,6 @@ #define CONFIG_SPL_BOARD_INIT #define CONFIG_SPL_NAND_SUPPORT #define CONFIG_SPL_NAND_SIMPLE -#define CONFIG_SPL_NAND_LOAD #define CONFIG_SPL_LIBGENERIC_SUPPORT /* for udelay and __div64_32 for NAND */ #define CONFIG_SPL_SERIAL_SUPPORT #define CONFIG_SPL_LDSCRIPT "board/$(BOARDDIR)/u-boot-spl-hawk.lds"
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 <trini@ti.com> --- Changes in v4: - Leave nand_spl_load.c alone, move the new load into nand_spl_simple.c arch/arm/cpu/arm926ejs/davinci/spl.c | 2 +- arch/arm/cpu/armv7/omap-common/Makefile | 6 -- arch/arm/cpu/armv7/omap-common/spl_nand.c | 102 ----------------------------- drivers/mtd/nand/nand_spl_simple.c | 76 +++++++++++++++++++++ include/configs/cam_enc_4xx.h | 1 - include/configs/hawkboard.h | 1 - 6 files changed, 77 insertions(+), 111 deletions(-) delete mode 100644 arch/arm/cpu/armv7/omap-common/spl_nand.c