From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932574AbcFNVS4 (ORCPT ); Tue, 14 Jun 2016 17:18:56 -0400 Received: from mout.kundenserver.de ([212.227.17.10]:62704 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752709AbcFNVSy (ORCPT ); Tue, 14 Jun 2016 17:18:54 -0400 From: Arnd Bergmann To: Li Dongpo 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: Tue, 14 Jun 2016 23:20:10 +0200 Message-ID: <247637090.PhpfAhp4VJ@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <576003F8.3090000@hisilicon.com> References: <1465798076-176393-1-git-send-email-lidongpo@hisilicon.com> <4004629.ilZU8AvAjF@wuerfel> <576003F8.3090000@hisilicon.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:ETyDVu6n1e+w8jM3SccGs0a6eG0WLLBCb8NqiNKSxrrY02Gi9IR Mdc1omc9F8iE6wPyxLU+t8jfnp5JMjtA9aIA6dschMVtXOFjI2brEjFm8e3ZYv4+SyHNKZS OFYIY/hgTXMe8i8GQczXtaXTxv0j0ukJPQ6dN66qgqLNz+jnk8nfrPuZKXhlN3zwqi4pNF4 M87PlNh/1ObmnJUAuIZhg== X-UI-Out-Filterresults: notjunk:1;V01:K0:zB4Ovxt1BcM=:xm3OSIasjiax5zGMYuaMJX aBB+ukBTzlYkKUMuQ2vKXFEMqwo+8eOZRFpVodTqumU8CqJCj00BasumLrZBbQ8aXvEerSzOf tmxDCzkK94NLzapSJAbNuMXDgdbgVTjVSklHixf2SEw4ELY/atXC8Ww/dvIlVmEuFiT6c2nw6 6VviHEmGwc2GhlIvK3k1ny+jQPc5zaP3EtCT4Wl8b94O3+RCFloEGc18qTMSctbmg6OHtEM1B 6XfAfk5pbiRC0zM7gNUzWSZRugBDRqkPJXC7LSI1Xx62k4ZfH8Rje0X+PzpiCzfbEtPMVHQNu gDHSBzXrNeaoT4wHvAxGrKvIB3LseqNuE5/fD7E9pdGPZBq8R80zUw1hzmEYB72P5Yu+yUFGN ReIkjJCZUbhZqxuKQ2ul9a6zCXLYwaV9iyqZaiyAR58G7lQ+WWl5czGEQAgbMJb/CR+269Ao9 4CDlxoKipJAFmrf2mgxwDkvFtq9Wq2RoOaI+5YVf0S6bPVj2K6qprBt42cLNr3dqlQV4CVJ59 e0OjBZzj7S3MQdGV7eUP9oNAa855z3Mucv9517vI7dyUYr2H/mN8DaypwArY91KWdopH4pTyH 0jMx68kvUm2bMepPiS5wRsHFAvoZl70GB55xfIxoZm1JNKAU24KBhBIrBTeZ9g5/Th6FEXQka TUvlB2Itlisus4j5djATRO+28FQCTUU1KSpH/SoU8A6TbpvQg8cSHEBnDhLzE23/tTH/Mqg1o LgUfdgMRYFOMZyI3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday, June 14, 2016 9:17:44 PM CEST Li Dongpo wrote: > On 2016/6/13 17:06, Arnd Bergmann wrote: > > On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote: > > 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? > > > In tx direction, there are only two kinds of interrupts, 'tx fifo empty' > and 'tx one packet finish'. I didn't use 'tx one packet finish' because > it would lead to high hardware interrupts rate. This has been verified in > our chips. It's ok to just use tx fifo empty interrupt. I'm not convinced by the explanation, I don't think that has anything to do with the hardware design, but instead is about the correctness of the BQL logic with your driver. Maybe your xmit function can do something like if (dql_avail(netdev_get_tx_queue(dev, 0)->dql) < 0) enable per-packet interrupt else use only fifo-empty interrupt That way, you don't get a lot of interrupts when the system is in a state of packets being received and sent continuously, but if you get to the point where your tx queue fills up and no rx interrupts arrive, you don't have to wait for it to become completely empty before adding new packets, and BQL won't keep growing the queue. > >> + 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. > > > Some drivers only call 'of_phy_connect' when ndo_open called, > some call when driver probed. But 'phy_mode' and 'phy_node' are > usually initialized when driver probed. > So I think it's not suitable to combine 'of_phy_connect' with > 'of_get_phy_mode' and 'of_parse_phandle'. > Do you have any more suggestions ? My idea was to add another interface that drivers could optionally call if they use the logic that you have here, but other drivers could keep using the plain of_phy_connect. Anyway, this was just an idea, it's not important. Arnd