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

[Rogan Dawes <rogan@dawes.za.net>]

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