Patchwork [U-Boot,RFC,5/8] arm, davinci: Replace pinmuxing in da850_lowlevel.c

login
register
mail settings
Submitter Christian Riesch
Date Nov. 15, 2011, 10:37 a.m.
Message ID <1321353452-28843-6-git-send-email-christian.riesch@omicron.at>
Download mbox | patch
Permalink /patch/125732/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Christian Riesch - Nov. 15, 2011, 10:37 a.m.
This patch replaces the pinmuxing functions from
arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c by those of
arch/arm/cpu/arm926ejs/davinci/pinmux.c

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
Cc: Heiko Schocher <hs@denx.de>
Cc: Sandeep Paulraj <s-paulraj@ti.com>
---
 arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |   34 +++++++++--------------
 1 files changed, 13 insertions(+), 21 deletions(-)
Heiko Schocher - Nov. 16, 2011, 6:49 a.m.
Hello Christian,

Christian Riesch wrote:
> This patch replaces the pinmuxing functions from
> arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c by those of
> arch/arm/cpu/arm926ejs/davinci/pinmux.c
> 
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Sandeep Paulraj <s-paulraj@ti.com>
> ---
>  arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |   34 +++++++++--------------
>  1 files changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
> index c7ec70f..8dd897b 100644
> --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
> +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
> @@ -30,6 +30,7 @@
>  #include <asm/arch/ddr2_defs.h>
>  #include <asm/arch/emif_defs.h>
>  #include <asm/arch/pll_defs.h>
> +#include <asm/arch/davinci_misc.h>
>  
>  void da850_waitloop(unsigned long loopcnt)
>  {
> @@ -248,6 +249,16 @@ void board_gpio_init(void)
>  	return;
>  }
>  
> +/* UART pin muxer settings */
> +static const struct pinmux_config uart_pins[] = {
> +#if CONFIG_SYS_NS16550_COM1 == DAVINCI_UART2_BASE
> +	{ pinmux(0), 4, 6 },
> +	{ pinmux(0), 4, 7 },
> +	{ pinmux(4), 2, 4 },
> +	{ pinmux(4), 2, 5 }
> +#endif
> +};
> +
>  int arch_cpu_init(void)
>  {
>  	/* Unlock kick registers */
> @@ -257,27 +268,8 @@ int arch_cpu_init(void)
>  	dv_maskbits(&davinci_syscfg_regs->suspsrc,
>  		CONFIG_SYS_DA850_SYSCFG_SUSPSRC);
>  
> -	/* Setup Pinmux */
> -	da850_pinmux_ctl(0, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX0);
> -	da850_pinmux_ctl(1, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX1);
> -	da850_pinmux_ctl(2, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX2);
> -	da850_pinmux_ctl(3, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX3);
> -	da850_pinmux_ctl(4, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX4);
> -	da850_pinmux_ctl(5, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX5);
> -	da850_pinmux_ctl(6, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX6);
> -	da850_pinmux_ctl(7, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX7);
> -	da850_pinmux_ctl(8, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX8);
> -	da850_pinmux_ctl(9, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX9);
> -	da850_pinmux_ctl(10, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX10);
> -	da850_pinmux_ctl(11, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX11);
> -	da850_pinmux_ctl(12, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX12);
> -	da850_pinmux_ctl(13, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX13);
> -	da850_pinmux_ctl(14, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX14);
> -	da850_pinmux_ctl(15, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX15);
> -	da850_pinmux_ctl(16, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX16);
> -	da850_pinmux_ctl(17, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX17);
> -	da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18);
> -	da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19);
> +	/* setup serial port */
> +	davinci_configure_pin_mux(uart_pins, ARRAY_SIZE(uart_pins));

Why only the uart pins? We could use here something like "board_pins"
and initialize here all pins for the board?

I reworked this for the enbw_cmc board too, and removed also the
CONFIG_SYS_DA850_PINMUX* defines complete ... but I am not really
happy with it. Why?

We have for example on the am1808 19 * 8 = 152 pins to setup up

If using the CONFIG_SYS_DA850_PINMUX* defines we have 19 register-
writes and have setup them all (And you must think about all
your pins, if we use such a struct, not defined pins are in
default state ... which is good or bad ...)

With using davinci_configure_pin_mux() we have 152 * (read, write
and some logic operations) ... and I have to code a "static const
struct pinmux_config board_pins" with 152 lines in the code ...

What do others think?

bye,
Heiko
Christian Riesch - Nov. 18, 2011, 8:22 a.m.
Hello Heiko,

