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

Marek Vasut marex at denx.de
Sat Jan 18 17:59:57 CET 2014


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 at 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 at xilinx.com>
> >> Cc: Marek Vasut <marex at 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 .


More information about the U-Boot mailing list