All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {}
       [not found] <1389969707-3781-1-git-send-email-jaganna@xilinx.com>
@ 2014-01-17 14:41 ` Jagannadha Sutradharudu Teki
  2014-01-17 16:31   ` Marek Vasut
  2014-01-17 14:41 ` [U-Boot] [PATCH 3/3] sf: Use shortcut names Jagannadha Sutradharudu Teki
  1 sibling, 1 reply; 10+ messages in thread
From: Jagannadha Sutradharudu Teki @ 2014-01-17 14:41 UTC (permalink / raw)
  To: u-boot

Combined spi flash stuff as minimum as possible.

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

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 3/3] sf: Use shortcut names
       [not found] <1389969707-3781-1-git-send-email-jaganna@xilinx.com>
  2014-01-17 14:41 ` [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {} Jagannadha Sutradharudu Teki
@ 2014-01-17 14:41 ` Jagannadha Sutradharudu Teki
  2014-01-17 16:31   ` Marek Vasut
  1 sibling, 1 reply; 10+ messages in thread
From: Jagannadha Sutradharudu Teki @ 2014-01-17 14:41 UTC (permalink / raw)
  To: u-boot

- SPI_FLASH -> SF
- ARRAY_SLOW -> AS
- ARRAY_FAST -> AF
- DUAL_OUTPUT_FAST -> DOF
- DUAL_IO_FAST - DIOF
- QUAD_OUTPUT_FAST - QOF
- QUAD_IO_FAST - QIOF

Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
Cc: Marek Vasut <marex@denx.de>
---
 drivers/mtd/spi/sf_internal.h | 44 +++++++++++++++++++++----------------------
 drivers/mtd/spi/sf_ops.c      | 25 ++++++++++++------------
 drivers/mtd/spi/sf_probe.c    | 44 +++++++++++++++++++++----------------------
 include/spi_flash.h           | 14 +++++++-------
 4 files changed, 63 insertions(+), 64 deletions(-)

diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
index 6bcd522..f959262 100644
--- a/drivers/mtd/spi/sf_internal.h
+++ b/drivers/mtd/spi/sf_internal.h
@@ -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_INST_3BADDR			3
+#define SF_CMD_LEN			(1 + SF_INST_3BADDR)
+#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
@@ -28,22 +28,22 @@
 
 /* Write commands */
 #define CMD_WRITE_STATUS		0x01
-#define CMD_PAGE_PROGRAM		0x02
+#define CMD_WRITE_PP			0x02
 #define CMD_WRITE_DISABLE		0x04
 #define CMD_READ_STATUS			0x05
-#define CMD_QUAD_PAGE_PROGRAM		0x32
+#define CMD_WRITE_QPP			0x32
 #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_READ_AS			0x03
+#define CMD_READ_AF			0x0b
+#define CMD_READ_DOF			0x3b
+#define CMD_READ_DIOF			0xbb
+#define CMD_READ_QOF			0x6b
+#define CMD_READ_QIOF			0xeb
 #define CMD_READ_ID			0x9f
 
 /* Bank addr access commands */
@@ -55,15 +55,15 @@
 #endif
 
 /* Common status */
-#define STATUS_WIP			(1 << 0)
-#define STATUS_QEB_WINSPAN		(1 << 1)
-#define STATUS_QEB_MXIC			(1 << 6)
-#define STATUS_PEC			(1 << 7)
+#define STATUS_WIP			1 << 0
+#define STATUS_QEB_WINSPAN		1 << 1
+#define STATUS_QEB_MXIC			1 << 6
+#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
diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
index 0d87e1b..e8e70bb 100644
--- a/drivers/mtd/spi/sf_ops.c
+++ b/drivers/mtd/spi/sf_ops.c
@@ -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 ?
+		      "program" : "page erase");
 		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);
 
 	cmd[0] = flash->read_cmd;
@@ -396,8 +395,8 @@ 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) *
-				(bank_sel + 1)) - offset;
+		remain_len = ((SF_16MB_BOUN << flash->shift) * (bank_sel + 1)) -
+				offset;
 		if (len < remain_len)
 			read_len = len;
 		else
@@ -441,7 +440,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,
@@ -488,7 +487,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;
 
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
index 003f635..ae11999 100644
--- a/drivers/mtd/spi/sf_probe.c
+++ b/drivers/mtd/spi/sf_probe.c
@@ -21,11 +21,11 @@ DECLARE_GLOBAL_DATA_PTR;
 
 /* Read commands array */
 static u8 spi_read_cmds_array[] = {
-	CMD_READ_ARRAY_SLOW,
-	CMD_READ_DUAL_OUTPUT_FAST,
-	CMD_READ_DUAL_IO_FAST,
-	CMD_READ_QUAD_OUTPUT_FAST,
-	CMD_READ_QUAD_IO_FAST,
+	CMD_READ_AS,
+	CMD_READ_DOF,
+	CMD_READ_DIOF,
+	CMD_READ_QOF,
+	CMD_READ_QIOF,
 };
 
 #ifdef CONFIG_SPI_FLASH_MACRONIX
