[U-Boot] [PATCH 1/2] IOMUX: Add console multiplexing support.
Wolfgang Denk
wd at denx.de
Sun Sep 14 20:29:57 CEST 2008
Dear Gary Jennejohn,
In message <20080914164107.32a9fce1 at 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 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_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: wd at denx.de
The price of curiosity is a terminal experience.
- Terry Pratchett, _The Dark Side of the Sun_
More information about the U-Boot
mailing list