[PATCH net-next 0/5] Realtek Ethernet PHY driver improvements

[PATCH net-next 0/5] Realtek Ethernet PHY driver improvements

[Martin Blumenstingl]

This series provides some small improvements and cleanups for the
Realtek Ethernet PHY driver.
None of the patches in this series should change any functionality.
The goal is to make the code a bit easier to read by:
- re-using the BIT and GENMASK macros (which makes it easier to compare
  the #defines in the kernel with the values from the datasheets)
- rename a #define from a generic name to a PHY-specific name since it's
  only used for one specific PHY
- logically group the register #defines and their register bit #defines
  together
- indentation cleanups
- removed some code duplicating for reading/writing registers on a
  Realtek specific "page"


Martin Blumenstingl (5):
  net: phy: realtek: use the BIT and GENMASK macros
  net: phy: realtek: rename RTL821x_INER_INIT to RTL8211B_INER_INIT
  net: phy: realtek: group all register bit #defines for RTL821x_INER
  net: phy: realtek: use the same indentation for all #defines
  net: phy: realtek: add utility functions to read/write page addresses

 drivers/net/phy/realtek.c | 116 ++++++++++++++++++++++++++++------------------
 1 file changed, 72 insertions(+), 44 deletions(-)

-- 
2.15.1

[PATCH net-next 1/5] net: phy: realtek: use the BIT and GENMASK macros

[Martin Blumenstingl]

This makes it easier to compare the #defines with the datasheets.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index eda0a6e86918..9708aa9c58dd 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -13,21 +13,22 @@
  * option) any later version.
  *
  */
+#include <linux/bitops.h>
 #include <linux/phy.h>
 #include <linux/module.h>
 
 #define RTL821x_PHYSR		0x11
-#define RTL821x_PHYSR_DUPLEX	0x2000
-#define RTL821x_PHYSR_SPEED	0xc000
+#define RTL821x_PHYSR_DUPLEX	BIT(13)
+#define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
 #define RTL821x_INER		0x12
 #define RTL821x_INER_INIT	0x6400
 #define RTL821x_INSR		0x13
 #define RTL821x_PAGE_SELECT	0x1f
-#define RTL8211E_INER_LINK_STATUS 0x400
+#define RTL8211E_INER_LINK_STATUS	BIT(10)
 
-#define RTL8211F_INER_LINK_STATUS 0x0010
+#define RTL8211F_INER_LINK_STATUS	BIT(4)
 #define RTL8211F_INSR		0x1d
-#define RTL8211F_TX_DELAY	0x100
+#define RTL8211F_TX_DELAY	BIT(8)
 
 #define RTL8201F_ISR		0x1e
 #define RTL8201F_IER		0x13
-- 
2.15.1

Re: [PATCH net-next 1/5] net: phy: realtek: use the BIT and GENMASK macros

[Andrew Lunn]

On Sat, Dec 02, 2017 at 10:51:24PM +0100, Martin Blumenstingl wrote:
> This makes it easier to compare the #defines with the datasheets.
> No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

[PATCH net-next 2/5] net: phy: realtek: rename RTL821x_INER_INIT to RTL8211B_INER_INIT

[Martin Blumenstingl]

This macro is only used by the RTL8211B code. RTL8211E and RTL8211F both
use other bits to initialize the RTL821x_INER register.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 9708aa9c58dd..59f0688e4d28 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -21,7 +21,7 @@
 #define RTL821x_PHYSR_DUPLEX	BIT(13)
 #define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
 #define RTL821x_INER		0x12
-#define RTL821x_INER_INIT	0x6400
+#define RTL8211B_INER_INIT	0x6400
 #define RTL821x_INSR		0x13
 #define RTL821x_PAGE_SELECT	0x1f
 #define RTL8211E_INER_LINK_STATUS	BIT(10)
@@ -92,7 +92,7 @@ static int rtl8211b_config_intr(struct phy_device *phydev)
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
 		err = phy_write(phydev, RTL821x_INER,
-				RTL821x_INER_INIT);
+				RTL8211B_INER_INIT);
 	else
 		err = phy_write(phydev, RTL821x_INER, 0);
 
-- 
2.15.1

Re: [PATCH net-next 2/5] net: phy: realtek: rename RTL821x_INER_INIT to RTL8211B_INER_INIT

[Andrew Lunn]

On Sat, Dec 02, 2017 at 10:51:25PM +0100, Martin Blumenstingl wrote:
> This macro is only used by the RTL8211B code. RTL8211E and RTL8211F both
> use other bits to initialize the RTL821x_INER register.
> No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

[PATCH net-next 3/5] net: phy: realtek: group all register bit #defines for RTL821x_INER

[Martin Blumenstingl]

