[U-Boot] [PATCH V5 2/4] serial: Add Tegra2 serial port support

[Tom Warren <twarren.nvidia@gmail.com>]

Peter,

On Mon, Jan 24, 2011 at 10:51 AM, Peter Tyser <ptyser@xes-inc.com> wrote:
> <snip>
>
>> > It looks like arch/arm/cpu/armv7/tegra2/board.h and
>> > arch/arm/cpu/armv7/tegra2/uart.c are added in the first patch, then
>> > moved in this patch. ?It'd be ideal to just add them once in the proper
>> > location.
>> >
>> > On a side note, if you pass "git format-patch" the -M and -C options it
>> > will make pretty diffs that only show what lines changed during a move.
>> > In the case that you do move files in the future its nice to use those
>> > options to ease review.
>> >
>> I'll use those options in the future (thanks!) to keep things cleaner.
>> Hopefully we can just go with this set of patches so I can get to the
>> other, more interesting code (drivers, A9 CPU init, etc.).
>
> Its up to the ARM maintainer and Wolfgang to decide if adding code in
> one patch just to move it around in the 2nd is acceptable. ?I'm guess it
> won't be acceptable since its considered "bad form", and its unclear
> what patch in this series contains what functionality. ?Eg this isn't
> really meant to "Add Tegra2 serial port support", it moves existing
> serial port code around? ?And more? ?Its not really just adding serial
> port support as the patch title states in any case.
I see what you're talking about now - I've got uart.c in 2 patch files - created
in 0001 and then moved in 0002. My bad - that wasn't the intent, just what
happened when I applied my V4 patches to a new branch to get the V5 patchset
built.  I'll fix it and resubmit.

As to 0002 not adding serial port support for Tegra2, that's all it does - adds
TEGRA2 defines to serial.h/serial.c for the eserial* tables, and then adds
code to turn on Tegra2-specific UART HW.  If I remove any mention of uart.c
in patch 0001 (add basic Tegra2 support), then does patch 0002 make
sense to you?

>
>> > <snip>
>> >
>> > +void uart_init(void)
>> >> +{
>> >> + ? ? /* Init each UART - there may be more than 1 on a board/build */
>> >> +#if (CONFIG_TEGRA2_ENABLE_UARTA)
>> >> + ? ? init_uart();
>> >> +#endif
>> >> +#if (CONFIG_TEGRA2_ENABLE_UARTD)
>> >> + ? ? init_uart();
>> >> +#endif
>> >> +}
>> >
>> > How about:
>> > #if defined(CONFIG_TEGRA2_ENABLE_UARTA) || defined(CONFIG_TEGRA2_ENABLE_UARTD)
>> > ? ? ? ?init_uart();
>> > #endif
>> That won't work for a board like Harmony where the developer wants
>> both UARTs active, since uart_init is only called once.
>
> Why should init_uart() be called two times? ?It looks to initialize both
> ports, meaning it should only be called once, right?
Correct, again (need more coffee!)  Your if defined construct wouldn't work
as written, though, because CONFIG_TEGRA2_ENABLE_UARTx is always
defined (as 0 or 1). I'd have to rework it.

>
> Also, just noticed:
> +static void init_uart(void)
> +{
> +#if CONFIG_TEGRA2_ENABLE_UARTA
> + ? ? ? ? ? ? ? uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTA_BASE;
> +
> + ? ? ? ? ? ? ? uart_clock_init();
> +
> + ? ? ? ? ? ? ? /* Enable UARTA - uses config 0 */
> + ? ? ? ? ? ? ? pin_mux_uart();
> +
> + ? ? ? ? ? ? ? setup_uart(uart);
> +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */
> +#if CONFIG_TEGRA2_ENABLE_UARTD
> + ? ? ? ? ? ? ? uart_ctlr *const uart = (uart_ctlr *)NV_PA_APB_UARTD_BASE;
> +
>
> Have you compiled with both UARTA and UARTD enabled? ?Redeclaring 'uart'
> in the middle of the function should throw an error or warning.
I'd tested with both enabled earlier, but maybe not since the rewrite.
I'll check & resubmit.

>
> + ? ? ? ? ? ? ? uart_clock_init();
> +
> + ? ? ? ? ? ? ? /* Enable UARTD - uses config 0 */
> + ? ? ? ? ? ? ? pin_mux_uart();
> +
> + ? ? ? ? ? ? ? setup_uart(uart);
> +#endif /* CONFIG_TEGRA2_ENABLE_UARTD */
> +}
>
> Best,
> Peter
Thanks, again, for your thoroughness!
Tom
>
>