On Thu, 30 Oct 2008 12:59:49 +0100 Wolfgang Denk wd@denx.de wrote:
In message 20081030112119.59036e03@ernst.jennejohn.org you wrote:
Modifications to support console multiplexing. This is controlled using CONFIG_SYS_CONSOLE_MUX in the board configuration file.
Is it correct to assume that this patch version does not yet include any changes resultiung from the discussion we had yesterday how to optimize timing behaviour?
Yes. It also doesn't appear to me that this has been resolved yet.
diff --git a/common/Makefile b/common/Makefile index f00cbd9..0296a8a 100644 --- a/common/Makefile +++ b/common/Makefile @@ -156,6 +156,7 @@ COBJS-$(CONFIG_LYNXKDI) += lynxkdi.o COBJS-$(CONFIG_UPDATE_TFTP) += update.o COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o COBJS-$(CONFIG_DDR_SPD) += ddr_spd.o +COBJS-$(CONFIG_SYS_CONSOLE_MUX) += iomux.o
As this is a user configurable option, it should be named CONFIG_CONSOLE_MUX. Please remove the "SYS_" part. And please keep the list sorted.
I chose this analogously to CONFIG_SYS_CONSOLE_IS_IN_ENV. Obviously I misunderstand something about how these names are chosen. Please enlighten me.
@@ -115,7 +182,41 @@ void serial_printf (const char *fmt, ...) int fgetc (int file) { if (file < MAX_FILES) +#if defined(CONFIG_SYS_CONSOLE_MUX)
- {
Please move the braces outside the #ifdef.
OK
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 rather slow.We discussed a potential solution to this problem yesterday. Do you want to try this out in your implementation?
See my comment above.
* 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;}Please do not re-invent the wheel. If the watchdog triggering rate needs to be limited then please use the CONFIG_WD_MAX_RATE parameter as it was done in commit d32a874b "lwmon5 watchdog: limit trigger rate".
Sounds good to me. I wasn't aware of CONFIG_WD_MAX_RATE. There are way too many undocumented options in u-boot.
@@ -123,7 +224,11 @@ int fgetc (int file) int ftstc (int file) { if (file < MAX_FILES) +#if defined(CONFIG_SYS_CONSOLE_MUX)
return iomux_tstc(file);+#else return stdio_devices[file]->tstc (); +#endif
return -1; } @@ -131,13 +236,21 @@ int ftstc (int file) void fputc (int file, const char c) { if (file < MAX_FILES) +#if defined(CONFIG_SYS_CONSOLE_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_SYS_CONSOLE_MUX)
iomux_puts(file, s);+#else stdio_devices[file]->puts (s); +#endif }
Would it be possible to just set
stdio_devices[file]->tstc = iomux_tstc; stdio_devices[file]->putc = iomux_putc; stdio_devices[file]->puts = iomux_puts;
?
This would eliminate the need for these #ifdef's (and possibly a few more) ?
@@ -438,15 +562,34 @@ int console_init_r (void) } /* Initializes output console first */ if (outputdev != NULL) { +#ifdef CONFIG_SYS_CONSOLE_MUX
/* need to set a console if not done above. */iomux_doenv(stdout, outputdev->name);+#else console_setfile (stdout, outputdev); +#endif } if (errdev != NULL) { +#ifdef CONFIG_SYS_CONSOLE_MUX
/* need to set a console if not done above. */iomux_doenv(stderr, errdev->name);+#else console_setfile (stderr, errdev); +#endif } if (inputdev != NULL) { +#ifdef CONFIG_SYS_CONSOLE_MUX
/* need to set a console if not done above. */iomux_doenv(stdin, inputdev->name);+#else console_setfile (stdin, inputdev); +#endif }
See above. Maybe these #ifdef's could be eliminated as well?
@@ -455,21 +598,33 @@ int console_init_r (void) if (stdio_devices[stdin] == NULL) { puts ("No input devices available!\n"); } else { +#ifdef CONFIG_SYS_CONSOLE_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_SYS_CONSOLE_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_SYS_CONSOLE_MUX
iomux_printdevs(stderr);+#else printf ("%s\n", stdio_devices[stderr]->name); +#endif }
And these, too?
Seems like a good idea. I'll look at it.
- int i;
- device_t *dev;
- for (i = 0; i < cd_count[console]; i++) {
dev = console_devices[console][i];serial_printf("%s ", dev->name);- }
- serial_printf("\n");
+}
The old code uses plain printf(), while your new version explicitely calls serial_printf(). I gues sthis is intentional? What is the rationale behind it?
To avoid writing to e.g. nc before it's ready for output.
+/* This tries to preserve the old list if an error occurs. */ +int iomux_doenv(const int console, const char *arg) +{
- char *console_args, *temp, **start;
- int i, j, io_flag, cs_idx;
- device_t *dev;
- device_t **cons_set;
+#ifdef CONFIG_SYS_CONSOLE_IS_IN_ENV
- if (arg == NULL)
return 1;+#endif
Do we need this special case test?
- console_args = strdup(arg);
- if (console_args == NULL)
return 1;I think this test here would catch it as well?
Ah, I see that it's safe to pass NULL to strdup(). OK, I can simplify the code knowing that.
- /*
* 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).** Have to do this twice - once to count the number of* commas and then again to populate start.*/- i = 0;
- temp = console_args;
- for (;;) {
temp = strchr(temp, ',');if (temp != NULL) {i++;temp++;continue;}/* There's always one entry more than the number of commas. */i++;break;- }
- start = (char **)malloc(i * sizeof(char *));
- if (start == NULL) {
free(console_args);return 1;- }
- i = 0;
- start[0] = console_args;
- for (;;) {
temp = strchr(start[i], ',');if (temp != NULL) {i++;*temp = '\0';start[i] = temp + 1;continue;}/* There's always one entry more than the number of commas. */i++;break;- }
I suggest to swap logic around as this allows for easier to read code:
for (;;) { temp = strchr(start[i++], ','); if (temp == NULL) break; *temp = '\0'; start[i] = temp + 1; }
Seems reasonable, but I have to consider it in more detail.
- cons_set = (device_t **)calloc(1, i * sizeof(device_t *));
Why not:
cons_set = (device_t **)calloc(i, sizeof(device_t *));
?
Hadn't thought of that. A good idea.
- if (cons_set == NULL) {
free(start);free(console_args);return 1;- }
- switch (console) {
- case stdin:
io_flag = DEV_FLAGS_INPUT;break;- case stdout:
- case stderr:
io_flag = DEV_FLAGS_OUTPUT;break;- default:
return 1;Memory leak? free start & console_args & cons_set ??
Yes, could be. I added default as an afterthought and didn't consider all its implications.
+Two new files, common/iomux.c and include/iomux.h, contain the heart +of the environment setting implementation.
It seems to me that they do much more than just setting environment variables?
Their whole reason for their existence is to handle setenv.
+The execution is then in common/cmd_nvedit.c and common/console.c.
Hm... but cmd_nvedit.c really is for environment variables handling only?
Yeah, the wording could be improved.
+A user can use a comma-separated list of devices to set stdin, stdout +and stderr. For example: setenv stdin serial,nc. NOTE: No spaces
Maybe it's better to quote the example to make it stand out:
...and stderr. For example: "setenv stdin serial,nc". NOTE: No spaces
?
I thought about that myself but didn't do it.
+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 +copy&paste will normally not work, even if serial is set as the only device +for stdin, stdout and stderr.
You should probably mention the conditions where you tested this with such results. It probably depends on the system performance and the console baud rate, doesn't it?
True.
+Using nc as a stdin device results in even more overhead because nc_tstc() +is quite slow.
There was discussion before how that could be accelerated. Has any work spent on actually implementing / testing such measures?
Not by me.
diff --git a/include/iomux.h b/include/iomux.h new file mode 100644 index 0000000..75b66e2 --- /dev/null +++ b/include/iomux.h
...
+/*
- Avoid invoking WATCHDOG via udelay() on every pass through the loop
- in fgetc(). Note this is all based on guesswork.
- */
+#define COUNT_TIL_UDELAY 10000
I think we should use a more determinitic approach. See comment above.
I agree.
--- Gary Jennejohn ********************************************************************* DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de *********************************************************************