[U-Boot-Users] Proposed change; What do you think?

[U-Boot-Users] Proposed change; What do you think?

[Jon Loeliger]

Guys,

I'd like to get your opinion on a proposed change to a
few files that handle some aspects of the various enetaddr
fields as found in asm-ppc/u-boot.h, common/cmd_bdinfo.c
and lib_ppc/board.c.

In particular, I'd like to propose a shift from having these
fields be present when certain boards are #defined to having
these fields be present when CONFIG_ETH1ADDR symbols are defined.

So, don't take the following as a literal patch.  It is just
some cut-n-paste diffs to show what I mean.

There are only a few boards for which this might make a difference.
I'm the bit swizzler for most of them (MPC85xxXDS), but the others
include the PN62, the PPCCHAMEONEVB, the 440GX, the GT_6426x.
SXNI855T, SVM_SC8xx and possibly the DB64360, DB64460, CATcenter,
and OCOTEA boards.  The latter define CONFIG_ETH1ADDR but don't
have their cases include in places like the coomon/cmd_bdinfo.c.

I think this sorr of change would make the handling of the
ETH1ADDR, ETH2ADDR and ETH3ADDR more consistent across any
platform that defines these fields.

Opinions?  If you buy this, I'll submit a proper patch, of course.

Thanks,
jdl




--- include/asm-ppc/u-boot.h    2004/04/13 21:42:46     1.1.1.2.2.1
+++ include/asm-ppc/u-boot.h    2004/08/06 10:51:41
@@ -77,25 +77,15 @@
 #if defined(CONFIG_HYMOD)
        hymod_conf_t    bi_hymod_conf;  /* hymod configuration
information */
 #endif
-#if defined(CFG_GT_6426x)              || \
-    defined(CONFIG_PN62)               || \
-    defined(CONFIG_PPCHAMELEONEVB)     || \
-    defined(CONFIG_SXNI855T)           || \
-    defined(CONFIG_SVM_SC8xx)          || \
-    defined(CONFIG_MPC8540ADS)          || \
-    defined(CONFIG_MPC8555CDS)         || \
-    defined(CONFIG_MPC8560ADS)         || \
-    defined(CONFIG_440_GX)
+#ifdef CONFIG_ETH1ADDR
        /* second onboard ethernet port */
        unsigned char   bi_enet1addr[6];
 #endif
-#if defined(CFG_GT_6426x) || defined(CONFIG_SVM_SC8xx) || \
-    defined(CONFIG_MPC8540ADS) || defined(CONFIG_MPC8560ADS) || \
-    defined(CONFIG_MPC8555CDS) || defined(CONFIG_440_GX)
+#ifdef CONFIG_ETH2ADDR
        /* third onboard ethernet port */
        unsigned char   bi_enet2addr[6];
 #endif
-#if defined(CONFIG_440_GX)
+#ifdef CONFIG_ETH3ADDR
        unsigned char   bi_enet3addr[6];
 #endif
 #if defined(CONFIG_405GP) || defined(CONFIG_405EP) || defined
(CONFIG_440_GX)


and also:


diff -u -r1.1.1.2.2.2 cmd_bdinfo.c
--- common/cmd_bdinfo.c 2004/06/17 20:37:25     1.1.1.2.2.2
+++ common/cmd_bdinfo.c 2004/08/06 10:51:40
@@ -83,16 +83,13 @@
        for (i=0; i<6; ++i) {
                printf ("%c%02X", i ? ':' : ' ', bd->bi_enetaddr[i]);
        }
-#if (defined CONFIG_PN62) || (defined CONFIG_PPCHAMELEONEVB) \
-    || (defined CONFIG_MPC8540ADS) || (defined CONFIG_MPC8560ADS) \
-    || (defined CONFIG_MPC8555CDS)
+#ifdef CONFIG_ETH1ADDR
        puts ("\neth1addr    =");
        for (i=0; i<6; ++i) {
                printf ("%c%02X", i ? ':' : ' ', bd->bi_enet1addr[i]);
        }
