[U-Boot-Users] [PATCH] cfi_flash.c patches

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Sangmoon Kim]

Hi,
The two patches attached are for drivers/cfi_flash.c.

cfi_flash-protect.patch adds CFG_FLASH_PROTECT_CLEAR
because for some flash memories(such as 28F320C3)
all banks are protected after reset.

cfi_flash-buffer.patch makes write_buff not to call 
flash_write_cfibuffer if buffer_size is1.
Because for flash memories with buffer_size 1,
buffer write is not supported.

Regards,
Sangmoon Kim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfi_flash-buffer.patch
Type: application/octet-stream
Size: 1358 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20050819/16713e20/attachment.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfi_flash-protect.patch
Type: application/octet-stream
Size: 1158 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20050819/16713e20/attachment-0001.obj 

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Dear Sangmoon,

I've examined your patch for clearing the protection of flash sectors
automatically during flash init. As a matter of fact a similar patch was
also proposed by someone else and I had commented on it as well.

I think this is wrong approach. These sectors are protected for a reason
(to prevent accidental writes - forgetting to enable protection).

You should enable CFG_FLASH_PROTECTION in your board config file. If you
don't do this U-Boot will do software protection of sectors (which is
really for those flash chips with no hardware protection capability) and
"protect off" will not issue unlock commands as you may have witnessed.

CFG_FLASH_PROTECTION will enable the "protect  off" command to disable
protection properly on these sectors as it should.

IMHO, No patch is needed here! Perhaps we need to add a couple of
comment lines in README (DULG?) for documentation purposes. Wolfgang,
can you comment here.

Best regards,
Tolunay

Sangmoon Kim wrote:
> Hi,
> The two patches attached are for drivers/cfi_flash.c.
> 
> cfi_flash-protect.patch adds CFG_FLASH_PROTECT_CLEAR
> because for some flash memories(such as 28F320C3)
> all banks are protected after reset.
> 
> cfi_flash-buffer.patch makes write_buff not to call 
> flash_write_cfibuffer if buffer_size is1.
> Because for flash memories with buffer_size 1,
> buffer write is not supported.
> 
> Regards,
> Sangmoon Kim

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Sangmoon Kim]

Dear Tolunay Orkun,

Sorry for late reply.

> I think this is wrong approach. These sectors are protected for a reason
>
But some chips(like 28F320C3 from intel) automatically
protect all sectors after reset.
Although the chips have good reasons to do it.
I think, some boards also have good reasons to undo it.

> You should enable CFG_FLASH_PROTECTION in your board config file. If you
> don't do this U-Boot will do software protection of sectors (which is
> really for those flash chips with no hardware protection capability) and
> "protect off" will not issue unlock commands as you may have witnessed.
> 
That's right. Power-on-protected-automatically chips need both
CFG_FLASH_PROTECTION and CFG_FLASH_PROTECT_CLEAR flags.

> CFG_FLASH_PROTECTION will enable the "protect  off" command to disable
> protection properly on these sectors as it should.
>
I don't think it is convenient to protect off sectors manually
which is automatically protected during power on.

Best regards,
Sangmoon Kim 

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Sangmoon Kim wrote:

>> protection properly on these sectors as it should.
>>
> CFG_FLASH_PROTECTION will enable the "protect off" command to disable
> I don't think it is convenient to protect off sectors manually
> which is automatically protected during power on.


Convenience is irrelevant. This flash is obviously designed with data
protection as priority.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Sangmoon Kim]

>> I don't think it is convenient to protect off sectors manually
>> which is automatically protected during power on.
> 
> 
> Convenience is irrelevant. This flash is obviously designed with data
> protection as priority.

The point is that all the sectors of the flash is protected
"automatically" on power up.
And for the flash which is not,
you can simply undefine CFG_FLASH_PROTECT_CLEAR.

Regards,
Sangmoon Kim

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Sangmoon Kim wrote:

>>> I don't think it is convenient to protect off sectors manually
>>> which is automatically protected during power on.
>>
>>
>>
>> Convenience is irrelevant. This flash is obviously designed with data
>> protection as priority.
>
>
> The point is that all the sectors of the flash is protected
> "automatically" on power up.
> And for the flash which is not,
> you can simply undefine CFG_FLASH_PROTECT_CLEAR.
>
> Regards,
> Sangmoon Kim

The point is you can simply use already available "protect off"
mechanism to lift the lock on these sectors instead of defining
something new. Existing CFG_FLASH_PROTECTION properly directs U-Boot to
issue unlock commands when you execute "protect off".

You are not solving a problem with CFG_FLASH_PROTECT_CLEAR. You are
introducing another solution which is not needed. Given that you cannot
distinguish between sectors that should be unlocked and that should
remain locked, unlocking lifts lock on sectors that should remain locked
as well. You might as well used a flash without any locking at all.

Wolfgang, as project leader, might take your patch but I am personally
irked with the spirit of this patch and the implications. Yes, it will
be optionally included by default but ugliness of the solution you
present does not go away. IMHO, There should be no code in real life
that simply unlocks all sectors. I guess we will have to agree to disagree.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

