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

[Ilya Yanok]

On Mon, Sep 17, 2012 at 1:55 PM, Ilya Yanok
wrote:

> > +#include 
>> > +#include 
>> > +#include 
>>
>> What in here needs this header?
>>
>
> Looks like it's unneeded. Thanks, I'll remove it.
>

Nope, it's needed for spl_parse_image_header.

Regards, Ilya.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Tom Rini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/17/12 11:10, Ilya Yanok wrote:
> 
> 
> On Mon, Sep 17, 2012 at 10:07 PM, Tom Rini  > wrote:
> 
>> That's not really about garbage collection in this case
>> (net-spl). I want to disable some functionality of generic net
>> code not some stuff used only by commands implementation. The
>> confusion comes from the fact that this code is protected by
>> CONFIG_CMD_* defines.
> 
> So I guess the code construct is roughly: function_we_need(...) { 
> #ifdef CONFIG_CMD_A ... stuff_spl_does_not_need(); #endif ... }
> 
> ?  Otherwise we would end up building files we don't use, but then
> all of the un-used code gets garbage collected.
> 
> 
> Exactly.

OK, config_uncmd_spl.h, nice big comment and v6, thanks :)

- -- 
Tom
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQV2m7AAoJENk4IS6UOR1WxuEP/RFkI74bRCQeKKjTTEjIhRJo
d1GQdX0GPJ0eFoMZ8ZX4Q1vo/j1VobTPABPu6DT0t/40biKrZvQu1EAf+i7Kct8c
P/90xoWI60L6gYo9/mKdxu5TDULbyg306RUr8D2KoXhmMm1w57Ug4LK/MXH3WcLk
rVMNJT6WXNDuAaHy8fM7ZxLIdDyuVlo4rqf6CgjKA4iE1LEyNf3RHE9PQn9780QJ
Eun4/sLN3ulBc94s6+I+OAY4HeBwXCxOlVjxu5ta/NnmY5OEMfMbwVl7ka2sLspt
R58ZxOJ3oSZbVL/joQ6zk6JnmWCX6Dku8M/V7eiryXMwx+F8rdYX+mcL3/0Jk337
2AzScXTzKMVQnv1IFuG+SlHMH0jEcsPsd/sa9C+mDQIfcwNby86uFs0Khj2kNAwq
0GWPdOvP+KKmdiaKpgth2vcvmJcln10p9YyB8K8MVwByDP9Mw+mlS4cxvr/yGf5V
3kDntGP9mpn6L1qp8MceKyz27gE9HkbTYGVk5r4TEwwwdahHiQ500UswivLDtO4V
IH1hzZsachaYyGlptiAvxw3ZpRQwQ2j7P/2s6G2uW1b4VoSYcDiT0uzYYYVqKi1u
u3jmpgH3Qse62rcZPg3gSqrwPFFhYV/6Ol+evcI6dk1I3o2a8wbMRJnSBZyqe0UV
/05A2dMTsdnNwpUe9f54
=QB8w
-END PGP SIGNATURE-
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Ilya Yanok]

BTW, I'm going to repost this serie soon. Shouldn't I rebase it on top of
your SPL rework patches? Where can I find the tree?

Regards, Ilya.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Ilya Yanok]

On Mon, Sep 17, 2012 at 10:07 PM, Tom Rini  wrote:

> > That's not really about garbage collection in this case (net-spl).
> > I want to disable some functionality of generic net code not some
> > stuff used only by commands implementation. The confusion comes
> > from the fact that this code is protected by CONFIG_CMD_* defines.
>
> So I guess the code construct is roughly:
> function_we_need(...) {
> #ifdef CONFIG_CMD_A
>   ... stuff_spl_does_not_need();
> #endif
> ...
> }
>
> ?  Otherwise we would end up building files we don't use, but then all
> of the un-used code gets garbage collected.
>

Exactly.

Regards, Ilya.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Tom Rini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/17/12 10:54, Ilya Yanok wrote:
> Hi Tom,
> 
> On Mon, Sep 17, 2012 at 9:04 PM, Tom Rini  > wrote:
> 
>> 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...
> 
> 
> How about config_uncmd_spl.h then and a nice big comment up top
> 
> 
> Well, it will be at least less confusing...
> 
> 
> explaining what we're doing.  Or can we take another stab at
> seeing why some stuff isn't being garbage collected?  If 
> garbage_collected_func_a calls never_seen_while_linking_func, we
> still succeed since we garbage collect the first func.  There's
> just the gcc issue I've noted before about strings.
> 
> 
> That's not really about garbage collection in this case (net-spl).
> I want to disable some functionality of generic net code not some
> stuff used only by commands implementation. The confusion comes
> from the fact that this code is protected by CONFIG_CMD_* defines.

