[PATCH] net: phy: at803x: Improve hibernation support on start up

[PATCH] net: phy: at803x: Improve hibernation support on start up

[Marek Vasut]

Toggle hibernation mode OFF and ON to wake the PHY up and
make it generate clock on RX_CLK pin for about 10 seconds.
These clock are needed during start up by MACs like DWMAC
in NXP i.MX8M Plus to release their DMA from reset. After
the MAC has started up, the PHY can enter hibernation and
disable the RX_CLK clock, this poses no problem for the MAC.

Originally, this issue has been described by NXP in commit
9ecf04016c87 ("net: phy: at803x: add disable hibernation mode support")
but this approach fully disables the hibernation support and
takes away any power saving benefit. This patch instead makes
the PHY generate the clock on start up for 10 seconds, which
should be long enough for the EQoS MAC to release DMA from
reset.

Before this patch on i.MX8M Plus board with AR8031 PHY:
"
$ ifconfig eth1 up
[   25.576734] imx-dwmac 30bf0000.ethernet eth1: Register MEM_TYPE_PAGE_POOL RxQ-0
[   25.658916] imx-dwmac 30bf0000.ethernet eth1: PHY [stmmac-1:00] driver [Qualcomm Atheros AR8031/AR8033] (irq=38)
[   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
[   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup: DMA engine initialization failed
[   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open: Hw setup failed
ifconfig: SIOCSIFFLAGS: Connection timed out
"

After this patch on i.MX8M Plus board with AR8031 PHY:
"
$ ifconfig eth1 up
[   19.419085] imx-dwmac 30bf0000.ethernet eth1: Register MEM_TYPE_PAGE_POOL RxQ-0
[   19.507380] imx-dwmac 30bf0000.ethernet eth1: PHY [stmmac-1:00] driver [Qualcomm Atheros AR8031/AR8033] (irq=38)
[   19.528464] imx-dwmac 30bf0000.ethernet eth1: No Safety Features support found
[   19.535769] imx-dwmac 30bf0000.ethernet eth1: IEEE 1588-2008 Advanced Timestamp supported
[   19.544302] imx-dwmac 30bf0000.ethernet eth1: registered PTP clock
[   19.552008] imx-dwmac 30bf0000.ethernet eth1: FPE workqueue start
[   19.558152] imx-dwmac 30bf0000.ethernet eth1: configuring for phy/rgmii-id link mode
"

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Oleksij Rempel <linux@rempel-privat.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Wei Fang <wei.fang@nxp.com>
Cc: netdev@vger.kernel.org
---
 drivers/net/phy/at803x.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 13c4121fa3092..8cb7b39c6cddc 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -986,6 +986,25 @@ static int at8031_pll_config(struct phy_device *phydev)
 static int at803x_hibernation_mode_config(struct phy_device *phydev)
 {
 	struct at803x_priv *priv = phydev->priv;
+	int ret;
+
+	/* Toggle hibernation mode OFF and ON to wake the PHY up and
+	 * make it generate clock on RX_CLK pin for about 10 seconds.
+	 * These clock are needed during start up by MACs like DWMAC
+	 * in NXP i.MX8M Plus to release their DMA from reset. After
+	 * the MAC has started up, the PHY can enter hibernation and
+	 * disable the RX_CLK clock, this poses no problem for the MAC.
+	 */
+	ret = at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+				    AT803X_DEBUG_HIB_CTRL_PS_HIB_EN, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+				    AT803X_DEBUG_HIB_CTRL_PS_HIB_EN,
+				    AT803X_DEBUG_HIB_CTRL_PS_HIB_EN);
+	if (ret < 0)
+		return ret;
 
 	/* The default after hardware reset is hibernation mode enabled. After
 	 * software reset, the value is retained.
-- 
2.40.1

RE: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Wei Fang]

Hi Marek,

> Toggle hibernation mode OFF and ON to wake the PHY up and make it
> generate clock on RX_CLK pin for about 10 seconds.
> These clock are needed during start up by MACs like DWMAC in NXP i.MX8M
> Plus to release their DMA from reset. After the MAC has started up, the PHY
> can enter hibernation and disable the RX_CLK clock, this poses no problem for
> the MAC.
> 
> Originally, this issue has been described by NXP in commit
> 9ecf04016c87 ("net: phy: at803x: add disable hibernation mode support") but
> this approach fully disables the hibernation support and takes away any power
> saving benefit. This patch instead makes the PHY generate the clock on start
> up for 10 seconds, which should be long enough for the EQoS MAC to release
> DMA from reset.
> 
> Before this patch on i.MX8M Plus board with AR8031 PHY:
> "
> $ ifconfig eth1 up
> [   25.576734] imx-dwmac 30bf0000.ethernet eth1: Register
> MEM_TYPE_PAGE_POOL RxQ-0
> [   25.658916] imx-dwmac 30bf0000.ethernet eth1: PHY [stmmac-1:00]
> driver [Qualcomm Atheros AR8031/AR8033] (irq=38)
> [   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
> [   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup:
> DMA engine initialization failed
> [   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open: Hw
> setup failed
> ifconfig: SIOCSIFFLAGS: Connection timed out "
> 

Have you reproduced this issue based on the upstream net-next or net tree?
If so, can this issue be reproduced? The reason why I ask this is because when
I tried to reproduce this problem on net-next 6.3.0 version, I found that it could
not be reproduced (I did not disable hibernation mode when I reproduced this
issue ). So I guess maybe other patches in eqos driver fixed the issue.

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Marek Vasut]

On 8/8/23 10:44, Wei Fang wrote:
> Hi Marek,

Hi,

>> Toggle hibernation mode OFF and ON to wake the PHY up and make it
>> generate clock on RX_CLK pin for about 10 seconds.
>> These clock are needed during start up by MACs like DWMAC in NXP i.MX8M
>> Plus to release their DMA from reset. After the MAC has started up, the PHY
>> can enter hibernation and disable the RX_CLK clock, this poses no problem for
>> the MAC.
>>
>> Originally, this issue has been described by NXP in commit
>> 9ecf04016c87 ("net: phy: at803x: add disable hibernation mode support") but
>> this approach fully disables the hibernation support and takes away any power
>> saving benefit. This patch instead makes the PHY generate the clock on start
>> up for 10 seconds, which should be long enough for the EQoS MAC to release
>> DMA from reset.
>>
>> Before this patch on i.MX8M Plus board with AR8031 PHY:
>> "
>> $ ifconfig eth1 up
>> [   25.576734] imx-dwmac 30bf0000.ethernet eth1: Register
>> MEM_TYPE_PAGE_POOL RxQ-0
>> [   25.658916] imx-dwmac 30bf0000.ethernet eth1: PHY [stmmac-1:00]
>> driver [Qualcomm Atheros AR8031/AR8033] (irq=38)
>> [   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
>> [   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup:
>> DMA engine initialization failed
>> [   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open: Hw
>> setup failed
>> ifconfig: SIOCSIFFLAGS: Connection timed out "
>>
> 
> Have you reproduced this issue based on the upstream net-next or net tree?

On current linux-next next-20230808 so 6.5.0-rc5 . As far as I can tell, 
net-next is merged into this tree too.

> If so, can this issue be reproduced? The reason why I ask this is because when
> I tried to reproduce this problem on net-next 6.3.0 version, I found that it could
> not be reproduced (I did not disable hibernation mode when I reproduced this
> issue ). So I guess maybe other patches in eqos driver fixed the issue.

This is what I use for testing:

- Make sure "qca,disable-hibernation-mode" is NOT present in PHY DT node
- Boot the machine with NO ethernet cable plugged into the affected port
   (i.e. the EQoS port), this is important
- Make sure the EQoS MAC is not brought up e.g. by systemd-networkd or
   whatever other tool, I use busybox initramfs for testing with plain
   script as init (it mounts the various filesystems and runs /bin/sh)
- Wait longer than 10 seconds
- If possible, measure AR8031 PHY pin 33 RX_CLK, wait for the RX_CLK to
   be turned OFF by the PHY (means PHY entered hibernation)
- ifconfig ethN up -- try to bring up the EQoS MAC
<observe failure>

[...]

RE: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Wei Fang]

> >> Toggle hibernation mode OFF and ON to wake the PHY up and make it
> >> generate clock on RX_CLK pin for about 10 seconds.
> >> These clock are needed during start up by MACs like DWMAC in NXP
> >> i.MX8M Plus to release their DMA from reset. After the MAC has
> >> started up, the PHY can enter hibernation and disable the RX_CLK
> >> clock, this poses no problem for the MAC.
> >>
> >> Originally, this issue has been described by NXP in commit
> >> 9ecf04016c87 ("net: phy: at803x: add disable hibernation mode
> >> support") but this approach fully disables the hibernation support
> >> and takes away any power saving benefit. This patch instead makes the
> >> PHY generate the clock on start up for 10 seconds, which should be
> >> long enough for the EQoS MAC to release DMA from reset.
> >>
> >> Before this patch on i.MX8M Plus board with AR8031 PHY:
> >> "
> >> $ ifconfig eth1 up
> >> [   25.576734] imx-dwmac 30bf0000.ethernet eth1: Register
> >> MEM_TYPE_PAGE_POOL RxQ-0
> >> [   25.658916] imx-dwmac 30bf0000.ethernet eth1: PHY [stmmac-1:00]
> >> driver [Qualcomm Atheros AR8031/AR8033] (irq=38)
> >> [   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
> >> [   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup:
> >> DMA engine initialization failed
> >> [   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open:
> Hw
> >> setup failed
> >> ifconfig: SIOCSIFFLAGS: Connection timed out "
> >>
> >
> > Have you reproduced this issue based on the upstream net-next or net
> tree?
> 
> On current linux-next next-20230808 so 6.5.0-rc5 . As far as I can tell,
> net-next is merged into this tree too.
> 

> > If so, can this issue be reproduced? The reason why I ask this is
> > because when I tried to reproduce this problem on net-next 6.3.0
> > version, I found that it could not be reproduced (I did not disable
> > hibernation mode when I reproduced this issue ). So I guess maybe other
> patches in eqos driver fixed the issue.
> 
> This is what I use for testing:
> 
> - Make sure "qca,disable-hibernation-mode" is NOT present in PHY DT node
Yes, I deleted this property when I reproduced this issue.

> - Boot the machine with NO ethernet cable plugged into the affected port
>    (i.e. the EQoS port), this is important
> - Make sure the EQoS MAC is not brought up e.g. by systemd-networkd or
>    whatever other tool, I use busybox initramfs for testing with plain
>    script as init (it mounts the various filesystems and runs /bin/sh)

It looks like something has been changed since I submitted the "disable hibernation
mode " patch. In previous test, I only need to unplug the cable and then use ifconfig
cmd to disable the interface, wait more than 10 seconds, then use ifconfig cmd to
enable the interface.

> - Wait longer than 10 seconds
> - If possible, measure AR8031 PHY pin 33 RX_CLK, wait for the RX_CLK to
>    be turned OFF by the PHY (means PHY entered hibernation)
> - ifconfig ethN up -- try to bring up the EQoS MAC <observe failure>
> 
> [...]

For the patch, I think your approach is better than mine, but I have a suggestion,
is the following modification more appropriate?

--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -991,12 +991,28 @@ static int at8031_pll_config(struct phy_device *phydev)
 static int at803x_hibernation_mode_config(struct phy_device *phydev)
 {
        struct at803x_priv *priv = phydev->priv;
+       int ret;

        /* The default after hardware reset is hibernation mode enabled. After
         * software reset, the value is retained.
         */
-       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
-               return 0;
+       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE)) {
+               /* Toggle hibernation mode OFF and ON to wake the PHY up and
+                * make it generate clock on RX_CLK pin for about 10 seconds.
+                * These clock are needed during start up by MACs like DWMAC
+                * in NXP i.MX8M Plus to release their DMA from reset. After
+                * the MAC has started up, the PHY can enter hibernation and
+                * disable the RX_CLK clock, this poses no problem for the MAC.
+                */
+               ret = at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+                                           AT803X_DEBUG_HIB_CTRL_PS_HIB_EN, 0);
+               if (ret < 0)
+                       return ret;
+
+               return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+                                            AT803X_DEBUG_HIB_CTRL_PS_HIB_EN,
+                                            AT803X_DEBUG_HIB_CTRL_PS_HIB_EN);
+       }

        return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Oleksij Rempel]

