From aa7a55b6ddab33d4c876b14124a16dc72afce539 Mon Sep 17 00:00:00 2001 From: Adam Green Date: Tue, 13 Aug 2013 20:33:49 -0700 Subject: [PATCH 1/6] Update LPC1768.ld linker script to work with net stack. The original script assigned memory ranges to USB_RAM and ETH_RAM but it never placed any section data in those regions. I added clauses towards the bottom of the script to place data that the programmer has marked for the AHBSRAM0 and AHBSRAM1 sections into these regions of RAM. Previously the data destined for these sections was being placed in the lower 32K RAM bank and overflowing it at link time. I also added a few Image$$ linker symbols to mimic those used by the online compiler. I have had samples in the past which took advantage of these to display static memory statistics for each SRAM region. I also changed LENGTH=0x7F38 to LENGTH=(32K - 0xC8) to make it more consistent with the sizing of the other regions in this script which use human readable K sizing information. The 0xC8 subtraction reflects the starting offset of 0xC8 for this region. --- .../TOOLCHAIN_GCC_ARM/LPC1768.ld | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/libraries/mbed/targets/cmsis/TARGET_NXP/TARGET_LPC176X/TOOLCHAIN_GCC_ARM/LPC1768.ld b/libraries/mbed/targets/cmsis/TARGET_NXP/TARGET_LPC176X/TOOLCHAIN_GCC_ARM/LPC1768.ld index 9ad0af0be0..c4d3447dab 100644 --- a/libraries/mbed/targets/cmsis/TARGET_NXP/TARGET_LPC176X/TOOLCHAIN_GCC_ARM/LPC1768.ld +++ b/libraries/mbed/targets/cmsis/TARGET_NXP/TARGET_LPC176X/TOOLCHAIN_GCC_ARM/LPC1768.ld @@ -4,7 +4,7 @@ MEMORY { FLASH (rx) : ORIGIN = 0x00000000, LENGTH = 512K - RAM (rwx) : ORIGIN = 0x100000C8, LENGTH = 0x7F38 + RAM (rwx) : ORIGIN = 0x100000C8, LENGTH = (32K - 0xC8) USB_RAM(rwx) : ORIGIN = 0x2007C000, LENGTH = 16K ETH_RAM(rwx) : ORIGIN = 0x20080000, LENGTH = 16K @@ -84,6 +84,7 @@ SECTIONS .data : AT (__etext) { __data_start__ = .; + Image$$RW_IRAM1$$Base = .; *(vtable) *(.data*) @@ -120,6 +121,7 @@ SECTIONS *(.bss*) *(COMMON) __bss_end__ = .; + Image$$RW_IRAM1$$ZI$$Limit = . ; } > RAM .heap : @@ -146,4 +148,23 @@ SECTIONS /* Check if data + heap + stack exceeds RAM limit */ ASSERT(__StackLimit >= __HeapLimit, "region RAM overflowed with stack") + + + /* Code can explicitly ask for data to be + placed in these higher RAM banks where + they will be left uninitialized. + */ + .AHBSRAM0 (NOLOAD): + { + Image$$RW_IRAM2$$Base = . ; + *(AHBSRAM0) + Image$$RW_IRAM2$$ZI$$Limit = .; + } > USB_RAM + + .AHBSRAM1 (NOLOAD): + { + Image$$RW_IRAM3$$Base = . ; + *(AHBSRAM1) + Image$$RW_IRAM3$$ZI$$Limit = .; + } > ETH_RAM } From e014b41377e628af7d6460c77a65e27ad56ad754 Mon Sep 17 00:00:00 2001 From: Adam Green Date: Tue, 13 Aug 2013 20:50:02 -0700 Subject: [PATCH 2/6] Silence signed/unsigned comparison warnings in GCC The dn variable in lpc_low_level_output() was originally defined as a u32_t but it is later compared to the s32_t return value from lpc_tx_ready(). Since it is intialized to pbuf_clean() which returns a u8_t, a s32_t type can safely hold the initial value and remains consistent with the signed lpc_tx_ready() comparison. I also modifed writtenLen in TCPSocketConnection::send_all() and readLen in TCPSocketConnection::recieve_all() to be of type int instead of size_t. This is more consistent with their usage within these methods (they accumulate int ret values and are compared to the int length value) and their use as a signed integer return values. --- libraries/net/eth/lwip-eth/arch/lpc17_emac.c | 6 +++--- libraries/net/lwip/Socket/TCPSocketConnection.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/libraries/net/eth/lwip-eth/arch/lpc17_emac.c b/libraries/net/eth/lwip-eth/arch/lpc17_emac.c index 0d06464ab7..06772f48f1 100644 --- a/libraries/net/eth/lwip-eth/arch/lpc17_emac.c +++ b/libraries/net/eth/lwip-eth/arch/lpc17_emac.c @@ -618,14 +618,14 @@ static err_t lpc_low_level_output(struct netif *netif, struct pbuf *p) struct lpc_enetdata *lpc_enetif = netif->state; struct pbuf *q; u8_t *dst; - u32_t idx; + u32_t idx, notdmasafe = 0; struct pbuf *np; - u32_t dn, notdmasafe = 0; + s32_t dn; /* Zero-copy TX buffers may be fragmented across mutliple payload chains. Determine the number of descriptors needed for the transfer. The pbuf chaining can be a mess! */ - dn = (u32_t) pbuf_clen(p); + dn = (s32_t) pbuf_clen(p); /* Test to make sure packet addresses are DMA safe. A DMA safe address is once that uses external memory or periphheral RAM. diff --git a/libraries/net/lwip/Socket/TCPSocketConnection.cpp b/libraries/net/lwip/Socket/TCPSocketConnection.cpp index d84dc9c6b3..b853304099 100644 --- a/libraries/net/lwip/Socket/TCPSocketConnection.cpp +++ b/libraries/net/lwip/Socket/TCPSocketConnection.cpp @@ -66,7 +66,7 @@ int TCPSocketConnection::send_all(char* data, int length) { if ((_sock_fd < 0) || !_is_connected) return -1; - size_t writtenLen = 0; + int writtenLen = 0; TimeInterval timeout(_timeout); while (writtenLen < length) { if (!_blocking) { @@ -110,7 +110,7 @@ int TCPSocketConnection::receive_all(char* data, int length) { if ((_sock_fd < 0) || !_is_connected) return -1; - size_t readLen = 0; + int readLen = 0; TimeInterval timeout(_timeout); while (readLen < length) { if (!_blocking) { From 6e95a5ecdeefb538aa0a1e92ff550fd832bf6711 Mon Sep 17 00:00:00 2001 From: Adam Green Date: Tue, 13 Aug 2013 21:03:23 -0700 Subject: [PATCH 3/6] Removed extra ALIGNED macro define --- libraries/net/lwip/lwip-sys/arch/cc.h | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/net/lwip/lwip-sys/arch/cc.h b/libraries/net/lwip/lwip-sys/arch/cc.h index f9b8e22385..394635a612 100644 --- a/libraries/net/lwip/lwip-sys/arch/cc.h +++ b/libraries/net/lwip/lwip-sys/arch/cc.h @@ -80,7 +80,6 @@ typedef uintptr_t mem_ptr_t; #define PACK_STRUCT_END #define PACK_STRUCT_FIELD(fld) fld #define ALIGNED(n) __attribute__((aligned (n))) - #define ALIGNED(n) __align(n) #endif /* Used with IP headers only */ From 1cf5243206862342ee4a2afbe8810f83718d1b66 Mon Sep 17 00:00:00 2001 From: Adam Green Date: Tue, 13 Aug 2013 21:06:04 -0700 Subject: [PATCH 4/6] Don't call ip_addr_isany() on objects which can't be NULL. GCC will issue a warning when the ip_addr_isany() macro is used on a pointer which can never be NULL since the macros NULL check will always be false: #define ip_addr_isany(addr1) ((addr1) == NULL || \ (addr1)->addr == IPADDR_ANY) In these cases, it is probably clearer to just perform the x.addr == IPADDR_ANY check inline. --- libraries/net/lwip/lwip/core/ipv4/ip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/net/lwip/lwip/core/ipv4/ip.c b/libraries/net/lwip/lwip/core/ipv4/ip.c index 6f248716b6..56843273d1 100644 --- a/libraries/net/lwip/lwip/core/ipv4/ip.c +++ b/libraries/net/lwip/lwip/core/ipv4/ip.c @@ -400,7 +400,7 @@ ip_input(struct pbuf *p, struct netif *inp) /* broadcast or multicast packet source address? Compliant with RFC 1122: 3.2.1.3 */ #if IP_ACCEPT_LINK_LAYER_ADDRESSING /* DHCP servers need 0.0.0.0 to be allowed as source address (RFC 1.1.2.2: 3.2.1.3/a) */ - if (check_ip_src && !ip_addr_isany(¤t_iphdr_src)) + if (check_ip_src && current_iphdr_src.addr != IPADDR_ANY) #endif /* IP_ACCEPT_LINK_LAYER_ADDRESSING */ { if ((ip_addr_isbroadcast(¤t_iphdr_src, inp)) || (ip_addr_ismulticast(¤t_iphdr_src))) { From 962dd8bd8fc37958d2648a6e5067937679cd1c17 Mon Sep 17 00:00:00 2001 From: Adam Green Date: Tue, 13 Aug 2013 21:17:25 -0700 Subject: [PATCH 5/6] Silence GCC warnings in dhcp.c The first was a potential out of range index read in dhcp_handle_ack(). The (n < DNS_MAX_SERVERS) check should occur first. There is also a documented lwIP bug for this issue here: http://savannah.nongnu.org/bugs/?36170 In dhcp_bind() there is no need to perform the NULL check in ip_addr_isany() for &gw_addr. Just check (gw_addr.addr == IPADDR_ANY) instead. I refactored the chaddr[] copy in dhcp_create_msg() to first copy all of the valid bytes in hwaddr and then pad the rest of the bytes with 0. Before it used to check on every destination byte if it should copy or pad. GCC originally complained about an index out of range read from the hwaddr[] array even though it was protected by a conditional operator. The refactor makes the intent a bit clearer and saves the extra comparison per loop iteration. It also stops GCC from complaining :) --- libraries/net/lwip/lwip/core/dhcp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/libraries/net/lwip/lwip/core/dhcp.c b/libraries/net/lwip/lwip/core/dhcp.c index 81b4be271e..03c0c62384 100644 --- a/libraries/net/lwip/lwip/core/dhcp.c +++ b/libraries/net/lwip/lwip/core/dhcp.c @@ -564,7 +564,7 @@ dhcp_handle_ack(struct netif *netif) #if LWIP_DNS /* DNS servers */ n = 0; - while(dhcp_option_given(dhcp, DHCP_OPTION_IDX_DNS_SERVER + n) && (n < DNS_MAX_SERVERS)) { + while((n < DNS_MAX_SERVERS) && dhcp_option_given(dhcp, DHCP_OPTION_IDX_DNS_SERVER + n)) { ip_addr_t dns_addr; ip4_addr_set_u32(&dns_addr, htonl(dhcp_get_option_value(dhcp, DHCP_OPTION_IDX_DNS_SERVER + n))); dns_setserver(n, &dns_addr); @@ -975,7 +975,7 @@ dhcp_bind(struct netif *netif) ip_addr_copy(gw_addr, dhcp->offered_gw_addr); /* gateway address not given? */ - if (ip_addr_isany(&gw_addr)) { + if (gw_addr.addr == IPADDR_ANY) { /* copy network address */ ip_addr_get_network(&gw_addr, &dhcp->offered_ip_addr, &sn_mask); /* use first host address on network as gateway */ @@ -1678,9 +1678,13 @@ dhcp_create_msg(struct netif *netif, struct dhcp *dhcp, u8_t message_type) ip_addr_set_zero(&dhcp->msg_out->yiaddr); ip_addr_set_zero(&dhcp->msg_out->siaddr); ip_addr_set_zero(&dhcp->msg_out->giaddr); - for (i = 0; i < DHCP_CHADDR_LEN; i++) { - /* copy netif hardware address, pad with zeroes */ - dhcp->msg_out->chaddr[i] = (i < netif->hwaddr_len) ? netif->hwaddr[i] : 0/* pad byte*/; + for (i = 0; i < netif->hwaddr_len; i++) { + /* copy netif hardware address */ + dhcp->msg_out->chaddr[i] = netif->hwaddr[i]; + } + for ( ; i < DHCP_CHADDR_LEN; i++) { + /* ... pad rest with zeroes */ + dhcp->msg_out->chaddr[i] = 0; } for (i = 0; i < DHCP_SNAME_LEN; i++) { dhcp->msg_out->sname[i] = 0; From 282c354ba733d80307fe9ecfde50320dec31bd7e Mon Sep 17 00:00:00 2001 From: Adam Green Date: Tue, 13 Aug 2013 22:38:48 -0700 Subject: [PATCH 6/6] The value of 2 can't fit in a 1 bit wide field. The phy_speed_100mbs, phy_full_duplex, and phy_link_active fields of PHY_STATUS_TYPE are 1 bit wide but lpc_phy_init() attempted to initialize them to a value of 2. I switched the initializations to be 0 instead and it still generated the same .bin image. --- libraries/net/eth/lwip-eth/arch/lpc_phy_dp83848.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/net/eth/lwip-eth/arch/lpc_phy_dp83848.c b/libraries/net/eth/lwip-eth/arch/lpc_phy_dp83848.c index 76ffc0cf9a..eca6d97338 100644 --- a/libraries/net/eth/lwip-eth/arch/lpc_phy_dp83848.c +++ b/libraries/net/eth/lwip-eth/arch/lpc_phy_dp83848.c @@ -228,9 +228,9 @@ err_t lpc_phy_init(struct netif *netif, int rmii) u32_t tmp; s32_t i; - physts.phy_speed_100mbs = olddphysts.phy_speed_100mbs = 2; - physts.phy_full_duplex = olddphysts.phy_full_duplex = 2; - physts.phy_link_active = olddphysts.phy_link_active = 2; + physts.phy_speed_100mbs = olddphysts.phy_speed_100mbs = 0; + physts.phy_full_duplex = olddphysts.phy_full_duplex = 0; + physts.phy_link_active = olddphysts.phy_link_active = 0; phyustate = 0; /* Only first read and write are checked for failure */