| Message ID | 20231121-net-phy-reset-once-v1-1-37c960b6336c@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: mdio_device: Reset device only when necessary | expand |
On Tue, Nov 21, 2023 at 04:10:37PM -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. > > 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. This only talks about GPIOs. There is also support for a linux reset controller. It is getting turned on/off as many times. You should mention this. Now, lets compare the GPIO and the reset controller: static int mdiobus_register_gpiod(struct mdio_device *mdiodev) { /* Deassert the optional reset signal */ mdiodev->reset_gpio = gpiod_get_optional(&mdiodev->dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(mdiodev->reset_gpio)) return PTR_ERR(mdiodev->reset_gpio); if (mdiodev->reset_gpio) gpiod_set_consumer_name(mdiodev->reset_gpio, "PHY reset"); return 0; } static int mdiobus_register_reset(struct mdio_device *mdiodev) { struct reset_control *reset; reset = reset_control_get_optional_exclusive(&mdiodev->dev, "phy"); if (IS_ERR(reset)) return PTR_ERR(reset); mdiodev->reset_ctrl = reset; return 0; } For the GPIO controller, its clear what state it is in, because the get call sets it to GPIOD_OUT_LOW. The reset controller however does not have a clear state, we just get a reference to it. But: in mdiobus_register_device() we have: err = mdiobus_register_reset(mdiodev); if (err) return err; /* Assert the reset signal */ mdio_device_reset(mdiodev, 1); suggesting the reset controller might have the opposite state to the GPIO? > diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c > index 044828d081d2..d2b9e62edaaa 100644 > --- a/drivers/net/phy/mdio_device.c > +++ b/drivers/net/phy/mdio_device.c > @@ -122,6 +122,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; > + mdiodev is set to all 0 at creation time, and the GPIO is also set to LOW when we get it. However, what about the reset controller? I think it would be better to initialize reset_state to -1, indicating we have no idea what the state is, and always perform the first reset to ensure we are into a know state for both GPIO and the reset controller. Andrew
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c index 044828d081d2..d2b9e62edaaa 100644 --- a/drivers/net/phy/mdio_device.c +++ b/drivers/net/phy/mdio_device.c @@ -122,6 +122,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 +138,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/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. 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> --- This patch isn't the prettiest thing in the world, but the myriad of call paths that lead to touching the reset gpio is proving daunting to me. I originally tried to remove some of the reset calls in various places, but after staring at the different call paths I decided this was safer, I am not confident I covered all the corner cases when going about this by removing the extra phy_device_reset/mdio_device_reset calls. --- drivers/net/phy/mdio_device.c | 5 +++++ include/linux/mdio.h | 1 + 2 files changed, 6 insertions(+) --- base-commit: 07b677953b9dca02928be323e2db853511305fa9 change-id: 20231121-net-phy-reset-once-1e2323982ae0 Best regards,