[PATCH v2 net-next 0/6] Disable CLKOUT on RTL8211F(D)(I)-VD-CG

[PATCH v2 net-next 0/6] Disable CLKOUT on RTL8211F(D)(I)-VD-CG

[Vladimir Oltean]

The Realtek RTL8211F(D)(I)-VD-CG is similar to other RTL8211F models in
that the CLKOUT signal can be turned off - a feature requested to reduce
EMI, and implemented via "realtek,clkout-disable" as documented in
Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml.

It is also dissimilar to said PHY models because it has no PHYCR2
register, and disabling CLKOUT is done through some other register.

The strategy adopted in this 6-patch series is to make the PHY driver
not think in terms of "priv->has_phycr2" and "priv->phycr2", but of more
high-level features ("priv->disable_clk_out") while maintaining behaviour.
Then, the logic is extended for the new PHY.

Very loosely based on previous work from Clark Wang, who took a
different approach, to pretend that the RTL8211FVD_CLKOUT_REG is
actually this PHY's PHYCR2.

v1 at:
https://lore.kernel.org/netdev/20251106111003.37023-1-vladimir.oltean@nxp.com/

Changes since v1:
- Apply Andrew's feedback regarding rtl8211f_config_clk_out() function
  naming
- Revisited the control flow that I was commenting on, and found an
  issue with RGMII delay handling which resulted in me declaring war on
  complex control flow schemes, and more specifically in patches 1/6 and
  6/6 which are new. Patch 1/6 is "probably" a bug fix, so it is at the
  top of the list in case the autosel bot wants to pick it up.

Vladimir Oltean (6):
  net: phy: realtek: create rtl8211f_config_rgmii_delay()
  net: phy: realtek: eliminate priv->phycr2 variable
  net: phy: realtek: eliminate has_phycr2 variable
  net: phy: realtek: allow CLKOUT to be disabled on RTL8211F(D)(I)-VD-CG
  net: phy: realtek: eliminate priv->phycr1 variable
  net: phy: realtek: create rtl8211f_config_phy_eee() helper

 drivers/net/phy/realtek/realtek_main.c | 156 ++++++++++++++++---------
 1 file changed, 101 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH v2 net-next 1/6] net: phy: realtek: create rtl8211f_config_rgmii_delay()

[Vladimir Oltean]

The control flow in rtl8211f_config_init() has some pitfalls which were
probably unintended. Specifically it has an early return:

	switch (phydev->interface) {
	...
	default: /* the rest of the modes imply leaving delay as is. */
		return 0;
	}

which exits the entire config_init() function. This means it also skips
doing things such as disabling CLKOUT or disabling PHY-mode EEE.

For the RTL8211FS, which uses PHY_INTERFACE_MODE_SGMII, this might be a
problem. However, I don't know that it is, so there is no Fixes: tag.
The issue was observed through code inspection.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 drivers/net/phy/realtek/realtek_main.c | 65 +++++++++++++++-----------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 417f9a88aab6..896351022682 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -587,22 +587,11 @@ static int rtl8211c_config_init(struct phy_device *phydev)
 			    CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER);
 }
 
