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 |
