Subject: Re: [PATCH v3 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

From: Simon Horman <[email-masked]>
Date: Wed, 26 Jun 2024 21:18:35 +0100

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 <[email-masked]>
> Signed-off-by: Lorenzo Bianconi <[email-masked]>

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(&eth->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 <[email-masked]>");
> +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

...