RE: [U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).
Wolfgang,
Yes indeed I duplicated cfi_flash.c. The changes in my previous patch to CFI were rejected. You indicated see the previous list mail on the subject. Your assertion at that time was to put things into the board specific files was one way to go.
The patch I had at that time was ifdef'ed just for my board in CFI and for the flash part I'm using would seem to show errors. I think those OMAP specific ifdef's may fix errors, but I being its such a highly coupled driver it is very difficult to add in changes with out breaking a lot of boards. How do you suggest real integration into that driver? The 1% changes are minor and isolated now and that's not acceptable what is?
I certainly can break the patch into parts, but they are all coupled. The chip and board are very new have went though flux and this results in large patches at this time. This is not a PPC821 which shouldn't have a lot of change.
Best Regards, Richard Woodruff
-----Original Message----- From: wd@denx.de [mailto:wd@denx.de] Sent: Wednesday, September 28, 2005 6:46 PM To: Woodruff, Richard Cc: u-boot-users@lists.sourceforge.net Subject: Re: [U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).
In message
EA12F909C0431D458B9D18A176BEE4A50246905D@dlee02.ent.ti.com
you wrote:
- Patch by Richard Woodruff, 28 Sep 2005:
OMAP242x H4 board update
- Switch to private cfi_flah.c file.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Make work for 2422 mono-ddr.
- Make work for 2420-POP.
- Use clock gauging to determine crystal speed.
- Fix warm reset problem with bad OTGCTRL access.
- Switch to DDR unlock mode operation for DLL errata.
- Remove obsolete APTIX conditionals.
- Display more information at startup.
- Use 24XX instead of 2420 in prep for future chips.
I reject this patch. You duplicate the common drivers/cfi_flash.c file (1269 lines) with less than 1% modifications into a board specific file (board/omap2420h4/cfi_flash.c).
This is not acceptable.
Also, your patch is much too big. Please break it down into digestable chunks. Please see the README, and re-read my posting http://sourceforge.net/mailarchive/message.php?msg_id=12658390
In short:
Make separate commits for logically separate changes.
Best regards,
Wolfgang Denk
-- Software Engineering: Embedded and Realtime Systems, Embedded Linux Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Many aligators will be slain, but the swamp will remain.
A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail?
Please don't top-post/full-quote! This is *really* annoying.
In message EA12F909C0431D458B9D18A176BEE4A502469206@dlee02.ent.ti.com you wrote:
Yes indeed I duplicated cfi_flash.c. The changes in my previous patch to CFI were rejected. You indicated see the previous list mail on the subject. Your assertion at that time was to put things into the board specific files was one way to go.
Yes, but not by duplicating thousands of lines of code!
The patch I had at that time was ifdef'ed just for my board in CFI and for the flash part I'm using would seem to show errors. I think those OMAP specific ifdef's may fix errors, but I being its such a highly coupled driver it is very difficult to add in changes with out breaking a lot of boards. How do you suggest real integration into that driver? The 1% changes are minor and isolated now and that's not acceptable what is?
The problem with the previous discussion was that there was no agree- ment wether the board should come up with the flash generally un- locked (which is probably what most users expect and what is assumed in the existing documentation), or if it should be left untouched (which is what some board maintainers want). Detlev ZUndel tried to start a poll of opinions, which - to the best of my knowledge - resulted in no replies at all.
I can see only one way to keep everybody satisfied:
The unlocking feature gets implemented (as part of the common CFI flash driver), with the default that flash automatically gets un- locked when the board comes up (this gives most users the expected standard behaviour).
A new environment variable "flash_unlock" can be defined which, when set to a value that starts with a 'n' (like "setenv flash_unlock no") will turn off the automatic unlock (so board maintainers or users who need to optimize boot times can turn this off, eventually as a default by pre-setting this variable in thier board config file).
It shall be a requirement (1) that the existing commands "lock" and "unlock" work as expected and (2) that the "flinfo" command shows the current lock state correctly, i. e. if the flash comes up locked it *must* be displayed as read-only.
Comments welcome.
[I will accept a clean patch that implements such a solution.]
I certainly can break the patch into parts, but they are all coupled. The chip and board are very new have went though flux and this results in large patches at this time. This is not a PPC821 which shouldn't have a lot of change.
Please separate at least the cfi_flash part.
-----Original Message----- From: wd@denx.de [mailto:wd@denx.de] Sent: Wednesday, September 28, 2005 6:46 PM To: Woodruff, Richard Cc: u-boot-users@lists.sourceforge.net Subject: Re: [U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).
[Full quote deleted].
Wolfgang Denk
Dear Wolfgang,
Wolfgang Denk wrote:
The problem with the previous discussion was that there was no agree- ment wether the board should come up with the flash generally un- locked (which is probably what most users expect and what is assumed in the existing documentation), or if it should be left untouched (which is what some board maintainers want). Detlev ZUndel tried to start a poll of opinions, which - to the best of my knowledge - resulted in no replies at all.
Actually, I think I had replied to that and there were a couple more from others as far as I can remember.
I can see only one way to keep everybody satisfied:
I am glad we are now looking at formulating a (better) solution now.
The unlocking feature gets implemented (as part of the common CFI flash driver), with the default that flash automatically gets un- locked when the board comes up (this gives most users the expected standard behaviour).
A new environment variable "flash_unlock" can be defined which, when set to a value that starts with a 'n' (like "setenv flash_unlock no") will turn off the automatic unlock (so board maintainers or users who need to optimize boot times can turn this off, eventually as a default by pre-setting this variable in thier board config file).
Currently, some Intel flash parts can maintain the lock/unlock state for their sectors. Is flash driver going to unlock these sectors if flash_unlock is undefined or leave at "current" state?
Unlocking all sectors on these parts would change the current behavior of the boards unless either the environment variable is defined or board config file is also updated.
I prefer undefined "flash_unlock" environment variable to mean use whatever default action in board config file or if none is defined in board config file assume flash_unlock=no. In short this means:
1) All boards with no locking support would not be effected anyway. 2) All boards with flash that maintain lock state from last change would work unmodified as well with existing environment settings and board config file. 3) All new flash parts that have all sectors coming up in locked state are used on relatively newer board designs and the board designers would need to add CONFIG_FLASH_UNLOCK (or something like that) to their board config file.
Also, Instead of all black or white, why don't we have unlock regions for partial unlocking sections of flash like like jffs2 partitions? E.g.
setenv flash_unlock no setenv flash_unlock yes setenv flash_unlock 1:12-39,2:0xfff00000-0xfffbffff,3:all
The last one above would give partial regions to unlock. If comma is not OK we can use another seperator character. Basically, it would list a number of sector range specifications as used by other flash related commands use.
Similarly if we adopt partial unlocks we can also have partial locks as well via flash_lock environment variable.
It shall be a requirement (1) that the existing commands "lock" and "unlock" work as expected and (2) that the "flinfo" command shows the current lock state correctly, i. e. if the flash comes up locked it *must* be displayed as read-only.
100% agreed.
Comments welcome.
Thank for asking. :)
Best regards, Tolunay
In message 433C4E60.7010700@orkun.us you wrote:
Actually, I think I had replied to that and there were a couple more from others as far as I can remember.
Sorry, must be my volatile memory then. I have to admit that I did not re-scan the archive.
The unlocking feature gets implemented (as part of the common CFI flash driver), with the default that flash automatically gets un- locked when the board comes up (this gives most users the expected standard behaviour).
A new environment variable "flash_unlock" can be defined which, when set to a value that starts with a 'n' (like "setenv flash_unlock no") will turn off the automatic unlock (so board maintainers or users who need to optimize boot times can turn this off, eventually as a default by pre-setting this variable in thier board config file).
Currently, some Intel flash parts can maintain the lock/unlock state for their sectors. Is flash driver going to unlock these sectors if flash_unlock is undefined or leave at "current" state?
If flash_unlock is undefined or not starting with 'n' hen flash shall be unlocked, to make it behave identically like flash on any other systems. Exception is the area occupied by U-boot and the envrionment sector(s) which shall be protected by default unless unpreotected (but then they shall be protected again after a reset) - exactly what you see with other flash types.
Unlocking all sectors on these parts would change the current behavior of the boards unless either the environment variable is defined or board config file is also updated.
Yes. From my point of view this means fixing these boards :-)
I prefer undefined "flash_unlock" environment variable to mean use whatever default action in board config file or if none is defined in board config file assume flash_unlock=no. In short this means:
I want to make sure that the default behaviour is the same for all users.
Also, Instead of all black or white, why don't we have unlock regions for partial unlocking sections of flash like like jffs2 partitions? E.g.
That would be even more confusing for most users.
Sorry, I can perfectly understand your intentions from a developers point of view. But guess how many users there are for each of us developers? They outnumber us many, many times. And even if there was perfect documentation for each of the ports - guess how many users would not read it? Providing the same look and feel across all implementations is an important issue for me. And I see it as a part of my task as a maintainer to keep the design simple and predictable.
KISS.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Unlocking all sectors on these parts would change the current behavior of the boards unless either the environment variable is defined or board config file is also updated.
Yes. From my point of view this means fixing these boards :-)
OK
Also, Instead of all black or white, why don't we have unlock regions for partial unlocking sections of flash like like jffs2 partitions? E.g.
That would be even more confusing for most users.
Sorry, I can perfectly understand your intentions from a developers point of view. But guess how many users there are for each of us developers? They outnumber us many, many times. And even if there was perfect documentation for each of the ports - guess how many users would not read it? Providing the same look and feel across all implementations is an important issue for me. And I see it as a part of my task as a maintainer to keep the design simple and predictable.
KISS.
OK. Probably an on/off knob for user is enough. I think board designer needs a bit more help in setting a policy for his/her board.
Would you consider something like CFG_FLASH_PROTECT_LIST in board config file which defines an array of blocks that needs to be kept protected? This would be a list similar to CFG_FLASH_BANKS_LIST. This would take care of important sections of flash (beyond u-boot and environment) that needs to be protected for that particular board.
Best regards, Tolunay
Dear Tolunay,
in message 433C658F.8010209@orkun.us you wrote:
OK. Probably an on/off knob for user is enough. I think board designer needs a bit more help in setting a policy for his/her board.
Just make sure you don't get into the way of your users.
"UNIX was not designed to stop you from doing stupid things, because that would also stop you from doing clever things." - Doug Gwyn
Would you consider something like CFG_FLASH_PROTECT_LIST in board config file which defines an array of blocks that needs to be kept protected? This would be a list similar to CFG_FLASH_BANKS_LIST. This would take care of important sections of flash (beyond u-boot and environment) that needs to be protected for that particular board.
Just to make sure I understand your intentions: such marked blocks would be handled in exactly the same way as the areas which are used to store U-Boot and the environment, i. e. they are protected by default after each reset, but can be unprotected by the user?
I see that such an extension would be useful for example to store FPGA images which might be vital to get the board running at all, and similar things.
Yes, I would consider such an extension, but then it must be generally available, i. e. for all flash types. I understand that probably the cfi_flash driver would be the first to support this feature, but maybe the code can be made generic enough that it can be called by custom flash drivers as well so all boards can use it (if they like), and we can use the same method for protecting U-Boot and it's environment sectors?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Would you consider something like CFG_FLASH_PROTECT_LIST in board config file which defines an array of blocks that needs to be kept protected? This would be a list similar to CFG_FLASH_BANKS_LIST. This would take care of important sections of flash (beyond u-boot and environment) that needs to be protected for that particular board.
Just to make sure I understand your intentions: such marked blocks would be handled in exactly the same way as the areas which are used to store U-Boot and the environment, i. e. they are protected by default after each reset, but can be unprotected by the user?
Yes
I see that such an extension would be useful for example to store FPGA images which might be vital to get the board running at all, and similar things.
Yes. Also useful to protect linux/initrd etc. images.
Yes, I would consider such an extension, but then it must be generally available, i. e. for all flash types. I understand that probably the cfi_flash driver would be the first to support this feature, but maybe the code can be made generic enough that it can be called by custom flash drivers as well so all boards can use it (if they like), and we can use the same method for protecting U-Boot and it's environment sectors?
I was initially thinking to add that functionality to flash_init() of cfi_flash.c. Other flash.c would need to implement the same way. However, there is a way to implement it in flash driver agnostic way.
I can create a generic flash_protect_init() function (pick a better name if you don't like it) which is called after flash_init() from lib_xxx/board.c files.
The implementation of flash_protect_init() would sit in a common file (perhaps in common/flash.c). flash_protect_init() would call flash_protect() for the list provided via CFG_FLASH_PROTECT_LIST (if defined) as well as for U-Boot code and environment areas.
This way the feature can be integrated consistently for all flash chip drivers and protection of U-Boot and environment would be standardized (and duplicate code could be removed from cfi_flash.c and other drivers)
Best regards, Tolunay
In message 433C7A66.1000904@orkun.us you wrote:
I was initially thinking to add that functionality to flash_init() of cfi_flash.c. Other flash.c would need to implement the same way. However, there is a way to implement it in flash driver agnostic way.
That's what I had in mind.
I can create a generic flash_protect_init() function (pick a better name if you don't like it) which is called after flash_init() from lib_xxx/board.c files.
Let's call it flash_init_protect().
The implementation of flash_protect_init() would sit in a common file (perhaps in common/flash.c). flash_protect_init() would call flash_protect() for the list provided via CFG_FLASH_PROTECT_LIST (if defined) as well as for U-Boot code and environment areas.
Yes. And here I would not only accept, but appreciate if the default setting could be overwritten using an environment variable so it can be udjusted in the field if something changes later.
This way the feature can be integrated consistently for all flash chip drivers and protection of U-Boot and environment would be standardized (and duplicate code could be removed from cfi_flash.c and other drivers)
Sounds excellent.
Best regards,
Wolfgang Denk
participants (3)
-
Tolunay Orkun -
Wolfgang Denk -
Woodruff, Richard