On Thu, 30 Oct 2008 12:59:49 +0100
Wolfgang Denk <[EMAIL PROTECTED]> wrote:
> In message <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
*********************************************************************
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