On Wed, Nov 16, 2011 at 7:49 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Christian,
>
> Christian Riesch wrote:
>> This patch replaces the pinmuxing functions from
>> arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c by those of
>> arch/arm/cpu/arm926ejs/davinci/pinmux.c
>>
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Heiko Schocher <hs@denx.de>
>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>> ---
>>  arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |   34 +++++++++--------------
>>  1 files changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> index c7ec70f..8dd897b 100644
>> --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> @@ -30,6 +30,7 @@
>>  #include <asm/arch/ddr2_defs.h>
>>  #include <asm/arch/emif_defs.h>
>>  #include <asm/arch/pll_defs.h>
>> +#include <asm/arch/davinci_misc.h>
>>
>>  void da850_waitloop(unsigned long loopcnt)
>>  {
>> @@ -248,6 +249,16 @@ void board_gpio_init(void)
>>       return;
>>  }
>>
>> +/* UART pin muxer settings */
>> +static const struct pinmux_config uart_pins[] = {
>> +#if CONFIG_SYS_NS16550_COM1 == DAVINCI_UART2_BASE
>> +     { pinmux(0), 4, 6 },
>> +     { pinmux(0), 4, 7 },
>> +     { pinmux(4), 2, 4 },
>> +     { pinmux(4), 2, 5 }
>> +#endif
>> +};
>> +
>>  int arch_cpu_init(void)
>>  {
>>       /* Unlock kick registers */
>> @@ -257,27 +268,8 @@ int arch_cpu_init(void)
>>       dv_maskbits(&davinci_syscfg_regs->suspsrc,
>>               CONFIG_SYS_DA850_SYSCFG_SUSPSRC);
>>
>> -     /* Setup Pinmux */
>> -     da850_pinmux_ctl(0, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX0);
>> -     da850_pinmux_ctl(1, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX1);
>> -     da850_pinmux_ctl(2, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX2);
>> -     da850_pinmux_ctl(3, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX3);
>> -     da850_pinmux_ctl(4, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX4);
>> -     da850_pinmux_ctl(5, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX5);
>> -     da850_pinmux_ctl(6, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX6);
>> -     da850_pinmux_ctl(7, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX7);
>> -     da850_pinmux_ctl(8, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX8);
>> -     da850_pinmux_ctl(9, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX9);
>> -     da850_pinmux_ctl(10, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX10);
>> -     da850_pinmux_ctl(11, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX11);
>> -     da850_pinmux_ctl(12, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX12);
>> -     da850_pinmux_ctl(13, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX13);
>> -     da850_pinmux_ctl(14, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX14);
>> -     da850_pinmux_ctl(15, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX15);
>> -     da850_pinmux_ctl(16, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX16);
>> -     da850_pinmux_ctl(17, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX17);
>> -     da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18);
>> -     da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19);
>> +     /* setup serial port */
>> +     davinci_configure_pin_mux(uart_pins, ARRAY_SIZE(uart_pins));
>
> Why only the uart pins? We could use here something like "board_pins"
> and initialize here all pins for the board?

Because only the UART pins are required here. Since the CPU has
already loaded the SPL from SPI flash or is executing the SPL from NOR
flash or whatever, the pins for memory access are already configured.
Later the board specific file can do all the configuration that it
actually needs, see board/davinci/da8xxevm/da850evm.c.

> I reworked this for the enbw_cmc board too, and removed also the
> CONFIG_SYS_DA850_PINMUX* defines complete ... but I am not really
> happy with it. Why?
>
> We have for example on the am1808 19 * 8 = 152 pins to setup up
>
> If using the CONFIG_SYS_DA850_PINMUX* defines we have 19 register-
> writes and have setup them all (And you must think about all
> your pins, if we use such a struct, not defined pins are in
> default state ... which is good or bad ...)
>
> With using davinci_configure_pin_mux() we have 152 * (read, write
> and some logic operations)

Actually the number of read, writes, logic operations will depend on
the number of GPIO pins you use on your board. I guess you will not
change the pinmux settings of pins you didn't connect on your board.
But yes, these are a lot of operations that need to be done.

... and I have to code a "static const
> struct pinmux_config board_pins" with 152 lines in the code ...

How about using an approach like in board/davinci/da8xxevm/da850evm.c.
There we have several structs like

static const struct pinmux config_spi1_pins[] = {
...
}

that defines pinmux for groups of pins that are usually used together.

Later, these groups are put together in

