[U-Boot] [PATCH v4 2/5] sf: Optimize flash features code

[Marek Vasut]

On Tuesday, February 04, 2014 at 05:06:14 PM, Jagannadha Sutradharudu Teki 
wrote:
> - Shrink spi_slave {}
> - Shrink spi_flash_params {}
> - Documentation for sf features
> 
> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna at xilinx.com>
> ---
>  doc/SPI/README.sf-features    | 121 +++++++++++++++++++++++++++++
>  drivers/mtd/spi/sf.c          |   4 +-
>  drivers/mtd/spi/sf_internal.h |   1 -
>  drivers/mtd/spi/sf_ops.c      |   8 +-
>  drivers/mtd/spi/sf_params.c   | 172
> +++++++++++++++++++++--------------------- drivers/mtd/spi/sf_probe.c    |
>  71 ++++++++---------
>  include/spi.h                 |  42 ++++-------
>  include/spi_flash.h           |  34 ++++-----
>  8 files changed, 272 insertions(+), 181 deletions(-)
>  create mode 100644 doc/SPI/README.sf-features
> 
> diff --git a/doc/SPI/README.sf-features b/doc/SPI/README.sf-features
> new file mode 100644
> index 0000000..e203bc0
> --- /dev/null
> +++ b/doc/SPI/README.sf-features
> @@ -0,0 +1,121 @@
> +SPI FLASH feature enhancements:
> +==============================

You're talking about SPI FLASH and the first thing you describe is struct 
spi_slave {} ? There should be a foreword about this being deprecated in this MW 
already and that such "extended" stuff is moving over to dedicated SPI NOR 
framework.

> +This document describes how to extend the current data structures in spi
> subsystem +for making use of new flash features/operations w.r.t to
> controller driver support. +
> +1. spi_slave:
> +
> +struct spi_slave {
> +    ..........
> +    u32 mode;
> +    ........
> +};
> +
> + at mode can be used to expose the SPI RX/TX operation modes, bus options and
> +few flags which are used to extended the flash specific
> features/operations +- include/spi.h
> +
> +mode:
> +- SPI_TX_QPP: 4-wire tx transfer

Quad Page Program ... should not ever have anything to do with generic SPI 
slave. See foreword above ...

> +- SPI_RX_SLOW: 1-wire rx transfer
> +- SPI_RX_DUAL: 2-wire rx transfer
> +- SPI_RX_DUAL_IO: 2-wire io rx transfer
> +- SPI_RX_QUAD: 4-wire rx transfer
> +- SPI_RX_QUAD_IO: 4-wire io rx transfer
> +- SPI_SHARED: bus shared with two slaves
> +- SPI_SEPARATED: bus parallel with two slaves
> +- SPI_U_PAGE: access second slave in bus shared
> +
> +2. spi_flash_params:
> +
> +struct spi_flash_params {
> +    ................
> +    u16 flags;
> +    ..............
> +};
> +
> + at flags can be use to verify the flash supported features/operations with
> respect +to controller driven through @mode and also some internal flash
> specific +operations - include/spi_flash.h
> +
> +flags:
> +- SST_WP: SST flash write protection
> +- SECT_4K: 4K erase sector
> +- SECT_32K: 32 erase sector
> +- E_FSR: Flag status register for erase/write for micron < 256MB flash
> +- WR_QPP: Quad page program
> +- RD_SLOW: Array slow read
> +- RD_DUAL: Dual fast read
> +- RD_DUAL_IO: Dual IO read
> +- RD_QUAD: Quad fast read
> +- RD_QUAD_IO: Quad IO read
> +- RD_2WIRE: All 2-wire read commands
> +- RD_FULL: All read commands
> +- ALL_CMDS: All read and write commands [1]
> +
> +3. spi_flash:
> +
> +struct spi_flash {
> +    ...............
> +	u8 dual_flash;
> +        u8 shift;
> +	u8 poll_cmd;
> +        u8 erase_cmd;
> +        u8 read_cmd;
> +        u8 write_cmd;
> +        u8 dummy_byte;
> +    ................

Please use spaces and tabs consistently.

> +};
> +
> +Few varibles from spi_flash {} can be used to perform the internal
> operations +based on the selected flash features/operations from spi_slave
> {} and +spi_flash_params {} - include/spi_flash.h
> +
> + at dual_flash: flash can be operated in dual flash [2]
> + at shift: variable shift operator useful for dual parallel
> + at poll_cmd: find the read_status or flag_status for polling erase/write
> operations + at erase_cmd: discovered erase command
> + at read_cmd: discovered read command
> + at write_cmd: discovered write command
> + at dummy_byte: read dummy_byte based on read dummy_cycles.
> +
> +dummy byte is determined based on the dummy cycles of a particular
> command. +Fast commands - dummy_byte = dummy_cycles/8
> +I/O commands- dummy_byte = (dummy_cycles * no.of lines)/8
> +For I/O commands except cmd[0] everything goes on no.of lines based on
> +particular command but in case of fast commands except data all go on
> +single line irrespective of command.
> +
> +4. Usage:
> +
> +In drivers/spi/*.c assign spi_slave {} with supported features through
> mode. +Ex: drivers/spi/ti_qspi.c
> +
> +struct ti_qspi_slave {
> +	................
> +	struct spi_slave slave;
> +	..............
> +}
> +
> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> +                                  unsigned int max_hz, unsigned int mode)
> +{
> +	.........
> +
> +	qslave = spi_alloc_slave(struct ti_qspi_slave, bus, cs);
> +	if (!qslave) {
> +		printf("SPI_error: Fail to allocate ti_qspi_slave\n");
> +		return NULL;
> +	}
> +
> +	qslave->slave.mode = SPI_TX_QPP | SPI_RX_QUAD;
> +	........
> +}

Sigh, looks like this document instructs people to abuse the SPI framework :-(

You need a BIG FAT WARNING that this was a design mistake here , really.

> +[1] http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf
> +[2] doc/SPI/README.dual-flash
> +
> +--
> +Jagannadha Sutradharudu Teki <jaganna at xilinx.com>
> +Sat Jan 18 14:44:28 IST 2014
> diff --git a/drivers/mtd/spi/sf.c b/drivers/mtd/spi/sf.c
> index 664e860..b4d7e6c 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 & SPI_U_PAGE)
> +		flags |= SPI_U_PAGE;

This change makes no sense, really, you're renaming flag and that's pointless. 
Please remove this globally.

[...]