[U-Boot] [PATCH v2 0/6] Reduce code size of libnet

[U-Boot] [PATCH v2 0/6] Reduce code size of libnet

[Simon Glass]

This patch series should change no functionality, but just reduce code size
slightly to what it was before the tftpput series.

Each patch is independent and each can be accepted or rejected as required.
Overall the effect is to reduce the code size to about 36 bytes lower
than it was before tftp when in on ARMv7 and PowerPC (IAD210) with my
toolchains.

Since this changes core net code, it needs to be examined carefully.

Changes in v2:
- Add net_transfer() fix
- Add patch with #ifdefs for cases which can only happen with tftpput
- Fix commit message to say memset() instead of memcpy()

Simon Glass (6):
  net: Hide more code behind CONFIG_CMD_TFTPPUT
  net: Make net_transfer() a static function
  net: Add more #ifdefs for tftpput to reduce code size
  net: Change for loop to memset()
  net: Be less picky about decoding the netretry env var
  net: Export auto_load, use it in rarp

 README        |    4 ++++
 include/net.h |    6 ++++++
 net/bootp.c   |   34 ++--------------------------------
 net/net.c     |   52 +++++++++++++++++++++++++++++++++++++++++++---------
 net/rarp.c    |   18 +-----------------
 net/tftp.c    |   19 +++++++++++++++----
 6 files changed, 71 insertions(+), 62 deletions(-)

-- 
1.7.3.1

[U-Boot] [PATCH v2 1/6] net: Hide more code behind CONFIG_CMD_TFTPPUT

[Simon Glass]

This commit reduces code size a little by making the ICMP handler only
available to tftpput. This is reasonable since it is the only user at
present (ping just uses the normal handler).

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 net/net.c  |    8 ++++++++
 net/tftp.c |    6 ++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index 84f28bf..11f41ed 100644
--- a/net/net.c
+++ b/net/net.c
@@ -215,7 +215,9 @@ volatile uchar *NetRxPackets[PKTBUFSRX];
 
 /* Current RX packet handler */
 static rxhand_f *packetHandler;
+#ifdef CONFIG_CMD_TFTPPUT
 static rxhand_icmp_f *packet_icmp_handler;	/* Current ICMP rx handler */
+#endif
 /* Current timeout handler */
 static thand_f *timeHandler;
 /* Time base value */
@@ -576,9 +578,11 @@ restart:
 	}
 
 done:
+#ifdef CONFIG_CMD_TFTPPUT
 	/* Clear out the handlers */
 	NetSetHandler(NULL);
 	net_set_icmp_handler(NULL);
+#endif
 	return ret;
 }
 
@@ -653,10 +657,12 @@ NetSetHandler(rxhand_f *f)
 	packetHandler = f;
 }
 
+#ifdef CONFIG_CMD_TFTPPUT
 void net_set_icmp_handler(rxhand_icmp_f *f)
 {
 	packet_icmp_handler = f;
 }
+#endif
 
 void
 NetSetTimeout(ulong iv, thand_f *f)
@@ -1397,10 +1403,12 @@ static void receive_icmp(IP_t *ip, int len, IPaddr_t src_ip, Ethernet_t *et)
 		break;
 #endif
 	default:
+#ifdef CONFIG_CMD_TFTPPUT
 		if (packet_icmp_handler)
 			packet_icmp_handler(icmph->type, icmph->code,
 				ntohs(ip->udp_dst), src_ip, ntohs(ip->udp_src),
 				icmph->un.data, ntohs(ip->udp_len));
+#endif
 		break;
 	}
 }
diff --git a/net/tftp.c b/net/tftp.c
index 961fdd1..e34f202 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -421,7 +421,7 @@ TftpSend(void)
 			 TftpOurPort, len);
 }
 
-
+#ifdef CONFIG_CMD_TFTPPUT
 static void icmp_handler(unsigned type, unsigned code, unsigned dest,
 			 IPaddr_t sip, unsigned src, uchar *pkt, unsigned len)
 {
@@ -430,6 +430,7 @@ static void icmp_handler(unsigned type, unsigned code, unsigned dest,
 		restart("TFTP server died");
 	}
 }