In message <4309F122.5090907@orkun.us> you wrote:
>
> > The point is that all the sectors of the flash is protected
> > "automatically" on power up.
...
> The point is you can simply use already available "protect off"
> mechanism to lift the lock on these sectors instead of defining

You can do this, but I would reject such a broken implementation.

U-Boot shall come  up  with  writapble  flash,  except  for  the  few
protected  sectors  where  U-Boot itself lives (plus the environment,
plus eventually FPGA images needed to boot the hardware).

> present does not go away. IMHO, There should be no code in real life
> that simply unlocks all sectors. I guess we will have to agree to disagree.

Please see my previous posting about this.

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
It is better to marry than to burn.
                                - Bible ``I Corinthians'' ch. 7, v. 9

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Wolfgang Denk wrote:

>In message <4309F122.5090907@orkun.us> you wrote:
>  
>
>
>>The point is you can simply use already available "protect off"
>>mechanism to lift the lock on these sectors instead of defining
>>    
>>
>
>You can do this, but I would reject such a broken implementation.
>  
>
I guess I do not understand what is broken by having to use "protect 
off" for a flash that auto protects all sectors. If you automatically 
unlock sectors how do you know that sector X that was explicitly locked 
or not. I would personally err on being on the safe side and keep it 
locked until explicitly told by the user to unlock the sectors prior to 
be written.

I consider unlocking all sectors unconditionally is broken implementation.

>U-Boot shall come  up  with  writapble  flash,  except  for  the  few
>protected  sectors  where  U-Boot itself lives (plus the environment,
>plus eventually FPGA images needed to boot the hardware).
>  
>

What about the sectors that are not in direct use by U-Boot. If I put a 
lock on a certain sector in Linux I would certainly would like to keep 
that lock to remain in that state across boot. U-Boot does not have any 
knowledge of the use of these other sectors and should not make 
assumptions on their lock/unlock state.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

In message <430A0237.9060703@orkun.us> you wrote:
> 
> I guess I do not understand what is broken by having to use "protect 
> off" for a flash that auto protects all sectors. If you automatically 

I already posted a detailed explanation what the design is, and shall
be. I see no further need for discussion.

> I consider unlocking all sectors unconditionally is broken implementation.

I understand that this is your opinion. It is not the U-Boot design.

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
In C we had to code our own bugs, in C++ we can inherit them.

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Scott McNutt]

Tolunay Orkun wrote:

> The point is you can simply use already available "protect off"

Not to add fuel to the fire ;-) ... but in the past, I did precisely
what Tolunay suggests. I just added a "protect off" to the boot command
to explicitly unprotect specific sectors. It worked just fine -- and
I did not consider the techique to lack convenience.

On-the-other-hand, preventing automatic "UN-protection" in this case
is _policy_ enforcement -- we would be making technical decisions for
others, for applications we know nothing about -- this, I am sure,
is not the u-boot "spirit" that I have observed over the past 5 years.

That being said, a default that does not automatically un-protect
seems appropriate -- or find a way to push the auto un-protect
capability into the board-specific tree.

Regards,
--Scott

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Sangmoon Kim]

Dear Tolunay Orkun,

No offense but...

> Wolfgang, as project leader, might take your patch but I am personally
> irked with the spirit of this patch and the implications. Yes, it will
> be optionally included by default but ugliness of the solution you
> present does not go away. IMHO, There should be no code in real life
> that simply unlocks all sectors. I guess we will have to agree to 
> disagree.
>
I think flexibility is more important than "spirit" even if it is "ugly".
Because by allowing doing stupid things,
you can also allow doing cleaver things too.

Software is soft because it is flexible.
It archives its flexibility by its ugliness.

Best regards,
Sangmoon Kim 

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

In message <4309712D.1040301@orkun.us> you wrote:
>
> Convenience is irrelevant. This flash is obviously designed with data
> protection as priority.

Convenience is not irrelevant. The existence of U-Boot itself is just
for convenience,

We don't care what the people who designed the flash had in mind.  In
U-Boot, the design is as follows:

* All flash is writable by default.

* Some parts of the flash may be  either  implictely  or  explicitely
  protected.

* Implicit protection: this covers those areas of the flash that  are
  used  to  store  data  that  are  required for correct operation of
  U-Boot and the hardware, i. e.

  - the U-Boot code and data
  - environment variables
  - any FPGA images etc. which are necessary for correct HW operation

* Explicit protection: arbitrary areas which have been  protected  by
  the user using the "protect" coommand.


Note:

* "protection" may or may  not  be  implemented  using  any  hardware
  protection mechanisms which may be available on some chips (and not
  be available on other types)

* In the current design there is *no* requirement that  any  explicit
  protection  settings  are  persistent,  i. e. it is perfectly legal
  (and actually the default, see above) that such settings  get  lost
  on  reboot, and that the board comes up from reset with unprotected
  flash


This design shall be implemented  by  all  boards  if  possible;  for
features  that  are  shared  across several boards like the cfi_flash
driver it is mandatory.


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
Lispers are among  the  best  grads  of  the  Sweep-It-Under-Someone-
Else's-Carpet  School of Simulated Simplicity. [Was that sufficiently
incendiary? :-)]  - Larry Wall in <1992Jan10.201804.11926@netlabs.com

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Wolfgang Denk wrote:

