[{"id":1225,"web_url":"http://patchwork.ozlabs.org/comment/1225/","msgid":"<2acbd3e40809181547m3ed512b8jd744f4b8a8a3bd59@mail.gmail.com>","list_archive_url":null,"date":"2008-09-18T22:47:03","subject":"Re: [PATCH 3/3] mv643xx_eth: convert to phylib","submitter":{"id":281,"url":"http://patchwork.ozlabs.org/api/people/281/","name":"Andy Fleming","email":"afleming@gmail.com"},"content":">\n> I'll make this function:\n>\n> static void phy_init(struct mv643xx_eth_private *mp, int speed, int duplex)\n> {\n> struct phy_device *phy = mp->phy;\n>\n> phy_reset(mp);\n>\n> phy_attach(mp->dev, phy->dev.bus_id, 0, PHY_INTERFACE_MODE_GMII);\n>\n> phy->state = PHY_HALTED;\n>\n> if (speed == 0) {\n> phy->autoneg = AUTONEG_ENABLE;\n> phy->speed = 0;\n> phy->duplex = 0;\n> phy->advertising = phy->supported | ADVERTISED_Autoneg;\n> } else {\n> phy->autoneg = AUTONEG_DISABLE;\n> phy->advertising = 0;\n> phy->speed = speed;\n> phy->duplex = duplex;\n> }\n> phy_start_aneg(phy);\n> }\n>\n> OK? (PHY_HALTED because that seems to be the way to stop the state\n> machine from running at all. Or should I call phy_stop() once?)\n\nYou don't need to disable the state machine. With phy_attach, it\nwon't start unless you call phy_start_machine(). So the state changes\nwon't cause the state machine to run. If the state information is\ninterfering with things, let me know. We might have to create a state\nfor \"state machine is not actually running\".\n\n>\n>\n>> You will, however, need to\n>> figure out how best to read out the status for the PHY, and how to\n>> know when autonegotiation is done. I'm assuming that your controller\n>> tracks that information, and that you can manually pull that\n>> information from local register space (rather than the SMI bus).\n>\n> I don't really need to access it all during normal operation -- the\n> controller just reads the link up/down and speed and duplex bits from\n> the PHY automatically and then configures itself accordingly. The\n> only time I need to access this information is when someone asks for\n> it via ethtool (or if I want to print a link status message). I.e. I\n> don't need any state machine, be it phylib's or my own.\n\nRight, but doesn't your driver react to the link going up and down by\nchanging the carrier state? If so, then it costs nothing to cache\nthat state in the phydev->link, and then a request for link status can\njust return phydev->link. Those are decisions you can make, though,\nas I'm not familiar with your device and your driver.\n\nIf you could check those things, you can submit the patch with:\n\nAcked-by: Andy Fleming \nAndy","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Received":["from vger.kernel.org (vger.kernel.org [209.132.176.167])\n\tby ozlabs.org (Postfix) with ESMTP id 525B6DE00D\n\tfor ;\n\tFri, 19 Sep 2008 08:47:12 +1000 (EST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755007AbYIRWrH (ORCPT );\n\tThu, 18 Sep 2008 18:47:07 -0400","(majordomo@vger.kernel.org) by vger.kernel.org id S1754828AbYIRWrG\n\t(ORCPT ); Thu, 18 Sep 2008 18:47:06 -0400","from wf-out-1314.google.com ([209.85.200.171]:65420 \"EHLO\n\twf-out-1314.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754741AbYIRWrF (ORCPT\n\t); Thu, 18 Sep 2008 18:47:05 -0400","by wf-out-1314.google.com with SMTP id 27so165442wfd.4\n\tfor ; Thu, 18 Sep 2008 15:47:04 -0700 (PDT)","by 10.143.155.7 with SMTP id h7mr1688359wfo.16.1221778023617;\n\tThu, 18 Sep 2008 15:47:03 -0700 (PDT)","by 10.142.88.5 with HTTP; Thu, 18 Sep 2008 15:47:03 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\n\th=domainkey-signature:received:received:message-id:date:from:to\n\t:subject:cc:in-reply-to:mime-version:content-type\n\t:content-transfer-encoding:content-disposition:references;\n\tbh=5/wwSo5oCnsN9XQqmt4OXwwyRWCmW3TfLgDP7j1GCnI=;\n\tb=VXW2cgCWVRBWkgLBJ43xTXROhFJrYloCQA0aNP2rbWEd8NSvpNNsIEhPnypIPxMlGT\n\tVQeOvVij66+pFEu0CDL1o4oIlI8dSneOU5ZR1MNAxo4aaFf4IjXZ5g5yuSoqUpv6AK6a\n\tntNAeovfzs5Diz56WzuXtL/PqxHplsXe+E7lU=","DomainKey-Signature":"a=rsa-sha1; c=nofws; d=gmail.com; s=gamma;\n\th=message-id:date:from:to:subject:cc:in-reply-to:mime-version\n\t:content-type:content-transfer-encoding:content-disposition\n\t:references;\n\tb=ACvE9NcASVdGPn4GrpL9GfCX0u6OoJIsofHwaCnn4wknzgt+8RAIRNe0LymHdHkTeB\n\tFq/fI/mKeTOhS1WuDjUySPIpS48pTWTwdpJ2eM1trfH4AiJ7P8A3IatrVIAC2xYFS8H2\n\tO85hS/4//WXpf2Mqcwx84WiWMcwgxA6NxUB9w=","Message-ID":"<2acbd3e40809181547m3ed512b8jd744f4b8a8a3bd59@mail.gmail.com>","Date":"Thu, 18 Sep 2008 17:47:03 -0500","From":"\"Andy Fleming\" ","To":"\"Lennert Buytenhek\" ","Subject":"Re: [PATCH 3/3] mv643xx_eth: convert to phylib","Cc":"\"Andy Fleming\" , netdev@vger.kernel.org","In-Reply-To":"<20080918124428.GB9157@xi.wantstofly.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=ISO-8859-1","Content-Transfer-Encoding":"7bit","Content-Disposition":"inline","References":"<1220448012-20347-1-git-send-email-buytenh@wantstofly.org>\n\t<1220448012-20347-4-git-send-email-buytenh@wantstofly.org>\n\t\n\t<20080918124428.GB9157@xi.wantstofly.org>","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1431,"web_url":"http://patchwork.ozlabs.org/comment/1431/","msgid":"<20080919175256.GB23288@xi.wantstofly.org>","list_archive_url":null,"date":"2008-09-19T17:52:56","subject":"Re: [PATCH 3/3] mv643xx_eth: convert to phylib","submitter":{"id":94,"url":"http://patchwork.ozlabs.org/api/people/94/","name":"Lennert Buytenhek","email":"buytenh@wantstofly.org"},"content":"On Thu, Sep 18, 2008 at 05:47:03PM -0500, Andy Fleming wrote:\n\n> > I'll make this function:\n> >\n> > static void phy_init(struct mv643xx_eth_private *mp, int speed, int duplex)\n> > {\n> > struct phy_device *phy = mp->phy;\n> >\n> > phy_reset(mp);\n> >\n> > phy_attach(mp->dev, phy->dev.bus_id, 0, PHY_INTERFACE_MODE_GMII);\n> >\n> > phy->state = PHY_HALTED;\n> >\n> > if (speed == 0) {\n> > phy->autoneg = AUTONEG_ENABLE;\n> > phy->speed = 0;\n> > phy->duplex = 0;\n> > phy->advertising = phy->supported | ADVERTISED_Autoneg;\n> > } else {\n> > phy->autoneg = AUTONEG_DISABLE;\n> > phy->advertising = 0;\n> > phy->speed = speed;\n> > phy->duplex = duplex;\n> > }\n> > phy_start_aneg(phy);\n> > }\n> >\n> > OK? (PHY_HALTED because that seems to be the way to stop the state\n> > machine from running at all. Or should I call phy_stop() once?)\n> \n> You don't need to disable the state machine. With phy_attach, it\n> won't start unless you call phy_start_machine(). So the state changes\n> won't cause the state machine to run. If the state information is\n> interfering with things, let me know. We might have to create a state\n> for \"state machine is not actually running\".\n\nOK. From a look at the state handling, trying to make sure that it's\nin HALTED seemed like the safe option, but as you say, just the one\ncall to phy_start_aneg() shouldn't mess things up -- so I'll get rid\nof that.\n\n\n> >> You will, however, need to\n> >> figure out how best to read out the status for the PHY, and how to\n> >> know when autonegotiation is done. I'm assuming that your controller\n> >> tracks that information, and that you can manually pull that\n> >> information from local register space (rather than the SMI bus).\n> >\n> > I don't really need to access it all during normal operation -- the\n> > controller just reads the link up/down and speed and duplex bits from\n> > the PHY automatically and then configures itself accordingly. The\n> > only time I need to access this information is when someone asks for\n> > it via ethtool (or if I want to print a link status message). I.e. I\n> > don't need any state machine, be it phylib's or my own.\n> \n> Right, but doesn't your driver react to the link going up and down by\n> changing the carrier state? If so, then it costs nothing to cache\n> that state in the phydev->link, and then a request for link status can\n> just return phydev->link. Those are decisions you can make, though,\n> as I'm not familiar with your device and your driver.\n\nOr, I can just return !!netif_carrier_ok(dev), since that'll always\nbe the same thing in this case.\n\n\n> If you could check those things, you can submit the patch with:\n> \n> Acked-by: Andy Fleming \n\nThanks for the review!","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Received":["from vger.kernel.org (vger.kernel.org [209.132.176.167])\n\tby ozlabs.org (Postfix) with ESMTP id 14442DE0A2\n\tfor ;\n\tSat, 20 Sep 2008 03:53:09 +1000 (EST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751309AbYISRxE (ORCPT );\n\tFri, 19 Sep 2008 13:53:04 -0400","(majordomo@vger.kernel.org) by vger.kernel.org id S1751498AbYISRxB\n\t(ORCPT ); Fri, 19 Sep 2008 13:53:01 -0400","from xi.wantstofly.org ([83.160.184.112]:54076 \"EHLO\n\txi.wantstofly.org\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751309AbYISRxA (ORCPT\n\t); Fri, 19 Sep 2008 13:53:00 -0400","by xi.wantstofly.org (Postfix, from userid 500)\n\tid 9B2A07FAC3; Fri, 19 Sep 2008 19:52:56 +0200 (CEST)"],"Date":"Fri, 19 Sep 2008 19:52:56 +0200","From":"Lennert Buytenhek ","To":"Andy Fleming ","Cc":"Andy Fleming , netdev@vger.kernel.org","Subject":"Re: [PATCH 3/3] mv643xx_eth: convert to phylib","Message-ID":"<20080919175256.GB23288@xi.wantstofly.org>","References":"<1220448012-20347-1-git-send-email-buytenh@wantstofly.org>\n\t<1220448012-20347-4-git-send-email-buytenh@wantstofly.org>\n\t\n\t<20080918124428.GB9157@xi.wantstofly.org>\n\t<2acbd3e40809181547m3ed512b8jd744f4b8a8a3bd59@mail.gmail.com>","Mime-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<2acbd3e40809181547m3ed512b8jd744f4b8a8a3bd59@mail.gmail.com>","User-Agent":"Mutt/1.4.2.2i","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]