On Thu, May 14, 2020 at 10:00 AM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>
> This adds the driver for the MediaTek Ethernet MAC used on the MT8* SoC
> family. For now we only support full-duplex.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
Looks very nice overall. Just a few things I noticed, and some ideas
that may or may not make sense:
> +/* This is defined to 0 on arm64 in arch/arm64/include/asm/processor.h but
> + * this IP doesn't work without this alignment being equal to 2.
> + */
> +#ifdef NET_IP_ALIGN
> +#undef NET_IP_ALIGN
> +#endif
> +#define NET_IP_ALIGN 2
Maybe you should just define your own macro instead of replacing
the normal one then?
> +static void mtk_mac_lock(struct mtk_mac_priv *priv)
> +{
> + spin_lock_irqsave(&priv->lock, priv->lock_flags);
> +}
> +
> +static void mtk_mac_unlock(struct mtk_mac_priv *priv)
> +{
> + spin_unlock_irqrestore(&priv->lock, priv->lock_flags);
> +}
This looks wrong: you should not have shared 'flags' passed into
spin_lock_irqsave(), and I don't even see a need to use the
irqsave variant of the lock in the first place.
Maybe start by open-coding the lock and remove the wrappers
above.
Then see if you can use a cheaper spin_lock_bh() or plain spin_lock()
instead of irqsave.
Finally, see if this can be done in a lockless way by relying on
appropriate barriers and separating the writers into separate
cache lines. From a brief look at the driver I think it can be done
without too much trouble.
> +static unsigned int mtk_mac_intr_read_and_clear(struct mtk_mac_priv *priv)
> +{
> + unsigned int val;
> +
> + regmap_read(priv->regs, MTK_MAC_REG_INT_STS, &val);
> + regmap_write(priv->regs, MTK_MAC_REG_INT_STS, val);
> +
> + return val;
> +}
Do you actually need to read the register? That is usually a relatively
expensive operation, so if possible try to use clear the bits when
you don't care which bits were set.
> +/* All processing for TX and RX happens in the napi poll callback. */
> +static irqreturn_t mtk_mac_handle_irq(int irq, void *data)
> +{
> + struct mtk_mac_priv *priv;
> + struct net_device *ndev;
> +
> + ndev = data;
> + priv = netdev_priv(ndev);
> +
> + if (netif_running(ndev)) {
> + mtk_mac_intr_mask_all(priv);
> + napi_schedule(&priv->napi);
> + }
> +
> + return IRQ_HANDLED;
> +static int mtk_mac_netdev_start_xmit(struct sk_buff *skb,
> + struct net_device *ndev)
> +{
> + struct mtk_mac_priv *priv = netdev_priv(ndev);
> + struct mtk_mac_ring *ring = &priv->tx_ring;
> + struct device *dev = mtk_mac_get_dev(priv);
> + struct mtk_mac_ring_desc_data desc_data;
> +
> + desc_data.dma_addr = mtk_mac_dma_map_tx(priv, skb);
> + if (dma_mapping_error(dev, desc_data.dma_addr))
> + goto err_drop_packet;
> +
> + desc_data.skb = skb;
> + desc_data.len = skb->len;
> +
> + mtk_mac_lock(priv);
> + mtk_mac_ring_push_head_tx(ring, &desc_data);
> +
> + if (mtk_mac_ring_full(ring))
> + netif_stop_queue(ndev);
> + mtk_mac_unlock(priv);
> +
> + mtk_mac_dma_resume_tx(priv);
> +
> + return NETDEV_TX_OK;
> +
> +err_drop_packet:
> + dev_kfree_skb(skb);
> + ndev->stats.tx_dropped++;
> + return NETDEV_TX_BUSY;
> +}
I would always add BQL flow control in new drivers, using
netdev_sent_queue here...
> +static int mtk_mac_tx_complete_one(struct mtk_mac_priv *priv)
> +{
> + struct mtk_mac_ring *ring = &priv->tx_ring;
> + struct mtk_mac_ring_desc_data desc_data;
> + int ret;
> +
> + ret = mtk_mac_ring_pop_tail(ring, &desc_data);
> + if (ret)
> + return ret;
> +
> + mtk_mac_dma_unmap_tx(priv, &desc_data);
> + dev_kfree_skb_irq(desc_data.skb);
> +
> + return 0;
> +}
... and netdev_completed_queue() here.
> +static void mtk_mac_tx_complete_all(struct mtk_mac_priv *priv)
> +{
> + struct mtk_mac_ring *ring = &priv->tx_ring;
> + struct net_device *ndev = priv->ndev;
> + int ret;
> +
> + for (;;) {
> + mtk_mac_lock(priv);
> +
> + if (!mtk_mac_ring_descs_available(ring)) {
> + mtk_mac_unlock(priv);
> + break;
> + }
> +
> + ret = mtk_mac_tx_complete_one(priv);
> + if (ret) {
> + mtk_mac_unlock(priv);
> + break;
> + }
> +
> + if (netif_queue_stopped(ndev))
> + netif_wake_queue(ndev);
> +
> + mtk_mac_unlock(priv);
> + }
> +}
It looks like most of the stuff inside of the loop can be pulled out
and only done once here.
> +static int mtk_mac_poll(struct napi_struct *napi, int budget)
> +{
> + struct mtk_mac_priv *priv;
> + unsigned int status;
> + int received = 0;
> +
> + priv = container_of(napi, struct mtk_mac_priv, napi);
> +
> + status = mtk_mac_intr_read_and_clear(priv);
> +
> + /* Clean up TX */
> + if (status & MTK_MAC_BIT_INT_STS_TNTC)
> + mtk_mac_tx_complete_all(priv);
> +
> + /* Receive up to $budget packets */
> + if (status & MTK_MAC_BIT_INT_STS_FNRC)
> + received = mtk_mac_process_rx(priv, budget);
> +
> + /* One of the counter reached 0x8000000 - update stats and reset all
> + * counters.
> + */
> + if (status & MTK_MAC_REG_INT_STS_MIB_CNT_TH) {
> + mtk_mac_update_stats(priv);
> + mtk_mac_reset_counters(priv);
> + }
> +
> + if (received < budget)
> + napi_complete_done(napi, received);
> +
> + mtk_mac_intr_unmask_all(priv);
> +
> + return received;
> +}
I think you want to leave (at least some of) the interrupts masked
if your budget is exhausted, to avoid generating unnecessary
irqs.
It may also be faster to not mask/unmask at all but just
clear the interrupts that you have finished processing
Arnd