On Wed, Aug 09, 2023 at 02:27:12AM +0000, Wei Fang wrote:
> > >> Toggle hibernation mode OFF and ON to wake the PHY up and make it
> > >> generate clock on RX_CLK pin for about 10 seconds.
> > >> These clock are needed during start up by MACs like DWMAC in NXP
> > >> i.MX8M Plus to release their DMA from reset. After the MAC has
> > >> started up, the PHY can enter hibernation and disable the RX_CLK
> > >> clock, this poses no problem for the MAC.
> > >>
> > >> Originally, this issue has been described by NXP in commit
> > >> 9ecf04016c87 ("net: phy: at803x: add disable hibernation mode
> > >> support") but this approach fully disables the hibernation support
> > >> and takes away any power saving benefit. This patch instead makes the
> > >> PHY generate the clock on start up for 10 seconds, which should be
> > >> long enough for the EQoS MAC to release DMA from reset.
> > >>
> > >> Before this patch on i.MX8M Plus board with AR8031 PHY:
> > >> "
> > >> $ ifconfig eth1 up
> > >> [   25.576734] imx-dwmac 30bf0000.ethernet eth1: Register
> > >> MEM_TYPE_PAGE_POOL RxQ-0
> > >> [   25.658916] imx-dwmac 30bf0000.ethernet eth1: PHY [stmmac-1:00]
> > >> driver [Qualcomm Atheros AR8031/AR8033] (irq=38)
> > >> [   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
> > >> [   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup:
> > >> DMA engine initialization failed
> > >> [   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open:
> > Hw
> > >> setup failed
> > >> ifconfig: SIOCSIFFLAGS: Connection timed out "
> > >>
> > >
> > > Have you reproduced this issue based on the upstream net-next or net
> > tree?
> > 
> > On current linux-next next-20230808 so 6.5.0-rc5 . As far as I can tell,
> > net-next is merged into this tree too.
> > 
> 
> > > If so, can this issue be reproduced? The reason why I ask this is
> > > because when I tried to reproduce this problem on net-next 6.3.0
> > > version, I found that it could not be reproduced (I did not disable
> > > hibernation mode when I reproduced this issue ). So I guess maybe other
> > patches in eqos driver fixed the issue.
> > 
> > This is what I use for testing:
> > 
> > - Make sure "qca,disable-hibernation-mode" is NOT present in PHY DT node
> Yes, I deleted this property when I reproduced this issue.
> 
> > - Boot the machine with NO ethernet cable plugged into the affected port
> >    (i.e. the EQoS port), this is important
> > - Make sure the EQoS MAC is not brought up e.g. by systemd-networkd or
> >    whatever other tool, I use busybox initramfs for testing with plain
> >    script as init (it mounts the various filesystems and runs /bin/sh)
> 
> It looks like something has been changed since I submitted the "disable hibernation
> mode " patch. In previous test, I only need to unplug the cable and then use ifconfig
> cmd to disable the interface, wait more than 10 seconds, then use ifconfig cmd to
> enable the interface.
> 
> > - Wait longer than 10 seconds
> > - If possible, measure AR8031 PHY pin 33 RX_CLK, wait for the RX_CLK to
> >    be turned OFF by the PHY (means PHY entered hibernation)
> > - ifconfig ethN up -- try to bring up the EQoS MAC <observe failure>
> > 
> > [...]
> 
> For the patch, I think your approach is better than mine, but I have a suggestion,
> is the following modification more appropriate?
> 
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -991,12 +991,28 @@ static int at8031_pll_config(struct phy_device *phydev)
>  static int at803x_hibernation_mode_config(struct phy_device *phydev)
>  {
>         struct at803x_priv *priv = phydev->priv;
> +       int ret;
> 
>         /* The default after hardware reset is hibernation mode enabled. After
>          * software reset, the value is retained.
>          */
> -       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
> -               return 0;
> +       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE)) {
> +               /* Toggle hibernation mode OFF and ON to wake the PHY up and
> +                * make it generate clock on RX_CLK pin for about 10 seconds.
> +                * These clock are needed during start up by MACs like DWMAC
> +                * in NXP i.MX8M Plus to release their DMA from reset. After
> +                * the MAC has started up, the PHY can enter hibernation and
> +                * disable the RX_CLK clock, this poses no problem for the MAC.
> +                */
> +               ret = at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
> +                                           AT803X_DEBUG_HIB_CTRL_PS_HIB_EN, 0);
> +               if (ret < 0)
> +                       return ret;
> +
> +               return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
> +                                            AT803X_DEBUG_HIB_CTRL_PS_HIB_EN,
> +                                            AT803X_DEBUG_HIB_CTRL_PS_HIB_EN);
> +       }
> 
>         return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,

Hm.. how about officially defining this PHY as the clock provider and
disable PHY automatic hibernation as long as clock is acquired?

Regards,
Oleksij
-- 
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 |

RE: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Wei Fang]

> > For the patch, I think your approach is better than mine, but I have a
> > suggestion, is the following modification more appropriate?
> >
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -991,12 +991,28 @@ static int at8031_pll_config(struct phy_device
> > *phydev)  static int at803x_hibernation_mode_config(struct phy_device
> > *phydev)  {
> >         struct at803x_priv *priv = phydev->priv;
> > +       int ret;
> >
> >         /* The default after hardware reset is hibernation mode enabled.
> After
> >          * software reset, the value is retained.
> >          */
> > -       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
> > -               return 0;
> > +       if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE)) {
> > +               /* Toggle hibernation mode OFF and ON to wake the
> PHY up and
> > +                * make it generate clock on RX_CLK pin for about 10
> seconds.
> > +                * These clock are needed during start up by MACs like
> DWMAC
> > +                * in NXP i.MX8M Plus to release their DMA from reset.
> After
> > +                * the MAC has started up, the PHY can enter
> hibernation and
> > +                * disable the RX_CLK clock, this poses no problem for
> the MAC.
> > +                */
> > +               ret = at803x_debug_reg_mask(phydev,
> AT803X_DEBUG_REG_HIB_CTRL,
> > +
> AT803X_DEBUG_HIB_CTRL_PS_HIB_EN, 0);
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               return at803x_debug_reg_mask(phydev,
> AT803X_DEBUG_REG_HIB_CTRL,
> > +
> AT803X_DEBUG_HIB_CTRL_PS_HIB_EN,
> > +
> AT803X_DEBUG_HIB_CTRL_PS_HIB_EN);
> > +       }
> >
> >         return at803x_debug_reg_mask(phydev,
> > AT803X_DEBUG_REG_HIB_CTRL,
> 
> Hm.. how about officially defining this PHY as the clock provider and disable
> PHY automatic hibernation as long as clock is acquired?
> 
Sorry, I don't know much about the clock provider/consumer, but I think there
will be more changes if we use clock provider/consume mechanism. Furthermore,
we would expect the hibernation mode is enabled when the ethernet interface
is brought up but the cable is not plugged, that is to say, we only need the PHY to
provide the clock for a while to make the MAC reset successfully. Therefore, I think
the current approach is more simple and effective, and it takes full advantage of the
characteristics of the hardware (The PHY will continue to provide the clock about
10 seconds after hibernation mode is enabled when the cable is not plugged and
automatically disable the clock after 10 seconds, so the 10 seconds is enough for
the MAC to reset successfully).

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Oleksij Rempel]

CCing clk folks in the loop.

On Wed, Aug 09, 2023 at 05:37:45AM +0000, Wei Fang wrote:
...
> > Hm.. how about officially defining this PHY as the clock provider and disable
> > PHY automatic hibernation as long as clock is acquired?
> > 
> Sorry, I don't know much about the clock provider/consumer, but I think there
> will be more changes if we use clock provider/consume mechanism.

Yes, more changes will be needed.

> Furthermore,
> we would expect the hibernation mode is enabled when the ethernet interface
> is brought up but the cable is not plugged, that is to say, we only need the PHY to
> provide the clock for a while to make the MAC reset successfully. 

Means, if external clock is not provided, MAC is not fully functional.
Correct?

What kind of MAC operation will fail in this case?
For example, if stmmac_open() fails without external clock, will
stmmac_release() work properly? Will we be able to do any configuration
on an interface which is opened, but without active link and hibernated
clock? How about self tests?

> Therefore, I think
> the current approach is more simple and effective, and it takes full advantage of the
> characteristics of the hardware (The PHY will continue to provide the clock about
> 10 seconds after hibernation mode is enabled when the cable is not plugged and
> automatically disable the clock after 10 seconds, so the 10 seconds is enough for
> the MAC to reset successfully).

If multiple independent operations are synchronized based on the
assumption that 10 seconds should be enough, bad thing happens.

If fully functional external clock provider is need to initialize the
MAC, just disabling this clock on already initialized HW without doing
proper re-initialization sequence is usually bad idea. HW may get some
glitch which will make troubleshooting a pain.

Regards,
Oleksij
-- 
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 |

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 08:08:36AM +0200, Oleksij Rempel wrote:
> If fully functional external clock provider is need to initialize the
> MAC, just disabling this clock on already initialized HW without doing
> proper re-initialization sequence is usually bad idea. HW may get some
> glitch which will make troubleshooting a pain.

There are cases where the PHY sits on a MDIO bus that is created
by the ethernet MAC driver, which means the PHY only exists during
the ethernet MAC driver probe.

I think that provided the clock is only obtained after we know the
PHY is present, that would probably be fine - but doing it before
the MDIO bus has been created will of course cause problems.

We've had these issues before with stmmac, so this "stmmac needs the
PHY receive clock" is nothing new - it's had problems with system
suspend/resume in the past, and I've made suggestions... and when
there's been two people trying to work on it, I've attempted to get
them to talk to each other which resulted in nothing further
happening.

Another solution could possibly be that we reserve bit 30 on the
PHY dev_flags to indicate that the receive clock must always be
provided. I suspect that would have an advantage in another
situation - for EEE, there's a control bit which allows the
receive clock to be stopped while the link is in low-power state.
If the MAC always needs the receive clock, then obviously that
should be blocked.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 09:43:49AM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 09, 2023 at 08:08:36AM +0200, Oleksij Rempel wrote:
> > If fully functional external clock provider is need to initialize the
> > MAC, just disabling this clock on already initialized HW without doing
> > proper re-initialization sequence is usually bad idea. HW may get some
> > glitch which will make troubleshooting a pain.
> 
> There are cases where the PHY sits on a MDIO bus that is created
> by the ethernet MAC driver, which means the PHY only exists during
> the ethernet MAC driver probe.
> 
> I think that provided the clock is only obtained after we know the
> PHY is present, that would probably be fine - but doing it before
> the MDIO bus has been created will of course cause problems.
> 
> We've had these issues before with stmmac, so this "stmmac needs the
> PHY receive clock" is nothing new - it's had problems with system
> suspend/resume in the past, and I've made suggestions... and when
> there's been two people trying to work on it, I've attempted to get
> them to talk to each other which resulted in nothing further
> happening.
> 
> Another solution could possibly be that we reserve bit 30 on the
> PHY dev_flags to indicate that the receive clock must always be
> provided. I suspect that would have an advantage in another
> situation - for EEE, there's a control bit which allows the
> receive clock to be stopped while the link is in low-power state.
> If the MAC always needs the receive clock, then obviously that
> should be blocked.

Something like this for starters:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fcab363d8dfa..a954f1d61709 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1254,6 +1254,11 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
 	priv->phylink_config.mac_managed_pm = true;
 
+	/* stmmac always requires a receive clock in order for things like
+	 * hardware reset to work.
+	 */
+	priv->phylink_config.mac_requires_rxc = true;
+
 	phylink = phylink_create(&priv->phylink_config, fwnode,
 				 mode, &stmmac_phylink_mac_ops);
 	if (IS_ERR(phylink))
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 13c4121fa309..619a63a0d14f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -990,7 +990,8 @@ static int at803x_hibernation_mode_config(struct phy_device *phydev)
 	/* The default after hardware reset is hibernation mode enabled. After
 	 * software reset, the value is retained.
 	 */
-	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
+	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE) &&
+	    !(phydev->dev_flags & PHY_F_RXC_ALWAYS_ON))
 		return 0;
 
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e9909b30938..4d1a37487923 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
 			goto out;
 	}
 
+        phy_disable_interrupts(phydev);
+
 	/* Start out supporting everything. Eventually,
 	 * a controller will attach, and may modify one
 	 * or both of these values
@@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
 	return 0;
 }
 
-static void phy_shutdown(struct device *dev)
-{
-	struct phy_device *phydev = to_phy_device(dev);
-
-	if (phydev->state == PHY_READY || !phydev->attached_dev)
-		return;
-
-	phy_disable_interrupts(phydev);
-}
-
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -3376,7 +3368,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.bus = &mdio_bus_type;
 	new_driver->mdiodrv.driver.probe = phy_probe;
 	new_driver->mdiodrv.driver.remove = phy_remove;
-	new_driver->mdiodrv.driver.shutdown = phy_shutdown;
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4f1c8bb199e9..6568a2759101 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1830,6 +1830,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 			      phy_interface_t interface)
 {
+	u32 flags = 0;
+
 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
 		     phy_interface_mode_is_8023z(interface) && !pl->sfp_bus)))
@@ -1838,7 +1840,10 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->phydev)
 		return -EBUSY;
 
-	return phy_attach_direct(pl->netdev, phy, 0, interface);
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
+	return phy_attach_direct(pl->netdev, phy, flags, interface);
 }
 
 /**
@@ -1941,6 +1946,9 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	phy_device_free(phy_dev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ba08b0e60279..79df5e01707d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -761,6 +761,7 @@ struct phy_device {
 
 /* Generic phy_device::dev_flags */
 #define PHY_F_NO_IRQ		0x80000000
