arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul.dtsi | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
The Ethernet PHY's reset GPIO should be specified in the node of the PHY
itself, instead of the MAC (`fec`). The latter is deprecated, and was an
i.MX-specific extension, incompatible with the new reset controller
subsystem.
Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>
Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
---
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 = <®_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 */
+ 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;
status = "okay";
};
---
base-commit: 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
change-id: 20250815-b4-tx6ul-dt-phy-rst-7afc190a6907
Best regards,
--
Bence Csókás <csokas.bence@prolan.hu>
Hello,
On 15.08.25 17:17, Bence Csókás wrote:
> The Ethernet PHY's reset GPIO should be specified in the node of the PHY
> itself, instead of the MAC (`fec`). The latter is deprecated, and was an
> i.MX-specific extension, incompatible with the new reset controller
> subsystem.
One reason to do it this way is that the PHY is in reset when the OS starts
and the external phy-reset-gpios allows MAC probe to get the PHY out of
reset, so it can be probed after reading its vendor/device IDs.
Does switching to this new binding address this scenario? If so, it should
be noted in the commit message.
>
> Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
> Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
> ---
> 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 = <®_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.
> + 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.
Cheers,
Ahmad
> status = "okay";
> };
>
>
> ---
> base-commit: 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
> change-id: 20250815-b4-tx6ul-dt-phy-rst-7afc190a6907
>
> Best regards,
--
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 |
Hi,
On 2025. 09. 03. 9:28, Ahmad Fatoum wrote:
> Hello,
>
> On 15.08.25 17:17, Bence Csókás wrote:
>> The Ethernet PHY's reset GPIO should be specified in the node of the PHY
>> itself, instead of the MAC (`fec`). The latter is deprecated, and was an
>> i.MX-specific extension, incompatible with the new reset controller
>> subsystem.
>
> One reason to do it this way is that the PHY is in reset when the OS starts
> and the external phy-reset-gpios allows MAC probe to get the PHY out of
> reset, so it can be probed after reading its vendor/device IDs.
>
> Does switching to this new binding address this scenario? If so, it should
> be noted in the commit message.
Yes, but after it has been reset, if the platform supports Power
Management, the PHY's clock will be turned off, which some PHYs (in our
case the LAN8710) don't tolerate. This has been reported many times,
just search LKML for "lan8710 reset".
So we want a more general solution [1] where the PHY subsystem resets
them before enumerating. However, if the MAC driver claims the GPIO,
then it can't be used by the PHY.
I will clarify the commit msg with this in mind.
[1]
https://lore.kernel.org/lkml/20250709133222.48802-4-buday.csaba@prolan.hu/
>>
>> Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
>> Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>lan8710 reset
>> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
>> ---
>> 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 = <®_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.
>> + 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!
> Cheers,
> Ahmad
>
>> status = "okay";
>> };
>>
>>
>> ---
>> base-commit: 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
>> change-id: 20250815-b4-tx6ul-dt-phy-rst-7afc190a6907
>>
>> Best regards,
>
>
Bence
Hi,
On 03.09.25 09:43, Csókás Bence wrote:
> Hi,
>
> On 2025. 09. 03. 9:28, Ahmad Fatoum wrote:
>> Hello,
>>
>> On 15.08.25 17:17, Bence Csókás wrote:
>>> The Ethernet PHY's reset GPIO should be specified in the node of the PHY
>>> itself, instead of the MAC (`fec`). The latter is deprecated, and was an
>>> i.MX-specific extension, incompatible with the new reset controller
>>> subsystem.
>>
>> One reason to do it this way is that the PHY is in reset when the OS starts
>> and the external phy-reset-gpios allows MAC probe to get the PHY out of
>> reset, so it can be probed after reading its vendor/device IDs.
>>
>> Does switching to this new binding address this scenario? If so, it should
>> be noted in the commit message.
>
> Yes, but after it has been reset, if the platform supports Power Management, the PHY's clock will be turned off, which some PHYs (in our case the LAN8710) don't tolerate. This has been reported many times, just search LKML for "lan8710 reset".
>
> So we want a more general solution [1] where the PHY subsystem resets them before enumerating. However, if the MAC driver claims the GPIO, then it can't be used by the PHY.
I agree that it makes sense for a PHY reset to be associated with the PHY
device and controlled by the PHY driver. I am wary of regressions though,
which is why I wanted the commit message to clearly spell out the implications.
> I will clarify the commit msg with this in mind.
Thanks.
> [1] https://lore.kernel.org/lkml/20250709133222.48802-4-buday.csaba@prolan.hu/
Is this mainline yet?
Cheers,
Ahmad
>
>>>
>>> Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
>>> Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>lan8710 reset
>>> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
>>> ---
>>> 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 = <®_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.
>
>>> + 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!
>
>> Cheers,
>> Ahmad
>>
>>> 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 |
On Wed, Sep 03, 2025 at 09:50:08AM +0200, Ahmad Fatoum wrote:
> Hi,
>
> On 03.09.25 09:43, Csókás Bence wrote:
> > Hi,
> >
> > On 2025. 09. 03. 9:28, Ahmad Fatoum wrote:
> >> Hello,
> >>
> >> On 15.08.25 17:17, Bence Csókás wrote:
> >>> The Ethernet PHY's reset GPIO should be specified in the node of the PHY
> >>> itself, instead of the MAC (`fec`). The latter is deprecated, and was an
> >>> i.MX-specific extension, incompatible with the new reset controller
> >>> subsystem.
> >>
> >> One reason to do it this way is that the PHY is in reset when the OS starts
> >> and the external phy-reset-gpios allows MAC probe to get the PHY out of
> >> reset, so it can be probed after reading its vendor/device IDs.
> >>
> >> Does switching to this new binding address this scenario? If so, it should
> >> be noted in the commit message.
> >
> > Yes, but after it has been reset, if the platform supports Power Management, the PHY's clock will be turned off, which some PHYs (in our case the LAN8710) don't tolerate. This has been reported many times, just search LKML for "lan8710 reset".
> >
> > So we want a more general solution [1] where the PHY subsystem resets them before enumerating. However, if the MAC driver claims the GPIO, then it can't be used by the PHY.
>
> I agree that it makes sense for a PHY reset to be associated with the PHY
> device and controlled by the PHY driver. I am wary of regressions though,
> which is why I wanted the commit message to clearly spell out the implications.
>
> > I will clarify the commit msg with this in mind.
>
> Thanks.
>
> > [1] https://lore.kernel.org/lkml/20250709133222.48802-4-buday.csaba@prolan.hu/
>
> 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.
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.
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.
Regards,
Csaba
> Cheers,
> Ahmad
>
> >
> >>>
> >>> Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
> >>> Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>lan8710 reset
> >>> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
> >>> ---
> >>> 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 = <®_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.
> >
> >>> + 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!
> >
> >> Cheers,
> >> Ahmad
> >>
> >>> 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 |
>
Hi,
On 9/3/25 10:01 AM, Buday Csaba wrote:
> On Wed, Sep 03, 2025 at 09:50:08AM +0200, Ahmad Fatoum wrote:
>>> [1] https://lore.kernel.org/lkml/20250709133222.48802-4-buday.csaba@prolan.hu/
>>
>> 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.
> 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.
> 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/
But it went no where. I don't recall the details.
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.
Cheers,
Ahmad
>
> Regards,
> Csaba
>
>> Cheers,
>> Ahmad
>>
>>>
>>>>>
>>>>> Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
>>>>> Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>lan8710 reset
>>>>> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
>>>>> ---
>>>>> 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 = <®_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.
>>>
>>>>> + 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!
>>>
>>>> Cheers,
>>>> Ahmad
>>>>
>>>>> 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 |
On Wed, Sep 03, 2025 at 10:43:46AM +0200, Ahmad Fatoum wrote:
> Hi,
>
> On 9/3/25 10:01 AM, Buday Csaba wrote:
> > On Wed, Sep 03, 2025 at 09:50:08AM +0200, Ahmad Fatoum wrote:
> >>> [1] https://lore.kernel.org/lkml/20250709133222.48802-4-buday.csaba@prolan.hu/
> >>
> >> 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.
>
> > 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.
>
> > 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/
>
> 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
>
> >
> > Regards,
> > Csaba
> >
> >> Cheers,
> >> Ahmad
> >>
> >>>
> >>>>>
> >>>>> Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
> >>>>> Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>lan8710 reset
> >>>>> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
> >>>>> ---
> >>>>> 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 = <®_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.
> >>>
> >>>>> + 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!
> >>>
> >>>> Cheers,
> >>>> Ahmad
> >>>>
> >>>>> 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 |
>
>
Hi, On 2025. 09. 03. 9:50, Ahmad Fatoum wrote: > Hi, > > On 03.09.25 09:43, Csókás Bence wrote: >> Hi, >> >> On 2025. 09. 03. 9:28, Ahmad Fatoum wrote: >>> Hello, >>> >>> On 15.08.25 17:17, Bence Csókás wrote: >>>> The Ethernet PHY's reset GPIO should be specified in the node of the PHY >>>> itself, instead of the MAC (`fec`). The latter is deprecated, and was an >>>> i.MX-specific extension, incompatible with the new reset controller >>>> subsystem. >>> >>> One reason to do it this way is that the PHY is in reset when the OS starts >>> and the external phy-reset-gpios allows MAC probe to get the PHY out of >>> reset, so it can be probed after reading its vendor/device IDs. >>> >>> Does switching to this new binding address this scenario? If so, it should >>> be noted in the commit message. >> >> Yes, but after it has been reset, if the platform supports Power Management, the PHY's clock will be turned off, which some PHYs (in our case the LAN8710) don't tolerate. This has been reported many times, just search LKML for "lan8710 reset". >> >> So we want a more general solution [1] where the PHY subsystem resets them before enumerating. However, if the MAC driver claims the GPIO, then it can't be used by the PHY. > > I agree that it makes sense for a PHY reset to be associated with the PHY > device and controlled by the PHY driver. I am wary of regressions though, > which is why I wanted the commit message to clearly spell out the implications. You're right. We'll put this all in the message. >> I will clarify the commit msg with this in mind. > > Thanks. > >> [1] https://lore.kernel.org/lkml/20250709133222.48802-4-buday.csaba@prolan.hu/ > > Is this mainline yet? Unfortunately, no. It apparently still needs some polish. Bence
Hi, On 9/3/25 9:57 AM, Csókás Bence wrote: > On 2025. 09. 03. 9:50, Ahmad Fatoum wrote: >> On 03.09.25 09:43, Csókás Bence wrote: >>> [1] https://lore.kernel.org/lkml/20250709133222.48802-4- >>> buday.csaba@prolan.hu/ >> >> Is this mainline yet? > > Unfortunately, no. It apparently still needs some polish. So you are trading one breakage for another here. This needs to be spelt out along with rationale. Alternatively, see my suggestion in the reply to Csaba. Cheers, Ahmad > > 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 |
On Fri, 15 Aug 2025 17:17:37 +0200, Bence Csókás wrote:
> The Ethernet PHY's reset GPIO should be specified in the node of the PHY
> itself, instead of the MAC (`fec`). The latter is deprecated, and was an
> i.MX-specific extension, incompatible with the new reset controller
> subsystem.
>
> Co-developed-by: Csaba Buday <buday.csaba@prolan.hu>
> Signed-off-by: Csaba Buday <buday.csaba@prolan.hu>
> Signed-off-by: Bence Csókás <csokas.bence@prolan.hu>
> ---
> arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul.dtsi | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
My bot found new DTB warnings on the .dts files added or changed in this
series.
Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.
If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:
pip3 install dtschema --upgrade
This patch series was applied (using b4) to base:
Base: using specified base-commit 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
If this is not the correct base, please add 'base-commit' tag
(or use b4 which does this automatically)
New warnings running 'make CHECK_DTBS=y for arch/arm/boot/dts/nxp/' for 20250815-b4-tx6ul-dt-phy-rst-v1-1-9b65e315d9d3@prolan.hu:
arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul-0011.dtb: / (karo,imx6ul-tx6ul): i2c-gpio: {'compatible': ['i2c-gpio'], '#address-cells': 1, '#size-cells': 0, 'pinctrl-names': ['default'], 'pinctrl-0': [[69]], 'sda-gpios': [[48, 1, 0]], 'scl-gpios': [[48, 0, 0]], 'clock-frequency': 400000, 'status': ['okay'], 'rtc@68': {'compatible': ['dallas,ds1339'], 'reg': [[104]], 'status': ['disabled']}} is not of type 'array'
from schema $id: http://devicetree.org/schemas/gpio/gpio-consumer.yaml#
arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul-0010.dtb: / (karo,imx6ul-tx6ul): i2c-gpio: {'compatible': ['i2c-gpio'], '#address-cells': 1, '#size-cells': 0, 'pinctrl-names': ['default'], 'pinctrl-0': [[68]], 'sda-gpios': [[48, 1, 0]], 'scl-gpios': [[48, 0, 0]], 'clock-frequency': 400000, 'status': ['okay'], 'rtc@68': {'compatible': ['dallas,ds1339'], 'reg': [[104]], 'status': ['disabled']}} is not of type 'array'
from schema $id: http://devicetree.org/schemas/gpio/gpio-consumer.yaml#
arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul-0011.dtb: ethernet-phy@0 (ethernet-phy-ieee802.3-c22): 'resets' is a dependency of 'reset-names'
from schema $id: http://devicetree.org/schemas/reset/reset.yaml#
arch/arm/boot/dts/nxp/imx/imx6ul-tx6ul-0010.dtb: ethernet-phy@0 (ethernet-phy-ieee802.3-c22): 'resets' is a dependency of 'reset-names'
from schema $id: http://devicetree.org/schemas/reset/reset.yaml#
© 2016 - 2025 Red Hat, Inc.