[PATCH V3 net-next 0/4] net: phy: add dt property for realtek phy

[PATCH V3 net-next 0/4] net: phy: add dt property for realtek phy

[Joakim Zhang]

Add dt property for realtek phy.

---
ChangeLogs:
V1->V2:
	* store the desired PHYCR1/2 register value in "priv" rather than
	using "quirks", per Russell King suggestion, as well as can
	cover the bootloader setting.
	* change the behavior of ALDPS mode, default is disabled, add dt
	property for users to enable it.
	* fix dt binding yaml build issues.
V2->V3:
	* update the commit title
	net: phy: realtek: add dt property to disable ALDPS mode ->
	net: phy: realtek: add dt property to enable ALDPS mode

Joakim Zhang (4):
  dt-bindings: net: add dt binding for realtek rtl82xx phy
  net: phy: realtek: add dt property to disable CLKOUT clock
  net: phy: realtek: add dt property to disable ALDPS mode
  net: phy: realtek: add delay to fix RXC generation issue

 .../bindings/net/realtek,rtl82xx.yaml         | 45 +++++++++++
 drivers/net/phy/realtek.c                     | 75 ++++++++++++++++++-
 2 files changed, 116 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml

-- 
2.17.1

[PATCH V3 net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy

[Joakim Zhang]

Add binding for realtek rtl82xx phy.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 .../bindings/net/realtek,rtl82xx.yaml         | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml

diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
new file mode 100644
index 000000000000..bb94a2388520
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: GPL-2.0+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/realtek,rtl82xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL82xx PHY
+
+maintainers:
+  - Andrew Lunn <andrew@lunn.ch>
+  - Florian Fainelli <f.fainelli@gmail.com>
+  - Heiner Kallweit <hkallweit1@gmail.com>
+
+description:
+  Bindings for Realtek RTL82xx PHYs
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+properties:
+  realtek,clkout-disable:
+    type: boolean
+    description:
+      Disable CLKOUT clock, CLKOUT clock default is enabled after hardware reset.
+
+
+  realtek,aldps-enable:
+    type: boolean
+    description:
+      Enable ALDPS mode, ALDPS mode default is disabled after hardware reset.
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethphy1: ethernet-phy@1 {
+                reg = <1>;
+                realtek,clkout-disable;
+                realtek,aldps-enable;
+        };
+    };
-- 
2.17.1

[PATCH V3 net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock

[Joakim Zhang]

CLKOUT is enabled by default after PHY hardware reset, this patch adds
"realtek,clkout-disable" property for user to disable CLKOUT clock
to save PHY power.

Per RTL8211F guide, a PHY reset should be issued after setting these
bits in PHYCR2 register. After this patch, CLKOUT clock output to be
disabled.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/phy/realtek.c | 42 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 821e85a97367..ca258f2a9613 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -8,6 +8,7 @@
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
  */
 #include <linux/bitops.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/module.h>
 #include <linux/delay.h>
@@ -27,6 +28,7 @@
 #define RTL821x_PAGE_SELECT			0x1f
 
 #define RTL8211F_PHYCR1				0x18
+#define RTL8211F_PHYCR2				0x19
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
@@ -40,6 +42,8 @@
 #define RTL8211E_TX_DELAY			BIT(12)
 #define RTL8211E_RX_DELAY			BIT(11)
 
+#define RTL8211F_CLKOUT_EN			BIT(0)
+
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_ISR_ANERR			BIT(15)
 #define RTL8201F_ISR_DUPLEX			BIT(13)
@@ -71,6 +75,10 @@ MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+struct rtl821x_priv {
+	u16 phycr2;
+};
+
 static int rtl821x_read_page(struct phy_device *phydev)
 {
 	return __phy_read(phydev, RTL821x_PAGE_SELECT);
@@ -81,6 +89,28 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+static int rtl821x_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct rtl821x_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->phycr2 = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR2);
+	if (priv->phycr2 < 0)
+		return priv->phycr2;
+
+	priv->phycr2 &= RTL8211F_CLKOUT_EN;
+	if (of_property_read_bool(dev->of_node, "realtek,clkout-disable"))
+		priv->phycr2 &= ~RTL8211F_CLKOUT_EN;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -291,6 +321,7 @@ static int rtl8211c_config_init(struct phy_device *phydev)
 
 static int rtl8211f_config_init(struct phy_device *phydev)
 {
+	struct rtl821x_priv *priv = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	u16 val_txdly, val_rxdly;
 	u16 val;
@@ -354,7 +385,15 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 			val_rxdly ? "enabled" : "disabled");
 	}
 
