| Message ID | 20250511180207.42945-2-verdun@hpe.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | *** Initial Wiznet W5500 support *** | expand |
Hi, Thanks for fixing it ! On 11/05/2025 20:02, verdun@hpe.com wrote: > From: Jean-Marie Verdun <verdun@hpe.com> > > Add support for the Wiznet W5500 spi to ethernet controller > > Signed-off-by: Jean-Marie Verdun <verdun@hpe.com> > --- > drivers/net/Kconfig | 9 + > drivers/net/Makefile | 1 + > drivers/net/w5500.c | 584 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 594 insertions(+) > create mode 100644 drivers/net/w5500.c > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > index 950ed0f25a9..74668f477ae 100644 > --- a/drivers/net/Kconfig > +++ b/drivers/net/Kconfig > @@ -19,6 +19,15 @@ config SPL_DM_ETH > depends on SPL_NET > def_bool y > > +config W5500 > + bool "W5500 device driver" > + depends on SPI > + help > + Enable w5500 driver > + > + Adds support for Wiznet W5500 device. The device must be attached > + to a SPI bus. > + > config DM_MDIO > bool "Enable Driver Model for MDIO devices" > depends on PHYLIB > diff --git a/drivers/net/Makefile b/drivers/net/Makefile > index 67bba3a8536..6d85c38d869 100644 > --- a/drivers/net/Makefile > +++ b/drivers/net/Makefile > @@ -103,6 +103,7 @@ obj-$(CONFIG_SUN8I_EMAC) += sun8i_emac.o > obj-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o > obj-$(CONFIG_TULIP) += dc2114x.o > obj-$(CONFIG_VSC7385_ENET) += vsc7385.o > +obj-$(CONFIG_W5500) += w5500.o > obj-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o > obj-$(CONFIG_XILINX_AXIMRMAC) += xilinx_axi_mrmac.o > obj-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o > diff --git a/drivers/net/w5500.c b/drivers/net/w5500.c > new file mode 100644 > index 00000000000..a15255bb457 > --- /dev/null > +++ b/drivers/net/w5500.c > @@ -0,0 +1,584 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright Hewlett Packard Enterprise Development LP. > + * > + * Jean-Marie Verdun <verdun@hpe.com> > + * > + * Inspired from the linux kernel driver from: > + * - Copyright (C) 2006-2008 WIZnet Co.,Ltd. > + * - Copyright (C) 2012 Mike Sinkovsky <msink@permonline.ru> > + * > + * available at > + * - https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/wiznet/w5100.c > + * > + * Datasheet: > + * - http://www.wiznet.co.kr/wp-content/uploads/wiznethome/Chip/W5100/Document/W5100_Datasheet_v1.2.6.pdf > + * - http://wiznethome.cafe24.com/wp-content/uploads/wiznethome/Chip/W5200/Documents/W5200_DS_V140E.pdf > + * - http://wizwiki.net/wiki/lib/exe/fetch.php?media=products:w5500:w5500_ds_v106e_141230.pdf > + * > + */ > + > +#include <dm.h> > +#include <log.h> > +#include <malloc.h> > +#include <spi.h> > +#include <net.h> > +#include <asm/global_data.h> > +#include <dm/device_compat.h> > +#include <linux/delay.h> > +#include <linux/iopoll.h> > +#include <net.h> > +#include <malloc.h> > + > +#define BUFFER_SZ 16384 > +#define MAX_PACKET_SZ 9000 > +#define W5100_SPI_WRITE_OPCODE 0xf0 > +#define W5100_SPI_READ_OPCODE 0x0f > +#define W5100_SHAR 0x0009 /* Source MAC address */ > +#define W5500_S0_REGS 0x10000 > +#define S0_REGS(priv) ((priv)->s0_regs) > +#define W5100_MR 0x0000 /* Mode Register */ > +#define MR_RST 0x80 /* S/W reset */ > +#define MR_PB 0x10 /* Ping block */ > + > +#define W5100_Sn_MR 0x0000 /* Sn Mode Register */ > +#define W5100_Sn_CR 0x0001 /* Sn Command Register */ > +#define W5100_Sn_IR 0x0002 /* Sn Interrupt Register */ > +#define W5100_Sn_SR 0x0003 /* Sn Status Register */ > +#define W5100_Sn_TX_FSR 0x0020 /* Sn Transmit free memory size */ > +#define W5100_Sn_TX_RD 0x0022 /* Sn Transmit memory read pointer */ > +#define W5100_Sn_TX_WR 0x0024 /* Sn Transmit memory write pointer */ > +#define W5100_Sn_RX_RSR 0x0026 /* Sn Receive free memory size */ > +#define W5100_Sn_RX_RD 0x0028 /* Sn Receive memory read pointer */ > + > +#define W5100_S0_MR(priv) (S0_REGS(priv) + W5100_Sn_MR) > + > +#define S0_MR_MACRAW 0x04 /* MAC RAW mode */ > +#define S0_MR_MF 0x40 /* MAC Filter for W5100 and W5200 */ > +#define W5500_S0_MR_MF 0x80 /* MAC Filter for W5500 */ > +#define W5100_S0_MR(priv) (S0_REGS(priv) + W5100_Sn_MR) > + > +#define S0_MR_MACRAW 0x04 /* MAC RAW mode */ > +#define S0_MR_MF 0x40 /* MAC Filter for W5100 and W5200 */ > +#define W5500_S0_MR_MF 0x80 /* MAC Filter for W5500 */ > + > +/* > + * W5100 and W5200 common registers about the same with the W5500 > + */ > +#define W5100_IMR 0x0016 /* Interrupt Mask Register */ > +#define IR_S0 0x01 /* S0 interrupt */ > +#define W5100_RTR 0x0017 /* Retry Time-value Register */ > +#define RTR_DEFAULT 2000 /* =0x07d0 (2000) */ > +#define W5500_SIMR 0x0018 /* Socket Interrupt Mask Register */ > +#define W5500_RTR 0x0019 /* Retry Time-value Register */ > + > +#define W5100_S0_CR(priv) (S0_REGS(priv) + W5100_Sn_CR) > +#define S0_CR_OPEN 0x01 /* OPEN command */ > +#define S0_CR_CLOSE 0x10 /* CLOSE command */ > +#define S0_CR_SEND 0x20 /* SEND command */ > +#define S0_CR_RECV 0x40 /* RECV command */ > +#define W5100_S0_IR(priv) (S0_REGS(priv) + W5100_Sn_IR) > +#define S0_IR_SENDOK 0x10 /* complete sending */ > +#define S0_IR_RECV 0x04 /* receiving data */ > +#define W5100_S0_SR(priv) (S0_REGS(priv) + W5100_Sn_SR) > +#define S0_SR_MACRAW 0x42 /* mac raw mode */ > +#define W5100_S0_TX_FSR(priv) (S0_REGS(priv) + W5100_Sn_TX_FSR) > +#define W5100_S0_TX_RD(priv) (S0_REGS(priv) + W5100_Sn_TX_RD) > +#define W5100_S0_TX_WR(priv) (S0_REGS(priv) + W5100_Sn_TX_WR) > +#define W5100_S0_RX_RSR(priv) (S0_REGS(priv) + W5100_Sn_RX_RSR) > +#define W5100_S0_RX_RD(priv) (S0_REGS(priv) + W5100_Sn_RX_RD) > + > +#define W5500_TX_MEM_START 0x20000 > +#define W5500_TX_MEM_SIZE 0x04000 > +#define W5500_RX_MEM_START 0x30000 > +#define W5500_RX_MEM_SIZE 0x04000 > + > +#define W5500_Sn_RXMEM_SIZE(n) \ > + (0x1001e + (n) * 0x40000) /* Sn RX Memory Size */ > +#define W5500_Sn_TXMEM_SIZE(n) \ > + (0x1001f + (n) * 0x40000) /* Sn TX Memory Size */ > + > +/** > + * struct eth_w5500_priv - memory for w5500 driver > + */ > +struct eth_w5500_priv { > + uchar host_hwaddr[ARP_HLEN]; > + bool disabled; > + uchar * recv_packet_buffer[PKTBUFSRX]; > + int recv_packet_length[PKTBUFSRX]; > + int recv_packets; > + const struct dm_spi_ops *spi_ops; > + struct udevice **spi_dev; > + u32 s0_regs; > + u32 s0_tx_buf; > + u16 s0_tx_buf_size; > + u32 s0_rx_buf; > + u16 s0_rx_buf_size; > + bool promisc; > + u32 msg_enable; > + u16 offset; > + void *priv; > +}; > + > +static int xfer(struct udevice *dev, void *dout, unsigned int bout, void *din, > + unsigned int bin) > +{ > + /* Ok the xfer function in uboot is symmetrical > + * (write and read operation happens at the same time) > + * w5500 requires bytes send followed by receive operation on the MISO line > + * So we need to create a buffer which contain bout+bin bytes and pass the various > + * pointer to the xfer function > + */ > + > + struct udevice *bus = dev_get_parent(dev); > + u8 buffin[BUFFER_SZ]; > + u8 buffout[BUFFER_SZ]; It's dangerous to allocate 2 16k buffers on the stack, move them global or allocate them at probe and store the pointers in eth_w5500_priv > + > + if ((bout + bin) < BUFFER_SZ) { > + for (int i = 0; i < bout; i++) { > + buffout[i] = ((u8 *)(dout))[i]; > + buffin[i] = 0; > + } > + for (int i = bout; i < (bin + bout); i++) { > + buffin[i] = 0; > + buffout[i] = 0; > + } > + if (!bus) > + return -1; > + dm_spi_xfer(dev, 8 * (bout + bin), buffout, buffin, > + SPI_XFER_BEGIN | SPI_XFER_END); > + for (int i = bout; i < (bin + bout); i++) > + ((u8 *)(din))[bin + bout - i - 1] = buffin[i]; > + } > + > + return 0; > +} > + > +static int w5500_spi_read(struct udevice *dev, u32 addr) > +{ > + u8 cmd[3]; > + int ret; > + u8 bank; > + u8 data; > + > + bank = (addr >> 16); > + > + if (bank > 0) > + bank = bank << 3; > + > + cmd[0] = addr >> 8; > + cmd[1] = addr & 0xff; > + cmd[2] = bank; > + > + ret = xfer(dev, cmd, sizeof(cmd), &data, 1); > + if (ret != 0) if (ret) > + return ret; > + > + return data; > +} > + > +static int w5500_spi_write(struct udevice *dev, u32 addr, u8 data) > +{ > + u8 cmd[4]; > + u8 bank; > + u8 din; > + > + bank = (addr >> 16); > + din = 0; > + > + if (bank > 0) > + bank = bank << 3; > + > + cmd[0] = (addr >> 8) & 0xff; > + cmd[1] = addr & 0xff; > + cmd[2] = bank | 0x4; > + cmd[3] = data; > + > + return xfer(dev, cmd, sizeof(cmd), &din, 0); > +} > + > +static int w5500_spi_read16(struct udevice *dev, u32 addr) > +{ > + u8 cmd[3]; > + u16 data; > + int ret; > + u8 bank; > + > + bank = (addr >> 16); > + > + if (bank > 0) > + bank = bank << 3; > + > + cmd[0] = addr >> 8; > + cmd[1] = addr & 0xff; > + cmd[2] = bank; > + > + ret = xfer(dev, cmd, sizeof(cmd), &data, 2); > + if (ret != 0) if (ret) > + return ret; > + > + return data; > +} > + > +static int w5500_writebulk(struct udevice *dev, u32 addr, const u8 *buf, > + int len) > +{ > + int i; > + u8 bank; > + u8 cmd[9000]; > + u8 din = 0; > + > + bank = (addr >> 16); > + if (bank > 0) > + bank = bank << 3; > + > + cmd[0] = (addr >> 8) & 0xff; > + cmd[1] = addr & 0xff; > + cmd[2] = bank | 0x4; > + > + for (i = 0; i < len; i++) > + cmd[i + 3] = buf[i]; > + > + return xfer(dev, cmd, len + 3, &din, 0); > +} > + > +static int w5500_spi_write16(struct udevice *dev, u32 addr, u16 data) > +{ > + u8 buf[2]; > + > + buf[0] = data >> 8; > + buf[1] = data & 0xff; > + > + return w5500_writebulk(dev, addr, &buf[0], 2); > +} > + > +static int w5500_readbulk(struct udevice *dev, u32 addr, u8 *buf, int len) > +{ > + u8 cmd[3]; > + u8 *data; > + int i; > + int ret; > + u8 bank; > + > + data = (u8 *)malloc(MAX_PACKET_SZ); Why not malloc(len) ? > + > + if (!data) > + return -1; > + > + bank = (addr >> 16); > + > + if (bank > 0) > + bank = bank << 3; > + > + cmd[0] = addr >> 8; > + cmd[1] = addr & 0xff; > + cmd[2] = bank; > + > + ret = xfer(dev, cmd, sizeof(cmd), &data, len); > + > + if (ret != 0) { if (ret) { > + free(data); > + return ret; > + } > + > + for (i = 0; i < len; i++) > + buf[(len - 1) - i] = data[i]; > + free(data); > + return 0; or add: out: free(data) return ret; and replace with: if (ret) goto out; > +} > + > +static int w5500_readbuf(struct udevice *dev, u16 offset, u8 *buf, int len) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + u32 addr; > + const u32 mem_start = priv->s0_rx_buf; > + int remain = 0; > + int ret; > + const u16 mem_size = priv->s0_rx_buf_size; Order by line length > + > + offset %= mem_size; > + addr = mem_start + offset; > + > + if (offset + len > mem_size) { > + remain = (offset + len) % mem_size; > + len = mem_size - offset; > + } > + > + ret = w5500_readbulk(dev, addr, buf, len); > + if (ret || !remain) > + return ret; > + > + return w5500_readbulk(dev, mem_start, buf + len, remain); > +} > + > +static int w5500_writebuf(struct udevice *dev, u16 offset, const u8 *buf, > + int len) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + u32 addr; > + const u32 mem_start = priv->s0_tx_buf; > + int ret; > + int remain = 0; > + const u16 mem_size = priv->s0_tx_buf_size; Ditto > + > + offset %= mem_size; > + addr = mem_start + offset; > + > + if (offset + len > mem_size) { > + remain = (offset + len) % mem_size; > + len = mem_size - offset; > + } > + > + ret = w5500_writebulk(dev, addr, buf, len); > + if (ret || !remain) > + return ret; > + > + return w5500_writebulk(dev, mem_start, buf + len, remain); > +} > + > +static int w5500_command(struct udevice *dev, u8 cmd) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + u8 val; > + u16 check; > + > + w5500_spi_write(dev, W5100_S0_CR(priv), cmd); > + check = read_poll_timeout(w5500_spi_read, val, val != 0, > + 10, 5000, dev, W5100_S0_CR(priv)); > + if (check != 0) if (check) > + return -EIO; return ret > + > + return 0; > +} > + > +static int w5500_eth_start(struct udevice *dev) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + > + debug("eth_w5500: Start\n"); > + > + u8 mode = S0_MR_MACRAW; > + > + if (!priv->promisc) > + mode |= W5500_S0_MR_MF; > + > + w5500_spi_write(dev, W5100_S0_MR(priv), mode); > + w5500_command(dev, S0_CR_OPEN); Please check return values of w5500_spi_write() & w5500_command() & co on all calls > + priv->offset = 0; > + > + return 0; > +} > + > +static int w5500_eth_send(struct udevice *dev, void *packet, int length) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + u16 offset; > + > + if (priv->disabled) > + return 0; > + > + offset = w5500_spi_read16(dev, W5100_S0_TX_WR(priv)); > + w5500_writebuf(dev, offset, packet, length); > + w5500_spi_write16(dev, W5100_S0_TX_WR(priv), offset + length); > + w5500_command(dev, S0_CR_SEND); Ditto > + > + return 0; > +} > + > +static int w5500_eth_recv(struct udevice *dev, int flags, u8 **packetp) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + u16 rx_len; > + u16 offset; > + u8 data[9000]; > + u16 tmp; > + u16 rx_buf_len; > + > + rx_buf_len = w5500_spi_read16(dev, W5100_S0_RX_RSR(priv)); > + > + /* > + * w5500 is updating rx_buf_len as soon as bytes are received > + * to avoid reading partial data we must wait for that register to > + * stay stable between to read otherwise we might be issuing > + * reading operations while buffer hasn't been completed by the driver > + * per architectural specifications Weird because the linux driver doesn't do that at all, perhaps check for the W5100_S0_IR S0_IR_RECV flag instead ? > + */ > + > + while ((tmp = > + w5500_spi_read16(dev, W5100_S0_RX_RSR(priv))) != rx_buf_len) > + rx_buf_len = tmp; > + > + if (rx_buf_len == 0) > + return 0; > + > + offset = w5500_spi_read16(dev, W5100_S0_RX_RD(priv)); > + rx_len = rx_buf_len - 2; > + w5500_readbuf(dev, offset + 2, data, rx_len); > + w5500_spi_write16(dev, W5100_S0_RX_RD(priv), offset + 2 + rx_len); > + w5500_command(dev, S0_CR_RECV); > + *packetp = data; > + > + priv->offset += rx_buf_len; > + > + return rx_len; > +} > + > +static int w5500_eth_free_pkt(struct udevice *dev, u8 *packet, int length) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + int i; > + > + if (!priv->recv_packets) > + return 0; > + > + --priv->recv_packets; > + for (i = 0; i < priv->recv_packets; i++) { > + priv->recv_packet_length[i] = priv->recv_packet_length[i + 1]; > + memcpy(priv->recv_packet_buffer[i], > + priv->recv_packet_buffer[i + 1], > + priv->recv_packet_length[i + 1]); > + } > + priv->recv_packet_length[priv->recv_packets] = 0; > + > + return 0; > +} > + > +static void w5500_eth_stop(struct udevice *dev) > +{ > + dev_dbg(dev, "eth_w5500: Stop\n"); > +} > + > +static void w5500_socket_intr_mask(struct udevice *dev, u8 mask) > +{ > + w5500_spi_write(dev, W5500_SIMR, mask); > +} > + > +static void w5500_disable_intr(struct udevice *dev) > +{ > + w5500_socket_intr_mask(dev, 0); > +} > + > +static void w5500_enable_intr(struct udevice *dev) > +{ > + w5500_socket_intr_mask(dev, IR_S0); > +} > + > +static int w5500_eth_write_hwaddr(struct udevice *dev) > +{ > + struct eth_pdata *pdata = dev_get_plat(dev); > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + u8 eth[6]; > + > + if (memcmp(priv->host_hwaddr, pdata->enetaddr, ARP_HLEN) != 0) { > + memcpy(priv->host_hwaddr, pdata->enetaddr, ARP_HLEN); > + w5500_writebulk(dev, W5100_SHAR, priv->host_hwaddr, ARP_HLEN); > + } > + > + w5500_readbulk(dev, W5100_SHAR, eth, 6); > + > + w5500_spi_write(dev, W5100_S0_MR(priv), 0x84); > + w5500_command(dev, S0_CR_OPEN); > + w5500_enable_intr(dev); This is already done in w5500_eth_start(), so drop > + > + return 0; > +} > + > +static const struct eth_ops w5500_eth_ops = { > + .start = w5500_eth_start, > + .send = w5500_eth_send, > + .recv = w5500_eth_recv, > + .free_pkt = w5500_eth_free_pkt, > + .stop = w5500_eth_stop, > + .write_hwaddr = w5500_eth_write_hwaddr, > +}; > + > +int w5500_eth_of_to_plat(struct udevice *dev) > +{ > + u8 mac[8]; > + const void *ptr; > + struct eth_pdata *pdata = dev_get_plat(dev); > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + ofnode remote; > + int size; > + > + size = 0; > + ptr = ofnode_read_prop(remote, "local-mac-address", &size); > + if (size != 6) > + return -1; > + mac[0] = ((u8 *)ptr)[0]; > + mac[1] = ((u8 *)ptr)[1]; > + mac[2] = ((u8 *)ptr)[2]; > + mac[3] = ((u8 *)ptr)[4]; > + mac[4] = ((u8 *)ptr)[5]; > + mac[5] = ((u8 *)ptr)[5]; > + > + memcpy(pdata->enetaddr, (void *)mac, ARP_HLEN); This looks fishy, no 3 and 5 is duplicated. And this is not the driver role to set this value, it will be set by eth_post_probe() or board code so drop this entirely. > + > + priv->disabled = false; plat is allocated zero-ed, so this can also be dropped, so drop the whole w5500_eth_of_to_plat() > + > + return 0; > +} > + > +int w5500_eth_probe(struct udevice *dev) > +{ > + struct eth_w5500_priv *priv = dev_get_priv(dev); > + u8 cmd[3]; > + int i; > + > + if (device_get_uclass_id(dev->parent) != UCLASS_SPI) { > + debug("Error device not attached to a SPI controlled\n"); > + return -ENODEV; > + } > + > + cmd[0] = 0x00; > + cmd[1] = 0x19; > + cmd[2] = 0; This is unused > + > + w5500_spi_write(dev, W5100_MR, MR_RST); > + w5500_spi_write(dev, W5100_MR, MR_PB); > + > + if (w5500_spi_read16(dev, W5500_RTR) != RTR_DEFAULT) { > + debug("RTR issue in probe .... %x\n", > + w5500_spi_read16(dev, W5500_RTR)); > + return -ENODEV; > + } > + > + w5500_disable_intr(dev); > + > + /* Configure internal RX memory as 16K RX buffer and > + * internal TX memory as 16K TX buffer > + */ > + > + w5500_spi_write(dev, W5500_Sn_RXMEM_SIZE(0), 0x10); > + w5500_spi_write(dev, W5500_Sn_TXMEM_SIZE(0), 0x10); > + > + for (i = 1; i < 8; i++) { > + w5500_spi_write(dev, W5500_Sn_RXMEM_SIZE(i), 0); > + w5500_spi_write(dev, W5500_Sn_TXMEM_SIZE(i), 0); > + } > + > + priv->s0_regs = W5500_S0_REGS; > + priv->s0_tx_buf = W5500_TX_MEM_START; > + priv->s0_tx_buf_size = W5500_TX_MEM_SIZE; > + priv->s0_rx_buf = W5500_RX_MEM_START; > + priv->s0_rx_buf_size = W5500_RX_MEM_SIZE; > + priv->offset = 0; > + > + return 0; > +} > + > +static const struct udevice_id w5500_eth_ids[] = { > + {.compatible = "wiznet,w5500" }, > + { } > +}; > + > +U_BOOT_DRIVER(eth_w5500) = { > + .name = "eth_w5500", > + .id = UCLASS_ETH, > + .of_match = w5500_eth_ids, > + .of_to_plat = w5500_eth_of_to_plat, > + .probe = w5500_eth_probe, > + .ops = &w5500_eth_ops, > + .priv_auto = sizeof(struct eth_w5500_priv), > + .plat_auto = sizeof(struct eth_pdata), > +}; Thanks, Neil
Hi Neil, Jean-Marie, On 5/12/25 10:10 AM, neil.armstrong@linaro.org wrote: > Hi, > > Thanks for fixing it ! > > On 11/05/2025 20:02, verdun@hpe.com wrote: >> From: Jean-Marie Verdun <verdun@hpe.com> >> >> Add support for the Wiznet W5500 spi to ethernet controller >> >> Signed-off-by: Jean-Marie Verdun <verdun@hpe.com> >> --- >> drivers/net/Kconfig | 9 + >> drivers/net/Makefile | 1 + >> drivers/net/w5500.c | 584 +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 594 insertions(+) >> create mode 100644 drivers/net/w5500.c >> >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig >> index 950ed0f25a9..74668f477ae 100644 >> --- a/drivers/net/Kconfig >> +++ b/drivers/net/Kconfig >> @@ -19,6 +19,15 @@ config SPL_DM_ETH >> depends on SPL_NET >> def_bool y >> +config W5500 >> + bool "W5500 device driver" >> + depends on SPI >> + help >> + Enable w5500 driver >> + >> + Adds support for Wiznet W5500 device. The device must be attached >> + to a SPI bus. >> + >> config DM_MDIO >> bool "Enable Driver Model for MDIO devices" >> depends on PHYLIB >> diff --git a/drivers/net/Makefile b/drivers/net/Makefile >> index 67bba3a8536..6d85c38d869 100644 >> --- a/drivers/net/Makefile >> +++ b/drivers/net/Makefile >> @@ -103,6 +103,7 @@ obj-$(CONFIG_SUN8I_EMAC) += sun8i_emac.o >> obj-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o >> obj-$(CONFIG_TULIP) += dc2114x.o >> obj-$(CONFIG_VSC7385_ENET) += vsc7385.o >> +obj-$(CONFIG_W5500) += w5500.o >> obj-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o >> obj-$(CONFIG_XILINX_AXIMRMAC) += xilinx_axi_mrmac.o >> obj-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o >> diff --git a/drivers/net/w5500.c b/drivers/net/w5500.c >> new file mode 100644 >> index 00000000000..a15255bb457 >> --- /dev/null >> +++ b/drivers/net/w5500.c >> @@ -0,0 +1,584 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright Hewlett Packard Enterprise Development LP. >> + * >> + * Jean-Marie Verdun <verdun@hpe.com> >> + * >> + * Inspired from the linux kernel driver from: >> + * - Copyright (C) 2006-2008 WIZnet Co.,Ltd. >> + * - Copyright (C) 2012 Mike Sinkovsky <msink@permonline.ru> >> + * >> + * available at >> + * - https://eur02.safelinks.protection.outlook.com/? >> url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fnet%2Fethernet%2Fwiznet%2Fw5100.c&data=05%7C02%7Cquentin.schulz%40cherry.de%7Cc46cc28f1cef4e123f8008dd912c7e86%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638826343382626512%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=nnWuRiS1B1wya4helTsVcJ92ZEjG81Ir%2BlSt0Kh9woI%3D&reserved=0 >> + * >> + * Datasheet: >> + * - https://eur02.safelinks.protection.outlook.com/? >> url=http%3A%2F%2Fwww.wiznet.co.kr%2Fwp- >> content%2Fuploads%2Fwiznethome%2FChip%2FW5100%2FDocument%2FW5100_Datasheet_v1.2.6.pdf&data=05%7C02%7Cquentin.schulz%40cherry.de%7Cc46cc28f1cef4e123f8008dd912c7e86%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638826343382649703%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Z57oXSn9Nmwbd5RkkxVq9BfG56JB0LgWuYavpYOJgA8%3D&reserved=0 >> + * - https://eur02.safelinks.protection.outlook.com/? >> url=http%3A%2F%2Fwiznethome.cafe24.com%2Fwp- >> content%2Fuploads%2Fwiznethome%2FChip%2FW5200%2FDocuments%2FW5200_DS_V140E.pdf&data=05%7C02%7Cquentin.schulz%40cherry.de%7Cc46cc28f1cef4e123f8008dd912c7e86%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638826343382668572%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=uDhG%2BbKQyck51qSdZCMvFbXJCDcj4I40Q18Ogl2oBnw%3D&reserved=0 >> + * - https://eur02.safelinks.protection.outlook.com/? >> url=http%3A%2F%2Fwizwiki.net%2Fwiki%2Flib%2Fexe%2Ffetch.php%3Fmedia%3Dproducts%3Aw5500%3Aw5500_ds_v106e_141230.pdf&data=05%7C02%7Cquentin.schulz%40cherry.de%7Cc46cc28f1cef4e123f8008dd912c7e86%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638826343382685514%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=NkSjVHnMa4kAWiB21NDTTF%2F2ODvFbUS33zwz5%2FK69Jw%3D&reserved=0 >> + * >> + */ >> + >> +#include <dm.h> >> +#include <log.h> >> +#include <malloc.h> >> +#include <spi.h> >> +#include <net.h> >> +#include <asm/global_data.h> >> +#include <dm/device_compat.h> >> +#include <linux/delay.h> >> +#include <linux/iopoll.h> >> +#include <net.h> >> +#include <malloc.h> >> + >> +#define BUFFER_SZ 16384 >> +#define MAX_PACKET_SZ 9000 >> +#define W5100_SPI_WRITE_OPCODE 0xf0 >> +#define W5100_SPI_READ_OPCODE 0x0f >> +#define W5100_SHAR 0x0009 /* Source MAC address */ >> +#define W5500_S0_REGS 0x10000 >> +#define S0_REGS(priv) ((priv)->s0_regs) >> +#define W5100_MR 0x0000 /* Mode Register */ >> +#define MR_RST 0x80 /* S/W reset */ >> +#define MR_PB 0x10 /* Ping block */ >> + >> +#define W5100_Sn_MR 0x0000 /* Sn Mode Register */ >> +#define W5100_Sn_CR 0x0001 /* Sn Command Register */ >> +#define W5100_Sn_IR 0x0002 /* Sn Interrupt Register */ >> +#define W5100_Sn_SR 0x0003 /* Sn Status Register */ >> +#define W5100_Sn_TX_FSR 0x0020 /* Sn Transmit free memory >> size */ >> +#define W5100_Sn_TX_RD 0x0022 /* Sn Transmit memory read >> pointer */ >> +#define W5100_Sn_TX_WR 0x0024 /* Sn Transmit memory write >> pointer */ >> +#define W5100_Sn_RX_RSR 0x0026 /* Sn Receive free memory >> size */ >> +#define W5100_Sn_RX_RD 0x0028 /* Sn Receive memory read >> pointer */ >> + >> +#define W5100_S0_MR(priv) (S0_REGS(priv) + W5100_Sn_MR) >> + >> +#define S0_MR_MACRAW 0x04 /* MAC RAW mode */ >> +#define S0_MR_MF 0x40 /* MAC Filter for W5100 and >> W5200 */ >> +#define W5500_S0_MR_MF 0x80 /* MAC Filter for W5500 */ >> +#define W5100_S0_MR(priv) (S0_REGS(priv) + W5100_Sn_MR) >> + >> +#define S0_MR_MACRAW 0x04 /* MAC RAW mode */ >> +#define S0_MR_MF 0x40 /* MAC Filter for W5100 and >> W5200 */ >> +#define W5500_S0_MR_MF 0x80 /* MAC Filter for W5500 */ >> + >> +/* >> + * W5100 and W5200 common registers about the same with the W5500 >> + */ >> +#define W5100_IMR 0x0016 /* Interrupt Mask Register */ >> +#define IR_S0 0x01 /* S0 interrupt */ >> +#define W5100_RTR 0x0017 /* Retry Time-value >> Register */ >> +#define RTR_DEFAULT 2000 /* =0x07d0 (2000) */ >> +#define W5500_SIMR 0x0018 /* Socket Interrupt Mask >> Register */ >> +#define W5500_RTR 0x0019 /* Retry Time-value >> Register */ >> + >> +#define W5100_S0_CR(priv) (S0_REGS(priv) + W5100_Sn_CR) >> +#define S0_CR_OPEN 0x01 /* OPEN command */ >> +#define S0_CR_CLOSE 0x10 /* CLOSE command */ >> +#define S0_CR_SEND 0x20 /* SEND command */ >> +#define S0_CR_RECV 0x40 /* RECV command */ >> +#define W5100_S0_IR(priv) (S0_REGS(priv) + W5100_Sn_IR) >> +#define S0_IR_SENDOK 0x10 /* complete sending */ >> +#define S0_IR_RECV 0x04 /* receiving data */ >> +#define W5100_S0_SR(priv) (S0_REGS(priv) + W5100_Sn_SR) >> +#define S0_SR_MACRAW 0x42 /* mac raw mode */ >> +#define W5100_S0_TX_FSR(priv) (S0_REGS(priv) + W5100_Sn_TX_FSR) >> +#define W5100_S0_TX_RD(priv) (S0_REGS(priv) + W5100_Sn_TX_RD) >> +#define W5100_S0_TX_WR(priv) (S0_REGS(priv) + W5100_Sn_TX_WR) >> +#define W5100_S0_RX_RSR(priv) (S0_REGS(priv) + W5100_Sn_RX_RSR) >> +#define W5100_S0_RX_RD(priv) (S0_REGS(priv) + W5100_Sn_RX_RD) >> + >> +#define W5500_TX_MEM_START 0x20000 >> +#define W5500_TX_MEM_SIZE 0x04000 >> +#define W5500_RX_MEM_START 0x30000 >> +#define W5500_RX_MEM_SIZE 0x04000 >> + >> +#define W5500_Sn_RXMEM_SIZE(n) \ >> + (0x1001e + (n) * 0x40000) /* Sn RX Memory Size */ >> +#define W5500_Sn_TXMEM_SIZE(n) \ >> + (0x1001f + (n) * 0x40000) /* Sn TX Memory Size */ >> + >> +/** >> + * struct eth_w5500_priv - memory for w5500 driver >> + */ >> +struct eth_w5500_priv { >> + uchar host_hwaddr[ARP_HLEN]; >> + bool disabled; >> + uchar * recv_packet_buffer[PKTBUFSRX]; >> + int recv_packet_length[PKTBUFSRX]; >> + int recv_packets; >> + const struct dm_spi_ops *spi_ops; >> + struct udevice **spi_dev; >> + u32 s0_regs; >> + u32 s0_tx_buf; >> + u16 s0_tx_buf_size; >> + u32 s0_rx_buf; >> + u16 s0_rx_buf_size; >> + bool promisc; >> + u32 msg_enable; >> + u16 offset; >> + void *priv; >> +}; >> + >> +static int xfer(struct udevice *dev, void *dout, unsigned int bout, >> void *din, >> + unsigned int bin) >> +{ >> + /* Ok the xfer function in uboot is symmetrical >> + * (write and read operation happens at the same time) >> + * w5500 requires bytes send followed by receive operation on the >> MISO line >> + * So we need to create a buffer which contain bout+bin bytes and >> pass the various >> + * pointer to the xfer function >> + */ >> + >> + struct udevice *bus = dev_get_parent(dev); >> + u8 buffin[BUFFER_SZ]; >> + u8 buffout[BUFFER_SZ]; > > It's dangerous to allocate 2 16k buffers on the stack, move them global > or allocate them at probe and store the pointers in eth_w5500_priv > >> + >> + if ((bout + bin) < BUFFER_SZ) { >> + for (int i = 0; i < bout; i++) { >> + buffout[i] = ((u8 *)(dout))[i]; >> + buffin[i] = 0; >> + } >> + for (int i = bout; i < (bin + bout); i++) { >> + buffin[i] = 0; >> + buffout[i] = 0; >> + } >> + if (!bus) >> + return -1; >> + dm_spi_xfer(dev, 8 * (bout + bin), buffout, buffin, >> + SPI_XFER_BEGIN | SPI_XFER_END); >> + for (int i = bout; i < (bin + bout); i++) >> + ((u8 *)(din))[bin + bout - i - 1] = buffin[i]; >> + } >> + >> + return 0; >> +} >> + >> +static int w5500_spi_read(struct udevice *dev, u32 addr) >> +{ >> + u8 cmd[3]; >> + int ret; >> + u8 bank; >> + u8 data; >> + >> + bank = (addr >> 16); >> + >> + if (bank > 0) >> + bank = bank << 3; >> + >> + cmd[0] = addr >> 8; >> + cmd[1] = addr & 0xff; >> + cmd[2] = bank; >> + >> + ret = xfer(dev, cmd, sizeof(cmd), &data, 1); >> + if (ret != 0) > > if (ret) > >> + return ret; >> + >> + return data; This is dangerous too. How do I know if the returned data is an error code or actual data? Right now, xfer returns 0 if successful, -1 otherwise. I would suggest return ret only if it is < 0. Or even better, propagate the error code and return data through a pointer as argument of the function? e.g. static int w5500_spi_read(struct udevice *dev, u32 addr, u8 *data) { [...] return xfer(dev, cmd, sizeof(cmd), &data, 1); } >> +} >> + >> +static int w5500_spi_write(struct udevice *dev, u32 addr, u8 data) >> +{ >> + u8 cmd[4]; >> + u8 bank; >> + u8 din; >> + >> + bank = (addr >> 16); >> + din = 0; >> + >> + if (bank > 0) >> + bank = bank << 3; >> + >> + cmd[0] = (addr >> 8) & 0xff; >> + cmd[1] = addr & 0xff; >> + cmd[2] = bank | 0x4; >> + cmd[3] = data; >> + >> + return xfer(dev, cmd, sizeof(cmd), &din, 0); >> +} >> + >> +static int w5500_spi_read16(struct udevice *dev, u32 addr) >> +{ >> + u8 cmd[3]; >> + u16 data; >> + int ret; >> + u8 bank; >> + >> + bank = (addr >> 16); >> + >> + if (bank > 0) >> + bank = bank << 3; >> + >> + cmd[0] = addr >> 8; >> + cmd[1] = addr & 0xff; >> + cmd[2] = bank; >> + >> + ret = xfer(dev, cmd, sizeof(cmd), &data, 2); >> + if (ret != 0) > > if (ret) > >> + return ret; >> + >> + return data; >> +} >> + Ditto. Considering the only difference between w5500_spi_read16 and w5500_spi_read is the 2 or 1 as last argument of xfer() call, you probably want to factor them out into a common function to limit duplicated code. If you have access to the info, I would suggest to NOT use a u8[3] array but create a small struct which specifies what each u8 is for so it's easier to read and debug later on. This applies to all other functions. >> +static int w5500_writebulk(struct udevice *dev, u32 addr, const u8 *buf, >> + int len) >> +{ >> + int i; >> + u8 bank; >> + u8 cmd[9000]; You really need to justify that big of an array, why do you need that? Can't we malloc it based on len? >> + u8 din = 0; >> + >> + bank = (addr >> 16); >> + if (bank > 0) >> + bank = bank << 3; >> + >> + cmd[0] = (addr >> 8) & 0xff; >> + cmd[1] = addr & 0xff; >> + cmd[2] = bank | 0x4; >> + >> + for (i = 0; i < len; i++) >> + cmd[i + 3] = buf[i]; >> + >> + return xfer(dev, cmd, len + 3, &din, 0); >> +} >> + >> +static int w5500_spi_write16(struct udevice *dev, u32 addr, u16 data) >> +{ >> + u8 buf[2]; >> + >> + buf[0] = data >> 8; >> + buf[1] = data & 0xff; >> + Looks like endianness swap to me? Is this supposed to happen if you have another endianness for the SoC than the one you're currently using to test this? >> + return w5500_writebulk(dev, addr, &buf[0], 2); >> +} >> + >> +static int w5500_readbulk(struct udevice *dev, u32 addr, u8 *buf, int >> len) >> +{ >> + u8 cmd[3]; >> + u8 *data; >> + int i; >> + int ret; >> + u8 bank; >> + >> + data = (u8 *)malloc(MAX_PACKET_SZ); > > Why not malloc(len) ? > >> + >> + if (!data) >> + return -1; -ENOMEM? >> + >> + bank = (addr >> 16); >> + >> + if (bank > 0) >> + bank = bank << 3; >> + >> + cmd[0] = addr >> 8; >> + cmd[1] = addr & 0xff; >> + cmd[2] = bank; >> + >> + ret = xfer(dev, cmd, sizeof(cmd), &data, len); >> + >> + if (ret != 0) { > > if (ret) { > >> + free(data); >> + return ret; >> + } >> + >> + for (i = 0; i < len; i++) >> + buf[(len - 1) - i] = data[i]; >> + free(data); >> + return 0; > > or add: > out: > free(data) > return ret; > > and replace with: > if (ret) > goto out; > Agree with Neil here, go for the typical goto solution for unwinding code in the error path(s). >> +} >> + >> +static int w5500_readbuf(struct udevice *dev, u16 offset, u8 *buf, >> int len) >> +{ >> + struct eth_w5500_priv *priv = dev_get_priv(dev); >> + u32 addr; >> + const u32 mem_start = priv->s0_rx_buf; >> + int remain = 0; >> + int ret; >> + const u16 mem_size = priv->s0_rx_buf_size; > > Order by line length > >> + >> + offset %= mem_size; >> + addr = mem_start + offset; >> + >> + if (offset + len > mem_size) { >> + remain = (offset + len) % mem_size; >> + len = mem_size - offset; >> + } >> + >> + ret = w5500_readbulk(dev, addr, buf, len); >> + if (ret || !remain) >> + return ret; >> + >> + return w5500_readbulk(dev, mem_start, buf + len, remain); >> +} >> + >> +static int w5500_writebuf(struct udevice *dev, u16 offset, const u8 >> *buf, >> + int len) >> +{ >> + struct eth_w5500_priv *priv = dev_get_priv(dev); >> + u32 addr; >> + const u32 mem_start = priv->s0_tx_buf; >> + int ret; >> + int remain = 0; >> + const u16 mem_size = priv->s0_tx_buf_size; > > Ditto > >> + >> + offset %= mem_size; >> + addr = mem_start + offset; >> + >> + if (offset + len > mem_size) { >> + remain = (offset + len) % mem_size; >> + len = mem_size - offset; >> + } >> + >> + ret = w5500_writebulk(dev, addr, buf, len); >> + if (ret || !remain) >> + return ret; >> + >> + return w5500_writebulk(dev, mem_start, buf + len, remain); >> +} >> + >> +static int w5500_command(struct udevice *dev, u8 cmd) >> +{ >> + struct eth_w5500_priv *priv = dev_get_priv(dev); >> + u8 val; >> + u16 check; >> + >> + w5500_spi_write(dev, W5100_S0_CR(priv), cmd); >> + check = read_poll_timeout(w5500_spi_read, val, val != 0, >> + 10, 5000, dev, W5100_S0_CR(priv)); >> + if (check != 0) > > if (check) > >> + return -EIO; > > return ret > and use ret instead of check for the variable name. >> + >> + return 0; >> +} >> + >> +static int w5500_eth_start(struct udevice *dev) >> +{ >> + struct eth_w5500_priv *priv = dev_get_priv(dev); >> + >> + debug("eth_w5500: Start\n"); Please use log_debug()/dev_dbg instead of debug(). Cheers, Quentin
>> This is dangerous too. How do I know if the returned data is an error >> code or actual data? >> Right now, xfer returns 0 if successful, -1 otherwise. I would suggest >> return ret only if it is < 0. >> Or even better, propagate the error code and return data through a >> pointer as argument of the function? >> e.g. >> static int w5500_spi_read(struct udevice *dev, u32 addr, u8 *data) >> { >> [...] >> return xfer(dev, cmd, sizeof(cmd), &data, 1); >>} I could do that but that won’t be the same logic as the linux kernel driver approach which is mixing up ret and data as return value of this call and many others too. >> Considering the only difference between w5500_spi_read16 and >> w5500_spi_read is the 2 or 1 as last argument of xfer() call, you >> probably want to factor them out into a common function to limit >> duplicated code. >> If you have access to the info, I would suggest to NOT use a u8[3] array >> but create a small struct which specifies what each u8 is for so it's >> easier to read and debug later on. This applies to all other functions. I can do that >> You really need to justify that big of an array, why do you need that? >> Can't we malloc it based on len? I switched it to a malloc for next patchset >> Looks like endianness swap to me? Is this supposed to happen if you have >> another endianness for the SoC than the one you're currently using to >> test this? I don’t have access to anything else other than an ARM SoC to test it. It seems required to swap the bytes from the SPI read process >> -ENOMEM? Done in next patchset >> Please use log_debug()/dev_dbg instead of debug(). Neil initially asked for debug, I can swap to any, just let me know which one. Cheers, Quentin
Hi Jean-Marie, On 5/15/25 4:01 PM, Verdun, Jean-Marie wrote: > > >>> This is dangerous too. How do I know if the returned data is an error >>> code or actual data? > >>> Right now, xfer returns 0 if successful, -1 otherwise. I would suggest >>> return ret only if it is < 0. > >>> Or even better, propagate the error code and return data through a >>> pointer as argument of the function? > >>> e.g. > >>> static int w5500_spi_read(struct udevice *dev, u32 addr, u8 *data) >>> { >>> [...] >>> return xfer(dev, cmd, sizeof(cmd), &data, 1); >>> } > > I could do that but that won’t be the same logic as the linux kernel > driver approach which is mixing up ret and data as return value of > this call and many others too. > Fair enough. You could have the same ternary operator to better match what's in the kernel. Can you also reuse the macros that are in the kernel already? W5500_SPI_BLOCK_SELECT, W5500_SPI_READ_CONTROL and W5500_SPI_WRITE_CONTROL seems to be of interest? > >>> Considering the only difference between w5500_spi_read16 and >>> w5500_spi_read is the 2 or 1 as last argument of xfer() call, you >>> probably want to factor them out into a common function to limit >>> duplicated code. > >>> If you have access to the info, I would suggest to NOT use a u8[3] array >>> but create a small struct which specifies what each u8 is for so it's >>> easier to read and debug later on. This applies to all other functions. > > I can do that > If you do that, please also update the kernel code to use that :) >>> Looks like endianness swap to me? Is this supposed to happen if you have >>> another endianness for the SoC than the one you're currently using to >>> test this? > > I don’t have access to anything else other than an ARM SoC to test it. It seems > required to swap the bytes from the SPI read process > Yeah the kernel seems to be using __be16 type for that. And the write part seems to be done by hand for the address and data. >>> Please use log_debug()/dev_dbg instead of debug(). > > Neil initially asked for debug, I can swap to any, just let me know which one. > debug() can use the log framework if enabled, but you still need to add #define DEBUG wherever you want to build the logging messages. Whereas with log_debug you would only need to raise the log level at compile time to see the logs. Same, but different. The benefit of using dev_dbg is when you're having multiple devices from the same driver, then you can identify from which one the message is being printed. You could have two Wiznet W5500 for example, how do you know which one's printing which message? Up to you, Cheers, Quentin
>> Fair enough. You could have the same ternary operator to better match >> what's in the kernel. >> Can you also reuse the macros that are in the kernel already? >> W5500_SPI_BLOCK_SELECT, W5500_SPI_READ_CONTROL and >> W5500_SPI_WRITE_CONTROL seems to be of interest? I will check it >> If you do that, please also update the kernel code to use that :) I have a new version of the driver code without the _spi_read* _spi_write* calls. I think for the u-boot version it will be ok as it shorten the source code. What I loved to do first is perhaps to get the new patchset upstream in u-boot and then change the cmd[xx] logic to a structure in the linux kernel and u-boot ? Syncing both might be a little bit tricky. I also have some performance patches to look at with u-boot (udelay call with a value of 0 is slow and I know useless too) >> The benefit of using dev_dbg is when you're having multiple devices from >> the same driver, then you can identify from which one the message is >> being printed. You could have two Wiznet W5500 for example, how do you >> know which one's printing which message? I am going to switch to dev_dbg. I will send the new patchset on Wednesday, as I liked to test it on real hardware before doing so.
Hi Jean-Marie, On 5/19/25 11:05 AM, Verdun, Jean-Marie wrote: >>> Fair enough. You could have the same ternary operator to better match >>> what's in the kernel. > >>> Can you also reuse the macros that are in the kernel already? >>> W5500_SPI_BLOCK_SELECT, W5500_SPI_READ_CONTROL and >>> W5500_SPI_WRITE_CONTROL seems to be of interest? > > I will check it > >>> If you do that, please also update the kernel code to use that :) > > I have a new version of the driver code without the _spi_read* _spi_write* calls. I think for the u-boot version it will be ok as it shorten the source code. Since you're already written the code, maybe it's fine to keep it? I assume the split in the kernel was done so that it would be easier to add support for other (future?) chips which would be able to operate on different buses than SPI? Though it seems that all their iEthernet product line only operate on SPI (https://docs.wiznet.io/Product/iEthernet)? There's typical interest in staying possibly as close as possible to Linux kernel drivers so that security fixes, bug fixes and improvements aren't too difficult to backport to U-Boot. But that's the maintainer's and contributor's decision to make :) > What I loved to do first is perhaps to get the new patchset upstream in u-boot and then change the cmd[xx] logic to a structure in the linux kernel and u-boot ? Syncing both might be a little bit tricky. I also have some performance patches to look at with u-boot (udelay call with a value of 0 is slow and I know useless too) > Typically what happens in open-source projects is that once the code is merged, the contributor disappears :) So there's usually reticence to merging code if there's some on-going discussion. Here, the struct for cmd change suggestion is not a blocker as it's mostly cosmetic anyway and it didn't prevent the code from being merged in the kernel (and it isn't a bug that we should fix in the kernel), so that's fine with me (I'm anyway not the maintainer so I wouldn't have the power to veto this :) ). Cheers, Quentin
>> Typically what happens in open-source projects is that once the code is >> merged, the contributor disappears :) So there's usually reticence to >> merging code if there's some on-going discussion. Here, the struct for >> cmd change suggestion is not a blocker as it's mostly cosmetic anyway >> and it didn't prevent the code from being merged in the kernel (and it >> isn't a bug that we should fix in the kernel), so that's fine with me >> (I'm anyway not the maintainer so I wouldn't have the power to veto this >> :) ). I can maintain it at least for a year without many issues. I need that code for many projects inside HPE, so it is not that hard on my side, and I will work with u-boot during the next couple of months. Promising anything after a year is tough to anybody, but I will give it a try without any issues. Other good news is that I soon got contact with other end users who tested the patch on live systems (different than my dev platform). So looks like there is some interest for this device. So stay tune, and new update on Wednesday as soon as I have tested the new patch. We can then decide what do we want keep and remove. I strip down the code the the “easiest” code that it could be, which is simplifying it against the kernel perhaps too much as you mention, but it is also easy to maintain that way.
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 950ed0f25a9..74668f477ae 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -19,6 +19,15 @@ config SPL_DM_ETH depends on SPL_NET def_bool y +config W5500 + bool "W5500 device driver" + depends on SPI + help + Enable w5500 driver + + Adds support for Wiznet W5500 device. The device must be attached + to a SPI bus. + config DM_MDIO bool "Enable Driver Model for MDIO devices" depends on PHYLIB diff --git a/drivers/net/Makefile b/drivers/net/Makefile index 67bba3a8536..6d85c38d869 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -103,6 +103,7 @@ obj-$(CONFIG_SUN8I_EMAC) += sun8i_emac.o obj-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o obj-$(CONFIG_TULIP) += dc2114x.o obj-$(CONFIG_VSC7385_ENET) += vsc7385.o +obj-$(CONFIG_W5500) += w5500.o obj-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o obj-$(CONFIG_XILINX_AXIMRMAC) += xilinx_axi_mrmac.o obj-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o diff --git a/drivers/net/w5500.c b/drivers/net/w5500.c new file mode 100644 index 00000000000..a15255bb457 --- /dev/null +++ b/drivers/net/w5500.c @@ -0,0 +1,584 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright Hewlett Packard Enterprise Development LP. + * + * Jean-Marie Verdun <verdun@hpe.com> + * + * Inspired from the linux kernel driver from: + * - Copyright (C) 2006-2008 WIZnet Co.,Ltd. + * - Copyright (C) 2012 Mike Sinkovsky <msink@permonline.ru> + * + * available at + * - https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/wiznet/w5100.c + * + * Datasheet: + * - http://www.wiznet.co.kr/wp-content/uploads/wiznethome/Chip/W5100/Document/W5100_Datasheet_v1.2.6.pdf + * - http://wiznethome.cafe24.com/wp-content/uploads/wiznethome/Chip/W5200/Documents/W5200_DS_V140E.pdf + * - http://wizwiki.net/wiki/lib/exe/fetch.php?media=products:w5500:w5500_ds_v106e_141230.pdf + * + */ + +#include <dm.h> +#include <log.h> +#include <malloc.h> +#include <spi.h> +#include <net.h> +#include <asm/global_data.h> +#include <dm/device_compat.h> +#include <linux/delay.h> +#include <linux/iopoll.h> +#include <net.h> +#include <malloc.h> + +#define BUFFER_SZ 16384 +#define MAX_PACKET_SZ 9000 +#define W5100_SPI_WRITE_OPCODE 0xf0 +#define W5100_SPI_READ_OPCODE 0x0f +#define W5100_SHAR 0x0009 /* Source MAC address */ +#define W5500_S0_REGS 0x10000 +#define S0_REGS(priv) ((priv)->s0_regs) +#define W5100_MR 0x0000 /* Mode Register */ +#define MR_RST 0x80 /* S/W reset */ +#define MR_PB 0x10 /* Ping block */ + +#define W5100_Sn_MR 0x0000 /* Sn Mode Register */ +#define W5100_Sn_CR 0x0001 /* Sn Command Register */ +#define W5100_Sn_IR 0x0002 /* Sn Interrupt Register */ +#define W5100_Sn_SR 0x0003 /* Sn Status Register */ +#define W5100_Sn_TX_FSR 0x0020 /* Sn Transmit free memory size */ +#define W5100_Sn_TX_RD 0x0022 /* Sn Transmit memory read pointer */ +#define W5100_Sn_TX_WR 0x0024 /* Sn Transmit memory write pointer */ +#define W5100_Sn_RX_RSR 0x0026 /* Sn Receive free memory size */ +#define W5100_Sn_RX_RD 0x0028 /* Sn Receive memory read pointer */ + +#define W5100_S0_MR(priv) (S0_REGS(priv) + W5100_Sn_MR) + +#define S0_MR_MACRAW 0x04 /* MAC RAW mode */ +#define S0_MR_MF 0x40 /* MAC Filter for W5100 and W5200 */ +#define W5500_S0_MR_MF 0x80 /* MAC Filter for W5500 */ +#define W5100_S0_MR(priv) (S0_REGS(priv) + W5100_Sn_MR) + +#define S0_MR_MACRAW 0x04 /* MAC RAW mode */ +#define S0_MR_MF 0x40 /* MAC Filter for W5100 and W5200 */ +#define W5500_S0_MR_MF 0x80 /* MAC Filter for W5500 */ + +/* + * W5100 and W5200 common registers about the same with the W5500 + */ +#define W5100_IMR 0x0016 /* Interrupt Mask Register */ +#define IR_S0 0x01 /* S0 interrupt */ +#define W5100_RTR 0x0017 /* Retry Time-value Register */ +#define RTR_DEFAULT 2000 /* =0x07d0 (2000) */ +#define W5500_SIMR 0x0018 /* Socket Interrupt Mask Register */ +#define W5500_RTR 0x0019 /* Retry Time-value Register */ + +#define W5100_S0_CR(priv) (S0_REGS(priv) + W5100_Sn_CR) +#define S0_CR_OPEN 0x01 /* OPEN command */ +#define S0_CR_CLOSE 0x10 /* CLOSE command */ +#define S0_CR_SEND 0x20 /* SEND command */ +#define S0_CR_RECV 0x40 /* RECV command */ +#define W5100_S0_IR(priv) (S0_REGS(priv) + W5100_Sn_IR) +#define S0_IR_SENDOK 0x10 /* complete sending */ +#define S0_IR_RECV 0x04 /* receiving data */ +#define W5100_S0_SR(priv) (S0_REGS(priv) + W5100_Sn_SR) +#define S0_SR_MACRAW 0x42 /* mac raw mode */ +#define W5100_S0_TX_FSR(priv) (S0_REGS(priv) + W5100_Sn_TX_FSR) +#define W5100_S0_TX_RD(priv) (S0_REGS(priv) + W5100_Sn_TX_RD) +#define W5100_S0_TX_WR(priv) (S0_REGS(priv) + W5100_Sn_TX_WR) +#define W5100_S0_RX_RSR(priv) (S0_REGS(priv) + W5100_Sn_RX_RSR) +#define W5100_S0_RX_RD(priv) (S0_REGS(priv) + W5100_Sn_RX_RD) + +#define W5500_TX_MEM_START 0x20000 +#define W5500_TX_MEM_SIZE 0x04000 +#define W5500_RX_MEM_START 0x30000 +#define W5500_RX_MEM_SIZE 0x04000 + +#define W5500_Sn_RXMEM_SIZE(n) \ + (0x1001e + (n) * 0x40000) /* Sn RX Memory Size */ +#define W5500_Sn_TXMEM_SIZE(n) \ + (0x1001f + (n) * 0x40000) /* Sn TX Memory Size */ + +/** + * struct eth_w5500_priv - memory for w5500 driver + */ +struct eth_w5500_priv { + uchar host_hwaddr[ARP_HLEN]; + bool disabled; + uchar * recv_packet_buffer[PKTBUFSRX]; + int recv_packet_length[PKTBUFSRX]; + int recv_packets; + const struct dm_spi_ops *spi_ops; + struct udevice **spi_dev; + u32 s0_regs; + u32 s0_tx_buf; + u16 s0_tx_buf_size; + u32 s0_rx_buf; + u16 s0_rx_buf_size; + bool promisc; + u32 msg_enable; + u16 offset; + void *priv; +}; + +static int xfer(struct udevice *dev, void *dout, unsigned int bout, void *din, + unsigned int bin) +{ + /* Ok the xfer function in uboot is symmetrical + * (write and read operation happens at the same time) + * w5500 requires bytes send followed by receive operation on the MISO line + * So we need to create a buffer which contain bout+bin bytes and pass the various + * pointer to the xfer function + */ + + struct udevice *bus = dev_get_parent(dev); + u8 buffin[BUFFER_SZ]; + u8 buffout[BUFFER_SZ]; + + if ((bout + bin) < BUFFER_SZ) { + for (int i = 0; i < bout; i++) { + buffout[i] = ((u8 *)(dout))[i]; + buffin[i] = 0; + } + for (int i = bout; i < (bin + bout); i++) { + buffin[i] = 0; + buffout[i] = 0; + } + if (!bus) + return -1; + dm_spi_xfer(dev, 8 * (bout + bin), buffout, buffin, + SPI_XFER_BEGIN | SPI_XFER_END); + for (int i = bout; i < (bin + bout); i++) + ((u8 *)(din))[bin + bout - i - 1] = buffin[i]; + } + + return 0; +} + +static int w5500_spi_read(struct udevice *dev, u32 addr) +{ + u8 cmd[3]; + int ret; + u8 bank; + u8 data; + + bank = (addr >> 16); + + if (bank > 0) + bank = bank << 3; + + cmd[0] = addr >> 8; + cmd[1] = addr & 0xff; + cmd[2] = bank; + + ret = xfer(dev, cmd, sizeof(cmd), &data, 1); + if (ret != 0) + return ret; + + return data; +} + +static int w5500_spi_write(struct udevice *dev, u32 addr, u8 data) +{ + u8 cmd[4]; + u8 bank; + u8 din; + + bank = (addr >> 16); + din = 0; + + if (bank > 0) + bank = bank << 3; + + cmd[0] = (addr >> 8) & 0xff; + cmd[1] = addr & 0xff; + cmd[2] = bank | 0x4; + cmd[3] = data; + + return xfer(dev, cmd, sizeof(cmd), &din, 0); +} + +static int w5500_spi_read16(struct udevice *dev, u32 addr) +{ + u8 cmd[3]; + u16 data; + int ret; + u8 bank; + + bank = (addr >> 16); + + if (bank > 0) + bank = bank << 3; + + cmd[0] = addr >> 8; + cmd[1] = addr & 0xff; + cmd[2] = bank; + + ret = xfer(dev, cmd, sizeof(cmd), &data, 2); + if (ret != 0) + return ret; + + return data; +} + +static int w5500_writebulk(struct udevice *dev, u32 addr, const u8 *buf, + int len) +{ + int i; + u8 bank; + u8 cmd[9000]; + u8 din = 0; + + bank = (addr >> 16); + if (bank > 0) + bank = bank << 3; + + cmd[0] = (addr >> 8) & 0xff; + cmd[1] = addr & 0xff; + cmd[2] = bank | 0x4; + + for (i = 0; i < len; i++) + cmd[i + 3] = buf[i]; + + return xfer(dev, cmd, len + 3, &din, 0); +} + +static int w5500_spi_write16(struct udevice *dev, u32 addr, u16 data) +{ + u8 buf[2]; + + buf[0] = data >> 8; + buf[1] = data & 0xff; + + return w5500_writebulk(dev, addr, &buf[0], 2); +} + +static int w5500_readbulk(struct udevice *dev, u32 addr, u8 *buf, int len) +{ + u8 cmd[3]; + u8 *data; + int i; + int ret; + u8 bank; + + data = (u8 *)malloc(MAX_PACKET_SZ); + + if (!data) + return -1; + + bank = (addr >> 16); + + if (bank > 0) + bank = bank << 3; + + cmd[0] = addr >> 8; + cmd[1] = addr & 0xff; + cmd[2] = bank; + + ret = xfer(dev, cmd, sizeof(cmd), &data, len); + + if (ret != 0) { + free(data); + return ret; + } + + for (i = 0; i < len; i++) + buf[(len - 1) - i] = data[i]; + free(data); + return 0; +} + +static int w5500_readbuf(struct udevice *dev, u16 offset, u8 *buf, int len) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + u32 addr; + const u32 mem_start = priv->s0_rx_buf; + int remain = 0; + int ret; + const u16 mem_size = priv->s0_rx_buf_size; + + offset %= mem_size; + addr = mem_start + offset; + + if (offset + len > mem_size) { + remain = (offset + len) % mem_size; + len = mem_size - offset; + } + + ret = w5500_readbulk(dev, addr, buf, len); + if (ret || !remain) + return ret; + + return w5500_readbulk(dev, mem_start, buf + len, remain); +} + +static int w5500_writebuf(struct udevice *dev, u16 offset, const u8 *buf, + int len) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + u32 addr; + const u32 mem_start = priv->s0_tx_buf; + int ret; + int remain = 0; + const u16 mem_size = priv->s0_tx_buf_size; + + offset %= mem_size; + addr = mem_start + offset; + + if (offset + len > mem_size) { + remain = (offset + len) % mem_size; + len = mem_size - offset; + } + + ret = w5500_writebulk(dev, addr, buf, len); + if (ret || !remain) + return ret; + + return w5500_writebulk(dev, mem_start, buf + len, remain); +} + +static int w5500_command(struct udevice *dev, u8 cmd) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + u8 val; + u16 check; + + w5500_spi_write(dev, W5100_S0_CR(priv), cmd); + check = read_poll_timeout(w5500_spi_read, val, val != 0, + 10, 5000, dev, W5100_S0_CR(priv)); + if (check != 0) + return -EIO; + + return 0; +} + +static int w5500_eth_start(struct udevice *dev) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + + debug("eth_w5500: Start\n"); + + u8 mode = S0_MR_MACRAW; + + if (!priv->promisc) + mode |= W5500_S0_MR_MF; + + w5500_spi_write(dev, W5100_S0_MR(priv), mode); + w5500_command(dev, S0_CR_OPEN); + priv->offset = 0; + + return 0; +} + +static int w5500_eth_send(struct udevice *dev, void *packet, int length) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + u16 offset; + + if (priv->disabled) + return 0; + + offset = w5500_spi_read16(dev, W5100_S0_TX_WR(priv)); + w5500_writebuf(dev, offset, packet, length); + w5500_spi_write16(dev, W5100_S0_TX_WR(priv), offset + length); + w5500_command(dev, S0_CR_SEND); + + return 0; +} + +static int w5500_eth_recv(struct udevice *dev, int flags, u8 **packetp) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + u16 rx_len; + u16 offset; + u8 data[9000]; + u16 tmp; + u16 rx_buf_len; + + rx_buf_len = w5500_spi_read16(dev, W5100_S0_RX_RSR(priv)); + + /* + * w5500 is updating rx_buf_len as soon as bytes are received + * to avoid reading partial data we must wait for that register to + * stay stable between to read otherwise we might be issuing + * reading operations while buffer hasn't been completed by the driver + * per architectural specifications + */ + + while ((tmp = + w5500_spi_read16(dev, W5100_S0_RX_RSR(priv))) != rx_buf_len) + rx_buf_len = tmp; + + if (rx_buf_len == 0) + return 0; + + offset = w5500_spi_read16(dev, W5100_S0_RX_RD(priv)); + rx_len = rx_buf_len - 2; + w5500_readbuf(dev, offset + 2, data, rx_len); + w5500_spi_write16(dev, W5100_S0_RX_RD(priv), offset + 2 + rx_len); + w5500_command(dev, S0_CR_RECV); + *packetp = data; + + priv->offset += rx_buf_len; + + return rx_len; +} + +static int w5500_eth_free_pkt(struct udevice *dev, u8 *packet, int length) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + int i; + + if (!priv->recv_packets) + return 0; + + --priv->recv_packets; + for (i = 0; i < priv->recv_packets; i++) { + priv->recv_packet_length[i] = priv->recv_packet_length[i + 1]; + memcpy(priv->recv_packet_buffer[i], + priv->recv_packet_buffer[i + 1], + priv->recv_packet_length[i + 1]); + } + priv->recv_packet_length[priv->recv_packets] = 0; + + return 0; +} + +static void w5500_eth_stop(struct udevice *dev) +{ + dev_dbg(dev, "eth_w5500: Stop\n"); +} + +static void w5500_socket_intr_mask(struct udevice *dev, u8 mask) +{ + w5500_spi_write(dev, W5500_SIMR, mask); +} + +static void w5500_disable_intr(struct udevice *dev) +{ + w5500_socket_intr_mask(dev, 0); +} + +static void w5500_enable_intr(struct udevice *dev) +{ + w5500_socket_intr_mask(dev, IR_S0); +} + +static int w5500_eth_write_hwaddr(struct udevice *dev) +{ + struct eth_pdata *pdata = dev_get_plat(dev); + struct eth_w5500_priv *priv = dev_get_priv(dev); + u8 eth[6]; + + if (memcmp(priv->host_hwaddr, pdata->enetaddr, ARP_HLEN) != 0) { + memcpy(priv->host_hwaddr, pdata->enetaddr, ARP_HLEN); + w5500_writebulk(dev, W5100_SHAR, priv->host_hwaddr, ARP_HLEN); + } + + w5500_readbulk(dev, W5100_SHAR, eth, 6); + + w5500_spi_write(dev, W5100_S0_MR(priv), 0x84); + w5500_command(dev, S0_CR_OPEN); + w5500_enable_intr(dev); + + return 0; +} + +static const struct eth_ops w5500_eth_ops = { + .start = w5500_eth_start, + .send = w5500_eth_send, + .recv = w5500_eth_recv, + .free_pkt = w5500_eth_free_pkt, + .stop = w5500_eth_stop, + .write_hwaddr = w5500_eth_write_hwaddr, +}; + +int w5500_eth_of_to_plat(struct udevice *dev) +{ + u8 mac[8]; + const void *ptr; + struct eth_pdata *pdata = dev_get_plat(dev); + struct eth_w5500_priv *priv = dev_get_priv(dev); + ofnode remote; + int size; + + size = 0; + ptr = ofnode_read_prop(remote, "local-mac-address", &size); + if (size != 6) + return -1; + mac[0] = ((u8 *)ptr)[0]; + mac[1] = ((u8 *)ptr)[1]; + mac[2] = ((u8 *)ptr)[2]; + mac[3] = ((u8 *)ptr)[4]; + mac[4] = ((u8 *)ptr)[5]; + mac[5] = ((u8 *)ptr)[5]; + + memcpy(pdata->enetaddr, (void *)mac, ARP_HLEN); + + priv->disabled = false; + + return 0; +} + +int w5500_eth_probe(struct udevice *dev) +{ + struct eth_w5500_priv *priv = dev_get_priv(dev); + u8 cmd[3]; + int i; + + if (device_get_uclass_id(dev->parent) != UCLASS_SPI) { + debug("Error device not attached to a SPI controlled\n"); + return -ENODEV; + } + + cmd[0] = 0x00; + cmd[1] = 0x19; + cmd[2] = 0; + + w5500_spi_write(dev, W5100_MR, MR_RST); + w5500_spi_write(dev, W5100_MR, MR_PB); + + if (w5500_spi_read16(dev, W5500_RTR) != RTR_DEFAULT) { + debug("RTR issue in probe .... %x\n", + w5500_spi_read16(dev, W5500_RTR)); + return -ENODEV; + } + + w5500_disable_intr(dev); + + /* Configure internal RX memory as 16K RX buffer and + * internal TX memory as 16K TX buffer + */ + + w5500_spi_write(dev, W5500_Sn_RXMEM_SIZE(0), 0x10); + w5500_spi_write(dev, W5500_Sn_TXMEM_SIZE(0), 0x10); + + for (i = 1; i < 8; i++) { + w5500_spi_write(dev, W5500_Sn_RXMEM_SIZE(i), 0); + w5500_spi_write(dev, W5500_Sn_TXMEM_SIZE(i), 0); + } + + priv->s0_regs = W5500_S0_REGS; + priv->s0_tx_buf = W5500_TX_MEM_START; + priv->s0_tx_buf_size = W5500_TX_MEM_SIZE; + priv->s0_rx_buf = W5500_RX_MEM_START; + priv->s0_rx_buf_size = W5500_RX_MEM_SIZE; + priv->offset = 0; + + return 0; +} + +static const struct udevice_id w5500_eth_ids[] = { + {.compatible = "wiznet,w5500" }, + { } +}; + +U_BOOT_DRIVER(eth_w5500) = { + .name = "eth_w5500", + .id = UCLASS_ETH, + .of_match = w5500_eth_ids, + .of_to_plat = w5500_eth_of_to_plat, + .probe = w5500_eth_probe, + .ops = &w5500_eth_ops, + .priv_auto = sizeof(struct eth_w5500_priv), + .plat_auto = sizeof(struct eth_pdata), +};