David Ho wrote:

...

...

I'm looking at the flash protection code in the cfi_flash driver, can anyone tell me why it claims intel's unprotect unprotects all locking? Certainly my K3 flash can unlock on a per block basis.

Which exactl line(s) of code are you alking about?

Inside flash_real_protect in cfi_flash.c from your gitweb. Line# 671 to 679

If you haven't noticed this function has not changed since the intial commit. 5653fc335a450fa46d89989e1afe5e8bb9a0a52e

If no one has any objection, I will remove the part of the code that relock each sector, for submission.

I do object!

First, That behavior is required because some Intel flash parts incorrectly unlock all sectors (not just current sector) so after unlocking the current sector we must redo the locking of all others that were supposed to remain locked. Again, the comment in that code reflects and explains this.

Secondly, AMD parts did not previously support real locking/unlocking so CFG_FLASH_PROTECTION option (which is needed to get the code you are talking) was not applicable for those parts. If that code now needs to be fixed for other parts that has this capability, fix it without breaking existing code.

Thirdly, even in its current form, has this code caused any problem at all? It just unnecessarily re-locks all those sectors/blocks that are supposed to remain locked wasting some time at the execution of "protect off" command.

Best regards, Tolunay

David Ho wrote:

Okay,

I really didn't mean to rip out someone's code, I know it was there for a reason. In any case, I like to understand which Intel flash is behaving badly. The spec does not say unprotect unlocks all blocks.

First, I am not the author of this part of code. I can tell you the Intel StrataFlash 28F128J3 parts on my Cogent CSB272 board needs this code. Original author must have hit the same issue. This is a chip bug and probably documented on some Intel Errata and it may be applying to certain Revs of the chip. I do not have a list.

...

...

If no one has any objection, I will remove the part of the code that relock each sector, for submission.

First, That behavior is required because some Intel flash parts incorrectly unlock all sectors (not just current sector) so after unlocking the current sector we must redo the locking of all others that were supposed to remain locked. Again, the comment in that code reflects and explains this.

I am sorry, my interpretation of your comment led me to think you are suggesting all Intel flashes behave this way. Really my original post

What is not clear about the word "some" in my description of the issue? I am obviously not claiming all Intel parts are this. Your interpretation is incorrect.

came from a confusion of the comment. A better comment may include what you just said above.

Do you know which parts behave this way? Has Intel confirmed this?

I do not need Intel's confirmation. It is a reality for me. Again, it is most likely documented in some Intel Errata (which might not be widely publicly circulated).

Certainly not all flashes need to have this workaround, perhaps it is sensible to option it out? Anyway, I have no problem with this code being there. Since I have not seen the same behaviour you saw, and google did not turn up and evidence to support this, I was just curious if this has been solved since the code was first submitted.

I certainly do not have the capability to test each new rev of every Intel flash. Without a complete list you can only take the safe path...

I also realized if flash is locked, Linux mtd/jffs2 will complain about writes to flash with these messages:

Write of 68 bytes at 0x03b9bc18 failed. returned -30, retlen 0 Not marking the space at 0x03b9bc18 as dirty because the flash driver returned r etlen zero

David

On 3/16/06, David Ho davidkwho@gmail.com wrote:

...

...

I am sorry, my interpretation of your comment led me to think you are suggesting all Intel flashes behave this way. Really my original post

What is not clear about the word "some" in my description of the issue? I am obviously not claiming all Intel parts are this. Your interpretation is incorrect.

When I said your comment I meant the comment in the code, to rephrase again:

"my interpretation of the comment in the code led me to think it claims all Intel flashes behave this way. Really my original post came from a confusion of the comment. A better comment may include what you just said above."

I appreciate your explanation. It made things much clearer.

David

Hi David,

On Thursday, 16. March 2006 20:59, David Ho wrote:

Some explanation is in order.

While Intel flashes K3/C3 use the that command sequence as block unlock, the same command sequence (0x60 0xD0) is used to "clear all blocks bits" on the J3. This is restrictly speaking not an intel flash bug. The datasheet had not mentioned there is a change to the lock/unlock command in the datasheet revision history so it appears to have been there in the J3 datasheet from the start. Just that there is an inconsistency in the behaviour of the command sequence. They refer to it as Legacy lock/unlock in the Primary Vendor Specifc Exended Query Table. So information can be extracted from CFI attributes.

After digging through some Intel manuals it seems that you are correct here. Good catch.

With the evidence gathered thus far, it is simply small detail overlooked by the implementor of the original code, which is perfectly acceptable. Just that I would have hoped this thread elicited discussion that led to this discovery.

I wonder how I can better present myself to make others more co-operative.

You did quite well. Sometimes you just need some patience (and endurance). ;-)

The best way to proceed (if nobody of the CFI experts objects) would be, if you could create a patch to fix this problem using the "Legacy lock/unlock" bit from the query table.

Best regards, Stefan

David,

Please do not top post. Post after the quoted section(s) as I do below.

David Ho wrote:

Hi all,

