[U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support

[Peter Tyser]

On Fri, 2011-01-14 at 13:41 -0700, Tom Warren wrote:
> On Fri, Jan 14, 2011 at 12:59 PM, Peter Tyser <ptyser at xes-inc.com> wrote:
> > Hi Tom,
> >
> > On Fri, 2011-01-14 at 10:11 -0700, Tom Warren wrote:
> >> This series of patches adds preliminary/baseline support for NVIDIA's
> >> Tegra2 SoC.  Basic CPU (AVP), RAM and UART init are covered so that the
> >> system (Harmony or Seaboard) can boot to the U-Boot serial cmd prompt.
> >>
> >> Further support (for Cortex-A9 CPU(s), USB, SD/MMC, etc.) to follow.
> >>
> >> V2: Make changes based on feedback from Peter Tyser and Sandeep Paulraj.
> >
> > If you didn't use all the feedback to the original patches, you should
> > state explicitly what you changed here, or respond to the original
> > comment email as to why they weren't made.  For example, I see you
> > didn't make the suggested change to use IO access functions, or allow
> > compiling out of support for UARTA and UARTD.  That should be made clear
> > somewhere (and the logic of why the changes weren't made) so that those
> > reviewing the patches know what changed between v1 and v2 and why.  As
> > is its unclear why the v1 comments weren't implemented, so they also
> > apply to this series.
> >
> Peter,
> 
> Sorry, since this is my first patch series to U-Boot upstream, I'm
> still learning the proper etiquette.
> Let me respond here rather than in the patch comments since there are
> only 2 unchanged areas WRT comments.

No worries.  I'm glad to see you're pushing your changes upstream.

> 1) IO access functions - I pre-reviewed my patch series with Wolfgang
> (to hopefully catch any blatant errors and smooth
> the process) and he indicated that C structs and I/O accessor funcs or
> macros were preferred to my base+offset original code.
> Since the ARM is 32-bit, and all of our registers are I/O mapped, it
> made sense just to cast the necessary HW mem-mapped
> regs as volatile structs and access the members directly. Works well,
> is easy to read & understand, etc. Let me know (with
> examples, if possible) how I can make it better.

Both Linux and U-Boot recommend using IO access functions instead
pointer accesses, at least in PPC-land, and even for memory mapped
registers.  I'm not too familiar with ARM, but assume they have the same
recommendation.  If pointers are used, some CPUs may optimize the access
order, thus causing problems.  Using IO accessors also ensures that the
code is portable.  Even if the Tegra doesn't re-order accesses, a driver
you write for use in a Tegra could be used on other CPUs that do.

eg:
http://www.mail-archive.com/u-boot@lists.denx.de/msg18435.html
http://lists.denx.de/pipermail/u-boot/2007-December/027595.html

It looks like other ARM processors use IO accessors too, eg the recent
armada CPU:
arch/arm/cpu/arm926ejs/armada100/*
A grep of writel in arch/arm shows a number of references.  Similarly in
the Linux code in arch/arm.

ARM maintainers, feel free to chime in if you have comments.

> 2) Compiling out support for UARTA or UARTD - didn't seem necessary -
> size isn't an issue at this point with Tegra2 U-Boot,
> and some boards (Harmony, for example) are populated w/hardware for
> both UARTA and UARTD, and can have both on at
> U-Boot runtime (perhaps for debug out to UARTA whilst normal console
> I/O goes to UARTD), so I chose to leave the init code
> for both intact. Plus I've never liked code with too many unnecessary
> ifdef's - makes it less readable, IMO.

Yeah, this is grey - I see both sides of the coin.  U-Boot generally
strives to be as small as possible.  You may use a large flash on your
eval boards and not care about space, but other system designers may
place as small of flash as possible to save cost.  It looked like
UART_A-UART_E were also going to be supported at some time, which is
quite a chunk of code.  You also already are defining which serial ports
are available at compile time via CONFIG_TEGRA2_ENABLE_UARTA and
CONFIG_TEGRA2_ENABLE_UARTD.

Anyway, I don't really care much which way you go, just wanted to make
sure you were making a conscious decision to not add the ifdefs.

> I was going to respond to your review w/a direct, inline reply, but I
> thought it better to get the V2 patch out there before the
> weekend (we're off for MLK, as well). I'm under some pressure to get a
> baseline Tegra2 patchset in before the merge window
> closes.  I'll be sure to respond to each issue directly on the list in
> the future, though.

As long as you initially submit your patches during the merge window
they will generally be merged, even if there are multiple revisions and
discussion that lasts past the end of official merge window.  So this
stuff should get in.

Best,
Peter