[U-Boot-Users] U-boot code etiquette

[U-Boot-Users] U-boot code etiquette

[Neil Bryan]

Hello group,

I have a problem to solve which effects U-boot environment variable 
storage.  Before I submit changes to core U-boot code, I want to
find out from the group 'what is acceptable code to modify?'.

I am afraid that this description is quite long, but I do not know
how to describe it in very few words.

TARGET DESCRIPTION:
Based on the AT91RM9200 MCU from Atmel with 8kbytes of I2C EEPROM.
For my initial investigation I am using the AT91RM9200DK port of 
U-boot.

PROBLEM:
The board has a limited EEPROM storage area and it has been decided that
U-boot environment variables are to be stored in EEPROM.  

U-boot uses quite a large amount of environment variable space,
in fact the default size for the AM91RM9200DK CFG_ENV_SIZE is 0x2000
(8kbytes). So all my EEPROM disappears to U-boot leaving non for Linux.

I can reduce this by changing ./include/configs/at91rm9200dk.h 
CFG_ENV_SIZE to 0x800 (2kbytes).  U-boot builds, runs and all is well.

BUT, cetain variables such as the board IP address need to be common 
to both Linux and U-boot.  So if the board IP address is changed by the
OS or by Linux, the change needs to be visible to the other.

SOLUTION:
To divide the EEPROM into a psuedo-file structure.  We have data 
structures of varying lengths and types to pack more data into the 
EEPROM.  We need to support a variety of OSes on the board and will 
have several OS-specific data structures.

One data structure would be reserved for U-boot, length 2kBytes,

However, common values such as IP address will be stored in
this psuedo-file structure.

AFFECT ON U-BOOT:
When U-boot reads the environment variables, it needs to check in two
places in EEPROM:

1) The standard U-boot storage of NUL terminated strings, e.g.
bootdelay, bootargs, bootcmd, etc.

2) Our board specific data structure(s) which most likely will be 
baudrate, ethaddr, ethxaddr, ipaddr, serverip.

COMMENTS:

 o I don't want to make board-specific changes in common code areas.
   I can see my proposal affecting cmd_nvedit.c.  This code does
   not have pre-processing for multiple board-types and so 
   architecturaly it feels wrong. 

 o I need to hook into functions getenv() and setenv().  I will read
   a table of constant strings and compare with the environment 
   variable which is being sought and then parse it appropriately.

So, people with lots of U-boot experience, I need your advice.
I don't want to spend lots of time making theses suggested changes
to have it rejected for inclusion into the mainline code.  Will 
it be accepted or is it just too far outside of the way U-boot 
evolves?  Is there a better solution?

Thanks for your time on this.

Regards,

Neil.

[U-Boot-Users] U-boot code etiquette

[Wolfgang Denk]

Dear Neil,

in message <1106558355.3698.47.camel@daroca> you wrote:
> 
> The board has a limited EEPROM storage area and it has been decided that
> U-boot environment variables are to be stored in EEPROM.  

Don't do that. Using  an  EEPROM  for  the  environment  has  several
serious   disadvantages   (slow   boot  speed,  risk  of  losing  the
environment). I strongly  recommend  to  use  flash,  porobably  even
redundand flash sectors.

> U-boot uses quite a large amount of environment variable space,

This depends on what you do with it. If you want,  you  can  do  with
just  128  bytes or so. But then a lot of the power of U-Boot results
from clever use of the environment, so you  have  to  decide  if  you
really want to castrate U-Boot.

> in fact the default size for the AM91RM9200DK CFG_ENV_SIZE is 0x2000

The default isto use flash, too, where this is not a problem at all.

> (8kbytes). So all my EEPROM disappears to U-boot leaving non for Linux.

Who says that Linux and U-Boot cannot share the same storage?

> BUT, cetain variables such as the board IP address need to be common 
> to both Linux and U-boot.  So if the board IP address is changed by the
> OS or by Linux, the change needs to be visible to the other.

Right. So share the environment.

> SOLUTION:
> To divide the EEPROM into a psuedo-file structure.  We have data 

What a overkill.

> structures of varying lengths and types to pack more data into the 

KISS: we already have that. We have  name=value  tuples,  encoded  in
plain text with variable length. What else do you need?

> EEPROM.  We need to support a variety of OSes on the board and will 
> have several OS-specific data structures.

Please provide an example that cannot be encoded in the way mentioned
above.

> One data structure would be reserved for U-boot, length 2kBytes,
> 
> However, common values such as IP address will be stored in
> this psuedo-file structure.

Doesn't make sense. Don't duplicate the same data if you can avoid it.

> When U-boot reads the environment variables, it needs to check in two
> places in EEPROM:

Such a modifcation is avoidable.

