[PATCH 1/4] net: phy: Add Edge-rate driver for Microsemi PHYs.

From: Nagaraju Lakkaraju <redacted>

Edge rate control support will be added for VSC 85xx Microsemi PHYs.

Signed-off-by: Nagaraju Lakkaraju <redacted>
---
 drivers/net/phy/mscc.c | 109 +++++++++++++++++++++++++++++++++++++------------
 include/linux/mscc.h   |  34 +++++++++++++++
 include/linux/phy.h    |   2 +
 3 files changed, 118 insertions(+), 27 deletions(-)
 create mode 100644 include/linux/mscc.h

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 6cc3036..963bf64 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c

@@ -11,34 +11,9 @@
 #include <linux/mdio.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
+#include <linux/mscc.h>

-enum rgmii_rx_clock_delay {
-       RGMII_RX_CLK_DELAY_0_2_NS = 0,
-       RGMII_RX_CLK_DELAY_0_8_NS = 1,
-       RGMII_RX_CLK_DELAY_1_1_NS = 2,
-       RGMII_RX_CLK_DELAY_1_7_NS = 3,
-       RGMII_RX_CLK_DELAY_2_0_NS = 4,
-       RGMII_RX_CLK_DELAY_2_3_NS = 5,
-       RGMII_RX_CLK_DELAY_2_6_NS = 6,
-       RGMII_RX_CLK_DELAY_3_4_NS = 7
-};
-
-#define MII_VSC85XX_INT_MASK              25
-#define MII_VSC85XX_INT_MASK_MASK         0xa000
-#define MII_VSC85XX_INT_STATUS            26
-
-#define MSCC_EXT_PAGE_ACCESS              31
-#define MSCC_PHY_PAGE_STANDARD            0x0000 /* Standard registers */
-#define MSCC_PHY_PAGE_EXTENDED_2          0x0002 /* Extended reg - page 2 */
-
-/* Extended Page 2 Registers */
-#define MSCC_PHY_RGMII_CNTL                       20
-#define RGMII_RX_CLK_DELAY_MASK                   0x0070
-#define RGMII_RX_CLK_DELAY_POS            4
-
-/* Microsemi PHY ID's */
-#define PHY_ID_VSC8531                            0x00070570
-#define PHY_ID_VSC8541                            0x00070770
+#include "mscc_reg.h"

 static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 {

@@ -109,6 +84,82 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
        return rc;
 }