-static int rtl8211f_config_init(struct phy_device *phydev)
+static int rtl8211f_config_rgmii_delay(struct phy_device *phydev)
 {
-	struct rtl821x_priv *priv = phydev->priv;
-	struct device *dev = &phydev->mdio.dev;
 	u16 val_txdly, val_rxdly;
 	int ret;
 
-	ret = phy_modify_paged_changed(phydev, RTL8211F_PHYCR_PAGE, 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:
 		val_txdly = 0;
@@ -632,34 +621,58 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 				       RTL8211F_TXCR, RTL8211F_TX_DELAY,
 				       val_txdly);
 	if (ret < 0) {
-		dev_err(dev, "Failed to update the TX delay register\n");
+		phydev_err(phydev, "Failed to update the TX delay register: %pe\n",
+			   ERR_PTR(ret));
 		return ret;
 	} else if (ret) {
-		dev_dbg(dev,
-			"%s 2ns TX delay (and changing the value from pin-strapping RXD1 or the bootloader)\n",
-			str_enable_disable(val_txdly));
+		phydev_dbg(phydev,
+			   "%s 2ns TX delay (and changing the value from pin-strapping RXD1 or the bootloader)\n",
+			   str_enable_disable(val_txdly));
 	} else {
-		dev_dbg(dev,
-			"2ns TX delay was already %s (by pin-strapping RXD1 or bootloader configuration)\n",
-			str_enabled_disabled(val_txdly));
+		phydev_dbg(phydev,
+			   "2ns TX delay was already %s (by pin-strapping RXD1 or bootloader configuration)\n",
+			   str_enabled_disabled(val_txdly));
 	}
 
 	ret = phy_modify_paged_changed(phydev, RTL8211F_RGMII_PAGE,
 				       RTL8211F_RXCR, RTL8211F_RX_DELAY,
 				       val_rxdly);
 	if (ret < 0) {
-		dev_err(dev, "Failed to update the RX delay register\n");
+		phydev_err(phydev, "Failed to update the RX delay register: %pe\n",
+			   ERR_PTR(ret));
 		return ret;
 	} else if (ret) {
-		dev_dbg(dev,
-			"%s 2ns RX delay (and changing the value from pin-strapping RXD0 or the bootloader)\n",
-			str_enable_disable(val_rxdly));
+		phydev_dbg(phydev,
+			   "%s 2ns RX delay (and changing the value from pin-strapping RXD0 or the bootloader)\n",
+			   str_enable_disable(val_rxdly));
 	} else {
-		dev_dbg(dev,
-			"2ns RX delay was already %s (by pin-strapping RXD0 or bootloader configuration)\n",
-			str_enabled_disabled(val_rxdly));
+		phydev_dbg(phydev,
+			   "2ns RX delay was already %s (by pin-strapping RXD0 or bootloader configuration)\n",
+			   str_enabled_disabled(val_rxdly));
 	}
 
+	return 0;
+}
+
+static int rtl8211f_config_init(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	int ret;
+
+	ret = phy_modify_paged_changed(phydev, RTL8211F_PHYCR_PAGE, 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;
+	}
+
+	ret = rtl8211f_config_rgmii_delay(phydev);
+	if (ret)
+		return ret;
+
 	if (!priv->has_phycr2)
 		return 0;
 
-- 
2.34.1

[PATCH v2 net-next 2/6] net: phy: realtek: eliminate priv->phycr2 variable

[Vladimir Oltean]

The RTL8211F(D)(I)-VD-CG PHY also has support for disabling the CLKOUT,
and we'd like to introduce the "realtek,clkout-disable" property for
that.

But it isn't done through the PHYCR2 register, and it becomes awkward to
have the driver pretend that it is. So just replace the machine-level
"u16 phycr2" variable with a logical "bool disable_clk_out", which
scales better to the other PHY as well.

The change is a complete functional equivalent. Before, if the device
tree property was absent, priv->phycr2 would contain the RTL8211F_CLKOUT_EN
bit as read from hardware. Now, we don't save priv->phycr2, but we just
don't call phy_modify_paged() on it. Also, we can simply call
phy_modify_paged() with the "set" argument to 0.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: rename rtl8211f_disable_clk_out() to rtl8211f_config_clk_out()

 drivers/net/phy/realtek/realtek_main.c | 31 ++++++++++++++------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 896351022682..ba58bdc3cf85 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -194,8 +194,8 @@ MODULE_LICENSE("GPL");
 
 struct rtl821x_priv {
 	u16 phycr1;
-	u16 phycr2;
 	bool has_phycr2;
+	bool disable_clk_out;
 	struct clk *clk;
 	/* rtl8211f */
 	u16 iner;
@@ -266,15 +266,8 @@ static int rtl821x_probe(struct phy_device *phydev)
 		priv->phycr1 |= RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF;
 
 	priv->has_phycr2 = !(phy_id == RTL_8211FVD_PHYID);
-	if (priv->has_phycr2) {
-		ret = phy_read_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR2);
-		if (ret < 0)
-			return ret;
-
-		priv->phycr2 = ret & RTL8211F_CLKOUT_EN;
-		if (of_property_read_bool(dev->of_node, "realtek,clkout-disable"))
-			priv->phycr2 &= ~RTL8211F_CLKOUT_EN;
-	}
+	priv->disable_clk_out = of_property_read_bool(dev->of_node,
+						      "realtek,clkout-disable");
 
 	phydev->priv = priv;
 
@@ -654,6 +647,18 @@ static int rtl8211f_config_rgmii_delay(struct phy_device *phydev)
 	return 0;
 }
 