@@ -76,16 +76,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
@@ -141,13 +141,13 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 		flash->dual_flash = SF_PARALLEL;
 #endif
 	/* Assign spi_flash ops */
+	flash->read = spi_flash_cmd_read_ops;
+	flash->erase = spi_flash_cmd_erase_ops;
 	flash->write = spi_flash_cmd_write_ops;
 #ifdef CONFIG_SPI_FLASH_SST
 	if (params->flags & SST_WP)
 		flash->write = sst_write_wp;
 #endif
-	flash->erase = spi_flash_cmd_erase_ops;
-	flash->read = spi_flash_cmd_read_ops;
 
 	/* Compute the flash size */
 	flash->shift = (flash->dual_flash & SF_PARALLEL) ? 1 : 0;
@@ -178,20 +178,20 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 		flash->read_cmd = cmd;
 	} else {
 		/* Go for default supported read cmd */
-		flash->read_cmd = CMD_READ_ARRAY_FAST;
+		flash->read_cmd = CMD_READ_AF;
 	}
 
 	/* Not require to look for fastest only two write cmds yet */
 	if (params->flags & WR_QPP && flash->spi->mode_bits & SPI_TX_QPP)
-		flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
+		flash->write_cmd = CMD_WRITE_QPP;
 	else
 		/* Go for default supported write cmd */
-		flash->write_cmd = CMD_PAGE_PROGRAM;
+		flash->write_cmd = CMD_WRITE_PP;
 
 	/* 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_READ_QOF) ||
+	    (flash->read_cmd == CMD_READ_QIOF) ||
+	    (flash->write_cmd == CMD_WRITE_QPP)) {
 		if (spi_flash_set_qeb(flash, idcode[0])) {
 			debug("SF: Fail to set QEB for %02x\n", idcode[0]);
 			return NULL;
@@ -207,10 +207,10 @@ static struct spi_flash *spi_flash_validate_params(struct spi_slave *spi,
 	 * data all go on single line irrespective of command.
 	 */
 	switch (flash->read_cmd) {
-	case CMD_READ_QUAD_IO_FAST:
+	case CMD_READ_QIOF:
 		flash->dummy_byte = 2;
 		break;
-	case CMD_READ_ARRAY_SLOW:
+	case CMD_READ_AS:
 		flash->dummy_byte = 0;
 		break;
 	default:
@@ -227,7 +227,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) ?
@@ -335,9 +335,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");
 	}
