Dear Gary Jennejohn, In message <[EMAIL PROTECTED]> you wrote: > > See doc/README.iomux for a general description of what this does.
Sorry, but this is not really a good commit message. Please explain at least in a shoprt summary what the patch is supposed to implement. > This is the first of two commits. The second commit touches net/eth.c > and has to go through the custodian, so I split it out for simplicity. Comments like this that are not suppoesed to be come part of the commit message should go below the '---' line. > diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c > index 637d6c9..6fc9313 100644 > --- a/common/cmd_nvedit.c > +++ b/common/cmd_nvedit.c > @@ -213,6 +213,11 @@ int _do_setenv (int flag, int argc, char *argv[]) > return 1; > } > > +#ifdef CONFIG_IO_MUX > + i = iomux_doenv(console, argv[2]); > + if (i) > + return i; > +#else > /* Try assigning specified device */ > if (console_assign (console, argv[2]) < 0) > return 1; > @@ -221,6 +226,7 @@ int _do_setenv (int flag, int argc, char *argv[]) > if (serial_assign (argv[2]) < 0) > return 1; > #endif > +#endif /* CONFIG_IO_MUX */ > } > > /* > diff --git a/common/console.c b/common/console.c > index 56d9118..6158af9 100644 > --- a/common/console.c > +++ b/common/console.c > @@ -93,6 +93,78 @@ static int console_setfile (int file, device_t * dev) > return error; > } > > +#if defined(CONFIG_IO_MUX) > +/** Console I/O multiplexing *******************************************/ > + > +static device_t *tstcdev; > +device_t *console_devices[MAX_FILES][MAX_CONSARGS]; > + > +/* > + * This depends on tstc() always being called before getc(). > + * No attempt is made to demultiplex multiple input sources. > + */ > +static int iomux_getc(void) > +{ > + unsigned char ret; > + > + ret = tstcdev->getc(); > + tstcdev = NULL; > + return ret; > +} > + > +static int iomux_tstc(int file) > +{ > + int i, ret; > + device_t *dev; > + > + disable_ctrlc(1); > + for (i = 0; i < MAX_CONSARGS; i++) { > + dev = console_devices[file][i]; > + if (dev == NULL) > + break; > + if (dev->tstc != NULL) { > + ret = dev->tstc(); > + if (ret > 0) { > + tstcdev = dev; > + disable_ctrlc(0); > + return ret; > + } > + } > + } > + disable_ctrlc(0); > + > + return 0; > +} > + > +static void iomux_putc(int file, const char c) > +{ > + int i; > + device_t *dev; > + > + for (i = 0; i < MAX_CONSARGS; i++) { > + dev = console_devices[file][i]; > + if (dev == NULL) > + break; > + if (dev->putc != NULL) > + dev->putc(c); > + } > +} > + > +static void iomux_puts(int file, const char *s) > +{ > + int i; > + device_t *dev; > + > + for (i = 0; i < MAX_CONSARGS; i++) { > + dev = console_devices[file][i]; > + if (dev == NULL) > + break; > + if (dev->puts != NULL) > + dev->puts(s); > + } > +} > +#endif /* defined(CONFIG_IO_MUX) */ > + > /** U-Boot INITIAL CONSOLE-NOT COMPATIBLE FUNCTIONS > *************************/ > > void serial_printf (const char *fmt, ...) > @@ -115,7 +187,41 @@ void serial_printf (const char *fmt, ...) > int fgetc (int file) > { > if (file < MAX_FILES) > +#if defined(CONFIG_IO_MUX) > + { > + int cntr = 0; > + > + /* > + * Effectively poll for input wherever it may be available. > + */ > + for (;;) { > + /* > + * Upper layer may have already called tstc() so > + * check for that first. > + */ > + if (tstcdev != NULL) > + return iomux_getc(); > + iomux_tstc(file); > + /* > + * Even this is too slow for serial cut&paste due > + * to the overhead of calling tstc() then getc(). > + * It gets worse if nc is set as a console because > + * nc_tstc() is really slow. > + * > + * The idea behind this counter is to avoid calling > + * the watchdog via udelay() too often. > + * COUNT_TIL_UDELAY is defined in iomux.h and is just > + * a guesstimate. > + */ > + if (cntr++ > COUNT_TIL_UDELAY) { > + udelay(1); > + cntr = 0; > + } > + } > + } > +#else > return stdio_devices[file]->getc (); > +#endif > > return -1; > } > @@ -123,7 +229,11 @@ int fgetc (int file) > int ftstc (int file) > { > if (file < MAX_FILES) > +#if defined(CONFIG_IO_MUX) > + return iomux_tstc(file); > +#else > return stdio_devices[file]->tstc (); > +#endif > > return -1; > } > @@ -131,13 +241,21 @@ int ftstc (int file) > void fputc (int file, const char c) > { > if (file < MAX_FILES) > +#if defined(CONFIG_IO_MUX) > + iomux_putc(file, c); > +#else > stdio_devices[file]->putc (c); > +#endif > } > > void fputs (int file, const char *s) > { > if (file < MAX_FILES) > +#if defined(CONFIG_IO_MUX) > + iomux_puts(file, s); > +#else > stdio_devices[file]->puts (s); > +#endif > } > > void fprintf (int file, const char *fmt, ...) > @@ -407,6 +525,9 @@ int console_init_r (void) > #ifdef CFG_CONSOLE_ENV_OVERWRITE > int i; > #endif /* CFG_CONSOLE_ENV_OVERWRITE */ > +#ifdef CONFIG_IO_MUX > + int iomux_err = 0; > +#endif > > /* set default handlers at first */ > gd->jt[XF_getc] = serial_getc; > @@ -425,6 +546,14 @@ int console_init_r (void) > inputdev = search_device (DEV_FLAGS_INPUT, stdinname); > outputdev = search_device (DEV_FLAGS_OUTPUT, stdoutname); > errdev = search_device (DEV_FLAGS_OUTPUT, stderrname); > +#ifdef CONFIG_IO_MUX > + iomux_err = iomux_doenv(stdin, stdinname); > + iomux_err += iomux_doenv(stdout, stdoutname); > + iomux_err += iomux_doenv(stderr, stderrname); > + if (!iomux_err) > + /* Successful, so skip all the code below. */ > + goto done; > +#endif > } > /* if the devices are overwritten or not found, use default device */ > if (inputdev == NULL) { > @@ -439,15 +568,40 @@ int console_init_r (void) > /* Initializes output console first */ > if (outputdev != NULL) { > console_setfile (stdout, outputdev); > +#ifdef CONFIG_IO_MUX > + /* need to set a console if not done above. */ > + console_devices[stdout][0] = outputdev; > +#endif Could you please explain the "if not done above" part? I don't see where this would be done or not be done, i. e. don;t we always have to do this here? > } > if (errdev != NULL) { > console_setfile (stderr, errdev); > +#ifdef CONFIG_IO_MUX > + /* need to set a console if not done above. */ > + console_devices[stderr][0] = errdev; > +#endif > } > if (inputdev != NULL) { > console_setfile (stdin, inputdev); > +#ifdef CONFIG_IO_MUX > + /* need to set a console if not done above. */ > + console_devices[stdin][0] = inputdev; > +#endif > } > > +#ifdef CONFIG_IO_MUX > +#ifdef CONFIG_NETCONSOLE > + /* > + * Must do this very late in net/eth.c because a network device may > + * be set as a console at boot. > + */ > + gd->flags |= 0; > +#else > + gd->flags |= GD_FLG_DEVINIT; /* device initialization completed */ > +#endif /* CONFIG_NETCONSOLE */ > +done: > +#else > gd->flags |= GD_FLG_DEVINIT; /* device initialization completed */ > +#endif /* CONFIG_IO_MUX */ Wouldn't this be easier to read if written as: #if !(defined(CONFIG_IO_MUX) && defined(CONFIG_NETCONSOLE) gd->flags |= GD_FLG_DEVINIT; /* device initialization completed */ #endif done: ; ? But then, I don't see why the netconsole initialization depends in any way on the I/O mux feature - I'd expect that the behaviour is the same with or without that. > #ifndef CFG_CONSOLE_INFO_QUIET > /* Print information */ > @@ -455,21 +609,33 @@ int console_init_r (void) > if (stdio_devices[stdin] == NULL) { > puts ("No input devices available!\n"); > } else { > +#ifdef CONFIG_IO_MUX > + iomux_printdevs(stdin); > +#else > printf ("%s\n", stdio_devices[stdin]->name); > +#endif > } > > puts ("Out: "); > if (stdio_devices[stdout] == NULL) { > puts ("No output devices available!\n"); > } else { > +#ifdef CONFIG_IO_MUX > + iomux_printdevs(stdout); > +#else > printf ("%s\n", stdio_devices[stdout]->name); > +#endif > } > > puts ("Err: "); > if (stdio_devices[stderr] == NULL) { > puts ("No error devices available!\n"); > } else { > +#ifdef CONFIG_IO_MUX > + iomux_printdevs(stderr); > +#else > printf ("%s\n", stdio_devices[stderr]->name); > +#endif Instead of repeating this sequence 3 times, we should probably make it a function? > diff --git a/common/iomux.c b/common/iomux.c > new file mode 100644 > index 0000000..d62f7e4 > --- /dev/null > +++ b/common/iomux.c > @@ -0,0 +1,133 @@ > +/* > + * (C) Copyright 2008 > + * Gary Jennejohn, DENX Software Engineering GmbH, [EMAIL PROTECTED] > + * > + * 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 <serial.h> > +#include <malloc.h> > + > +#ifdef CONFIG_IO_MUX > +void iomux_printdevs(const int console) > +{ > + int i; > + device_t *dev; > + > + for (i = 0; i < MAX_CONSARGS; i++) { > + dev = console_devices[console][i]; > + if (dev == NULL) > + break; > + serial_printf("%s ", dev->name); > + } > + serial_printf("\n"); > +} > + > +int iomux_doenv(const int console, const char *arg) > +{ > + char *console_args, *temp, *start[MAX_CONSARGS]; > + int i, j, io_flag, cs_idx; > + device_t *dev; > + device_t *cons_set[MAX_CONSARGS]; > + > +#ifdef CFG_CONSOLE_IS_IN_ENV > + if (arg == NULL) > + return 1; > +#endif > + > + i = 0; > + console_args = strdup(arg); > + if (console_args == NULL) > + return 1; > + start[0] = console_args; > + /* > + * Check whether a comma separated list of devices was > + * entered and count how many devices were entered. > + * The array start[] has pointers to the beginning of > + * each device name (up to MAX_CONSARGS devices). > + */ > + for (;;) { > + temp = strchr(start[i], ','); > + if (temp != NULL) { > + i++; > + *temp = '\0'; > + if (i == MAX_CONSARGS) > + break; > + start[i] = temp + 1; > + continue; > + } > + break; > + } > + if (i != MAX_CONSARGS) > + i++; One could rewrite the whole part as start[0] = temp = strdup(arg); if (temp == NULL) return 1; for (i=0; i<MAX_CONSARGS-1; ) { temp = strchr(start[i], ','); if (temp == NULL) break; *temp = '\0'; start[++i] = ++temp; } What do you think? > + cs_idx = 0; > + memset((void *)cons_set, 0, CONSDEVS_LINE_SIZE); > + > + switch (console) { > + case stdin: > + io_flag = DEV_FLAGS_INPUT; > + break; > + case stdout: > + case stderr: > + io_flag = DEV_FLAGS_OUTPUT; > + break; > + default: "free(console_args);" missing here. > + return 1; > + } > + > + for (j = 0; j < i; j++) { > + /* > + * Check whether the device exists and is valid. > + * console_assign() also calls search_device(), > + * but I need the pointer to the device. > + */ > + dev = search_device(io_flag, start[j]); > + if (dev == NULL) > + continue; > + /* > + * Try assigning the specified device. > + * This could screw up the console settings for apps. > + */ > + if (console_assign(console, start[j]) < 0) > + continue; > +#ifdef CONFIG_SERIAL_MULTI > + /* > + * This was taken from common/cmd_nvedit.c. > + * This will never work because serial_assign() returns > + * 1 upon error, not -1. Ummm... if you know this doesn't work, why don't you fix it and make it work? > + * This would almost always return an error anyway because > + * serial_assign() expects the name of a serial device, like > + * serial_smc, but the user generally only wants to set serial. With CONFIG_SERIAL_MULTI enabled, the user probably does want to select specifically between several serial ports - that's the whole idea of defining CONFIG_SERIAL_MULTI, isn't it? > + */ > + if (serial_assign(start[j]) < 0) > + continue; > +#endif > + cons_set[cs_idx++] = dev; > + } > + free(console_args); > + /* failed to set any console */ > + if (cs_idx == 0) > + return 1; > + else No "else" needed here; this saves a level of indentation. > +CAVEATS > +------- > + > +Note that common/iomux.c calls console_assign() for every registered > +device as it is discovered. This means that the environment settings > +for application consoles will be set to the last device in the list. > + > +The overhead associated with calling tstc() and then getc() means that > +cut&paste will not work, even if serial is set as the only device for > +stdin, stdout and stderr. You mentioned that you tested this on MPC8xx systems. for these, the tstc() implementation (smc_tstc() resp. scc_tstc() in "cpu/mpc8xx/serial.c") essentially consists of 3 lines of code and boils down to testing a bit; there are no delays or so. Then why does the new code become so slow? The overhead of calling tstc() alone can not be the only reason? > +Using nc as a stdin device results in even more overhead because nc_tstc() > +is extremely slow. The slowdown is so extreme that it negatively impacts > +timers such as the autoboot countdown. That means the code is unusable as is and needs to be fixed first? Please repost after fixing. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [EMAIL PROTECTED] The price of curiosity is a terminal experience. - Terry Pratchett, _The Dark Side of the Sun_ _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot