[RESEND net-next v5 0/4] net: phy: realtek: Add support for PHY LEDs on

[RESEND net-next v5 0/4] net: phy: realtek: Add support for PHY LEDs on

[Michael Klein]

Re-sending the patch after merge window.

Changes in V5:
- Split cleanup patch and improve code formatting

Changes in V4:
- Change (!ret) to (ret == 0)
- Replace set_bit() by __set_bit()

Changes in V3:
- move definition of rtl8211e_read_ext_page() to patch 2
- Wrap overlong lines

Changes in V2:
- Designate to net-next
- Add ExtPage access cleanup patch as suggested by Andrew Lunn

Michael Klein (4):
  net: phy: realtek: Group RTL82* macro definitions
  net: phy: realtek: Clean up RTL8211E ExtPage access
  net: phy: realtek: use __set_bit() in rtl8211f_led_hw_control_get()
  net: phy: realtek: Add support for PHY LEDs on RTL8211E

 drivers/net/phy/realtek/realtek_main.c | 201 ++++++++++++++++++++-----
 1 file changed, 160 insertions(+), 41 deletions(-)

-- 
2.39.5

[RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions

[Michael Klein]

Group macro definitions by chip number in lexicographic order.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 drivers/net/phy/realtek/realtek_main.c | 30 +++++++++++++-------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 893c82479671..b27c0f995e56 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -17,6 +17,15 @@
 
 #include "realtek.h"
 
+#define RTL8201F_ISR				0x1e
+#define RTL8201F_ISR_ANERR			BIT(15)
+#define RTL8201F_ISR_DUPLEX			BIT(13)
+#define RTL8201F_ISR_LINK			BIT(11)
+#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
+						 RTL8201F_ISR_DUPLEX | \
+						 RTL8201F_ISR_LINK)
+#define RTL8201F_IER				0x13
+
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
 #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
@@ -31,6 +40,10 @@
 #define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
+#define RTL8211E_CTRL_DELAY			BIT(13)
+#define RTL8211E_TX_DELAY			BIT(12)
+#define RTL8211E_RX_DELAY			BIT(11)
+
 #define RTL8211F_PHYCR1				0x18
 #define RTL8211F_PHYCR2				0x19
 #define RTL8211F_CLKOUT_EN			BIT(0)
@@ -47,6 +60,8 @@
 #define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
 #define RTL8211F_LEDCR_SHIFT			5
 
+#define RTL8211F_LED_COUNT			3
+
 #define RTL8211F_TX_DELAY			BIT(8)
 #define RTL8211F_RX_DELAY			BIT(3)
 
@@ -54,19 +69,6 @@
 #define RTL8211F_ALDPS_ENABLE			BIT(2)
 #define RTL8211F_ALDPS_XTAL_OFF			BIT(12)
 
-#define RTL8211E_CTRL_DELAY			BIT(13)
-#define RTL8211E_TX_DELAY			BIT(12)
-#define RTL8211E_RX_DELAY			BIT(11)
-
-#define RTL8201F_ISR				0x1e
-#define RTL8201F_ISR_ANERR			BIT(15)
-#define RTL8201F_ISR_DUPLEX			BIT(13)
-#define RTL8201F_ISR_LINK			BIT(11)
-#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
-						 RTL8201F_ISR_DUPLEX | \
-						 RTL8201F_ISR_LINK)
-#define RTL8201F_IER				0x13
-
 #define RTL822X_VND1_SERDES_OPTION			0x697a
 #define RTL822X_VND1_SERDES_OPTION_MODE_MASK		GENMASK(5, 0)
 #define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII		0
@@ -112,8 +114,6 @@
 #define RTL_8221B_VN_CG				0x001cc84a
 #define RTL_8251B				0x001cc862
 
-#define RTL8211F_LED_COUNT			3
-
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
-- 
2.39.5

[RESEND net-next v5 2/4] net: phy: realtek: Clean up RTL8211E ExtPage access

[Michael Klein]

- Factor out RTL8211E extension page access code to
  rtl8211e_modify_ext_page() and clean up rtl8211e_config_init()

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 drivers/net/phy/realtek/realtek_main.c | 38 +++++++++++++++-----------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index b27c0f995e56..e60c18551a4e 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -37,9 +37,11 @@
 
 #define RTL821x_INSR				0x13
 
