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

[Peter Tyser]

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

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.

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