[U-Boot-Users] Mixing CFI and non-CFI flashs?

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Michael Schwingen]

Hi,

I am currently porting u-boot to multiple IXP425-based boards that have 
accumulated here over the time. One of them has a slight problem, 
because it uses a combination of two flash roms:
 - one 8-bit SST 39VF020 boot flash on a 8-bit chip select as boot flash
 - one 16-bit Intel 64MBit flash on a 16-bit chip select.

The intel flash is CFI, while the SST flash is not.
I want to use both flash roms, and I want to use as much of the existing 
flash code as possible. It seems that when using the CFI code, there is 
no easy way to add  a non-CFI flash - right?

What would be the best way to use both flash roms? I have come up with:

(1) provide a hook in cfi_flash.c to insert hardcoded values into the 
flash_info structure for the non-CFI flash rom. Due to differences 
between CFI-AMD and non-CFI AMD flash command cycles, I need to  add a 
new commandset (eg. CFI_CMDSET_AMD_LEGACY) plus a bit of handler code.

(2) rename all external-visible functions in flash_cfi.c and add wrapper 
functions that call either the CFI code or my own code for the non-CFI 
flash. This has the disadvantage that I need to duplicate code that is 
already present in cfi_flash.c, but is not exported. I have ssen this 
done on one chip vendor's u-boot port, and it does not really look nice.

(3) extend the code in cfi_flash.c so that external functions can be 
added via function pointers for non-CFI flashs. Same disadvantage as (2).

I have implemented solution (1) now, and it seems to work quite well, 
with minimal code needed in the board-setup file, but maybe there are 
better ways to do this - comments?

cu
Michael

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Stefan Roese]

Hi Michael,

On Friday 02 November 2007, Michael Schwingen wrote:
> I am currently porting u-boot to multiple IXP425-based boards that have
> accumulated here over the time. One of them has a slight problem,
> because it uses a combination of two flash roms:
>  - one 8-bit SST 39VF020 boot flash on a 8-bit chip select as boot flash
>  - one 16-bit Intel 64MBit flash on a 16-bit chip select.
>
> The intel flash is CFI, while the SST flash is not.
> I want to use both flash roms, and I want to use as much of the existing
> flash code as possible. It seems that when using the CFI code, there is
> no easy way to add  a non-CFI flash - right?

Correct. This is currently known limitation.

> What would be the best way to use both flash roms? I have come up with:
>
> (1) provide a hook in cfi_flash.c to insert hardcoded values into the
> flash_info structure for the non-CFI flash rom. Due to differences
> between CFI-AMD and non-CFI AMD flash command cycles, I need to  add a
> new commandset (eg. CFI_CMDSET_AMD_LEGACY) plus a bit of handler code.
>
> (2) rename all external-visible functions in flash_cfi.c and add wrapper
> functions that call either the CFI code or my own code for the non-CFI
> flash. This has the disadvantage that I need to duplicate code that is
> already present in cfi_flash.c, but is not exported. I have ssen this
> done on one chip vendor's u-boot port, and it does not really look nice.
>
> (3) extend the code in cfi_flash.c so that external functions can be
> added via function pointers for non-CFI flashs. Same disadvantage as (2).
>
> I have implemented solution (1) now, and it seems to work quite well,
> with minimal code needed in the board-setup file, but maybe there are
> better ways to do this - comments?

I remember a discussion we had a while ago, about supporting non-CFI chips in 
a common driver, but I can't find the mails right now. IIRC, the idea was to 
add something close to the Linux JEDEC probe 
(drivers/mtd/chips/jedec_probe.c) for this.

I suggest that you post your current version for review. This sounds like a 
start in the correct direction.

Thanks.

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

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Michael Schwingen]

On Sat, Nov 03, 2007 at 08:13:40AM +0100, Stefan Roese wrote:
> I remember a discussion we had a while ago, about supporting non-CFI chips in 
> a common driver, but I can't find the mails right now. IIRC, the idea was to 
> add something close to the Linux JEDEC probe 
> (drivers/mtd/chips/jedec_probe.c) for this.

Good idea. I currently hard-coded the flash parameters for the boot flash,
since I know there is exactly one type on the boards, but having
jedec-probing would be nicer.

I think the CFI driver is a good start as a framework - all the
infrastructure for handling mutliple banks, different chip/bus mappings etc.
is there and could be used by jedec-flash code. For now, it is probably
easiest to add the JEDEC code to cfi_flash.c, to get access to the static
functions defined there, and then later split it out if necessary.

> I suggest that you post your current version for review. This sounds like a 
> start in the correct direction.

Fine. Here is what I have got now - it works for my board, but would
probably need some cleanup/renaming of symbols. The basic idea is that the
board-specific code provides a function "flash_detect_legacy" that is called
for every flash bank, which can provide hardcoded (or in the future
jedec-probed) flash information. If the function returns 0, the normal CFI
code is called.

Since that code is board-specific anyway, I think the board-specific code
can supply the information about chipwidth, buswidth etc. - there is no real
benefit in autoprobing these, and there is less probability for problems
(like 8-bit accesses to a 16-bit bus causing problems, etc.).


I have included one non-conditional patch I made:
@@ -1282,6 +1310,8 @@ ulong flash_get_size (ulong base, int banknum)
which causes the CFI code to stop in case CFG_MAX_FLASH_SECT is defined too
small - without that patch, u-boot overwrites other data and crashes
silently during startup. 

cu
Michael

Signed-off-by: Michael Schwingen <michael@schwingen.org>

