Re: [PATCH v3 09/10] net: add support for Designware XGMAC (10gb) ethernet

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Jun 23, 2025 at 03:57:54PM +0200, Steffen Trumtrar wrote:
>  
> +config DRIVER_NET_DESIGNWARE_XGMAC
> +	bool "Designware XGMAC Ethernet driver support" if COMPILE_TEST

Does it make sense to make this option visible?

> +	depends on HAS_DMA && OFTREE
> +	select PHYLIB
> +	select MFD_SYSCON
> +	help
> +	  This option enables support for the Synopsys Designware Ethernet XGMAC (10G Ethernet MAC).
> +
> +config DRIVER_NET_DESIGNWARE_XGMAC_SOCFPGA
> +	bool "Designware XGMAC Ethernet driver support for SOCFPGA"
> +	select DRIVER_NET_DESIGNWARE_XGMAC
> +	depends on (ARCH_SOCFPGA_AGILEX5 || COMPILE_TEST)
> +	select RESET_CONTROLLER if ARCH_SOCFPGA
> +	select RESET_SIMPLE if ARCH_SOCFPGA_AGILEX5
> +	help
> +	  This option enables support for the Synopsys Designware Ethernet XGMAC with specific configuration
> +	  for the Intel SoC FPGA chip.
> +
>  config DRIVER_NET_DM9K
>  	bool "Davicom dm9k[E|A|B] ethernet driver"
>  	depends on HAS_DM9000 || COMPILE_TEST
> +}
> +
> +static int xgmac_read_rom_hwaddr(struct eth_device *edev, unsigned char *mac)
> +{
> +	struct xgmac_priv *xgmac = to_xgmac(edev);
> +	int ret;
> +
> +	ret = xgmac->config->ops->xgmac_get_enetaddr(edev->parent);
> +	if (ret < 0)
> +		return ret;
> +
> +	return !is_valid_ether_addr(xgmac->macaddr);

Please return a proper error code here.

> +static int xgmac_send(struct eth_device *edev, void *packet, int length)
> +{
> +	struct xgmac_priv *xgmac = to_xgmac(edev);
> +	struct xgmac_desc *tx_desc;
> +	dma_addr_t dma;
> +	u32 des3_prev, des3;
> +	int ret;
> +
> +	tx_desc = &xgmac->tx_descs[xgmac->tx_desc_idx];
> +	xgmac->tx_desc_idx++;
> +	xgmac->tx_desc_idx %= XGMAC_DESCRIPTORS_NUM;
> +
> +	dma = dma_map_single(edev->parent, packet, length, DMA_TO_DEVICE);
> +	if (dma_mapping_error(edev->parent, dma))
> +		return -EFAULT;
> +
> +	tx_desc->des0 = lower_32_bits(dma);
> +	tx_desc->des1 = upper_32_bits(dma);
> +	tx_desc->des2 = length;
> +	/*
> +	 * Make sure that if HW sees the _OWN write below, it will see all the
> +	 * writes to the rest of the descriptor too.
> +	 */
> +	barrier();
> +
> +	des3_prev = XGMAC_DESC3_OWN | XGMAC_DESC3_FD | XGMAC_DESC3_LD | length;
> +	writel(des3_prev, &tx_desc->des3);
> +	writel((ulong)(tx_desc + 1), &xgmac->dma_regs->ch0_txdesc_tail_pointer); // <-- TODO

	if (upper_32_bits((ulong)(tx_desc)))
		dev_err(dev, "meeeeh\n");

To make clear what the TODO is about.

> +
> +	ret = readl_poll_timeout(&tx_desc->des3, des3,
> +				 !(des3 & XGMAC_DESC3_OWN),
> +				 100 * USEC_PER_MSEC);
> +
> +	dma_unmap_single(edev->parent, dma, length, DMA_TO_DEVICE);
> +
> +	if (ret == -ETIMEDOUT)
> +		dev_dbg(xgmac->dev, "%s: TX timeout 0x%08x\n", __func__, des3);
> +
> +	return ret;
> +}
> +
> +static void xgmac_recv(struct eth_device *edev)
> +{
> +	struct xgmac_priv *xgmac = to_xgmac(edev);
> +	struct xgmac_desc *rx_desc;
> +	dma_addr_t dma;
> +	void *pkt;
> +	int length;
> +
> +	rx_desc = &xgmac->rx_descs[xgmac->rx_desc_idx];
> +
> +	if (rx_desc->des3 & XGMAC_DESC3_OWN)
> +		return;
> +
> +	dma = xgmac->dma_rx_buf_phys[xgmac->rx_desc_idx];
> +	pkt = xgmac->dma_rx_buf_virt[xgmac->rx_desc_idx];
> +	length = rx_desc->des3 & XGMAC_RDES3_PKT_LENGTH_MASK;
> +
> +	dma_sync_single_for_cpu(edev->parent, dma, length, DMA_FROM_DEVICE);
> +	net_receive(edev, pkt, length);
> +	dma_sync_single_for_device(edev->parent, dma, length, DMA_FROM_DEVICE);
> +
> +	/* Read Format RX descriptor */
> +	rx_desc = &xgmac->rx_descs[xgmac->rx_desc_idx];
> +	rx_desc->des0 = lower_32_bits(dma);
> +	rx_desc->des1 = upper_32_bits(dma);

Is this necessary? I would expect the same address is still in the
descriptor ring.

> +	rx_desc->des2 = 0;
> +	/*
> +	 * Make sure that if HW sees the _OWN write below, it will see all the
> +	 * writes to the rest of the descriptor too.
> +	 */
> +	rx_desc->des3 = XGMAC_DESC3_OWN;
> +	barrier();
> +
> +	writel(xgmac->rx_descs_phys + (xgmac->rx_desc_idx * XGMAC_DESCRIPTOR_SIZE),
> +	       &xgmac->dma_regs->ch0_rxdesc_tail_pointer);
> +
> +	xgmac->rx_desc_idx++;
> +	xgmac->rx_desc_idx %= XGMAC_DESCRIPTORS_NUM;
> +}
> +
> +static int xgmac_probe_resources_core(struct xgmac_priv *xgmac)
> +{
> +	unsigned int desc_step;
> +	int ret = 0;
> +	void *p;
> +	int i;
> +
> +	/* Maximum distance between neighboring descriptors, in Bytes. */
> +	desc_step = sizeof(struct xgmac_desc);
> +
> +	xgmac->tx_descs = dma_alloc_coherent(DMA_DEVICE_BROKEN,
> +					     XGMAC_DESCRIPTORS_SIZE,
> +					     &xgmac->tx_descs_phys);

No need to pass DMA_DEVICE_BROKEN. You should have a device available
here.

> +	if (!xgmac->tx_descs) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	xgmac->rx_descs = dma_alloc_coherent(DMA_DEVICE_BROKEN,
> +					     XGMAC_DESCRIPTORS_SIZE,
> +					     &xgmac->rx_descs_phys);
> +	if (!xgmac->rx_descs) {
> +		ret = -ENOMEM;
> +		goto err_free_tx_descs;
> +	}
> +
> +	p = dma_alloc(XGMAC_RX_BUFFER_SIZE);
> +	if (!p)
> +		goto err_free_descs;
> +
> +	for (i = 0; i < XGMAC_DESCRIPTORS_NUM; i++) {
> +		struct xgmac_desc *rx_desc = &xgmac->rx_descs[i];
> +		dma_addr_t dma;
> +
> +		dma = dma_map_single(xgmac->dev, p, XGMAC_MAX_PACKET_SIZE, DMA_FROM_DEVICE);
> +		if (dma_mapping_error(xgmac->dev, dma)) {
> +			ret = -EFAULT;
> +			goto err_free_dma_bufs;
> +		}
> +
> +		rx_desc->des0 = lower_32_bits(dma);
> +		rx_desc->des1 = upper_32_bits(dma);
> +		xgmac->dma_rx_buf_phys[i] = dma;
> +		xgmac->dma_rx_buf_virt[i] = p;
> +
> +		p += XGMAC_MAX_PACKET_SIZE;
> +	}
> +
> +	return 0;
> +
> +err_free_dma_bufs:
> +	dma_free(phys_to_virt(xgmac->rx_descs[0].des0));

You should unmap before freeing this. Also you have the virtual pointer
available, please use this instead of phys_to_virt().

> +err_free_descs:
> +	dma_free_coherent(DMA_DEVICE_BROKEN, xgmac->rx_descs, xgmac->rx_descs_phys,
> +			  XGMAC_DESCRIPTORS_SIZE);
> +err_free_tx_descs:
> +	dma_free_coherent(DMA_DEVICE_BROKEN, xgmac->tx_descs, xgmac->tx_descs_phys,
> +			  XGMAC_DESCRIPTORS_SIZE);
> +err:
> +
> +	return ret;
> +}
> +
> +static int xgmac_remove_resources_core(struct xgmac_priv *xgmac)
> +{
> +	dma_free(xgmac->rx_dma_buf);

Please unmap before freeing.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |