From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter Tyser <ptyser@xes-inc.com>
Date: Mon, 24 Jan 2011 11:51:46 -0600
Subject: [U-Boot] [PATCH V5 2/4] serial: Add Tegra2 serial port support
In-Reply-To: <AANLkTikPJvW98r2ynuqtaeR4WvHAbj+u6kNy3nXtOWsp@mail.gmail.com>
References: <1295651217-32421-1-git-send-email-twarren@nvidia.com>
	<1295651217-32421-3-git-send-email-twarren@nvidia.com>
	<1295653584.29414.519.camel@petert>
	<AANLkTikPJvW98r2ynuqtaeR4WvHAbj+u6kNy3nXtOWsp@mail.gmail.com>
Message-ID: <1295891506.2045.146.camel@petert>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

<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.

> > <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?

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.

+               uart_clock_init();
+
+               /* Enable UARTD - uses config 0 */
+               pin_mux_uart();
+
+               setup_uart(uart);
+#endif /* CONFIG_TEGRA2_ENABLE_UARTD */
+}

Best,
Peter