static const struct pinmux_resource pinmuxes[] = {

}


>
> What do others think?
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Christian Riesch - Nov. 18, 2011, 8:24 a.m.
Hello Heiko,
sorry for my last (incomplete) mail.

On Wed, Nov 16, 2011 at 7:49 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Christian,
>
> Christian Riesch wrote:
>> This patch replaces the pinmuxing functions from
>> arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c by those of
>> arch/arm/cpu/arm926ejs/davinci/pinmux.c
>>
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Heiko Schocher <hs@denx.de>
>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>> ---
>>  arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |   34 +++++++++--------------
>>  1 files changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> index c7ec70f..8dd897b 100644
>> --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> @@ -30,6 +30,7 @@
>>  #include <asm/arch/ddr2_defs.h>
>>  #include <asm/arch/emif_defs.h>
>>  #include <asm/arch/pll_defs.h>
>> +#include <asm/arch/davinci_misc.h>
>>
>>  void da850_waitloop(unsigned long loopcnt)
>>  {
>> @@ -248,6 +249,16 @@ void board_gpio_init(void)
>>       return;
>>  }
>>
>> +/* UART pin muxer settings */
>> +static const struct pinmux_config uart_pins[] = {
>> +#if CONFIG_SYS_NS16550_COM1 == DAVINCI_UART2_BASE
>> +     { pinmux(0), 4, 6 },
>> +     { pinmux(0), 4, 7 },
>> +     { pinmux(4), 2, 4 },
>> +     { pinmux(4), 2, 5 }
>> +#endif
>> +};
>> +
>>  int arch_cpu_init(void)
>>  {
>>       /* Unlock kick registers */
>> @@ -257,27 +268,8 @@ int arch_cpu_init(void)
>>       dv_maskbits(&davinci_syscfg_regs->suspsrc,
>>               CONFIG_SYS_DA850_SYSCFG_SUSPSRC);
>>
>> -     /* Setup Pinmux */
>> -     da850_pinmux_ctl(0, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX0);
>> -     da850_pinmux_ctl(1, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX1);
>> -     da850_pinmux_ctl(2, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX2);
>> -     da850_pinmux_ctl(3, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX3);
>> -     da850_pinmux_ctl(4, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX4);
>> -     da850_pinmux_ctl(5, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX5);
>> -     da850_pinmux_ctl(6, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX6);
>> -     da850_pinmux_ctl(7, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX7);
>> -     da850_pinmux_ctl(8, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX8);
>> -     da850_pinmux_ctl(9, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX9);
>> -     da850_pinmux_ctl(10, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX10);
>> -     da850_pinmux_ctl(11, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX11);
>> -     da850_pinmux_ctl(12, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX12);
>> -     da850_pinmux_ctl(13, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX13);
>> -     da850_pinmux_ctl(14, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX14);
>> -     da850_pinmux_ctl(15, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX15);
>> -     da850_pinmux_ctl(16, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX16);
>> -     da850_pinmux_ctl(17, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX17);
>> -     da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18);
>> -     da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19);
>> +     /* setup serial port */
>> +     davinci_configure_pin_mux(uart_pins, ARRAY_SIZE(uart_pins));
>
> Why only the uart pins? We could use here something like "board_pins"
> and initialize here all pins for the board?
Because only the UART pins are required here. Since the CPU has
already loaded the SPL from SPI flash or is executing the SPL from NOR
flash or whatever, the pins for memory access are already configured.
Later the board specific file can do all the configuration that it
actually needs, see board/davinci/da8xxevm/da850evm.c.

>
> I reworked this for the enbw_cmc board too, and removed also the
> CONFIG_SYS_DA850_PINMUX* defines complete ... but I am not really
> happy with it. Why?
>
> We have for example on the am1808 19 * 8 = 152 pins to setup up
>
> If using the CONFIG_SYS_DA850_PINMUX* defines we have 19 register-
> writes and have setup them all (And you must think about all
> your pins, if we use such a struct, not defined pins are in
> default state ... which is good or bad ...)
>
> With using davinci_configure_pin_mux() we have 152 * (read, write
> and some logic operations) ...

Actually the number of read, writes, logic operations will depend on
the number of GPIO pins you use on your board. I guess you will not
change the pinmux settings of pins you didn't connect on your board.
But yes, these are a lot of operations that need to be done.

>and I have to code a "static const
> struct pinmux_config board_pins" with 152 lines in the code ...

How about using an approach like in board/davinci/da8xxevm/da850evm.c.
There we have several structs like

