RE: [PATCH V3 net-next 3/4] net: phy: realtek: add dt property to enable ALDPS mode

Hi Jisheng,

-----Original Message-----
From: Jisheng Zhang <redacted>
Sent: 2021年6月8日 17:51
To: Joakim Zhang <redacted>
Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
f.fainelli@gmail.com; dl-linux-imx [off-list ref];
netdev@vger.kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH V3 net-next 3/4] net: phy: realtek: add dt property to
enable ALDPS mode

On Tue,  8 Jun 2021 11:15:34 +0800
Joakim Zhang [off-list ref] wrote:

quoted

If enable Advance Link Down Power Saving (ALDPS) mode, it will change
crystal/clock behavior, which cause RXC clock stop for dozens to
hundreds of miliseconds. This is comfirmed by Realtek engineer. For
some MACs, it needs RXC clock to support RX logic, after this patch,
PHY can generate continuous RXC clock during auto-negotiation.

ALDPS default is disabled after hardware reset, it's more reasonable
to add a property to enable this feature, since ALDPS would introduce side

effect.

quoted

This patch adds dt property "realtek,aldps-enable" to enable ALDPS
mode per users' requirement.

Jisheng Zhang enables this feature, changes the default behavior.
Since mine patch breaks the rule that new implementation should not
break existing design, so Cc'ed let him know to see if it can be accepted.

Cc: Jisheng Zhang <redacted>
Signed-off-by: Joakim Zhang <redacted>
---
 drivers/net/phy/realtek.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index ca258f2a9613..79dc55bb4091 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c

@@ -76,6 +76,7 @@ MODULE_AUTHOR("Johnson Leung");

MODULE_LICENSE("GPL");

 struct rtl821x_priv {
+       u16 phycr1;
        u16 phycr2;
 };

@@ -98,6 +99,14 @@ static int rtl821x_probe(struct phy_device *phydev)
        if (!priv)
                return -ENOMEM;

+       priv->phycr1 = phy_read_paged(phydev, 0xa43,

RTL8211F_PHYCR1);

quoted

+       if (priv->phycr1 < 0)
+               return priv->phycr1;
+
+       priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF |
+ RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);

priv->phycr1 is 0 by default, so above 5 LoCs can be removed

The intention of this is to take bootloader into account. Such as uboot configure the PHY before.

Best Regards,
Joakim Zhang

quoted

+       if (of_property_read_bool(dev->of_node, "realtek,aldps-enable"))
+               priv->phycr1 |= RTL8211F_ALDPS_PLL_OFF |
+ RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF;
+
        priv->phycr2 = phy_read_paged(phydev, 0xa43,

RTL8211F_PHYCR2);

quoted

        if (priv->phycr2 < 0)
                return priv->phycr2;

@@ -324,11 +333,16 @@ static int rtl8211f_config_init(struct phy_device

*phydev)

quoted

        struct rtl821x_priv *priv = phydev->priv;
        struct device *dev = &phydev->mdio.dev;
        u16 val_txdly, val_rxdly;
-       u16 val;
        int ret;

-       val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF |

RTL8211F_ALDPS_XTAL_OFF;

quoted

-       phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val,

val);

quoted

+       ret = phy_modify_paged_changed(phydev, 0xa43,

RTL8211F_PHYCR1,

quoted

+                                      RTL8211F_ALDPS_PLL_OFF |

RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,

quoted

+                                      priv->phycr1);
+       if (ret < 0) {
+               dev_err(dev, "aldps mode  configuration failed: %pe\n",
+                       ERR_PTR(ret));
+               return ret;
+       }

        switch (phydev->interface) {
        case PHY_INTERFACE_MODE_RGMII:
--
2.17.1