[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debug console

[Peter Tyser]

Thanks for the comments.  Response inline:

On Sat, 2008-08-16 at 09:49 +0200, Wolfgang Denk wrote:
> Dear Peter,
> 
> In message <1218839108.1273.144.camel at localhost.localdomain> you wrote:
> > 
> > I attempted to account for this fact.  I followed the program flow of
> > run_command() and readline() in particular looking for global data
> > writes and bss reads.  For the instances where global data was being
> 
> Indeed you did this, but only for a minor part of the U-Boot code - at
> the cost of added compexity, ugly $ifdeffery and inconsistent and
> undefined behaviour as soon as you run any "uninspected" command.
> 
> > written, I made it conditional on (gd->flags & GD_FLG_RELOC).  I also
> > forced some variables to reside in the data section so that they could
> > be read while executing from flash (__attribute__ ((section(".data")))).
> 
> This just helps to make the initial values readable. The variables
> still cannot be written. Even worse, on some flash types (namely Intel
> ones) such write operations to flash memory can corrupt your U-Boot
> image.

If the debug console is being used, it would generally mean that the
board is not bootable.  Thus the absolute worst (and somewhat unlikely)
outcome of using the debug console would be to render the already
unbootable board further unbootable.

I'd also put the burden of using the debug console responsibly on the
end user (similar to most of U-Boot's normal commands).  Perhaps some
added documentation would alleviate misuse of the debug console?

> > I also monitored the flash WE pin while executing the memory and i2c
> > commands to make sure that the data section (which is still in flash)
> > was not being written to.  There is a chance that uninitialized data is
> 
> But that's just these two commands. What happens when I run any other
> command? And you don't prevent me from doing that.

The same argument applies - if the board already isn't booting, running
a "bad" command from the debug console can't do any real damage.

It would also be relatively easy to make the "repeatable" argument in
the cmd_tbl_s structure be a general purpose flag argument, in which
case commands which have been Ok-ed for use by the debug console could
be marked as such in the command definition.

> > > And your patch seems to be inclomplete, it does not apply.
> > 
> > Hmm, I built the patches off 07efc9e321619c3dec213310c32e011aa6f02783
> > with the 2 patches I just submitted also applied ("mod_i2c_mem()
> > bugfix" and "Replace references to extern console_buffer with a function
> > call").  What error are you seeing while applying it?
> > 
> > 
> > > Some parts seem to be missing (like the necessary changes to eliminate
> > > accesses to the console_buffer[] in BSS).
> > 
> > I don't follow.  The console_buffer_addr() conditionally places the
> > console_buffer in SDRAM if U-Boot has relocated, or at
> > CFG_DEBUG_CONSOLE_ADDR if executing from flash.  That's why the "Replace
> > references to extern console_buffer with a function call" patch I just
> > submitted was necessary.
> 
> Ah, so that patch is a prerequisite? You shoul have  mentioned  that,
> then, or poosted the three of them as a series.

This assumption was mentioned in the patch summary, see the first email
in the series "[PATCH] [RFC] Early serial debug console".

> > I agree that it is hokey that some commands work, some don't.  However,
> > the ones that are very useful do work:)  The memory test, modify, and
> > display functions as well as the i2c functions work.  These few commands
> > allow further diagnosis (and possible fixing) of most board hardware
> > problems that prevent a board from booting.
> 
> Well, someone who is working in these stages of bard bringup  usually
> uses a debugger.

>From my experience, there are a few shortcomings to using a debugger to
bring up a board and diagnose hardware issues:
1. Its time consuming/complicated to accurately diagnose hardware issues
2. Its much harder to bring out "tricky" issues like floating DDR
address lines, improper DDR termination, improper CPU DDR timing
parameters, etc
3. Not everyone has a debugger readily available
4. Some boards might not have JTAG/debugger signals brought out to a
header at all, which makes using a debugger impossible

> > Thanks for the feedback, I'm curious what others have to say as well.
> > Let me know if I can provide any additional details.
> 
> The longer I think about it the more I tend to reject  it.  It  makes
> the  code worse for the benefit of just a very special situation that
> probably can be handled as well (or even better)  using  a  debugger.

I personally feel that a debugger is not always the best tool for
initial board bringup/debugging.  After manufacturing, we program the
majority of our boards over the PCI bus (much faster than JTAG), then
run them through a manufacturing test from U-Boot/Linux.  Assuming some
hardware tests fail, it is no trivial matter to modify JTAG tests to try
and narrow down the problem.  Having access to a basic debug console
greatly speeds up this debug process.

I don't disagree that the implementation isn't the cleanest, but I
really do think an early debug console is very useful for board bringup,
debug, tuning, etc.  In my opinion, the few extra bytes of memory the
feature adds and the 20 lines of convoluted code are outweighed by the
usefulness of the feature.

Is there any way the debug console could be implemented in a cleaner
manner that would be more readily accepted into U-Boot mainline?

Best,
Peter