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

[Heiko Schocher <hs@denx.de>]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany