| Message ID | 20231127-net-phy-reset-once-v2-1-448e8658779e@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | df16c1c51d8166958f533c0c886766f7ee9dd50f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: phy: mdio_device: Reset device only when necessary | expand |
On Mon, Nov 27, 2023 at 03:41:10PM -0600, Andrew Halaney wrote: > Currently the phy reset sequence is as shown below for a > devicetree described mdio phy on boot: > > 1. Assert the phy_device's reset as part of registering > 2. Deassert the phy_device's reset as part of registering > 3. Deassert the phy_device's reset as part of phy_probe > 4. Deassert the phy_device's reset as part of phy_hw_init > > The extra two deasserts include waiting the deassert delay afterwards, > which is adding unnecessary delay. > > This applies to both possible types of resets (reset controller > reference and a reset gpio) that can be used. > > Here's some snipped tracing output using the following command line > params "trace_event=gpio:* trace_options=stacktrace" illustrating > the reset handling and where its coming from: ... > Reported-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 27 Nov 2023 15:41:10 -0600 you wrote: > Currently the phy reset sequence is as shown below for a > devicetree described mdio phy on boot: > > 1. Assert the phy_device's reset as part of registering > 2. Deassert the phy_device's reset as part of registering > 3. Deassert the phy_device's reset as part of phy_probe > 4. Deassert the phy_device's reset as part of phy_hw_init > > [...] Here is the summary with links: - [net-next,v2] net: phy: mdio_device: Reset device only when necessary https://git.kernel.org/netdev/net-next/c/df16c1c51d81 You are awesome, thank you!
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c index 044828d081d2..73f6539b9e50 100644 --- a/drivers/net/phy/mdio_device.c +++ b/drivers/net/phy/mdio_device.c @@ -62,6 +62,7 @@ struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr) mdiodev->device_remove = mdio_device_remove; mdiodev->bus = bus; mdiodev->addr = addr; + mdiodev->reset_state = -1; dev_set_name(&mdiodev->dev, PHY_ID_FMT, bus->id, addr); @@ -122,6 +123,9 @@ void mdio_device_reset(struct mdio_device *mdiodev, int value) if (!mdiodev->reset_gpio && !mdiodev->reset_ctrl) return; + if (mdiodev->reset_state == value) + return; + if (mdiodev->reset_gpio) gpiod_set_value_cansleep(mdiodev->reset_gpio, value); @@ -135,6 +139,8 @@ void mdio_device_reset(struct mdio_device *mdiodev, int value) d = value ? mdiodev->reset_assert_delay : mdiodev->reset_deassert_delay; if (d) fsleep(d); + + mdiodev->reset_state = value; } EXPORT_SYMBOL(mdio_device_reset); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 478126f6b5bc..843ce2479736 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -654,6 +654,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, mdiodev->flags = MDIO_DEVICE_FLAG_PHY; mdiodev->device_free = phy_mdio_device_free; mdiodev->device_remove = phy_mdio_device_remove; + mdiodev->reset_state = -1; dev->speed = SPEED_UNKNOWN; dev->duplex = DUPLEX_UNKNOWN; diff --git a/include/linux/mdio.h b/include/linux/mdio.h index 007fd9c3e4b6..79ceee3c8673 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -38,6 +38,7 @@ struct mdio_device { /* Bus address of the MDIO device (0-31) */ int addr; int flags; + int reset_state; struct gpio_desc *reset_gpio; struct reset_control *reset_ctrl; unsigned int reset_assert_delay;
Currently the phy reset sequence is as shown below for a devicetree described mdio phy on boot: 1. Assert the phy_device's reset as part of registering 2. Deassert the phy_device's reset as part of registering 3. Deassert the phy_device's reset as part of phy_probe 4. Deassert the phy_device's reset as part of phy_hw_init The extra two deasserts include waiting the deassert delay afterwards, which is adding unnecessary delay. This applies to both possible types of resets (reset controller reference and a reset gpio) that can be used. Here's some snipped tracing output using the following command line params "trace_event=gpio:* trace_options=stacktrace" illustrating the reset handling and where its coming from: /* Assert */ systemd-udevd-283 [002] ..... 6.780434: gpio_value: 544 set 0 systemd-udevd-283 [002] ..... 6.783849: <stack trace> => gpiod_set_raw_value_commit => gpiod_set_value_nocheck => gpiod_set_value_cansleep => mdio_device_reset => mdiobus_register_device => phy_device_register => fwnode_mdiobus_phy_device_register => fwnode_mdiobus_register_phy => __of_mdiobus_register => stmmac_mdio_register => stmmac_dvr_probe => stmmac_pltfr_probe => devm_stmmac_pltfr_probe => qcom_ethqos_probe => platform_probe /* Deassert */ systemd-udevd-283 [002] ..... 6.802480: gpio_value: 544 set 1 systemd-udevd-283 [002] ..... 6.805886: <stack trace> => gpiod_set_raw_value_commit => gpiod_set_value_nocheck => gpiod_set_value_cansleep => mdio_device_reset => phy_device_register => fwnode_mdiobus_phy_device_register => fwnode_mdiobus_register_phy => __of_mdiobus_register => stmmac_mdio_register => stmmac_dvr_probe => stmmac_pltfr_probe => devm_stmmac_pltfr_probe => qcom_ethqos_probe => platform_probe /* Deassert */ systemd-udevd-283 [002] ..... 6.882601: gpio_value: 544 set 1 systemd-udevd-283 [002] ..... 6.886014: <stack trace> => gpiod_set_raw_value_commit => gpiod_set_value_nocheck => gpiod_set_value_cansleep => mdio_device_reset => phy_probe => really_probe => __driver_probe_device => driver_probe_device => __device_attach_driver => bus_for_each_drv => __device_attach => device_initial_probe => bus_probe_device => device_add => phy_device_register => fwnode_mdiobus_phy_device_register => fwnode_mdiobus_register_phy => __of_mdiobus_register => stmmac_mdio_register => stmmac_dvr_probe => stmmac_pltfr_probe => devm_stmmac_pltfr_probe => qcom_ethqos_probe => platform_probe /* Deassert */ NetworkManager-477 [000] ..... 7.023144: gpio_value: 544 set 1 NetworkManager-477 [000] ..... 7.026596: <stack trace> => gpiod_set_raw_value_commit => gpiod_set_value_nocheck => gpiod_set_value_cansleep => mdio_device_reset => phy_init_hw => phy_attach_direct => phylink_fwnode_phy_connect => __stmmac_open => stmmac_open There's a lot of paths where the device is getting its reset asserted and deasserted. Let's track the state and only actually do the assert/deassert when it changes. Reported-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> Signed-off-by: Andrew Halaney <ahalaney@redhat.com> --- Changes in v2: - Mention the reset controller in the commit message (Andrew Lunn) - Make the initial reset_state unknown (so we always ensure the reset gpio and controller end up in the same state) instead of assuming they're both out of reset after acquiring them (Andrew Lunn) - Link to v1: https://lore.kernel.org/r/20231121-net-phy-reset-once-v1-1-37c960b6336c@redhat.com --- drivers/net/phy/mdio_device.c | 6 ++++++ drivers/net/phy/phy_device.c | 1 + include/linux/mdio.h | 1 + 3 files changed, 8 insertions(+) --- base-commit: 48bbaf8b793e0770798519f8ee1ea2908ff0943a change-id: 20231121-net-phy-reset-once-1e2323982ae0 Best regards,