diff --git a/include/spi_flash.h b/include/spi_flash.h
index b4b2f9b..283c569 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -27,14 +27,14 @@
 
 /* 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,
+	SF_AS = 1 << 0,
+	SF_DOF = 1 << 1,
+	SF_DIOF = 1 << 2,
+	SF_QOF = 1 << 3,
+	SF_QIOF = 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_EXTN		SF_AS | SF_DOF | SF_DIOF
+#define RD_FULL		RD_EXTN | SF_QOF | SF_QIOF
 
 /* Dual SPI flash memories */
 enum spi_dual_flash {
-- 
1.8.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {}
  2014-01-17 14:41 ` [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {} Jagannadha Sutradharudu Teki
@ 2014-01-17 16:31   ` Marek Vasut
  2014-01-17 17:12     ` Jagan Teki
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2014-01-17 16:31 UTC (permalink / raw)
  To: u-boot

On Friday, January 17, 2014 at 03:41:46 PM, Jagannadha Sutradharudu Teki wrote:
> Combined spi flash stuff as minimum as possible.

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

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

Looks new and unrelated.

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

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

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

[...]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 3/3] sf: Use shortcut names
  2014-01-17 14:41 ` [U-Boot] [PATCH 3/3] sf: Use shortcut names Jagannadha Sutradharudu Teki
@ 2014-01-17 16:31   ` Marek Vasut
  2014-01-20 11:05     ` Detlev Zundel
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2014-01-17 16:31 UTC (permalink / raw)
  To: u-boot

On Friday, January 17, 2014 at 03:41:47 PM, Jagannadha Sutradharudu Teki wrote:
> - SPI_FLASH -> SF
> - ARRAY_SLOW -> AS
> - ARRAY_FAST -> AF
> - DUAL_OUTPUT_FAST -> DOF
> - DUAL_IO_FAST - DIOF
> - QUAD_OUTPUT_FAST - QOF
> - QUAD_IO_FAST - QIOF

Now this really makes the code impossible to understand :-(

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {}
  2014-01-17 16:31   ` Marek Vasut
@ 2014-01-17 17:12     ` Jagan Teki
  2014-01-17 18:29       ` Gerhard Sittig
  2014-01-18 16:59       ` Marek Vasut
  0 siblings, 2 replies; 10+ messages in thread
From: Jagan Teki @ 2014-01-17 17:12 UTC (permalink / raw)
  To: u-boot

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

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

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

Yes - will shrink once we plan for new approach.
But I'm unclear with new SPI-NOR.

-- 
Thanks,
Jagan.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {}
  2014-01-17 17:12     ` Jagan Teki
@ 2014-01-17 18:29       ` Gerhard Sittig
  2014-01-18 17:03         ` Marek Vasut
  2014-01-18 16:59       ` Marek Vasut
  1 sibling, 1 reply; 10+ messages in thread
From: Gerhard Sittig @ 2014-01-17 18:29 UTC (permalink / raw)
  To: u-boot

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

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

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

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

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


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {}
  2014-01-17 17:12     ` Jagan Teki
  2014-01-17 18:29       ` Gerhard Sittig
@ 2014-01-18 16:59       ` Marek Vasut
  1 sibling, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2014-01-18 16:59 UTC (permalink / raw)
  To: u-boot

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

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

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

It's pretty clear in it's own right, do not polute generic code with SPI NOR 
specific stuff .

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {}
  2014-01-17 18:29       ` Gerhard Sittig
@ 2014-01-18 17:03         ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2014-01-18 17:03 UTC (permalink / raw)
  To: u-boot

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

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

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

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

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

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

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

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 3/3] sf: Use shortcut names
  2014-01-17 16:31   ` Marek Vasut
@ 2014-01-20 11:05     ` Detlev Zundel
  2014-01-20 11:41       ` Jagan Teki
  0 siblings, 1 reply; 10+ messages in thread
From: Detlev Zundel @ 2014-01-20 11:05 UTC (permalink / raw)
  To: u-boot

Hi Jagan and Marek,

> On Friday, January 17, 2014 at 03:41:47 PM, Jagannadha Sutradharudu
> Teki wrote:
>> - SPI_FLASH -> SF
>> - ARRAY_SLOW -> AS
>> - ARRAY_FAST -> AF
>> - DUAL_OUTPUT_FAST -> DOF
>> - DUAL_IO_FAST - DIOF
>> - QUAD_OUTPUT_FAST - QOF
>> - QUAD_IO_FAST - QIOF
>
> Now this really makes the code impossible to understand :-(

I totally agree with Marek that this is the wrong way to go.  Doing a
change like this is a change to the worse as it removes understandable
constants and replaces them with adhoc abbreviations.

Please don't do that.

Thanks
  Detlev

-- 
Men are born ignorant, not stupid; they are made stupid by education.
					  --Bertrand Russell
--
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [U-Boot] [PATCH 3/3] sf: Use shortcut names
  2014-01-20 11:05     ` Detlev Zundel
@ 2014-01-20 11:41       ` Jagan Teki
  0 siblings, 0 replies; 10+ messages in thread
From: Jagan Teki @ 2014-01-20 11:41 UTC (permalink / raw)
  To: u-boot

On Mon, Jan 20, 2014 at 4:35 PM, Detlev Zundel <dzu@denx.de> wrote:
> Hi Jagan and Marek,
>
>> On Friday, January 17, 2014 at 03:41:47 PM, Jagannadha Sutradharudu
>> Teki wrote:
>>> - SPI_FLASH -> SF
>>> - ARRAY_SLOW -> AS
>>> - ARRAY_FAST -> AF
>>> - DUAL_OUTPUT_FAST -> DOF
>>> - DUAL_IO_FAST - DIOF
>>> - QUAD_OUTPUT_FAST - QOF
>>> - QUAD_IO_FAST - QIOF
>>
>> Now this really makes the code impossible to understand :-(
>
> I totally agree with Marek that this is the wrong way to go.  Doing a
> change like this is a change to the worse as it removes understandable
> constants and replaces them with adhoc abbreviations.
>
> Please don't do that.

Just ignore this these were updated in
[PATCH 4/6] sf: Update read/write command macros

-- 
Thanks,
Jagan.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-01-20 11:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1389969707-3781-1-git-send-email-jaganna@xilinx.com>
2014-01-17 14:41 ` [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {} Jagannadha Sutradharudu Teki
2014-01-17 16:31   ` Marek Vasut
2014-01-17 17:12     ` Jagan Teki
2014-01-17 18:29       ` Gerhard Sittig
2014-01-18 17:03         ` Marek Vasut
2014-01-18 16:59       ` Marek Vasut
2014-01-17 14:41 ` [U-Boot] [PATCH 3/3] sf: Use shortcut names Jagannadha Sutradharudu Teki
2014-01-17 16:31   ` Marek Vasut
2014-01-20 11:05     ` Detlev Zundel
2014-01-20 11:41       ` Jagan Teki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.