[U-Boot-Users] flash protection code in cfi_flash

[U-Boot-Users] flash protection code in cfi_flash

[David Ho]

Hi,

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.

Another thing is, that same piece of code will be executed regardless
of the kind of flash you have.

If this piece of code is needed for a specific Intel flash, would
anyone kindly point out which one?

I'm using version 1.1.4.

David

[U-Boot-Users] flash protection code in cfi_flash

[Wolfgang Denk]

In message <4dd15d180603141115g748f2303h9846f5d8b0e9b59e@mail.gmail.com> you 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?

> I'm using version 1.1.4.

Which exact git ID are you talking  about?  Before  or  after  commit
f18e874ad548034552cc4a2cdfe1a21edd9ca392  , i. e. before or after all
the CFI driver fixes?

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
The one who says it cannot be done should never interrupt the one who
is doing it.

[U-Boot-Users] flash protection code in cfi_flash

[David Ho]

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

David

[U-Boot-Users] flash protection code in cfi_flash

[Tolunay Orkun]

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

[U-Boot-Users] flash protection code in cfi_flash

[David Ho]

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.

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

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.

David

[U-Boot-Users] flash protection code in cfi_flash

[Tolunay Orkun]

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

[U-Boot-Users] flash protection code in cfi_flash

[David Ho]

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

[U-Boot-Users] flash protection code in cfi_flash

[David Ho]

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
>

[U-Boot-Users] flash protection code in cfi_flash

[David Ho]

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.

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.

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.

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

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

[U-Boot-Users] flash protection code in cfi_flash

[Stefan Roese]

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

[U-Boot-Users] flash protection code in cfi_flash

[David Ho]

If anyone is working on Intel non-J3 flashes.  This should be of interest.

Unprotecting all flash blocks on a 64MB K3 flash, in msecs.

My board takes an additional 2 seconds to boot as a result, because
the board comes up with all blocks locked.

David

with J3 relocking code.

MON> protect off 0xfc000000 0xffffffff
................................................................................
................................................................................
................................................................................
................
Un-Protected 256 sectors
time elapsed 2544

without J3 relocking code.

MON> protect off 0xfc000000 0xffffffff
................................................................................
................................................................................
................................................................................
................
Un-Protected 256 sectors
time elapsed 291

[U-Boot-Users] flash protection code in cfi_flash

[Tolunay Orkun]

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