sf: ops: Squash the malloc+memset combo

PATCH Superseded Jagannadha Sutradharudu Teki 4 participants 27 messages Jan 18, 2014 – Jan 21, 2014 1 message not in the archive Memory technology devices (MTD)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jagannadha Sutradharudu Teki

author

2014-01-18 20:06 UTC #1 lore raw

PATCH 1/6 sf: ops: Squash the malloc+memset combo

Squash the malloc()+memset() combo in favor of calloc().

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

Cc Marek Vasut <[email protected]>

---
 drivers/mtd/spi/sf_ops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/mtd/spi/sf_ops.c +5 -2

index 1f1bb36..1fac63a 100644
@@ -381,8 +381,11 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 	}
 
 	cmdsz = SPI_FLASH_CMD_LEN + flash->dummy_byte;
-	cmd = malloc(cmdsz);
-	memset(cmd, 0, cmdsz);
+	cmd = calloc(1, cmdsz);
+	if (!cmd) {
+		printf("SF: Failed to allocate cmd\n");
+		return ret;
+	}
 
 	cmd[0] = flash->read_cmd;
 	while (len) {

Signature · 1 line

1.8.3

Jagannadha Sutradharudu Teki

author

2014-01-18 20:06 UTC #3 lore raw

PATCH 2/6 sf: Optimize flash features code

In reply to a message not in the archive

- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

Cc Marek Vasut <[email protected]>

---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features

doc/SPI/README.sf-features new file +122 -0

new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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_bits;
+    ........
+};
+
+ at mode_bits 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_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
+- SPI_SHARED: dual flash devices are connected in shared bus connection
+- SPI_SEPARATED: dual flash devices are connected in separate bus connection
+- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1]
+
+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_bits 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 [2]
+
+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;
+    ................
+};
+
+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 [3]
+ 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
+@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_bits.
+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_bits = SPI_TX_QPP | SPI_RX_QUAD;
+	........
+}
+
+[1] http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
+[2] http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf
+[3] doc/SPI/README.dual-flash
+
+--
+Jagannadha Sutradharudu Teki <[email protected]>
+Sat Jan 18 14:44:28 IST 2014

drivers/mtd/spi/sf.c +2 -2

index 664e860..fb91ac6 100644
@@ -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;

drivers/mtd/spi/sf_internal.h +0 -1

index 6bcd522..47d5ac2 100644
@@ -67,7 +67,6 @@
 
 /* SST specific */
 #ifdef CONFIG_SPI_FLASH_SST
-# define SST_WP			0x01	/* Supports AAI word program */
 # define CMD_SST_BP		0x02    /* Byte Program */
 # define CMD_SST_AAI_WP		0xAD	/* Auto Address Incr Word Program */

drivers/mtd/spi/sf_ops.c +4 -4

index 1fac63a..2f3b03a 100644
@@ -138,9 +138,9 @@ static void spi_flash_dual_flash(struct spi_flash *flash, u32 *addr)
 	case SF_DUAL_STACKED_FLASH:
 		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:

@@ -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) {

drivers/mtd/spi/sf_params.c +86 ~~-86~~

192 lines not on this page · raw message

drivers/mtd/spi/sf_probe.c +31 ~~-40~~

109 lines not on this page · raw message

include/spi.h +15 ~~-27~~

69 lines not on this page · raw message

include/spi_flash.h +10 ~~-14~~

index f79f0ea..16ca294 100644
@@ -20,21 +20,19 @@
 #include <linux/compiler.h>
 
 /* sf param flags */
+#define SST_WP		1 << 0
 #define SECT_4K		1 << 1
 #define SECT_32K	1 << 2
 #define E_FSR		1 << 3
 #define WR_QPP		1 << 4
-
-/* Enum list - Full read commands */
-enum spi_read_cmds {
-	ARRAY_SLOW = 1 << 0,
-	DUAL_OUTPUT_FAST = 1 << 1,
-	DUAL_IO_FAST = 1 << 2,
-	QUAD_OUTPUT_FAST = 1 << 3,
-	QUAD_IO_FAST = 1 << 4,
-};
-#define RD_EXTN		ARRAY_SLOW | DUAL_OUTPUT_FAST | DUAL_IO_FAST
-#define RD_FULL		RD_EXTN | QUAD_OUTPUT_FAST | QUAD_IO_FAST
+#define RD_SLOW		1 << 5
+#define RD_DUAL		1 << 6
+#define RD_DUAL_IO	1 << 7
+#define RD_QUAD		1 << 8
+#define RD_QUAD_IO	1 << 9
+#define RD_2WIRE	(RD_SLOW | RD_DUAL | RD_DUAL_IO)
+#define RD_FULL		(RD_2WIRE | RD_QUAD | RD_QUAD_IO)
+#define ALL_CMDS	(WR_QPP | RD_FULL)
 
 /* Dual SPI flash memories */
 enum spi_dual_flash {

@@ -51,8 +49,7 @@ enum spi_dual_flash {
  * @ext_jedec:		Device ext_jedec ID
  * @sector_size:	Sector size of this device
  * @nr_sectors:		No.of sectors on this device
- * @e_rd_cmd:		Enum list for read commands
- * @flags:		Importent param, for flash specific behaviour
+ * @flags:		Importent params, for flash specific behaviour
  */
 struct spi_flash_params {
 	const char *name;

@@ -60,7 +57,6 @@ struct spi_flash_params {
 	u16 ext_jedec;
 	u32 sector_size;
 	u32 nr_sectors;
-	u8 e_rd_cmd;
 	u16 flags;
 };

Signature · 1 line

1.8.3

Marek Vasut

2014-01-18 20:39 UTC #4 lore raw

PATCH 2/6 sf: Optimize flash features code

Quoted 52 lines · Jagannadha Sutradharudu Teki

- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

Cc Marek Vasut <[email protected]>

---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features

doc/SPI/README.sf-features new file +4 -0

new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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:

doc/SPI/README.sf-features new file +8 -0

+
+struct spi_slave {
+    ..........
+    u32 mode_bits;
+    ........
+};
+
+ at mode_bits 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

doc/SPI/README.sf-features new file +10 -0

+
+mode_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
+- SPI_SHARED: dual flash devices are connected in shared bus connection
+- SPI_SEPARATED: dual flash devices are connected in separate bus

Quoted 2 lines

connection +- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1] +

A generic SPI controller _does_ _not_ _care_ about any SPI flash crud. The SPI bus controller (which is what this is for) and SPI-NOR controller are two different things and must have two different slave structures.

Jagan Teki

2014-01-18 20:51 UTC #5 lore raw

PATCH 2/6 sf: Optimize flash features code

Quoted 52 lines · Marek Vasut

Quoted 50 lines · Jagannadha Sutradharudu Teki

- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

Cc Marek Vasut <[email protected]>

---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features

doc/SPI/README.sf-features new file +4 -0

new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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:

doc/SPI/README.sf-features new file +8 -0

+
+struct spi_slave {
+    ..........
+    u32 mode_bits;
+    ........
+};
+
+ at mode_bits 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

doc/SPI/README.sf-features new file +8 -0

+
+mode_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read

Quoted 8 lines

Quoted 4 lines
+- SPI_SHARED: dual flash devices are connected in shared bus connection
+- SPI_SEPARATED: dual flash devices are connected in separate bus
connection +- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1] +
A generic SPI controller _does_ _not_ _care_ about any SPI flash crud. The SPI bus controller (which is what this is for) and SPI-NOR controller are two different things and must have two different slave structures.

You mean mode_bits need to move in one more structure.
Just leave about new SPI-NOR as of now for this release we discuss more soon.

Signature · 2 lines

Thanks,
Jagan.

Marek Vasut

2014-01-20 13:19 UTC #6 lore raw

PATCH 2/6 sf: Optimize flash features code

Quoted 69 lines · Jagan Teki

Quoted 59 lines · Marek Vasut
On Saturday, January 18, 2014 at 09:06:29 PM, Jagannadha Sutradharudu Teki
Quoted 55 lines · wrote:
- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features
doc/SPI/README.sf-features new file +4 -0
new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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:
doc/SPI/README.sf-features new file +8 -0
+
+struct spi_slave {
+    ..........
+    u32 mode_bits;
+    ........
+};
+
+ at mode_bits 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
doc/SPI/README.sf-features new file +9 -0
+
+mode_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
+- SPI_SHARED: dual flash devices are connected in shared bus connection
Quoted 8 lines
Quoted 3 lines
+- SPI_SEPARATED: dual flash devices are connected in separate bus connection +- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1] +
A generic SPI controller _does_ _not_ _care_ about any SPI flash crud. The SPI bus controller (which is what this is for) and SPI-NOR controller are two different things and must have two different slave structures.

Quoted 3 lines

You mean mode_bits need to move in one more structure.
Just leave about new SPI-NOR as of now for this release we discuss more soon.

The mode_bits have no place in this structure. The slave can indicate whether it can be connected over 1,2,4... lines , but must not indicate that it supports some SPI-flash specific properties.

Best regards,
Marek Vasut

Jagan Teki

2014-01-20 13:32 UTC #7 lore raw

PATCH 2/6 sf: Optimize flash features code

Quoted 70 lines · Marek Vasut

Quoted 69 lines · Jagan Teki
Quoted 67 lines · Marek Vasut
On Saturday, January 18, 2014 at 09:06:29 PM, Jagannadha Sutradharudu Teki
Quoted 56 lines · wrote:
- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features
doc/SPI/README.sf-features new file +4 -0
new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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:
doc/SPI/README.sf-features new file +8 -0
+
+struct spi_slave {
+    ..........
+    u32 mode_bits;
+    ........
+};
+
+ at mode_bits 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
doc/SPI/README.sf-features new file +10 -0
+
+mode_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
+- SPI_SHARED: dual flash devices are connected in shared bus connection
+- SPI_SEPARATED: dual flash devices are connected in separate bus
Quoted 2 lines
connection +- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1] +
A generic SPI controller _does_ _not_ _care_ about any SPI flash crud. The SPI bus controller (which is what this is for) and SPI-NOR controller are two different things and must have two different slave structures.

Quoted 7 lines

Quoted 3 lines
You mean mode_bits need to move in one more structure.
Just leave about new SPI-NOR as of now for this release we discuss more soon.
The mode_bits have no place in this structure. The slave can indicate whether it can be connected over 1,2,4... lines , but must not indicate that it supports some SPI-flash specific properties.

What do you mean by this - can you elaborate.
As of now drivers in drivers/spi need to inform the flash through spi_slave {} no other alternative ie way remaining flash properties memory_map etc.. handle. Even Linux follow the same w/o new SPI-NOR framework.

If your question, like need a separate structure for flash specific properties please wait will wound-up all these in new framework.

I'm planning to push in today release.

Signature · 2 lines

Thanks,
Jagan.

Marek Vasut

2014-01-20 23:01 UTC #8 lore raw

PATCH 2/6 sf: Optimize flash features code

Quoted 74 lines · Jagan Teki

Quoted 73 lines · Marek Vasut
Quoted 71 lines · Jagan Teki
Quoted 58 lines · Marek Vasut
On Saturday, January 18, 2014 at 09:06:29 PM, Jagannadha Sutradharudu Teki
Quoted 54 lines · wrote:
- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features
doc/SPI/README.sf-features new file +4 -0
new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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:
doc/SPI/README.sf-features new file +8 -0
+
+struct spi_slave {
+    ..........
+    u32 mode_bits;
+    ........
+};
+
+ at mode_bits 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
doc/SPI/README.sf-features new file +9 -0
+
+mode_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
+- SPI_SHARED: dual flash devices are connected in shared bus
Quoted 8 lines
Quoted 3 lines
connection +- SPI_SEPARATED: dual flash devices are connected in separate bus connection +- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1] +
A generic SPI controller _does_ _not_ _care_ about any SPI flash crud. The SPI bus controller (which is what this is for) and SPI-NOR controller are two different things and must have two different slave structures.
You mean mode_bits need to move in one more structure.
Just leave about new SPI-NOR as of now for this release we discuss more soon.

Quoted 5 lines

Quoted 3 lines
The mode_bits have no place in this structure. The slave can indicate whether it can be connected over 1,2,4... lines , but must not indicate that it supports some SPI-flash specific properties.
What do you mean by this - can you elaborate.

I mean there do exist SPI controllers which can output data via more than one (MOSI) line . They do support 1, 2 or even 4 lines. This comes useful when driving SPI flashes indeed, _BUT_ (!) this is not tied to driving SPI flashes. On the contrary, you can connect any device which can be driven over such "parallelised" SPI and it will work. And only such property shall also land in the struct spi_slave {} .

Properties like "let's assume the SPI slave is a SPI NOR and it supports SPI NOR command FOO" do not go into struct spi_slave.

Quoted 3 lines

As of now drivers in drivers/spi need to inform the flash through spi_slave {} no other alternative ie way remaining flash properties memory_map etc.. handle. Even Linux follow the same w/o new SPI-NOR framework.

Linux is now going for a separate SPI-NOR controller framework and this is what we will do as well. The SF layer will be only a unifying layer providing access to either SPI-NOR API (for the SPI-NOR controllers) or a translation layer to communicate SPI-NOR commands over generic SPI API (for the old-school regular way of doing SPI NOR connection to SPI bus).

Quoted 4 lines

If your question, like need a separate structure for flash specific properties please wait will wound-up all these in new framework.
I'm planning to push in today release.

I doubt anything is going in today's release ;-)

Actually, you need to really start discerning which _must_ go into a release and which simply does not go into a release and waits for -next. This is important role of a maintainer and plays important role in keeping the codebase in good shape.

Code which is untested, code which changes core stuff and isn't an obvious bugfix etc. simply can wait. Code which is obvious bugfix goes in even in the - rc period. Of course, there are all kinds of exceptions, that depends on the rationale and negotiation.

Jagan Teki

2014-01-21 07:39 UTC #9 lore raw

PATCH 2/6 sf: Optimize flash features code

Hi Marek,

Quoted 95 lines · Marek Vasut

Quoted 74 lines · Jagan Teki
Quoted 73 lines · Marek Vasut
Quoted 71 lines · Jagan Teki
Quoted 58 lines · Marek Vasut
On Saturday, January 18, 2014 at 09:06:29 PM, Jagannadha Sutradharudu Teki
Quoted 54 lines · wrote:
- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features
doc/SPI/README.sf-features new file +4 -0
new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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:
doc/SPI/README.sf-features new file +8 -0
+
+struct spi_slave {
+    ..........
+    u32 mode_bits;
+    ........
+};
+
+ at mode_bits 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
doc/SPI/README.sf-features new file +9 -0
+
+mode_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
+- SPI_SHARED: dual flash devices are connected in shared bus
Quoted 8 lines
Quoted 3 lines
connection +- SPI_SEPARATED: dual flash devices are connected in separate bus connection +- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1] +
A generic SPI controller _does_ _not_ _care_ about any SPI flash crud. The SPI bus controller (which is what this is for) and SPI-NOR controller are two different things and must have two different slave structures.
You mean mode_bits need to move in one more structure.
Just leave about new SPI-NOR as of now for this release we discuss more soon.
Quoted 5 lines
Quoted 3 lines
The mode_bits have no place in this structure. The slave can indicate whether it can be connected over 1,2,4... lines , but must not indicate that it supports some SPI-flash specific properties.
What do you mean by this - can you elaborate.
I mean there do exist SPI controllers which can output data via more than one (MOSI) line . They do support 1, 2 or even 4 lines. This comes useful when driving SPI flashes indeed, _BUT_ (!) this is not tied to driving SPI flashes. On the contrary, you can connect any device which can be driven over such "parallelised" SPI and it will work. And only such property shall also land in the struct spi_slave {} .
Properties like "let's assume the SPI slave is a SPI NOR and it supports SPI NOR command FOO" do not go into struct spi_slave.
Quoted 3 lines
As of now drivers in drivers/spi need to inform the flash through spi_slave {} no other alternative ie way remaining flash properties memory_map etc.. handle. Even Linux follow the same w/o new SPI-NOR framework.

