[{"id":1773742,"web_url":"http://patchwork.ozlabs.org/comment/1773742/","msgid":"<20170922163258.GA3470@lunn.ch>","list_archive_url":null,"date":"2017-09-22T16:32:58","subject":"Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core","submitter":{"id":13608,"url":"http://patchwork.ozlabs.org/api/people/13608/","name":"Andrew Lunn","email":"andrew@lunn.ch"},"content":"On Fri, Sep 22, 2017 at 12:17:50PM -0400, Vivien Didelot wrote:\n> bcm_sf2 is currently the only driver using the phy argument passed to\n> .port_enable. It resets the state machine if the phy has been hard\n> reset. This check is generic and can be moved to DSA core.\n> \n> \tdsa_port_set_state_now(p->dp, stp_state);\n> \n> -\tif (p->phy)\n> -\t\tphy_start(p->phy);\n> +\tif (phy) {\n> +\t\t/* If phy_stop() has been called before, phy will be in\n> +\t\t * halted state, and phy_start() will call resume.\n> +\t\t *\n> +\t\t * The resume path does not configure back autoneg\n> +\t\t * settings, and since the internal phy may have been\n> +\t\t * hard reset, we need to reset the state machine also.\n> +\t\t */\n> +\t\tphy->state = PHY_READY;\n> +\t\tphy_init_hw(phy);\n> +\t\tphy_start(phy);\n> +\t}\n\nHi Vivien\n\nIf this is generic, why is it needed at all here? Shouldn't this\nactually by in phylib?\n\nFlorian ?\n\n Andrew","headers":{"Return-Path":"","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=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzJtz3Yy7z9s7v\n\tfor ;\n\tSat, 23 Sep 2017 02:33:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752544AbdIVQdG (ORCPT );\n\tFri, 22 Sep 2017 12:33:06 -0400","from vps0.lunn.ch ([185.16.172.187]:52611 \"EHLO vps0.lunn.ch\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752097AbdIVQdD (ORCPT );\n\tFri, 22 Sep 2017 12:33:03 -0400","from andrew by vps0.lunn.ch with local (Exim 4.84_2)\n\t(envelope-from )\n\tid 1dvQss-00016h-T4; Fri, 22 Sep 2017 18:32:58 +0200"],"Date":"Fri, 22 Sep 2017 18:32:58 +0200","From":"Andrew Lunn ","To":"Vivien Didelot ","Cc":"netdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tkernel@savoirfairelinux.com, \"David S. Miller\" ,\n\tFlorian Fainelli ","Subject":"Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core","Message-ID":"<20170922163258.GA3470@lunn.ch>","References":"<20170922161753.19563-1-vivien.didelot@savoirfairelinux.com>\n\t<20170922161753.19563-2-vivien.didelot@savoirfairelinux.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170922161753.19563-2-vivien.didelot@savoirfairelinux.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1773766,"web_url":"http://patchwork.ozlabs.org/comment/1773766/","msgid":"<06cb966d-9066-bc3d-13f0-c7a34ebf9878@gmail.com>","list_archive_url":null,"date":"2017-09-22T16:52:34","subject":"Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core","submitter":{"id":2800,"url":"http://patchwork.ozlabs.org/api/people/2800/","name":"Florian Fainelli","email":"f.fainelli@gmail.com"},"content":"On 09/22/2017 09:17 AM, Vivien Didelot wrote:\n> bcm_sf2 is currently the only driver using the phy argument passed to\n> .port_enable. It resets the state machine if the phy has been hard\n> reset. This check is generic and can be moved to DSA core.\n\nThis is completely specific to bcm_sf2 because it does call\nbcm_sf2_gphy_enable_set() which performs a HW reset of the PHY, you\ncan't move this to the generic portion of net/dsa/slave.c. NACK.\n\n> \n> Signed-off-by: Vivien Didelot \n> ---\n> drivers/net/dsa/bcm_sf2.c | 16 +---------------\n> net/dsa/slave.c | 15 +++++++++++++--\n> 2 files changed, 14 insertions(+), 17 deletions(-)\n> \n> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c\n> index 898d5642b516..ad96b9725a2c 100644\n> --- a/drivers/net/dsa/bcm_sf2.c\n> +++ b/drivers/net/dsa/bcm_sf2.c\n> @@ -184,22 +184,8 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,\n> \tcore_writel(priv, reg, CORE_PORT_TC2_QOS_MAP_PORT(port));\n> \n> \t/* Re-enable the GPHY and re-apply workarounds */\n> -\tif (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1) {\n> +\tif (priv->int_phy_mask & 1 << port && priv->hw_params.num_gphy == 1)\n> \t\tbcm_sf2_gphy_enable_set(ds, true);\n> -\t\tif (phy) {\n> -\t\t\t/* if phy_stop() has been called before, phy\n> -\t\t\t * will be in halted state, and phy_start()\n> -\t\t\t * will call resume.\n> -\t\t\t *\n> -\t\t\t * the resume path does not configure back\n> -\t\t\t * autoneg settings, and since we hard reset\n> -\t\t\t * the phy manually here, we need to reset the\n> -\t\t\t * state machine also.\n> -\t\t\t */\n> -\t\t\tphy->state = PHY_READY;\n> -\t\t\tphy_init_hw(phy);\n> -\t\t}\n> -\t}\n> \n> \t/* Enable MoCA port interrupts to get notified */\n> \tif (port == priv->moca_port)\n> diff --git a/net/dsa/slave.c b/net/dsa/slave.c\n> index 02ace7d462c4..606812160fd5 100644\n> --- a/net/dsa/slave.c\n> +++ b/net/dsa/slave.c\n> @@ -72,6 +72,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev)\n> static int dsa_slave_open(struct net_device *dev)\n> {\n> \tstruct dsa_slave_priv *p = netdev_priv(dev);\n> +\tstruct phy_device *phy = p->phy;\n> \tstruct dsa_port *dp = p->dp;\n> \tstruct dsa_switch *ds = dp->ds;\n> \tstruct net_device *master = dsa_master_netdev(p);\n> @@ -106,8 +107,18 @@ static int dsa_slave_open(struct net_device *dev)\n> \n> \tdsa_port_set_state_now(p->dp, stp_state);\n> \n> -\tif (p->phy)\n> -\t\tphy_start(p->phy);\n> +\tif (phy) {\n> +\t\t/* If phy_stop() has been called before, phy will be in\n> +\t\t * halted state, and phy_start() will call resume.\n> +\t\t *\n> +\t\t * The resume path does not configure back autoneg\n> +\t\t * settings, and since the internal phy may have been\n> +\t\t * hard reset, we need to reset the state machine also.\n> +\t\t */\n> +\t\tphy->state = PHY_READY;\n> +\t\tphy_init_hw(phy);\n> +\t\tphy_start(phy);\n> +\t}\n> \n> \treturn 0;\n> \n>","headers":{"Return-Path":"","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=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"DvVTgcov\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzKKd341gz9t3Z\n\tfor ;\n\tSat, 23 Sep 2017 02:52:53 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752298AbdIVQwm (ORCPT );\n\tFri, 22 Sep 2017 12:52:42 -0400","from mail-qk0-f193.google.com ([209.85.220.193]:36026 \"EHLO\n\tmail-qk0-f193.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751909AbdIVQwk (ORCPT\n\t); Fri, 22 Sep 2017 12:52:40 -0400","by mail-qk0-f193.google.com with SMTP id i14so991372qke.3;\n\tFri, 22 Sep 2017 09:52:40 -0700 (PDT)","from [10.112.156.244] ([192.19.255.250])\n\tby smtp.googlemail.com with ESMTPSA id\n\ty31sm188525qta.83.2017.09.22.09.52.36\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 22 Sep 2017 09:52:37 -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=vcBZxgSvdDdL+d3PrOhGpDXvEzt1EZp4p02uamRF36I=;\n\tb=DvVTgcovR/yiOlGToAGLxOtQieGiTv2bXQ86hqIa0ifdMVWl7Nj6JjmHrqmIS7Vfv5\n\txOb0wftDs5Vh8kTgmE3iPi/5ZOE0X+E6qSfyZFiGSAGZEryneCkOmIfZWV2Jou46zM1Y\n\tioKDWJX6eWWKsEvOyEydxXmFr0tws5mqAINfvNvwmiqMrO9d1966lW6vqZ4D4BB5I4yW\n\t7qxf3uzfLZN2NANc6ZaBqTsrPh/L2jDGebOwAEbtH64fAy4SwO2kooz3On3tJ1Lm3uBu\n\tC4HJII2+u+tk3Y1LNKlRxXFQC9TxIoIV4hFPrkD/12eN3HPD6eS2THJ5gYukIIzE+d01\n\tL4mg==","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=vcBZxgSvdDdL+d3PrOhGpDXvEzt1EZp4p02uamRF36I=;\n\tb=YFipthi+ud6WCMe2Hi1R7XVdPVCqW2wZkN3a2LKaTi8aPeBKAFdwA2XiZkyZewyNYk\n\trneEEQatIPbyAP/u1DvYOTXE3xxnjG3MudOMghEee1r6C1zz0nb/Annc82YCyMeuofSM\n\tT/Qn5s+zIlzNT2kMBTfAW2ZJ+uYDxmws71gj0nIkuuobEpU6TgL5UgVdoqOOeO5MN9OE\n\t+ineiBs/Och4Np8bzVlX3HG3tYSEEjaX7snRuYniH69JJESdlY6jANK/9/NtlGpp+PCC\n\tJxrT/+aFxI0hbHwzOEwGY8MK0x3QSdqb8yOB9bhSPaP25CnXSJXN/6aZWi2P2sm1TZc/\n\t6rGw==","X-Gm-Message-State":"AHPjjUju+o1NflWZ+ylY5pgMXOdC/n9RwDxk2nA0+gaY1RHmtNT9CiWz\n\tdstamuciDXunuXSRi70qq8GfFtfQ","X-Google-Smtp-Source":"AOwi7QDkxpJUoKjFeYzCImZOMJjrI8IaXFtT7IudEp22LL1FiwctNE46lAnR2lY9dFcHiiHqVfp6UQ==","X-Received":"by 10.55.15.170 with SMTP id 42mr8881988qkp.262.1506099159863;\n\tFri, 22 Sep 2017 09:52:39 -0700 (PDT)","Subject":"Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core","To":"Vivien Didelot ,\n\tnetdev@vger.kernel.org","Cc":"linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com,\n\t\"David S. Miller\" , Andrew Lunn ","References":"<20170922161753.19563-1-vivien.didelot@savoirfairelinux.com>\n\t<20170922161753.19563-2-vivien.didelot@savoirfairelinux.com>","From":"Florian Fainelli ","Message-ID":"<06cb966d-9066-bc3d-13f0-c7a34ebf9878@gmail.com>","Date":"Fri, 22 Sep 2017 09:52:34 -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":"<20170922161753.19563-2-vivien.didelot@savoirfairelinux.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1773772,"web_url":"http://patchwork.ozlabs.org/comment/1773772/","msgid":"","list_archive_url":null,"date":"2017-09-22T16:58:00","subject":"Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core","submitter":{"id":2800,"url":"http://patchwork.ozlabs.org/api/people/2800/","name":"Florian Fainelli","email":"f.fainelli@gmail.com"},"content":"On 09/22/2017 09:32 AM, Andrew Lunn wrote:\n> On Fri, Sep 22, 2017 at 12:17:50PM -0400, Vivien Didelot wrote:\n>> bcm_sf2 is currently the only driver using the phy argument passed to\n>> .port_enable. It resets the state machine if the phy has been hard\n>> reset. This check is generic and can be moved to DSA core.\n>> \n>> \tdsa_port_set_state_now(p->dp, stp_state);\n>> \n>> -\tif (p->phy)\n>> -\t\tphy_start(p->phy);\n>> +\tif (phy) {\n>> +\t\t/* If phy_stop() has been called before, phy will be in\n>> +\t\t * halted state, and phy_start() will call resume.\n>> +\t\t *\n>> +\t\t * The resume path does not configure back autoneg\n>> +\t\t * settings, and since the internal phy may have been\n>> +\t\t * hard reset, we need to reset the state machine also.\n>> +\t\t */\n>> +\t\tphy->state = PHY_READY;\n>> +\t\tphy_init_hw(phy);\n>> +\t\tphy_start(phy);\n>> +\t}\n> \n> Hi Vivien\n> \n> If this is generic, why is it needed at all here? Shouldn't this\n> actually by in phylib?\n\nThis does not belong in the core logic within net/dsa/slave.c. The\nreason why this is necessary here is because we are doing a HW-based\nreset of the PHY, as the comment explains this is specific to how the HW\nworks. There may be a cleaner solution to this problem, but in any case,\nI don't think other drivers should inherit that logic.","headers":{"Return-Path":"","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=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"TF1I159u\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xzKRv3Plwz9sP1\n\tfor ;\n\tSat, 23 Sep 2017 02:58:19 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752453AbdIVQ6H (ORCPT );\n\tFri, 22 Sep 2017 12:58:07 -0400","from mail-qt0-f196.google.com ([209.85.216.196]:37265 \"EHLO\n\tmail-qt0-f196.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752055AbdIVQ6G (ORCPT\n\t); Fri, 22 Sep 2017 12:58:06 -0400","by mail-qt0-f196.google.com with SMTP id u48so1018810qtc.4;\n\tFri, 22 Sep 2017 09:58:06 -0700 (PDT)","from [10.112.156.244] ([192.19.255.250])\n\tby smtp.googlemail.com with ESMTPSA id\n\tx55sm192848qth.91.2017.09.22.09.58.03\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 22 Sep 2017 09:58:04 -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=X7EcjWDbWt+xG4rWKwxUMQCUPdwNBywnKOYtSF2HqCE=;\n\tb=TF1I159uBZXYiDy+p6YFLg7iEB+kDvpGaITGq8c0jAF3LNr4jyg7cDdBG1YZDVCkEb\n\tikDJwvMmBSxlQg76edOFVIji10sAteyhp7Qtql5ArLlz+tnSu1uq6UalVkBCJyXaoA63\n\tS8TPNBKxm6ZEfg8AyxZghOxDI/Vl4AD/IsVNNbrXByANg6S+ubKEU5M6bWB/skQMDPpm\n\til4JdysA8vQA+N2dS11P+1y0iTHr6W6yMd+FLPnxeF5SxUyXLJ2GDNGfZaUNA2zzMndI\n\tqgpWMrMFD5ImYpApUPo1kwTXNttH32RwmzkX3NgAcqWMe0bOrGj6WbpWIYmjuJ3GKqbv\n\tHVYQ==","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=X7EcjWDbWt+xG4rWKwxUMQCUPdwNBywnKOYtSF2HqCE=;\n\tb=Z6McF0RZzEerL+eMIkFx8SNiP9fT5VXw0hlcQXQJH8G52MHWyk2ZKHLQv5RN0/MYHm\n\tJ/Bp5Iz/uwP4xhd6ILfogIoxCqGNdWXvY1ELt+ESRTTtf987/V76jpLfThiVYCLSVeYM\n\t9CXecOJnM3FtzUhrpElJMMFFxzPz4v8LSflraZM0KYv4WvNnzw6pS0Wm5drDvK5MbZKM\n\tHRcS0dPu1zgdMmIfxvBGDbbb5a+NlhBAIHmwUSoeojcT1XRot0Tt2XScOZgM9PXBpO48\n\trUTZJbs02j06zic97/IzxlOJDf+nUSjXXwuA8LyY27wJIqtdtxY31hIumjSP0YbYk1eU\n\tI89Q==","X-Gm-Message-State":"AHPjjUg+M93R/zEMkA8hu4fDDT8ywVjPdXI02Xbhs/G8peQY8QTmCLVj\n\tbBsJWrucAMqwZe1QeF1eZCY=","X-Google-Smtp-Source":"AOwi7QC+ejmC0Hg7J54CWPghkXyLRlTopzWg9q75NYXakwPLTPOYHvYgdv9TVpjd5gIm0nZkB+RrIQ==","X-Received":"by 10.237.61.108 with SMTP id h41mr9060671qtf.213.1506099485610; \n\tFri, 22 Sep 2017 09:58:05 -0700 (PDT)","Subject":"Re: [PATCH net-next 1/4] net: dsa: move up phy enabling in core","To":"Andrew Lunn ,\n\tVivien Didelot ","Cc":"netdev@vger.kernel.org, linux-kernel@vger.kernel.org,\n\tkernel@savoirfairelinux.com, \"David S. Miller\" ","References":"<20170922161753.19563-1-vivien.didelot@savoirfairelinux.com>\n\t<20170922161753.19563-2-vivien.didelot@savoirfairelinux.com>\n\t<20170922163258.GA3470@lunn.ch>","From":"Florian Fainelli ","Message-ID":"","Date":"Fri, 22 Sep 2017 09:58:00 -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":"<20170922163258.GA3470@lunn.ch>","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":"","X-Mailing-List":"netdev@vger.kernel.org"}}]