+#define PHY_F_RXC_ALWAYS_ON	BIT(30)
 
 static inline struct phy_device *to_phy_device(const struct device *dev)
 {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 789c516c6b4a..a83c1a77338f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -204,6 +204,7 @@ enum phylink_op_type {
  * @poll_fixed_state: if true, starts link_poll,
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
+ * @mac_requires_rxc: if true, the MAC always requires a receive clock from PHY.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
@@ -216,6 +217,7 @@ struct phylink_config {
 	enum phylink_op_type type;
 	bool poll_fixed_state;
 	bool mac_managed_pm;
+	bool mac_requires_rxc;
 	bool ovr_an_inband;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Romain Gantois]

Hello Russell,

On Thu, 10 Aug 2023, Russell King (Oracle) wrote:

> > We've had these issues before with stmmac, so this "stmmac needs the
> > PHY receive clock" is nothing new - it's had problems with system
> > suspend/resume in the past, and I've made suggestions... and when
> > there's been two people trying to work on it, I've attempted to get
> > them to talk to each other which resulted in nothing further
> > happening.
> > 
> > Another solution could possibly be that we reserve bit 30 on the
> > PHY dev_flags to indicate that the receive clock must always be
> > provided. I suspect that would have an advantage in another
> ...
> 
> Something like this for starters:
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> ...

I've implemented and tested the general-case solution you proposed to this 
receive clock issue with stmmac drivers. The core of your suggestion is pretty 
much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that 
also need to provide the receive clock.

I'd like to send a series for this upstream, which would allow solving this 
issue for both the DWMAC RZN1 case and the AT803x PHY suspend/hibernate case 
(and also potentially other cases with a similar bug).

I wanted to ask you how you would prefer to be credited in my patch series. I 
was considering putting you as author and first signer of the initial patch 
adding the phy_dev flag. Would that be okay or would you prefer something else?

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Marek Vasut]

On 12/14/23 09:13, Romain Gantois wrote:
> 
> Hello Russell,
> 
> On Thu, 10 Aug 2023, Russell King (Oracle) wrote:
> 
>>> We've had these issues before with stmmac, so this "stmmac needs the
>>> PHY receive clock" is nothing new - it's had problems with system
>>> suspend/resume in the past, and I've made suggestions... and when
>>> there's been two people trying to work on it, I've attempted to get
>>> them to talk to each other which resulted in nothing further
>>> happening.
>>>
>>> Another solution could possibly be that we reserve bit 30 on the
>>> PHY dev_flags to indicate that the receive clock must always be
>>> provided. I suspect that would have an advantage in another
>> ...
>>
>> Something like this for starters:
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> ...
> 
> I've implemented and tested the general-case solution you proposed to this
> receive clock issue with stmmac drivers. The core of your suggestion is pretty
> much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that
> also need to provide the receive clock.
> 
> I'd like to send a series for this upstream, which would allow solving this
> issue for both the DWMAC RZN1 case and the AT803x PHY suspend/hibernate case
> (and also potentially other cases with a similar bug).
> 
> I wanted to ask you how you would prefer to be credited in my patch series. I
> was considering putting you as author and first signer of the initial patch
> adding the phy_dev flag. Would that be okay or would you prefer something else?

Credit it whichever way you see fit, don't worry, better focus on the 
fix. I can test the result on MX8MM/MX8MP, so feel free to CC me on that.

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Dec 14, 2023 at 09:13:58AM +0100, Romain Gantois wrote:
> Hello Russell,
> 
> I've implemented and tested the general-case solution you proposed to this 
> receive clock issue with stmmac drivers. The core of your suggestion is pretty 
> much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that 
> also need to provide the receive clock.

So this affects the ability of PCS to operate correctly as well as MACs?
Would you enlighten which PCS are affected, and what PCS <--> PHY link
modes this is required for?

> I'd like to send a series for this upstream, which would allow solving this 
> issue for both the DWMAC RZN1 case and the AT803x PHY suspend/hibernate case 
> (and also potentially other cases with a similar bug).
> 
> I wanted to ask you how you would prefer to be credited in my patch series. I 
> was considering putting you as author and first signer of the initial patch 
> adding the phy_dev flag. Would that be okay or would you prefer something else?

It depends how big the changes are from my patches. If more than 50% of
the patch remains my work, please retain my authorship. If you wish to
also indicate your authorship, then there is a mechanism to do that -
Co-developed-by: from submitting-patches.rst:

Co-developed-by: states that the patch was co-created by multiple
developers; it is used to give attribution to co-authors (in addition
to the author attributed by the From: tag) when several people work on
a single patch. Since Co-developed-by: denotes authorship, every
Co-developed-by: must be immediately followed by a Signed-off-by: of
the associated co-author.

See submitting-patches.rst for examples of the ordering of the
attributations.

Thanks for asking.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Romain Gantois]

On Thu, 14 Dec 2023, Russell King (Oracle) wrote:

> On Thu, Dec 14, 2023 at 09:13:58AM +0100, Romain Gantois wrote:
> > Hello Russell,
> > 
> > I've implemented and tested the general-case solution you proposed to this 
> > receive clock issue with stmmac drivers. The core of your suggestion is pretty 
> > much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that 
> > also need to provide the receive clock.
> 
> So this affects the ability of PCS to operate correctly as well as MACs?
> Would you enlighten which PCS are affected, and what PCS <--> PHY link
> modes this is required for?

The affected hardware is the RZN1 GMAC1 that is found in the r9a06g032 SoC from 
Renesas. This MAC controller is internally connected to a custom PCS that
functions as a RGMII converter. This converter is handled by a standalone PCS 
driver that is already upstream, unlike the GMAC1 driver. So in hardware, the 
MAC/PHY links are organised this way:

    RZN1 GMAC1 <--[GMII]--> MII_CONV1 (internal) <--[RGMII]--> external PHY

The issue is that the RX clock from the external PHY has to be transmitted by 
the MII converter before it reaches GMAC1. Therefore, if the RZN1 PCS driver 
isn't configured to let the clock through before the stmmac GMAC1 driver 
initializes its hardware, then said initialization will fail with a vague DMA 
error.

To solve this, I added a flag to phylink_pcs and made the phylink core set it in 
phylink_validate_mac_and_pcs(). This gives the PCS driver a chance to check the 
flag in pcs_validate() and allow the clock through before the GMAC1 net device 
is brought up.

Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Wei Fang]

> > Furthermore,
> > we would expect the hibernation mode is enabled when the ethernet
> > interface is brought up but the cable is not plugged, that is to say,
> > we only need the PHY to provide the clock for a while to make the MAC
> reset successfully.
> 
> Means, if external clock is not provided, MAC is not fully functional.
> Correct?
> 
The MAC will failed to do software reset if the PHY input clocks are not
present. According to the datasheet from Synopsys, the software reset
is for the MAC and the DMA controller reset the logic and all internal
registers of the DMA, MTL, and MAC.