static const struct pinmux config_spi1_pins[] = {
...
}

that defines pinmux for groups of pins that are usually used together.

Later, these groups are put together in

static const struct pinmux_resource pinmuxes[] = {


>
> What do others think?
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Christian Riesch - Nov. 18, 2011, 8:35 a.m.
Hello Heiko,
I hope this is the complete mail now :-/

On Wed, Nov 16, 2011 at 7:49 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Christian,
>
> Christian Riesch wrote:
>> This patch replaces the pinmuxing functions from
>> arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c by those of
>> arch/arm/cpu/arm926ejs/davinci/pinmux.c
>>
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Heiko Schocher <hs@denx.de>
>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>> ---
>>  arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |   34 +++++++++--------------
>>  1 files changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> index c7ec70f..8dd897b 100644
>> --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> @@ -30,6 +30,7 @@
>>  #include <asm/arch/ddr2_defs.h>
>>  #include <asm/arch/emif_defs.h>
>>  #include <asm/arch/pll_defs.h>
>> +#include <asm/arch/davinci_misc.h>
>>
>>  void da850_waitloop(unsigned long loopcnt)
>>  {
>> @@ -248,6 +249,16 @@ void board_gpio_init(void)
>>       return;
>>  }
>>
>> +/* UART pin muxer settings */
>> +static const struct pinmux_config uart_pins[] = {
>> +#if CONFIG_SYS_NS16550_COM1 == DAVINCI_UART2_BASE
>> +     { pinmux(0), 4, 6 },
>> +     { pinmux(0), 4, 7 },
>> +     { pinmux(4), 2, 4 },
>> +     { pinmux(4), 2, 5 }
>> +#endif
>> +};
>> +
>>  int arch_cpu_init(void)
>>  {
>>       /* Unlock kick registers */
>> @@ -257,27 +268,8 @@ int arch_cpu_init(void)
>>       dv_maskbits(&davinci_syscfg_regs->suspsrc,
>>               CONFIG_SYS_DA850_SYSCFG_SUSPSRC);
>>
>> -     /* Setup Pinmux */
>> -     da850_pinmux_ctl(0, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX0);
>> -     da850_pinmux_ctl(1, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX1);
>> -     da850_pinmux_ctl(2, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX2);
>> -     da850_pinmux_ctl(3, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX3);
>> -     da850_pinmux_ctl(4, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX4);
>> -     da850_pinmux_ctl(5, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX5);
>> -     da850_pinmux_ctl(6, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX6);
>> -     da850_pinmux_ctl(7, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX7);
>> -     da850_pinmux_ctl(8, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX8);
>> -     da850_pinmux_ctl(9, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX9);
>> -     da850_pinmux_ctl(10, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX10);
>> -     da850_pinmux_ctl(11, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX11);
>> -     da850_pinmux_ctl(12, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX12);
>> -     da850_pinmux_ctl(13, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX13);
>> -     da850_pinmux_ctl(14, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX14);
>> -     da850_pinmux_ctl(15, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX15);
>> -     da850_pinmux_ctl(16, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX16);
>> -     da850_pinmux_ctl(17, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX17);
>> -     da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18);
>> -     da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19);
>> +     /* setup serial port */
>> +     davinci_configure_pin_mux(uart_pins, ARRAY_SIZE(uart_pins));
>
> Why only the uart pins? We could use here something like "board_pins"
> and initialize here all pins for the board?

Because only the UART pins are required here. Since the CPU has
already loaded the SPL from SPI flash or is executing the SPL from NOR
flash or whatever, the pins for memory access are already configured.
Later the board specific file can do all the configuration that it
actually needs, see board/davinci/da8xxevm/da850evm.c.

>
> I reworked this for the enbw_cmc board too, and removed also the
> CONFIG_SYS_DA850_PINMUX* defines complete ... but I am not really
> happy with it. Why?
>
> We have for example on the am1808 19 * 8 = 152 pins to setup up
>
> If using the CONFIG_SYS_DA850_PINMUX* defines we have 19 register-
> writes and have setup them all (And you must think about all
> your pins, if we use such a struct, not defined pins are in
> default state ... which is good or bad ...)
>
> With using davinci_configure_pin_mux() we have 152 * (read, write
> and some logic operations) ...

Actually the number of read, writes, logic operations will depend on
the number of GPIO pins you use on your board. I guess you will not
change the pinmux settings of pins you didn't connect on your board.
But yes, these are a lot of operations that need to be done.

