Re: [PATCH] ARM: dts: imx6ul-tx6ul: Switch away from deprecated `phy-reset-gpios`

On Wed, Sep 03, 2025 at 10:43:46AM +0200, Ahmad Fatoum wrote:

Hi,

On 9/3/25 10:01 AM, Buday Csaba wrote:

quoted

On Wed, Sep 03, 2025 at 09:50:08AM +0200, Ahmad Fatoum wrote:

quoted

quoted

[1] https://lore.kernel.org/lkml/20250709133222.48802-4-buday.csaba@prolan.hu/ (local)

Is this mainline yet?

No, it is not. It was never the most beautiful piece of code, so I understand
that.

But if you could give us some guidance, that would help a lot.

Specifically:

1)  If `phy-reset-gpios` is deprecated, than we should start treating it as
    such, and not rely on it in future releases. Perhaps we should also add a
    warning message, when it is found in the device tree.

Disagreed. Deprecated properties should be removed only about clarifying
the impact of the removal on users. Replacing a deprecated property with
an expectation that bootloader board code has deasserted reset is not
acceptable IMO.

I was only trying to reason, that since `phy-reset-gpios` has been marked as 
deprecated in 5.3 (which was 6 years ago), perhaps a inserting a warning note
now would be appropriate.
But that is related to the driver, not the DT.
I completely agree with the rest.

quoted

2)  On the other hand, if it is here to stay for a long time, it should be
    fixed. Now the gpio is claimed during fec_reset_phy(), and never released.
    It can not be used by the driver later, like in fec_init(), because the
    gpio reference is only stored in a local variable of fec_reset_phy().
    Previous patches that would have stored the reference in the driver were
    rejected on the grounds that it is deprecated. But if it is not, then we
    can create a patch that would make it work properly.

Ye, this needs to be solved differently.

quoted

3)  Andrew pointed out, that resetting a PHY before probing it may cause
    regressions. That is certainly a valid concern, but for most of the 
    devices resetting it means starting from a known state, and should be the
    default. But we could create a device tree property, that controls this
    behaviour.

Marco had a more involved series to address this:
https://lore.kernel.org/all/20230405-net-next-topic-net-phy-reset-v1-0-7e5329f08002@pengutronix.de/ (local)

But it went no where. I don't recall the details.

Interesting. So Andrew is not against resetting before probe, it just has to
be done properly. ;)
 

I think the best you can do with existing bindings is to give your PHY a
compatible that spells out vendor/device ID, e.g. ethernet-phy-id0141.0dd4.

Then Linux can probe the device even while it's in reset.

The downside is that it hardcodes a specific PHY ID, but this may be
acceptable here.

Yes, for now that would be acceptable for us, with some loss of generality.

Regards,
Csaba

Cheers,
Ahmad

quoted

Regards,
Csaba

quoted

Cheers,
Ahmad

quoted

quoted

quoted

Co-developed-by: Csaba Buday <redacted>
Signed-off-by: Csaba Buday <redacted>lan8710 reset
Signed-off-by: Bence Csókás <redacted>
---
  arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul.dtsi | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul.dtsi b/arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul.dtsi
index f053358bc9317f8447d65013a18670cb470106b2..0a5e90704ea481b0716d6ff6bc6d2110914d4f31 100644
--- a/arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul.dtsi
+++ b/arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul.dtsi

@@ -246,7 +246,6 @@ &fec1 {

      pinctrl-names = "default";
      pinctrl-0 = <&pinctrl_enet1 &pinctrl_enet1_mdio &pinctrl_etnphy0_rst>;
      phy-mode = "rmii";
-    phy-reset-gpios = <&gpio5 6 GPIO_ACTIVE_LOW>;
      phy-supply = <&reg_3v3_etn>;
      phy-handle = <&etnphy0>;
      status = "okay";

@@ -262,6 +261,13 @@ etnphy0: ethernet-phy@0 {

              pinctrl-0 = <&pinctrl_etnphy0_int>;
              interrupt-parent = <&gpio5>;
              interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
+            /* Reset SHOULD be a PHY property */

Comment belongs into commit message.

Agreed.

quoted

quoted

+            reset-names = "phy";
+            reset-gpios = <&gpio5 6 GPIO_ACTIVE_LOW>;
+            reset-assert-us = <100>;
+            reset-deassert-us = <25000>;
+            /* Energy detect sometimes causes link failures */
+            smsc,disable-energy-detect;

Unrelated change not described in the commit message.

Oh, this has accidentally made it into here from our DT. Thanks for spotting it!

quoted

Cheers,
Ahmad

quoted

              status = "okay";
          };
 
---
base-commit: 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
change-id: 20250815-b4-tx6ul-dt-phy-rst-7afc190a6907

Best regards,

Bence

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |