From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rogan Dawes Date: Sat, 02 Apr 2011 21:37:03 +0200 Subject: [U-Boot] [PATCH] Fix CFI flash driver for 8-bit bus support In-Reply-To: <4D970725.6080601@free.fr> References: <1301728621-27620-1-git-send-email-aaron.williams@caviumnetworks.com> <4D970725.6080601@free.fr> Message-ID: <4D977ADF.1090206@dawes.za.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > 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 >> + * > > 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 >> * >> * 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 >> #include >> @@ -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 > > 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