Re: [U-Boot] [PATCH 1/2 V2] IOMUX: Add console multiplexing support.

Dear Gary Jennejohn,

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?

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.

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index d280cb0..7a49296 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_SYS_CONSOLE_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_SYS_CONSOLE_MUX */ }

Just to be sure - does your new iomux_doenv() code handle the situation that is usually handled by serial_assign(), too (if CONFIG_SERIAL_MULTI is set) ?

diff --git a/common/console.c b/common/console.c index 6f0846f..c89020d 100644 --- a/common/console.c +++ b/common/console.c @@ -93,6 +93,73 @@ static int console_setfile (int file, device_t * dev) return error; }

+#if defined(CONFIG_SYS_CONSOLE_MUX) +/** Console I/O multiplexing *******************************************/

+static device_t *tstcdev; +device_t **console_devices[MAX_FILES]; +int cd_count[MAX_FILES];

+/*

• • This depends on tstc() always being called before getc().

I don't think this is a valid assumption.

• • No attempt is made to demultiplex multiple input sources.
• */

+static int iomux_getc(void) +{

• unsigned char ret;
• ret = tstcdev->getc();
• tstcdev = NULL;
• return ret;

+}

If iomux_getc() gets called twice we crash U-Boot because of a NULL pointer dereference?

+static int iomux_tstc(int file) +{

• int i, ret;
• device_t *dev;
• disable_ctrlc(1);
• for (i = 0; i < cd_count[file]; i++) {

• dev = console_devices[file][i];
  

• if (dev->tstc != NULL) {
  

• 	ret = dev->tstc();
  

• 	if (ret > 0) {
  

• 		tstcdev = dev;
  

• 		disable_ctrlc(0);
  

• 		return ret;
  

• 	}
  

• }
  

• }
• disable_ctrlc(0);
• return 0;

+}

If I'm reading this correctly, then there is not even a guarantee that tstcdev gets set in this function, so the protection against a NULL pointer in iomux_getc() seems even more vital to 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.

• 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?

• 	 * 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".

@@ -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?

diff --git a/common/iomux.c b/common/iomux.c new file mode 100644 index 0000000..b83bdb5 --- /dev/null +++ b/common/iomux.c @@ -0,0 +1,170 @@ +/*

• • (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_SYS_CONSOLE_MUX +void iomux_printdevs(const int console) +{

• 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?

+/* 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?

• /*

• * 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; }

• cons_set = (device_t **)calloc(1, i * sizeof(device_t *));

Why not:

cons_set = (device_t **)calloc(i, sizeof(device_t *));

?

• 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 ??

diff --git a/doc/README.iomux b/doc/README.iomux new file mode 100644 index 0000000..bff75fa --- /dev/null +++ b/doc/README.iomux @@ -0,0 +1,89 @@ +/*

• • (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
• */

+U-Boot console multiplexing +===========================

+HOW CONSOLE MULTIPLEXING WORKS +------------------------------

+This functionality is controlled with CONFIG_SYS_CONSOLE_MUX in the board +configuration file.

+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?

+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?

+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

?

+Thus, a user can see the ouput for any device registered for stdout +or stderr on all devices registered for stdout or stderr. As an +example, if stdin=serial,nc and stdout=serial,nc then all output

Quote examples ?

+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?

+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?

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.

Thanks.

Best regards,

Wolfgang Denk