+static int rtl8211f_config_clk_out(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+
+	/* The value is preserved if the device tree property is absent */
+	if (!priv->disable_clk_out)
+		return 0;
+
+	return phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE,
+				RTL8211F_PHYCR2, RTL8211F_CLKOUT_EN, 0);
+}
+
 static int rtl8211f_config_init(struct phy_device *phydev)
 {
 	struct rtl821x_priv *priv = phydev->priv;
@@ -682,10 +687,8 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE,
-			       RTL8211F_PHYCR2, RTL8211F_CLKOUT_EN,
-			       priv->phycr2);
-	if (ret < 0) {
+	ret = rtl8211f_config_clk_out(phydev);
+	if (ret) {
 		dev_err(dev, "clkout configuration failed: %pe\n",
 			ERR_PTR(ret));
 		return ret;
-- 
2.34.1

[PATCH v2 net-next 3/6] net: phy: realtek: eliminate has_phycr2 variable

[Vladimir Oltean]

This variable is assigned in rtl821x_probe() and used in
rtl8211f_config_init(), which is more complex than it needs to be.
Simply testing the same condition from rtl821x_probe() in
rtl8211f_config_init() yields the same result (the PHY driver ID is a
runtime invariant), but with one temporary variable less.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
v1->v2: just context changes

 drivers/net/phy/realtek/realtek_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index ba58bdc3cf85..9413c5e52998 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -194,7 +194,6 @@ MODULE_LICENSE("GPL");
 
 struct rtl821x_priv {
 	u16 phycr1;
-	bool has_phycr2;
 	bool disable_clk_out;
 	struct clk *clk;
 	/* rtl8211f */
@@ -245,7 +244,6 @@ static int rtl821x_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct rtl821x_priv *priv;
-	u32 phy_id = phydev->drv->phy_id;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -265,7 +263,6 @@ static int rtl821x_probe(struct phy_device *phydev)
 	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->has_phycr2 = !(phy_id == RTL_8211FVD_PHYID);
 	priv->disable_clk_out = of_property_read_bool(dev->of_node,
 						      "realtek,clkout-disable");
 
@@ -678,7 +675,8 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	if (!priv->has_phycr2)
+	/* RTL8211FVD has no PHYCR2 register */
+	if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
 		return 0;
 
 	/* Disable PHY-mode EEE so LPI is passed to the MAC */
-- 
2.34.1

[PATCH v2 net-next 4/6] net: phy: realtek: allow CLKOUT to be disabled on RTL8211F(D)(I)-VD-CG

[Vladimir Oltean]

Add CLKOUT disable support for RTL8211F(D)(I)-VD-CG. Like with other PHY
variants, this feature might be requested by customers when the clock
output is not used, in order to reduce electromagnetic interference (EMI).

In the common driver, the CLKOUT configuration is done through PHYCR2.
The RTL_8211FVD_PHYID is singled out as not having that register, and
execution in rtl8211f_config_init() returns early after commit
2c67301584f2 ("net: phy: realtek: Avoid PHYCR2 access if PHYCR2 not
present").

But actually CLKOUT is configured through a different register for this
PHY. Instead of pretending this is PHYCR2 (which it is not), just add
some code for modifying this register inside the rtl8211f_disable_clk_out()
function, and move that outside the code portion that runs only if
PHYCR2 exists.

In practice this reorders the PHYCR2 writes to disable PHY-mode EEE and
to disable the CLKOUT for the normal RTL8211F variants, but this should
be perfectly fine.

It was not noted that RTL8211F(D)(I)-VD-CG would need a genphy_soft_reset()
call after disabling the CLKOUT.

Co-developed-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: adapt to renaming of rtl8211f_config_clk_out() function

 drivers/net/phy/realtek/realtek_main.c | 27 +++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 9413c5e52998..dc2b0fcf13b2 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -90,6 +90,14 @@
 #define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
 #define RTL8211F_LEDCR_SHIFT			5
 
+/* RTL8211F(D)(I)-VD-CG CLKOUT configuration is specified via magic values
+ * to undocumented register pages. The names here do not reflect the datasheet.
+ * Unlike other PHY models, CLKOUT configuration does not go through PHYCR2.
+ */
+#define RTL8211FVD_CLKOUT_PAGE			0xd05
+#define RTL8211FVD_CLKOUT_REG			0x11
+#define RTL8211FVD_CLKOUT_EN			BIT(8)
+
 /* RTL8211F RGMII configuration */
 #define RTL8211F_RGMII_PAGE			0xd08
 
@@ -652,6 +660,11 @@ static int rtl8211f_config_clk_out(struct phy_device *phydev)
 	if (!priv->disable_clk_out)
 		return 0;
 
+	if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
+		return phy_modify_paged(phydev, RTL8211FVD_CLKOUT_PAGE,
+					RTL8211FVD_CLKOUT_REG,
+					RTL8211FVD_CLKOUT_EN, 0);
+
 	return phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE,
 				RTL8211F_PHYCR2, RTL8211F_CLKOUT_EN, 0);
 }
@@ -675,6 +688,13 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	ret = rtl8211f_config_clk_out(phydev);
+	if (ret) {
+		dev_err(dev, "clkout configuration failed: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
 	/* RTL8211FVD has no PHYCR2 register */
 	if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
 		return 0;
@@ -685,13 +705,6 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = rtl8211f_config_clk_out(phydev);
-	if (ret) {
-		dev_err(dev, "clkout configuration failed: %pe\n",
-			ERR_PTR(ret));
-		return ret;
-	}
-
 	return genphy_soft_reset(phydev);
 }
 
-- 
2.34.1

[PATCH v2 net-next 5/6] net: phy: realtek: eliminate priv->phycr1 variable

[Vladimir Oltean]

Previous changes have replaced the machine-level priv->phycr2 with a
high-level priv->disable_clk_out. This created a discrepancy with
priv->phycr1 which is resolved here, for uniformity.

One advantage of this new implementation is that we don't read
priv->phycr1 in rtl821x_probe() if we're never going to modify it.

We never test the positive return code from phy_modify_mmd_changed(), so
we could just as well use phy_modify_mmd().

I took the ALDPS feature description from commit d90db36a9e74 ("net:
phy: realtek: add dt property to enable ALDPS mode") and transformed it
into a function comment - the feature is sufficiently non-obvious to
deserve that.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 drivers/net/phy/realtek/realtek_main.c | 44 ++++++++++++++++----------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index dc2b0fcf13b2..4501b8923aad 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -201,7 +201,7 @@ MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
 struct rtl821x_priv {
-	u16 phycr1;
+	bool enable_aldps;
 	bool disable_clk_out;
 	struct clk *clk;
 	/* rtl8211f */
@@ -252,7 +252,6 @@ static int rtl821x_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct rtl821x_priv *priv;
-	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -263,14 +262,8 @@ static int rtl821x_probe(struct phy_device *phydev)
 		return dev_err_probe(dev, PTR_ERR(priv->clk),
 				     "failed to get phy clock\n");
 
-	ret = phy_read_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR1);
-	if (ret < 0)
-		return ret;
-
-	priv->phycr1 = ret & (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->enable_aldps = of_property_read_bool(dev->of_node,
+						   "realtek,aldps-enable");
 	priv->disable_clk_out = of_property_read_bool(dev->of_node,
 						      "realtek,clkout-disable");
 
@@ -669,17 +662,36 @@ static int rtl8211f_config_clk_out(struct phy_device *phydev)
 				RTL8211F_PHYCR2, RTL8211F_CLKOUT_EN, 0);
 }
 
-static int rtl8211f_config_init(struct phy_device *phydev)
+/* Advance Link Down Power Saving (ALDPS) mode changes crystal/clock behaviour,
+ * which causes the RXC clock signal to stop for tens to hundreds of
+ * milliseconds.
+ *
+ * Some MACs need the RXC clock to support their internal RX logic, so ALDPS is
+ * only enabled based on an opt-in device tree property.
+ */
+static int rtl8211f_config_aldps(struct phy_device *phydev)
 {
 	struct rtl821x_priv *priv = phydev->priv;
+	u16 mask = RTL8211F_ALDPS_PLL_OFF |
+		   RTL8211F_ALDPS_ENABLE |
+		   RTL8211F_ALDPS_XTAL_OFF;
+
+	/* The value is preserved if the device tree property is absent */
+	if (!priv->enable_aldps)
+		return 0;
+
+	return phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR1,
+				mask, mask);
+}
+
+static int rtl8211f_config_init(struct phy_device *phydev)
+{
 	struct device *dev = &phydev->mdio.dev;
 	int ret;
 
-	ret = phy_modify_paged_changed(phydev, RTL8211F_PHYCR_PAGE, 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",
+	ret = rtl8211f_config_aldps(phydev);
+	if (ret) {
+		dev_err(dev, "aldps mode configuration failed: %pe\n",
 			ERR_PTR(ret));
 		return ret;
 	}
-- 
2.34.1

[PATCH v2 net-next 6/6] net: phy: realtek: create rtl8211f_config_phy_eee() helper

[Vladimir Oltean]

To simplify the rtl8211f_config_init() control flow and get rid of
"early" returns for PHYs where the PHYCR2 register is absent, move the
entire logic sub-block that deals with disabling PHY-mode EEE to a
separate function. There, it is much more obvious what the early
"return 0" skips, and it becomes more difficult to accidentally skip
unintended stuff.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 drivers/net/phy/realtek/realtek_main.c | 29 ++++++++++++++++----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 4501b8923aad..6e75e124f27a 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -684,6 +684,23 @@ static int rtl8211f_config_aldps(struct phy_device *phydev)
 				mask, mask);
 }
 
+static int rtl8211f_config_phy_eee(struct phy_device *phydev)
+{
+	int ret;
+
+	/* RTL8211FVD has no PHYCR2 register */
+	if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
+		return 0;
+
+	/* Disable PHY-mode EEE so LPI is passed to the MAC */
+	ret = phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR2,
+			       RTL8211F_PHYCR2_PHY_EEE_ENABLE, 0);
+	if (ret)
+		return ret;
+
+	return genphy_soft_reset(phydev);
+}
+
 static int rtl8211f_config_init(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -707,17 +724,7 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 		return ret;
 	}
 
-	/* RTL8211FVD has no PHYCR2 register */
-	if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
-		return 0;
-
-	/* Disable PHY-mode EEE so LPI is passed to the MAC */
-	ret = phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR2,
-			       RTL8211F_PHYCR2_PHY_EEE_ENABLE, 0);
-	if (ret)
-		return ret;
-
-	return genphy_soft_reset(phydev);
+	return rtl8211f_config_phy_eee(phydev);
 }
 
 static int rtl821x_suspend(struct phy_device *phydev)
