Messages in this thread Patch in this message | | | Date | Sat, 25 Nov 2023 17:50:10 +0100 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next] net: phy: mdio_device: Reset device only when necessary |
| |
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
| |