On 21:52 Sat 04 Apr , Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
>
> In message <1238840105-19029-1-git-send-email-plagn...@jcrosoft.com> you
> wrote:
> > The new Framework is not yet activated by default, it will be next release
> > at the same time of the removal of the DATAFLASH support
> >
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagn...@jcrosoft.com>
> ...
> ...
> > diff --git a/board/afeb9260/afeb9260.c b/board/afeb9260/afeb9260.c
> > index 024db2b..d1606b6 100644
> > --- a/board/afeb9260/afeb9260.c
> > +++ b/board/afeb9260/afeb9260.c
> ...
> > +/* SPI chip select control */
> > +#ifdef CONFIG_ATMEL_SPI
> > +#include <spi.h>
>
> Please no #includes in the middle of the file.
>
> > +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> > +{
> > + return bus == 0 && cs < 2;
> > +}
> > +
> > +void spi_cs_activate(struct spi_slave *slave)
> > +{
> > + switch(slave->cs) {
> > + case 1:
> > + at91_set_gpio_value(AT91_PIN_PC11, 0);
> > + break;
> > + case 0:
> > + default:
> > + at91_set_gpio_value(AT91_PIN_PA3, 0);
> > + break;
> > + }
> > +}
> > +
> > +void spi_cs_deactivate(struct spi_slave *slave)
> > +{
> > + switch(slave->cs) {
> > + case 1:
> > + at91_set_gpio_value(AT91_PIN_PC11, 1);
> > + break;
> > + case 0:
> > + default:
> > + at91_set_gpio_value(AT91_PIN_PA3, 1);
> > + break;
> > + }
> > +}
>
> Are these really board specific, or is it likely that we will see the
> same code for other boards as well?
yes these code are board specific because you can use the cs you want
>
> > diff --git a/board/atmel/at91cap9adk/at91cap9adk.c
> > b/board/atmel/at91cap9adk/at91cap9adk.c
> > index f52edaa..1e99bba 100644
> > --- a/board/atmel/at91cap9adk/at91cap9adk.c
> > +++ b/board/atmel/at91cap9adk/at91cap9adk.c
> ...
> > +/* SPI chip select control */
> > +#ifdef CONFIG_ATMEL_SPI
> > +#include <spi.h>
>
> See above.
>
> > +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> > +{
> > + return bus == 0 && cs == 0;
> > +}
>
> Duplicated code. Move to common.
It's board specific
>
> > +void spi_cs_activate(struct spi_slave *slave)
> > +{
> > + at91_set_gpio_value(AT91_PIN_PA5, 0);
> > +}
> > +
> > +void spi_cs_deactivate(struct spi_slave *slave)
> > +{
> > + at91_set_gpio_value(AT91_PIN_PA5, 1);
> > +}
>
> How about making these inline?
it's the spi framework
>
> > diff --git a/board/atmel/at91sam9260ek/at91sam9260ek.c
> > b/board/atmel/at91sam9260ek/at91sam9260ek.c
> > index 6bd3b44..c4eaafd 100644
> > --- a/board/atmel/at91sam9260ek/at91sam9260ek.c
> > +++ b/board/atmel/at91sam9260ek/at91sam9260ek.c
> ...
> > index fbc82d1..e73f450 100644
> > --- a/cpu/arm926ejs/at91/Makefile
> > +++ b/cpu/arm926ejs/at91/Makefile
> > @@ -28,30 +28,36 @@ LIB = $(obj)lib$(SOC).a
> > ifdef CONFIG_AT91CAP9
> > COBJS-$(CONFIG_MACB) += at91cap9_macb.o
> > COBJS-y += at91cap9_serial.o
> > +COBJS-$(CONFIG_ATMEL_SPI) += at91cap9_spi.o
> > COBJS-$(CONFIG_HAS_DATAFLASH) += at91cap9_spi.o
> > endif
> > ifdef CONFIG_AT91SAM9260
> > COBJS-$(CONFIG_MACB) += at91sam9260_macb.o
> > COBJS-y += at91sam9260_serial.o
> > +COBJS-$(CONFIG_ATMEL_SPI) += at91sam9260_spi.o
> > COBJS-$(CONFIG_HAS_DATAFLASH) += at91sam9260_spi.o
> > endif
> > ifdef CONFIG_AT91SAM9G20
> > COBJS-$(CONFIG_MACB) += at91sam9260_macb.o
> > COBJS-y += at91sam9260_serial.o
> > +COBJS-$(CONFIG_ATMEL_SPI) += at91sam9260_spi.o
> > COBJS-$(CONFIG_HAS_DATAFLASH) += at91sam9260_spi.o
> > endif
> > ifdef CONFIG_AT91SAM9261
> > COBJS-y += at91sam9261_serial.o
> > +COBJS-$(CONFIG_ATMEL_SPI) += at91sam9261_spi.o
> > COBJS-$(CONFIG_HAS_DATAFLASH) += at91sam9261_spi.o
> > endif
> > ifdef CONFIG_AT91SAM9263
> > COBJS-$(CONFIG_MACB) += at91sam9263_macb.o
> > COBJS-y += at91sam9263_serial.o
> > +COBJS-$(CONFIG_ATMEL_SPI) += at91sam9263_spi.o
> > COBJS-$(CONFIG_HAS_DATAFLASH) += at91sam9263_spi.o
> > COBJS-$(CONFIG_USB_OHCI_NEW) += at91sam9263_usb.o
> > endif
> > ifdef CONFIG_AT91SAM9RL
> > COBJS-y += at91sam9rl_serial.o
> > +COBJS-$(CONFIG_ATMEL_SPI) += at91sam9rl_spi.o
> > COBJS-$(CONFIG_HAS_DATAFLASH) += at91sam9rl_spi.o
> > endif
> > COBJS-$(CONFIG_AT91_LED) += led.o
>
> This needs to be cleand up as well. Define some variable for the board
> name (don't we actually have one already?), and then use a single copy
> of
it's not board name it's cpu name
and due to multiple soc support for one file we can not do this
>
> COBJS-$(CONFIG_MACB) += $(BOARD)_macb.o
> COBJS-y += $(BOARD)_serial.o
> COBJS-$(CONFIG_ATMEL_SPI) += $(BOARD)_spi.o
> COBJS-$(CONFIG_HAS_DATAFLASH) += $(BOARD)_spi.o
>
> instead of such an endless list if "ifdef"ed code.
>
> > diff --git a/include/asm-arm/arch-at91/hardware.h
> > b/include/asm-arm/arch-at91/hardware.h
> > index 8704106..0403b09 100644
> > --- a/include/asm-arm/arch-at91/hardware.h
> > +++ b/include/asm-arm/arch-at91/hardware.h
> > @@ -21,25 +21,34 @@
> > #elif defined(CONFIG_AT91SAM9260) || defined(CONFIG_AT91SAM9G20)
> > #include <asm/arch/at91sam9260.h>
> > #define AT91_BASE_SPI AT91SAM9260_BASE_SPI0
> > +#define SPI0_BASE AT91SAM9260_BASE_SPI0
> > +#define SPI1_BASE AT91SAM9260_BASE_SPI1
> > #define AT91_ID_UHP AT91SAM9260_ID_UHP
> > #define AT91_PMC_UHP AT91SAM926x_PMC_UHP
> > #elif defined(CONFIG_AT91SAM9261)
> > #include <asm/arch/at91sam9261.h>
> > #define AT91_BASE_SPI AT91SAM9261_BASE_SPI0
> > +#define SPI0_BASE AT91SAM9261_BASE_SPI0
> > +#define SPI1_BASE AT91SAM9261_BASE_SPI1
> > #define AT91_ID_UHP AT91SAM9261_ID_UHP
> > #define AT91_PMC_UHP AT91SAM926x_PMC_UHP
> > #elif defined(CONFIG_AT91SAM9263)
> > #include <asm/arch/at91sam9263.h>
> > #define AT91_BASE_SPI AT91SAM9263_BASE_SPI0
> > +#define SPI0_BASE AT91SAM9263_BASE_SPI0
> > +#define SPI1_BASE AT91SAM9263_BASE_SPI1
> > #define AT91_ID_UHP AT91SAM9263_ID_UHP
> > #define AT91_PMC_UHP AT91SAM926x_PMC_UHP
> > #elif defined(CONFIG_AT91SAM9RL)
> > #include <asm/arch/at91sam9rl.h>
> > #define AT91_BASE_SPI AT91SAM9RL_BASE_SPI
> > +#define SPI0_BASE AT91SAM9RL_BASE_SPI
> > #define AT91_ID_UHP AT91SAM9RL_ID_UHP
> > #elif defined(CONFIG_AT91CAP9)
> > #include <asm/arch/at91cap9.h>
> > #define AT91_BASE_SPI AT91CAP9_BASE_SPI0
> > +#define SPI0_BASE AT91CAP9_BASE_SPI0
> > +#define SPI1_BASE AT91CAP9_BASE_SPI1
> > #define AT91_ID_UHP AT91CAP9_ID_UHP
> > #define AT91_PMC_UHP AT91CAP9_PMC_UHP
> > #elif defined(CONFIG_AT91X40)
>
> Same here. With a little clever use of macros we probably can get rid
> of all these "if defined" stuff.
we can not because it's shared with diffent soc and with AVR32
>
> > diff --git a/include/configs/afeb9260.h b/include/configs/afeb9260.h
> > index 89b16fe..a897c62 100644
> > --- a/include/configs/afeb9260.h
> > +++ b/include/configs/afeb9260.h
> > @@ -84,6 +84,12 @@
> > #define PHYS_SDRAM_SIZE 0x04000000 /* 64 megs */
> >
> > /* DataFlash */
> > +#ifdef CONFIG_ATMEL_SPI
> > +#define CONFIG_CMD_SF
> > +#define CONFIG_CMD_SPI
> > +#define CONFIG_SPI_FLASH 1
> > +#define CONFIG_SPI_FLASH_ATMEL 1
> > +#else
>
> In which way is this board specific configuration when you copy it to
> all (?) AT91 boards?
you can enalbe what you want as flash I can not force at91 boards evenif
actually they use the same
It's be more generic when we will have Kconfig
Best Regards,
J.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
