From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DB4A2C30658 for ; Fri, 28 Jun 2024 17:28:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Z7vUahLkuzAYQKyolos92Xs1J6SsXYQ3Mq59Y7lewvY=; b=K98Gj49cNFOGiuFBX+sBzHZDHG iljHGB7pSTH1hFR7X9+7Lj5KDmZ9s8YxKUC6rQt7UBg0oj2iIlEEun0auGCk4fgHJ7b2hnf8dV9QI gH0KbMB37HiFewXJjYGCGsjIBFBzBx0xxofTQwxUZhrRJ3dXIg+jafKV1nVB0O/TSH5culqk6fc2Z SZ1NWYCe6kf48MZpUxXPMzZgD/KW0ErvS9imEGzVBmzPtwJGPam7GlxlEyT066Nj081G8CiRxCZ3e 281f1SDpcEgyxBH7ZuQfChz10NhsJjAar/RZsnOocu+mxg/O7X0ErDqov7AMY5OnFogaC9bQuiZ15 AnPLyP5Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sNFOC-0000000EVPO-0jMN; Fri, 28 Jun 2024 17:28:00 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sNFO2-0000000EVNn-3zpF for linux-arm-kernel@lists.infradead.org; Fri, 28 Jun 2024 17:27:52 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 1CA09621AE; Fri, 28 Jun 2024 17:27:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 47FBEC116B1; Fri, 28 Jun 2024 17:27:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1719595669; bh=lBJ+V/WP7/c1CrV2xu7wL2q0aSyrJyvTu2qPUtu18Wo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mdbkcKzrCJJMsPQHLelcCjB/Xx9Cz8zcrgMcf/2Pk6X0MpzNdevHfJhlrlHX/x9Vx +rGrowUCcCYUx4wk/qiRnFGtwotlnMl8ZLST0NfaX8hJHsRjqfcy/XxSCbbSTPHI5o iLyrQHtjTg+wFLOYbF5g/2Agx5j5gnim/EzG6FEiMCOazC/N7cHBEVWOhH9QazsLfH BbyzRk2+n5rllmC7a5+ySfiaeAhkoCcl/CzENBWvg/LURrIURgjbdFrPAJXncR7gTt DDi9ruNwWmaJQIQZQ9dKdAKLeBRYozU4I/dH9N7DYjGnObLzTSE70v0viBVaXt1IUm YZY2JAI2d9/4g== Date: Fri, 28 Jun 2024 19:27:45 +0200 From: Lorenzo Bianconi To: Arnd Bergmann Cc: Netdev , Felix Fietkau , lorenzo.bianconi83@gmail.com, "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Conor Dooley , linux-arm-kernel@lists.infradead.org, Rob Herring , krzysztof.kozlowski+dt@linaro.org, Conor Dooley , devicetree@vger.kernel.org, Catalin Marinas , Will Deacon , upstream@airoha.com, AngeloGioacchino Del Regno , benjamin.larsson@genexis.eu, linux-clk@vger.kernel.org, Ratheesh Kannoth , Sunil Goutham , Andrew Lunn Subject: Re: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC Message-ID: References: <2d74f9c1-2b46-4544-a9c2-aa470ce36f80@app.fastmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="bMeKq3pp0ZgKPQdb" Content-Disposition: inline In-Reply-To: <2d74f9c1-2b46-4544-a9c2-aa470ce36f80@app.fastmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240628_102751_151727_9C04E953 X-CRM114-Status: GOOD ( 51.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --bMeKq3pp0ZgKPQdb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable > On Tue, Jun 18, 2024, at 09:49, 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 > > Signed-off-by: Lorenzo Bianconi >=20 > I noticed a few small things that you may want to improve: Hi Arnd, thx for the review. >=20 > > +static void airoha_qdma_set_irqmask(struct airoha_eth *eth, int index, > > + u32 clear, u32 set) > > +{ > > + unsigned long flags; > > + > > + if (WARN_ON_ONCE(index >=3D ARRAY_SIZE(eth->irqmask))) > > + return; > > + > > + spin_lock_irqsave(ð->irq_lock, flags); > > + > > + eth->irqmask[index] &=3D ~clear; > > + eth->irqmask[index] |=3D set; > > + airoha_qdma_wr(eth, REG_INT_ENABLE(index), eth->irqmask[index]); > > + > > + spin_unlock_irqrestore(ð->irq_lock, flags); > > +} >=20 > 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. >=20 > 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? >=20 > > + > > + dma_sync_single_for_device(dev, e->dma_addr, e->dma_len, dir); > > + > > + val =3D 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 =3D 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)); >=20 > 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. >=20 > 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. >=20 > > +static irqreturn_t airoha_irq_handler(int irq, void *dev_instance) > > +{ > > + struct airoha_eth *eth =3D dev_instance; > > + u32 intr[ARRAY_SIZE(eth->irqmask)]; > > + int i; > > + > > + for (i =3D 0; i < ARRAY_SIZE(eth->irqmask); i++) { > > + intr[i] =3D airoha_qdma_rr(eth, REG_INT_STATUS(i)); > > + intr[i] &=3D eth->irqmask[i]; > > + airoha_qdma_wr(eth, REG_INT_STATUS(i), intr[i]); > > + } >=20 > 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? >=20 > > + if (!test_bit(DEV_STATE_INITIALIZED, ð->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(ð->q_rx[i].napi); > > + } > > + } >=20 > 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. >=20 > 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. >=20 > > + val =3D FIELD_PREP(QDMA_DESC_LEN_MASK, len); > > + if (i < nr_frags - 1) > > + val |=3D FIELD_PREP(QDMA_DESC_MORE_MASK, 1); > > + WRITE_ONCE(desc->ctrl, cpu_to_le32(val)); > > + WRITE_ONCE(desc->addr, cpu_to_le32(addr)); > > + val =3D 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 =3D i ? NULL : skb; > > + e->dma_addr =3D addr; > > + e->dma_len =3D len; > > + > > + wmb(); > > + airoha_qdma_rmw(eth, REG_TX_CPU_IDX(qid), TX_RING_CPU_IDX_MASK, > > + FIELD_PREP(TX_RING_CPU_IDX_MASK, index)); >=20 > Same as above with the wmb(). ack, I will fix it in v4. >=20 > > +static int airoha_rx_queues_show(struct seq_file *s, void *data) > > +{ > > + struct airoha_eth *eth =3D s->private; > > + int i; > > + > ... > > +static int airoha_xmit_queues_show(struct seq_file *s, void *data) > > +{ > > + struct airoha_eth *eth =3D s->private; > > + int i; > > + >=20 > Isn't this information available through ethtool already? I guess we can get rid of this debugfs since it was just for debugging. >=20 > > 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 > > + */ > > + > > +#define AIROHA_MAX_NUM_RSTS 3 > > +#define AIROHA_MAX_NUM_XSI_RSTS 4 >=20 > 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 >=20 > Arnd=20 --bMeKq3pp0ZgKPQdb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCZn7ykQAKCRA6cBh0uS2t rM9CAQCiagTiOXfG6lOuuAmsCt3qGVZIIDLSfcOb+VChPh5bAgD/ROl7fEb/iugs RcHNy+AlLKCbxagsGTgZtDTKz4eC6AE= =nayO -----END PGP SIGNATURE----- --bMeKq3pp0ZgKPQdb--