[U-Boot] Question about M29W128G CFI QRY bug

[Stefan Roese <sr@denx.de>]

Richard,

On Wednesday 15 April 2009, Richard Retanubun wrote:
>   > Did you take a look at the CONFIG_SYS_FLASH_CFI_AMD_RESET define? I
>   > suggest
> >
> > you give it a try and let me know if this helps.
>
> This does help but for the wrong reason:
>
> #ifdef CONFIG_SYS_FLASH_CFI_AMD_RESET /* needed for STM_ID_29W320DB on
> UC100 */ # undef  FLASH_CMD_RESET
> # define FLASH_CMD_RESET	AMD_CMD_RESET /* use AMD-Reset instead */
> #endif
>
> This just makes the reset code send two AMD_CMD_RESET.

And do you need an Intel reset command on your board? Is there an option
for Intel command style FLASH chips?

> > How is this problem solved in the Linux MTD driver (referring to your
> > first email and the link you posted there)?
>
> The Linux way, it is addressed with this patch
> ==============================================
> http://lists.infradead.org/pipermail/linux-mtd/2008-August/022499.html
>
> Which does this:
>
>   static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
>   				   unsigned long *chip_map, struct cfi_private 
*cfi)
> @@ -116,11 +87,7 @@ static int __xipram cfi_probe_chip(struct map_info
> *map, __u32 base, }
>
>   	xip_disable();
>
>  >> This is the code that is similar to our cfi-flash code <<
>
> -	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
> -	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
> -	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL);
> -
> -	if (!qry_present(map,base,cfi)) {
> +	if (!qry_mode_on(base, map, cfi)) {
>   		xip_enable(base, map, cfi);
>   		return 0;
>   	}
>
> The new qry_mode_on function handles several "odd" flash cases (yeah, more
> of these... *sob*)
>
> The quick and dirty way
> ====================================================
> 1. Swap the order: (suggested in
> http://lists.infradead.org/pipermail/linux-mtd/2008-July/022252.html )
> flash_write_cmd (info, 0, 0, FLASH_CMD_RESET);
>          flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
>
> 2. Follow AMD_CMD_RESET with a FLASH_CMD_RESET: (Suggested by an e-mail
> from Numonyx) flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
>          flash_write_cmd (info, 0, 0, FLASH_CMD_RESET);
>          flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
>
> I tested both methods and they both work on Numonyx M29W128GH (AMD cmdset)
> and Numonyx PC28F320J3D75 (Intel cmdset)

I don't like both versions. We should implement something which doesn't
change the current behavior probably needed on some other boards. So how
about something like this:

=== Cut here ====
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 175d82a..c370a64 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -1683,15 +1683,25 @@ static void flash_read_cfi (flash_info_t *info, void *buf,
 		p[i] = flash_read_uchar(info, start + i);
 }
 
+void __flash_cmd_reset(flash_info_t *info)
+{
+	/*
+	 * We do not yet know what kind of commandset to use, so we issue
+	 * the reset command in both Intel and AMD variants, in the hope
+	 * that AMD flash roms ignore the Intel command.
+	 */
+	flash_write_cmd(info, 0, 0, AMD_CMD_RESET);
+	flash_write_cmd(info, 0, 0, FLASH_CMD_RESET);
+}
+void flash_cmd_reset(flash_info_t *info)
+	__attribute__((weak,alias("__flash_cmd_reset")));
+
 static int __flash_detect_cfi (flash_info_t * info, struct cfi_qry *qry)
 {
 	int cfi_offset;
 
-	/* We do not yet know what kind of commandset to use, so we issue
-	   the reset command in both Intel and AMD variants, in the hope
-	   that AMD flash roms ignore the Intel command. */
-	flash_write_cmd (info, 0, 0, AMD_CMD_RESET);
-	flash_write_cmd (info, 0, 0, FLASH_CMD_RESET);
+	/* Issue FLASH reset command */
+	flash_cmd_reset(info);
 
 	for (cfi_offset=0;
 	     cfi_offset < sizeof(flash_offset_cfi) / sizeof(uint);
=== Cut here ====

By introducing a "weak" default reset function, you could overwrite it with
your own board specific version.

> For our local development, I'm probably going to adopt method #1 or method
> #2. If either method #1 or #2 have a chance to be mainlined, I can submit a
> patch for them with explanations.
>
> Obviously, the Linux method is probably the better solution (until another
> odd part comes along), the porting of the linux method seems like it will
> require a lot of time and testing, more than I am capable of committing
> right now.

Understood.

Best regards,
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: office at denx.de
=====================================================================