[PATCH v4 1/1] Initial support for Wiznet W5500
Quentin Schulz
quentin.schulz at cherry.de
Mon May 12 11:42:56 CEST 2025
Hi Neil, Jean-Marie,
On 5/12/25 10:10 AM, neil.armstrong at linaro.org wrote:
> Hi,
>
> Thanks for fixing it !
>
> On 11/05/2025 20:02, verdun at hpe.com wrote:
>> From: Jean-Marie Verdun <verdun at hpe.com>
>>
>> Add support for the Wiznet W5500 spi to ethernet controller
>>
>> Signed-off-by: Jean-Marie Verdun <verdun at 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 at hpe.com>
>> + *
>> + * Inspired from the linux kernel driver from:
>> + * - Copyright (C) 2006-2008 WIZnet Co.,Ltd.
>> + * - Copyright (C) 2012 Mike Sinkovsky <msink at 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
More information about the U-Boot
mailing list