Patchwork [U-Boot] Fix CFI flash driver for 8-bit bus support

login
register
mail settings
Submitter Aaron Williams
Date April 2, 2011, 7:17 a.m.
Message ID <1301728621-27620-1-git-send-email-aaron.williams@caviumnetworks.com>
Download mbox | patch
Permalink /patch/89429/
State Changes Requested
Delegated to: Stefan Roese
Headers show

Comments

Aaron Williams - April 2, 2011, 7:17 a.m.
This patch corrects the addresses used when working with Spansion/AMD FLASH chips.
Addressing for 8 and 16 bits is almost identical except in the 16-bit case the
LSB of the address is always 0.  The confusion arose because the addresses 
in the datasheet for 16-bit mode are word addresses but this code assumed it was
byte addresses.

I have only been able to test this on our Octeon boards which use either an 8-bit 
or 16-bit bus.  I have not tested the case where there's an 8-bit part on a 16-bit
bus.

This patch also adds some delays as suggested by Spansion.

If a part can be both 8 and 16-bits, it forces it to work in 8-bit mode if an
8-bit bus is detected.

-Aaron Williams


Signed-off-by: Aaron Williams <aaron.williams@caviumnetworks.com>
---
 drivers/mtd/cfi_flash.c |   93 ++++++++++++++++++++++++++++++++++-------------
 include/mtd/cfi_flash.h |   40 ++++++++++----------
 2 files changed, 87 insertions(+), 46 deletions(-)
Albert ARIBAUD - April 2, 2011, 11:23 a.m.
Hi Aaron,

Le 02/04/2011 09:17, Aaron Williams a écrit :
> This patch corrects the addresses used when working with Spansion/AMD FLASH chips.
> Addressing for 8 and 16 bits is almost identical except in the 16-bit case the
> LSB of the address is always 0.  The confusion arose because the addresses
> in the datasheet for 16-bit mode are word addresses but this code assumed it was
> byte addresses.
>
> I have only been able to test this on our Octeon boards which use either an 8-bit
> or 16-bit bus.  I have not tested the case where there's an 8-bit part on a 16-bit
> bus.
>
> This patch also adds some delays as suggested by Spansion.
>
> If a part can be both 8 and 16-bits, it forces it to work in 8-bit mode if an
> 8-bit bus is detected.
>
> -Aaron Williams
>
>
> Signed-off-by: Aaron Williams<aaron.williams@caviumnetworks.com>

Comments related to testing and 'signatures' should not be part of the 
commit message and thus should go below the commit message delimiter.

> ---
>   drivers/mtd/cfi_flash.c |   93 ++++++++++++++++++++++++++++++++++-------------
>   include/mtd/cfi_flash.h |   40 ++++++++++----------
>   2 files changed, 87 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 0909fe7..eab1fbe 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -10,6 +10,9 @@
>    *
>    * Copyright (C) 2006
>    * Tolunay Orkun<listmember@orkun.us>
> + *

There is a space between the asterisk and end of line. You can check if 
your patch is whitespace-clean by applying it locally as if you were 
another u-boot user; 'git apply' will tell you where in the patch there 
are undue whitespace.