-#endif /* CONFIG_PN62 */
-#if defined(CONFIG_MPC8540ADS) || defined(CONFIG_MPC8560ADS) \
-   || defined(CONFIG_MPC8555CDS)
+#endif /* CONFIG_ETH1ADDR */
+#ifdef CONFIG_ETH2ADDR 
        puts ("\neth2addr    =");
        for (i=0; i<6; ++i) {
                printf ("%c%02X", i ? ':' : ' ', bd->bi_enet2addr[i])


And one other place over in:


diff -u -r1.1.1.2.2.4 board.c
--- lib_ppc/board.c     2004/06/17 20:37:41     1.1.1.2.2.4
+++ lib_ppc/board.c     2004/08/06 10:51:41

@@ -772,11 +774,7 @@
        load_sernum_ethaddr ();
 #endif
 
-#if defined(CFG_GT_6426x) || defined(CONFIG_PN62) ||
defined(CONFIG_PPCHAMELEONEVB) || \
-    defined(CONFIG_MPC8540ADS) || \
-    defined(CONFIG_MPC8555CDS) || \
-    defined(CONFIG_MPC8560ADS) || \
-    defined(CONFIG_440_GX)
+#ifdef CONFIG_ETH1ADDR
        /* handle the 2nd ethernet address */
 
        s = getenv ("eth1addr");
@@ -787,11 +785,7 @@
                        s = (*e) ? e + 1 : e;
        }
 #endif
-#if defined(CFG_GT_6426x) || \
-    defined(CONFIG_MPC8540ADS) || \
-    defined(CONFIG_MPC8555CDS) || \
-    defined(CONFIG_MPC8560ADS) || \
-    defined(CONFIG_440_GX)
+#ifdef CONFIG_ETH2ADDR
        /* handle the 3rd ethernet address */
 
        s = getenv ("eth2addr");
@@ -807,7 +801,7 @@
        }
 #endif
 
-#if defined(CONFIG_440_GX)
+#ifdef CONFIG_ETH3ADDR
        /* handle 4th ethernet address */
        s = getenv("eth3addr");
 #if defined(CONFIG_XPEDITE1K)

[U-Boot-Users] Proposed change; What do you think?

[Dan Malek]

On Aug 18, 2004, at 6:08 PM, Jon Loeliger wrote:

> In particular, I'd like to propose a shift from having these
> fields be present when certain boards are #defined to having
> these fields be present when CONFIG_ETH1ADDR symbols are defined.

I prefer we don't have #defines that change the size of a bd_t.
All board descriptors should be the same size, not all boards
use all of the fields, but that's OK.

This way, we have a better chance of using common Linux
binaries when that is useful.  Having different versions of
a kernel just because the bd_t is a different size doesn't
seem like a useful configuration.  If you want variability
in the parameters, let's use the bi_rec interface for passing
such information.

Thanks.

	-- Dan

[U-Boot-Users] Proposed change; What do you think?

[Jon Loeliger]

On Thu, 2004-08-19 at 13:35, Dan Malek wrote:
> On Aug 18, 2004, at 6:08 PM, Jon Loeliger wrote:
> 
> > In particular, I'd like to propose a shift from having these
> > fields be present when certain boards are #defined to having
> > these fields be present when CONFIG_ETH1ADDR symbols are defined.
> 
> I prefer we don't have #defines that change the size of a bd_t.
> All board descriptors should be the same size, not all boards
> use all of the fields, but that's OK.
> 
> This way, we have a better chance of using common Linux
> binaries when that is useful.  Having different versions of
> a kernel just because the bd_t is a different size doesn't
> seem like a useful configuration.  If you want variability
> in the parameters, let's use the bi_rec interface for passing
> such information.
> 
> Thanks.
> 
> 	-- Dan

So, I have no real problem with this approach either.

My concern was in trying to move from board-driven-features
to feature-driven-features. :-)

Constructing a "constant bd_t" is fine by me too, but that
might be construed as a Different Battle.  At this stage,
I was opting for not disturbing too much of the balance
and leaving things essentially as-they-were.

BTW, off-list, I've received several "thumbs up" indications
from a few different board maintainers here, and no negative
indicators.

Thanks,
jdl

[U-Boot-Users] Proposed change; What do you think?

[Dan Malek]

On Aug 19, 2004, at 2:57 PM, Jon Loeliger wrote:

> Constructing a "constant bd_t" is fine by me too, but that
> might be construed as a Different Battle.

I understand.  Just documenting my opinion :-)

> BTW, off-list, I've received several "thumbs up" indications
> from a few different board maintainers here, and no negative
> indicators.

It would be nice if people weren't so damned shy about
speaking up.  :-)  That's what these discussion lists are supposed
to be about, discussing.  I hate the opposite as well, when on
the lists we get consensus to do something, then a bunch of
negative e-mails get sent personally.

Thanks.


	-- Dan

[U-Boot-Users] Proposed change; What do you think?

[Wolfgang Denk]

