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

[Wolfgang Denk]

Dear Luka Perkov,

In message <20120213013045.GA25085 at w500.lan> you wrote:
> I'm not the author of version v1 of this patch but I need it to add
> support for a new board. You can see patch v1 discussion here:
> 
> http://lists.denx.de/pipermail/u-boot/2011-April/089606.html
> 
> Changes from v1:
>  * fix whitespaces
>  * remove udelay(1); calls because they have been merged in a90b9575f3ff71de58672295504e9ebaa8f051b4
>  * remove reset call in flash_erase()
> 
> Originally it was signed of by Aaron.

Patch change log and your comments do not belong into the commit
message; these should co into the comment section (below the "---"
line).

Instead, please add a description of the exact problem you are trying
to fix.

> index 722c3fc..55d866e 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -11,6 +11,9 @@
>   * Copyright (C) 2006
>   * Tolunay Orkun <listmember at orkun.us>
>   *
> + * Copyright (C) 2011 Cavium Networks, Inc.
> + * Aaron Williams <aaron.williams at caviumnetworks.com>

We don't add (C) entries for minor patches - your credits are
sufficiently recorded in the git commit log.

>  static inline void flash_unmap(flash_info_t *info, flash_sect_t sect,
> @@ -398,6 +403,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);

This makes little sense.  If this is a possible error, it should be a
printf().  But is it?

>  	/* Ensure all the instructions are fully finished */
> @@ -585,7 +592,6 @@ 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);

This is an unrelated change.  It has no place in this patch.

Please submit separately, with respective comments for why uyou are
doing this, if you really want to change this.

>  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;

I think this is bogus.   Please clean up your patch!


>  	saved_sector = sector;
> -	saved_info = info;

Ditto.

> @@ -1586,8 +1594,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);

Coding style cleanups should go in a separate patch.  In any case,
keep the blank line.

>  	/* JEDEC JEP106Z specifies ID codes up to bank 7 */
>  	while (manuId == FLASH_CONTINUATION_CODE && bankId < 0x800) {
>  		bankId += 0x100;
> @@ -1741,7 +1748,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));

On which systems / in which configurations has this code been tested?

>  void __flash_cmd_reset(flash_info_t *info)
> @@ -1762,21 +1769,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);

really?


>  	for (cfi_offset=0;
>  	     cfi_offset < sizeof(flash_offset_cfi) / sizeof(uint);
>  	     cfi_offset++) {
> +		/* Issue FLASH reset command */
> +		flash_cmd_reset(info);

really??

>  		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));

Where has this been tested?

> +			/* Some flash chips can support multiple bus widths.
> +			 * In this case, override the interface width and
> +			 * limit it to the port width.
> +			 */

Incorrect multiline comment style.

> +			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

Which configurations have been actually tested?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
On my planet, to rest is to rest -- to cease using energy. To me,  it
is  quite  illogical to run up and down on green grass, using energy,
instead of saving it.
	-- Spock, "Shore Leave", stardate 3025.2