[net-next,3/4] net: phy: realteck: add dt property to disable ALDPS mode

Commit Message

Joakim Zhang June 1, 2021, 9:04 a.m. UTC

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. This patch
adds dt property to disable ALDPS mode per users' requirement.

Signed-off-by: Joakim Zhang <[e-mail]>
---
 drivers/net/phy/realtek.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Russell King (Oracle) June 1, 2021, 11:52 a.m. UTC | #1

On Tue, Jun 01, 2021 at 05:04:07PM +0800, Joakim Zhang wrote:
> @@ -325,8 +329,10 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  	u16 val;
>  	int ret;
>  
> -	val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
> -	phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
> +	if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) {
> +		val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
> +		phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
> +	}

Similar questions as with the previous patch, but also... this doesn't
actually disable the feature if it was previously turned on. E.g. a
kexec() from a current kernel that has set these features into a
subsequent kernel that the DT requests the feature to be disabled. Or
a boot loader that has enabled this feature.

If DT specifies that this feature is disabled, shouldn't this code be
disabling it explicitly?

Joakim Zhang June 2, 2021, 2:22 a.m. UTC | #2

Hi Russell,

> -----Original Message-----
> From: Russell King <[e-mail]>
> Sent: 2021年6月1日 19:52
> To: Joakim Zhang <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; dl-linux-imx
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH net-next 3/4] net: phy: realteck: add dt property to disable
> ALDPS mode
> 
> On Tue, Jun 01, 2021 at 05:04:07PM +0800, Joakim Zhang wrote:
> > @@ -325,8 +329,10 @@ static int rtl8211f_config_init(struct phy_device
> *phydev)
> >  	u16 val;
> >  	int ret;
> >
> > -	val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF |
> RTL8211F_ALDPS_XTAL_OFF;
> > -	phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
> > +	if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) {
> > +		val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF |
> RTL8211F_ALDPS_XTAL_OFF;
> > +		phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val,
> val);
> > +	}
> 
> Similar questions as with the previous patch, but also... this doesn't actually
> disable the feature if it was previously turned on. E.g. a
> kexec() from a current kernel that has set these features into a subsequent
> kernel that the DT requests the feature to be disabled. Or a boot loader that
> has enabled this feature.
Sorry, I don't know what your meanings. As I explained before, boot loader also can configure PHY registers,
but kernel should not depend on what boot loader did.

If you use kexec() to boot kernel with DT request to disable this clock, PHY probe also can parse this property to disable it.
I know little about kexec(), could you please explain more if I misunderstand?

> If DT specifies that this feature is disabled, shouldn't this code be disabling it
> explicitly?
Yes, exactly should.

Best Regards,
Joakim Zhang
> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=04%7C01%7Cqiangqin
> g.zhang%40nxp.com%7Ceacabd25941448301acb08d924f3b6de%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581451436345731%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3jyYeGVBXXb5jCDtyTrt3DI9MwVE
> OT5Et9tpCZG26gY%3D&amp;reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Patch

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 4219c23ff2b0..90e3a8cbfc2f 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -73,6 +73,7 @@ 
 
 /* quirks for realtek phy */
 #define RTL821X_CLKOUT_DISABLE_FEATURE		BIT(0)
+#define RTL821X_ALDPS_DISABLE_FEATURE		BIT(1)
 
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
@@ -104,6 +105,9 @@  static int rtl821x_probe(struct phy_device *phydev)
 	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
 		priv->quirks |= RTL821X_CLKOUT_DISABLE_FEATURE;
 
+	if (of_property_read_bool(dev->of_node, "rtl821x,aldps-disable"))
+		priv->quirks |= RTL821X_ALDPS_DISABLE_FEATURE;
+
 	phydev->priv = priv;
 
 	return 0;
@@ -325,8 +329,10 @@  static int rtl8211f_config_init(struct phy_device *phydev)
 	u16 val;
 	int ret;
 
-	val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
-	phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
+	if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) {
+		val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
+		phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
+	}
 
 	switch (phydev->interface) {
 	case PHY_INTERFACE_MODE_RGMII: