Microsemi VSC 8531/41 PHY Driver

Hello All,

I would like to introduce myself as Nagaraju Lakkaraju (Raju), is working in Microsemi Communications India Pvt. Ltd (Formerly known as Vitesse Semiconductors Limited) - Hyderabad as Sr. Staff Engineer.
I do work on Microsemi PHY drivers development.
Microsemi is developing the new Physical Layer (PHY) chips for Internet Of Things (IoT) products with 1 Gbps link speed. 
As part of the development, Microsemi would like to contribute the new PHYs (i.e. VSC 8531 / VSC 8541) chip drivers in Linux Kernel Open source.
VSC 8531 / 8541 PHYs will have the following features a part of the basic features like Auto-neg, Speed, Duplex etc.
1. Wake on LAN 
2. Auto MDIX/MDI 
3. Link Speed Down shift
4. Fast Link Failure-2
5. Loopbacks (FAR-END, NEAR-END and Connector)
6. Ethernet Packet Generator (EPG)
7. Serial Management Interface (SMI) Interrupt
8. Clock Squelch configuration (SyncE)
9. Jumbo Frame Support
10. In-line Power On Ethernet (PoE) status
11. Acti PHY power Management 
12. Energy Efficiency Ethenet (EEE)
13. VeriPHY (Cable Diagnostics)
14. LED configuration
15. Ring Resiliency
16. Start Of Frame Detection (SOF)
17. COMA mode 

As part of Initial submission of the Linux Kernel Open source drivers,
I developed the VSC 8531 driver basic features and built the Linux Kernel image for Beaglebone Black hardware.
Also developed Ethtool enhancement for VSC 8531 register access functionality to test the VSC 8531 

Test Setup:
-------------
Hardware Details: Beaglebone Black with VSC 8531 PHY
Software Linux Kernel version: 4.6.4

Microsemi VSC 8531 chip is mount on Beaglebone Black hardware (replaced the Microchip PHY) and tested the following features.
1. Auto negotiation
2. Speed change ( 10 Mbps, 100 Mbps and 1 Gbps)
3. Full/Half Duplex
4. Ping 
5. Line rate traffic with Test center

I would like you to review my code and provide me the valuable comments.
Please find the attached git code diff patch.

Thanks and regards,
Raju
(Nagaraju Lakkaraju)
Sr. Staff Engg.
Microsemi Communications India Pvt Ltd.
Ph: +91 040  6686 0132

On Tue, Jul 26, 2016 at 09:49:53AM +0000, Raju Lakkaraju wrote:

Hello All,

I would like to introduce myself as Nagaraju Lakkaraju (Raju), is working in Microsemi Communications India Pvt. Ltd (Formerly known as Vitesse Semiconductors Limited) - Hyderabad as Sr. Staff Engineer.
I do work on Microsemi PHY drivers development.
Microsemi is developing the new Physical Layer (PHY) chips for Internet Of Things (IoT) products with 1 Gbps link speed. 
As part of the development, Microsemi would like to contribute the new PHYs (i.e. VSC 8531 / VSC 8541) chip drivers in Linux Kernel Open source.
VSC 8531 / 8541 PHYs will have the following features a part of the basic features like Auto-neg, Speed, Duplex etc.
1. Wake on LAN 
2. Auto MDIX/MDI 
3. Link Speed Down shift
4. Fast Link Failure-2
5. Loopbacks (FAR-END, NEAR-END and Connector)
6. Ethernet Packet Generator (EPG)
7. Serial Management Interface (SMI) Interrupt
8. Clock Squelch configuration (SyncE)
9. Jumbo Frame Support
10. In-line Power On Ethernet (PoE) status
11. Acti PHY power Management 
12. Energy Efficiency Ethenet (EEE)
13. VeriPHY (Cable Diagnostics)
14. LED configuration
15. Ring Resiliency
16. Start Of Frame Detection (SOF)
17. COMA mode 

Some interesting features. Is the datasheet publicly available?
 

As part of Initial submission of the Linux Kernel Open source drivers,
I developed the VSC 8531 driver basic features and built the Linux Kernel image for Beaglebone Black hardware.
Also developed Ethtool enhancement for VSC 8531 register access functionality to test the VSC 8531 

Test Setup:
-------------
Hardware Details: Beaglebone Black with VSC 8531 PHY
Software Linux Kernel version: 4.6.4

Microsemi VSC 8531 chip is mount on Beaglebone Black hardware (replaced the Microchip PHY) and tested the following features.
1. Auto negotiation
2. Speed change ( 10 Mbps, 100 Mbps and 1 Gbps)
3. Full/Half Duplex
4. Ping 
5. Line rate traffic with Test center

I would like you to review my code and provide me the valuable comments.
Please find the attached git code diff patch.

I would like to review your code, but please read the SubmittingPathes
document and in particularly, the bit about attachments.

	 Andrew

Hello Andrew,

VSC 8531 / 8541 is not yet release to Public. Now It's in draft version.
After commit the VSC 8531 PHY drivers in Linux Kernel Open source, Microsemi will release the VSC 8531/8541 data sheet to Public.
My apologies. I missed instructions about attachments in SubmittingPathes document.

VSC 8531/8541 PHY Driver code patch inline.

From d75dce082d229595211e8e7106a19c967e5ce07f Mon Sep 17 00:00:00 2001

From: "Nagaraju Lakkaraju" <redacted>
Date: Thu, 21 Jul 2016 12:47:54 +0200
Subject: [PATCH] net: phy: Add drivers for Microsemi PHYs

Adding support for Microsemi PHYs: VSC8531 and VSC8541.

Signed-off-by: Nagaraju Lakkaraju <redacted>
---
 drivers/net/phy/Kconfig  |   5 ++
 drivers/net/phy/Makefile |   1 +
 drivers/net/phy/mscc.c   | 183 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)
 create mode 100644 drivers/net/phy/mscc.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 6dad9a9..9d1bd0e 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig

@@ -271,6 +271,11 @@ config MDIO_BCM_IPROC
 	  This module provides a driver for the MDIO busses found in the
 	  Broadcom iProc SoC's.
 
+config MICROSEMI_PHY
+        tristate "Drivers for the Microsemi PHYs"
+        ---help---
+          Currently supports the VSC8531 and VSC8541 PHYs
+
 endif # PHYLIB
 
 config MICREL_KS8995MA

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index fcdbb92..f98ac90 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile

@@ -10,6 +10,7 @@ obj-$(CONFIG_CICADA_PHY)	+= cicada.o
 obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
 obj-$(CONFIG_SMSC_PHY)		+= smsc.o
+obj-$(CONFIG_MICROSEMI_PHY)     += mscc.o
 obj-$(CONFIG_TERANETICS_PHY)	+= teranetics.o
 obj-$(CONFIG_VITESSE_PHY)	+= vitesse.o
 obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
new file mode 100644
index 0000000..605e91c
--- /dev/null
+++ b/drivers/net/phy/mscc.c

@@ -0,0 +1,183 @@
+/*
+ * Driver for Microsemi VSC85xx PHYs
+ *
+ * Author: Nagaraju Lakkaraju
+ * License: Dual MIT/GPL
+ * Copyright (c) 2016 Microsemi Corporation
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mdio.h>
+#include <linux/mii.h>
+#include <linux/phy.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 registers - 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
+
+/* RGMII Rx Clock delay value change with board lay-out */
+static u8 rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;
+
+static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
+{
+	int rc = 0;
+
+	rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
+	return rc;
+}
+
+static int vsc85xx_default_config(struct phy_device *phydev)
+{
+	int rc = 0;
+	u16 reg_val = 0;
+
+	phydev->supported = (SUPPORTED_1000baseT_Full |
+			     SUPPORTED_1000baseT_Half |
+			     SUPPORTED_100baseT_Full  |
+			     SUPPORTED_100baseT_Half  |
+			     SUPPORTED_10baseT_Full   |
+			     SUPPORTED_10baseT_Half   |
+			     SUPPORTED_Autoneg        |
+			     SUPPORTED_Pause          |
+			     SUPPORTED_Asym_Pause     |
+			     SUPPORTED_TP);
+
+	phydev->speed = SPEED_1000;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+	phydev->interface = PHY_INTERFACE_MODE_RGMII;
+	phydev->mdix = ETH_TP_MDI_AUTO;
+
+	mutex_lock(&phydev->lock);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc != 0) {
+		rc = -EINVAL;
+		goto out_unlock;
+	}
+	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
+	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
+	reg_val |= (rgmii_rx_clk_delay << RGMII_RX_CLK_DELAY_POS);
+	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+	if (rc != 0)
+		rc = -EINVAL;
+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
+static int vsc85xx_config_init(struct phy_device *phydev)
+{
+	int rc = 0;
+
+	rc = vsc85xx_default_config(phydev);
+	rc = genphy_config_init(phydev);
+
+	return rc;
+}
+
+static int vsc85xx_ack_interrupt(struct phy_device *phydev)
+{
+	int rc = 0;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+	return (rc < 0) ? rc : 0;
+}
+
+static int vsc85xx_config_intr(struct phy_device *phydev)
+{
+	int rc;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+			       MII_VSC85XX_INT_MASK_MASK);
+	} else {
+		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+		if (rc < 0)
+			return rc;
+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
+	}
+
+	return rc;
+}
+
+/* Microsemi VSC85xx PHYs */
+static struct phy_driver vsc85xx_driver[] = {
+{
+	.phy_id         = PHY_ID_VSC8531,
+	.name           = "Microsemi VSC8531",
+	.phy_id_mask    = 0xfffffff0,
+	.features       = PHY_GBIT_FEATURES,
+	.flags          = PHY_HAS_INTERRUPT,
+	.soft_reset     = &genphy_soft_reset,
+	.config_init    = &vsc85xx_config_init,
+	.config_aneg    = &genphy_config_aneg,
+	.aneg_done      = &genphy_aneg_done,
+	.read_status    = &genphy_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.suspend        = &genphy_suspend,
+	.resume         = &genphy_resume,
+},
+{
+	.phy_id         = PHY_ID_VSC8541,
+	.name           = "Microsemi VSC8541 SyncE",
+	.phy_id_mask    = 0xfffffff0,
+	.features       = PHY_GBIT_FEATURES,
+	.flags          = PHY_HAS_INTERRUPT,
+	.soft_reset     = &genphy_soft_reset,
+	.config_init    = &vsc85xx_config_init,
+	.config_aneg    = &genphy_config_aneg,
+	.aneg_done      = &genphy_aneg_done,
+	.read_status    = &genphy_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.suspend        = &genphy_suspend,
+	.resume         = &genphy_resume,
+}
+
+};
+
+module_phy_driver(vsc85xx_driver);
+
+static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
+	{ PHY_ID_VSC8531, 0xfffffff0, },
+	{ PHY_ID_VSC8541, 0xfffffff0, },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, vsc85xx_tbl);
+
+MODULE_DESCRIPTION("Microsemi VSC85xx PHY driver");
+MODULE_AUTHOR("Nagaraju Lakkaraju");
+MODULE_LICENSE("Dual MIT/GPL");

-- 
2.7.3


Thanks and regards,
Raju
(Nagaraju Lakkaraju)
Sr. Staff Engg.
Microsemi Communications India Pvt Ltd.
Ph: +91 040 6686 0132

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch] 
Sent: Tuesday, July 26, 2016 5:26 PM
To: Raju Lakkaraju
Cc: netdev@vger.kernel.org; f.fainelli@gmail.com; Allan Nielsen
Subject: Re: Microsemi VSC 8531/41 PHY Driver