+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+                                     u8     *rate)
+{
+       int rc;
+       u16 reg_val;
+       u8  edge_rate = *rate;
+
+       mutex_lock(&phydev->lock);
+       rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+       if (rc != 0)
+               goto out_unlock;
+       reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+       reg_val &= ~(EDGE_RATE_CNTL_MASK);
+       reg_val |= (edge_rate << EDGE_RATE_CNTL_POS);
+       phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
+       rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+       mutex_unlock(&phydev->lock);
+
+       return rc;
+}
+
+static int vsc85xx_edge_rate_cntl_get(struct phy_device *phydev,
+                                     u8     *rate)
+{
+       int rc;
+       u16 reg_val;
+
+       mutex_lock(&phydev->lock);
+       rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+       if (rc != 0)
+               goto out_unlock;
+       reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+       reg_val &= EDGE_RATE_CNTL_MASK;
+       *rate = reg_val >> EDGE_RATE_CNTL_POS;
+       rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+       mutex_unlock(&phydev->lock);
+
+       return rc;
+}
+
+static int vsc85xx_features_set(struct phy_device *phydev)
+{
+       int rc = 0;
+       struct phy_features_t *ftrs = (struct phy_features_t *)phydev->priv;
+
+       switch (ftrs->cmd) {
+       case PHY_EDGE_RATE_CONTROL:
+               rc = vsc85xx_edge_rate_cntl_set(phydev, &ftrs->rate);
+               break;
+       default:
+               break;
+       }
+
+       return rc;
+}
+
+static int vsc85xx_features_get(struct phy_device *phydev)
+{
+       int rc = 0;
+       struct phy_features_t *ftrs = (struct phy_features_t *)phydev->priv;
+
+       switch (ftrs->cmd) {
+       case PHY_EDGE_RATE_CONTROL:
+               rc = vsc85xx_edge_rate_cntl_get(phydev, &ftrs->rate);
+               break;
+       default:
+               break;
+       }
+
+       return rc;
+}
+
 /* Microsemi VSC85xx PHYs */
 static struct phy_driver vsc85xx_driver[] = {
 {

@@ -126,6 +177,8 @@ static struct phy_driver vsc85xx_driver[] = {
        .config_intr    = &vsc85xx_config_intr,
        .suspend                = &genphy_suspend,
        .resume                 = &genphy_resume,
+       .phy_features_set = &vsc85xx_features_set,
+       .phy_features_get = &vsc85xx_features_get,
 },
 {
        .phy_id                 = PHY_ID_VSC8541,

@@ -142,6 +195,8 @@ static struct phy_driver vsc85xx_driver[] = {
        .config_intr    = &vsc85xx_config_intr,
        .suspend                = &genphy_suspend,
        .resume                 = &genphy_resume,
+       .phy_features_set = &vsc85xx_features_set,
+       .phy_features_get = &vsc85xx_features_get,
 }

 };

diff --git a/include/linux/mscc.h b/include/linux/mscc.h
new file mode 100644
index 0000000..b80a2ac
--- /dev/null
+++ b/include/linux/mscc.h

@@ -0,0 +1,34 @@
+/*
+ * Driver for Microsemi VSC85xx PHYs
+ *
+ * Author: Nagaraju Lakkaraju
+ * License: Dual MIT/GPL
+ * Copyright (c) 2016 Microsemi Corporation
+ */
+
+#ifndef __LINUX_MSCC_H
+#define __LINUX_MSCC_H
+
+enum phy_features {
+       PHY_EDGE_RATE_CONTROL = 0,
+       PHY_SUPPORTED_FEATURES_MAX
+};
+
+enum rgmii_rx_clock_delay {
+       RGMII_RX_CLK_DELAY_0_2_NS = 0,
+       RGMII_RX_CLK_DELAY_0_8_NS = 1,
+       RGMII_RX_CLK_DELAY_1_1_NS = 2,
+       RGMII_RX_CLK_DELAY_1_7_NS = 3,
+       RGMII_RX_CLK_DELAY_2_0_NS = 4,
+       RGMII_RX_CLK_DELAY_2_3_NS = 5,
+       RGMII_RX_CLK_DELAY_2_6_NS = 6,
+       RGMII_RX_CLK_DELAY_3_4_NS = 7
+};
+
+struct phy_features_t {
+       enum phy_features cmd;           /* PHY Supported Features */
+       u8   rate;                       /* Edge rate control */
+};
+
+#endif /*  __LINUX_MSCC_H */
+

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2d24b28..8ec4c09 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -586,6 +586,8 @@ struct phy_driver {
        void (*get_strings)(struct phy_device *dev, u8 *data);
        void (*get_stats)(struct phy_device *dev,
                          struct ethtool_stats *stats, u64 *data);
+       int (*phy_features_set)(struct phy_device *dev);
+       int (*phy_features_get)(struct phy_device *dev);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),                \
                                      struct phy_driver, mdiodrv)

-- 
2.7.4

On Wed, Aug 24, 2016 at 12:20:03PM +0000, Raju Lakkaraju wrote:

From: Nagaraju Lakkaraju <redacted>

Edge rate control support will be added for VSC 85xx Microsemi PHYs.

Now we need the missing cover note what should be in 0/4.  What is the
big picture? How are these two functions supposed to be used? Is there
going to be a user space API via netlink? Should the MAC driver
somehow call these functions? Are you going to extend the phylib with
code to call these?

Those are all general questions for these two functions.

Now specifically for edge control, why did you decide not to use
device tree? Both the micrel and renesas phy driver uses device tree
for skew control. You need to explain why you need to do something
different to other drivers.

	  Thanks
		Andrew

Hi Andrew,

Thank you for review the code and valuable comments.
I accepted your review comments.
I too use the Device Tree for Edge-rate and MAC interface 
configuration.

Thanks,
Raju.

On Wed, Aug 24, 2016 at 02:59:34PM +0200, Andrew Lunn wrote:

EXTERNAL EMAIL


On Wed, Aug 24, 2016 at 12:20:03PM +0000, Raju Lakkaraju wrote:

quoted

From: Nagaraju Lakkaraju <redacted>

Edge rate control support will be added for VSC 85xx Microsemi PHYs.

quoted

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2d24b28..8ec4c09 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -586,6 +586,8 @@ struct phy_driver {
        void (*get_strings)(struct phy_device *dev, u8 *data);
        void (*get_stats)(struct phy_device *dev,
                          struct ethtool_stats *stats, u64 *data);
+       int (*phy_features_set)(struct phy_device *dev);
+       int (*phy_features_get)(struct phy_device *dev);
 };

Now we need the missing cover note what should be in 0/4.  What is the
big picture? How are these two functions supposed to be used? Is there
going to be a user space API via netlink? Should the MAC driver
somehow call these functions? Are you going to extend the phylib with
code to call these?

Those are all general questions for these two functions.

Now specifically for edge control, why did you decide not to use
device tree? Both the micrel and renesas phy driver uses device tree
for skew control. You need to explain why you need to do something
different to other drivers.

          Thanks
                Andrew

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the Edge-rate as per review comments and
re-sending code for review

Signed-off-by: Raju Lakkaraju <redacted>
---
 drivers/net/phy/mscc.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index c09cc4a..f0a0e8d 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c

@@ -11,6 +11,7 @@
 #include <linux/mdio.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
+#include <linux/of.h>
 
 enum rgmii_rx_clock_delay {
 	RGMII_RX_CLK_DELAY_0_2_NS = 0,

@@ -36,10 +37,20 @@ enum rgmii_rx_clock_delay {
 #define RGMII_RX_CLK_DELAY_MASK		  0x0070
 #define RGMII_RX_CLK_DELAY_POS		  4
 
+#define MSCC_PHY_WOL_MAC_CONTROL	  27
+#define EDGE_RATE_CNTL_POS		  5
+#define EDGE_RATE_CNTL_MASK		  0x00E0
+#define SECURE_ON_ENABLE		  0x8000
+#define SECURE_ON_PASSWD_LEN_4		  0x4000
+
 /* Microsemi PHY ID's */
 #define PHY_ID_VSC8531			  0x00070570
 #define PHY_ID_VSC8541			  0x00070770
 
+struct vsc8531_private {
+	u8 edge_rate;
+};
+
 static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 {
 	int rc;

@@ -48,6 +59,28 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 	return rc;
 }
 
+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+				      u8     edge_rate)
+{
+	int rc;
+	u16 reg_val;
+
+	mutex_lock(&phydev->lock);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc != 0)
+		goto out_unlock;
+	reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+	reg_val &= ~(EDGE_RATE_CNTL_MASK);
+	reg_val |= (edge_rate << EDGE_RATE_CNTL_POS);
+	phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
 static int vsc85xx_default_config(struct phy_device *phydev)
 {
 	int rc;

@@ -70,13 +103,56 @@ out_unlock:
 	return rc;
 }
 
+#ifdef CONFIG_OF_MDIO
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+	int rc;
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+
+	if (!of_node)
+		return -ENODEV;
+
+	rc = of_property_read_u8(of_node, "vsc8531,edge-rate",
+				 &vsc8531->edge_rate);
+
+	return rc;
+}
+#else
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
 	int rc;
+	struct vsc8531_private *vsc8531;
+
+	if (!phydev->priv) {
+		vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531),
+				       GFP_KERNEL);
+		if (!vsc8531)
+			return -ENOMEM;
+
+		phydev->priv = vsc8531;
+		rc = vsc8531_of_init(phydev);
+		if (rc)
+			return rc;
+	} else {
+		vsc8531 = (struct vsc8531_private *)phydev->priv;
+	}
 
 	rc = vsc85xx_default_config(phydev);
 	if (rc)
 		return rc;