>  o I don't want to make board-specific changes in common code areas.
>    I can see my proposal affecting cmd_nvedit.c.  This code does
>    not have pre-processing for multiple board-types and so 
>    architecturaly it feels wrong. 

I don't see what  sort  of  board  specific  preprocessing  might  be
needed. Maube you can elucidate.

>  o I need to hook into functions getenv() and setenv().  I will read
>    a table of constant strings and compare with the environment 
>    variable which is being sought and then parse it appropriately.

Don't do that. Don't duplicate function, don't duplicate code,  don't
duplicate data. Use one set of data.

> I don't want to spend lots of time making theses suggested changes
> to have it rejected for inclusion into the mainline code.  Will 
> it be accepted or is it just too far outside of the way U-boot 
> evolves?  Is there a better solution?

If you have the choice, then provide access functions to  the  U-Boot
environment  for  your other components that need to access (read and
write) configuration data. For Linux this is trivial - see  the  code
in tools/env as example. If you really *must* use a different storage
format,  then  use  this  for  everything  and provide a complete new
access method for U-Boot.

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 don't you have a Linux partition installed so you can be  working
in  a  programmer-friendly environment instead of a keep-gates'-bank-
account-happy one? :-)                            -- Tom Christiansen

[U-Boot-Users] U-boot code etiquette

[nbryan at embebidos.com]

Dear Wolfgang,

I thought my suggestions may prove unpopular,
but I did not expect quite such a roasting ;-)

Please read my defence below.

u-boot-users at lists.sourceforge.net

To:             	Neil Bryan <nbryan@embebidos.com>
Copies to:      	u-boot-users at lists.sourceforge.net, pperez at embebidos.com
From:           	Wolfgang Denk <wd@denx.de>
Subject:        	Re: [U-Boot-Users] U-boot code etiquette 
Date sent:      	Mon, 24 Jan 2005 11:12:17 +0100

> Dear Neil,
> 
> in message <1106558355.3698.47.camel@daroca> you wrote:
> > 
> > The board has a limited EEPROM storage area and it has been decided that
> > U-boot environment variables are to be stored in EEPROM.  
> 
> Don't do that. Using  an  EEPROM  for  the  environment  has  several
> serious   disadvantages   (slow   boot  speed,  risk  of  losing  the
> environment). I strongly  recommend  to  use  flash,  porobably  even
> redundand flash sectors.

For some reason, which I do not know, a policy decision has 
been made to use EEPROM for all configuration data.  This 
includes U-Boot environment variables.  Now this is a 
particularly weak argument on my part, but in this context, 
I am just blindly following orders.  However, I can ask  
the question to my policy-makers and make a strong 
argument to them based on your reservations NOT to use 
EEPROM for U-boot environment variables.  This point
shall be followed up.

> 
> > U-boot uses quite a large amount of environment variable space,
> 
> This depends on what you do with it. If you want,  you  can  do  with
> just  128  bytes or so. But then a lot of the power of U-Boot results
> from clever use of the environment, so you  have  to  decide  if  you
> really want to castrate U-Boot.
No, the idea was to leave most of U-Boot's environment space  
untouched, only remove the parts that are common to both U-Boot
and the OS that is to be loaded.  This results in a very small
saving in space, but ties in with the proposed pseudo-file 
structure scheme.
> 
> > in fact the default size for the AM91RM9200DK CFG_ENV_SIZE is 0x2000
> 
> The default isto use flash, too, where this is not a problem at all.
At this point, lets just assume that we are using EEPROM!

> 
> > (8kbytes). So all my EEPROM disappears to U-boot leaving non for Linux.
> 
> Who says that Linux and U-Boot cannot share the same storage?
Good point, but parsing can be a pain with the way that U-Boot 
stores environment variables.  It throws everything into one big 
pile and passes all the work of finding what you need to the driver.
My proposed scheme is to help simplify parsing.

