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 <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
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/[email protected]
---
drivers/net/phy/mdio_device.c | 6 ++++++
drivers/net/phy/phy_device.c | 1 +
include/linux/mdio.h | 1 +
3 files changed, 8 insertions(+)
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;
---
base-commit: 48bbaf8b793e0770798519f8ee1ea2908ff0943a
change-id: 20231121-net-phy-reset-once-1e2323982ae0
Best regards,
--
Andrew Halaney <[email protected]>
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 <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html