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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7249 bytes --]

> 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@genexis.eu>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Hi Lorenzo,
> 
> Some minor nits from my side.

Hi Simon,

thx for the review.

> 
> ...
> 
> > 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

ack, I will fix it in v4.

> 
> ...
> 
> > +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);
> 
> ...

ack, I will fix it in v4.

> 
> > +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
> 
> ...

ack, I will fix it in v4.

> 
> > +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.

ack, I will fix it in v4.

> 
> > +
> > +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@kernel.org>");
> > +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

ack, I will fix it in v4.

Regards,
Lorenzo

> 
> ...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]