[U-Boot] [PATCH 01/27 v2] Blackfin: bfin_mac: force board_get_enetaddr() usage

[Mike Frysinger <vapier@gentoo.org>]

On Tuesday 03 February 2009 03:16:27 Wolfgang Denk wrote:
> Dear Mike Frysinger,
>
> In message <200902021937.26246.vapier@gentoo.org> you wrote:
> > > Please change:
> > >
> > > 	If the hardware design mandates that the MAC address is stored
> > > 	in some special place, like EEPROM etc., then the board
> > > 	specific init code (like the board-specific misc_init_r()
> > > 	function) is responsible for locating the MAC address(es) and
> > > 	initialize the respective environment variable(s) from it.
> > >
> > >         Note that this shall be done if, and only if, the environment
> > >         does not already contain these environment variables, i. e.
> > >         existing variable definitions must not be overwritten.
> > >
> > > 	The envrionment handling code (function setevn()) will update
> > > 	the global data accordingly.
> >
> > it will update the global data, but nowhere will it initially seed it. 
> > i'm thinking env_init() should be a unified entry point that first calls
> > down to the specific env storage (eeprom/flash/nand/etc...) and then
> > initializes the relevant bi_enetaddr members of the global data
> > structure.
>
> No, that would mix functions in a bad way as it creates new
> dependencies on what can or must be done when. env_init() in sone
> thing (initializes the environment data), but global data init is
> another issue.
>
> Maybe we should try and collect the global data initialization into
> one place - but I have to admit that I don't know if or how easily
> this can be done.

trying to do it in one go will probably be ugly and break crap, so i think 
starting with a few pieces and expanding from there is fine.  in this case, 
we'll start with updating all the lib_*/board.c files to call global_init() 
and that will in turn start with setting up bi_enetaddr.

> > > Please change:
> > >
> > > 	During runtime, the ethernet layer will use the global data
> > > 	to sync ...
> >
> > documenting it this way wont change the code ;).  the ethernet code does
> > not use the global data in any way.  look at eth_initialize() in
> > net/eth.c.  i imagine part of the reason for this is that working with
> > multiple ethernet devices is pretty ugly atm.  the static list of
> > CONFIG_HAS_ETH{1,2,3,...} makes working with more than one device very
> > messy.  the ethernet code today though builds the string dynamically
> > "eth%iaddr" and so can handle arbitrary number of devices without needing
> > any changes.
>
> Well, I think we agree that the current state is a mess.  Documenting
> the mess makes it easier to add to it. But should we not try to clean
> up  instead?  I.e. document what we think should be done, and fix the
> implementation accordingly?

yes, but having the documentation say one thing and the actual implementation 
do something completely different leads to confusion.  people dont know 
whether the documentation is out of date, plain wrong, or what.  to both ends, 
we can have the documentation read what *should* be happening and then add a 
note about what *actually* is happening and how it needs to be changed.

> > this also applies to the cascading of setenv() into the global data. 
> > that code only handles bi_enetaddr and none of the bi_enetNaddr ... doing
> > 'set eth1addr ..." will not update bi_enet1addr ...
>
> Agreed - that needs to be fixed, too.

so does it make sense for the env code to even be calling eth_set_enetaddr() ?  
after all, any network operation will call the eth init code which will take 
care of rereading the mac address from the env/global data.

also, are you saying that the current method of a static # of ethernet devices 
is the way to go ?  we'll have to maintain a hardcoded list of places like:
#ifdef CONFIG_HAS_ETH1
	... work with bi_enet1addr ...
#endif
#ifdef CONFIG_HAS_ETH2
	... do same thing but now with bi_enet2addr ...
#endif
#ifdef CONFIG_HAS_ETH3
	... do same thing but now with bi_enet3addr ...
#endif
#ifdef CONFIG_HAS_ETH4
	... do same thing but now with bi_enet4addr ...
#endif

i think we can agree that this does not scale at all.  if, however, we change 
the global data to something like:
typedef struct bd_info {
	...
#ifdef CONFIG_NET_MULTI
	uchar bi_enetXaddr[CONFIG_NET_MULTI_MAX][6];
# define bi_enetaddr bi_enetXaddr[0]
#else
	uchar bi_enetaddr[6];
#endif
	...
};
then we should be able to scale nicely ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090203/da737fe8/attachment-0001.pgp