+#endif
 
 static void
 TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
@@ -771,8 +772,9 @@ void TftpStart(enum proto_t protocol)
 
 	NetSetTimeout(TftpTimeoutMSecs, TftpTimeout);
 	NetSetHandler(TftpHandler);
+#ifdef CONFIG_CMD_TFTPPUT
 	net_set_icmp_handler(icmp_handler);
-
+#endif
 	TftpRemotePort = WELL_KNOWN_PORT;
 	TftpTimeoutCount = 0;
 	/* Use a pseudo-random port unless a specific port is set */
-- 
1.7.3.1

[U-Boot] [PATCH v2 1/6] net: Hide more code behind CONFIG_CMD_TFTPPUT

[Mike Frysinger]

i wish there was a way we could be clever and give gcc hints to do
better DCE rather than sprinkling #ifdef's.  but maybe this is a pipe
dream.
-mike

[U-Boot] [PATCH v2 1/6] net: Hide more code behind CONFIG_CMD_TFTPPUT

[Wolfgang Denk]

Dear Simon Glass,

In message <1319732672-29071-2-git-send-email-sjg@chromium.org> you wrote:
> This commit reduces code size a little by making the ICMP handler only
> available to tftpput. This is reasonable since it is the only user at
> present (ping just uses the normal handler).
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
God is real, unless declared integer.

[U-Boot] [PATCH v2 2/6] net: Make net_transfer() a static function

[Simon Glass]

This should be a static function so it can be inlined.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add net_transfer() fix

 net/tftp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index e34f202..81f9af4 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -198,7 +198,7 @@ store_block(unsigned block, uchar *src, unsigned len)
 }
 
 /* Clear our state ready for a new transfer */