> 
> > BUT, cetain variables such as the board IP address need to be common 
> > to both Linux and U-boot.  So if the board IP address is changed by the
> > OS or by Linux, the change needs to be visible to the other.
> 
> Right. So share the environment.
Yes, if the environment was nice to use.  I get the impression that
as it was designed to use FLASH, U-Boot can be very generous in the
amount of space it reserves for a simple value.  We have a small 
EEPROM and I cannot justify this expense.
> 
> > SOLUTION:
> > To divide the EEPROM into a psuedo-file structure.  We have data 
> 
> What a overkill.
How can you say this with such little knowledge of what is proposed?
I value your opinion as this is why I asked for it, but _I_ find 
it very hard to discount an idea until I at least understand the
detail.  
> 
> > structures of varying lengths and types to pack more data into the 
> 
> KISS: we already have that. We have  name=value  tuples,  encoded  in
> plain text with variable length. What else do you need?
The current U-Boot scheme works very well for the limited number of
variables needed by the environment.  But with other OSes (not Linux)
this number can be very large.  If a single flag (1bit) has to be 
stored as an ascii encoded identifier followed by 'yes' or 'no',
we will soon run out of space.  Using a single byte, I can represent
eight flags.  Of course, it is possible to use U-Boot in this way,
but it adds overhead.  However, you have got me thinking on this
point!
> 
> > EEPROM.  We need to support a variety of OSes on the board and will 
> > have several OS-specific data structures.
> 
> Please provide an example that cannot be encoded in the way mentioned
> above.
The difficulty is not to encode the data, but to encode it 
efficiently and easily accessible by the OS driver that has to use 
it.
> 
> > One data structure would be reserved for U-boot, length 2kBytes,
> > 
> > However, common values such as IP address will be stored in
> > this psuedo-file structure.
> 
> Doesn't make sense. Don't duplicate the same data if you can avoid it.
This is exactly the idea.  To have common data shared between an OS 
and U-Boot, but not using the U-Boot method for storing the common 
data.
> 
> > When U-boot reads the environment variables, it needs to check in two
> > places in EEPROM:
> 
> Such a modifcation is avoidable.
> 
> >  o I don't want to make board-specific changes in common code areas.
> >    I can see my proposal affecting cmd_nvedit.c.  This code does
> >    not have pre-processing for multiple board-types and so 
> >    architecturaly it feels wrong. 
> 
> I don't see what  sort  of  board  specific  preprocessing  might  be
> needed. Maube you can elucidate.
If my board uses a different data structure for storing NV data,
then when U-Boot attempts to read, for example ipaddr, it will be 
routed through a different interface to access the value stored on
the EEPROM.  The value returned to U-Boot will be the same, just
it will not be stored in the long chain of strings as is the
current scheme.  This requires board-specific code to be added
to functions like getenv().  But I don't want to do this as it
breaks the architecture of U-Boot.

> 
> >  o I need to hook into functions getenv() and setenv().  I will read
> >    a table of constant strings and compare with the environment 
> >    variable which is being sought and then parse it appropriately.
> 
> Don't do that. Don't duplicate function, don't duplicate code,  don't
> duplicate data. Use one set of data.
My proposal is to use one set of data.  The intention is to extend 
not duplicate.  The long and the short is that U-Boot stores data in 
a way that is friendly only to U-Boot.  Any other driver that needs
access to this data has an horrendous job of parsing it.

> 
> > I don't want to spend lots of time making theses suggested changes
> > to have it rejected for inclusion into the mainline code.  Will 
> > it be accepted or is it just too far outside of the way U-boot 
> > evolves?  Is there a better solution?
> 
> If you have the choice, then provide access functions to  the  U-Boot
> environment  for  your other components that need to access (read and
> write) configuration data. For Linux this is trivial - see  the  code
> in tools/env as example. If you really *must* use a different storage
> format,  then  use  this  for  everything  and provide a complete new
> access method for U-Boot.
I have to support several other OSes aside from Linux.  I am happy to 
change the EEPROM interface to U-Boot if it makes for an improved 
U-Boot architecture.  I really don't want to hack it, I want to keep
the same interface and only change areas that are specific to my 
board.  The problem is, that the env_x and cmd_x code is in the 
common area.

My final point is, if I make the changes in a board-specific 
directory, would you include it in the U-Boot tree?  If not,
then I shall stop working on this problem now.

Thanks for the discussion.

Regards,

Neil.
> 
> 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 don't you have a Linux partition installed so you can be  working
> in  a  programmer-friendly environment instead of a keep-gates'-bank-
> account-happy one? :-)                            -- Tom Christiansen
> 


************************************************************
 Neil Bryan
 Ingeniero de Desarrollo.
 Sistemas Embebidos, S.A. 
 C/ Calvo Sotelo, 1 1? Dcha, 26003 Logro?o
 Tfno: ++34 941 270060, Fax: ++34 941 237770
************************************************************

[U-Boot-Users] U-boot code etiquette

[Wolfgang Denk]

Dear Neil,

in message <41F4F2D7.31679.675965@localhost> you wrote:
> 
> I thought my suggestions may prove unpopular,
> but I did not expect quite such a roasting ;-)

This was not intended as "roasting" - sorry if it sounded as such.  I
just wanted to prevent you from running down a dead end road.

> Please read my defence below.

No need to defend yourself. We're just discussion technical options.

> > Don't do that. Using  an  EEPROM  for  the  environment  has  several
> > serious   disadvantages   (slow   boot  speed,  risk  of  losing  the
> > environment). I strongly  recommend  to  use  flash,  porobably  even
> > redundand flash sectors.
> 
> For some reason, which I do not know, a policy decision has 
> been made to use EEPROM for all configuration data.  This 

