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

[Tom Warren]

On Fri, Jan 14, 2011 at 2:36 PM, Peter Tyser <ptyser at xes-inc.com> wrote:
> 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.
>
So instead of, say uart->lcr = 0, you'd prefer writel(0, uart->lcr),
where writel = __arch_putl(v, a) = (*(volatile unsigned int *)(a) =
(v))?

Is that different enough from 'uart->lcr = 0' to warrant the change?
Does it add some HW barriers or forced read-before-write that the
'volatile' struct doesn't?

I've done a ton of embedded work, but all in x86 asm (and C) on PCs,
so pardon my ignorance & questions.

> 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.
As conscious I get before noon. ;)
>
>> 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.
>
Good to know. Thanks.
> Best,
> Peter
>
>