-	return 0;
+	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
+			       RTL8211F_CLKOUT_EN, priv->phycr2);
+	if (ret < 0) {
+		dev_err(dev, "clkout configuration failed: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	return genphy_soft_reset(phydev);
 }
 
 static int rtl8211e_config_init(struct phy_device *phydev)
@@ -847,6 +886,7 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc916),
 		.name		= "RTL8211F Gigabit Ethernet",
+		.probe		= rtl821x_probe,
 		.config_init	= &rtl8211f_config_init,
 		.read_status	= rtlgen_read_status,
 		.config_intr	= &rtl8211f_config_intr,
-- 
2.17.1

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

[Joakim Zhang]

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.
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 <Jisheng.Zhang@synaptics.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 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);
+	if (priv->phycr1 < 0)
+		return priv->phycr1;
+
+	priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);
+	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);
 	if (priv->phycr2 < 0)
 		return priv->phycr2;
@@ -324,11 +333,16 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	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;
-	phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
+	ret = phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1,
+				       RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,
+				       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

[PATCH V3 net-next 4/4] net: phy: realtek: add delay to fix RXC generation issue

[Joakim Zhang]

PHY will delay about 11.5ms to generate RXC clock when switching from
power down to normal operation. Read/write registers would also cause RXC
become unstable and stop for a while during this process. Realtek engineer
suggests 15ms or more delay can workaround this issue.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/phy/realtek.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 79dc55bb4091..1b844a06fe72 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -410,6 +410,19 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	return genphy_soft_reset(phydev);
 }
 
+static int rtl821x_resume(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_resume(phydev);
+	if (ret < 0)
+		return ret;
+
+	msleep(20);
+
+	return 0;
+}
+
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
 	int ret = 0, oldpage;
@@ -906,7 +919,7 @@ static struct phy_driver realtek_drvs[] = {
 		.config_intr	= &rtl8211f_config_intr,
 		.handle_interrupt = rtl8211f_handle_interrupt,
 		.suspend	= genphy_suspend,
-		.resume		= genphy_resume,
+		.resume		= rtl821x_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
 	}, {
-- 
2.17.1

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

[Jisheng Zhang]

On Tue,  8 Jun 2021 11:15:34 +0800
Joakim Zhang <qiangqing.zhang@nxp.com> wrote:


> 
> 
> 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.
> 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 <Jisheng.Zhang@synaptics.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  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);
> +       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

> +       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);
>         if (priv->phycr2 < 0)
>                 return priv->phycr2;
> @@ -324,11 +333,16 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>         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;
> -       phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
> +       ret = phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1,
> +                                      RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,
> +                                      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
> 

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

[Joakim Zhang]

Hi Jisheng,

> -----Original Message-----
> From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Sent: 2021年6月8日 17:51
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> 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 <linux-imx@nxp.com>;
> 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 <qiangqing.zhang@nxp.com> wrote:
> 
> 
> >
> >
> > 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.
> > 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 <Jisheng.Zhang@synaptics.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  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);
> > +       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
> > +       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);
> >         if (priv->phycr2 < 0)
> >                 return priv->phycr2;
> > @@ -324,11 +333,16 @@ static int rtl8211f_config_init(struct phy_device
> *phydev)
> >         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;
> > -       phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val,
> val);
> > +       ret = phy_modify_paged_changed(phydev, 0xa43,
> RTL8211F_PHYCR1,
> > +                                      RTL8211F_ALDPS_PLL_OFF |
> RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,
> > +                                      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
> >

Re: [PATCH V3 net-next 0/4] net: phy: add dt property for realtek phy

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Tue,  8 Jun 2021 11:15:31 +0800 you wrote:
> Add dt property for realtek phy.
> 
> ---
> ChangeLogs:
> V1->V2:
> 	* store the desired PHYCR1/2 register value in "priv" rather than
> 	using "quirks", per Russell King suggestion, as well as can
> 	cover the bootloader setting.
> 	* change the behavior of ALDPS mode, default is disabled, add dt
> 	property for users to enable it.
> 	* fix dt binding yaml build issues.
> V2->V3:
> 	* update the commit title
> 	net: phy: realtek: add dt property to disable ALDPS mode ->
> 	net: phy: realtek: add dt property to enable ALDPS mode
> 
> [...]

Here is the summary with links:
  - [V3,net-next,1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy
    https://git.kernel.org/netdev/net-next/c/a9f15dc2b973
  - [V3,net-next,2/4] net: phy: realtek: add dt property to disable CLKOUT clock
    https://git.kernel.org/netdev/net-next/c/0a4355c2b7f8
  - [V3,net-next,3/4] net: phy: realtek: add dt property to enable ALDPS mode
    https://git.kernel.org/netdev/net-next/c/d90db36a9e74
  - [V3,net-next,4/4] net: phy: realtek: add delay to fix RXC generation issue
    https://git.kernel.org/netdev/net-next/c/6813cc8cfdaf

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Jisheng Zhang]

On Tue, 8 Jun 2021 10:14:40 +0000
Joakim Zhang <qiangqing.zhang@nxp.com> wrote:


> 
> 
> Hi Jisheng,

Hi,

> 
> > -----Original Message-----
> > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > Sent: 2021年6月8日 17:51
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > 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 <linux-imx@nxp.com>;
> > 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 <qiangqing.zhang@nxp.com> wrote:
> >
> >  
> > >
> > >
> > > 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.  
> > > 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 <Jisheng.Zhang@synaptics.com>
> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > ---
> > >  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);  
> > > +       if (priv->phycr1 < 0)
> > > +               return priv->phycr1;
> > > +
> > > +       priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF |
> > > + RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF); 

