cfi_flash: use specific width types for cword

Message ID 1445615451-4471-1-git-send-email-ryan.harkin@linaro.org
State Accepted
Commit 622b95274e4d699f3c713c325e958565312625ad
Headers show

Commit Message

Ryan Harkin Oct. 23, 2015, 3:50 p.m.
This patch changes the cword union to use specific length types that are
architecture indepented.

This patch also renames the members of the cword union to represent
their usage, i.e.:

    c  -> w8
    s  -> w16
    l  -> w32
    ll -> w64

Where "w" stands for "width" in bits.

I discovered this problem when enabling CFI flash on vexpress64.
cword.l was an unsigned long int, but it was intended to be 32 bits wide.
Unfortunately, it's 64-bits wide on a 64-bit system, meaning that a
64-bit system fails when attempting to use 32-bit wide CFI flash parts.

Similar problems also existed with the other cword sizes.

Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>

---
 drivers/mtd/cfi_flash.c | 82 ++++++++++++++++++++++++-------------------------
 include/mtd/cfi_flash.h |  8 ++---
 2 files changed, 45 insertions(+), 45 deletions(-)

-- 
2.1.4

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Comments

Linus Walleij Oct. 27, 2015, 10:31 a.m. | #1
On Fri, Oct 23, 2015 at 5:50 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote:

> This patch changes the cword union to use specific length types that are

> architecture indepented.

>

> This patch also renames the members of the cword union to represent

> their usage, i.e.:

>

>     c  -> w8

>     s  -> w16

>     l  -> w32

>     ll -> w64

>

> Where "w" stands for "width" in bits.

>

> I discovered this problem when enabling CFI flash on vexpress64.

> cword.l was an unsigned long int, but it was intended to be 32 bits wide.

> Unfortunately, it's 64-bits wide on a 64-bit system, meaning that a

> 64-bit system fails when attempting to use 32-bit wide CFI flash parts.

>

> Similar problems also existed with the other cword sizes.

>

> Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>


Very nice patch.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>