+
+	rc = vsc85xx_edge_rate_cntl_set(phydev, vsc8531->edge_rate);
+	if (rc)
+		return rc;
+
 	rc = genphy_config_init(phydev);
 
 	return rc;

-- 
2.7.4

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the MAC Interface as per review comments and
re-sending code for review

Signed-off-by: Raju Lakkaraju <redacted>
---
 drivers/net/phy/mscc.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index f0a0e8d..dfbf4f3 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c

@@ -24,6 +24,16 @@ enum rgmii_rx_clock_delay {
 	RGMII_RX_CLK_DELAY_3_4_NS = 7
 };
 
+/* Microsemi VSC85xx PHY registers */
+/* IEEE 802. Std Registers */
+#define MSCC_PHY_EXT_PHY_CNTL_1           23
+#define MAC_IF_SELECTION_MASK             0x1800
+#define MAC_IF_SELECTION_GMII             0
+#define MAC_IF_SELECTION_RMII             1
+#define MAC_IF_SELECTION_RGMII            2
+#define MAC_IF_SELECTION_POS              11
+#define FAR_END_LOOPBACK_MODE_MASK        0x0008
+
 #define MII_VSC85XX_INT_MASK		  25
 #define MII_VSC85XX_INT_MASK_MASK	  0xa000
 #define MII_VSC85XX_INT_STATUS		  26

