From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751145AbdE3WTX (ORCPT ); Tue, 30 May 2017 18:19:23 -0400 Received: from mail-qk0-f193.google.com ([209.85.220.193]:34483 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750966AbdE3WTV (ORCPT ); Tue, 30 May 2017 18:19:21 -0400 Subject: Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume To: Leonard Crestez , Andrew Lunn , Andy Duan Cc: Johan Hovold , Alexandre Belloni , Philipp Zabel , Fabio Estevam , netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Shawn Guo References: <44ddeaeb-3e23-3d88-cc5c-9ed290507648@gmail.com> <1496182080.19669.11.camel@nxp.com> From: Florian Fainelli Message-ID: <30fb605c-675b-8458-aff7-36532635ccc2@gmail.com> Date: Tue, 30 May 2017 15:19:16 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <1496182080.19669.11.camel@nxp.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/30/2017 03:08 PM, Leonard Crestez wrote: > On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote: >> On 05/30/2017 10:34 AM, Leonard Crestez wrote: >>> These bits seem to be lost after a suspend/resume cycle so just set them >>> again. >>> >>> This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards. >>> >>> Signed-off-by: Leonard Crestez >>> --- >>> drivers/net/phy/micrel.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c >>> index 6a5fd18..c53ee17 100644 >>> --- a/drivers/net/phy/micrel.c >>> +++ b/drivers/net/phy/micrel.c >>> @@ -700,6 +700,9 @@ static int kszphy_suspend(struct phy_device *phydev) >>> >>> static int kszphy_resume(struct phy_device *phydev) >>> { >>> + struct kszphy_priv *priv = phydev->priv; >>> + int ret; >>> + >>> genphy_resume(phydev); >>> >>> /* Enable PHY Interrupts */ >>> @@ -709,6 +712,18 @@ static int kszphy_resume(struct phy_device *phydev) >>> phydev->drv->config_intr(phydev); >>> } >>> >>> + if (priv->rmii_ref_clk_sel) { >>> + ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val); >>> + if (ret) { >>> + phydev_err(phydev, >>> + "failed to set rmii reference clock\n"); >>> + return ret; >>> + } >>> + } >>> + >>> + if (priv->led_mode >= 0) >>> + kszphy_setup_led(phydev, priv->type->led_mode_reg, priv->led_mode); >> >> Should not we actually call kszphy_config_init() in order to restore >> broadcast and nand disable bits as well? > > I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and > NAND_TREE_ON is already off by the time it gets to linux. > > The bit that get lost seem to disappear just as the phy is resumed. I > added some prints and they look like this: > > PM: early resume of devices complete after 6.534 msecs > begin resume > 0x1F=0x8190 0x16=0x202 > after genphy_resume 0x1F=0x8100 0x16=0x202 > end > resume 0x1F=0x8190 0x16=0x202 OK, so it actually would not hurt to call config_init() again here, right? > >> If not, I would be more comfortable if we did create a specific function >> that takes care of setting the reference clock and LED mode. > > Ok, I can add a function called kszphy_config_reset() with a comment > explaining it's for bits lost on reset/resume. > > Or perhaps a better option would be to just save/restore the entire > 0x1F register? Register 0 through 15 are standardized, but those after that are not, and PHYs with a gazillion of registers already need to have a specific set of suspend/resume sequence due to their proprietary indirect register scheme, so we cannot quite come up with a generic function that would cache all 16 or 32 registers and flat out restore those. Similarly, having phy_resume() systematically calling into config_init() is a bit of a stretch and can be pretty inefficient. -- Florian