So I guess the code construct is roughly:
function_we_need(...) {
#ifdef CONFIG_CMD_A
  ... stuff_spl_does_not_need();
#endif
...
}

?  Otherwise we would end up building files we don't use, but then all
of the un-used code gets garbage collected.

- -- 
Tom
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIbBAEBAgAGBQJQV2bQAAoJENk4IS6UOR1WXNAP+MLdJeSu0x5xpbKEyURT6ISc
hKwYFO9esQRTnssp+0efz+RlYgAV1fet5pxhtn+RqMvA5XfWT413yZoEOg7XzsBE
TYcVs63TRuTDI5o0qgOryAQifY6GTFj1hw4BecAvODErJl7D6dtyGJjg4QWVUP4M
C1AFb0132ILlj5MSXCAC3d0ZYVNDqkIGl0BNGUSwOmZ8OT7IHi/x1tOUaKibOin6
wytrPXFwNxjvNeDVXe1Ot89Kx/t+kbNh6LDJLoIPK8SasuKdrtDxiQLCMMGGxT7R
vAS7//AqupWuzyWPdcQvM+YJetDKk/iYCy1FpYeo0yY1nKBHgh85lr6yBqjRnwKV
Tbh9ny41J1xd4AhIbRvEXirReZ/Zu3vEr01Qe+ddqgY+2mol0wucyhY2dgWlAn5G
jRCARRpEd8tIgzWBN5lxosHq+v7iltAOXZT/hHZpv19zzZ2KK3xG4CiqaZZvRS2B
DHWj2dOZW7CJGhCpYapLtCmxhh+M4X6YGflNCkiuQV9NGZjE3PhGh0N9QRQXBQjU
CzmY/cqWXG7QZ6NIlKyEG9nZMuFggSW2miHINGmk4G68rH6QfCpGilucOkN2LDjt
sE60qAIxRmkW2emn12V9dBU5CTyhgVetr0nYBD0PGO366Z+Xoq9sODG2aiQYdNTF
250WBWG235mtco6BNUw=
=v55G
-END PGP SIGNATURE-
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Ilya Yanok]

Hi Tom,

On Mon, Sep 17, 2012 at 9:04 PM, Tom Rini  wrote:

> > 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...
>
>
> How about config_uncmd_spl.h then and a nice big comment up top
>

Well, it will be at least less confusing...


> explaining what we're doing.  Or can we take another stab at seeing
> why some stuff isn't being garbage collected?  If
> garbage_collected_func_a calls never_seen_while_linking_func, we still
> succeed since we garbage collect the first func.  There's just the gcc
> issue I've noted before about strings.
>

That's not really about garbage collection in this case (net-spl). I want
to disable some functionality of generic net code not some stuff used only
by commands implementation. The confusion comes from the fact that this
code is protected by CONFIG_CMD_* defines.

Regards, Ilya.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Tom Rini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/17/12 02:55, Ilya Yanok wrote:
> Hi Joe,
> 
> On Thu, Aug 30, 2012 at 1:25 AM, Joe Hershberger 
> mailto:joe.hershber...@gmail.com>>
> wrote:
[snip]
>> 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  +#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...

How about config_uncmd_spl.h then and a nice big comment up top
explaining what we're doing.  Or can we take another stab at seeing
why some stuff isn't being garbage collected?  If
garbage_collected_func_a calls never_seen_while_linking_func, we still
succeed since we garbage collect the first func.  There's just the gcc
issue I've noted before about strings.

- -- 
Tom
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQV1goAAoJENk4IS6UOR1WmCIQAKmnvVA5ZiYmWNqTvXPtjCz3
DRBfE8iLLpDzMgzjgEyy/3e3at7zE4+svN5ncYvkxPRBqiBol9InXYnUBdgZXiMk
7/HahZdJpsX1KwFGQxUTuRErABQXT14IQn0Nz/7xKbpHcFlrJSxPaKb3RblwBylj
nZiRYhNfCFVzsMvmLPRk/IiHh9EMqwGDmKW1sAsF7qGw1CFzOTkLW9KKIqW6anUH
JXOy0fVeVOyBK6erK2R0kFYbYSvpYUJPG88pymu/hiSHvwI2vOZdJ0g6Q9O3RKP3
rXz5Xnei5ujnQ5Kb8T5gES5Y1My/m1LXCqSY2Aq0cuyDnUEDItZLYycPKkrXZ3yf
W73ydbMrT1BpFasWB/Crce/J54zIpLMnQiFpJ0vgv4sSD+yzrD4o02TsYeXgvekJ
KpJRhsdRp1r190s2wdyeJTTSsP2yQCbPWGL/vDQr9hb1tdLK+EIxLsG++9uZxHQ5
TgtaPyvlScaz/WC0TxE3NRNOGFRqvTGa+MgD3JHoUOMfc1FhsZXIjoW5K239GvkL
lIEFKYaTsFvo1lygPFUTZPEls4ElbeJ6OJjJy9xUs33avlpvTbkEqgkb2L39+uQM
cK5LoGKqwyrEa8CMxyCkzD6GTywkf+LYiUmKY6Ped2svN0WRgakISOuDRXdAdsB0
W2Gx6XbRea/1iE+wHEUC
=7bJ2
-END PGP SIGNATURE-
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [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
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 
> > +#include 
> > +#include 
>
> 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 
> > +#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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Tom Rini]

On 08/29/2012 02:25 PM, Joe Hershberger wrote:

[snip]
>>  #include 
>> +#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 was thinking about that too, include/config_cmd_spl.h should probably
be how this happens.

-- 
Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Joe Hershberger]

Hi Ilya,

On Tue, Aug 7, 2012 at 3:07 AM, Ilya Yanok
 wrote:
> This patch adds support for networking in SPL. Some devices are
> capable of loading SPL via network so it makes sense to load the
> main U-Boot binary via network too. This patch tries to use
> existing network code as much as possible. Unfortunately, it depends
> on environment which in turn depends on other code so SPL size
> is increased significantly. No effort was done to decouple network
> code and environment so far.
>
> Signed-off-by: Ilya Yanok 
>
> ---
> Changes in v3:
>  - use BOOTP in SPL regardless of CONFIG_CMD_DHCP
>  - add support for setting different VCI in SPL
>
> Changes in v4:
>  - fix compilation of SPL's libcommon with CONFIG_HUSH_PARSER
>and CONFIG_BOOTD defined
>  - rename spl_eth.c to spl_net.c
>  - set ethact variable if device name is passed
>
> Changes in v5:
>  - set up guards in cmd_nvedit.c more carefully
>  - now we don't need command.c and only need main.c for
>show_boot_progress() so defined it to be noop and remove
>both files from SPL sources
>  - SPL guards in command.c and main.c are no longer needed
>  - add some guards in env_common.c
>  - qsort.c is no longer needed
>  - add guard to hashtable.c to save some space
>  - undefine unneeded CONFIG_CMD_* while building SPL to save space
>
>  arch/arm/cpu/armv7/omap-common/Makefile  |3 ++
>  arch/arm/cpu/armv7/omap-common/spl.c |9 ++
>  arch/arm/cpu/armv7/omap-common/spl_net.c |   52 
> ++
>  arch/arm/include/asm/omap_common.h   |4 +++
>  common/Makefile  |4 +++
>  common/cmd_nvedit.c  |   11 ++-
>  common/command.c |2 +-
>  common/env_common.c  |7 ++--
>  include/bootstage.h  |6 +++-
>  lib/Makefile |9 --
>  lib/hashtable.c  |2 ++
>  lib/vsprintf.c   |2 +-
>  net/bootp.c  |   11 ++-
>  net/net.c|   13 
>  net/tftp.c   |4 +++
>  spl/Makefile |3 ++
>  16 files changed, 133 insertions(+), 9 deletions(-)
>  create mode 100644 arch/arm/cpu/armv7/omap-common/spl_net.c
>
> 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))

>  endif
>
>  ifndef CONFIG_SPL_BUILD
> diff --git a/arch/arm/cpu/armv7/omap-common/spl.c 
> b/arch/arm/cpu/armv7/omap-common/spl.c
> index f0d766c..53b9261 100644
> --- a/arch/arm/cpu/armv7/omap-common/spl.c
> +++ b/arch/arm/cpu/armv7/omap-common/spl.c
> @@ -178,6 +178,15 @@ void board_init_r(gd_t *id, ulong dummy)
> spl_ymodem_load_image();
> break;
>  #endif
> +#ifdef CONFIG_SPL_ETH_SUPPORT
> +   case BOOT_DEVICE_CPGMAC:
> +#ifdef CONFIG_SPL_ETH_DEVICE
> +   spl_net_load_image(CONFIG_SPL_ETH_DEVICE);
> +#else
> +   spl_net_load_image(NULL);
> +#endif
> +   break;
> +#endif
> default:
> printf("SPL: Un-supported Boot Device - %d!!!\n", 
> boot_device);
> hang();
> diff --git a/arch/arm/cpu/armv7/omap-common/spl_net.c 
> b/arch/arm/cpu/armv7/omap-common/spl_net.c
> new file mode 100644
> index 000..cbb3087
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/omap-common/spl_net.c
> @@ -0,0 +1,52 @@
> +/*
> + * (C) Copyright 2000-2004
> + * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
> + *
> + * (C) Copyright 2012
> + * Ilya Yanok 
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc.
> + */
> +#include 
> +#include 
> +#include 

What in here needs this header?

> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +void spl_net_load_image(const char *device)
> +{
> +   i