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

[Oleksij Rempel <o.rempel@pengutronix.de>]

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 |