Yours,
Linus Walleij
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Patch

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 50983b8..fc7a878 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -335,34 +335,34 @@  void flash_write_cmd (flash_info_t * info, flash_sect_t sect,
 	switch (info->portwidth) {
 	case FLASH_CFI_8BIT:
 		debug ("fwc addr %p cmd %x %x 8bit x %d bit\n", addr, cmd,
-		       cword.c, info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
-		flash_write8(cword.c, addr);
+		       cword.w8, info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
+		flash_write8(cword.w8, addr);
 		break;
 	case FLASH_CFI_16BIT:
 		debug ("fwc addr %p cmd %x %4.4x 16bit x %d bit\n", addr,
-		       cmd, cword.w,
+		       cmd, cword.w16,
 		       info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
-		flash_write16(cword.w, addr);
+		flash_write16(cword.w16, addr);
 		break;
 	case FLASH_CFI_32BIT:
-		debug ("fwc addr %p cmd %x %8.8lx 32bit x %d bit\n", addr,
-		       cmd, cword.l,
+		debug ("fwc addr %p cmd %x %8.8x 32bit x %d bit\n", addr,
+		       cmd, cword.w32,
 		       info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
-		flash_write32(cword.l, addr);
+		flash_write32(cword.w32, addr);
 		break;
 	case FLASH_CFI_64BIT:
 #ifdef DEBUG
 		{
 			char str[20];
 
-			print_longlong (str, cword.ll);
+			print_longlong (str, cword.w64);
 
 			debug ("fwrite addr %p cmd %x %s 64 bit x %d bit\n",
 			       addr, cmd, str,
 			       info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
 		}
 #endif
-		flash_write64(cword.ll, addr);
+		flash_write64(cword.w64, addr);
 		break;
 	}
 
@@ -393,16 +393,16 @@  static int flash_isequal (flash_info_t * info, flash_sect_t sect,
 	debug ("is= cmd %x(%c) addr %p ", cmd, cmd, addr);
 	switch (info->portwidth) {
 	case FLASH_CFI_8BIT:
-		debug ("is= %x %x\n", flash_read8(addr), cword.c);
-		retval = (flash_read8(addr) == cword.c);
+		debug ("is= %x %x\n", flash_read8(addr), cword.w8);
+		retval = (flash_read8(addr) == cword.w8);
 		break;
 	case FLASH_CFI_16BIT:
-		debug ("is= %4.4x %4.4x\n", flash_read16(addr), cword.w);
-		retval = (flash_read16(addr) == cword.w);
+		debug ("is= %4.4x %4.4x\n", flash_read16(addr), cword.w16);
+		retval = (flash_read16(addr) == cword.w16);
 		break;
 	case FLASH_CFI_32BIT:
-		debug ("is= %8.8x %8.8lx\n", flash_read32(addr), cword.l);
-		retval = (flash_read32(addr) == cword.l);
+		debug ("is= %8.8x %8.8x\n", flash_read32(addr), cword.w32);
+		retval = (flash_read32(addr) == cword.w32);
 		break;
 	case FLASH_CFI_64BIT:
 #ifdef DEBUG
@@ -411,11 +411,11 @@  static int flash_isequal (flash_info_t * info, flash_sect_t sect,
 			char str2[20];
 
 			print_longlong (str1, flash_read64(addr));
-			print_longlong (str2, cword.ll);
+			print_longlong (str2, cword.w64);
 			debug ("is= %s %s\n", str1, str2);
 		}
 #endif
-		retval = (flash_read64(addr) == cword.ll);
+		retval = (flash_read64(addr) == cword.w64);
 		break;
 	default:
 		retval = 0;
@@ -439,16 +439,16 @@  static int flash_isset (flash_info_t * info, flash_sect_t sect,
 	flash_make_cmd (info, cmd, &cword);
 	switch (info->portwidth) {
 	case FLASH_CFI_8BIT:
-		retval = ((flash_read8(addr) & cword.c) == cword.c);
+		retval = ((flash_read8(addr) & cword.w8) == cword.w8);
 		break;
 	case FLASH_CFI_16BIT:
-		retval = ((flash_read16(addr) & cword.w) == cword.w);
+		retval = ((flash_read16(addr) & cword.w16) == cword.w16);
 		break;
 	case FLASH_CFI_32BIT:
-		retval = ((flash_read32(addr) & cword.l) == cword.l);
+		retval = ((flash_read32(addr) & cword.w32) == cword.w32);
 		break;
 	case FLASH_CFI_64BIT:
-		retval = ((flash_read64(addr) & cword.ll) == cword.ll);
+		retval = ((flash_read64(addr) & cword.w64) == cword.w64);
 		break;
 	default:
 		retval = 0;
@@ -680,33 +680,33 @@  static void flash_add_byte (flash_info_t * info, cfiword_t * cword, uchar c)
 
 	switch (info->portwidth) {
 	case FLASH_CFI_8BIT:
-		cword->c = c;
+		cword->w8 = c;
 		break;
 	case FLASH_CFI_16BIT:
 #if defined(__LITTLE_ENDIAN) && !defined(CONFIG_SYS_WRITE_SWAPPED_DATA)
 		w = c;
 		w <<= 8;
-		cword->w = (cword->w >> 8) | w;
+		cword->w16 = (cword->w16 >> 8) | w;
 #else
-		cword->w = (cword->w << 8) | c;
+		cword->w16 = (cword->w16 << 8) | c;
 #endif
 		break;
 	case FLASH_CFI_32BIT:
 #if defined(__LITTLE_ENDIAN) && !defined(CONFIG_SYS_WRITE_SWAPPED_DATA)
 		l = c;
 		l <<= 24;
-		cword->l = (cword->l >> 8) | l;
+		cword->w32 = (cword->w32 >> 8) | l;
 #else
-		cword->l = (cword->l << 8) | c;
+		cword->w32 = (cword->w32 << 8) | c;
 #endif
 		break;
 	case FLASH_CFI_64BIT:
 #if defined(__LITTLE_ENDIAN) && !defined(CONFIG_SYS_WRITE_SWAPPED_DATA)
 		ll = c;
 		ll <<= 56;
-		cword->ll = (cword->ll >> 8) | ll;
+		cword->w64 = (cword->w64 >> 8) | ll;
 #else
-		cword->ll = (cword->ll << 8) | c;
+		cword->w64 = (cword->w64 << 8) | c;
 #endif
 		break;
 	}
@@ -753,16 +753,16 @@  static int flash_write_cfiword (flash_info_t * info, ulong dest,
 	/* Check if Flash is (sufficiently) erased */
 	switch (info->portwidth) {
 	case FLASH_CFI_8BIT:
-		flag = ((flash_read8(dstaddr) & cword.c) == cword.c);
+		flag = ((flash_read8(dstaddr) & cword.w8) == cword.w8);
 		break;
 	case FLASH_CFI_16BIT:
-		flag = ((flash_read16(dstaddr) & cword.w) == cword.w);
+		flag = ((flash_read16(dstaddr) & cword.w16) == cword.w16);
 		break;
 	case FLASH_CFI_32BIT:
-		flag = ((flash_read32(dstaddr) & cword.l) == cword.l);
+		flag = ((flash_read32(dstaddr) & cword.w32) == cword.w32);
 		break;
 	case FLASH_CFI_64BIT:
-		flag = ((flash_read64(dstaddr) & cword.ll) == cword.ll);
+		flag = ((flash_read64(dstaddr) & cword.w64) == cword.w64);
 		break;
 	default:
 		flag = 0;
@@ -800,16 +800,16 @@  static int flash_write_cfiword (flash_info_t * info, ulong dest,
 
 	switch (info->portwidth) {
 	case FLASH_CFI_8BIT:
-		flash_write8(cword.c, dstaddr);
+		flash_write8(cword.w8, dstaddr);
 		break;
 	case FLASH_CFI_16BIT:
-		flash_write16(cword.w, dstaddr);
+		flash_write16(cword.w16, dstaddr);
 		break;
 	case FLASH_CFI_32BIT:
-		flash_write32(cword.l, dstaddr);
+		flash_write32(cword.w32, dstaddr);
 		break;
 	case FLASH_CFI_64BIT:
-		flash_write64(cword.ll, dstaddr);
+		flash_write64(cword.w64, dstaddr);
 		break;
 	}
 
@@ -1115,7 +1115,7 @@  int flash_erase (flash_info_t * info, int s_first, int s_last)
 			if (use_flash_status_poll(info)) {
 				cfiword_t cword;
 				void *dest;
-				cword.ll = 0xffffffffffffffffULL;
+				cword.w64 = 0xffffffffffffffffULL;
 				dest = flash_map(info, sect, 0);
 				st = flash_status_poll(info, &cword, dest,
 						       info->erase_blk_tout, "erase");
@@ -1305,7 +1305,7 @@  int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 
 	/* handle unaligned start */
 	if ((aln = addr - wp) != 0) {
-		cword.l = 0;
+		cword.w32 = 0;
 		p = (uchar *)wp;
 		for (i = 0; i < aln; ++i)
 			flash_add_byte (info, &cword, flash_read8(p + i));
@@ -1332,7 +1332,7 @@  int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 	while (cnt >= info->portwidth) {
 		/* prohibit buffer write when buffer_size is 1 */
 		if (info->buffer_size == 1) {
-			cword.l = 0;
+			cword.w32 = 0;
 			for (i = 0; i < info->portwidth; i++)
 				flash_add_byte (info, &cword, *src++);
 			if ((rc = flash_write_cfiword (info, wp, cword)) != 0)
@@ -1359,7 +1359,7 @@  int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 	}
 #else
 	while (cnt >= info->portwidth) {
-		cword.l = 0;
+		cword.w32 = 0;
 		for (i = 0; i < info->portwidth; i++) {
 			flash_add_byte (info, &cword, *src++);
 		}
@@ -1381,7 +1381,7 @@  int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
 	/*
 	 * handle unaligned tail bytes
 	 */
-	cword.l = 0;
+	cword.w32 = 0;
 	p = (uchar *)wp;
 	for (i = 0; (i < info->portwidth) && (cnt > 0); ++i) {
 		flash_add_byte (info, &cword, *src++);
diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
index 048b477..52572b9 100644
--- a/include/mtd/cfi_flash.h
+++ b/include/mtd/cfi_flash.h
@@ -105,10 +105,10 @@ 
 #define NUM_ERASE_REGIONS	4 /* max. number of erase regions */
 
 typedef union {
-	unsigned char c;
-	unsigned short w;
-	unsigned long l;
-	unsigned long long ll;
+	u8 w8;
+	u16 w16;
+	u32 w32;
+	u64 w64;
 } cfiword_t;
 
 /* CFI standard query structure */