| Message ID | 1347487855-27077-9-git-send-email-twarren@nvidia.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On 09/12/2012 04:10 PM, Tom Warren wrote: > Signed-off-by: Tom Warren <twarren@nvidia.com> > --- > board/nvidia/common/board.c | 27 ++++++++++++++++++++++++++- > 1 files changed, 26 insertions(+), 1 deletions(-) Common code:-) :-) But ... > diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c > +#ifdef CONFIG_TEGRA30 > +#include "../cardhu/pinmux-config-common.h" > +#endif Not all Tegra30 will be Cardhu... Given this is really board-specific, shouldn't the following be an empty weak definition: > +/* > + * Routine: pinmux_init > + * Description: Do individual peripheral pinmux configs > + */ > +static void pinmux_init(void) > +{ > +#if defined(CONFIG_TEGRA30) > + pinmux_config_table(tegra3_pinmux_common, > + ARRAY_SIZE(tegra3_pinmux_common)); > + > + pinmux_config_table(unused_pins_lowpower, > + ARRAY_SIZE(unused_pins_lowpower)); > +#endif > +} ... and the function be overridden in board files as needed? If we are moving to a model of a single function that sets up the entire pin mux at boot (which seems fine to me, and could eventually be driven by DT if it happened late enough), then it seems like we wouldn't need e.g. pin_mux_mmc() or pin_mux_usb() any more.
Hi, On Thu, Sep 13, 2012 at 3:37 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 09/12/2012 04:10 PM, Tom Warren wrote: >> Signed-off-by: Tom Warren <twarren@nvidia.com> >> --- >> board/nvidia/common/board.c | 27 ++++++++++++++++++++++++++- >> 1 files changed, 26 insertions(+), 1 deletions(-) > > Common code:-) :-) But ... > >> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c > >> +#ifdef CONFIG_TEGRA30 >> +#include "../cardhu/pinmux-config-common.h" >> +#endif > > Not all Tegra30 will be Cardhu... > > Given this is really board-specific, shouldn't the following be an empty > weak definition: > >> +/* >> + * Routine: pinmux_init >> + * Description: Do individual peripheral pinmux configs >> + */ >> +static void pinmux_init(void) >> +{ >> +#if defined(CONFIG_TEGRA30) >> + pinmux_config_table(tegra3_pinmux_common, >> + ARRAY_SIZE(tegra3_pinmux_common)); >> + >> + pinmux_config_table(unused_pins_lowpower, >> + ARRAY_SIZE(unused_pins_lowpower)); >> +#endif >> +} > > ... and the function be overridden in board files as needed? > > If we are moving to a model of a single function that sets up the entire > pin mux at boot (which seems fine to me, and could eventually be driven > by DT if it happened late enough), then it seems like we wouldn't need > e.g. pin_mux_mmc() or pin_mux_usb() any more. While the fdt may eventually remove this discussion, I don't think forcing a one-time pinmux init is the best idea. Some peripherals will not be needed on every boot (e.g. normally boot from eMMC unless USB is available). Some peripherals may want to change their config based on run-time settings (although this is unlikely I suppose, particularly if we have the fdt). Regards, Simon > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Simon, On Tue, Sep 18, 2012 at 12:53 PM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On Thu, Sep 13, 2012 at 3:37 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 09/12/2012 04:10 PM, Tom Warren wrote: >>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>> --- >>> board/nvidia/common/board.c | 27 ++++++++++++++++++++++++++- >>> 1 files changed, 26 insertions(+), 1 deletions(-) >> >> Common code:-) :-) But ... >> >>> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c >> >>> +#ifdef CONFIG_TEGRA30 >>> +#include "../cardhu/pinmux-config-common.h" >>> +#endif >> >> Not all Tegra30 will be Cardhu... >> >> Given this is really board-specific, shouldn't the following be an empty >> weak definition: >> >>> +/* >>> + * Routine: pinmux_init >>> + * Description: Do individual peripheral pinmux configs >>> + */ >>> +static void pinmux_init(void) >>> +{ >>> +#if defined(CONFIG_TEGRA30) >>> + pinmux_config_table(tegra3_pinmux_common, >>> + ARRAY_SIZE(tegra3_pinmux_common)); >>> + >>> + pinmux_config_table(unused_pins_lowpower, >>> + ARRAY_SIZE(unused_pins_lowpower)); >>> +#endif >>> +} >> >> ... and the function be overridden in board files as needed? >> >> If we are moving to a model of a single function that sets up the entire >> pin mux at boot (which seems fine to me, and could eventually be driven >> by DT if it happened late enough), then it seems like we wouldn't need >> e.g. pin_mux_mmc() or pin_mux_usb() any more. > > While the fdt may eventually remove this discussion, I don't think > forcing a one-time pinmux init is the best idea. Some peripherals will > not be needed on every boot (e.g. normally boot from eMMC unless USB > is available). Some peripherals may want to change their config based > on run-time settings (although this is unlikely I suppose, > particularly if we have the fdt). I've been basically adapting our internal T30 U-Boot code to fit within the framework of what's upstream, including Allen's SPL reorg. Pinmux on our T30 codebase (including the Chromium U-Boot code, I believe) was done in a single monolithic file, as demonstrated in this patch. The advantages are that you can construct the table to ensure there are no conflicts, no small task with 4 options per mux and over 100 muxes. Even the HW power-on defaults have conflicts. The only exception in this patchset was the UART muxing, so we can start putting out debug/status spew to the console as early as possible. As far as I'm aware, an FDT pinmux for Tegra (which is on my plate, but quite a bit behind T30) would be essentially the same deal - one large list of mux settings per build/board. Also, we've had bugs submitted by the kernel guys at times because U-Boot didn't do _enough_ init. While I agree with the U-Boot philosophy of doing just what's necessary to get a kernel loaded, I don't consider pinmux init from a table as a time-consuming or code-intensive operation, and see no harm in leaving it in. The driver can always remap the muxes as it chooses at a later date, based on what it discovers or knows from its config or the DT. Thanks, Tom > > Regards, > Simon > >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot
On 09/18/2012 03:32 PM, Tom Warren wrote: ... > As far as I'm aware, an FDT pinmux for Tegra (which is on my plate, > but quite a bit behind T30) would be essentially the same deal - one > large list of mux settings per build/board. Initializing pinmux from DT can work either way, depending on how the DT author wrote the DT file. The pinmux DT node can contain a pinmux configuration which is applied as soon as the pinmux driver loads. This configuration can contain as little as you want (even nothing) all the way through to containing the entire board's static pinmux configuration. For portions of the pinmux settings which the pinmux driver's own DT node doesn't configure (if any, based on the above), the relevant individual driver DT node can configure the pinmux as required, and that configuration would be applied when the relevant driver loads and parses its DT node. In practice, so far, all the kernel board files for Tegra almost exclusively use static muxing in the pinmux controller's own DT node. However, there are a couple small dynamic cases (e.g. Seaboard/Springbank's pinctrl-based I2C mux for example).
diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index afe832a..4a86c30 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -1,5 +1,5 @@ /* - * (C) Copyright 2010,2011 + * (C) Copyright 2010-2012 * NVIDIA Corporation <www.nvidia.com> * * See file CREDITS for list of people who contributed to this @@ -25,7 +25,11 @@ #include <ns16550.h> #include <linux/compiler.h> #include <asm/io.h> +#if defined(CONFIG_TEGRA20) #include <asm/arch/tegra20.h> +#else /* Tegra30 */ +#include <asm/arch/tegra30.h> +#endif #include <asm/arch/sys_proto.h> #include <asm/arch/board.h> @@ -87,6 +91,25 @@ static void power_det_init(void) #endif } +#ifdef CONFIG_TEGRA30 +#include "../cardhu/pinmux-config-common.h" +#endif + +/* + * Routine: pinmux_init + * Description: Do individual peripheral pinmux configs + */ +static void pinmux_init(void) +{ +#if defined(CONFIG_TEGRA30) + pinmux_config_table(tegra3_pinmux_common, + ARRAY_SIZE(tegra3_pinmux_common)); + + pinmux_config_table(unused_pins_lowpower, + ARRAY_SIZE(unused_pins_lowpower)); +#endif +} + /* * Routine: board_init * Description: Early hardware init. @@ -152,6 +175,8 @@ void gpio_early_init(void) __attribute__((weak, alias("__gpio_early_init"))); int board_early_init_f(void) { + pinmux_init(); + board_init_uart_f(); /* Initialize periph GPIOs */
Signed-off-by: Tom Warren <twarren@nvidia.com> --- board/nvidia/common/board.c | 27 ++++++++++++++++++++++++++- 1 files changed, 26 insertions(+), 1 deletions(-)