I believe your intention is

priv->phycr1 &= ~(RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF); 
However, this is not necessary. See below.

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

The last param "set" of phy_modify_paged_changed() means *bit mask of bits to set*
If we don't want to enable ALDPS, 0 is enough.

Even if uboot configured the PHY before linux, I believe phy_modify_paged_changed()
can clear ALDPS bits w/o above 5 LoCs. 

Thanks

> 
> Best Regards,
> Joakim Zhang
> > > +       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);  
> > >         if (priv->phycr2 < 0)
> > >                 return priv->phycr2;
> > > @@ -324,11 +333,16 @@ static int rtl8211f_config_init(struct phy_device  
> > *phydev)  
> > >         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;  
> > > -       phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val,  
> > val);  
> > > +       ret = phy_modify_paged_changed(phydev, 0xa43,  
> > RTL8211F_PHYCR1,  
> > > +                                      RTL8211F_ALDPS_PLL_OFF |  
> > RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,  
> > > +                                      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
> > >  
> 

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

[Joakim Zhang]

Hi Jisheng,

> -----Original Message-----
> From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Sent: 2021年6月9日 9:57
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> 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 <linux-imx@nxp.com>;
> 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 10:14:40 +0000
> Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
> 
> 
> >
> >
> > Hi Jisheng,
> 
> Hi,
> 
> >
> > > -----Original Message-----
> > > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > > Sent: 2021年6月8日 17:51
> > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > 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 <linux-imx@nxp.com>;
> > > 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 <qiangqing.zhang@nxp.com> wrote:
> > >
> > >
> > > >
> > > >
> > > > 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.
> > > > 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 <Jisheng.Zhang@synaptics.com>
> > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > ---
> > > >  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);
> > > > +       if (priv->phycr1 < 0)
> > > > +               return priv->phycr1;
> > > > +
> > > > +       priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF |
> > > > + RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);
> 
> I believe your intention is
> 
> priv->phycr1 &= ~(RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE |
> priv->RTL8211F_ALDPS_XTAL_OFF);
> However, this is not necessary. See below.

No, mine intention is to read back this three bits what the register contained.

> > >
> > > 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.
> 
> The last param "set" of phy_modify_paged_changed() means *bit mask of bits
> to set* If we don't want to enable ALDPS, 0 is enough.
> 
> Even if uboot configured the PHY before linux, I believe
> phy_modify_paged_changed() can clear ALDPS bits w/o above 5 LoCs.

The logic is:
1) read back these three bits from the register.
2) if linux set "realtek,aldps-enable", assert these three bit; if not, keep these three bits read before.
3) call phy_modify_paged_changed() to configure, "mask" parameter to clear these three bits first, "set" parameter to assert these three bits per the result of step 2.

So, if step 1 read back the value is that these three bits are asserted, then in step 3, it will first clear these three bits and assert these three bits again. The result is ALDPS is enabled even without " realtek,aldps-enable " in DT.