>In message <4309712D.1040301@orkun.us> you wrote:
>  
>
>>Convenience is irrelevant. This flash is obviously designed with data
>>protection as priority.
>>    
>>
>
>Convenience is not irrelevant. The existence of U-Boot itself is just
>for convenience,
>  
>
I think "protect off" command is the convenience enough for this situation.

>We don't care what the people who designed the flash had in mind.  In
>U-Boot, the design is as follows:
>
>* All flash is writable by default.
>  
>
Why do you even attempt to provide software protection  for some sectors 
of flash when the chip does not provide such protection then?

>* Some parts of the flash may be  either  implictely  or  explicitely
>  protected.
>
>* Implicit protection: this covers those areas of the flash that  are
>  used  to  store  data  that  are  required for correct operation of
>  U-Boot and the hardware, i. e.
>
>  - the U-Boot code and data
>  - environment variables
>  - any FPGA images etc. which are necessary for correct HW operation
>  
>

Why do you override the policy of other applications for sectors that 
U-Boot has no actual use itself. Why do you unlock them all and present 
the opportunity of loss of critical data for other parts of the software 
solution? I would argue that there may be important and critical data 
stored in those sectors that are "required for correct operation of 
software" that runs on the CPU after U-Boot is done. Why do you think 
these parts deserve lesser protection?

Best regards,
Tolunay

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

In message <430A0538.2040908@orkun.us> you wrote:
> 
> I think "protect off" command is the convenience enough for this situation.

YMMV.

> >* All flash is writable by default.
> >
> Why do you even attempt to provide software protection  for some sectors 
> of flash when the chip does not provide such protection then?

To protect myself from doing stupid things too  often.  To  allow  me
being  lazy like running "era all" without need to think where U-Boot
or the environment might be located on a board.

> Why do you override the policy of other applications for sectors that 
> U-Boot has no actual use itself. Why do you unlock them all and present 

Because there simply *is* *no* policy at all.  Especially  not  in  a
one-size-fits-all  driver like cfi_flash which is what we are talking
about.

If you have special requirements please feel free to implement  these
in  your  board  specific code. But don't try to enforce your special
ideas of how things should be on everybody else.


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
There is a time in the tides of men, Which, taken at its flood, leads
on to success. On the other hand, don't count on it.   - T. K. Lawson

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Dear Wolfgang,

Wolfgang Denk wrote:

>Because there simply *is* *no* policy at all.  Especially  not  in  a
>  
>
That is not true. There are several policies already.

Just a couple of emails ago you were saying all sectors should be in 
writable state in U-Boot. This is a policy which is announced today by you.

Leaving the state of sectors (except for U-Boot managed sectors) until 
user takes explicit lock/unlock action as they are is another policy . 
This has been the policy so far which I would call "common sense" policy.

Providing software protection for flash that does not have hardware 
protection is yet another policy.

>one-size-fits-all  driver like cfi_flash which is what we are talking
>about.
>
>If you have special requirements please feel free to implement  these
>in  your  board  specific code. But don't try to enforce your special
>ideas of how things should be on everybody else.
>  
>
I am not trying to implement anything. Existing code works well for me 
(well after a couple of fixes which I submitted a patch for).

It is the new patch (not from me) that is introducing new policies and 
ways that needs to be questioned and discussed since it is effecting a 
common driver. This new patch is enforcing new ideas and policies. I've 
raised a number of issue with the new approach which you see to 
conveniently avoided. Could you please answer the following?

Why do you think it is OK for U-Boot to unlock sectors/blocks that it 
knows nothing about their usage? Wouldn't leaving these sectors in a 
safer state a common sense approach?

While you see it important to protect U-Boot environment (for various 
reasons and I agree), you do not seem to consider consistent protection 
for another area of flash that may be storing equally vital information 
for software system. Why?

Best regards,
Tolunay

Note: I had submitted a bug fix on July 2nd for a number of cfi_flash.c 
fixes. Do you still have that in your queue? I was expecting it would go 
to 1.1.3 since you picked some other fixes to go in that release. I am 
now worried that it is lost somewhere.

http://sourceforge.net/mailarchive/message.php?msg_id=12234135

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

In message <430A4C2C.60506@orkun.us> you wrote:
> 
> That is not true. There are several policies already.

Let's  agree  on  the  term   that   there   are   several   existing
implementations, ok?

> Just a couple of emails ago you were saying all sectors should be in 
> writable state in U-Boot. This is a policy which is announced today by you.

OK.

> Leaving the state of sectors (except for U-Boot managed sectors) until 
> user takes explicit lock/unlock action as they are is another policy . 

I don't call this a policy.

> Providing software protection for flash that does not have hardware 
> protection is yet another policy.

I don't call this a policy.

> It is the new patch (not from me) that is introducing new policies and 

I did not even review this  patch  yet.  I  just  commented  on  your
requirements, which I do not agree with.