Maybe if there is someone from Intel on this list they would have clear up this confusion much earlier. Better yet, if someone from Intel can correct me here.

I am not sure if we have anyone from Intel on this list. At least nobody with intel.com address posted to the list in the last couple of years as far as I can remember. U-Boot is not very popular in X86 systems.

Some explanation is in order.

While Intel flashes K3/C3 use the that command sequence as block unlock, the same command sequence (0x60 0xD0) is used to "clear all blocks bits" on the J3. This is restrictly speaking not an intel flash bug. The datasheet had not mentioned there is a change to the lock/unlock command in the datasheet revision history so it appears to have been there in the J3 datasheet from the start. Just that there is an inconsistency in the behaviour of the command sequence. They

I looked at the Intel datasheet for 28F128J3, 28F640J3 and 28F320J3 (Order # 290667-014 December 2003). Upon very careful read, I found on Page 18, Table 4 (Command Bus-Cycle Definitions (Sheet 2 of 2), the small type footnote that reads: "15. The clear block lock-bits operation simultaneously clears all the block lock-bits". Also the while the lock command reads as "Set Block-Lock Bit" the clear command says "Clear Block-Lock Bits" (notice plural).

So, this is indeed not a bug but a feature (or perhaps another example of a design bug being classified as a feature, who knows). So, my earlier comment that it is a bug is not correct. Nevertheless, bug or not this does not change the fact that re-locking is needed as in the current code so removing that functionality is NOT OK. So, the comment "/* Intel's unprotect unprotects all locking */" perhaps needs to be made more specific as /* Intel's Strataflash J3 unlocks all locking , redo remaining locks */

refer to it as Legacy lock/unlock in the Primary Vendor Specifc Exended Query Table. So information can be extracted from CFI attributes.

For J3 flash, "Optional Flash Features and Commands" has offset 36h-39h (4 bytes) bit 3 Legacy lock/unlock supported bit (bit 3). Again, for J3 chips my datasheet says this is set to "1". I see K3 datasheet has this set to 0. If you are absolutely sure that this is foolproof indication that all parts with bit 3 clear do not require re-locking, submit a suitable patch though this will also mean slight increased code base at the expense of saving minor time for K3 parts on unlocking.

BTW, the vendor specific extended query table is unique to each CFI vendor (and might be even to the product family) and might not be implemented by all vendors. So, if you use information from this table you must properly identify vendor, product family before trying to use that information. This might add to the code further.

All indication suggests that Intel is not at fault.

With the evidence gathered thus far, it is simply small detail overlooked by the implementor of the original code, which is perfectly acceptable. Just that I would have hoped this thread elicited discussion that led to this discovery.

Perhaps the code was implemented before K3 families were out. The comment on the code does not indicate this. My earlier comment that this is a bug was indeed wrong but since I did not implement that part of the code my mistake does not necessarily carry to the original author. I am actually happy to another detail that is easy to miss.

I wonder how I can better present myself to make others more co-operative.

Sorry if I sounded not so cooperative but when you simply suggested removing the code, that is obviously not acceptable to me and I raised my objection. Anyhow, through this discussion the need is validated and possibility of improvement is brought out. The question now is that is it worth it? How often do you lock unlock the flash in U-Boot to implement the improvement as the effect is some extra delay in K3 family chips.

Best regards, Tolunay

David

On 3/16/06, Tolunay Orkun listmember@orkun.us wrote:

...

David Ho wrote:

...

Okay,

I really didn't mean to rip out someone's code, I know it was there for a reason. In any case, I like to understand which Intel flash is behaving badly. The spec does not say unprotect unlocks all blocks.

First, I am not the author of this part of code. I can tell you the Intel StrataFlash 28F128J3 parts on my Cogent CSB272 board needs this code. Original author must have hit the same issue. This is a chip bug and probably documented on some Intel Errata and it may be applying to certain Revs of the chip. I do not have a list.

...

...

...

If no one has any objection, I will remove the part of the code that relock each sector, for submission.

First, That behavior is required because some Intel flash parts incorrectly unlock all sectors (not just current sector) so after unlocking the current sector we must redo the locking of all others that were supposed to remain locked. Again, the comment in that code reflects and explains this.

I am sorry, my interpretation of your comment led me to think you are suggesting all Intel flashes behave this way. Really my original post

What is not clear about the word "some" in my description of the issue? I am obviously not claiming all Intel parts are this. Your interpretation is incorrect.

...

came from a confusion of the comment. A better comment may include what you just said above.

Do you know which parts behave this way? Has Intel confirmed this?

I do not need Intel's confirmation. It is a reality for me. Again, it is most likely documented in some Intel Errata (which might not be widely publicly circulated).

...

Certainly not all flashes need to have this workaround, perhaps it is sensible to option it out? Anyway, I have no problem with this code being there. Since I have not seen the same behaviour you saw, and google did not turn up and evidence to support this, I was just curious if this has been solved since the code was first submitted.

I certainly do not have the capability to test each new rev of every Intel flash. Without a complete list you can only take the safe path...