[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