One obvious error log can be seen from the console shows as follows.
[   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
[   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup: DMA engine initialization failed
[   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open: Hw setup failed

I believe other manufacturers who use the dwmac IP also have encountered
the similar issue and use other methods to resolve it, such as intel,
49725ffc15fc ("net: stmmac: power up/down serdes in stmmac_open/release ")

> What kind of MAC operation will fail in this case?
Below are the steps from Marek to reproduce the issue.
- Make sure "qca,disable-hibernation-mode" is NOT present in PHY DT node
- Boot the machine with NO ethernet cable plugged into the affected port
   (i.e. the EQoS port), this is important
- Make sure the EQoS MAC is not brought up e.g. by systemd-networkd or
   whatever other tool, I use busybox initramfs for testing with plain
   script as init (it mounts the various filesystems and runs /bin/sh)
- Wait longer than 10 seconds
- If possible, measure AR8031 PHY pin 33 RX_CLK, wait for the RX_CLK to
   be turned OFF by the PHY (means PHY entered hibernation)
- ifconfig ethN up -- try to bring up the EQoS MAC <observe failure>

But the situation I encountered on i.MX8DXL-EVK platform was not as
complicated as above. At that time, it only took 3 steps to reproduce
this issue. But now, the latest upstream tree cannot reproduce this
issue based on the following three steps. Something has been changed,
I think it should be feafeb53140a (arm64: dts: imx8dxl-evk: Fix eqos phy reset gpio).
- unplug the cable
- ifconfig eth0 down and wait for about 10 seconds
- ifconfig eth0 up

> For example, if stmmac_open() fails without external clock, will
> stmmac_release() work properly?
Actually, I don't know much about stmmac driver and the dwmac IP,
Because I'm not responsible for this IP on NXP i.MX platform. But I
have a look at the code of stmmac_release(), I think stmmac_release()
will work properly, because it invokes phylink_stop() and phylink_disconnect_phy()
first which will disable the clock from PHY.

>Will we be able to do any configuration on
> an interface which is opened, but without active link and hibernated clock?
I don't know, but as far as I know, we can not set the VLAN filter successfully
due to lack of clock. So there may be other configurations that depend on the
clock.

> How about self tests?
> 
> > Therefore, I think
> > the current approach is more simple and effective, and it takes full
> > advantage of the characteristics of the hardware (The PHY will
> > continue to provide the clock about
> > 10 seconds after hibernation mode is enabled when the cable is not
> > plugged and automatically disable the clock after 10 seconds, so the
> > 10 seconds is enough for the MAC to reset successfully).
> 
> If multiple independent operations are synchronized based on the
> assumption that 10 seconds should be enough, bad thing happens.
> 
> If fully functional external clock provider is need to initialize the MAC, just
> disabling this clock on already initialized HW without doing proper
> re-initialization sequence is usually bad idea. HW may get some glitch which
> will make troubleshooting a pain.
> 

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 03:28:13AM +0000, Wei Fang wrote:
> > > Furthermore,
> > > we would expect the hibernation mode is enabled when the ethernet
> > > interface is brought up but the cable is not plugged, that is to say,
> > > we only need the PHY to provide the clock for a while to make the MAC
> > reset successfully.
> > 
> > Means, if external clock is not provided, MAC is not fully functional.
> > Correct?
> > 
> The MAC will failed to do software reset if the PHY input clocks are not
> present.

Yes, that's well known to me - it's been brought up before with stmmac
and phylink. I said earlier in this thread about this.

> > For example, if stmmac_open() fails without external clock, will
> > stmmac_release() work properly?
> Actually, I don't know much about stmmac driver and the dwmac IP,
> Because I'm not responsible for this IP on NXP i.MX platform. But I
> have a look at the code of stmmac_release(), I think stmmac_release()
> will work properly, because it invokes phylink_stop() and phylink_disconnect_phy()
> first which will disable the clock from PHY.

When tearing down a network device, there should be no reason to call
phylink_stop() - unregistering the netdev will take the network device
down, and thus call the .ndo_stop method if the device was up. .ndo_stop
should already be calling phylink_stop().

I just don't get why driver authors don't check things like this...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Andrew Lunn]

> > Hm.. how about officially defining this PHY as the clock provider and disable
> > PHY automatic hibernation as long as clock is acquired?
> > 
> Sorry, I don't know much about the clock provider/consumer, but I think there
> will be more changes if we use clock provider/consume mechanism.

Less changes is not always best. What happens when a different PHY is
used? By having a clock provider in the PHY, you are defining a clear
API that any PHY needs to provide to work with this MAC.

As Russell has point out, this is not the first time we have run into
this problem. So far, it seems everybody has tried to solve it for
just their system. So maybe now we should look at the whole picture
and put in place a generic solution which can be made to work for any
PHY.

    Andrew

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Marek Vasut]

On 8/9/23 15:40, Andrew Lunn wrote:
>>> Hm.. how about officially defining this PHY as the clock provider and disable
>>> PHY automatic hibernation as long as clock is acquired?
>>>
>> Sorry, I don't know much about the clock provider/consumer, but I think there
>> will be more changes if we use clock provider/consume mechanism.
> 
> Less changes is not always best. What happens when a different PHY is
> used?

Then the system wouldn't be affected by this AR803x specific behavior.

> By having a clock provider in the PHY, you are defining a clear
> API that any PHY needs to provide to work with this MAC.
> 
> As Russell has point out, this is not the first time we have run into
> this problem. So far, it seems everybody has tried to solve it for
> just their system. So maybe now we should look at the whole picture
> and put in place a generic solution which can be made to work for any
> PHY.

I'll see what I can do, it might take a while though.

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Andrew Lunn]

On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> On 8/9/23 15:40, Andrew Lunn wrote:
> > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > PHY automatic hibernation as long as clock is acquired?
> > > > 
> > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > will be more changes if we use clock provider/consume mechanism.
> > 
> > Less changes is not always best. What happens when a different PHY is
> > used?
> 
> Then the system wouldn't be affected by this AR803x specific behavior.

Do you know it really is specific to the AR803x? Turning the clock off
seams a reasonable thing to do when saving power, or when there is no
link partner.

     Andrew

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Marek Vasut]

On 8/10/23 00:06, Andrew Lunn wrote:
> On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
>> On 8/9/23 15:40, Andrew Lunn wrote:
>>>>> Hm.. how about officially defining this PHY as the clock provider and disable
>>>>> PHY automatic hibernation as long as clock is acquired?
>>>>>
>>>> Sorry, I don't know much about the clock provider/consumer, but I think there
>>>> will be more changes if we use clock provider/consume mechanism.
>>>
>>> Less changes is not always best. What happens when a different PHY is
>>> used?
>>
>> Then the system wouldn't be affected by this AR803x specific behavior.
> 
> Do you know it really is specific to the AR803x? Turning the clock off
> seams a reasonable thing to do when saving power, or when there is no
> link partner.

This hibernation behavior seem specific to this PHY, I haven't seen it 
on another PHY connected to the EQoS so far.

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Oleksij Rempel]

On Thu, Aug 10, 2023 at 02:49:55AM +0200, Marek Vasut wrote:
> On 8/10/23 00:06, Andrew Lunn wrote:
> > On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > > 
> > > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > > will be more changes if we use clock provider/consume mechanism.
> > > > 
> > > > Less changes is not always best. What happens when a different PHY is
> > > > used?
> > > 
> > > Then the system wouldn't be affected by this AR803x specific behavior.
> > 
> > Do you know it really is specific to the AR803x? Turning the clock off
> > seams a reasonable thing to do when saving power, or when there is no
> > link partner.
> 
> This hibernation behavior seem specific to this PHY, I haven't seen it on
> another PHY connected to the EQoS so far.

Hm.. if I see it correctly I have right now kind of similar issues with
Microchip KSZ9893 switch attached to EQoS. The switch is clock
provider and I need to make sure that switch has CPU port enabled before
probing the ethernet controller.

-- 
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 |

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 06:32:42AM +0200, Oleksij Rempel wrote:
> On Thu, Aug 10, 2023 at 02:49:55AM +0200, Marek Vasut wrote:
> > On 8/10/23 00:06, Andrew Lunn wrote:
> > > On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > > > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > > > 
> > > > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > > > will be more changes if we use clock provider/consume mechanism.
> > > > > 
> > > > > Less changes is not always best. What happens when a different PHY is
> > > > > used?
> > > > 
> > > > Then the system wouldn't be affected by this AR803x specific behavior.
> > > 
> > > Do you know it really is specific to the AR803x? Turning the clock off
> > > seams a reasonable thing to do when saving power, or when there is no
> > > link partner.
> > 
> > This hibernation behavior seem specific to this PHY, I haven't seen it on
> > another PHY connected to the EQoS so far.
> 
> Hm.. if I see it correctly I have right now kind of similar issues with
> Microchip KSZ9893 switch attached to EQoS. The switch is clock
> provider and I need to make sure that switch has CPU port enabled before
> probing the ethernet controller.

... and Cadence macb.

So, this is a thing, and we need to be able to cater for it.

Can we agree that:

(a) some ethernet MAC controllers need the RGMII receive clock to
function.

(b) IEEE 802.3 permits clocks to be disabled when entering EEE low
power state, and there is a defined control bit to allow this to
happen - so IEEE 802.3 _recognises_ that some MAC controllers need
this clock whereas others do not.

Therefore:

(c) Given that IEEE 802.3 appears to recognise this, we should add
some sort of control to phylib so that MACs can tell phylib that they
require the receive clock to always be present.

(d) We need to ensure that PHY drivers respect that bit and program
the hardware appropriately to ensure that where a MAC always needs
a receive clock, it is maintained.

(e) MACs that always need the receive clock enabled need to indicate
to phylib/phylink that this is the case.

My suggestion is to use a bit on the PHY device dev_flags (bit 30)
for this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 02:49:55AM +0200, Marek Vasut wrote:
> On 8/10/23 00:06, Andrew Lunn wrote:
> > On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > > 
> > > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > > will be more changes if we use clock provider/consume mechanism.
> > > > 
> > > > Less changes is not always best. What happens when a different PHY is
> > > > used?
> > > 
> > > Then the system wouldn't be affected by this AR803x specific behavior.
> > 
> > Do you know it really is specific to the AR803x? Turning the clock off
> > seams a reasonable thing to do when saving power, or when there is no
> > link partner.
> 
> This hibernation behavior seem specific to this PHY, I haven't seen it on
> another PHY connected to the EQoS so far.