>and I have to code a "static const
> struct pinmux_config board_pins" with 152 lines in the code ...

How about using an approach like in board/davinci/da8xxevm/da850evm.c.
There we have several structs like

static const struct pinmux_config spi1_pins[] = {
...
}

that defines pinmux for groups of pins that are usually used together.

Later, these groups are put together in

static const struct pinmux_resource pinmuxes[] = {
        { DAVINCI_LPSC_AEMIF }, /* NAND, NOR */
        { DAVINCI_LPSC_SPI1 },  /* Serial Flash */
        { DAVINCI_LPSC_EMAC },  /* image download */
        { DAVINCI_LPSC_UART2 }, /* console */
        { DAVINCI_LPSC_GPIO },
};

We could move the pinmux_config structs to a header file which would reduce
the code in your board config file to a few lines, you only would need
a pinmux_resource struct.

Still we need to do pinmuxing for UART (and maybe also for other pins) in
the SPL. How do you like the approach in static void set_mux_conf_regs(void)
in arch/arm/cpu/armv7/omap-common/hwinit-common.c? Depending on the
context either the pins that are essential for the SPL or
all other pins are configured.

This would at least reduce the number of code lines that you need for a
new board. And this code would be much easier to understand than this
list of magic numbers.

> What do others think?
>
> bye,
> Heiko

Regards, Christian
Heiko Schocher - Nov. 18, 2011, 10:09 a.m.
Hello Christian,

Christian Riesch wrote:
> Hello Heiko,
> I hope this is the complete mail now :-/

seems so ...

> On Wed, Nov 16, 2011 at 7:49 AM, Heiko Schocher <hs@denx.de> wrote:
>> Hello Christian,
>>
>> Christian Riesch wrote:
[...]
>>> -     da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18);
>>> -     da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19);
>>> +     /* setup serial port */
>>> +     davinci_configure_pin_mux(uart_pins, ARRAY_SIZE(uart_pins));
>> Why only the uart pins? We could use here something like "board_pins"
>> and initialize here all pins for the board?
> 
> Because only the UART pins are required here. Since the CPU has

And if you need some other pins, for example gpio?

> already loaded the SPL from SPI flash or is executing the SPL from NOR
> flash or whatever, the pins for memory access are already configured.
> Later the board specific file can do all the configuration that it
> actually needs, see board/davinci/da8xxevm/da850evm.c.

Yes, but, why not setup all pinmux settings immediately in the
spl code?

>> I reworked this for the enbw_cmc board too, and removed also the
>> CONFIG_SYS_DA850_PINMUX* defines complete ... but I am not really
>> happy with it. Why?
>>
>> We have for example on the am1808 19 * 8 = 152 pins to setup up
>>
>> If using the CONFIG_SYS_DA850_PINMUX* defines we have 19 register-
>> writes and have setup them all (And you must think about all
>> your pins, if we use such a struct, not defined pins are in
>> default state ... which is good or bad ...)
>>
>> With using davinci_configure_pin_mux() we have 152 * (read, write
>> and some logic operations) ...
> 
> Actually the number of read, writes, logic operations will depend on
> the number of GPIO pins you use on your board. I guess you will not
> change the pinmux settings of pins you didn't connect on your board.
> But yes, these are a lot of operations that need to be done.

Not with the define approach! ... There we have only 19 register
writes.

>> and I have to code a "static const
>> struct pinmux_config board_pins" with 152 lines in the code ...
> 
> How about using an approach like in board/davinci/da8xxevm/da850evm.c.
> There we have several structs like
> 
> static const struct pinmux_config spi1_pins[] = {
> ...
> }
> 
> that defines pinmux for groups of pins that are usually used together.
> 
> Later, these groups are put together in
> 
> static const struct pinmux_resource pinmuxes[] = {
>         { DAVINCI_LPSC_AEMIF }, /* NAND, NOR */
>         { DAVINCI_LPSC_SPI1 },  /* Serial Flash */
>         { DAVINCI_LPSC_EMAC },  /* image download */
>         { DAVINCI_LPSC_UART2 }, /* console */
>         { DAVINCI_LPSC_GPIO },
> };

You mean here:

static const struct pinmux_resource pinmuxes[] = {
#ifdef CONFIG_SPI_FLASH
        PINMUX_ITEM(spi1_pins),
#endif
        PINMUX_ITEM(uart_pins),
        PINMUX_ITEM(i2c_pins),
#ifdef CONFIG_NAND_DAVINCI
        PINMUX_ITEM(nand_pins),
#elif defined(CONFIG_USE_NOR)
        PINMUX_ITEM(nor_pins),
#endif
};

