| Message ID | 1446995514-26357-11-git-send-email-nikita@compulab.co.il |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
On 8 November 2015 at 07:11, Nikita Kiryanov <nikita@compulab.co.il> wrote: > Introduce spl_boot_list array, which defines a list of boot devices > that SPL will try before hanging. By default this list will consist > of only spl_boot_device(), but board_boot_order() can be overridden > by board code to populate the array with custom values. > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Igor Grinberg <grinberg@compulab.co.il> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > Reviewed-by: Tom Rini <trini@konsulko.com> > --- > Changes in V4: > - No changes. > > Changes in V3: > - No changes. > > Changes in V2: > - No changes. > > common/spl/spl.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On Sun, Nov 08, 2015 at 05:11:51PM +0200, Nikita Kiryanov wrote: > Introduce spl_boot_list array, which defines a list of boot devices > that SPL will try before hanging. By default this list will consist > of only spl_boot_device(), but board_boot_order() can be overridden > by board code to populate the array with custom values. > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Igor Grinberg <grinberg@compulab.co.il> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > Reviewed-by: Tom Rini <trini@konsulko.com> > Reviewed-by: Simon Glass <sjg@chromium.org> So, a problem with this patch is that we push the x600 board, which is an 8KiB SPL target, over the line. I feel like maybe we need a follow-up patch that makes announcing depend not on libcommon (which x600 needs) but something else to know that there's a reason to announce.
On Sun, Nov 08, 2015 at 05:11:51PM +0200, Nikita Kiryanov wrote: > Introduce spl_boot_list array, which defines a list of boot devices > that SPL will try before hanging. By default this list will consist > of only spl_boot_device(), but board_boot_order() can be overridden > by board code to populate the array with custom values. > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Igor Grinberg <grinberg@compulab.co.il> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > Reviewed-by: Tom Rini <trini@konsulko.com> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
Hi Tom, On Wed, Nov 18, 2015 at 05:33:20PM -0500, Tom Rini wrote: > On Sun, Nov 08, 2015 at 05:11:51PM +0200, Nikita Kiryanov wrote: > > > Introduce spl_boot_list array, which defines a list of boot devices > > that SPL will try before hanging. By default this list will consist > > of only spl_boot_device(), but board_boot_order() can be overridden > > by board code to populate the array with custom values. > > > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > > Cc: Igor Grinberg <grinberg@compulab.co.il> > > Cc: Tom Rini <trini@konsulko.com> > > Cc: Simon Glass <sjg@chromium.org> > > Reviewed-by: Tom Rini <trini@konsulko.com> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > So, a problem with this patch is that we push the x600 board, which is > an 8KiB SPL target, over the line. I feel like maybe we need a > follow-up patch that makes announcing depend not on libcommon (which > x600 needs) but something else to know that there's a reason to > announce. Based on the content of your reply I'm guessing you're referring to the next patch, not this one. I suppose that announcing can be made into an optional feature. However, I also think that since printing is an optional feature that can greatly increase binary size, it shouldn't be coupled with other, often non-optional libcommon features the way it currently is via CONFIG_SPL_LIBCOMMON_SUPPORT. The best fix in my opinion would be to implement a way to exclude printing support from SPL even if libcommon is included (CONFIG_SPL_SILENT that replaces printfs with empty stubs?). This will also make it possible to remove all those #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT checks that appear all over the SPL code. > > -- > Tom
On 19.11.2015 12:19, Nikita Kiryanov wrote: > Hi Tom, > > On Wed, Nov 18, 2015 at 05:33:20PM -0500, Tom Rini wrote: >> On Sun, Nov 08, 2015 at 05:11:51PM +0200, Nikita Kiryanov wrote: >> >>> Introduce spl_boot_list array, which defines a list of boot devices >>> that SPL will try before hanging. By default this list will consist >>> of only spl_boot_device(), but board_boot_order() can be overridden >>> by board code to populate the array with custom values. >>> >>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >>> Cc: Igor Grinberg <grinberg@compulab.co.il> >>> Cc: Tom Rini <trini@konsulko.com> >>> Cc: Simon Glass <sjg@chromium.org> >>> Reviewed-by: Tom Rini <trini@konsulko.com> >>> Reviewed-by: Simon Glass <sjg@chromium.org> >> >> So, a problem with this patch is that we push the x600 board, which is >> an 8KiB SPL target, over the line. I feel like maybe we need a >> follow-up patch that makes announcing depend not on libcommon (which >> x600 needs) but something else to know that there's a reason to >> announce. > > Based on the content of your reply I'm guessing you're referring to the > next patch, not this one. > > I suppose that announcing can be made into an optional feature. However, > I also think that since printing is an optional feature that can greatly > increase binary size, it shouldn't be coupled with other, often > non-optional libcommon features the way it currently is via > CONFIG_SPL_LIBCOMMON_SUPPORT. The best fix in my opinion would be to > implement a way to exclude printing support from SPL even if libcommon > is included (CONFIG_SPL_SILENT that replaces printfs with empty stubs?). > > This will also make it possible to remove all those #ifdef > CONFIG_SPL_LIBCOMMON_SUPPORT checks that appear all over the SPL code. I think that my recently posted tiny-printf patches: https://patchwork.ozlabs.org/patch/545034/ https://patchwork.ozlabs.org/patch/545033/ https://patchwork.ozlabs.org/patch/545036/ https://patchwork.ozlabs.org/patch/545035/ can solve this size issue on x600 (and perhaps other) board. Comments welcome... Thanks, Stefan
On Thu, Nov 19, 2015 at 01:19:39PM +0200, Nikita Kiryanov wrote: > Hi Tom, > > On Wed, Nov 18, 2015 at 05:33:20PM -0500, Tom Rini wrote: > > On Sun, Nov 08, 2015 at 05:11:51PM +0200, Nikita Kiryanov wrote: > > > > > Introduce spl_boot_list array, which defines a list of boot devices > > > that SPL will try before hanging. By default this list will consist > > > of only spl_boot_device(), but board_boot_order() can be overridden > > > by board code to populate the array with custom values. > > > > > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > > > Cc: Igor Grinberg <grinberg@compulab.co.il> > > > Cc: Tom Rini <trini@konsulko.com> > > > Cc: Simon Glass <sjg@chromium.org> > > > Reviewed-by: Tom Rini <trini@konsulko.com> > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > So, a problem with this patch is that we push the x600 board, which is > > an 8KiB SPL target, over the line. I feel like maybe we need a > > follow-up patch that makes announcing depend not on libcommon (which > > x600 needs) but something else to know that there's a reason to > > announce. > > Based on the content of your reply I'm guessing you're referring to the > next patch, not this one. Oh yes, oops. > I suppose that announcing can be made into an optional feature. However, > I also think that since printing is an optional feature that can greatly > increase binary size, it shouldn't be coupled with other, often > non-optional libcommon features the way it currently is via > CONFIG_SPL_LIBCOMMON_SUPPORT. The best fix in my opinion would be to > implement a way to exclude printing support from SPL even if libcommon > is included (CONFIG_SPL_SILENT that replaces printfs with empty stubs?). > > This will also make it possible to remove all those #ifdef > CONFIG_SPL_LIBCOMMON_SUPPORT checks that appear all over the SPL code. So the reason I'm thinking that we want this announce thing separate is that (due to the way you re-worked it I think, based on Simons' request) this added a huge amount of space. We went from OK to overflowing by more than 1KiB. So I'm not immediately sure that we can regain that space with a more (space) efficient printing infrastructure.
On Thu, Nov 19, 2015 at 12:46:58PM +0100, Stefan Roese wrote: > On 19.11.2015 12:19, Nikita Kiryanov wrote: > >Hi Tom, > > > >On Wed, Nov 18, 2015 at 05:33:20PM -0500, Tom Rini wrote: > >>On Sun, Nov 08, 2015 at 05:11:51PM +0200, Nikita Kiryanov wrote: > >> > >>>Introduce spl_boot_list array, which defines a list of boot devices > >>>that SPL will try before hanging. By default this list will consist > >>>of only spl_boot_device(), but board_boot_order() can be overridden > >>>by board code to populate the array with custom values. > >>> > >>>Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > >>>Cc: Igor Grinberg <grinberg@compulab.co.il> > >>>Cc: Tom Rini <trini@konsulko.com> > >>>Cc: Simon Glass <sjg@chromium.org> > >>>Reviewed-by: Tom Rini <trini@konsulko.com> > >>>Reviewed-by: Simon Glass <sjg@chromium.org> > >> > >>So, a problem with this patch is that we push the x600 board, which is > >>an 8KiB SPL target, over the line. I feel like maybe we need a > >>follow-up patch that makes announcing depend not on libcommon (which > >>x600 needs) but something else to know that there's a reason to > >>announce. > > > >Based on the content of your reply I'm guessing you're referring to the > >next patch, not this one. > > > >I suppose that announcing can be made into an optional feature. However, > >I also think that since printing is an optional feature that can greatly > >increase binary size, it shouldn't be coupled with other, often > >non-optional libcommon features the way it currently is via > >CONFIG_SPL_LIBCOMMON_SUPPORT. The best fix in my opinion would be to > >implement a way to exclude printing support from SPL even if libcommon > >is included (CONFIG_SPL_SILENT that replaces printfs with empty stubs?). > > > >This will also make it possible to remove all those #ifdef > >CONFIG_SPL_LIBCOMMON_SUPPORT checks that appear all over the SPL code. > > I think that my recently posted tiny-printf patches: > > https://patchwork.ozlabs.org/patch/545034/ > https://patchwork.ozlabs.org/patch/545033/ > https://patchwork.ozlabs.org/patch/545036/ > https://patchwork.ozlabs.org/patch/545035/ > > can solve this size issue on x600 (and perhaps other) board. If you can see if x600 builds again in mainline that would be good :)
Hi Tom, On 19.11.2015 23:11, Tom Rini wrote: > On Thu, Nov 19, 2015 at 12:46:58PM +0100, Stefan Roese wrote: >> On 19.11.2015 12:19, Nikita Kiryanov wrote: >>> Hi Tom, >>> >>> On Wed, Nov 18, 2015 at 05:33:20PM -0500, Tom Rini wrote: >>>> On Sun, Nov 08, 2015 at 05:11:51PM +0200, Nikita Kiryanov wrote: >>>> >>>>> Introduce spl_boot_list array, which defines a list of boot devices >>>>> that SPL will try before hanging. By default this list will consist >>>>> of only spl_boot_device(), but board_boot_order() can be overridden >>>>> by board code to populate the array with custom values. >>>>> >>>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >>>>> Cc: Igor Grinberg <grinberg@compulab.co.il> >>>>> Cc: Tom Rini <trini@konsulko.com> >>>>> Cc: Simon Glass <sjg@chromium.org> >>>>> Reviewed-by: Tom Rini <trini@konsulko.com> >>>>> Reviewed-by: Simon Glass <sjg@chromium.org> >>>> >>>> So, a problem with this patch is that we push the x600 board, which is >>>> an 8KiB SPL target, over the line. I feel like maybe we need a >>>> follow-up patch that makes announcing depend not on libcommon (which >>>> x600 needs) but something else to know that there's a reason to >>>> announce. >>> >>> Based on the content of your reply I'm guessing you're referring to the >>> next patch, not this one. >>> >>> I suppose that announcing can be made into an optional feature. However, >>> I also think that since printing is an optional feature that can greatly >>> increase binary size, it shouldn't be coupled with other, often >>> non-optional libcommon features the way it currently is via >>> CONFIG_SPL_LIBCOMMON_SUPPORT. The best fix in my opinion would be to >>> implement a way to exclude printing support from SPL even if libcommon >>> is included (CONFIG_SPL_SILENT that replaces printfs with empty stubs?). >>> >>> This will also make it possible to remove all those #ifdef >>> CONFIG_SPL_LIBCOMMON_SUPPORT checks that appear all over the SPL code. >> >> I think that my recently posted tiny-printf patches: >> >> https://patchwork.ozlabs.org/patch/545034/ >> https://patchwork.ozlabs.org/patch/545033/ >> https://patchwork.ozlabs.org/patch/545036/ >> https://patchwork.ozlabs.org/patch/545035/ >> >> can solve this size issue on x600 (and perhaps other) board. > > If you can see if x600 builds again in mainline that would be good :) Yes, I can confirm, that build with the tiny-printf fixes the build issue on x600. So once you add this tiny-printf patchset, I'll send a patch to move x600 over to use this smaller version. Thanks, Stefan
diff --git a/common/spl/spl.c b/common/spl/spl.c index 56fccca..7913c52 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -178,6 +178,23 @@ int spl_init(void) return 0; } +#ifndef BOOT_DEVICE_NONE +#define BOOT_DEVICE_NONE 0xdeadbeef +#endif + +static u32 spl_boot_list[] = { + BOOT_DEVICE_NONE, + BOOT_DEVICE_NONE, + BOOT_DEVICE_NONE, + BOOT_DEVICE_NONE, + BOOT_DEVICE_NONE, +}; + +__weak void board_boot_order(u32 *spl_boot_list) +{ + spl_boot_list[0] = spl_boot_device(); +} + static int spl_load_image(u32 boot_device) { switch (boot_device) { @@ -247,7 +264,7 @@ static int spl_load_image(u32 boot_device) void board_init_r(gd_t *dummy1, ulong dummy2) { - u32 boot_device; + int i; debug(">>spl:board_init_r()\n"); @@ -272,10 +289,18 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_board_init(); #endif - boot_device = spl_boot_device(); - debug("boot device - %d\n", boot_device); - if (spl_load_image(boot_device)) + board_boot_order(spl_boot_list); + for (i = 0; i < ARRAY_SIZE(spl_boot_list) && + spl_boot_list[i] != BOOT_DEVICE_NONE; i++) { + if (!spl_load_image(spl_boot_list[i])) + break; + } + + if (i == ARRAY_SIZE(spl_boot_list) || + spl_boot_list[i] == BOOT_DEVICE_NONE) { + puts("SPL: failed to boot from all boot devices\n"); hang(); + } switch (spl_image.os) { case IH_OS_U_BOOT: