Re: [PATCH v3 01/11] net: phy: Add rockchip phy driver support

[David.Wu]

Hi Andrew,

在 2017/8/2 21:21, Andrew Lunn 写道:

+static struct phy_driver rockchip_phy_driver[] = {
+{
+   .phy_id = 0x1234d400,
+   .phy_id_mask= 0xfff0,
+   .name   = "Rockchip internal EPHY",
+   .features   = (PHY_BASIC_FEATURES | SUPPORTED_Pause
+  | SUPPORTED_Asym_Pause),


Please take a look at Documentation/networking/phy.txt and
Fixes: 529ed1275263 ("net: phy: phy drivers should not set 
SUPPORTED_[Asym_]Pause")

Pause frames / flow control

  The PHY does not participate directly in flow control/pause frames except by
  making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
  MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
  controller supports such a thing. Since flow control/pause frames generation
  involves the Ethernet MAC driver, it is recommended that this driver takes 
care
  of properly indicating advertisement and support for such features by setting
  the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
  either before or after phy_connect() and/or as a result of implementing the
  ethtool::set_pauseparam feature.



Thanks for the reminder, I'll remove it.


Andrew

Re: [PATCH v3 01/11] net: phy: Add rockchip phy driver support

[Andrew Lunn]

> +static struct phy_driver rockchip_phy_driver[] = {
> +{
> + .phy_id = 0x1234d400,
> + .phy_id_mask= 0xfff0,
> + .name   = "Rockchip internal EPHY",
> + .features   = (PHY_BASIC_FEATURES | SUPPORTED_Pause
> +| SUPPORTED_Asym_Pause),

Please take a look at Documentation/networking/phy.txt and
Fixes: 529ed1275263 ("net: phy: phy drivers should not set 
SUPPORTED_[Asym_]Pause")

Pause frames / flow control

 The PHY does not participate directly in flow control/pause frames except by
 making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
 MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
 controller supports such a thing. Since flow control/pause frames generation
 involves the Ethernet MAC driver, it is recommended that this driver takes care
 of properly indicating advertisement and support for such features by setting
 the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
 either before or after phy_connect() and/or as a result of implementing the
 ethtool::set_pauseparam feature.

Andrew

Re: [PATCH v3 01/11] net: phy: Add rockchip phy driver support

[Corentin Labbe]

Hello I have some minor comment below

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

in alphabetic order please

[...]
> +static int rockchip_init_tstmode(struct phy_device *phydev)
> +{
> + int ret;
> +
> + /* Enable access to Analog and DSP register banks */
> + ret = phy_write(phydev, SMI_ADDR_TSTCNTL, 0x0400);
> + if (ret)
> + return ret;
> +
> + ret = phy_write(phydev, SMI_ADDR_TSTCNTL, 0x);
> + if (ret)
> + return ret;
> +
> + return phy_write(phydev, SMI_ADDR_TSTCNTL, 0x0400);
> +}
> +
> +static int rockchip_close_tstmode(struct phy_device *phydev)
> +{
> + /* Back to basic register bank */
> + return phy_write(phydev, SMI_ADDR_TSTCNTL, 0x);

The reuse of 0x and 0x0400 seems to promote a define use

[...]
> +static struct phy_driver rockchip_phy_driver[] = {
> +{
> + .phy_id = 0x1234d400,
> + .phy_id_mask= 0xfff0,
> + .name   = "Rockchip internal EPHY",
> + .features   = (PHY_BASIC_FEATURES | SUPPORTED_Pause
> +| SUPPORTED_Asym_Pause),
> + .flags  = PHY_IS_INTERNAL,
> + .link_change_notify = rockchip_link_change_notify,
> + .soft_reset = genphy_soft_reset,
> + .config_init= rockchip_internal_phy_config_init,
> + .config_aneg= rockchip_config_aneg,
> + .read_status= genphy_read_status,
> + .suspend= genphy_suspend,
> + .resume = rockchip_phy_resume,
> +},
> +};
> +
> +module_phy_driver(rockchip_phy_driver);
> +
> +static struct mdio_device_id __maybe_unused rockchip_phy_tbl[] = {
> + { 0x1234d400, 0xfff0 },

Same comment for phy_id, use a define

Regards
Corentin Labbe

[PATCH v3 01/11] net: phy: Add rockchip phy driver support

[David Wu]

Support internal ethernet phy currently.

Signed-off-by: David Wu 
---
 drivers/net/phy/Kconfig|   5 +
 drivers/net/phy/Makefile   |   1 +
 drivers/net/phy/rockchip.c | 229 +
 3 files changed, 235 insertions(+)
 create mode 100644 drivers/net/phy/rockchip.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 2dda720..22cc702 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -334,6 +334,11 @@ config REALTEK_PHY
---help---
  Supports the Realtek 821x PHY.
 
+config ROCKCHIP_PHY
+tristate "Driver for Rockchip Ethernet PHYs"
+---help---
+  Currently supports the internal Ethernet PHY.
+
 config SMSC_PHY
tristate "SMSC PHYs"
---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 8e9b9f3..350520e 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_MICROSEMI_PHY)   += mscc.o
 obj-$(CONFIG_NATIONAL_PHY) += national.o
 obj-$(CONFIG_QSEMI_PHY)+= qsemi.o
 obj-$(CONFIG_REALTEK_PHY)  += realtek.o
+obj-$(CONFIG_ROCKCHIP_PHY) += rockchip.o
 obj-$(CONFIG_SMSC_PHY) += smsc.o
 obj-$(CONFIG_STE10XP)  += ste10Xp.o
 obj-$(CONFIG_TERANETICS_PHY)   += teranetics.o
diff --git a/drivers/net/phy/rockchip.c b/drivers/net/phy/rockchip.c
new file mode 100644
index 000..c1f07d6
--- /dev/null
+++ b/drivers/net/phy/rockchip.c
@@ -0,0 +1,229 @@
+/**
+ * drivers/net/phy/rockchip.c
+ *
+ * Driver for ROCKCHIP Ethernet PHYs
+ *
+ * Copyright (c) 2017, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * David Wu 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define MII_INTERNAL_CTRL_STATUS   17
+#define SMI_ADDR_TSTCNTL   20
+#define SMI_ADDR_TSTREAD1  21
+#define SMI_ADDR_TSTREAD2  22
+#define SMI_ADDR_TSTWRITE  23
+#define MII_SPECIAL_CONTROL_STATUS 31
+
+#define MII_AUTO_MDIX_EN   BIT(7)
+#define MII_MDIX_ENBIT(6)
+
+#define MII_SPEED_10   BIT(2)
+#define MII_SPEED_100  BIT(3)
+
+#define TSTCNTL_RD (BIT(15) | BIT(10))
+#define TSTCNTL_WR (BIT(14) | BIT(10))
+
+#define WR_ADDR_A7CFG  0x18
+
+static int rockchip_init_tstmode(struct phy_device *phydev)
+{
+   int ret;
+
+   /* Enable access to Analog and DSP register banks */
+   ret = phy_write(phydev, SMI_ADDR_TSTCNTL, 0x0400);
+   if (ret)
+   return ret;
+
+   ret = phy_write(phydev, SMI_ADDR_TSTCNTL, 0x);
+   if (ret)
+   return ret;
+
+   return phy_write(phydev, SMI_ADDR_TSTCNTL, 0x0400);
+}
+
+static int rockchip_close_tstmode(struct phy_device *phydev)
+{
+   /* Back to basic register bank */
+   return phy_write(phydev, SMI_ADDR_TSTCNTL, 0x);
+}
+
+static int rockchip_internal_phy_analog_init(struct phy_device *phydev)
+{
+   int ret;
+
+   ret = rockchip_init_tstmode(phydev);
+   if (ret)
+   return ret;
+
+   /*
+* Adjust tx amplitude to make sginal better,
+* the default value is 0x8.
+*/
+   ret = phy_write(phydev, SMI_ADDR_TSTWRITE, 0xB);
+   if (ret)
+   return ret;
+   ret = phy_write(phydev, SMI_ADDR_TSTCNTL, TSTCNTL_WR | WR_ADDR_A7CFG);
+   if (ret)
+   return ret;
+
+   return rockchip_close_tstmode(phydev);
+}
+
+static int rockchip_internal_phy_config_init(struct phy_device *phydev)
+{
+   int val, ret;
+
+   /*
+* The auto MIDX has linked problem on some board,
+* workround to disable auto MDIX.
+*/
+   val = phy_read(phydev, MII_INTERNAL_CTRL_STATUS);
+   if (val < 0)
+   return val;
+   val &= ~MII_AUTO_MDIX_EN;
+   ret = phy_write(phydev, MII_INTERNAL_CTRL_STATUS, val);
+   if (ret)
+   return ret;
+
+   return rockchip_internal_phy_analog_init(phydev);
+}
+
+static void rockchip_link_change_notify(struct phy_device *phydev)
+{
+   int speed = SPEED_10;
+
+   if (phydev->autoneg == AUTONEG_ENABLE) {
+   int reg = phy_read(phydev, MII_SPECIAL_CONTROL_STATUS);
+
+   if (reg < 0) {
+   phydev_err(phydev, "phy_read err: %d.\n", reg);
+   return;
+   }
+
+   if (reg & MII_SPEED_100)
+   speed = SPEED_100;
+   else if (reg & MII_SPEED_10)
+   speed = SPEED_10;
+   } else {
+   int bmcr = phy_read(phydev,