> Why do you think it is OK for U-Boot to unlock sectors/blocks that it 
> knows nothing about their usage? Wouldn't leaving these sectors in a 

Because in the general case (and this is what cfi_flash is used  for)
you  don't  expect  to  have  any  hardware protected sectors. Not in
U-Boot, and neither in Linux when you for example want to  use  these
for a writable MTD partition.

> safer state a common sense approach?

Not for me. I don't like the hardware doing magic  things  to  me.  I
want to be in control over the hardware - not vice versa.

> While you see it important to protect U-Boot environment (for various 
> reasons and I agree), you do not seem to consider consistent protection 
> for another area of flash that may be storing equally vital information 
> for software system. Why?

Not on  a  *automatic*  base.  I  accept  this  only  if  explicitely
requested  by  the user (by using the "protect on" command) *and* the
board designer (by providing a  flash  implementation  that  supports
hardware  write  protection both in hardware [by selcting appropriate
flash chips] and in software [by  enabling  the  needed  features  in
U-Boot]).

As mentioned before: if you want to have this on a  board,  OK,  then
implement  it there and put apropriate big warnings and notes in your
board documentation. If this is general code which is  used  by  many
boards  that  you  don't  control  (and  do not test!) then I want to
provide a common interface. And common behaviour is that flash can be
erased and written to in the boot loader.


> Note: I had submitted a bug fix on July 2nd for a number of cfi_flash.c 
> fixes. Do you still have that in your queue? I was expecting it would go 

Yes.

> to 1.1.3 since you picked some other fixes to go in that release. I am 
> now worried that it is lost somewhere.

It's not lost. The selection of patches for 1.1.3 was done  based  on
their  complexity - simple fixes or changes that were obviously local
to one board only went in, but more complex  modifications  or  stuff
like yours that needs testing on many platforms was further delayed.

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
Shakespeare's Law of Prototyping: (Hamlet III, iv, 156-160)
        O, throw away the worser part of it,
        And live the purer with the other half.

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Yuli Barcohen]

>>>>> Wolfgang Denk writes:

...

    Wolfgang> Not for me. I don't like the hardware doing magic things
    Wolfgang> to me.  I want to be in control over the hardware - not
    Wolfgang> vice versa.

    Tolunay> While you see it important to protect U-Boot environment
    Tolunay> (for various reasons and I agree), you do not seem to
    Tolunay> consider consistent protection for another area of flash
    Tolunay> that may be storing equally vital information for software
    Tolunay> system. Why?

    Wolfgang> Not on a *automatic* base.  I accept this only if
    Wolfgang> explicitely requested by the user (by using the "protect
    Wolfgang> on" command) *and* the board designer (by providing a
    Wolfgang> flash implementation that supports hardware write
    Wolfgang> protection both in hardware [by selcting appropriate flash
    Wolfgang> chips] and in software [by enabling the needed features in
    Wolfgang> U-Boot]).

...

There are two main types of Intel flashes (AMD/Spansion do not provide
software-controllable hardware protection): flash files (uniform
sectors) and boot blocks. The former come out of reset unprotected, the
latter - protected. If the above mentioned board designer chooses to use
a boot block, he/she selects flash which is protected by default. If
U-Boot must automatically change this behaviour, why to use such a flash
in the first place? Regarding the implementation, unprotecting the flash
on U-Boot's start up requires exactly the same operations as implemented
in the flash_real_protect function (which is controlled by
CFG_FLASH_PROTECTION) so IMHO the same function must be used for
automatic unprotection too to avoid code duplication though I agree with
Tolunay that defining CFG_FLASH_PROTECTION and calling "protect off" is
the way to go.

-- 
========================================================================
 Yuli Barcohen       | Phone +972-9-765-1788 |  Software Project Leader
 yuli at arabellasw.com | Fax   +972-9-765-7494 | Arabella Software, Israel
========================================================================

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Sangmoon Kim]

Dear Yuli Barcohen,

I'm almost exhausted by this topic. The patch is only an option.
It is just for convenience and consistency of environment variables between 
boards.
It has nothing to do with any philosophy.

> If the above mentioned board designer chooses to use
> a boot block, he/she selects flash which is protected by default.
Bootblock and environment is protected again just after the unprotection.
Please check the code.

> If U-Boot must automatically change this behaviour,
It is not mandatory. It is an option again.

> why to use such a flash in the first place?
People do mistakes. And to currect it on software is easy.
Changing hardware is very expensive and time consuming.
Although I come to think changing software is more difficult sometimes
because of this issue.

> Regarding the implementation, unprotecting the flash
> on U-Boot's start up requires exactly the same operations as implemented
> in the flash_real_protect function (which is controlled by
> CFG_FLASH_PROTECTION) so IMHO the same function must be used for
> automatic unprotection too to avoid code duplication though I agree with
> Tolunay that defining CFG_FLASH_PROTECTION and calling "protect off" is
> the way to go.
>
flash_protect calls flash_real_protect if CFG_FLASH_PROTECTION is defined.
It is not a code duplication.
If flash_init tries to directly calls flash_real_protect without
CFG_FLASH_PROTECTION.
It produces compile time error because flash_real_protect is not defined
without CFG_FLASH_PROTECTION.
I don't think such dependency between configuration is a good 
implementation.
If you still don't like the implementation just send a better patch.

