[U-Boot] [PATCH 1/2 V2] IOMUX: Add console multiplexing support.
Wolfgang Denk
wd at denx.de
Thu Oct 30 12:59:49 CET 2008
Dear Gary Jennejohn,
In message <20081030112119.59036e03 at 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 at 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 at 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
--
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: wd at denx.de
In a survey taken several years ago, all incoming freshmen at MIT
were asked if they expected to graduate in the top half of their
class. Ninety-seven percent responded that they did.
More information about the U-Boot
mailing list