EXTERNAL EMAIL


On Tue, Jul 26, 2016 at 09:49:53AM +0000, Raju Lakkaraju wrote:
> Hello All,
>
> I would like to introduce myself as Nagaraju Lakkaraju (Raju), is working in Microsemi Communications India Pvt. Ltd (Formerly known as Vitesse Semiconductors Limited) - Hyderabad as Sr. Staff Engineer.
> I do work on Microsemi PHY drivers development.
> Microsemi is developing the new Physical Layer (PHY) chips for Internet Of Things (IoT) products with 1 Gbps link speed.
> As part of the development, Microsemi would like to contribute the new PHYs (i.e. VSC 8531 / VSC 8541) chip drivers in Linux Kernel Open source.
> VSC 8531 / 8541 PHYs will have the following features a part of the basic features like Auto-neg, Speed, Duplex etc.
> 1. Wake on LAN
> 2. Auto MDIX/MDI
> 3. Link Speed Down shift
> 4. Fast Link Failure-2
> 5. Loopbacks (FAR-END, NEAR-END and Connector) 6. Ethernet Packet 
> Generator (EPG) 7. Serial Management Interface (SMI) Interrupt 8. 
> Clock Squelch configuration (SyncE) 9. Jumbo Frame Support 10. In-line 
> Power On Ethernet (PoE) status 11. Acti PHY power Management 12. 
> Energy Efficiency Ethenet (EEE) 13. VeriPHY (Cable Diagnostics) 14. 
> LED configuration 15. Ring Resiliency 16. Start Of Frame Detection 
> (SOF) 17. COMA mode

Some interesting features. Is the datasheet publicly available?

> As part of Initial submission of the Linux Kernel Open source drivers, 
> I developed the VSC 8531 driver basic features and built the Linux Kernel image for Beaglebone Black hardware.
> Also developed Ethtool enhancement for VSC 8531 register access 
> functionality to test the VSC 8531
>
> Test Setup:
> -------------
> Hardware Details: Beaglebone Black with VSC 8531 PHY Software Linux 
> Kernel version: 4.6.4
>
> Microsemi VSC 8531 chip is mount on Beaglebone Black hardware (replaced the Microchip PHY) and tested the following features.
> 1. Auto negotiation
> 2. Speed change ( 10 Mbps, 100 Mbps and 1 Gbps) 3. Full/Half Duplex 4. 
> Ping 5. Line rate traffic with Test center
>
> I would like you to review my code and provide me the valuable comments.
> Please find the attached git code diff patch.

I would like to review your code, but please read the SubmittingPathes document and in particularly, the bit about attachments.

         Andrew

+/* RGMII Rx Clock delay value change with board lay-out */
+static u8 rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different
layouts? You should be getting this value from the device tree.

+	phydev->supported = (SUPPORTED_1000baseT_Full |
+			     SUPPORTED_1000baseT_Half |
+			     SUPPORTED_100baseT_Full  |
+			     SUPPORTED_100baseT_Half  |
+			     SUPPORTED_10baseT_Full   |
+			     SUPPORTED_10baseT_Half   |
+			     SUPPORTED_Autoneg        |
+			     SUPPORTED_Pause          |
+			     SUPPORTED_Asym_Pause     |
+			     SUPPORTED_TP);
+
+	phydev->speed = SPEED_1000;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+	phydev->interface = PHY_INTERFACE_MODE_RGMII;
+	phydev->mdix = ETH_TP_MDI_AUTO;

Why are you setting all these? This is not normal, if you look at
other drivers.

+
+	mutex_lock(&phydev->lock);

What are you locking against?

+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+	if (rc != 0) {
+		rc = -EINVAL;

Why do you overwrite the error code vsc85xx_phy_page_set gives you?

+		goto out_unlock;
+	}
+	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
+	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
+	reg_val |= (rgmii_rx_clk_delay << RGMII_RX_CLK_DELAY_POS);
+	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+	if (rc != 0)
+		rc = -EINVAL;

Same here.

+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
+static int vsc85xx_config_init(struct phy_device *phydev)
+{
+	int rc = 0;

No need to initialise rc.

+	rc = vsc85xx_default_config(phydev);

  	if (rc)
    		return rc;

+	rc = genphy_config_init(phydev);
+
+	return rc;

Or just 
	return genphy_config_init(phydev);

+}
+
+static int vsc85xx_ack_interrupt(struct phy_device *phydev)
+{
+	int rc = 0;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+	return (rc < 0) ? rc : 0;
+}
+
+static int vsc85xx_config_intr(struct phy_device *phydev)
+{
+	int rc;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+			       MII_VSC85XX_INT_MASK_MASK);
+	} else {
+		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+		if (rc < 0)
+			return rc;

And the purpose of this read is? I assume it clears an outstanding
interrupt? If so, shouldn't you do it after disabling interrupts, not
before? Otherwise you have a race condition.

+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
+	}
+
+	return rc;

  Andrew

Hello Andrew,

Thank you for given valuable comments.
Please see the my responses inline.

Thanks,
Raju

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch] 
Sent: Tuesday, July 26, 2016 6:14 PM
To: Raju Lakkaraju
Cc: netdev@vger.kernel.org; f.fainelli@gmail.com; Allan Nielsen
Subject: Re: Microsemi VSC 8531/41 PHY Driver

EXTERNAL EMAIL

+/* RGMII Rx Clock delay value change with board lay-out */ static u8 
+rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.

Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value. 
We tested on Beaglebone Black with VSC 8531 PHY.
We would like to provide new function to configure correct/require value based on PHY layouts 
alone with other RGMII configuration parameters as part of our next implementation.

