| Message ID | 1348782724-19151-1-git-send-email-dev@lynxeye.de |
|---|---|
| State | Accepted |
| Delegated to: | Tom Warren |
| Headers | show |
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.
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
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
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?
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
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 {
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