Marvell PHYs can be programmed so that RXCLK stops when the PHY
enters power down or energy-detect state, although it defaults to
always keeping the RGMII interface powered (and thus providing a
clock.)

One Micrel PHY - "To save more power, the KSZ9031RNX stops the RX_CLK
clock output to the MAC after 10 or more RX_CLK clock
cycles have occurred in the receive LPI state." which seems to imply
if EEE is enabled, then the receive clock will be stopped when
entering low-power state.

I've said this several times in this thread - I think we need a bit
in the PHY device's dev_flags to allow the MAC to say "do not power
down the receive clock" which is used by the PHY drivers to (a) program
the hardware to prevent the receive clock being stopped in situations
such as the AR803x hibernate mode, and (b) to program the hardware not
to stop the receive clock when entering EEE low power. This does seem
to be a generic thing and not specific to just one PHY - especially as
the stopping of clocks when entering EEE low power is a IEEE 802.3
defined thing.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 11:01:53AM +0100, Russell King (Oracle) wrote:
> On Thu, Aug 10, 2023 at 02:49:55AM +0200, Marek Vasut wrote:
> > On 8/10/23 00:06, Andrew Lunn wrote:
> > > On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > > > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > > > 
> > > > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > > > will be more changes if we use clock provider/consume mechanism.
> > > > > 
> > > > > Less changes is not always best. What happens when a different PHY is
> > > > > used?
> > > > 
> > > > Then the system wouldn't be affected by this AR803x specific behavior.
> > > 
> > > Do you know it really is specific to the AR803x? Turning the clock off
> > > seams a reasonable thing to do when saving power, or when there is no
> > > link partner.
> > 
> > This hibernation behavior seem specific to this PHY, I haven't seen it on
> > another PHY connected to the EQoS so far.
> 
> Marvell PHYs can be programmed so that RXCLK stops when the PHY
> enters power down or energy-detect state, although it defaults to
> always keeping the RGMII interface powered (and thus providing a
> clock.)
> 
> One Micrel PHY - "To save more power, the KSZ9031RNX stops the RX_CLK
> clock output to the MAC after 10 or more RX_CLK clock
> cycles have occurred in the receive LPI state." which seems to imply
> if EEE is enabled, then the receive clock will be stopped when
> entering low-power state.
> 
> I've said this several times in this thread - I think we need a bit
> in the PHY device's dev_flags to allow the MAC to say "do not power
> down the receive clock" which is used by the PHY drivers to (a) program
> the hardware to prevent the receive clock being stopped in situations
> such as the AR803x hibernate mode, and (b) to program the hardware not
> to stop the receive clock when entering EEE low power. This does seem
> to be a generic thing and not specific to just one PHY - especially as
> the stopping of clocks when entering EEE low power is a IEEE 802.3
> defined thing.

Like this:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fcab363d8dfa..a954f1d61709 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1254,6 +1254,11 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
 	priv->phylink_config.mac_managed_pm = true;
 
+	/* stmmac always requires a receive clock in order for things like
+	 * hardware reset to work.
+	 */
+	priv->phylink_config.mac_requires_rxc = true;
+
 	phylink = phylink_create(&priv->phylink_config, fwnode,
 				 mode, &stmmac_phylink_mac_ops);
 	if (IS_ERR(phylink))
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 13c4121fa309..619a63a0d14f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -990,7 +990,8 @@ static int at803x_hibernation_mode_config(struct phy_device *phydev)
 	/* The default after hardware reset is hibernation mode enabled. After
 	 * software reset, the value is retained.
 	 */
-	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
+	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE) &&
+	    !(phydev->dev_flags & PHY_F_RXC_ALWAYS_ON))
 		return 0;
 
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e9909b30938..4d1a37487923 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
 			goto out;
 	}
 
+        phy_disable_interrupts(phydev);
+
 	/* Start out supporting everything. Eventually,
 	 * a controller will attach, and may modify one
 	 * or both of these values
@@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
 	return 0;
 }
 
-static void phy_shutdown(struct device *dev)
-{
-	struct phy_device *phydev = to_phy_device(dev);
-
-	if (phydev->state == PHY_READY || !phydev->attached_dev)
-		return;
-
-	phy_disable_interrupts(phydev);
-}
-
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -3376,7 +3368,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.bus = &mdio_bus_type;
 	new_driver->mdiodrv.driver.probe = phy_probe;
 	new_driver->mdiodrv.driver.remove = phy_remove;
-	new_driver->mdiodrv.driver.shutdown = phy_shutdown;
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4f1c8bb199e9..6568a2759101 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1830,6 +1830,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 			      phy_interface_t interface)
 {
+	u32 flags = 0;
+
 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
 		     phy_interface_mode_is_8023z(interface) && !pl->sfp_bus)))
@@ -1838,7 +1840,10 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->phydev)
 		return -EBUSY;
 
-	return phy_attach_direct(pl->netdev, phy, 0, interface);
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
+	return phy_attach_direct(pl->netdev, phy, flags, interface);
 }
 
 /**
@@ -1941,6 +1946,9 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	phy_device_free(phy_dev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ba08b0e60279..79df5e01707d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -761,6 +761,7 @@ struct phy_device {
 
 /* Generic phy_device::dev_flags */
 #define PHY_F_NO_IRQ		0x80000000