-- 
2.34.1

Re: [PATCH v2 net-next 1/6] net: phy: realtek: create rtl8211f_config_rgmii_delay()

[Andrew Lunn]

On Fri, Nov 07, 2025 at 01:08:12PM +0200, Vladimir Oltean wrote:
> The control flow in rtl8211f_config_init() has some pitfalls which were
> probably unintended. Specifically it has an early return:
> 
> 	switch (phydev->interface) {
> 	...
> 	default: /* the rest of the modes imply leaving delay as is. */
> 		return 0;
> 	}
> 
> which exits the entire config_init() function. This means it also skips
> doing things such as disabling CLKOUT or disabling PHY-mode EEE.
> 
> For the RTL8211FS, which uses PHY_INTERFACE_MODE_SGMII, this might be a
> problem. However, I don't know that it is, so there is no Fixes: tag.
> The issue was observed through code inspection.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Pulling this out into a helper makes sense.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 2/6] net: phy: realtek: eliminate priv->phycr2 variable

[Andrew Lunn]

On Fri, Nov 07, 2025 at 01:08:13PM +0200, Vladimir Oltean wrote:
> The RTL8211F(D)(I)-VD-CG PHY also has support for disabling the CLKOUT,
> and we'd like to introduce the "realtek,clkout-disable" property for
> that.
> 
> But it isn't done through the PHYCR2 register, and it becomes awkward to
> have the driver pretend that it is. So just replace the machine-level
> "u16 phycr2" variable with a logical "bool disable_clk_out", which
> scales better to the other PHY as well.
> 
> The change is a complete functional equivalent. Before, if the device
> tree property was absent, priv->phycr2 would contain the RTL8211F_CLKOUT_EN
> bit as read from hardware. Now, we don't save priv->phycr2, but we just
> don't call phy_modify_paged() on it. Also, we can simply call
> phy_modify_paged() with the "set" argument to 0.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: rename rtl8211f_disable_clk_out() to rtl8211f_config_clk_out()

