| Message ID | 1336776649-9316-5-git-send-email-michael@walle.cc |
|---|---|
| State | Superseded |
| Delegated to: | Prafulla Wadaskar |
| Headers | show |
Am Samstag 12 Mai 2012, 00:50:48 schrieb Michael Walle: > Signed-off-by: Michael Walle <michael@walle.cc> > Cc: Joe Hershberger <joe.hershberger@gmail.com> > --- > include/net.h | 16 ++++++++++++++++ > net/eth.c | 15 +++++++++++++++ > 2 files changed, 31 insertions(+), 0 deletions(-) > > diff --git a/include/net.h b/include/net.h > index f6aeba2..bdc3da6 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int > index); /* get dev @ index */ extern int eth_get_dev_index (void); /* get > the device index */ > extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr); > extern int eth_getenv_enetaddr(char *name, uchar *enetaddr); > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX > extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); > +#endif sorry, wrong function, will be corrected in next version. -- michael
Hi Michael, On Fri, May 11, 2012 at 5:50 PM, Michael Walle <michael@walle.cc> wrote: > Signed-off-by: Michael Walle <michael@walle.cc> > Cc: Joe Hershberger <joe.hershberger@gmail.com> > --- > include/net.h | 16 ++++++++++++++++ > net/eth.c | 15 +++++++++++++++ > 2 files changed, 31 insertions(+), 0 deletions(-) > > diff --git a/include/net.h b/include/net.h > index f6aeba2..bdc3da6 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */ > extern int eth_get_dev_index (void); /* get the device index */ > extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr); > extern int eth_getenv_enetaddr(char *name, uchar *enetaddr); > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX > extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); > +#endif Which other function did you intend? You already guarded the only definition whose implementation is guarded. > /* > * Get the hardware address for an ethernet interface . > @@ -118,6 +120,20 @@ extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); > extern int eth_getenv_enetaddr_by_index(const char *base_name, int index, > uchar *enetaddr); > > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX Personally I don't like config options for something so tiny. If you really must guard against including it, use the CONFIG_RANDOM_MACADDR instead. I'd prefer to just let the linker exclude it when it's never referenced. > +/* > + * Set the hardware address for an ethernet interface . > + * Args: > + * base_name - base name for device (normally "eth") > + * index - device index number (0 for first) > + * enetaddr - returns 6 byte hardware address > + * Returns: > + * 0 on success, else 1. > + */ > +extern int eth_setenv_enetaddr_by_index(const char *base_name, int index, > + const uchar *enetaddr); > +#endif > + > #ifdef CONFIG_RANDOM_MACADDR > /* > * The u-boot policy does not allow hardcoded ethernet addresses. Under the > diff --git a/net/eth.c b/net/eth.c > index afce863..d66e22a 100644 > --- a/net/eth.c > +++ b/net/eth.c > @@ -67,6 +67,21 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, > return eth_getenv_enetaddr(enetvar, enetaddr); > } > > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX > +int eth_setenv_enetaddr_by_index(const char *base_name, int index, > + const uchar *enetaddr) > +{ > + char enetvar[32]; > + > + if (index) > + sprintf(enetvar, "%s%daddr", base_name, index); > + else > + sprintf(enetvar, "%saddr", base_name); > + > + return eth_setenv_enetaddr(enetvar, enetaddr); > +} > +#endif You should synchronize with Rob Herring who is also trying to add this eth_setenv_enetaddr_by_index() function. http://patchwork.ozlabs.org/patch/152590/ Also since Rob's patch would use this function all the time, the CONFIG_SETENV_ENETADDR_BY_INDEX would be removed, right? Thanks, -Joe
Hi Joe, Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger: > Hi Michael, > > On Fri, May 11, 2012 at 5:50 PM, Michael Walle <michael@walle.cc> wrote: > > Signed-off-by: Michael Walle <michael@walle.cc> > > Cc: Joe Hershberger <joe.hershberger@gmail.com> > > --- > > include/net.h | 16 ++++++++++++++++ > > net/eth.c | 15 +++++++++++++++ > > 2 files changed, 31 insertions(+), 0 deletions(-) > > > > diff --git a/include/net.h b/include/net.h > > index f6aeba2..bdc3da6 100644 > > --- a/include/net.h > > +++ b/include/net.h > > @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int > > index); /* get dev @ index */ extern int eth_get_dev_index (void); > > /* get the device index */ extern void eth_parse_enetaddr(const char > > *addr, uchar *enetaddr); extern int eth_getenv_enetaddr(char *name, > > uchar *enetaddr); > > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX > > extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); > > +#endif > > Which other function did you intend? You already guarded the only > definition whose implementation is guarded. That was a mistake. Will be fixed in the next version. > > /* > > * Get the hardware address for an ethernet interface . > > @@ -118,6 +120,20 @@ extern int eth_setenv_enetaddr(char *name, const > > uchar *enetaddr); extern int eth_getenv_enetaddr_by_index(const char > > *base_name, int index, uchar *enetaddr); > > > > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX > > Personally I don't like config options for something so tiny. If you > really must guard against including it, use the CONFIG_RANDOM_MACADDR > instead. Me, too. But Wolfgang doesn't accept patches which increases the code size for other boards. I don't think it is a good idea to use CONFIG_RANDOM_MACADDR for such a generic function, which will (hopefully) be used by others too. > I'd prefer to just let the linker exclude it when it's never > referenced. Correct, me if i'm wrong, but the linker can only discard unused sections. But there was some discussion on -ffunction-sections and -fdata-sections on the ML. So maybe theres some light at the end of the tunnel ;) > > > +/* > > + * Set the hardware address for an ethernet interface . > > + * Args: > > + * base_name - base name for device (normally "eth") > > + * index - device index number (0 for first) > > + * enetaddr - returns 6 byte hardware address > > + * Returns: > > + * 0 on success, else 1. > > + */ > > +extern int eth_setenv_enetaddr_by_index(const char *base_name, int > > index, + const uchar *enetaddr); > > +#endif > > + > > #ifdef CONFIG_RANDOM_MACADDR > > /* > > * The u-boot policy does not allow hardcoded ethernet addresses. Under > > the diff --git a/net/eth.c b/net/eth.c > > index afce863..d66e22a 100644 > > --- a/net/eth.c > > +++ b/net/eth.c > > @@ -67,6 +67,21 @@ int eth_getenv_enetaddr_by_index(const char > > *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); > > } > > > > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX > > +int eth_setenv_enetaddr_by_index(const char *base_name, int index, > > + const uchar *enetaddr) > > +{ > > + char enetvar[32]; > > + > > + if (index) > > + sprintf(enetvar, "%s%daddr", base_name, index); > > + else > > + sprintf(enetvar, "%saddr", base_name); > > + > > + return eth_setenv_enetaddr(enetvar, enetaddr); > > +} > > +#endif > > You should synchronize with Rob Herring who is also trying to add this > eth_setenv_enetaddr_by_index() function. > http://patchwork.ozlabs.org/patch/152590/ ah thanks for the hint. > > Also since Rob's patch would use this function all the time, the > CONFIG_SETENV_ENETADDR_BY_INDEX would be removed, right? right, but see above.
Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger: > You should synchronize with Rob Herring who is also trying to add this > eth_setenv_enetaddr_by_index() function. > http://patchwork.ozlabs.org/patch/152590/ Hi Rob, Joe mentioned you have the same patch as me here: http://patchwork.ozlabs.org/patch/158624/ What is the current status of your patch? Wolfgang had some comments on my patch: http://patchwork.ozlabs.org/patch/158354/ Could you incorporate these changes into your patch? Unfortunately, i don't see a way to add the helper function without meeting Wolfgangs requirement not to increase the binary file for other boards. Unless gc-sections and function-sections are used on ARM, or adding an CONFIG_XX macro to mask the helper on compile time.
On 05/22/2012 04:18 PM, Michael Walle wrote: > Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger: >> You should synchronize with Rob Herring who is also trying to add this >> eth_setenv_enetaddr_by_index() function. >> http://patchwork.ozlabs.org/patch/152590/ > > Hi Rob, > > Joe mentioned you have the same patch as me here: > http://patchwork.ozlabs.org/patch/158624/ > > What is the current status of your patch? I have no idea. It's been in the patch system for a month, gotten no comments, and delegated to jhersh. > Wolfgang had some comments on my patch: > http://patchwork.ozlabs.org/patch/158354/ > > Could you incorporate these changes into your patch? > > Unfortunately, i don't see a way to add the helper function without meeting > Wolfgangs requirement not to increase the binary file for other boards. Unless > gc-sections and function-sections are used on ARM, or adding an CONFIG_XX > macro to mask the helper on compile time. There was a patch series for gc-sections recently... I don't see a way to meet this impossible requirement either, and I don't really have the bandwidth right now. Rob
Am Dienstag 22 Mai 2012, 23:41:55 schrieb Rob Herring: > On 05/22/2012 04:18 PM, Michael Walle wrote: > > Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger: > >> You should synchronize with Rob Herring who is also trying to add this > >> eth_setenv_enetaddr_by_index() function. > >> http://patchwork.ozlabs.org/patch/152590/ > > > > Hi Rob, > > > > Joe mentioned you have the same patch as me here: > > http://patchwork.ozlabs.org/patch/158624/ > > > > What is the current status of your patch? > > I have no idea. It's been in the patch system for a month, gotten no > comments, and delegated to jhersh. > > > Wolfgang had some comments on my patch: > > http://patchwork.ozlabs.org/patch/158354/ > > > > Could you incorporate these changes into your patch? > > > > Unfortunately, i don't see a way to add the helper function without > > meeting Wolfgangs requirement not to increase the binary file for other > > boards. Unless gc-sections and function-sections are used on ARM, or > > adding an CONFIG_XX macro to mask the helper on compile time. > > There was a patch series for gc-sections recently... yeah saw that too, but someone mentioned there were problems on arm, iirc. > I don't see a way to meet this impossible requirement either, and I > don't really have the bandwidth right now. ok, thanks for the clarification :)
diff --git a/include/net.h b/include/net.h index f6aeba2..bdc3da6 100644 --- a/include/net.h +++ b/include/net.h @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */ extern int eth_get_dev_index (void); /* get the device index */ extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr); extern int eth_getenv_enetaddr(char *name, uchar *enetaddr); +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); +#endif /* * Get the hardware address for an ethernet interface . @@ -118,6 +120,20 @@ extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); extern int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr); +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX +/* + * Set the hardware address for an ethernet interface . + * Args: + * base_name - base name for device (normally "eth") + * index - device index number (0 for first) + * enetaddr - returns 6 byte hardware address + * Returns: + * 0 on success, else 1. + */ +extern int eth_setenv_enetaddr_by_index(const char *base_name, int index, + const uchar *enetaddr); +#endif + #ifdef CONFIG_RANDOM_MACADDR /* * The u-boot policy does not allow hardcoded ethernet addresses. Under the diff --git a/net/eth.c b/net/eth.c index afce863..d66e22a 100644 --- a/net/eth.c +++ b/net/eth.c @@ -67,6 +67,21 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); } +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX +int eth_setenv_enetaddr_by_index(const char *base_name, int index, + const uchar *enetaddr) +{ + char enetvar[32]; + + if (index) + sprintf(enetvar, "%s%daddr", base_name, index); + else + sprintf(enetvar, "%saddr", base_name); + + return eth_setenv_enetaddr(enetvar, enetaddr); +} +#endif + static int eth_mac_skip(int index) { char enetvar[15];
Signed-off-by: Michael Walle <michael@walle.cc> Cc: Joe Hershberger <joe.hershberger@gmail.com> --- include/net.h | 16 ++++++++++++++++ net/eth.c | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 0 deletions(-)