+     phydev->supported = (SUPPORTED_1000baseT_Full |
+                          SUPPORTED_1000baseT_Half |
+                          SUPPORTED_100baseT_Full  |
+                          SUPPORTED_100baseT_Half  |
+                          SUPPORTED_10baseT_Full   |
+                          SUPPORTED_10baseT_Half   |
+                          SUPPORTED_Autoneg        |
+                          SUPPORTED_Pause          |
+                          SUPPORTED_Asym_Pause     |
+                          SUPPORTED_TP);
+
+     phydev->speed = SPEED_1000;
+     phydev->duplex = DUPLEX_FULL;
+     phydev->pause = 0;
+     phydev->asym_pause = 0;
+     phydev->interface = PHY_INTERFACE_MODE_RGMII;
+     phydev->mdix = ETH_TP_MDI_AUTO;

Why are you setting all these? This is not normal, if you look at other drivers.

Raju: I would like to update the default values in software data structure (phydev). 
Our PHY is 1G speed support device and RGMII supported device.

+
+     mutex_lock(&phydev->lock);

What are you locking against?

Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY control registers, 
first set the page number then read/write the register address. Default page should be Page 0.
When I want to access not default page register, I have to lock phy device access and change 
the page number and register access as atomic operation. 

+     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+     if (rc != 0) {
+             rc = -EINVAL;

Why do you overwrite the error code vsc85xx_phy_page_set gives you?

Raju: initially I would like to create new type of Error code. Then, I decided to use existing one. 
I accept your comment. I will remove the code.

+             goto out_unlock;
+     }
+     reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
+     reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
+     reg_val |= (rgmii_rx_clk_delay << RGMII_RX_CLK_DELAY_POS);
+     phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
+     rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+     if (rc != 0)
+             rc = -EINVAL;

Same here.

Raju: I accept your comment. I will remove the code.

+
+out_unlock:
+     mutex_unlock(&phydev->lock);
+
+     return rc;
+}
+
+static int vsc85xx_config_init(struct phy_device *phydev) {
+     int rc = 0;

No need to initialise rc.

Raju: I accept your comment. I will remove the code.

+     rc = vsc85xx_default_config(phydev);

        if (rc)
                return rc;

+     rc = genphy_config_init(phydev);
+
+     return rc;

Or just
        return genphy_config_init(phydev);

Raju: I accept your comment. I will remove the code.

+}
+
+static int vsc85xx_ack_interrupt(struct phy_device *phydev) {
+     int rc = 0;
+
+     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+     return (rc < 0) ? rc : 0;
+}
+
+static int vsc85xx_config_intr(struct phy_device *phydev) {
+     int rc;
+
+     if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+             rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+                            MII_VSC85XX_INT_MASK_MASK);
+     } else {
+             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+             if (rc < 0)
+                     return rc;

And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition.

Raju: The Interrupt status register is read on clean. When, PHY_INTERRUPT_DISABLE case, 
I should make sure that status should be clear. If I read the Interrupt status registers, it clears all preexisting interrupts.

+             rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
+     }
+
+     return rc;

  Andrew

On 07/27/2016 11:44 PM, Raju Lakkaraju wrote:

Hello Andrew,

Thank you for given valuable comments.
Please see the my responses inline.

Thanks,
Raju

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch] 
Sent: Tuesday, July 26, 2016 6:14 PM
To: Raju Lakkaraju
Cc: netdev@vger.kernel.org; f.fainelli@gmail.com; Allan Nielsen
Subject: Re: Microsemi VSC 8531/41 PHY Driver

EXTERNAL EMAIL

quoted

+/* RGMII Rx Clock delay value change with board lay-out */ static u8 
+rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.

Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value. 
We tested on Beaglebone Black with VSC 8531 PHY.

That is true, until the next design with a PHY that does not need this
value and then, it will have to be adjusted.

We would like to provide new function to configure correct/require value based on PHY layouts 
alone with other RGMII configuration parameters as part of our next implementation.

You can either introduce a Device Tree property to allow boards to
specify what the correct delay(s) should be, or if the platform does not
use Device Tree, using phy_register_fixup_for_id would be acceptable for
that.

quoted

+     phydev->supported = (SUPPORTED_1000baseT_Full |
+                          SUPPORTED_1000baseT_Half |
+                          SUPPORTED_100baseT_Full  |
+                          SUPPORTED_100baseT_Half  |
+                          SUPPORTED_10baseT_Full   |
+                          SUPPORTED_10baseT_Half   |
+                          SUPPORTED_Autoneg        |
+                          SUPPORTED_Pause          |
+                          SUPPORTED_Asym_Pause     |
+                          SUPPORTED_TP);

This is not necessary, your driver should advertise what the PHY is
capable of in phy_driver::features. The Ethernet MAC driver later should
be adjusting phydev->supported with what it actually support, there are
cases where you connect a 10/100Mbits MAC to a 1Gbits PHY, and you want
to properly restrict unsupported speeds.

quoted

+
+     phydev->speed = SPEED_1000;
+     phydev->duplex = DUPLEX_FULL;
+     phydev->pause = 0;
+     phydev->asym_pause = 0;
+     phydev->interface = PHY_INTERFACE_MODE_RGMII;
+     phydev->mdix = ETH_TP_MDI_AUTO;

Why are you setting all these? This is not normal, if you look at other drivers.

Raju: I would like to update the default values in software data structure (phydev). 
Our PHY is 1G speed support device and RGMII supported device.

Whether RGMII is used as an interface/connection type between the MAC
and PHY is something that is within the consumer of the PHYLIB API
(typically Ethernet MAC/Switch driver), your PHY cannot enforce
anything, but the driver can check that the connection interface is sensble.

All of these default values that you are setting here will need to be
potentially changed by the state machine (link, duplex, pause) upon
reaction to link state changes, this change needs to be dropped.

quoted

+
+     mutex_lock(&phydev->lock);