> + * Copyright (C) 2011 Cavium Networks, Inc.
> + * Aaron Williams<Aaron.Williams@caviumnetworks.com>
>    *
>    * See file CREDITS for list of people who contributed to this
>    * project.
> @@ -32,7 +35,6 @@
>    */
>
>   /* The DEBUG define must be before common to enable debugging */
> -/* #define DEBUG	*/
>
>   #include<common.h>
>   #include<asm/processor.h>
> @@ -209,9 +211,11 @@ unsigned long flash_sector_size(flash_info_t *info, flash_sect_t sect)
>   static inline void *
>   flash_map (flash_info_t * info, flash_sect_t sect, uint offset)
>   {
> -	unsigned int byte_offset = offset * info->portwidth;
> -

Whitespace (see 'git apply' comment above).

> -	return (void *)(info->start[sect] + byte_offset);
> +	unsigned int byte_offset = offset * info->portwidth / info->chipwidth;
> +	unsigned int addr = (info->start[sect] + byte_offset);
> +	unsigned int mask = 0xffffffff<<  (info->portwidth - 1);
> +	
> +	return (void *)(addr&  mask);
>   }
>
>   static inline void flash_unmap(flash_info_t *info, flash_sect_t sect,
> @@ -397,6 +401,8 @@ void flash_write_cmd (flash_info_t * info, flash_sect_t sect,
>   #endif
>   		flash_write64(cword.ll, addr);
>   		break;
> +	default:
> +		debug ("fwc: Unknown port width %d\n", info->portwidth);
>   	}
>
>   	/* Ensure all the instructions are fully finished */
> @@ -581,6 +587,7 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
>   				prompt, info->start[sector],
>   				flash_read_long (info, sector, 0));
>   			flash_write_cmd (info, sector, 0, info->cmd_reset);
> +			udelay(1);
>   			return ERR_TIMOUT;
>   		}
>   		udelay (1);		/* also triggers watchdog */
> @@ -628,6 +635,7 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
>   				puts ("Vpp Low Error.\n");
>   		}
>   		flash_write_cmd (info, sector, 0, info->cmd_reset);
> +		udelay(1);
>   		break;
>   	default:
>   		break;
> @@ -744,12 +752,8 @@ static void flash_add_byte (flash_info_t * info, cfiword_t * cword, uchar c)
>   static flash_sect_t find_sector (flash_info_t * info, ulong addr)
>   {
>   	static flash_sect_t saved_sector = 0; /* previously found sector */
> -	static flash_info_t *saved_info = 0; /* previously used flash bank */
>   	flash_sect_t sector = saved_sector;
>
> -	if ((info != saved_info) || (sector>= info->sector_count))
> -		sector = 0;
> -
>   	while ((info->start[sector]<  addr)
>   			&&  (sector<  info->sector_count - 1))
>   		sector++;
> @@ -761,7 +765,6 @@ static flash_sect_t find_sector (flash_info_t * info, ulong addr)
>   		sector--;
>
>   	saved_sector = sector;
> -	saved_info = info;
>   	return sector;
>   }
>
> @@ -825,12 +828,15 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
>
>   	switch (info->portwidth) {
>   	case FLASH_CFI_8BIT:
> +		debug("%s: 8-bit 0x%02x\n", __func__, cword.c);
>   		flash_write8(cword.c, dstaddr);
>   		break;
>   	case FLASH_CFI_16BIT:
> +		debug("%s: 16-bit 0x%04x\n", __func__, cword.w);
>   		flash_write16(cword.w, dstaddr);
>   		break;
>   	case FLASH_CFI_32BIT:
> +		debug("%s: 32-bit 0x%08lx\n", __func__, cword.l);
>   		flash_write32(cword.l, dstaddr);
>   		break;
>   	case FLASH_CFI_64BIT:
> @@ -1043,6 +1049,8 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
>   	int prot;
>   	flash_sect_t sect;
>   	int st;
> +	

Whitespace (see 'git apply' comment above).

> +	debug("%s: erasing sectors %d to %d\n", __func__, s_first, s_last);
>
>   	if (info->flash_id != FLASH_MAN_CFI) {
>   		puts ("Can't erase unknown flash type - aborted\n");
> @@ -1082,6 +1090,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
>   				break;
>   			case CFI_CMDSET_AMD_STANDARD:
>   			case CFI_CMDSET_AMD_EXTENDED:
> +				flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
>   				flash_unlock_seq (info, sect);
>   				flash_write_cmd (info, sect,
>   						info->addr_unlock1,
> @@ -1121,6 +1130,8 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
>   				rcode = 1;
>   			else if (flash_verbose)
>   				putc ('.');
> +		} else {
> +			debug("\nSector %d is protected.\n", sect);
>   		}
>   	}
>
> @@ -1490,6 +1501,7 @@ void flash_read_user_serial (flash_info_t * info, void *buffer, int offset,
>   	flash_write_cmd (info, 0, 0, FLASH_CMD_READ_ID);
>   	memcpy (dst, src + offset, len);
>   	flash_write_cmd (info, 0, 0, info->cmd_reset);
> +	udelay(1);
>   	flash_unmap(info, 0, FLASH_OFFSET_USER_PROTECTION, src);
>   }
>
> @@ -1505,6 +1517,7 @@ void flash_read_factory_serial (flash_info_t * info, void *buffer, int offset,
>   	flash_write_cmd (info, 0, 0, FLASH_CMD_READ_ID);
>   	memcpy (buffer, src + offset, len);
>   	flash_write_cmd (info, 0, 0, info->cmd_reset);
> +	udelay(1);
>   	flash_unmap(info, 0, FLASH_OFFSET_INTEL_PROTECTION, src);
>   }
>
> @@ -1536,6 +1549,7 @@ static void cfi_reverse_geometry(struct cfi_qry *qry)
>   static void cmdset_intel_read_jedec_ids(flash_info_t *info)
>   {
>   	flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
> +	udelay(1);
>   	flash_write_cmd(info, 0, 0, FLASH_CMD_READ_ID);
>   	udelay(1000); /* some flash are slow to respond */
>   	info->manufacturer_id = flash_read_uchar (info,
> @@ -1573,8 +1587,7 @@ static void cmdset_amd_read_jedec_ids(flash_info_t *info)
>   	flash_unlock_seq(info, 0);
>   	flash_write_cmd(info, 0, info->addr_unlock1, FLASH_CMD_READ_ID);
>   	udelay(1000); /* some flash are slow to respond */
> -
> -	manuId = flash_read_uchar (info, FLASH_OFFSET_MANUFACTURER_ID);
> +	manuId = flash_read_uchar(info, FLASH_OFFSET_MANUFACTURER_ID);
>   	/* JEDEC JEP106Z specifies ID codes up to bank 7 */
>   	while (manuId == FLASH_CONTINUATION_CODE&&  bankId<  0x800) {
>   		bankId += 0x100;
> @@ -1604,6 +1617,7 @@ static void cmdset_amd_read_jedec_ids(flash_info_t *info)
>   		break;
>   	}
>   	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
> +	udelay(1);
>   }
>
>   static int cmdset_amd_init(flash_info_t *info, struct cfi_qry *qry)
> @@ -1719,7 +1733,7 @@ static void flash_read_cfi (flash_info_t *info, void *buf,
>   	unsigned int i;
>
>   	for (i = 0; i<  len; i++)
> -		p[i] = flash_read_uchar(info, start + i);
> +		p[i] = flash_read_uchar(info, start + (i * 2));

This "2" seems a bit magical to me -- apparently the semantics of 
flash_read_uchar seem to change with your patch If they do, please add 
comments before the definition of flash_read_uchar() to make the meaning 
of its arguments completely clear.

Besides, does this still work with pure 8 bit devices, where the CFI 
area is byte-spaced?

>   }
>
>   void __flash_cmd_reset(flash_info_t *info)
> @@ -1730,6 +1744,7 @@ void __flash_cmd_reset(flash_info_t *info)
>   	 * that AMD flash roms ignore the Intel command.
>   	 */
>   	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
> +	udelay(1);
>   	flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
>   }
>   void flash_cmd_reset(flash_info_t *info)
> @@ -1739,21 +1754,38 @@ static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
>   {
>   	int cfi_offset;
>
> -	/* Issue FLASH reset command */
> -	flash_cmd_reset(info);
> -
>   	for (cfi_offset=0;
>   	     cfi_offset<  sizeof(flash_offset_cfi) / sizeof(uint);
>   	     cfi_offset++) {
> +		/* Issue FLASH reset command */
> +		flash_cmd_reset(info);
>   		flash_write_cmd (info, 0, flash_offset_cfi[cfi_offset],
>   				 FLASH_CMD_CFI);
>   		if (flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP, 'Q')
> -		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 1, 'R')
> -		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'Y')) {
> +		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'R')
> +		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 4, 'Y')) {

Here too, the change requires that the semantics of flash_isequal be 
explicited in a comment before the definition of flash_isequal() to make 
the meaning of its arguments completely clear.

>   			flash_read_cfi(info, qry, FLASH_OFFSET_CFI_RESP,
>   					sizeof(struct cfi_qry));
> +#ifdef CONFIG_SYS_FLASH_INTERFACE_WIDTH
> +			info->interface = CONFIG_SYS_FLASH_INTERFACE_WIDTH;
> +#else
>   			info->interface	= le16_to_cpu(qry->interface_desc);
> -
> +			/* Some flash chips can support multiple bus widths.
> +			 * In this case, override the interface width and
> +			 * limit it to the port width.
> +			 */
> +			if ((info->interface == FLASH_CFI_X8X16)&&
> +			    (info->portwidth == FLASH_CFI_8BIT)) {
> +				debug("Overriding 16-bit interface width to "
> +				      " 8-bit port width.\n");
> +				info->interface = FLASH_CFI_X8;
> +			} else if ((info->interface == FLASH_CFI_X16X32)&&
> +				 (info->portwidth == FLASH_CFI_16BIT)) {
> +				debug("Overriding 16-bit interface width to "
> +				      " 16-bit port width.\n");
> +				info->interface = FLASH_CFI_X16;
> +			}
> +#endif
>   			info->cfi_offset = flash_offset_cfi[cfi_offset];
>   			debug ("device interface is %d\n",
>   			       info->interface);
> @@ -1764,8 +1796,8 @@ static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
>   			       info->chipwidth<<  CFI_FLASH_SHIFT_WIDTH);
>
>   			/* calculate command offsets as in the Linux driver */
> -			info->addr_unlock1 = 0x555;
> -			info->addr_unlock2 = 0x2aa;
> +			info->addr_unlock1 = 0xaaa;
> +			info->addr_unlock2 = 0x555;
>
>   			/*
>   			 * modify the unlock address if we are
> @@ -1799,8 +1831,11 @@ static int flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
>   		for (info->chipwidth = FLASH_CFI_BY8;
>   		     info->chipwidth<= info->portwidth;
>   		     info->chipwidth<<= 1)
> -			if (__flash_detect_cfi(info, qry))
> +			if (__flash_detect_cfi(info, qry)) {
> +				debug("Found CFI flash, portwidth %d, chipwidth %d\n",
> +				      info->portwidth, info->chipwidth);
>   				return 1;
> +			}
>   	}
>   	debug ("not found\n");
>   	return 0;
> @@ -1819,7 +1854,7 @@ static void flash_fixup_amd(flash_info_t *info, struct cfi_qry *qry)
>   			/* CFI<  1.1, try to guess from device id */
>   			if ((info->device_id&  0x80) != 0)
>   				cfi_reverse_geometry(qry);
> -		} else if (flash_read_uchar(info, info->ext_addr + 0xf) == 3) {
> +		} else if (flash_read_uchar(info, info->ext_addr + 0x1e) == 3) {

Please add comments to explain what the '0x1e' actually means -- that's 
not a fault of your patch, btw, as the 0xf in the original code could 
already have used some commenting; since you're at it, you might as well 
improve on code understandability as well. :)

>   			/* CFI>= 1.1, deduct from top/bottom flag */
>   			/* note: ext_addr is valid since cfi_version>  0 */
>   			cfi_reverse_geometry(qry);
> @@ -1891,14 +1926,15 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>
>   	if (flash_detect_cfi (info,&qry)) {
>   		info->vendor = le16_to_cpu(qry.p_id);
> -		info->ext_addr = le16_to_cpu(qry.p_adr);
> +		info->ext_addr = le16_to_cpu(qry.p_adr) * 2;
> +		debug("extended address is 0x%x\n", info->ext_addr);
>   		num_erase_regions = qry.num_erase_regions;
>
>   		if (info->ext_addr) {
>   			info->cfi_version = (ushort) flash_read_uchar (info,
> -						info->ext_addr + 3)<<  8;
> +						info->ext_addr + 6)<<  8;
>   			info->cfi_version |= (ushort) flash_read_uchar (info,
> -						info->ext_addr + 4);
> +						info->ext_addr + 8);
>   		}

Same remark re: "magic" for the changes above: tell readers what units 
(bytes, words, something else) these offsets are expressed in.

