[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 12:14 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
> <snip>
>
>> 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?
>
> Yeah, that'd make more sense. ?Patch 2 would just contain:
> ?common/serial.c ? ? ? ? ? ? ? ? ? ?| ? ?3 +-
> ?drivers/serial/Makefile ? ? ? ? ? ?| ? ?1 +
> ?drivers/serial/serial_tegra2.c ? ? | ?205 ++++++++++++++++++++++++++++++++++
> ?drivers/serial/serial_tegra2.h ? ? | ? 49 ++++++++
> ?include/serial.h ? ? ? ? ? ? ? ? ? | ? ?3 +-
>
Exactly what I was thinking. I'll try to get it right in patch V6.

>> >> > <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.
>
> You could also just get rid of uart_init() altogether and rename
> init_uart() to uart_init(). ?That would get rid of some idefs and
> simplify the flow.
Yeah, I saw that as I was cleaning up the indentation & reworking the
code to compile
with both UARTs defined. I'll get rid of  uart_init (renamed to
init_uart). Thanks.

>
> Best,
> Peter
Thanks,

Tom
>
>