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

[Peter Tyser]

On Wed, 2011-01-26 at 10:13 +0200, Mike Rapoport wrote:
> On 01/26/11 00:24, Peter Tyser wrote:
> >>>>>> As I've already pointed out (1) this covers only one possibility of UART pin
> >>>>>> muxing options. I agree that it makes sense when this is a part of the
> >>>>>> board-specific code. However, forcing specific pins configuration in the generic
> >>>>>> driver seems problematic to me, even if most Tegra2 boards use the same
> >>>>>> configuration.
> >>>>>> As a side note, I'd prefer to have all the pin multiplexing defined in one place
> >>>>>> in board file rather than scattered among different drivers.
> >>>>>>
> >>>>> Alright. I'd like to get this wrapped up and accepted before the merge window
> >>>>> closes so I can get on with the important bits (drivers, etc.). What
> >>>>> would you like
> >>>>> here? A generic function like 'pinmux_set_config(reg, val, bit)' that
> >>>>> lives in the board
> >>>>> files and gets called from the driver code with specifics of that
> >>>>> board's/drivers pinmux
> >>>>> config? Or do you see the pinmux code living entirely in the board
> >>>>> files, and being done
> >>>>> as part of board init? That's where it was before, BTW.
> >>>>
> >>>> I think that the pinmux code should live entirely in the board file and
> >>>> performed as part of board init. The drivers than can assume that the pins are
> >>>> configured properly.
> >>> OK. I'll move the clock/pinmux init code into armv7/tegra/board.c
> >>> (since it's SoC-centric),
> >>> and call it during board_init().
> >> Actually, I think it makes more sense to move pinmux_init_uart and
> >> clock_init_uart to board/nvidia/common/board.c.
> >> These routines are more board-specific than SoC-specific - they depend
> >> on how the UART is routed on a board.
> >> Let me do that & gen up a new patchset.
> > 
> > You previously mentioned that "To date, all of our Tegra boards use
> > these pinmux options for both UARTs.  If a board vendor chooses to use
> > different pinmuxes, then they can override these funcs in their board
> > files, or use their own code triggered by their own defines. But
> > according to our HW guys, the vast majority will use these pins"
> > 
> > If the vast majority of boards really will use the same pinmuxing, it
> > would be nice to provide that common muxing as a default which can be
> > overridden.  Perhaps moving pinmux_init_uart() and uart_clock_init()
> > into armv7/tegra/*, and making them weak functions would make everyone
> > happy.
> 
> My point was that pin muxing belongs to the board code rather than to the
> driver. Driver should just assume that pins are configured elsewhere and it does
> not need to deal with pin muxing at all.

I understand your point, but think if 9/10 boards use the same pin
muxing its good to provide a default so we don't have 9 chunks of
duplicate code floating around in board/*.  What's the harm in providing
a default?  It can be overridden if needed.

> Moreover, I'd prefer to see pinmux_board_init or something similar that
> configures all the pins at once rather than collection of pinmux_init_uart,
> pinmux_init_sdmmc, pinmux_init_gmi etc that will grow as more drivers are added.

I can see that point but its a different discussion.  I don't know
enough about the Tegra2 to comment on this, but it seems like a good
idea based on previous experiences with boards with similar pinmuxing
(eg mpc8260).  In my last email I mentioned a table-driven approach
(similar to the mpc8260 implementation?) which sounds somewhat like
you're proposing.

Best,
Peter