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

[Tom Warren]

Mike et al,

On Tue, Jan 25, 2011 at 2:37 PM, Tom Warren <twarren.nvidia at gmail.com> wrote:
> Mike,
>
> On Tue, Jan 25, 2011 at 2:12 PM, Mike Rapoport <mike at compulab.co.il> wrote:
>> On 01/25/11 18:50, Tom Warren wrote:
>>> Mike,
>>>
>>> On Tue, Jan 25, 2011 at 1:11 AM, Mike Rapoport <mike at compulab.co.il> wrote:
>>>> On 01/22/11 01:06, Tom Warren wrote:
>>>>> Signed-off-by: Tom Warren <twarren at nvidia.com>
>>>>> ---
>>>>
>>>> [ snip ]
>>>>
>>>>> +/*
>>>>> + * Routine: pin_mux_uart
>>>>> + * Description: setup the pin muxes/tristate values for a UART
>>>>> + */
>>>>> +static void pin_mux_uart(void)
>>>>> +{
>>>>> +     pinmux_tri_ctlr *const pmt = (pinmux_tri_ctlr *)NV_PA_APB_MISC_BASE;
>>>>> +     u32 reg;
>>>>> +
>>>>> +#if CONFIG_TEGRA2_ENABLE_UARTA
>>>>> +             reg = readl(&pmt->pmt_ctl_c);
>>>>> +             reg &= 0xFFF0FFFF;      /* IRRX_/IRTX_SEL [19:16] = 00 UARTA */
>>>>> +             writel(reg, &pmt->pmt_ctl_c);
>>>>> +
>>>>> +             reg = readl(&pmt->pmt_tri_a);
>>>>> +             reg &= ~Z_IRRX;         /* Z_IRRX = normal (0) */
>>>>> +             reg &= ~Z_IRTX;         /* Z_IRTX = normal (0) */
>>>>> +             writel(reg, &pmt->pmt_tri_a);
>>>>> +#endif       /* CONFIG_TEGRA2_ENABLE_UARTA */
>>>>> +#if CONFIG_TEGRA2_ENABLE_UARTD
>>>>> +             reg = readl(&pmt->pmt_ctl_b);
>>>>> +             reg &= 0xFFFFFFF3;      /* GMC_SEL [3:2] = 00, UARTD */
>>>>> +             writel(reg, &pmt->pmt_ctl_b);
>>>>> +
>>>>> +             reg = readl(&pmt->pmt_tri_a);
>>>>> +             reg &= ~Z_GMC;          /* Z_GMC = normal (0) */
>>>>> +             writel(reg, &pmt->pmt_tri_a);
>>>>> +#endif       /* CONFIG_TEGRA2_ENABLE_UARTD */
>>>>
>>>> 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.

>
>>
>>> If there are examples that you approve of in any extant U-Boot code (omap,
>>> ti91, davinci, etc.), please point them out.  I'd really like to
>>> finalize this patch series.
>>
>> I think OMAP is a good example. There's set_muxconf_regs function in each board
>> file that is responsible for configuration of all the pins.
> I'll take a look. Thanks.
>>
>>>> (1) http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/92887/focus=93233
>>>>
>>>>
>>>> --
>>>> Sincerely yours,
>>>> Mike.
>>> Thanks,
>>>
>>> Tom
>>>>
>>
>>
>> --
>> Sincerely yours,
>> Mike.
>>
>