What are you locking against?

Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY control registers, 
first set the page number then read/write the register address. Default page should be Page 0.
When I want to access not default page register, I have to lock phy device access and change 
the page number and register access as atomic operation. 

Based on the execution context of this function, acquiring the mutex is
not necessary, the state machine has not started yet, so there cannot be
a conflicting PHY read which would end up changing the page selection.

[snip]

quoted

+
+static int vsc85xx_ack_interrupt(struct phy_device *phydev) {
+     int rc = 0;
+
+     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+     return (rc < 0) ? rc : 0;
+}
+
+static int vsc85xx_config_intr(struct phy_device *phydev) {
+     int rc;
+
+     if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+             rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+                            MII_VSC85XX_INT_MASK_MASK);
+     } else {
+             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+             if (rc < 0)
+                     return rc;

And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition.

Raju: The Interrupt status register is read on clean. When, PHY_INTERRUPT_DISABLE case, 
I should make sure that status should be clear. If I read the Interrupt status registers, it clears all preexisting interrupts.

Should not you clear the interrupt status irrespective of whether PHY
interrupts will be enabled or not as a first operation? Then later on,
if the PHY needs to enable interrupt or not, this can be based on
PHY_INTERRUPT_ENABLED.
-- 
Florian

On Thu, Jul 28, 2016 at 06:44:37AM +0000, Raju Lakkaraju wrote:

Hello Andrew,

Thank you for given valuable comments.
Please see the my responses inline.

Thanks,
Raju

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch] 
Sent: Tuesday, July 26, 2016 6:14 PM
To: Raju Lakkaraju
Cc: netdev@vger.kernel.org; f.fainelli@gmail.com; Allan Nielsen
Subject: Re: Microsemi VSC 8531/41 PHY Driver

EXTERNAL EMAIL

quoted

+/* RGMII Rx Clock delay value change with board lay-out */ static u8 
+rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.

Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value. 
We tested on Beaglebone Black with VSC 8531 PHY.
We would like to provide new function to configure correct/require value based on PHY layouts 
alone with other RGMII configuration parameters as part of our next implementation.

Hi Raju

Please can you use standard email quoting, just like everybody else does.

       Andrew

quoted

+/* RGMII Rx Clock delay value change with board lay-out */ static u8 
+rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.

Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value. 
We tested on Beaglebone Black with VSC 8531 PHY.
We would like to provide new function to configure correct/require value based on PHY layouts 
alone with other RGMII configuration parameters as part of our next implementation.

Please either do it properly now or hard code it as the default, and
then later replace it with device tree, etc. We don't like to see half
finished features.

What are you locking against?

Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY control registers, 
first set the page number then read/write the register address. Default page should be Page 0.
When I want to access not default page register, I have to lock phy device access and change 
the page number and register access as atomic operation. 

I understand all that, which is why i asked, "what are you locking
against?", not "why are you locking?" What are the other call paths? I
don't see you taking this lock anywhere else? Should you be? I would
just like to see a comment which suggests you understand when this
lock is needed, and when not.

     Andrew

On Thu, Jul 28, 2016 at 10:35:05AM -0700, Florian Fainelli wrote:

EXTERNAL EMAIL


On 07/27/2016 11:44 PM, Raju Lakkaraju wrote:

quoted

Hello Andrew,

Thank you for given valuable comments.
Please see the my responses inline.

Thanks,
Raju

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Tuesday, July 26, 2016 6:14 PM
To: Raju Lakkaraju
Cc: netdev@vger.kernel.org; f.fainelli@gmail.com; Allan Nielsen
Subject: Re: Microsemi VSC 8531/41 PHY Driver

EXTERNAL EMAIL

quoted

+/* RGMII Rx Clock delay value change with board lay-out */ static u8
+rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.

Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value.
We tested on Beaglebone Black with VSC 8531 PHY.

That is true, until the next design with a PHY that does not need this
value and then, it will have to be adjusted.

I accepted your review comment. I do hard code it as the default value.

quoted

We would like to provide new function to configure correct/require value based on PHY layouts
alone with other RGMII configuration parameters as part of our next implementation.

You can either introduce a Device Tree property to allow boards to
specify what the correct delay(s) should be, or if the platform does not
use Device Tree, using phy_register_fixup_for_id would be acceptable for
that.

I do hard code this value as the default value.

quoted

quoted

+     phydev->supported = (SUPPORTED_1000baseT_Full |
+                          SUPPORTED_1000baseT_Half |
+                          SUPPORTED_100baseT_Full  |
+                          SUPPORTED_100baseT_Half  |
+                          SUPPORTED_10baseT_Full   |
+                          SUPPORTED_10baseT_Half   |
+                          SUPPORTED_Autoneg        |
+                          SUPPORTED_Pause          |
+                          SUPPORTED_Asym_Pause     |
+                          SUPPORTED_TP);

This is not necessary, your driver should advertise what the PHY is
capable of in phy_driver::features. The Ethernet MAC driver later should
be adjusting phydev->supported with what it actually support, there are
cases where you connect a 10/100Mbits MAC to a 1Gbits PHY, and you want
to properly restrict unsupported speeds.

I accepted your reveiw comment. I delete initialization.

quoted

quoted

+
+     phydev->speed = SPEED_1000;
+     phydev->duplex = DUPLEX_FULL;
+     phydev->pause = 0;
+     phydev->asym_pause = 0;
+     phydev->interface = PHY_INTERFACE_MODE_RGMII;
+     phydev->mdix = ETH_TP_MDI_AUTO;

Why are you setting all these? This is not normal, if you look at other drivers.

Raju: I would like to update the default values in software data structure (phydev).
Our PHY is 1G speed support device and RGMII supported device.

