[PATCH net-next 0/3] r8169: use rtl821x_modify_extpage exported from Realtek PHY driver

[PATCH net-next 0/3] r8169: use rtl821x_modify_extpage exported from Realtek PHY driver

[Heiner Kallweit]

Certain Realtek PHY's support a proprietary "extended page" access mode
that is used in the Realtek PHY driver and in r8169 network driver.
Let's implement it properly in the Realtek PHY driver and export it for
use in other drivers like r8169.

Heiner Kallweit (3):
  net: phy: realtek: export rtl821x_modify_extpage
  r8169: use rtl821x_modify_extpage
  r8169: consider new hard dependency on REALTEK_PHY

 drivers/net/ethernet/realtek/Kconfig      |  3 +-
 drivers/net/ethernet/realtek/r8169_main.c | 41 +++++++++--------------
 drivers/net/phy/realtek.c                 | 36 ++++++++++++--------
 include/linux/realtek_phy.h               |  8 +++++
 4 files changed, 46 insertions(+), 42 deletions(-)
 create mode 100644 include/linux/realtek_phy.h

-- 
2.24.0

[PATCH net-next 1/3] net: phy: realtek: export rtl821x_modify_extpage

[Heiner Kallweit]

Certain Realtek PHY's support a proprietary "extended page" access mode
that is used in the Realtek PHY driver and in r8169 network driver.
Let's implement it properly in the Realtek PHY driver and export it for
use in other drivers like r8169.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/realtek.c   | 36 ++++++++++++++++++++++--------------
 include/linux/realtek_phy.h |  8 ++++++++
 2 files changed, 30 insertions(+), 14 deletions(-)
 create mode 100644 include/linux/realtek_phy.h

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 677c45985..89f125191 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -11,6 +11,7 @@
 #include <linux/bitops.h>
 #include <linux/phy.h>
 #include <linux/module.h>
+#include <linux/realtek_phy.h>
 
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
@@ -63,6 +64,24 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+int rtl821x_modify_extpage(struct phy_device *phydev, int extpage, int reg,
+			   u16 mask, u16 val)
+{
+	int oldpage = phy_select_page(phydev, 0x0007), ret = 0;
+
+	if (oldpage < 0)
+		goto out;
+
+	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, extpage);
+	if (ret)
+		goto out;
+
+	ret = __phy_modify(phydev, reg, mask, val);
+out:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+EXPORT_SYMBOL(rtl821x_modify_extpage);
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -194,7 +213,6 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
-	int ret = 0, oldpage;
 	u16 val;
 
 	/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
@@ -223,19 +241,9 @@ static int rtl8211e_config_init(struct phy_device *phydev)
 	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
 	 * for details).
 	 */
-	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_TX_DELAY | RTL8211E_RX_DELAY,
-			   val);
-
-err_restore_page:
-	return phy_restore_page(phydev, oldpage, ret);
+	return rtl821x_modify_extpage(phydev, 0xa4, 0x1c,
+				      RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+				      val);
 }
 
 static int rtl8211b_suspend(struct phy_device *phydev)
diff --git a/include/linux/realtek_phy.h b/include/linux/realtek_phy.h
new file mode 100644
index 000000000..dd9eee56f
--- /dev/null
+++ b/include/linux/realtek_phy.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _REALTEK_PHY_H
+#define _REALTEK_PHY_H
+
+int rtl821x_modify_extpage(struct phy_device *phydev, int extpage,
+			   int reg, u16 mask, u16 val);
+
+#endif /* _REALTEK_PHY_H */
-- 
2.24.0

Re: [PATCH net-next 1/3] net: phy: realtek: export rtl821x_modify_extpage

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

Hi Heiner,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v5.4-rc7 next-20191113]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Heiner-Kallweit/r8169-use-rtl821x_modify_extpage-exported-from-Realtek-PHY-driver/20191113-172323
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git e0580b50d9d4b61d6c56085acdc2bb01c040aca9
config: x86_64-randconfig-a001-201945 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:0:0:
>> include/linux/realtek_phy.h:6:16: error: unknown type name 'u16'
          int reg, u16 mask, u16 val);
                   ^~~
   include/linux/realtek_phy.h:6:26: error: unknown type name 'u16'
          int reg, u16 mask, u16 val);
                             ^~~

vim +/u16 +6 include/linux/realtek_phy.h

     4	
     5	int rtl821x_modify_extpage(struct phy_device *phydev, int extpage,
   > 6				   int reg, u16 mask, u16 val);
     7	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35469 bytes --]

[PATCH net-next 2/3] r8169: use rtl821x_modify_extpage

[Heiner Kallweit]

