diff mbox series

[net-next] net: phy: mdio_device: Reset device only when necessary

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

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1740 this patch: 1740
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1176 this patch: 1176
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1784 this patch: 1784
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 24 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Andrew Halaney Nov. 21, 2023, 10:10 p.m. UTC
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,

Comments

Andrew Lunn Nov. 25, 2023, 4:50 p.m. UTC | #1
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 mbox series

Patch

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;