Patchwork [U-Boot,1/2] tegra: clean up board include hell

login
register
mail settings
Submitter Lucas Stach
Date Sept. 27, 2012, 9:52 p.m.
Message ID <1348782724-19151-1-git-send-email-dev@lynxeye.de>
Download mbox | patch
Permalink /patch/187485/
State Accepted
Delegated to: Tom Warren
Headers show

Comments

Lucas Stach - Sept. 27, 2012, 9:52 p.m.
The prototypes used in board files were all scattered out, which lead to
code duplication between SPL and normal U-Boot and some prototypes not actually
being used. Consolidate this in a common board header.

This will allow to push down the calling of the pinmux functions into the
respective drivers and this way cut down on complexity from the common board
code.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/arm720t/tegra-common/board.h | 25 ---------------------
 arch/arm/cpu/arm720t/tegra-common/spl.c   |  2 +-
 arch/arm/cpu/tegra-common/board.c         |  1 +
 arch/arm/include/asm/arch-tegra/board.h   | 19 +++++++++++++++-
 board/nvidia/common/board.c               |  1 -
 board/nvidia/common/board.h               | 37 -------------------------------
 board/nvidia/common/uart-spi-switch.c     |  2 +-
 7 Dateien geändert, 21 Zeilen hinzugefügt(+), 66 Zeilen entfernt(-)
 delete mode 100644 arch/arm/cpu/arm720t/tegra-common/board.h
 delete mode 100644 board/nvidia/common/board.h
Stephen Warren - Sept. 27, 2012, 10:32 p.m.
On 09/27/2012 03:52 PM, Lucas Stach wrote:
> The prototypes used in board files were all scattered out, which lead to
> code duplication between SPL and normal U-Boot and some prototypes not actually
> being used. Consolidate this in a common board header.

> This will allow to push down the calling of the pinmux functions into the
> respective drivers and this way cut down on complexity from the common board
> code.

I don't think that (calling pinmux from drivers) would be a good idea.
The entire pinmux should be set up globally when the system boots in
order to avoid conflicts part-way through a change, and to avoid
duplicating pinmux calls into every single driver. Unless a particular
driver actively needs to switch between different pinmux configurations
at run-time (e.g. an I2C bus mux that uses pinmux to do the muxing).

That said, this change to simplify all the #includes is probably
reasonable. One comment below.

> diff --git a/arch/arm/include/asm/arch-tegra/board.h b/arch/arm/include/asm/arch-tegra/board.h

> -/* Setup UARTs for the board according to the selected config */
> +/* Set up pinmux to make UART usable */
> +void gpio_config_uart(void);      /* CONFIG_SPI_UART_SWITCH */
> +void gpio_early_init_uart(void);  /*!CONFIG_SPI_UART_SWITCH */
> +
> +/* Set up early UART output */
>  void board_init_uart_f(void);
>  
> +/* Set up any early GPIOs the board might need for proper operation */
> +void gpio_early_init(void);  /* overrideable GPIO config        */

I think we should just rip out all the CONFIG_SPI_UART_SWITCH stuff.
It's only there to support one board with a completely broken HW design.
Still, that could happen in a separate patch after this though.
Lucas Stach - Sept. 27, 2012, 10:42 p.m.
Am Donnerstag, den 27.09.2012, 16:32 -0600 schrieb Stephen Warren:
> On 09/27/2012 03:52 PM, Lucas Stach wrote:
> > The prototypes used in board files were all scattered out, which lead to
> > code duplication between SPL and normal U-Boot and some prototypes not actually
> > being used. Consolidate this in a common board header.
> 
> > This will allow to push down the calling of the pinmux functions into the
> > respective drivers and this way cut down on complexity from the common board
> > code.
> 
> I don't think that (calling pinmux from drivers) would be a good idea.
> The entire pinmux should be set up globally when the system boots in
> order to avoid conflicts part-way through a change, and to avoid
> duplicating pinmux calls into every single driver. Unless a particular
> driver actively needs to switch between different pinmux configurations
> at run-time (e.g. an I2C bus mux that uses pinmux to do the muxing).
> 
Ok, this means Patch 2/2 is the wrong approach. I will wait if there are
other comments before respinning.