Now that the "extended page" access function is exported by the
Realtek PHY driver we don't have to implement it too.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 40 +++++++++--------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index c177837b9..785987aae 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/realtek_phy.h>
 #include <linux/if_vlan.h>
 #include <linux/crc32.h>
 #include <linux/in.h>
@@ -1089,17 +1090,6 @@ static void rtl_w0w1_phy(struct rtl8169_private *tp, int reg_addr, int p, int m)
 	rtl_writephy(tp, reg_addr, (val & ~m) | p);
 }
 
-static void r8168d_modify_extpage(struct phy_device *phydev, int extpage,
-				  int reg, u16 mask, u16 val)
-{
-	int oldpage = phy_select_page(phydev, 0x0007);
-
-	__phy_write(phydev, 0x1e, extpage);
-	__phy_modify(phydev, reg, mask, val);
-
-	phy_restore_page(phydev, oldpage, 0);
-}
-
 static void r8168d_phy_param(struct phy_device *phydev, u16 parm,
 			     u16 mask, u16 val)
 {
@@ -2850,13 +2840,13 @@ static void rtl8168d_3_hw_phy_config(struct rtl8169_private *tp)
 
 	rtl_writephy_batch(tp, phy_reg_init);
 
-	r8168d_modify_extpage(tp->phydev, 0x0023, 0x16, 0xffff, 0x0000);
+	rtl821x_modify_extpage(tp->phydev, 0x0023, 0x16, 0xffff, 0x0000);
 }
 
 static void rtl8168d_4_hw_phy_config(struct rtl8169_private *tp)
 {
 	phy_write_paged(tp->phydev, 0x0001, 0x17, 0x0cc0);
-	r8168d_modify_extpage(tp->phydev, 0x002d, 0x18, 0xffff, 0x0040);
+	rtl821x_modify_extpage(tp->phydev, 0x002d, 0x18, 0xffff, 0x0040);
 	phy_set_bits(tp->phydev, 0x0d, BIT(5));
 }
 
@@ -2882,24 +2872,24 @@ static void rtl8168e_1_hw_phy_config(struct rtl8169_private *tp)
 	rtl_writephy_batch(tp, phy_reg_init);
 
 	/* Update PFM & 10M TX idle timer */
-	r8168d_modify_extpage(phydev, 0x002f, 0x15, 0xffff, 0x1919);
+	rtl821x_modify_extpage(phydev, 0x002f, 0x15, 0xffff, 0x1919);
 
-	r8168d_modify_extpage(phydev, 0x00ac, 0x18, 0xffff, 0x0006);
+	rtl821x_modify_extpage(phydev, 0x00ac, 0x18, 0xffff, 0x0006);
 
 	/* DCO enable for 10M IDLE Power */
-	r8168d_modify_extpage(phydev, 0x0023, 0x17, 0x0000, 0x0006);
+	rtl821x_modify_extpage(phydev, 0x0023, 0x17, 0x0000, 0x0006);
 
 	/* For impedance matching */
 	phy_modify_paged(phydev, 0x0002, 0x08, 0x7f00, 0x8000);
 
 	/* PHY auto speed down */
-	r8168d_modify_extpage(phydev, 0x002d, 0x18, 0x0000, 0x0050);
+	rtl821x_modify_extpage(phydev, 0x002d, 0x18, 0x0000, 0x0050);
 	phy_set_bits(phydev, 0x14, BIT(15));
 
 	r8168d_phy_param(phydev, 0x8b86, 0x0000, 0x0001);
 	r8168d_phy_param(phydev, 0x8b85, 0x2000, 0x0000);
 
-	r8168d_modify_extpage(phydev, 0x0020, 0x15, 0x1100, 0x0000);
+	rtl821x_modify_extpage(phydev, 0x0020, 0x15, 0x1100, 0x0000);
 	phy_write_paged(phydev, 0x0006, 0x00, 0x5a00);
 
 	phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0x0000);
@@ -2926,7 +2916,7 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp)
 	rtl_apply_firmware(tp);
 
 	/* Enable Delay cap */
-	r8168d_modify_extpage(phydev, 0x00ac, 0x18, 0xffff, 0x0006);
+	rtl821x_modify_extpage(phydev, 0x00ac, 0x18, 0xffff, 0x0006);
 
 	/* Channel estimation fine tune */
 	phy_write_paged(phydev, 0x0003, 0x09, 0xa20f);
@@ -2943,7 +2933,7 @@ static void rtl8168e_2_hw_phy_config(struct rtl8169_private *tp)
 	rtl_writephy(tp, 0x1f, 0x0000);
 
 	/* PHY auto speed down */
