diff mbox

[U-Boot,2/3] sf: Shrink spi_slave {}

Message ID eff03829-cf09-4fac-a68d-9c1d3bbcd028@CO9EHSMHS025.ehs.local
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Commit Message

Jagannadha Sutradharudu Teki Jan. 17, 2014, 2:41 p.m. UTC
Combined spi flash stuff as minimum as possible.

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
Cc: Marek Vasut <marex@denx.de>
---
 drivers/mtd/spi/sf.c       |  4 ++--
 drivers/mtd/spi/sf_ops.c   | 18 +++++++++---------
 drivers/mtd/spi/sf_probe.c | 19 ++++++++++++-------
 include/spi.h              | 45 +++++++++++++++++++--------------------------
 include/spi_flash.h        |  6 +++---
 5 files changed, 45 insertions(+), 47 deletions(-)

Comments

Marek Vasut Jan. 17, 2014, 4:31 p.m. UTC | #1
On Friday, January 17, 2014 at 03:41:46 PM, Jagannadha Sutradharudu Teki wrote:
> Combined spi flash stuff as minimum as possible.

Squashing the names of the flags only makes it unreadable :-(

> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
>  drivers/mtd/spi/sf.c       |  4 ++--
>  drivers/mtd/spi/sf_ops.c   | 18 +++++++++---------
>  drivers/mtd/spi/sf_probe.c | 19 ++++++++++++-------
>  include/spi.h              | 45
> +++++++++++++++++++-------------------------- include/spi_flash.h        |
>  6 +++---
>  5 files changed, 45 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/mtd/spi/sf.c b/drivers/mtd/spi/sf.c
> index 664e860..fb91ac6 100644
> --- a/drivers/mtd/spi/sf.c
> +++ b/drivers/mtd/spi/sf.c
> @@ -19,8 +19,8 @@ static int spi_flash_read_write(struct spi_slave *spi,
>  	int ret;
> 
>  #ifdef CONFIG_SF_DUAL_FLASH
> -	if (spi->flags & SPI_XFER_U_PAGE)
> -		flags |= SPI_XFER_U_PAGE;
> +	if (spi->mode_bits & SPI_U_PAGE)
> +		flags |= SPI_U_PAGE;
>  #endif
>  	if (data_len == 0)
>  		flags |= SPI_XFER_END;
> diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
> index 9650486..0d87e1b 100644
> --- a/drivers/mtd/spi/sf_ops.c
> +++ b/drivers/mtd/spi/sf_ops.c
> @@ -135,15 +135,15 @@ static int spi_flash_bank(struct spi_flash *flash,
> u32 offset) static void spi_flash_dual_flash(struct spi_flash *flash, u32
> *addr) {
>  	switch (flash->dual_flash) {
> -	case SF_DUAL_STACKED_FLASH:
> +	case SF_STACKED:
>  		if (*addr >= (flash->size >> 1)) {
>  			*addr -= flash->size >> 1;
> -			flash->spi->flags |= SPI_XFER_U_PAGE;
> +			flash->spi->mode_bits |= SPI_U_PAGE;
>  		} else {
> -			flash->spi->flags &= ~SPI_XFER_U_PAGE;
> +			flash->spi->mode_bits &= ~SPI_U_PAGE;
>  		}
>  		break;
> -	case SF_DUAL_PARALLEL_FLASH:
> +	case SF_PARALLEL:
>  		*addr >>= flash->shift;
>  		break;
>  	default:
> @@ -170,8 +170,8 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash,
> unsigned long timeout) }
> 
>  #ifdef CONFIG_SF_DUAL_FLASH
> -	if (spi->flags & SPI_XFER_U_PAGE)
> -		flags |= SPI_XFER_U_PAGE;
> +	if (spi->mode_bits & SPI_U_PAGE)
> +		flags |= SPI_U_PAGE;
>  #endif
>  	ret = spi_xfer(spi, 8, &cmd, NULL, flags);
>  	if (ret) {
> @@ -261,7 +261,7 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash,
> u32 offset, size_t len) erase_addr = offset;
> 
>  #ifdef CONFIG_SF_DUAL_FLASH
> -		if (flash->dual_flash > SF_SINGLE_FLASH)
> +		if (flash->dual_flash > SF_SINGLE)
>  			spi_flash_dual_flash(flash, &erase_addr);
>  #endif
>  #ifdef CONFIG_SPI_FLASH_BAR
> @@ -303,7 +303,7 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash,
> u32 offset, write_addr = offset;
> 
>  #ifdef CONFIG_SF_DUAL_FLASH
> -		if (flash->dual_flash > SF_SINGLE_FLASH)
> +		if (flash->dual_flash > SF_SINGLE)
>  			spi_flash_dual_flash(flash, &write_addr);
>  #endif
>  #ifdef CONFIG_SPI_FLASH_BAR
> @@ -388,7 +388,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32
> offset, read_addr = offset;
> 
>  #ifdef CONFIG_SF_DUAL_FLASH
> -		if (flash->dual_flash > SF_SINGLE_FLASH)
> +		if (flash->dual_flash > SF_SINGLE)
>  			spi_flash_dual_flash(flash, &read_addr);
>  #endif
>  #ifdef CONFIG_SPI_FLASH_BAR
> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> index e84ab13..003f635 100644
> --- a/drivers/mtd/spi/sf_probe.c
> +++ b/drivers/mtd/spi/sf_probe.c
> @@ -133,8 +133,13 @@ static struct spi_flash
> *spi_flash_validate_params(struct spi_slave *spi, flash->spi = spi;
>  	flash->name = params->name;
>  	flash->memory_map = spi->memory_map;
> -	flash->dual_flash = flash->spi->option;
> 
> +#ifdef CONFIG_SF_DUAL_FLASH

Looks new and unrelated.

> +	if (flash->spi->mode_bits & SPI_SHARED)
> +		flash->dual_flash = SF_STACKED;
> +	else if (flash->spi->mode_bits & SPI_SEPARATED)
> +		flash->dual_flash = SF_PARALLEL;
> +#endif
>  	/* Assign spi_flash ops */
>  	flash->write = spi_flash_cmd_write_ops;
>  #ifdef CONFIG_SPI_FLASH_SST
> @@ -145,12 +150,12 @@ static struct spi_flash
> *spi_flash_validate_params(struct spi_slave *spi, flash->read =
> spi_flash_cmd_read_ops;
> 
>  	/* Compute the flash size */
> -	flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 : 0;
> +	flash->shift = (flash->dual_flash & SF_PARALLEL) ? 1 : 0;
>  	flash->page_size = ((ext_jedec == 0x4d00) ? 512 : 256) << flash->shift;
>  	flash->sector_size = params->sector_size << flash->shift;
>  	flash->size = flash->sector_size * params->nr_sectors << flash->shift;
>  #ifdef CONFIG_SF_DUAL_FLASH
> -	if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
> +	if (flash->dual_flash & SF_STACKED)
>  		flash->size <<= 1;
>  #endif
> 
> @@ -167,7 +172,7 @@ static struct spi_flash
> *spi_flash_validate_params(struct spi_slave *spi, }
> 
>  	/* Look for the fastest read cmd */
> -	cmd = fls(params->e_rd_cmd & flash->spi->op_mode_rx);
> +	cmd = fls(params->e_rd_cmd & flash->spi->rx_mode);
>  	if (cmd) {
>  		cmd = spi_read_cmds_array[cmd - 1];
>  		flash->read_cmd = cmd;
> @@ -177,7 +182,7 @@ static struct spi_flash
> *spi_flash_validate_params(struct spi_slave *spi, }
> 
>  	/* Not require to look for fastest only two write cmds yet */
> -	if (params->flags & WR_QPP && flash->spi->op_mode_tx & SPI_OPM_TX_QPP)
> +	if (params->flags & WR_QPP && flash->spi->mode_bits & SPI_TX_QPP)
>  		flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
>  	else
>  		/* Go for default supported write cmd */
> @@ -329,9 +334,9 @@ static struct spi_flash *spi_flash_probe_slave(struct
> spi_slave *spi) puts("\n");
>  #endif
>  #ifndef CONFIG_SPI_FLASH_BAR
> -	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
> +	if (((flash->dual_flash == SF_SINGLE) &&
>  	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
> -	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
> +	     ((flash->dual_flash > SF_SINGLE) &&
>  	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
>  		puts("SF: Warning - Only lower 16MiB accessible,");
>  		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
> diff --git a/include/spi.h b/include/spi.h
> index ffd6647..07ba4d0 100644
> --- a/include/spi.h
> +++ b/include/spi.h
> @@ -30,29 +30,25 @@
>  #define SPI_XFER_MMAP		0x08	/* Memory Mapped start */
>  #define SPI_XFER_MMAP_END	0x10	/* Memory Mapped End */
>  #define SPI_XFER_ONCE		(SPI_XFER_BEGIN | SPI_XFER_END)
> -#define SPI_XFER_U_PAGE		(1 << 5)
> -
> -/* SPI TX operation modes */
> -#define SPI_OPM_TX_QPP		1 << 0
> -
> -/* SPI RX operation modes */
> -#define SPI_OPM_RX_AS		1 << 0
> -#define SPI_OPM_RX_DOUT		1 << 1
> -#define SPI_OPM_RX_DIO		1 << 2
> -#define SPI_OPM_RX_QOF		1 << 3
> -#define SPI_OPM_RX_QIOF		1 << 4
> -#define SPI_OPM_RX_EXTN		SPI_OPM_RX_AS | SPI_OPM_RX_DOUT | \
> -				SPI_OPM_RX_DIO | SPI_OPM_RX_QOF | \
> -				SPI_OPM_RX_QIOF
> -
> -/* SPI bus connection options */
> -#define SPI_CONN_DUAL_SHARED	1 << 0
> -#define SPI_CONN_DUAL_SEPARATED	1 << 1
> 
>  /* Header byte that marks the start of the message */
>  #define SPI_PREAMBLE_END_BYTE	0xec
> +#define SPI_DEFAULT_WORDLEN	8
> +
> +/* SPI RX operation modes */
> +#define SPI_RX_AS	1 << 0
> +#define SPI_RX_DOUT	1 << 1
> +#define SPI_RX_DIO	1 << 2
> +#define SPI_RX_QOF	1 << 3
> +#define SPI_RX_QIOF	1 << 4
> +#define SPI_RX_EXTN	SPI_RX_AS | SPI_RX_DOUT | SPI_RX_DIO | \
> +			SPI_RX_QOF | SPI_RX_QIOF
> 
> -#define SPI_DEFAULT_WORDLEN 8
> +/* SPI mode bits */
> +#define SPI_TX_QPP	1 << 0
> +#define SPI_SHARED	1 << 1
> +#define SPI_SEPARATED	1 << 2
> +#define SPI_U_PAGE	1 << 3
> 
>  /**
>   * struct spi_slave - Representation of a SPI slave
> @@ -61,25 +57,22 @@
>   *
>   * @bus:		ID of the bus that the slave is attached to.
>   * @cs:			ID of the chip select connected to the slave.
> - * @op_mode_rx:		SPI RX operation mode.
> - * @op_mode_tx:		SPI TX operation mode.
>   * @wordlen:		Size of SPI word in number of bits
>   * @max_write_size:	If non-zero, the maximum number of bytes which can
>   *			be written at once, excluding command bytes.
>   * @memory_map:		Address of read-only SPI flash access.
> - * @option:		Varies SPI bus options - separate, shared bus.
> + * @rx_mode:		SPI RX operation mode.
> + * @mode_bits:		SPI TX operation modes, bus options and few 
flags.

This naming change doesn't make sense. rx_mode vs. mode_bits don't have any 
relationship even if they are related apparently.

Anyway, I feel we're sinking deeper and deeper into this sh*t, we should instead 
take a step back and re-think the whole approach until we break it even more.

[...]
Jagan Teki Jan. 17, 2014, 5:12 p.m. UTC | #2
On Fri, Jan 17, 2014 at 10:01 PM, Marek Vasut <marex@denx.de> wrote:
> On Friday, January 17, 2014 at 03:41:46 PM, Jagannadha Sutradharudu Teki wrote:
>> Combined spi flash stuff as minimum as possible.
>
> Squashing the names of the flags only makes it unreadable :-(
>
>> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
>> Cc: Marek Vasut <marex@denx.de>
>> ---
>>  drivers/mtd/spi/sf.c       |  4 ++--
>>  drivers/mtd/spi/sf_ops.c   | 18 +++++++++---------
>>  drivers/mtd/spi/sf_probe.c | 19 ++++++++++++-------
>>  include/spi.h              | 45
>> +++++++++++++++++++-------------------------- include/spi_flash.h        |
>>  6 +++---
>>  5 files changed, 45 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/sf.c b/drivers/mtd/spi/sf.c
>> index 664e860..fb91ac6 100644
>> --- a/drivers/mtd/spi/sf.c
>> +++ b/drivers/mtd/spi/sf.c
>> @@ -19,8 +19,8 @@ static int spi_flash_read_write(struct spi_slave *spi,
>>       int ret;
>>
>>  #ifdef CONFIG_SF_DUAL_FLASH
>> -     if (spi->flags & SPI_XFER_U_PAGE)
>> -             flags |= SPI_XFER_U_PAGE;
>> +     if (spi->mode_bits & SPI_U_PAGE)
>> +             flags |= SPI_U_PAGE;
>>  #endif
>>       if (data_len == 0)
>>               flags |= SPI_XFER_END;
>> diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
>> index 9650486..0d87e1b 100644
>> --- a/drivers/mtd/spi/sf_ops.c
>> +++ b/drivers/mtd/spi/sf_ops.c
>> @@ -135,15 +135,15 @@ static int spi_flash_bank(struct spi_flash *flash,
>> u32 offset) static void spi_flash_dual_flash(struct spi_flash *flash, u32
>> *addr) {
>>       switch (flash->dual_flash) {
>> -     case SF_DUAL_STACKED_FLASH:
>> +     case SF_STACKED:
>>               if (*addr >= (flash->size >> 1)) {
>>                       *addr -= flash->size >> 1;
>> -                     flash->spi->flags |= SPI_XFER_U_PAGE;
>> +                     flash->spi->mode_bits |= SPI_U_PAGE;
>>               } else {
>> -                     flash->spi->flags &= ~SPI_XFER_U_PAGE;
>> +                     flash->spi->mode_bits &= ~SPI_U_PAGE;
>>               }
>>               break;
>> -     case SF_DUAL_PARALLEL_FLASH:
>> +     case SF_PARALLEL:
>>               *addr >>= flash->shift;
>>               break;
>>       default:
>> @@ -170,8 +170,8 @@ int spi_flash_cmd_wait_ready(struct spi_flash *flash,
>> unsigned long timeout) }
>>
>>  #ifdef CONFIG_SF_DUAL_FLASH
>> -     if (spi->flags & SPI_XFER_U_PAGE)
>> -             flags |= SPI_XFER_U_PAGE;
>> +     if (spi->mode_bits & SPI_U_PAGE)
>> +             flags |= SPI_U_PAGE;
>>  #endif
>>       ret = spi_xfer(spi, 8, &cmd, NULL, flags);
>>       if (ret) {
>> @@ -261,7 +261,7 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash,
>> u32 offset, size_t len) erase_addr = offset;
>>
>>  #ifdef CONFIG_SF_DUAL_FLASH
>> -             if (flash->dual_flash > SF_SINGLE_FLASH)
>> +             if (flash->dual_flash > SF_SINGLE)
>>                       spi_flash_dual_flash(flash, &erase_addr);
>>  #endif
>>  #ifdef CONFIG_SPI_FLASH_BAR
>> @@ -303,7 +303,7 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash,
>> u32 offset, write_addr = offset;
>>
>>  #ifdef CONFIG_SF_DUAL_FLASH
>> -             if (flash->dual_flash > SF_SINGLE_FLASH)
>> +             if (flash->dual_flash > SF_SINGLE)
>>                       spi_flash_dual_flash(flash, &write_addr);
>>  #endif
>>  #ifdef CONFIG_SPI_FLASH_BAR
>> @@ -388,7 +388,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32
>> offset, read_addr = offset;
>>
>>  #ifdef CONFIG_SF_DUAL_FLASH
>> -             if (flash->dual_flash > SF_SINGLE_FLASH)
>> +             if (flash->dual_flash > SF_SINGLE)
>>                       spi_flash_dual_flash(flash, &read_addr);
>>  #endif
>>  #ifdef CONFIG_SPI_FLASH_BAR
>> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
>> index e84ab13..003f635 100644
>> --- a/drivers/mtd/spi/sf_probe.c
>> +++ b/drivers/mtd/spi/sf_probe.c
>> @@ -133,8 +133,13 @@ static struct spi_flash
>> *spi_flash_validate_params(struct spi_slave *spi, flash->spi = spi;
>>       flash->name = params->name;
>>       flash->memory_map = spi->memory_map;
>> -     flash->dual_flash = flash->spi->option;
>>
>> +#ifdef CONFIG_SF_DUAL_FLASH
>
> Looks new and unrelated.
>
>> +     if (flash->spi->mode_bits & SPI_SHARED)
>> +             flash->dual_flash = SF_STACKED;
>> +     else if (flash->spi->mode_bits & SPI_SEPARATED)
>> +             flash->dual_flash = SF_PARALLEL;
>> +#endif
>>       /* Assign spi_flash ops */
>>       flash->write = spi_flash_cmd_write_ops;
>>  #ifdef CONFIG_SPI_FLASH_SST
>> @@ -145,12 +150,12 @@ static struct spi_flash
>> *spi_flash_validate_params(struct spi_slave *spi, flash->read =
>> spi_flash_cmd_read_ops;
>>
>>       /* Compute the flash size */
>> -     flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 : 0;
>> +     flash->shift = (flash->dual_flash & SF_PARALLEL) ? 1 : 0;
>>       flash->page_size = ((ext_jedec == 0x4d00) ? 512 : 256) << flash->shift;
>>       flash->sector_size = params->sector_size << flash->shift;
>>       flash->size = flash->sector_size * params->nr_sectors << flash->shift;
>>  #ifdef CONFIG_SF_DUAL_FLASH
>> -     if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
>> +     if (flash->dual_flash & SF_STACKED)
>>               flash->size <<= 1;
>>  #endif
>>
>> @@ -167,7 +172,7 @@ static struct spi_flash
>> *spi_flash_validate_params(struct spi_slave *spi, }
>>
>>       /* Look for the fastest read cmd */
>> -     cmd = fls(params->e_rd_cmd & flash->spi->op_mode_rx);
>> +     cmd = fls(params->e_rd_cmd & flash->spi->rx_mode);
>>       if (cmd) {
>>               cmd = spi_read_cmds_array[cmd - 1];
>>               flash->read_cmd = cmd;
>> @@ -177,7 +182,7 @@ static struct spi_flash
>> *spi_flash_validate_params(struct spi_slave *spi, }
>>
>>       /* Not require to look for fastest only two write cmds yet */
>> -     if (params->flags & WR_QPP && flash->spi->op_mode_tx & SPI_OPM_TX_QPP)
>> +     if (params->flags & WR_QPP && flash->spi->mode_bits & SPI_TX_QPP)
>>               flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
>>       else
>>               /* Go for default supported write cmd */
>> @@ -329,9 +334,9 @@ static struct spi_flash *spi_flash_probe_slave(struct
>> spi_slave *spi) puts("\n");
>>  #endif
>>  #ifndef CONFIG_SPI_FLASH_BAR
>> -     if (((flash->dual_flash == SF_SINGLE_FLASH) &&
>> +     if (((flash->dual_flash == SF_SINGLE) &&
>>            (flash->size > SPI_FLASH_16MB_BOUN)) ||
>> -          ((flash->dual_flash > SF_SINGLE_FLASH) &&
>> +          ((flash->dual_flash > SF_SINGLE) &&
>>            (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
>>               puts("SF: Warning - Only lower 16MiB accessible,");
>>               puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
>> diff --git a/include/spi.h b/include/spi.h
>> index ffd6647..07ba4d0 100644
>> --- a/include/spi.h
>> +++ b/include/spi.h
>> @@ -30,29 +30,25 @@
>>  #define SPI_XFER_MMAP                0x08    /* Memory Mapped start */
>>  #define SPI_XFER_MMAP_END    0x10    /* Memory Mapped End */
>>  #define SPI_XFER_ONCE                (SPI_XFER_BEGIN | SPI_XFER_END)
>> -#define SPI_XFER_U_PAGE              (1 << 5)
>> -
>> -/* SPI TX operation modes */
>> -#define SPI_OPM_TX_QPP               1 << 0
>> -
>> -/* SPI RX operation modes */
>> -#define SPI_OPM_RX_AS                1 << 0
>> -#define SPI_OPM_RX_DOUT              1 << 1
>> -#define SPI_OPM_RX_DIO               1 << 2
>> -#define SPI_OPM_RX_QOF               1 << 3
>> -#define SPI_OPM_RX_QIOF              1 << 4
>> -#define SPI_OPM_RX_EXTN              SPI_OPM_RX_AS | SPI_OPM_RX_DOUT | \
>> -                             SPI_OPM_RX_DIO | SPI_OPM_RX_QOF | \
>> -                             SPI_OPM_RX_QIOF
>> -
>> -/* SPI bus connection options */
>> -#define SPI_CONN_DUAL_SHARED 1 << 0
>> -#define SPI_CONN_DUAL_SEPARATED      1 << 1
>>
>>  /* Header byte that marks the start of the message */
>>  #define SPI_PREAMBLE_END_BYTE        0xec
>> +#define SPI_DEFAULT_WORDLEN  8
>> +
>> +/* SPI RX operation modes */
>> +#define SPI_RX_AS    1 << 0
>> +#define SPI_RX_DOUT  1 << 1
>> +#define SPI_RX_DIO   1 << 2
>> +#define SPI_RX_QOF   1 << 3
>> +#define SPI_RX_QIOF  1 << 4
>> +#define SPI_RX_EXTN  SPI_RX_AS | SPI_RX_DOUT | SPI_RX_DIO | \
>> +                     SPI_RX_QOF | SPI_RX_QIOF
>>
>> -#define SPI_DEFAULT_WORDLEN 8
>> +/* SPI mode bits */
>> +#define SPI_TX_QPP   1 << 0
>> +#define SPI_SHARED   1 << 1
>> +#define SPI_SEPARATED        1 << 2
>> +#define SPI_U_PAGE   1 << 3
>>
>>  /**
>>   * struct spi_slave - Representation of a SPI slave
>> @@ -61,25 +57,22 @@
>>   *
>>   * @bus:             ID of the bus that the slave is attached to.
>>   * @cs:                      ID of the chip select connected to the slave.
>> - * @op_mode_rx:              SPI RX operation mode.
>> - * @op_mode_tx:              SPI TX operation mode.
>>   * @wordlen:         Size of SPI word in number of bits
>>   * @max_write_size:  If non-zero, the maximum number of bytes which can
>>   *                   be written at once, excluding command bytes.
>>   * @memory_map:              Address of read-only SPI flash access.
>> - * @option:          Varies SPI bus options - separate, shared bus.
>> + * @rx_mode:         SPI RX operation mode.
>> + * @mode_bits:               SPI TX operation modes, bus options and few
> flags.
>
> This naming change doesn't make sense. rx_mode vs. mode_bits don't have any
> relationship even if they are related apparently.

rx_mode need to separate here as it involve some calculation to find
fastest command.

>
> Anyway, I feel we're sinking deeper and deeper into this sh*t, we should instead
> take a step back and re-think the whole approach until we break it even more.

Yes - will shrink once we plan for new approach.
But I'm unclear with new SPI-NOR.
Gerhard Sittig Jan. 17, 2014, 6:29 p.m. UTC | #3
On Fri, Jan 17, 2014 at 22:42 +0530, Jagan Teki wrote:
> 
> On Fri, Jan 17, 2014 at 10:01 PM, Marek Vasut <marex@denx.de> wrote:
> >
> > Anyway, I feel we're sinking deeper and deeper into this
> > sh*t, we should instead take a step back and re-think the
> > whole approach until we break it even more.
> 
> Yes - will shrink once we plan for new approach.  But I'm
> unclear with new SPI-NOR.

Regarding this specific patch:  I assume what Marek suggested was
to restrict the "SPI slave" information to what's specific to an
SPI slave.  It's just not true that every SPI slave is a flash
chip (an assumption which QSPI developers appear to fall for
rather easily).

I was about to make a similar comment, that trimming the
identifiers so rigorously leads to code that only "initiated"
people can read.  Even those who want to learn have no chance,
there would not be a legend of any kind (except for the commit
message, which soon is buried and not obvious to look up).  And
even with the legend, it's tedious to train the casual
co-developer to those specific abbreviations, which may not even
be in wide spread use outside of the U-Boot code base.

So I agree with Marek that we should take a deep breath, and be
aware of the consequences before taking a specific direction (and
having a clear direction would also be beneficial).

A more involved answer I will send to the quad SPI thread.


virtually yours
Gerhard Sittig
Marek Vasut Jan. 18, 2014, 4:59 p.m. UTC | #4
On Friday, January 17, 2014 at 06:12:49 PM, Jagan Teki wrote:
> On Fri, Jan 17, 2014 at 10:01 PM, Marek Vasut <marex@denx.de> wrote:
> > On Friday, January 17, 2014 at 03:41:46 PM, Jagannadha Sutradharudu Teki 
wrote:
> >> Combined spi flash stuff as minimum as possible.
> > 
> > Squashing the names of the flags only makes it unreadable :-(
> > 
> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> >> Cc: Marek Vasut <marex@denx.de>
> >> ---
> >> 
> >>  drivers/mtd/spi/sf.c       |  4 ++--
> >>  drivers/mtd/spi/sf_ops.c   | 18 +++++++++---------
> >>  drivers/mtd/spi/sf_probe.c | 19 ++++++++++++-------
> >>  include/spi.h              | 45
> >> 
> >> +++++++++++++++++++-------------------------- include/spi_flash.h       
> >> |
> >> 
> >>  6 +++---
> >>  5 files changed, 45 insertions(+), 47 deletions(-)
> >> 
> >> diff --git a/drivers/mtd/spi/sf.c b/drivers/mtd/spi/sf.c
> >> index 664e860..fb91ac6 100644
> >> --- a/drivers/mtd/spi/sf.c
> >> +++ b/drivers/mtd/spi/sf.c
> >> @@ -19,8 +19,8 @@ static int spi_flash_read_write(struct spi_slave *spi,
> >> 
> >>       int ret;
> >>  
> >>  #ifdef CONFIG_SF_DUAL_FLASH
> >> 
> >> -     if (spi->flags & SPI_XFER_U_PAGE)
> >> -             flags |= SPI_XFER_U_PAGE;
> >> +     if (spi->mode_bits & SPI_U_PAGE)
> >> +             flags |= SPI_U_PAGE;
> >> 
> >>  #endif
> >>  
> >>       if (data_len == 0)
> >>       
> >>               flags |= SPI_XFER_END;
> >> 
> >> diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
> >> index 9650486..0d87e1b 100644
> >> --- a/drivers/mtd/spi/sf_ops.c
> >> +++ b/drivers/mtd/spi/sf_ops.c
> >> @@ -135,15 +135,15 @@ static int spi_flash_bank(struct spi_flash *flash,
> >> u32 offset) static void spi_flash_dual_flash(struct spi_flash *flash,
> >> u32 *addr) {
> >> 
> >>       switch (flash->dual_flash) {
> >> 
> >> -     case SF_DUAL_STACKED_FLASH:
> >> 
> >> +     case SF_STACKED:
> >>               if (*addr >= (flash->size >> 1)) {
> >>               
> >>                       *addr -= flash->size >> 1;
> >> 
> >> -                     flash->spi->flags |= SPI_XFER_U_PAGE;
> >> +                     flash->spi->mode_bits |= SPI_U_PAGE;
> >> 
> >>               } else {
> >> 
> >> -                     flash->spi->flags &= ~SPI_XFER_U_PAGE;
> >> +                     flash->spi->mode_bits &= ~SPI_U_PAGE;
> >> 
> >>               }
> >>               break;
> >> 
> >> -     case SF_DUAL_PARALLEL_FLASH:
> >> 
> >> +     case SF_PARALLEL:
> >>               *addr >>= flash->shift;
> >>               break;
> >>       
> >>       default:
> >> @@ -170,8 +170,8 @@ int spi_flash_cmd_wait_ready(struct spi_flash
> >> *flash, unsigned long timeout) }
> >> 
> >>  #ifdef CONFIG_SF_DUAL_FLASH
> >> 
> >> -     if (spi->flags & SPI_XFER_U_PAGE)
> >> -             flags |= SPI_XFER_U_PAGE;
> >> +     if (spi->mode_bits & SPI_U_PAGE)
> >> +             flags |= SPI_U_PAGE;
> >> 
> >>  #endif
> >>  
> >>       ret = spi_xfer(spi, 8, &cmd, NULL, flags);
> >>       if (ret) {
> >> 
> >> @@ -261,7 +261,7 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash,
> >> u32 offset, size_t len) erase_addr = offset;
> >> 
> >>  #ifdef CONFIG_SF_DUAL_FLASH
> >> 
> >> -             if (flash->dual_flash > SF_SINGLE_FLASH)
> >> +             if (flash->dual_flash > SF_SINGLE)
> >> 
> >>                       spi_flash_dual_flash(flash, &erase_addr);
> >>  
> >>  #endif
> >>  #ifdef CONFIG_SPI_FLASH_BAR
> >> 
> >> @@ -303,7 +303,7 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash,
> >> u32 offset, write_addr = offset;
> >> 
> >>  #ifdef CONFIG_SF_DUAL_FLASH
> >> 
> >> -             if (flash->dual_flash > SF_SINGLE_FLASH)
> >> +             if (flash->dual_flash > SF_SINGLE)
> >> 
> >>                       spi_flash_dual_flash(flash, &write_addr);
> >>  
> >>  #endif
> >>  #ifdef CONFIG_SPI_FLASH_BAR
> >> 
> >> @@ -388,7 +388,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash,
> >> u32 offset, read_addr = offset;
> >> 
> >>  #ifdef CONFIG_SF_DUAL_FLASH
> >> 
> >> -             if (flash->dual_flash > SF_SINGLE_FLASH)
> >> +             if (flash->dual_flash > SF_SINGLE)
> >> 
> >>                       spi_flash_dual_flash(flash, &read_addr);
> >>  
> >>  #endif
> >>  #ifdef CONFIG_SPI_FLASH_BAR
> >> 
> >> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> >> index e84ab13..003f635 100644
> >> --- a/drivers/mtd/spi/sf_probe.c
> >> +++ b/drivers/mtd/spi/sf_probe.c
> >> @@ -133,8 +133,13 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, flash->spi = spi;
> >> 
> >>       flash->name = params->name;
> >>       flash->memory_map = spi->memory_map;
> >> 
> >> -     flash->dual_flash = flash->spi->option;
> >> 
> >> +#ifdef CONFIG_SF_DUAL_FLASH
> > 
> > Looks new and unrelated.
> > 
> >> +     if (flash->spi->mode_bits & SPI_SHARED)
> >> +             flash->dual_flash = SF_STACKED;
> >> +     else if (flash->spi->mode_bits & SPI_SEPARATED)
> >> +             flash->dual_flash = SF_PARALLEL;
> >> +#endif
> >> 
> >>       /* Assign spi_flash ops */
> >>       flash->write = spi_flash_cmd_write_ops;
> >>  
> >>  #ifdef CONFIG_SPI_FLASH_SST
> >> 
> >> @@ -145,12 +150,12 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, flash->read =
> >> spi_flash_cmd_read_ops;
> >> 
> >>       /* Compute the flash size */
> >> 
> >> -     flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 :
> >> 0; +     flash->shift = (flash->dual_flash & SF_PARALLEL) ? 1 : 0;
> >> 
> >>       flash->page_size = ((ext_jedec == 0x4d00) ? 512 : 256) <<
> >>       flash->shift; flash->sector_size = params->sector_size <<
> >>       flash->shift; flash->size = flash->sector_size *
> >>       params->nr_sectors << flash->shift;
> >>  
> >>  #ifdef CONFIG_SF_DUAL_FLASH
> >> 
> >> -     if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
> >> +     if (flash->dual_flash & SF_STACKED)
> >> 
> >>               flash->size <<= 1;
> >>  
> >>  #endif
> >> 
> >> @@ -167,7 +172,7 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, }
> >> 
> >>       /* Look for the fastest read cmd */
> >> 
> >> -     cmd = fls(params->e_rd_cmd & flash->spi->op_mode_rx);
> >> +     cmd = fls(params->e_rd_cmd & flash->spi->rx_mode);
> >> 
> >>       if (cmd) {
> >>       
> >>               cmd = spi_read_cmds_array[cmd - 1];
> >>               flash->read_cmd = cmd;
> >> 
> >> @@ -177,7 +182,7 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, }
> >> 
> >>       /* Not require to look for fastest only two write cmds yet */
> >> 
> >> -     if (params->flags & WR_QPP && flash->spi->op_mode_tx &
> >> SPI_OPM_TX_QPP) +     if (params->flags & WR_QPP &&
> >> flash->spi->mode_bits & SPI_TX_QPP)
> >> 
> >>               flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
> >>       
> >>       else
> >>       
> >>               /* Go for default supported write cmd */
> >> 
> >> @@ -329,9 +334,9 @@ static struct spi_flash
> >> *spi_flash_probe_slave(struct spi_slave *spi) puts("\n");
> >> 
> >>  #endif
> >>  #ifndef CONFIG_SPI_FLASH_BAR
> >> 
> >> -     if (((flash->dual_flash == SF_SINGLE_FLASH) &&
> >> +     if (((flash->dual_flash == SF_SINGLE) &&
> >> 
> >>            (flash->size > SPI_FLASH_16MB_BOUN)) ||
> >> 
> >> -          ((flash->dual_flash > SF_SINGLE_FLASH) &&
> >> +          ((flash->dual_flash > SF_SINGLE) &&
> >> 
> >>            (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
> >>            
> >>               puts("SF: Warning - Only lower 16MiB accessible,");
> >>               puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
> >> 
> >> diff --git a/include/spi.h b/include/spi.h
> >> index ffd6647..07ba4d0 100644
> >> --- a/include/spi.h
> >> +++ b/include/spi.h
> >> @@ -30,29 +30,25 @@
> >> 
> >>  #define SPI_XFER_MMAP                0x08    /* Memory Mapped start */
> >>  #define SPI_XFER_MMAP_END    0x10    /* Memory Mapped End */
> >>  #define SPI_XFER_ONCE                (SPI_XFER_BEGIN | SPI_XFER_END)
> >> 
> >> -#define SPI_XFER_U_PAGE              (1 << 5)
> >> -
> >> -/* SPI TX operation modes */
> >> -#define SPI_OPM_TX_QPP               1 << 0
> >> -
> >> -/* SPI RX operation modes */
> >> -#define SPI_OPM_RX_AS                1 << 0
> >> -#define SPI_OPM_RX_DOUT              1 << 1
> >> -#define SPI_OPM_RX_DIO               1 << 2
> >> -#define SPI_OPM_RX_QOF               1 << 3
> >> -#define SPI_OPM_RX_QIOF              1 << 4
> >> -#define SPI_OPM_RX_EXTN              SPI_OPM_RX_AS | SPI_OPM_RX_DOUT |
> >> \ -                             SPI_OPM_RX_DIO | SPI_OPM_RX_QOF | \ -  
> >>                           SPI_OPM_RX_QIOF
> >> -
> >> -/* SPI bus connection options */
> >> -#define SPI_CONN_DUAL_SHARED 1 << 0
> >> -#define SPI_CONN_DUAL_SEPARATED      1 << 1
> >> 
> >>  /* Header byte that marks the start of the message */
> >>  #define SPI_PREAMBLE_END_BYTE        0xec
> >> 
> >> +#define SPI_DEFAULT_WORDLEN  8
> >> +
> >> +/* SPI RX operation modes */
> >> +#define SPI_RX_AS    1 << 0
> >> +#define SPI_RX_DOUT  1 << 1
> >> +#define SPI_RX_DIO   1 << 2
> >> +#define SPI_RX_QOF   1 << 3
> >> +#define SPI_RX_QIOF  1 << 4
> >> +#define SPI_RX_EXTN  SPI_RX_AS | SPI_RX_DOUT | SPI_RX_DIO | \
> >> +                     SPI_RX_QOF | SPI_RX_QIOF
> >> 
> >> -#define SPI_DEFAULT_WORDLEN 8
> >> +/* SPI mode bits */
> >> +#define SPI_TX_QPP   1 << 0
> >> +#define SPI_SHARED   1 << 1
> >> +#define SPI_SEPARATED        1 << 2
> >> +#define SPI_U_PAGE   1 << 3
> >> 
> >>  /**
> >>  
> >>   * struct spi_slave - Representation of a SPI slave
> >> 
> >> @@ -61,25 +57,22 @@
> >> 
> >>   *
> >>   * @bus:             ID of the bus that the slave is attached to.
> >>   * @cs:                      ID of the chip select connected to the
> >>   slave.
> >> 
> >> - * @op_mode_rx:              SPI RX operation mode.
> >> - * @op_mode_tx:              SPI TX operation mode.
> >> 
> >>   * @wordlen:         Size of SPI word in number of bits
> >>   * @max_write_size:  If non-zero, the maximum number of bytes which can
> >>   *                   be written at once, excluding command bytes.
> >>   * @memory_map:              Address of read-only SPI flash access.
> >> 
> >> - * @option:          Varies SPI bus options - separate, shared bus.
> >> + * @rx_mode:         SPI RX operation mode.
> >> + * @mode_bits:               SPI TX operation modes, bus options and
> >> few
> > 
> > flags.
> > 
> > This naming change doesn't make sense. rx_mode vs. mode_bits don't have
> > any relationship even if they are related apparently.
> 
> rx_mode need to separate here as it involve some calculation to find
> fastest command.

... and this is really starting to become horrible nonsense. Fastest command for 
SPI NOR, right ? But this is struct spi_slave {} describing generic SPI slave 
here, not an SPI NOR!

> > Anyway, I feel we're sinking deeper and deeper into this sh*t, we should
> > instead take a step back and re-think the whole approach until we break
> > it even more.
> 
> Yes - will shrink once we plan for new approach.
> But I'm unclear with new SPI-NOR.

It's pretty clear in it's own right, do not polute generic code with SPI NOR 
specific stuff .
Marek Vasut Jan. 18, 2014, 5:03 p.m. UTC | #5
On Friday, January 17, 2014 at 07:29:54 PM, Gerhard Sittig wrote:
> On Fri, Jan 17, 2014 at 22:42 +0530, Jagan Teki wrote:
> > On Fri, Jan 17, 2014 at 10:01 PM, Marek Vasut <marex@denx.de> wrote:
> > > Anyway, I feel we're sinking deeper and deeper into this
> > > sh*t, we should instead take a step back and re-think the
> > > whole approach until we break it even more.
> > 
> > Yes - will shrink once we plan for new approach.  But I'm
> > unclear with new SPI-NOR.
> 
> Regarding this specific patch:  I assume what Marek suggested was
> to restrict the "SPI slave" information to what's specific to an
> SPI slave.  It's just not true that every SPI slave is a flash
> chip (an assumption which QSPI developers appear to fall for
> rather easily).

Heh, really ? :) Otherwise I agree with you.

btw. I honestly don't quite understand this inclination to building separate SPI 
NOR controller instead of building full-fat SPI bus controller :(

> I was about to make a similar comment, that trimming the
> identifiers so rigorously leads to code that only "initiated"
> people can read.

OK, I have to admit I am rather blunt and my rambling may sound nasty. Please 
don't take it personally ;-)

> Even those who want to learn have no chance,
> there would not be a legend of any kind (except for the commit
> message, which soon is buried and not obvious to look up).  And
> even with the legend, it's tedious to train the casual
> co-developer to those specific abbreviations, which may not even
> be in wide spread use outside of the U-Boot code base.
> 
> So I agree with Marek that we should take a deep breath, and be
> aware of the consequences before taking a specific direction (and
> having a clear direction would also be beneficial).
> 
> A more involved answer I will send to the quad SPI thread.

Thanks for expainding so and please keep me in the loop on the qspi :)

Best regards,
Marek Vasut
diff mbox

Patch

diff --git a/drivers/mtd/spi/sf.c b/drivers/mtd/spi/sf.c
index 664e860..fb91ac6 100644
--- a/drivers/mtd/spi/sf.c
+++ b/drivers/mtd/spi/sf.c
@@ -19,8 +19,8 @@  static int spi_flash_read_write(struct spi_slave *spi,
 	int ret;
 
 #ifdef CONFIG_SF_DUAL_FLASH
-	if (spi->flags & SPI_XFER_U_PAGE)
-		flags |= SPI_XFER_U_PAGE;
+	if (spi->mode_bits & SPI_U_PAGE)
+		flags |= SPI_U_PAGE;
 #endif
 	if (data_len == 0)
 		flags |= SPI_XFER_END;
diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
index 9650486..0d87e1b 100644
--- a/drivers/mtd/spi/sf_ops.c
+++ b/drivers/mtd/spi/sf_ops.c
@@ -135,15 +135,15 @@  static int spi_flash_bank(struct spi_flash *flash, u32 offset)
 static void spi_flash_dual_flash(struct spi_flash *flash, u32 *addr)
 {
 	switch (flash->dual_flash) {
-	case SF_DUAL_STACKED_FLASH:
+	case SF_STACKED:
 		if (*addr >= (flash->size >> 1)) {
 			*addr -= flash->size >> 1;
-			flash->spi->flags |= SPI_XFER_U_PAGE;
+			flash->spi->mode_bits |= SPI_U_PAGE;
 		} else {
-			flash->spi->flags &= ~SPI_XFER_U_PAGE;
+			flash->spi->mode_bits &= ~SPI_U_PAGE;
 		}
 		break;
-	case SF_DUAL_PARALLEL_FLASH:
+	case SF_PARALLEL:
 		*addr >>= flash->shift;
 		break;
 	default:
@@ -170,8 +170,8 @@  int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
 	}
 
 #ifdef CONFIG_SF_DUAL_FLASH
-	if (spi->flags & SPI_XFER_U_PAGE)
-		flags |= SPI_XFER_U_PAGE;
+	if (spi->mode_bits & SPI_U_PAGE)
+		flags |= SPI_U_PAGE;
 #endif
 	ret = spi_xfer(spi, 8, &cmd, NULL, flags);
 	if (ret) {
@@ -261,7 +261,7 @@  int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
 		erase_addr = offset;
 
 #ifdef CONFIG_SF_DUAL_FLASH
-		if (flash->dual_flash > SF_SINGLE_FLASH)
+		if (flash->dual_flash > SF_SINGLE)
 			spi_flash_dual_flash(flash, &erase_addr);
 #endif
 #ifdef CONFIG_SPI_FLASH_BAR
@@ -303,7 +303,7 @@  int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
 		write_addr = offset;
 
 #ifdef CONFIG_SF_DUAL_FLASH
-		if (flash->dual_flash > SF_SINGLE_FLASH)
+		if (flash->dual_flash > SF_SINGLE)
 			spi_flash_dual_flash(flash, &write_addr);
 #endif
 #ifdef CONFIG_SPI_FLASH_BAR
@@ -388,7 +388,7 @@  int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 		read_addr = offset;
 
 #ifdef CONFIG_SF_DUAL_FLASH
-		if (flash->dual_flash > SF_SINGLE_FLASH)
+		if (flash->dual_flash > SF_SINGLE)
 			spi_flash_dual_flash(flash, &read_addr);
 #endif
 #ifdef CONFIG_SPI_FLASH_BAR
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
index e84ab13..003f635 100644
--- a/drivers/mtd/spi/sf_probe.c
+++ b/drivers/mtd/spi/sf_probe.c
@@ -133,8 +133,13 @@  static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	flash->spi = spi;
 	flash->name = params->name;
 	flash->memory_map = spi->memory_map;
-	flash->dual_flash = flash->spi->option;
 
+#ifdef CONFIG_SF_DUAL_FLASH
+	if (flash->spi->mode_bits & SPI_SHARED)
+		flash->dual_flash = SF_STACKED;
+	else if (flash->spi->mode_bits & SPI_SEPARATED)
+		flash->dual_flash = SF_PARALLEL;
+#endif
 	/* Assign spi_flash ops */
 	flash->write = spi_flash_cmd_write_ops;
 #ifdef CONFIG_SPI_FLASH_SST
@@ -145,12 +150,12 @@  static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	flash->read = spi_flash_cmd_read_ops;
 
 	/* Compute the flash size */
-	flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 : 0;
+	flash->shift = (flash->dual_flash & SF_PARALLEL) ? 1 : 0;
 	flash->page_size = ((ext_jedec == 0x4d00) ? 512 : 256) << flash->shift;
 	flash->sector_size = params->sector_size << flash->shift;
 	flash->size = flash->sector_size * params->nr_sectors << flash->shift;
 #ifdef CONFIG_SF_DUAL_FLASH
-	if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
+	if (flash->dual_flash & SF_STACKED)
 		flash->size <<= 1;
 #endif
 
@@ -167,7 +172,7 @@  static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	}
 
 	/* Look for the fastest read cmd */
-	cmd = fls(params->e_rd_cmd & flash->spi->op_mode_rx);
+	cmd = fls(params->e_rd_cmd & flash->spi->rx_mode);
 	if (cmd) {
 		cmd = spi_read_cmds_array[cmd - 1];
 		flash->read_cmd = cmd;
@@ -177,7 +182,7 @@  static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	}
 
 	/* Not require to look for fastest only two write cmds yet */
-	if (params->flags & WR_QPP && flash->spi->op_mode_tx & SPI_OPM_TX_QPP)
+	if (params->flags & WR_QPP && flash->spi->mode_bits & SPI_TX_QPP)
 		flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
 	else
 		/* Go for default supported write cmd */
@@ -329,9 +334,9 @@  static struct spi_flash *spi_flash_probe_slave(struct spi_slave *spi)
 	puts("\n");
 #endif
 #ifndef CONFIG_SPI_FLASH_BAR
-	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
+	if (((flash->dual_flash == SF_SINGLE) &&
 	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
-	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
+	     ((flash->dual_flash > SF_SINGLE) &&
 	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
 		puts("SF: Warning - Only lower 16MiB accessible,");
 		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
diff --git a/include/spi.h b/include/spi.h
index ffd6647..07ba4d0 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -30,29 +30,25 @@ 
 #define SPI_XFER_MMAP		0x08	/* Memory Mapped start */
 #define SPI_XFER_MMAP_END	0x10	/* Memory Mapped End */
 #define SPI_XFER_ONCE		(SPI_XFER_BEGIN | SPI_XFER_END)
-#define SPI_XFER_U_PAGE		(1 << 5)
-
-/* SPI TX operation modes */
-#define SPI_OPM_TX_QPP		1 << 0
-
-/* SPI RX operation modes */
-#define SPI_OPM_RX_AS		1 << 0
-#define SPI_OPM_RX_DOUT		1 << 1
-#define SPI_OPM_RX_DIO		1 << 2
-#define SPI_OPM_RX_QOF		1 << 3
-#define SPI_OPM_RX_QIOF		1 << 4
-#define SPI_OPM_RX_EXTN		SPI_OPM_RX_AS | SPI_OPM_RX_DOUT | \
-				SPI_OPM_RX_DIO | SPI_OPM_RX_QOF | \
-				SPI_OPM_RX_QIOF
-
-/* SPI bus connection options */
-#define SPI_CONN_DUAL_SHARED	1 << 0
-#define SPI_CONN_DUAL_SEPARATED	1 << 1
 
 /* Header byte that marks the start of the message */
 #define SPI_PREAMBLE_END_BYTE	0xec
+#define SPI_DEFAULT_WORDLEN	8
+
+/* SPI RX operation modes */
+#define SPI_RX_AS	1 << 0
+#define SPI_RX_DOUT	1 << 1
+#define SPI_RX_DIO	1 << 2
+#define SPI_RX_QOF	1 << 3
+#define SPI_RX_QIOF	1 << 4
+#define SPI_RX_EXTN	SPI_RX_AS | SPI_RX_DOUT | SPI_RX_DIO | \
+			SPI_RX_QOF | SPI_RX_QIOF
 
-#define SPI_DEFAULT_WORDLEN 8
+/* SPI mode bits */
+#define SPI_TX_QPP	1 << 0
+#define SPI_SHARED	1 << 1
+#define SPI_SEPARATED	1 << 2
+#define SPI_U_PAGE	1 << 3
 
 /**
  * struct spi_slave - Representation of a SPI slave
@@ -61,25 +57,22 @@ 
  *
  * @bus:		ID of the bus that the slave is attached to.
  * @cs:			ID of the chip select connected to the slave.
- * @op_mode_rx:		SPI RX operation mode.
- * @op_mode_tx:		SPI TX operation mode.
  * @wordlen:		Size of SPI word in number of bits
  * @max_write_size:	If non-zero, the maximum number of bytes which can
  *			be written at once, excluding command bytes.
  * @memory_map:		Address of read-only SPI flash access.
- * @option:		Varies SPI bus options - separate, shared bus.
+ * @rx_mode:		SPI RX operation mode.
+ * @mode_bits:		SPI TX operation modes, bus options and few flags.
  * @flags:		Indication of SPI flags.
  */
 struct spi_slave {
 	unsigned int bus;
 	unsigned int cs;
-	u8 op_mode_rx;
-	u8 op_mode_tx;
 	unsigned int wordlen;
 	unsigned int max_write_size;
+	u16 rx_mode;
+	u16 mode_bits;
 	void *memory_map;
-	u8 option;
-	u8 flags;
 };
 
 /**
diff --git a/include/spi_flash.h b/include/spi_flash.h
index f79f0ea..b4b2f9b 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -38,9 +38,9 @@  enum spi_read_cmds {
 
 /* Dual SPI flash memories */
 enum spi_dual_flash {
-	SF_SINGLE_FLASH = 0,
-	SF_DUAL_STACKED_FLASH = 1 << 0,
-	SF_DUAL_PARALLEL_FLASH = 1 << 1,
+	SF_SINGLE = 0,
+	SF_STACKED = 1 << 0,
+	SF_PARALLEL = 1 << 1,
 };
 
 /**