Again, I don't really like to mess up with you because of some philosophy or 
spirit.
Software engineering is just for convenience after all.

Best regards,
Sangmoon Kim

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Brian Waite]

On Monday 22 August 2005 6:46 pm, Wolfgang Denk wrote:
> > Just a couple of emails ago you were saying all sectors should be in
> > writable state in U-Boot. This is a policy which is announced today by
> > you.
>
> OK.
Why does *u-boot* want the FLASH in a writeable state? Some boards may want 
FLASH in a writeable state, some command lines may want FLASH in a writable 
state, but u-boot does not need FLASH in a writeable state to boot. 
 
>
> > Leaving the state of sectors (except for U-Boot managed sectors) until
> > user takes explicit lock/unlock action as they are is another policy .
>
> I don't call this a policy.
Would you call it a policy of u-boot not to change the state of hardware in 
common code unless it is needed to run u-boot?  Ie many cpu features are not 
enabled by default in u-boot.  Would changing the powered up state of the 
FLASH be considered a deviation of this policy? 
>
> > Why do you think it is OK for U-Boot to unlock sectors/blocks that it
> > knows nothing about their usage? Wouldn't leaving these sectors in a
>
> Because in the general case (and this is what cfi_flash is used  for)
> you  don't  expect  to  have  any  hardware protected sectors. Not in
> U-Boot, and neither in Linux when you for example want to  use  these
> for a writable MTD partition.
>
In the general case, if I lock my FLASH to protect a Linux kernel I have there 
I have explicitly locked that region and I do not expect anyone to unlock it 
for me. 

> > safer state a common sense approach?
>
> Not for me. I don't like the hardware doing magic  things  to  me.  I
> want to be in control over the hardware - not vice versa.
>
You should change that in the board package. I do not consider this magic if I 
have spec-ed the FLASH  part for my board because of this feature. I consider 
it software magic to undo a a feature I designed in.

> > While you see it important to protect U-Boot environment (for various
> > reasons and I agree), you do not seem to consider consistent protection
> > for another area of flash that may be storing equally vital information
> > for software system. Why?
>
> Not on  a  *automatic*  base.  I  accept  this  only  if  explicitely
> requested  by  the user (by using the "protect on" command) *and* the
> board designer (by providing a  flash  implementation  that  supports
> hardware  write  protection both in hardware [by selcting appropriate
> flash chips] and in software [by  enabling  the  needed  features  in
> U-Boot]).
>
> As mentioned before: if you want to have this on a  board,  OK,  then
> implement  it there and put apropriate big warnings and notes in your
> board documentation. If this is general code which is  used  by  many
> boards  that  you  don't  control  (and  do not test!) then I want to
> provide a common interface. And common behaviour is that flash can be
> erased and written to in the boot loader.
>
You cannot tell the difference in the Intel part that was origianlly 
referenced between sectors locked at reset and sectors explicitly locked. 
Therfore you are unlocking explicitly locked sectors at the same time. 

Another implimentation detail would be the additional time needed to unprotect 
the FLASH at each powerup. On my board, with 64 MB of FLASH, you would be 
adding ~2 seconds to the u-boot boot time by unprotecting the FLASH. I would 
then need to waste ~1.5 seconds re locking most of my FLASH. (I only provide 
write access to a small portion of the 64 MB). Your policy will add almost 
3.5 seconds to boot time.  

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

In message <200508231047.37928.bwaite@irobot.com> you wrote:
>
> Why does *u-boot* want the FLASH in a writeable state? Some boards may want 

This has already been explained before: to provide a consistent  user
interface across as many systems as possible (i. e. all those without
special requirements).

> FLASH in a writeable state, some command lines may want FLASH in a writable 
> state, but u-boot does not need FLASH in a writeable state to boot. 

Indeed. So what are you trying to proof?

> Would you call it a policy of u-boot not to change the state of hardware in 
> common code unless it is needed to run u-boot?  Ie many cpu features are not 

No. We do many things that are not strictly needed to run U-Boot, for
several reasons.

> enabled by default in u-boot.  Would changing the powered up state of the 
> FLASH be considered a deviation of this policy? 

See above.

> > Because in the general case (and this is what cfi_flash is used  for)
> > you  don't  expect  to  have  any  hardware protected sectors. Not in
> > U-Boot, and neither in Linux when you for example want to  use  these
> > for a writable MTD partition.
> >
> In the general case, if I lock my FLASH to protect a Linux kernel I have there 
> I have explicitly locked that region and I do not expect anyone to unlock it 
> for me. 

This requires that your flash chips supports hardware locking in  the
first  place,  which  is  not  ger=nerally the case, so your "general
case" is not so much general. I think I explained this before. Please
re-read my posting.

> You should change that in the board package. I do not consider this magic if I 
> have spec-ed the FLASH  part for my board because of this feature. I consider 
> it software magic to undo a a feature I designed in.

If you have this requirement, then please feel free to  implement  it
in your board specific driver. I wrote this before, too.