Thanks

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 4/6] net: phy: realtek: allow CLKOUT to be disabled on RTL8211F(D)(I)-VD-CG

[Andrew Lunn]

On Fri, Nov 07, 2025 at 01:08:15PM +0200, Vladimir Oltean wrote:
> Add CLKOUT disable support for RTL8211F(D)(I)-VD-CG. Like with other PHY
> variants, this feature might be requested by customers when the clock
> output is not used, in order to reduce electromagnetic interference (EMI).
> 
> In the common driver, the CLKOUT configuration is done through PHYCR2.
> The RTL_8211FVD_PHYID is singled out as not having that register, and
> execution in rtl8211f_config_init() returns early after commit
> 2c67301584f2 ("net: phy: realtek: Avoid PHYCR2 access if PHYCR2 not
> present").
> 
> But actually CLKOUT is configured through a different register for this
> PHY. Instead of pretending this is PHYCR2 (which it is not), just add
> some code for modifying this register inside the rtl8211f_disable_clk_out()
> function, and move that outside the code portion that runs only if
> PHYCR2 exists.
> 
> In practice this reorders the PHYCR2 writes to disable PHY-mode EEE and
> to disable the CLKOUT for the normal RTL8211F variants, but this should
> be perfectly fine.
> 
> It was not noted that RTL8211F(D)(I)-VD-CG would need a genphy_soft_reset()
> call after disabling the CLKOUT.
> 
> Co-developed-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 5/6] net: phy: realtek: eliminate priv->phycr1 variable