In message <90EAE755-F20E-11D8-8765-003065F9B7DC@embeddededge.com> you wrote:
> 
> seem like a useful configuration.  If you want variability
> in the parameters, let's use the bi_rec interface for passing
> such information.

Has there been any agreement about  such  an  interface?  I  remember
several  rounds  of  discussions, but heard nothing about a generally
accepted solution. Am I missing something?

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
"How is this place run - is it an anarchy?"
"No, I wouldn't say so; it is not that well organised..."

[U-Boot-Users] Proposed change; What do you think?

[Wolfgang Denk]

Dear Jon,

in message <1092866935.31694.16.camel@blarg.somerset.sps.mot.com> you wrote:
> 
> I'd like to get your opinion on a proposed change to a
> few files that handle some aspects of the various enetaddr
> fields as found in asm-ppc/u-boot.h, common/cmd_bdinfo.c
> and lib_ppc/board.c.
> 
> In particular, I'd like to propose a shift from having these
> fields be present when certain boards are #defined to having
> these fields be present when CONFIG_ETH1ADDR symbols are defined.

I like your patch, and there was no protest on the  list.  You  wrote
"don't  take the following as a literal patch", so I don;t take it as
such.

Please feel free to submit the real patch whenever you like.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
The people of Gideon have always believed that life is  sacred.  That
the  love  of  life  is  the  greatest  gift  ... We are incapable of
destroying or interfering with the creation of that which we love  so
deeply -- life in every form from fetus to developed being.
	-- Hodin of Gideon, "The Mark of Gideon", stardate 5423.4

[U-Boot-Users] Proposed change; What do you think?

[Dave Ellis]

Jon Loeliger wrote:
> I'd like to get your opinion on a proposed change to a
> few files that handle some aspects of the various enetaddr
> fields as found in asm-ppc/u-boot.h, common/cmd_bdinfo.c
> and lib_ppc/board.c.
> 
> In particular, I'd like to propose a shift from having these
> fields be present when certain boards are #defined to having
> these fields be present when CONFIG_ETH1ADDR symbols are defined.

I like the idea of using a common symbol, but I would prefer a new
one, something like CONFIG_HAS_ETH1, so I can have bi_enet1addr in the
kernel interface without putting a default value for it in the
environment.

> There are only a few boards for which this might make a difference.
> I'm the bit swizzler for most of them (MPC85xxXDS), but the others
> include the PN62, the PPCCHAMEONEVB, the 440GX, the GT_6426x.
> SXNI855T, SVM_SC8xx and possibly the DB64360, DB64460, CATcenter,
> and OCOTEA boards.  The latter define CONFIG_ETH1ADDR but don't
> have their cases include in places like the coomon/cmd_bdinfo.c.

As the SXNI855T maintainer, I don't see any problems with using
CONFIG_ETH1ADDR or CONFIG_HAS_ETH1.

Dave Ellis

[U-Boot-Users] Proposed change; What do you think?

[Jon Loeliger]

On Thu, 2004-08-19 at 09:40, Dave Ellis wrote:
> Jon Loeliger wrote:
> > I'd like to get your opinion on a proposed change to a
> > few files that handle some aspects of the various enetaddr
> > fields as found in asm-ppc/u-boot.h, common/cmd_bdinfo.c
> > and lib_ppc/board.c.
> > 
> > In particular, I'd like to propose a shift from having these
> > fields be present when certain boards are #defined to having
> > these fields be present when CONFIG_ETH1ADDR symbols are defined.
> 
> I like the idea of using a common symbol, but I would prefer a new
> one, something like CONFIG_HAS_ETH1, so I can have bi_enet1addr in the
> kernel interface without putting a default value for it in the
> environment.

BTW, I am willing to make the change so that the code uses
the symbols:

    CONFIG_HAS_ETH
    CONFIG_HAS_ETH1
    CONFIG_HAS_ETH2
and
    CONFIG_HAS_ETH3

as suggested.  I like it.

However, now I need answers to the following question:  Do you want
me to retrofit code into all the Config files to #define CONFIG_HAS_ETHx
where it currently also has CONFIG_ETHxADDR defined, or where the code
has a board name even though a CONIG_ETHxADDR is not defined too?

Happy to do this, just realize that to be backwards compatible
with existing config files, I'll have to change many config files.
I can not test them all.  I can test the 4 I have in front of me.


FYI, I am also willing to remove the #ifdef conditionality from
the bd_t structure around these ETH addr fields as well, but with
the caveat that it changes other people's bd_t structures and
potentially messes up their Linux interfaces.  Again, I can't
test all that either...

Thanks,
jdl

[U-Boot-Users] Proposed change; What do you think?

[Wolfgang Denk]

In message <1092945083.8297.24.camel@blarg.somerset.sps.mot.com> you wrote:
>
> However, now I need answers to the following question:  Do you want
> me to retrofit code into all the Config files to #define CONFIG_HAS_ETHx
> where it currently also has CONFIG_ETHxADDR defined, or where the code
> has a board name even though a CONIG_ETHxADDR is not defined too?

Do you want to have your patch accepted?

> Happy to do this, just realize that to be backwards compatible
> with existing config files, I'll have to change many config files.
> I can not test them all.  I can test the 4 I have in front of me.

Please keep all files in a konsistent state.

> FYI, I am also willing to remove the #ifdef conditionality from
> the bd_t structure around these ETH addr fields as well, but with
> the caveat that it changes other people's bd_t structures and
> potentially messes up their Linux interfaces.  Again, I can't
> test all that either...

Don't put to many different things into a  single  patch.  This  last
part  has  a chance of being rejected (depending on what you're going
to do; I'm not sure I understand your intentions).

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
We fight only when there is no other choice. We prefer  the  ways  of
peaceful contact.
	-- Kirk, "Spectre of the Gun", stardate 4385.3

[U-Boot-Users] Proposed change; What do you think?

[Jon Loeliger]

On Sun, 2004-08-22 at 18:38, Wolfgang Denk wrote:
> In message <1092945083.8297.24.camel@blarg.somerset.sps.mot.com> you wrote:
> >
> > However, now I need answers to the following question:  Do you want
> > me to retrofit code into all the Config files to #define CONFIG_HAS_ETHx
> > where it currently also has CONFIG_ETHxADDR defined, or where the code
> > has a board name even though a CONIG_ETHxADDR is not defined too?
> 
> Do you want to have your patch accepted?
> 
> > Happy to do this, just realize that to be backwards compatible
> > with existing config files, I'll have to change many config files.
> > I can not test them all.  I can test the 4 I have in front of me.
> 
> Please keep all files in a konsistent state.

Oh, I'm happy to keep it in a consistent state.  That's not the
question I was trying to get answered.  I'm seeking acceptance of
the condition that I can not test all the affected boards even
though the patch would necessarily touch many boards.  Compile, sure.
Test, no way.

> > FYI, I am also willing to remove the #ifdef conditionality from
> > the bd_t structure around these ETH addr fields as well, but with
> > the caveat that it changes other people's bd_t structures and
> > potentially messes up their Linux interfaces.  Again, I can't
> > test all that either...
> 
> Don't put to many different things into a  single  patch.  This  last
> part  has  a chance of being rejected (depending on what you're going
> to do; I'm not sure I understand your intentions).

I never said it would all be one patch.

Furthermore, I was merely suggesting I'd be willing to work on
the suggestion that Dan Malek had proposed.  ACtually, just one
aspect of it, specifically WRT the ethernet MAC address fields.

As I have it in my tree now, it has been left conditional, just
the names of the #ifdef conditional has changed.

jdl

[U-Boot-Users] Proposed change; What do you think?

[Wolfgang Denk]

In message <1093270471.29614.7.camel@blarg.somerset.sps.mot.com> you wrote:
>
> Oh, I'm happy to keep it in a consistent state.  That's not the
> question I was trying to get answered.  I'm seeking acceptance of
> the condition that I can not test all the affected boards even
> though the patch would necessarily touch many boards.  Compile, sure.
> Test, no way.

Submit a patch and hope that the board maintainers will test  it.  If
they  don't  test  and/or  don't  find any problems, there will be no
complaints, and the patch will get added one day to the CVS tree.

> > Don't put to many different things into a  single  patch.  This  last
> > part  has  a chance of being rejected (depending on what you're going
> > to do; I'm not sure I understand your intentions).
> 
> I never said it would all be one patch.

Thanks. Maybe I misunderstood.

> Furthermore, I was merely suggesting I'd be willing to work on
> the suggestion that Dan Malek had proposed.  ACtually, just one
> aspect of it, specifically WRT the ethernet MAC address fields.

I think you misunderstood this. We already have  different  bd_info's
for  different  processors, i. e. it looks different on a 8xx than it
does on a 4xx. But within one  class  of  processors  iit  should  be
fixed. Should ;-)

> As I have it in my tree now, it has been left conditional, just
> the names of the #ifdef conditional has changed.

Thanks.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
"In matrimony, to hesitate is sometimes to be saved."        - Butler