[U-Boot] [PATCH v5 4/5] OMAP: networking support for SPL

[Ilya Yanok]

Hi Joe,

On Thu, Aug 30, 2012 at 1:25 AM, Joe Hershberger
<joe.hershberger at gmail.com>wrote:

>
> > diff --git a/arch/arm/cpu/armv7/omap-common/Makefile
> b/arch/arm/cpu/armv7/omap-common/Makefile
> > index d37b22d..f042078 100644
> > --- a/arch/arm/cpu/armv7/omap-common/Makefile
> > +++ b/arch/arm/cpu/armv7/omap-common/Makefile
> > @@ -53,6 +53,9 @@ endif
> >  ifdef CONFIG_SPL_YMODEM_SUPPORT
> >  COBJS  += spl_ymodem.o
> >  endif
> > +ifdef CONFIG_SPL_NET_SUPPORT
> > +COBJS  += spl_net.o
> > +endif
>
>
> Why not use common pattern of...
>
> COBJS-$(CONFIG_SPL_NET_SUPPORT) += spl_net.o
>
> COBJS   := $(sort $(COBJS-y))
>

In fact, I'm just following the existing style here... But I can change it
as you requested.


>
> > +#include <common.h>
> > +#include <net.h>
> > +#include <asm/omap_common.h>
>
> What in here needs this header?
>

Looks like it's unneeded. Thanks, I'll remove it.


>
> > diff --git a/common/command.c b/common/command.c
> > index aa0fb0a..9827c70 100644
> > --- a/common/command.c
> > +++ b/common/command.c
> > @@ -526,7 +526,7 @@ enum command_ret_t cmd_process(int flag, int argc,
> char * const argv[],
> >         if (argc > cmdtp->maxargs)
> >                 rc = CMD_RET_USAGE;
> >
> > -#if defined(CONFIG_CMD_BOOTD)
> > +#ifdef CONFIG_CMD_BOOTD
>
> Unrelated style change.
>

Yep, it came from earlier version. Will fix.


> > -#ifdef CONFIG_BOOTP_VCI_STRING
> > +#if defined(CONFIG_BOOTP_VCI_STRING) || \
> > +       (defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_NET_VCI_STRING))
> > +#ifndef CONFIG_SPL_BUILD
>
> Don't use negative logic for no reason.
>

Ok, Will fix.


>
> >         put_vci(e, CONFIG_VCI_STRING);
> > +#else
> > +       put_vci(e, CONFIG_SPL_NET_VCI_STRING);
> > +#endif
> >  #endif
> >
> >  #if defined(CONFIG_BOOTP_SUBNETMASK)
> > diff --git a/net/net.c b/net/net.c
> > index e8ff066..bbd1a6d 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -81,6 +81,19 @@
> >
> >
> >  #include <common.h>
> > +#ifdef CONFIG_SPL_BUILD
> > +/* SPL needs only BOOTP + TFTP so undefine other stuff to save space */
> > +#undef CONFIG_CMD_DHCP
> > +#undef CONFIG_CMD_CDP
> > +#undef CONFIG_CMD_DNS
> > +#undef CONFIG_CMD_LINK_LOCAL
> > +#undef CONFIG_CMD_NFS
> > +#undef CONFIG_CMD_PING
> > +#undef CONFIG_CMD_RARP
> > +#undef CONFIG_CMD_SNTP
> > +#undef CONFIG_CMD_TFTPPUT
> > +#undef CONFIG_CMD_TFTPSRV
> > +#endif
>
> Is this the best place to do this?  Seems it would be clearer to
> modify config_cmd_default.h or add a config_cmd_spl.h that will
> undefine them, and include that.
>

I agree it's not the best place... config_cmd_spl.h sounds a little bit
crazy as we don't have any commands at all in SPL...

Regards, Ilya.