Tell them that others did that before, and failed. Depending on  your
hardware,  you  may lose all data in EEPROM, even if everything works
fine initially. See doc/I2C_Edge_Conditions - to give  you  just  one
example  of  failure modes: assume for example that your power supply
starts to age in two years from now,  and  that  causes  the  voltage
rises more slowly. Then your CPU may start running, but will be reset
by  the  system  monitor, eventually several times until the voltage.
Chances are, that one of these resets will  catch  the  system  right
after  the  processor  started  reading the environment. Chances are,
that this will write random data into the EEPROM.  --  This  is  more
than  theory.  One of our customers had a system that lost the EEPROM
contents in 2 out of 3 power cycles.

> includes U-Boot environment variables.  Now this is a 
> particularly weak argument on my part, but in this context, 
> I am just blindly following orders.  However, I can ask  

Never do that. NEVER.

Always keep your eyes open and the brain online. It is part  of  your
responsibility  as  an engineer to prevent managers from doing stupid
things.

> the question to my policy-makers and make a strong 
> argument to them based on your reservations NOT to use 
> EEPROM for U-boot environment variables.  This point
> shall be followed up.

[but probably not here on the list - unless it is of general interest]

> > Who says that Linux and U-Boot cannot share the same storage?
> Good point, but parsing can be a pain with the way that U-Boot 
> stores environment variables.  It throws everything into one big 

You must be joking. The code  do  parse  the  U-Boot  environment  is
around  20  lines  of C code, including inti calls and error handling
and everything.

> pile and passes all the work of finding what you need to the driver.
> My proposed scheme is to help simplify parsing.

Sorry, but I realy don;t now what you mean.  Reading  or  writing  an
U-Boot  environment  variable  is  a  very simple and straightforward
procedure. It may be non-optimal in terms of speed  (linear  search),
but in terms of simplicity you will have problems to beat it.

> > Right. So share the environment.
> Yes, if the environment was nice to use.  I get the impression that
> as it was designed to use FLASH, U-Boot can be very generous in the
> amount of space it reserves for a simple value.  We have a small 
> EEPROM and I cannot justify this expense.

What exactly is your problem? That somebody might define long  values
for variables? You don't have to do that.

> > > To divide the EEPROM into a psuedo-file structure.  We have data 
> > 
> > What a overkill.
> How can you say this with such little knowledge of what is proposed?

The "name=value" list method  used  for  the  U-Boot  environment  is
pretty  efficient.  If  you  don't  fall  back  to  hard-coded binary
structures which are a major PITA to maintain then you will have hard
times to find a simpler solution.

> The current U-Boot scheme works very well for the limited number of
> variables needed by the environment.  But with other OSes (not Linux)
> this number can be very large.  If a single flag (1bit) has to be 
> stored as an ascii encoded identifier followed by 'yes' or 'no',
> we will soon run out of space.  Using a single byte, I can represent

Then don't do it. Who prevents you from defining "flags=deadbeef"? 15
bytes of storage for 4 byte of data or 32 bits of data. Or use a long
long with a single character name - 19 bytes of storage for 8 byte or
64 bits.

> > Please provide an example that cannot be encoded in the way mentioned
> > above.
> The difficulty is not to encode the data, but to encode it 
> efficiently and easily accessible by the OS driver that has to use 
> it.

Don't forget the benefits from  being  able  to  read  your  settings
without  special  tools. With the current U-Boot method, you can even
read the environment in flah when all you have available  is  a  JTAG
debugger.

> This is exactly the idea.  To have common data shared between an OS 
> and U-Boot, but not using the U-Boot method for storing the common 
> data.

> My proposal is to use one set of data.  The intention is to extend 
> not duplicate.  The long and the short is that U-Boot stores data in 
> a way that is friendly only to U-Boot.  Any other driver that needs
> access to this data has an horrendous job of parsing it.

C'me on. Did you really read the code of the getenv() function?

> I have to support several other OSes aside from Linux.  I am happy to 

This is OK - but try to implement a method which is common to all  of
then, and to U-Boot.

> board.  The problem is, that the env_x and cmd_x code is in the 
> common area.

Thisi s not a problem, it is  a  good  thing,  preventing  people  to
reinvent the wheel :-)

> My final point is, if I make the changes in a board-specific 
> directory, would you include it in the U-Boot tree?  If not,
> then I shall stop working on this problem now.

I have  to  repeat  myself:  I  don't  think  that  there  should  be
board-specific  code for this. Either provide access functions to the
U-Boot environment for your other OSes that need to access (read  and
write)  configuration  data,  or  then  use  a  new  format  to store
"environment variables"  for  all  parties  involved  and  provide  a
complete new access method for U-Boot (which then goes into common/).

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
If a man had a child who'd gone  anti-social,  killed  perhaps,  he'd
still tend to protect that child.
	-- McCoy, "The Ultimate Computer", stardate 4731.3