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

Wolfgang Denk wd at denx.de
Sat Apr 4 21:52:06 CEST 2009


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?

> 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.

> +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?

> 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
...
> +/* 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 < 2;
> +}

Duplicated code. Move to common.


> +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;
> +	}
> +}

Duplicated code. Move to common.

> +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;
> +	}
> +}

Duplicated code. Move to common.

> diff --git a/board/atmel/at91sam9261ek/at91sam9261ek.c b/board/atmel/at91sam9261ek/at91sam9261ek.c
> index a89cb8b..9d31324 100644
> --- a/board/atmel/at91sam9261ek/at91sam9261ek.c
> +++ b/board/atmel/at91sam9261ek/at91sam9261ek.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 || cs == 3);
> +}
> +
> +void spi_cs_activate(struct spi_slave *slave)
> +{
> +	switch(slave->cs) {
> +	case 3:
> +		at91_set_gpio_value(AT91_PIN_PA6, 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 3:
> +		at91_set_gpio_value(AT91_PIN_PA6, 1);
> +		break;
> +	case 0:
> +	default:
> +		at91_set_gpio_value(AT91_PIN_PA3, 1);
> +		break;
> +	}
> +}

This is again nearly the same code as before.

May be it can be generalized with some parameters so we don;t need to
duplicate it again and again?

> diff --git a/board/atmel/at91sam9263ek/at91sam9263ek.c b/board/atmel/at91sam9263ek/at91sam9263ek.c
> index 57d5c95..e9c73af 100644
> --- a/board/atmel/at91sam9263ek/at91sam9263ek.c
> +++ b/board/atmel/at91sam9263ek/at91sam9263ek.c
...
> +/* SPI chip select control */
> +#ifdef CONFIG_ATMEL_SPI
> +#include <spi.h>
> +
> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> +{
> +	return bus == 0 && cs == 0;
> +}
> +
> +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);
> +}

Gain: all repeated code.

> +#endif /* CONFIG_ATMEL_SPI */
> diff --git a/board/atmel/at91sam9rlek/at91sam9rlek.c b/board/atmel/at91sam9rlek/at91sam9rlek.c
> index 7013ba2..35adfcd 100644
> --- a/board/atmel/at91sam9rlek/at91sam9rlek.c
> +++ b/board/atmel/at91sam9rlek/at91sam9rlek.c
...
> +
> +/* SPI chip select control */
> +#ifdef CONFIG_ATMEL_SPI
> +#include <spi.h>
> +
> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> +{
> +	return bus == 0 && cs == 0;
> +}
> +
> +void spi_cs_activate(struct spi_slave *slave)
> +{
> +	at91_set_gpio_value(AT91_PIN_PA28, 0);
> +}
> +
> +void spi_cs_deactivate(struct spi_slave *slave)
> +{
> +	at91_set_gpio_value(AT91_PIN_PA28, 1);
> +}

And again.

> diff --git a/board/ronetix/pm9263/pm9263.c b/board/ronetix/pm9263/pm9263.c
> index 801b268..c16bea0 100644
> --- a/board/ronetix/pm9263/pm9263.c
> +++ b/board/ronetix/pm9263/pm9263.c
...
> +/* SPI chip select control */
> +#ifdef CONFIG_ATMEL_SPI
> +#include <spi.h>
> +
> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> +{
> +	return bus == 0 && cs == 0;
> +}
> +
> +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);
> +}

And again.

Sorry, I'm not going to accept this.


> diff --git a/cpu/arm926ejs/at91/Makefile b/cpu/arm926ejs/at91/Makefile
> 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

	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.

> 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?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Harrison's Postulate:
	For every action, there is an equal and opposite criticism.


More information about the U-Boot mailing list