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

Stefan Roese
2007-11-05T11:21:50Z

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