On Sun, Jun 23, 2024 at 06:19:57PM +0200, Lorenzo Bianconi wrote:
> Add airoha_eth driver in order to introduce ethernet support for
> Airoha EN7581 SoC available on EN7581 development board (en7581-evb).
> en7581-evb networking architecture is composed by airoha_eth as mac
> controller (cpu port) and a mt7530 dsa based switch.
> EN7581 mac controller is mainly composed by Frame Engine (FE) and
> QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic
> functionalities are supported now) while QDMA is used for DMA operation
> and QOS functionalities between mac layer and the dsa switch (hw QoS is
> not available yet and it will be added in the future).
> Currently only hw lan features are available, hw wan will be added with
> subsequent patches.
>
> Tested-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
Hi Lorenzo,
Some minor nits from my side.
...
> diff --git a/drivers/net/ethernet/mediatek/airoha_eth.c b/drivers/net/ethernet/mediatek/airoha_eth.c
...
> +#define airoha_fe_rr(eth, offset) airoha_rr((eth)->fe_regs, (offset))
> +#define airoha_fe_wr(eth, offset, val) airoha_wr((eth)->fe_regs, (offset), (val))
> +#define airoha_fe_rmw(eth, offset, mask, val) airoha_rmw((eth)->fe_regs, (offset), (mask), (val))
> +#define airoha_fe_set(eth, offset, val) airoha_rmw((eth)->fe_regs, (offset), 0, (val))
> +#define airoha_fe_clear(eth, offset, val) airoha_rmw((eth)->fe_regs, (offset), (val), 0)
> +
> +#define airoha_qdma_rr(eth, offset) airoha_rr((eth)->qdma_regs, (offset))
> +#define airoha_qdma_wr(eth, offset, val) airoha_wr((eth)->qdma_regs, (offset), (val))
> +#define airoha_qdma_rmw(eth, offset, mask, val) airoha_rmw((eth)->qdma_regs, (offset), (mask), (val))
> +#define airoha_qdma_set(eth, offset, val) airoha_rmw((eth)->qdma_regs, (offset), 0, (val))
> +#define airoha_qdma_clear(eth, offset, val) airoha_rmw((eth)->qdma_regs, (offset), (val), 0)
nit: Please consider line-wrapping the above so lines are 80 columns wide
or less, which is still preferred in Networking code.
Flagged by checkpatch.pl --max-line-length=80
...
> +static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct skb_shared_info *sinfo = skb_shinfo(skb);
> + struct airoha_eth *eth = netdev_priv(dev);
> + int i, qid = skb_get_queue_mapping(skb);
> + u32 nr_frags, msg0 = 0, msg1;
> + u32 len = skb_headlen(skb);
> + struct netdev_queue *txq;
> + struct airoha_queue *q;
> + void *data = skb->data;
> + u16 index;
> +
> + if (skb->ip_summed == CHECKSUM_PARTIAL)
> + msg0 |= FIELD_PREP(QDMA_ETH_TXMSG_TCO_MASK, 1) |
> + FIELD_PREP(QDMA_ETH_TXMSG_UCO_MASK, 1) |
> + FIELD_PREP(QDMA_ETH_TXMSG_ICO_MASK, 1);
> +
> + /* TSO: fill MSS info in tcp checksum field */
> + if (skb_is_gso(skb)) {
> + if (skb_cow_head(skb, 0))
> + goto error;
> +
> + if (sinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
> + tcp_hdr(skb)->check = cpu_to_be16(sinfo->gso_size);
Probably we could do better with an appropriate helper - perhaps
there is one I couldn't find one - but I think you need a cast here
to keep Sparse happy.
Something like this (completely untested!):
tcp_hdr(skb)->check =
(__force __sum16)cpu_to_be16(sinfo->gso_size);
...
> +static int airoha_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct net_device *dev;
> + struct airoha_eth *eth;
> + int err;
> +
> + dev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*eth),
> + AIROHA_NUM_TX_RING, AIROHA_NUM_RX_RING);
> + if (!dev) {
> + dev_err(&pdev->dev, "alloc_etherdev failed\n");
> + return -ENOMEM;
> + }
> +
> + eth = netdev_priv(dev);
> + eth->net_dev = dev;
> +
> + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (err) {
> + dev_err(&pdev->dev, "failed configuring DMA mask\n");
> + return err;
> + }
> +
> + eth->fe_regs = devm_platform_ioremap_resource_byname(pdev, "fe");
> + if (IS_ERR(eth->fe_regs))
> + return dev_err_probe(&pdev->dev, PTR_ERR(eth->fe_regs),
> + "failed to iomap fe regs\n");
> +
> + eth->qdma_regs = devm_platform_ioremap_resource_byname(pdev, "qdma0");
> + if (IS_ERR(eth->qdma_regs))
> + return dev_err_probe(&pdev->dev, PTR_ERR(eth->qdma_regs),
> + "failed to iomap qdma regs\n");
> +
> + eth->rsts[0].id = "fe";
> + eth->rsts[1].id = "pdma";
> + eth->rsts[2].id = "qdma";
> + err = devm_reset_control_bulk_get_exclusive(&pdev->dev,
> + ARRAY_SIZE(eth->rsts),
> + eth->rsts);
> + if (err) {
> + dev_err(&pdev->dev, "failed to get bulk reset lines\n");
> + return err;
> + }
> +
> + eth->xsi_rsts[0].id = "xsi-mac";
> + eth->xsi_rsts[1].id = "hsi0-mac";
> + eth->xsi_rsts[2].id = "hsi1-mac";
> + eth->xsi_rsts[3].id = "hsi-mac";
> + eth->xsi_rsts[4].id = "xfp-mac";
> + err = devm_reset_control_bulk_get_exclusive(&pdev->dev,
> + ARRAY_SIZE(eth->xsi_rsts),
> + eth->xsi_rsts);
> + if (err) {
> + dev_err(&pdev->dev, "failed to get bulk xsi reset lines\n");
> + return err;
> + }
> +
> + spin_lock_init(ð->irq_lock);
> + eth->irq = platform_get_irq(pdev, 0);
> + if (eth->irq < 0) {
> + dev_err(&pdev->dev, "failed reading irq line\n");
Coccinelle says:
.../airoha_eth.c:1698:2-9: line 1698 is redundant because platform_get_irq() already prints an error
...
> +const struct of_device_id of_airoha_match[] = {
> + { .compatible = "airoha,en7581-eth" },
> + { /* sentinel */ }
> +};
of_airoha_match appears to only be used in this file.
If so, it should be static.
Flagged by Sparse.
> +
> +static struct platform_driver airoha_driver = {
> + .probe = airoha_probe,
> + .remove_new = airoha_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = of_airoha_match,
> + },
> +};
> +module_platform_driver(airoha_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Lorenzo Bianconi <lorenzo@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Ethernet driver for Airoha SoC");
> diff --git a/drivers/net/ethernet/mediatek/airoha_eth.h b/drivers/net/ethernet/mediatek/airoha_eth.h
> new file mode 100644
> index 000000000000..f7b984be4d60
> --- /dev/null
> +++ b/drivers/net/ethernet/mediatek/airoha_eth.h
> @@ -0,0 +1,793 @@
> +// SPDX-License-Identifier: GPL-2.0
The correct SPDX header comment style for .h (but not .c) files is /* ... */
https://docs.kernel.org/6.9/process/license-rules.html#license-identifier-syntax
Flagged by checkpatch
...