right?

> We could move the pinmux_config structs to a header file which would reduce
> the code in your board config file to a few lines, you only would need
> a pinmux_resource struct.

Yep, if we go this way, we should move them to a include
file, so we can use them for all da8xx boards.

> Still we need to do pinmuxing for UART (and maybe also for other pins) in
> the SPL. How do you like the approach in static void set_mux_conf_regs(void)
> in arch/arm/cpu/armv7/omap-common/hwinit-common.c? Depending on the
> context either the pins that are essential for the SPL or
> all other pins are configured.

Yes, looks good to me.

> This would at least reduce the number of code lines that you need for a
> new board. And this code would be much easier to understand than this
> list of magic numbers.

Yes. Don't understand me wrong against the "struct pinmux_resource"
approach, it looks  good to me also, and I agree this is (maybe) better
to read (maybe, because if something is setup wrong, you need in both
approaches the help from the doc ...), but I didn't get the disadvantages
of "my" define approach setting up the pinmux in one place ...

Advantages of it I think:
- if settings are wrong i find it fast (because in one place)
- setup with a minimum nr of instructions.
- smaller code size
- if using the pinmux setup tool from TI
  (URL: http://www-s.ti.com/sc/techlit/spraba2.zip)
  you can easy setup all pins and gets a check for pinmux conflicts
  for free ... and it exports a header file, from where you easy can
  get the values for the CONFIG_SYS_DA850_PINMUX* defines ... if you
  want to use this tool, it is more work to convert this to the
  "struct pinmux_resource" approach ...

So let us now decide which way to go ... (BTW: If we decide to go the
"struct pinmux_resource" approach, can you provide a patch, which
moves the pinmux settings from the existing da8xx boards to an include
file (whats with arch/arm/include/asm/arch-davinci/da8xx_pinmux.h)?

bye,
Heiko

Patch

diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
index c7ec70f..8dd897b 100644
--- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
+++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
@@ -30,6 +30,7 @@ 
 #include <asm/arch/ddr2_defs.h>
 #include <asm/arch/emif_defs.h>
 #include <asm/arch/pll_defs.h>
+#include <asm/arch/davinci_misc.h>
 
 void da850_waitloop(unsigned long loopcnt)
 {
@@ -248,6 +249,16 @@  void board_gpio_init(void)
 	return;
 }
 
+/* UART pin muxer settings */
+static const struct pinmux_config uart_pins[] = {
+#if CONFIG_SYS_NS16550_COM1 == DAVINCI_UART2_BASE
+	{ pinmux(0), 4, 6 },
+	{ pinmux(0), 4, 7 },
+	{ pinmux(4), 2, 4 },
+	{ pinmux(4), 2, 5 }
+#endif
+};
+
 int arch_cpu_init(void)
 {
 	/* Unlock kick registers */
@@ -257,27 +268,8 @@  int arch_cpu_init(void)
 	dv_maskbits(&davinci_syscfg_regs->suspsrc,
 		CONFIG_SYS_DA850_SYSCFG_SUSPSRC);
 
-	/* Setup Pinmux */
-	da850_pinmux_ctl(0, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX0);
-	da850_pinmux_ctl(1, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX1);
-	da850_pinmux_ctl(2, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX2);
-	da850_pinmux_ctl(3, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX3);
-	da850_pinmux_ctl(4, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX4);
-	da850_pinmux_ctl(5, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX5);
-	da850_pinmux_ctl(6, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX6);
-	da850_pinmux_ctl(7, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX7);
-	da850_pinmux_ctl(8, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX8);
-	da850_pinmux_ctl(9, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX9);
-	da850_pinmux_ctl(10, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX10);
-	da850_pinmux_ctl(11, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX11);
-	da850_pinmux_ctl(12, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX12);
-	da850_pinmux_ctl(13, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX13);
-	da850_pinmux_ctl(14, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX14);
-	da850_pinmux_ctl(15, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX15);
-	da850_pinmux_ctl(16, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX16);
-	da850_pinmux_ctl(17, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX17);
-	da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18);
-	da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19);
+	/* setup serial port */
+	davinci_configure_pin_mux(uart_pins, ARRAY_SIZE(uart_pins));
 
 	/* PLL setup */
 	da850_pll_init(davinci_pllc0_regs, CONFIG_SYS_DA850_PLL0_PLLM);