+#define PHY_F_RXC_ALWAYS_ON	BIT(30)
 
 static inline struct phy_device *to_phy_device(const struct device *dev)
 {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 789c516c6b4a..a83c1a77338f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -204,6 +204,7 @@ enum phylink_op_type {
  * @poll_fixed_state: if true, starts link_poll,
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
+ * @mac_requires_rxc: if true, the MAC always requires a receive clock from PHY.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
@@ -216,6 +217,7 @@ struct phylink_config {
 	enum phylink_op_type type;
 	bool poll_fixed_state;
 	bool mac_managed_pm;
+	bool mac_requires_rxc;
 	bool ovr_an_inband;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Oleksij Rempel]

On Thu, Aug 10, 2023 at 11:34:02AM +0100, Russell King (Oracle) wrote:
> On Thu, Aug 10, 2023 at 11:01:53AM +0100, Russell King (Oracle) wrote:
> > On Thu, Aug 10, 2023 at 02:49:55AM +0200, Marek Vasut wrote:
> > > On 8/10/23 00:06, Andrew Lunn wrote:
> > > > On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > > > > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > > > > 
> > > > > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > > > > will be more changes if we use clock provider/consume mechanism.
> > > > > > 
> > > > > > Less changes is not always best. What happens when a different PHY is
> > > > > > used?
> > > > > 
> > > > > Then the system wouldn't be affected by this AR803x specific behavior.
> > > > 
> > > > Do you know it really is specific to the AR803x? Turning the clock off
> > > > seams a reasonable thing to do when saving power, or when there is no
> > > > link partner.
> > > 
> > > This hibernation behavior seem specific to this PHY, I haven't seen it on
> > > another PHY connected to the EQoS so far.
> > 
> > Marvell PHYs can be programmed so that RXCLK stops when the PHY
> > enters power down or energy-detect state, although it defaults to
> > always keeping the RGMII interface powered (and thus providing a
> > clock.)
> > 
> > One Micrel PHY - "To save more power, the KSZ9031RNX stops the RX_CLK
> > clock output to the MAC after 10 or more RX_CLK clock
> > cycles have occurred in the receive LPI state." which seems to imply
> > if EEE is enabled, then the receive clock will be stopped when
> > entering low-power state.
> > 
> > I've said this several times in this thread - I think we need a bit
> > in the PHY device's dev_flags to allow the MAC to say "do not power
> > down the receive clock" which is used by the PHY drivers to (a) program
> > the hardware to prevent the receive clock being stopped in situations
> > such as the AR803x hibernate mode, and (b) to program the hardware not
> > to stop the receive clock when entering EEE low power. This does seem
> > to be a generic thing and not specific to just one PHY - especially as
> > the stopping of clocks when entering EEE low power is a IEEE 802.3
> > defined thing.
> 
> Like this:
... 
>  
> +        phy_disable_interrupts(phydev);
> +
>  	/* Start out supporting everything. Eventually,
>  	 * a controller will attach, and may modify one
>  	 * or both of these values
> @@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
>  	return 0;
>  }
>  
> -static void phy_shutdown(struct device *dev)
> -{
> -	struct phy_device *phydev = to_phy_device(dev);
> -
> -	if (phydev->state == PHY_READY || !phydev->attached_dev)
> -		return;
> -
> -	phy_disable_interrupts(phydev);
> -}
> -

Except of shutdown part from other discussion, looks fine for me.

What will be the best way to solve this issue for DSA switches attached to
MAC with RGMII RXC requirements?

Regards,
Oleksij
-- 
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 |

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 02:51:17PM +0200, Oleksij Rempel wrote:
> On Thu, Aug 10, 2023 at 11:34:02AM +0100, Russell King (Oracle) wrote:
> > On Thu, Aug 10, 2023 at 11:01:53AM +0100, Russell King (Oracle) wrote:
> > > On Thu, Aug 10, 2023 at 02:49:55AM +0200, Marek Vasut wrote:
> > > > On 8/10/23 00:06, Andrew Lunn wrote:
> > > > > On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > > > > > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > > > > > 
> > > > > > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > > > > > will be more changes if we use clock provider/consume mechanism.
> > > > > > > 
> > > > > > > Less changes is not always best. What happens when a different PHY is
> > > > > > > used?
> > > > > > 
> > > > > > Then the system wouldn't be affected by this AR803x specific behavior.
> > > > > 
> > > > > Do you know it really is specific to the AR803x? Turning the clock off
> > > > > seams a reasonable thing to do when saving power, or when there is no
> > > > > link partner.
> > > > 
> > > > This hibernation behavior seem specific to this PHY, I haven't seen it on
> > > > another PHY connected to the EQoS so far.
> > > 
> > > Marvell PHYs can be programmed so that RXCLK stops when the PHY
> > > enters power down or energy-detect state, although it defaults to
> > > always keeping the RGMII interface powered (and thus providing a
> > > clock.)
> > > 
> > > One Micrel PHY - "To save more power, the KSZ9031RNX stops the RX_CLK
> > > clock output to the MAC after 10 or more RX_CLK clock
> > > cycles have occurred in the receive LPI state." which seems to imply
> > > if EEE is enabled, then the receive clock will be stopped when
> > > entering low-power state.
> > > 
> > > I've said this several times in this thread - I think we need a bit
> > > in the PHY device's dev_flags to allow the MAC to say "do not power
> > > down the receive clock" which is used by the PHY drivers to (a) program
> > > the hardware to prevent the receive clock being stopped in situations
> > > such as the AR803x hibernate mode, and (b) to program the hardware not
> > > to stop the receive clock when entering EEE low power. This does seem
> > > to be a generic thing and not specific to just one PHY - especially as
> > > the stopping of clocks when entering EEE low power is a IEEE 802.3
> > > defined thing.
> > 
> > Like this:
> ... 
> >  
> > +        phy_disable_interrupts(phydev);
> > +
> >  	/* Start out supporting everything. Eventually,
> >  	 * a controller will attach, and may modify one
> >  	 * or both of these values
> > @@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
> >  	return 0;
> >  }
> >  
> > -static void phy_shutdown(struct device *dev)
> > -{
> > -	struct phy_device *phydev = to_phy_device(dev);
> > -
> > -	if (phydev->state == PHY_READY || !phydev->attached_dev)
> > -		return;
> > -
> > -	phy_disable_interrupts(phydev);
> > -}
> > -
> 
> Except of shutdown part from other discussion, looks fine for me.

Sigh... clearly I can't cope with email, especially when most of the
subject line is hidden! All I see in the index is "Re: [PATCH] net: phy:
at803x: Improve hi" for this thread, and I can't remember what the
other thread was... and it's buried in email.

> What will be the best way to solve this issue for DSA switches attached to
> MAC with RGMII RXC requirements?

I have no idea - the problem there is the model that has been adopted
in Linux is that there is no direct relationship between the DSA switch
and the MAC like there is with a PHY.

The MAC sees only a fixed-link, as does the DSA switch. So from the
software perspective, it's:

MAC ---- fixed-link

Switch ---- fixed-link

And it just so happens that packets pass between the two by "magic".
With a PHY, there is a direct relationship:

MAC ---- PHY

The MAC has a direct relationship with the PHY to the extent that the
MAC driver is involved in managing the PHY through phylink/phylib.

The effect of that de-coupling is that the MAC driver becomes quite
independent of the switch driver - I don't believe the switch driver
can query the MAC driver at probe time, nor the other way. If I'm
mistaken about that, someone needs to say!

What that leaves us with is firmware telling the switch driver...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Andrew Lunn]

> > What will be the best way to solve this issue for DSA switches attached to
> > MAC with RGMII RXC requirements?
> 
> I have no idea - the problem there is the model that has been adopted
> in Linux is that there is no direct relationship between the DSA switch
> and the MAC like there is with a PHY.

A clock provider/consumer relationship can be expressed in DT. The DSA
switch port would provide the clock, rather than the PHY.

       Andrew

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 03:49:24PM +0200, Andrew Lunn wrote:
> > > What will be the best way to solve this issue for DSA switches attached to
> > > MAC with RGMII RXC requirements?
> > 
> > I have no idea - the problem there is the model that has been adopted
> > in Linux is that there is no direct relationship between the DSA switch
> > and the MAC like there is with a PHY.
> 
> A clock provider/consumer relationship can be expressed in DT. The DSA
> switch port would provide the clock, rather than the PHY.

Then we'll be in to people wanting to do it for PHYs as well, and as
we've recently discussed that isn't something we want because of the
dependencies it creates between mdio drivers and mac drivers.

Wouldn't the same dependency issue also apply for a DSA switch on a
MDIO bus, where the MDIO bus is part of the MAC driver?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Andrew Lunn]

On Thu, Aug 10, 2023 at 02:54:58PM +0100, Russell King (Oracle) wrote:
> On Thu, Aug 10, 2023 at 03:49:24PM +0200, Andrew Lunn wrote:
> > > > What will be the best way to solve this issue for DSA switches attached to
> > > > MAC with RGMII RXC requirements?
> > > 
> > > I have no idea - the problem there is the model that has been adopted
> > > in Linux is that there is no direct relationship between the DSA switch
> > > and the MAC like there is with a PHY.
> > 
> > A clock provider/consumer relationship can be expressed in DT. The DSA
> > switch port would provide the clock, rather than the PHY.
> 
> Then we'll be in to people wanting to do it for PHYs as well, and as
> we've recently discussed that isn't something we want because of the
> dependencies it creates between mdio drivers and mac drivers.
> 
> Wouldn't the same dependency issue also apply for a DSA switch on a
> MDIO bus, where the MDIO bus is part of the MAC driver?

We already have some level of circular dependencies with DSA, e.g. the
MAC driver provides the MDIO bus with the switch on it. It registers
the MDIO bus, causing the switch to probe. That probe fails because
the MAC driver has not registered its interface yet, which is the CPU
interface. We end up deferring the switch probe, and by the second
attempt, the MAC is fully registered and the switch probes.

The circular dependency with a clock consumer/provider between the MAC
and switch will be worse. We would need to avoid getting the clock in
the probe function. It would need to happen in during open, by which
time the switch should be present. MAC drivers also typically connect
to their PHY during open, not probe, so i don't see this as being too
big a problem.

As to unbind, my gut feeling is, the general case is too complex. It
is not a simple tree, where you can ensure unbind from the leaves
towards the root. Either we let root take its chance, or we just block
unbind.

	Andrew

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 04:23:08PM +0200, Andrew Lunn wrote:
> On Thu, Aug 10, 2023 at 02:54:58PM +0100, Russell King (Oracle) wrote:
> > On Thu, Aug 10, 2023 at 03:49:24PM +0200, Andrew Lunn wrote:
> > > > > What will be the best way to solve this issue for DSA switches attached to
> > > > > MAC with RGMII RXC requirements?
> > > > 
> > > > I have no idea - the problem there is the model that has been adopted
> > > > in Linux is that there is no direct relationship between the DSA switch
> > > > and the MAC like there is with a PHY.
> > > 
> > > A clock provider/consumer relationship can be expressed in DT. The DSA
> > > switch port would provide the clock, rather than the PHY.
> > 
> > Then we'll be in to people wanting to do it for PHYs as well, and as
> > we've recently discussed that isn't something we want because of the
> > dependencies it creates between mdio drivers and mac drivers.
> > 
> > Wouldn't the same dependency issue also apply for a DSA switch on a
> > MDIO bus, where the MDIO bus is part of the MAC driver?
> 
> We already have some level of circular dependencies with DSA, e.g. the
> MAC driver provides the MDIO bus with the switch on it. It registers
> the MDIO bus, causing the switch to probe. That probe fails because
> the MAC driver has not registered its interface yet, which is the CPU
> interface. We end up deferring the switch probe, and by the second
> attempt, the MAC is fully registered and the switch probes.

If that sequence occurs with a MAC that wants a clock from DSA, then
we're at a loss...

The DSA driver probe fails because the MAC hasn't fully registered
itself, so doesn't create the clock. The MAC driver tries to get the
clock but it doesn't exist, so it defers, tearing down the MDIO bus
in the process.

> The circular dependency with a clock consumer/provider between the MAC
> and switch will be worse. We would need to avoid getting the clock in
> the probe function. It would need to happen in during open, by which
> time the switch should be present. MAC drivers also typically connect
> to their PHY during open, not probe, so i don't see this as being too
> big a problem.

Providing nothing is happening in the MAC driver initialisation which
requires that clock, then that would be fine.

Removal should be possible provided the MAC driver doesn't need
anything before it removes the MDIO bus.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> On 8/9/23 15:40, Andrew Lunn wrote:
> > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > PHY automatic hibernation as long as clock is acquired?
> > > > 
> > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > will be more changes if we use clock provider/consume mechanism.
> > 
> > Less changes is not always best. What happens when a different PHY is
> > used?
> 
> Then the system wouldn't be affected by this AR803x specific behavior.

I don't think it is AR803x specific behaviour. I think it is an
interesting stmmac behaviour, where it requires the clock from
the PHY in order to do even the most trivial things (like reset
itself.) That is a very interesting design decision.

how does stmmac hardware complete a power-on reset if it needs
the external clock from a PHY that itself might be in the process
of powering itself up and establishing its clocks...

There have been *hacks* to phylink requested over the years to
work around this peculiar behaviour by stmmac - and it seems that
it's not common behaviour.

This kind of thing might affect Cadence's macb driver as well, but
rather than it being a clock from the ethernet PHY, it seems to be
from the serdes PHY built in to the SoC - if I understand what's
reported in the proposed patch commit log (which I don't fully.)

In the case of stmmac, I don't think it's fair to blame the AR803x.
It's a hardware integration issue - the AR803x implementation which
works fine elsewhere has a problem with the stmmac implementation,
because design decisions made in both implementations end up being
incompatible with each other.

However, pair them with different implementations, and they're fine.

Given that stmmac requires a clock from the PHY, I'm of the opinion
that we need to have a way to tell phylib that "hey, this MAC must
always have a receive clock from the PHY, please arrange for that
to happen". AR803x needs to check that and arrange for the receive
clock to always be output. phylib can also use that to ensure that
when EEE mode is active in the PHY, clock-stop support is disabled...
and that's actually a key part to getting EEE properly implemented.

Clearly, the IEEE 802.3 folk catered for this issue when specifying
EEE, where some MACs must always be fed a receive clock, and so I
think phylib in Linux needs to recognise that this is A Thing that
it should allow MACs to specify.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 12:15:14AM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 09, 2023 at 11:34:19PM +0200, Marek Vasut wrote:
> > On 8/9/23 15:40, Andrew Lunn wrote:
> > > > > Hm.. how about officially defining this PHY as the clock provider and disable
> > > > > PHY automatic hibernation as long as clock is acquired?
> > > > > 
> > > > Sorry, I don't know much about the clock provider/consumer, but I think there
> > > > will be more changes if we use clock provider/consume mechanism.
> > > 
> > > Less changes is not always best. What happens when a different PHY is
> > > used?
> > 
> > Then the system wouldn't be affected by this AR803x specific behavior.
> 
> I don't think it is AR803x specific behaviour. I think it is an
> interesting stmmac behaviour, where it requires the clock from
> the PHY in order to do even the most trivial things (like reset
> itself.) That is a very interesting design decision.
> 
> how does stmmac hardware complete a power-on reset if it needs
> the external clock from a PHY that itself might be in the process
> of powering itself up and establishing its clocks...
> 
> There have been *hacks* to phylink requested over the years to
> work around this peculiar behaviour by stmmac - and it seems that
> it's not common behaviour.
> 
> This kind of thing might affect Cadence's macb driver as well, but
> rather than it being a clock from the ethernet PHY, it seems to be
> from the serdes PHY built in to the SoC - if I understand what's
> reported in the proposed patch commit log (which I don't fully.)
> 
> In the case of stmmac, I don't think it's fair to blame the AR803x.
> It's a hardware integration issue - the AR803x implementation which
> works fine elsewhere has a problem with the stmmac implementation,
> because design decisions made in both implementations end up being
> incompatible with each other.
> 
> However, pair them with different implementations, and they're fine.
> 
> Given that stmmac requires a clock from the PHY, I'm of the opinion
> that we need to have a way to tell phylib that "hey, this MAC must
> always have a receive clock from the PHY, please arrange for that
> to happen". AR803x needs to check that and arrange for the receive
> clock to always be output. phylib can also use that to ensure that
> when EEE mode is active in the PHY, clock-stop support is disabled...
> and that's actually a key part to getting EEE properly implemented.
> 
> Clearly, the IEEE 802.3 folk catered for this issue when specifying
> EEE, where some MACs must always be fed a receive clock, and so I
> think phylib in Linux needs to recognise that this is A Thing that
> it should allow MACs to specify.

Like this:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fcab363d8dfa..a954f1d61709 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1254,6 +1254,11 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
 	priv->phylink_config.mac_managed_pm = true;
 
+	/* stmmac always requires a receive clock in order for things like
+	 * hardware reset to work.
+	 */
+	priv->phylink_config.mac_requires_rxc = true;
+
 	phylink = phylink_create(&priv->phylink_config, fwnode,
 				 mode, &stmmac_phylink_mac_ops);
 	if (IS_ERR(phylink))
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 13c4121fa309..619a63a0d14f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -990,7 +990,8 @@ static int at803x_hibernation_mode_config(struct phy_device *phydev)
 	/* The default after hardware reset is hibernation mode enabled. After
 	 * software reset, the value is retained.
 	 */
-	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
+	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE) &&
+	    !(phydev->dev_flags & PHY_F_RXC_ALWAYS_ON))
 		return 0;
 
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e9909b30938..4d1a37487923 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
 			goto out;
 	}
 