This simply moves all register bit #defines which describe the (PHY
specific) bits in the RTL821x_INER right below the RTL821x_INER register
definition. This makes it easier to spot which registers and bits belong
together.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 59f0688e4d28..da263a92d6b1 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -20,13 +20,16 @@
 #define RTL821x_PHYSR		0x11
 #define RTL821x_PHYSR_DUPLEX	BIT(13)
 #define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
+
 #define RTL821x_INER		0x12
 #define RTL8211B_INER_INIT	0x6400
+#define RTL8211E_INER_LINK_STATUS	BIT(10)
+#define RTL8211F_INER_LINK_STATUS	BIT(4)
+
 #define RTL821x_INSR		0x13
+
 #define RTL821x_PAGE_SELECT	0x1f
-#define RTL8211E_INER_LINK_STATUS	BIT(10)
 
-#define RTL8211F_INER_LINK_STATUS	BIT(4)
 #define RTL8211F_INSR		0x1d
 #define RTL8211F_TX_DELAY	BIT(8)
 
-- 
2.15.1

Re: [PATCH net-next 3/5] net: phy: realtek: group all register bit #defines for RTL821x_INER

[Andrew Lunn]

On Sat, Dec 02, 2017 at 10:51:26PM +0100, Martin Blumenstingl wrote:
> This simply moves all register bit #defines which describe the (PHY
> specific) bits in the RTL821x_INER right below the RTL821x_INER register
> definition. This makes it easier to spot which registers and bits belong
> together.
> No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

[PATCH net-next 4/5] net: phy: realtek: use the same indentation for all #defines

[Martin Blumenstingl]

This simply makes the code easier to read. No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index da263a92d6b1..d6868e8daaab 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -17,24 +17,25 @@
 #include <linux/phy.h>
 #include <linux/module.h>
 
-#define RTL821x_PHYSR		0x11
-#define RTL821x_PHYSR_DUPLEX	BIT(13)
-#define RTL821x_PHYSR_SPEED	GENMASK(15, 14)
+#define RTL821x_PHYSR				0x11
+#define RTL821x_PHYSR_DUPLEX			BIT(13)
+#define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
 
-#define RTL821x_INER		0x12
-#define RTL8211B_INER_INIT	0x6400
-#define RTL8211E_INER_LINK_STATUS	BIT(10)
-#define RTL8211F_INER_LINK_STATUS	BIT(4)
+#define RTL821x_INER				0x12
+#define RTL8211B_INER_INIT			0x6400
+#define RTL8211E_INER_LINK_STATUS		BIT(10)
+#define RTL8211F_INER_LINK_STATUS		BIT(4)
 
-#define RTL821x_INSR		0x13
+#define RTL821x_INSR				0x13
 
-#define RTL821x_PAGE_SELECT	0x1f
+#define RTL821x_PAGE_SELECT			0x1f
 
-#define RTL8211F_INSR		0x1d
-#define RTL8211F_TX_DELAY	BIT(8)
+#define RTL8211F_INSR				0x1d
 
-#define RTL8201F_ISR		0x1e
-#define RTL8201F_IER		0x13
+#define RTL8211F_TX_DELAY			BIT(8)
+
+#define RTL8201F_ISR				0x1e
+#define RTL8201F_IER				0x13
 
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
-- 
2.15.1

Re: [PATCH net-next 4/5] net: phy: realtek: use the same indentation for all #defines

[Andrew Lunn]

On Sat, Dec 02, 2017 at 10:51:27PM +0100, Martin Blumenstingl wrote:
> This simply makes the code easier to read. No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

[PATCH net-next 5/5] net: phy: realtek: add utility functions to read/write page addresses

[Martin Blumenstingl]

Realtek PHYs implement the concept of so-called "extension pages". The
reason for this is probably because these PHYs expose more registers
than available in the standard address range.
After all read/write operations on such a page are done the driver
should switch back to page 0 where the standard MII registers (such as
MII_BMCR) are available.

When referring to such a register the datasheets of RTL8211E and
RTL8211F always specify:
- the page / "ext. page" which has to be written to RTL821x_PAGE_SELECT
- an address (sometimes also called reg)

These new utility functions make the existing code easier to read since
it removes some duplication (switching back to page 0 is done within the
new helpers for example).

No functional changes are intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/realtek.c | 83 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index d6868e8daaab..5416ec5af042 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -41,6 +41,39 @@ MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+static int rtl8211x_page_read(struct phy_device *phydev, u16 page, u16 address)
+{
+	int ret;
+
+	ret = phy_write(phydev, RTL821x_PAGE_SELECT, page);
+	if (ret)
+		return ret;
+
+	ret = phy_read(phydev, address);
+
+	/* restore to default page 0 */
+	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+
+	return ret;
+}
+
+static int rtl8211x_page_write(struct phy_device *phydev, u16 page,
+			       u16 address, u16 val)
+{
+	int ret;
+
+	ret = phy_write(phydev, RTL821x_PAGE_SELECT, page);
+	if (ret)
+		return ret;
+
+	ret = phy_write(phydev, address, val);
+
+	/* restore to default page 0 */
+	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+
+	return ret;
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -63,31 +96,21 @@ static int rtl8211f_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa43);
-	err = phy_read(phydev, RTL8211F_INSR);
-	/* restore to default page 0 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+	err = rtl8211x_page_read(phydev, 0xa43, RTL8211F_INSR);
 
 	return (err < 0) ? err : 0;
 }
 
 static int rtl8201_config_intr(struct phy_device *phydev)
 {
-	int err;
-
-	/* switch to page 7 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x7);
+	u16 val;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL8201F_IER,
-				BIT(13) | BIT(12) | BIT(11));
+		val = BIT(13) | BIT(12) | BIT(11);
 	else
-		err = phy_write(phydev, RTL8201F_IER, 0);
+		val = 0;
 
-	/* restore to default page 0 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
-
-	return err;
+	return rtl8211x_page_write(phydev, 0x7, RTL8201F_IER, val);
 }
 
 static int rtl8211b_config_intr(struct phy_device *phydev)
@@ -118,41 +141,41 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 
 static int rtl8211f_config_intr(struct phy_device *phydev)
 {
-	int err;
+	u16 val;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa42);
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		err = phy_write(phydev, RTL821x_INER,
-				RTL8211F_INER_LINK_STATUS);
+		val = RTL8211F_INER_LINK_STATUS;
 	else
-		err = phy_write(phydev, RTL821x_INER, 0);
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
+		val = 0;
 
-	return err;
+	return rtl8211x_page_write(phydev, 0xa42, RTL821x_INER, val);
 }
 
 static int rtl8211f_config_init(struct phy_device *phydev)
 {
 	int ret;
-	u16 reg;
+	u16 val;
 
 	ret = genphy_config_init(phydev);
 	if (ret < 0)
 		return ret;
 
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0xd08);
-	reg = phy_read(phydev, 0x11);
+	ret = rtl8211x_page_read(phydev, 0xd08, 0x11);
+	if (ret < 0)
+		return ret;
+
+	val = ret & 0xffff;
 
 	/* enable TX-delay for rgmii-id and rgmii-txid, otherwise disable it */
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
 	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
-		reg |= RTL8211F_TX_DELAY;
+		val |= RTL8211F_TX_DELAY;
 	else
-		reg &= ~RTL8211F_TX_DELAY;
+		val &= ~RTL8211F_TX_DELAY;
 
-	phy_write(phydev, 0x11, reg);
-	/* restore to default page 0 */
-	phy_write(phydev, RTL821x_PAGE_SELECT, 0x0);
+	ret = rtl8211x_page_write(phydev, 0xd08, 0x11, val);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
2.15.1

Re: [PATCH net-next 5/5] net: phy: realtek: add utility functions to read/write page addresses

[Andrew Lunn]

On Sat, Dec 02, 2017 at 10:51:28PM +0100, Martin Blumenstingl wrote:
> Realtek PHYs implement the concept of so-called "extension pages". The
> reason for this is probably because these PHYs expose more registers
> than available in the standard address range.
> After all read/write operations on such a page are done the driver
> should switch back to page 0 where the standard MII registers (such as
> MII_BMCR) are available.
> 
> When referring to such a register the datasheets of RTL8211E and
> RTL8211F always specify:
> - the page / "ext. page" which has to be written to RTL821x_PAGE_SELECT
> - an address (sometimes also called reg)
> 
> These new utility functions make the existing code easier to read since
> it removes some duplication (switching back to page 0 is done within the
> new helpers for example).
> 
> No functional changes are intended.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

Re: [PATCH net-next 0/5] Realtek Ethernet PHY driver improvements

[David Miller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Sat,  2 Dec 2017 22:51:23 +0100

> This series provides some small improvements and cleanups for the
> Realtek Ethernet PHY driver.
> None of the patches in this series should change any functionality.
> The goal is to make the code a bit easier to read by:
> - re-using the BIT and GENMASK macros (which makes it easier to compare
>   the #defines in the kernel with the values from the datasheets)
> - rename a #define from a generic name to a PHY-specific name since it's
>   only used for one specific PHY
> - logically group the register #defines and their register bit #defines
>   together
> - indentation cleanups
> - removed some code duplicating for reading/writing registers on a
>   Realtek specific "page"

Series applied.