Stefan Roese wrote:

I am right now trying to integrate all outstanding CFI patches. Could you please resubmit this patch without all the incorrect "cosmetic changes" And please with a proper CHANGELOG entry describing the CFI driver changes.

The attached gzipped patch was made against the U-Boot GIT commit 6624b687bc2b747233090e67628df37d1c84ed17.

It is a completely independent patch, containing the patches to drivers/cfi_flash.c which were in my patch 012.

The patch - Makes flash_get_size() static. - Changes flash_full_status_check() to test for timeout OR failure, rather than timeout AND failure. - Makes find_sector() available for BOTH versions of flash_write_cfiword()

CHANGELOG: Update drivers/cfi_flash.c. - flash_get_size() now static. - flash_full_status_check() to test for timeout OR failure, rather than timeout AND failure. - find_sector() called in both versions of flash_write_cfiword() Patch by Peter Pearse, 27th Feb 2006

I intend to re-submit the changes for the boards (Integrator/CP, VersatileAB & PB) separately, once you have completed your CFI work.

Regards

Peter

Stefan Roese wrote:

Hi Peter,

...

The patch

• Makes flash_get_size() static.

Rejected. This function is called from other board files.

I don't mind but:- - flash_get_size() is not declared in include/flash.h - I don't see a board which calls flash_get_size() without providing its own flash.c::flash_get_size(), although not all are declared static. Arguably the board level code might have some reason to subvert the flash interface (?), but making it static guards against accidental cross-linking. - IMHO flash info should be accessed from the flash_info[] array, set up by a call to flash_init(), keeping the flash interface small.

Please take a look at the patch from Marcus Hall:

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/18530

Yes this patch is correct, mine would give two errors....

Regards

Peter

Stefan Roese wrote:

On Monday, 27. February 2006 16:59, Peter Pearse wrote:

...

• Changes flash_full_status_check() to test for timeout OR failure,

rather than timeout AND failure.

I'm not sure here. Please take a look at the patch from Marcus Hall:

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/18530

Could you and others (Marcus, Tolunay) please comment on what patch should be applied here. I tend to take the patch from Marcus right now.

Well, there are two variables here. I will abstract it to be Timeout :: flash_status_check returns != ERR_OK Fail :: status != FLASH_STATUS_DONE The following table should correspond to what gets returned by either code patch:

Timeout Fail Marcus Peter 0 0 ERR_OK ERR_OK 0 1 ERR_INVAL ERR_INVAL 1 0 ERR_TIMEOUT ERR_INVAL 1 1 ERR_TIMEOUT ERR_INVAL

Additionally, Peter's patch may output an additional error message after a timeout if it appears that some error flags are set (but they are not necessarily valid if the flash has timed out)

So, I believe that either would work to ensure that if there is an error it does get reported, but I believe that my patch returns a more useful return code and doesn't output potentially confusing error messages.

Marcus Hall

Stefan Roese wrote:

...

• Changes flash_full_status_check() to test for timeout OR failure,

rather than timeout AND failure.

I'm not sure here. Please take a look at the patch from Marcus Hall:

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/18530

Could you and others (Marcus, Tolunay) please comment on what patch should be applied here. I tend to take the patch from Marcus right now.

I looked at the Stephan's patch to this (which I had included in my alternative patch as well) as well as Marcus's patch.

Yes, flash_status_check() exists only with ERR_OK or ERR_TIMEOUT. ERR_OK is really mapping to not busy in that context (yet operation might have failed).

Marcus's patch checks the extended status register if flush was not busy with last operation (ERR_OK) and identifies the error of extended status indicates an error. Stephan's patch does not care about busy status (if the flash is not busy the real status is in extended status register) but also redirects the ERR_TIMEOUT case to the if branch which gets mapped ERR_INVAL (probably not right).

I now think Marcus patch is preferable but it would also be better to inform on the console regarding timeout case as well. Also since flash_status_check() did reset the flash for timeout (contrary to its description) we should not be resetting the flash second time here.

I mean we should have something like the following instead:

case CFI_CMDSET_INTEL_EXTENDED: case CFI_CMDSET_INTEL_STANDARD: if ((retcode == ERR_TIMOUT) { printf ("Flash %s timeout at address %lx\n", prompt, info->start[sector]); /* break out, flash_status_check() performed reset already */ break; } if (!flash_isequal (info, sector, 0, FLASH_STATUS_DONE)) { retcode = ERR_INVAL; printf ("Flash %s error at address %lx\n", prompt, info->start[sector]); if (flash_isset (info, sector, 0, FLASH_STATUS_ECLBS | FLASH_STATUS_PSLBS)) { puts ("Command Sequence Error.\n"); } else if (flash_isset (info, sector, 0, FLASH_STATUS_ECLBS)) { puts ("Block Erase Error.\n"); retcode = ERR_NOT_ERASED; } else if (flash_isset (info, sector, 0, FLASH_STATUS_PSLBS)) { puts ("Locking Error\n"); } if (flash_isset (info, sector, 0, FLASH_STATUS_DPS)) { puts ("Block locked.\n"); retcode = ERR_PROTECTED; } if (flash_isset (info, sector, 0, FLASH_STATUS_VPENS)) puts ("Vpp Low Error.\n"); } flash_write_cmd (info, sector, 0, FLASH_CMD_RESET); break;

Best regards, Tolunay

Marcus Hall wrote:

...

I now think Marcus patch is preferable but it would also be better to inform on the console regarding timeout case as well.

Well, flash_status_check() does print an error message if it does timeout...

You are right. We don't need print in flash_full_status_check for timeout but we still reset the flash 2nd time in flash_full_status_check for timeouts. Probably harmless but unnecessary.

I guess, it is best to leave it as it is now after your patch is applied.

Tolunay