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 <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
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(+)
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;
---
base-commit: 07b677953b9dca02928be323e2db853511305fa9
change-id: 20231121-net-phy-reset-once-1e2323982ae0
Best regards,
--
Andrew Halaney <[email protected]>
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