> Another implimentation detail would be the additional time needed to unprotect 
> the FLASH at each powerup. On my board, with 64 MB of FLASH, you would be 
> adding ~2 seconds to the u-boot boot time by unprotecting the FLASH. I would 
> then need to waste ~1.5 seconds re locking most of my FLASH. (I only provide 
> write access to a small portion of the 64 MB). Your policy will add almost 
> 3.5 seconds to boot time.  

Arghh... it is *obvious* that the one-size-fits-all  approach  cannot
work. If you don't like it, don't use it. 

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
Prepare for tomorrow -- get ready.
	-- Edith Keeler, "The City On the Edge of Forever",
	   stardate unknown

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Yuli Barcohen]

>>>>> Wolfgang Denk writes:

...

    Wolfgang> Because in the general case (and this is what cfi_flash is
    Wolfgang> used for) you don't expect to have any hardware protected
    Wolfgang> sectors. Not in U-Boot, and neither in Linux when you for
    Wolfgang> example want to use these for a writable MTD partition.

    Brian> In the general case, if I lock my FLASH to protect a Linux
    Brian> kernel I have there I have explicitly locked that region and
    Brian> I do not expect anyone to unlock it for me.

    Wolfgang> This requires that your flash chips supports hardware
    Wolfgang> locking in the first place, which is not ger=nerally the
    Wolfgang> case, so your "general case" is not so much general.

...

Every Intel-compatible chip supports hardware locking while no
AMD-compatible does. So, for AMD you don't need any special code to have
the flash in writable state and for Intel, "general case" is explained
above.

-- 
========================================================================
 Yuli Barcohen       | Phone +972-9-765-1788 |  Software Project Leader
 yuli at arabellasw.com | Fax   +972-9-765-7494 | Arabella Software, Israel
========================================================================

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Detlev Zundel]

Hello all,

sorry for jumping in so late in this discussion but I just want to
make my opinion heard.

For what its worth, I consider "unlocking everything besides u-boot
relevant code (or add something to the opposite in your board code)"
as policy.  I think U-Boot should provide the mechanisms, i.e. commands
to protect and unprotect sectors and by correctly indicating protected
sectors in the fli display.

As it seems from my perspective, these latter goals have already been
achieved with the available commands.  They leave sector protection to
be an aspect of the hardware that is not influenced implicitely or per
default by u-boot running effectively including hardware design
decisions being included in the way u-boot runs.

I think Wolfgang votes against this as he expects u-boot to provide
him with a common view over many boards - thus seeing the hardware
protection by default rather as a design decision to be abstracted by
u-boot.

Therefore I guess one question that should drive the design of u-boot
code is

Q: Is hardware protection in flash chips a deliberate measure by the
   board designer not to be abstracted by the bootloader?

If the answer is "no" then the current design is presenting this
u-boot abstraction on every board.

If on the other hand the answer is "yes" then I think u-boot should
not make all flash writable by default.

Just my 2 cents - Cheers
  Detlev

-- 
Another helpful hint for successful MIME processing:

application/msword; rm -f %s; description="MS Word Text";

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Hi Detlev,

I am almost banished for causing this discussion and I will probably be 
subjected to "tar and feathers" after this :)

Detlev Zundel wrote:
> Hello all,
> 
> sorry for jumping in so late in this discussion but I just want to
> make my opinion heard.
> 
> For what its worth, I consider "unlocking everything besides u-boot
> relevant code (or add something to the opposite in your board code)"
> as policy.  I think U-Boot should provide the mechanisms, i.e. commands
> to protect and unprotect sectors and by correctly indicating protected
> sectors in the fli display.

Yes, I consider this as a "policy" as well.

> I think Wolfgang votes against this as he expects u-boot to provide
> him with a common view over many boards - thus seeing the hardware
> protection by default rather as a design decision to be abstracted by
> u-boot.

But, he is making an assumption on the usage of portions of flash which 
is not defined by U-Boot.

A bootloader is only one part of the total software solution. There is 
typically an application loaded following the U-Boot portion that 
defines the usage of portion of hardware that might be shared with 
U-Boot. Even if you might have common hardware you could end up uncommon 
usage since it is more appropriate to the application. This is typically 
the case for off-the-shelf boards that gets embedded in many different 
applications.

> Therefore I guess one question that should drive the design of u-boot
> code is
> 
> Q: Is hardware protection in flash chips a deliberate measure by the
>    board designer not to be abstracted by the bootloader?
 > If the answer is "no" then the current design is presenting this
 >
 > u-boot abstraction on every board.
 >
 > If on the other hand the answer is "yes" then I think u-boot should
 > not make all flash writable by default.

The answer is "No" in some cases and in others it is a "Yes". However, 
once the choice of chip is made, there are implications of the choice.

When the answer is "No", convenience, availability, cost are probably 
factors that resulted in the current hardware design. In this case, the 
designer generally does not have a preference. Other factors have had 
higher importance with that choice.

When the answer is "Yes", the designer really desires to use that 
feature as presented by the hardware.