-void new_transfer(void)
+static void new_transfer(void)
 {
 	TftpLastBlock = 0;
 	TftpBlockWrap = 0;
-- 
1.7.3.1

[U-Boot] [PATCH v2 2/6] net: Make net_transfer() a static function

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[U-Boot] [PATCH v2 2/6] net: Make net_transfer() a static function

[Wolfgang Denk]

Dear Simon Glass,

In message <1319732672-29071-3-git-send-email-sjg@chromium.org> you wrote:
> This should be a static function so it can be inlined.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Add net_transfer() fix
> 
>  net/tftp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Most people would like to be delivered  from  temptation  but  would
like it to keep in touch."                             - Robert Orben

[U-Boot] [PATCH v2 3/6] net: Add more #ifdefs for tftpput to reduce code size

[Simon Glass]

If CONFIG_CMD_TFTPPUT is not enabled, we want minimal code size impact
on the tftp code. This introduces a few more #ifdefs.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add patch with #ifdefs for cases which can only happen with tftpput

 net/tftp.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index 81f9af4..4999707 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -332,8 +332,12 @@ TftpSend(void)
 	case STATE_SEND_WRQ:
 		xp = pkt;
 		s = (ushort *)pkt;
+#ifdef CONFIG_CMD_TFTPPUT
 		*s++ = htons(TftpState == STATE_SEND_RRQ ? TFTP_RRQ :
 			TFTP_WRQ);
+#else
+		*s++ = htons(TFTP_RRQ);
+#endif
 		pkt = (uchar *)s;
 		strcpy((char *)pkt, tftp_filename);
 		pkt += strlen(tftp_filename) + 1;
@@ -730,7 +734,12 @@ void TftpStart(enum proto_t protocol)
 
 	printf("Using %s device\n", eth_get_name());
 	printf("TFTP %s server %pI4; our IP address is %pI4",
-	       protocol == TFTPPUT ? "to" : "from", &TftpRemoteIP, &NetOurIP);
+#ifdef CONFIG_CMD_TFTPPUT
+	       protocol == TFTPPUT ? "to" : "from",
+#else
+		"from",
+#endif
+		&TftpRemoteIP, &NetOurIP);
 
 	/* Check if we need to send across this subnet */
 	if (NetOurGatewayIP && NetOurSubnetMask) {
-- 
1.7.3.1

[U-Boot] [PATCH v2 3/6] net: Add more #ifdefs for tftpput to reduce code size

[Wolfgang Denk]

Dear Simon Glass,

In message <1319732672-29071-4-git-send-email-sjg@chromium.org> you wrote:
> If CONFIG_CMD_TFTPPUT is not enabled, we want minimal code size impact
> on the tftp code. This introduces a few more #ifdefs.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Add patch with #ifdefs for cases which can only happen with tftpput
> 
>  net/tftp.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The two most common things in the universe are hydrogen  and  stupi-
dity."

[U-Boot] [PATCH v2 4/6] net: Change for loop to memset()

[Simon Glass]

This is intended purely as a code size reduction.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Fix commit message to say memset() instead of memcpy()

 net/net.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/net.c b/net/net.c
index 11f41ed..cd34bf9 100644
--- a/net/net.c
+++ b/net/net.c
@@ -246,7 +246,6 @@ int		NetArpWaitTry;
 
 void ArpRequest(void)
 {
-	int i;
 	volatile uchar *pkt;
 	ARP_t *arp;
 
@@ -268,11 +267,8 @@ void ArpRequest(void)
 	memcpy(&arp->ar_data[0], NetOurEther, 6);
 	/* source IP addr */
 	NetWriteIP((uchar *) &arp->ar_data[6], NetOurIP);
-	for (i = 10; i < 16; ++i) {
-		/* dest ET addr = 0 */
-		arp->ar_data[i] = 0;
-	}
-
+	/* dest ET addr = 0 */
+	memset(&arp->ar_data[10], '\0', 6);
 	if ((NetArpWaitPacketIP & NetOurSubnetMask) !=
 	    (NetOurIP & NetOurSubnetMask)) {
 		if (NetOurGatewayIP == 0) {
-- 
1.7.3.1

[U-Boot] [PATCH v2 4/6] net: Change for loop to memset()

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[U-Boot] [PATCH v2 4/6] net: Change for loop to memset()

[Wolfgang Denk]

Dear Simon Glass,

In message <1319732672-29071-5-git-send-email-sjg@chromium.org> you wrote:
> This is intended purely as a code size reduction.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Fix commit message to say memset() instead of memcpy()
> 
>  net/net.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery

[U-Boot] [PATCH v2 5/6] net: Be less picky about decoding the netretry env var

[Simon Glass]

This is intended purely as a code size reduction.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 README    |    4 ++++
 net/net.c |    6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/README b/README
index a1910dc..4d9ab77 100644
--- a/README
+++ b/README
@@ -3579,6 +3579,10 @@ List of environment variables (most likely not complete):
 		  are tried once without success.
 		  Useful on scripts which control the retry operation
 		  themselves.
+		  When unset, or set to "yes", each network operation
+		  will retry forever. Note that only the first letter
+		  is significant (any word beginning with y, n, o
+		  will select the corressponding option).
 
   npe_ucode	- set load address for the NPE microcode
 
diff --git a/net/net.c b/net/net.c
index cd34bf9..3712e17 100644
--- a/net/net.c
+++ b/net/net.c
@@ -605,11 +605,11 @@ void NetStartAgain(void)
 
 	nretry = getenv("netretry");
 	if (nretry) {
-		if (!strcmp(nretry, "yes"))
+		if (*nretry == 'y')
 			retry_forever = 1;
-		else if (!strcmp(nretry, "no"))
+		else if (!*nretry == 'n')
 			retrycnt = 0;
-		else if (!strcmp(nretry, "once"))
+		else if (*nretry == 'o')	/* "once" */
 			retrycnt = 1;
 		else
 			retrycnt = simple_strtoul(nretry, NULL, 0);
-- 
1.7.3.1

[U-Boot] [PATCH v2 5/6] net: Be less picky about decoding the netretry env var

[Wolfgang Denk]

Dear Simon Glass,

In message <1319732672-29071-6-git-send-email-sjg@chromium.org> you wrote:
> This is intended purely as a code size reduction.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  README    |    4 ++++
>  net/net.c |    6 +++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/README b/README
> index a1910dc..4d9ab77 100644
> --- a/README
> +++ b/README
> @@ -3579,6 +3579,10 @@ List of environment variables (most likely not complete):
>  		  are tried once without success.
>  		  Useful on scripts which control the retry operation
>  		  themselves.
> +		  When unset, or set to "yes", each network operation
> +		  will retry forever. Note that only the first letter
> +		  is significant (any word beginning with y, n, o
> +		  will select the corressponding option).

Sorry, but especially the 'o' scares me.

May I drop this one?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"What if" is a trademark of Hewlett Packard, so stop using it in your
sentences without permission, or risk being sued.

[U-Boot] [PATCH v2 5/6] net: Be less picky about decoding the netretry env var

[Simon Glass]

Hi Wolfgang,

On Thu, Oct 27, 2011 at 2:40 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1319732672-29071-6-git-send-email-sjg@chromium.org> you wrote:
>> This is intended purely as a code size reduction.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> ?README ? ?| ? ?4 ++++
>> ?net/net.c | ? ?6 +++---
>> ?2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/README b/README
>> index a1910dc..4d9ab77 100644
>> --- a/README
>> +++ b/README
>> @@ -3579,6 +3579,10 @@ List of environment variables (most likely not complete):
>> ? ? ? ? ? ? ? ? are tried once without success.
>> ? ? ? ? ? ? ? ? Useful on scripts which control the retry operation
>> ? ? ? ? ? ? ? ? themselves.
>> + ? ? ? ? ? ? ? When unset, or set to "yes", each network operation
>> + ? ? ? ? ? ? ? will retry forever. Note that only the first letter
>> + ? ? ? ? ? ? ? is significant (any word beginning with y, n, o
>> + ? ? ? ? ? ? ? will select the corressponding option).
>
> Sorry, but especially the 'o' scares me.
>
> May I drop this one?

Yes of course, these are just ideas and I agree about the 'o'.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> "What if" is a trademark of Hewlett Packard, so stop using it in your
> sentences without permission, or risk being sued.
>

[U-Boot] [PATCH v2 6/6] net: Export auto_load, use it in rarp

[Simon Glass]

The rarp code includes another instance of the auto_load logic, so call
what is now net_auto_load() instead.

This also fixes an incorrect call to TftpStart() which was never seen
since apparently no boards enable rarp.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 include/net.h |    6 ++++++
 net/bootp.c   |   34 ++--------------------------------
 net/net.c     |   30 ++++++++++++++++++++++++++++++
 net/rarp.c    |   18 +-----------------
 4 files changed, 39 insertions(+), 49 deletions(-)

diff --git a/include/net.h b/include/net.h
index b408dea..ad9afbf 100644
--- a/include/net.h
+++ b/include/net.h
@@ -430,6 +430,12 @@ extern int	NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, i
 extern void	NetReceive(volatile uchar *, int);
 
 /*
+ * Check if autoload is enabled. If so, use either NFS or TFTP to download
+ * the boot file.
+ */
+void net_auto_load(void);
+
+/*
  * The following functions are a bit ugly, but necessary to deal with
  * alignment restrictions on ARM.
  *
diff --git a/net/bootp.c b/net/bootp.c
index b703f42..b789eec 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -138,36 +138,6 @@ static int truncate_sz (const char *name, int maxlen, int curlen)
 	return (curlen);
 }
 
-/*
- * Check if autoload is enabled. If so, use either NFS or TFTP to download
- * the boot file.
- */
-static void auto_load(void)
-{
-	const char *s = getenv("autoload");
-
-	if (s != NULL) {
-		if (*s == 'n') {
-			/*
-			 * Just use BOOTP to configure system;
-			 * Do not use TFTP to load the bootfile.
-			 */
-			NetState = NETLOOP_SUCCESS;
-			return;
-		}
-#if defined(CONFIG_CMD_NFS)
-		if (strcmp(s, "NFS") == 0) {
-			/*
-			 * Use NFS to load the bootfile.
-			 */
-			NfsStart();
-			return;
-		}
-#endif
-	}
-	TftpStart(TFTPGET);
-}
-
 #if !defined(CONFIG_CMD_DHCP)
 
 static void BootpVendorFieldProcess (u8 * ext)
@@ -354,7 +324,7 @@ BootpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
 
 	debug("Got good BOOTP\n");
 
-	auto_load();
+	net_auto_load();
 }
 #endif
 
@@ -979,7 +949,7 @@ DhcpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
 			dhcp_state = BOUND;
 			printf ("DHCP client bound to address %pI4\n", &NetOurIP);
 
-			auto_load();
+			net_auto_load();
 			return;
 		}
 		break;
diff --git a/net/net.c b/net/net.c
index 3712e17..ac37f7c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -309,6 +309,36 @@ void ArpTimeoutCheck(void)
 	}
 }
 
+/*
+ * Check if autoload is enabled. If so, use either NFS or TFTP to download
+ * the boot file.
+ */
+void net_auto_load(void)
+{
+	const char *s = getenv("autoload");
+
+	if (s != NULL) {
+		if (*s == 'n') {
+			/*
+			 * Just use BOOTP/RARP to configure system;
+			 * Do not use TFTP to load the bootfile.
+			 */
+			NetState = NETLOOP_SUCCESS;
+			return;
+		}
+#if defined(CONFIG_CMD_NFS)
+		if (strcmp(s, "NFS") == 0) {
+			/*
+			 * Use NFS to load the bootfile.
+			 */
+			NfsStart();
+			return;
+		}
+#endif
+	}
+	TftpStart(TFTPGET);
+}
+
 static void NetInitLoop(enum proto_t protocol)
 {
 	static int env_changed_id;
diff --git a/net/rarp.c b/net/rarp.c
index 94c86d3..097f970 100644
--- a/net/rarp.c
+++ b/net/rarp.c
@@ -46,24 +46,8 @@ static void
 RarpHandler(uchar *dummi0, unsigned dummi1, IPaddr_t sip, unsigned dummi2,
 	    unsigned dummi3)
 {
-	char *s;
 	debug("Got good RARP\n");
-	if ((s = getenv("autoload")) != NULL) {
-		if (*s == 'n') {
-			/*
-			 * Just use RARP to configure system;
-			 * Do not use TFTP/NFS to to load the bootfile.
-			 */
-			NetState = NETLOOP_SUCCESS;
-			return;
-#if defined(CONFIG_CMD_NFS)
-		} else if ((s != NULL) && !strcmp(s, "NFS")) {
-			NfsStart();
-			return;
-#endif
-		}
-	}
-	TftpStart ();
+	net_auto_load();
 }
 
 
-- 
1.7.3.1

[U-Boot] [PATCH v2 6/6] net: Export auto_load, use it in rarp

[Mike Frysinger]

On Thu, Oct 27, 2011 at 18:24, Simon Glass wrote:
> The rarp code includes another instance of the auto_load logic, so call
> what is now net_auto_load() instead.

Acked-by: Mike Frysinger <vapier@gentoo.org>

> This also fixes an incorrect call to TftpStart() which was never seen
> since apparently no boards enable rarp.

i wonder if we shouldn't just drop rarp then if no one actually uses it ...
-mike

[U-Boot] [PATCH v2 6/6] net: Export auto_load, use it in rarp

[Simon Glass]

Hi Mike,

On Thu, Oct 27, 2011 at 2:12 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thu, Oct 27, 2011 at 18:24, Simon Glass wrote:
>> The rarp code includes another instance of the auto_load logic, so call
>> what is now net_auto_load() instead.
>
> Acked-by: Mike Frysinger <vapier@gentoo.org>
>
>> This also fixes an incorrect call to TftpStart() which was never seen
>> since apparently no boards enable rarp.
>
> i wonder if we shouldn't just drop rarp then if no one actually uses it ...
> -mike
>

Well that's the definition of dead code, but of course there may be
thousands of boards out there that use it, where the code is not
upstream. I suppose we could take the code out and see if anyone
notices :-) It is at least in a separate file and not built unless
requested.

Regards,
Simon

[U-Boot] [PATCH v2 6/6] net: Export auto_load, use it in rarp

[Mike Frysinger]

On Fri, Oct 28, 2011 at 00:24, Simon Glass wrote:
> On Thu, Oct 27, 2011 at 2:12 PM, Mike Frysinger wrote:
>> On Thu, Oct 27, 2011 at 18:24, Simon Glass wrote:
>>> This also fixes an incorrect call to TftpStart() which was never seen
>>> since apparently no boards enable rarp.
>>
>> i wonder if we shouldn't just drop rarp then if no one actually uses it ...
>
> Well that's the definition of dead code, but of course there may be
> thousands of boards out there that use it, where the code is not
> upstream. I suppose we could take the code out and see if anyone
> notices :-) It is at least in a separate file and not built unless
> requested.

