From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0DA56F506CF for ; Mon, 16 Mar 2026 13:14:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=DrTshH/5XD1DwX0wXvoMVhK/9Oqc9g5Mye/0IL4m/yc=; b=XJSTBE8bPNUAp0+IIXOFD/yR4r cO0rDb5c7c1zgVbf3xNLvuqi4Xx+iaNgxMuZ+2bwAV/0bT4C7kF3a4tqYDTiUWx/VWgpyiOrQjCSa 9EtLxQQqo89HRczimqUyFLVjfSqs44s2b52aCDoURBcNtyKcvpBmP6Y5iKckgVAfRxmHN0wHnVGz8 ramTREBsH5gNiQLLmuSnHNJyka6Cew9RjNdu6Dv5sWksGqdkycsG/Y19QYaWUnd3k3+2Il2qb5Lh4 xPmCgehGlbxJLkPhW8VBvL0vu+JJP64HZ+A2IqalhTmrYhX5cCkV64B6TQppCod5TkfgEk6TtMiu+ EORWJqLw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1w27mR-000000043WZ-0F4Y; Mon, 16 Mar 2026 13:14:47 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:32c8:5054:ff:fe00:142]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1w27mN-000000043Vr-1r1U for linux-arm-kernel@lists.infradead.org; Mon, 16 Mar 2026 13:14:45 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=DrTshH/5XD1DwX0wXvoMVhK/9Oqc9g5Mye/0IL4m/yc=; b=LrceKKVuKrubslp3vHydqm6i8p 1P/v3Oa8Q70B1SvOBLekVnJBGKwzboYjMm65GO1N8LbHLkTrHDxlIqTxWhYzAZC7USrdTq23QSJmd Anu27vbAtIhVhlhVj/66jIN85OLJxGQOYlQdq3bxAH3G9FgNvqQxMZLL7rOuYF9dg6/x+bIRU4YHf SBrRwvdtGH+99ATjmbpGfp8RWWY24FnGRXQSzbt9KnquOnBd9sAX/wtqTKyoggQyXME6KXdK4aOgq ltezR7567RM5hY3G/yphScuJYe3ra0DOffk7WbdaEdfUpLx1CrfISHdHLDSb2UK/76cU/lRiRp5kY A0T88G9w==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:44540) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.98.2) (envelope-from ) id 1w27mB-000000003pQ-1oCh; Mon, 16 Mar 2026 13:14:31 +0000 Received: from linux by shell.armlinux.org.uk with local (Exim 4.98.2) (envelope-from ) id 1w27m8-0000000056Y-1GC9; Mon, 16 Mar 2026 13:14:28 +0000 Date: Mon, 16 Mar 2026 13:14:28 +0000 From: "Russell King (Oracle)" To: Christophe ROULLIER Cc: Linus Walleij , Andrew Lunn , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , Alexandre Torgue , antonio.borneo@foss.st.com, Maxime Chevallier , Vladimir Oltean , netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/2] net: stmmac: fix pinctrl management during suspend/resume Message-ID: References: <20260313105718.359614-1-christophe.roullier@foss.st.com> <20260313105718.359614-2-christophe.roullier@foss.st.com> <4de50fb9-35e6-48e7-8111-c5a94099d4f7@foss.st.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4de50fb9-35e6-48e7-8111-c5a94099d4f7@foss.st.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260316_061443_631510_32E95EC3 X-CRM114-Status: GOOD ( 42.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Mar 16, 2026 at 11:01:02AM +0100, Christophe ROULLIER wrote: > Hi Russell, Linus, All, > > Le 16/03/2026 à 10:03, Russell King (Oracle) a écrit : > > On Sat, Mar 14, 2026 at 12:37:19AM +0000, Russell King (Oracle) wrote: > > > On Sat, Mar 14, 2026 at 12:44:56AM +0100, Linus Walleij wrote: > > > > On Fri, Mar 13, 2026 at 12:08 PM Russell King (Oracle) > > > > wrote: > > > > > On Fri, Mar 13, 2026 at 11:57:16AM +0100, Christophe Roullier wrote: > > > > > > In the deepest low-power modes, the pinctrl configuration is lost > > > > > > and is never restored if the interface is down. > > > > > > This commit ensures that the pinctrl state is set in all cases. > > > > > Shouldn't the pin state be restored by the pinctrl layer? > > > > What we have in the device core only applies "init" and "default" > > > > states, and provides these handles for transitioning to "sleep" > > > > and "default" again (like a state machine). > > > What I was meaning is that - for a driver using the "default" state, > > > if the hardware loses the pinctrl state during sleep, isn't it the > > > responsibility of the pinctrl driver to restore the state rather > > > than leaving it in whatever states it happens to be when the SoC > > > comes back from suspend? > > > > > > If that is not the case, then don't we have a major issue where > > > drivers using pinctrl but do not issue any pinctrl calls in the > > > resume function are buggy? > > I would like an answer on this before this patch is merged, because > > even with your reviewed-by, I don't think this patch is correct. > > > > For example, if pinctrl loses the pinmux state across suspend/resume, > > then this patch only solves the case where the NIC is down when > > suspending. > > > > It does not address the case where the NIC is up but WoL is disabled. > > Also, what happens when WoL is enabled at the MAC, when we expect the > > NIC to still be functional - which means that the pinmux state must > > remain active over suspend. > > For me this case (when NIC is up) is already managed by the driver and > function suspend/resume: > > On stmmac_suspend : > > ==>      /* Enable Power down mode by programming the PMT regs */ >      if (priv->wolopts) { >          stmmac_pmt(priv, priv->hw, priv->wolopts); >          priv->irq_wake = 1; >      } else { >          stmmac_mac_set(priv, priv->ioaddr, false); > *         pinctrl_pm_select_sleep_state(priv->device);* >      } > > On stmmac_resume : > > ==>     if (priv->wolopts) { >          mutex_lock(&priv->lock); >          stmmac_pmt(priv, priv->hw, 0); >          mutex_unlock(&priv->lock); >          priv->irq_wake = 0; >      } else { > *         pinctrl_pm_select_default_state(priv->device);* >          /* reset the phy so that it's ready */ >          if (priv->mii) >              stmmac_mdio_reset(priv->mii); >      } > I don't know if there's a difference between your two seperate replies that appear to be saying the same thing. I also didn't realise that we're already changing the pinctrl state on suspend/resume when the interface is up - thanks for pointing that out. I think it may be better to move the pinctrl changes after the priv->plat->suspend() call: diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 428b2e5bb4c4..596f9e2349cb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -8174,6 +8174,7 @@ int stmmac_suspend(struct device *dev) struct net_device *ndev = dev_get_drvdata(dev); struct stmmac_priv *priv = netdev_priv(ndev); u8 chan; + int ret; if (!ndev || !netif_running(ndev)) goto suspend_bsp; @@ -8203,7 +8204,6 @@ int stmmac_suspend(struct device *dev) priv->irq_wake = 1; } else { stmmac_mac_set(priv, priv->ioaddr, false); - pinctrl_pm_select_sleep_state(priv->device); } mutex_unlock(&priv->lock); @@ -8225,8 +8225,14 @@ int stmmac_suspend(struct device *dev) ethtool_mmsv_stop(&priv->fpe_cfg.mmsv); suspend_bsp: - if (priv->plat->suspend) - return priv->plat->suspend(dev, priv->plat->bsp_priv); + if (priv->plat->suspend) { + ret = priv->plat->suspend(dev, priv->plat->bsp_priv); + if (ret) + return ret; + } + + if (!priv->wolopts) + pinctrl_pm_select_sleep_state(priv->device); return 0; } @@ -8280,6 +8286,9 @@ int stmmac_resume(struct device *dev) struct stmmac_priv *priv = netdev_priv(ndev); int ret; + if (!priv->wolopts) + pinctrl_pm_select_default_state(priv->device); + if (priv->plat->resume) { ret = priv->plat->resume(dev, priv->plat->bsp_priv); if (ret) @@ -8301,7 +8310,6 @@ int stmmac_resume(struct device *dev) mutex_unlock(&priv->lock); priv->irq_wake = 0; } else { - pinctrl_pm_select_default_state(priv->device); /* reset the phy so that it's ready */ if (priv->mii) stmmac_mdio_reset(priv->mii); This means that we don't switch the pinctrl state until we've finished suspending the device as necessary, which seems way more sane than trying to do it part-way through suspending - for example, while phylink and phylib is still active. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!