| Message ID | 883852451.2404814.1344959214390.JavaMail.root@advansee.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Tue, Aug 14, 2012 at 10:46 AM, Benoît Thébaudeau <benoit.thebaudeau@advansee.com> wrote: > Create default pin initialization functions for the default iomux function > assignments of the main peripherals. > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > Cc: Stefano Babic <sbabic@denx.de> > --- > .../arch/arm/cpu/armv7/mx5/soc.c | 139 ++++++++++++++++++++ > .../arch/arm/include/asm/arch-mx5/sys_proto.h | 5 + > 2 files changed, 144 insertions(+) > > diff --git u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c > index 3f5a4f7..ee19b54 100644 > --- u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c > +++ u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c > @@ -25,6 +25,8 @@ > > #include <common.h> > #include <asm/arch/imx-regs.h> > +#include <asm/arch/mx5x_pins.h> > +#include <asm/arch/iomux.h> > #include <asm/arch/clock.h> > #include <asm/arch/sys_proto.h> > > @@ -71,6 +73,143 @@ u32 get_cpu_rev(void) > return system_rev; > } > > +#ifdef CONFIG_MXC_UART > +#if CONFIG_MXC_UART_BASE == UART1_BASE > +#ifdef CONFIG_MX51 > +void mx51_uart1_init_pins(void) > +{ > + int in_pad, out_pad; > + > + /* Set up pins for UART1. */ > + mxc_request_iomux(MX51_PIN_UART1_RXD, IOMUX_CONFIG_ALT0); > + mxc_request_iomux(MX51_PIN_UART1_TXD, IOMUX_CONFIG_ALT0); > + mxc_request_iomux(MX51_PIN_UART1_RTS, IOMUX_CONFIG_ALT0); > + mxc_request_iomux(MX51_PIN_UART1_CTS, IOMUX_CONFIG_ALT0); > + > + mxc_iomux_set_input(MX51_UART1_IPP_UART_RXD_MUX_SELECT_INPUT, > + INPUT_CTL_PATH0); > + mxc_iomux_set_input(MX51_UART1_IPP_UART_RTS_B_SELECT_INPUT, > + INPUT_CTL_PATH0); > + > + in_pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE | PAD_CTL_PUE_PULL | > + PAD_CTL_100K_PU | PAD_CTL_ODE_OPENDRAIN_NONE | > + PAD_CTL_DRV_HIGH | PAD_CTL_SRE_SLOW; > + out_pad = PAD_CTL_HYS_NONE | PAD_CTL_PKE_NONE | > + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_HIGH | > + PAD_CTL_SRE_SLOW; If we're nitpicking - none of the UART1_* pads on MX51 have valid ODE bits, it's a reserved area. Even though you're setting it to 0 here, including it in the pad settings is bad behavior.
Dear Matt Sealey, > On Tue, Aug 14, 2012 at 10:46 AM, Benoît Thébaudeau > <benoit.thebaudeau@advansee.com> wrote: > > Create default pin initialization functions for the default iomux > > function > > assignments of the main peripherals. > > > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > > Cc: Stefano Babic <sbabic@denx.de> > > --- > > .../arch/arm/cpu/armv7/mx5/soc.c | 139 > > ++++++++++++++++++++ > > .../arch/arm/include/asm/arch-mx5/sys_proto.h | 5 + > > 2 files changed, 144 insertions(+) > > > > diff --git u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c > > u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c > > index 3f5a4f7..ee19b54 100644 > > --- u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c > > +++ u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c > > @@ -25,6 +25,8 @@ > > > > #include <common.h> > > #include <asm/arch/imx-regs.h> > > +#include <asm/arch/mx5x_pins.h> > > +#include <asm/arch/iomux.h> > > #include <asm/arch/clock.h> > > #include <asm/arch/sys_proto.h> > > > > @@ -71,6 +73,143 @@ u32 get_cpu_rev(void) > > return system_rev; > > } > > > > +#ifdef CONFIG_MXC_UART > > +#if CONFIG_MXC_UART_BASE == UART1_BASE > > +#ifdef CONFIG_MX51 > > +void mx51_uart1_init_pins(void) > > +{ > > + int in_pad, out_pad; > > + > > + /* Set up pins for UART1. */ > > + mxc_request_iomux(MX51_PIN_UART1_RXD, IOMUX_CONFIG_ALT0); > > + mxc_request_iomux(MX51_PIN_UART1_TXD, IOMUX_CONFIG_ALT0); > > + mxc_request_iomux(MX51_PIN_UART1_RTS, IOMUX_CONFIG_ALT0); > > + mxc_request_iomux(MX51_PIN_UART1_CTS, IOMUX_CONFIG_ALT0); > > + > > + > > mxc_iomux_set_input(MX51_UART1_IPP_UART_RXD_MUX_SELECT_INPUT, > > + INPUT_CTL_PATH0); > > + mxc_iomux_set_input(MX51_UART1_IPP_UART_RTS_B_SELECT_INPUT, > > + INPUT_CTL_PATH0); > > + > > + in_pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE | > > PAD_CTL_PUE_PULL | > > + PAD_CTL_100K_PU | PAD_CTL_ODE_OPENDRAIN_NONE | > > + PAD_CTL_DRV_HIGH | PAD_CTL_SRE_SLOW; > > + out_pad = PAD_CTL_HYS_NONE | PAD_CTL_PKE_NONE | > > + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_HIGH | > > + PAD_CTL_SRE_SLOW; > > If we're nitpicking - none of the UART1_* pads on MX51 have valid ODE > bits, it's a reserved area. I know that. > Even though you're setting it to 0 here, Indeed. > including it in the pad settings is bad behavior. No, it's here to emphasize the pad settings that are used. Moreover, in some cases, the same pad value is used for several pads that do not all have the same bits available or not, so that wouldn't make sense to have the same pad setting written in many different ways only because of that. This kind of thing will also very likely occur in your new iomux-mx51.h. Best regards, Benoît
On 14/08/2012 17:46, Benoît Thébaudeau wrote: > Create default pin initialization functions for the default iomux function > assignments of the main peripherals. > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > Cc: Stefano Babic <sbabic@denx.de> > --- Hi Benoît, > .../arch/arm/cpu/armv7/mx5/soc.c | 139 ++++++++++++++++++++ > .../arch/arm/include/asm/arch-mx5/sys_proto.h | 5 + > 2 files changed, 144 insertions(+) > > diff --git u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c > index 3f5a4f7..ee19b54 100644 > --- u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c > +++ u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c > @@ -25,6 +25,8 @@ > > #include <common.h> > #include <asm/arch/imx-regs.h> > +#include <asm/arch/mx5x_pins.h> > +#include <asm/arch/iomux.h> > #include <asm/arch/clock.h> > #include <asm/arch/sys_proto.h> > > @@ -71,6 +73,143 @@ u32 get_cpu_rev(void) > return system_rev; > } > > +#ifdef CONFIG_MXC_UART > +#if CONFIG_MXC_UART_BASE == UART1_BASE > +#ifdef CONFIG_MX51 > +void mx51_uart1_init_pins(void) > +{ > + int in_pad, out_pad; > + > + /* Set up pins for UART1. */ > + mxc_request_iomux(MX51_PIN_UART1_RXD, IOMUX_CONFIG_ALT0); > + mxc_request_iomux(MX51_PIN_UART1_TXD, IOMUX_CONFIG_ALT0); > + mxc_request_iomux(MX51_PIN_UART1_RTS, IOMUX_CONFIG_ALT0); > + mxc_request_iomux(MX51_PIN_UART1_CTS, IOMUX_CONFIG_ALT0); > + > + mxc_iomux_set_input(MX51_UART1_IPP_UART_RXD_MUX_SELECT_INPUT, > + INPUT_CTL_PATH0); > + mxc_iomux_set_input(MX51_UART1_IPP_UART_RTS_B_SELECT_INPUT, > + INPUT_CTL_PATH0); > + > + in_pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE | PAD_CTL_PUE_PULL | > + PAD_CTL_100K_PU | PAD_CTL_ODE_OPENDRAIN_NONE | > + PAD_CTL_DRV_HIGH | PAD_CTL_SRE_SLOW; > + out_pad = PAD_CTL_HYS_NONE | PAD_CTL_PKE_NONE | > + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_HIGH | > + PAD_CTL_SRE_SLOW; > + > + mxc_iomux_set_pad(MX51_PIN_UART1_RXD, in_pad); > + mxc_iomux_set_pad(MX51_PIN_UART1_TXD, out_pad); > + mxc_iomux_set_pad(MX51_PIN_UART1_RTS, in_pad); > + mxc_iomux_set_pad(MX51_PIN_UART1_CTS, out_pad); > +} Yes, the intention is surely to rationalize code and to avoid duplication. But it requires to add such kind of function for any instance of a peripheral (mx51_uart2_init_pins, for example) and cannot worked on all boards. For example, a board will not use RTS and CTS, and these pins are GPIOs (very common example). Or it can use other pins for RX and TXD: the SOC allows this, and maybe someone want to use it. The other thing I see (not in this patch, but checking the current implementation in mainline) is that we have different implementation for MX51, MX53 / MX6. This is bad. I think that setting each pin with mxc_request_iomux() is not a great idea. Maybe another solution would be to provide a table with the pinmux for the whole SOC and a funtion to set all of them, as imx_iomux_v3_setup_multiple_pads. This is also similar to the current implementation in u-boot for other SOCs, for example TI. IMHO to improve the current situation in i.MX pinmux code we should try to use the same code, or at least, the same API for the board maintainer, and putting the pinmux configuration in a table. This is also what the legacy kernel does, putting in the table only the pins required by u-boot itself (current kernel sets itself the pinmux, via device tree or initializing the board when dts is not available). Best regards, Stefano Babic
On Fri, Aug 17, 2012 at 3:25 PM, Stefano Babic <sbabic@denx.de> wrote: > On 14/08/2012 17:46, Benoît Thébaudeau wrote: >> Create default pin initialization functions for the default iomux function >> assignments of the main peripherals. >> >> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> >> Cc: Stefano Babic <sbabic@denx.de> >> --- > > I think that setting each pin with mxc_request_iomux() is not a great > idea. Maybe another solution would be to provide a table with the pinmux > for the whole SOC and a funtion to set all of them, as > imx_iomux_v3_setup_multiple_pads. This is also similar to the current > implementation in u-boot for other SOCs, for example TI. Done and submitted for MX51 - and Efika MX is ported and through QA. Can you review? Benoit has some valid suggestions for changes, which can come later, but for now moving to that for MX51 boards should be a net zero difference for all implemented boards. I don't have space on my desk to fire up my MX53 Quickstart or EVK to test those, but porting the files for MX53 from Linux should not be a big deal and further combining the 3 implementations differences (moving GPIO_NUMBER to imx-common/iomux-v3.h instead of it being SoC-specific for example) is something that would better be done once everything's in and working.
diff --git u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c index 3f5a4f7..ee19b54 100644 --- u-boot-4d3c95f.orig/arch/arm/cpu/armv7/mx5/soc.c +++ u-boot-4d3c95f/arch/arm/cpu/armv7/mx5/soc.c @@ -25,6 +25,8 @@ #include <common.h> #include <asm/arch/imx-regs.h> +#include <asm/arch/mx5x_pins.h> +#include <asm/arch/iomux.h> #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> @@ -71,6 +73,143 @@ u32 get_cpu_rev(void) return system_rev; } +#ifdef CONFIG_MXC_UART +#if CONFIG_MXC_UART_BASE == UART1_BASE +#ifdef CONFIG_MX51 +void mx51_uart1_init_pins(void) +{ + int in_pad, out_pad; + + /* Set up pins for UART1. */ + mxc_request_iomux(MX51_PIN_UART1_RXD, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_UART1_TXD, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_UART1_RTS, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_UART1_CTS, IOMUX_CONFIG_ALT0); + + mxc_iomux_set_input(MX51_UART1_IPP_UART_RXD_MUX_SELECT_INPUT, + INPUT_CTL_PATH0); + mxc_iomux_set_input(MX51_UART1_IPP_UART_RTS_B_SELECT_INPUT, + INPUT_CTL_PATH0); + + in_pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE | PAD_CTL_PUE_PULL | + PAD_CTL_100K_PU | PAD_CTL_ODE_OPENDRAIN_NONE | + PAD_CTL_DRV_HIGH | PAD_CTL_SRE_SLOW; + out_pad = PAD_CTL_HYS_NONE | PAD_CTL_PKE_NONE | + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_HIGH | + PAD_CTL_SRE_SLOW; + + mxc_iomux_set_pad(MX51_PIN_UART1_RXD, in_pad); + mxc_iomux_set_pad(MX51_PIN_UART1_TXD, out_pad); + mxc_iomux_set_pad(MX51_PIN_UART1_RTS, in_pad); + mxc_iomux_set_pad(MX51_PIN_UART1_CTS, out_pad); +} +#endif +#endif +#endif + +#ifdef CONFIG_MXC_SPI +void mx51_ecspi1_init_pins(void) +{ + int act_lo_pad, act_hi_pad; + + /* Set up pins for eCSPI1. */ + mxc_request_iomux(MX51_PIN_CSPI1_SCLK, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_CSPI1_RDY, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_CSPI1_MOSI, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_CSPI1_MISO, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_CSPI1_SS0, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_CSPI1_SS1, IOMUX_CONFIG_ALT0); + + act_lo_pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE | + PAD_CTL_PUE_PULL | PAD_CTL_100K_PU | + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_HIGH | + PAD_CTL_SRE_SLOW; + act_hi_pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE | + PAD_CTL_PUE_PULL | PAD_CTL_100K_PD | + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_HIGH | + PAD_CTL_SRE_SLOW; + + mxc_iomux_set_pad(MX51_PIN_CSPI1_SCLK, act_hi_pad); + mxc_iomux_set_pad(MX51_PIN_CSPI1_RDY, act_lo_pad); + mxc_iomux_set_pad(MX51_PIN_CSPI1_MOSI, act_hi_pad); + mxc_iomux_set_pad(MX51_PIN_CSPI1_MISO, act_hi_pad); + mxc_iomux_set_pad(MX51_PIN_CSPI1_SS0, act_lo_pad); + mxc_iomux_set_pad(MX51_PIN_CSPI1_SS1, act_lo_pad); +} +#endif + +#ifdef CONFIG_FSL_ESDHC +void mx51_esdhc1_init_pins(void) +{ + int out_pad, io_pad; + + /* Set up pins for eSDHC1. */ + mxc_request_iomux(MX51_PIN_SD1_CMD, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD1_CLK, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD1_DATA0, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD1_DATA1, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD1_DATA2, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD1_DATA3, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_GPIO1_1, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); /* WP */ + mxc_request_iomux(MX51_PIN_GPIO1_0, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); /* CD */ + + out_pad = PAD_CTL_DRV_VOT_HIGH | PAD_CTL_HYS_NONE | PAD_CTL_PKE_ENABLE | + PAD_CTL_PUE_PULL | PAD_CTL_47K_PU | PAD_CTL_ODE_OPENDRAIN_NONE | + PAD_CTL_DRV_MAX | PAD_CTL_SRE_FAST; + io_pad = PAD_CTL_DRV_VOT_HIGH | PAD_CTL_HYS_ENABLE | + PAD_CTL_PKE_ENABLE | PAD_CTL_PUE_PULL | PAD_CTL_47K_PU | + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_MAX | PAD_CTL_SRE_FAST; + + mxc_iomux_set_pad(MX51_PIN_SD1_CMD, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD1_CLK, out_pad); + mxc_iomux_set_pad(MX51_PIN_SD1_DATA0, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD1_DATA1, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD1_DATA2, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD1_DATA3, io_pad); + mxc_iomux_set_pad(MX51_PIN_GPIO1_1, PAD_CTL_HYS_ENABLE); /* WP */ + mxc_iomux_set_pad(MX51_PIN_GPIO1_0, PAD_CTL_HYS_ENABLE); /* CD */ +} + +void mx51_esdhc2_init_pins(void) +{ + int out_pad, io_pad; + + /* Set up pins for eSDHC2. */ + mxc_request_iomux(MX51_PIN_SD2_CMD, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD2_CLK, IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD2_DATA0, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD2_DATA1, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD2_DATA2, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + mxc_request_iomux(MX51_PIN_SD2_DATA3, + IOMUX_CONFIG_SION | IOMUX_CONFIG_ALT0); + + out_pad = PAD_CTL_DRV_VOT_HIGH | PAD_CTL_HYS_NONE | PAD_CTL_PKE_ENABLE | + PAD_CTL_PUE_PULL | PAD_CTL_47K_PU | PAD_CTL_ODE_OPENDRAIN_NONE | + PAD_CTL_DRV_MAX | PAD_CTL_SRE_FAST; + io_pad = PAD_CTL_DRV_VOT_HIGH | PAD_CTL_HYS_ENABLE | + PAD_CTL_PKE_ENABLE | PAD_CTL_PUE_PULL | PAD_CTL_47K_PU | + PAD_CTL_ODE_OPENDRAIN_NONE | PAD_CTL_DRV_MAX | PAD_CTL_SRE_FAST; + + mxc_iomux_set_pad(MX51_PIN_SD2_CMD, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD2_CLK, out_pad); + mxc_iomux_set_pad(MX51_PIN_SD2_DATA0, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD2_DATA1, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD2_DATA2, io_pad); + mxc_iomux_set_pad(MX51_PIN_SD2_DATA3, io_pad); +} +#endif + #if defined(CONFIG_FEC_MXC) void imx_get_mac_from_fuse(int dev_id, unsigned char *mac) { diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx5/sys_proto.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx5/sys_proto.h index 7b5246e..ce4a94c 100644 --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx5/sys_proto.h +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx5/sys_proto.h @@ -39,4 +39,9 @@ u32 get_ahb_clk(void); u32 get_periph_clk(void); char *get_reset_cause(void); +void mx51_uart1_init_pins(void); +void mx51_ecspi1_init_pins(void); +void mx51_esdhc1_init_pins(void); +void mx51_esdhc2_init_pins(void); + #endif
Create default pin initialization functions for the default iomux function assignments of the main peripherals. Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> Cc: Stefano Babic <sbabic@denx.de> --- .../arch/arm/cpu/armv7/mx5/soc.c | 139 ++++++++++++++++++++ .../arch/arm/include/asm/arch-mx5/sys_proto.h | 5 + 2 files changed, 144 insertions(+)