-	r8168d_modify_extpage(phydev, 0x002d, 0x18, 0x0000, 0x0010);
+	rtl821x_modify_extpage(phydev, 0x002d, 0x18, 0x0000, 0x0010);
 	phy_set_bits(phydev, 0x14, BIT(15));
 
 	/* improve 10M EEE waveform */
@@ -2976,7 +2966,7 @@ static void rtl8168f_hw_phy_config(struct rtl8169_private *tp)
 	r8168d_phy_param(phydev, 0x8b80, 0x0000, 0x0006);
 
 	/* PHY auto speed down */
-	r8168d_modify_extpage(phydev, 0x002d, 0x18, 0x0000, 0x0010);
+	rtl821x_modify_extpage(phydev, 0x002d, 0x18, 0x0000, 0x0010);
 	phy_set_bits(phydev, 0x14, BIT(15));
 
 	/* Improve 10M EEE waveform */
@@ -3000,8 +2990,8 @@ static void rtl8168f_1_hw_phy_config(struct rtl8169_private *tp)
 	r8168d_phy_param(phydev, 0x8b5e, 0xffff, 0x0000);
 	r8168d_phy_param(phydev, 0x8b67, 0xffff, 0x0000);
 	r8168d_phy_param(phydev, 0x8b70, 0xffff, 0x0000);
-	r8168d_modify_extpage(phydev, 0x0078, 0x17, 0xffff, 0x0000);
-	r8168d_modify_extpage(phydev, 0x0078, 0x19, 0xffff, 0x00fb);
+	rtl821x_modify_extpage(phydev, 0x0078, 0x17, 0xffff, 0x0000);
+	rtl821x_modify_extpage(phydev, 0x0078, 0x19, 0xffff, 0x00fb);
 
 	/* Modify green table for 10M */
 	r8168d_phy_param(phydev, 0x8b79, 0xffff, 0xaa00);
@@ -3041,8 +3031,8 @@ static void rtl8411_hw_phy_config(struct rtl8169_private *tp)
 	r8168d_phy_param(phydev, 0x8b5e, 0xffff, 0x0000);
 	r8168d_phy_param(phydev, 0x8b67, 0xffff, 0x0000);
 	r8168d_phy_param(phydev, 0x8b70, 0xffff, 0x0000);
-	r8168d_modify_extpage(phydev, 0x0078, 0x17, 0xffff, 0x0000);
-	r8168d_modify_extpage(phydev, 0x0078, 0x19, 0xffff, 0x00aa);
+	rtl821x_modify_extpage(phydev, 0x0078, 0x17, 0xffff, 0x0000);
+	rtl821x_modify_extpage(phydev, 0x0078, 0x19, 0xffff, 0x00aa);
 
 	/* Modify green table for 10M */
 	r8168d_phy_param(phydev, 0x8b79, 0xffff, 0xaa00);
-- 
2.24.0

[PATCH net-next 3/3] r8169: consider new hard dependency on REALTEK_PHY

[Heiner Kallweit]

Now r8169 has a hard dependency on the Realtek PHY driver. Reflect this
in Kconfig and remove the soft dependency. REALTEK_PHY depends on
PHYLIB, therefore r8169 doesn't need an explicit dependency on PHYLIB.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/Kconfig      | 3 +--
 drivers/net/ethernet/realtek/r8169_main.c | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 5e0b9d2f1..9551ad5c1 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -98,10 +98,9 @@ config 8139_OLD_RX_RESET
 config R8169
 	tristate "Realtek 8169/8168/8101/8125 ethernet support"
 	depends on PCI
+	depends on REALTEK_PHY
 	select FW_LOADER
 	select CRC32
-	select PHYLIB
-	select REALTEK_PHY
 	---help---
 	  Say Y here if you have a Realtek Ethernet adapter belonging to
 	  the following families:
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 785987aae..73186e9b0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -697,7 +697,6 @@ MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
 MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
 module_param_named(debug, debug.msg_enable, int, 0);
 MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 16=all)");
-MODULE_SOFTDEP("pre: realtek");
 MODULE_LICENSE("GPL");
 MODULE_FIRMWARE(FIRMWARE_8168D_1);
 MODULE_FIRMWARE(FIRMWARE_8168D_2);
-- 
2.24.0

Re: [PATCH net-next 0/3] r8169: use rtl821x_modify_extpage exported from Realtek PHY driver

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 12 Nov 2019 22:22:55 +0100

> Certain Realtek PHY's support a proprietary "extended page" access mode
> that is used in the Realtek PHY driver and in r8169 network driver.
> Let's implement it properly in the Realtek PHY driver and export it for
> use in other drivers like r8169.