-#define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
+#define RTL8211E_EXT_PAGE_SELECT		0x1e
+#define RTL8211E_SET_EXT_PAGE			0x07
+
 #define RTL8211E_CTRL_DELAY			BIT(13)
 #define RTL8211E_TX_DELAY			BIT(12)
 #define RTL8211E_RX_DELAY			BIT(11)
@@ -135,6 +137,21 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+static int rtl8211e_modify_ext_page(struct phy_device *phydev, u16 ext_page,
+				    u32 regnum, u16 mask, u16 set)
+{
+	int oldpage, ret = 0;
+
+	oldpage = phy_select_page(phydev, RTL8211E_SET_EXT_PAGE);
+	if (oldpage >= 0) {
+		ret = __phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, ext_page);
+		if (ret == 0)
+			ret = __phy_modify(phydev, regnum, mask, set);
+	}
+
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl821x_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -607,7 +624,9 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
 
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
-	int ret = 0, oldpage;
+	const u16 delay_mask = RTL8211E_CTRL_DELAY |
+			       RTL8211E_TX_DELAY |
+			       RTL8211E_RX_DELAY;
 	u16 val;
 
 	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
@@ -637,20 +656,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
 	 * 12 = RX Delay, 11 = TX Delay
 	 * 10:0 = Test && debug settings reserved by realtek
 	 */
-	oldpage = phy_select_page(phydev, 0x7);
-	if (oldpage < 0)
-		goto err_restore_page;
-
-	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
-	if (ret)
-		goto err_restore_page;
-
-	ret = __phy_modify(phydev, 0x1c, RTL8211E_CTRL_DELAY
-			   | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
-			   val);
-
-err_restore_page:
-	return phy_restore_page(phydev, oldpage, ret);
+	return rtl8211e_modify_ext_page(phydev, 0xa4, 0x1c, delay_mask, val);
 }
 
 static int rtl8211b_suspend(struct phy_device *phydev)
-- 
2.39.5

[RESEND net-next v5 3/4] net: phy: realtek: use __set_bit() in rtl8211f_led_hw_control_get()

[Michael Klein]

rtl8211f_led_hw_control_get() does not need atomic bit operations,
replace set_bit() by __set_bit().

Signed-off-by: Michael Klein <michael@fossekall.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/realtek/realtek_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index e60c18551a4e..0fcc57ad777f 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -577,17 +577,17 @@ static int rtl8211f_led_hw_control_get(struct phy_device *phydev, u8 index,
 	val &= RTL8211F_LEDCR_MASK;
 
 	if (val & RTL8211F_LEDCR_LINK_10)
-		set_bit(TRIGGER_NETDEV_LINK_10, rules);
+		__set_bit(TRIGGER_NETDEV_LINK_10, rules);
 
 	if (val & RTL8211F_LEDCR_LINK_100)
-		set_bit(TRIGGER_NETDEV_LINK_100, rules);
+		__set_bit(TRIGGER_NETDEV_LINK_100, rules);
 
 	if (val & RTL8211F_LEDCR_LINK_1000)
-		set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+		__set_bit(TRIGGER_NETDEV_LINK_1000, rules);
 
 	if (val & RTL8211F_LEDCR_ACT_TXRX) {
-		set_bit(TRIGGER_NETDEV_RX, rules);
-		set_bit(TRIGGER_NETDEV_TX, rules);
+		__set_bit(TRIGGER_NETDEV_RX, rules);
+		__set_bit(TRIGGER_NETDEV_TX, rules);
 	}
 
 	return 0;
-- 
2.39.5

[RESEND net-next v5 4/4] net: phy: realtek: Add support for PHY LEDs on RTL8211E

[Michael Klein]

Like the RTL8211F, the RTL8211E PHY supports up to three LEDs.
Add netdev trigger support for them, too.

Signed-off-by: Michael Klein <michael@fossekall.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/realtek/realtek_main.c | 125 +++++++++++++++++++++++--
 1 file changed, 119 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 0fcc57ad777f..9c3727a646f2 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -46,6 +46,20 @@
 #define RTL8211E_TX_DELAY			BIT(12)
 #define RTL8211E_RX_DELAY			BIT(11)
 
+#define RTL8211E_LEDCR_EXT_PAGE			0x2c
+
+#define RTL8211E_LEDCR1				0x1a
+#define RTL8211E_LEDCR1_ACT_TXRX		BIT(4)
+#define RTL8211E_LEDCR1_MASK			BIT(4)
+#define RTL8211E_LEDCR1_SHIFT			1
+
+#define RTL8211E_LEDCR2				0x1c
+#define RTL8211E_LEDCR2_LINK_1000		BIT(2)
+#define RTL8211E_LEDCR2_LINK_100		BIT(1)
+#define RTL8211E_LEDCR2_LINK_10			BIT(0)
+#define RTL8211E_LEDCR2_MASK			GENMASK(2, 0)
+#define RTL8211E_LEDCR2_SHIFT			4
+
 #define RTL8211F_PHYCR1				0x18
 #define RTL8211F_PHYCR2				0x19
 #define RTL8211F_CLKOUT_EN			BIT(0)
@@ -62,7 +76,8 @@
 #define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
 #define RTL8211F_LEDCR_SHIFT			5
 
-#define RTL8211F_LED_COUNT			3
+/* RTL8211E and RTL8211F support up to three LEDs */
+#define RTL8211x_LED_COUNT			3
 
 #define RTL8211F_TX_DELAY			BIT(8)
 #define RTL8211F_RX_DELAY			BIT(3)
@@ -137,6 +152,21 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+static int rtl8211e_read_ext_page(struct phy_device *phydev, u16 ext_page,
+				  u32 regnum)
+{
+	int oldpage, ret = 0;
+
+	oldpage = phy_select_page(phydev, RTL8211E_SET_EXT_PAGE);
+	if (oldpage >= 0) {
+		ret = __phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, ext_page);
+		if (ret == 0)
+			ret = __phy_read(phydev, regnum);
+	}
+
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8211e_modify_ext_page(struct phy_device *phydev, u16 ext_page,
 				    u32 regnum, u16 mask, u16 set)
 {
@@ -526,7 +556,7 @@ static int rtl821x_resume(struct phy_device *phydev)
 	return 0;
 }
 
-static int rtl8211f_led_hw_is_supported(struct phy_device *phydev, u8 index,
+static int rtl8211x_led_hw_is_supported(struct phy_device *phydev, u8 index,
 					unsigned long rules)
 {
 	const unsigned long mask = BIT(TRIGGER_NETDEV_LINK_10) |
@@ -545,9 +575,11 @@ static int rtl8211f_led_hw_is_supported(struct phy_device *phydev, u8 index,
 	 *      rates and Active indication always at all three 10+100+1000
 	 *      link rates.
 	 * This code currently uses mode B only.
+	 *
+	 * RTL8211E PHY LED has one mode, which works like RTL8211F mode B.
 	 */
 
-	if (index >= RTL8211F_LED_COUNT)
+	if (index >= RTL8211x_LED_COUNT)
 		return -EINVAL;
 
 	/* Filter out any other unsupported triggers. */
@@ -566,7 +598,7 @@ static int rtl8211f_led_hw_control_get(struct phy_device *phydev, u8 index,
 {
 	int val;
 
-	if (index >= RTL8211F_LED_COUNT)
+	if (index >= RTL8211x_LED_COUNT)
 		return -EINVAL;
 
 	val = phy_read_paged(phydev, 0xd04, RTL8211F_LEDCR);
@@ -599,7 +631,7 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
 	const u16 mask = RTL8211F_LEDCR_MASK << (RTL8211F_LEDCR_SHIFT * index);
 	u16 reg = 0;
 
-	if (index >= RTL8211F_LED_COUNT)
+	if (index >= RTL8211x_LED_COUNT)
 		return -EINVAL;
 
 	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
@@ -622,6 +654,84 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
 	return phy_modify_paged(phydev, 0xd04, RTL8211F_LEDCR, mask, reg);
 }
 
+static int rtl8211e_led_hw_control_get(struct phy_device *phydev, u8 index,
+				       unsigned long *rules)
+{
+	int ret;
+	u16 cr1, cr2;
+
+	if (index >= RTL8211x_LED_COUNT)
+		return -EINVAL;
+
+	ret = rtl8211e_read_ext_page(phydev, RTL8211E_LEDCR_EXT_PAGE,
+				     RTL8211E_LEDCR1);
+	if (ret < 0)
+		return ret;
+
+	cr1 = ret >> RTL8211E_LEDCR1_SHIFT * index;
+	if (cr1 & RTL8211E_LEDCR1_ACT_TXRX) {
+		__set_bit(TRIGGER_NETDEV_RX, rules);
+		__set_bit(TRIGGER_NETDEV_TX, rules);
+	}
+
+	ret = rtl8211e_read_ext_page(phydev, RTL8211E_LEDCR_EXT_PAGE,
+				     RTL8211E_LEDCR2);
+	if (ret < 0)
+		return ret;
+
+	cr2 = ret >> RTL8211E_LEDCR2_SHIFT * index;
+	if (cr2 & RTL8211E_LEDCR2_LINK_10)
+		__set_bit(TRIGGER_NETDEV_LINK_10, rules);
+
+	if (cr2 & RTL8211E_LEDCR2_LINK_100)
+		__set_bit(TRIGGER_NETDEV_LINK_100, rules);
+
+	if (cr2 & RTL8211E_LEDCR2_LINK_1000)
+		__set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+
+	return ret;
+}
+
+static int rtl8211e_led_hw_control_set(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	const u16 cr1mask =
+		RTL8211E_LEDCR1_MASK << (RTL8211E_LEDCR1_SHIFT * index);
+	const u16 cr2mask =
+		RTL8211E_LEDCR2_MASK << (RTL8211E_LEDCR2_SHIFT * index);
+	u16 cr1 = 0, cr2 = 0;
+	int ret;
+
+	if (index >= RTL8211x_LED_COUNT)
+		return -EINVAL;
+
+	if (test_bit(TRIGGER_NETDEV_RX, &rules) ||
+	    test_bit(TRIGGER_NETDEV_TX, &rules)) {
+		cr1 |= RTL8211E_LEDCR1_ACT_TXRX;
+	}
+
+	cr1 <<= RTL8211E_LEDCR1_SHIFT * index;
+	ret = rtl8211e_modify_ext_page(phydev, RTL8211E_LEDCR_EXT_PAGE,
+				       RTL8211E_LEDCR1, cr1mask, cr1);
+	if (ret < 0)
+		return ret;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+		cr2 |= RTL8211E_LEDCR2_LINK_10;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+		cr2 |= RTL8211E_LEDCR2_LINK_100;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		cr2 |= RTL8211E_LEDCR2_LINK_1000;
+
+	cr2 <<= RTL8211E_LEDCR2_SHIFT * index;
+	ret = rtl8211e_modify_ext_page(phydev, RTL8211E_LEDCR_EXT_PAGE,
+				       RTL8211E_LEDCR2, cr2mask, cr2);
+
+	return ret;
+}
+
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
 	const u16 delay_mask = RTL8211E_CTRL_DELAY |
@@ -1398,6 +1508,9 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.led_hw_is_supported = rtl8211x_led_hw_is_supported,
+		.led_hw_control_get = rtl8211e_led_hw_control_get,
+		.led_hw_control_set = rtl8211e_led_hw_control_set,
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc916),
 		.name		= "RTL8211F Gigabit Ethernet",
@@ -1411,7 +1524,7 @@ static struct phy_driver realtek_drvs[] = {
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
 		.flags		= PHY_ALWAYS_CALL_SUSPEND,
-		.led_hw_is_supported = rtl8211f_led_hw_is_supported,
+		.led_hw_is_supported = rtl8211x_led_hw_is_supported,
 		.led_hw_control_get = rtl8211f_led_hw_control_get,
 		.led_hw_control_set = rtl8211f_led_hw_control_set,
 	}, {
-- 
2.39.5

Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions

[Joe Damato]

On Mon, Apr 07, 2025 at 08:21:40PM +0200, Michael Klein wrote:
> Group macro definitions by chip number in lexicographic order.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  drivers/net/phy/realtek/realtek_main.c | 30 +++++++++++++-------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index 893c82479671..b27c0f995e56 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -17,6 +17,15 @@
>  
>  #include "realtek.h"
>  
> +#define RTL8201F_ISR				0x1e
> +#define RTL8201F_ISR_ANERR			BIT(15)
> +#define RTL8201F_ISR_DUPLEX			BIT(13)
> +#define RTL8201F_ISR_LINK			BIT(11)
> +#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
> +						 RTL8201F_ISR_DUPLEX | \
> +						 RTL8201F_ISR_LINK)
> +#define RTL8201F_IER				0x13

If sorting lexicographically, wouldn't RTL8201F_IER come before
RTL8201F_ISR ?

>  #define RTL821x_PHYSR				0x11
>  #define RTL821x_PHYSR_DUPLEX			BIT(13)
>  #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
> @@ -31,6 +40,10 @@
>  #define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
> +#define RTL8211E_CTRL_DELAY			BIT(13)
> +#define RTL8211E_TX_DELAY			BIT(12)
> +#define RTL8211E_RX_DELAY			BIT(11)

Maybe I'm reading this wrong but these don't seem sorted
lexicographically ?

Re: [RESEND net-next v5 2/4] net: phy: realtek: Clean up RTL8211E ExtPage access

[Joe Damato]

On Mon, Apr 07, 2025 at 08:21:41PM +0200, Michael Klein wrote:
> - Factor out RTL8211E extension page access code to
>   rtl8211e_modify_ext_page() and clean up rtl8211e_config_init()
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  drivers/net/phy/realtek/realtek_main.c | 38 +++++++++++++++-----------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index b27c0f995e56..e60c18551a4e 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -37,9 +37,11 @@
>  
>  #define RTL821x_INSR				0x13
>  
> -#define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
> +#define RTL8211E_EXT_PAGE_SELECT		0x1e
> +#define RTL8211E_SET_EXT_PAGE			0x07
> +
>  #define RTL8211E_CTRL_DELAY			BIT(13)
>  #define RTL8211E_TX_DELAY			BIT(12)
>  #define RTL8211E_RX_DELAY			BIT(11)
> @@ -135,6 +137,21 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
>  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
>  }
>  
> +static int rtl8211e_modify_ext_page(struct phy_device *phydev, u16 ext_page,
> +				    u32 regnum, u16 mask, u16 set)
> +{
> +	int oldpage, ret = 0;
> +
> +	oldpage = phy_select_page(phydev, RTL8211E_SET_EXT_PAGE);
> +	if (oldpage >= 0) {
> +		ret = __phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, ext_page);
> +		if (ret == 0)
> +			ret = __phy_modify(phydev, regnum, mask, set);
> +	}
> +
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  static int rtl821x_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
> @@ -607,7 +624,9 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
>  
>  static int rtl8211e_config_init(struct phy_device *phydev)
>  {
> -	int ret = 0, oldpage;
> +	const u16 delay_mask = RTL8211E_CTRL_DELAY |
> +			       RTL8211E_TX_DELAY |
> +			       RTL8211E_RX_DELAY;
>  	u16 val;
>  
>  	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
> @@ -637,20 +656,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
>  	 * 12 = RX Delay, 11 = TX Delay
>  	 * 10:0 = Test && debug settings reserved by realtek
>  	 */
> -	oldpage = phy_select_page(phydev, 0x7);
> -	if (oldpage < 0)
> -		goto err_restore_page;
> -
> -	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> -	if (ret)
> -		goto err_restore_page;
> -
> -	ret = __phy_modify(phydev, 0x1c, RTL8211E_CTRL_DELAY
> -			   | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> -			   val);
> -
> -err_restore_page:
> -	return phy_restore_page(phydev, oldpage, ret);
> +	return rtl8211e_modify_ext_page(phydev, 0xa4, 0x1c, delay_mask, val);
>  }

Seems good to add RTL8211E_SET_EXT_PAGE to remove a constant from
the code. Any reason to avoid adding constants for 0xa4 and 0x1c ?

Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions

[Andrew Lunn]

On Mon, Apr 07, 2025 at 07:52:50PM -0700, Joe Damato wrote:
> On Mon, Apr 07, 2025 at 08:21:40PM +0200, Michael Klein wrote:
> > Group macro definitions by chip number in lexicographic order.
> > 
> > Signed-off-by: Michael Klein <michael@fossekall.de>
> > ---
> >  drivers/net/phy/realtek/realtek_main.c | 30 +++++++++++++-------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> > index 893c82479671..b27c0f995e56 100644
> > --- a/drivers/net/phy/realtek/realtek_main.c
> > +++ b/drivers/net/phy/realtek/realtek_main.c
> > @@ -17,6 +17,15 @@
> >  
> >  #include "realtek.h"
> >  
> > +#define RTL8201F_ISR				0x1e
> > +#define RTL8201F_ISR_ANERR			BIT(15)
> > +#define RTL8201F_ISR_DUPLEX			BIT(13)
> > +#define RTL8201F_ISR_LINK			BIT(11)
> > +#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
> > +						 RTL8201F_ISR_DUPLEX | \
> > +						 RTL8201F_ISR_LINK)
> > +#define RTL8201F_IER				0x13
> 
> If sorting lexicographically, wouldn't RTL8201F_IER come before
> RTL8201F_ISR ?

The change log says "chip_number" lexicographic order. RTL8201F is the
chip number, ISR is the register name.

You would normally sub sort register number, so i would of put
IER=0x13 before ISR=0x1e, within RTL8201F.

> 
> >  #define RTL821x_PHYSR				0x11
> >  #define RTL821x_PHYSR_DUPLEX			BIT(13)
> >  #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
> > @@ -31,6 +40,10 @@
> >  #define RTL821x_EXT_PAGE_SELECT			0x1e
> >  #define RTL821x_PAGE_SELECT			0x1f
> >  
> > +#define RTL8211E_CTRL_DELAY			BIT(13)
> > +#define RTL8211E_TX_DELAY			BIT(12)
> > +#define RTL8211E_RX_DELAY			BIT(11)
> 
> Maybe I'm reading this wrong but these don't seem sorted
> lexicographically ?

This i don't follow, you normally keep register bits next to the
register. This is particularly important when the register bits don't
have the register name embedded within it.

    Andrew

---
pw-bot: cr

Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions

[Russell King (Oracle)]

On Tue, Apr 08, 2025 at 02:17:19PM +0200, Andrew Lunn wrote:
> This i don't follow, you normally keep register bits next to the
> register. This is particularly important when the register bits don't
> have the register name embedded within it.

Agreed - the worst thing is when one reads driver code, where the
registers offsets are all defined one after each other, and the
individual register bits are defined elsewhere and without prefixes
that identify which register they pertain to or comments that identify
that.

So yes, please keep register bits and bitfield definitions next to
the register offset definition they pertain to, it's way nicer to
read that way.

Also, having register offset definitions sorted by offset means when
reading documentation, locating the definitions actually used is much
easier. Using the same value (hex or decimal) as the documentation
also aids this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RESEND net-next v5 2/4] net: phy: realtek: Clean up RTL8211E ExtPage access

[Michael Klein]

On Mon, Apr 07, 2025 at 07:56:14PM -0700, Joe Damato wrote:
>> - Factor out RTL8211E extension page access code to
>>   rtl8211e_modify_ext_page() and clean up rtl8211e_config_init()
>>
>> Signed-off-by: Michael Klein <michael@fossekall.de>
>> ---
>>  drivers/net/phy/realtek/realtek_main.c | 38 +++++++++++++++-----------
>>  1 file changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
>> index b27c0f995e56..e60c18551a4e 100644
>> --- a/drivers/net/phy/realtek/realtek_main.c
>> +++ b/drivers/net/phy/realtek/realtek_main.c
>> @@ -37,9 +37,11 @@
>>
>>  #define RTL821x_INSR				0x13
>>
>> -#define RTL821x_EXT_PAGE_SELECT			0x1e
>>  #define RTL821x_PAGE_SELECT			0x1f
>>
>> +#define RTL8211E_EXT_PAGE_SELECT		0x1e
>> +#define RTL8211E_SET_EXT_PAGE			0x07
>> +
>>  #define RTL8211E_CTRL_DELAY			BIT(13)
>>  #define RTL8211E_TX_DELAY			BIT(12)
>>  #define RTL8211E_RX_DELAY			BIT(11)
>> @@ -135,6 +137,21 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
>>  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
>>  }
>>
>> +static int rtl8211e_modify_ext_page(struct phy_device *phydev, u16 ext_page,
>> +				    u32 regnum, u16 mask, u16 set)
>> +{
>> +	int oldpage, ret = 0;
>> +
>> +	oldpage = phy_select_page(phydev, RTL8211E_SET_EXT_PAGE);
>> +	if (oldpage >= 0) {
>> +		ret = __phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, ext_page);
>> +		if (ret == 0)
>> +			ret = __phy_modify(phydev, regnum, mask, set);
>> +	}
>> +
>> +	return phy_restore_page(phydev, oldpage, ret);
>> +}
>> +
>>  static int rtl821x_probe(struct phy_device *phydev)
>>  {
>>  	struct device *dev = &phydev->mdio.dev;
>> @@ -607,7 +624,9 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
>>
>>  static int rtl8211e_config_init(struct phy_device *phydev)
>>  {
>> -	int ret = 0, oldpage;
>> +	const u16 delay_mask = RTL8211E_CTRL_DELAY |
>> +			       RTL8211E_TX_DELAY |
>> +			       RTL8211E_RX_DELAY;
>>  	u16 val;
>>
>>  	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
>> @@ -637,20 +656,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
>>  	 * 12 = RX Delay, 11 = TX Delay
>>  	 * 10:0 = Test && debug settings reserved by realtek
>>  	 */
>> -	oldpage = phy_select_page(phydev, 0x7);
>> -	if (oldpage < 0)
>> -		goto err_restore_page;
>> -
>> -	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
>> -	if (ret)
>> -		goto err_restore_page;
>> -
>> -	ret = __phy_modify(phydev, 0x1c, RTL8211E_CTRL_DELAY
>> -			   | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
>> -			   val);
>> -
>> -err_restore_page:
>> -	return phy_restore_page(phydev, oldpage, ret);
>> +	return rtl8211e_modify_ext_page(phydev, 0xa4, 0x1c, delay_mask, val);
>>  }
>
>Seems good to add RTL8211E_SET_EXT_PAGE to remove a constant from
>the code. Any reason to avoid adding constants for 0xa4 and 0x1c ?

My copy of the datasheet does not document this register, so I did not
feel qualified to come up with a meaningful name.

Re: [RESEND net-next v5 2/4] net: phy: realtek: Clean up RTL8211E ExtPage access

[Andrew Lunn]

On Tue, Apr 08, 2025 at 08:47:20PM +0200, Michael Klein wrote:
> On Mon, Apr 07, 2025 at 07:56:14PM -0700, Joe Damato wrote:
> > > - Factor out RTL8211E extension page access code to
> > >   rtl8211e_modify_ext_page() and clean up rtl8211e_config_init()
> > > 
> > > Signed-off-by: Michael Klein <michael@fossekall.de>
> > > ---
> > >  drivers/net/phy/realtek/realtek_main.c | 38 +++++++++++++++-----------
> > >  1 file changed, 22 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> > > index b27c0f995e56..e60c18551a4e 100644
> > > --- a/drivers/net/phy/realtek/realtek_main.c
> > > +++ b/drivers/net/phy/realtek/realtek_main.c
> > > @@ -37,9 +37,11 @@
> > > 
> > >  #define RTL821x_INSR				0x13
> > > 
> > > -#define RTL821x_EXT_PAGE_SELECT			0x1e
> > >  #define RTL821x_PAGE_SELECT			0x1f
> > > 
> > > +#define RTL8211E_EXT_PAGE_SELECT		0x1e
> > > +#define RTL8211E_SET_EXT_PAGE			0x07
> > > +
> > >  #define RTL8211E_CTRL_DELAY			BIT(13)
> > >  #define RTL8211E_TX_DELAY			BIT(12)
> > >  #define RTL8211E_RX_DELAY			BIT(11)
> > > @@ -135,6 +137,21 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> > >  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> > >  }
> > > 
> > > +static int rtl8211e_modify_ext_page(struct phy_device *phydev, u16 ext_page,
> > > +				    u32 regnum, u16 mask, u16 set)
> > > +{
> > > +	int oldpage, ret = 0;
> > > +
> > > +	oldpage = phy_select_page(phydev, RTL8211E_SET_EXT_PAGE);
> > > +	if (oldpage >= 0) {
> > > +		ret = __phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, ext_page);
> > > +		if (ret == 0)
> > > +			ret = __phy_modify(phydev, regnum, mask, set);
> > > +	}
> > > +
> > > +	return phy_restore_page(phydev, oldpage, ret);
> > > +}
> > > +
> > >  static int rtl821x_probe(struct phy_device *phydev)
> > >  {
> > >  	struct device *dev = &phydev->mdio.dev;
> > > @@ -607,7 +624,9 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
> > > 
> > >  static int rtl8211e_config_init(struct phy_device *phydev)
> > >  {
> > > -	int ret = 0, oldpage;
> > > +	const u16 delay_mask = RTL8211E_CTRL_DELAY |
> > > +			       RTL8211E_TX_DELAY |
> > > +			       RTL8211E_RX_DELAY;
> > >  	u16 val;
> > > 
> > >  	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
> > > @@ -637,20 +656,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
> > >  	 * 12 = RX Delay, 11 = TX Delay
> > >  	 * 10:0 = Test && debug settings reserved by realtek
> > >  	 */
> > > -	oldpage = phy_select_page(phydev, 0x7);
> > > -	if (oldpage < 0)
> > > -		goto err_restore_page;
> > > -
> > > -	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> > > -	if (ret)
> > > -		goto err_restore_page;
> > > -
> > > -	ret = __phy_modify(phydev, 0x1c, RTL8211E_CTRL_DELAY
> > > -			   | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> > > -			   val);
> > > -
> > > -err_restore_page:
> > > -	return phy_restore_page(phydev, oldpage, ret);
> > > +	return rtl8211e_modify_ext_page(phydev, 0xa4, 0x1c, delay_mask, val);
> > >  }
> > 
> > Seems good to add RTL8211E_SET_EXT_PAGE to remove a constant from
> > the code. Any reason to avoid adding constants for 0xa4 and 0x1c ?
> 
> My copy of the datasheet does not document this register, so I did not
> feel qualified to come up with a meaningful name.

Is the page documented?

As for the register, it appears to contain RGMII delay configuration,
so why not call it RTL8211E_RGMII_DELAY ?

Sometimes you just have to make names up. If somebody has a datasheet
which lists it and wants to rename it, they can.

   Andrew

Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions

[Michael Klein]

On Tue, Apr 08, 2025 at 02:17:19PM +0200, Andrew Lunn wrote:
>On Mon, Apr 07, 2025 at 07:52:50PM -0700, Joe Damato wrote:
>> On Mon, Apr 07, 2025 at 08:21:40PM +0200, Michael Klein wrote:
>> >  #define RTL821x_PHYSR				0x11
>> >  #define RTL821x_PHYSR_DUPLEX			BIT(13)
>> >  #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
>> > @@ -31,6 +40,10 @@
>> >  #define RTL821x_EXT_PAGE_SELECT			0x1e
>> >  #define RTL821x_PAGE_SELECT			0x1f
>> >
>> > +#define RTL8211E_CTRL_DELAY			BIT(13)
>> > +#define RTL8211E_TX_DELAY			BIT(12)
>> > +#define RTL8211E_RX_DELAY			BIT(11)
>>
>> Maybe I'm reading this wrong but these don't seem sorted
>> lexicographically ?
>
>This i don't follow, you normally keep register bits next to the
>register. This is particularly important when the register bits don't
>have the register name embedded within it.

Well, there is no definition for the register these bits pertain to, 
and adding one in _this_ patch would break the scope of this change. So 
I will address this in a later patch of this series in the next version.

-- 
Michael