Peter, On Mon, Jan 24, 2011 at 10:51 AM, Peter Tyser <pty...@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 > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot