[U-Boot-Users] [PATCH] cfi_flash.c patches
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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.
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
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.
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
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
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
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.
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
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
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
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
participants (7)
-
Brian Waite -
Detlev Zundel -
Sangmoon Kim -
Scott McNutt -
Tolunay Orkun -
Wolfgang Denk -
Yuli Barcohen