Hello Stelian and Jean-Christophe, > I agree with the general idea of moving the serial and spi from board > specific files to the cpu specific files.
I agree partly on this, but I do not like with the way this patch is doing it, and this is the reason why: PIN multiplexing is board specific, not only SoC specific. pins can be multiplexed in different ways per board per SoC. In this patch all this 'knowledge' is moved to the generic SoC code, where the assumption is made that all boards use the same pin layout. This is _not_ true; in the generic code is, for example(!!), assumed that all 9261 SoC derived boards only configure NPCS0, while a board can utilise all NPCS pins and can have multiple dataflashes. It may be true that all boards, currently existing in U-boot, use a similar pin layout per SoC, but that does not mean that that is a valid assumption for all future boards. The framework should be that flexible that boards that are currently not in mainline can be added in the future, without reverting patches like this, or creating standalone copies from this framework. We should make it easier for others to integrate their custom boards, instead of making it harder... The current code makes it very easy to create custom boards, this patch makes that harder! So, in short: pin layout must be specified by the board specific files, not by the generic SoC code, although the generic SoC code can provide (all) the possible options to choose from... Kind Regards, Remy > > Note however that serial and spi are not the only initialisations that > can be moved: the very same thing can be done with all the other device > initialisations. Some devices will need to specify which GPIOs are used > when it's board specific, and this can be passed as arguments to the cpu > specfic function. One can look at the Linux kernel files which nicely > separates the cpu specific GPIOs (in arch/arm/mach-at91/XXX_devices.c) > and the board specific GPIOs (in arch/arm/mach-at91/board-YYY.c). Or one > can also RTFM of course. > > I also don't like this: > >> --- a/board/atmel/at91cap9adk/at91cap9adk.c >> +++ b/board/atmel/at91cap9adk/at91cap9adk.c > >> #ifdef CONFIG_USART0 >> - at91_set_A_periph(AT91_PIN_PA22, 1); /* TXD0 */ >> - at91_set_A_periph(AT91_PIN_PA23, 0); /* RXD0 */ >> - at91_sys_write(AT91_PMC_PCER, 1 << AT91CAP9_ID_US0); >> + at91_serial_hw_init(0); >> #endif > > Why don't just do at91_serial_hw_init() once, and test the CONFIG_USART* > inside the cpu specific at91_serial_hw_init function() ? > > This also eliminates that unneeded switch case in that function. > > Stelian. > -- > Stelian Pop <[EMAIL PROTECTED]> > > _______________________________________________ > U-Boot mailing list > [email protected] > http://lists.denx.de/mailman/listinfo/u-boot > _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
