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

[Gary Jennejohn]

On Thu, 30 Oct 2008 12:59:49 +0100
Wolfgang Denk <wd at denx.de> wrote:

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

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 at denx.de
*********************************************************************