[PATCHv5 09/13] net/lwip: implement lwip port to u-boot
Maxim Uvarov
maxim.uvarov at linaro.org
Tue Aug 8 12:07:04 CEST 2023
On Thu, 3 Aug 2023 at 22:21, Maxim Uvarov <maxim.uvarov at linaro.org> wrote:
>
>
> On Thu, 3 Aug 2023 at 03:32, Simon Glass <sjg at google.com> wrote:
>
>> Hi Maxim,
>>
>> On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov <maxim.uvarov at linaro.org>
>> wrote:
>> >
>>
>> subject: U-Boot
>>
>> commit message please (throughout series)
>>
>> > Signed-off-by: Maxim Uvarov <maxim.uvarov at linaro.org>
>> > ---
>> > 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. <maxim.uvarov at linaro.org>
>> > + */
>> > +
>> > +#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. <maxim.uvarov at linaro.org>
>> > + */
>> > +
>> > +#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. <maxim.uvarov at linaro.org>
>> > + */
>> > +
>> > +#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. <maxim.uvarov at linaro.org>
>> > + */
>> > +
>> > +#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
>>
>
More information about the U-Boot
mailing list