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 Williamsaaron.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 Orkunlistmember@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 WilliamsAaron.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);@@ -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);debug("%s: 16-bit 0x%04x\n", __func__, cword.w);debug("%s: 32-bit 0x%08lx\n", __func__, cword.l);@@ -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_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;}@@ -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);+#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