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

[U-Boot] [PATCH 0/3] 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 (on ARM) is to reduce the code size to about 5 bytes lower
than it was before tftp when in. On PowerPC the number seems to be a little
more (39 bytes less on the IAD210 board).

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


Simon Glass (3):
  net: Hide more code behind CONFIG_CMD_TFTPPUT
  net: Change for loop to memcpy()
  net: Be less picky about decoding the netretry env var

 net/net.c  |   22 +++++++++++++---------
 net/tftp.c |    6 ++++--
 2 files changed, 17 insertions(+), 11 deletions(-)

-- 
1.7.3.1

[U-Boot] [PATCH 1/3] 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 2/3] net: Change for loop to memcpy()

[Simon Glass]

This is intended purely as a code size reduction.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 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 3/3] 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>
---
 net/net.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

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 3/3] net: Be less picky about decoding the netretry env var

[Mike Frysinger]

On Thu, Oct 27, 2011 at 02:18, Simon Glass wrote:
> --- a/net/net.c
> +++ b/net/net.c
>
> - ? ? ? ? ? ? ? if (!strcmp(nretry, "yes"))
> + ? ? ? ? ? ? ? if (*nretry == 'y')

not sure about this as it makes it hard to add code in the future if
we care about compatibility.  if we support just "y", people will
start using it.
-mike

[U-Boot] [PATCH 2/3] net: Change for loop to memcpy()

[Mike Frysinger]

On Thu, Oct 27, 2011 at 02:18, Simon Glass wrote:
> + ? ? ? memset(&arp->ar_data[10], '\0', 6);

summary says memcpy.  code says memset.
-mike

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

[Simon Glass]

Hi Mike,

On Wed, Oct 26, 2011 at 11:00 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thu, Oct 27, 2011 at 02:18, Simon Glass wrote:
>> --- a/net/net.c
>> +++ b/net/net.c
>>
>> - ? ? ? ? ? ? ? if (!strcmp(nretry, "yes"))
>> + ? ? ? ? ? ? ? if (*nretry == 'y')
>
> not sure about this as it makes it hard to add code in the future if
> we care about compatibility. ?if we support just "y", people will
> start using it.
> -mike
>

Yes, I'm not sure either. I have found a few env variables that use
single letters (autoload, flashchecksum) and noticed that quite a few
commands only decode as much of their subcommand as they need to be
unique. I am happier with y and n than o!

Regards,
Simon

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

[Mike Frysinger]

On Thu, Oct 27, 2011 at 15:35, Simon Glass wrote:
> On Wed, Oct 26, 2011 at 11:00 PM, Mike Frysinger wrote:
>> On Thu, Oct 27, 2011 at 02:18, Simon Glass wrote:
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>>
>>> - ? ? ? ? ? ? ? if (!strcmp(nretry, "yes"))
>>> + ? ? ? ? ? ? ? if (*nretry == 'y')
>>
>> not sure about this as it makes it hard to add code in the future if
>> we care about compatibility. ?if we support just "y", people will
>> start using it.
>
> Yes, I'm not sure either. I have found a few env variables that use
> single letters (autoload, flashchecksum) and noticed that quite a few
> commands only decode as much of their subcommand as they need to be
> unique. I am happier with y and n than o!

right, u-boot is inconsistent

the short command names are a bit unique in that we already say "if
you don't use the full command name, that's your fault".  after all,
how much of the short name you need to use changes based on the
commands enabled and the autocomplete define being enabled.

if you enable "reginfo" and "reset", then you can't use "r" or "re".
but if you only have "reset", then you can use "r" and "re" to reset.

env vars are slightly different in that they're not terribly well
documented.  so people look at the source to see what is accepted and
then do that.  maybe the answer is to document the exact values and
then when people screw up, we can point to that as a fallback.
-mike

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1RDqAER6aWZ08o7pymW98o8-1y8pMM2p6NNv2XWn44LA@mail.gmail.com> you wrote:
>
> Yes, I'm not sure either. I have found a few env variables that use
> single letters (autoload, flashchecksum) and noticed that quite a few
> commands only decode as much of their subcommand as they need to be
> unique. I am happier with y and n than o!

The 'o' indeed kills this.  I don't think this is a good idea.

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
Operating-system software is the program that  orchestrates  all  the
basic functions of a computer.
- The Wall Street Journal, Tuesday, September 15, 1987, page 40