Whether RGMII is used as an interface/connection type between the MAC
and PHY is something that is within the consumer of the PHYLIB API
(typically Ethernet MAC/Switch driver), your PHY cannot enforce
anything, but the driver can check that the connection interface is sensble.

All of these default values that you are setting here will need to be
potentially changed by the state machine (link, duplex, pause) upon
reaction to link state changes, this change needs to be dropped.

I accepted your reveiw comment. I delete initialization.

quoted

quoted

+
+     mutex_lock(&phydev->lock);

What are you locking against?

Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY control registers,
first set the page number then read/write the register address. Default page should be Page 0.
When I want to access not default page register, I have to lock phy device access and change
the page number and register access as atomic operation.

Based on the execution context of this function, acquiring the mutex is
not necessary, the state machine has not started yet, so there cannot be
a conflicting PHY read which would end up changing the page selection.

[snip]

This is Read Modify Write (RMW) operation on register MSCC_PHY_RGMII_CNTL. This register in different page i.e. EXTENDED-2. I would like to execute all these operations in atomic operation.

quoted

quoted

+
+static int vsc85xx_ack_interrupt(struct phy_device *phydev) {
+     int rc = 0;
+
+     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+     return (rc < 0) ? rc : 0;
+}
+
+static int vsc85xx_config_intr(struct phy_device *phydev) {
+     int rc;
+
+     if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+             rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+                            MII_VSC85XX_INT_MASK_MASK);
+     } else {
+             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+             if (rc < 0)
+                     return rc;

And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition.

Raju: The Interrupt status register is read on clean. When, PHY_INTERRUPT_DISABLE case,
I should make sure that status should be clear. If I read the Interrupt status registers, it clears all preexisting interrupts.

Should not you clear the interrupt status irrespective of whether PHY
interrupts will be enabled or not as a first operation? Then later on,
if the PHY needs to enable interrupt or not, this can be based on
PHY_INTERRUPT_ENABLED.

In PHY_INTERRUPT_DISABLE case, after disable the interrupts, I would like to clear the interrupt status.

--
Florian

On Fri, Jul 29, 2016 at 10:17:36AM +0200, Andrew Lunn wrote:

EXTERNAL EMAIL

quoted

quoted

+/* RGMII Rx Clock delay value change with board lay-out */ static u8
+rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;

Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.

Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value.
We tested on Beaglebone Black with VSC 8531 PHY.
We would like to provide new function to configure correct/require value based on PHY layouts
alone with other RGMII configuration parameters as part of our next implementation.

Please either do it properly now or hard code it as the default, and
then later replace it with device tree, etc. We don't like to see half
finished features.

I accepted your review comment. I do hard code it as the default values.

quoted

What are you locking against?

Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY control registers,
first set the page number then read/write the register address. Default page should be Page 0.
When I want to access not default page register, I have to lock phy device access and change
the page number and register access as atomic operation.

I understand all that, which is why i asked, "what are you locking
against?", not "why are you locking?" What are the other call paths? I
don't see you taking this lock anywhere else? Should you be? I would
just like to see a comment which suggests you understand when this
lock is needed, and when not.

This is Read Modify Write (RMW) operation on register MSCC_PHY_RGMII_CNTL. This register in different page i.e. EXTENDED-2. I would like to execute all these operations in atomic.
I also use the mutex in different functions. But not in this patch.

     Andrew

Hello,

I added all review comments and re-sending for review.

From a5017f5878a92d2acec86a6a29b1498c457cb73a Mon Sep 17 00:00:00 2001

From: Nagaraju Lakkaraju <redacted>
Date: Wed, 3 Aug 2016 18:28:24 +0530
Subject: [PATCH v2] net: phy: Add drivers for Microsemi PHYs

Signed-off-by: Nagaraju Lakkaraju <redacted>
---
 drivers/net/phy/Kconfig  |   5 ++
 drivers/net/phy/Makefile |   1 +
 drivers/net/phy/mscc.c   | 161 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/net/phy/mscc.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 47a6434..1b534ea 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig

@@ -307,6 +307,11 @@ config MDIO_XGENE
          This module provides a driver for the MDIO busses found in the
          APM X-Gene SoC's.

+config MICROSEMI_PHY
+    tristate "Drivers for the Microsemi PHYs"
+    ---help---
+      Currently supports the VSC8531 and VSC8541 PHYs
+
 endif # PHYLIB

 config MICREL_KS8995MA

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 534dfa7..a713bd4 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile

@@ -11,6 +11,7 @@ obj-$(CONFIG_CICADA_PHY)      += cicada.o
 obj-$(CONFIG_LXT_PHY)          += lxt.o
 obj-$(CONFIG_QSEMI_PHY)                += qsemi.o
 obj-$(CONFIG_SMSC_PHY)         += smsc.o
+obj-$(CONFIG_MICROSEMI_PHY) += mscc.o
 obj-$(CONFIG_TERANETICS_PHY)   += teranetics.o
 obj-$(CONFIG_VITESSE_PHY)      += vitesse.o
 obj-$(CONFIG_BCM_NET_PHYLIB)   += bcm-phy-lib.o

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
new file mode 100644
index 0000000..49c7506
--- /dev/null
+++ b/drivers/net/phy/mscc.c

@@ -0,0 +1,161 @@
+/*
+ * Driver for Microsemi VSC85xx PHYs
+ *
+ * Author: Nagaraju Lakkaraju
+ * License: Dual MIT/GPL
+ * Copyright (c) 2016 Microsemi Corporation
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mdio.h>
+#include <linux/mii.h>
+#include <linux/phy.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
+
+static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
+{
+       int rc;
+
+       rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
+       return rc;
+}
+
+static int vsc85xx_default_config(struct phy_device *phydev)
+{
+       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_RGMII_CNTL);
+       reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
+       reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
+       phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
+       rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+       mutex_unlock(&phydev->lock);
+
+       return rc;
+}
+
+static int vsc85xx_config_init(struct phy_device *phydev)
+{
+       int rc;
+
+       rc = vsc85xx_default_config(phydev);
+       if (rc)
+               return rc;
+       rc = genphy_config_init(phydev);
+
+       return rc;
+}
+
+static int vsc85xx_ack_interrupt(struct phy_device *phydev)
+{
+       int rc;
+
+       if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+               rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+       return (rc < 0) ? rc : 0;
+}
+
+static int vsc85xx_config_intr(struct phy_device *phydev)
+{
+       int rc;
+
+       if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+               rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+                                  MII_VSC85XX_INT_MASK_MASK);
+       } else {
+               rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
+               if (rc < 0)
+                       return rc;
+               rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+       }
+
+       return rc;
+}
+
+/* Microsemi VSC85xx PHYs */
+static struct phy_driver vsc85xx_driver[] = {
+{
+       .phy_id                 = PHY_ID_VSC8531,
+       .name                   = "Microsemi VSC8531",
+       .phy_id_mask    = 0xfffffff0,
+       .features               = PHY_GBIT_FEATURES,
+       .flags                  = PHY_HAS_INTERRUPT,
+       .soft_reset             = &genphy_soft_reset,
+       .config_init    = &vsc85xx_config_init,
+       .config_aneg    = &genphy_config_aneg,
+       .aneg_done              = &genphy_aneg_done,
+       .read_status    = &genphy_read_status,
+       .ack_interrupt  = &vsc85xx_ack_interrupt,
+       .config_intr    = &vsc85xx_config_intr,
+       .suspend                = &genphy_suspend,
+       .resume                 = &genphy_resume,
+},
+{
+       .phy_id                 = PHY_ID_VSC8541,
+       .name                   = "Microsemi VSC8541 SyncE",
+       .phy_id_mask    = 0xfffffff0,
+       .features               = PHY_GBIT_FEATURES,
+       .flags                  = PHY_HAS_INTERRUPT,
+       .soft_reset             = &genphy_soft_reset,
+       .config_init    = &vsc85xx_config_init,
+       .config_aneg    = &genphy_config_aneg,
+       .aneg_done              = &genphy_aneg_done,
+       .read_status    = &genphy_read_status,
+       .ack_interrupt  = &vsc85xx_ack_interrupt,
+       .config_intr    = &vsc85xx_config_intr,
+       .suspend                = &genphy_suspend,
+       .resume                 = &genphy_resume,
+}
+
+};
+
+module_phy_driver(vsc85xx_driver);
+
+static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
+       { PHY_ID_VSC8531, 0xfffffff0, },
+       { PHY_ID_VSC8541, 0xfffffff0, },
+       { }
+};
+
+MODULE_DEVICE_TABLE(mdio, vsc85xx_tbl);
+
+MODULE_DESCRIPTION("Microsemi VSC85xx PHY driver");
+MODULE_AUTHOR("Nagaraju Lakkaraju");
+MODULE_LICENSE("Dual MIT/GPL");