[Andrew Lunn]

On Fri, Nov 07, 2025 at 01:08:16PM +0200, Vladimir Oltean wrote:
> Previous changes have replaced the machine-level priv->phycr2 with a
> high-level priv->disable_clk_out. This created a discrepancy with
> priv->phycr1 which is resolved here, for uniformity.
> 
> One advantage of this new implementation is that we don't read
> priv->phycr1 in rtl821x_probe() if we're never going to modify it.
> 
> We never test the positive return code from phy_modify_mmd_changed(), so
> we could just as well use phy_modify_mmd().
> 
> I took the ALDPS feature description from commit d90db36a9e74 ("net:
> phy: realtek: add dt property to enable ALDPS mode") and transformed it
> into a function comment - the feature is sufficiently non-obvious to
> deserve that.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 6/6] net: phy: realtek: create rtl8211f_config_phy_eee() helper

[Andrew Lunn]

On Fri, Nov 07, 2025 at 01:08:17PM +0200, Vladimir Oltean wrote:
> To simplify the rtl8211f_config_init() control flow and get rid of
> "early" returns for PHYs where the PHYCR2 register is absent, move the
> entire logic sub-block that deals with disabling PHY-mode EEE to a
> separate function. There, it is much more obvious what the early
> "return 0" skips, and it becomes more difficult to accidentally skip
> unintended stuff.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: patch is new
> 
>  drivers/net/phy/realtek/realtek_main.c | 29 ++++++++++++++++----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index 4501b8923aad..6e75e124f27a 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -684,6 +684,23 @@ static int rtl8211f_config_aldps(struct phy_device *phydev)
>  				mask, mask);
>  }
>  
> +static int rtl8211f_config_phy_eee(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* RTL8211FVD has no PHYCR2 register */
> +	if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
> +		return 0;
> +
> +	/* Disable PHY-mode EEE so LPI is passed to the MAC */
> +	ret = phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR2,
> +			       RTL8211F_PHYCR2_PHY_EEE_ENABLE, 0);
> +	if (ret)
> +		return ret;
> +
> +	return genphy_soft_reset(phydev);

Is this soft reset only required for EEE? None of the other
configuration needs it?

For the Marvell PHYs, lots of registers need a soft reset to put
changes into effect. I would not want to hide the soft reset inside a
helper, because of the danger more calls to helps are added
afterwards.

	Andrew