Quoted 5 lines

Linux is now going for a separate SPI-NOR controller framework and this is what we will do as well. The SF layer will be only a unifying layer providing access to either SPI-NOR API (for the SPI-NOR controllers) or a translation layer to communicate SPI-NOR commands over generic SPI API (for the old-school regular way of doing SPI NOR connection to SPI bus).

Please correct me logic here!
Just take an example of ti qspi controller.

cmd_sf.c
------------
spi_flash.h
--------------
sf (_probe, _ops), sf.c
-------------------------------------------
drivers/spi/ti_qspi.c (inform flash info mode_bits through spi_slave {}
------------------------------------------------------------

Suppose If we have one more qspi controller(zynq) which is connected to SPI-NOR

                                                cmd_sf.c
                                                -----------
                                               spi_flash.h
                                              ---------------
                                               SF-NOR

------------------------------------------------------------------------------------
drivers/spi/ti_qspi.c
drivers/mtd/sf_nor/zynq_qspi.c
(inform flash properties
(inform flash properties
through "mode_bits" of
through "mode_bits" of
spi_slave {})

Quoted 17 lines

Quoted 5 lines
If your question, like need a separate structure for flash specific properties please wait will wound-up all these in new framework.
I'm planning to push in today release.
I doubt anything is going in today's release ;-)
Actually, you need to really start discerning which _must_ go into a release and which simply does not go into a release and waits for -next. This is important role of a maintainer and plays important role in keeping the codebase in good shape.
Code which is untested, code which changes core stuff and isn't an obvious bugfix etc. simply can wait. Code which is obvious bugfix goes in even in the - rc period. Of course, there are all kinds of exceptions, that depends on the rationale and negotiation.

Signature · 1 line

Jagan.

Jagan Teki

2014-01-21 07:45 UTC #10 lore raw

PATCH 2/6 sf: Optimize flash features code

Hi Marek,

Please ignore the last sent!

Quoted 103 lines · Jagan Teki

Hi Marek,
Quoted 100 lines · Marek Vasut
Quoted 74 lines · Jagan Teki
Quoted 73 lines · Marek Vasut
Quoted 71 lines · Jagan Teki
Quoted 58 lines · Marek Vasut
On Saturday, January 18, 2014 at 09:06:29 PM, Jagannadha Sutradharudu Teki
Quoted 54 lines · wrote:
- Shrink spi_slave {}
- Shrink spi_flash_params {}
- Documentation for sf features
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
---
 doc/SPI/README.sf-features    | 122 ++++++++++++++++++++++++++++++
 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           |  24 +++---
 8 files changed, 270 insertions(+), 174 deletions(-)
 create mode 100644 doc/SPI/README.sf-features
doc/SPI/README.sf-features new file +4 -0
new file mode 100644
index 0000000..d35f56d
@@ -0,0 +1,122 @@
+SPI FLASH feature enhancements:
+==============================
+
+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:
doc/SPI/README.sf-features new file +8 -0
+
+struct spi_slave {
+    ..........
+    u32 mode_bits;
+    ........
+};
+
+ at mode_bits 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
doc/SPI/README.sf-features new file +9 -0
+
+mode_bits:
+- SPI_TX_QPP: 4-Wire tx transfer operation quad page program
+- SPI_RX_SLOW: 1-wire rx transfer operation array slow read
+- SPI_RX_DUAL: 2-wire rx transfer operation dual fast read
+- SPI_RX_DUAL_IO: 2-wire rx transfer operation dual io fast read
+- SPI_RX_QUAD: 4-wire rx transfer operation quad fast read
+- SPI_RX_QUAD_IO: 4-wire rx transfer operation quad io fast read
+- SPI_SHARED: dual flash devices are connected in shared bus
Quoted 8 lines
Quoted 3 lines
connection +- SPI_SEPARATED: dual flash devices are connected in separate bus connection +- SPI_U_PAGE: select the upper flash in dual flash shared bus connection [1] +
A generic SPI controller _does_ _not_ _care_ about any SPI flash crud. The SPI bus controller (which is what this is for) and SPI-NOR controller are two different things and must have two different slave structures.
You mean mode_bits need to move in one more structure.
Just leave about new SPI-NOR as of now for this release we discuss more soon.
Quoted 5 lines
Quoted 3 lines
The mode_bits have no place in this structure. The slave can indicate whether it can be connected over 1,2,4... lines , but must not indicate that it supports some SPI-flash specific properties.
What do you mean by this - can you elaborate.
I mean there do exist SPI controllers which can output data via more than one (MOSI) line . They do support 1, 2 or even 4 lines. This comes useful when driving SPI flashes indeed, _BUT_ (!) this is not tied to driving SPI flashes. On the contrary, you can connect any device which can be driven over such "parallelised" SPI and it will work. And only such property shall also land in the struct spi_slave {} .
Properties like "let's assume the SPI slave is a SPI NOR and it supports SPI NOR command FOO" do not go into struct spi_slave.
Quoted 3 lines
As of now drivers in drivers/spi need to inform the flash through spi_slave {} no other alternative ie way remaining flash properties memory_map etc.. handle. Even Linux follow the same w/o new SPI-NOR framework.
Linux is now going for a separate SPI-NOR controller framework and this is what we will do as well. The SF layer will be only a unifying layer providing access to either SPI-NOR API (for the SPI-NOR controllers) or a translation layer to communicate SPI-NOR commands over generic SPI API (for the old-school regular way of doing SPI NOR connection to SPI bus).

Just take an example of ti qspi controller which is connected to serial nor

Current design:

cmd_sf.c
------------
spi_flash.h
--------------
sf (_probe, _ops), sf.c
-------------------------------------------
drivers/spi/ti_qspi.c (inform flash properties through "mode_bits" spi_slave {}
-------------------------------------------------------------------------------

Proposed one (may be):

Suppose If we have one more qspi controller(zynq) which is connected to serial nor

                                                cmd_sf.c
                                                -----------
                                               spi_flash.h
                                              ---------------
                                               SF-NOR

------------------------------------------------------------------------------------------------
drivers/spi/ti_qspi.c
drivers/mtd/sf_nor/zynq_qspi.c
(inform flash properties
(inform flash properties
through "mode_bits" of
through "mode_bits" of
spi_slave {})

        sf_nor_device {}

Please comment on above proposed design will ask you more once this set..thanks!

Signature · 1 line

Jagan.

Marek Vasut

2014-01-21 17:48 UTC #11 lore raw

PATCH 2/6 sf: Optimize flash features code

On Tuesday, January 21, 2014 at 08:45:52 AM, Jagan Teki wrote:
[...]

Quoted 3 lines

Current design:
cmd_sf.c

This is just the one of the users of the SPI flash API, but not the only one.

Quoted 28 lines

------------
spi_flash.h
--------------
sf (_probe, _ops), sf.c
-------------------------------------------
drivers/spi/ti_qspi.c (inform flash properties through "mode_bits" spi_slave {}
--------------------------------------------------------------------------
-----
Proposed one (may be):
Suppose If we have one more qspi controller(zynq) which is connected to serial nor
                                                cmd_sf.c
                                                -----------
                                               spi_flash.h
                                              ---------------
                                               SF-NOR
---------------------------------------------------------------------------
--------------------- drivers/spi/ti_qspi.c
drivers/mtd/sf_nor/zynq_qspi.c
(inform flash properties
(inform flash properties
through "mode_bits" of
through "mode_bits" of
spi_slave {})

Quoted 1 line

        sf_nor_device {}

Did the formating here go awry ?

But anyway, your sf_probe.c/sf_ops.c/sf.c will only need to somehow discern whether to call the regular SPI API or SPI-NOR API functions.

Best regards,
Marek Vasut

Jagannadha Sutradharudu Teki

author

2014-01-18 20:06 UTC #12 lore raw

PATCH 3/6 sf: Renames on dual_flash stuff

In reply to a message not in the archive

- Used small names for dual_flash macros
- Updated doc/SPI/README.dual-flash

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

Cc Marek Vasut <[email protected]>

---
 doc/SPI/README.dual-flash  |  9 +++++----
 doc/SPI/README.sf-features |  3 +++
 drivers/mtd/spi/sf_ops.c   | 10 +++++-----
 drivers/mtd/spi/sf_probe.c | 18 +++++++++++-------
 include/spi_flash.h        | 14 +++++++-------
 5 files changed, 31 insertions(+), 23 deletions(-)

doc/SPI/README.dual-flash +5 -4

index 6c88d65..6c4236d 100644
@@ -9,11 +9,12 @@ to a given controller with single chip select line, but there are some
 hw logics(ex: xilinx zynq qspi) that describes two/dual memories are
 connected with a single chip select line from a controller.
 
-"dual_flash" from include/spi.h describes these types of connection mode
+"dual_flash" from include/spi_flash.h describes these types of connection mode
+in spi flash side and "mode_bits" options for controller driver.
 
 Possible connections:
 --------------------
-SF_SINGLE_FLASH:
+SF_SINGLE:
        - single spi flash memory connected with single chip select line.
 
   +------------+             CS         +---------------+

@@ -24,7 +25,7 @@ SF_SINGLE_FLASH:
   |            |----------------------->|               |
   +------------+                        +---------------+
 
-SF_DUAL_STACKED_FLASH:
+SF_STACKED:
        - dual spi/qspi flash memories are connected with a single chipselect
          line and these two memories are operating stacked fasion with shared buses.
        - xilinx zynq qspi controller has implemented this feature [1]

@@ -54,7 +55,7 @@ SF_DUAL_STACKED_FLASH:
                by default, if U_PAGE is unset lower memory should accessible,
                once user wants to access upper memory need to set U_PAGE.
 
-SPI_FLASH_CONN_DUALPARALLEL:
+SF_PARALLEL:
 	- dual spi/qspi flash memories are connected with a single chipselect
 	  line and these two memories are operating parallel with separate buses.
 	- xilinx zynq qspi controller has implemented this feature [1]

doc/SPI/README.sf-features +3 -0

index d35f56d..851dfa8 100644
@@ -73,6 +73,9 @@ based on the selected flash features/operations from spi_slave {} and
 spi_flash_params {} - include/spi_flash.h
 
 @dual_flash: flash can be operated in dual flash [3]
+- SF_SINGLE: default connection single flash
+- SF_STACKED: dual flash with dual stacked connection
+- SF_PARALLEL: dual flash with dual parallel connection
 @shift: variable shift operator useful for dual parallel
 @poll_cmd: find the read_status or flag_status for polling erase/write operations
 @erase_cmd: discovered erase command

drivers/mtd/spi/sf_ops.c +5 -5

index 2f3b03a..6cbbfe3 100644
@@ -135,7 +135,7 @@ 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->mode_bits |= SPI_U_PAGE;

@@ -143,7 +143,7 @@ static void spi_flash_dual_flash(struct spi_flash *flash, u32 *addr)
 			flash->spi->mode_bits &= ~SPI_U_PAGE;
 		}
 		break;
-	case SF_DUAL_PARALLEL_FLASH:
+	case SF_PARALLEL:
 		*addr >>= flash->shift;
 		break;
 	default:

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

@@ -392,7 +392,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

drivers/mtd/spi/sf_probe.c +11 -7

index d1ebf72..79fbad7 100644
@@ -123,7 +123,6 @@ 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;
 
 	/* Assign spi_flash ops */
 	flash->read = spi_flash_cmd_read_ops;

@@ -133,17 +132,22 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	if (params->flags & SST_WP)
 		flash->write = sst_write_wp;
 #endif
-
+	/* Get the dual flash connection modes */
+#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
 	/* 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
-
 	/* Compute erase sector and command */
 	if (params->flags & SECT_4K) {
 		flash->erase_cmd = CMD_ERASE_4K;

@@ -320,9 +324,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");

include/spi_flash.h +7 -7

index 16ca294..11488e1 100644
@@ -19,6 +19,13 @@
 #include <linux/types.h>
 #include <linux/compiler.h>
 
+/* Dual SPI flash memories */
+enum spi_dual_flash {
+	SF_SINGLE = 0,
+	SF_STACKED = 1 << 0,
+	SF_PARALLEL = 1 << 1,
+};
+
 /* sf param flags */
 #define SST_WP		1 << 0
 #define SECT_4K		1 << 1

@@ -34,13 +41,6 @@
 #define RD_FULL		(RD_2WIRE | RD_QUAD | RD_QUAD_IO)
 #define ALL_CMDS	(WR_QPP | RD_FULL)
 
-/* Dual SPI flash memories */
-enum spi_dual_flash {
-	SF_SINGLE_FLASH = 0,
-	SF_DUAL_STACKED_FLASH = 1 << 0,
-	SF_DUAL_PARALLEL_FLASH = 1 << 1,
-};
-
 /**
  * struct spi_flash_params - SPI/QSPI flash device params structure
  *

Signature · 1 line

1.8.3

Marek Vasut

2014-01-18 20:37 UTC #13 lore raw

PATCH 3/6 sf: Renames on dual_flash stuff

Quoted 5 lines · Jagannadha Sutradharudu Teki

- Used small names for dual_flash macros
- Updated doc/SPI/README.dual-flash
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>

I agree with the documentation, but disagree with the rename. Please split the patch into multiple logical blocks so these can be reviewed separatelly. One would be the documentation, the next the rename and I also see some new #ifdef in the code, which would be a third block.

Best regards,
Marek Vasut

Jagan Teki

2014-01-18 20:49 UTC #14 lore raw

PATCH 3/6 sf: Renames on dual_flash stuff

Quoted 5 lines · Marek Vasut

Quoted 3 lines · Jagannadha Sutradharudu Teki
- Used small names for dual_flash macros
- Updated doc/SPI/README.dual-flash

Quoted 7 lines

Quoted 2 lines
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
I agree with the documentation, but disagree with the rename. Please split the patch into multiple logical blocks so these can be reviewed separatelly. One would be the documentation, the next the rename and I also see some new #ifdef in the code, which would be a third block.

I don't think we need to separate here! as documentation got changed because of rename of macros' and also new #ifdef is already added one CONFIG_SF_DUAL_FLASH which is also dual_flash specific.

Signature · 2 lines

Thanks,
Jagan.

Marek Vasut

2014-01-20 13:16 UTC #15 lore raw

PATCH 3/6 sf: Renames on dual_flash stuff

Quoted 11 lines · Jagan Teki

Quoted 10 lines · Marek Vasut
On Saturday, January 18, 2014 at 09:06:30 PM, Jagannadha Sutradharudu Teki
Quoted 5 lines · wrote:
- Used small names for dual_flash macros
- Updated doc/SPI/README.dual-flash
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>

Quoted 8 lines

Quoted 4 lines
I agree with the documentation, but disagree with the rename. Please split the patch into multiple logical blocks so these can be reviewed separatelly. One would be the documentation, the next the rename and I also see some new #ifdef in the code, which would be a third block.
I don't think we need to separate here! as documentation got changed because of rename of macros' and also new #ifdef is already added one CONFIG_SF_DUAL_FLASH which is also dual_flash specific.

I still see documentation fixes, renames and even newly added code. This really makes no sense to me to meld all these into a single patch.

Best regards,
Marek Vasut

Jagan Teki

2014-01-20 13:35 UTC #16 lore raw

PATCH 3/6 sf: Renames on dual_flash stuff

Quoted 17 lines · Marek Vasut

Quoted 16 lines · Jagan Teki
Quoted 14 lines · Marek Vasut
On Saturday, January 18, 2014 at 09:06:30 PM, Jagannadha Sutradharudu Teki
Quoted 5 lines · wrote:
- Used small names for dual_flash macros
- Updated doc/SPI/README.dual-flash
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
I agree with the documentation, but disagree with the rename. Please split the patch into multiple logical blocks so these can be reviewed separatelly. One would be the documentation, the next the rename and I also see some new #ifdef in the code, which would be a third block.

Quoted 6 lines

Quoted 3 lines
I don't think we need to separate here! as documentation got changed because of rename of macros' and also new #ifdef is already added one CONFIG_SF_DUAL_FLASH which is also dual_flash specific.
I still see documentation fixes, renames and even newly added code. This really makes no sense to me to meld all these into a single patch.

Will be clear in next series for sure!

Signature · 2 lines

Thanks,
Jagan.

Jagannadha Sutradharudu Teki

author

2014-01-18 20:06 UTC #17 lore raw

PATCH 4/6 sf: Update read/write command macros

In reply to a message not in the archive

- Used readable names for read/write command macros
- Added comments for the same

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Simon Glass <[email protected]>

---
 drivers/mtd/spi/ramtron.c     | 10 ++++------
 drivers/mtd/spi/sandbox.c     | 12 ++++++------
 drivers/mtd/spi/sf_internal.h | 16 ++++++++--------
 drivers/mtd/spi/sf_probe.c    | 20 ++++++++++----------
 4 files changed, 28 insertions(+), 30 deletions(-)

drivers/mtd/spi/ramtron.c +4 -6

index d50da37..bdf69f7 100644
@@ -167,7 +167,7 @@ static int ramtron_common(struct spi_flash *flash,
 		return ret;
 	}
 
-	if (command == CMD_PAGE_PROGRAM) {
+	if (command == CMD_WR_PAGE) {
 		/* send WREN */
 		ret = spi_flash_cmd_write_enable(flash);
 		if (ret < 0) {

@@ -177,7 +177,7 @@ static int ramtron_common(struct spi_flash *flash,
 	}
 
 	/* do the transaction */
-	if (command == CMD_PAGE_PROGRAM)
+	if (command == CMD_WR_PAGE)
 		ret = spi_flash_cmd_write(flash->spi, cmd, cmd_len, buf, len);
 	else
 		ret = spi_flash_cmd_read(flash->spi, cmd, cmd_len, buf, len);

@@ -193,15 +193,13 @@ releasebus:
 static int ramtron_read(struct spi_flash *flash,
 		u32 offset, size_t len, void *buf)
 {
-	return ramtron_common(flash, offset, len, buf,
-		CMD_READ_ARRAY_SLOW);
+	return ramtron_common(flash, offset, len, buf, CMD_RD_SLOW);
 }
 
 static int ramtron_write(struct spi_flash *flash,
 		u32 offset, size_t len, const void *buf)
 {
-	return ramtron_common(flash, offset, len, (void *)buf,
-		CMD_PAGE_PROGRAM);
+	return ramtron_common(flash, offset, len, (void *)buf, CMD_WR_PAGE);
 }
 
 static int ramtron_erase(struct spi_flash *flash, u32 offset, size_t len)

drivers/mtd/spi/sandbox.c +6 -6

index a62ef4c..bebfb32 100644
@@ -219,10 +219,10 @@ static int sandbox_sf_process_cmd(struct sandbox_spi_flash *sbsf, const u8 *rx,
 		sbsf->state = SF_ID;
 		sbsf->cmd = SF_ID;
 		break;
-	case CMD_READ_ARRAY_FAST:
+	case CMD_RD_FAST:
 		sbsf->pad_addr_bytes = 1;
-	case CMD_READ_ARRAY_SLOW:
-	case CMD_PAGE_PROGRAM:
+	case CMD_RD_SLOW:
+	case CMD_WR_PAGE:
  state_addr:
 		sbsf->state = SF_ADDR;
 		break;

@@ -339,11 +339,11 @@ static int sandbox_sf_xfer(void *priv, const u8 *rx, u8 *tx,
 				return 1;
 			}
 			switch (sbsf->cmd) {
-			case CMD_READ_ARRAY_FAST:
-			case CMD_READ_ARRAY_SLOW:
+			case CMD_RD_FAST:
+			case CMD_RD_SLOW:
 				sbsf->state = SF_READ;
 				break;
-			case CMD_PAGE_PROGRAM:
+			case CMD_WR_PAGE:
 				sbsf->state = SF_WRITE;
 				break;
 			default:

drivers/mtd/spi/sf_internal.h +8 -8

index 47d5ac2..6b6fa22 100644
@@ -28,22 +28,22 @@
 
 /* Write commands */
 #define CMD_WRITE_STATUS		0x01
-#define CMD_PAGE_PROGRAM		0x02
+#define CMD_WR_PAGE			0x02	/* Page program */
 #define CMD_WRITE_DISABLE		0x04
 #define CMD_READ_STATUS			0x05
-#define CMD_QUAD_PAGE_PROGRAM		0x32
+#define CMD_WR_QUAD			0x32	/* Quad page program */
 #define CMD_READ_STATUS1		0x35
 #define CMD_WRITE_ENABLE		0x06
 #define CMD_READ_CONFIG			0x35
 #define CMD_FLAG_STATUS			0x70
 
 /* Read commands */
-#define CMD_READ_ARRAY_SLOW		0x03
-#define CMD_READ_ARRAY_FAST		0x0b
-#define CMD_READ_DUAL_OUTPUT_FAST	0x3b
-#define CMD_READ_DUAL_IO_FAST		0xbb
-#define CMD_READ_QUAD_OUTPUT_FAST	0x6b
-#define CMD_READ_QUAD_IO_FAST		0xeb
+#define CMD_RD_SLOW			0x03	/* Array slow */
+#define CMD_RD_FAST			0x0b	/* Array fast */
+#define CMD_RD_DUAL			0x3b	/* Dual output fast */
+#define CMD_RD_DUAL_IO			0xbb	/* Dual IO fast */
+#define CMD_RD_QUAD			0x6b	/* Quad output fast */
+#define CMD_RD_QUAD_IO			0xeb	/* Quad IO fast */
 #define CMD_READ_ID			0x9f
 
 /* Bank addr access commands */

drivers/mtd/spi/sf_probe.c +10 ~~-10~~

index 79fbad7..7ba0605 100644
@@ -161,30 +161,30 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	}
 
 	/* Compute read command and dummy_byte */
-	flash->read_cmd = CMD_READ_ARRAY_FAST;
+	flash->read_cmd = CMD_RD_FAST;
 	flash->dummy_byte = 1;
 	switch (SPI_RX_MODES & flash->spi->mode_bits) {
 	case SPI_RX_SLOW:
 		if (params->flags & RD_SLOW) {
-			flash->read_cmd = CMD_READ_ARRAY_FAST;
+			flash->read_cmd = CMD_RD_SLOW;
 			flash->dummy_byte = 0;
 		}
 		break;
 	case SPI_RX_DUAL:
 		if (params->flags & RD_DUAL)
-			flash->read_cmd = CMD_READ_DUAL_OUTPUT_FAST;
+			flash->read_cmd = CMD_RD_DUAL;
 		break;
 	case SPI_RX_DUAL_IO:
 		if (params->flags & RD_DUAL_IO)
-			flash->read_cmd = CMD_READ_DUAL_OUTPUT_IO;
+			flash->read_cmd = CMD_RD_DUAL_IO;
 		break;
 	case SPI_RX_QUAD:
 		if (params->flags & RD_QUAD)
-			flash->read_cmd = CMD_READ_QUAD_OUTPUT_FAST;
+			flash->read_cmd = CMD_RD_QUAD;
 		break;
 	case SPI_RX_QUAD_IO:
 		if (params->flags & RD_QUAD_IO) {
-			flash->read_cmd = CMD_READ_QUAD_OUTPUT_FAST_IO;
+			flash->read_cmd = CMD_RD_QUAD_IO;
 			flash->dummy_byte = 2;
 		}
 		break;

@@ -195,12 +195,12 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 		flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
 	else
 		/* Go for default supported write cmd */
-		flash->write_cmd = CMD_PAGE_PROGRAM;
+		flash->write_cmd = CMD_WR_PAGE;
 
 	/* Set the quad enable bit - only for quad commands */
-	if ((flash->read_cmd == CMD_READ_QUAD_OUTPUT_FAST) ||
-	    (flash->read_cmd == CMD_READ_QUAD_IO_FAST) ||
-	    (flash->write_cmd == CMD_QUAD_PAGE_PROGRAM)) {
+	if ((flash->read_cmd == CMD_RD_QUAD) ||
+	    (flash->read_cmd == CMD_RD_QUAD_IO) ||
+	    (flash->write_cmd == CMD_WR_QUAD)) {
 		if (spi_flash_set_qeb(flash, idcode[0])) {
 			debug("SF: Fail to set QEB for %02x\n", idcode[0]);
 			return NULL;

Signature · 1 line

1.8.3

Marek Vasut

2014-01-18 20:36 UTC #18 lore raw

PATCH 4/6 sf: Update read/write command macros

Quoted 6 lines · Jagannadha Sutradharudu Teki

- Used readable names for read/write command macros
- Added comments for the same
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>

Does this patch have any impact other than making the code harder to understand ? :-(

What's the rationale for making the code more cryptic ?

Best regards,
Marek Vasut

Jagan Teki

2014-01-18 20:45 UTC #19 lore raw

PATCH 4/6 sf: Update read/write command macros

Quoted 5 lines · Marek Vasut

Quoted 3 lines · Jagannadha Sutradharudu Teki
- Used readable names for read/write command macros
- Added comments for the same

Quoted 8 lines

Quoted 3 lines
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Does this patch have any impact other than making the code harder to understand ? :-(
What's the rationale for making the code more cryptic ?

No issues I guess with the readability as each macro we can easily understand. like CMD_RD_QUAD --> command_read_quad

      CMD_WR_PAGE --> command_write_page_program

And this will minimize the macro length - good for in coding and more over description is added in drivers/mtd/spi/sf_internal.h anyway.

Signature · 2 lines

Thanks,
Jagan.

Detlev Zundel

2014-01-20 11:13 UTC #20 lore raw

PATCH 4/6 sf: Update read/write command macros

Hi Jagan,

Quoted 17 lines · Marek Vasut

Quoted 10 lines
Quoted 6 lines · Jagannadha Sutradharudu Teki
- Used readable names for read/write command macros
- Added comments for the same
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Quoted 5 lines
Does this patch have any impact other than making the code harder to understand
? :-(
What's the rationale for making the code more cryptic ?

Quoted 7 lines

No issues I guess with the readability as each macro we can easily understand.
like CMD_RD_QUAD --> command_read_quad
      CMD_WR_PAGE --> command_write_page_program
And this will minimize the macro length - good for in coding and more over description is added in drivers/mtd/spi/sf_internal.h anyway.

Again I agree with Marek that readability of code is more important than saving a few characters while coding. This is especially true as editors can support you in coding (Emacs has lots of packages to help here for example).

Cheers
Detlev

Signature · 7 lines

(7)  It is always something
(7a) (corollary). Good, Fast, Cheap: Pick any two (you can't have all three).
                                   -- The Twelve Networking Truths (RFC 1925)
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

Jagan Teki

2014-01-20 11:46 UTC #21 lore raw

PATCH 4/6 sf: Update read/write command macros

Quoted 24 lines · Detlev Zundel

Hi Jagan,
Quoted 22 lines · Marek Vasut
Quoted 10 lines
Quoted 6 lines · Jagannadha Sutradharudu Teki
- Used readable names for read/write command macros
- Added comments for the same
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Quoted 5 lines
Does this patch have any impact other than making the code harder to understand
? :-(
What's the rationale for making the code more cryptic ?
No issues I guess with the readability as each macro we can easily understand.
like CMD_RD_QUAD --> command_read_quad
      CMD_WR_PAGE --> command_write_page_program

Quoted 7 lines

Quoted 2 lines
And this will minimize the macro length - good for in coding and more over description is added in drivers/mtd/spi/sf_internal.h anyway.
Again I agree with Marek that readability of code is more important than saving a few characters while coding. This is especially true as editors can support you in coding (Emacs has lots of packages to help here for example).

I don't think nothing much gone the readability with these updated:
CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like easy to understand. and anyway I have added comments for full name as well.

Few of the flashes can be call this as array fast read and fewer call this as fast read
and few more call this as high frequency read. CMD_RD_FAST will suits all these names.

Comments please!

Signature · 2 lines

Thanks,
Jagan.

Marek Vasut

2014-01-20 13:06 UTC #22 lore raw

PATCH 4/6 sf: Update read/write command macros

Quoted 30 lines · Jagan Teki

Quoted 29 lines · Detlev Zundel
Hi Jagan,
Quoted 26 lines · Marek Vasut
Quoted 11 lines
On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha Sutradharudu Teki
Quoted 6 lines · wrote:
- Used readable names for read/write command macros
- Added comments for the same
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Quoted 5 lines
Does this patch have any impact other than making the code harder to understand
? :-(
What's the rationale for making the code more cryptic ?
No issues I guess with the readability as each macro we can easily understand.
like CMD_RD_QUAD --> command_read_quad
      CMD_WR_PAGE --> command_write_page_program
And this will minimize the macro length - good for in coding and more over description is added in drivers/mtd/spi/sf_internal.h anyway.

Quoted 8 lines

Quoted 4 lines
Again I agree with Marek that readability of code is more important than saving a few characters while coding. This is especially true as editors can support you in coding (Emacs has lots of packages to help here for example).
I don't think nothing much gone the readability with these updated:
CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like easy to understand. and anyway I have added comments for full name as well.

CMD_READ_ARRAY_FAST contains all the necessary bits for me to understand what the macro means. CMD_RD_FAST does not. I fail to see the rationale behind changing the names.

Quoted 6 lines

Few of the flashes can be call this as array fast read and fewer call this as fast read
and few more call this as high frequency read. CMD_RD_FAST will suits all these names.
Comments please!

If you want to align the names with anything, align then with linux's m25p80.c driver . But I see this change as moot and confusing, sorry.

Best regards,
Marek Vasut

Jagan Teki

2014-01-20 13:10 UTC #23 lore raw

PATCH 4/6 sf: Update read/write command macros

Quoted 49 lines · Marek Vasut

Quoted 30 lines · Jagan Teki
Quoted 29 lines · Detlev Zundel
Hi Jagan,
Quoted 26 lines · Marek Vasut
Quoted 11 lines
On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha Sutradharudu Teki
Quoted 6 lines · wrote:
- Used readable names for read/write command macros
- Added comments for the same
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Quoted 5 lines
Does this patch have any impact other than making the code harder to understand
? :-(
What's the rationale for making the code more cryptic ?
No issues I guess with the readability as each macro we can easily understand.
like CMD_RD_QUAD --> command_read_quad
      CMD_WR_PAGE --> command_write_page_program
And this will minimize the macro length - good for in coding and more over description is added in drivers/mtd/spi/sf_internal.h anyway.
Quoted 8 lines
Quoted 4 lines
Again I agree with Marek that readability of code is more important than saving a few characters while coding. This is especially true as editors can support you in coding (Emacs has lots of packages to help here for example).
I don't think nothing much gone the readability with these updated:
CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like easy to understand. and anyway I have added comments for full name as well.
CMD_READ_ARRAY_FAST contains all the necessary bits for me to understand what the macro means. CMD_RD_FAST does not. I fail to see the rationale behind changing the names.
Quoted 5 lines
Few of the flashes can be call this as array fast read and fewer call this as fast read
and few more call this as high frequency read. CMD_RD_FAST will suits all these names.

Quoted 4 lines

Quoted 1 line
Comments please!
If you want to align the names with anything, align then with linux's m25p80.c driver . But I see this change as moot and confusing, sorry.

No issues, we can skip these as of now for this release.!

Signature · 2 lines

Thanks,
Jagan.

Jagan Teki

2014-01-20 13:13 UTC #24 lore raw

PATCH 4/6 sf: Update read/write command macros

Quoted 50 lines · Jagan Teki

Quoted 49 lines · Marek Vasut
Quoted 30 lines · Jagan Teki
Quoted 29 lines · Detlev Zundel
Hi Jagan,
Quoted 26 lines · Marek Vasut
Quoted 11 lines
On Saturday, January 18, 2014 at 09:06:31 PM, Jagannadha Sutradharudu Teki
Quoted 6 lines · wrote:
- Used readable names for read/write command macros
- Added comments for the same
Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Quoted 5 lines
Does this patch have any impact other than making the code harder to understand
? :-(
What's the rationale for making the code more cryptic ?
No issues I guess with the readability as each macro we can easily understand.
like CMD_RD_QUAD --> command_read_quad
      CMD_WR_PAGE --> command_write_page_program
And this will minimize the macro length - good for in coding and more over description is added in drivers/mtd/spi/sf_internal.h anyway.
Quoted 8 lines
Quoted 4 lines
Again I agree with Marek that readability of code is more important than saving a few characters while coding. This is especially true as editors can support you in coding (Emacs has lots of packages to help here for example).
I don't think nothing much gone the readability with these updated:
CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like easy to understand. and anyway I have added comments for full name as well.
CMD_READ_ARRAY_FAST contains all the necessary bits for me to understand what the macro means. CMD_RD_FAST does not. I fail to see the rationale behind changing the names.
Quoted 5 lines
Few of the flashes can be call this as array fast read and fewer call this as fast read
and few more call this as high frequency read. CMD_RD_FAST will suits all these names.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Comments please!
If you want to align the names with anything, align then with linux's m25p80.c driver . But I see this change as moot and confusing, sorry.
No issues, we can skip these as of now for this release.!

Just fyi: if we need a reference of m25p80.c the name as OPCODE_NORM_READ which is similar to what i did :)

Signature · 2 lines

Thanks,
Jagan.

Detlev Zundel

2014-01-20 14:33 UTC #25 lore raw

PATCH 4/6 sf: Update read/write command macros

In reply to Jagan Teki

Hi Jagan,

[...]

Quoted 3 lines

I don't think nothing much gone the readability with these updated:
CMD_READ_ARRAY_FAST has updated CMD_RD_FAST and it seems like easy to understand. and anyway I have added comments for full name as well.

Comments in a far away place cannot compensate for self-explaining constructs at the location where they are used. Stating that a constants needs comments to explain it is actually a good sign that its name is not chosen carefully enough.

Really, naming constants and variables for readable and maintainable code is a much harder problem than it looks (cf. my signature) and unfortunately not easily measurable. But I assure you that good names can make a world of a difference. That's why Marek and me are so passionate about this seemingly "trivial" change.

Quoted 5 lines

Few of the flashes can be call this as array fast read and fewer call this as fast read and few more call this as high frequency read. CMD_RD_FAST will suits all these names.
Comments please!

When we change code, we don't do this for the sake of changing it, but in order to improve one aspect of it, i.e. the performance, the maintainability or the features. When everything "stays the same", we are even _opposed_ to a change because there is nothing to outweigh the effort to adjusting to the new things.

To summarize - we need proof that a change _improves_ something. Showing that we "do not loose something" is clearly not enough.

Now in this specific case, we have multiple people voicing the concern that the renaming looses vital information, thus effectively making reading and maintining the code harder. On the other hand even you agree that "something" although "not much" will be gone after the rename. So taking this into account we have only "saving of a few keystorkes" on the positive side and substantial degradation on readability and maintainability on the negative side.

Best wishes
Detlev

Signature · 6 lines

There are two hard things in computer science: cache invalidation,
naming things, and off-by-one errors.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

Jagannadha Sutradharudu Teki

author

2014-01-18 20:06 UTC #26 lore raw

PATCH 5/6 sf: Minor macro cleanups

In reply to a message not in the archive

Renamed SPI_FLASH with SF in few places in sf code.

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

---
 drivers/mtd/spi/sf_internal.h | 20 ++++++++++----------
 drivers/mtd/spi/sf_ops.c      | 23 +++++++++++------------
 drivers/mtd/spi/sf_probe.c    | 19 +++++++++----------
 3 files changed, 30 insertions(+), 32 deletions(-)

drivers/mtd/spi/sf_internal.h +10 ~~-10~~

index 6b6fa22..fba7218 100644
@@ -10,15 +10,15 @@
 #ifndef _SF_INTERNAL_H_
 #define _SF_INTERNAL_H_
 
-#define SPI_FLASH_3B_ADDR_LEN		3
-#define SPI_FLASH_CMD_LEN		(1 + SPI_FLASH_3B_ADDR_LEN)
-#define SPI_FLASH_16MB_BOUN		0x1000000
+#define SF_3BYTE_ADDR			3
+#define SF_CMD_LEN			(1 + SF_3BYTE_ADDR)
+#define SF_16MB_BOUN			0x1000000
 
 /* CFI Manufacture ID's */
-#define SPI_FLASH_CFI_MFR_SPANSION	0x01
-#define SPI_FLASH_CFI_MFR_STMICRO	0x20
-#define SPI_FLASH_CFI_MFR_MACRONIX	0xc2
-#define SPI_FLASH_CFI_MFR_WINBOND	0xef
+#define SF_CFI_MFR_SPANSION		0x01
+#define SF_CFI_MFR_STMICRO		0x20
+#define SF_CFI_MFR_MACRONIX		0xc2
+#define SF_CFI_MFR_WINBOND		0xef
 
 /* Erase commands */
 #define CMD_ERASE_4K			0x20

@@ -61,9 +61,9 @@
 #define STATUS_PEC			(1 << 7)
 
 /* Flash timeout values */
-#define SPI_FLASH_PROG_TIMEOUT		(2 * CONFIG_SYS_HZ)
-#define SPI_FLASH_PAGE_ERASE_TIMEOUT	(5 * CONFIG_SYS_HZ)
-#define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONFIG_SYS_HZ)
+#define SF_PROG_TIMEOUT		(2 * CONFIG_SYS_HZ)
+#define SF_PAGE_ERASE_TIMEOUT		(5 * CONFIG_SYS_HZ)
+#define SF_SECTOR_ERASE_TIMEOUT	(10 * CONFIG_SYS_HZ)
 
 /* SST specific */
 #ifdef CONFIG_SPI_FLASH_SST

drivers/mtd/spi/sf_ops.c +11 ~~-12~~

index 6cbbfe3..02b12c1 100644
@@ -119,7 +119,7 @@ static int spi_flash_bank(struct spi_flash *flash, u32 offset)
 	u8 bank_sel;
 	int ret;
 
-	bank_sel = offset / (SPI_FLASH_16MB_BOUN << flash->shift);
+	bank_sel = offset / (SF_16MB_BOUN << flash->shift);
 
 	ret = spi_flash_cmd_bankaddr_write(flash, bank_sel);
 	if (ret) {

@@ -207,11 +207,11 @@ int spi_flash_write_common(struct spi_flash *flash, const u8 *cmd,
 		size_t cmd_len, const void *buf, size_t buf_len)
 {
 	struct spi_slave *spi = flash->spi;
-	unsigned long timeout = SPI_FLASH_PROG_TIMEOUT;
+	unsigned long timeout = SF_PROG_TIMEOUT;
 	int ret;
 
 	if (buf == NULL)
-		timeout = SPI_FLASH_PAGE_ERASE_TIMEOUT;
+		timeout = SF_PAGE_ERASE_TIMEOUT;
 
 	ret = spi_claim_bus(flash->spi);
 	if (ret) {

@@ -233,9 +233,8 @@ int spi_flash_write_common(struct spi_flash *flash, const u8 *cmd,
 
 	ret = spi_flash_cmd_wait_ready(flash, timeout);
 	if (ret < 0) {
-		debug("SF: write %s timed out\n",
-		      timeout == SPI_FLASH_PROG_TIMEOUT ?
-			"program" : "page erase");
+		debug("SF: write %s timed out\n", buf == NULL ?
+			"page erase" : "program");
 		return ret;
 	}

@@ -247,7 +246,7 @@ int spi_flash_write_common(struct spi_flash *flash, const u8 *cmd,
 int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
 {
 	u32 erase_size, erase_addr;
-	u8 cmd[SPI_FLASH_CMD_LEN];
+	u8 cmd[SF_CMD_LEN];
 	int ret = -1;
 
 	erase_size = flash->erase_size;

@@ -293,7 +292,7 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
 	unsigned long byte_addr, page_size;
 	u32 write_addr;
 	size_t chunk_len, actual;
-	u8 cmd[SPI_FLASH_CMD_LEN];
+	u8 cmd[SF_CMD_LEN];
 	int ret = -1;
 
 	page_size = flash->page_size;

@@ -380,7 +379,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 		return 0;
 	}
 
-	cmdsz = SPI_FLASH_CMD_LEN + flash->dummy_byte;
+	cmdsz = SF_CMD_LEN + flash->dummy_byte;
 	cmd = calloc(1, cmdsz);
 	if (!cmd) {
 		printf("SF: Failed to allocate cmd\n");

@@ -400,7 +399,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 		if (bank_sel < 0)
 			return ret;
 #endif
-		remain_len = ((SPI_FLASH_16MB_BOUN << flash->shift) *
+		remain_len = ((SF_16MB_BOUN << flash->shift) *
 				(bank_sel + 1)) - offset;
 		if (len < remain_len)
 			read_len = len;

@@ -445,7 +444,7 @@ static int sst_byte_write(struct spi_flash *flash, u32 offset, const void *buf)
 	if (ret)
 		return ret;
 
-	return spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
+	return spi_flash_cmd_wait_ready(flash, SF_PROG_TIMEOUT);
 }
 
 int sst_write_wp(struct spi_flash *flash, u32 offset, size_t len,

@@ -492,7 +491,7 @@ int sst_write_wp(struct spi_flash *flash, u32 offset, size_t len,
 			break;
 		}
 
-		ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
+		ret = spi_flash_cmd_wait_ready(flash, SF_PROG_TIMEOUT);
 		if (ret)
 			break;

drivers/mtd/spi/sf_probe.c +9 ~~-10~~

index 7ba0605..8f92333 100644
@@ -67,16 +67,16 @@ static int spi_flash_set_qeb(struct spi_flash *flash, u8 idcode0)
 {
 	switch (idcode0) {
 #ifdef CONFIG_SPI_FLASH_MACRONIX
-	case SPI_FLASH_CFI_MFR_MACRONIX:
+	case SF_CFI_MFR_MACRONIX:
 		return spi_flash_set_qeb_mxic(flash);
 #endif
 #if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
-	case SPI_FLASH_CFI_MFR_SPANSION:
-	case SPI_FLASH_CFI_MFR_WINBOND:
+	case SF_CFI_MFR_SPANSION:
+	case SF_CFI_MFR_WINBOND:
 		return spi_flash_set_qeb_winspan(flash);
 #endif
 #ifdef CONFIG_SPI_FLASH_STMICRO
-	case SPI_FLASH_CFI_MFR_STMICRO:
+	case SF_CFI_MFR_STMICRO:
 		debug("SF: QEB is volatile for %02x flash\n", idcode0);
 		return 0;
 #endif

@@ -190,11 +190,10 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 		break;
 	}
 
-	/* Not require to look for fastest only two write cmds yet */
+	/* Compute write command */
 	if (flash->spi->mode_bits & SPI_TX_QPP && params->flags & WR_QPP)
-		flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
+		flash->write_cmd = CMD_WR_QUAD;
 	else
-		/* Go for default supported write cmd */
 		flash->write_cmd = CMD_WR_PAGE;
 
 	/* Set the quad enable bit - only for quad commands */

@@ -217,7 +216,7 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	/* Configure the BAR - discover bank cmds and read current bank */
 #ifdef CONFIG_SPI_FLASH_BAR
 	u8 curr_bank = 0;
-	if (flash->size > SPI_FLASH_16MB_BOUN) {
+	if (flash->size > SF_16MB_BOUN) {
 		flash->bank_read_cmd = (idcode[0] == 0x01) ?
 					CMD_BANKADDR_BRRD : CMD_EXTNADDR_RDEAR;
 		flash->bank_write_cmd = (idcode[0] == 0x01) ?

@@ -325,9 +324,9 @@ static struct spi_flash *spi_flash_probe_slave(struct spi_slave *spi)
 #endif
 #ifndef CONFIG_SPI_FLASH_BAR
 	if (((flash->dual_flash == SF_SINGLE) &&
-	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
+	     (flash->size > SF_16MB_BOUN)) ||
 	     ((flash->dual_flash > SF_SINGLE) &&
-	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
+	     (flash->size > SF_16MB_BOUN << 1))) {
 		puts("SF: Warning - Only lower 16MiB accessible,");
 		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
 	}

Signature · 1 line

1.8.3

Jagannadha Sutradharudu Teki

author

2014-01-18 20:06 UTC #27 lore raw

PATCH 6/6 sf: Update bank configuration

In reply to a message not in the archive

Updated bank configuration code to more readable.

Signed-off-by Jagannadha Sutradharudu Teki <[email protected]>

---
 drivers/mtd/spi/sf_probe.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

drivers/mtd/spi/sf_probe.c +12 ~~-10~~

index 8f92333..06f99da 100644
@@ -215,21 +215,23 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 
 	/* Configure the BAR - discover bank cmds and read current bank */
 #ifdef CONFIG_SPI_FLASH_BAR
-	u8 curr_bank = 0;
+	flash->bank_curr = 0;
 	if (flash->size > SF_16MB_BOUN) {
-		flash->bank_read_cmd = (idcode[0] == 0x01) ?
-					CMD_BANKADDR_BRRD : CMD_EXTNADDR_RDEAR;
-		flash->bank_write_cmd = (idcode[0] == 0x01) ?
-					CMD_BANKADDR_BRWR : CMD_EXTNADDR_WREAR;
+		switch (idcode[0]) {
+		case SF_CFI_MFR_SPANSION:
+			flash->bank_read_cmd = CMD_BANKADDR_BRRD;
+			flash->bank_write_cmd = CMD_BANKADDR_BRWR;
+			break;
+		default:
+			flash->bank_read_cmd = CMD_EXTNADDR_RDEAR;
+			flash->bank_write_cmd = CMD_EXTNADDR_WREAR;
+		}
 
 		if (spi_flash_read_common(flash, &flash->bank_read_cmd, 1,
-					  &curr_bank, 1)) {
-			debug("SF: fail to read bank addr register\n");
+					&flash->bank_curr, 1)) {
+			debug("SF: Fail to read bank addr register\n");
 			return NULL;
 		}
-		flash->bank_curr = curr_bank;
-	} else {
-		flash->bank_curr = curr_bank;
 	}
 #endif

Signature · 1 line

1.8.3