From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422637AbcFMKdd (ORCPT ); Mon, 13 Jun 2016 06:33:33 -0400 Received: from mout.kundenserver.de ([217.72.192.75]:62674 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964910AbcFMKd2 (ORCPT ); Mon, 13 Jun 2016 06:33:28 -0400 From: Arnd Bergmann To: Dongpo Li Cc: f.fainelli@gmail.com, robh+dt@kernel.org, mark.rutland@arm.com, davem@davemloft.net, xuejiancheng@hisilicon.com, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Date: Mon, 13 Jun 2016 11:06:05 +0200 Message-ID: <4004629.ilZU8AvAjF@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <1465798076-176393-4-git-send-email-lidongpo@hisilicon.com> References: <1465798076-176393-1-git-send-email-lidongpo@hisilicon.com> <1465798076-176393-4-git-send-email-lidongpo@hisilicon.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:W6stcNMgwbBDdKs/y2mOHNLVk/QZ6OVK20K2hkLmTjKfmuZcR2r MH9hoUO7WLzmgTJiEy0tveloxRW3PSGXgNgOVEFm0NBOuAbh0jEnoZQVwzhRPAmgGDe/cN9 r81i0SmoouNjyZQwIZmDtogK9kz7ry+C01zIT/+MPCWDOTD8KcZPZmFsuIuMITp9RYl8c3I 3s5U9UtrPQa5sVzX1DlaA== X-UI-Out-Filterresults: notjunk:1;V01:K0:P39+gFikRIM=:7RmPlFOiUjcfI2mEwx851f ifYPTSccRifhTzjOgHWYYEsjy1QQ98joZ5CTxip3P7Hws7RgAefzm6FcAVnLZ22ZAw/9C459y Z2k+Yxy9QeRfjvBmG+3FRMUGi/lRXohHOtOybiq3Dui+tvLbt5D8wzeEo7dVe+LvumX8Gi/9d TqDZbFrt6zF1oAjoi1RH+bL3tP4sNBO+NzGnjwyLCvqMYAPxrzwomUrcXAMxcyzRx/AxHNZmx a+Lj0GEBwrlX0GgkfKnV5OG3QqR5Wc5xXSJiyfpVxb9jqa1lOVjZ1EORHKBWKmO6WXaO6jB6p JnMpSLxtOfwl34ThNPvtrvBLG8WshbfRD1QiTy4rYV1gDuLbux1NNvBB+lKhNaHa6Zq2+CElg L8ea6oHwVwoLT+LiOb/DrP8bjZmhQTWTw1CmtfISRPLLHZvF6V3bDxJ197gWgW0uANHSoSbgx wgbBKu+QKMiDldiVrvZYgvMmg1ube/92kUJMns92m20MVNmrs89eK3Ei60UdTCpyXowQm+HIW XpvtZlRbP5NDzOlD7XZrKS6dtLqjLgny819ZaIy9U0xJAMaxIoODayjxVZPIPC696H4wMwxSw W1nZ7tIA/tSBjgOdNA9/1jYvc/f71YvwfIKwmuXBt/PClukGfwAi9qWxMjhK82UP88G7nNnKy hGz+p2VnO6Eipnc8KL2NFlTVEugyYQip6LYKee0B5B46FO4exIMuvz/A4bwuUFdRBKH0UWeF5 l/FLLUa8Q5ykpcDK Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote: > +- reset-names: should contain the reset signal name "mac_reset"(required) > + and "phy_reset"(optional). Maybe just name the resets 'mac' and 'phy'? The '_reset' part is implied by the property. I gave the driver a brief review and basically everything looks great, very nice work! There are two small things that I noticed: > + > + do { > + hisi_femac_xmit_reclaim(dev); > + num = hisi_femac_rx(dev, task); > + work_done += num; > + task -= num; > + if ((work_done >= budget) || (num == 0)) > + break; > + > + ints = readl(priv->glb_base + GLB_IRQ_STAT); > + writel(ints & DEF_INT_MASK, > + priv->glb_base + GLB_IRQ_RAW); > + } while (ints & DEF_INT_MASK); > + > + if (work_done < budget) { > + napi_complete(napi); > + hisi_femac_irq_enable(priv, DEF_INT_MASK); > + } You tx function uses BQL to optimize the queue length, and that is great. You also check xmit reclaim for rx interrupts, so as long as you have both rx and tx traffic, this should work great. However, I notice that you only have a 'tx fifo empty' interrupt triggering the napi poll, so I guess on a tx-only workload you will always end up pushing packets into the queue until BQL throttles tx, and then get the interrupt after all packets have been sent, which will cause BQL to make the queue longer up to the maximum queue size, and that negates the effect of BQL. Is there any way you can get a tx interrupt earlier than this in order to get a more balanced queue, or is it ok to just rely on rx packets to come in occasionally, and just use the tx fifo empty interrupt as a fallback? > + priv->phy_mode = of_get_phy_mode(node); > + if (priv->phy_mode < 0) { > + dev_err(dev, "not find phy-mode\n"); > + ret = -EINVAL; > + goto out_disable_clk; > + } > + > + priv->phy_node = of_parse_phandle(node, "phy-handle", 0); > + if (!priv->phy_node) { > + dev_err(dev, "not find phy-handle\n"); > + ret = -EINVAL; > + goto out_disable_clk; > + } > + > + priv->phy = of_phy_connect(ndev, priv->phy_node, > + hisi_femac_adjust_link, 0, priv->phy_mode); > + if (!(priv->phy) || IS_ERR(priv->phy)) { > + dev_err(dev, "connect to PHY failed!\n"); > + ret = -ENODEV; > + goto out_phy_node; > + } I wonder if we could generalize this set of three calls, I get the impression that we duplicate this across several drivers that shouldn't need to bother with the specific phy-handle and phy-mode properties. Arnd