Dear Gary Jennejohn,
In message 20080914164107.32a9fce1@peedub.jennejohn.org 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, garyj@denx.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 <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