@@ -59,6 +69,52 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 	return rc;
 }
 
+static int vsc85xx_soft_reset(struct phy_device *phydev)
+{
+	int rc;
+	u16 reg_val;
+
+	reg_val = phy_read(phydev, MII_BMCR);
+	reg_val |= BMCR_RESET;
+	rc = phy_write(phydev, MII_BMCR, reg_val);
+
+	return rc;
+}
+
+static int vsc85xx_mac_if_set(struct phy_device *phydev,
+			      phy_interface_t   interface)
+{
+	int rc;
+	u16 reg_val;
+
+	mutex_lock(&phydev->lock);
+	reg_val = phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_1);
+	reg_val &= ~(MAC_IF_SELECTION_MASK);
+	switch (interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		reg_val |= (MAC_IF_SELECTION_RGMII << MAC_IF_SELECTION_POS);
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		reg_val |= (MAC_IF_SELECTION_RMII << MAC_IF_SELECTION_POS);
+		break;
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_GMII:
+	default:
+		reg_val |= (MAC_IF_SELECTION_GMII << MAC_IF_SELECTION_POS);
+		break;
+	}
+	rc = phy_write(phydev, MSCC_PHY_EXT_PHY_CNTL_1, reg_val);
+	if (rc != 0)
+		goto out_unlock;
+
+	rc = vsc85xx_soft_reset(phydev);
+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
 static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
 				      u8     edge_rate)
 {

@@ -153,6 +209,10 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	if (rc)
 		return rc;
 
+	rc = vsc85xx_mac_if_set(phydev, phydev->interface);
+	if (rc)
+		return rc;
+
 	rc = genphy_config_init(phydev);
 
 	return rc;

-- 
2.7.4

Patch 1/2:

This is Edge rate control feature.
As system and networking speeds increase, a signal's output transition,
also know as the edge rate or slew rate (V/ns), takes on greater importance
because high-speed signals come with a price. That price is an assortment of
interference problems like ringing on the line, signal overshoot and
undershoot, extended signal settling times, crosstalk noise, transmission line
reflections, false signal detection by the receiving device and electromagnetic
interference (EMI) -- all of which can negate the potential gains designers are
seeking when they try to increase system speeds through the use of higher
performance logic devices. The fact is, faster signaling edge rates can cause
a higher level of electrical noise or other type of interference that can
actually lead to slower line speeds and lower maximum system frequencies.

Microsemi PHY have the provision to configure the edge rate. Edge-rate function
program the right value based on Device Tree configuration.

Tested on Beaglebone Black with VSC 8531 PHY.

Patch 2/2:

This is MAC interface feature.
Microsemi PHY can support RGMII, RMII or GMII/MII interface between MAC and PHY.
MAC-IF function program the right value based on Device tree configuration.

Tested on Beaglebone Black with VSC 8531 PHY.

Raju Lakkaraju (2):
  net: phy: Add Edge-rate driver for Microsemi PHYs.
  net: phy: Add MAC-IF driver for Microsemi PHYs.

 drivers/net/phy/mscc.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

-- 
2.7.4

On Thu, Sep 08, 2016 at 02:47:20PM +0530, Raju Lakkaraju wrote:

Patch 1/2:

This is Edge rate control feature.
As system and networking speeds increase, a signal's output transition,
also know as the edge rate or slew rate (V/ns), takes on greater importance
because high-speed signals come with a price. That price is an assortment of
interference problems like ringing on the line, signal overshoot and
undershoot, extended signal settling times, crosstalk noise, transmission line
reflections, false signal detection by the receiving device and electromagnetic
interference (EMI) -- all of which can negate the potential gains designers are
seeking when they try to increase system speeds through the use of higher
performance logic devices. The fact is, faster signaling edge rates can cause
a higher level of electrical noise or other type of interference that can
actually lead to slower line speeds and lower maximum system frequencies.

Microsemi PHY have the provision to configure the edge rate. Edge-rate function
program the right value based on Device Tree configuration.

Tested on Beaglebone Black with VSC 8531 PHY.

Patch 2/2:

This is MAC interface feature.
Microsemi PHY can support RGMII, RMII or GMII/MII interface between MAC and PHY.
MAC-IF function program the right value based on Device tree configuration.

Hi Raju

In future, please start a new thread for a new version of the
patchset.

	Thanks
		Andrew

On Thu, Sep 08, 2016 at 02:47:21PM +0530, Raju Lakkaraju wrote:

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the Edge-rate as per review comments and
re-sending code for review

Signed-off-by: Raju Lakkaraju <redacted>
---
 drivers/net/phy/mscc.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++

Hi Raju

You need to also document the new property in the device tree binding
documentation.

+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+				      u8     edge_rate)

