From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume Date: Tue, 5 Jun 2018 21:39:37 +0200 Message-ID: References: <8fe93623-9d05-6182-fe5f-da0bd32bae0b@gmail.com> <20180523220418.GB5128@lunn.ch> <5b9eceab-35b4-922c-7410-d8968d8364c7@gmail.com> <20180530203512.GA16286@lunn.ch> <1d79c2ae-6f63-694f-6c63-6369c854de69@gmail.com> <20180531183040.GA7378@lunn.ch> <20180601001043.GA2303@lunn.ch> <28a8846a-f7a7-3266-3aec-7f58fe8dac72@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Florian Fainelli , David Miller , "netdev@vger.kernel.org" To: Andrew Lunn Return-path: Received: from mail-wm0-f65.google.com ([74.125.82.65]:35807 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751816AbeFETjp (ORCPT ); Tue, 5 Jun 2018 15:39:45 -0400 Received: by mail-wm0-f65.google.com with SMTP id j15-v6so7395632wme.0 for ; Tue, 05 Jun 2018 12:39:45 -0700 (PDT) In-Reply-To: <28a8846a-f7a7-3266-3aec-7f58fe8dac72@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 02.06.2018 22:27, Heiner Kallweit wrote: > On 01.06.2018 02:10, Andrew Lunn wrote: >>> Configuring the different WoL options isn't handled by writing to >>> the PHY registers but by writing to chip / MAC registers. >>> Therefore phy_suspend() isn't able to figure out whether WoL is >>> enabled or not. Only the parent has the full picture. >> >> Hi Heiner >> >> I think you need to look at your different runtime PM domains. If i >> understand the code right, you runtime suspend if there is no >> link. But for this to work correctly, your PHY needs to keep working. >> You also cannot assume all accesses to the PHY go via the MAC. Some >> calls will go direct to the PHY, and they can trigger MDIO bus >> accesses. So i think you need two runtime PM domains. MAC and MDIO >> bus. Maybe just the pll? An MDIO bus is a device, so it can have its >> on PM callbacks. It is not clear what you need to resume in order to >> make MDIO work. >> > Thanks for your comments! > The actual problem is quite small: I get an error at MDIO suspend, > the PHY however is suspended later by the driver's suspend callback > anyway. Because the problem is small I'm somewhat reluctant to > consider bigger changes like introducing different PM domains. > > Primary reason for the error is that the network chip is in PCI D3hot > at that moment. In addition to that for some of the chips supported by > the driver also MDIO-relevant PLL's might be disabled. > > By the way: > When checking PM handling for PHY/MDIO I stumbled across something > that can be improved IMO, I'll send a patch for your review. > I experimented a little and with adding Runtime PM to MDIO bus and PHY device I can make it work: PHY runtime-resumes before entering suspend and resumes its parent (MDIO bus) which then resumes its parent (PCI device). However this needs quite some code and is hard to read / understand w/o reading through this mail thread. And in general I still have doubts this is the right way. Let's consider the following scenario: A network driver configures WoL in its suspend callback (incl. setting the device to wakeup-enabled). The suspend callback of the PHY is called before this and therefore has no clue that WoL will be configured a little later, with the consequence that it will do an unsolicited power-down. The network driver then has to detect this and power-up the PHY again. This doesn't seem to make much sense and still my best idea is to establish a mechanism that a device can state: I orchestrate PM of my children. Heiner >> It might also help if you do the phy_connect in .ndo_open and >> disconnect in .ndo_stop. This is a common pattern in drivers. But some >> also do it is probe and remove. >> > Thanks for the hint. I will move phy_connect_direct accordingly. > >> Andrew >> > Heiner >