i think our (reasonable) position is that if no one is using it, and
no one is noticing when it breaks, then we should simply drop it.  if
out of tree boards cared, they'd be in tree.

i like to think that the barrier for inclusion in u-boot is
significantly lower than the linux kernel ...
-mike

[U-Boot] [PATCH v2 6/6] net: Export auto_load, use it in rarp

[Scott Wood]

On 10/27/2011 05:40 PM, Mike Frysinger wrote:
> On Fri, Oct 28, 2011 at 00:24, Simon Glass wrote:
>> On Thu, Oct 27, 2011 at 2:12 PM, Mike Frysinger wrote:
>>> On Thu, Oct 27, 2011 at 18:24, Simon Glass wrote:
>>>> This also fixes an incorrect call to TftpStart() which was never seen
>>>> since apparently no boards enable rarp.
>>>
>>> i wonder if we shouldn't just drop rarp then if no one actually uses it ...
>>
>> Well that's the definition of dead code, but of course there may be
>> thousands of boards out there that use it, where the code is not
>> upstream. I suppose we could take the code out and see if anyone
>> notices :-) It is at least in a separate file and not built unless
>> requested.
> 
> i think our (reasonable) position is that if no one is using it, and
> no one is noticing when it breaks, then we should simply drop it.  if
> out of tree boards cared, they'd be in tree.

