| Message ID | 1328047438-26294-1-git-send-email-trini@ti.com |
|---|---|
| State | Accepted |
| Commit | 24de357a30dbabf8e0eb5acb43397851b0bc53fb |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Tom, Hi Matt, On Tuesday, January 31, 2012, Tom Rini <trini@ti.com> wrote: > From: Matt Porter <mporter@ti.com> > > Adds support for loading U-Boot from UART using YMODEM protocol. > If YMODEM support is enabled in SPL and the romcode indicates > that SPL loaded via UART then SPL will wait for start of a > YMODEM transfer via the console port. :-) I like this feature! I tried something similar a few month ago for the da850evm and my own board, it worked but I never had time to finished it and submit it, it was just a hack. For my tests I used the UART loader utility from TI to load the SPL and then minicom for loading u-boot. Do you have an automated solution for this? Would be nice to have something like this in tools/. > Signed-off-by: Matt Porter <mporter@ti.com> > Signed-off-by: Tom Rini <trini@ti.com> > --- > arch/arm/cpu/armv7/omap-common/Makefile | 3 + > arch/arm/cpu/armv7/omap-common/spl.c | 5 ++ > arch/arm/cpu/armv7/omap-common/spl_ymodem.c | 76 +++++++++++++++++++++++++++ > arch/arm/include/asm/omap_common.h | 3 + > common/Makefile | 3 + > lib/Makefile | 3 + > 6 files changed, 93 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/cpu/armv7/omap-common/spl_ymodem.c > [...] > diff --git a/arch/arm/cpu/armv7/omap-common/spl_ymodem.c b/arch/arm/cpu/armv7/omap-common/spl_ymodem.c Could this be somewhere where it's accessible for other SoC families like davinci? E.g. We have nand_spl_load.c in drivers/mtd/nand and something similar for SPI flash. Regards, Christian > new file mode 100644 > index 0000000..47663f7 > --- /dev/null > +++ b/arch/arm/cpu/armv7/omap-common/spl_ymodem.c > @@ -0,0 +1,76 @@ > +/* > + * (C) Copyright 2000-2004 > + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. > + * > + * (C) Copyright 2011 > + * Texas Instruments, <www.ti.com> > + * > + * Matt Porter <mporter@ti.com> > + * > + * 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 <xyzModem.h> > +#include <asm/u-boot.h> > +#include <asm/utils.h> > +#include <asm/arch/sys_proto.h> > +#include <asm/omap_common.h> > + > +#define BUF_SIZE 1024 > + > +static int getcymodem(void) { > + if (tstc()) > + return (getc()); > + return -1; > +} > + > +void spl_ymodem_load_image(void) > +{ > + int size = 0; > + int err; > + int res; > + int ret; > + connection_info_t info; > + char buf[BUF_SIZE]; > + ulong store_addr = ~0; > + ulong addr = 0; > + > + info.mode = xyzModem_ymodem; > + ret = xyzModem_stream_open(&info, &err); > + > + if (!ret) { > + while ((res = > + xyzModem_stream_read(buf, BUF_SIZE, &err)) > 0) { > + if (addr == 0) > + spl_parse_image_header((struct image_header *)buf); > + store_addr = addr + spl_image.load_addr; > + size += res; > + addr += res; > + memcpy((char *)(store_addr), buf, res); > + } > + } else { > + printf("spl: ymodem err - %s\n", xyzModem_error(err)); > + hang(); > + } > + > + xyzModem_stream_close(&err); > + xyzModem_stream_terminate(false, &getcymodem); > + > + printf("Loaded %d bytes\n", size); > +}
Dear Tom Rini, In message <1328047438-26294-1-git-send-email-trini@ti.com> you wrote: > From: Matt Porter <mporter@ti.com> > > Adds support for loading U-Boot from UART using YMODEM protocol. > If YMODEM support is enabled in SPL and the romcode indicates > that SPL loaded via UART then SPL will wait for start of a > YMODEM transfer via the console port. Does it really make sense to add all kind of bloat to the SPL? What originally has been designed to be minimal code now start to pull in all kinds of other crap, that has never been designed to run before relocation. Best regards, Wolfgang Denk
On Wed, Feb 1, 2012 at 3:24 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Tom Rini, > > In message <1328047438-26294-1-git-send-email-trini@ti.com> you wrote: >> From: Matt Porter <mporter@ti.com> >> >> Adds support for loading U-Boot from UART using YMODEM protocol. >> If YMODEM support is enabled in SPL and the romcode indicates >> that SPL loaded via UART then SPL will wait for start of a >> YMODEM transfer via the console port. > > Does it really make sense to add all kind of bloat to the SPL? > > What originally has been designed to be minimal code now start to pull > in all kinds of other crap, that has never been designed to run before > relocation. Boards that don't need it can opt out. We use this internally for automated testing of U-Boot on hardware (and I'm setting up the same thing locally).
On Tue, Jan 31, 2012 at 11:40 PM, Christian Riesch <christian.riesch@omicron.at> wrote: > Hi Tom, Hi Matt, > > On Tuesday, January 31, 2012, Tom Rini <trini@ti.com> wrote: >> From: Matt Porter <mporter@ti.com> >> >> Adds support for loading U-Boot from UART using YMODEM protocol. >> If YMODEM support is enabled in SPL and the romcode indicates >> that SPL loaded via UART then SPL will wait for start of a >> YMODEM transfer via the console port. > > :-) I like this feature! > > I tried something similar a few month ago for the da850evm and my own > board, it worked but I never had time to finished it and submit it, it was > just a hack. > > For my tests I used the UART loader utility from TI to load the SPL and > then minicom for loading u-boot. Do you have an automated solution for > this? Would be nice to have something like this in tools/. IIRC the da850evm ROM needs a special tool to load via UART. omap4/5/am33xx ROM does XMODEM for real so 'sx' works fine. [snip] > Could this be somewhere where it's accessible for other SoC families like > davinci? E.g. We have nand_spl_load.c in drivers/mtd/nand and something > similar for SPI flash. In theory, sure, just need a location to place it at.
On 02/01/2012 04:24 AM, Wolfgang Denk wrote: > Dear Tom Rini, > > In message <1328047438-26294-1-git-send-email-trini@ti.com> you wrote: >> From: Matt Porter <mporter@ti.com> >> >> Adds support for loading U-Boot from UART using YMODEM protocol. >> If YMODEM support is enabled in SPL and the romcode indicates >> that SPL loaded via UART then SPL will wait for start of a >> YMODEM transfer via the console port. > > Does it really make sense to add all kind of bloat to the SPL? > > What originally has been designed to be minimal code now start to pull > in all kinds of other crap, What's the alternative -- another name and separate makefile infrastructure for non-minimal initial loaders? It's important that SPL be *able* to produce very small images when needed, but its purpose has grown beyond that. > that has never been designed to run before relocation. At least some SPLs do relocation. -Scott
On Wed, Feb 1, 2012 at 11:14 AM, Scott Wood <scottwood@freescale.com> wrote: > On 02/01/2012 04:24 AM, Wolfgang Denk wrote: >> Dear Tom Rini, >> >> In message <1328047438-26294-1-git-send-email-trini@ti.com> you wrote: >>> From: Matt Porter <mporter@ti.com> >>> >>> Adds support for loading U-Boot from UART using YMODEM protocol. >>> If YMODEM support is enabled in SPL and the romcode indicates >>> that SPL loaded via UART then SPL will wait for start of a >>> YMODEM transfer via the console port. >> >> Does it really make sense to add all kind of bloat to the SPL? >> >> What originally has been designed to be minimal code now start to pull >> in all kinds of other crap, > > What's the alternative -- another name and separate makefile > infrastructure for non-minimal initial loaders? > > It's important that SPL be *able* to produce very small images when > needed, but its purpose has grown beyond that. I talked with Wolfgang a little on IRC on this, and we didn't reach a conclusion nor come up with an alternative, but I guess the problem is we need to figure something out, now. We've hit a roadblock on "allow UART loading" and regret over keeping MMC+VFAT working. How bad are things going to get when patches pop up for USB (AM33xx ROM allows for RNDIS USB, iirc) ? Maybe we need to separate out the systems which are very resource constrained from the ones that aren't, share code where we can and make that code obviously marked as such. >> that has never been designed to run before relocation. > > At least some SPLs do relocation. Or don't need to exactly.
Dear Tom Rini, In message <CA+M6bXmKum1B39RVcJ+jgDRmFCFr5sToZYDUVXnM4NfUZ-XQaQ@mail.gmail.com> you wrote: > > > What originally has been designed to be minimal code now start to pull > > in all kinds of other crap, that has never been designed to run before > > relocation. > > Boards that don't need it can opt out. We use this internally for > automated testing of U-Boot on hardware (and I'm setting up the same > thing locally). I don't deny that it's useful. And it may even ork fine in your specific case. But the approach is wrong. This code has never been meant to run before relocation. I object against opening this pandora box. If you have so ample resources to run such code, then load a minimal configuration of U-Boot. Best regards, Wolfgang Denk
On Wed, Feb 1, 2012 at 12:37 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Tom Rini, > > In message <CA+M6bXmKum1B39RVcJ+jgDRmFCFr5sToZYDUVXnM4NfUZ-XQaQ@mail.gmail.com> you wrote: >> >> > What originally has been designed to be minimal code now start to pull >> > in all kinds of other crap, that has never been designed to run before >> > relocation. >> >> Boards that don't need it can opt out. We use this internally for >> automated testing of U-Boot on hardware (and I'm setting up the same >> thing locally). > > I don't deny that it's useful. And it may even ork fine in your > specific case. > > But the approach is wrong. This code has never been meant to run > before relocation. > > I object against opening this pandora box. > > If you have so ample resources to run such code, then load a minimal > configuration of U-Boot. Is this a change of heart then? http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/109170
Dear Scott Wood, In message <4F29811D.5080207@freescale.com> you wrote: > > What's the alternative -- another name and separate makefile > infrastructure for non-minimal initial loaders? No, configura and use a small U-Boot image where the normal execution environment can be guaranteed. > It's important that SPL be *able* to produce very small images when > needed, but its purpose has grown beyond that. But it's restrictions have not. You know how things work - people take exsiting code examples from one system and expect them to work on another - and they don't. The SPL code that works fine today breaks tomorrow because someone changes some part of code in an apparently unrelated area of the code. He could not know his changes would cause trouble because this code was not meant to run in restricted environments. Etc. etc. We once had a very strict rule for which code was allowed before relocation (or you may call it in "regular U-Boot enviropnment"), and which was not. We are starting to obliterate these boundaries. This is dangerous, and once you start, impossible to control. It was a mistake of mine to allow the use of malloc() in this environment. I am not willing to let this grow out of control. If such fancy features are needed in the SPL, then new methods must be developed that give the user a clear indication if what he is doing is actually legal and will result in working code. I don't have a solution for this, but I am convinced that this approach is wrong, and should be stopped rather now than later. Best regards, Wolfgang Denk
Dear Tom Rini, In message <CA+M6bXkXA+356AFLGDS55o=7bZuuQCcPCXAVEjo9AWDENn3wfQ@mail.gmail.com> you wrote: > > Maybe we need to separate out the systems which are very resource > constrained from the ones that aren't, share code where we can and > make that code obviously marked as such. I think it's the other way round. You ned to find out which code is safe to call from a restricted environment, and track down the linker dependencies to make sure you don't pull something in that breaks. Which means you need to review all code and mark it's requirements. Best regards, Wolfgang Denk
Dear Tom Rini, In message <CA+M6bXnb5k6gA8kbqr0BQ+JLMVFXpzCT+g+dEXfJx307fYn=ew@mail.gmail.com> you wrote: > > > I object against opening this pandora box. > > > > If you have so ample resources to run such code, then load a minimal > > configuration of U-Boot. > > Is this a change of heart then? No - I never was happy about pulling arbitrary code into SPL. > http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/109170 I don't see how this is related? Best regards, Wolfgang Denk
On Wed, Feb 1, 2012 at 12:51 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Tom Rini, > > In message <CA+M6bXnb5k6gA8kbqr0BQ+JLMVFXpzCT+g+dEXfJx307fYn=ew@mail.gmail.com> you wrote: >> >> > I object against opening this pandora box. >> > >> > If you have so ample resources to run such code, then load a minimal >> > configuration of U-Boot. >> >> Is this a change of heart then? > > No - I never was happy about pulling arbitrary code into SPL. > >> http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/109170 > > I don't see how this is related? If I follow you right, to my question of "How would a user make use of 'ROM loads next loader via USB' program?" your answer is "Have it load a normal U-Boot that's minimally configured". That link is TI saying "we have split things up such that ROM loads U-Boot-Min (a minimally configured U-Boot) which can then load a full U-Boot". You NAK'd the idea then. Have you changed your mind? If so, where are we going to draw the demarcation between "OK for SPL" and "Must use a full U-Boot" ?
On Wed, Feb 1, 2012 at 12:49 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Tom Rini, > > In message <CA+M6bXkXA+356AFLGDS55o=7bZuuQCcPCXAVEjo9AWDENn3wfQ@mail.gmail.com> you wrote: >> >> Maybe we need to separate out the systems which are very resource >> constrained from the ones that aren't, share code where we can and >> make that code obviously marked as such. > > I think it's the other way round. You ned to find out which code is > safe to call from a restricted environment, and track down the linker > dependencies to make sure you don't pull something in that breaks. > Which means you need to review all code and mark it's requirements. So are you then open to playing more linker games, at least for the boot modes where this is rather feasible? (.data.splsafe / .text.splsafe and head.o)
diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile index 3f7a0b2..447fcd5 100644 --- a/arch/arm/cpu/armv7/omap-common/Makefile +++ b/arch/arm/cpu/armv7/omap-common/Makefile @@ -52,6 +52,9 @@ endif ifdef CONFIG_SPL_MMC_SUPPORT COBJS += spl_mmc.o endif +ifdef CONFIG_SPL_YMODEM_SUPPORT +COBJS += spl_ymodem.o +endif endif ifndef CONFIG_SPL_BUILD diff --git a/arch/arm/cpu/armv7/omap-common/spl.c b/arch/arm/cpu/armv7/omap-common/spl.c index 9c1f7e3..9018787 100644 --- a/arch/arm/cpu/armv7/omap-common/spl.c +++ b/arch/arm/cpu/armv7/omap-common/spl.c @@ -134,6 +134,11 @@ void board_init_r(gd_t *id, ulong dummy) spl_nand_load_image(); break; #endif +#ifdef CONFIG_SPL_YMODEM_SUPPORT + case BOOT_DEVICE_UART: + spl_ymodem_load_image(); + break; +#endif default: printf("SPL: Un-supported Boot Device - %d!!!\n", boot_device); hang(); diff --git a/arch/arm/cpu/armv7/omap-common/spl_ymodem.c b/arch/arm/cpu/armv7/omap-common/spl_ymodem.c new file mode 100644 index 0000000..47663f7 --- /dev/null +++ b/arch/arm/cpu/armv7/omap-common/spl_ymodem.c @@ -0,0 +1,76 @@ +/* + * (C) Copyright 2000-2004 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * (C) Copyright 2011 + * Texas Instruments, <www.ti.com> + * + * Matt Porter <mporter@ti.com> + * + * 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 <xyzModem.h> +#include <asm/u-boot.h> +#include <asm/utils.h> +#include <asm/arch/sys_proto.h> +#include <asm/omap_common.h> + +#define BUF_SIZE 1024 + +static int getcymodem(void) { + if (tstc()) + return (getc()); + return -1; +} + +void spl_ymodem_load_image(void) +{ + int size = 0; + int err; + int res; + int ret; + connection_info_t info; + char buf[BUF_SIZE]; + ulong store_addr = ~0; + ulong addr = 0; + + info.mode = xyzModem_ymodem; + ret = xyzModem_stream_open(&info, &err); + + if (!ret) { + while ((res = + xyzModem_stream_read(buf, BUF_SIZE, &err)) > 0) { + if (addr == 0) + spl_parse_image_header((struct image_header *)buf); + store_addr = addr + spl_image.load_addr; + size += res; + addr += res; + memcpy((char *)(store_addr), buf, res); + } + } else { + printf("spl: ymodem err - %s\n", xyzModem_error(err)); + hang(); + } + + xyzModem_stream_close(&err); + xyzModem_stream_terminate(false, &getcymodem); + + printf("Loaded %d bytes\n", size); +} diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h index 34bec45..25f95b4 100644 --- a/arch/arm/include/asm/omap_common.h +++ b/arch/arm/include/asm/omap_common.h @@ -99,6 +99,9 @@ void spl_nand_load_image(void); /* MMC SPL functions */ void spl_mmc_load_image(void); +/* YMODEM SPL functions */ +void spl_ymodem_load_image(void); + #ifdef CONFIG_SPL_BOARD_INIT void spl_board_init(void); #endif diff --git a/common/Makefile b/common/Makefile index 2d9ae8c..0189aac 100644 --- a/common/Makefile +++ b/common/Makefile @@ -184,6 +184,9 @@ COBJS-$(CONFIG_UPDATE_TFTP) += update.o COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o endif +ifdef CONFIG_SPL_BUILD +COBJS-$(CONFIG_SPL_YMODEM_SUPPORT) += xyzModem.o +endif COBJS-y += console.o COBJS-y += dlmalloc.o COBJS-y += memsize.o diff --git a/lib/Makefile b/lib/Makefile index 35ba7ff..eb60b99 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -54,6 +54,9 @@ else COBJS-$(CONFIG_SPL_SPI_FLASH_SUPPORT) += display_options.o endif +ifdef CONFIG_SPL_BUILD +COBJS-$(CONFIG_SPL_YMODEM_SUPPORT) += crc16.o +endif COBJS-y += ctype.o COBJS-y += div64.o COBJS-y += string.o