diff --git a/include/flash.h b/include/flash.h
index b0bf733..f0ef6f7 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -101,6 +101,11 @@ extern void flash_read_user_serial(flash_info_t * info,
void * buffer, int offse
 extern void flash_read_factory_serial(flash_info_t * info, void * buffer,
int offset, int len);
 #endif /* CFG_FLASH_PROTECTION */
 
+#ifdef CFG_FLASH_CFI_LEGACY
+extern ulong flash_detect_legacy(ulong base, int banknum);
+#define CFI_CMDSET_AMD_LEGACY          0xFFF0
+#endif
+
 /*-----------------------------------------------------------------------
  * return codes from flash_write():
  */


diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
index 5579a1e..20ccc3a 100644
--- a/drivers/cfi_flash.c
+++ b/drivers/cfi_flash.c
@@ -345,7 +345,13 @@ unsigned long flash_init (void)
 	/* Init: no FLASHes known */
 	for (i = 0; i < CFG_MAX_FLASH_BANKS; ++i) {
 		flash_info[i].flash_id = FLASH_UNKNOWN;
-		size += flash_info[i].size = flash_get_size (bank_base[i], i);
+
+#ifdef CFG_FLASH_CFI_LEGACY
+		flash_info[i].size = flash_detect_legacy (bank_base[i], i);
+		if (flash_info[i].size == 0)
+#endif
+		flash_info[i].size = flash_get_size (bank_base[i], i);
+		size += flash_info[i].size;
 		if (flash_info[i].flash_id == FLASH_UNKNOWN) {
 #ifndef CFG_FLASH_QUIET_TEST
 			printf ("## Unknown FLASH on Bank %d - Size = 0x%08lx = %ld MB\n",
@@ -488,6 +494,16 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
 				flash_unlock_seq (info, sect);
 				flash_write_cmd (info, sect, 0, AMD_CMD_ERASE_SECTOR);
 				break;
+#ifdef CFG_FLASH_CFI_LEGACY
+			case CFI_CMDSET_AMD_LEGACY:
+				flash_write_cmd (info, 0,    0x5555, AMD_CMD_UNLOCK_START);
+				flash_write_cmd (info, 0,    0x2AAA, AMD_CMD_UNLOCK_ACK);
+				flash_write_cmd (info, 0,    0x5555, AMD_CMD_ERASE_START);
+				flash_write_cmd (info, 0,    0x5555, AMD_CMD_UNLOCK_START);
+				flash_write_cmd (info, 0,    0x2AAA, AMD_CMD_UNLOCK_ACK);
+				flash_write_cmd (info, sect, 0,      AMD_CMD_ERASE_SECTOR);
+				break;
+#endif
 			default:
 				debug ("Unkown flash vendor %d\n",
 				       info->vendor);
@@ -518,8 +534,12 @@ void flash_print_info (flash_info_t * info)
 
 	printf ("CFI conformant FLASH (%d x %d)",
 		(info->portwidth << 3), (info->chipwidth << 3));
-	printf ("  Size: %ld MB in %d Sectors\n",
-		info->size >> 20, info->sector_count);
+	if (info->size < 1024*1024)
+		printf ("  Size: %ld kB in %d Sectors\n",
+			info->size >> 10, info->sector_count);
+	else
+		printf ("  Size: %ld MB in %d Sectors\n",
+			info->size >> 20, info->sector_count);
 	printf ("  ");
 	switch (info->vendor) {
 		case CFI_CMDSET_INTEL_STANDARD:
@@ -534,6 +554,11 @@ void flash_print_info (flash_info_t * info)
 		case CFI_CMDSET_AMD_EXTENDED:
 			printf ("AMD Extended");
 			break;
+#ifdef CFG_FLASH_CFI_LEGACY
+		case CFI_CMDSET_AMD_LEGACY:
+			printf ("AMD (non-CFI)");
+			break;
+#endif
 		default:
 			printf ("Unknown (%d)", info->vendor);
 			break;
@@ -777,6 +802,9 @@ static int flash_is_busy (flash_info_t * info, flash_sect_t sect)
 		break;
 	case CFI_CMDSET_AMD_STANDARD:
 	case CFI_CMDSET_AMD_EXTENDED:
+#ifdef CFG_FLASH_CFI_LEGACY
+	case CFI_CMDSET_AMD_LEGACY:
+#endif
 		retval = flash_toggle (info, sect, 0, AMD_STATUS_TOGGLE);
 		break;
 	default:
@@ -1282,6 +1310,8 @@ ulong flash_get_size (ulong base, int banknum)
 			debug ("erase_region_count = %d erase_region_size = %d\n",
 				erase_region_count, erase_region_size);
 			for (j = 0; j < erase_region_count; j++) {
+				if (sect_cnt >= CFG_MAX_FLASH_SECT)
+					break;
 				info->start[sect_cnt] = sector;
 				sector += (erase_region_size * size_ratio);
 
@@ -1387,6 +1417,12 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
 		flash_unlock_seq (info, 0);
 		flash_write_cmd (info, 0, AMD_ADDR_START, AMD_CMD_WRITE);
 		break;
+#ifdef CFG_FLASH_CFI_LEGACY
+	case CFI_CMDSET_AMD_LEGACY:
+		flash_write_cmd (info, 0, 0x5555, AMD_CMD_UNLOCK_START);
+		flash_write_cmd (info, 0, 0x2AAA, AMD_CMD_UNLOCK_ACK);
+		flash_write_cmd (info, 0, 0x5555, AMD_CMD_WRITE);
+#endif
 	}
 
 	switch (info->portwidth) {





and then in the board-init code:

extern flash_info_t flash_info[];
#define AMD_CMD_RESET			0xF0

ulong flash_detect_legacy(ulong base, int banknum)
{
	if (banknum == 0) {
		flash_info_t *info = &flash_info[banknum];
		int i;
		unsigned long sector;

		info->portwidth = 1;
		info->chipwidth = 1;
		info->ext_addr = 0;
		info->cfi_version = 0;
		info->legacy_unlock = 0;
		info->start[0] = base;
		info->cmd_reset = AMD_CMD_RESET;
		info->vendor = CFI_CMDSET_AMD_LEGACY;
		info->flash_id = FLASH_MAN_CFI;

		info->sector_count = 64;

		for (i = 0, sector = base; i < info->sector_count; i++) {
			info->protect[i] = 0;
			info->start[i] = sector;
			sector += 4096;
		}

		info->buffer_size = 1;
		info->erase_blk_tout = 10000;
		info->buffer_write_tout = 1000;
		info->write_tout = 100;
		return 256*1024;
	}
	else
		return 0;
}

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Stefan Roese]

Hi Michael,

On Saturday 03 November 2007, Michael Schwingen wrote:
> Since that code is board-specific anyway, I think the board-specific code
> can supply the information about chipwidth, buswidth etc. - there is no
> real benefit in autoprobing these, and there is less probability for
> problems (like 8-bit accesses to a 16-bit bus causing problems, etc.).

Yes, I think this is ok for now.

> I have included one non-conditional patch I made:
> @@ -1282,6 +1310,8 @@ ulong flash_get_size (ulong base, int banknum)
> which causes the CFI code to stop in case CFG_MAX_FLASH_SECT is defined too
> small - without that patch, u-boot overwrites other data and crashes
> silently during startup.

Good catch. I remember seeing a patch dealing with this problem before.

Please find some first comments below.

> cu
> Michael
>
> Signed-off-by: Michael Schwingen <michael@schwingen.org>
>
> diff --git a/include/flash.h b/include/flash.h
> index b0bf733..f0ef6f7 100644
> --- a/include/flash.h
> +++ b/include/flash.h
> @@ -101,6 +101,11 @@ extern void flash_read_user_serial(flash_info_t *
> info, void * buffer, int offse
>  extern void flash_read_factory_serial(flash_info_t * info, void * buffer,
> int offset, int len);
>  #endif /* CFG_FLASH_PROTECTION */
>
> +#ifdef CFG_FLASH_CFI_LEGACY
> +extern ulong flash_detect_legacy(ulong base, int banknum);
> +#define CFI_CMDSET_AMD_LEGACY          0xFFF0
> +#endif
> +
>  /*-----------------------------------------------------------------------
>   * return codes from flash_write():
>   */
>
>
> diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
> index 5579a1e..20ccc3a 100644
> --- a/drivers/cfi_flash.c
> +++ b/drivers/cfi_flash.c
> @@ -345,7 +345,13 @@ unsigned long flash_init (void)
>  	/* Init: no FLASHes known */
>  	for (i = 0; i < CFG_MAX_FLASH_BANKS; ++i) {
>  		flash_info[i].flash_id = FLASH_UNKNOWN;
> -		size += flash_info[i].size = flash_get_size (bank_base[i], i);
> +
> +#ifdef CFG_FLASH_CFI_LEGACY
> +		flash_info[i].size = flash_detect_legacy (bank_base[i], i);
> +		if (flash_info[i].size == 0)
> +#endif
> +		flash_info[i].size = flash_get_size (bank_base[i], i);
> +		size += flash_info[i].size;

I don't like the indentation problem we get from this #ifdef here. Perhaps we 
should add a __weak__ function flash_detect_legacy() in this file, that can 
be overridden by board specific functions. Like this:

ulong __flash_detect_legacy(ulong base, int banknum)
{
	return 0;
}
ulong flash_detect_legacy(ulong base, int banknum) __attribute__((weak, 
alias("__flash_detect_legacy")));

This way we get rid of the #ifdef too.

>  		if (flash_info[i].flash_id == FLASH_UNKNOWN) {
>  #ifndef CFG_FLASH_QUIET_TEST
>  			printf ("## Unknown FLASH on Bank %d - Size = 0x%08lx = %ld MB\n",
> @@ -488,6 +494,16 @@ int flash_erase (flash_info_t * info, int s_first, int
> s_last) flash_unlock_seq (info, sect);
>  				flash_write_cmd (info, sect, 0, AMD_CMD_ERASE_SECTOR);
>  				break;
> +#ifdef CFG_FLASH_CFI_LEGACY
> +			case CFI_CMDSET_AMD_LEGACY:
> +				flash_write_cmd (info, 0,    0x5555, AMD_CMD_UNLOCK_START);
> +				flash_write_cmd (info, 0,    0x2AAA, AMD_CMD_UNLOCK_ACK);
> +				flash_write_cmd (info, 0,    0x5555, AMD_CMD_ERASE_START);
> +				flash_write_cmd (info, 0,    0x5555, AMD_CMD_UNLOCK_START);
> +				flash_write_cmd (info, 0,    0x2AAA, AMD_CMD_UNLOCK_ACK);
> +				flash_write_cmd (info, sect, 0,      AMD_CMD_ERASE_SECTOR);
> +				break;
> +#endif
>  			default:
>  				debug ("Unkown flash vendor %d\n",
>  				       info->vendor);
> @@ -518,8 +534,12 @@ void flash_print_info (flash_info_t * info)
>
>  	printf ("CFI conformant FLASH (%d x %d)",
>  		(info->portwidth << 3), (info->chipwidth << 3));
> -	printf ("  Size: %ld MB in %d Sectors\n",
> -		info->size >> 20, info->sector_count);
> +	if (info->size < 1024*1024)
> +		printf ("  Size: %ld kB in %d Sectors\n",
> +			info->size >> 10, info->sector_count);
> +	else
> +		printf ("  Size: %ld MB in %d Sectors\n",
> +			info->size >> 20, info->sector_count);
>  	printf ("  ");
>  	switch (info->vendor) {
>  		case CFI_CMDSET_INTEL_STANDARD:
> @@ -534,6 +554,11 @@ void flash_print_info (flash_info_t * info)
>  		case CFI_CMDSET_AMD_EXTENDED:
>  			printf ("AMD Extended");
>  			break;
> +#ifdef CFG_FLASH_CFI_LEGACY
> +		case CFI_CMDSET_AMD_LEGACY:
> +			printf ("AMD (non-CFI)");
> +			break;
> +#endif
>  		default:
>  			printf ("Unknown (%d)", info->vendor);
>  			break;
> @@ -777,6 +802,9 @@ static int flash_is_busy (flash_info_t * info,
> flash_sect_t sect) break;
>  	case CFI_CMDSET_AMD_STANDARD:
>  	case CFI_CMDSET_AMD_EXTENDED:
> +#ifdef CFG_FLASH_CFI_LEGACY
> +	case CFI_CMDSET_AMD_LEGACY:
> +#endif
>  		retval = flash_toggle (info, sect, 0, AMD_STATUS_TOGGLE);
>  		break;
>  	default:
> @@ -1282,6 +1310,8 @@ ulong flash_get_size (ulong base, int banknum)
>  			debug ("erase_region_count = %d erase_region_size = %d\n",
>  				erase_region_count, erase_region_size);
>  			for (j = 0; j < erase_region_count; j++) {
> +				if (sect_cnt >= CFG_MAX_FLASH_SECT)
> +					break;

Please add an error output here too.

Looks good otherwise. Thanks.

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

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Michael Schwingen]

On Mon, Nov 05, 2007 at 12:21:50PM +0100, Stefan Roese wrote:
> 
> I don't like the indentation problem we get from this #ifdef here. Perhaps we 
> should add a __weak__ function flash_detect_legacy() in this file, that can 
> be overridden by board specific functions. Like this:
> 
> ulong __flash_detect_legacy(ulong base, int banknum)
> {
> 	return 0;
> }
> ulong flash_detect_legacy(ulong base, int banknum) __attribute__((weak, 
> alias("__flash_detect_legacy")));
> 
> This way we get rid of the #ifdef too.

Hm - I need some common code at that point, which would otherwise be
duplicated in every board code. I have now moved the code to a function
flash_detect_legacy.

> >  			for (j = 0; j < erase_region_count; j++) {
> > +				if (sect_cnt >= CFG_MAX_FLASH_SECT)
> > +					break;
> 
> Please add an error output here too.

Done.


Okey, next version. The board-specific code may either fill out the complete
flash_info struct as in the previous patch, or (preferred), only set
info->portwidth, info->chipwidth and info->interface, in which case the code
will probe the flash by jedec IDs and look up a table in jedec_flash.c. The
table is a near copy of the table in the Linux jedec_flash.c, with the
removal of some unused fields.

There is one problem with the AMD_ADDR_* definitions: I believe they are
only correct for 16-bit flash ROMS (in 8/16 bit mode), but are wrong for
8-bit flashs. I think the CFI "interface" parameter should be the correct
way to distinguish these cases. What is left is the number of bits in the
unlock addresses - I have used 0x2AAA/0x5555, since all flash roms I know
treat the upper bits as "don't care" (and specify that behaviour in the
datasheet), so if the flash datasheet specifies 0xAAA/0x555, using the
longer constants should do no harm. However, there are probably lots of
flash roms I do *not* know, so I would appreciate feedback on this. If
different addresses are really necessary, we would need to add them to the
flash_info struct.

This currently works on my IXP425 board (big endian) with one SST39VF020 and
one Intel TE28F640J3. I am not sure if the jedec flash code is correct in
case of 16-bit JEDEC flashs or multiple 8-bit flash roms on a wider data
bus.

Signed-off-by: Michael Schwingen <michael@schwingen.org>

diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
index 5579a1e..399deab 100644
--- a/drivers/cfi_flash.c
+++ b/drivers/cfi_flash.c
@@ -98,9 +98,9 @@
 #define AMD_STATUS_TOGGLE		0x40
 #define AMD_STATUS_ERROR		0x20
 
-#define AMD_ADDR_ERASE_START	((info->portwidth == FLASH_CFI_8BIT) ? 0xAAA : 0x555)
-#define AMD_ADDR_START		((info->portwidth == FLASH_CFI_8BIT) ? 0xAAA : 0x555)
-#define AMD_ADDR_ACK		((info->portwidth == FLASH_CFI_8BIT) ? 0x555 : 0x2AA)
+#define AMD_ADDR_ERASE_START	((info->portwidth == FLASH_CFI_8BIT && info->interface == FLASH_CFI_X8X16) ? 0x2AAA : 0x5555)
+#define AMD_ADDR_START		((info->portwidth == FLASH_CFI_8BIT && info->interface == FLASH_CFI_X8X16) ? 0x2AAA : 0x5555)
+#define AMD_ADDR_ACK		((info->portwidth == FLASH_CFI_8BIT && info->interface == FLASH_CFI_X8X16) ? 0x5555 : 0x2AAA)
 
 #define FLASH_OFFSET_MANUFACTURER_ID	0x00
 #define FLASH_OFFSET_DEVICE_ID		0x01
@@ -331,6 +331,43 @@ ulong flash_read_long (flash_info_t * info, flash_sect_t sect, uint offset)
 }
 
 
+#ifdef CFG_FLASH_CFI_LEGACY
+ulong flash_detect_legacy(ulong base, int banknum)
+{
+	flash_info_t *info = &flash_info[banknum];
+	if (board_flash_get_legacy(base, banknum, info)) {
+		/* board code may have filled info completely. If not, we
+		   use JEDEC ID probing. */
+		if (!info->vendor) {
+			int modes[] = { CFI_CMDSET_AMD_STANDARD, CFI_CMDSET_INTEL_STANDARD };
+			int i;
+
+			for(i=0; i<sizeof(modes)/sizeof(modes[0]); i++) {
+				info->vendor = modes[i];
+				info->start[0] = base;
+				flash_read_jedec_ids(info);
+				debug("JEDEC PROBE: ID %x %x %x\n", info->manufacturer_id, info->device_id, info->device_id2);
+				if (jedec_flash_match(info, base))
+					break;
+			}
+		}
+		switch(info->vendor) {
+		case CFI_CMDSET_AMD_LEGACY:
+			info->cmd_reset = AMD_CMD_RESET;
+			break;
+		}
+		info->flash_id = FLASH_MAN_CFI;
+	}
+	return info->size;
+}
+#else
+ulong inline flash_detect_legacy(ulong base, int banknum)
+{
+	return 0;
+}
+#endif
+
+
 /*-----------------------------------------------------------------------
  */
 unsigned long flash_init (void)
@@ -345,7 +382,11 @@ unsigned long flash_init (void)
 	/* Init: no FLASHes known */
 	for (i = 0; i < CFG_MAX_FLASH_BANKS; ++i) {
 		flash_info[i].flash_id = FLASH_UNKNOWN;
-		size += flash_info[i].size = flash_get_size (bank_base[i], i);
+
+		flash_detect_legacy (bank_base[i], i);
+		if (flash_info[i].size == 0)
+			flash_info[i].size = flash_get_size (bank_base[i], i);
+		size += flash_info[i].size;
 		if (flash_info[i].flash_id == FLASH_UNKNOWN) {
 #ifndef CFG_FLASH_QUIET_TEST
 			printf ("## Unknown FLASH on Bank %d - Size = 0x%08lx = %ld MB\n",
@@ -488,6 +529,16 @@ int flash_erase (flash_info_t * info, int s_first, int s_last)
 				flash_unlock_seq (info, sect);
 				flash_write_cmd (info, sect, 0, AMD_CMD_ERASE_SECTOR);
 				break;
+#ifdef CFG_FLASH_CFI_LEGACY
+			case CFI_CMDSET_AMD_LEGACY:
+				flash_write_cmd (info, 0,    0x5555, AMD_CMD_UNLOCK_START);
+				flash_write_cmd (info, 0,    0x2AAA, AMD_CMD_UNLOCK_ACK);
+				flash_write_cmd (info, 0,    0x5555, AMD_CMD_ERASE_START);
+				flash_write_cmd (info, 0,    0x5555, AMD_CMD_UNLOCK_START);
+				flash_write_cmd (info, 0,    0x2AAA, AMD_CMD_UNLOCK_ACK);
+				flash_write_cmd (info, sect, 0,      AMD_CMD_ERASE_SECTOR);
+				break;
+#endif
 			default:
 				debug ("Unkown flash vendor %d\n",
 				       info->vendor);
@@ -518,8 +569,12 @@ void flash_print_info (flash_info_t * info)
 
 	printf ("CFI conformant FLASH (%d x %d)",
 		(info->portwidth << 3), (info->chipwidth << 3));
-	printf ("  Size: %ld MB in %d Sectors\n",
-		info->size >> 20, info->sector_count);
+	if (info->size < 1024*1024)
+		printf ("  Size: %ld kB in %d Sectors\n",
+			info->size >> 10, info->sector_count);
+	else
+		printf ("  Size: %ld MB in %d Sectors\n",
+			info->size >> 20, info->sector_count);
 	printf ("  ");
 	switch (info->vendor) {
 		case CFI_CMDSET_INTEL_STANDARD:
@@ -534,6 +589,11 @@ void flash_print_info (flash_info_t * info)
 		case CFI_CMDSET_AMD_EXTENDED:
 			printf ("AMD Extended");
 			break;
+#ifdef CFG_FLASH_CFI_LEGACY
+		case CFI_CMDSET_AMD_LEGACY:
+			printf ("AMD (non-CFI)");
+			break;
+#endif
 		default:
 			printf ("Unknown (%d)", info->vendor);
 			break;
@@ -777,6 +837,9 @@ static int flash_is_busy (flash_info_t * info, flash_sect_t sect)
 		break;
 	case CFI_CMDSET_AMD_STANDARD:
 	case CFI_CMDSET_AMD_EXTENDED:
+#ifdef CFG_FLASH_CFI_LEGACY
+	case CFI_CMDSET_AMD_LEGACY:
+#endif
 		retval = flash_toggle (info, sect, 0, AMD_STATUS_TOGGLE);
 		break;
 	default:
@@ -1282,6 +1345,10 @@ ulong flash_get_size (ulong base, int banknum)
 			debug ("erase_region_count = %d erase_region_size = %d\n",
 				erase_region_count, erase_region_size);
 			for (j = 0; j < erase_region_count; j++) {
+				if (sect_cnt >= CFG_MAX_FLASH_SECT) {
+					printf("ERROR: too many flash sectors\n");
+					break;
+				}
 				info->start[sect_cnt] = sector;
 				sector += (erase_region_size * size_ratio);
 
@@ -1387,6 +1454,12 @@ static int flash_write_cfiword (flash_info_t * info, ulong dest,
 		flash_unlock_seq (info, 0);
 		flash_write_cmd (info, 0, AMD_ADDR_START, AMD_CMD_WRITE);
 		break;
+#ifdef CFG_FLASH_CFI_LEGACY
+	case CFI_CMDSET_AMD_LEGACY:
+		flash_write_cmd (info, 0, 0x5555, AMD_CMD_UNLOCK_START);
+		flash_write_cmd (info, 0, 0x2AAA, AMD_CMD_UNLOCK_ACK);
+		flash_write_cmd (info, 0, 0x5555, AMD_CMD_WRITE);
+#endif
 	}
 
 	switch (info->portwidth) {
diff --git a/include/flash.h b/include/flash.h
index b0bf733..c4d8ded 100644
--- a/include/flash.h
+++ b/include/flash.h
@@ -101,6 +101,12 @@ extern void flash_read_user_serial(flash_info_t * info, void * buffer, int offse
 extern void flash_read_factory_serial(flash_info_t * info, void * buffer, int offset, int len);
 #endif	/* CFG_FLASH_PROTECTION */
 
+#ifdef CFG_FLASH_CFI_LEGACY
+extern ulong board_flash_get_legacy(ulong base, int banknum, flash_info_t *info);
+extern int jedec_flash_match(flash_info_t *info, ulong base);
+#define CFI_CMDSET_AMD_LEGACY		0xFFF0
+#endif
+
 /*-----------------------------------------------------------------------
  * return codes from flash_write():
  */


new file jedec_flash.c:

/*
 * (C) Copyright 2007
 * Michael Schwingen, <michael@schwingen.org>
 *
 * based in great part on jedec_probe.c from linux kernel:
 * (C) 2000 Red Hat. GPL'd.
 * Occasionally maintained by Thayne Harbaugh tharbaugh at lnxi dot com
 *
 * See file CREDITS for list of people who contributed to this
 * project.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License as
 * published by the Free Software Foundation; either version 2 of
 * the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 * MA 02111-1307 USA
 *
 */

/* The DEBUG define must be before common to enable debugging */
#define DEBUG

#include <common.h>
#include <asm/processor.h>
#include <asm/io.h>
#include <asm/byteorder.h>
#include <environment.h>

#if defined CFG_FLASH_CFI_DRIVER && defined CFG_FLASH_CFI_LEGACY

#define P_ID_AMD_STD CFI_CMDSET_AMD_LEGACY

/* Manufacturers */
#define MANUFACTURER_SST	0x00BF


/* SST */
#define SST39LF800	0x2781
#define SST39LF160	0x2782
#define SST39VF1601	0x234b
#define SST39LF512	0x00D4
#define SST39LF010	0x00D5
#define SST39LF020	0x00D6
#define SST39LF040	0x00D7
#define SST39SF010A	0x00B5
#define SST39SF020A	0x00B6


struct amd_flash_info {
	const __u16 mfr_id;
	const __u16 dev_id;
	const char *name;
	const int NumEraseRegions;
	const int CmdSet;
	const ulong regions[6];
};

#define ERASEINFO(size,blocks) (size<<8)|(blocks-1)

/*
 * Please keep this list ordered by manufacturer!
 * Fortunately, the list isn't searched often and so a
 * slow, linear search isn't so bad.
 */
static const struct amd_flash_info jedec_table[] = {
        {
		.mfr_id		= MANUFACTURER_SST,
		.dev_id		= SST39LF010,
		.name		= "SST 39LF010",
		.CmdSet		= P_ID_AMD_STD,
		.NumEraseRegions= 1,
		.regions	= {
			ERASEINFO(0x01000,32),
		}
	}, {
		.mfr_id		= MANUFACTURER_SST,
		.dev_id		= SST39LF020,
		.name		= "SST 39LF020",
		.CmdSet		= P_ID_AMD_STD,
		.NumEraseRegions= 1,
		.regions	= {
			ERASEINFO(0x01000,64),
		}
        }, {
		.mfr_id		= MANUFACTURER_SST,
		.dev_id		= SST39LF040,
		.name		= "SST 39LF040",
		.CmdSet		= P_ID_AMD_STD,
		.NumEraseRegions= 1,
		.regions	= {
			ERASEINFO(0x01000,128),
		}
        }
};


#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))


static inline void fill_info(flash_info_t *info, const struct amd_flash_info *jedec_entry, ulong base)
{
	int i,j;
	int sect_cnt;
	int size_ratio;
	int total_size;

	size_ratio = info->portwidth / info->chipwidth;

	debug("Found JEDEC Flash: %s\n", jedec_entry->name);
	info->vendor = jedec_entry->CmdSet;
	/* Todo: do we need device-specific timeouts? */
	info->erase_blk_tout = 30000;
	info->buffer_write_tout = 1000;
	info->write_tout = 100;

	sect_cnt = 0;
	total_size = 0;
	for (i = 0; i < jedec_entry->NumEraseRegions; i++) {
		ulong erase_region_size = jedec_entry->regions[i] >> 8;
		ulong erase_region_count = (jedec_entry->regions[i] & 0xff) + 1;

		total_size += erase_region_size * erase_region_count;
		debug ("erase_region_count = %d erase_region_size = %d\n",
		       erase_region_count, erase_region_size);
		for (j = 0; j < erase_region_count; j++) {
			if (sect_cnt >= CFG_MAX_FLASH_SECT) {
				printf("ERROR: too many flash sectors\n");
				break;
			}
			info->start[sect_cnt] = base;
			base += (erase_region_size * size_ratio);
			sect_cnt++;
		}
	}
	info->sector_count = sect_cnt;
	info->size = total_size * size_ratio;
}

/*-----------------------------------------------------------------------
 * match jedec ids against table. If a match is found, fill flash_info entry
 */
int jedec_flash_match(flash_info_t *info, ulong base)
{
	int ret = 0;
	int i;
	ulong mask = 0xFFFF;
	if (info->chipwidth == 1)
		mask = 0xFF;

	for (i = 0; i < ARRAY_SIZE(jedec_table); i++) {
		if ( (jedec_table[i].mfr_id & mask) == (info->manufacturer_id & mask) &&
		     (jedec_table[i].dev_id & mask) == (info->device_id & mask)) {
			fill_info(info, &jedec_table[i], base);
			ret = 1;
			break;
		}
	}
	return ret;
}



#endif /* defined CFG_FLASH_CFI_DRIVER && defined CFG_FLASH_CFI_LEGACY */



And finally, the board-specific code:


ulong board_flash_get_legacy(ulong base, int banknum, flash_info_t *info)
{
	if (banknum == 0) {
		info->portwidth = 1;
		info->chipwidth = 1;
		info->interface = FLASH_CFI_X8;
		return 1;
	}
	else
		return 0;
}


cu
Michael

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Stefan Roese]

On Tuesday 06 November 2007, Michael Schwingen wrote:
> On Mon, Nov 05, 2007 at 12:21:50PM +0100, Stefan Roese wrote:
> > I don't like the indentation problem we get from this #ifdef here.
> > Perhaps we should add a __weak__ function flash_detect_legacy() in this
> > file, that can be overridden by board specific functions. Like this:
> >
> > ulong __flash_detect_legacy(ulong base, int banknum)
> > {
> > 	return 0;
> > }
> > ulong flash_detect_legacy(ulong base, int banknum) __attribute__((weak,
> > alias("__flash_detect_legacy")));
> >
> > This way we get rid of the #ifdef too.
>
> Hm - I need some common code at that point, which would otherwise be
> duplicated in every board code. I have now moved the code to a function
> flash_detect_legacy.

OK.

> > >  			for (j = 0; j < erase_region_count; j++) {
> > > +				if (sect_cnt >= CFG_MAX_FLASH_SECT)
> > > +					break;
> >
> > Please add an error output here too.
>
> Done.
>
>
> Okey, next version. The board-specific code may either fill out the
> complete flash_info struct as in the previous patch, or (preferred), only
> set info->portwidth, info->chipwidth and info->interface, in which case the
> code will probe the flash by jedec IDs and look up a table in
> jedec_flash.c. The table is a near copy of the table in the Linux
> jedec_flash.c, with the removal of some unused fields.

Wouldn't it be better not to remove those unused fields? This way porting of 
new devices from Linux to U-Boot would be easier.

> There is one problem with the AMD_ADDR_* definitions: I believe they are
> only correct for 16-bit flash ROMS (in 8/16 bit mode), but are wrong for
> 8-bit flashs. I think the CFI "interface" parameter should be the correct
> way to distinguish these cases. What is left is the number of bits in the
> unlock addresses - I have used 0x2AAA/0x5555, since all flash roms I know
> treat the upper bits as "don't care" (and specify that behaviour in the
> datasheet), so if the flash datasheet specifies 0xAAA/0x555, using the
> longer constants should do no harm. However, there are probably lots of
> flash roms I do *not* know, so I would appreciate feedback on this. If
> different addresses are really necessary, we would need to add them to the
> flash_info struct.

Correct. And this is the case in the current Linux implementation. So we 
should not remove it, even if we don't use it for now.

> This currently works on my IXP425 board (big endian) with one SST39VF020
> and one Intel TE28F640J3. I am not sure if the jedec flash code is correct
> in case of 16-bit JEDEC flashs or multiple 8-bit flash roms on a wider data
> bus.

Thanks. Very nice work.

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

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Michael Schwingen]

On Tue, Nov 06, 2007 at 08:48:39AM +0100, Stefan Roese wrote:
> > Okey, next version. The board-specific code may either fill out the
> > complete flash_info struct as in the previous patch, or (preferred), only
> > set info->portwidth, info->chipwidth and info->interface, in which case the
> > code will probe the flash by jedec IDs and look up a table in
> > jedec_flash.c. The table is a near copy of the table in the Linux
> > jedec_flash.c, with the removal of some unused fields.
> 
> Wouldn't it be better not to remove those unused fields? This way porting of 
> new devices from Linux to U-Boot would be easier.

Right. I was concerned about the size of the table - the complete table from
Linux contains *lots* of devices and may take up quite some space, so
stripping out unused fields should help a bit. If there is a consensus to
keep the fields to ease porting new parts, that is fine with me.

I think in the future we may need some ifdefs in the table to select
sub-sets of supported parts - if a board has a 29LV040, it is useful to have
support for other flash ROMS with similar layout and pinout that may be used
as alternatives, but completely different parts (eg. 29LV160) may be left
out.

cu
Michael

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Stefan Roese]

On Tuesday 06 November 2007, Michael Schwingen wrote:
> > Wouldn't it be better not to remove those unused fields? This way porting
> > of new devices from Linux to U-Boot would be easier.
>
> Right. I was concerned about the size of the table - the complete table
> from Linux contains *lots* of devices and may take up quite some space, so
> stripping out unused fields should help a bit. If there is a consensus to
> keep the fields to ease porting new parts, that is fine with me.

I vote for keeping the fields in. From my point of view, it's better to only 
enable a small part of the available FLASH definitions, than to strip some 
fields from the struct and make porting more difficult.

> I think in the future we may need some ifdefs in the table to select
> sub-sets of supported parts - if a board has a 29LV040, it is useful to
> have support for other flash ROMS with similar layout and pinout that may
> be used as alternatives, but completely different parts (eg. 29LV160) may
> be left out.

ACK.

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

[U-Boot-Users] Mixing CFI and non-CFI flashs?

[Michael Schwingen]

On Tue, Nov 06, 2007 at 09:59:28AM +0100, Stefan Roese wrote:
> 
> I vote for keeping the fields in. From my point of view, it's better to only 
> enable a small part of the available FLASH definitions, than to strip some 
> fields from the struct and make porting more difficult.

Okay. I changed the patch so that the device table entries can now be copied
1:1 from the Linux source.

Since there still is the problem of the different unlock addresses, I have
moved the unlock address values to the flash_info_t struct - this gets rid
of the non-abvious code in the AMD_ADDR_* macros, and allows to override
these values without changing the behaviour of the CFI driver. I also added
a name field so that the correct name can be printed in flinfo.

Now the only problem I see is the code in flash_erase, which is the only
place where CFI_CMDSET_AMD_LEGACY needs different behaviour: the CFI code
does the unlock sequence to the current sector address, while the jedec
flash needs the unlock sequence to the flash base address. I believe the CFI
behaviour is wrong (but works because the CFI flashs ignore the extra
address bits), but I left the special case in - that way, there should be no
change in the existing CFI code behaviour, so the patch should be safe to
apply that way without breaking existing boards.

I will post the patch in a separate mail with PATCH: in the subject, so it
is not lost deep in this thread.

The resulting output now looks like this:

Flash:  8.3 MB
=> flinfo

Bank # 1: SST 39LF020 FLASH (8 x 8)  Size: 256 kB in 64 Sectors
  AMD Legacy command set, Manufacturer ID: 0xBF, Device ID: 0xD6
  Erase timeout: 30000 ms, write timeout: 100 ms

  Sector Start Addresses:
  50000000   RO   50001000   RO   50002000   RO   50003000   RO   50004000   RO 
  50005000   RO   50006000   RO   50007000   RO   50008000   RO   50009000   RO 
  5000A000   RO   5000B000   RO   5000C000   RO   5000D000   RO   5000E000   RO 
  5000F000   RO   50010000   RO   50011000   RO   50012000   RO   50013000   RO 
  50014000   RO   50015000   RO   50016000   RO   50017000   RO   50018000   RO 
  50019000   RO   5001A000   RO   5001B000   RO   5001C000   RO   5001D000   RO 
  5001E000   RO   5001F000   RO   50020000   RO   50021000   RO   50022000   RO 
  50023000   RO   50024000   RO   50025000   RO   50026000   RO   50027000   RO 
  50028000   RO   50029000   RO   5002A000   RO   5002B000   RO   5002C000   RO 
  5002D000   RO   5002E000   RO   5002F000   RO   50030000   RO   50031000   RO 
  50032000   RO   50033000   RO   50034000   RO   50035000   RO   50036000   RO 
  50037000 E      50038000 E      50039000 E      5003A000 E      5003B000 E    
  5003C000 E      5003D000 E      5003E000 E      5003F000   RO 

Bank # 2: CFI conformant FLASH (16 x 16)  Size: 8 MB in 64 Sectors
  Intel Extended command set, Manufacturer ID: 0x89, Device ID: 0x17
  Erase timeout: 16384 ms, write timeout: 2 ms
  Buffer write timeout: 2 ms, buffer size: 32 bytes

  Sector Start Addresses:
  51000000        51020000        51040000        51060000        51080000      
  510A0000        510C0000        510E0000        51100000        51120000      
  51140000        51160000        51180000        511A0000        511C0000      
  511E0000        51200000        51220000        51240000        51260000      
  51280000        512A0000        512C0000        512E0000        51300000      
  51320000        51340000        51360000        51380000        513A0000      
  513C0000        513E0000        51400000        51420000        51440000      
  51460000        51480000        514A0000        514C0000        514E0000      
  51500000        51520000        51540000        51560000        51580000      
  515A0000        515C0000        515E0000        51600000        51620000      
  51640000        51660000        51680000        516A0000        516C0000      
  516E0000        51700000        51720000        51740000        51760000      
  51780000        517A0000        517C0000        517E0000      
=> 

cu
Michael