From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks Date: Tue, 23 Oct 2018 11:58:29 +0100 Message-ID: <20181023105828.GN30658@n2100.armlinux.org.uk> References: <20181022122839.GB24112@lunn.ch> <8114f345-0efb-9a64-b867-2cfe2fba09ab@synopsys.com> <20181022154851.GA1492@n2100.armlinux.org.uk> <943a6194-001a-d6a0-84ba-b93b728ce64b@synopsys.com> <20181023102023.GM30658@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Florian Fainelli , Andrew Lunn , netdev@vger.kernel.org, "David S. Miller" , Joao Pinto To: Jose Abreu Return-path: Received: from pandora.armlinux.org.uk ([78.32.30.218]:54316 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726970AbeJWTVh (ORCPT ); Tue, 23 Oct 2018 15:21:37 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote: > On 23-10-2018 11:20, Russell King - ARM Linux wrote: > > I have no idea what you're proposing there - your patches weren't copied > > to me. > > They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that > without this remote end doesn't detect link is down ... > > If it's okay for Generic 10G driver I can submit only this and > manually reset PHY in stmmac driver so that I don't need to > implement custom PHY driver ... > BTW, I just found out currently Generic 10G Driver is broken > without patch 4/4 of this series [1] > > [1] https://patchwork.ozlabs.org/patch/987570/ How is it broken - what are the symptoms? The generic 10G driver is bound not via the normal bus matching and phy_bus_match(), but via a manual bind in phy_attach_direct(). This calls the probe function, which is phy_probe(), which initialises the supported/advertising to the driver's features (which as you note are zero.) However, phy_attach_direct() goes on to call phy_init_hw(), which calls the config_init() method. The config_init() method initialises the supported/advertising masks to 10GbaseT. This is (partly) what I refer to when I say that the generic 10G support is crippled - it only supports this single speed and media. So the supported/advertising masks should be forced to only 10GbaseT at the completion of phy_attach_direct(). The "generic 10G" support doesn't do autonegotiation, configuration or link mode forcing. It only assumes 10GbaseT is supported, and only checks for the "link up" bits. It isn't like the non-10G generic PHY support due to history - it was added in 2014 by Andy Fleming (see 124059fd53af). BTW, your patch 1 is wrong as well (introducing phy_update_link()). You don't take account that a 10G phy may have alternative ways of reading the link (like 88x3310 does, because it has multiple instances of AN/PCS/PHYXS at 1k offsets.) All the gen10g_* functions are legacy functions for the crippled "generic" 10G support. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up