Hi Maxim,
On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
subject: U-Boot
commit message please (throughout series)
> Signed-off-by: Maxim Uvarov <maxim.uvarov@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.
>
> 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@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.
> +
> +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.
> + */
> +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 = "">
> +
> +#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)
> +#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
> + 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 ()
> + 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@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?
> +{
> + 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@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@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