So, should U-Boot be making the abstraction suitable for this lowest 
common denomiator, denying the capabilities of more featured chips? I 
think U-Boot should not match the common denominator but rather reflect 
the capabilities of actual hardware and provide necessary and sufficient 
tools to deal with it. I would like to see the common view in the tools 
presented but any further abstraction is abstraction gone too far, too 
rigid.

I think it is wrong for U-Boot to make any abstraction on any portion of 
flash that it does not know anything about it's use. The tools available 
today within U-Boot provides all the necessary and sufficient facilities 
to deal with any usage model.

Best regards,
Tolunay Orkun

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

Dear Tolunay,

in message <430CEC01.1070800@orkun.us> you wrote:
> 
> > I think Wolfgang votes against this as he expects u-boot to provide
> > him with a common view over many boards - thus seeing the hardware
> > protection by default rather as a design decision to be abstracted by
> > u-boot.
> 
> But, he is making an assumption on the usage of portions of flash which 
> is not defined by U-Boot.

I am not. As I wrote before I am aware that specific requirements may
exist, and that these shallbe handled where they belong  to:  in  the
board specific sections of the code.

> When the answer is "Yes", the designer really desires to use that 
> feature as presented by the hardware.

And then such board specific design shall  be  dealt  with  in  board
specific code.

> So, should U-Boot be making the abstraction suitable for this lowest 
> common denomiator, denying the capabilities of more featured chips? I 

Yes, as the default case. But giving you each  and  every  option  to
handle things as you like in your board specific code.

> I think it is wrong for U-Boot to make any abstraction on any portion of 
> flash that it does not know anything about it's use. The tools available 
> today within U-Boot provides all the necessary and sufficient facilities 
> to deal with any usage model.

Right. The discussion is just what the  default  configuration  shall
look like, and I get tired of pointing this out again and again.

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
Why can you only have two doors on a chicken coop? If it had four  it
would be a chicken sedan.

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Brian Waite]

On Wednesday 24 August 2005 7:12 pm, Wolfgang Denk wrote:
> Dear Tolunay,
>
> in message <430CEC01.1070800@orkun.us> you wrote:
> > > I think Wolfgang votes against this as he expects u-boot to provide
> > > him with a common view over many boards - thus seeing the hardware
> > > protection by default rather as a design decision to be abstracted by
> > > u-boot.
> >
> > But, he is making an assumption on the usage of portions of flash which
> > is not defined by U-Boot.
>
> I am not. As I wrote before I am aware that specific requirements may
> exist, and that these shallbe handled where they belong  to:  in  the
> board specific sections of the code.
>
Ok I surrender the point that common code shall unprotect all FLASH at boot. 
Can I ask you to consider a method of allowing the board maintainers to 
easily override this feature, either through a conditional compile or through 
a function pointer to be overridden by the board maintainer if desired? Here 
is my issue, I have been porting ppcboot and u-boot to boards for nearly 4 
years now. In all this time I have happend to have used Inelstrata FLASH with 
hardware protection, the protection was deigned into the product. That said, 
to forward port these boards to newer version of u-boot I will have to either 
patch common code, or copy cfi_flash.c to my board directoy remove the 
unprotect in flash_init() and maintain the copy on common code in each board 
set. This does not seem to make for the most modular design of u-boot. Can we 
have a hook such as initflash() that will by default call the common 
flash_init() if not overridden by the board. At least then I only have to 
maintain a copy of a single function. 

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Tolunay Orkun]

Wolfgang Denk wrote:

>>I think it is wrong for U-Boot to make any abstraction on any portion of 
>>flash that it does not know anything about it's use. The tools available 
>>today within U-Boot provides all the necessary and sufficient facilities 
>>to deal with any usage model.
> 
> 
> Right. The discussion is just what the  default  configuration  shall
> look like, and I get tired of pointing this out again and again.

You are thinking that the default configuration you have chosen for a 
common driver is "mainstream" and more common than not and I an not so 
sure about it. My experience is pointing to the opposite. I think 
"common" default configuration should be covering as many cases as 
possible rather than shrink the potential application of a common driver.

Deviation from your chosen "default" will mean more  board designers 
will need to duplicate cfi_flash.c and have maintain fixes/changes 
introduced to cfi_flash.c separately. This is back to the time where 
there were many flash.c in board directories where each had 90% common code.

To solve this, I guess we will need more hooks (increase 
configurability) into this common driver so we can override the behavior 
of cfi_flash.c from board directories and prevent code duplication. I 
hope you would not object to this as long as the code size for other 
boards would not be increased.

Best regards,
Tolunay

U-Boot policy on flash protection (was [U-Boot-Users] [PATCH] cfi_flash.c patches)

[Detlev Zundel]

Hi Tolunay & all board maintainers,

> Wolfgang Denk wrote:
>
>>> I think it is wrong for U-Boot to make any abstraction on any
>>> portion of flash that it does not know anything about it's use. The
>>> tools available today within U-Boot provides all the necessary and
>>> sufficient facilities to deal with any usage model.
>> Right. The discussion is just what the  default  configuration  shall
>> look like, and I get tired of pointing this out again and again.
>
> You are thinking that the default configuration you have chosen for a
> common driver is "mainstream" and more common than not and I an not so
> sure about it. My experience is pointing to the opposite. I think
> "common" default configuration should be covering as many cases as
> possible rather than shrink the potential application of a common
> driver.

