[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Woodruff, Richard]

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 at denx.de [mailto:wd at denx.de]
> Sent: Wednesday, September 28, 2005 6:46 PM
> To: Woodruff, Richard
> Cc: u-boot-users at 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 at denx.de
> Many aligators will be slain, but the swamp will remain.

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Wolfgang Denk]

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 at denx.de [mailto:wd at denx.de]
> > Sent: Wednesday, September 28, 2005 6:46 PM
> > To: Woodruff, Richard
> > Cc: u-boot-users at lists.sourceforge.net
> > Subject: Re: [U-Boot-Users] [PATCH] Update OMAP242x for git head (plus
> > sign).

[Full quote deleted].


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
I wish Captain Vimes were here. He wouldn't have  known  what  to  do
either, but he's got a much better vocabulary to be baffled in.
                                 - Terry Pratchett, _Guards! Guards!_

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Tolunay Orkun]

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

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Wolfgang Denk]

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

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
People seldom know what they want until you give them what  they  ask
for.

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Tolunay Orkun]

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

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Wolfgang Denk]

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

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Men will always be men -- no matter where they are.
	-- Harry Mudd, "Mudd's Women", stardate 1329.8

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Tolunay Orkun]

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

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Wolfgang Denk]

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

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Don't tell me how hard you work.  Tell me how much you get done.
- James J. Ling

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Woodruff, Richard]

Hello Wolfgang,
 
Attached is a patch generated today from a pull from the u-boot.git tree
using cg-mkpatch.  Please apply this and dump any patches you might have
in your queue.  Your updates over the weekend left the omap242x
processor
on head non-functional.
 
This patch is completely 24xx contained.  I have stopped using
drivers/cfi_flash.c and moved to using a private board copy as discussed
in an earlier thread.  The couple of files external touches are just
changing from CONFG_OMAP2420 to CONFIG_OMAP24XX.

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

Signed-off-by: Richard Woodruff <r-woodruff2@ti.com>
 
README                                  |    2
 board/omap2420h4/Makefile               |    2
 board/omap2420h4/mem.c                  |  150 ++++++++-
 board/omap2420h4/omap2420h4.c           |  497
+++++++++++++-------------
-----
 board/omap2420h4/platform.S             |    2
 board/omap2420h4/sys_info.c             |   61 +++-
 common/env_flash.c                      |    8
 cpu/arm1136/cpu.c                       |    3
 cpu/arm1136/interrupts.c                |    2
 cpu/arm1136/start.S                     |    4
 drivers/ns16550.c                       |    4
 drivers/serial.c                        |    8
 include/asm-arm/arch-arm1136/clocks.h   |   91 +-----
 include/asm-arm/arch-arm1136/mem.h      |   61 ++--
 include/asm-arm/arch-arm1136/mux.h      |  215 +++++++------
 include/asm-arm/arch-arm1136/omap2420.h |  151 ++-------
 include/asm-arm/arch-arm1136/sys_info.h |   57 +++-
 include/configs/omap2420h4.h            |   73 +----
 lib_arm/cache.c                         |    2
 19 files changed, 643 insertions(+), 750 deletions(-)
 
Regards,
Richard W.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: omap242x-git.patch.gz
Type: application/x-gzip
Size: 21676 bytes
Desc: omap242x-git.patch.gz
Url : http://lists.denx.de/pipermail/u-boot/attachments/20050928/3e2a0de3/attachment.bin 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: changelog-git.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20050928/3e2a0de3/attachment.txt 

[U-Boot-Users] [PATCH] Update OMAP242x for git head (plus sign).

[Wolfgang Denk]

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 at denx.de
Many aligators will be slain, but the swamp will remain.