On Thu, 3 Aug 2023 at 22:21, Maxim Uvarov <[email protected]> wrote:
> > > On Thu, 3 Aug 2023 at 03:32, Simon Glass <[email protected]> wrote: > >> Hi Maxim, >> >> On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov <[email protected]> >> wrote: >> > >> >> subject: U-Boot >> >> commit message please (throughout series) >> >> > Signed-off-by: Maxim Uvarov <[email protected]> >> > --- >> > lib/lwip/port/if.c | 256 ++++++++++++++++++++++++++ >> > lib/lwip/port/include/arch/cc.h | 46 +++++ >> > lib/lwip/port/include/arch/sys_arch.h | 59 ++++++ >> > lib/lwip/port/include/limits.h | 0 >> > lib/lwip/port/sys-arch.c | 20 ++ >> > 5 files changed, 381 insertions(+) >> > create mode 100644 lib/lwip/port/if.c >> > create mode 100644 lib/lwip/port/include/arch/cc.h >> > create mode 100644 lib/lwip/port/include/arch/sys_arch.h >> > create mode 100644 lib/lwip/port/include/limits.h >> > create mode 100644 lib/lwip/port/sys-arch.c >> >> I wonder if this port/ directory should go away and this code should >> be in net/ ? It feels a bit odd, as lib/ code suggests it is for >> libraries, not the integration with them. >> >> In all lwIP examples I was port/ directory with specific files for the platform. And here I also planned to follow the same rule. And yes, I think it's better to move all this code to net/ and cmd/. > > >> > diff --git a/lib/lwip/port/if.c b/lib/lwip/port/if.c >> > new file mode 100644 >> > index 0000000000..2ed59a0c4e >> > --- /dev/null >> > +++ b/lib/lwip/port/if.c >> > @@ -0,0 +1,256 @@ >> > +// SPDX-License-Identifier: GPL-2.0 >> > + >> > +/* >> > + * (C) Copyright 2023 Linaro Ltd. <[email protected]> >> > + */ >> > + >> > +#include <common.h> >> > +#include <command.h> >> > +extern int eth_init(void); /* net.h */ >> > +extern void string_to_enetaddr(const char *addr, uint8_t *enetaddr); >> /* net.h */ >> > +extern struct in_addr net_ip; >> > +extern u8 net_ethaddr[6]; >> >> Can that go in a header file? >> > > I tried to do as minimal changes as I could. In general these are > definitions from include/net.h. > And the problem that net.h has not only ethernet things, but also protocol > defines which overlaps > with lwip protocol defines and data structures. More clean fix will be to > clean up net.h and move > ip to net/ip.h udp to net/udp.h and so on. So here we can include <net.h> > to get eth_init() and > friends accessed. > > >> >> > + >> > +#include "lwip/debug.h" >> > +#include "lwip/arch.h" >> > +#include "netif/etharp.h" >> > +#include "lwip/stats.h" >> > +#include "lwip/def.h" >> > +#include "lwip/mem.h" >> > +#include "lwip/pbuf.h" >> > +#include "lwip/sys.h" >> > +#include "lwip/netif.h" >> > + >> > +#include "lwip/ip.h" >> > + >> > +#define IFNAME0 'e' >> > +#define IFNAME1 '0' >> > + >> > +static struct pbuf *low_level_input(struct netif *netif); >> > +static int uboot_net_use_lwip; >> >> Can we put this stuff in the UCLASS_ETH private data instead of using >> statics? They require BSS which is typically not available in SPL >> builds. >> >> > yes, that will work. I expect this flag to be used for compatibility mode. > I.e. if it's set before cmd then lwip controls net_loop(), if not set then > an original code controls net_loop(). I can even add an argument to > eth_init(). But because there are too many eth_init() calls I think I will > add an entry to uclass. > moved to UCLASS_ETH with split net.h on net.h, eth.h and arp.h. And I like how it looks now, thanks! > > >> > + >> > +int ulwip_enabled(void) >> > +{ >> > + return uboot_net_use_lwip; >> > +} >> > + >> > +/* 1 - in loop >> > + * 0 - no loop >> > + */ >> > +static int loop_lwip; >> > + >> > +/* ret 1 - in loop >> > + * 0 - no loop >> >> ?? >> >> This all needs some more detail in the comments >> > > Yes, maybe with UCLASS_ETH some of that will go away. > > Yes, that goes away after moving. > >> > + */ >> > +int ulwip_in_loop(void) >> > +{ >> > + return loop_lwip; >> > +} >> > + >> > +void ulwip_loop_set(int loop) >> > +{ >> > + loop_lwip = loop; >> > +} >> > + >> > +static int ulwip_app_err; >> > + >> > +void ulwip_exit(int err) >> > +{ >> > + ulwip_app_err = err; >> > + ulwip_loop_set(0); >> > +} >> > + >> > +int ulwip_app_get_err(void) >> > +{ >> > + return ulwip_app_err; >> > +} >> > + >> > +struct uboot_lwip_if { >> > +}; >> > + >> > +static struct netif uboot_netif; >> > + >> > +#define LWIP_PORT_INIT_NETMASK(addr) IP4_ADDR((addr), 255, 255, 255, >> 0) >> > + >> > +extern uchar *net_rx_packet; >> > +extern int net_rx_packet_len; >> >> header file please >> >> > ok, the same thing here, it's from net.h > > >> > + >> > +int uboot_lwip_poll(void) >> > +{ >> > + struct pbuf *p; >> > + int err; >> > + >> > + p = low_level_input(&uboot_netif); >> > + if (!p) { >> > + printf("error p = low_level_input = NULL\n"); >> > + return 0; >> > + } >> > + >> > + err = ethernet_input(p, &uboot_netif); >> > + if (err) >> > + printf("ip4_input err %d\n", err); >> >> log_err() ? >> >> But what is going on here? Just return the error code, rather than >> suppressing it, then the caller can handle it. >> >> > Looked more detail - current version ethernet_input() (lwip master) always > returns ERR_OK (0). When I wrote I added a return code check for non > function. > So I expect that it can be considered as a bug if some time later we > receive something non 0. > > But in general the poll() function has to be void. I will correct it. > > > + >> > + return 0; >> > +} >> > + >> > +static struct pbuf *low_level_input(struct netif *netif) >> > +{ >> > + struct pbuf *p, *q; >> > + u16_t len = net_rx_packet_len; >> > + uchar *data = net_rx_packet; >> > + >> > +#if ETH_PAD_SIZE >> > + len += ETH_PAD_SIZE; /* allow room for Ethernet padding */ >> >> Please find a way to drop any use of #if >> >> This can be defined to 0 if not needed, for example. Or you could have >> a static inline eth_pad_size() in the header file (where #if is >> permitted) >> >> let's drop for n > > +#endif >> > + >> > + /* We allocate a pbuf chain of pbufs from the pool. */ >> > + p = pbuf_alloc(PBUF_RAW, len, PBUF_POOL); >> > + if (p) { >> > +#if ETH_PAD_SIZE >> > + pbuf_remove_header(p, ETH_PAD_SIZE); /* drop the >> padding word */ >> > +#endif >> > + /* We iterate over the pbuf chain until we have read >> the entire >> > + * packet into the pbuf. >> > + */ >> > + for (q = p; q != NULL; q = q->next) { >> > + /* Read enough bytes to fill this pbuf in the >> chain. The >> >> Comment style >> >> /* >> * Read >> >> > + * available data in the pbuf is given by the >> q->len >> > + * variable. >> > + * This does not necessarily have to be a >> memcpy, you can also preallocate >> > + * pbufs for a DMA-enabled MAC and after >> receiving truncate it to the >> > + * actually received size. In this case, ensure >> the tot_len member of the >> > + * pbuf is the sum of the chained pbuf len >> members. >> > + */ >> > + MEMCPY(q->payload, data, q->len); >> > + data += q->len; >> > + } >> > + //acknowledge that packet has been read(); >> >> Space after // (please fix throughout) >> >> > + >> > +#if ETH_PAD_SIZE >> > + pbuf_add_header(p, ETH_PAD_SIZE); /* reclaim the >> padding word */ >> > +#endif >> > + LINK_STATS_INC(link.recv); >> > + } else { >> > + //drop packet(); >> > + LINK_STATS_INC(link.memerr); >> > + LINK_STATS_INC(link.drop); >> > + } >> > + >> > + return p; >> > +} >> > + >> > +static int ethernetif_input(struct pbuf *p, struct netif *netif) >> > +{ >> > + struct ethernetif *ethernetif; >> > + >> > + ethernetif = netif->state; >> > + >> > + /* move received packet into a new pbuf */ >> > + p = low_level_input(netif); >> > + >> > + /* if no packet could be read, silently ignore this */ >> > + if (p) { >> > + /* pass all packets to ethernet_input, which decides >> what packets it supports */ >> > + if (netif->input(p, netif) != ERR_OK) { >> > + LWIP_DEBUGF(NETIF_DEBUG, ("%s: IP input >> error\n", __func__)); >> > + pbuf_free(p); >> > + p = NULL; >> > + } >> > + } >> >> blank line before final return >> >> > + return 0; >> > +} >> > + >> > +static err_t low_level_output(struct netif *netif, struct pbuf *p) >> > +{ >> > + int err; >> > + >> > + err = eth_send(p->payload, p->len); >> > + if (err != 0) { >> >> if (err) >> >> > + printf("eth_send error %d\n", err); >> > + return ERR_ABRT; >> > + } >> > + return ERR_OK; >> > +} >> > + >> > +err_t uboot_lwip_if_init(struct netif *netif) >> > +{ >> > + struct uboot_lwip_if *uif = (struct uboot_lwip_if >> *)malloc(sizeof(struct uboot_lwip_if)); >> > + >> > + if (!uif) { >> > + printf("uboot_lwip_if: out of memory\n"); >> > + return ERR_MEM; >> > + } >> > + netif->state = uif; >> > + >> > + netif->name[0] = IFNAME0; >> > + netif->name[1] = IFNAME1; >> > + >> > + netif->hwaddr_len = ETHARP_HWADDR_LEN; >> > + string_to_enetaddr(env_get("ethaddr"), netif->hwaddr); >> > +#if defined(CONFIG_LWIP_LIB_DEBUG) >> > + printf(" MAC: %02X:%02X:%02X:%02X:%02X:%02X\n", >> > + netif->hwaddr[0], netif->hwaddr[1], netif->hwaddr[2], >> > + netif->hwaddr[3], netif->hwaddr[4], netif->hwaddr[5]); >> > +#endif >> > + >> > +#if LWIP_IPV4 >> > + netif->output = etharp_output; >> > +#endif /* LWIP_IPV4 */ >> > +#if LWIP_IPV6 >> > + netif->output_ip6 = ethip6_output; >> > +#endif /* LWIP_IPV6 */ >> >> You may need to add accessors in the header file (as global_data.h) so >> you don't need the #if >> >> I did not understand the comment about global_data.h. lwiIP as I understand can work in 3 modes 1. ipv4 2.ipv6 3. ipv4+ipv6. If we want optimization for size I think we need to pass this to Kconfig because lwip has some structs under ifdefs. > > + netif->linkoutput = low_level_output; >> > + netif->mtu = 1500; >> > + netif->flags = NETIF_FLAG_BROADCAST | NETIF_FLAG_ETHARP | >> NETIF_FLAG_LINK_UP; >> > + >> > + eth_init(); /* activate u-boot eth dev */ >> > + >> > + if (IS_ENABLED(CONFIG_LWIP_LIB_DEBUG)) { >> > + printf("Initialized LWIP stack\n"); >> > + } >> > + >> > + return ERR_OK; >> > +} >> > + >> > +int uboot_lwip_init(void) >> > +{ >> > + ip4_addr_t ipaddr, netmask, gw; >> > + >> > + if (uboot_net_use_lwip) >> > + return CMD_RET_SUCCESS; >> > + >> > + ip4_addr_set_zero(&gw); >> > + ip4_addr_set_zero(&ipaddr); >> > + ip4_addr_set_zero(&netmask); >> > + >> > + ipaddr_aton(env_get("ipaddr"), &ipaddr); >> > + ipaddr_aton(env_get("ipaddr"), &netmask); >> > + >> > + LWIP_PORT_INIT_NETMASK(&netmask); >> > + if (IS_ENABLED(CONFIG_LWIP_LIB_DEBUG)) { >> > + printf("Starting lwIP, IP: %s\n", >> ip4addr_ntoa(&ipaddr)); >> > + printf(" GW: %s\n", ip4addr_ntoa(&gw)); >> > + printf(" mask: %s\n", >> ip4addr_ntoa(&netmask)); >> > + } >> > + >> > + if (!netif_add(&uboot_netif, &ipaddr, &netmask, &gw, >> > + &uboot_netif, uboot_lwip_if_init, >> ethernetif_input)) >> > + printf("err: netif_add failed!\n"); >> > + >> > + netif_set_up(&uboot_netif); >> > + netif_set_link_up(&uboot_netif); >> > +#if LWIP_IPV6 >> >> if () >> >> No, we are using lwip header files. it will not work if LWIP_IPV6 set to 0. > > + netif_create_ip6_linklocal_address(&uboot_netif, 1); >> > + printf(" IPv6: %s\n", >> ip6addr_ntoa(netif_ip6_addr(uboot_netif, 0))); >> > +#endif /* LWIP_IPV6 */ >> > + >> > + uboot_net_use_lwip = 1; >> > + >> > + return CMD_RET_SUCCESS; >> > +} >> > + >> > +/* placeholder, not used now */ >> > +void uboot_lwip_destroy(void) >> > +{ >> > + uboot_net_use_lwip = 0; >> > +} >> > diff --git a/lib/lwip/port/include/arch/cc.h >> b/lib/lwip/port/include/arch/cc.h >> > new file mode 100644 >> > index 0000000000..db30d7614e >> > --- /dev/null >> > +++ b/lib/lwip/port/include/arch/cc.h >> > @@ -0,0 +1,46 @@ >> > +/* SPDX-License-Identifier: GPL-2.0 */ >> > + >> > +/* >> >> It would help to have a little one-line comment as to what these files >> are for. >> >> > + * (C) Copyright 2023 Linaro Ltd. <[email protected]> >> > + */ >> > + >> > +#ifndef LWIP_ARCH_CC_H >> > +#define LWIP_ARCH_CC_H >> > + >> > +#include <linux/types.h> >> > +#include <linux/kernel.h> >> > + >> > +#define LWIP_ERRNO_INCLUDE <errno.h> >> > + >> > +#define LWIP_ERRNO_STDINCLUDE 1 >> > +#define LWIP_NO_UNISTD_H 1 >> > +#define LWIP_TIMEVAL_PRIVATE 1 >> > + >> > +extern unsigned int lwip_port_rand(void); >> > +#define LWIP_RAND() (lwip_port_rand()) >> > + >> > +/* different handling for unit test, normally not needed */ >> > +#ifdef LWIP_NOASSERT_ON_ERROR >> > +#define LWIP_ERROR(message, expression, handler) do { if >> (!(expression)) { \ >> > + handler; }} while >> (0) >> > +#endif >> > + >> > +#define LWIP_DONT_PROVIDE_BYTEORDER_FUNCTIONS >> > + >> > +#define LWIP_PLATFORM_ASSERT(x) do {printf("Assertion \"%s\" failed at >> line %d in %s\n", \ >> > + x, __LINE__, __FILE__); } while (0) >> > + >> > +static inline int atoi(const char *str) >> >> Can we use U-Boot's version? >> >> #include <stdlib.h> /* getenv, atoi */ compiles but generates error on linking. I guess there is no atoi in U-Boot. > > +{ >> > + int r = 0; >> > + int i; >> > + >> > + for (i = 0; str[i] != '\0'; ++i) >> > + r = r * 10 + str[i] - '0'; >> > + >> > + return r; >> > +} >> > + >> > +#define LWIP_ERR_T int >> > + >> > +#endif /* LWIP_ARCH_CC_H */ >> > diff --git a/lib/lwip/port/include/arch/sys_arch.h >> b/lib/lwip/port/include/arch/sys_arch.h >> > new file mode 100644 >> > index 0000000000..8d95146275 >> > --- /dev/null >> > +++ b/lib/lwip/port/include/arch/sys_arch.h >> > @@ -0,0 +1,59 @@ >> > +/* SPDX-License-Identifier: GPL-2.0 */ >> > + >> > +/* >> > + * (C) Copyright 2023 Linaro Ltd. <[email protected]> >> > + */ >> > + >> > +#ifndef LWIP_ARCH_SYS_ARCH_H >> > +#define LWIP_ARCH_SYS_ARCH_H >> > + >> > +#include "lwip/opt.h" >> > +#include "lwip/arch.h" >> > +#include "lwip/err.h" >> > + >> > +#define ERR_NEED_SCHED 123 >> > + >> > +void sys_arch_msleep(u32_t delay_ms); >> > +#define sys_msleep(ms) sys_arch_msleep(ms) >> > + >> > +#if SYS_LIGHTWEIGHT_PROT >> > +typedef u32_t sys_prot_t; >> > +#endif /* SYS_LIGHTWEIGHT_PROT */ >> > + >> > +#include <errno.h> >> > + >> > +#define SYS_MBOX_NULL NULL >> > +#define SYS_SEM_NULL NULL >> > + >> > +typedef u32_t sys_prot_t; >> > + >> > +struct sys_sem; >> > +typedef struct sys_sem *sys_sem_t; >> > +#define sys_sem_valid(sem) (((sem) != NULL) && (*(sem) != NULL)) >> > +#define sys_sem_set_invalid(sem) do { if ((sem) != NULL) { *(sem) = >> NULL; }} while (0) >> > + >> > +/* let sys.h use binary semaphores for mutexes */ >> > +#define LWIP_COMPAT_MUTEX 1 >> > +#define LWIP_COMPAT_MUTEX_ALLOWED 1 >> > + >> > +struct sys_mbox; >> > +typedef struct sys_mbox *sys_mbox_t; >> > +#define sys_mbox_valid(mbox) (((mbox) != NULL) && (*(mbox) != NULL)) >> > +#define sys_mbox_set_invalid(mbox) do { if ((mbox) != NULL) { *(mbox) >> = NULL; }} while (0) >> > + >> > +struct sys_thread; >> > +typedef struct sys_thread *sys_thread_t; >> > + >> > +static inline u32_t sys_arch_sem_wait(sys_sem_t *sem, u32_t timeout) >> > +{ >> > + return 0; >> > +}; >> > + >> > +static inline err_t sys_mbox_trypost(sys_mbox_t *mbox, void *msg) >> > +{ >> > + return 0; >> > +}; >> > + >> > +#define sys_sem_signal(s) >> > + >> > +#endif /* LWIP_ARCH_SYS_ARCH_H */ >> > diff --git a/lib/lwip/port/include/limits.h >> b/lib/lwip/port/include/limits.h >> > new file mode 100644 >> > index 0000000000..e69de29bb2 >> > diff --git a/lib/lwip/port/sys-arch.c b/lib/lwip/port/sys-arch.c >> > new file mode 100644 >> > index 0000000000..609eeccf8c >> > --- /dev/null >> > +++ b/lib/lwip/port/sys-arch.c >> > @@ -0,0 +1,20 @@ >> > +// SPDX-License-Identifier: GPL-2.0 >> > + >> > +/* >> > + * (C) Copyright 2023 Linaro Ltd. <[email protected]> >> > + */ >> > + >> > +#include <common.h> >> > +#include <rand.h> >> > +#include "lwip/opt.h" >> > + >> > +u32_t sys_now(void) >> > +{ >> > + return get_timer(0); >> > +} >> > + >> > +u32_t lwip_port_rand(void) >> > +{ >> > + return (u32_t)rand(); >> > +} >> > + >> > -- >> > 2.30.2 >> > >> >> >> Regards, >> Simon >> >
