LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Joel Stanley <joel@jms.id.au>
Cc: "David S . Miller" <davem@davemloft.net>,
	Rob Herring <robh+dt@kernel.org>,
	Stafford Horne <shorne@gmail.com>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Anton Blanchard <anton@ozlabs.org>,
	Gabriel Somlo <gsomlo@gmail.com>, David Shah <dave@ds0.me>,
	Karol Gugala <kgugala@antmicro.com>,
	Mateusz Holenko <mholenko@antmicro.com>,
	devicetree@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] net: Add driver for LiteX's LiteETH network interface
Date: Fri, 6 Aug 2021 16:10:30 -0700	[thread overview]
Message-ID: <20210806161030.52a7ae93@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210806054904.534315-3-joel@jms.id.au>

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;
> +}

  parent reply	other threads:[~2021-08-06 23:10 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-06  5:49 [PATCH 0/2] net: Add LiteETH network driver Joel Stanley
2021-08-06  5:49 ` [PATCH 1/2] dt-bindings: net: Add bindings for LiteETH Joel Stanley
2021-08-06 17:07   ` Rob Herring
2021-08-07 19:05   ` Andrew Lunn
2021-08-09  7:59     ` Joel Stanley
2021-08-09 13:27       ` Andrew Lunn
2021-08-06  5:49 ` [PATCH 2/2] net: Add driver for LiteX's LiteETH network interface Joel Stanley
2021-08-06 11:48   ` Gabriel L. Somlo
     [not found]     ` <CA+96J6JxAkNd_QkVxfSdAJwWVLJTtSaDsCmVKw3KBfYySJytKA@mail.gmail.com>
2021-08-20  7:46       ` Joel Stanley
2021-08-06 23:10   ` Jakub Kicinski [this message]
2021-08-09 12:03     ` Joel Stanley
2021-08-09 16:16       ` Jakub Kicinski
2021-08-07 19:19   ` Andrew Lunn
2021-08-09  3:20     ` Joel Stanley
2021-08-09 13:09       ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210806161030.52a7ae93@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=anton@ozlabs.org \
    --cc=benh@kernel.crashing.org \
    --cc=dave@ds0.me \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=gsomlo@gmail.com \
    --cc=joel@jms.id.au \
    --cc=kgugala@antmicro.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mholenko@antmicro.com \
    --cc=netdev@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=shorne@gmail.com \
    --subject='Re: [PATCH 2/2] net: Add driver for LiteX'\''s LiteETH network interface' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).