>   #ifdef DEBUG
> @@ -1945,13 +1981,16 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>   		debug ("device id is 0x%x\n", info->device_id);
>   		debug ("device id2 is 0x%x\n", info->device_id2);
>   		debug ("cfi version is 0x%04x\n", info->cfi_version);
> -
> +		debug ("port width: %d, chipwidth: %d, interface: %d\n",
> +		       info->portwidth, info->chipwidth, info->interface);
>   		size_ratio = info->portwidth / info->chipwidth;
> +#if 0
>   		/* if the chip is x8/x16 reduce the ratio by half */
>   		if ((info->interface == FLASH_CFI_X8X16)
>   		&&  (info->chipwidth == FLASH_CFI_BY8)) {
>   			size_ratio>>= 1;
>   		}
> +#endif

NAK on this one. If you want to remove some code, remove it. If you 
still want it, dont comment it out.

>   		debug ("size_ratio %d port %d bits chip %d bits\n",
>   		       size_ratio, info->portwidth<<  CFI_FLASH_SHIFT_WIDTH,
>   		       info->chipwidth<<  CFI_FLASH_SHIFT_WIDTH);
> @@ -2023,6 +2062,8 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>   				sect_cnt++;
>   			}
>   		}
> +		debug ("port width: %d, chipwidth: %d, interface: %d\n",
> +		       info->portwidth, info->chipwidth, info->interface);
>
>   		info->sector_count = sect_cnt;
>   		info->buffer_size = 1<<  le16_to_cpu(qry.max_buf_write_size);
> diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
> index 3245b44..edcf9be 100644
> --- a/include/mtd/cfi_flash.h
> +++ b/include/mtd/cfi_flash.h