Re: [PATCH v2 net-next 6/6] net: phy: realtek: create rtl8211f_config_phy_eee() helper

[Vladimir Oltean]

On Fri, Nov 07, 2025 at 02:34:54PM +0100, Andrew Lunn wrote:
> On Fri, Nov 07, 2025 at 01:08:17PM +0200, Vladimir Oltean wrote:
> > To simplify the rtl8211f_config_init() control flow and get rid of
> > "early" returns for PHYs where the PHYCR2 register is absent, move the
> > entire logic sub-block that deals with disabling PHY-mode EEE to a
> > separate function. There, it is much more obvious what the early
> > "return 0" skips, and it becomes more difficult to accidentally skip
> > unintended stuff.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> > v1->v2: patch is new
> > 
> >  drivers/net/phy/realtek/realtek_main.c | 29 ++++++++++++++++----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> > index 4501b8923aad..6e75e124f27a 100644
> > --- a/drivers/net/phy/realtek/realtek_main.c
> > +++ b/drivers/net/phy/realtek/realtek_main.c
> > @@ -684,6 +684,23 @@ static int rtl8211f_config_aldps(struct phy_device *phydev)
> >  				mask, mask);
> >  }
> >  
> > +static int rtl8211f_config_phy_eee(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	/* RTL8211FVD has no PHYCR2 register */
> > +	if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
> > +		return 0;
> > +
> > +	/* Disable PHY-mode EEE so LPI is passed to the MAC */
> > +	ret = phy_modify_paged(phydev, RTL8211F_PHYCR_PAGE, RTL8211F_PHYCR2,
> > +			       RTL8211F_PHYCR2_PHY_EEE_ENABLE, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return genphy_soft_reset(phydev);
> 
> Is this soft reset only required for EEE? None of the other
> configuration needs it?

It's good you point this out. Somehow, among all transformations, I lost
along the way the fact that the soft reset is necessary for disabling
clkout on RTL8211F, not for PHY-mode EEE :-/
https://elixir.bootlin.com/linux/v6.16.12/source/drivers/net/phy/realtek/realtek_main.c#L598

I checked the RTL8211F datasheet and it doesn't say that changes to the
"PHY-mode EEE Enable" field would need a write to 0.15 to take effect.
But it does say that about "CLKOUT Source".

Curiously, the RTL8211FVD datasheet doesn't suggest that modifying the
CLKOUT source needs a soft reset when providing the steps to do so.

Anyway, this code transformation from patch 6/6 is not buggy per se
(even if we change the CLKOUT on RTL8211F, we still get the
genphy_soft_reset() that we need), but very misleading and confusing.

pw-bot: cr

> For the Marvell PHYs, lots of registers need a soft reset to put
> changes into effect. I would not want to hide the soft reset inside a
> helper, because of the danger more calls to helps are added
> afterwards.

Ok, I get your point and I agree, but what to do?

static int rtl8211f_config_init(struct phy_device *phydev)
{
	struct device *dev = &phydev->mdio.dev;
	bool needs_reset;
	int ret;

	ret = rtl8211f_config_aldps(phydev);
	if (ret) {
		dev_err(dev, "aldps mode configuration failed: %pe\n",
			ERR_PTR(ret));
		return ret;
	}

	ret = rtl8211f_config_rgmii_delay(phydev);
	if (ret)
		return ret;

	ret = rtl8211f_config_clk_out(phydev, &needs_reset); // RTL8211F needs it, RTL8211FVD doesn't
	if (ret) {
		dev_err(dev, "clkout configuration failed: %pe\n",
			ERR_PTR(ret));
		return ret;
	}

	ret = rtl8211f_config_phy_eee(phydev);
	if (ret)
		return ret;

	if (needs_reset) {
		ret = genphy_soft_reset(phydev);
		if (ret)
			return ret;
	}

	return 0;
}

?

Re: [PATCH v2 net-next 6/6] net: phy: realtek: create rtl8211f_config_phy_eee() helper

[Andrew Lunn]

> It's good you point this out. Somehow, among all transformations, I lost
> along the way the fact that the soft reset is necessary for disabling
> clkout on RTL8211F, not for PHY-mode EEE :-/
> https://elixir.bootlin.com/linux/v6.16.12/source/drivers/net/phy/realtek/realtek_main.c#L598
> 
> I checked the RTL8211F datasheet and it doesn't say that changes to the
> "PHY-mode EEE Enable" field would need a write to 0.15 to take effect.
> But it does say that about "CLKOUT Source".
> 
> Curiously, the RTL8211FVD datasheet doesn't suggest that modifying the
> CLKOUT source needs a soft reset when providing the steps to do so.
> 
> Anyway, this code transformation from patch 6/6 is not buggy per se
> (even if we change the CLKOUT on RTL8211F, we still get the
> genphy_soft_reset() that we need), but very misleading and confusing.
> 
> pw-bot: cr
> 
> > For the Marvell PHYs, lots of registers need a soft reset to put
> > changes into effect. I would not want to hide the soft reset inside a
> > helper, because of the danger more calls to helps are added
> > afterwards.
> 
> Ok, I get your point and I agree, but what to do?

If only the clk out that needs it, i would put it in the clock out
helper.

Is a soft reset expensive? Is a soft reset destructive? The marvell
one is both fast and does not seem to change any registers, it just
activates changes.

If you think some other registers might need it, i would probably just
do it unconditionally after all the configuration, assuming it is
cheap and non-destructive. Maybe add a comment that at least clk out
needs its, but other registers might need it as well?

      Andrew

Re: [PATCH v2 net-next 6/6] net: phy: realtek: create rtl8211f_config_phy_eee() helper

[Vladimir Oltean]

On Fri, Nov 07, 2025 at 05:18:01PM +0100, Andrew Lunn wrote:
> > It's good you point this out. Somehow, among all transformations, I lost
> > along the way the fact that the soft reset is necessary for disabling
> > clkout on RTL8211F, not for PHY-mode EEE :-/
> > https://elixir.bootlin.com/linux/v6.16.12/source/drivers/net/phy/realtek/realtek_main.c#L598
> > 
> > I checked the RTL8211F datasheet and it doesn't say that changes to the
> > "PHY-mode EEE Enable" field would need a write to 0.15 to take effect.
> > But it does say that about "CLKOUT Source".
> > 
> > Curiously, the RTL8211FVD datasheet doesn't suggest that modifying the
> > CLKOUT source needs a soft reset when providing the steps to do so.
> > 
> > Anyway, this code transformation from patch 6/6 is not buggy per se
> > (even if we change the CLKOUT on RTL8211F, we still get the
> > genphy_soft_reset() that we need), but very misleading and confusing.
> > 
> > pw-bot: cr
> > 
> > > For the Marvell PHYs, lots of registers need a soft reset to put
> > > changes into effect. I would not want to hide the soft reset inside a
> > > helper, because of the danger more calls to helps are added
> > > afterwards.
> > 
> > Ok, I get your point and I agree, but what to do?
> 
> If only the clk out that needs it, i would put it in the clock out
> helper.
> 
> Is a soft reset expensive? Is a soft reset destructive? The marvell
> one is both fast and does not seem to change any registers, it just
> activates changes.
> 
> If you think some other registers might need it, i would probably just
> do it unconditionally after all the configuration, assuming it is
> cheap and non-destructive. Maybe add a comment that at least clk out
> needs its, but other registers might need it as well?
> 
>       Andrew
>

With the risk of being unsatisfactory, I can't answer these questions
without completely throwing off eveything else I have scheduled to do.

The patch was delivered by Clark to a customer a number of years ago,
and the customer hooked up the scope to confirm that CLKOUT is disabled.
The datasheet doesn't suggest that genphy_soft_reset() is needed, but
looking at the BSP implementation, I see we did it anyway:
https://github.com/nxp-qoriq/linux/blob/lf-6.6.y/drivers/net/phy/realtek.c#L427

I think the safest thing to do is to keep the genphy_soft_reset() in the
CLKOUT configuration procedure, regardless of PHY version, which is what
I am going to do for v3.

Re: [PATCH v2 net-next 6/6] net: phy: realtek: create rtl8211f_config_phy_eee() helper

[Andrew Lunn]

> I think the safest thing to do is to keep the genphy_soft_reset() in the
> CLKOUT configuration procedure, regardless of PHY version, which is what
> I am going to do for v3.

O.K.

	Andrew