[{"id":1765378,"web_url":"http://patchwork.ozlabs.org/comment/1765378/","msgid":"<20170908135030.GA25219@lunn.ch>","list_archive_url":null,"date":"2017-09-08T13:50:30","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"> +static int ave_mdio_busywait(struct net_device *ndev)\n> +{\n> +\tint ret = 1, loop = 100;\n> +\tu32 mdiosr;\n> +\n> +\t/* wait until completion */\n> +\twhile (1) {\n> +\t\tmdiosr = ave_r32(ndev, AVE_MDIOSR);\n> +\t\tif (!(mdiosr & AVE_MDIOSR_STS))\n> +\t\t\tbreak;\n> +\n> +\t\tusleep_range(10, 20);\n> +\t\tif (!loop--) {\n> +\t\t\tnetdev_err(ndev,\n> +\t\t\t\t   \"failed to read from MDIO (status:0x%08x)\\n\",\n> +\t\t\t\t   mdiosr);\n> +\t\t\tret = 0;\n\nETIMEDOUT would be better.\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn ret;\n\nand then return 0 on success. That is the normal convention for return\nvalues. An error code, and 0.\n\n> +static int ave_mdiobus_write(struct mii_bus *bus,\n> +\t\t\t     int phyid, int regnum, u16 val)\n> +{\n> +\tstruct net_device *ndev = bus->priv;\n> +\tu32 mdioctl;\n> +\n> +\t/* write address */\n> +\tave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);\n> +\n> +\t/* write data */\n> +\tave_w32(ndev, AVE_MDIOWDR, val);\n> +\n> +\t/* write request */\n> +\tmdioctl = ave_r32(ndev, AVE_MDIOCTR);\n> +\tave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);\n> +\n> +\tif (!ave_mdio_busywait(ndev)) {\n> +\t\tnetdev_err(ndev, \"phy-%d reg-%x write failed\\n\",\n> +\t\t\t   phyid, regnum);\n> +\t\treturn -1;\n\nIf ave_mdio_busywait() returns ETIMEDOUT, you can just return\nit. Returning -1 is not good.\n\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static irqreturn_t ave_interrupt(int irq, void *netdev)\n> +{\n> +\tstruct net_device *ndev = (struct net_device *)netdev;\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 gimr_val, gisr_val;\n> +\n> +\tgimr_val = ave_irq_disable_all(ndev);\n> +\n> +\t/* get interrupt status */\n> +\tgisr_val = ave_r32(ndev, AVE_GISR);\n> +\n> +\t/* PHY */\n> +\tif (gisr_val & AVE_GI_PHY) {\n> +\t\tave_w32(ndev, AVE_GISR, AVE_GI_PHY);\n> +\t\tif (priv->internal_phy_interrupt)\n> +\t\t\tphy_mac_interrupt(ndev->phydev, ndev->phydev->link);\n\nHumm. I don't think this is correct. You are supposed to give it the\nnew link state, not the old.\n\nWhat does a PHY interrupt mean here? \n\n> +static void ave_adjust_link(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tstruct phy_device *phydev = ndev->phydev;\n> +\tu32 val, txcr, rxcr, rxcr_org;\n> +\n> +\t/* set RGMII speed */\n> +\tval = ave_r32(ndev, AVE_TXCR);\n> +\tval &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);\n> +\n> +\tif (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&\n> +\t    phydev->speed == SPEED_1000)\n\nphy_interface_mode_is_rgmii(), so that you handle all the RGMII modes.\n\n> +\t\tval |= AVE_TXCR_TXSPD_1G;\n> +\telse if (phydev->speed == SPEED_100)\n> +\t\tval |= AVE_TXCR_TXSPD_100;\n> +\n> +\tave_w32(ndev, AVE_TXCR, val);\n> +\n> +\t/* set RMII speed (100M/10M only) */\n> +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n\nNot so safe. It would be better to check for the modes you actually\nsupport.\n\n> +\tif (phydev->link)\n> +\t\tnetif_carrier_on(ndev);\n> +\telse\n> +\t\tnetif_carrier_off(ndev);\n\nI don't think you need this. The phylib should do it for you.\n\n> +\n> +\tphy_print_status(phydev);\n> +}\n> +\n> +static int ave_init(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tstruct device *dev = ndev->dev.parent;\n> +\tstruct device_node *phy_node, *np = dev->of_node;\n> +\tstruct phy_device *phydev;\n> +\tconst void *mac_addr;\n> +\tu32 supported;\n> +\n> +\t/* get mac address */\n> +\tmac_addr = of_get_mac_address(np);\n> +\tif (mac_addr)\n> +\t\tether_addr_copy(ndev->dev_addr, mac_addr);\n> +\n> +\t/* if the mac address is invalid, use random mac address */\n> +\tif (!is_valid_ether_addr(ndev->dev_addr)) {\n> +\t\teth_hw_addr_random(ndev);\n> +\t\tdev_warn(dev, \"Using random MAC address: %pM\\n\",\n> +\t\t\t ndev->dev_addr);\n> +\t}\n> +\n> +\t/* attach PHY with MAC */\n> +\tphy_node =  of_get_next_available_child(np, NULL);\n\n???\n\nShould this not be looking for a phy-handle property?\nDocumentation/devicetree/binds/net/ethernet.txt:\n\n- phy-handle: phandle, specifies a reference to a node representing a PHY\n  device; this property is described in the Devicetree Specification and so\n  preferred;\n\n\n> +\tphydev = of_phy_connect(ndev, phy_node,\n> +\t\t\t\tave_adjust_link, 0, priv->phy_mode);\n> +\tif (!phydev) {\n> +\t\tdev_err(dev, \"could not attach to PHY\\n\");\n> +\t\treturn -ENODEV;\n> +\t}\n> +\tof_node_put(phy_node);\n> +\n> +\tpriv->phydev = phydev;\n> +\tphydev->autoneg = AUTONEG_ENABLE;\n> +\tphydev->speed = 0;\n> +\tphydev->duplex = 0;\n\nAnd this should not be needed.\n\n> +\n> +\tdev_info(dev, \"connected to %s phy with id 0x%x\\n\",\n> +\t\t phydev->drv->name, phydev->phy_id);\n\nphy_attached_info()\n\n> +\n> +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n\nSame comment as above.\n\n> +\t\tsupported = phydev->supported;\n> +\t\tphydev->supported &= ~PHY_GBIT_FEATURES;\n> +\t\tphydev->supported |= supported & PHY_BASIC_FEATURES;\n> +\t}\n> +\n> +\t/* PHY interrupt stop instruction is needed because the interrupt\n> +\t * continues to assert.\n> +\t */\n> +\tphy_stop_interrupts(phydev);\n\nCould you explain this some more? It sounds like your interrupt\ncontroller is broken.\n\n> +\n> +\t/* When PHY driver can't handle its interrupt directly,\n> +\t * interrupt request always fails and polling method is used\n> +\t * alternatively. In this case, the libphy says\n> +\t * \"libphy: uniphier-mdio: Can't get IRQ -1 (PHY)\".\n> +\t */\n> +\tphy_start_interrupts(phydev);\n\n-1 is PHY_POLL. So calling phy_start_interrupts() is wrong. In fact,\nyou should not be calling phy_start_interrupts() at all. No other\nEthernet driver does.\n\n> +\n> +\tphy_start_aneg(phydev);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void ave_uninit(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\n> +\tphy_stop_interrupts(priv->phydev);\n\nAnd no other Ethernet driver calls phy_stop_interrupts either.\nPlease take a look at this.\n\n> +\tphy_disconnect(priv->phydev);\n> +}\n> +\n\n  Andrew","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpdy72NBLz9s8J\n\tfor <patchwork-incoming@ozlabs.org>;\n\tFri,  8 Sep 2017 23:50:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753030AbdIHNun (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tFri, 8 Sep 2017 09:50:43 -0400","from vps0.lunn.ch ([178.209.37.122]:32971 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751780AbdIHNuj (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tFri, 8 Sep 2017 09:50:39 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from <andrew@lunn.ch>)\n\tid 1dqJfy-0006ye-OM; Fri, 08 Sep 2017 15:50:30 +0200"],"Date":"Fri, 8 Sep 2017 15:50:30 +0200","From":"Andrew Lunn <andrew@lunn.ch>","To":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","Cc":"netdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tFlorian Fainelli <f.fainelli@gmail.com>,\n\tRob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Message-ID":"<20170908135030.GA25219@lunn.ch>","References":"<1504875731-3680-1-git-send-email-hayashi.kunihiko@socionext.com>\n\t<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765437,"web_url":"http://patchwork.ozlabs.org/comment/1765437/","msgid":"<CAK7LNASqfRbG08gvLxx4WxY2Vs2YYPkZ6NUpjj3_OOvOdEBKMA@mail.gmail.com>","list_archive_url":null,"date":"2017-09-08T14:44:13","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":65882,"url":"http://patchwork.ozlabs.org/api/people/65882/","name":"Masahiro Yamada","email":"yamada.masahiro@socionext.com"},"content":"2017-09-08 22:02 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>:\n\n> diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig\n> new file mode 100644\n> index 0000000..788f26f\n> --- /dev/null\n> +++ b/drivers/net/ethernet/socionext/Kconfig\n> @@ -0,0 +1,22 @@\n> +config NET_VENDOR_SOCIONEXT\n> +       bool \"Socionext ethernet drivers\"\n> +       default y\n> +       ---help---\n> +         Option to select ethernet drivers for Socionext platforms.\n> +\n> +         Note that the answer to this question doesn't directly affect the\n> +         kernel: saying N will just cause the configurator to skip all\n> +         the questions about Agere devices. If you say Y, you will be asked\n> +         for your specific card in the following questions.\n\n\nAgere?\n\n\n\n> +\n> +       dev_info(dev, \"Socionext %c%c%c%c Ethernet IP %s (irq=%d, phy=%s)\\n\",\n> +                (ave_id >> 24) & 0xff, (ave_id >> 16) & 0xff,\n> +                (ave_id >> 8) & 0xff, (ave_id >> 0) & 0xff,\n> +                buf, ndev->irq, phy_modes(phy_mode));\n> +\n> +       return 0;\n> +err_netdev_register:\n\nMaybe, a bad label name.\nfor ex. out_del_napi or whatever.\n\nDocumentation/process/coding-style.rst says\n\"Choose label names which say what the goto does ...\"\n\n\n> +       netif_napi_del(&priv->napi_rx);\n> +       netif_napi_del(&priv->napi_tx);\n> +       mdiobus_unregister(priv->mdio);\n> +err_mdiobus_register:\n> +err_mdiobus_alloc:\n> +err_req_irq:\n\nThese three should be merged, for ex.\nout_free_device.\n\n\n\nI ran sparse for you.\n\nPlease take a look if it is worthwhile.\nSeems endianess around mac_addr.\n\n\ndrivers/net/ethernet/socionext/sni_ave.c:423:15: warning: cast to\nrestricted __le32\ndrivers/net/ethernet/socionext/sni_ave.c:425:20: warning: cast to\nrestricted __le16\ndrivers/net/ethernet/socionext/sni_ave.c:1194:15: warning: cast to\nrestricted __le32\ndrivers/net/ethernet/socionext/sni_ave.c:1196:20: warning: cast to\nrestricted __le16\ndrivers/net/ethernet/socionext/sni_ave.c:1398:15: warning: cast to\nrestricted __le32\ndrivers/net/ethernet/socionext/sni_ave.c:1400:20: warning: cast to\nrestricted __le16","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=nifty.com header.i=@nifty.com\n\theader.b=\"IJEvWXFC\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpgVt6LXdz9s81\n\tfor <patchwork-incoming@ozlabs.org>;\n\tSat,  9 Sep 2017 01:00:54 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753183AbdIHPAw (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tFri, 8 Sep 2017 11:00:52 -0400","from condef-02.nifty.com ([202.248.20.67]:26278 \"EHLO\n\tcondef-02.nifty.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751166AbdIHPAv (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Fri, 8 Sep 2017 11:00:51 -0400","from conssluserg-04.nifty.com ([10.126.8.83])by\n\tcondef-02.nifty.com with ESMTP id v88EixDp017256\n\tfor <netdev@vger.kernel.org>; Fri, 8 Sep 2017 23:44:59 +0900","from mail-yw0-f178.google.com (mail-yw0-f178.google.com\n\t[209.85.161.178]) (authenticated)\n\tby conssluserg-04.nifty.com with ESMTP id v88Eis63020467;\n\tFri, 8 Sep 2017 23:44:55 +0900","by mail-yw0-f178.google.com with SMTP id q80so8076837ywg.2;\n\tFri, 08 Sep 2017 07:44:54 -0700 (PDT)","by 10.37.164.225 with HTTP; Fri, 8 Sep 2017 07:44:13 -0700 (PDT)"],"X-Greylist":"delayed 572 seconds by postgrey-1.27 at vger.kernel.org;\n\tFri, 08 Sep 2017 11:00:51 EDT","DKIM-Filter":"OpenDKIM Filter v2.10.3 conssluserg-04.nifty.com v88Eis63020467","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.com;\n\ts=dec2015msa; t=1504881895;\n\tbh=rhdrKWTJPX/lmudPDuwIotZGBextt3ajHj1zC86+SNg=;\n\th=In-Reply-To:References:From:Date:Subject:To:Cc:From;\n\tb=IJEvWXFCfD4Af7FRvN5T0bx9fUdcqfyyUhGnVrmidiCwmSwjZie0ISDqZpVRz8ztw\n\tvudOQ7RKnsWWbrFm0WR+8M8rdrk76qKhMuZuLH4T34gd2JzyxC9qmX1BPe2VLBzswg\n\tdLaOUlYBBiuHbgGm4RCvSrxOs/G6q94rdOHD5HhXDUvrYmICV7pvukf8tM8KBOi4Lq\n\t48lpPGMrYfHuaE7zgZqz5qFJorGREDMxWdu3ci+9Z9GLKtdIdsFKBgMJpivyxWj4h8\n\t4frZxqkOlZPYG0aCQGWEkMKlfG0cR3QVRSfXBcKsfOHCtQcgc9evdcf9sV5fI4gMZr\n\tCJeWq/oR+rzVQ==","X-Nifty-SrcIP":"[209.85.161.178]","X-Gm-Message-State":"AHPjjUjodnWTvNli2Hm9yqRksOtAieo4fx/7WuwRHAS32k8EORren9OF\n\ttAGOaJptVX3yzVgmnLapn4v1/IpJZA==","X-Google-Smtp-Source":"ADKCNb41OFAN8MVkiSiIhl1UiJ9UWhWTqcU2Kz+ReVD0ynigzap+y9DEyS0WnwyJkGGHbhhA59snTH3/1sUgivxVSJw=","X-Received":"by 10.37.80.213 with SMTP id e204mr2617879ybb.215.1504881893733; \n\tFri, 08 Sep 2017 07:44:53 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","References":"<1504875731-3680-1-git-send-email-hayashi.kunihiko@socionext.com>\n\t<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","From":"Masahiro Yamada <yamada.masahiro@socionext.com>","Date":"Fri, 8 Sep 2017 23:44:13 +0900","X-Gmail-Original-Message-ID":"<CAK7LNASqfRbG08gvLxx4WxY2Vs2YYPkZ6NUpjj3_OOvOdEBKMA@mail.gmail.com>","Message-ID":"<CAK7LNASqfRbG08gvLxx4WxY2Vs2YYPkZ6NUpjj3_OOvOdEBKMA@mail.gmail.com>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","To":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","Cc":"netdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tAndrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>,\n\tRob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel <linux-arm-kernel@lists.infradead.org>,\n\tLinux Kernel Mailing List <linux-kernel@vger.kernel.org>,\n\tdevicetree@vger.kernel.org,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765597,"web_url":"http://patchwork.ozlabs.org/comment/1765597/","msgid":"<df4ec8b2-67cf-5dee-2600-88cffd1bcd11@gmail.com>","list_archive_url":null,"date":"2017-09-08T19:31:18","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":2800,"url":"http://patchwork.ozlabs.org/api/people/2800/","name":"Florian Fainelli","email":"f.fainelli@gmail.com"},"content":"On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:\n> The UniPhier platform from Socionext provides the AVE ethernet\n> controller that includes MAC and MDIO bus supporting RGMII/RMII\n> modes. The controller is named AVE.\n> \n> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>\n> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>\n> ---\n>  drivers/net/ethernet/Kconfig             |    1 +\n>  drivers/net/ethernet/Makefile            |    1 +\n>  drivers/net/ethernet/socionext/Kconfig   |   22 +\n>  drivers/net/ethernet/socionext/Makefile  |    4 +\n>  drivers/net/ethernet/socionext/sni_ave.c | 1618 ++++++++++++++++++++++++++++++\n>  5 files changed, 1646 insertions(+)\n>  create mode 100644 drivers/net/ethernet/socionext/Kconfig\n>  create mode 100644 drivers/net/ethernet/socionext/Makefile\n>  create mode 100644 drivers/net/ethernet/socionext/sni_ave.c\n> \n> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig\n> index c604213..d50519e 100644\n> --- a/drivers/net/ethernet/Kconfig\n> +++ b/drivers/net/ethernet/Kconfig\n> @@ -170,6 +170,7 @@ source \"drivers/net/ethernet/sis/Kconfig\"\n>  source \"drivers/net/ethernet/sfc/Kconfig\"\n>  source \"drivers/net/ethernet/sgi/Kconfig\"\n>  source \"drivers/net/ethernet/smsc/Kconfig\"\n> +source \"drivers/net/ethernet/socionext/Kconfig\"\n>  source \"drivers/net/ethernet/stmicro/Kconfig\"\n>  source \"drivers/net/ethernet/sun/Kconfig\"\n>  source \"drivers/net/ethernet/tehuti/Kconfig\"\n> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile\n> index a0a03d4..9f55b36 100644\n> --- a/drivers/net/ethernet/Makefile\n> +++ b/drivers/net/ethernet/Makefile\n> @@ -81,6 +81,7 @@ obj-$(CONFIG_SFC) += sfc/\n>  obj-$(CONFIG_SFC_FALCON) += sfc/falcon/\n>  obj-$(CONFIG_NET_VENDOR_SGI) += sgi/\n>  obj-$(CONFIG_NET_VENDOR_SMSC) += smsc/\n> +obj-$(CONFIG_NET_VENDOR_SOCIONEXT) += socionext/\n>  obj-$(CONFIG_NET_VENDOR_STMICRO) += stmicro/\n>  obj-$(CONFIG_NET_VENDOR_SUN) += sun/\n>  obj-$(CONFIG_NET_VENDOR_TEHUTI) += tehuti/\n> diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig\n> new file mode 100644\n> index 0000000..788f26f\n> --- /dev/null\n> +++ b/drivers/net/ethernet/socionext/Kconfig\n> @@ -0,0 +1,22 @@\n> +config NET_VENDOR_SOCIONEXT\n> +\tbool \"Socionext ethernet drivers\"\n> +\tdefault y\n> +\t---help---\n> +\t  Option to select ethernet drivers for Socionext platforms.\n> +\n> +\t  Note that the answer to this question doesn't directly affect the\n> +\t  kernel: saying N will just cause the configurator to skip all\n> +\t  the questions about Agere devices. If you say Y, you will be asked\n> +\t  for your specific card in the following questions.\n> +\n> +if NET_VENDOR_SOCIONEXT\n> +\n> +config SNI_AVE\n> +\ttristate \"Socionext AVE ethernet support\"\n> +\tdepends on (ARCH_UNIPHIER || COMPILE_TEST) && OF\n> +\tselect PHYLIB\n> +\t---help---\n> +\t  Driver for gigabit ethernet MACs, called AVE, in the\n> +\t  Socionext UniPhier family.\n> +\n> +endif #NET_VENDOR_SOCIONEXT\n> diff --git a/drivers/net/ethernet/socionext/Makefile b/drivers/net/ethernet/socionext/Makefile\n> new file mode 100644\n> index 0000000..0356341\n> --- /dev/null\n> +++ b/drivers/net/ethernet/socionext/Makefile\n> @@ -0,0 +1,4 @@\n> +#\n> +# Makefile for all ethernet ip drivers on Socionext platforms\n> +#\n> +obj-$(CONFIG_SNI_AVE) += sni_ave.o\n> diff --git a/drivers/net/ethernet/socionext/sni_ave.c b/drivers/net/ethernet/socionext/sni_ave.c\n> new file mode 100644\n> index 0000000..c870777\n> --- /dev/null\n> +++ b/drivers/net/ethernet/socionext/sni_ave.c\n> @@ -0,0 +1,1618 @@\n> +/**\n> + * sni_ave.c - Socionext UniPhier AVE ethernet driver\n> + *\n> + * Copyright 2014 Panasonic Corporation\n> + * Copyright 2015-2017 Socionext Inc.\n> + *\n> + * This program is free software: you can redistribute it and/or modify\n> + * it under the terms of the GNU General Public License version 2  of\n> + * the License as published by the Free Software Foundation.\n> + *\n> + * This program is distributed in the hope that it will be useful,\n> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the\n> + * GNU General Public License for more details.\n> + */\n> +\n> +#include <linux/bitops.h>\n> +#include <linux/clk.h>\n> +#include <linux/etherdevice.h>\n> +#include <linux/interrupt.h>\n> +#include <linux/io.h>\n> +#include <linux/mii.h>\n> +#include <linux/module.h>\n> +#include <linux/netdevice.h>\n> +#include <linux/of_net.h>\n> +#include <linux/of_mdio.h>\n> +#include <linux/of_platform.h>\n> +#include <linux/phy.h>\n> +#include <linux/reset.h>\n> +#include <linux/types.h>\n> +\n> +/* General Register Group */\n> +#define AVE_IDR\t\t0x0\t/* ID */\n> +#define AVE_VR\t\t0x4\t/* Version */\n> +#define AVE_GRR\t\t0x8\t/* Global Reset */\n> +#define AVE_CFGR\t0xc\t/* Configuration */\n> +\n> +/* Interrupt Register Group */\n> +#define AVE_GIMR\t0x100\t/* Global Interrupt Mask */\n> +#define AVE_GISR\t0x104\t/* Global Interrupt Status */\n> +\n> +/* MAC Register Group */\n> +#define AVE_TXCR\t0x200\t/* TX Setup */\n> +#define AVE_RXCR\t0x204\t/* RX Setup */\n> +#define AVE_RXMAC1R\t0x208\t/* MAC address (lower) */\n> +#define AVE_RXMAC2R\t0x20c\t/* MAC address (upper) */\n> +#define AVE_MDIOCTR\t0x214\t/* MDIO Control */\n> +#define AVE_MDIOAR\t0x218\t/* MDIO Address */\n> +#define AVE_MDIOWDR\t0x21c\t/* MDIO Data */\n> +#define AVE_MDIOSR\t0x220\t/* MDIO Status */\n> +#define AVE_MDIORDR\t0x224\t/* MDIO Rd Data */\n> +\n> +/* Descriptor Control Register Group */\n> +#define AVE_DESCC\t0x300\t/* Descriptor Control */\n> +#define AVE_TXDC\t0x304\t/* TX Descriptor Configuration */\n> +#define AVE_RXDC0\t0x308\t/* RX Descriptor Ring0 Configuration */\n> +#define AVE_IIRQC\t0x34c\t/* Interval IRQ Control */\n> +\n> +/* Frame Counter Register Group */\n> +#define AVE_BFCR\t0x400\t/* Bad Frame Counter */\n> +#define AVE_RX0OVFFC\t0x414\t/* OVF Frame Counter (Ring0) */\n> +#define AVE_SN5FC\t0x438\t/* Frame Counter No5 */\n> +#define AVE_SN6FC\t0x43c\t/* Frame Counter No6 */\n> +#define AVE_SN7FC\t0x440\t/* Frame Counter No7 */\n> +\n> +/* Packet Filter Register Group */\n> +#define AVE_PKTF_BASE\t\t0x800\t/* PF Base Address */\n> +#define AVE_PFMBYTE_BASE\t0xd00\t/* PF Mask Byte Base Address */\n> +#define AVE_PFMBIT_BASE\t\t0xe00\t/* PF Mask Bit Base Address */\n> +#define AVE_PFSEL_BASE\t\t0xf00\t/* PF Selector Base Address */\n> +#define AVE_PFEN\t\t0xffc\t/* Packet Filter Enable */\n> +#define AVE_PKTF(ent)\t\t(AVE_PKTF_BASE + (ent) * 0x40)\n> +#define AVE_PFMBYTE(ent)\t(AVE_PFMBYTE_BASE + (ent) * 8)\n> +#define AVE_PFMBIT(ent)\t\t(AVE_PFMBIT_BASE + (ent) * 4)\n> +#define AVE_PFSEL(ent)\t\t(AVE_PFSEL_BASE + (ent) * 4)\n> +\n> +/* 64bit descriptor memory */\n> +#define AVE_DESC_SIZE_64\t12\t/* Descriptor Size */\n> +\n> +#define AVE_TXDM_64\t\t0x1000\t/* Tx Descriptor Memory */\n> +#define AVE_RXDM_64\t\t0x1c00\t/* Rx Descriptor Memory */\n> +\n> +#define AVE_TXDM_SIZE_64\t0x0ba0\t/* Tx Descriptor Memory Size 3KB */\n> +#define AVE_RXDM_SIZE_64\t0x6000\t/* Rx Descriptor Memory Size 24KB */\n> +\n> +/* 32bit descriptor memory */\n> +#define AVE_DESC_SIZE_32\t8\t/* Descriptor Size */\n> +\n> +#define AVE_TXDM_32\t\t0x1000\t/* Tx Descriptor Memory */\n> +#define AVE_RXDM_32\t\t0x1800\t/* Rx Descriptor Memory */\n> +\n> +#define AVE_TXDM_SIZE_32\t0x07c0\t/* Tx Descriptor Memory Size 2KB */\n> +#define AVE_RXDM_SIZE_32\t0x4000\t/* Rx Descriptor Memory Size 16KB */\n> +\n> +/* RMII Bridge Register Group */\n> +#define AVE_RSTCTRL\t\t0x8028\t/* Reset control */\n> +#define AVE_RSTCTRL_RMIIRST\tBIT(16)\n> +#define AVE_LINKSEL\t\t0x8034\t/* Link speed setting */\n> +#define AVE_LINKSEL_100M\tBIT(0)\n> +\n> +/* AVE_GRR */\n> +#define AVE_GRR_RXFFR\t\tBIT(5)\t/* Reset RxFIFO */\n> +#define AVE_GRR_PHYRST\t\tBIT(4)\t/* Reset external PHY */\n> +#define AVE_GRR_GRST\t\tBIT(0)\t/* Reset all MAC */\n> +\n> +/* AVE_GISR (common with GIMR) */\n> +#define AVE_GI_PHY\t\tBIT(24)\t/* PHY interrupt */\n> +#define AVE_GI_TX\t\tBIT(16)\t/* Tx complete */\n> +#define AVE_GI_RXERR\t\tBIT(8)\t/* Receive frame more than max size */\n> +#define AVE_GI_RXOVF\t\tBIT(7)\t/* Overflow at the RxFIFO */\n> +#define AVE_GI_RXDROP\t\tBIT(6)\t/* Drop packet */\n> +#define AVE_GI_RXIINT\t\tBIT(5)\t/* Interval interrupt */\n> +\n> +/* AVE_TXCR */\n> +#define AVE_TXCR_FLOCTR\t\tBIT(18)\t/* Flow control */\n> +#define AVE_TXCR_TXSPD_1G\tBIT(17)\n> +#define AVE_TXCR_TXSPD_100\tBIT(16)\n> +\n> +/* AVE_RXCR */\n> +#define AVE_RXCR_RXEN\t\tBIT(30)\t/* Rx enable */\n> +#define AVE_RXCR_FDUPEN\t\tBIT(22)\t/* Interface mode */\n> +#define AVE_RXCR_FLOCTR\t\tBIT(21)\t/* Flow control */\n> +#define AVE_RXCR_AFEN\t\tBIT(19)\t/* MAC address filter */\n> +#define AVE_RXCR_DRPEN\t\tBIT(18)\t/* Drop pause frame */\n> +#define AVE_RXCR_MPSIZ_MASK\tGENMASK(10, 0)\n> +\n> +/* AVE_MDIOCTR */\n> +#define AVE_MDIOCTR_RREQ\tBIT(3)\t/* Read request */\n> +#define AVE_MDIOCTR_WREQ\tBIT(2)\t/* Write request */\n> +\n> +/* AVE_MDIOSR */\n> +#define AVE_MDIOSR_STS\t\tBIT(0)\t/* access status */\n> +\n> +/* AVE_DESCC */\n> +#define AVE_DESCC_TD\t\tBIT(0)\t/* Enable Tx descriptor */\n> +#define AVE_DESCC_RDSTP\t\tBIT(4)\t/* Pause Rx descriptor */\n> +#define AVE_DESCC_RD0\t\tBIT(8)\t/* Enable Rx descriptor Ring0 */\n> +\n> +#define AVE_DESCC_CTRL_MASK\tGENMASK(15, 0)\n> +\n> +/* AVE_TXDC */\n> +#define AVE_TXDC_SIZE\t\tGENMASK(27, 16)\t/* Size of Tx descriptor */\n> +#define AVE_TXDC_ADDR\t\tGENMASK(11, 0)\t/* Start address */\n> +#define AVE_TXDC_ADDR_START\t0\n> +\n> +/* AVE_RXDC0 */\n> +#define AVE_RXDC0_SIZE\t\tGENMASK(30, 16)\t/* Size of Rx descriptor */\n> +#define AVE_RXDC0_ADDR\t\tGENMASK(14, 0)\t/* Start address */\n> +#define AVE_RXDC0_ADDR_START\t0\n> +\n> +/* AVE_IIRQC */\n> +#define AVE_IIRQC_EN0\t\tBIT(27)\t/* Enable interval interrupt Ring0 */\n> +#define AVE_IIRQC_BSCK\t\tGENMASK(15, 0)\t/* Interval count unit */\n> +\n> +/* Command status for Descriptor */\n> +#define AVE_STS_OWN\t\tBIT(31)\t/* Descriptor ownership */\n> +#define AVE_STS_INTR\t\tBIT(29)\t/* Request for interrupt */\n> +#define AVE_STS_OK\t\tBIT(27)\t/* Normal transmit */\n> +/* TX */\n> +#define AVE_STS_NOCSUM\t\tBIT(28)\t/* No use HW checksum */\n> +#define AVE_STS_1ST\t\tBIT(26)\t/* Head of buffer chain */\n> +#define AVE_STS_LAST\t\tBIT(25)\t/* Tail of buffer chain */\n> +#define AVE_STS_OWC\t\tBIT(21)\t/* Out of window,Late Collision */\n> +#define AVE_STS_EC\t\tBIT(20)\t/* Excess collision occurred */\n> +#define AVE_STS_PKTLEN_TX\tGENMASK(15, 0)\n> +/* RX */\n> +#define AVE_STS_CSSV\t\tBIT(21)\t/* Checksum check performed */\n> +#define AVE_STS_CSER\t\tBIT(20)\t/* Checksum error detected */\n> +#define AVE_STS_PKTLEN_RX\tGENMASK(10, 0)\n> +\n> +/* AVE_CFGR */\n> +#define AVE_CFGR_FLE\t\tBIT(31)\t/* Filter Function */\n> +#define AVE_CFGR_CHE\t\tBIT(30)\t/* Checksum Function */\n> +#define AVE_CFGR_MII\t\tBIT(27)\t/* Func mode (1:MII/RMII, 0:RGMII) */\n> +#define AVE_CFGR_IPFCEN\t\tBIT(24)\t/* IP fragment sum Enable */\n> +\n> +#define AVE_MAX_ETHFRAME\t1518\n> +\n> +/* Packet filter size */\n> +#define AVE_PF_SIZE\t\t17\t/* Number of all packet filter */\n> +#define AVE_PF_MULTICAST_SIZE\t7\t/* Number of multicast filter */\n> +\n> +/* Packet filter definition */\n> +#define AVE_PFNUM_FILTER\t0\t/* No.0 */\n> +#define AVE_PFNUM_UNICAST\t1\t/* No.1 */\n> +#define AVE_PFNUM_BROADCAST\t2\t/* No.2 */\n> +#define AVE_PFNUM_MULTICAST\t11\t/* No.11-17 */\n> +\n> +/* NETIF Message control */\n> +#define AVE_DEFAULT_MSG_ENABLE\t(NETIF_MSG_DRV    |\t\\\n> +\t\t\t\t NETIF_MSG_PROBE  |\t\\\n> +\t\t\t\t NETIF_MSG_LINK   |\t\\\n> +\t\t\t\t NETIF_MSG_TIMER  |\t\\\n> +\t\t\t\t NETIF_MSG_IFDOWN |\t\\\n> +\t\t\t\t NETIF_MSG_IFUP   |\t\\\n> +\t\t\t\t NETIF_MSG_RX_ERR |\t\\\n> +\t\t\t\t NETIF_MSG_TX_ERR)\n> +\n> +/* Parameter for descriptor */\n> +#define AVE_NR_TXDESC\t\t32\t/* Tx descriptor */\n> +#define AVE_NR_RXDESC\t\t64\t/* Rx descriptor */\n> +\n> +/* Parameter for interrupt */\n> +#define AVE_INTM_COUNT\t\t20\n> +#define AVE_FORCE_TXINTCNT\t1\n> +\n> +#define IS_DESC_64BIT(p)\t((p)->desc_size == AVE_DESC_SIZE_64)\n> +\n> +enum desc_id {\n> +\tAVE_DESCID_TX = 0,\n> +\tAVE_DESCID_RX,\n> +};\n> +\n> +enum desc_state {\n> +\tAVE_DESC_STOP = 0,\n> +\tAVE_DESC_START,\n> +\tAVE_DESC_RX_SUSPEND,\n> +\tAVE_DESC_RX_PERMIT,\n> +};\n> +\n> +struct ave_desc {\n> +\tstruct sk_buff\t*skbs;\n> +\tdma_addr_t\tskbs_dma;\n> +\tsize_t\t\tskbs_dmalen;\n> +};\n> +\n> +struct ave_desc_info {\n> +\tu32\tndesc;\t\t/* number of descriptor */\n> +\tu32\tdaddr;\t\t/* start address of descriptor */\n> +\tu32\tproc_idx;\t/* index of processing packet */\n> +\tu32\tdone_idx;\t/* index of processed packet */\n> +\tstruct ave_desc *desc;\t/* skb info related descriptor */\n> +};\n> +\n> +struct ave_private {\n> +\tint\t\t\tphy_id;\n> +\tunsigned int\t\tdesc_size;\n> +\tu32\t\t\tmsg_enable;\n> +\tstruct clk\t\t*clk;\n> +\tphy_interface_t\t\tphy_mode;\n> +\tstruct phy_device\t*phydev;\n> +\tstruct mii_bus\t\t*mdio;\n> +\tstruct net_device_stats\tstats;\n> +\tbool\t\t\tinternal_phy_interrupt;\n> +\n> +\t/* NAPI support */\n> +\tstruct net_device\t*ndev;\n> +\tstruct napi_struct\tnapi_rx;\n> +\tstruct napi_struct\tnapi_tx;\n> +\n> +\t/* descriptor */\n> +\tstruct ave_desc_info\trx;\n> +\tstruct ave_desc_info\ttx;\n> +};\n> +\n> +static inline u32 ave_r32(struct net_device *ndev, u32 offset)\n> +{\n> +\tvoid __iomem *addr = (void __iomem *)ndev->base_addr;\n> +\n> +\treturn readl_relaxed(addr + offset);\n> +}\n\nDon't do this, ndev->base_addr is convenient, but is now deprecated\n(unlike ISA cards, you can't change this anymore) instead, just pass a\nreference to an ave_private structure and store the base register\npointer there.\n\n> +\n> +static inline void ave_w32(struct net_device *ndev, u32 offset, u32 value)\n> +{\n> +\tvoid __iomem *addr = (void __iomem *)ndev->base_addr;\n> +\n> +\twritel_relaxed(value, addr + offset);\n> +}\n\nSame here.\n\n> +\n> +static inline u32 ave_rdesc(struct net_device *ndev, enum desc_id id,\n> +\t\t\t    int entry, int offset)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 addr = (id == AVE_DESCID_TX) ? priv->tx.daddr : priv->rx.daddr;\n> +\n> +\taddr += entry * priv->desc_size + offset;\n> +\n> +\treturn ave_r32(ndev, addr);\n> +}\n> +\n> +static inline void ave_wdesc(struct net_device *ndev, enum desc_id id,\n> +\t\t\t     int entry, int offset, u32 val)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 addr = (id == AVE_DESCID_TX) ? priv->tx.daddr : priv->rx.daddr;\n> +\n> +\taddr += entry * priv->desc_size + offset;\n> +\n> +\tave_w32(ndev, addr, val);\n> +}\n> +\n> +static void ave_wdesc_addr(struct net_device *ndev, enum desc_id id,\n> +\t\t\t   int entry, int offset, dma_addr_t paddr)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\n> +\tave_wdesc(ndev, id, entry, offset, (u32)((u64)paddr & 0xFFFFFFFFULL));\n\nlower_32_bits()\n\n> +\tif (IS_DESC_64BIT(priv))\n> +\t\tave_wdesc(ndev, id,\n> +\t\t\t  entry, offset + 4, (u32)((u64)paddr >> 32));\n\nupper_32_bits()\n\n> +\telse if ((u64)paddr > (u64)U32_MAX)\n> +\t\tnetdev_warn(ndev, \"DMA address exceeds the address space\\n\");\n> +}\n> +\n> +static void ave_get_fwversion(struct net_device *ndev, char *buf, int len)\n> +{\n> +\tu32 major, minor;\n> +\n> +\tmajor = (ave_r32(ndev, AVE_VR) & GENMASK(15, 8)) >> 8;\n> +\tminor = (ave_r32(ndev, AVE_VR) & GENMASK(7, 0));\n> +\tsnprintf(buf, len, \"v%u.%u\", major, minor);\n> +}\n> +\n> +static void ave_get_drvinfo(struct net_device *ndev,\n> +\t\t\t    struct ethtool_drvinfo *info)\n> +{\n> +\tstruct device *dev = ndev->dev.parent;\n> +\n> +\tstrlcpy(info->driver, dev->driver->name, sizeof(info->driver));\n> +\tstrlcpy(info->bus_info, dev_name(dev), sizeof(info->bus_info));\n\nbus_info would likely be platform, since this is a memory mapped\nperipheral, right?\n\n> +\tave_get_fwversion(ndev, info->fw_version, sizeof(info->fw_version));\n> +}\n> +\n> +static int ave_nway_reset(struct net_device *ndev)\n> +{\n> +\treturn genphy_restart_aneg(ndev->phydev);\n> +}\n\nYou can just set your ethtool_ops::nway_reset to be\nphy_ethtool_nway_reset() which does additional checks.\n\n> +\n> +static u32 ave_get_link(struct net_device *ndev)\n> +{\n> +\tint err;\n> +\n> +\terr = genphy_update_link(ndev->phydev);\n> +\tif (err)\n> +\t\treturn ethtool_op_get_link(ndev);\n\nNo, calling genphy_update_link() is bogus see:\n\ne69e46261063a25c3907bed16a2e9d18b115d1fd (\"net: dsa: Do not clobber PHY\nlink outside of state machine\")\n\n> +\n> +\treturn ndev->phydev->link;\n\nThis can just be ethtool_op_get_link()\n\n> +}\n> +\n> +static u32 ave_get_msglevel(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\n> +\treturn priv->msg_enable;\n> +}\n> +\n> +static void ave_set_msglevel(struct net_device *ndev, u32 val)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\n> +\tpriv->msg_enable = val;\n> +}\n> +\n> +static void ave_get_wol(struct net_device *ndev,\n> +\t\t\tstruct ethtool_wolinfo *wol)\n> +{\n> +\twol->supported = 0;\n> +\twol->wolopts   = 0;\n> +\tphy_ethtool_get_wol(ndev->phydev, wol);\n\nThis can be called before you successfully connected to a PHY so you\nneed to check that ndev->phydev is not NULL at the very least.\n\n> +}\n> +\n> +static int ave_set_wol(struct net_device *ndev,\n> +\t\t       struct ethtool_wolinfo *wol)\n> +{\n> +\tif (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE))\n> +\t\treturn -EOPNOTSUPP;\n> +\n> +\treturn phy_ethtool_set_wol(ndev->phydev, wol);\n\nSame here.\n\n\n> +\n> +static int ave_mdio_busywait(struct net_device *ndev)\n> +{\n> +\tint ret = 1, loop = 100;\n> +\tu32 mdiosr;\n> +\n> +\t/* wait until completion */\n> +\twhile (1) {\n\nSince you are already bound by the number of times you allow this look,\njust make that a clear condition for the while() loop to exit.\n\n> +\t\tmdiosr = ave_r32(ndev, AVE_MDIOSR);\n> +\t\tif (!(mdiosr & AVE_MDIOSR_STS))\n> +\t\t\tbreak;\n> +\n> +\t\tusleep_range(10, 20);\n> +\t\tif (!loop--) {\n> +\t\t\tnetdev_err(ndev,\n> +\t\t\t\t   \"failed to read from MDIO (status:0x%08x)\\n\",\n> +\t\t\t\t   mdiosr);\n> +\t\t\tret = 0;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +static int ave_mdiobus_read(struct mii_bus *bus, int phyid, int regnum)\n> +{\n> +\tstruct net_device *ndev = bus->priv;\n> +\tu32 mdioctl;\n> +\n> +\t/* write address */\n> +\tave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);\n> +\n> +\t/* read request */\n> +\tmdioctl = ave_r32(ndev, AVE_MDIOCTR);\n> +\tave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_RREQ);\n> +\n> +\tif (!ave_mdio_busywait(ndev)) {\n> +\t\tnetdev_err(ndev, \"phy-%d reg-%x read failed\\n\",\n> +\t\t\t   phyid, regnum);\n> +\t\treturn 0xffff;\n> +\t}\n> +\n> +\treturn ave_r32(ndev, AVE_MDIORDR) & GENMASK(15, 0);\n> +}\n> +\n> +static int ave_mdiobus_write(struct mii_bus *bus,\n> +\t\t\t     int phyid, int regnum, u16 val)\n> +{\n> +\tstruct net_device *ndev = bus->priv;\n> +\tu32 mdioctl;\n> +\n> +\t/* write address */\n> +\tave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);\n> +\n> +\t/* write data */\n> +\tave_w32(ndev, AVE_MDIOWDR, val);\n> +\n> +\t/* write request */\n> +\tmdioctl = ave_r32(ndev, AVE_MDIOCTR);\n> +\tave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);\n> +\n> +\tif (!ave_mdio_busywait(ndev)) {\n> +\t\tnetdev_err(ndev, \"phy-%d reg-%x write failed\\n\",\n> +\t\t\t   phyid, regnum);\n> +\t\treturn -1;\n> +\t}\n\nJust propagate the return value of ave_mdio_busywait().\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +static dma_addr_t ave_dma_map(struct net_device *ndev, struct ave_desc *desc,\n> +\t\t\t      void *ptr, size_t len,\n> +\t\t\t      enum dma_data_direction dir)\n> +{\n> +\tdma_addr_t paddr;\n> +\n> +\tpaddr = dma_map_single(ndev->dev.parent, ptr, len, dir);\n> +\tif (dma_mapping_error(ndev->dev.parent, paddr)) {\n> +\t\tdesc->skbs_dma = 0;\n> +\t\tpaddr = (dma_addr_t)virt_to_phys(ptr);\n\nWhy do you do that? If dma_map_single() failed, that's it, game over,\nyou need to discad the packet, not assume that it is in the kernel's\nlinear mapping!\n\n> +\t} else {\n> +\t\tdesc->skbs_dma = paddr;\n> +\t\tdesc->skbs_dmalen = len;\n> +\t}\n> +\n> +\treturn paddr;\n> +}\n> +\n> +static void ave_dma_unmap(struct net_device *ndev, struct ave_desc *desc,\n> +\t\t\t  enum dma_data_direction dir)\n> +{\n> +\tif (!desc->skbs_dma)\n> +\t\treturn;\n> +\n> +\tdma_unmap_single(ndev->dev.parent,\n> +\t\t\t desc->skbs_dma, desc->skbs_dmalen, dir);\n> +\tdesc->skbs_dma = 0;\n> +}\n> +\n> +/* Set Rx descriptor and memory */\n> +static int ave_set_rxdesc(struct net_device *ndev, int entry)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tstruct sk_buff *skb;\n> +\tunsigned long align;\n> +\tdma_addr_t paddr;\n> +\tvoid *buffptr;\n> +\tint ret = 0;\n> +\n> +\tskb = priv->rx.desc[entry].skbs;\n> +\tif (!skb) {\n> +\t\tskb = netdev_alloc_skb_ip_align(ndev,\n> +\t\t\t\t\t\tAVE_MAX_ETHFRAME + NET_SKB_PAD);\n> +\t\tif (!skb) {\n> +\t\t\tnetdev_err(ndev, \"can't allocate skb for Rx\\n\");\n> +\t\t\treturn -ENOMEM;\n> +\t\t}\n> +\t}\n> +\n> +\t/* set disable to cmdsts */\n> +\tave_wdesc(ndev, AVE_DESCID_RX, entry, 0, AVE_STS_INTR | AVE_STS_OWN);\n> +\n> +\t/* align skb data for cache size */\n> +\talign = (unsigned long)skb_tail_pointer(skb) & (NET_SKB_PAD - 1);\n> +\talign = NET_SKB_PAD - align;\n> +\tskb_reserve(skb, align);\n> +\tbuffptr = (void *)skb_tail_pointer(skb);\n\nAre you positive you need this? Because by default, the networking stack\nwill align to the maximum between your L1 cache line size and 64 bytes,\nwhich should be a pretty good alignment guarantee.\n\n> +\n> +\tpaddr = ave_dma_map(ndev, &priv->rx.desc[entry], buffptr,\n> +\t\t\t    AVE_MAX_ETHFRAME + NET_IP_ALIGN, DMA_FROM_DEVICE);\n\nThis needs to be checked, as mentioned before, you can't just assume the\nlinear virtual map is going to be satisfactory.\n\n> +\tpriv->rx.desc[entry].skbs = skb;\n> +\n> +\t/* set buffer pointer */\n> +\tave_wdesc_addr(ndev, AVE_DESCID_RX, entry, 4, paddr);\n\nOK, so 4 is an offset, can you add a define for that so it's clear it is\nnot an address size instead?\n\n> +\n> +\t/* set enable to cmdsts */\n> +\tave_wdesc(ndev, AVE_DESCID_RX,\n> +\t\t  entry, 0, AVE_STS_INTR | AVE_MAX_ETHFRAME);\n> +\n> +\treturn ret;\n> +}\n> +\n> +/* Switch state of descriptor */\n> +static int ave_desc_switch(struct net_device *ndev, enum desc_state state)\n> +{\n> +\tint counter;\n> +\tu32 val;\n> +\n> +\tswitch (state) {\n> +\tcase AVE_DESC_START:\n> +\t\tave_w32(ndev, AVE_DESCC, AVE_DESCC_TD | AVE_DESCC_RD0);\n> +\t\tbreak;\n> +\n> +\tcase AVE_DESC_STOP:\n> +\t\tave_w32(ndev, AVE_DESCC, 0);\n> +\t\tcounter = 0;\n> +\t\twhile (1) {\n> +\t\t\tusleep_range(100, 150);\n> +\t\t\tif (!ave_r32(ndev, AVE_DESCC))\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tcounter++;\n> +\t\t\tif (counter > 100) {\n> +\t\t\t\tnetdev_err(ndev, \"can't stop descriptor\\n\");\n> +\t\t\t\treturn -EBUSY;\n> +\t\t\t}\n> +\t\t}\n\nSame here, pleas rewrite the loop so the exit condition is clear.\n\n> +\t\tbreak;\n> +\n> +\tcase AVE_DESC_RX_SUSPEND:\n> +\t\tval = ave_r32(ndev, AVE_DESCC);\n> +\t\tval |= AVE_DESCC_RDSTP;\n> +\t\tval &= AVE_DESCC_CTRL_MASK;\n\nShould not this be a &= ~AVE_DESCC_CTRL_MASK? OK maybe not.\n\n> +\t\tave_w32(ndev, AVE_DESCC, val);\n> +\n> +\t\tcounter = 0;\n> +\t\twhile (1) {\n> +\t\t\tusleep_range(100, 150);\n> +\t\t\tval = ave_r32(ndev, AVE_DESCC);\n> +\t\t\tif (val & (AVE_DESCC_RDSTP << 16))\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tcounter++;\n> +\t\t\tif (counter > 1000) {\n> +\t\t\t\tnetdev_err(ndev, \"can't suspend descriptor\\n\");\n> +\t\t\t\treturn -EBUSY;\n> +\t\t\t}\n> +\t\t}\n> +\t\tbreak;\n\nSame here, please rewrite with the counter as an exit condition.\n\n> +\n> +\tcase AVE_DESC_RX_PERMIT:\n> +\t\tval = ave_r32(ndev, AVE_DESCC);\n> +\t\tval &= ~AVE_DESCC_RDSTP;\n> +\t\tval &= AVE_DESCC_CTRL_MASK;\n> +\t\tave_w32(ndev, AVE_DESCC, val);\n> +\t\tbreak;\n> +\n> +\tdefault:\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int ave_tx_completion(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 proc_idx, done_idx, ndesc, val;\n> +\tint freebuf_flag = 0;\n> +\n> +\tproc_idx = priv->tx.proc_idx;\n> +\tdone_idx = priv->tx.done_idx;\n> +\tndesc    = priv->tx.ndesc;\n> +\n> +\t/* free pre stored skb from done to proc */\n> +\twhile (proc_idx != done_idx) {\n> +\t\t/* get cmdsts */\n> +\t\tval = ave_rdesc(ndev, AVE_DESCID_TX, done_idx, 0);\n> +\n> +\t\t/* do nothing if owner is HW */\n> +\t\tif (val & AVE_STS_OWN)\n> +\t\t\tbreak;\n> +\n> +\t\t/* check Tx status and updates statistics */\n> +\t\tif (val & AVE_STS_OK) {\n> +\t\t\tpriv->stats.tx_bytes += val & AVE_STS_PKTLEN_TX;\n\nAVE_STS_PKTLEN_TX should be suffixed with _MASK to make it clear this is\na bitmask.\n\n> +\n> +\t\t\t/* success */\n> +\t\t\tif (val & AVE_STS_LAST)\n> +\t\t\t\tpriv->stats.tx_packets++;\n> +\t\t} else {\n> +\t\t\t/* error */\n> +\t\t\tif (val & AVE_STS_LAST) {\n> +\t\t\t\tpriv->stats.tx_errors++;\n> +\t\t\t\tif (val & (AVE_STS_OWC | AVE_STS_EC))\n> +\t\t\t\t\tpriv->stats.collisions++;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/* release skb */\n> +\t\tif (priv->tx.desc[done_idx].skbs) {\n> +\t\t\tave_dma_unmap(ndev, &priv->tx.desc[done_idx],\n> +\t\t\t\t      DMA_TO_DEVICE);\n> +\t\t\tdev_consume_skb_any(priv->tx.desc[done_idx].skbs);\n\nKudos for using dev_consume_skb_any()!\n\n> +\t\t\tpriv->tx.desc[done_idx].skbs = NULL;\n> +\t\t\tfreebuf_flag++;\n> +\t\t}\n> +\t\tdone_idx = (done_idx + 1) % ndesc;\n> +\t}\n> +\n> +\tpriv->tx.done_idx = done_idx;\n> +\n> +\t/* wake queue for freeing buffer */\n> +\tif (netif_queue_stopped(ndev))\n> +\t\t\tif (freebuf_flag)\n> +\t\t\t\tnetif_wake_queue(ndev);\n\nThis can be simplified to:\n\n\tif (netif_queue_stopped(ndev) && freebuf_flag)\n\t\tnetif_wake_queue(ndev)\n\nbecause checking for netif_running() is implicit by actually getting\ncalled here, this would not happen if the network interface was not\noperational.\n\n> +static irqreturn_t ave_interrupt(int irq, void *netdev)\n> +{\n> +\tstruct net_device *ndev = (struct net_device *)netdev;\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 gimr_val, gisr_val;\n> +\n> +\tgimr_val = ave_irq_disable_all(ndev);\n> +\n> +\t/* get interrupt status */\n> +\tgisr_val = ave_r32(ndev, AVE_GISR);\n> +\n> +\t/* PHY */\n> +\tif (gisr_val & AVE_GI_PHY) {\n> +\t\tave_w32(ndev, AVE_GISR, AVE_GI_PHY);\n> +\t\tif (priv->internal_phy_interrupt)\n> +\t\t\tphy_mac_interrupt(ndev->phydev, ndev->phydev->link);\n\nThis is not correct you should be telling the PHY the new link state. If\nyou cannot, because what happens here is really that the PHY interrupt\nis routed to your MAC controller, then what I would suggest doing is the\nfollowing: create an interrupt controller domain which allows the PHY\ndriver to manage the interrupt line using normal request_irq().\n\n\n> +static int ave_poll_tx(struct napi_struct *napi, int budget)\n> +{\n> +\tstruct ave_private *priv;\n> +\tstruct net_device *ndev;\n> +\tint num;\n> +\n> +\tpriv = container_of(napi, struct ave_private, napi_tx);\n> +\tndev = priv->ndev;\n> +\n> +\tnum = ave_tx_completion(ndev);\n> +\tif (num < budget) {\n\nTX reclamation should not be bound against the budget, always process\nall packets that have been successfully submitted.\n\n> +\t\tnapi_complete(napi);\n> +\n> +\t\t/* enable Tx interrupt when NAPI finishes */\n> +\t\tave_irq_enable(ndev, AVE_GI_TX);\n> +\t}\n> +\n> +\treturn num;\n> +}\n> +\n\n> +static void ave_adjust_link(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tstruct phy_device *phydev = ndev->phydev;\n> +\tu32 val, txcr, rxcr, rxcr_org;\n> +\n> +\t/* set RGMII speed */\n> +\tval = ave_r32(ndev, AVE_TXCR);\n> +\tval &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);\n> +\n> +\tif (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&\n> +\t    phydev->speed == SPEED_1000)\n> +\t\tval |= AVE_TXCR_TXSPD_1G;\n\nUsing phy_interface_is_rgmii() would be more robust here.\n\n> +\telse if (phydev->speed == SPEED_100)\n> +\t\tval |= AVE_TXCR_TXSPD_100;\n> +\n> +\tave_w32(ndev, AVE_TXCR, val);\n> +\n> +\t/* set RMII speed (100M/10M only) */\n> +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n\nPHY_INTERFACE_MODE_MII, PHY_INTERFACE_MODE_REVMII,\nPHY_INTERFACE_MODE_RMII would all qualify here presumably so you need\nthis check to be more restrictive to just the modes that you support.\n\n> +\t\tval = ave_r32(ndev, AVE_LINKSEL);\n> +\t\tif (phydev->speed == SPEED_10)\n> +\t\t\tval &= ~AVE_LINKSEL_100M;\n> +\t\telse\n> +\t\t\tval |= AVE_LINKSEL_100M;\n> +\t\tave_w32(ndev, AVE_LINKSEL, val);\n> +\t}\n> +\n> +\t/* check current RXCR/TXCR */\n> +\trxcr = ave_r32(ndev, AVE_RXCR);\n> +\ttxcr = ave_r32(ndev, AVE_TXCR);\n> +\trxcr_org = rxcr;\n> +\n> +\tif (phydev->duplex)\n> +\t\trxcr |= AVE_RXCR_FDUPEN;\n> +\telse\n> +\t\trxcr &= ~AVE_RXCR_FDUPEN;\n> +\n> +\tif (phydev->pause) {\n> +\t\trxcr |= AVE_RXCR_FLOCTR;\n> +\t\ttxcr |= AVE_TXCR_FLOCTR;\n\nYou must also check for phydev->asym_pause and keep in mind that\nphydev->pause and phydev->asym_pause reflect what the link partner\nreflects, you need to implement .get_pauseparam and .set_pauseparam or\ndefault to flow control ON (which is what you are doing here).\n\n> +\t} else {\n> +\t\trxcr &= ~AVE_RXCR_FLOCTR;\n> +\t\ttxcr &= ~AVE_TXCR_FLOCTR;\n> +\t}\n> +\n> +\tif (rxcr_org != rxcr) {\n> +\t\t/* disable Rx mac */\n> +\t\tave_w32(ndev, AVE_RXCR, rxcr & ~AVE_RXCR_RXEN);\n> +\t\t/* change and enable TX/Rx mac */\n> +\t\tave_w32(ndev, AVE_TXCR, txcr);\n> +\t\tave_w32(ndev, AVE_RXCR, rxcr);\n> +\t}\n> +\n> +\tif (phydev->link)\n> +\t\tnetif_carrier_on(ndev);\n> +\telse\n> +\t\tnetif_carrier_off(ndev);\n\nThis is not necessary, PHYLIB is specifically taking care of that.\n\n> +\n> +\tphy_print_status(phydev);\n> +}\n> +\n> +static int ave_init(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tstruct device *dev = ndev->dev.parent;\n> +\tstruct device_node *phy_node, *np = dev->of_node;\n> +\tstruct phy_device *phydev;\n> +\tconst void *mac_addr;\n> +\tu32 supported;\n> +\n> +\t/* get mac address */\n> +\tmac_addr = of_get_mac_address(np);\n> +\tif (mac_addr)\n> +\t\tether_addr_copy(ndev->dev_addr, mac_addr);\n> +\n> +\t/* if the mac address is invalid, use random mac address */\n> +\tif (!is_valid_ether_addr(ndev->dev_addr)) {\n> +\t\teth_hw_addr_random(ndev);\n> +\t\tdev_warn(dev, \"Using random MAC address: %pM\\n\",\n> +\t\t\t ndev->dev_addr);\n> +\t}\n> +\n> +\t/* attach PHY with MAC */\n> +\tphy_node =  of_get_next_available_child(np, NULL);\n\nYou should be using a \"phy-handle\" property to connect to a designated\nPHY, not the next child DT node.\n\n> +\tphydev = of_phy_connect(ndev, phy_node,\n> +\t\t\t\tave_adjust_link, 0, priv->phy_mode);\n> +\tif (!phydev) {\n> +\t\tdev_err(dev, \"could not attach to PHY\\n\");\n> +\t\treturn -ENODEV;\n> +\t}\n> +\tof_node_put(phy_node);\n> +\n> +\tpriv->phydev = phydev;\n> +\tphydev->autoneg = AUTONEG_ENABLE;\n> +\tphydev->speed = 0;\n> +\tphydev->duplex = 0;\n\nThis is not necessary.\n\n> +\n> +\tdev_info(dev, \"connected to %s phy with id 0x%x\\n\",\n> +\t\t phydev->drv->name, phydev->phy_id);\n> +\n> +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n> +\t\tsupported = phydev->supported;\n> +\t\tphydev->supported &= ~PHY_GBIT_FEATURES;\n> +\t\tphydev->supported |= supported & PHY_BASIC_FEATURES;\n\nOne of these two statements is redundant here.\n\n> +\t}\n> +\n> +\t/* PHY interrupt stop instruction is needed because the interrupt\n> +\t * continues to assert.\n> +\t */\n> +\tphy_stop_interrupts(phydev);\n\nWait, what?\n\n> +\n> +\t/* When PHY driver can't handle its interrupt directly,\n> +\t * interrupt request always fails and polling method is used\n> +\t * alternatively. In this case, the libphy says\n> +\t * \"libphy: uniphier-mdio: Can't get IRQ -1 (PHY)\".\n> +\t */\n> +\tphy_start_interrupts(phydev);\n> +\n> +\tphy_start_aneg(phydev);\n\nNo, no, phy_start() would take care of all of that correctly for you,\nyou don't have have to do it just there because ave_open() eventually\ncalls phy_start() for you, so just drop these two calls.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void ave_uninit(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\n> +\tphy_stop_interrupts(priv->phydev);\n\nYou are missing a call to phy_stop() here, calling phy_stop_interrupts()\ndirectly should not happen.\n\n> +\tphy_disconnect(priv->phydev);\n> +}\n> +\n> +static int ave_open(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tint entry;\n> +\tu32 val;\n> +\n> +\t/* initialize Tx work */\n> +\tpriv->tx.proc_idx = 0;\n> +\tpriv->tx.done_idx = 0;\n> +\tmemset(priv->tx.desc, 0, sizeof(struct ave_desc) * priv->tx.ndesc);\n> +\n> +\t/* initialize Tx descriptor */\n> +\tfor (entry = 0; entry < priv->tx.ndesc; entry++) {\n> +\t\tave_wdesc(ndev, AVE_DESCID_TX, entry, 0, 0);\n> +\t\tave_wdesc_addr(ndev, AVE_DESCID_TX, entry, 4, 0);\n> +\t}\n> +\tave_w32(ndev, AVE_TXDC, AVE_TXDC_ADDR_START\n> +\t\t| (((priv->tx.ndesc * priv->desc_size) << 16) & AVE_TXDC_SIZE));\n> +\n> +\t/* initialize Rx work */\n> +\tpriv->rx.proc_idx = 0;\n> +\tpriv->rx.done_idx = 0;\n> +\tmemset(priv->rx.desc, 0, sizeof(struct ave_desc) * priv->rx.ndesc);\n> +\n> +\t/* initialize Rx descriptor and buffer */\n> +\tfor (entry = 0; entry < priv->rx.ndesc; entry++) {\n> +\t\tif (ave_set_rxdesc(ndev, entry))\n> +\t\t\tbreak;\n> +\t}\n> +\tave_w32(ndev, AVE_RXDC0, AVE_RXDC0_ADDR_START\n> +\t    | (((priv->rx.ndesc * priv->desc_size) << 16) & AVE_RXDC0_SIZE));\n> +\n> +\t/* enable descriptor */\n> +\tave_desc_switch(ndev, AVE_DESC_START);\n> +\n> +\t/* initialize filter */\n> +\tave_pfsel_init(ndev);\n> +\n> +\t/* set macaddr */\n> +\tval = le32_to_cpu(((u32 *)ndev->dev_addr)[0]);\n> +\tave_w32(ndev, AVE_RXMAC1R, val);\n> +\tval = (u32)le16_to_cpu(((u16 *)ndev->dev_addr)[2]);\n> +\tave_w32(ndev, AVE_RXMAC2R, val);\n> +\n> +\t/* set Rx configuration */\n> +\t/* full duplex, enable pause drop, enalbe flow control */\n> +\tval = AVE_RXCR_RXEN | AVE_RXCR_FDUPEN | AVE_RXCR_DRPEN |\n> +\t\tAVE_RXCR_FLOCTR | (AVE_MAX_ETHFRAME & AVE_RXCR_MPSIZ_MASK);\n> +\tave_w32(ndev, AVE_RXCR, val);\n> +\n> +\t/* set Tx configuration */\n> +\t/* enable flow control, disable loopback */\n> +\tave_w32(ndev, AVE_TXCR, AVE_TXCR_FLOCTR);\n> +\n> +\t/* enable timer, clear EN,INTM, and mask interval unit(BSCK) */\n> +\tval = ave_r32(ndev, AVE_IIRQC) & AVE_IIRQC_BSCK;\n> +\tval |= AVE_IIRQC_EN0 | (AVE_INTM_COUNT << 16);\n> +\tave_w32(ndev, AVE_IIRQC, val);\n> +\n> +\t/* set interrupt mask */\n> +\tval = AVE_GI_RXIINT | AVE_GI_RXOVF | AVE_GI_TX;\n> +\tval |= (priv->internal_phy_interrupt) ? AVE_GI_PHY : 0;\n> +\tave_irq_restore(ndev, val);\n> +\n> +\tnapi_enable(&priv->napi_rx);\n> +\tnapi_enable(&priv->napi_tx);\n> +\n> +\tphy_start(ndev->phydev);\n> +\tnetif_start_queue(ndev);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int ave_stop(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tint entry;\n> +\n> +\t/* disable all interrupt */\n> +\tave_irq_disable_all(ndev);\n> +\tdisable_irq(ndev->irq);\n> +\n> +\tnetif_tx_disable(ndev);\n> +\tphy_stop(ndev->phydev);\n> +\tnapi_disable(&priv->napi_tx);\n> +\tnapi_disable(&priv->napi_rx);\n> +\n> +\t/* free Tx buffer */\n> +\tfor (entry = 0; entry < priv->tx.ndesc; entry++) {\n> +\t\tif (!priv->tx.desc[entry].skbs)\n> +\t\t\tcontinue;\n> +\n> +\t\tave_dma_unmap(ndev, &priv->tx.desc[entry], DMA_TO_DEVICE);\n> +\t\tdev_kfree_skb_any(priv->tx.desc[entry].skbs);\n> +\t\tpriv->tx.desc[entry].skbs = NULL;\n> +\t}\n> +\tpriv->tx.proc_idx = 0;\n> +\tpriv->tx.done_idx = 0;\n> +\n> +\t/* free Rx buffer */\n> +\tfor (entry = 0; entry < priv->rx.ndesc; entry++) {\n> +\t\tif (!priv->rx.desc[entry].skbs)\n> +\t\t\tcontinue;\n> +\n> +\t\tave_dma_unmap(ndev, &priv->rx.desc[entry], DMA_FROM_DEVICE);\n> +\t\tdev_kfree_skb_any(priv->rx.desc[entry].skbs);\n> +\t\tpriv->rx.desc[entry].skbs = NULL;\n> +\t}\n> +\tpriv->rx.proc_idx = 0;\n> +\tpriv->rx.done_idx = 0;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 proc_idx, done_idx, ndesc, cmdsts;\n> +\tint freepkt;\n> +\tunsigned char *buffptr = NULL; /* buffptr for descriptor */\n> +\tunsigned int len;\n> +\tdma_addr_t paddr;\n> +\n> +\tproc_idx = priv->tx.proc_idx;\n> +\tdone_idx = priv->tx.done_idx;\n> +\tndesc = priv->tx.ndesc;\n> +\tfreepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;\n> +\n> +\t/* not enough entry, then we stop queue */\n> +\tif (unlikely(freepkt < 2)) {\n> +\t\tnetif_stop_queue(ndev);\n> +\t\tif (unlikely(freepkt < 1))\n> +\t\t\treturn NETDEV_TX_BUSY;\n> +\t}\n> +\n> +\tpriv->tx.desc[proc_idx].skbs = skb;\n> +\n> +\t/* add padding for short packet */\n> +\tif (skb_padto(skb, ETH_ZLEN)) {\n> +\t\tdev_kfree_skb_any(skb);\n> +\t\treturn NETDEV_TX_OK;\n> +\t}\n> +\n> +\tbuffptr = skb->data - NET_IP_ALIGN;\n> +\tlen = max_t(unsigned int, ETH_ZLEN, skb->len);\n> +\n> +\tpaddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,\n> +\t\t\t    len + NET_IP_ALIGN, DMA_TO_DEVICE);\n> +\tpaddr += NET_IP_ALIGN;\n> +\n> +\t/* set buffer address to descriptor */\n> +\tave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);\n> +\n> +\t/* set flag and length to send */\n> +\tcmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST\n> +\t\t| (len & AVE_STS_PKTLEN_TX);\n> +\n> +\t/* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */\n> +\tif (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))\n> +\t\tcmdsts |= AVE_STS_INTR;\n> +\n> +\t/* disable checksum calculation when skb doesn't calurate checksum */\n> +\tif (skb->ip_summed == CHECKSUM_NONE ||\n> +\t    skb->ip_summed == CHECKSUM_UNNECESSARY)\n> +\t\tcmdsts |= AVE_STS_NOCSUM;\n> +\n> +\t/* set cmdsts */\n> +\tave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);\n> +\n> +\tpriv->tx.proc_idx = (proc_idx + 1) % ndesc;\n> +\n> +\treturn NETDEV_TX_OK;\n> +}\n> +\n> +static int ave_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)\n> +{\n> +\treturn phy_mii_ioctl(ndev->phydev, ifr, cmd);\n> +}\n> +\n> +static void ave_set_rx_mode(struct net_device *ndev)\n> +{\n> +\tint count, mc_cnt = netdev_mc_count(ndev);\n> +\tstruct netdev_hw_addr *hw_adr;\n> +\tu32 val;\n> +\tu8 v4multi_macadr[6] = { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00 };\n> +\tu8 v6multi_macadr[6] = { 0x33, 0x00, 0x00, 0x00, 0x00, 0x00 };\n> +\n> +\t/* MAC addr filter enable for promiscious mode */\n> +\tval = ave_r32(ndev, AVE_RXCR);\n> +\tif (ndev->flags & IFF_PROMISC || !mc_cnt)\n> +\t\tval &= ~AVE_RXCR_AFEN;\n> +\telse\n> +\t\tval |= AVE_RXCR_AFEN;\n> +\tave_w32(ndev, AVE_RXCR, val);\n> +\n> +\t/* set all multicast address */\n> +\tif ((ndev->flags & IFF_ALLMULTI) || (mc_cnt > AVE_PF_MULTICAST_SIZE)) {\n> +\t\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST,\n> +\t\t\t\t      v4multi_macadr, 1);\n> +\t\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST + 1,\n> +\t\t\t\t      v6multi_macadr, 1);\n> +\t} else {\n> +\t\t/* stop all multicast filter */\n> +\t\tfor (count = 0; count < AVE_PF_MULTICAST_SIZE; count++)\n> +\t\t\tave_pfsel_stop(ndev, AVE_PFNUM_MULTICAST + count);\n> +\n> +\t\t/* set multicast addresses */\n> +\t\tcount = 0;\n> +\t\tnetdev_for_each_mc_addr(hw_adr, ndev) {\n> +\t\t\tif (count == mc_cnt)\n> +\t\t\t\tbreak;\n> +\t\t\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST + count,\n> +\t\t\t\t\t      hw_adr->addr, 6);\n> +\t\t\tcount++;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +static struct net_device_stats *ave_stats(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 drop_num = 0;\n> +\n> +\tpriv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);\n> +\n> +\tdrop_num += ave_r32(ndev, AVE_RX0OVFFC);\n> +\tdrop_num += ave_r32(ndev, AVE_SN5FC);\n> +\tdrop_num += ave_r32(ndev, AVE_SN6FC);\n> +\tdrop_num += ave_r32(ndev, AVE_SN7FC);\n> +\tpriv->stats.rx_dropped = drop_num;\n> +\n> +\treturn &priv->stats;\n> +}\n> +\n> +static int ave_set_mac_address(struct net_device *ndev, void *p)\n> +{\n> +\tint ret = eth_mac_addr(ndev, p);\n> +\tu32 val;\n> +\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* set macaddr */\n> +\tval = le32_to_cpu(((u32 *)ndev->dev_addr)[0]);\n> +\tave_w32(ndev, AVE_RXMAC1R, val);\n> +\tval = (u32)le16_to_cpu(((u16 *)ndev->dev_addr)[2]);\n> +\tave_w32(ndev, AVE_RXMAC2R, val);\n> +\n> +\t/* pfsel unicast entry */\n> +\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_UNICAST, ndev->dev_addr, 6);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static const struct net_device_ops ave_netdev_ops = {\n> +\t.ndo_init\t\t= ave_init,\n> +\t.ndo_uninit\t\t= ave_uninit,\n> +\t.ndo_open\t\t= ave_open,\n> +\t.ndo_stop\t\t= ave_stop,\n> +\t.ndo_start_xmit\t\t= ave_start_xmit,\n> +\t.ndo_do_ioctl\t\t= ave_ioctl,\n> +\t.ndo_set_rx_mode\t= ave_set_rx_mode,\n> +\t.ndo_get_stats\t\t= ave_stats,\n> +\t.ndo_set_mac_address\t= ave_set_mac_address,\n> +};\n> +\n> +static int ave_probe(struct platform_device *pdev)\n> +{\n> +\tstruct device *dev = &pdev->dev;\n> +\tstruct device_node *np = dev->of_node;\n> +\tu32 desc_bits, ave_id;\n> +\tstruct reset_control *rst;\n> +\tstruct ave_private *priv;\n> +\tphy_interface_t phy_mode;\n> +\tstruct net_device *ndev;\n> +\tstruct resource\t*res;\n> +\tvoid __iomem *base;\n> +\tint irq, ret = 0;\n> +\tchar buf[ETHTOOL_FWVERS_LEN];\n> +\n> +\t/* get phy mode */\n> +\tphy_mode = of_get_phy_mode(np);\n> +\tif (phy_mode < 0) {\n> +\t\tdev_err(dev, \"phy-mode not found\\n\");\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tirq = platform_get_irq(pdev, 0);\n> +\tif (irq < 0) {\n> +\t\tdev_err(dev, \"IRQ not found\\n\");\n> +\t\treturn irq;\n> +\t}\n> +\n> +\tres = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n> +\tbase = devm_ioremap_resource(dev, res);\n> +\tif (IS_ERR(base))\n> +\t\treturn PTR_ERR(base);\n> +\n> +\t/* alloc netdevice */\n> +\tndev = alloc_etherdev(sizeof(struct ave_private));\n> +\tif (!ndev) {\n> +\t\tdev_err(dev, \"can't allocate ethernet device\\n\");\n> +\t\treturn -ENOMEM;\n> +\t}\n> +\n> +\tndev->base_addr = (unsigned long)base;\n\nThis is deprecated as mentioned earlier, just store this in\npriv->base_adr or something.\n\n> +\tndev->irq = irq;\n\nSame here.\n\n> +\tndev->netdev_ops = &ave_netdev_ops;\n> +\tndev->ethtool_ops = &ave_ethtool_ops;\n> +\tSET_NETDEV_DEV(ndev, dev);\n> +\n> +\t/* support hardware checksum */\n> +\tndev->features    |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);\n> +\tndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);\n> +\n> +\tndev->max_mtu = AVE_MAX_ETHFRAME - (ETH_HLEN + ETH_FCS_LEN);\n> +\n> +\tpriv = netdev_priv(ndev);\n> +\tpriv->ndev = ndev;\n> +\tpriv->msg_enable = netif_msg_init(-1, AVE_DEFAULT_MSG_ENABLE);\n> +\tpriv->phy_mode = phy_mode;\n> +\n> +\t/* get bits of descriptor from devicetree */\n> +\tof_property_read_u32(np, \"socionext,desc-bits\", &desc_bits);\n> +\tpriv->desc_size = AVE_DESC_SIZE_32;\n> +\tif (desc_bits == 64)\n> +\t\tpriv->desc_size = AVE_DESC_SIZE_64;\n> +\telse if (desc_bits != 32)\n> +\t\tdev_warn(dev, \"Defaulting to 32bit descriptor\\n\");\n\nThis should really be determined by the compatible string.\n\n> +\n> +\t/* use internal phy interrupt */\n> +\tpriv->internal_phy_interrupt =\n> +\t\tof_property_read_bool(np, \"socionext,internal-phy-interrupt\");\n\nSame here.\n\n> +\n> +\t/* setting private data for descriptor */\n> +\tpriv->tx.daddr = IS_DESC_64BIT(priv) ? AVE_TXDM_64 : AVE_TXDM_32;\n> +\tpriv->tx.ndesc = AVE_NR_TXDESC;\n> +\tpriv->tx.desc = devm_kzalloc(dev,\n> +\t\t\t\t     sizeof(struct ave_desc) * priv->tx.ndesc,\n> +\t\t\t\t     GFP_KERNEL);\n> +\tif (!priv->tx.desc)\n> +\t\treturn -ENOMEM;\n> +\n> +\tpriv->rx.daddr = IS_DESC_64BIT(priv) ? AVE_RXDM_64 : AVE_RXDM_32;\n> +\tpriv->rx.ndesc = AVE_NR_RXDESC;\n> +\tpriv->rx.desc = devm_kzalloc(dev,\n> +\t\t\t\t     sizeof(struct ave_desc) * priv->rx.ndesc,\n> +\t\t\t\t     GFP_KERNEL);\n> +\tif (!priv->rx.desc)\n> +\t\treturn -ENOMEM;\n\nIf your network device driver is probed, but never used after that, that\nis the network device is not open, you just this memory sitting for\nnothing, you should consider moving that to ndo_open() time.\n\n> +\n> +\t/* enable clock */\n> +\tpriv->clk = devm_clk_get(dev, NULL);\n> +\tif (IS_ERR(priv->clk))\n> +\t\tpriv->clk = NULL;\n> +\tclk_prepare_enable(priv->clk);\n\nSame here with the clock, the block is clocked, so it can consume some\namount of power, just do the necessary HW initialization with the clock\nenabled, then defer until ndo_open() before turning it back on.\n\n> +\n> +\t/* reset block */\n> +\trst = devm_reset_control_get(dev, NULL);\n> +\tif (!IS_ERR_OR_NULL(rst)) {\n> +\t\treset_control_deassert(rst);\n> +\t\treset_control_put(rst);\n> +\t}\n\nAh so that's interesting, you need it clocked first, then reset, I guess\nthat works :)\n\n> +\n> +\t/* reset mac and phy */\n> +\tave_global_reset(ndev);\n> +\n> +\t/* request interrupt */\n> +\tret = devm_request_irq(dev, irq, ave_interrupt,\n> +\t\t\t       IRQF_SHARED, ndev->name, ndev);\n> +\tif (ret)\n> +\t\tgoto err_req_irq;\n\nSame here, this is usually moved to ndo_open() for network drivers, but\nthen remember not to use devm_, just normal request_irq() because it\nneed to be balanced in ndo_close().\n\nThis looks like a pretty good first submission, and there does not\nappear to be any obvious functional problems!","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"s1cgduUr\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpnWk2zxJz9t2c\n\tfor <patchwork-incoming@ozlabs.org>;\n\tSat,  9 Sep 2017 05:32:02 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1756761AbdIHTbb (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tFri, 8 Sep 2017 15:31:31 -0400","from mail-wr0-f195.google.com ([209.85.128.195]:34769 \"EHLO\n\tmail-wr0-f195.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1756581AbdIHTb3 (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Fri, 8 Sep 2017 15:31:29 -0400","by mail-wr0-f195.google.com with SMTP id k20so1640004wre.1;\n\tFri, 08 Sep 2017 12:31:28 -0700 (PDT)","from [10.112.156.244] ([192.19.255.250])\n\tby smtp.googlemail.com with ESMTPSA id\n\te8sm4571118wmf.45.2017.09.08.12.31.20\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 08 Sep 2017 12:31:25 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=upi4qowNJl5jEh1cvuMyBM+QYDxdrHhVFxyYyhXvuro=;\n\tb=s1cgduUrRsf1Hwd+uT96ZCB6YfGmby6oAB6oDLjJATHxOrTBoms2Y5KvYNWPIBfme/\n\tFp9b0snc4P2CpZupmIUfZFmgzvH+AiII12Sk5hpwXzu+DcBbrNqgIwWHx6cO9E0WljbP\n\tbQa0ECRQgHT2PWnitd5Kww6kLL5zzF8ZLY4jCcBKUPSU6CPnnwoMyxDyY30DPx/F4pss\n\tWK0mVlcDD2552RhY5YpHbz8mnasNzuC9RbMz9LEKzgmTp1NBBhliOu8/iQhVQfErfLP8\n\tAzSA8eDB+m1t8RnSMOOHHREWr5j7Hi+xVfA/h5VyHJB1XfRybeXreCTya6S+uPpBsfdF\n\tKLIA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=upi4qowNJl5jEh1cvuMyBM+QYDxdrHhVFxyYyhXvuro=;\n\tb=AXfHIBzrKraznIzA6TX5TBTbeOiuh80KeZ9kXfzyNaSjtSCaFTUUuTzD99dlW0A89q\n\tUis9if+PR/1JdzWU7u0N/3N3lijXT+IeEMcGHybeh+PBx7F0ZgpQJ/Mb4R7W59JvkMRS\n\tIW6GvBsOiTsFprDLwAWGAfP5FNw3pBssri6K5t7qZWCel6irEq1U3IsBqsXvB/eNF3Z4\n\tgzedYBH0dAT+M9oOWVCBlOsWtGtZ1x8JSojf45nwhd68pI/1esgB2V2witKyLv4yNC/q\n\tL5EChiM69ffm4VvplWBZmtYtX7otwRSXFJUetQw0ki0Fvgzs7z9mtMiCDjigWZ7rKkln\n\t5HsA==","X-Gm-Message-State":"AHPjjUihU+bcihX+luGS+nVZV8/w3trlsW/L4gEYMB89CalfNgioJN42\n\tpSN3raF5ra8XDA==","X-Google-Smtp-Source":"ADKCNb6AFzeNefbziXctQ6csQp2/QYNA9+OIwTlh9kDHuaKkYKSeI+cozD+uSOXLh5s8pJ+IV186tg==","X-Received":"by 10.223.130.166 with SMTP id 35mr3277970wrc.249.1504899086541; \n\tFri, 08 Sep 2017 12:31:26 -0700 (PDT)","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","To":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>,\n\tnetdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tAndrew Lunn <andrew@lunn.ch>","Cc":"Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","References":"<1504875731-3680-1-git-send-email-hayashi.kunihiko@socionext.com>\n\t<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","From":"Florian Fainelli <f.fainelli@gmail.com>","Message-ID":"<df4ec8b2-67cf-5dee-2600-88cffd1bcd11@gmail.com>","Date":"Fri, 8 Sep 2017 12:31:18 -0700","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765822,"web_url":"http://patchwork.ozlabs.org/comment/1765822/","msgid":"<46b1bd9e-35ff-dc7f-fa32-f8d22b37a403@gmail.com>","list_archive_url":null,"date":"2017-09-09T16:30:58","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":2800,"url":"http://patchwork.ozlabs.org/api/people/2800/","name":"Florian Fainelli","email":"f.fainelli@gmail.com"},"content":"On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:\n> The UniPhier platform from Socionext provides the AVE ethernet\n> controller that includes MAC and MDIO bus supporting RGMII/RMII\n> modes. The controller is named AVE.\n> \n> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>\n> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>\n> ---\n\n[snip]\n\n> +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 proc_idx, done_idx, ndesc, cmdsts;\n> +\tint freepkt;\n> +\tunsigned char *buffptr = NULL; /* buffptr for descriptor */\n> +\tunsigned int len;\n> +\tdma_addr_t paddr;\n> +\n> +\tproc_idx = priv->tx.proc_idx;\n> +\tdone_idx = priv->tx.done_idx;\n> +\tndesc = priv->tx.ndesc;\n> +\tfreepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;\n> +\n> +\t/* not enough entry, then we stop queue */\n> +\tif (unlikely(freepkt < 2)) {\n> +\t\tnetif_stop_queue(ndev);\n> +\t\tif (unlikely(freepkt < 1))\n> +\t\t\treturn NETDEV_TX_BUSY;\n\nThis looks wrong, why are you checking first for less than 2\ndescriptors, and if there is none, NETDEV_TX_BUSY? If you need 2 slots\nto complete a transmision, stop the transmit queue and return\nNETDEV_TX_BUSY.\n\n> +\t}\n> +\n> +\tpriv->tx.desc[proc_idx].skbs = skb;\n> +\n> +\t/* add padding for short packet */\n> +\tif (skb_padto(skb, ETH_ZLEN)) {\n> +\t\tdev_kfree_skb_any(skb);\n> +\t\treturn NETDEV_TX_OK;\n> +\t}\n\nskb_padto() frees the SKB in case of error, that would lead to a double\nfree here.\n\n> +\n> +\tbuffptr = skb->data - NET_IP_ALIGN;\n> +\tlen = max_t(unsigned int, ETH_ZLEN, skb->len);\n\nIf you use skb_put_padto() if padding was necessary skb->len will be at\nleast ETH_ZLEN, so you can remove this.\n\n> +\n> +\tpaddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,\n> +\t\t\t    len + NET_IP_ALIGN, DMA_TO_DEVICE);\n\nAs mentioned before you can't assume this will never fail.\n\n> +\tpaddr += NET_IP_ALIGN;\n> +\n> +\t/* set buffer address to descriptor */\n> +\tave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);\n\nAlso mentioned in the other email, make this 4 a constant so we know\nit's an offset and not a length.\n\n> +\n> +\t/* set flag and length to send */\n> +\tcmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST\n> +\t\t| (len & AVE_STS_PKTLEN_TX);\n\nAVE_STS_PKTLEN_TX would be better named with a _MASK suffix.\n\n> +\n> +\t/* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */\n> +\tif (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))\n> +\t\tcmdsts |= AVE_STS_INTR;\n> +\n> +\t/* disable checksum calculation when skb doesn't calurate checksum */\n> +\tif (skb->ip_summed == CHECKSUM_NONE ||\n> +\t    skb->ip_summed == CHECKSUM_UNNECESSARY)\n> +\t\tcmdsts |= AVE_STS_NOCSUM;\n> +\n> +\t/* set cmdsts */\n> +\tave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);\n> +\n> +\tpriv->tx.proc_idx = (proc_idx + 1) % ndesc;\n\nYou should also check the ring space after transmission and assert flow\ncontrol on the transmit queue if needed.\n\n> +\n> +\treturn NETDEV_TX_OK;\n> +}\n\n[snip]\n\n> +static struct net_device_stats *ave_stats(struct net_device *ndev)\n> +{\n> +\tstruct ave_private *priv = netdev_priv(ndev);\n> +\tu32 drop_num = 0;\n> +\n> +\tpriv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);\n> +\n> +\tdrop_num += ave_r32(ndev, AVE_RX0OVFFC);\n> +\tdrop_num += ave_r32(ndev, AVE_SN5FC);\n> +\tdrop_num += ave_r32(ndev, AVE_SN6FC);\n> +\tdrop_num += ave_r32(ndev, AVE_SN7FC);\n> +\tpriv->stats.rx_dropped = drop_num;\n> +\n\nYou should consider switching to 64-bit statistics, this requires a\nlittle bit more work for 32-bit hosts (see\ninclude/linux/u64_stats_sync.h) but this allows you to keep statistics\naround above 4GB.\n\n> +\treturn &priv->stats;\n> +}\n> +-- \nFlorian","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"sdC/CZIq\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xqKSh61Bjz9s7G\n\tfor <patchwork-incoming@ozlabs.org>;\n\tSun, 10 Sep 2017 02:31:16 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1757623AbdIIQbF (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tSat, 9 Sep 2017 12:31:05 -0400","from mail-oi0-f66.google.com ([209.85.218.66]:33793 \"EHLO\n\tmail-oi0-f66.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1757462AbdIIQbD (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Sat, 9 Sep 2017 12:31:03 -0400","by mail-oi0-f66.google.com with SMTP id a197so3688464oib.1;\n\tSat, 09 Sep 2017 09:31:02 -0700 (PDT)","from ?IPv6:2001:470:d:73f:f932:45ba:5438:993c?\n\t([2001:470:d:73f:f932:45ba:5438:993c])\n\tby smtp.gmail.com with ESMTPSA id\n\t188sm4458388oid.5.2017.09.09.09.30.59\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tSat, 09 Sep 2017 09:31:01 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=48+Orz67QCdFO9yms5cWSfVDvnUYc+IaceUqkZBawpI=;\n\tb=sdC/CZIqNFcG+jX4SscnCorSoALqKkyfNO4LkT+5yLL+ZhdziKoWqTjqOuzW0sP9AZ\n\tEAMbSTIsJ5P9svVg7a+5D6wiPxF3SJF1q+/xdMej7ChnLgOCFoAs4Hr7UGequ8p9v17q\n\txhwJpe0VHT6VpHAoX8NlwU97jl6sGeMRyXANYQl/qVPpPL42I8lIsMq4rgo8KVlrLqfq\n\tDg2o8HC84Ce6cCJ2+eL9eihtrcnFPggwHbqD8TsleER2ybEGeIvkYv4FJHMgeHfehi4B\n\tcytU4vHPus1nrf9Apb7u6DNFLcLW+LRhoOpRcVI3BXoJHxhNDLRGeQ+QExU6ls9cXSRB\n\toX8A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=48+Orz67QCdFO9yms5cWSfVDvnUYc+IaceUqkZBawpI=;\n\tb=QDEMw2h5HEldu7n3bwetrwA52z9JR8umpKCIdcNzmyE4WXWmIrZsDxkV1zA6sPSZKe\n\tRnGQjdmdS8Ex+XWniwnmoxY1TFXlcajigiTiawlPhLolXVDYcHH+S+CBe6jkQoWVQzQc\n\tx8TNUijqM/IWetZ585eBoBtX3OQeO8efV0pcyfLKXMjzb4QYMIv3srLWBg/Pgv2zIlw6\n\t8VeY/pKsJDjS55NpppOYT3rvyNdbGe/mU+yTf+4I4OXQ8tL1Q7kPb968NhsH59Q3PfZT\n\teA229+aUWDTQgSMui42EJagVC3eQKtQZvcpxBNciwhEfT3bwmFfX6sHlwq2KKAYHnvcM\n\tLywQ==","X-Gm-Message-State":"AHPjjUj/k1JdxeCBwTBYSegxwC0JBFheInsee/PgG2dZIErRx1qEXHcU\n\tSBymzZBmFUd6nw==","X-Google-Smtp-Source":"AOwi7QCqXxPEyUHHXwrLvVv5U5jFZUYiZ4uBPJt8N3FRYKS2DeRL5a2CjDH+adhTzQWiHWU67gYTdw==","X-Received":"by 10.202.206.1 with SMTP id e1mr5868384oig.150.1504974662297;\n\tSat, 09 Sep 2017 09:31:02 -0700 (PDT)","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","To":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>,\n\tnetdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tAndrew Lunn <andrew@lunn.ch>","Cc":"Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","References":"<1504875731-3680-1-git-send-email-hayashi.kunihiko@socionext.com>\n\t<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","From":"Florian Fainelli <f.fainelli@gmail.com>","Message-ID":"<46b1bd9e-35ff-dc7f-fa32-f8d22b37a403@gmail.com>","Date":"Sat, 9 Sep 2017 09:30:58 -0700","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>","Content-Type":"text/plain; charset=windows-1252","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766074,"web_url":"http://patchwork.ozlabs.org/comment/1766074/","msgid":"<20170911155047.6717.4A936039@socionext.com>","list_archive_url":null,"date":"2017-09-11T06:50:47","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":71700,"url":"http://patchwork.ozlabs.org/api/people/71700/","name":"Kunihiko Hayashi","email":"hayashi.kunihiko@socionext.com"},"content":"Hi Andrew,\nThank you for your comments.\n\nOn Fri, 8 Sep 2017 15:50:30 +0200 <andrew@lunn.ch> wrote:\n\n> > +static int ave_mdio_busywait(struct net_device *ndev)\n> > +{\n> > +\tint ret = 1, loop = 100;\n> > +\tu32 mdiosr;\n> > +\n> > +\t/* wait until completion */\n> > +\twhile (1) {\n> > +\t\tmdiosr = ave_r32(ndev, AVE_MDIOSR);\n> > +\t\tif (!(mdiosr & AVE_MDIOSR_STS))\n> > +\t\t\tbreak;\n> > +\n> > +\t\tusleep_range(10, 20);\n> > +\t\tif (!loop--) {\n> > +\t\t\tnetdev_err(ndev,\n> > +\t\t\t\t   \"failed to read from MDIO (status:0x%08x)\\n\",\n> > +\t\t\t\t   mdiosr);\n> > +\t\t\tret = 0;\n> \n> ETIMEDOUT would be better.\n\nI see. I'll use this.\n\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn ret;\n> \n> and then return 0 on success. That is the normal convention for return\n> values. An error code, and 0.\n\nSurely, it's desiable to return zero and error code.\n\n> > +static int ave_mdiobus_write(struct mii_bus *bus,\n> > +\t\t\t     int phyid, int regnum, u16 val)\n> > +{\n> > +\tstruct net_device *ndev = bus->priv;\n> > +\tu32 mdioctl;\n> > +\n> > +\t/* write address */\n> > +\tave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);\n> > +\n> > +\t/* write data */\n> > +\tave_w32(ndev, AVE_MDIOWDR, val);\n> > +\n> > +\t/* write request */\n> > +\tmdioctl = ave_r32(ndev, AVE_MDIOCTR);\n> > +\tave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);\n> > +\n> > +\tif (!ave_mdio_busywait(ndev)) {\n> > +\t\tnetdev_err(ndev, \"phy-%d reg-%x write failed\\n\",\n> > +\t\t\t   phyid, regnum);\n> > +\t\treturn -1;\n> \n> If ave_mdio_busywait() returns ETIMEDOUT, you can just return\n> it. Returning -1 is not good.\n\nIndeed. I'll re-consider handling return value.\n\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static irqreturn_t ave_interrupt(int irq, void *netdev)\n> > +{\n> > +\tstruct net_device *ndev = (struct net_device *)netdev;\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 gimr_val, gisr_val;\n> > +\n> > +\tgimr_val = ave_irq_disable_all(ndev);\n> > +\n> > +\t/* get interrupt status */\n> > +\tgisr_val = ave_r32(ndev, AVE_GISR);\n> > +\n> > +\t/* PHY */\n> > +\tif (gisr_val & AVE_GI_PHY) {\n> > +\t\tave_w32(ndev, AVE_GISR, AVE_GI_PHY);\n> > +\t\tif (priv->internal_phy_interrupt)\n> > +\t\t\tphy_mac_interrupt(ndev->phydev, ndev->phydev->link);\n> \n> Humm. I don't think this is correct. You are supposed to give it the\n> new link state, not the old.\n> \n> What does a PHY interrupt mean here? \n\nIn the general case, I think PHY events like changing link state are transmitted\nto CPU as interrupt via interrupt controller, then PHY driver itself can handle\nthe interrupt.\n\nAnd in this case, PHY events are transmitted to MAC as one of its interrupt factor,\nthen I thought that MAC driver had to tell the events to PHY.\n\n> > +static void ave_adjust_link(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tstruct phy_device *phydev = ndev->phydev;\n> > +\tu32 val, txcr, rxcr, rxcr_org;\n> > +\n> > +\t/* set RGMII speed */\n> > +\tval = ave_r32(ndev, AVE_TXCR);\n> > +\tval &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);\n> > +\n> > +\tif (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&\n> > +\t    phydev->speed == SPEED_1000)\n> \n> phy_interface_mode_is_rgmii(), so that you handle all the RGMII modes.\n\nIt's convenient. I'll replace it.\n\n> > +\t\tval |= AVE_TXCR_TXSPD_1G;\n> > +\telse if (phydev->speed == SPEED_100)\n> > +\t\tval |= AVE_TXCR_TXSPD_100;\n> > +\n> > +\tave_w32(ndev, AVE_TXCR, val);\n> > +\n> > +\t/* set RMII speed (100M/10M only) */\n> > +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n> \n> Not so safe. It would be better to check for the modes you actually\n> support.\n\nIndeed. This part has to check if the specified mode is supported accurately.\n\n> > +\tif (phydev->link)\n> > +\t\tnetif_carrier_on(ndev);\n> > +\telse\n> > +\t\tnetif_carrier_off(ndev);\n> \n> I don't think you need this. The phylib should do it for you.\n\nOkay. I'll remove it.\n\n> > +\n> > +\tphy_print_status(phydev);\n> > +}\n> > +\n> > +static int ave_init(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tstruct device *dev = ndev->dev.parent;\n> > +\tstruct device_node *phy_node, *np = dev->of_node;\n> > +\tstruct phy_device *phydev;\n> > +\tconst void *mac_addr;\n> > +\tu32 supported;\n> > +\n> > +\t/* get mac address */\n> > +\tmac_addr = of_get_mac_address(np);\n> > +\tif (mac_addr)\n> > +\t\tether_addr_copy(ndev->dev_addr, mac_addr);\n> > +\n> > +\t/* if the mac address is invalid, use random mac address */\n> > +\tif (!is_valid_ether_addr(ndev->dev_addr)) {\n> > +\t\teth_hw_addr_random(ndev);\n> > +\t\tdev_warn(dev, \"Using random MAC address: %pM\\n\",\n> > +\t\t\t ndev->dev_addr);\n> > +\t}\n> > +\n> > +\t/* attach PHY with MAC */\n> > +\tphy_node =  of_get_next_available_child(np, NULL);\n> \n> ???\n> \n> Should this not be looking for a phy-handle property?\n> Documentation/devicetree/binds/net/ethernet.txt:\n> \n> - phy-handle: phandle, specifies a reference to a node representing a PHY\n>   device; this property is described in the Devicetree Specification and so\n>   preferred;\n\nYes, I found it was wrong.\nThe device node has not a child node but phy-handle to specify PHY.\n\n> > +\tphydev = of_phy_connect(ndev, phy_node,\n> > +\t\t\t\tave_adjust_link, 0, priv->phy_mode);\n> > +\tif (!phydev) {\n> > +\t\tdev_err(dev, \"could not attach to PHY\\n\");\n> > +\t\treturn -ENODEV;\n> > +\t}\n> > +\tof_node_put(phy_node);\n> > +\n> > +\tpriv->phydev = phydev;\n> > +\tphydev->autoneg = AUTONEG_ENABLE;\n> > +\tphydev->speed = 0;\n> > +\tphydev->duplex = 0;\n> \n> And this should not be needed.\n\nI understand that these parameters have been initialized.\n\n> > +\n> > +\tdev_info(dev, \"connected to %s phy with id 0x%x\\n\",\n> > +\t\t phydev->drv->name, phydev->phy_id);\n> \n> phy_attached_info()\n\nIt's convernient.\n\n> > +\n> > +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n> \n> Same comment as above.\n\nDitto.\n\n> > +\t\tsupported = phydev->supported;\n> > +\t\tphydev->supported &= ~PHY_GBIT_FEATURES;\n> > +\t\tphydev->supported |= supported & PHY_BASIC_FEATURES;\n> > +\t}\n> > +\n> > +\t/* PHY interrupt stop instruction is needed because the interrupt\n> > +\t * continues to assert.\n> > +\t */\n> > +\tphy_stop_interrupts(phydev);\n> \n> Could you explain this some more? It sounds like your interrupt\n> controller is broken.\n\nI thought that the driver had to stop the own interrupt of PHY device\nbecause it might be already enabled.\nBut surely we don't think of such a case.\n\n> > +\n> > +\t/* When PHY driver can't handle its interrupt directly,\n> > +\t * interrupt request always fails and polling method is used\n> > +\t * alternatively. In this case, the libphy says\n> > +\t * \"libphy: uniphier-mdio: Can't get IRQ -1 (PHY)\".\n> > +\t */\n> > +\tphy_start_interrupts(phydev);\n> \n> -1 is PHY_POLL. So calling phy_start_interrupts() is wrong. In fact,\n> you should not be calling phy_start_interrupts() at all. No other\n> Ethernet driver does.\n\nThe phy_start_interrupts() calls request_threaded_irq(), and it wll succeed\nwhen PHY node has own 'interrupts' property.\nIn this case, it will fail and phydev->irq sets PHY_POLL.\n\nHowever, phydev->irq is initialized PHY_POLL in phy_probe(),\nsurely it isn't necessary.\n\n> > +\n> > +\tphy_start_aneg(phydev);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static void ave_uninit(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\n> > +\tphy_stop_interrupts(priv->phydev);\n> \n> And no other Ethernet driver calls phy_stop_interrupts either.\n> Please take a look at this.\n\nI see. I'll check it.\n\n> \n> > +\tphy_disconnect(priv->phydev);\n> > +}\n> > +\n> \n>   Andrew\n\n---\nBest Regards,\nKunihiko Hayashi","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrJVH0sQlz9rxj\n\tfor <patchwork-incoming@ozlabs.org>;\n\tMon, 11 Sep 2017 16:51:03 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751090AbdIKGuw (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tMon, 11 Sep 2017 02:50:52 -0400","from mx.socionext.com ([202.248.49.38]:12806 \"EHLO\n\tmx.socionext.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750939AbdIKGuu (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tMon, 11 Sep 2017 02:50:50 -0400","from unknown (HELO iyokan-ex.css.socionext.com) ([172.31.9.54])\n\tby mx.socionext.com with ESMTP; 11 Sep 2017 15:50:48 +0900","from mail.mfilter.local (unknown [10.213.24.61])\n\tby iyokan-ex.css.socionext.com (Postfix) with ESMTP id DFA50610C6;\n\tMon, 11 Sep 2017 15:50:48 +0900 (JST)","from 172.31.9.53 (172.31.9.53) by m-FILTER with ESMTP;\n\tMon, 11 Sep 2017 15:50:48 +0900","from yuzu.css.socionext.com (yuzu [172.31.8.45])\n\tby iyokan.css.socionext.com (Postfix) with ESMTP id 8B05C40512;\n\tMon, 11 Sep 2017 15:50:48 +0900 (JST)","from [127.0.0.1] (unknown [10.213.134.37])\n\tby yuzu.css.socionext.com (Postfix) with ESMTP id 48748120453;\n\tMon, 11 Sep 2017 15:50:48 +0900 (JST)"],"Date":"Mon, 11 Sep 2017 15:50:47 +0900","From":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","To":"Andrew Lunn <andrew@lunn.ch>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Cc":"<netdev@vger.kernel.org>, \"David S. Miller\" <davem@davemloft.net>,\n\tFlorian Fainelli <f.fainelli@gmail.com>,\n\tRob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\t<linux-arm-kernel@lists.infradead.org>,\n\t<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","In-Reply-To":"<20170908135030.GA25219@lunn.ch>","References":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>\n\t<20170908135030.GA25219@lunn.ch>","Message-Id":"<20170911155047.6717.4A936039@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"US-ASCII\"","Content-Transfer-Encoding":"7bit","X-Mailer":"Becky! ver. 2.70 [ja]","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766076,"web_url":"http://patchwork.ozlabs.org/comment/1766076/","msgid":"<20170911155144.6718.4A936039@socionext.com>","list_archive_url":null,"date":"2017-09-11T06:51:44","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":71700,"url":"http://patchwork.ozlabs.org/api/people/71700/","name":"Kunihiko Hayashi","email":"hayashi.kunihiko@socionext.com"},"content":"Hi Yamada-san,\nThank you for your comments,\n\nOn Fri, 8 Sep 2017 23:44:13 +0900 <yamada.masahiro@socionext.com> wrote:\n\n> 2017-09-08 22:02 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@socionext.com>:\n> \n> > diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig\n> > new file mode 100644\n> > index 0000000..788f26f\n> > --- /dev/null\n> > +++ b/drivers/net/ethernet/socionext/Kconfig\n> > @@ -0,0 +1,22 @@\n> > +config NET_VENDOR_SOCIONEXT\n> > +       bool \"Socionext ethernet drivers\"\n> > +       default y\n> > +       ---help---\n> > +         Option to select ethernet drivers for Socionext platforms.\n> > +\n> > +         Note that the answer to this question doesn't directly affect the\n> > +         kernel: saying N will just cause the configurator to skip all\n> > +         the questions about Agere devices. If you say Y, you will be asked\n> > +         for your specific card in the following questions.\n> \n> \n> Agere?\n\nIt's wrong. I have template strings left.\nI'll fix it.\n\n> > +\n> > +       dev_info(dev, \"Socionext %c%c%c%c Ethernet IP %s (irq=%d, phy=%s)\\n\",\n> > +                (ave_id >> 24) & 0xff, (ave_id >> 16) & 0xff,\n> > +                (ave_id >> 8) & 0xff, (ave_id >> 0) & 0xff,\n> > +                buf, ndev->irq, phy_modes(phy_mode));\n> > +\n> > +       return 0;\n> > +err_netdev_register:\n> \n> Maybe, a bad label name.\n> for ex. out_del_napi or whatever.\n> \n> Documentation/process/coding-style.rst says\n> \"Choose label names which say what the goto does ...\"\n\nYes, I'll rename it according to the document.\n\n> > +       netif_napi_del(&priv->napi_rx);\n> > +       netif_napi_del(&priv->napi_tx);\n> > +       mdiobus_unregister(priv->mdio);\n> > +err_mdiobus_register:\n> > +err_mdiobus_alloc:\n> > +err_req_irq:\n> \n> These three should be merged, for ex.\n> out_free_device.\n\nI see. These will be merged.\n\n> I ran sparse for you.\n> \n> Please take a look if it is worthwhile.\n> Seems endianess around mac_addr.\n\nOkay, I'll check the suspicious code.\n\n> drivers/net/ethernet/socionext/sni_ave.c:423:15: warning: cast to\n> restricted __le32\n> drivers/net/ethernet/socionext/sni_ave.c:425:20: warning: cast to\n> restricted __le16\n> drivers/net/ethernet/socionext/sni_ave.c:1194:15: warning: cast to\n> restricted __le32\n> drivers/net/ethernet/socionext/sni_ave.c:1196:20: warning: cast to\n> restricted __le16\n> drivers/net/ethernet/socionext/sni_ave.c:1398:15: warning: cast to\n> restricted __le32\n> drivers/net/ethernet/socionext/sni_ave.c:1400:20: warning: cast to\n> restricted __le16\n> \n> \n> \n> \n> \n> -- \n> Best Regards\n> Masahiro Yamada\n\n---\nBest Regards,\nKunihiko Hayashi","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrJWL2gfDz9rxj\n\tfor <patchwork-incoming@ozlabs.org>;\n\tMon, 11 Sep 2017 16:51:58 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751244AbdIKGvs (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tMon, 11 Sep 2017 02:51:48 -0400","from mx.socionext.com ([202.248.49.38]:12833 \"EHLO\n\tmx.socionext.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751041AbdIKGvr (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tMon, 11 Sep 2017 02:51:47 -0400","from unknown (HELO iyokan-ex.css.socionext.com) ([172.31.9.54])\n\tby mx.socionext.com with ESMTP; 11 Sep 2017 15:51:45 +0900","from mail.mfilter.local (unknown [10.213.24.61])\n\tby iyokan-ex.css.socionext.com (Postfix) with ESMTP id B6ECA610C6;\n\tMon, 11 Sep 2017 15:51:45 +0900 (JST)","from 172.31.9.51 (172.31.9.51) by m-FILTER with ESMTP;\n\tMon, 11 Sep 2017 15:51:45 +0900","from yuzu.css.socionext.com (yuzu [172.31.8.45])\n\tby kinkan.css.socionext.com (Postfix) with ESMTP id 188861A0E16;\n\tMon, 11 Sep 2017 15:51:45 +0900 (JST)","from [127.0.0.1] (unknown [10.213.134.37])\n\tby yuzu.css.socionext.com (Postfix) with ESMTP id CD1D6120453;\n\tMon, 11 Sep 2017 15:51:44 +0900 (JST)"],"Date":"Mon, 11 Sep 2017 15:51:44 +0900","From":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","To":"Masahiro Yamada <yamada.masahiro@socionext.com>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Cc":"netdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tAndrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>,\n\tRob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel <linux-arm-kernel@lists.infradead.org>,\n\tLinux Kernel Mailing List <linux-kernel@vger.kernel.org>,\n\tdevicetree@vger.kernel.org,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","In-Reply-To":"<CAK7LNASqfRbG08gvLxx4WxY2Vs2YYPkZ6NUpjj3_OOvOdEBKMA@mail.gmail.com>","References":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>\n\t<CAK7LNASqfRbG08gvLxx4WxY2Vs2YYPkZ6NUpjj3_OOvOdEBKMA@mail.gmail.com>","Message-Id":"<20170911155144.6718.4A936039@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"US-ASCII\"","Content-Transfer-Encoding":"7bit","X-Mailer":"Becky! ver. 2.70 [ja]","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766080,"web_url":"http://patchwork.ozlabs.org/comment/1766080/","msgid":"<20170911155555.6719.4A936039@socionext.com>","list_archive_url":null,"date":"2017-09-11T06:55:56","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":71700,"url":"http://patchwork.ozlabs.org/api/people/71700/","name":"Kunihiko Hayashi","email":"hayashi.kunihiko@socionext.com"},"content":"Hi Florian,\nThank you for your comments,\n\nOn Fri, 8 Sep 2017 12:31:18 -0700 <f.fainelli@gmail.com> wrote:\n\n> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:\n> > The UniPhier platform from Socionext provides the AVE ethernet\n> > controller that includes MAC and MDIO bus supporting RGMII/RMII\n> > modes. The controller is named AVE.\n> > \n> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>\n> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>\n> > ---\n\n..snip..\n\n> > +static inline u32 ave_r32(struct net_device *ndev, u32 offset)\n> > +{\n> > +\tvoid __iomem *addr = (void __iomem *)ndev->base_addr;\n> > +\n> > +\treturn readl_relaxed(addr + offset);\n> > +}\n> \n> Don't do this, ndev->base_addr is convenient, but is now deprecated\n> (unlike ISA cards, you can't change this anymore) instead, just pass a\n> reference to an ave_private structure and store the base register\n> pointer there.\n\nOh, I didn't notice that ndev->base_addr was deprecated.\nI'll rewrite it using an ave_private structure.\n\n> > +\n> > +static inline void ave_w32(struct net_device *ndev, u32 offset, u32 value)\n> > +{\n> > +\tvoid __iomem *addr = (void __iomem *)ndev->base_addr;\n> > +\n> > +\twritel_relaxed(value, addr + offset);\n> > +}\n> \n> Same here.\n\nDitto.\n\n> > +\n> > +static inline u32 ave_rdesc(struct net_device *ndev, enum desc_id id,\n> > +\t\t\t    int entry, int offset)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 addr = (id == AVE_DESCID_TX) ? priv->tx.daddr : priv->rx.daddr;\n> > +\n> > +\taddr += entry * priv->desc_size + offset;\n> > +\n> > +\treturn ave_r32(ndev, addr);\n> > +}\n> > +\n> > +static inline void ave_wdesc(struct net_device *ndev, enum desc_id id,\n> > +\t\t\t     int entry, int offset, u32 val)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 addr = (id == AVE_DESCID_TX) ? priv->tx.daddr : priv->rx.daddr;\n> > +\n> > +\taddr += entry * priv->desc_size + offset;\n> > +\n> > +\tave_w32(ndev, addr, val);\n> > +}\n> > +\n> > +static void ave_wdesc_addr(struct net_device *ndev, enum desc_id id,\n> > +\t\t\t   int entry, int offset, dma_addr_t paddr)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\n> > +\tave_wdesc(ndev, id, entry, offset, (u32)((u64)paddr & 0xFFFFFFFFULL));\n> \n> lower_32_bits()\n\nIt's convenient.\n\n> > +\tif (IS_DESC_64BIT(priv))\n> > +\t\tave_wdesc(ndev, id,\n> > +\t\t\t  entry, offset + 4, (u32)((u64)paddr >> 32));\n> \n> upper_32_bits()\n\nDitto.\n\n> > +\telse if ((u64)paddr > (u64)U32_MAX)\n> > +\t\tnetdev_warn(ndev, \"DMA address exceeds the address space\\n\");\n> > +}\n> > +\n> > +static void ave_get_fwversion(struct net_device *ndev, char *buf, int len)\n> > +{\n> > +\tu32 major, minor;\n> > +\n> > +\tmajor = (ave_r32(ndev, AVE_VR) & GENMASK(15, 8)) >> 8;\n> > +\tminor = (ave_r32(ndev, AVE_VR) & GENMASK(7, 0));\n> > +\tsnprintf(buf, len, \"v%u.%u\", major, minor);\n> > +}\n> > +\n> > +static void ave_get_drvinfo(struct net_device *ndev,\n> > +\t\t\t    struct ethtool_drvinfo *info)\n> > +{\n> > +\tstruct device *dev = ndev->dev.parent;\n> > +\n> > +\tstrlcpy(info->driver, dev->driver->name, sizeof(info->driver));\n> > +\tstrlcpy(info->bus_info, dev_name(dev), sizeof(info->bus_info));\n> \n> bus_info would likely be platform, since this is a memory mapped\n> peripheral, right?\n\nYes, this is a memory mapped peripheral.\nNow ethtool says \"bus-info: 65000000.ethernet\" in our case.\nIs it reasonable for bus-info? or is null desirable?\n\n> > +\tave_get_fwversion(ndev, info->fw_version, sizeof(info->fw_version));\n> > +}\n> > +\n> > +static int ave_nway_reset(struct net_device *ndev)\n> > +{\n> > +\treturn genphy_restart_aneg(ndev->phydev);\n> > +}\n> \n> You can just set your ethtool_ops::nway_reset to be\n> phy_ethtool_nway_reset() which does additional checks.\n\nI see. I'll use it.\n\n> > +\n> > +static u32 ave_get_link(struct net_device *ndev)\n> > +{\n> > +\tint err;\n> > +\n> > +\terr = genphy_update_link(ndev->phydev);\n> > +\tif (err)\n> > +\t\treturn ethtool_op_get_link(ndev);\n> \n> No, calling genphy_update_link() is bogus see:\n> \n> e69e46261063a25c3907bed16a2e9d18b115d1fd (\"net: dsa: Do not clobber PHY\n> link outside of state machine\")\n\nYou mean that phylib can't guarantee link-state when the driver calls\ngenphy_update_link() here, that is outside of phy_state_machine().\n\n> > +\n> > +\treturn ndev->phydev->link;\n> \n> This can just be ethtool_op_get_link()\n\nOkay, I'll replace it.\n\n> > +}\n> > +\n> > +static u32 ave_get_msglevel(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\n> > +\treturn priv->msg_enable;\n> > +}\n> > +\n> > +static void ave_set_msglevel(struct net_device *ndev, u32 val)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\n> > +\tpriv->msg_enable = val;\n> > +}\n> > +\n> > +static void ave_get_wol(struct net_device *ndev,\n> > +\t\t\tstruct ethtool_wolinfo *wol)\n> > +{\n> > +\twol->supported = 0;\n> > +\twol->wolopts   = 0;\n> > +\tphy_ethtool_get_wol(ndev->phydev, wol);\n> \n> This can be called before you successfully connected to a PHY so you\n> need to check that ndev->phydev is not NULL at the very least.\n\nI see. I'll add check code for ndev->phydev.\n\n> > +}\n> > +\n> > +static int ave_set_wol(struct net_device *ndev,\n> > +\t\t       struct ethtool_wolinfo *wol)\n> > +{\n> > +\tif (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE))\n> > +\t\treturn -EOPNOTSUPP;\n> > +\n> > +\treturn phy_ethtool_set_wol(ndev->phydev, wol);\n> \n> Same here.\n\nDitto.\n\n> > +\n> > +static int ave_mdio_busywait(struct net_device *ndev)\n> > +{\n> > +\tint ret = 1, loop = 100;\n> > +\tu32 mdiosr;\n> > +\n> > +\t/* wait until completion */\n> > +\twhile (1) {\n> \n> Since you are already bound by the number of times you allow this look,\n> just make that a clear condition for the while() loop to exit.\n\nIndeed. I can replace while condition.\n\n> > +\t\tmdiosr = ave_r32(ndev, AVE_MDIOSR);\n> > +\t\tif (!(mdiosr & AVE_MDIOSR_STS))\n> > +\t\t\tbreak;\n> > +\n> > +\t\tusleep_range(10, 20);\n> > +\t\tif (!loop--) {\n> > +\t\t\tnetdev_err(ndev,\n> > +\t\t\t\t   \"failed to read from MDIO (status:0x%08x)\\n\",\n> > +\t\t\t\t   mdiosr);\n> > +\t\t\tret = 0;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +static int ave_mdiobus_read(struct mii_bus *bus, int phyid, int regnum)\n> > +{\n> > +\tstruct net_device *ndev = bus->priv;\n> > +\tu32 mdioctl;\n> > +\n> > +\t/* write address */\n> > +\tave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);\n> > +\n> > +\t/* read request */\n> > +\tmdioctl = ave_r32(ndev, AVE_MDIOCTR);\n> > +\tave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_RREQ);\n> > +\n> > +\tif (!ave_mdio_busywait(ndev)) {\n> > +\t\tnetdev_err(ndev, \"phy-%d reg-%x read failed\\n\",\n> > +\t\t\t   phyid, regnum);\n> > +\t\treturn 0xffff;\n> > +\t}\n> > +\n> > +\treturn ave_r32(ndev, AVE_MDIORDR) & GENMASK(15, 0);\n> > +}\n> > +\n> > +static int ave_mdiobus_write(struct mii_bus *bus,\n> > +\t\t\t     int phyid, int regnum, u16 val)\n> > +{\n> > +\tstruct net_device *ndev = bus->priv;\n> > +\tu32 mdioctl;\n> > +\n> > +\t/* write address */\n> > +\tave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum);\n> > +\n> > +\t/* write data */\n> > +\tave_w32(ndev, AVE_MDIOWDR, val);\n> > +\n> > +\t/* write request */\n> > +\tmdioctl = ave_r32(ndev, AVE_MDIOCTR);\n> > +\tave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ);\n> > +\n> > +\tif (!ave_mdio_busywait(ndev)) {\n> > +\t\tnetdev_err(ndev, \"phy-%d reg-%x write failed\\n\",\n> > +\t\t\t   phyid, regnum);\n> > +\t\treturn -1;\n> > +\t}\n> \n> Just propagate the return value of ave_mdio_busywait().\n\nYes, I'll reconsider the way to handle return value.\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static dma_addr_t ave_dma_map(struct net_device *ndev, struct ave_desc *desc,\n> > +\t\t\t      void *ptr, size_t len,\n> > +\t\t\t      enum dma_data_direction dir)\n> > +{\n> > +\tdma_addr_t paddr;\n> > +\n> > +\tpaddr = dma_map_single(ndev->dev.parent, ptr, len, dir);\n> > +\tif (dma_mapping_error(ndev->dev.parent, paddr)) {\n> > +\t\tdesc->skbs_dma = 0;\n> > +\t\tpaddr = (dma_addr_t)virt_to_phys(ptr);\n> \n> Why do you do that? If dma_map_single() failed, that's it, game over,\n> you need to discad the packet, not assume that it is in the kernel's\n> linear mapping!\n\nI see. It's not necessary to worry about failing dma_map_single().\nI'll rewrite it to discard the packet.\n\n> > +\t} else {\n> > +\t\tdesc->skbs_dma = paddr;\n> > +\t\tdesc->skbs_dmalen = len;\n> > +\t}\n> > +\n> > +\treturn paddr;\n> > +}\n> > +\n> > +static void ave_dma_unmap(struct net_device *ndev, struct ave_desc *desc,\n> > +\t\t\t  enum dma_data_direction dir)\n> > +{\n> > +\tif (!desc->skbs_dma)\n> > +\t\treturn;\n> > +\n> > +\tdma_unmap_single(ndev->dev.parent,\n> > +\t\t\t desc->skbs_dma, desc->skbs_dmalen, dir);\n> > +\tdesc->skbs_dma = 0;\n> > +}\n> > +\n> > +/* Set Rx descriptor and memory */\n> > +static int ave_set_rxdesc(struct net_device *ndev, int entry)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tstruct sk_buff *skb;\n> > +\tunsigned long align;\n> > +\tdma_addr_t paddr;\n> > +\tvoid *buffptr;\n> > +\tint ret = 0;\n> > +\n> > +\tskb = priv->rx.desc[entry].skbs;\n> > +\tif (!skb) {\n> > +\t\tskb = netdev_alloc_skb_ip_align(ndev,\n> > +\t\t\t\t\t\tAVE_MAX_ETHFRAME + NET_SKB_PAD);\n> > +\t\tif (!skb) {\n> > +\t\t\tnetdev_err(ndev, \"can't allocate skb for Rx\\n\");\n> > +\t\t\treturn -ENOMEM;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* set disable to cmdsts */\n> > +\tave_wdesc(ndev, AVE_DESCID_RX, entry, 0, AVE_STS_INTR | AVE_STS_OWN);\n> > +\n> > +\t/* align skb data for cache size */\n> > +\talign = (unsigned long)skb_tail_pointer(skb) & (NET_SKB_PAD - 1);\n> > +\talign = NET_SKB_PAD - align;\n> > +\tskb_reserve(skb, align);\n> > +\tbuffptr = (void *)skb_tail_pointer(skb);\n> \n> Are you positive you need this? Because by default, the networking stack\n> will align to the maximum between your L1 cache line size and 64 bytes,\n> which should be a pretty good alignment guarantee.\n\nNow if L1 cache line size is 128,\nthe skb buffer is also aligned to 128, isn't it?\nSo this code doesn't make sense.\n\n> > +\n> > +\tpaddr = ave_dma_map(ndev, &priv->rx.desc[entry], buffptr,\n> > +\t\t\t    AVE_MAX_ETHFRAME + NET_IP_ALIGN, DMA_FROM_DEVICE);\n> \n> This needs to be checked, as mentioned before, you can't just assume the\n> linear virtual map is going to be satisfactory.\n\nI see.\n\n> > +\tpriv->rx.desc[entry].skbs = skb;\n> > +\n> > +\t/* set buffer pointer */\n> > +\tave_wdesc_addr(ndev, AVE_DESCID_RX, entry, 4, paddr);\n> \n> OK, so 4 is an offset, can you add a define for that so it's clear it is\n> not an address size instead?\n\nYes, 4 is an offset. I'll add the definition for '4'.\n\n> > +\n> > +\t/* set enable to cmdsts */\n> > +\tave_wdesc(ndev, AVE_DESCID_RX,\n> > +\t\t  entry, 0, AVE_STS_INTR | AVE_MAX_ETHFRAME);\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +/* Switch state of descriptor */\n> > +static int ave_desc_switch(struct net_device *ndev, enum desc_state state)\n> > +{\n> > +\tint counter;\n> > +\tu32 val;\n> > +\n> > +\tswitch (state) {\n> > +\tcase AVE_DESC_START:\n> > +\t\tave_w32(ndev, AVE_DESCC, AVE_DESCC_TD | AVE_DESCC_RD0);\n> > +\t\tbreak;\n> > +\n> > +\tcase AVE_DESC_STOP:\n> > +\t\tave_w32(ndev, AVE_DESCC, 0);\n> > +\t\tcounter = 0;\n> > +\t\twhile (1) {\n> > +\t\t\tusleep_range(100, 150);\n> > +\t\t\tif (!ave_r32(ndev, AVE_DESCC))\n> > +\t\t\t\tbreak;\n> > +\n> > +\t\t\tcounter++;\n> > +\t\t\tif (counter > 100) {\n> > +\t\t\t\tnetdev_err(ndev, \"can't stop descriptor\\n\");\n> > +\t\t\t\treturn -EBUSY;\n> > +\t\t\t}\n> > +\t\t}\n> \n> Same here, pleas rewrite the loop so the exit condition is clear.\n\nDitto.\n\n> > +\t\tbreak;\n> > +\n> > +\tcase AVE_DESC_RX_SUSPEND:\n> > +\t\tval = ave_r32(ndev, AVE_DESCC);\n> > +\t\tval |= AVE_DESCC_RDSTP;\n> > +\t\tval &= AVE_DESCC_CTRL_MASK;\n> \n> Should not this be a &= ~AVE_DESCC_CTRL_MASK? OK maybe not.\n\nThis may be confusing. I'll fix it.\n\n> > +\t\tave_w32(ndev, AVE_DESCC, val);\n> > +\n> > +\t\tcounter = 0;\n> > +\t\twhile (1) {\n> > +\t\t\tusleep_range(100, 150);\n> > +\t\t\tval = ave_r32(ndev, AVE_DESCC);\n> > +\t\t\tif (val & (AVE_DESCC_RDSTP << 16))\n> > +\t\t\t\tbreak;\n> > +\n> > +\t\t\tcounter++;\n> > +\t\t\tif (counter > 1000) {\n> > +\t\t\t\tnetdev_err(ndev, \"can't suspend descriptor\\n\");\n> > +\t\t\t\treturn -EBUSY;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t\tbreak;\n> \n> Same here, please rewrite with the counter as an exit condition.\n\nDitto.\n\n> > +\n> > +\tcase AVE_DESC_RX_PERMIT:\n> > +\t\tval = ave_r32(ndev, AVE_DESCC);\n> > +\t\tval &= ~AVE_DESCC_RDSTP;\n> > +\t\tval &= AVE_DESCC_CTRL_MASK;\n> > +\t\tave_w32(ndev, AVE_DESCC, val);\n> > +\t\tbreak;\n> > +\n> > +\tdefault:\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static int ave_tx_completion(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 proc_idx, done_idx, ndesc, val;\n> > +\tint freebuf_flag = 0;\n> > +\n> > +\tproc_idx = priv->tx.proc_idx;\n> > +\tdone_idx = priv->tx.done_idx;\n> > +\tndesc    = priv->tx.ndesc;\n> > +\n> > +\t/* free pre stored skb from done to proc */\n> > +\twhile (proc_idx != done_idx) {\n> > +\t\t/* get cmdsts */\n> > +\t\tval = ave_rdesc(ndev, AVE_DESCID_TX, done_idx, 0);\n> > +\n> > +\t\t/* do nothing if owner is HW */\n> > +\t\tif (val & AVE_STS_OWN)\n> > +\t\t\tbreak;\n> > +\n> > +\t\t/* check Tx status and updates statistics */\n> > +\t\tif (val & AVE_STS_OK) {\n> > +\t\t\tpriv->stats.tx_bytes += val & AVE_STS_PKTLEN_TX;\n> \n> AVE_STS_PKTLEN_TX should be suffixed with _MASK to make it clear this is\n> a bitmask.\n\nI see. I'll rename it.\n\n> > +\n> > +\t\t\t/* success */\n> > +\t\t\tif (val & AVE_STS_LAST)\n> > +\t\t\t\tpriv->stats.tx_packets++;\n> > +\t\t} else {\n> > +\t\t\t/* error */\n> > +\t\t\tif (val & AVE_STS_LAST) {\n> > +\t\t\t\tpriv->stats.tx_errors++;\n> > +\t\t\t\tif (val & (AVE_STS_OWC | AVE_STS_EC))\n> > +\t\t\t\t\tpriv->stats.collisions++;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\t/* release skb */\n> > +\t\tif (priv->tx.desc[done_idx].skbs) {\n> > +\t\t\tave_dma_unmap(ndev, &priv->tx.desc[done_idx],\n> > +\t\t\t\t      DMA_TO_DEVICE);\n> > +\t\t\tdev_consume_skb_any(priv->tx.desc[done_idx].skbs);\n> \n> Kudos for using dev_consume_skb_any()!\n\nThank you! I was worried about whether I could use it.\n\n> > +\t\t\tpriv->tx.desc[done_idx].skbs = NULL;\n> > +\t\t\tfreebuf_flag++;\n> > +\t\t}\n> > +\t\tdone_idx = (done_idx + 1) % ndesc;\n> > +\t}\n> > +\n> > +\tpriv->tx.done_idx = done_idx;\n> > +\n> > +\t/* wake queue for freeing buffer */\n> > +\tif (netif_queue_stopped(ndev))\n> > +\t\t\tif (freebuf_flag)\n> > +\t\t\t\tnetif_wake_queue(ndev);\n> \n> This can be simplified to:\n> \n> \tif (netif_queue_stopped(ndev) && freebuf_flag)\n> \t\tnetif_wake_queue(ndev)\n> \n> because checking for netif_running() is implicit by actually getting\n> called here, this would not happen if the network interface was not\n> operational.\n\nOh, it can be simple. I understand the reason.\n\n> > +static irqreturn_t ave_interrupt(int irq, void *netdev)\n> > +{\n> > +\tstruct net_device *ndev = (struct net_device *)netdev;\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 gimr_val, gisr_val;\n> > +\n> > +\tgimr_val = ave_irq_disable_all(ndev);\n> > +\n> > +\t/* get interrupt status */\n> > +\tgisr_val = ave_r32(ndev, AVE_GISR);\n> > +\n> > +\t/* PHY */\n> > +\tif (gisr_val & AVE_GI_PHY) {\n> > +\t\tave_w32(ndev, AVE_GISR, AVE_GI_PHY);\n> > +\t\tif (priv->internal_phy_interrupt)\n> > +\t\t\tphy_mac_interrupt(ndev->phydev, ndev->phydev->link);\n> \n> This is not correct you should be telling the PHY the new link state. If\n> you cannot, because what happens here is really that the PHY interrupt\n> is routed to your MAC controller, then what I would suggest doing is the\n> following: create an interrupt controller domain which allows the PHY\n> driver to manage the interrupt line using normal request_irq().\n\nYou're right. The interrupt from PHY is routed to the MAC controller.\nHmm...I think that I want to use normal PHY sequence including request_irq,\nso I'll try to consider about applying the interrupt controller domain.\n\n> > +static int ave_poll_tx(struct napi_struct *napi, int budget)\n> > +{\n> > +\tstruct ave_private *priv;\n> > +\tstruct net_device *ndev;\n> > +\tint num;\n> > +\n> > +\tpriv = container_of(napi, struct ave_private, napi_tx);\n> > +\tndev = priv->ndev;\n> > +\n> > +\tnum = ave_tx_completion(ndev);\n> > +\tif (num < budget) {\n> \n> TX reclamation should not be bound against the budget, always process\n> all packets that have been successfully submitted.\n\nIt's helpful for me.\nI'll reconsider it to submit all packets.\n\n> > +\t\tnapi_complete(napi);\n> > +\n> > +\t\t/* enable Tx interrupt when NAPI finishes */\n> > +\t\tave_irq_enable(ndev, AVE_GI_TX);\n> > +\t}\n> > +\n> > +\treturn num;\n> > +}\n> > +\n> \n> > +static void ave_adjust_link(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tstruct phy_device *phydev = ndev->phydev;\n> > +\tu32 val, txcr, rxcr, rxcr_org;\n> > +\n> > +\t/* set RGMII speed */\n> > +\tval = ave_r32(ndev, AVE_TXCR);\n> > +\tval &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G);\n> > +\n> > +\tif (priv->phy_mode == PHY_INTERFACE_MODE_RGMII &&\n> > +\t    phydev->speed == SPEED_1000)\n> > +\t\tval |= AVE_TXCR_TXSPD_1G;\n> \n> Using phy_interface_is_rgmii() would be more robust here.\n\nIt's convenience.\n\n> > +\telse if (phydev->speed == SPEED_100)\n> > +\t\tval |= AVE_TXCR_TXSPD_100;\n> > +\n> > +\tave_w32(ndev, AVE_TXCR, val);\n> > +\n> > +\t/* set RMII speed (100M/10M only) */\n> > +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n> \n> PHY_INTERFACE_MODE_MII, PHY_INTERFACE_MODE_REVMII,\n> PHY_INTERFACE_MODE_RMII would all qualify here presumably so you need\n> this check to be more restrictive to just the modes that you support.\n\nI'll rewrite it to check the supported modes restrictly.\n\n> > +\t\tval = ave_r32(ndev, AVE_LINKSEL);\n> > +\t\tif (phydev->speed == SPEED_10)\n> > +\t\t\tval &= ~AVE_LINKSEL_100M;\n> > +\t\telse\n> > +\t\t\tval |= AVE_LINKSEL_100M;\n> > +\t\tave_w32(ndev, AVE_LINKSEL, val);\n> > +\t}\n> > +\n> > +\t/* check current RXCR/TXCR */\n> > +\trxcr = ave_r32(ndev, AVE_RXCR);\n> > +\ttxcr = ave_r32(ndev, AVE_TXCR);\n> > +\trxcr_org = rxcr;\n> > +\n> > +\tif (phydev->duplex)\n> > +\t\trxcr |= AVE_RXCR_FDUPEN;\n> > +\telse\n> > +\t\trxcr &= ~AVE_RXCR_FDUPEN;\n> > +\n> > +\tif (phydev->pause) {\n> > +\t\trxcr |= AVE_RXCR_FLOCTR;\n> > +\t\ttxcr |= AVE_TXCR_FLOCTR;\n> \n> You must also check for phydev->asym_pause and keep in mind that\n> phydev->pause and phydev->asym_pause reflect what the link partner\n> reflects, you need to implement .get_pauseparam and .set_pauseparam or\n> default to flow control ON (which is what you are doing here).\n\nI see.\nI'll consider how to implement flow control with pause and asym_pause.\n\n> > +\t} else {\n> > +\t\trxcr &= ~AVE_RXCR_FLOCTR;\n> > +\t\ttxcr &= ~AVE_TXCR_FLOCTR;\n> > +\t}\n> > +\n> > +\tif (rxcr_org != rxcr) {\n> > +\t\t/* disable Rx mac */\n> > +\t\tave_w32(ndev, AVE_RXCR, rxcr & ~AVE_RXCR_RXEN);\n> > +\t\t/* change and enable TX/Rx mac */\n> > +\t\tave_w32(ndev, AVE_TXCR, txcr);\n> > +\t\tave_w32(ndev, AVE_RXCR, rxcr);\n> > +\t}\n> > +\n> > +\tif (phydev->link)\n> > +\t\tnetif_carrier_on(ndev);\n> > +\telse\n> > +\t\tnetif_carrier_off(ndev);\n> \n> This is not necessary, PHYLIB is specifically taking care of that.\n\nOkay, I'll remove it.\n\n> > +\n> > +\tphy_print_status(phydev);\n> > +}\n> > +\n> > +static int ave_init(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tstruct device *dev = ndev->dev.parent;\n> > +\tstruct device_node *phy_node, *np = dev->of_node;\n> > +\tstruct phy_device *phydev;\n> > +\tconst void *mac_addr;\n> > +\tu32 supported;\n> > +\n> > +\t/* get mac address */\n> > +\tmac_addr = of_get_mac_address(np);\n> > +\tif (mac_addr)\n> > +\t\tether_addr_copy(ndev->dev_addr, mac_addr);\n> > +\n> > +\t/* if the mac address is invalid, use random mac address */\n> > +\tif (!is_valid_ether_addr(ndev->dev_addr)) {\n> > +\t\teth_hw_addr_random(ndev);\n> > +\t\tdev_warn(dev, \"Using random MAC address: %pM\\n\",\n> > +\t\t\t ndev->dev_addr);\n> > +\t}\n> > +\n> > +\t/* attach PHY with MAC */\n> > +\tphy_node =  of_get_next_available_child(np, NULL);\n> \n> You should be using a \"phy-handle\" property to connect to a designated\n> PHY, not the next child DT node.\n\nYes, I found it was wrong. I'll fix it.\n\n> > +\tphydev = of_phy_connect(ndev, phy_node,\n> > +\t\t\t\tave_adjust_link, 0, priv->phy_mode);\n> > +\tif (!phydev) {\n> > +\t\tdev_err(dev, \"could not attach to PHY\\n\");\n> > +\t\treturn -ENODEV;\n> > +\t}\n> > +\tof_node_put(phy_node);\n> > +\n> > +\tpriv->phydev = phydev;\n> > +\tphydev->autoneg = AUTONEG_ENABLE;\n> > +\tphydev->speed = 0;\n> > +\tphydev->duplex = 0;\n> \n> This is not necessary.\n\nI see. I'll remove it.\n\n> > +\n> > +\tdev_info(dev, \"connected to %s phy with id 0x%x\\n\",\n> > +\t\t phydev->drv->name, phydev->phy_id);\n> > +\n> > +\tif (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {\n> > +\t\tsupported = phydev->supported;\n> > +\t\tphydev->supported &= ~PHY_GBIT_FEATURES;\n> > +\t\tphydev->supported |= supported & PHY_BASIC_FEATURES;\n> \n> One of these two statements is redundant here.\n\nI'll shirink the statements.\n\n> > +\t}\n> > +\n> > +\t/* PHY interrupt stop instruction is needed because the interrupt\n> > +\t * continues to assert.\n> > +\t */\n> > +\tphy_stop_interrupts(phydev);\n> \n> Wait, what?\n\nI've thought that PHY interrupts might be enabled, but It's wrong.\n\n> > +\n> > +\t/* When PHY driver can't handle its interrupt directly,\n> > +\t * interrupt request always fails and polling method is used\n> > +\t * alternatively. In this case, the libphy says\n> > +\t * \"libphy: uniphier-mdio: Can't get IRQ -1 (PHY)\".\n> > +\t */\n> > +\tphy_start_interrupts(phydev);\n> > +\n> > +\tphy_start_aneg(phydev);\n> \n> No, no, phy_start() would take care of all of that correctly for you,\n> you don't have have to do it just there because ave_open() eventually\n> calls phy_start() for you, so just drop these two calls.\n\nOh, I see.\nOnce calling phy_start(), all are done.\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static void ave_uninit(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\n> > +\tphy_stop_interrupts(priv->phydev);\n> \n> You are missing a call to phy_stop() here, calling phy_stop_interrupts()\n> directly should not happen.\n\nI'll replace it.\n\n> > +\tphy_disconnect(priv->phydev);\n> > +}\n> > +\n> > +static int ave_open(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tint entry;\n> > +\tu32 val;\n> > +\n> > +\t/* initialize Tx work */\n> > +\tpriv->tx.proc_idx = 0;\n> > +\tpriv->tx.done_idx = 0;\n> > +\tmemset(priv->tx.desc, 0, sizeof(struct ave_desc) * priv->tx.ndesc);\n> > +\n> > +\t/* initialize Tx descriptor */\n> > +\tfor (entry = 0; entry < priv->tx.ndesc; entry++) {\n> > +\t\tave_wdesc(ndev, AVE_DESCID_TX, entry, 0, 0);\n> > +\t\tave_wdesc_addr(ndev, AVE_DESCID_TX, entry, 4, 0);\n> > +\t}\n> > +\tave_w32(ndev, AVE_TXDC, AVE_TXDC_ADDR_START\n> > +\t\t| (((priv->tx.ndesc * priv->desc_size) << 16) & AVE_TXDC_SIZE));\n> > +\n> > +\t/* initialize Rx work */\n> > +\tpriv->rx.proc_idx = 0;\n> > +\tpriv->rx.done_idx = 0;\n> > +\tmemset(priv->rx.desc, 0, sizeof(struct ave_desc) * priv->rx.ndesc);\n> > +\n> > +\t/* initialize Rx descriptor and buffer */\n> > +\tfor (entry = 0; entry < priv->rx.ndesc; entry++) {\n> > +\t\tif (ave_set_rxdesc(ndev, entry))\n> > +\t\t\tbreak;\n> > +\t}\n> > +\tave_w32(ndev, AVE_RXDC0, AVE_RXDC0_ADDR_START\n> > +\t    | (((priv->rx.ndesc * priv->desc_size) << 16) & AVE_RXDC0_SIZE));\n> > +\n> > +\t/* enable descriptor */\n> > +\tave_desc_switch(ndev, AVE_DESC_START);\n> > +\n> > +\t/* initialize filter */\n> > +\tave_pfsel_init(ndev);\n> > +\n> > +\t/* set macaddr */\n> > +\tval = le32_to_cpu(((u32 *)ndev->dev_addr)[0]);\n> > +\tave_w32(ndev, AVE_RXMAC1R, val);\n> > +\tval = (u32)le16_to_cpu(((u16 *)ndev->dev_addr)[2]);\n> > +\tave_w32(ndev, AVE_RXMAC2R, val);\n> > +\n> > +\t/* set Rx configuration */\n> > +\t/* full duplex, enable pause drop, enalbe flow control */\n> > +\tval = AVE_RXCR_RXEN | AVE_RXCR_FDUPEN | AVE_RXCR_DRPEN |\n> > +\t\tAVE_RXCR_FLOCTR | (AVE_MAX_ETHFRAME & AVE_RXCR_MPSIZ_MASK);\n> > +\tave_w32(ndev, AVE_RXCR, val);\n> > +\n> > +\t/* set Tx configuration */\n> > +\t/* enable flow control, disable loopback */\n> > +\tave_w32(ndev, AVE_TXCR, AVE_TXCR_FLOCTR);\n> > +\n> > +\t/* enable timer, clear EN,INTM, and mask interval unit(BSCK) */\n> > +\tval = ave_r32(ndev, AVE_IIRQC) & AVE_IIRQC_BSCK;\n> > +\tval |= AVE_IIRQC_EN0 | (AVE_INTM_COUNT << 16);\n> > +\tave_w32(ndev, AVE_IIRQC, val);\n> > +\n> > +\t/* set interrupt mask */\n> > +\tval = AVE_GI_RXIINT | AVE_GI_RXOVF | AVE_GI_TX;\n> > +\tval |= (priv->internal_phy_interrupt) ? AVE_GI_PHY : 0;\n> > +\tave_irq_restore(ndev, val);\n> > +\n> > +\tnapi_enable(&priv->napi_rx);\n> > +\tnapi_enable(&priv->napi_tx);\n> > +\n> > +\tphy_start(ndev->phydev);\n> > +\tnetif_start_queue(ndev);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static int ave_stop(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tint entry;\n> > +\n> > +\t/* disable all interrupt */\n> > +\tave_irq_disable_all(ndev);\n> > +\tdisable_irq(ndev->irq);\n> > +\n> > +\tnetif_tx_disable(ndev);\n> > +\tphy_stop(ndev->phydev);\n> > +\tnapi_disable(&priv->napi_tx);\n> > +\tnapi_disable(&priv->napi_rx);\n> > +\n> > +\t/* free Tx buffer */\n> > +\tfor (entry = 0; entry < priv->tx.ndesc; entry++) {\n> > +\t\tif (!priv->tx.desc[entry].skbs)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tave_dma_unmap(ndev, &priv->tx.desc[entry], DMA_TO_DEVICE);\n> > +\t\tdev_kfree_skb_any(priv->tx.desc[entry].skbs);\n> > +\t\tpriv->tx.desc[entry].skbs = NULL;\n> > +\t}\n> > +\tpriv->tx.proc_idx = 0;\n> > +\tpriv->tx.done_idx = 0;\n> > +\n> > +\t/* free Rx buffer */\n> > +\tfor (entry = 0; entry < priv->rx.ndesc; entry++) {\n> > +\t\tif (!priv->rx.desc[entry].skbs)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tave_dma_unmap(ndev, &priv->rx.desc[entry], DMA_FROM_DEVICE);\n> > +\t\tdev_kfree_skb_any(priv->rx.desc[entry].skbs);\n> > +\t\tpriv->rx.desc[entry].skbs = NULL;\n> > +\t}\n> > +\tpriv->rx.proc_idx = 0;\n> > +\tpriv->rx.done_idx = 0;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 proc_idx, done_idx, ndesc, cmdsts;\n> > +\tint freepkt;\n> > +\tunsigned char *buffptr = NULL; /* buffptr for descriptor */\n> > +\tunsigned int len;\n> > +\tdma_addr_t paddr;\n> > +\n> > +\tproc_idx = priv->tx.proc_idx;\n> > +\tdone_idx = priv->tx.done_idx;\n> > +\tndesc = priv->tx.ndesc;\n> > +\tfreepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;\n> > +\n> > +\t/* not enough entry, then we stop queue */\n> > +\tif (unlikely(freepkt < 2)) {\n> > +\t\tnetif_stop_queue(ndev);\n> > +\t\tif (unlikely(freepkt < 1))\n> > +\t\t\treturn NETDEV_TX_BUSY;\n> > +\t}\n> > +\n> > +\tpriv->tx.desc[proc_idx].skbs = skb;\n> > +\n> > +\t/* add padding for short packet */\n> > +\tif (skb_padto(skb, ETH_ZLEN)) {\n> > +\t\tdev_kfree_skb_any(skb);\n> > +\t\treturn NETDEV_TX_OK;\n> > +\t}\n> > +\n> > +\tbuffptr = skb->data - NET_IP_ALIGN;\n> > +\tlen = max_t(unsigned int, ETH_ZLEN, skb->len);\n> > +\n> > +\tpaddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,\n> > +\t\t\t    len + NET_IP_ALIGN, DMA_TO_DEVICE);\n> > +\tpaddr += NET_IP_ALIGN;\n> > +\n> > +\t/* set buffer address to descriptor */\n> > +\tave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);\n> > +\n> > +\t/* set flag and length to send */\n> > +\tcmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST\n> > +\t\t| (len & AVE_STS_PKTLEN_TX);\n> > +\n> > +\t/* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */\n> > +\tif (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))\n> > +\t\tcmdsts |= AVE_STS_INTR;\n> > +\n> > +\t/* disable checksum calculation when skb doesn't calurate checksum */\n> > +\tif (skb->ip_summed == CHECKSUM_NONE ||\n> > +\t    skb->ip_summed == CHECKSUM_UNNECESSARY)\n> > +\t\tcmdsts |= AVE_STS_NOCSUM;\n> > +\n> > +\t/* set cmdsts */\n> > +\tave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);\n> > +\n> > +\tpriv->tx.proc_idx = (proc_idx + 1) % ndesc;\n> > +\n> > +\treturn NETDEV_TX_OK;\n> > +}\n> > +\n> > +static int ave_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)\n> > +{\n> > +\treturn phy_mii_ioctl(ndev->phydev, ifr, cmd);\n> > +}\n> > +\n> > +static void ave_set_rx_mode(struct net_device *ndev)\n> > +{\n> > +\tint count, mc_cnt = netdev_mc_count(ndev);\n> > +\tstruct netdev_hw_addr *hw_adr;\n> > +\tu32 val;\n> > +\tu8 v4multi_macadr[6] = { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00 };\n> > +\tu8 v6multi_macadr[6] = { 0x33, 0x00, 0x00, 0x00, 0x00, 0x00 };\n> > +\n> > +\t/* MAC addr filter enable for promiscious mode */\n> > +\tval = ave_r32(ndev, AVE_RXCR);\n> > +\tif (ndev->flags & IFF_PROMISC || !mc_cnt)\n> > +\t\tval &= ~AVE_RXCR_AFEN;\n> > +\telse\n> > +\t\tval |= AVE_RXCR_AFEN;\n> > +\tave_w32(ndev, AVE_RXCR, val);\n> > +\n> > +\t/* set all multicast address */\n> > +\tif ((ndev->flags & IFF_ALLMULTI) || (mc_cnt > AVE_PF_MULTICAST_SIZE)) {\n> > +\t\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST,\n> > +\t\t\t\t      v4multi_macadr, 1);\n> > +\t\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST + 1,\n> > +\t\t\t\t      v6multi_macadr, 1);\n> > +\t} else {\n> > +\t\t/* stop all multicast filter */\n> > +\t\tfor (count = 0; count < AVE_PF_MULTICAST_SIZE; count++)\n> > +\t\t\tave_pfsel_stop(ndev, AVE_PFNUM_MULTICAST + count);\n> > +\n> > +\t\t/* set multicast addresses */\n> > +\t\tcount = 0;\n> > +\t\tnetdev_for_each_mc_addr(hw_adr, ndev) {\n> > +\t\t\tif (count == mc_cnt)\n> > +\t\t\t\tbreak;\n> > +\t\t\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_MULTICAST + count,\n> > +\t\t\t\t\t      hw_adr->addr, 6);\n> > +\t\t\tcount++;\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> > +static struct net_device_stats *ave_stats(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 drop_num = 0;\n> > +\n> > +\tpriv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);\n> > +\n> > +\tdrop_num += ave_r32(ndev, AVE_RX0OVFFC);\n> > +\tdrop_num += ave_r32(ndev, AVE_SN5FC);\n> > +\tdrop_num += ave_r32(ndev, AVE_SN6FC);\n> > +\tdrop_num += ave_r32(ndev, AVE_SN7FC);\n> > +\tpriv->stats.rx_dropped = drop_num;\n> > +\n> > +\treturn &priv->stats;\n> > +}\n> > +\n> > +static int ave_set_mac_address(struct net_device *ndev, void *p)\n> > +{\n> > +\tint ret = eth_mac_addr(ndev, p);\n> > +\tu32 val;\n> > +\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/* set macaddr */\n> > +\tval = le32_to_cpu(((u32 *)ndev->dev_addr)[0]);\n> > +\tave_w32(ndev, AVE_RXMAC1R, val);\n> > +\tval = (u32)le16_to_cpu(((u16 *)ndev->dev_addr)[2]);\n> > +\tave_w32(ndev, AVE_RXMAC2R, val);\n> > +\n> > +\t/* pfsel unicast entry */\n> > +\tave_pfsel_macaddr_set(ndev, AVE_PFNUM_UNICAST, ndev->dev_addr, 6);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +static const struct net_device_ops ave_netdev_ops = {\n> > +\t.ndo_init\t\t= ave_init,\n> > +\t.ndo_uninit\t\t= ave_uninit,\n> > +\t.ndo_open\t\t= ave_open,\n> > +\t.ndo_stop\t\t= ave_stop,\n> > +\t.ndo_start_xmit\t\t= ave_start_xmit,\n> > +\t.ndo_do_ioctl\t\t= ave_ioctl,\n> > +\t.ndo_set_rx_mode\t= ave_set_rx_mode,\n> > +\t.ndo_get_stats\t\t= ave_stats,\n> > +\t.ndo_set_mac_address\t= ave_set_mac_address,\n> > +};\n> > +\n> > +static int ave_probe(struct platform_device *pdev)\n> > +{\n> > +\tstruct device *dev = &pdev->dev;\n> > +\tstruct device_node *np = dev->of_node;\n> > +\tu32 desc_bits, ave_id;\n> > +\tstruct reset_control *rst;\n> > +\tstruct ave_private *priv;\n> > +\tphy_interface_t phy_mode;\n> > +\tstruct net_device *ndev;\n> > +\tstruct resource\t*res;\n> > +\tvoid __iomem *base;\n> > +\tint irq, ret = 0;\n> > +\tchar buf[ETHTOOL_FWVERS_LEN];\n> > +\n> > +\t/* get phy mode */\n> > +\tphy_mode = of_get_phy_mode(np);\n> > +\tif (phy_mode < 0) {\n> > +\t\tdev_err(dev, \"phy-mode not found\\n\");\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tirq = platform_get_irq(pdev, 0);\n> > +\tif (irq < 0) {\n> > +\t\tdev_err(dev, \"IRQ not found\\n\");\n> > +\t\treturn irq;\n> > +\t}\n> > +\n> > +\tres = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n> > +\tbase = devm_ioremap_resource(dev, res);\n> > +\tif (IS_ERR(base))\n> > +\t\treturn PTR_ERR(base);\n> > +\n> > +\t/* alloc netdevice */\n> > +\tndev = alloc_etherdev(sizeof(struct ave_private));\n> > +\tif (!ndev) {\n> > +\t\tdev_err(dev, \"can't allocate ethernet device\\n\");\n> > +\t\treturn -ENOMEM;\n> > +\t}\n> > +\n> > +\tndev->base_addr = (unsigned long)base;\n> \n> This is deprecated as mentioned earlier, just store this in\n> priv->base_adr or something.\n\nYes, Andrew teaches me in his comment and I'll replace it.\n\n> > +\tndev->irq = irq;\n> \n> Same here.\n\nI'll move it to ave_private, too.\n\n> > +\tndev->netdev_ops = &ave_netdev_ops;\n> > +\tndev->ethtool_ops = &ave_ethtool_ops;\n> > +\tSET_NETDEV_DEV(ndev, dev);\n> > +\n> > +\t/* support hardware checksum */\n> > +\tndev->features    |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);\n> > +\tndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);\n> > +\n> > +\tndev->max_mtu = AVE_MAX_ETHFRAME - (ETH_HLEN + ETH_FCS_LEN);\n> > +\n> > +\tpriv = netdev_priv(ndev);\n> > +\tpriv->ndev = ndev;\n> > +\tpriv->msg_enable = netif_msg_init(-1, AVE_DEFAULT_MSG_ENABLE);\n> > +\tpriv->phy_mode = phy_mode;\n> > +\n> > +\t/* get bits of descriptor from devicetree */\n> > +\tof_property_read_u32(np, \"socionext,desc-bits\", &desc_bits);\n> > +\tpriv->desc_size = AVE_DESC_SIZE_32;\n> > +\tif (desc_bits == 64)\n> > +\t\tpriv->desc_size = AVE_DESC_SIZE_64;\n> > +\telse if (desc_bits != 32)\n> > +\t\tdev_warn(dev, \"Defaulting to 32bit descriptor\\n\");\n> \n> This should really be determined by the compatible string.\n\nOkay, I'll reconsider about compatible strings for each SoCs.\n\n> > +\n> > +\t/* use internal phy interrupt */\n> > +\tpriv->internal_phy_interrupt =\n> > +\t\tof_property_read_bool(np, \"socionext,internal-phy-interrupt\");\n> \n> Same here.\n\nDitto.\n\n> > +\n> > +\t/* setting private data for descriptor */\n> > +\tpriv->tx.daddr = IS_DESC_64BIT(priv) ? AVE_TXDM_64 : AVE_TXDM_32;\n> > +\tpriv->tx.ndesc = AVE_NR_TXDESC;\n> > +\tpriv->tx.desc = devm_kzalloc(dev,\n> > +\t\t\t\t     sizeof(struct ave_desc) * priv->tx.ndesc,\n> > +\t\t\t\t     GFP_KERNEL);\n> > +\tif (!priv->tx.desc)\n> > +\t\treturn -ENOMEM;\n> > +\n> > +\tpriv->rx.daddr = IS_DESC_64BIT(priv) ? AVE_RXDM_64 : AVE_RXDM_32;\n> > +\tpriv->rx.ndesc = AVE_NR_RXDESC;\n> > +\tpriv->rx.desc = devm_kzalloc(dev,\n> > +\t\t\t\t     sizeof(struct ave_desc) * priv->rx.ndesc,\n> > +\t\t\t\t     GFP_KERNEL);\n> > +\tif (!priv->rx.desc)\n> > +\t\treturn -ENOMEM;\n> \n> If your network device driver is probed, but never used after that, that\n> is the network device is not open, you just this memory sitting for\n> nothing, you should consider moving that to ndo_open() time.\n\nIndeed. \nThe driver allocates memory when the driver is opened. I'll reconsider it.\n\n> > +\n> > +\t/* enable clock */\n> > +\tpriv->clk = devm_clk_get(dev, NULL);\n> > +\tif (IS_ERR(priv->clk))\n> > +\t\tpriv->clk = NULL;\n> > +\tclk_prepare_enable(priv->clk);\n> \n> Same here with the clock, the block is clocked, so it can consume some\n> amount of power, just do the necessary HW initialization with the clock\n> enabled, then defer until ndo_open() before turning it back on.\n\nOkay, also the clocks.\n\n> > +\n> > +\t/* reset block */\n> > +\trst = devm_reset_control_get(dev, NULL);\n> > +\tif (!IS_ERR_OR_NULL(rst)) {\n> > +\t\treset_control_deassert(rst);\n> > +\t\treset_control_put(rst);\n> > +\t}\n> \n> Ah so that's interesting, you need it clocked first, then reset, I guess\n> that works :)\n\nYes, this device starts to enable clock first.\n\n> > +\n> > +\t/* reset mac and phy */\n> > +\tave_global_reset(ndev);\n> > +\n> > +\t/* request interrupt */\n> > +\tret = devm_request_irq(dev, irq, ave_interrupt,\n> > +\t\t\t       IRQF_SHARED, ndev->name, ndev);\n> > +\tif (ret)\n> > +\t\tgoto err_req_irq;\n> \n> Same here, this is usually moved to ndo_open() for network drivers, but\n> then remember not to use devm_, just normal request_irq() because it\n> need to be balanced in ndo_close().\n\nOkay, also interrupts without devm_, and I'll take care of ndo_close().\n\n> This looks like a pretty good first submission, and there does not\n> appear to be any obvious functional problems!\n\nThanks a lot for your helpful advice!\n\n> -- \n> Florian\n\n---\nBest Regards,\nKunihiko Hayashi","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrJcF6Ttgz9s2G\n\tfor <patchwork-incoming@ozlabs.org>;\n\tMon, 11 Sep 2017 16:56:13 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751111AbdIKG4B convert rfc822-to-8bit (ORCPT\n\t<rfc822;patchwork-incoming@ozlabs.org>);\n\tMon, 11 Sep 2017 02:56:01 -0400","from mx.socionext.com ([202.248.49.38]:12933 \"EHLO\n\tmx.socionext.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751048AbdIKGz7 (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tMon, 11 Sep 2017 02:55:59 -0400","from unknown (HELO iyokan-ex.css.socionext.com) ([172.31.9.54])\n\tby mx.socionext.com with ESMTP; 11 Sep 2017 15:55:56 +0900","from mail.mfilter.local (unknown [10.213.24.61])\n\tby iyokan-ex.css.socionext.com (Postfix) with ESMTP id 08B8A610C6;\n\tMon, 11 Sep 2017 15:55:57 +0900 (JST)","from 172.31.9.53 (172.31.9.53) by m-FILTER with ESMTP;\n\tMon, 11 Sep 2017 15:55:57 +0900","from yuzu.css.socionext.com (yuzu [172.31.8.45])\n\tby iyokan.css.socionext.com (Postfix) with ESMTP id C757B40512;\n\tMon, 11 Sep 2017 15:55:56 +0900 (JST)","from [127.0.0.1] (unknown [10.213.134.37])\n\tby yuzu.css.socionext.com (Postfix) with ESMTP id 87707120453;\n\tMon, 11 Sep 2017 15:55:56 +0900 (JST)"],"Date":"Mon, 11 Sep 2017 15:55:56 +0900","From":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","To":"Florian Fainelli <f.fainelli@gmail.com>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Cc":"netdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tAndrew Lunn <andrew@lunn.ch>, Rob Herring <robh+dt@kernel.org>,\n\tMark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","In-Reply-To":"<df4ec8b2-67cf-5dee-2600-88cffd1bcd11@gmail.com>","References":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>\n\t<df4ec8b2-67cf-5dee-2600-88cffd1bcd11@gmail.com>","Message-Id":"<20170911155555.6719.4A936039@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"US-ASCII\"","Content-Transfer-Encoding":"8BIT","X-Mailer":"Becky! ver. 2.70 [ja]","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766083,"web_url":"http://patchwork.ozlabs.org/comment/1766083/","msgid":"<20170911155631.671A.4A936039@socionext.com>","list_archive_url":null,"date":"2017-09-11T06:56:32","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":71700,"url":"http://patchwork.ozlabs.org/api/people/71700/","name":"Kunihiko Hayashi","email":"hayashi.kunihiko@socionext.com"},"content":"Hi Florian,\n\nOn Sat, 9 Sep 2017 09:30:58 -0700 <f.fainelli@gmail.com> wrote:\n\n> \n> \n> On 09/08/2017 06:02 AM, Kunihiko Hayashi wrote:\n> > The UniPhier platform from Socionext provides the AVE ethernet\n> > controller that includes MAC and MDIO bus supporting RGMII/RMII\n> > modes. The controller is named AVE.\n> > \n> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>\n> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>\n> > ---\n> \n> [snip]\n> \n> > +static int ave_start_xmit(struct sk_buff *skb, struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 proc_idx, done_idx, ndesc, cmdsts;\n> > +\tint freepkt;\n> > +\tunsigned char *buffptr = NULL; /* buffptr for descriptor */\n> > +\tunsigned int len;\n> > +\tdma_addr_t paddr;\n> > +\n> > +\tproc_idx = priv->tx.proc_idx;\n> > +\tdone_idx = priv->tx.done_idx;\n> > +\tndesc = priv->tx.ndesc;\n> > +\tfreepkt = ((done_idx + ndesc - 1) - proc_idx) % ndesc;\n> > +\n> > +\t/* not enough entry, then we stop queue */\n> > +\tif (unlikely(freepkt < 2)) {\n> > +\t\tnetif_stop_queue(ndev);\n> > +\t\tif (unlikely(freepkt < 1))\n> > +\t\t\treturn NETDEV_TX_BUSY;\n> \n> This looks wrong, why are you checking first for less than 2\n> descriptors, and if there is none, NETDEV_TX_BUSY? If you need 2 slots\n> to complete a transmision, stop the transmit queue and return\n> NETDEV_TX_BUSY.\n\nThis code is misleading and I have to fix this.\nThe device needs a slot to complete a transmission.\n\n> > +\t}\n> > +\n> > +\tpriv->tx.desc[proc_idx].skbs = skb;\n> > +\n> > +\t/* add padding for short packet */\n> > +\tif (skb_padto(skb, ETH_ZLEN)) {\n> > +\t\tdev_kfree_skb_any(skb);\n> > +\t\treturn NETDEV_TX_OK;\n> > +\t}\n> \n> skb_padto() frees the SKB in case of error, that would lead to a double\n> free here.\n\nAh, it occurs double free. I'll fix it.\n\n> > +\n> > +\tbuffptr = skb->data - NET_IP_ALIGN;\n> > +\tlen = max_t(unsigned int, ETH_ZLEN, skb->len);\n> \n> If you use skb_put_padto() if padding was necessary skb->len will be at\n> least ETH_ZLEN, so you can remove this.\n\nI see. It's reasonable.\n\n> > +\n> > +\tpaddr = ave_dma_map(ndev, &priv->tx.desc[proc_idx], buffptr,\n> > +\t\t\t    len + NET_IP_ALIGN, DMA_TO_DEVICE);\n> \n> As mentioned before you can't assume this will never fail.\n\nOkay, I'll rewrite it in consideration of error case.\n\n> > +\tpaddr += NET_IP_ALIGN;\n> > +\n> > +\t/* set buffer address to descriptor */\n> > +\tave_wdesc_addr(ndev, AVE_DESCID_TX, proc_idx, 4, paddr);\n> \n> Also mentioned in the other email, make this 4 a constant so we know\n> it's an offset and not a length.\n\nI see.\n\n> > +\n> > +\t/* set flag and length to send */\n> > +\tcmdsts = AVE_STS_OWN | AVE_STS_1ST | AVE_STS_LAST\n> > +\t\t| (len & AVE_STS_PKTLEN_TX);\n> \n> AVE_STS_PKTLEN_TX would be better named with a _MASK suffix.\n\nYes.\n\n> > +\n> > +\t/* set interrupt per AVE_FORCE_TXINTCNT or when queue is stopped */\n> > +\tif (!(proc_idx % AVE_FORCE_TXINTCNT) || netif_queue_stopped(ndev))\n> > +\t\tcmdsts |= AVE_STS_INTR;\n> > +\n> > +\t/* disable checksum calculation when skb doesn't calurate checksum */\n> > +\tif (skb->ip_summed == CHECKSUM_NONE ||\n> > +\t    skb->ip_summed == CHECKSUM_UNNECESSARY)\n> > +\t\tcmdsts |= AVE_STS_NOCSUM;\n> > +\n> > +\t/* set cmdsts */\n> > +\tave_wdesc(ndev, AVE_DESCID_TX, proc_idx, 0, cmdsts);\n> > +\n> > +\tpriv->tx.proc_idx = (proc_idx + 1) % ndesc;\n> \n> You should also check the ring space after transmission and assert flow\n> control on the transmit queue if needed.\n\nOkay, I'll add this.\n\n> > +\n> > +\treturn NETDEV_TX_OK;\n> > +}\n> \n> [snip]\n> \n> > +static struct net_device_stats *ave_stats(struct net_device *ndev)\n> > +{\n> > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > +\tu32 drop_num = 0;\n> > +\n> > +\tpriv->stats.rx_errors = ave_r32(ndev, AVE_BFCR);\n> > +\n> > +\tdrop_num += ave_r32(ndev, AVE_RX0OVFFC);\n> > +\tdrop_num += ave_r32(ndev, AVE_SN5FC);\n> > +\tdrop_num += ave_r32(ndev, AVE_SN6FC);\n> > +\tdrop_num += ave_r32(ndev, AVE_SN7FC);\n> > +\tpriv->stats.rx_dropped = drop_num;\n> > +\n> \n> You should consider switching to 64-bit statistics, this requires a\n> little bit more work for 32-bit hosts (see\n> include/linux/u64_stats_sync.h) but this allows you to keep statistics\n> around above 4GB.\n\nI see.\nI'll refer to this header and its examples, and rewrite it to be suitable\nfor 32-bit and 64-bit hosts.\n\n> \n> > +\treturn &priv->stats;\n> > +}\n> > +-- \n> Florian\n\n---\nBest Regards,\nKunihiko Hayashi","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrJcv6ldvz9s2G\n\tfor <patchwork-incoming@ozlabs.org>;\n\tMon, 11 Sep 2017 16:56:47 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751318AbdIKG4g (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tMon, 11 Sep 2017 02:56:36 -0400","from mx.socionext.com ([202.248.49.38]:12951 \"EHLO\n\tmx.socionext.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751202AbdIKG4e (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tMon, 11 Sep 2017 02:56:34 -0400","from unknown (HELO kinkan-ex.css.socionext.com) ([172.31.9.52])\n\tby mx.socionext.com with ESMTP; 11 Sep 2017 15:56:33 +0900","from mail.mfilter.local (unknown [10.213.24.62])\n\tby kinkan-ex.css.socionext.com (Postfix) with ESMTP id 0A206180070;\n\tMon, 11 Sep 2017 15:56:33 +0900 (JST)","from 172.31.9.53 (172.31.9.53) by m-FILTER with ESMTP;\n\tMon, 11 Sep 2017 15:56:33 +0900","from yuzu.css.socionext.com (yuzu [172.31.8.45])\n\tby iyokan.css.socionext.com (Postfix) with ESMTP id D5A3940512;\n\tMon, 11 Sep 2017 15:56:32 +0900 (JST)","from [127.0.0.1] (unknown [10.213.134.37])\n\tby yuzu.css.socionext.com (Postfix) with ESMTP id 97D88120453;\n\tMon, 11 Sep 2017 15:56:32 +0900 (JST)"],"Date":"Mon, 11 Sep 2017 15:56:32 +0900","From":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","To":"Florian Fainelli <f.fainelli@gmail.com>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Cc":"netdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tAndrew Lunn <andrew@lunn.ch>, Rob Herring <robh+dt@kernel.org>,\n\tMark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","In-Reply-To":"<46b1bd9e-35ff-dc7f-fa32-f8d22b37a403@gmail.com>","References":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>\n\t<46b1bd9e-35ff-dc7f-fa32-f8d22b37a403@gmail.com>","Message-Id":"<20170911155631.671A.4A936039@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"US-ASCII\"","Content-Transfer-Encoding":"7bit","X-Mailer":"Becky! ver. 2.70 [ja]","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766277,"web_url":"http://patchwork.ozlabs.org/comment/1766277/","msgid":"<20170911120009.GA24174@lunn.ch>","list_archive_url":null,"date":"2017-09-11T12:00:09","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"> > > +static irqreturn_t ave_interrupt(int irq, void *netdev)\n> > > +{\n> > > +\tstruct net_device *ndev = (struct net_device *)netdev;\n> > > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > > +\tu32 gimr_val, gisr_val;\n> > > +\n> > > +\tgimr_val = ave_irq_disable_all(ndev);\n> > > +\n> > > +\t/* get interrupt status */\n> > > +\tgisr_val = ave_r32(ndev, AVE_GISR);\n> > > +\n> > > +\t/* PHY */\n> > > +\tif (gisr_val & AVE_GI_PHY) {\n> > > +\t\tave_w32(ndev, AVE_GISR, AVE_GI_PHY);\n> > > +\t\tif (priv->internal_phy_interrupt)\n> > > +\t\t\tphy_mac_interrupt(ndev->phydev, ndev->phydev->link);\n> > \n> > Humm. I don't think this is correct. You are supposed to give it the\n> > new link state, not the old.\n> > \n> > What does a PHY interrupt mean here? \n> \n> In the general case, I think PHY events like changing link state are transmitted\n> to CPU as interrupt via interrupt controller, then PHY driver itself can handle\n> the interrupt.\n> \n> And in this case, PHY events are transmitted to MAC as one of its interrupt factor,\n> then I thought that MAC driver had to tell the events to PHY.\n\nCould this be in-band SGMI signalling from the PHY to the MAC? Does\nthe documentation give examples of when this interrupt will happen?\n\n    Andrew","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrS3h4m0Fz9s4q\n\tfor <patchwork-incoming@ozlabs.org>;\n\tMon, 11 Sep 2017 22:32:00 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751800AbdIKMbt (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tMon, 11 Sep 2017 08:31:49 -0400","from vps0.lunn.ch ([178.209.37.122]:37085 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750980AbdIKMbs (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tMon, 11 Sep 2017 08:31:48 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from <andrew@lunn.ch>)\n\tid 1drNNp-0006Ls-5L; Mon, 11 Sep 2017 14:00:09 +0200"],"Date":"Mon, 11 Sep 2017 14:00:09 +0200","From":"Andrew Lunn <andrew@lunn.ch>","To":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","Cc":"netdev@vger.kernel.org, \"David S. Miller\" <davem@davemloft.net>,\n\tFlorian Fainelli <f.fainelli@gmail.com>,\n\tRob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\tlinux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,\n\tdevicetree@vger.kernel.org,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Message-ID":"<20170911120009.GA24174@lunn.ch>","References":"<1504875731-3680-3-git-send-email-hayashi.kunihiko@socionext.com>\n\t<20170908135030.GA25219@lunn.ch>\n\t<20170911155047.6717.4A936039@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170911155047.6717.4A936039@socionext.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766804,"web_url":"http://patchwork.ozlabs.org/comment/1766804/","msgid":"<20170912182400.180E.4A936039@socionext.com>","list_archive_url":null,"date":"2017-09-12T09:24:01","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":71700,"url":"http://patchwork.ozlabs.org/api/people/71700/","name":"Kunihiko Hayashi","email":"hayashi.kunihiko@socionext.com"},"content":"Hi Andrew,\n\nOn Mon, 11 Sep 2017 14:00:09 +0200 Andrew Lunn <andrew@lunn.ch> wrote:\n\n> > > > +static irqreturn_t ave_interrupt(int irq, void *netdev)\n> > > > +{\n> > > > +\tstruct net_device *ndev = (struct net_device *)netdev;\n> > > > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > > > +\tu32 gimr_val, gisr_val;\n> > > > +\n> > > > +\tgimr_val = ave_irq_disable_all(ndev);\n> > > > +\n> > > > +\t/* get interrupt status */\n> > > > +\tgisr_val = ave_r32(ndev, AVE_GISR);\n> > > > +\n> > > > +\t/* PHY */\n> > > > +\tif (gisr_val & AVE_GI_PHY) {\n> > > > +\t\tave_w32(ndev, AVE_GISR, AVE_GI_PHY);\n> > > > +\t\tif (priv->internal_phy_interrupt)\n> > > > +\t\t\tphy_mac_interrupt(ndev->phydev, ndev->phydev->link);\n> > > \n> > > Humm. I don't think this is correct. You are supposed to give it the\n> > > new link state, not the old.\n> > > \n> > > What does a PHY interrupt mean here? \n> > \n> > In the general case, I think PHY events like changing link state are transmitted\n> > to CPU as interrupt via interrupt controller, then PHY driver itself can handle\n> > the interrupt.\n> > \n> > And in this case, PHY events are transmitted to MAC as one of its interrupt factor,\n> > then I thought that MAC driver had to tell the events to PHY.\n> \n> Could this be in-band SGMI signalling from the PHY to the MAC? Does\n> the documentation give examples of when this interrupt will happen?\n> \n>     Andrew\n\nUnfortunately this MAC doesn't support SGMII.\nAnd there aren't any examples of when this interrupt will happen.\nThis interrupt happens after ave_open() is called and link is established.\n\nHowever, I found that auto negotiation didn't start when this interrupt wasn't handled.\n\nAlthough ave_init() calls phy_start_aneg(), it doesn't make sense\nbecause phy driver doesn't start yet.\n\nAnd according to Florian's comment in ave_init(),\n\n> +\tphy_start_interrupts(phydev);\n> +\n> +\tphy_start_aneg(phydev);\n>\n> No, no, phy_start() would take care of all of that correctly for you,\n> you don't have have to do it just there because ave_open() eventually\n> calls phy_start() for you, so just drop these two calls.\n\nWhen moving phy_start_aneg() to ave_open(), the handler doesn't need to call\nphy_mac_interrupt() and I can remove it from the handler.\n\n---\nBest Regards,\nKunihiko Hayashi","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrzrt1vcTz9s81\n\tfor <patchwork-incoming@ozlabs.org>;\n\tTue, 12 Sep 2017 19:24:30 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751472AbdILJYF (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tTue, 12 Sep 2017 05:24:05 -0400","from mx.socionext.com ([202.248.49.38]:35374 \"EHLO\n\tmx.socionext.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751214AbdILJYD (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tTue, 12 Sep 2017 05:24:03 -0400","from unknown (HELO iyokan-ex.css.socionext.com) ([172.31.9.54])\n\tby mx.socionext.com with ESMTP; 12 Sep 2017 18:24:02 +0900","from mail.mfilter.local (unknown [10.213.24.61])\n\tby iyokan-ex.css.socionext.com (Postfix) with ESMTP id 742E0610C6;\n\tTue, 12 Sep 2017 18:24:02 +0900 (JST)","from 172.31.9.53 (172.31.9.53) by m-FILTER with ESMTP;\n\tTue, 12 Sep 2017 18:24:02 +0900","from yuzu.css.socionext.com (yuzu [172.31.8.45])\n\tby iyokan.css.socionext.com (Postfix) with ESMTP id D0827408B6;\n\tTue, 12 Sep 2017 18:24:01 +0900 (JST)","from [127.0.0.1] (unknown [10.213.134.37])\n\tby yuzu.css.socionext.com (Postfix) with ESMTP id 914041204AE;\n\tTue, 12 Sep 2017 18:24:01 +0900 (JST)"],"Date":"Tue, 12 Sep 2017 18:24:01 +0900","From":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","To":"Andrew Lunn <andrew@lunn.ch>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Cc":"<netdev@vger.kernel.org>, \"David S. Miller\" <davem@davemloft.net>,\n\tFlorian Fainelli <f.fainelli@gmail.com>,\n\tRob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>,\n\t<linux-arm-kernel@lists.infradead.org>,\n\t<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","In-Reply-To":"<20170911120009.GA24174@lunn.ch>","References":"<20170911155047.6717.4A936039@socionext.com>\n\t<20170911120009.GA24174@lunn.ch>","Message-Id":"<20170912182400.180E.4A936039@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"US-ASCII\"","Content-Transfer-Encoding":"7bit","X-Mailer":"Becky! ver. 2.70 [ja]","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1772772,"web_url":"http://patchwork.ozlabs.org/comment/1772772/","msgid":"<20170921212746.9119.4A936039@socionext.com>","list_archive_url":null,"date":"2017-09-21T12:27:47","subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","submitter":{"id":71700,"url":"http://patchwork.ozlabs.org/api/people/71700/","name":"Kunihiko Hayashi","email":"hayashi.kunihiko@socionext.com"},"content":"On Mon, 11 Sep 2017 15:55:56 +0900 <hayashi.kunihiko@socionext.com> wrote:\n\n> > > +static int ave_set_rxdesc(struct net_device *ndev, int entry)\n> > > +{\n> > > +\tstruct ave_private *priv = netdev_priv(ndev);\n> > > +\tstruct sk_buff *skb;\n> > > +\tunsigned long align;\n> > > +\tdma_addr_t paddr;\n> > > +\tvoid *buffptr;\n> > > +\tint ret = 0;\n> > > +\n> > > +\tskb = priv->rx.desc[entry].skbs;\n> > > +\tif (!skb) {\n> > > +\t\tskb = netdev_alloc_skb_ip_align(ndev,\n> > > +\t\t\t\t\t\tAVE_MAX_ETHFRAME + NET_SKB_PAD);\n> > > +\t\tif (!skb) {\n> > > +\t\t\tnetdev_err(ndev, \"can't allocate skb for Rx\\n\");\n> > > +\t\t\treturn -ENOMEM;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\t/* set disable to cmdsts */\n> > > +\tave_wdesc(ndev, AVE_DESCID_RX, entry, 0, AVE_STS_INTR | AVE_STS_OWN);\n> > > +\n> > > +\t/* align skb data for cache size */\n> > > +\talign = (unsigned long)skb_tail_pointer(skb) & (NET_SKB_PAD - 1);\n> > > +\talign = NET_SKB_PAD - align;\n> > > +\tskb_reserve(skb, align);\n> > > +\tbuffptr = (void *)skb_tail_pointer(skb);\n> > \n> > Are you positive you need this? Because by default, the networking stack\n> > will align to the maximum between your L1 cache line size and 64 bytes,\n> > which should be a pretty good alignment guarantee.\n> \n> Now if L1 cache line size is 128,\n> the skb buffer is also aligned to 128, isn't it?\n> So this code doesn't make sense.\n\nAlthough the above cache-alignment operation isn't necessary,\nwe should add the address adjustment because of the restriction of the hardware\nspecification.\n\nThe netdev_alloc_skb_ip_align() allocates the cache-aligned buffer\nand add 2 byte to skb->data by skb_reserve(skb, NET_IP_ALIGN).\nThen skb->data points to \"aligned address + 2 byte\".\n\nWhen we call dma_map_single() with skb->data, it might return the aligned address\nand there might not be 2 byte space.\n\nOn the other hand, according to the hardware specification,\nthe Rx buffer address set to the descriptor is assumed that:\n - the Rx address is 4 byte aligned,\n - the Rx address begins with 2 byte headroom, data will be put from (buffer+2).\n\nTherefore, to make headroom in front of returned address from ave_dma_map(),\nI think that the buffer address should be adjusted like that:\n\n    skb = netdev_alloc_skb_ip_align(ndev, AVE_MAX_ETHFRAME);\n\n    paddr = ave_dma_map(ndev, &priv->rx.desc[entry],\n\t\tskb->data - NET_IP_ALIGN,\n\t\tAVE_MAX_ETHFRAME + NET_IP_ALIGN, DMA_FROM_DEVICE);\n\n    ave_wdesc_addr(ndev, AVE_DESCID_RX, entry, 4, paddr);\n\nI'll apply the code to next patch.\n\nBTW, since the Tx buffer address doesn't have any restrictions, the adjustment\nlike this isn't necessary.\n\n\n> > > +\n> > > +\t/* enable clock */\n> > > +\tpriv->clk = devm_clk_get(dev, NULL);\n> > > +\tif (IS_ERR(priv->clk))\n> > > +\t\tpriv->clk = NULL;\n> > > +\tclk_prepare_enable(priv->clk);\n> > \n> > Same here with the clock, the block is clocked, so it can consume some\n> > amount of power, just do the necessary HW initialization with the clock\n> > enabled, then defer until ndo_open() before turning it back on.\n\nThere are a number of the functions that needs clock enabled and \"block reset\"\noperations, like mdiobus_register(), phy_connect(), and so on.\n\nI tried to move such functions to ndo_open() to defer clock enabled until ndo_open().\nHowever, the driver didn't work for some reasons of hardware restriction.\nI think it's hard to change this sequence.\n\n---\nBest Regards,\nKunihiko Hayashi","headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xybVS6wBvz9s5L\n\tfor <patchwork-incoming@ozlabs.org>;\n\tThu, 21 Sep 2017 22:28:00 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751629AbdIUM1v (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tThu, 21 Sep 2017 08:27:51 -0400","from mx.socionext.com ([202.248.49.38]:6956 \"EHLO mx.socionext.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751387AbdIUM1t (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tThu, 21 Sep 2017 08:27:49 -0400","from unknown (HELO iyokan-ex.css.socionext.com) ([172.31.9.54])\n\tby mx.socionext.com with ESMTP; 21 Sep 2017 21:27:48 +0900","from mail.mfilter.local (unknown [10.213.24.62])\n\tby iyokan-ex.css.socionext.com (Postfix) with ESMTP id 234C961128;\n\tThu, 21 Sep 2017 21:27:48 +0900 (JST)","from 172.31.9.53 (172.31.9.53) by m-FILTER with ESMTP;\n\tThu, 21 Sep 2017 21:27:48 +0900","from yuzu.css.socionext.com (yuzu [172.31.8.45])\n\tby iyokan.css.socionext.com (Postfix) with ESMTP id D258640864;\n\tThu, 21 Sep 2017 21:27:47 +0900 (JST)","from [127.0.0.1] (unknown [10.213.134.37])\n\tby yuzu.css.socionext.com (Postfix) with ESMTP id 94F1D120499;\n\tThu, 21 Sep 2017 21:27:47 +0900 (JST)"],"Date":"Thu, 21 Sep 2017 21:27:47 +0900","From":"Kunihiko Hayashi <hayashi.kunihiko@socionext.com>","To":"Florian Fainelli <f.fainelli@gmail.com>, <netdev@vger.kernel.org>","Subject":"Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet\n\tdriver","Cc":"\"David S. Miller\" <davem@davemloft.net>,\n\t\"Andrew Lunn\" <andrew@lunn.ch>, Rob Herring <robh+dt@kernel.org>,\n\tMark Rutland <mark.rutland@arm.com>,\n\t<linux-arm-kernel@lists.infradead.org>,\n\t<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,\n\tMasahiro Yamada <yamada.masahiro@socionext.com>,\n\tMasami Hiramatsu <masami.hiramatsu@linaro.org>,\n\tJassi Brar <jaswinder.singh@linaro.org>","In-Reply-To":"<20170911155555.6719.4A936039@socionext.com>","References":"<df4ec8b2-67cf-5dee-2600-88cffd1bcd11@gmail.com>\n\t<20170911155555.6719.4A936039@socionext.com>","Message-Id":"<20170921212746.9119.4A936039@socionext.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"US-ASCII\"","Content-Transfer-Encoding":"7bit","X-Mailer":"Becky! ver. 2.70 [ja]","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"}}]