+        phy_disable_interrupts(phydev);
+
 	/* Start out supporting everything. Eventually,
 	 * a controller will attach, and may modify one
 	 * or both of these values
@@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
 	return 0;
 }
 
-static void phy_shutdown(struct device *dev)
-{
-	struct phy_device *phydev = to_phy_device(dev);
-
-	if (phydev->state == PHY_READY || !phydev->attached_dev)
-		return;
-
-	phy_disable_interrupts(phydev);
-}
-
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -3376,7 +3368,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.bus = &mdio_bus_type;
 	new_driver->mdiodrv.driver.probe = phy_probe;
 	new_driver->mdiodrv.driver.remove = phy_remove;
-	new_driver->mdiodrv.driver.shutdown = phy_shutdown;
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4f1c8bb199e9..6568a2759101 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1830,6 +1830,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 			      phy_interface_t interface)
 {
+	u32 flags = 0;
+
 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
 		     phy_interface_mode_is_8023z(interface) && !pl->sfp_bus)))
@@ -1838,7 +1840,10 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->phydev)
 		return -EBUSY;
 
-	return phy_attach_direct(pl->netdev, phy, 0, interface);
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
+	return phy_attach_direct(pl->netdev, phy, flags, interface);
 }
 
 /**
@@ -1941,6 +1946,9 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	phy_device_free(phy_dev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ba08b0e60279..79df5e01707d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -761,6 +761,7 @@ struct phy_device {
 
 /* Generic phy_device::dev_flags */
 #define PHY_F_NO_IRQ		0x80000000
+#define PHY_F_RXC_ALWAYS_ON	BIT(30)
 
 static inline struct phy_device *to_phy_device(const struct device *dev)
 {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 789c516c6b4a..a83c1a77338f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -204,6 +204,7 @@ enum phylink_op_type {
  * @poll_fixed_state: if true, starts link_poll,
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
+ * @mac_requires_rxc: if true, the MAC always requires a receive clock from PHY.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
@@ -216,6 +217,7 @@ struct phylink_config {
 	enum phylink_op_type type;
 	bool poll_fixed_state;
 	bool mac_managed_pm;
+	bool mac_requires_rxc;
 	bool ovr_an_inband;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Wei Fang]

> > > Hm.. how about officially defining this PHY as the clock provider
> > > and disable PHY automatic hibernation as long as clock is acquired?
> > >
> > Sorry, I don't know much about the clock provider/consumer, but I
> > think there will be more changes if we use clock provider/consume
> mechanism.
> 
> Less changes is not always best. What happens when a different PHY is used?
> By having a clock provider in the PHY, you are defining a clear API that any
> PHY needs to provide to work with this MAC.
> 
Yes, as Russell mentioned the issue of suspend/resume, that i.MX platform uses
the RTL8211 PHY. But now I don't have a clear idea to solve this issue on dwmac
IP, the design of this IP is so different.

Cc to Clark to aware of this issue on dwmac IP.

> As Russell has point out, this is not the first time we have run into this
> problem. So far, it seems everybody has tried to solve it for just their system.
> So maybe now we should look at the whole picture and put in place a
> generic solution which can be made to work for any PHY.