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 X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3891FC54E8D for ; Tue, 12 May 2020 06:35:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1165520714 for ; Tue, 12 May 2020 06:35:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=bgdev-pl.20150623.gappssmtp.com header.i=@bgdev-pl.20150623.gappssmtp.com header.b="NvN3bHID" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728760AbgELGfe (ORCPT ); Tue, 12 May 2020 02:35:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48536 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1726289AbgELGfd (ORCPT ); Tue, 12 May 2020 02:35:33 -0400 Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9590EC061A0F for ; Mon, 11 May 2020 23:35:32 -0700 (PDT) Received: by mail-io1-xd42.google.com with SMTP id s10so12648204iog.7 for ; Mon, 11 May 2020 23:35:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=hmqc9TQ9Quh7+US/Jmh4IXg0bVpfrPAKCJNshFYSv+Q=; b=NvN3bHIDGwK+p0qEwYf7zjtLlsxm2ceG5S0GYtDcV8aLAqcsJfqOTGl1gkyIlhzl56 Z9+A/IMEvGy4ZRM0/h9CnkQShR6M/HA4DFloyhf88t+b64r/X4SaOrgaUi+rdf2GGmha cvaSBe+ut2HCvfoRg2GMxsWf6PqOgRKYk0l6PWeo1znnU/k7z8yLPqV2DkosG+22KThS +pXuvun5l0Uwje9BpaVzjG2kJpbk1JW2IVBJFyar1BKa7F61PvPqDdP1c4wfHS485XIc MlKgocOGFtkGz3BKYGLQ6g5TKWxZpOJYjdJlC9flK8333jlQOL1oOx4rRvibwF2SEbz1 RWdw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=hmqc9TQ9Quh7+US/Jmh4IXg0bVpfrPAKCJNshFYSv+Q=; b=rKtMjta2adjVjbDuh9x4DLuZts91+WT9owCgzPLEWhoD622L9gcQCwqVGe+izZBZpj VjckYZ8jkjeA+XvS/guZW3wMaxlAe2aTyGhGefr1fdll+ZRlOcc9T3qNRNUk3/Uih7xQ thxHXPxEl5RUfdyvP5n3GKD5OrOWAezlaD28EOp/16bNQIhhRcSyYG4al7uLuouV9cwQ WCp4s/bBuF3E/8M2Sx3ZUp+WP12lputh6kdfEPYUjrP7annQX0dJPRNv69esWpsjWgp8 rshmo5CFteMKS6LuwewOSz9eVesfrVgcrI/PEjkFY+NmVVUnaO/ewWEV1Cd1bxEby0mf jgoA== X-Gm-Message-State: AGi0PubztMoRE2269OMEbmL/W9NpUUVXTGw8MRVub0Y8W0Ybh2LFz2vb +HxN667C65fQyPiyXRJugtPSP8T2PP+8YEXBDuogZw== X-Google-Smtp-Source: APiQypLWnzw4EWWNr1vmFUqwTdjDAprfuZXfBBKAg6Df7mrHkghpjwc+KH+GGaVrIhljRYARBkWpEGcIU/MDkPlDt6c= X-Received: by 2002:a05:6638:68f:: with SMTP id i15mr19299578jab.136.1589265331761; Mon, 11 May 2020 23:35:31 -0700 (PDT) MIME-Version: 1.0 References: <20200511150759.18766-1-brgl@bgdev.pl> <20200511150759.18766-10-brgl@bgdev.pl> In-Reply-To: From: Bartosz Golaszewski Date: Tue, 12 May 2020 08:35:21 +0200 Message-ID: Subject: Re: [PATCH v2 09/14] net: ethernet: mtk-eth-mac: new driver To: Florian Fainelli Cc: Rob Herring , "David S . Miller" , Matthias Brugger , John Crispin , Sean Wang , Mark Lee , Jakub Kicinski , Arnd Bergmann , Fabien Parent , Heiner Kallweit , Edwin Peer , devicetree , Linux Kernel Mailing List , netdev , Linux ARM , linux-mediatek@lists.infradead.org, Stephane Le Provost , Pedro Tsai , Andrew Perepech , Bartosz Golaszewski Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org pon., 11 maj 2020 o 21:24 Florian Fainelli napisa=C5= =82(a): > > > > On 5/11/2020 8:07 AM, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski > > > > This adds the driver for the MediaTek Ethernet MAC used on the MT8* SoC > > family. For now we only support full-duplex. > > > > Signed-off-by: Bartosz Golaszewski > > --- > > [snip] > > > +static int mtk_mac_ring_pop_tail(struct mtk_mac_ring *ring, > > + struct mtk_mac_ring_desc_data *desc_data= ) > > +{ > > + struct mtk_mac_ring_desc *desc =3D &ring->descs[ring->tail]; > > + unsigned int status; > > + > > + /* Let the device release the descriptor. */ > > + dma_rmb(); > > + status =3D desc->status; > > + > > + if (!(status & MTK_MAC_DESC_BIT_COWN)) > > + return -1; > > + > > + desc_data->len =3D status & MTK_MAC_DESC_MSK_LEN; > > + desc_data->flags =3D status & ~MTK_MAC_DESC_MSK_LEN; > > + desc_data->dma_addr =3D desc->data_ptr; > > + desc_data->skb =3D ring->skbs[ring->tail]; > > + > > + desc->data_ptr =3D 0; > > + desc->status =3D MTK_MAC_DESC_BIT_COWN; > > + if (status & MTK_MAC_DESC_BIT_EOR) > > + desc->status |=3D MTK_MAC_DESC_BIT_EOR; > > Don't you need a dma_wmb() for the device to observe the new descriptor > here? > HW has released the descriptor (set the COWN bit) and I just clear all other bits here really. Yeah, I guess it won't hurt to make sure. > [snip] > > > +static void mtk_mac_dma_unmap_tx(struct mtk_mac_priv *priv, > > + struct mtk_mac_ring_desc_data *desc_data= ) > > +{ > > + struct device *dev =3D mtk_mac_get_dev(priv); > > + > > + return dma_unmap_single(dev, desc_data->dma_addr, > > + desc_data->len, DMA_TO_DEVICE); > > If you stored a pointer to the sk_buff you transmitted, then you would > need an expensive read to the descriptor to determine the address and > length, and you would also not be at the mercy of the HW putting > incorrect values there. > You mean store the mapped addresses? Yeah I can do that but I'll still need to read the descriptor memory to verify it was released by HW. > sp > > +static void mtk_mac_dma_init(struct mtk_mac_priv *priv) > > +{ > > + struct mtk_mac_ring_desc *desc; > > + unsigned int val; > > + int i; > > + > > + priv->descs_base =3D (struct mtk_mac_ring_desc *)priv->ring_base; > > + > > + for (i =3D 0; i < MTK_MAC_NUM_DESCS_TOTAL; i++) { > > + desc =3D &priv->descs_base[i]; > > + > > + memset(desc, 0, sizeof(*desc)); > > + desc->status =3D MTK_MAC_DESC_BIT_COWN; > > + if ((i =3D=3D MTK_MAC_NUM_TX_DESCS - 1) || > > + (i =3D=3D MTK_MAC_NUM_DESCS_TOTAL - 1)) > > + desc->status |=3D MTK_MAC_DESC_BIT_EOR; > > + } > > + > > + mtk_mac_ring_init(&priv->tx_ring, priv->descs_base, 0); > > + mtk_mac_ring_init(&priv->rx_ring, > > + priv->descs_base + MTK_MAC_NUM_TX_DESCS, > > + MTK_MAC_NUM_RX_DESCS); > > + > > + /* Set DMA pointers. */ > > + val =3D (unsigned int)priv->dma_addr; > > You would probably add a WARN_ON() or something that catches the upper > 32-bits of the dma_addr being set, see my comment about the DMA mask > setting. > Can it still happen if I check the return value of dma_set_mask_and_coheren= t()? > [snip] > > > +static void mtk_mac_tx_complete_all(struct mtk_mac_priv *priv) > > +{ > > + struct net_device *ndev =3D priv_to_netdev(priv); > > + struct mtk_mac_ring *ring =3D &priv->tx_ring; > > + int ret; > > + > > + for (;;) { > > + mtk_mac_lock(priv); > > + > > + if (!mtk_mac_ring_descs_available(ring)) { > > + mtk_mac_unlock(priv); > > + break; > > + } > > + > > + ret =3D mtk_mac_tx_complete_one(priv); > > + if (ret) { > > + mtk_mac_unlock(priv); > > + break; > > + } > > + > > + if (netif_queue_stopped(ndev)) > > + netif_wake_queue(ndev); > > + > > + mtk_mac_unlock(priv); > > + } > > Where do you increment the net_device statistics to indicate the bytes > and packets transmitted? > I don't. I use the counters provided by HW for that. > [snip] > > > + mtk_mac_set_mode_rmii(priv); > > + > > + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > > Your code assumes that DMA addresses are not going to be >=3D 4GB so you > should be checking this function's return code and abort here otherwise > your driver will fail in surprisingly difficult ways to debug. Sure, thanks. Bart