Applied, but I really wish these deps worked more nicely.

Now I have to know what PHY drivers my ethernet card uses just to have
the main driver show up as a possible option in the Kconfig.  That's
not nice at all.

Re: [PATCH net-next 0/3] r8169: use rtl821x_modify_extpage exported from Realtek PHY driver

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 12 Nov 2019 19:56:03 -0800 (PST)

> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Tue, 12 Nov 2019 22:22:55 +0100
> 
>> Certain Realtek PHY's support a proprietary "extended page" access mode
>> that is used in the Realtek PHY driver and in r8169 network driver.
>> Let's implement it properly in the Realtek PHY driver and export it for
>> use in other drivers like r8169.
> 
> Applied, but I really wish these deps worked more nicely.
> 
> Now I have to know what PHY drivers my ethernet card uses just to have
> the main driver show up as a possible option in the Kconfig.  That's
> not nice at all.

I had to revert, missing types.h include in the new header file or similar:

./include/linux/realtek_phy.h:6:16: error: unknown type name ‘u16’
       int reg, u16 mask, u16 val);
                ^~~
./include/linux/realtek_phy.h:6:26: error: unknown type name ‘u16’
       int reg, u16 mask, u16 val);
                          ^~~

Re: [PATCH net-next 0/3] r8169: use rtl821x_modify_extpage exported from Realtek PHY driver

[Florian Fainelli]

On 11/12/2019 1:22 PM, Heiner Kallweit wrote:
> Certain Realtek PHY's support a proprietary "extended page" access mode
> that is used in the Realtek PHY driver and in r8169 network driver.
> Let's implement it properly in the Realtek PHY driver and export it for
> use in other drivers like r8169.
> 
> Heiner Kallweit (3):
>   net: phy: realtek: export rtl821x_modify_extpage
>   r8169: use rtl821x_modify_extpage
>   r8169: consider new hard dependency on REALTEK_PHY
> 
>  drivers/net/ethernet/realtek/Kconfig      |  3 +-
>  drivers/net/ethernet/realtek/r8169_main.c | 41 +++++++++--------------
>  drivers/net/phy/realtek.c                 | 36 ++++++++++++--------
>  include/linux/realtek_phy.h               |  8 +++++
>  4 files changed, 46 insertions(+), 42 deletions(-)

The delta is not that impressive and this creates not quite a layering
violation, but some really weird inter dependency if nothing else. Could
we simply move all the PHY programming down the PHY driver instead or is
this too cumbersome/fragile to do right now?
-- 
Florian

Re: [PATCH net-next 0/3] r8169: use rtl821x_modify_extpage exported from Realtek PHY driver

[Heiner Kallweit]

On 13.11.2019 05:13, Florian Fainelli wrote:
> 
> 
> On 11/12/2019 1:22 PM, Heiner Kallweit wrote:
>> Certain Realtek PHY's support a proprietary "extended page" access mode
>> that is used in the Realtek PHY driver and in r8169 network driver.
>> Let's implement it properly in the Realtek PHY driver and export it for
>> use in other drivers like r8169.
>>
>> Heiner Kallweit (3):
>>   net: phy: realtek: export rtl821x_modify_extpage
>>   r8169: use rtl821x_modify_extpage
>>   r8169: consider new hard dependency on REALTEK_PHY
>>
>>  drivers/net/ethernet/realtek/Kconfig      |  3 +-
>>  drivers/net/ethernet/realtek/r8169_main.c | 41 +++++++++--------------
>>  drivers/net/phy/realtek.c                 | 36 ++++++++++++--------
>>  include/linux/realtek_phy.h               |  8 +++++
>>  4 files changed, 46 insertions(+), 42 deletions(-)
> 
> The delta is not that impressive and this creates not quite a layering
> violation, but some really weird inter dependency if nothing else. Could
> we simply move all the PHY programming down the PHY driver instead or is
> this too cumbersome/fragile to do right now?
> 
The Realtek chips come with integrated MAC and PHY, so the dependency
reflects the physical structure. Moving all PHY configuration to the
PHY driver would be best of course, but:
- Even though few chip versions use the PHY ID of a PHY that exists
  also standalone, the configuration sequence differs. So it seems
  they differ.
- From a certain chip version the PHY ID is always the same:
  Realtek OUI, but model and revision number being zero. Means we'd have
  to intercept the PHY ID reads and return artificial PHY ID's.
- PHY config sequence partially includes efuse reads from MAC registers,
  see rtl8168d_1_hw_phy_config. OK, maybe we could read the efuse first,
  and hand over the value to the PHY driver via PHY driver private data
  structure.
  
Heiner