Hi Thomas,
On Tuesday 23 March 2010 04:24:08 Thomas Chou wrote:
> This patch adds status polling method to offer an alternative to
> data toggle method for amd flash chips.
>
> This patch is needed for nios2 cfi flash interface, where the bus
> controller performs 4 bytes read cycles for a single byte read
> instruction. The data toggle method can not detect chip busy
> status correctly. So we have to poll DQ7, which will be inverted
> when the chip is busy.
>
> This feature is enabled with the config def,
> CONFIG_SYS_CFI_FLASH_STATUS_POLL
>
> Include patch, "drivers/mtd/cfi_flash: precision and underflow
> problem in tout calculation", submitted by
> Alessandro Rubini <rub...@gnudd.com>
> Renato Andreola <renato.andre...@imagos.it>
Thanks for taking care of this. But we shouldn't really integrate this patch
into your flash_status_poll() patch. Please re-send this part as a separate
patch (some subject as original patch with S-o-b line of original author
please). Thanks again for this.
Please find still more enhancement suggestions below.
> Signed-off-by: Thomas Chou <tho...@wytron.com.tw>
> ---
> drivers/mtd/cfi_flash.c | 78
> ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 77
> insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index fdba297..db22255 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -537,7 +537,10 @@ static int flash_status_check (flash_info_t * info,
> flash_sect_t sector, ulong start;
>
> #if CONFIG_SYS_HZ != 1000
> - tout *= CONFIG_SYS_HZ/1000;
> + if ((ulong)CONFIG_SYS_HZ > 100000)
> + tout *= (ulong)CONFIG_SYS_HZ / 1000; /* for a big HZ, avoid
overflow */
> + else
> + tout = DIV_ROUND_UP (tout * (ulong)CONFIG_SYS_HZ, 1000);
> #endif
>
> /* Wait for command completion */
> @@ -555,6 +558,53 @@ static int flash_status_check (flash_info_t * info,
> flash_sect_t sector, return ERR_OK;
> }
>
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> +static int flash_status_poll (flash_info_t *info, void *src, void *dst,
> + ulong tout, char *prompt)
> +{
> + ulong start;
> + int ready;
> +
> +#if CONFIG_SYS_HZ != 1000
> + if ((ulong)CONFIG_SYS_HZ > 100000)
> + tout *= (ulong)CONFIG_SYS_HZ / 1000; /* for a big HZ, avoid
overflow */
> + else
> + tout = DIV_ROUND_UP (tout * (ulong)CONFIG_SYS_HZ, 1000);
> +#endif
> +
> + /* Wait for command completion */
> + start = get_timer (0);
> + while (1) {
> + switch (info->portwidth) {
> + case FLASH_CFI_8BIT:
> + ready = flash_read8(dst) == flash_read8(src);
> + break;
> + case FLASH_CFI_16BIT:
> + ready = flash_read16(dst) == flash_read16(src);
> + break;
> + case FLASH_CFI_32BIT:
> + ready = flash_read32(dst) == flash_read32(src);
> + break;
> + case FLASH_CFI_64BIT:
> + ready = flash_read64(dst) == flash_read64(src);
> + break;
> + default:
> + ready = 0;
> + break;
> + }
> + if (ready)
> + break;
> + if (get_timer (start) > tout) {
> + printf ("Flash %s timeout at address %lx data %lx\n",
> + prompt, (ulong)dst, (ulong)flash_read8(dst));
> + return ERR_TIMOUT;
> + }
> + udelay (1); /* also triggers watchdog */
> + }
> + return ERR_OK;
> +}
> +#endif
> +
> /*-----------------------------------------------------------------------
> * Wait for XSR.7 to be set, if it times out print an error, otherwise
> * do a full status check.
> @@ -749,6 +799,13 @@ static int flash_write_cfiword (flash_info_t * info,
> ulong dest, if (!sect_found)
> sect = find_sector (info, dest);
>
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> + if (info->vendor == CFI_CMDSET_AMD_EXTENDED ||
> + info->vendor == CFI_CMDSET_AMD_STANDARD)
> + return flash_status_poll (info, &cword, dstaddr,
> + info->write_tout, "write");
> + else
> +#endif
> return flash_full_status_check (info, sect, info->write_tout,
"write");
I don't like this ugly "incorrect" indentation of the line below the "#endif"
above. Perhaps you could implement this in another way. By moving the #ifdef
and the vendor == amd check into a separate function:
static int use_flash_status_poll(flash_info_t *info)
{
#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
if (info->vendor == CFI_CMDSET_AMD_EXTENDED ||
info->vendor == CFI_CMDSET_AMD_STANDARD)
return 1;
#endif
return 0;
}
And then use this code in flash_write_cfiword():
if (use_flash_status_poll(info)) {
return flash_status_poll(info, &cword, dstaddr,
info->write_tout, "write");
} else {
return flash_full_status_check(info, sect,
info->write_tout, "write");
}
What do you think? Looks nicer to me. And we only need to implement this
vendor == AMD check once (see below).
> }
>
> @@ -911,9 +968,14 @@ static int flash_write_cfibuffer (flash_info_t * info,
> ulong dest, uchar * cp, }
>
> flash_write_cmd (info, sector, 0,
AMD_CMD_WRITE_BUFFER_CONFIRM);
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> + retcode = flash_status_poll (info, src - (1 << shift),
> + dst - (1 << shift), info->buffer_write_tout, "buffer
write");
> +#else
> retcode = flash_full_status_check (info, sector,
> info->buffer_write_tout,
> "buffer write");
> +#endif
> break;
>
> default:
> @@ -935,6 +997,10 @@ int flash_erase (flash_info_t * info, int s_first, int
> s_last) int rcode = 0;
> int prot;
> flash_sect_t sect;
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> + cfiword_t cword = (cfiword_t)0xffffffffffffffffULL;
> + void *dest;
> +#endif
>
> if (info->flash_id != FLASH_MAN_CFI) {
> puts ("Can't erase unknown flash type - aborted\n");
> @@ -998,6 +1064,16 @@ int flash_erase (flash_info_t * info, int s_first,
> int s_last) break;
> }
>
> +#ifdef CONFIG_SYS_CFI_FLASH_STATUS_POLL
> + dest = flash_map (info, sect, 0);
> + flash_unmap (info, sect, 0, dest);
> + if ((info->vendor == CFI_CMDSET_AMD_EXTENDED ||
> + info->vendor == CFI_CMDSET_AMD_STANDARD) &&
> + flash_status_poll (info, &cword, dest,
> + info->erase_blk_tout, "erase"))
> + rcode = 1;
> + else
> +#endif
Again, you could use the newly created function here.
Please give it a try and let me know if this works.
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: off...@denx.de
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
