Re: [PATCH 2/2] net: Add fsl_enetc network driver support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Sascha,


On 03.01.24 15:51, Sascha Hauer wrote:
> +static int enetc_ls1028a_write_hwaddr(struct eth_device *edev, const unsigned char *mac)
> +{
> +	struct enetc_priv *priv = edev->priv;
> +	struct pci_dev *pdev = to_pci_dev(priv->dev);
> +	const int devfn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
> +	int devfn = PCI_FUNC(pdev->devfn);
> +	u32 lower, upper;
> +	int pf;
> +
> +	if (devfn >= ARRAY_SIZE(devfn_to_pf))
> +		return 0;
> +
> +	pf = devfn_to_pf[devfn];
> +	if (pf < 0)
> +		return 0;
> +
> +	lower = *(const u16 *)(mac + 4);
> +	upper = *(const u32 *)mac;

Please use get_unaligned accessors or similar to avoid alignment
issues.

> +
> +	out_le32(LS1028A_IERB_PSIPMAR0(pf, 0), upper);
> +	out_le32(LS1028A_IERB_PSIPMAR1(pf, 0), lower);
> +
> +	return 0;
> +}
> +
> +static int enetc_write_hwaddr(struct eth_device *edev, const unsigned char *mac)
> +{
> +	struct enetc_priv *priv = edev->priv;
> +
> +	u16 lower = *(const u16 *)(mac + 4);
> +	u32 upper = *(const u32 *)mac;

Same.

> +	/* prepare Tx BD */
> +	memset(&priv->enetc_txbd[pi], 0x0, sizeof(struct enetc_tx_bd));
> +	priv->enetc_txbd[pi].addr = cpu_to_le64(dma);
> +	priv->enetc_txbd[pi].buf_len = cpu_to_le16(length);
> +	priv->enetc_txbd[pi].frm_len = cpu_to_le16(length);
> +	priv->enetc_txbd[pi].flags = cpu_to_le16(ENETC_TXBD_FLAGS_F);
> +
> +	dmb();

I am not fond of adding these memory barriers. I was under the impression that
the way we map memory means that accesses are already serialized and a compiler
barrier (or volatile accesses) would suffice here?

> +
> +	/* send frame: increment producer index */
> +	pi = (pi + 1) % txr->bd_count;
> +	txr->next_prod_idx = pi;
> +	enetc_write_reg(txr->prod_idx, pi);
> +
> +	start = get_time_ns();
> +
> +	while (1) {
> +		if (is_timeout(start, 100 * USECOND)) {
> +			ret = -ETIMEDOUT;
> +			break;
> +		}
> +
> +		if (pi == (enetc_read_reg(txr->cons_idx) & ENETC_BDR_IDX_MASK)) {
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	dma_unmap_single(priv->dev, dma, length, DMA_TO_DEVICE);
> +
> +	return ret;
> +}
> +
> +/*
> + * Receive frame:
> + * - wait for the next BD to get ready bit set
> + * - clean up the descriptor
> + * - move on and indicate to HW that the cleaned BD is available for Rx
> + */
> +static int enetc_recv(struct eth_device *edev)
> +{
> +	struct enetc_priv *priv = edev->priv;
> +	struct bd_ring *rxr = &priv->rx_bdr;
> +	int pi = rxr->next_prod_idx;
> +	int ci = rxr->next_cons_idx;
> +	u32 status;
> +	void *pkg;
> +	int len;
> +
> +	status = le32_to_cpu(priv->enetc_rxbd[pi].r.lstatus);
> +
> +	/* check if current BD is ready to be consumed */
> +	if (!ENETC_RXBD_STATUS_R(status))
> +		return 0;
> +
> +	dmb();
> +
> +	len = le16_to_cpu(priv->enetc_rxbd[pi].r.buf_len);
> +
> +	dev_dbg(&edev->dev, "RxBD[%d]: len=%d err=%d pkt=0x%p\n", pi, len,
> +		  ENETC_RXBD_STATUS_ERRORS(status), pkg);
> +
> +	dma_sync_single_for_cpu(priv->dev, priv->rx_pkg_phys[pi], PKTSIZE, DMA_FROM_DEVICE);
> +	net_receive(edev, priv->rx_pkg[pi], len);
> +	dma_sync_single_for_device(priv->dev, priv->rx_pkg_phys[pi], PKTSIZE, DMA_FROM_DEVICE);
> +
> +	/* BD clean up and advance to next in ring */
> +	memset(&priv->enetc_rxbd[pi], 0, sizeof(union enetc_rx_bd));
> +	priv->enetc_rxbd[pi].w.addr = priv->rx_pkg_phys[pi];
> +	rxr->next_prod_idx = (pi + 1) % rxr->bd_count;
> +	ci = (ci + 1) % rxr->bd_count;
> +	rxr->next_cons_idx = ci;
> +	dmb();
> +	/* free up the slot in the ring for HW */
> +	enetc_write_reg(rxr->cons_idx, ci);
> +
> +	return 0;
> +}
> +
> +static int enetc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct enetc_priv *priv;
> +	struct eth_device *edev;
> +	int ret;
> +
> +	pci_enable_device(pdev);
> +	pci_set_master(pdev);
> +
> +	priv = xzalloc(sizeof(*priv));
> +	priv->dev = dev;
> +
> +	priv->enetc_txbd = dma_alloc_coherent(sizeof(struct enetc_tx_bd) * ENETC_BD_CNT, NULL);
> +	priv->enetc_rxbd = dma_alloc_coherent(sizeof(union enetc_rx_bd) * ENETC_BD_CNT, NULL);

Can you store the DMA address and use that instead of assuming 1:1 mapping?

> +
> +	if (!priv->enetc_txbd || !priv->enetc_rxbd) {
> +		/* free should be able to handle NULL, just free all pointers */
> +		free(priv->enetc_txbd);
> +		free(priv->enetc_rxbd);

dma_alloc_coherent should be freed with dma_free_coherent.

> +
> +		return -ENOMEM;
> +	}
> +
> +	/* initialize register */
> +	priv->regs_base = pci_iomap(pdev, 0);
> +	if (!priv->regs_base) {
> +		dev_err(dev, "failed to map BAR0\n");
> +		return -EINVAL;
> +	}
> +
> +	edev = &priv->edev;
> +	dev->priv = priv;
> +	edev->priv = priv;
> +	edev->open = enetc_start;
> +	edev->send = enetc_send;
> +	edev->recv = enetc_recv;
> +	edev->halt = enetc_stop;
> +	edev->get_ethaddr = enetc_get_hwaddr;
> +
> +	if (of_machine_is_compatible("fsl,ls1028a"))
> +		edev->set_ethaddr = enetc_ls1028a_write_hwaddr;
> +	else
> +		edev->set_ethaddr = enetc_write_hwaddr;
> +
> +	edev->parent = dev;
> +
> +	priv->port_regs = priv->regs_base + ENETC_PORT_REGS_OFF;
> +
> +	enetc_start_pcs(&priv->edev);
> +
> +	ret = eth_register(edev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

You could directly return eth_register(edev) here.

> +static struct pci_driver enetc_eth_driver = {
> +        .name = "fsl_enetc",
> +        .id_table = enetc_pci_tbl,
> +        .probe = enetc_probe,

Don't we need a remove function here to quiesce the device's DMA?

> +};
> +device_pci_driver(enetc_eth_driver);
> diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
> new file mode 100644
> index 0000000000..d79b0a337c
> --- /dev/null
> +++ b/drivers/net/fsl_enetc.h
> @@ -0,0 +1,249 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * ENETC ethernet controller driver
> + * Copyright 2017-2021 NXP
> + */
> +
> +#ifndef _ENETC_H
> +#define _ENETC_H
> +
> +#include <linux/bitops.h>

Nitpick: <linux/bits.h> has narrower scope and should suffice.

> +struct enetc_mdio_priv {
> +	void *regs_base;

__iomem

> +	struct mii_bus bus;
> +};

> +static void enetc_mdio_wait_bsy(struct enetc_mdio_priv *priv)
> +{
> +	int to = 10000;
> +
> +	while ((enetc_read(priv, ENETC_MDIO_CFG) & ENETC_EMDIO_CFG_BSY) &&
> +	       --to)
> +		cpu_relax();
> +	if (!to)
> +		printf("T");

This will run in pollers and should not print to stdout.


Cheers,
Ahmad

> +};
> +device_pci_driver(enetc_mdio_driver);

-- 
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 |