> That said, this change to simplify all the #includes is probably
> reasonable. One comment below.
> 
> > diff --git a/arch/arm/include/asm/arch-tegra/board.h b/arch/arm/include/asm/arch-tegra/board.h
> 
> > -/* Setup UARTs for the board according to the selected config */
> > +/* Set up pinmux to make UART usable */
> > +void gpio_config_uart(void);      /* CONFIG_SPI_UART_SWITCH */
> > +void gpio_early_init_uart(void);  /*!CONFIG_SPI_UART_SWITCH */
> > +
> > +/* Set up early UART output */
> >  void board_init_uart_f(void);
> >  
> > +/* Set up any early GPIOs the board might need for proper operation */
> > +void gpio_early_init(void);  /* overrideable GPIO config        */
> 
> I think we should just rip out all the CONFIG_SPI_UART_SWITCH stuff.
> It's only there to support one board with a completely broken HW design.
> Still, that could happen in a separate patch after this though.
> 
Yeah, if everyone is fine with this, i'll do this. Tom, Simon?

Now that Colibri support is mostly finished I plan on doing some janitor
tasks like moving the USB driver to the right location in the tree and
moving over MMC to device tree (wrong sport to call pinmux will go away
then). Might as well kill UART_SWITCH along the way.

Lucas
Simon Glass - Sept. 27, 2012, 10:59 p.m.
Hi,

On Thu, Sep 27, 2012 at 3:32 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/27/2012 03:52 PM, Lucas Stach wrote:
>> The prototypes used in board files were all scattered out, which lead to
>> code duplication between SPL and normal U-Boot and some prototypes not actually
>> being used. Consolidate this in a common board header.
>
>> This will allow to push down the calling of the pinmux functions into the
>> respective drivers and this way cut down on complexity from the common board
>> code.
>
> I don't think that (calling pinmux from drivers) would be a good idea.
> The entire pinmux should be set up globally when the system boots in
> order to avoid conflicts part-way through a change, and to avoid
> duplicating pinmux calls into every single driver. Unless a particular
> driver actively needs to switch between different pinmux configurations
> at run-time (e.g. an I2C bus mux that uses pinmux to do the muxing).

Well I'm not so keen on this approach. Ultimately we want to be able
to start a driver (and set up its pinmux) only if it is needed during
boot. The idea behind funcmux is that it is a single line call from a
driver or board to select the required setting, so the overhead is
small - and we know that for most peripherals there are only a small
number of options.

I can't predict where we might end up with fdt-based pinmux, but it
seems to me that global pinmux should be a board/implementer option,
not something required by the use of Tegra.

Just my 2c worth.

>
> That said, this change to simplify all the #includes is probably
> reasonable. One comment below.
>
>> diff --git a/arch/arm/include/asm/arch-tegra/board.h b/arch/arm/include/asm/arch-tegra/board.h
>
>> -/* Setup UARTs for the board according to the selected config */
>> +/* Set up pinmux to make UART usable */
>> +void gpio_config_uart(void);      /* CONFIG_SPI_UART_SWITCH */
>> +void gpio_early_init_uart(void);  /*!CONFIG_SPI_UART_SWITCH */
>> +
>> +/* Set up early UART output */
>>  void board_init_uart_f(void);
>>
>> +/* Set up any early GPIOs the board might need for proper operation */
>> +void gpio_early_init(void);  /* overrideable GPIO config        */
>
> I think we should just rip out all the CONFIG_SPI_UART_SWITCH stuff.
> It's only there to support one board with a completely broken HW design.
> Still, that could happen in a separate patch after this though.

Yes I agree the seaboard is broken, but this is an upstream board and
is in fact the only one I have to test with. Could we perhaps delay
ripping this out for a little while longer?

Regards,
Simon

> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Stephen Warren - Sept. 27, 2012, 11:25 p.m.
On 09/27/2012 04:59 PM, Simon Glass wrote:
> Hi,
> 
> On Thu, Sep 27, 2012 at 3:32 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/27/2012 03:52 PM, Lucas Stach wrote:
>>> The prototypes used in board files were all scattered out, which lead to
>>> code duplication between SPL and normal U-Boot and some prototypes not actually
>>> being used. Consolidate this in a common board header.
>>
>>> This will allow to push down the calling of the pinmux functions into the
>>> respective drivers and this way cut down on complexity from the common board
>>> code.
>>
>> I don't think that (calling pinmux from drivers) would be a good idea.
>> The entire pinmux should be set up globally when the system boots in
>> order to avoid conflicts part-way through a change, and to avoid
>> duplicating pinmux calls into every single driver. Unless a particular
>> driver actively needs to switch between different pinmux configurations
>> at run-time (e.g. an I2C bus mux that uses pinmux to do the muxing).
> 
> Well I'm not so keen on this approach. Ultimately we want to be able
> to start a driver (and set up its pinmux) only if it is needed during
> boot.

I disagree.