Best Regards,
Joakim Zhang
> Thanks
> 
> >
> > Best Regards,
> > Joakim Zhang
> > > > +       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);
> > > >         if (priv->phycr2 < 0)
> > > >                 return priv->phycr2; @@ -324,11 +333,16 @@ static
> > > > int rtl8211f_config_init(struct phy_device
> > > *phydev)
> > > >         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;
> > > > -       phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1,
> val,
> > > val);
> > > > +       ret = phy_modify_paged_changed(phydev, 0xa43,
> > > RTL8211F_PHYCR1,
> > > > +
> RTL8211F_ALDPS_PLL_OFF |
> > > RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,
> > > > +                                      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
> > > >
> >

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

[Jisheng Zhang]

On Wed, 9 Jun 2021 02:51:11 +0000
Joakim Zhang <qiangqing.zhang@nxp.com> wrote:

> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> Hi Jisheng,
> 
> > -----Original Message-----
> > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > Sent: 2021年6月9日 9:57
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > 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 <linux-imx@nxp.com>;
> > 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 10:14:40 +0000
> > Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
> >
> >  
> > >
> > >
> > > Hi Jisheng,  
> >
> > Hi,
> >  
> > >  
> > > > -----Original Message-----
> > > > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > > > Sent: 2021年6月8日 17:51
> > > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > 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 <linux-imx@nxp.com>;
> > > > 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 <qiangqing.zhang@nxp.com> wrote:
> > > >
> > > >  
> > > > >
> > > > >
> > > > > 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.  
> > > > > 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 <Jisheng.Zhang@synaptics.com>
> > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > > ---
> > > > >  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);  
> > > > > +       if (priv->phycr1 < 0)
> > > > > +               return priv->phycr1;
> > > > > +
> > > > > +       priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF |
> > > > > + RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);  
> >
> > I believe your intention is
> >
> > priv->phycr1 &= ~(RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE |
> > priv->RTL8211F_ALDPS_XTAL_OFF);
> > However, this is not necessary. See below.  
> 
> No, mine intention is to read back this three bits what the register contained.
> 
> > > >
> > > > 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.
> >
> > The last param "set" of phy_modify_paged_changed() means *bit mask of bits
> > to set* If we don't want to enable ALDPS, 0 is enough.
> >
> > Even if uboot configured the PHY before linux, I believe
> > phy_modify_paged_changed() can clear ALDPS bits w/o above 5 LoCs.  
> 
> The logic is:
> 1) read back these three bits from the register.
> 2) if linux set "realtek,aldps-enable", assert these three bit; if not, keep these three bits read before.
> 3) call phy_modify_paged_changed() to configure, "mask" parameter to clear these three bits first, "set" parameter to assert these three bits per the result of step 2.
> 
> So, if step 1 read back the value is that these three bits are asserted, then in step 3, it will first clear these three bits and assert these three bits again. The result is ALDPS is enabled even without " realtek,aldps-enable " in DT.
> 

Aha, I see you want to keep the ALDPS bits(maybe configured by prelinux env) untouched.
If ALDPS has been enabled by prelinux env, even there's no "realtek,aldps-enable"
in DT, the ALDPS may be keep enabled in linux. Thus the ALDPS behavior rely on
the prelinux env. I'm not sure whether this is correct or not.

IMHO, the "realtek,aldps-enable" is a "yes" or "no" bool. If it's set, ALDPS
will be enabled in linux; If it's no, ALDPS will be disabled in linux. We
should not rely on prelinux env.

Thanks

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

[Joakim Zhang]

Hi Jisheng,