No spaces place.

+#ifdef CONFIG_OF_MDIO
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+	int rc;
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+
+	if (!of_node)
+		return -ENODEV;
+
+	rc = of_property_read_u8(of_node, "vsc8531,edge-rate",
+				 &vsc8531->edge_rate);

Until you have written the Documentation, it is hard for me to tell,
but device tree bindings should use real units, like seconds, Ohms,
Farads, etc. Is the edge rate in nS? Or is it some magic value which
just gets written into the register?

+
+	return rc;
+}
+#else
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
 	int rc;
+	struct vsc8531_private *vsc8531;
+
+	if (!phydev->priv) {

How can this happen?

+		vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531),
+				       GFP_KERNEL);
+		if (!vsc8531)
+			return -ENOMEM;
+
+		phydev->priv = vsc8531;
+		rc = vsc8531_of_init(phydev);
+		if (rc)
+			return rc;
+	} else {
+		vsc8531 = (struct vsc8531_private *)phydev->priv;
+	}
 
 	rc = vsc85xx_default_config(phydev);
 	if (rc)
 		return rc;
+
+	rc = vsc85xx_edge_rate_cntl_set(phydev, vsc8531->edge_rate);

If there is no vsc8531,edge-rate property in device tree, is the phy
going to work O.K, if you configure it for 0nS edges? Or should there
be some default value assigned?

Thanks
	Andrew

On Thu, Sep 08, 2016 at 02:47:22PM +0530, Raju Lakkaraju wrote:

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the MAC Interface as per review comments and
re-sending code for review

I don't see anything about device tree in this patch...

Do you need to wait for the reset to complete?

Does it make sense to call genphy_soft_reset() which will poll the phy
waiting for the BMCR_RESET bit to clear?

+
+static int vsc85xx_mac_if_set(struct phy_device *phydev,
+			      phy_interface_t   interface)
+{
+	int rc;
+	u16 reg_val;
+
+	mutex_lock(&phydev->lock);
+	reg_val = phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_1);
+	reg_val &= ~(MAC_IF_SELECTION_MASK);
+	switch (interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		reg_val |= (MAC_IF_SELECTION_RGMII << MAC_IF_SELECTION_POS);
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		reg_val |= (MAC_IF_SELECTION_RMII << MAC_IF_SELECTION_POS);
+		break;
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_GMII:
+	default:

So somebody asks you to configure the phy as PHY_INTERFACE_MODE_NA or
PHY_INTERFACE_MODE_TBI, you are going to use GMII. Maybe returning
-EINVAL would be better?

	Andrew

Hi Andrew,

Thank you for review the code and valuable comments.

On Thu, Sep 08, 2016 at 03:27:27PM +0200, Andrew Lunn wrote:

EXTERNAL EMAIL


On Thu, Sep 08, 2016 at 02:47:22PM +0530, Raju Lakkaraju wrote:

quoted

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the MAC Interface as per review comments and
re-sending code for review

I don't see anything about device tree in this patch...

Ethernet driver (in my BBB environment, TI cpsw driver) read the device tree 
phy interface parameter and update in phydev structure.

In device tree the following code holds the phy interface configuration.
&cpsw_emac0 {
        phy_id = <&davinci_mdio>, <0>;
        phy-mode = "rgmii";
};

I tested with different modes by changing device tree parameter (i.e. rmii/rgmii/mii).
I have used this parameter to configure the MAC interface.

quoted

Signed-off-by: Raju Lakkaraju <redacted>
---
 drivers/net/phy/mscc.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index f0a0e8d..dfbf4f3 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c

@@ -24,6 +24,16 @@ enum rgmii_rx_clock_delay {
      RGMII_RX_CLK_DELAY_3_4_NS = 7
 };

+/* Microsemi VSC85xx PHY registers */
+/* IEEE 802. Std Registers */
+#define MSCC_PHY_EXT_PHY_CNTL_1           23
+#define MAC_IF_SELECTION_MASK             0x1800
+#define MAC_IF_SELECTION_GMII             0
+#define MAC_IF_SELECTION_RMII             1
+#define MAC_IF_SELECTION_RGMII            2
+#define MAC_IF_SELECTION_POS              11
+#define FAR_END_LOOPBACK_MODE_MASK        0x0008
+
 #define MII_VSC85XX_INT_MASK           25
 #define MII_VSC85XX_INT_MASK_MASK      0xa000
 #define MII_VSC85XX_INT_STATUS                 26

@@ -59,6 +69,52 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
      return rc;
 }