> The idea behind funcmux is that it is a single line call from a
> driver or board to select the required setting, so the overhead is
> small - and we know that for most peripherals there are only a small
> number of options.

Tegra30's more flexibile pinmux (if not the more convoluted options on
Tegra20) will eventually convince you that funcmux is not scalable, once
we support more than a few of the possible options.

...
>> I think we should just rip out all the CONFIG_SPI_UART_SWITCH stuff.
>> It's only there to support one board with a completely broken HW design.
>> Still, that could happen in a separate patch after this though.
> 
> Yes I agree the seaboard is broken, but this is an upstream board and
> is in fact the only one I have to test with.

There's zero use for the functionality though. Because the HW design is
broken, we've decided not to support the SPI flash on
Seaboard/Springbank. The SPI driver itself works fine on TrimSlice
without any issues, so there's no testing hole there.

> Could we perhaps delay
> ripping this out for a little while longer?

What is the event that would trigger the end of this delay?
Simon Glass - Sept. 27, 2012, 11:40 p.m.
Hi Stephen,

On Thu, Sep 27, 2012 at 4:25 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/27/2012 04:59 PM, Simon Glass wrote:
>> Hi,
>>
>> On Thu, Sep 27, 2012 at 3:32 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 09/27/2012 03:52 PM, Lucas Stach wrote:
>>>> The prototypes used in board files were all scattered out, which lead to
>>>> code duplication between SPL and normal U-Boot and some prototypes not actually
>>>> being used. Consolidate this in a common board header.
>>>
>>>> This will allow to push down the calling of the pinmux functions into the
>>>> respective drivers and this way cut down on complexity from the common board
>>>> code.
>>>
>>> I don't think that (calling pinmux from drivers) would be a good idea.
>>> The entire pinmux should be set up globally when the system boots in
>>> order to avoid conflicts part-way through a change, and to avoid
>>> duplicating pinmux calls into every single driver. Unless a particular
>>> driver actively needs to switch between different pinmux configurations
>>> at run-time (e.g. an I2C bus mux that uses pinmux to do the muxing).
>>
>> Well I'm not so keen on this approach. Ultimately we want to be able
>> to start a driver (and set up its pinmux) only if it is needed during
>> boot.
>
> I disagree.
>
>> The idea behind funcmux is that it is a single line call from a
>> driver or board to select the required setting, so the overhead is
>> small - and we know that for most peripherals there are only a small
>> number of options.
>
> Tegra30's more flexibile pinmux (if not the more convoluted options on
> Tegra20) will eventually convince you that funcmux is not scalable, once
> we support more than a few of the possible options.

Well I am already convinced of that - it is just a placeholder until
we have fdt support after all. Its entire purpose is to avoid lots of
individual pinmux calls in all the board files and drivers, which we
are in the pre-fdt state. Without funcmux we would have had long lists
of duplicated code in each board file.

>
> ...
>>> I think we should just rip out all the CONFIG_SPI_UART_SWITCH stuff.
>>> It's only there to support one board with a completely broken HW design.
>>> Still, that could happen in a separate patch after this though.
>>
>> Yes I agree the seaboard is broken, but this is an upstream board and
>> is in fact the only one I have to test with.
>
> There's zero use for the functionality though. Because the HW design is
> broken, we've decided not to support the SPI flash on
> Seaboard/Springbank. The SPI driver itself works fine on TrimSlice
> without any issues, so there's no testing hole there.

I thought Tom submitted some patches to make it work? It seems fine
for me, but I admit I don't use it extensively.

>
>> Could we perhaps delay
>> ripping this out for a little while longer?
>
> What is the event that would trigger the end of this delay?

Merging of the LCD series, for me.

Regards,
Simon

Patch

diff --git a/arch/arm/cpu/arm720t/tegra-common/board.h b/arch/arm/cpu/arm720t/tegra-common/board.h
deleted file mode 100644
index 260767d..0000000
--- a/arch/arm/cpu/arm720t/tegra-common/board.h
+++ /dev/null
@@ -1,25 +0,0 @@ 
-/*
- * (C) Copyright 2010-2011
- * NVIDIA Corporation <www.nvidia.com>
- *
- * See file CREDITS for list of people who contributed to this
- * project.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of
- * the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- * MA 02111-1307 USA
- */
-
-void board_init_uart_f(void);
-void gpio_early_init_uart(void);
diff --git a/arch/arm/cpu/arm720t/tegra-common/spl.c b/arch/arm/cpu/arm720t/tegra-common/spl.c
index dfe36b9..0d37ce8 100644
--- a/arch/arm/cpu/arm720t/tegra-common/spl.c
+++ b/arch/arm/cpu/arm720t/tegra-common/spl.c
@@ -33,13 +33,13 @@ 
 #include <image.h>
 #include <malloc.h>
 #include <linux/compiler.h>
