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 4D850C982EA for ; Mon, 21 Sep 2026 01:41:34 +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:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=HBB22xTvlIdnnUpDmcGb9WM9KBxzgtzh1eAOBEppS6s=; b=fMrNr0o5in2S6vEYn0RzrocLxm s5Pig2ZW5DoRPtcSXpLQ8tl0VTlfg8feaj6fHr8v445xrYEPWoxiXwn7CsELf4zoMepXN/O/VNG/P cMC23Uyj4EQT6ylhakX4bP5wDM3obsLaMpWstEhjcJClMNlhPxGPjnX2Gz+a3lZo/e0gFAVkJ/46e BeBsewgFcNLZyC1m69hrKZWg3A61W4BEWGMxkx6HivXYyX3hNfDyp5c66XCJZAP1m9oC44EytYx1E 7Hh8VVKXZx1gslagqqJRDYHYEMmeTnvMQ+VYIil11638poFyFU6N0A5FYT1kC61QST/bIlZZBRKv9 HG+jgbpg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1x8T24-00000000gx4-1xaq; Mon, 21 Sep 2026 01:41:24 +0000 Received: from proxy25212.mail.163.com ([103.129.252.12]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1x8T21-00000000gwa-1iDF for linux-arm-kernel@lists.infradead.org; Mon, 21 Sep 2026 01:41:22 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=126.com; s=s110527; h=Message-ID:Date:MIME-Version:Subject:To:From: Content-Type; bh=HBB22xTvlIdnnUpDmcGb9WM9KBxzgtzh1eAOBEppS6s=; b=cjkE9x5tD8Olglh0wr6jrDc8SleOEYplfrtufXlmxLuRCPL2flZtr56UuzY4K4 XBgNIOkZbYBqKgXEMWdvO55dllBcG3Sc7A1bWe9eGQHsBw9ER2b2Bi4qep+RVXjp f3Yvoj8qb5KYf7wX1+maqszaESRl9N40ufvFqgk7WPHlA= Message-ID: Date: Mon, 21 Sep 2026 09:39:45 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net v2] net: stmmac: request the MDIO reset GPIO only once To: Lorenzo Bianconi Cc: maxime.chevallier@bootlin.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Linkui Xiao , stable@vger.kernel.org References: <20260920054703.1897755-1-xiaolinkui@126.com> Content-Language: en-US From: Linkui Xiao In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CM-TRANSID: _____wD3P+vhirBqrK9QBw--.58600S2 X-Coremail-Antispam: 1Uf129KBjvJXoWxKr15uFyxur4DKrWUXFyrJFb_yoWxGr1Up3 y5Zay5ArWkJr4Iqws7Xw4UZF9YkFW3tr4UCw1F9rWSyan8KrWSvr1Sqry5ZFy2krWUWFya vr18uFnruF4qyFJanT9S1TB71UUUUU7qnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07U4Vb9UUUUU= X-CM-SenderInfo: p0ld0z5lqn3xa6rslhhfrp/xtbBlAKheGqwiuJbYwAA3F X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260920_184121_765490_31AE36F3 X-CRM114-Status: GOOD ( 23.78 ) 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 Hi Lorenzo, On 2026/9/20 16:07, Lorenzo Bianconi wrote: >> From: Linkui Xiao >> >> stmmac_mdio_reset() calls devm_gpiod_get_optional() every time it runs. >> A GPIO line can only be requested once, so from the second call on >> gpiod_request_commit() returns -EBUSY. devm_gpiod_get_optional() only >> turns -ENOENT into NULL, hence the error is passed straight back and >> stmmac_mdio_reset() bails out before pulsing "snps,reset" and before >> running the STE101P MDC workaround. >> >> The first call, made by of_mdiobus_register(), succeeds, so the failure >> is only visible later on: every resume that does not use WoL goes >> through stmmac_resume() -> stmmac_mdio_reset(), and that caller ignores >> the return value, so the PHY silently stays un-reset. >> >> The descriptor used to be requested exactly once: stmmac_mdio_reset() >> resolved "snps,reset-gpio" itself and cached the GPIO number in >> stmmac_mdio_bus_data::reset_gpio, and commit ae26c1c6cb9b ("stmmac: fix >> PHY reset during resume") relies on that cache to reuse the line on >> every call. commit 7c86f20d15b7 ("net: stmmac: use GPIO descriptors in >> stmmac_mdio_reset") replaced it with a devm_gpiod_get_optional() that >> caches nothing, so the request is repeated on every call and fails from >> the second one on. >> >> Request the GPIO in stmmac_mdio_register(), at probe time, and keep the >> descriptor in struct stmmac_priv. This is where devm-gpiod is meant to be >> used: the line is acquired with the device and released with it, and any >> failure to acquire it is reported during probe instead of being ignored >> by stmmac_resume(). stmmac_mdio_reset() then only pulses the cached line. >> >> The request is still gated on mdio_bus_data->needs_reset, which is the >> condition that installs the reset callback, and on the device using DT, >> as the reset itself is. >> >> Fixes: 7c86f20d15b7 ("net: stmmac: use GPIO descriptors in stmmac_mdio_reset") >> Cc: stable@vger.kernel.org >> Signed-off-by: Linkui Xiao >> --- >> Changes in v2: >> - Do not request the GPIO from stmmac_mdio_reset(); request it once in >> stmmac_mdio_register() at probe time instead. (Maxime Chevallier) >> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++ >> .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 19 ++++++++++--------- >> 2 files changed, 12 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> index 7582fca63741..986fb43db45f 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> @@ -25,6 +25,7 @@ >> #include >> #include >> >> +struct gpio_desc; >> struct stmmac_pcs; >> >> struct stmmac_resources { >> @@ -287,6 +288,7 @@ struct stmmac_priv { >> >> unsigned int pause_time; >> struct mii_bus *mii; >> + struct gpio_desc *mdio_reset_gpio; >> >> struct stmmac_pcs *integrated_pcs; >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >> index afe98ff5bdcb..346f93f86abe 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >> @@ -386,15 +386,8 @@ int stmmac_mdio_reset(struct mii_bus *bus) >> >> #ifdef CONFIG_OF >> if (priv->device->of_node) { >> - struct gpio_desc *reset_gpio; >> u32 delays[3] = { 0, 0, 0 }; >> >> - reset_gpio = devm_gpiod_get_optional(priv->device, >> - "snps,reset", >> - GPIOD_OUT_LOW); >> - if (IS_ERR(reset_gpio)) >> - return PTR_ERR(reset_gpio); >> - >> device_property_read_u32_array(priv->device, >> "snps,reset-delays-us", >> delays, ARRAY_SIZE(delays)); > > nit: what about moving even these properties to stmmac_mdio_register()? It > seems a bit odd to have half of the parsing in stmmac_mdio_register() and > half in stmmac_mdio_reset(). What do you think? Agreed, and thanks for pointing it out. v3 reads "snps,reset-delays-us" in stmmac_mdio_register() as well and keeps the three values in struct stmmac_priv, next to the descriptor, so the whole reset description - the line and its timings - is parsed once, at probe time. stmmac_mdio_reset() then only waits and pulses the cached line. With the parsing gone, the callback doesn't need its CONFIG_OF/of_node guard either, so I dropped that too: stmmac_mdio_reset() no longer looks at the device tree at all. Where the description is absent, the cached descriptor is NULL and the delays are all zero, so the pulse is a no-op - exactly what the of_node check used to make of it. The request and the delays stay gated on mdio_bus_data->needs_reset and on the device using DT, in stmmac_mdio_register(). Nothing else changes: struct stmmac_priv is zeroed when it is allocated, so a missing or short "snps,reset-delays-us" still gives the same all-zero delays the local initialiser did. Thanks, Linkui > > Regards, > Lorenzo > >> @@ -402,11 +395,11 @@ int stmmac_mdio_reset(struct mii_bus *bus) >> if (delays[0]) >> msleep(DIV_ROUND_UP(delays[0], 1000)); >> >> - gpiod_set_value_cansleep(reset_gpio, 1); >> + gpiod_set_value_cansleep(priv->mdio_reset_gpio, 1); >> if (delays[1]) >> msleep(DIV_ROUND_UP(delays[1], 1000)); >> >> - gpiod_set_value_cansleep(reset_gpio, 0); >> + gpiod_set_value_cansleep(priv->mdio_reset_gpio, 0); >> if (delays[2]) >> msleep(DIV_ROUND_UP(delays[2], 1000)); >> } >> @@ -608,6 +601,14 @@ int stmmac_mdio_register(struct net_device *ndev) >> if (!mdio_bus_data) >> return 0; >> >> + if (mdio_bus_data->needs_reset && dev_of_node(priv->device)) { >> + priv->mdio_reset_gpio = >> + devm_gpiod_get_optional(priv->device, "snps,reset", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(priv->mdio_reset_gpio)) >> + return PTR_ERR(priv->mdio_reset_gpio); >> + } >> + >> stmmac_mdio_bus_config(priv); >> >> new_bus = mdiobus_alloc(); >> -- >> 2.25.1 >> >>