[U-Boot] [PATCH] at91sam9/at91cap: add new spi framework and spi flash support

Jean-Christophe PLAGNIOL-VILLARD plagnioj at jcrosoft.com
Sat Apr 4 21:48:42 CEST 2009


On 21:52 Sat 04 Apr     , Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
> 
> In message <1238840105-19029-1-git-send-email-plagnioj at 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 <plagnioj at 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.


More information about the U-Boot mailing list