Patchwork [U-Boot,8/9] Tegra30: Add common pinmux config in board_early_init_f

login
register
mail settings
Submitter Tom Warren
Date Sept. 12, 2012, 10:10 p.m.
Message ID <1347487855-27077-9-git-send-email-twarren@nvidia.com>
Download mbox | patch
Permalink /patch/183463/
State Changes Requested
Delegated to: Tom Rini
Headers show

Comments

Tom Warren - Sept. 12, 2012, 10:10 p.m.
Signed-off-by: Tom Warren <twarren@nvidia.com>
---
 board/nvidia/common/board.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)
Stephen Warren - Sept. 13, 2012, 10:37 p.m.
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.
Simon Glass - Sept. 18, 2012, 7:53 p.m.
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
Tom Warren - Sept. 18, 2012, 9:32 p.m.
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
Stephen Warren - Sept. 18, 2012, 9:53 p.m.
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).

Patch

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 */