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

[Stefan Roese <sr@denx.de>]

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