For features like this that are user-enableable, rather than e.g. a
board code dependency, it's users that care, not boards.  I don't think
we're asking people to submit a variant "board" config with their own
personal preferences/requirements, if they differ from the board
maintainer's.  Unfortunately U-Boot lacks the ability to do an
"allyesconfig" to at least build-test such features.

Of course, if it's actually been broken for a while, that suggests that
the users don't care either. :-)  Or perhaps are running older versions,
but might upgrade at some point.

It's probably best to follow the normal feature removal process to see
if anyone's interested in maintaining it, before removing it.

-Scott

[U-Boot] [PATCH v2 6/6] net: Export auto_load, use it in rarp

[Mike Frysinger]

On Fri, Oct 28, 2011 at 01:09, Scott Wood wrote:
> On 10/27/2011 05:40 PM, Mike Frysinger wrote:
>> On Fri, Oct 28, 2011 at 00:24, Simon Glass wrote:
>>> On Thu, Oct 27, 2011 at 2:12 PM, Mike Frysinger wrote:
>>>> On Thu, Oct 27, 2011 at 18:24, Simon Glass wrote:
>>>>> This also fixes an incorrect call to TftpStart() which was never seen
>>>>> since apparently no boards enable rarp.
>>>>
>>>> i wonder if we shouldn't just drop rarp then if no one actually uses it ...
>>>
>>> Well that's the definition of dead code, but of course there may be
>>> thousands of boards out there that use it, where the code is not
>>> upstream. I suppose we could take the code out and see if anyone
>>> notices :-) It is at least in a separate file and not built unless
>>> requested.
>>
>> i think our (reasonable) position is that if no one is using it, and
>> no one is noticing when it breaks, then we should simply drop it. ?if
>> out of tree boards cared, they'd be in tree.
>
> For features like this that are user-enableable, rather than e.g. a
> board code dependency, it's users that care, not boards. ?I don't think
> we're asking people to submit a variant "board" config with their own
> personal preferences/requirements, if they differ from the board
> maintainer's. ?Unfortunately U-Boot lacks the ability to do an
> "allyesconfig" to at least build-test such features.

i'd submit that the boards in the tree are a fairly good sampling of
what people actually deploy.  by virtue of no default board config
enabling this, it's most likely that no user does either.

> It's probably best to follow the normal feature removal process to see
> if anyone's interested in maintaining it, before removing it.

certainly
-mike