lkml.org 
[lkml]   [2021]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] net: Add driver for LiteX's LiteETH network interface
    On Fri,  6 Aug 2021 15:19:04 +0930 Joel Stanley wrote:
    > LiteX is a soft system-on-chip that targets FPGAs. LiteETH is a basic
    > network device that is commonly used in LiteX designs.
    >
    > The driver was first written in 2017 and has been maintained by the
    > LiteX community in various trees. Thank you to all who have contributed.

    > +config NET_VENDOR_LITEX
    > + bool "LiteX devices"
    > + default y
    > + help
    > + If you have a network (Ethernet) card belonging to this class, say Y.
    > +
    > + Note that the answer to this question doesn't directly affect the
    > + kernel: saying N will just cause the configurator to skip all
    > + the questions about LiteX devices. If you say Y, you will be asked
    > + for your specific card in the following questions.

    Maybe mention where the device is usually found (FPGAs) like you did in
    the commit message, to help folks make a decision here?

    > +config LITEX_LITEETH
    > + tristate "LiteX Ethernet support"
    > + help
    > + If you wish to compile a kernel for hardware with a LiteX LiteEth
    > + device then you should answer Y to this.

    > +struct liteeth {
    > + void __iomem *base;
    > + void __iomem *mdio_base;
    > + struct net_device *netdev;
    > + struct device *dev;
    > + struct mii_bus *mii_bus;

    unused field

    > +
    > + /* Link management */
    > + int cur_duplex;
    > + int cur_speed;
    > +
    > + /* Tx */
    > + int tx_slot;
    > + int num_tx_slots;
    > + void __iomem *tx_base;
    > +
    > + /* Rx */
    > + int rx_slot;
    > + int num_rx_slots;
    > + void __iomem *rx_base;
    > +};
    > +
    > +
    > +static int liteeth_rx(struct net_device *netdev)
    > +{
    > + struct liteeth *priv = netdev_priv(netdev);
    > + struct sk_buff *skb;
    > + unsigned char *data;
    > + u8 rx_slot;
    > + int len;
    > +
    > + rx_slot = readb(priv->base + LITEETH_WRITER_SLOT);
    > + len = readl(priv->base + LITEETH_WRITER_LENGTH);
    > +
    > + skb = netdev_alloc_skb(netdev, len + NET_IP_ALIGN);

    netdev_alloc_skb_ip_align() ...

    > + if (!skb) {
    > + netdev_err(netdev, "couldn't get memory");

    \n at the end? You can skip it but be consistent across messages

    > + netdev->stats.rx_dropped++;
    > + return NET_RX_DROP;
    > + }
    > +
    > + /* Ensure alignemnt of the ip header within the skb */
    > + skb_reserve(skb, NET_IP_ALIGN);

    ... then skip this

    > + if (len == 0 || len > 2048)
    > + return NET_RX_DROP;

    Should this be counted somehow?

    > + data = skb_put(skb, len);
    > + memcpy_fromio(data, priv->rx_base + rx_slot * LITEETH_BUFFER_SIZE, len);
    > + skb->protocol = eth_type_trans(skb, netdev);
    > +
    > + netdev->stats.rx_packets++;
    > + netdev->stats.rx_bytes += len;
    > +
    > + return netif_rx(skb);
    > +}
    > +
    > +static irqreturn_t liteeth_interrupt(int irq, void *dev_id)
    > +{
    > + struct net_device *netdev = dev_id;
    > + struct liteeth *priv = netdev_priv(netdev);
    > + u8 reg;
    > +
    > + reg = readb(priv->base + LITEETH_READER_EV_PENDING);
    > + if (reg) {
    > + netdev->stats.tx_packets++;
    > + writeb(reg, priv->base + LITEETH_READER_EV_PENDING);
    > + }
    > +
    > + reg = readb(priv->base + LITEETH_WRITER_EV_PENDING);
    > + if (reg) {
    > + liteeth_rx(netdev);
    > + writeb(reg, priv->base + LITEETH_WRITER_EV_PENDING);
    > + }
    > +
    > + return IRQ_HANDLED;
    > +}
    > +
    > +static int liteeth_open(struct net_device *netdev)
    > +{
    > + struct liteeth *priv = netdev_priv(netdev);
    > + int err;
    > +
    > + /* Clear pending events */
    > + writeb(1, priv->base + LITEETH_WRITER_EV_PENDING);
    > + writeb(1, priv->base + LITEETH_READER_EV_PENDING);
    > +
    > + err = request_irq(netdev->irq, liteeth_interrupt, 0, netdev->name, netdev);
    > + if (err) {
    > + netdev_err(netdev, "failed to request irq %d\n", netdev->irq);
    > + return err;
    > + }
    > +
    > + /* Enable IRQs */
    > + writeb(1, priv->base + LITEETH_WRITER_EV_ENABLE);
    > + writeb(1, priv->base + LITEETH_READER_EV_ENABLE);
    > +
    > + /* TODO: Remove these once we have working mdio support */
    > + priv->cur_duplex = DUPLEX_FULL;
    > + priv->cur_speed = SPEED_100;

    please remove the fields until they're actually used

    > + netif_carrier_on(netdev);
    > +
    > + netif_start_queue(netdev);
    > +
    > + return 0;
    > +}
    > +
    > +static int liteeth_stop(struct net_device *netdev)
    > +{
    > + struct liteeth *priv = netdev_priv(netdev);

    carrier_off() for symmetry?

    > + netif_stop_queue(netdev);
    > +
    > + writeb(0, priv->base + LITEETH_WRITER_EV_ENABLE);
    > + writeb(0, priv->base + LITEETH_READER_EV_ENABLE);
    > +
    > + free_irq(netdev->irq, netdev);
    > + return 0;
    > +}
    > +
    > +static int liteeth_start_xmit(struct sk_buff *skb, struct net_device *netdev)
    > +{
    > + struct liteeth *priv = netdev_priv(netdev);
    > + void __iomem *txbuffer;
    > + int ret;
    > + u8 val;
    > +
    > + /* Reject oversize packets */
    > + if (unlikely(skb->len > MAX_PKT_SIZE)) {
    > + if (net_ratelimit())
    > + netdev_dbg(netdev, "tx packet too big\n");
    > + goto drop;
    > + }
    > +
    > + txbuffer = priv->tx_base + priv->tx_slot * LITEETH_BUFFER_SIZE;
    > + memcpy_toio(txbuffer, skb->data, skb->len);
    > + writeb(priv->tx_slot, priv->base + LITEETH_READER_SLOT);
    > + writew(skb->len, priv->base + LITEETH_READER_LENGTH);
    > +
    > + ret = readl_poll_timeout_atomic(priv->base + LITEETH_READER_READY, val, val, 5, 1000);

    Why the need for poll if there is an interrupt?
    Why not stop the Tx queue once you're out of slots and restart
    it when the completion interrupt comes?

    > + if (ret == -ETIMEDOUT) {
    > + netdev_err(netdev, "LITEETH_READER_READY timed out\n");

    ratelimit this as well, please

    > + goto drop;
    > + }
    > +
    > + writeb(1, priv->base + LITEETH_READER_START);
    > +
    > + netdev->stats.tx_bytes += skb->len;

    Please count bytes and packets in the same place

    > + priv->tx_slot = (priv->tx_slot + 1) % priv->num_tx_slots;
    > + dev_kfree_skb_any(skb);
    > + return NETDEV_TX_OK;
    > +drop:
    > + /* Drop the packet */
    > + dev_kfree_skb_any(skb);
    > + netdev->stats.tx_dropped++;
    > +
    > + return NETDEV_TX_OK;
    > +}

    > +static int liteeth_probe(struct platform_device *pdev)
    > +{
    > + struct net_device *netdev;
    > + void __iomem *buf_base;
    > + struct resource *res;
    > + struct liteeth *priv;
    > + int irq, err;
    > +
    > + netdev = alloc_etherdev(sizeof(*priv));
    > + if (!netdev)
    > + return -ENOMEM;
    > +
    > + priv = netdev_priv(netdev);
    > + priv->netdev = netdev;
    > + priv->dev = &pdev->dev;
    > +
    > + irq = platform_get_irq(pdev, 0);
    > + if (irq < 0) {
    > + dev_err(&pdev->dev, "Failed to get IRQ\n");
    > + goto err;

    `err` variable is not set here, you'd return 0

    > + }
    > +
    > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    > + priv->base = devm_ioremap_resource(&pdev->dev, res);
    > + if (IS_ERR(priv->base)) {
    > + err = PTR_ERR(priv->base);
    > + goto err;
    > + }
    > +
    > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
    > + priv->mdio_base = devm_ioremap_resource(&pdev->dev, res);
    > + if (IS_ERR(priv->mdio_base)) {
    > + err = PTR_ERR(priv->mdio_base);
    > + goto err;
    > + }
    > +
    > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
    > + buf_base = devm_ioremap_resource(&pdev->dev, res);
    > + if (IS_ERR(buf_base)) {
    > + err = PTR_ERR(buf_base);
    > + goto err;
    > + }
    > +
    > + err = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth",
    > + &priv->num_rx_slots);

    Please run checkpatch --strict and fix what it points out

    > + if (err) {
    > + dev_err(&pdev->dev, "unable to get rx-fifo-depth\n");
    > + goto err;
    > + }

    > + return 0;
    > +err:
    > + free_netdev(netdev);
    > + return err;
    > +}

    \
     
     \ /
      Last update: 2021-08-07 01:11    [W:4.918 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site