On Tue, Jun 18, 2024, at 09:49, Lorenzo Bianconi wrote:

quoted

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 <redacted>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

I noticed a few small things that you may want to improve:

Hi Arnd,

thx for the review.

quoted

+static void airoha_qdma_set_irqmask(struct airoha_eth *eth, int index,
+				    u32 clear, u32 set)
+{
+	unsigned long flags;
+
+	if (WARN_ON_ONCE(index >= ARRAY_SIZE(eth->irqmask)))
+		return;
+
+	spin_lock_irqsave(&eth->irq_lock, flags);
+
+	eth->irqmask[index] &= ~clear;
+	eth->irqmask[index] |= set;
+	airoha_qdma_wr(eth, REG_INT_ENABLE(index), eth->irqmask[index]);
+
+	spin_unlock_irqrestore(&eth->irq_lock, flags);
+}

spin_lock_irqsave() is fairly expensive here, and it doesn't
actually protect the register write since that is posted
and can leak out of the spinlock.

You can probably just remove the lock and instead do the mask
with atomic_cmpxchg() here.

I did not get what you mean here. I guess the spin_lock is used to avoid
concurrent irq registers updates from user/bh context or irq handler.
Am I missing something?

quoted

+
+		dma_sync_single_for_device(dev, e->dma_addr, e->dma_len, dir);
+
+		val = FIELD_PREP(QDMA_DESC_LEN_MASK, e->dma_len);
+		WRITE_ONCE(desc->ctrl, cpu_to_le32(val));
+		WRITE_ONCE(desc->addr, cpu_to_le32(e->dma_addr));
+		val = FIELD_PREP(QDMA_DESC_NEXT_ID_MASK, q->head);
+		WRITE_ONCE(desc->data, cpu_to_le32(val));
+		WRITE_ONCE(desc->msg0, 0);
+		WRITE_ONCE(desc->msg1, 0);
+		WRITE_ONCE(desc->msg2, 0);
+		WRITE_ONCE(desc->msg3, 0);
+
+		wmb();
+		airoha_qdma_rmw(eth, REG_RX_CPU_IDX(qid), RX_RING_CPU_IDX_MASK,
+				FIELD_PREP(RX_RING_CPU_IDX_MASK, q->head));

The wmb() between the descriptor write and the MMIO does nothing
and can probably just be removed here, a writel() already contains
all the barriers you need to make DMA memory visible before the
MMIO write.

If there is a chance that the device might read the descriptor
while it is being updated by you have not written to the
register, there should be a barrier before the last store to
the descriptor that sets a 'valid' bit. That one can be a
cheaper dma_wmb() then.

ack, I will fix it in v4.

quoted

+static irqreturn_t airoha_irq_handler(int irq, void *dev_instance)
+{
+	struct airoha_eth *eth = dev_instance;
+	u32 intr[ARRAY_SIZE(eth->irqmask)];
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(eth->irqmask); i++) {
+		intr[i] = airoha_qdma_rr(eth, REG_INT_STATUS(i));
+		intr[i] &= eth->irqmask[i];
+		airoha_qdma_wr(eth, REG_INT_STATUS(i), intr[i]);
+	}

This looks like you send an interrupt Ack to each
interrupt in order to re-arm it, but then you disable
it again. Would it be possible to leave the interrupt enabled
but defer the Ack until the napi poll function is completed?

I guess doing so we are not using NAPIs as expected since they are
supposed to run with interrupt disabled. Agree?

quoted

+	if (!test_bit(DEV_STATE_INITIALIZED, &eth->state))
+		return IRQ_NONE;
+
+	if (intr[1] & RX_DONE_INT_MASK) {
+		airoha_qdma_irq_disable(eth, QDMA_INT_REG_IDX1,
+					RX_DONE_INT_MASK);
+		airoha_qdma_for_each_q_rx(eth, i) {
+			if (intr[1] & BIT(i))
+				napi_schedule(&eth->q_rx[i].napi);
+		}
+	}

Something seems wrong here, but that's probably just me
misunderstanding the design: if all queues are signaled
through the same interrupt handler, and you then do
napi_schedule() for each queue, I would expect them to
just all get run on the same CPU.

If you have separate queues, doesn't that mean you also need
separate irq numbers here so they can be distributed to the
available CPUs?

Actually I missed to mark the NAPI as threaded. Doing so, even if we have a
single irq line shared by all Rx queues, the scheduler can run the NAPIs in
parallel on different CPUs. I will fix it in v4.

quoted

+		val = FIELD_PREP(QDMA_DESC_LEN_MASK, len);
+		if (i < nr_frags - 1)
+			val |= FIELD_PREP(QDMA_DESC_MORE_MASK, 1);
+		WRITE_ONCE(desc->ctrl, cpu_to_le32(val));
+		WRITE_ONCE(desc->addr, cpu_to_le32(addr));
+		val = FIELD_PREP(QDMA_DESC_NEXT_ID_MASK, index);
+		WRITE_ONCE(desc->data, cpu_to_le32(val));
+		WRITE_ONCE(desc->msg0, cpu_to_le32(msg0));
+		WRITE_ONCE(desc->msg1, cpu_to_le32(msg1));
+		WRITE_ONCE(desc->msg2, cpu_to_le32(0xffff));
+
+		e->skb = i ? NULL : skb;
+		e->dma_addr = addr;
+		e->dma_len = len;
+
+		wmb();
+		airoha_qdma_rmw(eth, REG_TX_CPU_IDX(qid), TX_RING_CPU_IDX_MASK,
+				FIELD_PREP(TX_RING_CPU_IDX_MASK, index));

Same as above with the wmb().

ack, I will fix it in v4.

quoted

+static int airoha_rx_queues_show(struct seq_file *s, void *data)
+{
+	struct airoha_eth *eth = s->private;
+	int i;
+

...

quoted

+static int airoha_xmit_queues_show(struct seq_file *s, void *data)
+{
+	struct airoha_eth *eth = s->private;
+	int i;
+

Isn't this information available through ethtool already?

I guess we can get rid of this debugfs since it was just for debugging.

quoted

b/drivers/net/ethernet/mediatek/airoha_eth.h
new file mode 100644
index 000000000000..fcd684e1418a

--- /dev/null
+++ b/drivers/net/ethernet/mediatek/airoha_eth.h

@@ -0,0 +1,793 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Lorenzo Bianconi <lorenzo@kernel.org>
+ */
+
+#define AIROHA_MAX_NUM_RSTS		3
+#define AIROHA_MAX_NUM_XSI_RSTS		4

If your driver only has a single .c file, I would suggest moving all the
contents of the .h file into that as well for better readability.

I do not have a strong opinion about it but since we will extend the driver
in the future, keeping .c and .h in different files, seems a bit more tidy.
What do you think?

Regards,
Lorenzo

      Arnd