-#include "board.h"
 #include "cpu.h"
 
 #include <asm/io.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/pinmux.h>
 #include <asm/arch/tegra.h>
+#include <asm/arch-tegra/board.h>
 #include <asm/arch-tegra/clk_rst.h>
 #include <asm/arch-tegra/pmc.h>
 #include <asm/arch-tegra/scu.h>
diff --git a/arch/arm/cpu/tegra-common/board.c b/arch/arm/cpu/tegra-common/board.c
index ff90a52..b2e10c6 100644
--- a/arch/arm/cpu/tegra-common/board.c
+++ b/arch/arm/cpu/tegra-common/board.c
@@ -26,6 +26,7 @@ 
 #include <asm/arch/clock.h>
 #include <asm/arch/funcmux.h>
 #include <asm/arch/tegra.h>
+#include <asm/arch-tegra/board.h>
 #include <asm/arch-tegra/pmc.h>
 #include <asm/arch-tegra/sys_proto.h>
 #include <asm/arch-tegra/warmboot.h>
diff --git a/arch/arm/include/asm/arch-tegra/board.h b/arch/arm/include/asm/arch-tegra/board.h
index a90d36c..7e56df7 100644
--- a/arch/arm/include/asm/arch-tegra/board.h
+++ b/arch/arm/include/asm/arch-tegra/board.h
@@ -24,7 +24,24 @@ 
 #ifndef _TEGRA_BOARD_H_
 #define _TEGRA_BOARD_H_
 
-/* Setup UARTs for the board according to the selected config */
+/* Set up pinmux to make UART usable */
+void gpio_config_uart(void);      /* CONFIG_SPI_UART_SWITCH */
+void gpio_early_init_uart(void);  /*!CONFIG_SPI_UART_SWITCH */
+
+/* Set up early UART output */
 void board_init_uart_f(void);
 
+/* Set up any early GPIOs the board might need for proper operation */
+void gpio_early_init(void);  /* overrideable GPIO config        */
+
+/*
+ * Hooks to allow boards to set up the pinmux for a specific function.
+ * Has to be implemented in the board files as we don't yet support pinmux
+ * setup from FTD. If a board file does not implement one of those functions
+ * an empty stub function will be called.
+ */
+
+void pin_mux_usb(void);      /* overrideable USB pinmux setup   */
+void pin_mux_spi(void);      /* overrideable SPI pinmux setup   */
+
 #endif
diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index bd194bc..dc301e7 100644
--- a/board/nvidia/common/board.c
+++ b/board/nvidia/common/board.c
@@ -39,7 +39,6 @@ 
 #include <asm/arch-tegra/warmboot.h>
 #include <spi.h>
 #include <i2c.h>
-#include "board.h"
 #include "emc.h"
 
 DECLARE_GLOBAL_DATA_PTR;
diff --git a/board/nvidia/common/board.h b/board/nvidia/common/board.h
deleted file mode 100644
index dada4c4..0000000
--- a/board/nvidia/common/board.h
+++ /dev/null
@@ -1,37 +0,0 @@ 
-/*
- *  (C) Copyright 2010,2011
- *  NVIDIA Corporation <www.nvidia.com>
- *
- * See file CREDITS for list of people who contributed to this
- * project.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of
- * the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- * MA 02111-1307 USA
- */
-
-#ifndef _BOARD_H_
-#define _BOARD_H_
-
-void gpio_config_uart(void);
-void gpio_early_init(void);
-void gpio_early_init_uart(void);
-
-/*
- * Set up any pin muxing needed for USB (for now, since fdt doesn't support
- * it). Boards can overwrite the default fucction which does nothing.
- */
-void pin_mux_usb(void);
-
-#endif	/* BOARD_H */
diff --git a/board/nvidia/common/uart-spi-switch.c b/board/nvidia/common/uart-spi-switch.c
index a0aeb7f..e9d445d 100644
--- a/board/nvidia/common/uart-spi-switch.c
+++ b/board/nvidia/common/uart-spi-switch.c
@@ -26,7 +26,7 @@ 
 #include <asm/arch/uart-spi-switch.h>
 #include <asm/arch/tegra.h>
 #include <asm/arch-tegra/tegra_spi.h>
-
+#include <asm/arch-tegra/board.h>
 
 /* position of the UART/SPI select switch */
 enum spi_uart_switch {