-- 
2.1.0

--

Thanks,
Raju
On Fri, Jul 29, 2016 at 10:17:36AM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
> 
> 
> > > +/* RGMII Rx Clock delay value change with board lay-out */ static u8
> > > +rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;
> >
> > Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.
> >
> > Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value.
> > We tested on Beaglebone Black with VSC 8531 PHY.
> > We would like to provide new function to configure correct/require value based on PHY layouts
> > alone with other RGMII configuration parameters as part of our next implementation.
> 
> Please either do it properly now or hard code it as the default, and
> then later replace it with device tree, etc. We don't like to see half
> finished features.
> 
> > What are you locking against?
> >
> > Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY control registers,
> > first set the page number then read/write the register address. Default page should be Page 0.
> > When I want to access not default page register, I have to lock phy device access and change
> > the page number and register access as atomic operation.
> 
> I understand all that, which is why i asked, "what are you locking
> against?", not "why are you locking?" What are the other call paths? I
> don't see you taking this lock anywhere else? Should you be? I would
> just like to see a comment which suggests you understand when this
> lock is needed, and when not.
> 
>      Andrew

On Fri, Aug 05, 2016 at 05:54:21PM +0530, Nagaraju Lakkaraju wrote:

Hello,

I added all review comments and re-sending for review.

quoted

From a5017f5878a92d2acec86a6a29b1498c457cb73a Mon Sep 17 00:00:00 2001

From: Nagaraju Lakkaraju <redacted>
Date: Wed, 3 Aug 2016 18:28:24 +0530
Subject: [PATCH v2] net: phy: Add drivers for Microsemi PHYs

Please use git send-email to send patches.

All comments which should not be committed to the change log should
appear after the ---. It is normal to briefly list changes between
this version and the previous version.

+static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
+{
+       int rc;
+
+       rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
+       return rc;

Please remove rc and just do

	return phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);

+}
+
+static int vsc85xx_default_config(struct phy_device *phydev)

This function is setting the RGMII delay. So vsc85xx_default_config()
is not a particularly good name.

   Andrew

On Fri, Aug 05, 2016 at 05:54:21PM +0530, Nagaraju Lakkaraju wrote:

Hello,

I added all review comments and re-sending for review.

You should also take a look at the output of scripts/checkpatch.pl:

total: 9 errors, 82 warnings, 1 checks, 179 lines checked

       Andrew

From: Raju Lakkaraju <redacted>

The existing VSC85xx PHY driver did not follow the coding style and caused "checkpatch" to complain. This commit fixes this.

Signed-off-by: Raju Lakkaraju <redacted>
---
 drivers/net/phy/Kconfig |   6 +-
 drivers/net/phy/mscc.c  | 178 ++++++++++++++++++++++++------------------------
 2 files changed, 92 insertions(+), 92 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1c3e07c..87b566f 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig

@@ -274,9 +274,9 @@ config MICROCHIP_PHY
 	  Supports the LAN88XX PHYs.
 
 config MICROSEMI_PHY
-    tristate "Microsemi PHYs"
-    ---help---
-      Currently supports the VSC8531 and VSC8541 PHYs
+	tristate "Microsemi PHYs"
+	---help---
+	  Currently supports the VSC8531 and VSC8541 PHYs
 
 config NATIONAL_PHY
 	tristate "National Semiconductor PHYs"

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

@@ -13,135 +13,135 @@
 #include <linux/phy.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
+	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 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 */
+#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
+#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
+#define PHY_ID_VSC8531			  0x00070570
+#define PHY_ID_VSC8541			  0x00070770
 
 static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 {
-       int rc;
+	int rc;
 
-       rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
-       return rc;
+	rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
+	return rc;
 }
 
 static int vsc85xx_default_config(struct phy_device *phydev)
 {
-       int rc;
-       u16 reg_val;
+	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;
+	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_RGMII_CNTL);
-       reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
-       reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
-       phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
-       rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+	reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
+	reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
+	reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
+	phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
 
 out_unlock:
-       mutex_unlock(&phydev->lock);
+	mutex_unlock(&phydev->lock);
 
-       return rc;
+	return rc;
 }
 
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
-       int rc;
+	int rc;
 
-       rc = vsc85xx_default_config(phydev);
-       if (rc)
-               return rc;
-       rc = genphy_config_init(phydev);
+	rc = vsc85xx_default_config(phydev);
+	if (rc)
+		return rc;
+	rc = genphy_config_init(phydev);
 
-       return rc;
+	return rc;
 }
 
 static int vsc85xx_ack_interrupt(struct phy_device *phydev)
 {
-       int rc = 0;
+	int rc = 0;
 
-       if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-               rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
 
-       return (rc < 0) ? rc : 0;
+	return (rc < 0) ? rc : 0;
 }
 
 static int vsc85xx_config_intr(struct phy_device *phydev)
 {
-       int rc;
-
-       if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
-               rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
-                                  MII_VSC85XX_INT_MASK_MASK);
-       } else {
-               rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
-               if (rc < 0)
-                       return rc;
-               rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
-       }
-
-       return rc;
+	int rc;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+			       MII_VSC85XX_INT_MASK_MASK);
+	} else {
+		rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
+		if (rc < 0)
+			return rc;
+		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+	}
+
+	return rc;
 }
 
 /* Microsemi VSC85xx PHYs */
 static struct phy_driver vsc85xx_driver[] = {
 {
-       .phy_id                 = PHY_ID_VSC8531,
-       .name                   = "Microsemi VSC8531",
-       .phy_id_mask    = 0xfffffff0,
-       .features               = PHY_GBIT_FEATURES,
-       .flags                  = PHY_HAS_INTERRUPT,
-       .soft_reset             = &genphy_soft_reset,
-       .config_init    = &vsc85xx_config_init,
-       .config_aneg    = &genphy_config_aneg,
-       .aneg_done              = &genphy_aneg_done,
-       .read_status    = &genphy_read_status,
-       .ack_interrupt  = &vsc85xx_ack_interrupt,
-       .config_intr    = &vsc85xx_config_intr,
-       .suspend                = &genphy_suspend,
-       .resume                 = &genphy_resume,
+	.phy_id		= PHY_ID_VSC8531,
+	.name		= "Microsemi VSC8531",
+	.phy_id_mask    = 0xfffffff0,
+	.features	= PHY_GBIT_FEATURES,
+	.flags		= PHY_HAS_INTERRUPT,
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc85xx_config_init,
+	.config_aneg    = &genphy_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status    = &genphy_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
 },
 {
-       .phy_id                 = PHY_ID_VSC8541,
-       .name                   = "Microsemi VSC8541 SyncE",
-       .phy_id_mask    = 0xfffffff0,
-       .features               = PHY_GBIT_FEATURES,
-       .flags                  = PHY_HAS_INTERRUPT,
-       .soft_reset             = &genphy_soft_reset,
-       .config_init    = &vsc85xx_config_init,
-       .config_aneg    = &genphy_config_aneg,
-       .aneg_done              = &genphy_aneg_done,
-       .read_status    = &genphy_read_status,
-       .ack_interrupt  = &vsc85xx_ack_interrupt,
-       .config_intr    = &vsc85xx_config_intr,
-       .suspend                = &genphy_suspend,
-       .resume                 = &genphy_resume,
+	.phy_id		= PHY_ID_VSC8541,
+	.name		= "Microsemi VSC8541 SyncE",
+	.phy_id_mask    = 0xfffffff0,
+	.features	= PHY_GBIT_FEATURES,
+	.flags		= PHY_HAS_INTERRUPT,
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc85xx_config_init,
+	.config_aneg    = &genphy_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status    = &genphy_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
 }
 
 };

@@ -149,9 +149,9 @@ static struct phy_driver vsc85xx_driver[] = {
 module_phy_driver(vsc85xx_driver);
 
 static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
-       { PHY_ID_VSC8531, 0xfffffff0, },
-       { PHY_ID_VSC8541, 0xfffffff0, },
-       { }
+	{ PHY_ID_VSC8531, 0xfffffff0, },
+	{ PHY_ID_VSC8541, 0xfffffff0, },
+	{ }
 };
 
 MODULE_DEVICE_TABLE(mdio, vsc85xx_tbl);

-- 
2.7.4

From: Raju Lakkaraju <redacted>
Date: Thu, 8 Sep 2016 14:09:31 +0530

From: Raju Lakkaraju <redacted>

The existing VSC85xx PHY driver did not follow the coding style and caused "checkpatch" to complain. This commit fixes this.

Signed-off-by: Raju Lakkaraju <redacted>

Applied.