> -----Original Message-----
> From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> Sent: 2021年6月9日 11:04
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> 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 <linux-imx@nxp.com>;
> 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 Wed, 9 Jun 2021 02:51:11 +0000
> Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
> 
> > CAUTION: Email originated externally, do not click links or open attachments
> unless you recognize the sender and know the content is safe.
> >
> >
> > Hi Jisheng,
> >
> > > -----Original Message-----
> > > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > > Sent: 2021年6月9日 9:57
> > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > 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 <linux-imx@nxp.com>;
> > > 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 10:14:40 +0000
> > > Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
> > >
> > >
> > > >
> > > >
> > > > Hi Jisheng,
> > >
> > > Hi,
> > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > > > > Sent: 2021年6月8日 17:51
> > > > > To: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > > 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 <linux-imx@nxp.com>;
> > > > > 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
> > > > > <qiangqing.zhang@nxp.com> wrote:
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > 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.
> > > > > > 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 <Jisheng.Zhang@synaptics.com>
> > > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > > > > > ---
> > > > > >  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);
> > > > > > +       if (priv->phycr1 < 0)
> > > > > > +               return priv->phycr1;
> > > > > > +
> > > > > > +       priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF |
> > > > > > + RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);
> > >
> > > I believe your intention is
> > >
> > > priv->phycr1 &= ~(RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE |
> > > priv->RTL8211F_ALDPS_XTAL_OFF);
> > > However, this is not necessary. See below.
> >
> > No, mine intention is to read back this three bits what the register contained.
> >
> > > > >
> > > > > 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.
> > >
> > > The last param "set" of phy_modify_paged_changed() means *bit mask
> > > of bits to set* If we don't want to enable ALDPS, 0 is enough.
> > >
> > > Even if uboot configured the PHY before linux, I believe
> > > phy_modify_paged_changed() can clear ALDPS bits w/o above 5 LoCs.
> >
> > The logic is:
> > 1) read back these three bits from the register.
> > 2) if linux set "realtek,aldps-enable", assert these three bit; if not, keep these
> three bits read before.
> > 3) call phy_modify_paged_changed() to configure, "mask" parameter to clear
> these three bits first, "set" parameter to assert these three bits per the result
> of step 2.
> >
> > So, if step 1 read back the value is that these three bits are asserted, then in
> step 3, it will first clear these three bits and assert these three bits again. The
> result is ALDPS is enabled even without " realtek,aldps-enable " in DT.
> >
> 
> Aha, I see you want to keep the ALDPS bits(maybe configured by prelinux env)
> untouched.
> If ALDPS has been enabled by prelinux env, even there's no
> "realtek,aldps-enable"
> in DT, the ALDPS may be keep enabled in linux. Thus the ALDPS behavior rely on
> the prelinux env. I'm not sure whether this is correct or not.
> 
> IMHO, the "realtek,aldps-enable" is a "yes" or "no" bool. If it's set, ALDPS will
> be enabled in linux; If it's no, ALDPS will be disabled in linux. We should not rely
> on prelinux env.

You can refer to below V1 comments, we may not take enough cases into account, others may care about boot loader. 
https://lkml.org/lkml/2021/6/1/264

Anyway, keep the original value in registers should not have side effects. Thanks for your reviewing, and I saw this patch set has been
accepted, if you want to improve this code, I am happy to review and test your patch.

Best Regards,
Joakim Zhang
> Thanks

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

[Andrew Lunn]

> Aha, I see you want to keep the ALDPS bits(maybe configured by prelinux env) untouched.
> If ALDPS has been enabled by prelinux env, even there's no "realtek,aldps-enable"
> in DT, the ALDPS may be keep enabled in linux. Thus the ALDPS behavior rely on
> the prelinux env. I'm not sure whether this is correct or not.
> 
> IMHO, the "realtek,aldps-enable" is a "yes" or "no" bool. If it's set, ALDPS
> will be enabled in linux; If it's no, ALDPS will be disabled in linux. We
> should not rely on prelinux env.

If you look at V1 of this patch, you will see i commented that maybe
it needs to be a tristate, not a boolean. Disable it, enable it, leave
it as is. If we do need the third state, we can add it latter.

There is something to be said for not relying on the bootloader. But
the hardware default appears to be ALDPS enabled. So this case seems
reasonably safe.

	   Andrew

Re: [PATCH V3 net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy

[Rob Herring]

On Tue, Jun 08, 2021 at 11:15:32AM +0800, Joakim Zhang wrote:
> Add binding for realtek rtl82xx phy.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  .../bindings/net/realtek,rtl82xx.yaml         | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
> new file mode 100644
> index 000000000000..bb94a2388520
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/realtek,rtl82xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek RTL82xx PHY
> +
> +maintainers:
> +  - Andrew Lunn <andrew@lunn.ch>
> +  - Florian Fainelli <f.fainelli@gmail.com>
> +  - Heiner Kallweit <hkallweit1@gmail.com>
> +
> +description:
> +  Bindings for Realtek RTL82xx PHYs
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +properties:
> +  realtek,clkout-disable:
> +    type: boolean
> +    description:
> +      Disable CLKOUT clock, CLKOUT clock default is enabled after hardware reset.
> +
> +

Extra blank line.

> +  realtek,aldps-enable:
> +    type: boolean
> +    description:
> +      Enable ALDPS mode, ALDPS mode default is disabled after hardware reset.
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethphy1: ethernet-phy@1 {
> +                reg = <1>;
> +                realtek,clkout-disable;
> +                realtek,aldps-enable;

The schema here will never be applied because there is nothing to match 
on to determine which nodes it is valid. This should have a compatible 
(because ethernet phys are not special).

Rob