+static int vsc85xx_soft_reset(struct phy_device *phydev)
+{
+     int rc;
+     u16 reg_val;
+
+     reg_val = phy_read(phydev, MII_BMCR);
+     reg_val |= BMCR_RESET;
+     rc = phy_write(phydev, MII_BMCR, reg_val);
+
+     return rc;
+}

Do you need to wait for the reset to complete?

Does it make sense to call genphy_soft_reset() which will poll the phy
waiting for the BMCR_RESET bit to clear?

I accepted your review comment.
I can use genphy_soft_reset( ) instead of creating another same function.

quoted

+
+static int vsc85xx_mac_if_set(struct phy_device *phydev,
+                           phy_interface_t   interface)
+{
+     int rc;
+     u16 reg_val;
+
+     mutex_lock(&phydev->lock);
+     reg_val = phy_read(phydev, MSCC_PHY_EXT_PHY_CNTL_1);
+     reg_val &= ~(MAC_IF_SELECTION_MASK);
+     switch (interface) {
+     case PHY_INTERFACE_MODE_RGMII:
+             reg_val |= (MAC_IF_SELECTION_RGMII << MAC_IF_SELECTION_POS);
+             break;
+     case PHY_INTERFACE_MODE_RMII:
+             reg_val |= (MAC_IF_SELECTION_RMII << MAC_IF_SELECTION_POS);
+             break;
+     case PHY_INTERFACE_MODE_MII:
+     case PHY_INTERFACE_MODE_GMII:
+     default:

So somebody asks you to configure the phy as PHY_INTERFACE_MODE_NA or
PHY_INTERFACE_MODE_TBI, you are going to use GMII. Maybe returning
-EINVAL would be better?

Microsemi PHY can support only 3 modes (RGMII/RMII/GMII). Default configuration should be GMII
in PHY hardware.
I accepted your review comment. 
In default switch case i will return -EINVAL.

        Andrew

Thanks,
Raju.

Hi Andrew,

Thank you for review the code and valuable comments.

On Thu, Sep 08, 2016 at 03:14:15PM +0200, Andrew Lunn wrote:

EXTERNAL EMAIL


On Thu, Sep 08, 2016 at 02:47:21PM +0530, Raju Lakkaraju wrote:

quoted

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the Edge-rate as per review comments and
re-sending code for review

Signed-off-by: Raju Lakkaraju <redacted>
---
 drivers/net/phy/mscc.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++

Hi Raju

You need to also document the new property in the device tree binding
documentation.

Sure. I will do.
I created device tree binding header file. i will submit in different patch.

quoted

+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+                                   u8     edge_rate)

No spaces place.

I ran the checkpatch. I did not find any error. I created another workspace and
applied the same patch. It shows the correct alignement. I have used tabs (8 space width).
then some spaces to align braces.

quoted

+#ifdef CONFIG_OF_MDIO
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+     int rc;
+     struct vsc8531_private *vsc8531 = phydev->priv;
+     struct device *dev = &phydev->mdio.dev;
+     struct device_node *of_node = dev->of_node;
+
+     if (!of_node)
+             return -ENODEV;
+
+     rc = of_property_read_u8(of_node, "vsc8531,edge-rate",
+                              &vsc8531->edge_rate);

Until you have written the Documentation, it is hard for me to tell,
but device tree bindings should use real units, like seconds, Ohms,
Farads, etc. Is the edge rate in nS? Or is it some magic value which
just gets written into the register?

This is some magic value which just gets written into the register.

In device tree file, defined in davinci_mdio structure:

        vsc8531_0: ethernet-phy@0 {
                compatible = "ethernet-phy-id0007.0570";
                reg = <0>;
                vsc8531,edge-rate = /bits/ 8 <MSCC_EDGE_RATE_CNTL_FASTEST>;
        };

In device tree binding header file, MACRO has defined as
i.e. include/dt-bindings/net/mscc-vsc8531.h

/* MAC interface Edge rate control pad */
#define MSCC_EDGE_RATE_CNTL_SLOWEST      0x0
#define MSCC_EDGE_RATE_CNTL_PLUS_1       0x1
#define MSCC_EDGE_RATE_CNTL_PLUS_2       0x2
#define MSCC_EDGE_RATE_CNTL_PLUS_3       0x3
#define MSCC_EDGE_RATE_CNTL_PLUS_4       0x4
#define MSCC_EDGE_RATE_CNTL_PLUS_5       0x5
#define MSCC_EDGE_RATE_CNTL_PLUS_6       0x6
#define MSCC_EDGE_RATE_CNTL_FASTEST      0x7

quoted

+
+     return rc;
+}
+#else
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+     return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
      int rc;
+     struct vsc8531_private *vsc8531;
+
+     if (!phydev->priv) {

How can this happen?

VSC 8531 driver don't have any private structure assigned initially.
Allways priv points to NULL. 
Allocate vsc8531 private structure and initialize by calling vsc8531_of_init( )
function.

quoted

+             vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531),
+                                    GFP_KERNEL);
+             if (!vsc8531)
+                     return -ENOMEM;
+
+             phydev->priv = vsc8531;
+             rc = vsc8531_of_init(phydev);
+             if (rc)
+                     return rc;
+     } else {
+             vsc8531 = (struct vsc8531_private *)phydev->priv;
+     }

      rc = vsc85xx_default_config(phydev);
      if (rc)
              return rc;
+
+     rc = vsc85xx_edge_rate_cntl_set(phydev, vsc8531->edge_rate);

If there is no vsc8531,edge-rate property in device tree, is the phy
going to work O.K, if you configure it for 0nS edges? Or should there
be some default value assigned?

Yes. Default values configured as Fast Edge rate control (i.e.0b111).
Edge rate control has defined 3 bits (Bit 7:5) in register.
Hardware default value is 3 (i.e. 0b111)

Thanks
        Andrew

Thanks
Raju.

On Fri, Sep 09, 2016 at 11:23:52AM +0530, Raju Lakkaraju wrote:

Hi Andrew,

Thank you for review the code and valuable comments.

On Thu, Sep 08, 2016 at 03:27:27PM +0200, Andrew Lunn wrote:

quoted

EXTERNAL EMAIL


On Thu, Sep 08, 2016 at 02:47:22PM +0530, Raju Lakkaraju wrote:

quoted

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the MAC Interface as per review comments and
re-sending code for review

I don't see anything about device tree in this patch...

Ethernet driver (in my BBB environment, TI cpsw driver) read the device tree 
phy interface parameter and update in phydev structure.

In device tree the following code holds the phy interface configuration.
&cpsw_emac0 {
        phy_id = <&davinci_mdio>, <0>;
        phy-mode = "rgmii";
};

O.K, that is one place it can come from. But it is not the only,
e.g. platform data or ACPI. A better comment might be:

Configure the MAC/PHY interface as indicated in phydev->interface,
eg. GMII, RMII, RGMII.

    Andrew

quoted

quoted

+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+                                   u8     edge_rate)

No spaces place.

I ran the checkpatch. I did not find any error. I created another workspace and
applied the same patch. It shows the correct alignement. I have used tabs (8 space width).
then some spaces to align braces.

Sorry, i worded that poorly. I was meaning between the u8 and edge. A
single space is enough.

quoted

quoted

+#ifdef CONFIG_OF_MDIO
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+     int rc;
+     struct vsc8531_private *vsc8531 = phydev->priv;
+     struct device *dev = &phydev->mdio.dev;
+     struct device_node *of_node = dev->of_node;
+
+     if (!of_node)
+             return -ENODEV;
+
+     rc = of_property_read_u8(of_node, "vsc8531,edge-rate",
+                              &vsc8531->edge_rate);

Until you have written the Documentation, it is hard for me to tell,
but device tree bindings should use real units, like seconds, Ohms,
Farads, etc. Is the edge rate in nS? Or is it some magic value which
just gets written into the register?

This is some magic value which just gets written into the register.

Magic values are generally not accepted in device tree bindings. Both
Micrel and Renesas define their clock skew in ps, for example. Since
this is rise time, it should also be possible to define it in a unit
of time.

quoted

quoted

 static int vsc85xx_config_init(struct phy_device *phydev)
 {
      int rc;
+     struct vsc8531_private *vsc8531;
+
+     if (!phydev->priv) {

How can this happen?

VSC 8531 driver don't have any private structure assigned initially.
Allways priv points to NULL. 

So if it cannot happen, don't check for it.

Also, by convention, you allocate memory in the .probe() function of a
driver. Please do it there.

	Andrew

Hi Andrew,

Thank you for review the code.

On Fri, Sep 09, 2016 at 03:18:32PM +0200, Andrew Lunn wrote:

EXTERNAL EMAIL

quoted

quoted

quoted

+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+                                   u8     edge_rate)

No spaces place.

I ran the checkpatch. I did not find any error. I created another workspace and
applied the same patch. It shows the correct alignement. I have used tabs (8 space width).
then some spaces to align braces.

Sorry, i worded that poorly. I was meaning between the u8 and edge. A
single space is enough.

I accepted your suggestion.

quoted

quoted

quoted

+#ifdef CONFIG_OF_MDIO
+static int vsc8531_of_init(struct phy_device *phydev)
+{
+     int rc;
+     struct vsc8531_private *vsc8531 = phydev->priv;
+     struct device *dev = &phydev->mdio.dev;
+     struct device_node *of_node = dev->of_node;
+
+     if (!of_node)
+             return -ENODEV;
+
+     rc = of_property_read_u8(of_node, "vsc8531,edge-rate",
+                              &vsc8531->edge_rate);

Until you have written the Documentation, it is hard for me to tell,
but device tree bindings should use real units, like seconds, Ohms,
Farads, etc. Is the edge rate in nS? Or is it some magic value which
just gets written into the register?

This is some magic value which just gets written into the register.

Magic values are generally not accepted in device tree bindings. Both
Micrel and Renesas define their clock skew in ps, for example. Since
this is rise time, it should also be possible to define it in a unit
of time.

I accepted your comment. I had discussion with my hardware team and explained
the code review comments.
They asked me to define as picoseconds as units.

quoted

quoted

quoted

 static int vsc85xx_config_init(struct phy_device *phydev)
 {
      int rc;
+     struct vsc8531_private *vsc8531;
+
+     if (!phydev->priv) {

How can this happen?

VSC 8531 driver don't have any private structure assigned initially.
Allways priv points to NULL.

So if it cannot happen, don't check for it.

Also, by convention, you allocate memory in the .probe() function of a
driver. Please do it there.

I accepted your review comment. 
I will re-send the patch with updates.

        Andrew

---
Thanks,
Raju.

Hi Andrew,

Thank you for review the code.
I accepted all your review comments.
I will send the update patch for review again.

Thanks,
Raju.
On Fri, Sep 09, 2016 at 02:03:46PM +0200, Andrew Lunn wrote:

EXTERNAL EMAIL


On Fri, Sep 09, 2016 at 11:23:52AM +0530, Raju Lakkaraju wrote:

quoted

Hi Andrew,

Thank you for review the code and valuable comments.

On Thu, Sep 08, 2016 at 03:27:27PM +0200, Andrew Lunn wrote:

quoted

EXTERNAL EMAIL


On Thu, Sep 08, 2016 at 02:47:22PM +0530, Raju Lakkaraju wrote:

quoted

From: Raju Lakkaraju <redacted>

Used Device Tree to configure the MAC Interface as per review comments and
re-sending code for review

I don't see anything about device tree in this patch...

Ethernet driver (in my BBB environment, TI cpsw driver) read the device tree
phy interface parameter and update in phydev structure.

In device tree the following code holds the phy interface configuration.
&cpsw_emac0 {
        phy_id = <&davinci_mdio>, <0>;
        phy-mode = "rgmii";
};

O.K, that is one place it can come from. But it is not the only,
e.g. platform data or ACPI. A better comment might be:

Configure the MAC/PHY interface as indicated in phydev->interface,
eg. GMII, RMII, RGMII.

    Andrew