Overall remark on the .h file: please explain what unit the values are 
expressed in (byte offets, word offsets, width-independent, whatever.

>

Apart from this, I am happy to report that this code works well with my 
ED Mini V2, allowing me to remove the CONFIG_FLASH_CFI_LEGACY config 
option and use the standard driver, so:

Tested-by: Albert ARIBAUD <albert.aribaud@free.fr>

Thanks a lot, Aaron!

Amicalement,
Rogan Dawes - April 2, 2011, 7:37 p.m.
On 2011/04/02 1:23 PM, Albert ARIBAUD wrote:
> Hi Aaron,
> 
> Le 02/04/2011 09:17, Aaron Williams a écrit :
>> This patch corrects the addresses used when working with Spansion/AMD FLASH chips.
>> Addressing for 8 and 16 bits is almost identical except in the 16-bit case the
>> LSB of the address is always 0.  The confusion arose because the addresses
>> in the datasheet for 16-bit mode are word addresses but this code assumed it was
>> byte addresses.
>>
>> I have only been able to test this on our Octeon boards which use either an 8-bit
>> or 16-bit bus.  I have not tested the case where there's an 8-bit part on a 16-bit
>> bus.
>>
>> This patch also adds some delays as suggested by Spansion.
>>
>> If a part can be both 8 and 16-bits, it forces it to work in 8-bit mode if an
>> 8-bit bus is detected.
>>
>> -Aaron Williams
>>
>>
>> Signed-off-by: Aaron Williams<aaron.williams@caviumnetworks.com>
> 
> Comments related to testing and 'signatures' should not be part of the 
> commit message and thus should go below the commit message delimiter.
> 
>> ---
>>   drivers/mtd/cfi_flash.c |   93 ++++++++++++++++++++++++++++++++++-------------
>>   include/mtd/cfi_flash.h |   40 ++++++++++----------
>>   2 files changed, 87 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>> index 0909fe7..eab1fbe 100644
>> --- a/drivers/mtd/cfi_flash.c
>> +++ b/drivers/mtd/cfi_flash.c
>> @@ -10,6 +10,9 @@
>>    *
>>    * Copyright (C) 2006
>>    * Tolunay Orkun<listmember@orkun.us>
>> + *
> 
> There is a space between the asterisk and end of line. You can check if 
> your patch is whitespace-clean by applying it locally as if you were 
> another u-boot user; 'git apply' will tell you where in the patch there 
> are undue whitespace.
> 
>> + * Copyright (C) 2011 Cavium Networks, Inc.
>> + * Aaron Williams<Aaron.Williams@caviumnetworks.com>
>>    *
>>    * See file CREDITS for list of people who contributed to this
>>    * project.
>> @@ -32,7 +35,6 @@
>>    */
>>
>>   /* The DEBUG define must be before common to enable debugging */
>> -/* #define DEBUG	*/

Not sure why you removed this?

>>
>>   #include<common.h>
>>   #include<asm/processor.h>
>> @@ -209,9 +211,11 @@ unsigned long flash_sector_size(flash_info_t *info, flash_sect_t sect)
>>   static inline void *
>>   flash_map (flash_info_t * info, flash_sect_t sect, uint offset)
>>   {
>> -	unsigned int byte_offset = offset * info->portwidth;
>> -
> 
> Whitespace (see 'git apply' comment above).
> 
>> -	return (void *)(info->start[sect] + byte_offset);
>> +	unsigned int byte_offset = offset * info->portwidth / info->chipwidth;
>> +	unsigned int addr = (info->start[sect] + byte_offset);
>> +	unsigned int mask = 0xffffffff<<  (info->portwidth - 1);
>> +	
>> +	return (void *)(addr&  mask);
>>   }
>>
>>   static inline void flash_unmap(flash_info_t *info, flash_sect_t sect,
>> @@ -397,6 +401,8 @@ void flash_write_cmd (flash_info_t * info, flash_sect_t sect,
>>   #endif
>>   		flash_write64(cword.ll, addr);
>>   		break;
>> +	default:
>> +		debug ("fwc: Unknown port width %d\n", info->portwidth);
>>   	}
>>
>>   	/* Ensure all the instructions are fully finished */
>> @@ -581,6 +587,7 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
>>   				prompt, info->start[sector],
>>   				flash_read_long (info, sector, 0));
>>   			flash_write_cmd (info, sector, 0, info->cmd_reset);
>> +			udelay(1);
>>   			return ERR_TIMOUT;

Is there a point to introducing a delay if you are already returning a
timeout error?

>>   		}
>>   		udelay (1);		/* also triggers watchdog */
>> @@ -628,6 +635,7 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
>>   				puts ("Vpp Low Error.\n");
>>   		}
>>   		flash_write_cmd (info, sector, 0, info->cmd_reset);
>> +		udelay(1);
>>   		break;
>>   	default:
>>   		break;
>> @@ -744,12 +752,8 @@ static void flash_add_byte (flash_info_t * info, cfiword_t * cword, uchar c)
>>   static flash_sect_t find_sector (flash_info_t * info, ulong addr)
>>   {
>>   	static flash_sect_t saved_sector = 0; /* previously found sector */
>> -	static flash_info_t *saved_info = 0; /* previously used flash bank */
>>   	flash_sect_t sector = saved_sector;
>>
>> -	if ((info != saved_info) || (sector>= info->sector_count))
>> -		sector = 0;
>> -
>>   	while ((info->start[sector]<  addr)
>>   			&&  (sector<  info->sector_count - 1))
>>   		sector++;
>> @@ -761,7 +765,6 @@ static flash_sect_t find_sector (flash_info_t * info, ulong addr)
>>   		sector--;
>>
>>   	saved_sector = sector;
>> -	saved_info = info;
>>   	return sector;
>>   }
>>
>> @@ -825,12 +828,15 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
>>
>>   	switch (info->portwidth) {
>>   	case FLASH_CFI_8BIT:
>> +		debug("%s: 8-bit 0x%02x\n", __func__, cword.c);
>>   		flash_write8(cword.c, dstaddr);
>>   		break;
>>   	case FLASH_CFI_16BIT:
>> +		debug("%s: 16-bit 0x%04x\n", __func__, cword.w);
>>   		flash_write16(cword.w, dstaddr);
>>   		break;
>>   	case FLASH_CFI_32BIT:
>> +		debug("%s: 32-bit 0x%08lx\n", __func__, cword.l);
>>   		flash_write32(cword.l, dstaddr);
>>   		break;
>>   	case FLASH_CFI_64BIT:
>> @@ -1043,6 +1049,8 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
>>   	int prot;
>>   	flash_sect_t sect;
>>   	int st;
>> +	
> 
> Whitespace (see 'git apply' comment above).
> 
>> +	debug("%s: erasing sectors %d to %d\n", __func__, s_first, s_last);
>>
>>   	if (info->flash_id != FLASH_MAN_CFI) {
>>   		puts ("Can't erase unknown flash type - aborted\n");
>> @@ -1082,6 +1090,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
>>   				break;
>>   			case CFI_CMDSET_AMD_STANDARD:
>>   			case CFI_CMDSET_AMD_EXTENDED:
>> +				flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
>>   				flash_unlock_seq (info, sect);
>>   				flash_write_cmd (info, sect,
>>   						info->addr_unlock1,
>> @@ -1121,6 +1130,8 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
>>   				rcode = 1;
>>   			else if (flash_verbose)
>>   				putc ('.');
>> +		} else {
>> +			debug("\nSector %d is protected.\n", sect);
>>   		}
>>   	}
>>
>> @@ -1490,6 +1501,7 @@ void flash_read_user_serial (flash_info_t * info, void *buffer, int offset,
>>   	flash_write_cmd (info, 0, 0, FLASH_CMD_READ_ID);
>>   	memcpy (dst, src + offset, len);
>>   	flash_write_cmd (info, 0, 0, info->cmd_reset);
>> +	udelay(1);
>>   	flash_unmap(info, 0, FLASH_OFFSET_USER_PROTECTION, src);
>>   }
>>
>> @@ -1505,6 +1517,7 @@ void flash_read_factory_serial (flash_info_t * info, void *buffer, int offset,
>>   	flash_write_cmd (info, 0, 0, FLASH_CMD_READ_ID);
>>   	memcpy (buffer, src + offset, len);
>>   	flash_write_cmd (info, 0, 0, info->cmd_reset);
>> +	udelay(1);
>>   	flash_unmap(info, 0, FLASH_OFFSET_INTEL_PROTECTION, src);
>>   }
>>
>> @@ -1536,6 +1549,7 @@ static void cfi_reverse_geometry(struct cfi_qry *qry)
>>   static void cmdset_intel_read_jedec_ids(flash_info_t *info)
>>   {
>>   	flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
>> +	udelay(1);
>>   	flash_write_cmd(info, 0, 0, FLASH_CMD_READ_ID);
>>   	udelay(1000); /* some flash are slow to respond */
>>   	info->manufacturer_id = flash_read_uchar (info,
>> @@ -1573,8 +1587,7 @@ static void cmdset_amd_read_jedec_ids(flash_info_t *info)
>>   	flash_unlock_seq(info, 0);
>>   	flash_write_cmd(info, 0, info->addr_unlock1, FLASH_CMD_READ_ID);
>>   	udelay(1000); /* some flash are slow to respond */
>> -
>> -	manuId = flash_read_uchar (info, FLASH_OFFSET_MANUFACTURER_ID);
>> +	manuId = flash_read_uchar(info, FLASH_OFFSET_MANUFACTURER_ID);
>>   	/* JEDEC JEP106Z specifies ID codes up to bank 7 */
>>   	while (manuId == FLASH_CONTINUATION_CODE&&  bankId<  0x800) {
>>   		bankId += 0x100;
>> @@ -1604,6 +1617,7 @@ static void cmdset_amd_read_jedec_ids(flash_info_t *info)
>>   		break;
>>   	}
>>   	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
>> +	udelay(1);
>>   }
>>
>>   static int cmdset_amd_init(flash_info_t *info, struct cfi_qry *qry)
>> @@ -1719,7 +1733,7 @@ static void flash_read_cfi (flash_info_t *info, void *buf,
>>   	unsigned int i;
>>
>>   	for (i = 0; i<  len; i++)
>> -		p[i] = flash_read_uchar(info, start + i);
>> +		p[i] = flash_read_uchar(info, start + (i * 2));
> 
> This "2" seems a bit magical to me -- apparently the semantics of 
> flash_read_uchar seem to change with your patch If they do, please add 
> comments before the definition of flash_read_uchar() to make the meaning 
> of its arguments completely clear.
> 
> Besides, does this still work with pure 8 bit devices, where the CFI 
> area is byte-spaced?
> 
>>   }
>>
>>   void __flash_cmd_reset(flash_info_t *info)
>> @@ -1730,6 +1744,7 @@ void __flash_cmd_reset(flash_info_t *info)
>>   	 * that AMD flash roms ignore the Intel command.
>>   	 */
>>   	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
>> +	udelay(1);
>>   	flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
>>   }
>>   void flash_cmd_reset(flash_info_t *info)
>> @@ -1739,21 +1754,38 @@ static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
>>   {
>>   	int cfi_offset;
>>
>> -	/* Issue FLASH reset command */
>> -	flash_cmd_reset(info);
>> -
>>   	for (cfi_offset=0;
>>   	     cfi_offset<  sizeof(flash_offset_cfi) / sizeof(uint);
>>   	     cfi_offset++) {
>> +		/* Issue FLASH reset command */
>> +		flash_cmd_reset(info);
>>   		flash_write_cmd (info, 0, flash_offset_cfi[cfi_offset],
>>   				 FLASH_CMD_CFI);
>>   		if (flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP, 'Q')
>> -		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 1, 'R')
>> -		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'Y')) {
>> +		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'R')
>> +		&&  flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 4, 'Y')) {
> 
> Here too, the change requires that the semantics of flash_isequal be 
> explicited in a comment before the definition of flash_isequal() to make 
> the meaning of its arguments completely clear.
> 
>>   			flash_read_cfi(info, qry, FLASH_OFFSET_CFI_RESP,
>>   					sizeof(struct cfi_qry));
>> +#ifdef CONFIG_SYS_FLASH_INTERFACE_WIDTH
>> +			info->interface = CONFIG_SYS_FLASH_INTERFACE_WIDTH;
>> +#else
>>   			info->interface	= le16_to_cpu(qry->interface_desc);
>> -
>> +			/* Some flash chips can support multiple bus widths.
>> +			 * In this case, override the interface width and
>> +			 * limit it to the port width.
>> +			 */
>> +			if ((info->interface == FLASH_CFI_X8X16)&&
>> +			    (info->portwidth == FLASH_CFI_8BIT)) {
>> +				debug("Overriding 16-bit interface width to "
>> +				      " 8-bit port width.\n");
>> +				info->interface = FLASH_CFI_X8;
>> +			} else if ((info->interface == FLASH_CFI_X16X32)&&
>> +				 (info->portwidth == FLASH_CFI_16BIT)) {
>> +				debug("Overriding 16-bit interface width to "
>> +				      " 16-bit port width.\n");
>> +				info->interface = FLASH_CFI_X16;
>> +			}
>> +#endif
>>   			info->cfi_offset = flash_offset_cfi[cfi_offset];
>>   			debug ("device interface is %d\n",
>>   			       info->interface);
>> @@ -1764,8 +1796,8 @@ static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
>>   			       info->chipwidth<<  CFI_FLASH_SHIFT_WIDTH);
>>
>>   			/* calculate command offsets as in the Linux driver */
>> -			info->addr_unlock1 = 0x555;
>> -			info->addr_unlock2 = 0x2aa;
>> +			info->addr_unlock1 = 0xaaa;
>> +			info->addr_unlock2 = 0x555;
>>
>>   			/*
>>   			 * modify the unlock address if we are
>> @@ -1799,8 +1831,11 @@ static int flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
>>   		for (info->chipwidth = FLASH_CFI_BY8;
>>   		     info->chipwidth<= info->portwidth;
>>   		     info->chipwidth<<= 1)
>> -			if (__flash_detect_cfi(info, qry))
>> +			if (__flash_detect_cfi(info, qry)) {
>> +				debug("Found CFI flash, portwidth %d, chipwidth %d\n",
>> +				      info->portwidth, info->chipwidth);
>>   				return 1;
>> +			}
>>   	}
>>   	debug ("not found\n");
>>   	return 0;
>> @@ -1819,7 +1854,7 @@ static void flash_fixup_amd(flash_info_t *info, struct cfi_qry *qry)
>>   			/* CFI<  1.1, try to guess from device id */
>>   			if ((info->device_id&  0x80) != 0)
>>   				cfi_reverse_geometry(qry);
>> -		} else if (flash_read_uchar(info, info->ext_addr + 0xf) == 3) {
>> +		} else if (flash_read_uchar(info, info->ext_addr + 0x1e) == 3) {
> 
> Please add comments to explain what the '0x1e' actually means -- that's 
> not a fault of your patch, btw, as the 0xf in the original code could 
> already have used some commenting; since you're at it, you might as well 
> improve on code understandability as well. :)
> 
>>   			/* CFI>= 1.1, deduct from top/bottom flag */
>>   			/* note: ext_addr is valid since cfi_version>  0 */
>>   			cfi_reverse_geometry(qry);
>> @@ -1891,14 +1926,15 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>>
>>   	if (flash_detect_cfi (info,&qry)) {
>>   		info->vendor = le16_to_cpu(qry.p_id);
>> -		info->ext_addr = le16_to_cpu(qry.p_adr);
>> +		info->ext_addr = le16_to_cpu(qry.p_adr) * 2;
>> +		debug("extended address is 0x%x\n", info->ext_addr);
>>   		num_erase_regions = qry.num_erase_regions;
>>
>>   		if (info->ext_addr) {
>>   			info->cfi_version = (ushort) flash_read_uchar (info,
>> -						info->ext_addr + 3)<<  8;
>> +						info->ext_addr + 6)<<  8;
>>   			info->cfi_version |= (ushort) flash_read_uchar (info,
>> -						info->ext_addr + 4);
>> +						info->ext_addr + 8);
>>   		}
> 
> Same remark re: "magic" for the changes above: tell readers what units 
> (bytes, words, something else) these offsets are expressed in.
> 
>>   #ifdef DEBUG
>> @@ -1945,13 +1981,16 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>>   		debug ("device id is 0x%x\n", info->device_id);
>>   		debug ("device id2 is 0x%x\n", info->device_id2);
>>   		debug ("cfi version is 0x%04x\n", info->cfi_version);
>> -
>> +		debug ("port width: %d, chipwidth: %d, interface: %d\n",
>> +		       info->portwidth, info->chipwidth, info->interface);
>>   		size_ratio = info->portwidth / info->chipwidth;
>> +#if 0
>>   		/* if the chip is x8/x16 reduce the ratio by half */
>>   		if ((info->interface == FLASH_CFI_X8X16)
>>   		&&  (info->chipwidth == FLASH_CFI_BY8)) {
>>   			size_ratio>>= 1;
>>   		}
>> +#endif
> 
> NAK on this one. If you want to remove some code, remove it. If you 
> still want it, dont comment it out.
> 
>>   		debug ("size_ratio %d port %d bits chip %d bits\n",
>>   		       size_ratio, info->portwidth<<  CFI_FLASH_SHIFT_WIDTH,
>>   		       info->chipwidth<<  CFI_FLASH_SHIFT_WIDTH);
>> @@ -2023,6 +2062,8 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>>   				sect_cnt++;
>>   			}
>>   		}
>> +		debug ("port width: %d, chipwidth: %d, interface: %d\n",
>> +		       info->portwidth, info->chipwidth, info->interface);
>>
>>   		info->sector_count = sect_cnt;
>>   		info->buffer_size = 1<<  le16_to_cpu(qry.max_buf_write_size);
>> diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
>> index 3245b44..edcf9be 100644
>> --- a/include/mtd/cfi_flash.h
>> +++ b/include/mtd/cfi_flash.h
> 
> Overall remark on the .h file: please explain what unit the values are 
> expressed in (byte offets, word offsets, width-independent, whatever.
> 
>>
> 
> Apart from this, I am happy to report that this code works well with my 
> ED Mini V2, allowing me to remove the CONFIG_FLASH_CFI_LEGACY config 
> option and use the standard driver, so:
> 
> Tested-by: Albert ARIBAUD <albert.aribaud@free.fr>
> 
> Thanks a lot, Aaron!
> 
> Amicalement,

Also tested with the DNS323, this patch allows me to remove the
"horrible hacks" that allowed CFI to work on that board too.

Thanks Aaron!

Rogan
Stefan Roese - April 4, 2011, 10:14 a.m.
Hi Aaron,

On Saturday 02 April 2011 09:17:01 Aaron Williams wrote:
> This patch corrects the addresses used when working with Spansion/AMD FLASH
> chips. Addressing for 8 and 16 bits is almost identical except in the
> 16-bit case the LSB of the address is always 0.  The confusion arose
> because the addresses in the datasheet for 16-bit mode are word addresses
> but this code assumed it was byte addresses.
> 
> I have only been able to test this on our Octeon boards which use either an
> 8-bit or 16-bit bus.  I have not tested the case where there's an 8-bit
> part on a 16-bit bus.
> 
> This patch also adds some delays as suggested by Spansion.
> 
> If a part can be both 8 and 16-bits, it forces it to work in 8-bit mode if
> an 8-bit bus is detected.

Thanks.

Apart from the comments from Albert and Rogan, here some further notes:

- Please explain why the delay() calls are added. If really needed, please
  move them into a separate patch, as this seems unrelated to the 8/16
  bit issue.

- Please add a short description and rationale to the commit text (and
  perhaps to cfi_flash.h), why the offsets for the CFI data are now
  changed (e.g. 0x10 -> 0x20).

And one further comment below:
 
> @@ -1043,6 +1049,8 @@ int flash_erase (flash_info_t * info, int s_first,
> int s_last) int prot;
>  	flash_sect_t sect;
>  	int st;
> +
> +	debug("%s: erasing sectors %d to %d\n", __func__, s_first, s_last);
> 
>  	if (info->flash_id != FLASH_MAN_CFI) {
>  		puts ("Can't erase unknown flash type - aborted\n");
> @@ -1082,6 +1090,7 @@ int flash_erase (flash_info_t * info, int s_first,
> int s_last) break;
>  			case CFI_CMDSET_AMD_STANDARD:
>  			case CFI_CMDSET_AMD_EXTENDED:
> +				flash_write_cmd (info, 0, 0, AMD_CMD_RESET);

Is this correct? A reset command in every flash_erase() call?

Thanks.

Cheers,
Stefan

--
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@denx.de
Aaron Williams - April 12, 2011, 7:46 a.m.
I believe this is correct.  I have redone the patch as two patches. The first 
patch fixes the 8-bit addressing and has been tested with both 8 and 16-bit 
support with Spansion. The second patch adds a 1us delay after every reset 
call. In my email correspondence with Spansion they said that at least some 
devices require a 500ns delay after the reset command.

-Aaron

On Monday, April 04, 2011 03:14:17 AM Stefan Roese wrote:
> Hi Aaron,
> 
> On Saturday 02 April 2011 09:17:01 Aaron Williams wrote:
> > This patch corrects the addresses used when working with Spansion/AMD
> > FLASH chips. Addressing for 8 and 16 bits is almost identical except in
> > the 16-bit case the LSB of the address is always 0.  The confusion arose
> > because the addresses in the datasheet for 16-bit mode are word
> > addresses but this code assumed it was byte addresses.
> > 
> > I have only been able to test this on our Octeon boards which use either
> > an 8-bit or 16-bit bus.  I have not tested the case where there's an
> > 8-bit part on a 16-bit bus.
> > 
> > This patch also adds some delays as suggested by Spansion.
> > 
> > If a part can be both 8 and 16-bits, it forces it to work in 8-bit mode
> > if an 8-bit bus is detected.
> 
> Thanks.
> 
> Apart from the comments from Albert and Rogan, here some further notes:
> 
> - Please explain why the delay() calls are added. If really needed, please
>   move them into a separate patch, as this seems unrelated to the 8/16
>   bit issue.
> 
> - Please add a short description and rationale to the commit text (and
>   perhaps to cfi_flash.h), why the offsets for the CFI data are now
>   changed (e.g. 0x10 -> 0x20).
> 
> And one further comment below:
> > @@ -1043,6 +1049,8 @@ int flash_erase (flash_info_t * info, int s_first,
> > int s_last) int prot;
> > 
> >  	flash_sect_t sect;
> >  	int st;
> > 
> > +
> > +	debug("%s: erasing sectors %d to %d\n", __func__, s_first, s_last);
> > 
> >  	if (info->flash_id != FLASH_MAN_CFI) {
> >  	
> >  		puts ("Can't erase unknown flash type - aborted\n");
> > 
> > @@ -1082,6 +1090,7 @@ int flash_erase (flash_info_t * info, int s_first,
> > int s_last) break;
> > 
> >  			case CFI_CMDSET_AMD_STANDARD:
> > 
> >  			case CFI_CMDSET_AMD_EXTENDED:
> > +				flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
> 
> Is this correct? A reset command in every flash_erase() call?
> 
> Thanks.
> 
> Cheers,
> Stefan
>
Stefan Roese - April 12, 2011, 8:09 a.m.
Hi Aaron,

On Tuesday 12 April 2011 09:46:22 Aaron Williams wrote:
> I believe this is correct.

Hmmm, I'm still not convinced about this reset call in the erase function. Do 
you really need it for the CFI driver to work correctly on your board? Could 
you please test without this reset command?

> I have redone the patch as two patches. The
> first patch fixes the 8-bit addressing and has been tested with both 8 and
> 16-bit support with Spansion. The second patch adds a 1us delay after
> every reset call. In my email correspondence with Spansion they said that
> at least some devices require a 500ns delay after the reset command.

Thanks. The only real concern I still have is the new reset command as 
mentioned above. Please either explain why it is needed, or remove it from 
your patch. I'll push the patches upstream once this issue is resolved.

<snip>
 
> > > @@ -1043,6 +1049,8 @@ int flash_erase (flash_info_t * info, int
> > > s_first, int s_last) int prot;
> > > 
> > >  	flash_sect_t sect;
> > >  	int st;
> > > 
> > > +
> > > +	debug("%s: erasing sectors %d to %d\n", __func__, s_first, s_last);
> > > 
> > >  	if (info->flash_id != FLASH_MAN_CFI) {
> > >  	
> > >  		puts ("Can't erase unknown flash type - aborted\n");
> > > 
> > > @@ -1082,6 +1090,7 @@ int flash_erase (flash_info_t * info, int
> > > s_first, int s_last) break;
> > > 
> > >  			case CFI_CMDSET_AMD_STANDARD:
> > > 
> > >  			case CFI_CMDSET_AMD_EXTENDED:
> > > +				flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
> > 
> > Is this correct? A reset command in every flash_erase() call?
> > 
> > Thanks.
> > 
> > Cheers,
> > Stefan

Cheers,
Stefan

--
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@denx.de
Aaron Williams - April 12, 2011, 8:33 a.m.
Hi Stefan,

It looks like the other reset is not needed. The delay is needed. Without it 
sometimes the reset would fail on some of our boards.

Here's what Garret Swalling at Spansion told me:

...
The CFI reset calls into two subroutines that resove to:
        flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
        flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);

According to the GL-N datasheet, even when there is no embedded operation 
ongoing, we need to allow at least 500ns for the reset to complete.  Maybe the 
extra command write and two additional function returns are allowing the flash 
enough time to complete the reset and respond to the next command.
---

While it seems to work without the delay at least on the one board I'm testing 
at the moment, I added it at at suggestion of Garret. I could try testing on 
all of the boards I'm supporting but I'd prefer not to (I'm supporting 15 
different boards and 7 different processor families at the moment).

-Aaron

On Tuesday, April 12, 2011 01:09:18 AM Stefan Roese wrote:
> Hi Aaron,
> 
> On Tuesday 12 April 2011 09:46:22 Aaron Williams wrote:
> > I believe this is correct.
> 
> Hmmm, I'm still not convinced about this reset call in the erase function.
> Do you really need it for the CFI driver to work correctly on your board?
> Could you please test without this reset command?
> 
> > I have redone the patch as two patches. The
> > first patch fixes the 8-bit addressing and has been tested with both 8
> > and 16-bit support with Spansion. The second patch adds a 1us delay
> > after every reset call. In my email correspondence with Spansion they
> > said that at least some devices require a 500ns delay after the reset
> > command.
> 
> Thanks. The only real concern I still have is the new reset command as
> mentioned above. Please either explain why it is needed, or remove it from
> your patch. I'll push the patches upstream once this issue is resolved.
> 
> <snip>
> 
> > > > @@ -1043,6 +1049,8 @@ int flash_erase (flash_info_t * info, int
> > > > s_first, int s_last) int prot;
> > > > 
> > > >  	flash_sect_t sect;
> > > >  	int st;
> > > > 
> > > > +
> > > > +	debug("%s: erasing sectors %d to %d\n", __func__, s_first, s_last);
> > > > 
> > > >  	if (info->flash_id != FLASH_MAN_CFI) {
> > > >  	
> > > >  		puts ("Can't erase unknown flash type - aborted\n");
> > > > 
> > > > @@ -1082,6 +1090,7 @@ int flash_erase (flash_info_t * info, int
> > > > s_first, int s_last) break;
> > > > 
> > > >  			case CFI_CMDSET_AMD_STANDARD:
> > > > 
> > > >  			case CFI_CMDSET_AMD_EXTENDED:
> > > > +				flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
> > > 
> > > Is this correct? A reset command in every flash_erase() call?
> > > 
> > > Thanks.
> > > 
> > > Cheers,
> > > Stefan
> 
> Cheers,
> Stefan
> 
> --
> 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@denx.de
Stefan Roese - April 12, 2011, 9:10 a.m.
Hi Aaron,

On Tuesday 12 April 2011 10:33:05 Aaron Williams wrote:
> It looks like the other reset is not needed.

Good. Then please remove it from your patch and resend a new version labled 
"v2" [PATCH v2]. And please include the patch revision history as mentioned by 
Albert. See this link for details (especially "Sending updated patch 
versions"):

http://www.denx.de/wiki/view/U-Boot/Patches

> The delay is needed. Without
> it sometimes the reset would fail on some of our boards.

Understood.
 
> Here's what Garret Swalling at Spansion told me:
> 
> ...
> The CFI reset calls into two subroutines that resove to:
>         flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
>         flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
> 
> According to the GL-N datasheet, even when there is no embedded operation
> ongoing, we need to allow at least 500ns for the reset to complete.  Maybe
> the extra command write and two additional function returns are allowing
> the flash enough time to complete the reset and respond to the next
> command. ---
> 
> While it seems to work without the delay at least on the one board I'm
> testing at the moment, I added it at at suggestion of Garret. I could try
> testing on all of the boards I'm supporting but I'd prefer not to (I'm
> supporting 15 different boards and 7 different processor families at the
> moment).

I see. I have no problems with your "cfi_flash driver - Add delay after reset 
command" patch. But please resend the 8/16 bit patch as mentioned above.

Thanks.

Cheers,
Stefan

--
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@denx.de

Patch

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 0909fe7..eab1fbe 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -10,6 +10,9 @@ 
  *
  * Copyright (C) 2006
  * Tolunay Orkun <listmember@orkun.us>
+ * 
+ * Copyright (C) 2011 Cavium Networks, Inc.
+ * Aaron Williams <Aaron.Williams@caviumnetworks.com>
  *
  * See file CREDITS for list of people who contributed to this
  * project.
@@ -32,7 +35,6 @@ 
  */
 
 /* The DEBUG define must be before common to enable debugging */
-/* #define DEBUG	*/
 
 #include <common.h>
 #include <asm/processor.h>
@@ -209,9 +211,11 @@  unsigned long flash_sector_size(flash_info_t *info, flash_sect_t sect)
 static inline void *
 flash_map (flash_info_t * info, flash_sect_t sect, uint offset)
 {
-	unsigned int byte_offset = offset * info->portwidth;
-
-	return (void *)(info->start[sect] + byte_offset);
+	unsigned int byte_offset = offset * info->portwidth / info->chipwidth;
+	unsigned int addr = (info->start[sect] + byte_offset);
+	unsigned int mask = 0xffffffff << (info->portwidth - 1);
+	
+	return (void *)(addr & mask);
 }
 
 static inline void flash_unmap(flash_info_t *info, flash_sect_t sect,
@@ -397,6 +401,8 @@  void flash_write_cmd (flash_info_t * info, flash_sect_t sect,
 #endif
 		flash_write64(cword.ll, addr);
 		break;
+	default:
+		debug ("fwc: Unknown port width %d\n", info->portwidth);
 	}
 
 	/* Ensure all the instructions are fully finished */
@@ -581,6 +587,7 @@  static int flash_status_check (flash_info_t * info, flash_sect_t sector,
 				prompt, info->start[sector],
 				flash_read_long (info, sector, 0));
 			flash_write_cmd (info, sector, 0, info->cmd_reset);
+			udelay(1);
 			return ERR_TIMOUT;
 		}
 		udelay (1);		/* also triggers watchdog */
@@ -628,6 +635,7 @@  static int flash_full_status_check (flash_info_t * info, flash_sect_t sector,
 				puts ("Vpp Low Error.\n");
 		}
 		flash_write_cmd (info, sector, 0, info->cmd_reset);
+		udelay(1);
 		break;
 	default:
 		break;
@@ -744,12 +752,8 @@  static void flash_add_byte (flash_info_t * info, cfiword_t * cword, uchar c)
 static flash_sect_t find_sector (flash_info_t * info, ulong addr)
 {
 	static flash_sect_t saved_sector = 0; /* previously found sector */
-	static flash_info_t *saved_info = 0; /* previously used flash bank */
 	flash_sect_t sector = saved_sector;
 
-	if ((info != saved_info) || (sector >= info->sector_count))
-		sector = 0;
-
 	while ((info->start[sector] < addr)
 			&& (sector < info->sector_count - 1))
 		sector++;
@@ -761,7 +765,6 @@  static flash_sect_t find_sector (flash_info_t * info, ulong addr)
 		sector--;
 
 	saved_sector = sector;
-	saved_info = info;
 	return sector;
 }
 
@@ -825,12 +828,15 @@  static int flash_write_cfiword (flash_info_t * info, ulong dest,
 
 	switch (info->portwidth) {
 	case FLASH_CFI_8BIT:
+		debug("%s: 8-bit 0x%02x\n", __func__, cword.c);
 		flash_write8(cword.c, dstaddr);
 		break;
 	case FLASH_CFI_16BIT:
+		debug("%s: 16-bit 0x%04x\n", __func__, cword.w);
 		flash_write16(cword.w, dstaddr);
 		break;
 	case FLASH_CFI_32BIT:
+		debug("%s: 32-bit 0x%08lx\n", __func__, cword.l);
 		flash_write32(cword.l, dstaddr);
 		break;
 	case FLASH_CFI_64BIT:
@@ -1043,6 +1049,8 @@  int flash_erase (flash_info_t * info, int s_first, int s_last)
 	int prot;
 	flash_sect_t sect;
 	int st;
+	
+	debug("%s: erasing sectors %d to %d\n", __func__, s_first, s_last);
 
 	if (info->flash_id != FLASH_MAN_CFI) {
 		puts ("Can't erase unknown flash type - aborted\n");
@@ -1082,6 +1090,7 @@  int flash_erase (flash_info_t * info, int s_first, int s_last)
 				break;
 			case CFI_CMDSET_AMD_STANDARD:
 			case CFI_CMDSET_AMD_EXTENDED:
+				flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
 				flash_unlock_seq (info, sect);
 				flash_write_cmd (info, sect,
 						info->addr_unlock1,
@@ -1121,6 +1130,8 @@  int flash_erase (flash_info_t * info, int s_first, int s_last)
 				rcode = 1;
 			else if (flash_verbose)
 				putc ('.');
+		} else {
+			debug("\nSector %d is protected.\n", sect);
 		}
 	}
 
@@ -1490,6 +1501,7 @@  void flash_read_user_serial (flash_info_t * info, void *buffer, int offset,
 	flash_write_cmd (info, 0, 0, FLASH_CMD_READ_ID);
 	memcpy (dst, src + offset, len);
 	flash_write_cmd (info, 0, 0, info->cmd_reset);
+	udelay(1);
 	flash_unmap(info, 0, FLASH_OFFSET_USER_PROTECTION, src);
 }
 
@@ -1505,6 +1517,7 @@  void flash_read_factory_serial (flash_info_t * info, void *buffer, int offset,
 	flash_write_cmd (info, 0, 0, FLASH_CMD_READ_ID);
 	memcpy (buffer, src + offset, len);
 	flash_write_cmd (info, 0, 0, info->cmd_reset);
+	udelay(1);
 	flash_unmap(info, 0, FLASH_OFFSET_INTEL_PROTECTION, src);
 }
 
@@ -1536,6 +1549,7 @@  static void cfi_reverse_geometry(struct cfi_qry *qry)
 static void cmdset_intel_read_jedec_ids(flash_info_t *info)
 {
 	flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
+	udelay(1);
 	flash_write_cmd(info, 0, 0, FLASH_CMD_READ_ID);
 	udelay(1000); /* some flash are slow to respond */
 	info->manufacturer_id = flash_read_uchar (info,
@@ -1573,8 +1587,7 @@  static void cmdset_amd_read_jedec_ids(flash_info_t *info)
 	flash_unlock_seq(info, 0);
 	flash_write_cmd(info, 0, info->addr_unlock1, FLASH_CMD_READ_ID);
 	udelay(1000); /* some flash are slow to respond */
-
-	manuId = flash_read_uchar (info, FLASH_OFFSET_MANUFACTURER_ID);
+	manuId = flash_read_uchar(info, FLASH_OFFSET_MANUFACTURER_ID);
 	/* JEDEC JEP106Z specifies ID codes up to bank 7 */
 	while (manuId == FLASH_CONTINUATION_CODE && bankId < 0x800) {
 		bankId += 0x100;
@@ -1604,6 +1617,7 @@  static void cmdset_amd_read_jedec_ids(flash_info_t *info)
 		break;
 	}
 	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
+	udelay(1);
 }
 
 static int cmdset_amd_init(flash_info_t *info, struct cfi_qry *qry)
@@ -1719,7 +1733,7 @@  static void flash_read_cfi (flash_info_t *info, void *buf,
 	unsigned int i;
 
 	for (i = 0; i < len; i++)
-		p[i] = flash_read_uchar(info, start + i);
+		p[i] = flash_read_uchar(info, start + (i * 2));
 }
 
 void __flash_cmd_reset(flash_info_t *info)
@@ -1730,6 +1744,7 @@  void __flash_cmd_reset(flash_info_t *info)
 	 * that AMD flash roms ignore the Intel command.
 	 */
 	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
+	udelay(1);
 	flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
 }
 void flash_cmd_reset(flash_info_t *info)
@@ -1739,21 +1754,38 @@  static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
 {
 	int cfi_offset;
 
-	/* Issue FLASH reset command */
-	flash_cmd_reset(info);
-
 	for (cfi_offset=0;
 	     cfi_offset < sizeof(flash_offset_cfi) / sizeof(uint);
 	     cfi_offset++) {
+		/* Issue FLASH reset command */
+		flash_cmd_reset(info);
 		flash_write_cmd (info, 0, flash_offset_cfi[cfi_offset],
 				 FLASH_CMD_CFI);
 		if (flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP, 'Q')
-		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 1, 'R')
-		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'Y')) {
+		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 2, 'R')
+		    && flash_isequal (info, 0, FLASH_OFFSET_CFI_RESP + 4, 'Y')) {
 			flash_read_cfi(info, qry, FLASH_OFFSET_CFI_RESP,
 					sizeof(struct cfi_qry));
+#ifdef CONFIG_SYS_FLASH_INTERFACE_WIDTH
+			info->interface = CONFIG_SYS_FLASH_INTERFACE_WIDTH;
+#else
 			info->interface	= le16_to_cpu(qry->interface_desc);
-
+			/* Some flash chips can support multiple bus widths.
+			 * In this case, override the interface width and
+			 * limit it to the port width.
+			 */
+			if ((info->interface == FLASH_CFI_X8X16) &&
+			    (info->portwidth == FLASH_CFI_8BIT)) {
+				debug("Overriding 16-bit interface width to "
+				      " 8-bit port width.\n");
+				info->interface = FLASH_CFI_X8;
+			} else if ((info->interface == FLASH_CFI_X16X32) &&
+				 (info->portwidth == FLASH_CFI_16BIT)) {
+				debug("Overriding 16-bit interface width to "
+				      " 16-bit port width.\n");
+				info->interface = FLASH_CFI_X16;
+			}
+#endif
 			info->cfi_offset = flash_offset_cfi[cfi_offset];
 			debug ("device interface is %d\n",
 			       info->interface);
@@ -1764,8 +1796,8 @@  static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
 			       info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
 
 			/* calculate command offsets as in the Linux driver */
-			info->addr_unlock1 = 0x555;
-			info->addr_unlock2 = 0x2aa;
+			info->addr_unlock1 = 0xaaa;
+			info->addr_unlock2 = 0x555;
 
 			/*
 			 * modify the unlock address if we are
@@ -1799,8 +1831,11 @@  static int flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
 		for (info->chipwidth = FLASH_CFI_BY8;
 		     info->chipwidth <= info->portwidth;
 		     info->chipwidth <<= 1)
-			if (__flash_detect_cfi(info, qry))
+			if (__flash_detect_cfi(info, qry)) {
+				debug("Found CFI flash, portwidth %d, chipwidth %d\n",
+				      info->portwidth, info->chipwidth);
 				return 1;
+			}
 	}
 	debug ("not found\n");
 	return 0;
@@ -1819,7 +1854,7 @@  static void flash_fixup_amd(flash_info_t *info, struct cfi_qry *qry)
 			/* CFI < 1.1, try to guess from device id */
 			if ((info->device_id & 0x80) != 0)
 				cfi_reverse_geometry(qry);
-		} else if (flash_read_uchar(info, info->ext_addr + 0xf) == 3) {
+		} else if (flash_read_uchar(info, info->ext_addr + 0x1e) == 3) {
 			/* CFI >= 1.1, deduct from top/bottom flag */
 			/* note: ext_addr is valid since cfi_version > 0 */
 			cfi_reverse_geometry(qry);
@@ -1891,14 +1926,15 @@  ulong flash_get_size (phys_addr_t base, int banknum)
 
 	if (flash_detect_cfi (info, &qry)) {
 		info->vendor = le16_to_cpu(qry.p_id);
-		info->ext_addr = le16_to_cpu(qry.p_adr);
+		info->ext_addr = le16_to_cpu(qry.p_adr) * 2;
+		debug("extended address is 0x%x\n", info->ext_addr);
 		num_erase_regions = qry.num_erase_regions;
 
 		if (info->ext_addr) {
 			info->cfi_version = (ushort) flash_read_uchar (info,
-						info->ext_addr + 3) << 8;
+						info->ext_addr + 6) << 8;
 			info->cfi_version |= (ushort) flash_read_uchar (info,
-						info->ext_addr + 4);
+						info->ext_addr + 8);
 		}
 
 #ifdef DEBUG
@@ -1945,13 +1981,16 @@  ulong flash_get_size (phys_addr_t base, int banknum)
 		debug ("device id is 0x%x\n", info->device_id);
 		debug ("device id2 is 0x%x\n", info->device_id2);
 		debug ("cfi version is 0x%04x\n", info->cfi_version);
-
+		debug ("port width: %d, chipwidth: %d, interface: %d\n",
+		       info->portwidth, info->chipwidth, info->interface);
 		size_ratio = info->portwidth / info->chipwidth;
+#if 0
 		/* if the chip is x8/x16 reduce the ratio by half */
 		if ((info->interface == FLASH_CFI_X8X16)
 		    && (info->chipwidth == FLASH_CFI_BY8)) {
 			size_ratio >>= 1;
 		}
+#endif
 		debug ("size_ratio %d port %d bits chip %d bits\n",
 		       size_ratio, info->portwidth << CFI_FLASH_SHIFT_WIDTH,
 		       info->chipwidth << CFI_FLASH_SHIFT_WIDTH);
@@ -2023,6 +2062,8 @@  ulong flash_get_size (phys_addr_t base, int banknum)
 				sect_cnt++;
 			}
 		}
+		debug ("port width: %d, chipwidth: %d, interface: %d\n",
+		       info->portwidth, info->chipwidth, info->interface);
 
 		info->sector_count = sect_cnt;
 		info->buffer_size = 1 << le16_to_cpu(qry.max_buf_write_size);
diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
index 3245b44..edcf9be 100644
--- a/include/mtd/cfi_flash.h
+++ b/include/mtd/cfi_flash.h
@@ -71,29 +71,29 @@ 
 #define FLASH_CONTINUATION_CODE		0x7F
 
 #define FLASH_OFFSET_MANUFACTURER_ID	0x00
-#define FLASH_OFFSET_DEVICE_ID		0x01
-#define FLASH_OFFSET_DEVICE_ID2		0x0E
-#define FLASH_OFFSET_DEVICE_ID3		0x0F
-#define FLASH_OFFSET_CFI		0x55
+#define FLASH_OFFSET_DEVICE_ID		0x02
+#define FLASH_OFFSET_DEVICE_ID2		0x1C
+#define FLASH_OFFSET_DEVICE_ID3		0x1E
+#define FLASH_OFFSET_CFI		0xAA
 #define FLASH_OFFSET_CFI_ALT		0x555
-#define FLASH_OFFSET_CFI_RESP		0x10
-#define FLASH_OFFSET_PRIMARY_VENDOR	0x13
+#define FLASH_OFFSET_CFI_RESP		0x20
+#define FLASH_OFFSET_PRIMARY_VENDOR	0x26
 /* extended query table primary address */
-#define FLASH_OFFSET_EXT_QUERY_T_P_ADDR	0x15
+#define FLASH_OFFSET_EXT_QUERY_T_P_ADDR	0x2A
 #define FLASH_OFFSET_WTOUT		0x1F
-#define FLASH_OFFSET_WBTOUT		0x20
-#define FLASH_OFFSET_ETOUT		0x21
-#define FLASH_OFFSET_CETOUT		0x22
-#define FLASH_OFFSET_WMAX_TOUT		0x23
-#define FLASH_OFFSET_WBMAX_TOUT		0x24
-#define FLASH_OFFSET_EMAX_TOUT		0x25
-#define FLASH_OFFSET_CEMAX_TOUT		0x26
-#define FLASH_OFFSET_SIZE		0x27
-#define FLASH_OFFSET_INTERFACE		0x28
-#define FLASH_OFFSET_BUFFER_SIZE	0x2A
-#define FLASH_OFFSET_NUM_ERASE_REGIONS	0x2C
-#define FLASH_OFFSET_ERASE_REGIONS	0x2D
-#define FLASH_OFFSET_PROTECT		0x02
+#define FLASH_OFFSET_WBTOUT		0x40
+#define FLASH_OFFSET_ETOUT		0x4A
+#define FLASH_OFFSET_CETOUT		0x44
+#define FLASH_OFFSET_WMAX_TOUT		0x46
+#define FLASH_OFFSET_WBMAX_TOUT		0x48
+#define FLASH_OFFSET_EMAX_TOUT		0x4A
+#define FLASH_OFFSET_CEMAX_TOUT		0x4C
+#define FLASH_OFFSET_SIZE		0x4E
+#define FLASH_OFFSET_INTERFACE		0x50
+#define FLASH_OFFSET_BUFFER_SIZE	0x54
+#define FLASH_OFFSET_NUM_ERASE_REGIONS	0x58
+#define FLASH_OFFSET_ERASE_REGIONS	0x5A
+#define FLASH_OFFSET_PROTECT		0x04
 #define FLASH_OFFSET_USER_PROTECTION	0x85
 #define FLASH_OFFSET_INTEL_PROTECTION	0x81