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

[Tom Warren]

Mike,

On Wed, Jan 26, 2011 at 1:13 AM, Mike Rapoport <mike at compulab.co.il> 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 that point - sorry if I wasn't clear. No objection to
having pinmux code in board files.

> 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 see a couple of reasons not to do it that way. First, I don't know
at this time what all the pinmux settings will be, since I haven't
ported all the periph driver code yet. It's vastly different from
what's acceptable in U-Boot, and will all need significant rewrite.
It'd take me a week to gather all that info, and I'm not at full BW on
this project (one of 4 on my plate right now).
Second, I've been chastised before for including code/features in this
initial patchset that aren't needed or used.  I'm trying to keep the
code as simple as possible to make it easier on reviewers and get
through the review in as short a time as possible. This has already
dragged on far longer than I thought it would.
I'm willing to change the pinmux code to make it as generic as
possible, but only if there's a consensus on the list that it has to
be that way to get accepted & pushed.
>> 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.
>
Thanks,
Tom