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

[Tom Warren]

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

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