To be honest, I share your point of view in this respect but to
convince Wolfgang, more of the maintainers have to speak up.

In private communications it turns out that Wolfgang weighs in years
of experience added to the technical question so the board maintainers
have to come up with sufficient proof that this really is a point
where a consensus has to be reached.

As one example note the Linux MTD subsystem which (as I gather) still
has some problems with hw protected chips - so unprotecting all flash
in U-Boot makes live easier for those using MTD.

To sum up my point again - I don't see any "wrong" or "correct"
solution - I simply feel that the current U-Boot policy of using a
least common denominator doesn't anymore fit what we see today.

> Deviation from your chosen "default" will mean more  board designers
> will need to duplicate cfi_flash.c and have maintain fixes/changes
> introduced to cfi_flash.c separately. This is back to the time where
> there were many flash.c in board directories where each had 90% common
> code.
>
> To solve this, I guess we will need more hooks (increase
> configurability) into this common driver so we can override the
> behavior of cfi_flash.c from board directories and prevent code
> duplication. I hope you would not object to this as long as the code
> size for other boards would not be increased.

This is actually a very strong point - Wolfgang speculates that most
of the board ports will only make use of the "standard" behaviour of
the flash driver so that in the end he ends up with a broad range of
systems working alike.  If many porters are going to deviate from this
standard then the situation changes fundamentally and it then would
make sense to adapt a different policy so that more boards work alike.

So in the end it is a question of what the board maintainers will
choose - if many deviate by including board specific code then
Wolfgang will surely reconsider his position.  If not too many people
care then things will stay as they are.  So its up to the people here
on this mailing list - speak up if you want your opinion to be heard.
And by the way - don't forget to consider the "user" of U-Boot also.
Sometimes things being pleasant for U-Boot porters make life
unneccessary hard for them.

Ok, my vote goes for not trying to abstrace the hw protection of flash
chips in U-Boot - and by the way - no I am not trying to prolong a
fruitless thread, I am just trying to really discuss an important part
of the U-Boot design.

Cheers
  Detlev

-- 
LISP has survived for 21 years because it is an approximate local
optimum in the space of programming languages.
                                           -- John McCarthy (1980)

U-Boot policy on flash protection (was [U-Boot-Users] [PATCH] cfi_flash.c patches)

[Wolfgang Denk]

In message <m2br3klrc8.fsf_-_@sowhat.denx.de> you wrote:
> 
> To be honest, I share your point of view in this respect but to
> convince Wolfgang, more of the maintainers have to speak up.

And users, please!

> In private communications it turns out that Wolfgang weighs in years
> of experience added to the technical question so the board maintainers
> have to come up with sufficient proof that this really is a point
> where a consensus has to be reached.

It's  not  only  board  maintainers.  Actually  I  don't  care  about
maintainers  :-)  They usually understand the code and know what they
are doing, and why, and why things work as they do.

I also try to care about stupid users, who often don;t even bother to
use the help function of a command, not to mention reading the FAQ or
even the whole manual.

The maintainer is just one person, but his board configuration may be
a pain for many, many users - most of them unheard. Well,  a  few  of
send "bug reports" to me. Maybe more than a few :-(

> This is actually a very strong point - Wolfgang speculates that most
> of the board ports will only make use of the "standard" behaviour of
> the flash driver so that in the end he ends up with a broad range of
> systems working alike.  If many porters are going to deviate from this
> standard then the situation changes fundamentally and it then would
> make sense to adapt a different policy so that more boards work alike.

Right. "Default" should be what is being  used  by  the  overwhelming
majority of the users (read: *users*, not maintainers).

> So in the end it is a question of what the board maintainers will
> choose - if many deviate by including board specific code then

No, it's what users want. The maintainer shall be just a  servant  of
his users. Amen.

> Wolfgang will surely reconsider his position.  If not too many people

I do. Even if I seem to stand like a rock I listen to what the  water
is doing to my ankles :-)

> And by the way - don't forget to consider the "user" of U-Boot also.
> Sometimes things being pleasant for U-Boot porters make life
> unneccessary hard for them.

Indeed.


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
A dog always bit deepest on the veterinary hand.
                                    - Terry Pratchett, _Wyrd Sisters_

[U-Boot-Users] [PATCH] cfi_flash.c patches

[Wolfgang Denk]

In message <00b301c5a476$5c707fe0$212d4cdc@smkim> you wrote:
> 
> The two patches attached are for drivers/cfi_flash.c.
> 
> cfi_flash-protect.patch adds CFG_FLASH_PROTECT_CLEAR
> because for some flash memories(such as 28F320C3)
> all banks are protected after reset.
> 
> cfi_flash-buffer.patch makes write_buff not to call 
> flash_write_cfibuffer if buffer_size is1.
> Because for flash memories with buffer_size 1,
> buffer write is not supported.

Applied, thanks. But please provide a  proper  CHANGELOG  entry  next
time.

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
Old programmers never die, they just branch to a new address.