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

[Mike Rapoport]

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

> Or could you make default defines that board's could override as needed?
> eg in pin_mux_uart():
> #ifndef CONFIG_TEGRA2_PMT_CTLC_MASK
> #define CONFIG_TEGRA2_PMC_CTLC_MASK 0xfff0ffff
> #endif
> #ifndef CONFIG_TEGRA2_PMT_TRI_A_MASK
> #define CONFIG_TEGRA2_PMC_TRI_A_MASK ~(Z_IRRX | Z_IRTX)
> #endif
> pin_mux_uart() {
> 	reg = readl(&pmt->pmt_ctl_c);
> 	reg &= CONFIG_TEGRA2_PMT_CTLC_MASK;
> 	writel(reg, &pmt->pmt_ctl_c);
> 
> 	reg = readl(&pmt->pmt_tri_a);
> 	reg &= CONFIG_TEGRA2_PMC_TRI_A_MASK;
> 	writel(reg, &pmt->pmt_tri_a);
> }
> 
> Or make the functions table driven so each board would define a
> pinmux/clock configuration table?
> 
> I think its worthwhile to try and reduce duplicated code whenever
> possible.
> 
> Best,
> Peter
> 

-- 
Sincerely yours,
Mike.