[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Peter Pearse]

The attached gzipped patch was made against the U-Boot GIT head of Oct
31 2005.

It is my patch number 012.
The patch is independent of patches 009 & 010, but dependent on patch
011. 

The patch updates the versatile and integrator/CP boards code to use CFI
flash,
rather than having a hardware dependent flash.c.

CHANGELOG:
	Versatile & Integrator/CP to use CFI flash..
	Patch by Peter Pearse, 31st Oct 2005

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 012-cfi.patch.gz
Type: application/x-gzip
Size: 13387 bytes
Desc: 012-cfi.patch.gz
Url : http://lists.denx.de/pipermail/u-boot/attachments/20051031/d067f3ba/attachment.bin 

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Stefan Roese]

Hi Peter,

On Monday, 31. October 2005 17:44, Peter Pearse wrote:
> The attached gzipped patch was made against the U-Boot GIT head of Oct
> 31 2005.
>
> It is my patch number 012.
> The patch is independent of patches 009 & 010, but dependent on patch
> 011.
>
> The patch updates the versatile and integrator/CP boards code to use CFI
> flash,
> rather than having a hardware dependent flash.c.
>
> CHANGELOG:
> 	Versatile & Integrator/CP to use CFI flash..
> 	Patch by Peter Pearse, 31st Oct 2005

I am right now trying to integrate all outstanding CFI patches. Could you 
please resubmit this patch without all the incorrect "cosmetic changes" like:

@@ -279,7 +279,7 @@ ushort flash_read_ushort (flash_info_t *
 
 #ifdef DEBUG
 	debug ("ushort addr is at %p info->portwidth = %d\n", addr,
-	       info->portwidth);
+				 info->portwidth);
 	for (x = 0; x < 2 * info->portwidth; x++) {
 		debug ("addr[%x] = 0x%x\n", x, addr[x]);
 	}

And please with a proper CHANGELOG entry describing the CFI driver changes.

Thanks.

Best regards,
Stefan

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Peter Pearse]

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 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfi_flash.patch.gz
Type: application/octet-stream
Size: 978 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060227/8779bf2e/attachment.obj 

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Stefan Roese]

Hi Peter,

On Monday, 27. February 2006 16:59, Peter Pearse wrote:
> The attached gzipped patch was made against the U-Boot GIT commit
> 6624b687bc2b747233090e67628df37d1c84ed17.

Thanks.

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

Rejected. This function is called from other board files. I will remove this 
part of the patch.

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

> - Makes find_sector() available for BOTH versions of flash_write_cfiword()

Applied.

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

OK. Please give us a few more days and the new CFI driver should be available.

Best regards,
Stefan

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Peter Pearse]

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

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Stefan Roese]

Hi Peter,

On Tuesday, 28. February 2006 12:58, Peter Pearse wrote:
> >> 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

Good point. I will fix this.

> - I don't see a board which calls flash_get_size() without providing its
> own flash.c::flash_get_size(),

For example "board/tqm85xx/tqm85xx.c".

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

Generally ok, but in some cases this function is used to re-init the sector 
setup when not using the max. flash amount (especially on high-boot systems).

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

OK. Thanks.

Best regards,
Stefan

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Marcus Hall]

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

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Stefan Roese]

Hi Marcus,

On Tuesday, 28. February 2006 19:46, Marcus Hall wrote:
> > 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.

Thanks for the detailed statement. Your (Marcus's) patch has been integrated.

Best regards,
Stefan

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Tolunay Orkun]

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

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Marcus Hall]

Tolunay Orkun 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...

Marcus Hall

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Tolunay Orkun]

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

[U-Boot-Users] [PATCH] CFI support for Versatile & Integrator/CP boards

[Wolfgang Denk]

In message <89A528FE6DB0FA44877BB2F05B84671803490F81@ZIPPY.Emea.Arm.com> you wrote:
> 
> It is my patch number 012.
...
> CHANGELOG:
> 	Versatile & Integrator/CP to use CFI flash..
> 	Patch by Peter Pearse, 31st Oct 2005

See previous discussion with Stefan Roese.  Problems  with  "cosmetic
changes" in this patch. Rejected for now. Used new patch from 28. 02.
2006 instead (only partially). Considered closed.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He's dead, Jim
	-- McCoy, "The Devil in the Dark", stardate 3196.1